[PATCH] clk-si5341: Support NVM programming through sysfs

[PATCH] clk-si5341: Support NVM programming through sysfs

[Mike Looijmans]

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(+)

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, &regval);
+	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;
 }
 
-- 
2.17.1

Re: [PATCH] clk-si5341: Support NVM programming through sysfs

[Stephen Boyd]

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, &regval);
> +       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.

Re: [PATCH] clk-si5341: Support NVM programming through sysfs

[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 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, &regval);
>> +       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.

Re: [PATCH] clk-si5341: Support NVM programming through sysfs

[Stephen Boyd]

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.