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

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: <717bd06f-3eba-4825-a53f-b2f9aa1c81c8@tuxon.dev>
Date: Sat, 24 Aug 2024 19:17:43 +0300
From: claudiu beznea <claudiu.beznea@...on.dev>
To: Alexander Dahl <ada@...rsis.com>,
 Nicolas Ferre <nicolas.ferre@...rochip.com>
Cc: Christian Melki <christian.melki@...ata.com>,
 linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v1 00/12] Microchip OTPC driver on SAM9X60 exposing UIDxR
 as additional nvmem device

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.

> 
> 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@...ts.infradead.org
> Cc: devicetree@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-clk@...r.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