| Message ID | 20210311222436.3826800-3-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Si5341 driver updates | expand |
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@topicproducts.com W: www.topicproducts.com 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@calian.com> > --- > 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'm also pretty sure there's a built-in kernel function to poll a register with timeout that you should use here. > + 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)) {
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@topicproducts.com > 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@calian.com> > > --- > > 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)) { > >
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"); + 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)) {
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@calian.com> --- drivers/clk/clk-si5341.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)