The bridge has three bootstrap pins which are sampled to determine the
frequency of the external reference clock. The driver will also
(over)write that setting. But it seems this is racy after the bridge is
enabled. It was observed that although the driver write the correct
value (by sniffing on the I2C bus), the register has the wrong value.
The datasheet states that the GPIO lines have to be stable for at least
5us after asserting the EN signal. Thus, there seems to be some logic
which samples the GPIO lines and this logic appears to overwrite the
register value which was set by the driver. Waiting 20us after
asserting the EN line resolves this issue.

Signed-off-by: Michael Walle <mwalle@kernel.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 464390372b34..ae0d08e5e960 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -392,6 +392,17 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 
 	gpiod_set_value_cansleep(pdata->enable_gpio, 1);
 
+	/*
+	 * After EN is deasserted and an external clock is detected, the bridge
+	 * will sample GPIO3:1 to determine its frequency. The driver will
+	 * overwrite this setting in ti_sn_bridge_set_refclk_freq(). But this is
+	 * racy. Thus we have to wait a couple of us. According to the datasheet
+	 * the GPIO lines has to be stable at least 5 us (td5) but it seems that
+	 * is not enough and the refclk frequency value is still lost or
+	 * overwritten by the bridge itself. Waiting for 20us seems to work.
+	 */
+	usleep_range(20, 30);
+
 	/*
 	 * If we have a reference clock we can enable communication w/ the
 	 * panel (including the aux channel) w/out any need for an input clock
-- 
2.39.5

Hi,

On Thu, Aug 21, 2025 at 5:23 AM Michael Walle <mwalle@kernel.org> wrote:
>
> The bridge has three bootstrap pins which are sampled to determine the
> frequency of the external reference clock. The driver will also
> (over)write that setting. But it seems this is racy after the bridge is
> enabled. It was observed that although the driver write the correct
> value (by sniffing on the I2C bus), the register has the wrong value.
> The datasheet states that the GPIO lines have to be stable for at least
> 5us after asserting the EN signal. Thus, there seems to be some logic
> which samples the GPIO lines and this logic appears to overwrite the
> register value which was set by the driver. Waiting 20us after
> asserting the EN line resolves this issue.
>
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

nit: officially you're supposed to move your Signed-off-by all the way
at the bottom of all the other tags any time you post a patch. I don't
think it's important enough to re-send, though.

In any case, thanks for re-posting. I guess it kinda stagnated. I'll
give this another week on the list and then plan to apply to
drm-misc-fixes unless there are any other comments.

-Doug

Hi,

On Thu, Aug 21, 2025 at 7:36 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Aug 21, 2025 at 5:23 AM Michael Walle <mwalle@kernel.org> wrote:
> >
> > The bridge has three bootstrap pins which are sampled to determine the
> > frequency of the external reference clock. The driver will also
> > (over)write that setting. But it seems this is racy after the bridge is
> > enabled. It was observed that although the driver write the correct
> > value (by sniffing on the I2C bus), the register has the wrong value.
> > The datasheet states that the GPIO lines have to be stable for at least
> > 5us after asserting the EN signal. Thus, there seems to be some logic
> > which samples the GPIO lines and this logic appears to overwrite the
> > register value which was set by the driver. Waiting 20us after
> > asserting the EN line resolves this issue.
> >
> > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> nit: officially you're supposed to move your Signed-off-by all the way
> at the bottom of all the other tags any time you post a patch. I don't
> think it's important enough to re-send, though.
>
> In any case, thanks for re-posting. I guess it kinda stagnated. I'll
> give this another week on the list and then plan to apply to
> drm-misc-fixes unless there are any other comments.

I realized that this is lacking a Fixes: tag. I went back and
confirmed that even in the first version of the driver, AKA commit
a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver"),
we still had no delay between these two lines:

pm_runtime_get_sync(pdata->dev);

/* configure bridge ref_clk */
ti_sn_bridge_set_refclk_freq(pdata);

...and the last line of the runtime resume function was turning on the
enable. So I believe this means that the bug has always been there.
Does that sound right to others? If so, I'll add that Fixes tag when
applying.,..

