Message-ID: <20240828-steadily-nearby-1fe97d4cbe97@thorsis.com>
Date: Wed, 28 Aug 2024 09:31:05 +0200
From: Alexander Dahl <ada@...rsis.com>
To: claudiu beznea <claudiu.beznea@...on.dev>
Cc: Alexander Dahl <ada@...rsis.com>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	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

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
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.

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

Powered by blists - more mailing lists