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

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

[Daniel Mack]

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(+)

diff --git a/Documentation/devicetree/bindings/clock/cirrus,cs2000-cp.yaml b/Documentation/devicetree/bindings/clock/cirrus,cs2000-cp.yaml
index 0abd6ba82dfd..8e68e1746d1c 100644
--- a/Documentation/devicetree/bindings/clock/cirrus,cs2000-cp.yaml
+++ b/Documentation/devicetree/bindings/clock/cirrus,cs2000-cp.yaml
@@ -62,6 +62,11 @@ properties:
       output signal directly from the REF_CLK input.
     $ref: /schemas/types.yaml#/definitions/flag
 
+  cirrus,pll-lock-timeout-ms:
+    description:
+      Specifies the maximum time to wait for the PLL to lock, in milliseconds.
+    default: 100
+
 required:
   - compatible
   - reg
-- 
2.37.2

[PATCH v2 2/2] clk: cs2000-cp: make PLL lock timeout configurable

[Daniel Mack]

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".

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.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/clk/clk-cs2000-cp.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk-cs2000-cp.c b/drivers/clk/clk-cs2000-cp.c
index aa5c72bab83e..2b33617727c2 100644
--- a/drivers/clk/clk-cs2000-cp.c
+++ b/drivers/clk/clk-cs2000-cp.c
@@ -110,6 +110,8 @@ struct cs2000_priv {
 	bool lf_ratio;
 	bool clk_skip;
 
+	unsigned int pll_lock_timeout_ms;
+
 	/* suspend/resume */
 	unsigned long saved_rate;
 	unsigned long saved_parent_rate;
@@ -171,21 +173,16 @@ static int cs2000_ref_clk_bound_rate(struct cs2000_priv *priv,
 static int cs2000_wait_pll_lock(struct cs2000_priv *priv)
 {
 	struct device *dev = priv_to_dev(priv);
-	unsigned int i, val;
+	unsigned int val;
 	int ret;
 
-	for (i = 0; i < 256; i++) {
-		ret = regmap_read(priv->regmap, DEVICE_CTRL, &val);
-		if (ret < 0)
-			return ret;
-		if (!(val & PLL_UNLOCK))
-			return 0;
-		udelay(1);
-	}
-
-	dev_err(dev, "pll lock failed\n");
+	ret = regmap_read_poll_timeout(priv->regmap, DEVICE_CTRL, val,
+				       !(val & PLL_UNLOCK), USEC_PER_MSEC,
+				       priv->pll_lock_timeout_ms * USEC_PER_MSEC);
+	if (ret < 0)
+		dev_err(dev, "pll lock failed\n");
 
-	return -ETIMEDOUT;
+	return ret;
 }
 
 static int cs2000_clk_out_enable(struct cs2000_priv *priv, bool enable)
@@ -481,6 +478,10 @@ static int cs2000_clk_register(struct cs2000_priv *priv)
 	if (ret < 0)
 		return ret;
 
+	priv->pll_lock_timeout_ms = 100;
+	of_property_read_u32(np, "cirrus,pll-lock-timeout-ms",
+			     &priv->pll_lock_timeout_ms);
+
 	priv->clk_skip = of_property_read_bool(np, "cirrus,clock-skip");
 
 	ref_clk_rate = clk_get_rate(priv->ref_clk);
-- 
2.37.2

Re: [PATCH v2 2/2] clk: cs2000-cp: make PLL lock timeout configurable

[Stephen Boyd]

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.
>

Re: [PATCH v2 2/2] clk: cs2000-cp: make PLL lock timeout configurable

[Rob Herring]

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

Re: [PATCH v2 2/2] clk: cs2000-cp: make PLL lock timeout configurable

[Daniel Mack]

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