| Message ID | 20201103141741.2511-1-mike.looijmans@topic.nl (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk-si5341: Support NVM programming through sysfs | expand |
Quoting Mike Looijmans (2020-11-03 06:17:41) > Export an attribute program_nvm_bank that when read reports the current > bank value. To program the chip's current state into NVM, write the > magic value 0xC7 into this attribute. > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > --- Any chance this can be done through the nvmem framework? > drivers/clk/clk-si5341.c | 73 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c > index e0446e66fa64..4e025a5ea2b7 100644 > --- a/drivers/clk/clk-si5341.c > +++ b/drivers/clk/clk-si5341.c > @@ -1199,6 +1205,69 @@ static const struct regmap_config si5341_regmap_config = { > .volatile_table = &si5341_regmap_volatile, > }; > > +static ssize_t program_nvm_bank_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct clk_si5341 *data = i2c_get_clientdata(client); > + unsigned int regval; > + int ret; > + > + ret = regmap_read(data->regmap, SI5341_ACTIVE_NVM_BANK, ®val); > + if (ret) > + return ret; > + > + return sprintf(buf, "%#x\n", regval); > +} > + > +static ssize_t program_nvm_bank_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + struct clk_si5341 *data = i2c_get_clientdata(to_i2c_client(dev)); > + int ret; > + unsigned int value; > + unsigned int timeout; > + > + ret = kstrtouint(buf, 0, &value); > + if (ret < 0) > + return ret; > + > + /* Write the magic value to this attribute to program the NVM */ > + if (value != SI5341_SI5341_NVM_WRITE_COOKIE) > + return -EINVAL; > + > + ret = regmap_write(data->regmap, SI5341_NVM_WRITE, > + SI5341_SI5341_NVM_WRITE_COOKIE); > + if (ret) > + return ret; > + > + /* Wait for SI5341_DEVICE_READY register to become 0x0f */ > + for (timeout = 10000; timeout; --timeout) { > + ret = regmap_read(data->regmap, SI5341_DEVICE_READY, &value); This is regmap_read_poll_timeout()? > + if (ret) > + return ret; > + > + if (value == 0x0f) > + break; > + } > + > + return count; > +} > + > +static DEVICE_ATTR_RW(program_nvm_bank); > + > +static struct attribute *si5341_sysfs_entries[] = { > + &dev_attr_program_nvm_bank.attr, > + NULL, > +}; > + > +static struct attribute_group si5341_attr_group = { > + .name = NULL, /* put in device directory */ > + .attrs = si5341_sysfs_entries, > +}; If not nvmem framework, then this needs to be documented in Documentation/ABI/ > + > static int si5341_dt_parse_dt(struct i2c_client *client, > struct clk_si5341_output_config *config) > { > @@ -1544,6 +1613,10 @@ static int si5341_probe(struct i2c_client *client, > for (i = 0; i < data->num_synth; ++i) > devm_kfree(&client->dev, (void *)synth_clock_names[i]); > > + err = sysfs_create_group(&client->dev.kobj, &si5341_attr_group); > + if (err) > + dev_err(&client->dev, "failed to create sysfs entries\n"); > + Cool, I as a user would do what in this situation? The error message seems sort of worthless.
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 05-11-2020 02:48, Stephen Boyd wrote: > Quoting Mike Looijmans (2020-11-03 06:17:41) >> Export an attribute program_nvm_bank that when read reports the current >> bank value. To program the chip's current state into NVM, write the >> magic value 0xC7 into this attribute. >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >> --- > > Any chance this can be done through the nvmem framework? This part doesn't fit. The purpose is to store the current state of the clock chip into its non-volatile storage so it boots up with that configuration the next POR. Main use case is that some vendors initialize PLLs only in a bootloader and thus need the clock running at boot. Or it might just be to save on that 300ms initialization time. Having said that, the clock chip does have some "scratch" areas that'd be useful as NVMEM storage. That'd be for a separate patch. For this device to be NVMEM compatible, nvmem would need to have a sort of transaction model, where you write several values and then "commit" them all to NVM in one call. The nvmem framework wasn't intended for that I think. > >> drivers/clk/clk-si5341.c | 73 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 73 insertions(+) >> >> diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c >> index e0446e66fa64..4e025a5ea2b7 100644 >> --- a/drivers/clk/clk-si5341.c >> +++ b/drivers/clk/clk-si5341.c >> @@ -1199,6 +1205,69 @@ static const struct regmap_config si5341_regmap_config = { >> .volatile_table = &si5341_regmap_volatile, >> }; >> >> +static ssize_t program_nvm_bank_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct i2c_client *client = to_i2c_client(dev); >> + struct clk_si5341 *data = i2c_get_clientdata(client); >> + unsigned int regval; >> + int ret; >> + >> + ret = regmap_read(data->regmap, SI5341_ACTIVE_NVM_BANK, ®val); >> + if (ret) >> + return ret; >> + >> + return sprintf(buf, "%#x\n", regval); >> +} >> + >> +static ssize_t program_nvm_bank_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, >> + size_t count) >> +{ >> + struct clk_si5341 *data = i2c_get_clientdata(to_i2c_client(dev)); >> + int ret; >> + unsigned int value; >> + unsigned int timeout; >> + >> + ret = kstrtouint(buf, 0, &value); >> + if (ret < 0) >> + return ret; >> + >> + /* Write the magic value to this attribute to program the NVM */ >> + if (value != SI5341_SI5341_NVM_WRITE_COOKIE) >> + return -EINVAL; >> + >> + ret = regmap_write(data->regmap, SI5341_NVM_WRITE, >> + SI5341_SI5341_NVM_WRITE_COOKIE); >> + if (ret) >> + return ret; >> + >> + /* Wait for SI5341_DEVICE_READY register to become 0x0f */ >> + for (timeout = 10000; timeout; --timeout) { >> + ret = regmap_read(data->regmap, SI5341_DEVICE_READY, &value); > > This is regmap_read_poll_timeout()? Yes, indeed. > >> + if (ret) >> + return ret; >> + >> + if (value == 0x0f) >> + break; >> + } >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR_RW(program_nvm_bank); >> + >> +static struct attribute *si5341_sysfs_entries[] = { >> + &dev_attr_program_nvm_bank.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group si5341_attr_group = { >> + .name = NULL, /* put in device directory */ >> + .attrs = si5341_sysfs_entries, >> +}; > > If not nvmem framework, then this needs to be documented in > Documentation/ABI/ Okay, will do. > >> + >> static int si5341_dt_parse_dt(struct i2c_client *client, >> struct clk_si5341_output_config *config) >> { >> @@ -1544,6 +1613,10 @@ static int si5341_probe(struct i2c_client *client, >> for (i = 0; i < data->num_synth; ++i) >> devm_kfree(&client->dev, (void *)synth_clock_names[i]); >> >> + err = sysfs_create_group(&client->dev.kobj, &si5341_attr_group); >> + if (err) >> + dev_err(&client->dev, "failed to create sysfs entries\n"); >> + > > Cool, I as a user would do what in this situation? The error message > seems sort of worthless. > It's not critical for the driver to be able to register this. So I could just silently ignore the error. M.
Quoting Mike Looijmans (2020-11-05 01:06:54) > On 05-11-2020 02:48, Stephen Boyd wrote: > > Quoting Mike Looijmans (2020-11-03 06:17:41) > >> Export an attribute program_nvm_bank that when read reports the current > >> bank value. To program the chip's current state into NVM, write the > >> magic value 0xC7 into this attribute. > >> > >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > >> --- > > > > Any chance this can be done through the nvmem framework? > > This part doesn't fit. The purpose is to store the current state of the clock > chip into its non-volatile storage so it boots up with that configuration the > next POR. Main use case is that some vendors initialize PLLs only in a > bootloader and thus need the clock running at boot. Or it might just be to > save on that 300ms initialization time. Can these details be put in the commit text? It would go a long way towards helping reviewers understand the motivation if it is to speed up boot and stash away clk configuration across resets. Why is userspace involved? Why can't the driver write to the non-volatile storage with shutdown or reboot driver hooks instead of implementing a bespoke sysfs ABI? > > Having said that, the clock chip does have some "scratch" areas that'd be > useful as NVMEM storage. That'd be for a separate patch. > > For this device to be NVMEM compatible, nvmem would need to have a sort of > transaction model, where you write several values and then "commit" them all > to NVM in one call. The nvmem framework wasn't intended for that I think. I suppose patches could be written to support that if desired.
diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c index e0446e66fa64..4e025a5ea2b7 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_ACTIVE_NVM_BANK 0x00E2 +#define SI5341_NVM_WRITE 0x00E3 +#define SI5341_DEVICE_READY 0x00FE #define SI5341_SOFT_RST 0x001C #define SI5341_IN_SEL 0x0021 #define SI5341_XAXB_CFG 0x090E @@ -144,6 +147,9 @@ struct clk_si5341_output_config { #define SI5341_OUT_CFG_OE BIT(1) #define SI5341_OUT_CFG_RDIV_FORCE2 BIT(2) +/* Programming NVM, magic value to write to program the NVM */ +#define SI5341_SI5341_NVM_WRITE_COOKIE 0xC7 + /* Static configuration (to be moved to firmware) */ struct si5341_reg_default { u16 address; @@ -1199,6 +1205,69 @@ static const struct regmap_config si5341_regmap_config = { .volatile_table = &si5341_regmap_volatile, }; +static ssize_t program_nvm_bank_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct i2c_client *client = to_i2c_client(dev); + struct clk_si5341 *data = i2c_get_clientdata(client); + unsigned int regval; + int ret; + + ret = regmap_read(data->regmap, SI5341_ACTIVE_NVM_BANK, ®val); + if (ret) + return ret; + + return sprintf(buf, "%#x\n", regval); +} + +static ssize_t program_nvm_bank_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t count) +{ + struct clk_si5341 *data = i2c_get_clientdata(to_i2c_client(dev)); + int ret; + unsigned int value; + unsigned int timeout; + + ret = kstrtouint(buf, 0, &value); + if (ret < 0) + return ret; + + /* Write the magic value to this attribute to program the NVM */ + if (value != SI5341_SI5341_NVM_WRITE_COOKIE) + return -EINVAL; + + ret = regmap_write(data->regmap, SI5341_NVM_WRITE, + SI5341_SI5341_NVM_WRITE_COOKIE); + if (ret) + return ret; + + /* Wait for SI5341_DEVICE_READY register to become 0x0f */ + for (timeout = 10000; timeout; --timeout) { + ret = regmap_read(data->regmap, SI5341_DEVICE_READY, &value); + if (ret) + return ret; + + if (value == 0x0f) + break; + } + + return count; +} + +static DEVICE_ATTR_RW(program_nvm_bank); + +static struct attribute *si5341_sysfs_entries[] = { + &dev_attr_program_nvm_bank.attr, + NULL, +}; + +static struct attribute_group si5341_attr_group = { + .name = NULL, /* put in device directory */ + .attrs = si5341_sysfs_entries, +}; + static int si5341_dt_parse_dt(struct i2c_client *client, struct clk_si5341_output_config *config) { @@ -1544,6 +1613,10 @@ static int si5341_probe(struct i2c_client *client, for (i = 0; i < data->num_synth; ++i) devm_kfree(&client->dev, (void *)synth_clock_names[i]); + err = sysfs_create_group(&client->dev.kobj, &si5341_attr_group); + if (err) + dev_err(&client->dev, "failed to create sysfs entries\n"); + return 0; }
Export an attribute program_nvm_bank that when read reports the current bank value. To program the chip's current state into NVM, write the magic value 0xC7 into this attribute. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- drivers/clk/clk-si5341.c | 73 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+)