drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
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>
---
I couldn't find a good commit for a Fixes: tag and I'm not sure how
fixes are handled in drm.
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 60224f476e1d..fcef43154558 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -386,6 +386,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. 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 lost/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 Wed, May 28, 2025 at 6:21 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. +Jayesh might have some insight? > Signed-off-by: Michael Walle <mwalle@kernel.org> > --- > I couldn't find a good commit for a Fixes: tag and I'm not sure how > fixes are handled in drm. > > 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 60224f476e1d..fcef43154558 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -386,6 +386,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. 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 lost/overwritten by the bridge itself. > + * Waiting for 20us seems to work. > + */ > + usleep_range(20, 30); It might be worth pointing at _where_ the driver overwrites this setting, or maybe at least pointing to something that makes it easy to find which exact bits you're talking about. This looks reasonable to me, though. Reviewed-by: Douglas Anderson <dianders@chromium.org>
Hello Michael, Doug, On 10/06/25 03:59, Doug Anderson wrote: > Hi, > > On Wed, May 28, 2025 at 6:21 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. > > +Jayesh might have some insight? > > > >> Signed-off-by: Michael Walle <mwalle@kernel.org> >> --- >> I couldn't find a good commit for a Fixes: tag and I'm not sure how >> fixes are handled in drm. >> >> 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 60224f476e1d..fcef43154558 100644 >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c >> @@ -386,6 +386,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. 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 lost/overwritten by the bridge itself. >> + * Waiting for 20us seems to work. >> + */ >> + usleep_range(20, 30); > > It might be worth pointing at _where_ the driver overwrites this > setting, or maybe at least pointing to something that makes it easy to > find which exact bits you're talking about. > > This looks reasonable to me, though. I think we are talking about SN_DPPLL_SRC_REG[3:1] bits? What exact mismatch are you observing in register value? I am assuming that you have a clock at REFCLK pin. For that: If refclk is described in devicetree node, then I see that the driver modifies it in every resume call based solely on the clock value in dts. If refclk is not described in dts, then this register is modified by the driver only when pre_enable() calls enable_comms(). Here also, the value depends on crtc_mode and the refclk_rate often would not be equal to the values in "ti_sn_bridge_dsiclk_lut" (supported frequencies), and you would fallback to "001" register value. Rest of time, I guess it depends on reading the status from GPIO and changing the register. Is the latter one your usecase? - Jayesh > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
Hi Jayesh, >>> + /* >>> + * 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. 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 lost/overwritten by the bridge >>> itself. >>> + * Waiting for 20us seems to work. >>> + */ >>> + usleep_range(20, 30); >> >> It might be worth pointing at _where_ the driver overwrites this >> setting, or maybe at least pointing to something that makes it easy to >> find which exact bits you're talking about. Yeah, Jayesh just pointed that out below. I'll add add it to the comment. >> This looks reasonable to me, though. > > I think we are talking about SN_DPPLL_SRC_REG[3:1] bits? Yes. > What exact mismatch are you observing in register value? The one set by the chip itself vs the one from the driver, see below. > I am assuming that you have a clock at REFCLK pin. For that: Yes, I'm using an external clock. > If refclk is described in devicetree node, then I see that > the driver modifies it in every resume call based solely on the > clock value in dts. Exactly. But that is racy with what the chip itself is doing. I.e. if you don't have that usleep() above, the chip will win the race and the refclk frequency setting will be set according to the external GPIOs (which is poorly described in the datasheet, btw), regardless what the linux driver is setting (because that I2C write happens too early). > If refclk is not described in dts, then this register is modified by > the > driver only when pre_enable() calls enable_comms(). Here also, the > value depends on crtc_mode and the refclk_rate often would not be equal > to the values in "ti_sn_bridge_dsiclk_lut" (supported frequencies), and > you would fallback to "001" register value. > Rest of time, I guess it depends on reading the status from GPIO and > changing the register. Not "rest of the time", the reading of the strapping option from the GPIO always happens if an external refclk is detected. It's part of the chip after all. It will just sometimes be overwritten by the linux driver. > Is the latter one your usecase? My use case is that the GPIO setting is wrong on my board (it's really non-existent) and I'm relying on the linux driver to set the correct frequency. HTH, -michael
Hello Michael, On 10/06/25 12:45, Michael Walle wrote: > Hi Jayesh, > >>>> + /* >>>> + * 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. 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 lost/overwritten by the bridge >>>> itself. >>>> + * Waiting for 20us seems to work. >>>> + */ >>>> + usleep_range(20, 30); >>> >>> It might be worth pointing at _where_ the driver overwrites this >>> setting, or maybe at least pointing to something that makes it easy to >>> find which exact bits you're talking about. > > Yeah, Jayesh just pointed that out below. I'll add add it to the comment. > >>> This looks reasonable to me, though. >> >> I think we are talking about SN_DPPLL_SRC_REG[3:1] bits? > > Yes. > >> What exact mismatch are you observing in register value? > > The one set by the chip itself vs the one from the driver, see below. > >> I am assuming that you have a clock at REFCLK pin. For that: > > Yes, I'm using an external clock. > >> If refclk is described in devicetree node, then I see that >> the driver modifies it in every resume call based solely on the >> clock value in dts. > > Exactly. But that is racy with what the chip itself is doing. I.e. > if you don't have that usleep() above, the chip will win the race > and the refclk frequency setting will be set according to the > external GPIOs (which is poorly described in the datasheet, btw), > regardless what the linux driver is setting (because that I2C write > happens too early). I am a little confused here. Won't it be opposite? If we have this delay here, GPIO will stabilize and set the register accordingly? In the driver, I came across the case when we do not have refclk. (My platform does have a refclk, I am just removing the property from the dts node to check the affect of GPIO[3:1] in question because clock is not a required property for the bridge as per the bindings) In the ti_sn65dsi86_probe(), before we read SN_DEVICE_ID_REGS, when we go to resume(), we do not do enable_comms() that calls ti_sn_bridge_set_refclk_freq() to set SN_DPPLL_SRC_REG. I see that register read for SN_DEVICE_ID_REGS fails in that case. Adding this delay fixes that issue. This made me think that we need the delay for GPIO to stabilize and set the refclk. Is my understanding incorrect? I am totally on board with your change especially after observing the above but is my understanding incorrect somewhere? Warm Regards, Jayesh > >> If refclk is not described in dts, then this register is modified by the >> driver only when pre_enable() calls enable_comms(). Here also, the >> value depends on crtc_mode and the refclk_rate often would not be equal >> to the values in "ti_sn_bridge_dsiclk_lut" (supported frequencies), and >> you would fallback to "001" register value. > >> Rest of time, I guess it depends on reading the status from GPIO and >> changing the register. > > Not "rest of the time", the reading of the strapping option from the > GPIO always happens if an external refclk is detected. It's part of > the chip after all. It will just sometimes be overwritten by the > linux driver. > >> Is the latter one your usecase? > > My use case is that the GPIO setting is wrong on my board (it's really > non-existent) and I'm relying on the linux driver to set the correct > frequency. > > HTH, > -michael
Hi, On Thu, Jun 12, 2025 at 12:35 AM Jayesh Choudhary <j-choudhary@ti.com> wrote: > > >> If refclk is described in devicetree node, then I see that > >> the driver modifies it in every resume call based solely on the > >> clock value in dts. > > > > Exactly. But that is racy with what the chip itself is doing. I.e. > > if you don't have that usleep() above, the chip will win the race > > and the refclk frequency setting will be set according to the > > external GPIOs (which is poorly described in the datasheet, btw), > > regardless what the linux driver is setting (because that I2C write > > happens too early). > > I am a little confused here. > Won't it be opposite? > If we have this delay here, GPIO will stabilize and set the register > accordingly? > > In the driver, I came across the case when we do not have refclk. > (My platform does have a refclk, I am just removing the property from > the dts node to check the affect of GPIO[3:1] in question because clock > is not a required property for the bridge as per the bindings) > > In the ti_sn65dsi86_probe(), before we read SN_DEVICE_ID_REGS, > when we go to resume(), we do not do enable_comms() that calls > ti_sn_bridge_set_refclk_freq() to set SN_DPPLL_SRC_REG. > I see that register read for SN_DEVICE_ID_REGS fails in that case. > > Adding this delay fixes that issue. This made me think that we need > the delay for GPIO to stabilize and set the refclk. FWIW, it's been on my plate for a while to delete the "no refclk" support. The chip is really hard to use properly without a refclk and I'm not at all convinced that the current code actually works properly without a refclk. I'm not aware of any current hardware working this way. I know we had some very early prototype hardware ages ago that tried it and we got it limping along at one point, but the driver looked _very_ different then. I believe someone on the lists once mentioned trying to do something without a refclk and it didn't work and I strongly encouraged them to add a refclk. -Doug
Hi,
On Thu, Jun 12, 2025 at 10:52 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Jun 12, 2025 at 12:35 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
> >
> > >> If refclk is described in devicetree node, then I see that
> > >> the driver modifies it in every resume call based solely on the
> > >> clock value in dts.
> > >
> > > Exactly. But that is racy with what the chip itself is doing. I.e.
> > > if you don't have that usleep() above, the chip will win the race
> > > and the refclk frequency setting will be set according to the
> > > external GPIOs (which is poorly described in the datasheet, btw),
> > > regardless what the linux driver is setting (because that I2C write
> > > happens too early).
> >
> > I am a little confused here.
> > Won't it be opposite?
> > If we have this delay here, GPIO will stabilize and set the register
> > accordingly?
> >
> > In the driver, I came across the case when we do not have refclk.
> > (My platform does have a refclk, I am just removing the property from
> > the dts node to check the affect of GPIO[3:1] in question because clock
> > is not a required property for the bridge as per the bindings)
> >
> > In the ti_sn65dsi86_probe(), before we read SN_DEVICE_ID_REGS,
> > when we go to resume(), we do not do enable_comms() that calls
> > ti_sn_bridge_set_refclk_freq() to set SN_DPPLL_SRC_REG.
> > I see that register read for SN_DEVICE_ID_REGS fails in that case.
> >
> > Adding this delay fixes that issue. This made me think that we need
> > the delay for GPIO to stabilize and set the refclk.
>
> FWIW, it's been on my plate for a while to delete the "no refclk"
> support. The chip is really hard to use properly without a refclk and
> I'm not at all convinced that the current code actually works properly
> without a refclk. I'm not aware of any current hardware working this
> way. I know we had some very early prototype hardware ages ago that
> tried it and we got it limping along at one point, but the driver
> looked _very_ different then. I believe someone on the lists once
> mentioned trying to do something without a refclk and it didn't work
> and I strongly encouraged them to add a refclk.
Actually, I may have to eat my words here. I double-checked the dts
and I see there's at least two mainline users
("meson-g12b-bananapi-cm4-mnt-reform2.dts" and
"/imx8mq-mnt-reform2.dts") that don't seem to be specifying a `refclk`
to `ti,sn65dsi86`.
Neil / Lucas: is that correct? ...and it actually works?
-Doug
Am Donnerstag, dem 12.06.2025 um 15:31 -0700 schrieb Doug Anderson:
> Hi,
>
> On Thu, Jun 12, 2025 at 10:52 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Thu, Jun 12, 2025 at 12:35 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
> > >
> > > > > If refclk is described in devicetree node, then I see that
> > > > > the driver modifies it in every resume call based solely on the
> > > > > clock value in dts.
> > > >
> > > > Exactly. But that is racy with what the chip itself is doing. I.e.
> > > > if you don't have that usleep() above, the chip will win the race
> > > > and the refclk frequency setting will be set according to the
> > > > external GPIOs (which is poorly described in the datasheet, btw),
> > > > regardless what the linux driver is setting (because that I2C write
> > > > happens too early).
> > >
> > > I am a little confused here.
> > > Won't it be opposite?
> > > If we have this delay here, GPIO will stabilize and set the register
> > > accordingly?
> > >
> > > In the driver, I came across the case when we do not have refclk.
> > > (My platform does have a refclk, I am just removing the property from
> > > the dts node to check the affect of GPIO[3:1] in question because clock
> > > is not a required property for the bridge as per the bindings)
> > >
> > > In the ti_sn65dsi86_probe(), before we read SN_DEVICE_ID_REGS,
> > > when we go to resume(), we do not do enable_comms() that calls
> > > ti_sn_bridge_set_refclk_freq() to set SN_DPPLL_SRC_REG.
> > > I see that register read for SN_DEVICE_ID_REGS fails in that case.
> > >
> > > Adding this delay fixes that issue. This made me think that we need
> > > the delay for GPIO to stabilize and set the refclk.
> >
> > FWIW, it's been on my plate for a while to delete the "no refclk"
> > support. The chip is really hard to use properly without a refclk and
> > I'm not at all convinced that the current code actually works properly
> > without a refclk. I'm not aware of any current hardware working this
> > way. I know we had some very early prototype hardware ages ago that
> > tried it and we got it limping along at one point, but the driver
> > looked _very_ different then. I believe someone on the lists once
> > mentioned trying to do something without a refclk and it didn't work
> > and I strongly encouraged them to add a refclk.
>
> Actually, I may have to eat my words here. I double-checked the dts
> and I see there's at least two mainline users
> ("meson-g12b-bananapi-cm4-mnt-reform2.dts" and
> "/imx8mq-mnt-reform2.dts") that don't seem to be specifying a `refclk`
> to `ti,sn65dsi86`.
>
> Neil / Lucas: is that correct? ...and it actually works?
>
The description is correct, the refclock is not connected on the reform
baseboard.
It sort of works, as-in AUX channel is not working before the display
link is up to provide a reference clock and I guess that also means HPD
is broken. On the reform the connected panel is described as a simple
panel with a fixed mode, not using the EDID from panel.
Regards,
Lucas
Hi Jayesh,
> >>>> + /*
> >>>> + * 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. 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 lost/overwritten by the bridge
> >>>> itself.
> >>>> + * Waiting for 20us seems to work.
> >>>> + */
> >>>> + usleep_range(20, 30);
> >>>
> >>> It might be worth pointing at _where_ the driver overwrites this
> >>> setting, or maybe at least pointing to something that makes it easy to
> >>> find which exact bits you're talking about.
> >
> > Yeah, Jayesh just pointed that out below. I'll add add it to the comment.
> >
> >>> This looks reasonable to me, though.
> >>
> >> I think we are talking about SN_DPPLL_SRC_REG[3:1] bits?
> >
> > Yes.
> >
> >> What exact mismatch are you observing in register value?
> >
> > The one set by the chip itself vs the one from the driver, see below.
> >
> >> I am assuming that you have a clock at REFCLK pin. For that:
> >
> > Yes, I'm using an external clock.
> >
> >> If refclk is described in devicetree node, then I see that
> >> the driver modifies it in every resume call based solely on the
> >> clock value in dts.
> >
> > Exactly. But that is racy with what the chip itself is doing. I.e.
> > if you don't have that usleep() above, the chip will win the race
> > and the refclk frequency setting will be set according to the
> > external GPIOs (which is poorly described in the datasheet, btw),
> > regardless what the linux driver is setting (because that I2C write
> > happens too early).
>
> I am a little confused here.
> Won't it be opposite?
> If we have this delay here, GPIO will stabilize and set the register
> accordingly?
What do you mean by GPIO? Maybe we are talking about the very same
thing. From my understanding there are two "parties" involved here:
(1) the linux driver
(2) the bridge IC that comes out of reset when EN is asserted
And both are trying to write to the same setting.
From what I understand, is that (2) is running some kind of state
machine or even firmware that will figure out if there is a refclk
present. If so it will sample the GPIOs and set the refclk frequency
setting accordingly. This happens autonomously after EN is asserted.
Now there is also (1) which will assert the EN signal and shortly
after trying to write the refclk frequency setting.
With this patch we will delay the register write from (1) to a point
after (2) updated its refclk setting. Thus (1) will win.
> In the driver, I came across the case when we do not have refclk.
> (My platform does have a refclk, I am just removing the property from
> the dts node to check the affect of GPIO[3:1] in question because clock
> is not a required property for the bridge as per the bindings)
I'd expect that in this case the refclk is set according to the GPIO
strapping. Correct?
> In the ti_sn65dsi86_probe(), before we read SN_DEVICE_ID_REGS,
> when we go to resume(), we do not do enable_comms() that calls
> ti_sn_bridge_set_refclk_freq() to set SN_DPPLL_SRC_REG.
> I see that register read for SN_DEVICE_ID_REGS fails in that case.
Does it work with the property still in the device tree? I might try
that on my board later.
> Adding this delay fixes that issue. This made me think that we need
> the delay for GPIO to stabilize and set the refclk.
>
> Is my understanding incorrect?
Unfortunately, the datasheet is really sparse on details here, but
maybe the bridge needs some time after EN is assert to respond on
the I2C bus in general. I'm basing my guesswork on the td5 timing
with the vague description "GPIO[3:1] stable after EN assertion". I
assume that somewhere during that time the chip will sample the
GPIOs and do something with that setting (presumable setting its
internal refclk frequency setting). FWIW there is also a td4
("GPIO[3:1] stable before EN assertion"). Both td4 and td5, makes
me believe that this is not some setting which is sampled (and hold)
at reset, otherwise td5 wouldn't make much sense.
> I am totally on board with your change especially after observing the
> above but is my understanding incorrect somewhere?
>
> Warm Regards,
> Jayesh
>
> >
> >> If refclk is not described in dts, then this register is modified by the
> >> driver only when pre_enable() calls enable_comms(). Here also, the
> >> value depends on crtc_mode and the refclk_rate often would not be equal
> >> to the values in "ti_sn_bridge_dsiclk_lut" (supported frequencies), and
> >> you would fallback to "001" register value.
> >
> >> Rest of time, I guess it depends on reading the status from GPIO and
> >> changing the register.
> >
> > Not "rest of the time", the reading of the strapping option from the
> > GPIO always happens if an external refclk is detected. It's part of
> > the chip after all. It will just sometimes be overwritten by the
> > linux driver.
> >
> >> Is the latter one your usecase?
> >
> > My use case is that the GPIO setting is wrong on my board (it's really
> > non-existent) and I'm relying on the linux driver to set the correct
> > frequency.
> >
> > HTH,
> > -michael
© 2016 - 2026 Red Hat, Inc.