On Fri, Aug 10, 2018 at 10:22:58AM +0530, spa...@codeaurora.org wrote:
> Hi Sean,
> 
> Thanks for your patch.
> I also made this similar change as part of my PSR support changes (yet to be
> posted for review). But since this patch is posted now, i will pick this for
> my PSR changes.

Hi Sandeep,
Thanks so much for your review. I didn't realize you were working on PSR, that's
cool.

I've added a few comments below, and I'll upload a v2 shortly.

> 
> On 2018-08-09 03:23, Sean Paul wrote:
> > This was hand-rolled in the first version, and will surely be useful as
> > we expand the driver to support more varied use cases.
> > 
> > Cc: Sandeep Panda <spa...@codeaurora.org>
> > Signed-off-by: Sean Paul <seanp...@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 107 ++++++++++++++++++++++++--
> >  1 file changed, 100 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 1b6e8b72be58..50aa8c3c39fc 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -7,12 +7,14 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_dp_helper.h>
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> >  #include <linux/clk.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/of_graph.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> > @@ -29,12 +31,15 @@
> >  #define SN_DATARATE_CONFIG_REG                     0x94
> >  #define SN_PLL_ENABLE_REG                  0x0D
> >  #define SN_SCRAMBLE_CONFIG_REG                     0x95
> > -#define SN_AUX_WDATA0_REG                  0x64
> > +#define SN_AUX_WDATA_REG(x)                        (0x64 + x)
> >  #define SN_AUX_ADDR_19_16_REG                      0x74
> >  #define SN_AUX_ADDR_15_8_REG                       0x75
> >  #define SN_AUX_ADDR_7_0_REG                        0x76
> >  #define SN_AUX_LENGTH_REG                  0x77
> >  #define SN_AUX_CMD_REG                             0x78
> > +#define  AUX_CMD_SEND                              0x01
> > +#define  AUX_CMD_REQ(x)                            (x << 4)
> > +#define SN_AUX_RDATA_REG(x)                        (0x79 + x)
> >  #define SN_ML_TX_MODE_REG                  0x96
> >  /* video config specific registers */
> >  #define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG  0x20
> > @@ -64,6 +69,9 @@
> >  #define SN_DP_DATA_RATE_OFFSET     5
> >  #define SN_SYNC_POLARITY_OFFSET    7
> > 
> > +/* Matches DP_AUX_MAX_PAYLOAD_BYTES (for now) */
> > +#define SN_AUX_MAX_PAYLOAD_BYTES   16
> > +
> >  #define SN_ENABLE_VID_STREAM_BIT   BIT(3)
> >  #define SN_REFCLK_FREQ_BITS                GENMASK(3, 1)
> >  #define SN_DSIA_NUM_LANES_BITS             GENMASK(4, 3)
> > @@ -76,6 +84,7 @@
> >  struct ti_sn_bridge {
> >     struct device                   *dev;
> >     struct regmap                   *regmap;
> > +   struct drm_dp_aux               aux;
> >     struct drm_bridge               bridge;
> >     struct drm_connector            connector;
> >     struct device_node              *host_node;
> > @@ -473,12 +482,7 @@ static void ti_sn_bridge_enable(struct drm_bridge
> > *bridge)
> >      * authentication method. We need to enable this method in the eDP
> > panel
> >      * at DisplayPort address 0x0010A prior to link training.
> >      */
> > -   regmap_write(pdata->regmap, SN_AUX_WDATA0_REG, 0x01);
> > -   regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, 0x00);
> > -   regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG, 0x01);
> > -   regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, 0x0A);
> > -   regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, 0x01);
> > -   regmap_write(pdata->regmap, SN_AUX_CMD_REG, 0x81);
> > +   drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, 0);
> 
> I think the last argument here should be 0x1 instead of 0. or better put it
> as DP_ALTERNATE_SCRAMBLER_RESET_ENABLE.

Ah shoot, good catch, not sure what I was thinking.

