In certain situations it was seen that we could wedge up the DDC bus
on the HDMI adapter on rk3288. The only way to unwedge was to mux one
of the pins over to GPIO output-driven-low temporarily and then
quickly mux back. Full details can be found in the patch
("drm/bridge/synopsys: dw-hdmi: Add "unwedge" for ddc bus").
Since unwedge requires remuxing the pins, we first need to add to the
bindings so that we can specify what state the pins should be in for
unwedging.
Signed-off-by: Douglas Anderson <[email protected]>
---
.../bindings/display/rockchip/dw_hdmi-rockchip.txt | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
index 39143424a474..8346bac81f1c 100644
--- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
@@ -38,6 +38,13 @@ Optional properties
- phys: from general PHY binding: the phandle for the PHY device.
- phy-names: Should be "hdmi" if phys references an external phy.
+Optional pinctrl entry:
+- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
+ will switch to the unwedge pinctrl state for 10ms if it ever gets an
+ i2c timeout. It's intended that this unwedge pinctrl entry will
+ cause the SDA line to be driven low to work around a hardware
+ errata.
+
Example:
hdmi: hdmi@ff980000 {
--
2.21.0.1020.gf2820cf01a-goog
See the PhD thesis in the comments in this patch for details, but to
summarize this adds a hacky "unwedge" feature to the dw_hdmi i2c bus to
workaround what appears to be a hardware errata. This relies on a
pinctrl entry to help change around muxing to perform the unwedge.
NOTE that the specific TV this was tested on was the "Samsung
UN40HU6950FXZA" and the specific port was the "STB" port.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 116 +++++++++++++++++++---
1 file changed, 100 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index db761329a1e3..c66587e33813 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -19,6 +19,7 @@
#include <linux/hdmi.h>
#include <linux/mutex.h>
#include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/regmap.h>
#include <linux/spinlock.h>
@@ -169,6 +170,10 @@ struct dw_hdmi {
bool sink_is_hdmi;
bool sink_has_audio;
+ struct pinctrl *pinctrl;
+ struct pinctrl_state *default_state;
+ struct pinctrl_state *unwedge_state;
+
struct mutex mutex; /* for state below and previous_mode */
enum drm_connector_force force; /* mutex-protected force state */
bool disabled; /* DRM has disabled our bridge */
@@ -247,11 +252,82 @@ static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
HDMI_IH_MUTE_I2CM_STAT0);
}
+static bool dw_hdmi_i2c_unwedge(struct dw_hdmi *hdmi)
+{
+ /* If no unwedge state then give up */
+ if (IS_ERR(hdmi->unwedge_state))
+ return false;
+
+ dev_info(hdmi->dev, "Attempting to unwedge stuck i2c bus\n");
+
+ /*
+ * This is a huge hack to workaround a problem where the dw_hdmi i2c
+ * bus could sometimes get wedged. Once wedged there doesn't appear
+ * to be any way to unwedge it (including the HDMI_I2CM_SOFTRSTZ)
+ * other than pulsing the SDA line.
+ *
+ * We appear to be able to pulse the SDA line (in the eyes of dw_hdmi)
+ * by:
+ * 1. Remux the pin as a GPIO output, driven low.
+ * 2. Wait a little while. 1 ms seems to work, but we'll do 10.
+ * 3. Immediately jump to remux the pin as dw_hdmi i2c again.
+ *
+ * At the moment of remuxing, the line will still be low due to its
+ * recent stint as an output, but then it will be pulled high by the
+ * (presumed) external pullup. dw_hdmi seems to see this as a rising
+ * edge and that seems to get it out of its jam.
+ *
+ * This wedging was only ever seen on one TV, and only on one of
+ * its HDMI ports. It happened when the TV was powered on while the
+ * device was plugged in. A scope trace shows the TV bringing both SDA
+ * and SCL low, then bringing them both back up at roughly the same
+ * time. Presumably this confuses dw_hdmi because it saw activity but
+ * no real STOP (maybe it thinks there's another master on the bus?).
+ * Giving it a clean rising edge of SDA while SCL is already high
+ * presumably makes dw_hdmi see a STOP which seems to bring dw_hdmi out
+ * of its stupor.
+ *
+ * Note that after coming back alive, transfers seem to immediately
+ * resume, so if we unwedge due to a timeout we should wait a little
+ * longer for our transfer to finish, since it might have just started
+ * now.
+ */
+ pinctrl_select_state(hdmi->pinctrl, hdmi->unwedge_state);
+ msleep(10);
+ pinctrl_select_state(hdmi->pinctrl, hdmi->default_state);
+
+ return true;
+}
+
+static int dw_hdmi_i2c_wait(struct dw_hdmi *hdmi)
+{
+ struct dw_hdmi_i2c *i2c = hdmi->i2c;
+ int stat;
+
+ stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
+ if (!stat) {
+ /* If we can't unwedge, return timeout */
+ if (!dw_hdmi_i2c_unwedge(hdmi))
+ return -EAGAIN;
+
+ /* We tried to unwedge; give it another chance */
+ stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
+ if (!stat)
+ return -EAGAIN;
+ }
+
+ /* Check for error condition on the bus */
+ if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR)
+ return -EIO;
+
+ return 0;
+}
+
static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
unsigned char *buf, unsigned int length)
{
struct dw_hdmi_i2c *i2c = hdmi->i2c;
- int stat;
+ int ret;
if (!i2c->is_regaddr) {
dev_dbg(hdmi->dev, "set read register address to 0\n");
@@ -270,13 +346,9 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
HDMI_I2CM_OPERATION);
- stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
- if (!stat)
- return -EAGAIN;
-
- /* Check for error condition on the bus */
- if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR)
- return -EIO;
+ ret = dw_hdmi_i2c_wait(hdmi);
+ if (ret)
+ return ret;
*buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
}
@@ -289,7 +361,7 @@ static int dw_hdmi_i2c_write(struct dw_hdmi *hdmi,
unsigned char *buf, unsigned int length)
{
struct dw_hdmi_i2c *i2c = hdmi->i2c;
- int stat;
+ int ret;
if (!i2c->is_regaddr) {
/* Use the first write byte as register address */
@@ -307,13 +379,9 @@ static int dw_hdmi_i2c_write(struct dw_hdmi *hdmi,
hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_WRITE,
HDMI_I2CM_OPERATION);
- stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
- if (!stat)
- return -EAGAIN;
-
- /* Check for error condition on the bus */
- if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR)
- return -EIO;
+ ret = dw_hdmi_i2c_wait(hdmi);
+ if (ret)
+ return ret;
}
return 0;
@@ -2606,6 +2674,22 @@ __dw_hdmi_probe(struct platform_device *pdev,
/* If DDC bus is not specified, try to register HDMI I2C bus */
if (!hdmi->ddc) {
+ /* Look for (optional) stuff related to unwedging */
+ hdmi->pinctrl = devm_pinctrl_get(dev);
+ if (!IS_ERR(hdmi->pinctrl)) {
+ hdmi->unwedge_state =
+ pinctrl_lookup_state(hdmi->pinctrl, "unwedge");
+ hdmi->default_state =
+ pinctrl_lookup_state(hdmi->pinctrl, "default");
+
+ if (IS_ERR(hdmi->default_state) &&
+ !IS_ERR(hdmi->unwedge_state)) {
+ dev_warn(dev,
+ "Unwedge requires default pinctrl\n");
+ hdmi->unwedge_state = ERR_PTR(-ENODEV);
+ }
+ }
+
hdmi->ddc = dw_hdmi_i2c_adapter(hdmi);
if (IS_ERR(hdmi->ddc))
hdmi->ddc = NULL;
--
2.21.0.1020.gf2820cf01a-goog
Downstream Chrome OS kernels use the builtin DDC bus from dw_hdmi on
veyron. This is the only way to get them to negotiate HDCP.
Although HDCP isn't currently all supported upstream, it still seems
like it makes sense to use dw_hdmi's builtin I2C. Maybe eventually we
can get HDCP negotiation working.
Signed-off-by: Douglas Anderson <[email protected]>
---
arch/arm/boot/dts/rk3288-veyron.dtsi | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
index 1252522392c7..e1bee663d2c5 100644
--- a/arch/arm/boot/dts/rk3288-veyron.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
@@ -163,7 +163,8 @@
};
&hdmi {
- ddc-i2c-bus = <&i2c5>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&hdmi_ddc>;
status = "okay";
};
@@ -334,14 +335,6 @@
i2c-scl-rising-time-ns = <300>; /* 225ns measured */
};
-&i2c5 {
- status = "okay";
-
- clock-frequency = <100000>;
- i2c-scl-falling-time-ns = <300>;
- i2c-scl-rising-time-ns = <1000>;
-};
-
&io_domains {
status = "okay";
--
2.21.0.1020.gf2820cf01a-goog
This adds the "unwedge" pinctrl entries introduced by a recent dw_hdmi
change that can unwedge the dw_hdmi i2c bus in some cases. It's
expected that any boards using this would add:
pinctrl-names = "default", "unwedge";
pinctrl-0 = <&hdmi_ddc>;
pinctrl-1 = <&hdmi_ddc_unwedge>;
Note that this isn't added by default because some boards may choose
to mux i2c5 for their DDC bus (if that is more tested for them).
Signed-off-by: Douglas Anderson <[email protected]>
---
arch/arm/boot/dts/rk3288.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 74c9517c4f92..eebc04fa1e4d 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -1545,6 +1545,15 @@
rockchip,pins = <7 RK_PC3 2 &pcfg_pull_none>,
<7 RK_PC4 2 &pcfg_pull_none>;
};
+
+ hdmi_ddc_unwedge: hdmi-ddc-unwedge {
+ rockchip,pins = <7 RK_PC3 RK_FUNC_GPIO &pcfg_output_low>,
+ <7 RK_PC4 2 &pcfg_pull_none>;
+ };
+ };
+
+ pcfg_output_low: pcfg-output-low {
+ output-low;
};
pcfg_pull_up: pcfg-pull-up {
--
2.21.0.1020.gf2820cf01a-goog
Veyron uses the builtin i2c controller that's part of dw-hdmi. Hook
up the unwedging feature.
Signed-off-by: Douglas Anderson <[email protected]>
---
arch/arm/boot/dts/rk3288-veyron.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
index e1bee663d2c5..340b276b6333 100644
--- a/arch/arm/boot/dts/rk3288-veyron.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
@@ -163,8 +163,9 @@
};
&hdmi {
- pinctrl-names = "default";
+ pinctrl-names = "default", "unwedge";
pinctrl-0 = <&hdmi_ddc>;
+ pinctrl-1 = <&hdmi_ddc_unwedge>;
status = "okay";
};
--
2.21.0.1020.gf2820cf01a-goog
On Thu, 2 May 2019 15:53:32 -0700, Douglas Anderson wrote:
> In certain situations it was seen that we could wedge up the DDC bus
> on the HDMI adapter on rk3288. The only way to unwedge was to mux one
> of the pins over to GPIO output-driven-low temporarily and then
> quickly mux back. Full details can be found in the patch
> ("drm/bridge/synopsys: dw-hdmi: Add "unwedge" for ddc bus").
>
> Since unwedge requires remuxing the pins, we first need to add to the
> bindings so that we can specify what state the pins should be in for
> unwedging.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> .../bindings/display/rockchip/dw_hdmi-rockchip.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On Thu, May 02, 2019 at 03:53:33PM -0700, Douglas Anderson wrote:
> See the PhD thesis in the comments in this patch for details, but to
> summarize this adds a hacky "unwedge" feature to the dw_hdmi i2c bus to
> workaround what appears to be a hardware errata. This relies on a
> pinctrl entry to help change around muxing to perform the unwedge.
>
> NOTE that the specific TV this was tested on was the "Samsung
> UN40HU6950FXZA" and the specific port was the "STB" port.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 116 +++++++++++++++++++---
> 1 file changed, 100 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index db761329a1e3..c66587e33813 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -19,6 +19,7 @@
> #include <linux/hdmi.h>
> #include <linux/mutex.h>
> #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
> #include <linux/regmap.h>
> #include <linux/spinlock.h>
>
> @@ -169,6 +170,10 @@ struct dw_hdmi {
> bool sink_is_hdmi;
> bool sink_has_audio;
>
> + struct pinctrl *pinctrl;
> + struct pinctrl_state *default_state;
> + struct pinctrl_state *unwedge_state;
> +
> struct mutex mutex; /* for state below and previous_mode */
> enum drm_connector_force force; /* mutex-protected force state */
> bool disabled; /* DRM has disabled our bridge */
> @@ -247,11 +252,82 @@ static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
> HDMI_IH_MUTE_I2CM_STAT0);
> }
>
> +static bool dw_hdmi_i2c_unwedge(struct dw_hdmi *hdmi)
> +{
> + /* If no unwedge state then give up */
> + if (IS_ERR(hdmi->unwedge_state))
> + return false;
> +
> + dev_info(hdmi->dev, "Attempting to unwedge stuck i2c bus\n");
> +
> + /*
> + * This is a huge hack to workaround a problem where the dw_hdmi i2c
> + * bus could sometimes get wedged. Once wedged there doesn't appear
> + * to be any way to unwedge it (including the HDMI_I2CM_SOFTRSTZ)
> + * other than pulsing the SDA line.
> + *
> + * We appear to be able to pulse the SDA line (in the eyes of dw_hdmi)
> + * by:
> + * 1. Remux the pin as a GPIO output, driven low.
> + * 2. Wait a little while. 1 ms seems to work, but we'll do 10.
> + * 3. Immediately jump to remux the pin as dw_hdmi i2c again.
> + *
> + * At the moment of remuxing, the line will still be low due to its
> + * recent stint as an output, but then it will be pulled high by the
> + * (presumed) external pullup. dw_hdmi seems to see this as a rising
> + * edge and that seems to get it out of its jam.
> + *
> + * This wedging was only ever seen on one TV, and only on one of
> + * its HDMI ports. It happened when the TV was powered on while the
> + * device was plugged in. A scope trace shows the TV bringing both SDA
> + * and SCL low, then bringing them both back up at roughly the same
> + * time. Presumably this confuses dw_hdmi because it saw activity but
> + * no real STOP (maybe it thinks there's another master on the bus?).
> + * Giving it a clean rising edge of SDA while SCL is already high
> + * presumably makes dw_hdmi see a STOP which seems to bring dw_hdmi out
> + * of its stupor.
> + *
> + * Note that after coming back alive, transfers seem to immediately
> + * resume, so if we unwedge due to a timeout we should wait a little
> + * longer for our transfer to finish, since it might have just started
> + * now.
> + */
> + pinctrl_select_state(hdmi->pinctrl, hdmi->unwedge_state);
> + msleep(10);
> + pinctrl_select_state(hdmi->pinctrl, hdmi->default_state);
> +
> + return true;
> +}
> +
> +static int dw_hdmi_i2c_wait(struct dw_hdmi *hdmi)
> +{
> + struct dw_hdmi_i2c *i2c = hdmi->i2c;
> + int stat;
> +
> + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
> + if (!stat) {
> + /* If we can't unwedge, return timeout */
> + if (!dw_hdmi_i2c_unwedge(hdmi))
> + return -EAGAIN;
> +
> + /* We tried to unwedge; give it another chance */
> + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
> + if (!stat)
> + return -EAGAIN;
> + }
> +
> + /* Check for error condition on the bus */
> + if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR)
> + return -EIO;
> +
> + return 0;
> +}
> +
> static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
> unsigned char *buf, unsigned int length)
> {
> struct dw_hdmi_i2c *i2c = hdmi->i2c;
> - int stat;
> + int ret;
>
> if (!i2c->is_regaddr) {
> dev_dbg(hdmi->dev, "set read register address to 0\n");
> @@ -270,13 +346,9 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
> hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
> HDMI_I2CM_OPERATION);
>
> - stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
> - if (!stat)
> - return -EAGAIN;
> -
> - /* Check for error condition on the bus */
> - if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR)
> - return -EIO;
> + ret = dw_hdmi_i2c_wait(hdmi);
> + if (ret)
> + return ret;
>
> *buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
> }
> @@ -289,7 +361,7 @@ static int dw_hdmi_i2c_write(struct dw_hdmi *hdmi,
> unsigned char *buf, unsigned int length)
> {
> struct dw_hdmi_i2c *i2c = hdmi->i2c;
> - int stat;
> + int ret;
>
> if (!i2c->is_regaddr) {
> /* Use the first write byte as register address */
> @@ -307,13 +379,9 @@ static int dw_hdmi_i2c_write(struct dw_hdmi *hdmi,
> hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_WRITE,
> HDMI_I2CM_OPERATION);
>
> - stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
> - if (!stat)
> - return -EAGAIN;
> -
> - /* Check for error condition on the bus */
> - if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR)
> - return -EIO;
> + ret = dw_hdmi_i2c_wait(hdmi);
> + if (ret)
> + return ret;
> }
>
> return 0;
> @@ -2606,6 +2674,22 @@ __dw_hdmi_probe(struct platform_device *pdev,
>
> /* If DDC bus is not specified, try to register HDMI I2C bus */
> if (!hdmi->ddc) {
> + /* Look for (optional) stuff related to unwedging */
> + hdmi->pinctrl = devm_pinctrl_get(dev);
> + if (!IS_ERR(hdmi->pinctrl)) {
> + hdmi->unwedge_state =
> + pinctrl_lookup_state(hdmi->pinctrl, "unwedge");
> + hdmi->default_state =
> + pinctrl_lookup_state(hdmi->pinctrl, "default");
> +
> + if (IS_ERR(hdmi->default_state) &&
> + !IS_ERR(hdmi->unwedge_state)) {
> + dev_warn(dev,
> + "Unwedge requires default pinctrl\n");
Can you downgrade this message to info or dbg? Given how rare this issue is, we
probably don't want to spam everyone who is happily using dw-hdmi.
With that,
Reviewed-by: Sean Paul <[email protected]>
Sean
> + hdmi->unwedge_state = ERR_PTR(-ENODEV);
> + }
> + }
> +
> hdmi->ddc = dw_hdmi_i2c_adapter(hdmi);
> if (IS_ERR(hdmi->ddc))
> hdmi->ddc = NULL;
> --
> 2.21.0.1020.gf2820cf01a-goog
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, May 02, 2019 at 03:53:34PM -0700, Douglas Anderson wrote:
> Downstream Chrome OS kernels use the builtin DDC bus from dw_hdmi on
> veyron. This is the only way to get them to negotiate HDCP.
>
> Although HDCP isn't currently all supported upstream, it still seems
> like it makes sense to use dw_hdmi's builtin I2C. Maybe eventually we
> can get HDCP negotiation working.
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Sean Paul <[email protected]>
> ---
>
> arch/arm/boot/dts/rk3288-veyron.dtsi | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
> index 1252522392c7..e1bee663d2c5 100644
> --- a/arch/arm/boot/dts/rk3288-veyron.dtsi
> +++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
> @@ -163,7 +163,8 @@
> };
>
> &hdmi {
> - ddc-i2c-bus = <&i2c5>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&hdmi_ddc>;
> status = "okay";
> };
>
> @@ -334,14 +335,6 @@
> i2c-scl-rising-time-ns = <300>; /* 225ns measured */
> };
>
> -&i2c5 {
> - status = "okay";
> -
> - clock-frequency = <100000>;
> - i2c-scl-falling-time-ns = <300>;
> - i2c-scl-rising-time-ns = <1000>;
> -};
> -
> &io_domains {
> status = "okay";
>
> --
> 2.21.0.1020.gf2820cf01a-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, May 02, 2019 at 03:53:35PM -0700, Douglas Anderson wrote:
> This adds the "unwedge" pinctrl entries introduced by a recent dw_hdmi
> change that can unwedge the dw_hdmi i2c bus in some cases. It's
> expected that any boards using this would add:
>
> pinctrl-names = "default", "unwedge";
> pinctrl-0 = <&hdmi_ddc>;
> pinctrl-1 = <&hdmi_ddc_unwedge>;
>
> Note that this isn't added by default because some boards may choose
> to mux i2c5 for their DDC bus (if that is more tested for them).
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Sean Paul <[email protected]>
> ---
>
> arch/arm/boot/dts/rk3288.dtsi | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 74c9517c4f92..eebc04fa1e4d 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -1545,6 +1545,15 @@
> rockchip,pins = <7 RK_PC3 2 &pcfg_pull_none>,
> <7 RK_PC4 2 &pcfg_pull_none>;
> };
> +
> + hdmi_ddc_unwedge: hdmi-ddc-unwedge {
> + rockchip,pins = <7 RK_PC3 RK_FUNC_GPIO &pcfg_output_low>,
> + <7 RK_PC4 2 &pcfg_pull_none>;
> + };
> + };
> +
> + pcfg_output_low: pcfg-output-low {
> + output-low;
> };
>
> pcfg_pull_up: pcfg-pull-up {
> --
> 2.21.0.1020.gf2820cf01a-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, May 02, 2019 at 03:53:36PM -0700, Douglas Anderson wrote:
> Veyron uses the builtin i2c controller that's part of dw-hdmi. Hook
> up the unwedging feature.
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Sean Paul <[email protected]>
> ---
>
> arch/arm/boot/dts/rk3288-veyron.dtsi | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
> index e1bee663d2c5..340b276b6333 100644
> --- a/arch/arm/boot/dts/rk3288-veyron.dtsi
> +++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
> @@ -163,8 +163,9 @@
> };
>
> &hdmi {
> - pinctrl-names = "default";
> + pinctrl-names = "default", "unwedge";
> pinctrl-0 = <&hdmi_ddc>;
> + pinctrl-1 = <&hdmi_ddc_unwedge>;
> status = "okay";
> };
>
> --
> 2.21.0.1020.gf2820cf01a-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Hi,
On Wed, May 15, 2019 at 11:20 AM Sean Paul <[email protected]> wrote:
> > + if (IS_ERR(hdmi->default_state) &&
> > + !IS_ERR(hdmi->unwedge_state)) {
> > + dev_warn(dev,
> > + "Unwedge requires default pinctrl\n");
>
> Can you downgrade this message to info or dbg? Given how rare this issue is, we
> probably don't want to spam everyone who is happily using dw-hdmi.
I don't think it will spam anyone, will it? It will only spam if you
_do_ specify an unwedge state and you _don't_ specify a default state.
This seems like something you'd want a pretty serious warning about
because it meant that you wanted to use unwedge but you didn't specify
it properly.
> Reviewed-by: Sean Paul <[email protected]>
Thanks!
-Doug
On Wed, May 15, 2019 at 11:36:33AM -0700, Doug Anderson wrote:
> Hi,
>
> On Wed, May 15, 2019 at 11:20 AM Sean Paul <[email protected]> wrote:
>
> > > + if (IS_ERR(hdmi->default_state) &&
> > > + !IS_ERR(hdmi->unwedge_state)) {
> > > + dev_warn(dev,
> > > + "Unwedge requires default pinctrl\n");
> >
> > Can you downgrade this message to info or dbg? Given how rare this issue is, we
> > probably don't want to spam everyone who is happily using dw-hdmi.
>
> I don't think it will spam anyone, will it? It will only spam if you
> _do_ specify an unwedge state and you _don't_ specify a default state.
> This seems like something you'd want a pretty serious warning about
> because it meant that you wanted to use unwedge but you didn't specify
> it properly.
>
That'll teach me for skimming, you're right on, thanks for the correction!
>
> > Reviewed-by: Sean Paul <[email protected]>
>
> Thanks!
>
> -Doug
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, May 02, 2019 at 03:53:32PM -0700, Douglas Anderson wrote:
> In certain situations it was seen that we could wedge up the DDC bus
> on the HDMI adapter on rk3288. The only way to unwedge was to mux one
> of the pins over to GPIO output-driven-low temporarily and then
> quickly mux back. Full details can be found in the patch
> ("drm/bridge/synopsys: dw-hdmi: Add "unwedge" for ddc bus").
>
> Since unwedge requires remuxing the pins, we first need to add to the
> bindings so that we can specify what state the pins should be in for
> unwedging.
Pushed to drm-misc-next along with patch 2. I'll let Heiko land the dts patches.
Thanks!
Sean
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> .../bindings/display/rockchip/dw_hdmi-rockchip.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> index 39143424a474..8346bac81f1c 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> +++ b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> @@ -38,6 +38,13 @@ Optional properties
> - phys: from general PHY binding: the phandle for the PHY device.
> - phy-names: Should be "hdmi" if phys references an external phy.
>
> +Optional pinctrl entry:
> +- If you have both a "unwedge" and "default" pinctrl entry, dw_hdmi
> + will switch to the unwedge pinctrl state for 10ms if it ever gets an
> + i2c timeout. It's intended that this unwedge pinctrl entry will
> + cause the SDA line to be driven low to work around a hardware
> + errata.
> +
> Example:
>
> hdmi: hdmi@ff980000 {
> --
> 2.21.0.1020.gf2820cf01a-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Am Freitag, 3. Mai 2019, 00:53:34 CEST schrieb Douglas Anderson:
> Downstream Chrome OS kernels use the builtin DDC bus from dw_hdmi on
> veyron. This is the only way to get them to negotiate HDCP.
>
> Although HDCP isn't currently all supported upstream, it still seems
> like it makes sense to use dw_hdmi's builtin I2C. Maybe eventually we
> can get HDCP negotiation working.
>
> Signed-off-by: Douglas Anderson <[email protected]>
applied patches 3 to 5 for 5.3
Thanks
Heiko