Re: [PATCH v2 3/3] ARM: dts: stm32: Add nvmem-syscon node to TAMP to expose boot count on DHSOM

[Patrick DELAUNAY <patrick.delaunay@foss.st.com>]

Hi,

On 6/1/23 01:09, Marek Vasut wrote:
> On 5/26/23 17:28, patrick.delaunay@foss.st.com wrote:
>> Hi Marek,
>
> Hi,
>
>>> From: Marek Vasut <marex@denx.de>
>>> Sent: Wednesday, May 17, 2023 5:25 PM
>>> Subject: [PATCH v2 3/3] ARM: dts: stm32: Add nvmem-syscon node to 
>>> TAMP to
>>> expose boot count on DHSOM
>>>
>>> Add nvmem-syscon subnode to expose TAMP_BKPxR register 19 to user 
>>> space.
>>> This register contains U-Boot boot counter, by exposing it to user 
>>> space the user
>>> space can reset the boot counter.
>>>
>>> Read access example:
>>> "
>>> $ hexdump -vC /sys/bus/nvmem/devices/5c00a000.tamp\:nvmem0/nvmem
>>> 00000000  0c 00 c4 b0
>>> "
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: kernel@dh-electronics.com
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-stm32@st-md-mailman.stormreply.com
>>> ---
>>> V2: No change
>>> ---
>>>   arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi | 11 +++++++++++
>>> arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi | 11 +++++++++++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
>>> b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
>>> index 74735552f4803..b2557bb718f52 100644
>>> --- a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
>>> +++ b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
>>> @@ -537,6 +537,17 @@ &sdmmc3 {
>>>       status = "okay";
>>>   };
>>>
>>> +&tamp {
>>> +    #address-cells = <1>;
>>> +    #size-cells = <1>;
>>> +
>>> +    /* Boot counter */
>>> +    nvmem {
>>> +        compatible = "nvmem-syscon";
>>> +        reg = <0x14c 0x4>;
>>> +    };
>>> +};
>>> +
>>>   &uart4 {
>>>       pinctrl-names = "default";
>>>       pinctrl-0 = <&uart4_pins_a>;
>>> diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi
>>> b/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi
>>> index bba19f21e5277..864960387e634 100644
>>> --- a/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi
>>> +++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi
>>> @@ -269,3 +269,14 @@ &rng1 {
>>>   &rtc {
>>>       status = "okay";
>>>   };
>>> +
>>> +&tamp {
>>> +    #address-cells = <1>;
>>> +    #size-cells = <1>;
>>> +
>>> +    /* Boot counter */
>>> +    nvmem {
>>
>> According binding you need to add "@<reg>" => nvmem@14c
>>
>> And you export only TAMP_BKP19R directly in a nvmem region ?
>
> 4 bytes is more than plenty for boot counter , yes.
>
>>> +        compatible = "nvmem-syscon";
>>> +        reg = <0x14c 0x4>;
>>> +    };
>>> +};
>>
>>
>> the boot counter could be a nvem cell so you could expose  other 
>> backup registers
>>
>> For example :
>>
>> &tamp {
>>     #address-cells = <1>;
>>     #size-cells = <1>;
>>
>>     nvmem@14c  {
>>         compatible = "nvmem-syscon";
>>         reg = <0x14c 0x4>;
>>
>>         /* Data cells */
>>         boot_counter: boot-counter@14c {
>>             reg = <0x14c 0x4>;
>>         };
>>     };
>> };
>>
>> Even if you export more backup register the cell will be correctly 
>> described in DT
>> and could be accessible directly  with sysfs without managed offset 
>> in userland
>>
>> with https://lore.kernel.org/lkml/202305240724.z3McDuYM-lkp@intel.com/T/
>> Or previous serie 
>> https://lore.kernel.org/lkml/20211220064730.28806-1-zajec5@gmail.com/
>>
>>
>> for example to export all the free register:
>>
>> Reference: https://wiki.st.com/stm32mpu/wiki/STM32MP15_backup_registers
>>
>> the cell " boot-counter" will be always available for users.
>>
>> &tamp {
>>     #address-cells = <1>;
>>     #size-cells = <1>;
>>
>>     /* backup register: 10 to 21 */
>>     nvmem@0x128  {
>>         compatible = "nvmem-syscon";
>>         reg = <0x128 0x44>;
>>
>>         /* Data cells */
>>         boot_counter: boot-counter@14c {
>>             reg = <0x14c 0x4>;
>>         };
>>         boot_mode: boot-mode@150 {
>>             reg = <0x150 0x4>;
>>         };
>> ....
>>     };
>> };
>
> Sure, thanks. I'm not sure I understood the message above.


sorry if it wasn't clear


TAMP register a nvmem driver = NVRAM provider

=> it should export ALL the free backup registers

       as they can used by application / kernel for many purpose....

       and not only for boot counterfor you use-case


So limit the exported backup register to this 4 bytes is strange for me.


and COUNTER is a nvem cell =  a part of backup register = TAMP_BKP19R

=> I agree 4 byte for this count is fine for this cell


NB: today we have no means to read only one nvmem cell with sysfs API

        but I see this feature is proposed to have something as

/sys/bus/nvmem/devices/nvmem@0x128/ => all the backup registers

/sys/bus/nvmem/devices/nvmem@0x128/cells/boot-mode => only the nvmem 
cell used as counter I think it is more safe for long term support to 
manage your counter as a nvmem cell. regards Patrick