> 
> >     usleep_range(10000, 10500); /* 10ms delay recommended by spec */
> 
> >  We can remove this usleep now, as you are already ensuring in the newly
> > added function that aux transaction is successful.
> 
> >     /* Semi auto link training mode */
> > @@ -527,6 +531,90 @@ static const struct drm_bridge_funcs
> > ti_sn_bridge_funcs = {
> >     .post_disable = ti_sn_bridge_post_disable,
> >  };
> > 
> > +static struct ti_sn_bridge *aux_to_ti_sn_bridge(struct drm_dp_aux *aux)
> > +{
> > +   return container_of(aux, struct ti_sn_bridge, aux);
> > +}
> > +
> > +static bool ti_sn_cmd_done(struct ti_sn_bridge *pdata)
> > +{
> > +   int ret;
> > +   unsigned int val;
> > +
> > +   ret = regmap_read(pdata->regmap, SN_AUX_CMD_REG, &val);
> 
> Can we read back register offset 0xF4, instead of 0x78, to check if AUX
> transaction was success or any error has occurred.

I think we should continue to read the SEND bit here. The interrupt in F4 is
really just tracking SEND, so best to check at the source.

I will add a check for some of the error bits in 0xF4 in transfer after the
poll to ensure everything succeeded.

> 
> > +   WARN_ON(ret);
> > +   return ret || !(val & AUX_CMD_SEND);
> > +}
> > +
> > +static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
> > +                             struct drm_dp_aux_msg *msg)
> > +{
> > +   struct ti_sn_bridge *pdata = aux_to_ti_sn_bridge(aux);
> > +   u32 request = msg->request & ~DP_AUX_I2C_MOT;
> > +   u32 request_val = AUX_CMD_REQ(msg->request);
> > +   u8 *buf = (u8 *)msg->buffer;
> > +   bool cmd_done;
> > +   int ret, i;
> > +
> > +   if (msg->size > SN_AUX_MAX_PAYLOAD_BYTES)
> > +           return -EINVAL;
> > +
> > +   switch (request) {
> > +   case DP_AUX_NATIVE_WRITE:
> > +   case DP_AUX_I2C_WRITE:
> > +   case DP_AUX_NATIVE_READ:
> > +   case DP_AUX_I2C_READ:
> > +           regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val);
> 
> This regmap_write is not needed since you will be writing it again towards
> the end of this function.

Yeah, I noticed you were doing this earlier. The programming guide in 8.4.5.2.1
(gosh those sub-sections are atrocious) suggests updating CMD_REG before
programming ADDR and LENGTH, so I've done that out of an abundance of caution.
We need this switch anyways, so it's not hurting us.

> 
> > +           break;
> > +   default:
> > +           return -EINVAL;
> > +   }
> > +
> > +   regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG,
> > +                (msg->address >> 16) & 0xFF);
> 
> Since we are only concerned about 4 bits (bit 16-19) better to & with 0xF
> instead of 0xFF

Makes sense, will do.

> 
> > +   regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG,
> > +                (msg->address >> 8) & 0xFF);
> > +   regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, msg->address & 0xFF);
> > +
> > +   regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, msg->size);
> > +
> > +   if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {
> > +           for (i = 0; i < msg->size; i++)
> > +                   regmap_write(pdata->regmap, SN_AUX_WDATA_REG(i),
> > +                                buf[i]);
> > +   }
> > +
> > +   regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val |
> > AUX_CMD_SEND);
> > +
> > +   ret = readx_poll_timeout(ti_sn_cmd_done, pdata, cmd_done, cmd_done,
> > +                            200, 50 * 1000);
> > +   if (ret)
> > +           return ret;
> > +
> > +   if (request == DP_AUX_NATIVE_READ || request == DP_AUX_I2C_READ) {
> > +           unsigned int val;
> > +
> > +           ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &val);
> > +           if (ret)
> > +                   return ret;
> > +           else if (val != msg->size)
> > +                   return -ENXIO;
> > +
> > +           for (i = 0; i < msg->size; i++) {
> > +                   unsigned int val;
> > +                   ret = regmap_read(pdata->regmap, SN_AUX_RDATA_REG(i),
> > +                                     &val);
> > +                   if (ret)
> > +                           return ret;
> > +
> > +                   WARN_ON(val & ~0xFF);
> > +                   buf[i] = (u8)(val & 0xFF);
> > +           }
> > +   }
> > +
> > +   return msg->size;
> > +}
> > +
> >  static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> >  {
> >     struct device_node *np = pdata->dev->of_node;
> > @@ -606,6 +694,11 @@ static int ti_sn_bridge_probe(struct i2c_client
> > *client,
> > 
> >     i2c_set_clientdata(client, pdata);
> > 
> > +   pdata->aux.name = "ti-sn65dsi86-aux";
> > +   pdata->aux.dev = pdata->dev;
> > +   pdata->aux.transfer = ti_sn_aux_transfer;
> > +   drm_dp_aux_register(&pdata->aux);
> > +
> >     pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >     pdata->bridge.of_node = client->dev.of_node;

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to