Re: [PATCH 2/9] clk: si5341: Wait for DEVICE_READY on startup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2021-03-12 at 07:39 +0100, Mike Looijmans wrote:
> One remark below.
> 
> 
> 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@xxxxxxxxxxxxxxxxx
> W: 
> https://urldefense.com/v3/__http://www.topicproducts.com__;!!IOGos0k!2aBLUJa-5kaozesyVznxIfxn2Hn5L4i0RdhELnnHU1WJ85FIzmGyRCRmXW5-kXbcsi0$
>  
> 
> Please consider the environment before printing this e-mail
> On 11-03-2021 23:24, Robert Hancock wrote:
> > The Si5341 datasheet warns that before accessing any other registers,
> > including the PAGE register, we need to wait for the DEVICE_READY register
> > to indicate the device is ready, or the process of the device loading its
> > state from NVM can be corrupted. Wait for DEVICE_READY on startup before
> > continuing initialization. This is done using a raw I2C register read
> > prior to setting up regmap to avoid any potential unwanted automatic PAGE
> > register accesses from regmap at this stage.
> > 
> > Fixes: 3044a860fd ("clk: Add Si5341/Si5340 driver")
> > Signed-off-by: Robert Hancock <robert.hancock@xxxxxxxxxx>
> > ---
> >   drivers/clk/clk-si5341.c | 31 +++++++++++++++++++++++++++++++
> >   1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c
> > index e0446e66fa64..f210860fb96e 100644
> > --- a/drivers/clk/clk-si5341.c
> > +++ b/drivers/clk/clk-si5341.c
> > @@ -94,6 +94,7 @@ struct clk_si5341_output_config {
> >   #define SI5341_STATUS		0x000C
> >   #define SI5341_SOFT_RST		0x001C
> >   #define SI5341_IN_SEL		0x0021
> > +#define SI5341_DEVICE_READY	0x00FE
> >   #define SI5341_XAXB_CFG		0x090E
> >   #define SI5341_IN_EN		0x0949
> >   #define SI5341_INX_TO_PFD_EN	0x094A
> > @@ -1189,6 +1190,31 @@ static const struct regmap_range_cfg
> > si5341_regmap_ranges[] = {
> >   	},
> >   };
> >   
> > +static int si5341_wait_device_ready(struct i2c_client *client)
> > +{
> > +	int count;
> > +
> > +	/* Datasheet warns: Any attempt to read or write any register other
> > +	 * than DEVICE_READY before DEVICE_READY reads as 0x0F may corrupt the
> > +	 * NVM programming and may corrupt the register contents, as they are
> > +	 * read from NVM. Note that this includes accesses to the PAGE
> > register.
> > +	 * Also: DEVICE_READY is available on every register page, so no page
> > +	 * change is needed to read it.
> > +	 * Do this outside regmap to avoid automatic PAGE register access.
> > +	 */
> > +	for (count = 0; count < 10; ++count) {
> > +		s32 result = i2c_smbus_read_byte_data(client,
> > +						      SI5341_DEVICE_READY);
> > +		if (result < 0)
> > +			return result;
> > +		if (result == 0x0F)
> > +			return 0;
> > +		usleep_range(1000, 20000);
> > +	}
> > +	dev_err(&client->dev, "timeout waiting for DEVICE_READY\n");
> 
> The "timeout" here is random between 10 and 200 milliseconds.
> 
> The datasheet says that the device may take 300ms to initialize, so I 
> guess 300 milliseconds would be a good timeout.

I think I missed the 300ms figure. We should definitely allow at least that
much time then. Will address in v2.

> 
> I'm also pretty sure there's a built-in kernel function to poll a 
> register with timeout that you should use here.

There is a regmap_read_poll_timeout function, but we're trying to specifically
not use regmap here because it may access other registers behind our back. I'm
not aware of something better that operates at the I2C layer..

> 
> 
> > +	return -EIO;
> > +}
> > +
> >   static const struct regmap_config si5341_regmap_config = {
> >   	.reg_bits = 8,
> >   	.val_bits = 8,
> > @@ -1385,6 +1411,11 @@ static int si5341_probe(struct i2c_client *client,
> >   
> >   	data->i2c_client = client;
> >   
> > +	/* Must be done before otherwise touching hardware */
> > +	err = si5341_wait_device_ready(client);
> > +	if (err)
> > +		return err;
> > +
> >   	for (i = 0; i < SI5341_NUM_INPUTS; ++i) {
> >   		input = devm_clk_get(&client->dev,
> > si5341_input_clock_names[i]);
> >   		if (IS_ERR(input)) {
> 
> 
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux