| Message ID | 20210311222436.3826800-1-robert.hancock@calian.com |
|---|---|
| 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)) { -- Mike Looijmans
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: > Add a property to allow specifying that the external I2C IO pins are using > 3.3V voltage thresholds rather than 1.8V. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > drivers/clk/clk-si5341.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c > index 11740855bcde..4cd80ef389d2 100644 > --- a/drivers/clk/clk-si5341.c > +++ b/drivers/clk/clk-si5341.c > @@ -81,6 +81,7 @@ struct clk_si5341 { > u8 num_synth; > u16 chip_id; > bool xaxb_ext_clk; > + bool iovdd_33; > }; > #define to_clk_si5341(_hw) container_of(_hw, struct clk_si5341, hw) > > @@ -103,6 +104,7 @@ struct clk_si5341_output_config { > #define SI5341_IN_SEL 0x0021 > #define SI5341_DEVICE_READY 0x00FE > #define SI5341_XAXB_CFG 0x090E > +#define SI5341_IO_VDD_SEL 0x0943 > #define SI5341_IN_EN 0x0949 > #define SI5341_INX_TO_PFD_EN 0x094A > > @@ -351,7 +353,6 @@ static const struct si5341_reg_default si5341_reg_defaults[] = { > { 0x0804, 0x00 }, /* Not in datasheet */ > { 0x090E, 0x02 }, /* XAXB_EXTCLK_EN=0 XAXB_PDNB=1 (use XTAL) */ > { 0x091C, 0x04 }, /* ZDM_EN=4 (Normal mode) */ > - { 0x0943, 0x00 }, /* IO_VDD_SEL=0 (0=1v8, use 1=3v3) */ > { 0x0949, 0x00 }, /* IN_EN (disable input clocks) */ > { 0x094A, 0x00 }, /* INx_TO_PFD_EN (disabled) */ > { 0x0A02, 0x00 }, /* Not in datasheet */ > @@ -1160,6 +1161,11 @@ static int si5341_finalize_defaults(struct clk_si5341 *data) > int res; > u32 revision; > > + res = regmap_write(data->regmap, SI5341_IO_VDD_SEL, > + data->iovdd_33 ? 1 : 0); > + if (res < 0) > + return res; > + > res = regmap_read(data->regmap, SI5341_DEVICE_REV, &revision); > if (res < 0) > return res; > @@ -1565,6 +1571,8 @@ static int si5341_probe(struct i2c_client *client, > } > data->xaxb_ext_clk = of_property_read_bool(client->dev.of_node, > "silabs,xaxb-ext-clk"); > + data->iovdd_33 = of_property_read_bool(client->dev.of_node, > + "silabs,iovdd-33"); > Seems a waste to me to store this in the 'data' object forever while it is to be used only once during init and never again after that. > if (initialization_required) { > /* Populate the regmap cache in preparation for "cache only" */ -- Mike Looijmans
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
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)) { > > -- Robert Hancock Senior Hardware Designer, Calian Advanced Technologies www.calian.com
On Fri, 2021-03-12 at 07:47 +0100, Mike Looijmans wrote: > 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!wiWtHGRxxa459e4cDke1e1LNom9k55h5ayHt6t0CboF-tMxY8bgYWiRsplbzxz6sN94$ > > > 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. Indeed, regmap_read_poll_timeout would likely work here. Will address in v2. > > > + } > > + > > + 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 */ > >
On Fri, 2021-03-12 at 07:44 +0100, Mike Looijmans wrote: > 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!1ZKoWVwv09fDC-o4jRw92NZvHBl_3MOcBIPl8DLLDN0szJlo9IH0GC-6qH0-Pq1W-PA$ > > > Please consider the environment before printing this e-mail > On 11-03-2021 23:24, Robert Hancock wrote: > > Add a property to allow specifying that the external I2C IO pins are using > > 3.3V voltage thresholds rather than 1.8V. > > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > --- > > drivers/clk/clk-si5341.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c > > index 11740855bcde..4cd80ef389d2 100644 > > --- a/drivers/clk/clk-si5341.c > > +++ b/drivers/clk/clk-si5341.c > > @@ -81,6 +81,7 @@ struct clk_si5341 { > > u8 num_synth; > > u16 chip_id; > > bool xaxb_ext_clk; > > + bool iovdd_33; > > }; > > #define to_clk_si5341(_hw) container_of(_hw, struct clk_si5341, > > hw) > > > > @@ -103,6 +104,7 @@ struct clk_si5341_output_config { > > #define SI5341_IN_SEL 0x0021 > > #define SI5341_DEVICE_READY 0x00FE > > #define SI5341_XAXB_CFG 0x090E > > +#define SI5341_IO_VDD_SEL 0x0943 > > #define SI5341_IN_EN 0x0949 > > #define SI5341_INX_TO_PFD_EN 0x094A > > > > @@ -351,7 +353,6 @@ static const struct si5341_reg_default > > si5341_reg_defaults[] = { > > { 0x0804, 0x00 }, /* Not in datasheet */ > > { 0x090E, 0x02 }, /* XAXB_EXTCLK_EN=0 XAXB_PDNB=1 (use XTAL) */ > > { 0x091C, 0x04 }, /* ZDM_EN=4 (Normal mode) */ > > - { 0x0943, 0x00 }, /* IO_VDD_SEL=0 (0=1v8, use 1=3v3) */ > > { 0x0949, 0x00 }, /* IN_EN (disable input clocks) */ > > { 0x094A, 0x00 }, /* INx_TO_PFD_EN (disabled) */ > > { 0x0A02, 0x00 }, /* Not in datasheet */ > > @@ -1160,6 +1161,11 @@ static int si5341_finalize_defaults(struct > > clk_si5341 *data) > > int res; > > u32 revision; > > > > + res = regmap_write(data->regmap, SI5341_IO_VDD_SEL, > > + data->iovdd_33 ? 1 : 0); > > + if (res < 0) > > + return res; > > + > > res = regmap_read(data->regmap, SI5341_DEVICE_REV, &revision); > > if (res < 0) > > return res; > > @@ -1565,6 +1571,8 @@ static int si5341_probe(struct i2c_client *client, > > } > > data->xaxb_ext_clk = of_property_read_bool(client->dev.of_node, > > "silabs,xaxb-ext-clk"); > > + data->iovdd_33 = of_property_read_bool(client->dev.of_node, > > + "silabs,iovdd-33"); > > > > Seems a waste to me to store this in the 'data' object forever while it > is to be used only once during init and never again after that. That is true, but it would then have to be passed down into the si5341_finalize_defaults function separately and it would be inconsistent with how all of the other configuration is handled. So I'm not sure it's worth saving ~ 1 byte in the data object.. > > > > if (initialization_required) { > > /* Populate the regmap cache in preparation for "cache only" */ > >
Various fixes and enhancements to the Si5341 driver. Robert Hancock (9): dt-bindings: clock: clk-si5341: Add new attributes clk: si5341: Wait for DEVICE_READY on startup clk: si5341: Avoid divide errors due to bogus register contents clk: si5341: Check for input clock presence and PLL lock on startup clk: si5341: Update initialization magic clk: si5341: Allow different output VDD_SEL values clk: si5341: Add silabs,xaxb-ext-clk property clk: si5341: Add silabs,iovdd-33 property clk: si5341: Add sysfs properties to allow checking/resetting device faults .../bindings/clock/silabs,si5341.txt | 16 +- drivers/clk/clk-si5341.c | 343 ++++++++++++++++-- 2 files changed, 323 insertions(+), 36 deletions(-)