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

[Michael Walle <mwalle@kernel.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