[v2,1/2] dt-bindings: clock: cs2000-cp: Document cirrus,pll-lock-timeout-ms

This property can be used to set the maximum time to wait for the PLL to
lock.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
v1 -> v2: rename property to standard unit suffix, drop $ref, amend default value
 
.../devicetree/bindings/clock/cirrus,cs2000-cp.yaml          | 5 +++++
 1 file changed, 5 insertions(+)

Quoting Daniel Mack (2022-08-26 02:11:22)
> The driver currently does 256 iterations of reads from the DEVICE_CTRL
> register to wait for the PLL_LOCK bit to clear, and sleeps one
> microsecond after each attempt.
> 
> This isn't ideal because
> 
>  a) the total time this allows for the device to settle depends on the I2C
>     bus speed, and
>  b) the device might need more time, depending on the application.
> 
> This patch allows users to configure this timeout through a new device-tree
> property "cirrus,pll-lock-timeout-ms".

It's a timeout, so why not just increase the timeout regardless of
everything else? Or can we parse the bus speed (100kHz or 400kHz)
instead of adding a new property?

> 
> In order to not break existing applications, a default value of 100 ms is
> assumed: For each read cycle, 8 bits are sent for the register address, and
> 8 bits are read with the values. 16 bits take about 160 us on a 100 kHz bus
> and 40 us on a 400 kHz bus. Hence 256 iterations would take a maximum of
> around 44 ms. Round up and double that value to be on the safe side.
>

On Mon, Aug 29, 2022 at 06:49:06PM -0700, Stephen Boyd wrote:
> Quoting Daniel Mack (2022-08-26 02:11:22)
> > The driver currently does 256 iterations of reads from the DEVICE_CTRL
> > register to wait for the PLL_LOCK bit to clear, and sleeps one
> > microsecond after each attempt.
> > 
> > This isn't ideal because
> > 
> >  a) the total time this allows for the device to settle depends on the I2C
> >     bus speed, and
> >  b) the device might need more time, depending on the application.
> > 
> > This patch allows users to configure this timeout through a new device-tree
> > property "cirrus,pll-lock-timeout-ms".
> 
> It's a timeout, so why not just increase the timeout regardless of
> everything else? Or can we parse the bus speed (100kHz or 400kHz)
> instead of adding a new property?

My thought too. Usually PLLs have a spec for max/typ lock times. Given 
it's a should never happen type of thing, it doesn't seem like we need a 
super precise time.

Rob

On 8/30/22 22:25, Rob Herring wrote:
> On Mon, Aug 29, 2022 at 06:49:06PM -0700, Stephen Boyd wrote:
>> Quoting Daniel Mack (2022-08-26 02:11:22)
>>> The driver currently does 256 iterations of reads from the DEVICE_CTRL
>>> register to wait for the PLL_LOCK bit to clear, and sleeps one
>>> microsecond after each attempt.
>>>
>>> This isn't ideal because
>>>
>>>  a) the total time this allows for the device to settle depends on the I2C
>>>     bus speed, and
>>>  b) the device might need more time, depending on the application.
>>>
>>> This patch allows users to configure this timeout through a new device-tree
>>> property "cirrus,pll-lock-timeout-ms".
>>
>> It's a timeout, so why not just increase the timeout regardless of
>> everything else? Or can we parse the bus speed (100kHz or 400kHz)
>> instead of adding a new property?
> 
> My thought too. Usually PLLs have a spec for max/typ lock times. Given 
> it's a should never happen type of thing, it doesn't seem like we need a 
> super precise time.

Alright. I guess the real problem here is with the generation of the
input clock signals, so I need to address that side of the design instead.


Thanks for the feedback!

Daniel