Message ID | 20180808215310.38044-1-seanpaul@chromium.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/panel: sn65dsi86: Implement AUX channel | expand |
On Wed, Aug 8, 2018, 5:53 PM Sean Paul <seanpaul@chromium.org> wrote: Just realized I put drm/panel in the subject, should be drm/bridge. Sean 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 <spanda@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@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); > usleep_range(10000, 10500); /* 10ms delay recommended by spec */ > > /* 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); > + 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); > + break; > + default: > + return -EINVAL; > + } > + > + regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, > + (msg->address >> 16) & 0xFF); > + 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 > > <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 8, 2018, 5:53 PM Sean Paul <<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>> wrote:<br></div><div dir="ltr"><br></div><div dir="ltr">Just realized I put drm/panel in the subject, should be drm/bridge. </div><div dir="ltr"><br></div><div dir="ltr">Sean</div><div dir="ltr"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This was hand-rolled in the first version, and will surely be useful as<br> we expand the driver to support more varied use cases.<br> <br> Cc: Sandeep Panda <<a href="mailto:spanda@codeaurora.org" target="_blank" rel="noreferrer">spanda@codeaurora.org</a>><br> Signed-off-by: Sean Paul <<a href="mailto:seanpaul@chromium.org" target="_blank" rel="noreferrer">seanpaul@chromium.org</a>><br> ---<br> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 107 ++++++++++++++++++++++++--<br> 1 file changed, 100 insertions(+), 7 deletions(-)<br> <br> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c<br> index 1b6e8b72be58..50aa8c3c39fc 100644<br> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c<br> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c<br> @@ -7,12 +7,14 @@<br> #include <drm/drm_atomic.h><br> #include <drm/drm_atomic_helper.h><br> #include <drm/drm_crtc_helper.h><br> +#include <drm/drm_dp_helper.h><br> #include <drm/drm_mipi_dsi.h><br> #include <drm/drm_of.h><br> #include <drm/drm_panel.h><br> #include <linux/clk.h><br> #include <linux/gpio/consumer.h><br> #include <linux/i2c.h><br> +#include <linux/iopoll.h><br> #include <linux/of_graph.h><br> #include <linux/pm_runtime.h><br> #include <linux/regmap.h><br> @@ -29,12 +31,15 @@<br> #define SN_DATARATE_CONFIG_REG 0x94<br> #define SN_PLL_ENABLE_REG 0x0D<br> #define SN_SCRAMBLE_CONFIG_REG 0x95<br> -#define SN_AUX_WDATA0_REG 0x64<br> +#define SN_AUX_WDATA_REG(x) (0x64 + x)<br> #define SN_AUX_ADDR_19_16_REG 0x74<br> #define SN_AUX_ADDR_15_8_REG 0x75<br> #define SN_AUX_ADDR_7_0_REG 0x76<br> #define SN_AUX_LENGTH_REG 0x77<br> #define SN_AUX_CMD_REG 0x78<br> +#define AUX_CMD_SEND 0x01<br> +#define AUX_CMD_REQ(x) (x << 4)<br> +#define SN_AUX_RDATA_REG(x) (0x79 + x)<br> #define SN_ML_TX_MODE_REG 0x96<br> /* video config specific registers */<br> #define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG 0x20<br> @@ -64,6 +69,9 @@<br> #define SN_DP_DATA_RATE_OFFSET 5<br> #define SN_SYNC_POLARITY_OFFSET 7<br> <br> +/* Matches DP_AUX_MAX_PAYLOAD_BYTES (for now) */<br> +#define SN_AUX_MAX_PAYLOAD_BYTES 16<br> +<br> #define SN_ENABLE_VID_STREAM_BIT BIT(3)<br> #define SN_REFCLK_FREQ_BITS GENMASK(3, 1)<br> #define SN_DSIA_NUM_LANES_BITS GENMASK(4, 3)<br> @@ -76,6 +84,7 @@<br> struct ti_sn_bridge {<br> struct device *dev;<br> struct regmap *regmap;<br> + struct drm_dp_aux aux;<br> struct drm_bridge bridge;<br> struct drm_connector connector;<br> struct device_node *host_node;<br> @@ -473,12 +482,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)<br> * authentication method. We need to enable this method in the eDP panel<br> * at DisplayPort address 0x0010A prior to link training.<br> */<br> - regmap_write(pdata->regmap, SN_AUX_WDATA0_REG, 0x01);<br> - regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, 0x00);<br> - regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG, 0x01);<br> - regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, 0x0A);<br> - regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, 0x01);<br> - regmap_write(pdata->regmap, SN_AUX_CMD_REG, 0x81);<br> + drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, 0);<br> usleep_range(10000, 10500); /* 10ms delay recommended by spec */<br> <br> /* Semi auto link training mode */<br> @@ -527,6 +531,90 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {<br> .post_disable = ti_sn_bridge_post_disable,<br> };<br> <br> +static struct ti_sn_bridge *aux_to_ti_sn_bridge(struct drm_dp_aux *aux)<br> +{<br> + return container_of(aux, struct ti_sn_bridge, aux);<br> +}<br> +<br> +static bool ti_sn_cmd_done(struct ti_sn_bridge *pdata)<br> +{<br> + int ret;<br> + unsigned int val;<br> +<br> + ret = regmap_read(pdata->regmap, SN_AUX_CMD_REG, &val);<br> + WARN_ON(ret);<br> + return ret || !(val & AUX_CMD_SEND);<br> +}<br> +<br> +static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,<br> + struct drm_dp_aux_msg *msg)<br> +{<br> + struct ti_sn_bridge *pdata = aux_to_ti_sn_bridge(aux);<br> + u32 request = msg->request & ~DP_AUX_I2C_MOT;<br> + u32 request_val = AUX_CMD_REQ(msg->request);<br> + u8 *buf = (u8 *)msg->buffer;<br> + bool cmd_done;<br> + int ret, i;<br> +<br> + if (msg->size > SN_AUX_MAX_PAYLOAD_BYTES)<br> + return -EINVAL;<br> +<br> + switch (request) {<br> + case DP_AUX_NATIVE_WRITE:<br> + case DP_AUX_I2C_WRITE:<br> + case DP_AUX_NATIVE_READ:<br> + case DP_AUX_I2C_READ:<br> + regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val);<br> + break;<br> + default:<br> + return -EINVAL;<br> + }<br> +<br> + regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG,<br> + (msg->address >> 16) & 0xFF);<br> + regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG,<br> + (msg->address >> 8) & 0xFF);<br> + regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, msg->address & 0xFF);<br> +<br> + regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, msg->size);<br> +<br> + if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {<br> + for (i = 0; i < msg->size; i++)<br> + regmap_write(pdata->regmap, SN_AUX_WDATA_REG(i),<br> + buf[i]);<br> + }<br> +<br> + regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val | AUX_CMD_SEND);<br> +<br> + ret = readx_poll_timeout(ti_sn_cmd_done, pdata, cmd_done, cmd_done,<br> + 200, 50 * 1000);<br> + if (ret)<br> + return ret;<br> +<br> + if (request == DP_AUX_NATIVE_READ || request == DP_AUX_I2C_READ) {<br> + unsigned int val;<br> +<br> + ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &val);<br> + if (ret)<br> + return ret;<br> + else if (val != msg->size)<br> + return -ENXIO;<br> +<br> + for (i = 0; i < msg->size; i++) {<br> + unsigned int val;<br> + ret = regmap_read(pdata->regmap, SN_AUX_RDATA_REG(i),<br> + &val);<br> + if (ret)<br> + return ret;<br> +<br> + WARN_ON(val & ~0xFF);<br> + buf[i] = (u8)(val & 0xFF);<br> + }<br> + }<br> +<br> + return msg->size;<br> +}<br> +<br> static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)<br> {<br> struct device_node *np = pdata->dev->of_node;<br> @@ -606,6 +694,11 @@ static int ti_sn_bridge_probe(struct i2c_client *client,<br> <br> i2c_set_clientdata(client, pdata);<br> <br> + pdata-><a href="http://aux.name" rel="noreferrer noreferrer" target="_blank">aux.name</a> = "ti-sn65dsi86-aux";<br> + pdata->aux.dev = pdata->dev;<br> + pdata->aux.transfer = ti_sn_aux_transfer;<br> + drm_dp_aux_register(&pdata->aux);<br> +<br> pdata->bridge.funcs = &ti_sn_bridge_funcs;<br> pdata->bridge.of_node = client->dev.of_node;<br> <br> -- <br> Sean Paul, Software Engineer, Google / Chromium OS<br> <br> </blockquote></div></div></div>
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. 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 <spanda@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@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. > 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. > + 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. > + 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 > + 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;
On Fri, Aug 10, 2018 at 10:22:58AM +0530, spanda@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 <spanda@codeaurora.org> > > Signed-off-by: Sean Paul <seanpaul@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;
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); usleep_range(10000, 10500); /* 10ms delay recommended by spec */ /* 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); + 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); + break; + default: + return -EINVAL; + } + + regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, + (msg->address >> 16) & 0xFF); + 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;
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 <spanda@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 107 ++++++++++++++++++++++++-- 1 file changed, 100 insertions(+), 7 deletions(-)