-Doug

Hi,

On Fri Aug 29, 2025 at 12:52 AM CEST, Doug Anderson wrote:
> > On Thu, Aug 21, 2025 at 5:23 AM Michael Walle <mwalle@kernel.org> wrote:
> > >
> > > The bridge has three bootstrap pins which are sampled to determine the
> > > frequency of the external reference clock. The driver will also
> > > (over)write that setting. But it seems this is racy after the bridge is
> > > enabled. It was observed that although the driver write the correct
> > > value (by sniffing on the I2C bus), the register has the wrong value.
> > > The datasheet states that the GPIO lines have to be stable for at least
> > > 5us after asserting the EN signal. Thus, there seems to be some logic
> > > which samples the GPIO lines and this logic appears to overwrite the
> > > register value which was set by the driver. Waiting 20us after
> > > asserting the EN line resolves this issue.
> > >
> > > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > nit: officially you're supposed to move your Signed-off-by all the way
> > at the bottom of all the other tags any time you post a patch. I don't
> > think it's important enough to re-send, though.
> >
> > In any case, thanks for re-posting. I guess it kinda stagnated. I'll
> > give this another week on the list and then plan to apply to
> > drm-misc-fixes unless there are any other comments.
>
> I realized that this is lacking a Fixes: tag. I went back and
> confirmed that even in the first version of the driver, AKA commit
> a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver"),
> we still had no delay between these two lines:
>
> pm_runtime_get_sync(pdata->dev);
>
> /* configure bridge ref_clk */
> ti_sn_bridge_set_refclk_freq(pdata);
>
> ...and the last line of the runtime resume function was turning on the
> enable. So I believe this means that the bug has always been there.
> Does that sound right to others? If so, I'll add that Fixes tag when
> applying.,..

Yes, that's right. Thanks for amending the patch.

-michael

Hi,

On Sun, Aug 31, 2025 at 11:20 PM Michael Walle <mwalle@kernel.org> wrote:
>
> Hi,
>
> On Fri Aug 29, 2025 at 12:52 AM CEST, Doug Anderson wrote:
> > > On Thu, Aug 21, 2025 at 5:23 AM Michael Walle <mwalle@kernel.org> wrote:
> > > >
> > > > The bridge has three bootstrap pins which are sampled to determine the
> > > > frequency of the external reference clock. The driver will also
> > > > (over)write that setting. But it seems this is racy after the bridge is
> > > > enabled. It was observed that although the driver write the correct
> > > > value (by sniffing on the I2C bus), the register has the wrong value.
> > > > The datasheet states that the GPIO lines have to be stable for at least
> > > > 5us after asserting the EN signal. Thus, there seems to be some logic
> > > > which samples the GPIO lines and this logic appears to overwrite the
> > > > register value which was set by the driver. Waiting 20us after
> > > > asserting the EN line resolves this issue.
> > > >
> > > > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > >
> > > nit: officially you're supposed to move your Signed-off-by all the way
> > > at the bottom of all the other tags any time you post a patch. I don't
> > > think it's important enough to re-send, though.
> > >
> > > In any case, thanks for re-posting. I guess it kinda stagnated. I'll
> > > give this another week on the list and then plan to apply to
> > > drm-misc-fixes unless there are any other comments.
> >
> > I realized that this is lacking a Fixes: tag. I went back and
> > confirmed that even in the first version of the driver, AKA commit
> > a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver"),
> > we still had no delay between these two lines:
> >
> > pm_runtime_get_sync(pdata->dev);
> >
> > /* configure bridge ref_clk */
> > ti_sn_bridge_set_refclk_freq(pdata);
> >
> > ...and the last line of the runtime resume function was turning on the
> > enable. So I believe this means that the bug has always been there.
> > Does that sound right to others? If so, I'll add that Fixes tag when
> > applying.,..
>
> Yes, that's right. Thanks for amending the patch.

Pushed to drm-misc-fixes:

[1/1] drm/bridge: ti-sn65dsi86: fix REFCLK setting
      commit: bdd5a14e660062114bdebaef9ad52adf04970a89