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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b18625cf-7dd0-db96-9460-bb0ff920b5db@topic.nl>
Date:   Thu, 5 Nov 2020 10:06:54 +0100
From:   Mike Looijmans <mike.looijmans@...ic.nl>
To:     Stephen Boyd <sboyd@...nel.org>, linux-clk@...r.kernel.org
CC:     mturquette@...libre.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk-si5341: Support NVM programming through sysfs


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@...icproducts.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@...ic.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.