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

[claudiu beznea <claudiu.beznea@tuxon.dev>]

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