Re: [PATCH 4/9] clk: si5341: Check for input clock presence and PLL lock on startup

[Mike Looijmans <mike.looijmans@topic.nl>]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 11-03-2021 23:24, Robert Hancock wrote:
> After initializing the device, allow sufficient time for the PLL to lock
> (if we reconfigured it) and verify that the input clock is present and the
> PLL has locked before declaring success.
>
> Fixes: 3044a860fd ("clk: Add Si5341/Si5340 driver")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>   drivers/clk/clk-si5341.c | 46 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
>
> diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c
> index 2d69b2144acf..5221e431f6cb 100644
> --- a/drivers/clk/clk-si5341.c
> +++ b/drivers/clk/clk-si5341.c
> @@ -92,6 +92,9 @@ struct clk_si5341_output_config {
>   #define SI5341_PN_BASE		0x0002
>   #define SI5341_DEVICE_REV	0x0005
>   #define SI5341_STATUS		0x000C
> +#define SI5341_LOS		0x000D
> +#define SI5341_STATUS_STICKY	0x0011
> +#define SI5341_LOS_STICKY	0x0012
>   #define SI5341_SOFT_RST		0x001C
>   #define SI5341_IN_SEL		0x0021
>   #define SI5341_DEVICE_READY	0x00FE
> @@ -99,6 +102,12 @@ struct clk_si5341_output_config {
>   #define SI5341_IN_EN		0x0949
>   #define SI5341_INX_TO_PFD_EN	0x094A
>   
> +/* Status bits */
> +#define SI5341_STATUS_SYSINCAL	BIT(0)
> +#define SI5341_STATUS_LOSXAXB	BIT(1)
> +#define SI5341_STATUS_LOSREF	BIT(2)
> +#define SI5341_STATUS_LOL	BIT(3)
> +
>   /* Input selection */
>   #define SI5341_IN_SEL_MASK	0x06
>   #define SI5341_IN_SEL_SHIFT	1
> @@ -1403,6 +1412,29 @@ static int si5341_clk_select_active_input(struct clk_si5341 *data)
>   	return res;
>   }
>   
> +static int si5341_check_healthy(struct clk_si5341 *data)
> +{
> +	u32 status;
> +	int res = regmap_read(data->regmap, SI5341_STATUS, &status);
> +
> +	if (res < 0) {
> +		dev_err(&data->i2c_client->dev, "failed to read status\n");
> +		return res;
> +	}
> +
> +	if ((status & SI5341_STATUS_LOSREF)) {
> +		dev_err(&data->i2c_client->dev, "input clock not present\n");
> +		return -EIO;
> +	}
> +
> +	if ((status & SI5341_STATUS_LOL)) {
> +		dev_err(&data->i2c_client->dev, "PLL not locked\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
>   static int si5341_probe(struct i2c_client *client,
>   		const struct i2c_device_id *id)
>   {
> @@ -1580,6 +1612,20 @@ static int si5341_probe(struct i2c_client *client,
>   		err = si5341_finalize_defaults(data);
>   		if (err < 0)
>   			return err;
> +
> +		/* allow time for PLL to lock */
> +		msleep(250);

Can't this be a poll loop with timeout? Seems rather harsh to just sleep 
here.

> +	}
> +
> +	err = si5341_check_healthy(data);
> +	if (err)
> +		return err;
> +
> +	/* clear sticky alarm bits from initialization */
> +	err = regmap_write(data->regmap, SI5341_STATUS_STICKY, 0);
> +	if (err) {
> +		dev_err(&client->dev, "unable to clear sticky status\n");
> +		return err;
>   	}
>   
>   	/* Free the names, clk framework makes copies */


-- 
Mike Looijmans