Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting

[Jayesh Choudhary]

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