Re: [PATCH v1 00/12] Microchip OTPC driver on SAM9X60 exposing UIDxR as additional nvmem device

claudiu beznea
2024-08-31T15:38:10Z

Hi, Alexander,

On 28.08.2024 10:31, Alexander Dahl wrote:
> Hello Claudiu,
>
> thanks for having a closer look on the series. The issues the bots
> complained about are already fixed in my working copy and will be part
> of v2. Detailed discussion on particular patches itself over there,
> general remarks below.
>
> Am Sat, Aug 24, 2024 at 07:17:43PM +0300 schrieb claudiu beznea:
>> Hi, Alexander,
>>
>> On 21.08.2024 13:59, Alexander Dahl wrote:
>>> Hei hei,
>>>
>>> on a custom sam9x60 based board we want to access a unique ID of the
>>> SoC. Microchip sam-ba has a command 'readuniqueid' which returns the
>>> content of the OTPC Product UID x Register in that case.
>>>
>>> (On different boards with a SAMA5D2 we use the Serial Number x Register
>>> exposed through the atmel soc driver. Those registers are not present
>>> in the SAM9X60 series, but only for SAMA5D2/SAMA5D4 AFAIK.)
>>
>> Not sure if you are talking about Chip ID, Chip ID extension registers.
>> These are available also on SAM9X60.
>
> No, this is not what I'm talking about. The Chip ID and Chip ID
> extension registers are common over all SoCs of the same type.
>
> What I need is a unique ID, the same sam-ba returns with the
> "readuniqueid" applet. The SAMA5D2 has this in SFR_SN0 and SFR_SN1,
> handled by drivers/soc/atmel/sfr.c driver. The SFR block on sam9x60

I see, I missed this one.

> has no SNx registers, the unique ID comes from OTPC_UIDxR here.
>
> Best thing would be a simple nvmem device for the SAM9X60 providing
> just those 4 registers, in a similar way the sfr driver does for
> SAMA5D2. This is the motivation for the series and what's eventually
> done in patch 12. The other patches are just fixing the otpc driver
> for SAM9X60 so I can add that nvmem stuff.

Got it, thanks for details.

Thank you,
Claudiu Beznea

>
> Greets
> Alex
>
>>> There is a driver for the OTPC of the SAMA7G5 and after comparing
>>> register layouts it seems that one is almost identical to the one used
>>> by SAM9X60. Currently that driver has no support for the UIDx
>>> registers, but I suppose it would be the right place to implement it,
>>> because the registers are within the OTPC register address offsets.
>>>
>>> The patch series starts with fixups for the current driver. It then
>>> adds the necessary pieces to DT and driver to work on SAM9X60 in
>>> general. Later support for enabling the main RC oscillator is added,
>>> which is required on SAM9X60 for the OTPC to work. The last patch adds
>>> an additional nvmem device for the UIDx registers.
>>>
>>> This v1 of the series was _not_ tested on SAMA7G5, because I don't have
>>> such a board for testing. Actually I don't know if the main_rc_osc
>>> clock is required on SAMA7G5 too, and if yes how to handle that with
>>> regard to the different clock ids. If someone could test on SAMA7G5
>>> and/or help me sorting out the core clock id things, that would be
>>> highly appreciated.
>>
>> Please add Nicolas in the loop on the next revisions of this series as this
>> should also be tested on SAMA7G5. I don't have a SAMA7G5 with OTP memory
>> populated.
>>
>>>
>>> Also I assume some more devicetree and/or sysfs documentation is
>>> necessary. If someone could point me what's exactly required, this
>>> would be very helpful for me. You see I expect at least another version
>>> v2 of the series. ;-)
>>>
>>> Maybe some files having that "sama7g5" should be renamed, because that
>>> DT binding is used for more SoCs now and deserves a more generic name?
>>
>> Not needed, adding your compatible there is enough.
>>
>>> Thinking of these for example:
>>>
>>> - Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
>>> - include/dt-bindings/nvmem/microchip,sama7g5-otpc.h
>>>
>>> Are there other SoCs than SAMA7G5 and SAM9X60 using the same OTPC?
>>>
>>> Last question: Should the UID be added to the device entropy pool with
>>> add_device_randomness() as done in the SAMA5D2 sfr driver?
>>>
>>> I sent an RFC patch on this topic earlier this year, you'll find the
>>> link below as a reference to the discussion. The patch itself was
>>> trivial and not meant for applying as is anyways, so I decided to not
>>> write a full changelog from RFC to v1.
>>>
>>> Last not least, special thanks to Christian Melki on IRC, who wrote and
>>> tested parts of this, and was very kind and helpful in discussing the
>>> topic several times in the past months.
>>>
>>> Christian, if you feel there's credit missing, just point me where to
>>> add Co-developed-by and I'll happily do that for v2.
>>>
>>> Greets
>>> Alex
>>>
>>> (series based on v6.11-rc4)
>>>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: devicetree@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: linux-clk@vger.kernel.org
>>> Link: https://lore.kernel.org/all/20240412140802.1571935-2-ada@thorsis.com/
>>>
>>> Alexander Dahl (12):
>>> nvmem: microchip-otpc: Avoid writing a write-only register
>>> nvmem: microchip-otpc: Fix swapped 'sleep' and 'timeout' parameters
>>> dt-bindings: nvmem: microchip-otpc: Add compatible for SAM9X60
>>> nvmem: microchip-otpc: Add SAM9X60 support
>>> ARM: dts: microchip: sam9x60: Add OTPC node
>>> ARM: dts: microchip: sam9x60_curiosity: Enable OTP Controller
>>> nvmem: microchip-otpc: Add missing register definitions
>>> nvmem: microchip-otpc: Add warnings for bad OTPC conditions on probe
>>> clk: at91: sam9x60: Allow enabling main_rc_osc through DT
>>> ARM: dts: microchip: sam9x60: Add clock properties to OTPC
>>> nvmem: microchip-otpc: Enable main RC oscillator clock
>>> nvmem: microchip-otpc: Expose UID registers as 2nd nvmem device
>>>
>>> .../nvmem/microchip,sama7g5-otpc.yaml | 1 +
>>> .../dts/microchip/at91-sam9x60_curiosity.dts | 4 +
>>> arch/arm/boot/dts/microchip/sam9x60.dtsi | 10 +++
>>> drivers/clk/at91/sam9x60.c | 3 +-
>>> drivers/nvmem/microchip-otpc.c | 86 ++++++++++++++++++-
>>> include/dt-bindings/clock/at91.h | 1 +
>>> 6 files changed, 100 insertions(+), 5 deletions(-)
>>>
>>>
>>> base-commit: 47ac09b91befbb6a235ab620c32af719f8208399