Message-Id: <DAKJD89W5O3D.1WXVSY2RLLEFZ@kernel.org>
Date: Thu, 12 Jun 2025 13:59:31 +0200
From: "Michael Walle" <mwalle@...nel.org>
To: "Jayesh Choudhary" <j-choudhary@...com>, "Doug Anderson"
 <dianders@...omium.org>
Cc: "Andrzej Hajda" <andrzej.hajda@...el.com>, "Neil Armstrong"
 <neil.armstrong@...aro.org>, "Robert Foss" <rfoss@...nel.org>, "Laurent
 Pinchart" <Laurent.pinchart@...asonboard.com>, "Jonas Karlman"
 <jonas@...boo.se>, "Jernej Skrabec" <jernej.skrabec@...il.com>, "Maarten
 Lankhorst" <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard"
 <mripard@...nel.org>, "Thomas Zimmermann" <tzimmermann@...e.de>, "David
 Airlie" <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>,
 <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/bridge: ti-sn65dsi86: fix REFCLK setting

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


Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)

Powered by blists - more mailing lists