dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
@ 2025-05-28 13:21 Michael Walle
  2025-06-09 22:29 ` Doug Anderson
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Walle @ 2025-05-28 13:21 UTC (permalink / raw)
  To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter
  Cc: dri-devel, linux-kernel, Michael Walle

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
  2025-05-28 13:21 [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting Michael Walle
@ 2025-06-09 22:29 ` Doug Anderson
  2025-06-10  7:03   ` Jayesh Choudhary
  0 siblings, 1 reply; 9+ messages in thread
From: Doug Anderson @ 2025-06-09 22:29 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, dri-devel,
	linux-kernel, Jayesh Choudhary

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>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
  2025-06-09 22:29 ` Doug Anderson
@ 2025-06-10  7:03   ` Jayesh Choudhary
  2025-06-10  7:15     ` Michael Walle
  0 siblings, 1 reply; 9+ messages in thread
From: Jayesh Choudhary @ 2025-06-10  7:03 UTC (permalink / raw)
  To: Doug Anderson, Michael Walle
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, dri-devel,
	linux-kernel

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>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
  2025-06-10  7:03   ` Jayesh Choudhary
@ 2025-06-10  7:15     ` Michael Walle
  2025-06-12  7:34       ` Jayesh Choudhary
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Walle @ 2025-06-10  7:15 UTC (permalink / raw)
  To: Jayesh Choudhary
  Cc: Doug Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, dri-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
  2025-06-10  7:15     ` Michael Walle
@ 2025-06-12  7:34       ` Jayesh Choudhary
  2025-06-12 11:59         ` Michael Walle
  2025-06-12 17:52         ` Doug Anderson
  0 siblings, 2 replies; 9+ messages in thread
From: Jayesh Choudhary @ 2025-06-12  7:34 UTC (permalink / raw)
  To: Michael Walle, Doug Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, dri-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
  2025-06-12  7:34       ` Jayesh Choudhary
@ 2025-06-12 11:59         ` Michael Walle
  2025-06-12 17:52         ` Doug Anderson
  1 sibling, 0 replies; 9+ messages in thread
From: Michael Walle @ 2025-06-12 11:59 UTC (permalink / raw)
  To: Jayesh Choudhary, Doug Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, dri-devel,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 5661 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
  2025-06-12  7:34       ` Jayesh Choudhary
  2025-06-12 11:59         ` Michael Walle
@ 2025-06-12 17:52         ` Doug Anderson
  2025-06-12 22:31           ` Doug Anderson
  1 sibling, 1 reply; 9+ messages in thread
From: Doug Anderson @ 2025-06-12 17:52 UTC (permalink / raw)
  To: Jayesh Choudhary
  Cc: Michael Walle, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, dri-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
  2025-06-12 17:52         ` Doug Anderson
@ 2025-06-12 22:31           ` Doug Anderson
  2025-06-13  6:41             ` Lucas Stach
  0 siblings, 1 reply; 9+ messages in thread
From: Doug Anderson @ 2025-06-12 22:31 UTC (permalink / raw)
  To: Jayesh Choudhary, Lucas Stach, Neil Armstrong
  Cc: Michael Walle, Andrzej Hajda, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, dri-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting
  2025-06-12 22:31           ` Doug Anderson
@ 2025-06-13  6:41             ` Lucas Stach
  0 siblings, 0 replies; 9+ messages in thread
From: Lucas Stach @ 2025-06-13  6:41 UTC (permalink / raw)
  To: Doug Anderson, Jayesh Choudhary, Neil Armstrong
  Cc: Michael Walle, Andrzej Hajda, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, dri-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-06-13  6:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-28 13:21 [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting Michael Walle
2025-06-09 22:29 ` Doug Anderson
2025-06-10  7:03   ` Jayesh Choudhary
2025-06-10  7:15     ` Michael Walle
2025-06-12  7:34       ` Jayesh Choudhary
2025-06-12 11:59         ` Michael Walle
2025-06-12 17:52         ` Doug Anderson
2025-06-12 22:31           ` Doug Anderson
2025-06-13  6:41             ` Lucas Stach

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).