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

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

[Alexander Dahl]

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

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.

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?
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
-- 
2.39.2

[PATCH v1 01/12] nvmem: microchip-otpc: Avoid writing a write-only register

[Alexander Dahl]

The OTPC Control Register (OTPC_CR) has just write-only members.
Reading from that register leads to a warning in OTPC Write Protection
Status Register (OTPC_WPSR) in field Software Error Type (SWETYP) of
type READ_WO (A write-only register has been read (warning).)

Just create the register write content from scratch is sufficient here.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/nvmem/microchip-otpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
index 7cf81738a3e0..03e60b99f2c9 100644
--- a/drivers/nvmem/microchip-otpc.c
+++ b/drivers/nvmem/microchip-otpc.c
@@ -82,7 +82,7 @@ static int mchp_otpc_prepare_read(struct mchp_otpc *otpc,
 	writel_relaxed(tmp, otpc->base + MCHP_OTPC_MR);
 
 	/* Set read. */
-	tmp = readl_relaxed(otpc->base + MCHP_OTPC_CR);
+	tmp = 0;
 	tmp |= MCHP_OTPC_CR_READ;
 	writel_relaxed(tmp, otpc->base + MCHP_OTPC_CR);
 
-- 
2.39.2

[PATCH v1 02/12] nvmem: microchip-otpc: Fix swapped 'sleep' and 'timeout' parameters

[Alexander Dahl]

Makes no sense to have a timeout shorter than the sleep time, it would
run into timeout right after the first sleep already.
While at it, use a more specific macro instead of the generic one, which
does exactly the same, but needs less parameters.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/nvmem/microchip-otpc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
index 03e60b99f2c9..bd3383eabdf6 100644
--- a/drivers/nvmem/microchip-otpc.c
+++ b/drivers/nvmem/microchip-otpc.c
@@ -87,8 +87,8 @@ static int mchp_otpc_prepare_read(struct mchp_otpc *otpc,
 	writel_relaxed(tmp, otpc->base + MCHP_OTPC_CR);
 
 	/* Wait for packet to be transferred into temporary buffers. */
-	return read_poll_timeout(readl_relaxed, tmp, !(tmp & MCHP_OTPC_SR_READ),
-				 10000, 2000, false, otpc->base + MCHP_OTPC_SR);
+	return readl_relaxed_poll_timeout(otpc->base + MCHP_OTPC_SR, tmp,
+					  !(tmp & MCHP_OTPC_SR_READ), 2000, 10000);
 }
 
 /*
-- 
2.39.2

[PATCH v1 03/12] dt-bindings: nvmem: microchip-otpc: Add compatible for SAM9X60

[Alexander Dahl]

The SAM9X60 SoC family has a similar OTPC to the SAMA7G5 family.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 .../devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml        | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
index cc25f2927682..d98b6711bdfd 100644
--- a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
+++ b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
@@ -21,6 +21,7 @@ allOf:
 properties:
   compatible:
     items:
+      - const: microchip,sam9x60-otpc
       - const: microchip,sama7g5-otpc
       - const: syscon
 
-- 
2.39.2

[PATCH v1 04/12] nvmem: microchip-otpc: Add SAM9X60 support

[Alexander Dahl]

Register layout is almost identical to sama7g5 OTPC.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/nvmem/microchip-otpc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
index bd3383eabdf6..b8ed7412dbca 100644
--- a/drivers/nvmem/microchip-otpc.c
+++ b/drivers/nvmem/microchip-otpc.c
@@ -271,6 +271,7 @@ static int mchp_otpc_probe(struct platform_device *pdev)
 
 static const struct of_device_id __maybe_unused mchp_otpc_ids[] = {
 	{ .compatible = "microchip,sama7g5-otpc", },
+	{ .compatible = "microchip,sam9x60-otpc", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, mchp_otpc_ids);
-- 
2.39.2

[PATCH v1 06/12] ARM: dts: microchip: sam9x60_curiosity: Enable OTP Controller

[Alexander Dahl]

Allows to access the OTP memory now, and Product UID later.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 arch/arm/boot/dts/microchip/at91-sam9x60_curiosity.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/microchip/at91-sam9x60_curiosity.dts b/arch/arm/boot/dts/microchip/at91-sam9x60_curiosity.dts
index c6fbdd29019f..754ce8134f73 100644
--- a/arch/arm/boot/dts/microchip/at91-sam9x60_curiosity.dts
+++ b/arch/arm/boot/dts/microchip/at91-sam9x60_curiosity.dts
@@ -254,6 +254,10 @@ ethernet-phy@0 {
 	};
 };
 
+&otpc {
+	status = "okay";
+};
+
 &pinctrl {
 	adc {
 		pinctrl_adc_default: adc-default {
-- 
2.39.2

[PATCH v1 07/12] nvmem: microchip-otpc: Add missing register definitions

[Alexander Dahl]

According to datasheets DS60001765B for SAMA7G5 and DS60001579G for
SAM9X60.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/nvmem/microchip-otpc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
index b8ed7412dbca..4630e96243ac 100644
--- a/drivers/nvmem/microchip-otpc.c
+++ b/drivers/nvmem/microchip-otpc.c
@@ -21,9 +21,24 @@
 #define MCHP_OTPC_AR			(0x8)
 #define MCHP_OTPC_SR			(0xc)
 #define MCHP_OTPC_SR_READ		BIT(6)
+#define MCHP_OTPC_IER			(0x10)
+#define MCHP_OTPC_IDR			(0x14)
+#define MCHP_OTPC_IMR			(0x18)
+#define MCHP_OTPC_ISR			(0x1C)
+#define MCHP_OTPC_ISR_COERR		BIT(13)
 #define MCHP_OTPC_HR			(0x20)
 #define MCHP_OTPC_HR_SIZE		GENMASK(15, 8)
 #define MCHP_OTPC_DR			(0x24)
+#define MCHP_OTPC_BAR			(0x30)
+#define MCHP_OTPC_CAR			(0x34)
+#define MCHP_OTPC_UHC0R			(0x50)
+#define MCHP_OTPC_UHC1R			(0x54)
+#define MCHP_OTPC_UID0R			(0x60)
+#define MCHP_OTPC_UID1R			(0x64)
+#define MCHP_OTPC_UID2R			(0x68)
+#define MCHP_OTPC_UID3R			(0x6C)
+#define MCHP_OTPC_WPMR			(0xE4)
+#define MCHP_OTPC_WPSR			(0xE8)
 
 #define MCHP_OTPC_NAME			"mchp-otpc"
 #define MCHP_OTPC_SIZE			(11 * 1024)
-- 
2.39.2

[PATCH v1 08/12] nvmem: microchip-otpc: Add warnings for bad OTPC conditions on probe

[Alexander Dahl]

These conditions could affect correct function of the driver.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/nvmem/microchip-otpc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
index 4630e96243ac..a80535c3d162 100644
--- a/drivers/nvmem/microchip-otpc.c
+++ b/drivers/nvmem/microchip-otpc.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/dev_printk.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/nvmem-provider.h>
@@ -260,6 +261,7 @@ static int mchp_otpc_probe(struct platform_device *pdev)
 	struct nvmem_device *nvmem;
 	struct mchp_otpc *otpc;
 	u32 size;
+	u32 reg;
 	int ret;
 
 	otpc = devm_kzalloc(&pdev->dev, sizeof(*otpc), GFP_KERNEL);
@@ -270,6 +272,16 @@ static int mchp_otpc_probe(struct platform_device *pdev)
 	if (IS_ERR(otpc->base))
 		return PTR_ERR(otpc->base);
 
+	reg = readl_relaxed(otpc->base + MCHP_OTPC_WPSR);
+	if (reg)
+		dev_warn(&pdev->dev,
+			 "Write Protection Status Register Bit set: 0x%08x\n", reg);
+
+	reg = readl_relaxed(otpc->base + MCHP_OTPC_ISR);
+	if (reg & MCHP_OTPC_ISR_COERR)
+		dev_warn(&pdev->dev,
+			 "A corruption occurred since the last read of OTPC_ISR.\n");
+
 	otpc->dev = &pdev->dev;
 	ret = mchp_otpc_init_packets_list(otpc, &size);
 	if (ret)
-- 
2.39.2

[PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[Alexander Dahl]

SAM9X60 Datasheet (DS60001579G) Section "23.4 Product Dependencies"
says:

    "The OTPC is clocked through the Power Management Controller (PMC).
    The user must power on the main RC oscillator and enable the
    peripheral clock of the OTPC prior to reading or writing the OTP
    memory."

The code for enabling/disabling that clock is already present, it was
just not possible to hook into DT anymore, after at91 clk devicetree
binding rework back in 2018 for kernel v4.19.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/clk/at91/sam9x60.c       | 3 ++-
 include/dt-bindings/clock/at91.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/at91/sam9x60.c b/drivers/clk/at91/sam9x60.c
index e309cbf3cb9a..4d5ee20b8fc4 100644
--- a/drivers/clk/at91/sam9x60.c
+++ b/drivers/clk/at91/sam9x60.c
@@ -207,7 +207,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
 	if (IS_ERR(regmap))
 		return;
 
-	sam9x60_pmc = pmc_data_allocate(PMC_PLLACK + 1,
+	sam9x60_pmc = pmc_data_allocate(PMC_MAIN_RC + 1,
 					nck(sam9x60_systemck),
 					nck(sam9x60_periphck),
 					nck(sam9x60_gck), 8);
@@ -218,6 +218,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
 					   50000000);
 	if (IS_ERR(hw))
 		goto err_free;
+	sam9x60_pmc->chws[PMC_MAIN_RC] = hw;
 
 	hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
 	if (IS_ERR(hw))
diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
index 3e3972a814c1..f957625cb3ac 100644
--- a/include/dt-bindings/clock/at91.h
+++ b/include/dt-bindings/clock/at91.h
@@ -25,6 +25,7 @@
 #define PMC_PLLBCK		8
 #define PMC_AUDIOPLLCK		9
 #define PMC_AUDIOPINCK		10
+#define PMC_MAIN_RC		11
 
 /* SAMA7G5 */
 #define PMC_CPUPLL		(PMC_MAIN + 1)
-- 
2.39.2

[PATCH v1 11/12] nvmem: microchip-otpc: Enable main RC oscillator clock

[Alexander Dahl]

Without enabling that clock, initializing the packet list leads to a
read timeout on the first packet.

According to SAM9X60 datasheet (DS60001579G) section "23.4 Product
Dependencies" the clock must be enabled for reading and writing.

Tested on sam9x60-curiosity board.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/nvmem/microchip-otpc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
index a80535c3d162..047ca5ac6407 100644
--- a/drivers/nvmem/microchip-otpc.c
+++ b/drivers/nvmem/microchip-otpc.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/clk.h>
 #include <linux/dev_printk.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
@@ -54,6 +55,7 @@
 struct mchp_otpc {
 	void __iomem *base;
 	struct device *dev;
+	struct clk *clk;
 	struct list_head packets;
 	u32 npackets;
 };
@@ -272,6 +274,15 @@ static int mchp_otpc_probe(struct platform_device *pdev)
 	if (IS_ERR(otpc->base))
 		return PTR_ERR(otpc->base);
 
+	// NOTE: Maybe make this optional, especially if sama7g5 testing
+	// shows the clock is not required there?
+	otpc->clk = devm_clk_get_enabled(&pdev->dev, "main_rc_osc");
+	if (IS_ERR(otpc->clk)) {
+		dev_err(&pdev->dev, "Error (%ld) getting clock!\n",
+			PTR_ERR(otpc->clk));
+		return PTR_ERR(otpc->clk);
+	}
+
 	reg = readl_relaxed(otpc->base + MCHP_OTPC_WPSR);
 	if (reg)
 		dev_warn(&pdev->dev,
-- 
2.39.2

[PATCH v1 12/12] nvmem: microchip-otpc: Expose UID registers as 2nd nvmem device

[Alexander Dahl]

For SAM9X60 the Product UID x Register containing the Unique Product ID
is part of the OTPC registers.  We have everything at hand here to just
create a trivial nvmem device for those.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/nvmem/microchip-otpc.c | 41 +++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
index 047ca5ac6407..52af4c137204 100644
--- a/drivers/nvmem/microchip-otpc.c
+++ b/drivers/nvmem/microchip-otpc.c
@@ -45,6 +45,9 @@
 #define MCHP_OTPC_NAME			"mchp-otpc"
 #define MCHP_OTPC_SIZE			(11 * 1024)
 
+#define MCHP_OTPC_UID_NAME		"mchp-uid"
+#define MCHP_OTPC_UID_SIZE		16
+
 /**
  * struct mchp_otpc - OTPC private data structure
  * @base: base address
@@ -249,6 +252,16 @@ static int mchp_otpc_init_packets_list(struct mchp_otpc *otpc, u32 *size)
 	return 0;
 }
 
+static int mchp_otpc_uid_read(void *priv, unsigned int offset,
+			      void *val, size_t bytes)
+{
+	struct mchp_otpc *otpc = priv;
+
+	memcpy_fromio(val, otpc->base + MCHP_OTPC_UID0R + offset, bytes);
+
+	return 0;
+}
+
 static struct nvmem_config mchp_nvmem_config = {
 	.name = MCHP_OTPC_NAME,
 	.type = NVMEM_TYPE_OTP,
@@ -258,6 +271,15 @@ static struct nvmem_config mchp_nvmem_config = {
 	.reg_read = mchp_otpc_read,
 };
 
+static struct nvmem_config mchp_otpc_uid_nvmem_config = {
+	.name = MCHP_OTPC_UID_NAME,
+	.read_only = true,
+	.word_size = 4,
+	.stride = 4,
+	.size = MCHP_OTPC_UID_SIZE,
+	.reg_read = mchp_otpc_uid_read,
+};
+
 static int mchp_otpc_probe(struct platform_device *pdev)
 {
 	struct nvmem_device *nvmem;
@@ -303,8 +325,25 @@ static int mchp_otpc_probe(struct platform_device *pdev)
 	mchp_nvmem_config.size = size;
 	mchp_nvmem_config.priv = otpc;
 	nvmem = devm_nvmem_register(&pdev->dev, &mchp_nvmem_config);
+	if (IS_ERR(nvmem)) {
+		dev_err(&pdev->dev,
+			"Error (%ld) registering OTP as nvmem device\n",
+			PTR_ERR(nvmem));
+		return PTR_ERR(nvmem);
+	}
 
-	return PTR_ERR_OR_ZERO(nvmem);
+	mchp_otpc_uid_nvmem_config.dev = otpc->dev;
+	mchp_otpc_uid_nvmem_config.priv = otpc;
+
+	nvmem = devm_nvmem_register(&pdev->dev, &mchp_otpc_uid_nvmem_config);
+	if (IS_ERR(nvmem)) {
+		dev_err(&pdev->dev,
+			"Error (%ld) registering UIDxR as nvmem device\n",
+			PTR_ERR(nvmem));
+		return PTR_ERR(nvmem);
+	}
+
+	return 0;
 }
 
 static const struct of_device_id __maybe_unused mchp_otpc_ids[] = {
-- 
2.39.2

Re: [PATCH v1 03/12] dt-bindings: nvmem: microchip-otpc: Add compatible for SAM9X60

[Rob Herring (Arm)]

On Wed, 21 Aug 2024 12:59:34 +0200, Alexander Dahl wrote:
> The SAM9X60 SoC family has a similar OTPC to the SAMA7G5 family.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  .../devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml        | 1 +
>  1 file changed, 1 insertion(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.example.dtb: efuse@e8c00000: compatible:0: 'microchip,sam9x60-otpc' was expected
	from schema $id: http://devicetree.org/schemas/nvmem/microchip,sama7g5-otpc.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.example.dtb: efuse@e8c00000: compatible:1: 'microchip,sama7g5-otpc' was expected
	from schema $id: http://devicetree.org/schemas/nvmem/microchip,sama7g5-otpc.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.example.dtb: efuse@e8c00000: compatible: ['microchip,sama7g5-otpc', 'syscon'] is too short
	from schema $id: http://devicetree.org/schemas/nvmem/microchip,sama7g5-otpc.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.example.dtb: efuse@e8c00000: Unevaluated properties are not allowed ('compatible' was unexpected)
	from schema $id: http://devicetree.org/schemas/nvmem/microchip,sama7g5-otpc.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240821105943.230281-4-ada@thorsis.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 03/12] dt-bindings: nvmem: microchip-otpc: Add compatible for SAM9X60

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

On Wed, Aug 21, 2024 at 12:59:34PM +0200, Alexander Dahl wrote:
> The SAM9X60 SoC family has a similar OTPC to the SAMA7G5 family.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  .../devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml        | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
> index cc25f2927682..d98b6711bdfd 100644
> --- a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
> @@ -21,6 +21,7 @@ allOf:
>  properties:
>    compatible:
>      items:
> +      - const: microchip,sam9x60-otpc
>        - const: microchip,sama7g5-otpc
>        - const: syscon

As Rob's bot pointed out, this breaks the existing devicetrees. If you
want a fallback to the sama7g5, then you will need to add a new items
list here, alongside the existing one.

Cheers,
Conor.

>  
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2307 bytes --]

On Wed, Aug 21, 2024 at 12:59:40PM +0200, Alexander Dahl wrote:
> SAM9X60 Datasheet (DS60001579G) Section "23.4 Product Dependencies"
> says:
> 
>     "The OTPC is clocked through the Power Management Controller (PMC).
>     The user must power on the main RC oscillator and enable the
>     peripheral clock of the OTPC prior to reading or writing the OTP
>     memory."
> 
> The code for enabling/disabling that clock is already present, it was
> just not possible to hook into DT anymore, after at91 clk devicetree
> binding rework back in 2018 for kernel v4.19.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  drivers/clk/at91/sam9x60.c       | 3 ++-
>  include/dt-bindings/clock/at91.h | 1 +

Generally we don't want binding changes in the same patch as a driver
change. If your fix was determined to be faulty down the line and
reverted, the binding change would remain valid, for example. Can you
split it into two patches please, for the next version please?

>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/at91/sam9x60.c b/drivers/clk/at91/sam9x60.c
> index e309cbf3cb9a..4d5ee20b8fc4 100644
> --- a/drivers/clk/at91/sam9x60.c
> +++ b/drivers/clk/at91/sam9x60.c
> @@ -207,7 +207,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>  	if (IS_ERR(regmap))
>  		return;
>  
> -	sam9x60_pmc = pmc_data_allocate(PMC_PLLACK + 1,
> +	sam9x60_pmc = pmc_data_allocate(PMC_MAIN_RC + 1,
>  					nck(sam9x60_systemck),
>  					nck(sam9x60_periphck),
>  					nck(sam9x60_gck), 8);
> @@ -218,6 +218,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>  					   50000000);
>  	if (IS_ERR(hw))
>  		goto err_free;
> +	sam9x60_pmc->chws[PMC_MAIN_RC] = hw;
>  
>  	hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
>  	if (IS_ERR(hw))
> diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
> index 3e3972a814c1..f957625cb3ac 100644
> --- a/include/dt-bindings/clock/at91.h
> +++ b/include/dt-bindings/clock/at91.h
> @@ -25,6 +25,7 @@
>  #define PMC_PLLBCK		8
>  #define PMC_AUDIOPLLCK		9
>  #define PMC_AUDIOPINCK		10
> +#define PMC_MAIN_RC		11
>  
>  /* SAMA7G5 */
>  #define PMC_CPUPLL		(PMC_MAIN + 1)
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 11/12] nvmem: microchip-otpc: Enable main RC oscillator clock

[kernel test robot]

Hi Alexander,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 47ac09b91befbb6a235ab620c32af719f8208399]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexander-Dahl/nvmem-microchip-otpc-Avoid-writing-a-write-only-register/20240821-193942
base:   47ac09b91befbb6a235ab620c32af719f8208399
patch link:    https://lore.kernel.org/r/20240821105943.230281-12-ada%40thorsis.com
patch subject: [PATCH v1 11/12] nvmem: microchip-otpc: Enable main RC oscillator clock
config: x86_64-buildonly-randconfig-001-20240822 (https://download.01.org/0day-ci/archive/20240822/202408222300.BEv0hBO5-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408222300.BEv0hBO5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408222300.BEv0hBO5-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/nvmem/microchip-otpc.c:61: warning: Function parameter or struct member 'clk' not described in 'mchp_otpc'


vim +61 drivers/nvmem/microchip-otpc.c

98830350d3fc82 Claudiu Beznea 2022-07-06  47  
98830350d3fc82 Claudiu Beznea 2022-07-06  48  /**
98830350d3fc82 Claudiu Beznea 2022-07-06  49   * struct mchp_otpc - OTPC private data structure
98830350d3fc82 Claudiu Beznea 2022-07-06  50   * @base: base address
98830350d3fc82 Claudiu Beznea 2022-07-06  51   * @dev: struct device pointer
98830350d3fc82 Claudiu Beznea 2022-07-06  52   * @packets: list of packets in OTP memory
98830350d3fc82 Claudiu Beznea 2022-07-06  53   * @npackets: number of packets in OTP memory
98830350d3fc82 Claudiu Beznea 2022-07-06  54   */
98830350d3fc82 Claudiu Beznea 2022-07-06  55  struct mchp_otpc {
98830350d3fc82 Claudiu Beznea 2022-07-06  56  	void __iomem *base;
98830350d3fc82 Claudiu Beznea 2022-07-06  57  	struct device *dev;
1acc431703527a Alexander Dahl 2024-08-21  58  	struct clk *clk;
98830350d3fc82 Claudiu Beznea 2022-07-06  59  	struct list_head packets;
98830350d3fc82 Claudiu Beznea 2022-07-06  60  	u32 npackets;
98830350d3fc82 Claudiu Beznea 2022-07-06 @61  };
98830350d3fc82 Claudiu Beznea 2022-07-06  62  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 12/12] nvmem: microchip-otpc: Expose UID registers as 2nd nvmem device

[claudiu beznea]

On 21.08.2024 13:59, Alexander Dahl wrote:
> For SAM9X60 the Product UID x Register containing the Unique Product ID
> is part of the OTPC registers.  We have everything at hand here to just
> create a trivial nvmem device for those.

I'm not sure what is the best option to expose this. I let it to NVMEM
maintainers.

> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  drivers/nvmem/microchip-otpc.c | 41 +++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index 047ca5ac6407..52af4c137204 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -45,6 +45,9 @@
>  #define MCHP_OTPC_NAME			"mchp-otpc"
>  #define MCHP_OTPC_SIZE			(11 * 1024)
>  
> +#define MCHP_OTPC_UID_NAME		"mchp-uid"
> +#define MCHP_OTPC_UID_SIZE		16
> +
>  /**
>   * struct mchp_otpc - OTPC private data structure
>   * @base: base address
> @@ -249,6 +252,16 @@ static int mchp_otpc_init_packets_list(struct mchp_otpc *otpc, u32 *size)
>  	return 0;
>  }
>  
> +static int mchp_otpc_uid_read(void *priv, unsigned int offset,
> +			      void *val, size_t bytes)
> +{
> +	struct mchp_otpc *otpc = priv;
> +
> +	memcpy_fromio(val, otpc->base + MCHP_OTPC_UID0R + offset, bytes);
> +
> +	return 0;
> +}
> +
>  static struct nvmem_config mchp_nvmem_config = {
>  	.name = MCHP_OTPC_NAME,
>  	.type = NVMEM_TYPE_OTP,
> @@ -258,6 +271,15 @@ static struct nvmem_config mchp_nvmem_config = {
>  	.reg_read = mchp_otpc_read,
>  };
>  
> +static struct nvmem_config mchp_otpc_uid_nvmem_config = {
> +	.name = MCHP_OTPC_UID_NAME,
> +	.read_only = true,
> +	.word_size = 4,
> +	.stride = 4,
> +	.size = MCHP_OTPC_UID_SIZE,
> +	.reg_read = mchp_otpc_uid_read,
> +};
> +
>  static int mchp_otpc_probe(struct platform_device *pdev)
>  {
>  	struct nvmem_device *nvmem;
> @@ -303,8 +325,25 @@ static int mchp_otpc_probe(struct platform_device *pdev)
>  	mchp_nvmem_config.size = size;
>  	mchp_nvmem_config.priv = otpc;
>  	nvmem = devm_nvmem_register(&pdev->dev, &mchp_nvmem_config);
> +	if (IS_ERR(nvmem)) {
> +		dev_err(&pdev->dev,
> +			"Error (%ld) registering OTP as nvmem device\n",
> +			PTR_ERR(nvmem));
> +		return PTR_ERR(nvmem);

return dev_err_probe();

> +	}
>  
> -	return PTR_ERR_OR_ZERO(nvmem);
> +	mchp_otpc_uid_nvmem_config.dev = otpc->dev;
> +	mchp_otpc_uid_nvmem_config.priv = otpc;
> +
> +	nvmem = devm_nvmem_register(&pdev->dev, &mchp_otpc_uid_nvmem_config);
> +	if (IS_ERR(nvmem)) {
> +		dev_err(&pdev->dev,
> +			"Error (%ld) registering UIDxR as nvmem device\n",
> +			PTR_ERR(nvmem));
> +		return PTR_ERR(nvmem);

return dev_err_probe();

> +	}
> +
> +	return 0;
>  }
>  
>  static const struct of_device_id __maybe_unused mchp_otpc_ids[] = {

Re: [PATCH v1 08/12] nvmem: microchip-otpc: Add warnings for bad OTPC conditions on probe

[claudiu beznea]

On 21.08.2024 13:59, Alexander Dahl wrote:
> These conditions could affect correct function of the driver.

Can you please detail why this has been added, what was the issue you
identified etc?

> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  drivers/nvmem/microchip-otpc.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index 4630e96243ac..a80535c3d162 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/dev_printk.h>
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/nvmem-provider.h>
> @@ -260,6 +261,7 @@ static int mchp_otpc_probe(struct platform_device *pdev)
>  	struct nvmem_device *nvmem;
>  	struct mchp_otpc *otpc;
>  	u32 size;
> +	u32 reg;
>  	int ret;
>  
>  	otpc = devm_kzalloc(&pdev->dev, sizeof(*otpc), GFP_KERNEL);
> @@ -270,6 +272,16 @@ static int mchp_otpc_probe(struct platform_device *pdev)
>  	if (IS_ERR(otpc->base))
>  		return PTR_ERR(otpc->base);
>  
> +	reg = readl_relaxed(otpc->base + MCHP_OTPC_WPSR);
> +	if (reg)
> +		dev_warn(&pdev->dev,
> +			 "Write Protection Status Register Bit set: 0x%08x\n", reg);

There are many status bits in WPSR. Some of these bits suggests warnings
even when their values are zero (e.g., ECLASS, SWETYP). I don't know how
accurate is this message.


> +
> +	reg = readl_relaxed(otpc->base + MCHP_OTPC_ISR);
> +	if (reg & MCHP_OTPC_ISR_COERR)
> +		dev_warn(&pdev->dev,
> +			 "A corruption occurred since the last read of OTPC_ISR.\n");
> +
>  	otpc->dev = &pdev->dev;
>  	ret = mchp_otpc_init_packets_list(otpc, &size);
>  	if (ret)

Re: [PATCH v1 01/12] nvmem: microchip-otpc: Avoid writing a write-only register

[claudiu beznea]

Hi, ALexander,

On 21.08.2024 13:59, Alexander Dahl wrote:
> The OTPC Control Register (OTPC_CR) has just write-only members.
> Reading from that register leads to a warning in OTPC Write Protection
> Status Register (OTPC_WPSR) in field Software Error Type (SWETYP) of
> type READ_WO (A write-only register has been read (warning).)
> 
> Just create the register write content from scratch is sufficient here.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>

Maybe worth a Fixes tag here.

> ---
>  drivers/nvmem/microchip-otpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index 7cf81738a3e0..03e60b99f2c9 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -82,7 +82,7 @@ static int mchp_otpc_prepare_read(struct mchp_otpc *otpc,
>  	writel_relaxed(tmp, otpc->base + MCHP_OTPC_MR);
>  
>  	/* Set read. */
> -	tmp = readl_relaxed(otpc->base + MCHP_OTPC_CR);
> +	tmp = 0;
>  	tmp |= MCHP_OTPC_CR_READ;
>  	writel_relaxed(tmp, otpc->base + MCHP_OTPC_CR);

	writel_relaxed(MCHP_OTPC_CR_READ, otpc->base + MCHP_OTPC_CR);

should be enough here.

Thank you,
Claudiu Beznea

>  

Re: [PATCH v1 02/12] nvmem: microchip-otpc: Fix swapped 'sleep' and 'timeout' parameters

[claudiu beznea]

On 21.08.2024 13:59, Alexander Dahl wrote:
> Makes no sense to have a timeout shorter than the sleep time, it would
> run into timeout right after the first sleep already.
> While at it, use a more specific macro instead of the generic one, which
> does exactly the same, but needs less parameters.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>

Please add a Fixes tag.

> ---
>  drivers/nvmem/microchip-otpc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index 03e60b99f2c9..bd3383eabdf6 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -87,8 +87,8 @@ static int mchp_otpc_prepare_read(struct mchp_otpc *otpc,
>  	writel_relaxed(tmp, otpc->base + MCHP_OTPC_CR);
>  
>  	/* Wait for packet to be transferred into temporary buffers. */
> -	return read_poll_timeout(readl_relaxed, tmp, !(tmp & MCHP_OTPC_SR_READ),
> -				 10000, 2000, false, otpc->base + MCHP_OTPC_SR);
> +	return readl_relaxed_poll_timeout(otpc->base + MCHP_OTPC_SR, tmp,
> +					  !(tmp & MCHP_OTPC_SR_READ), 2000, 10000);
>  }
>  
>  /*

Re: [PATCH v1 11/12] nvmem: microchip-otpc: Enable main RC oscillator clock

[claudiu beznea]

On 21.08.2024 13:59, Alexander Dahl wrote:
> Without enabling that clock, initializing the packet list leads to a
> read timeout on the first packet.
> 
> According to SAM9X60 datasheet (DS60001579G) section "23.4 Product
> Dependencies" the clock must be enabled for reading and writing.
> 
> Tested on sam9x60-curiosity board.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  drivers/nvmem/microchip-otpc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index a80535c3d162..047ca5ac6407 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/clk.h>
>  #include <linux/dev_printk.h>
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
> @@ -54,6 +55,7 @@
>  struct mchp_otpc {
>  	void __iomem *base;
>  	struct device *dev;
> +	struct clk *clk;
>  	struct list_head packets;
>  	u32 npackets;
>  };
> @@ -272,6 +274,15 @@ static int mchp_otpc_probe(struct platform_device *pdev)
>  	if (IS_ERR(otpc->base))
>  		return PTR_ERR(otpc->base);
>  
> +	// NOTE: Maybe make this optional, especially if sama7g5 testing

Looking though DS, on SAMA7G5 the clock here should be MCK0. I think it
should be added for SAMA7G5, too, with a fixes tag.

> +	// shows the clock is not required there?

Use C style comments /* comment */

> +	otpc->clk = devm_clk_get_enabled(&pdev->dev, "main_rc_osc");

Maybe name it "bus-clk", "bus" or something otpc specific.

> +	if (IS_ERR(otpc->clk)) {
> +		dev_err(&pdev->dev, "Error (%ld) getting clock!\n",
> +			PTR_ERR(otpc->clk));
> +		return PTR_ERR(otpc->clk);

return dev_err_probe().

> +	}
> +
>  	reg = readl_relaxed(otpc->base + MCHP_OTPC_WPSR);
>  	if (reg)
>  		dev_warn(&pdev->dev,

Re: [PATCH v1 04/12] nvmem: microchip-otpc: Add SAM9X60 support

[claudiu beznea]

On 21.08.2024 13:59, Alexander Dahl wrote:
> Register layout is almost identical to sama7g5 OTPC.

Can you please mention some major differences?

> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  drivers/nvmem/microchip-otpc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index bd3383eabdf6..b8ed7412dbca 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -271,6 +271,7 @@ static int mchp_otpc_probe(struct platform_device *pdev)
>  
>  static const struct of_device_id __maybe_unused mchp_otpc_ids[] = {
>  	{ .compatible = "microchip,sama7g5-otpc", },
> +	{ .compatible = "microchip,sam9x60-otpc", },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, mchp_otpc_ids);

Re: [PATCH v1 07/12] nvmem: microchip-otpc: Add missing register definitions

[claudiu beznea]

On 21.08.2024 13:59, Alexander Dahl wrote:
> According to datasheets DS60001765B for SAMA7G5 and DS60001579G for
> SAM9X60.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  drivers/nvmem/microchip-otpc.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index b8ed7412dbca..4630e96243ac 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -21,9 +21,24 @@
>  #define MCHP_OTPC_AR			(0x8)
>  #define MCHP_OTPC_SR			(0xc)
>  #define MCHP_OTPC_SR_READ		BIT(6)
> +#define MCHP_OTPC_IER			(0x10)
> +#define MCHP_OTPC_IDR			(0x14)
> +#define MCHP_OTPC_IMR			(0x18)
> +#define MCHP_OTPC_ISR			(0x1C)
> +#define MCHP_OTPC_ISR_COERR		BIT(13)
>  #define MCHP_OTPC_HR			(0x20)
>  #define MCHP_OTPC_HR_SIZE		GENMASK(15, 8)
>  #define MCHP_OTPC_DR			(0x24)
> +#define MCHP_OTPC_BAR			(0x30)
> +#define MCHP_OTPC_CAR			(0x34)
> +#define MCHP_OTPC_UHC0R			(0x50)
> +#define MCHP_OTPC_UHC1R			(0x54)
> +#define MCHP_OTPC_UID0R			(0x60)
> +#define MCHP_OTPC_UID1R			(0x64)
> +#define MCHP_OTPC_UID2R			(0x68)
> +#define MCHP_OTPC_UID3R			(0x6C)
> +#define MCHP_OTPC_WPMR			(0xE4)
> +#define MCHP_OTPC_WPSR			(0xE8)

Are all these used in driver?

>  
>  #define MCHP_OTPC_NAME			"mchp-otpc"
>  #define MCHP_OTPC_SIZE			(11 * 1024)

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

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

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

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

[Alexander Dahl]

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

Re: [PATCH v1 04/12] nvmem: microchip-otpc: Add SAM9X60 support

[Alexander Dahl]

Hello Claudiu,

Am Sat, Aug 24, 2024 at 06:53:53PM +0300 schrieb claudiu beznea:
> 
> 
> On 21.08.2024 13:59, Alexander Dahl wrote:
> > Register layout is almost identical to sama7g5 OTPC.
> 
> Can you please mention some major differences?

- SAMA7G5 has an additional bit SECURE in the OTPC Header Register
  (OTPC_HR) not present on SAM9X60.
- SAMA7G5 has an additional register OTPC Secure Custom Address
  Register (OTPC_SCAR) not present on SAM9X60.
- SAMA7G5 has an additional field SECDBG[7:0] in OTPC User Hardware
  Configuration 0 Register (OTPC_UHC0R) not present on SAM9X60.
- SAMA7G5 has three additional bits (SCPGDIS, SCLKDIS, SCINVDIS) in
  the OTPC User Hardware Configuration 1 Register (OTPC_UHC1R) not
  present on SAM9X60.

All are currently not used by the driver.

Is adding this information to the commit message sufficient?

Greets
Alex

> 
> > 
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> >  drivers/nvmem/microchip-otpc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> > index bd3383eabdf6..b8ed7412dbca 100644
> > --- a/drivers/nvmem/microchip-otpc.c
> > +++ b/drivers/nvmem/microchip-otpc.c
> > @@ -271,6 +271,7 @@ static int mchp_otpc_probe(struct platform_device *pdev)
> >  
> >  static const struct of_device_id __maybe_unused mchp_otpc_ids[] = {
> >  	{ .compatible = "microchip,sama7g5-otpc", },
> > +	{ .compatible = "microchip,sam9x60-otpc", },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(of, mchp_otpc_ids);

Re: [PATCH v1 07/12] nvmem: microchip-otpc: Add missing register definitions

[Alexander Dahl]

Hello Claudiu,

Am Sat, Aug 24, 2024 at 06:54:02PM +0300 schrieb claudiu beznea:
> 
> 
> On 21.08.2024 13:59, Alexander Dahl wrote:
> > According to datasheets DS60001765B for SAMA7G5 and DS60001579G for
> > SAM9X60.
> > 
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> >  drivers/nvmem/microchip-otpc.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> > index b8ed7412dbca..4630e96243ac 100644
> > --- a/drivers/nvmem/microchip-otpc.c
> > +++ b/drivers/nvmem/microchip-otpc.c
> > @@ -21,9 +21,24 @@
> >  #define MCHP_OTPC_AR			(0x8)
> >  #define MCHP_OTPC_SR			(0xc)
> >  #define MCHP_OTPC_SR_READ		BIT(6)
> > +#define MCHP_OTPC_IER			(0x10)
> > +#define MCHP_OTPC_IDR			(0x14)
> > +#define MCHP_OTPC_IMR			(0x18)
> > +#define MCHP_OTPC_ISR			(0x1C)
> > +#define MCHP_OTPC_ISR_COERR		BIT(13)
> >  #define MCHP_OTPC_HR			(0x20)
> >  #define MCHP_OTPC_HR_SIZE		GENMASK(15, 8)
> >  #define MCHP_OTPC_DR			(0x24)
> > +#define MCHP_OTPC_BAR			(0x30)
> > +#define MCHP_OTPC_CAR			(0x34)
> > +#define MCHP_OTPC_UHC0R			(0x50)
> > +#define MCHP_OTPC_UHC1R			(0x54)
> > +#define MCHP_OTPC_UID0R			(0x60)
> > +#define MCHP_OTPC_UID1R			(0x64)
> > +#define MCHP_OTPC_UID2R			(0x68)
> > +#define MCHP_OTPC_UID3R			(0x6C)
> > +#define MCHP_OTPC_WPMR			(0xE4)
> > +#define MCHP_OTPC_WPSR			(0xE8)
> 
> Are all these used in driver?

Not all, but some.  What are you implying?  Only add register
definitions actually used in the driver?  Why?

Those register offsets won't change, but helped us when debugging.
Debug code (e.g. register dump) is not part of the patch series.

Greets
Alex

> 
> >  
> >  #define MCHP_OTPC_NAME			"mchp-otpc"
> >  #define MCHP_OTPC_SIZE			(11 * 1024)

Re: [PATCH v1 04/12] nvmem: microchip-otpc: Add SAM9X60 support

[claudiu beznea]

Hi, Alexander,

On 28.08.2024 11:09, Alexander Dahl wrote:
> Hello Claudiu,
> 
> Am Sat, Aug 24, 2024 at 06:53:53PM +0300 schrieb claudiu beznea:
>>
>>
>> On 21.08.2024 13:59, Alexander Dahl wrote:
>>> Register layout is almost identical to sama7g5 OTPC.
>>
>> Can you please mention some major differences?
> 
> - SAMA7G5 has an additional bit SECURE in the OTPC Header Register
>   (OTPC_HR) not present on SAM9X60.
> - SAMA7G5 has an additional register OTPC Secure Custom Address
>   Register (OTPC_SCAR) not present on SAM9X60.
> - SAMA7G5 has an additional field SECDBG[7:0] in OTPC User Hardware
>   Configuration 0 Register (OTPC_UHC0R) not present on SAM9X60.
> - SAMA7G5 has three additional bits (SCPGDIS, SCLKDIS, SCINVDIS) in
>   the OTPC User Hardware Configuration 1 Register (OTPC_UHC1R) not
>   present on SAM9X60.
> 
> All are currently not used by the driver.
> 
> Is adding this information to the commit message sufficient?

More than enough. If you can do a summary of it would be better, I think.

Thank you,
Claudiu Beznea

> 
> Greets
> Alex
> 
>>
>>>
>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>> ---
>>>  drivers/nvmem/microchip-otpc.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
>>> index bd3383eabdf6..b8ed7412dbca 100644
>>> --- a/drivers/nvmem/microchip-otpc.c
>>> +++ b/drivers/nvmem/microchip-otpc.c
>>> @@ -271,6 +271,7 @@ static int mchp_otpc_probe(struct platform_device *pdev)
>>>  
>>>  static const struct of_device_id __maybe_unused mchp_otpc_ids[] = {
>>>  	{ .compatible = "microchip,sama7g5-otpc", },
>>> +	{ .compatible = "microchip,sam9x60-otpc", },
>>>  	{ },
>>>  };
>>>  MODULE_DEVICE_TABLE(of, mchp_otpc_ids);

Re: [PATCH v1 07/12] nvmem: microchip-otpc: Add missing register definitions

[claudiu beznea]

On 28.08.2024 11:14, Alexander Dahl wrote:
> Hello Claudiu,
> 
> Am Sat, Aug 24, 2024 at 06:54:02PM +0300 schrieb claudiu beznea:
>>
>>
>> On 21.08.2024 13:59, Alexander Dahl wrote:
>>> According to datasheets DS60001765B for SAMA7G5 and DS60001579G for
>>> SAM9X60.
>>>
>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>> ---
>>>  drivers/nvmem/microchip-otpc.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
>>> index b8ed7412dbca..4630e96243ac 100644
>>> --- a/drivers/nvmem/microchip-otpc.c
>>> +++ b/drivers/nvmem/microchip-otpc.c
>>> @@ -21,9 +21,24 @@
>>>  #define MCHP_OTPC_AR			(0x8)
>>>  #define MCHP_OTPC_SR			(0xc)
>>>  #define MCHP_OTPC_SR_READ		BIT(6)
>>> +#define MCHP_OTPC_IER			(0x10)
>>> +#define MCHP_OTPC_IDR			(0x14)
>>> +#define MCHP_OTPC_IMR			(0x18)
>>> +#define MCHP_OTPC_ISR			(0x1C)
>>> +#define MCHP_OTPC_ISR_COERR		BIT(13)
>>>  #define MCHP_OTPC_HR			(0x20)
>>>  #define MCHP_OTPC_HR_SIZE		GENMASK(15, 8)
>>>  #define MCHP_OTPC_DR			(0x24)
>>> +#define MCHP_OTPC_BAR			(0x30)
>>> +#define MCHP_OTPC_CAR			(0x34)
>>> +#define MCHP_OTPC_UHC0R			(0x50)
>>> +#define MCHP_OTPC_UHC1R			(0x54)
>>> +#define MCHP_OTPC_UID0R			(0x60)
>>> +#define MCHP_OTPC_UID1R			(0x64)
>>> +#define MCHP_OTPC_UID2R			(0x68)
>>> +#define MCHP_OTPC_UID3R			(0x6C)
>>> +#define MCHP_OTPC_WPMR			(0xE4)
>>> +#define MCHP_OTPC_WPSR			(0xE8)
>>
>> Are all these used in driver?
> 
> Not all, but some.  What are you implying?  Only add register
> definitions actually used in the driver? 

Yes!

> Why?

Less code to maintain. If it's not used there is no meaning to have it.


> 
> Those register offsets won't change, but helped us when debugging.
> Debug code (e.g. register dump) is not part of the patch series.
> 
> Greets
> Alex
> 
>>
>>>  
>>>  #define MCHP_OTPC_NAME			"mchp-otpc"
>>>  #define MCHP_OTPC_SIZE			(11 * 1024)

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

[claudiu beznea]

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

Re: [PATCH v1 07/12] nvmem: microchip-otpc: Add missing register definitions

[Alexander Dahl]

Hello Claudiu,

Am Sat, Aug 31, 2024 at 06:33:50PM +0300 schrieb claudiu beznea:
> 
> 
> On 28.08.2024 11:14, Alexander Dahl wrote:
> > Hello Claudiu,
> > 
> > Am Sat, Aug 24, 2024 at 06:54:02PM +0300 schrieb claudiu beznea:
> >>
> >>
> >> On 21.08.2024 13:59, Alexander Dahl wrote:
> >>> According to datasheets DS60001765B for SAMA7G5 and DS60001579G for
> >>> SAM9X60.
> >>>
> >>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> >>> ---
> >>>  drivers/nvmem/microchip-otpc.c | 15 +++++++++++++++
> >>>  1 file changed, 15 insertions(+)
> >>>
> >>> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> >>> index b8ed7412dbca..4630e96243ac 100644
> >>> --- a/drivers/nvmem/microchip-otpc.c
> >>> +++ b/drivers/nvmem/microchip-otpc.c
> >>> @@ -21,9 +21,24 @@
> >>>  #define MCHP_OTPC_AR			(0x8)
> >>>  #define MCHP_OTPC_SR			(0xc)
> >>>  #define MCHP_OTPC_SR_READ		BIT(6)
> >>> +#define MCHP_OTPC_IER			(0x10)
> >>> +#define MCHP_OTPC_IDR			(0x14)
> >>> +#define MCHP_OTPC_IMR			(0x18)
> >>> +#define MCHP_OTPC_ISR			(0x1C)
> >>> +#define MCHP_OTPC_ISR_COERR		BIT(13)
> >>>  #define MCHP_OTPC_HR			(0x20)
> >>>  #define MCHP_OTPC_HR_SIZE		GENMASK(15, 8)
> >>>  #define MCHP_OTPC_DR			(0x24)
> >>> +#define MCHP_OTPC_BAR			(0x30)
> >>> +#define MCHP_OTPC_CAR			(0x34)
> >>> +#define MCHP_OTPC_UHC0R			(0x50)
> >>> +#define MCHP_OTPC_UHC1R			(0x54)
> >>> +#define MCHP_OTPC_UID0R			(0x60)
> >>> +#define MCHP_OTPC_UID1R			(0x64)
> >>> +#define MCHP_OTPC_UID2R			(0x68)
> >>> +#define MCHP_OTPC_UID3R			(0x6C)
> >>> +#define MCHP_OTPC_WPMR			(0xE4)
> >>> +#define MCHP_OTPC_WPSR			(0xE8)
> >>
> >> Are all these used in driver?
> > 
> > Not all, but some.  What are you implying?  Only add register
> > definitions actually used in the driver? 
> 
> Yes!

Okay.

So if I drop the patch with the warnings on driver probe you did not
like (checking for pre probe error conditions in interrupt register
for example), then it is just the MCHP_OTPC_UID0R and I would squash
that one definition in the last patch adding the nvmem for the UID
then, okay?

Greets
Alex

> 
> > Why?
> 
> Less code to maintain. If it's not used there is no meaning to have it.
> 
> 
> > 
> > Those register offsets won't change, but helped us when debugging.
> > Debug code (e.g. register dump) is not part of the patch series.
> > 
> > Greets
> > Alex
> > 
> >>
> >>>  
> >>>  #define MCHP_OTPC_NAME			"mchp-otpc"
> >>>  #define MCHP_OTPC_SIZE			(11 * 1024)
> 

Re: [PATCH v1 07/12] nvmem: microchip-otpc: Add missing register definitions

[claudiu beznea]

On 02.09.2024 11:08, Alexander Dahl wrote:
> Hello Claudiu,
> 
> Am Sat, Aug 31, 2024 at 06:33:50PM +0300 schrieb claudiu beznea:
>>
>>
>> On 28.08.2024 11:14, Alexander Dahl wrote:
>>> Hello Claudiu,
>>>
>>> Am Sat, Aug 24, 2024 at 06:54:02PM +0300 schrieb claudiu beznea:
>>>>
>>>>
>>>> On 21.08.2024 13:59, Alexander Dahl wrote:
>>>>> According to datasheets DS60001765B for SAMA7G5 and DS60001579G for
>>>>> SAM9X60.
>>>>>
>>>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>>>> ---
>>>>>  drivers/nvmem/microchip-otpc.c | 15 +++++++++++++++
>>>>>  1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
>>>>> index b8ed7412dbca..4630e96243ac 100644
>>>>> --- a/drivers/nvmem/microchip-otpc.c
>>>>> +++ b/drivers/nvmem/microchip-otpc.c
>>>>> @@ -21,9 +21,24 @@
>>>>>  #define MCHP_OTPC_AR			(0x8)
>>>>>  #define MCHP_OTPC_SR			(0xc)
>>>>>  #define MCHP_OTPC_SR_READ		BIT(6)
>>>>> +#define MCHP_OTPC_IER			(0x10)
>>>>> +#define MCHP_OTPC_IDR			(0x14)
>>>>> +#define MCHP_OTPC_IMR			(0x18)
>>>>> +#define MCHP_OTPC_ISR			(0x1C)
>>>>> +#define MCHP_OTPC_ISR_COERR		BIT(13)
>>>>>  #define MCHP_OTPC_HR			(0x20)
>>>>>  #define MCHP_OTPC_HR_SIZE		GENMASK(15, 8)
>>>>>  #define MCHP_OTPC_DR			(0x24)
>>>>> +#define MCHP_OTPC_BAR			(0x30)
>>>>> +#define MCHP_OTPC_CAR			(0x34)
>>>>> +#define MCHP_OTPC_UHC0R			(0x50)
>>>>> +#define MCHP_OTPC_UHC1R			(0x54)
>>>>> +#define MCHP_OTPC_UID0R			(0x60)
>>>>> +#define MCHP_OTPC_UID1R			(0x64)
>>>>> +#define MCHP_OTPC_UID2R			(0x68)
>>>>> +#define MCHP_OTPC_UID3R			(0x6C)
>>>>> +#define MCHP_OTPC_WPMR			(0xE4)
>>>>> +#define MCHP_OTPC_WPSR			(0xE8)
>>>>
>>>> Are all these used in driver?
>>>
>>> Not all, but some.  What are you implying?  Only add register
>>> definitions actually used in the driver? 
>>
>> Yes!
> 
> Okay.
> 
> So if I drop the patch with the warnings on driver probe you did not
> like (checking for pre probe error conditions in interrupt register
> for example), then it is just the MCHP_OTPC_UID0R and I would squash
> that one definition in the last patch adding the nvmem for the UID
> then, okay?

OK for me.

The idea would be to keep just what is used.

Thank you,
Claudiu Beznea

> 
> Greets
> Alex
> 
>>
>>> Why?
>>
>> Less code to maintain. If it's not used there is no meaning to have it.
>>
>>
>>>
>>> Those register offsets won't change, but helped us when debugging.
>>> Debug code (e.g. register dump) is not part of the patch series.
>>>
>>> Greets
>>> Alex
>>>
>>>>
>>>>>  
>>>>>  #define MCHP_OTPC_NAME			"mchp-otpc"
>>>>>  #define MCHP_OTPC_SIZE			(11 * 1024)
>>

Re: [PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[Alexander Dahl]

Hello Claudiu,

after being busy with other things, I'm back looking at this series.
As Nicolas pointed out [1], we need three clocks for the OTPC to work,
quote:

  "for all the products, the main RC oscillator, the OTPC peripheral
  clock and the MCKx clocks associated to OTP must be enabled."

I have a problem with making the main_rc_osc accessible for both
SAM9X60 and SAMA7G5 here, see below.

Am Wed, Aug 21, 2024 at 12:59:40PM +0200 schrieb Alexander Dahl:
> SAM9X60 Datasheet (DS60001579G) Section "23.4 Product Dependencies"
> says:
> 
>     "The OTPC is clocked through the Power Management Controller (PMC).
>     The user must power on the main RC oscillator and enable the
>     peripheral clock of the OTPC prior to reading or writing the OTP
>     memory."
> 
> The code for enabling/disabling that clock is already present, it was
> just not possible to hook into DT anymore, after at91 clk devicetree
> binding rework back in 2018 for kernel v4.19.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  drivers/clk/at91/sam9x60.c       | 3 ++-
>  include/dt-bindings/clock/at91.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/at91/sam9x60.c b/drivers/clk/at91/sam9x60.c
> index e309cbf3cb9a..4d5ee20b8fc4 100644
> --- a/drivers/clk/at91/sam9x60.c
> +++ b/drivers/clk/at91/sam9x60.c
> @@ -207,7 +207,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>  	if (IS_ERR(regmap))
>  		return;
>  
> -	sam9x60_pmc = pmc_data_allocate(PMC_PLLACK + 1,
> +	sam9x60_pmc = pmc_data_allocate(PMC_MAIN_RC + 1,
>  					nck(sam9x60_systemck),
>  					nck(sam9x60_periphck),
>  					nck(sam9x60_gck), 8);
> @@ -218,6 +218,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>  					   50000000);
>  	if (IS_ERR(hw))
>  		goto err_free;
> +	sam9x60_pmc->chws[PMC_MAIN_RC] = hw;
>  
>  	hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
>  	if (IS_ERR(hw))
> diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
> index 3e3972a814c1..f957625cb3ac 100644
> --- a/include/dt-bindings/clock/at91.h
> +++ b/include/dt-bindings/clock/at91.h
> @@ -25,6 +25,7 @@
>  #define PMC_PLLBCK		8
>  #define PMC_AUDIOPLLCK		9
>  #define PMC_AUDIOPINCK		10
> +#define PMC_MAIN_RC		11
>  
>  /* SAMA7G5 */
>  #define PMC_CPUPLL		(PMC_MAIN + 1)

There are IDs defined in the devicetree bindings here, which are used
both in dts and in driver code as array indexes.  In v1 of the patch
series I just added a new last element in the end of the generic list
and used that for SAM9X60.

For SAMA7G5 those IDs are branched of from PMC_MAIN in between, making
SAMA7G5 using a different last element, and different values after
PMC_MAIN.

Now we need a new ID for main rc osc, but not only for SAM9X60, but
also for SAMA7G5.  I'm not sure what the implications would be, if the
new ID would be added in between before PMC_MAIN, so all values would
change?  Adding it to the end of the lists would probably be safe, but
then you would need a diffently named variant for SAMA7G5's different
IDs.  I find the current status somewhat unfortunate for future
extensions.  How should this new ID be added here?  What would be the
way forward?

Greets
Alex

[1] https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u

> -- 
> 2.39.2
> 
> 

Re: [PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[Ryan Wanner]

Hello Alex,

I think a possible solution is to put the DT binding ID for main rc oc
after PMC_MCK and then add 1 to all the other IDs that are not dependent
on PMC_MAIN, the IDs that are before the branch for the sama7g54.

One issue I see with this solution is with SoCs that do not want the
main rc os exported to the DT the driver array might be allocating too
much memory, this can be solved by removing the +1 that is in the clock
drivers next to the device tree binding macro, since this macro is now
increased by 1 with this change.

Doing a quick test on the sam9x60 and sama7g54 I did not see any glaring
issues with this potential solution.

Best,

Ryan


On 9/19/24 05:39, Alexander Dahl wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello Claudiu,
> 
> after being busy with other things, I'm back looking at this series.
> As Nicolas pointed out [1], we need three clocks for the OTPC to work,
> quote:
> 
>   "for all the products, the main RC oscillator, the OTPC peripheral
>   clock and the MCKx clocks associated to OTP must be enabled."
> 
> I have a problem with making the main_rc_osc accessible for both
> SAM9X60 and SAMA7G5 here, see below.
> 
> Am Wed, Aug 21, 2024 at 12:59:40PM +0200 schrieb Alexander Dahl:
>> SAM9X60 Datasheet (DS60001579G) Section "23.4 Product Dependencies"
>> says:
>>
>>     "The OTPC is clocked through the Power Management Controller (PMC).
>>     The user must power on the main RC oscillator and enable the
>>     peripheral clock of the OTPC prior to reading or writing the OTP
>>     memory."
>>
>> The code for enabling/disabling that clock is already present, it was
>> just not possible to hook into DT anymore, after at91 clk devicetree
>> binding rework back in 2018 for kernel v4.19.
>>
>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>> ---
>>  drivers/clk/at91/sam9x60.c       | 3 ++-
>>  include/dt-bindings/clock/at91.h | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/at91/sam9x60.c b/drivers/clk/at91/sam9x60.c
>> index e309cbf3cb9a..4d5ee20b8fc4 100644
>> --- a/drivers/clk/at91/sam9x60.c
>> +++ b/drivers/clk/at91/sam9x60.c
>> @@ -207,7 +207,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>>       if (IS_ERR(regmap))
>>               return;
>>
>> -     sam9x60_pmc = pmc_data_allocate(PMC_PLLACK + 1,
>> +     sam9x60_pmc = pmc_data_allocate(PMC_MAIN_RC + 1,
>>                                       nck(sam9x60_systemck),
>>                                       nck(sam9x60_periphck),
>>                                       nck(sam9x60_gck), 8);
>> @@ -218,6 +218,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>>                                          50000000);
>>       if (IS_ERR(hw))
>>               goto err_free;
>> +     sam9x60_pmc->chws[PMC_MAIN_RC] = hw;
>>
>>       hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
>>       if (IS_ERR(hw))
>> diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
>> index 3e3972a814c1..f957625cb3ac 100644
>> --- a/include/dt-bindings/clock/at91.h
>> +++ b/include/dt-bindings/clock/at91.h
>> @@ -25,6 +25,7 @@
>>  #define PMC_PLLBCK           8
>>  #define PMC_AUDIOPLLCK               9
>>  #define PMC_AUDIOPINCK               10
>> +#define PMC_MAIN_RC          11
>>
>>  /* SAMA7G5 */
>>  #define PMC_CPUPLL           (PMC_MAIN + 1)
> 
> There are IDs defined in the devicetree bindings here, which are used
> both in dts and in driver code as array indexes.  In v1 of the patch
> series I just added a new last element in the end of the generic list
> and used that for SAM9X60.
> 
> For SAMA7G5 those IDs are branched of from PMC_MAIN in between, making
> SAMA7G5 using a different last element, and different values after
> PMC_MAIN.
> 
> Now we need a new ID for main rc osc, but not only for SAM9X60, but
> also for SAMA7G5.  I'm not sure what the implications would be, if the
> new ID would be added in between before PMC_MAIN, so all values would
> change?  Adding it to the end of the lists would probably be safe, but
> then you would need a diffently named variant for SAMA7G5's different
> IDs.  I find the current status somewhat unfortunate for future
> extensions.  How should this new ID be added here?  What would be the
> way forward?
> 
> Greets
> Alex
> 
> [1] https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u
> 
>> --
>> 2.39.2
>>
>>
> 

Re: [PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[Nicolas Ferre]

On 24/09/2024 at 17:52, Ryan Wanner wrote:
> Hello Alex,
> 
> I think a possible solution is to put the DT binding ID for main rc oc
> after PMC_MCK and then add 1 to all the other IDs that are not dependent
> on PMC_MAIN, the IDs that are before the branch for the sama7g54.
> 
> One issue I see with this solution is with SoCs that do not want the
> main rc os exported to the DT the driver array might be allocating too
> much memory, this can be solved by removing the +1 that is in the clock

We're talking about a handful of bytes, we can surely afford that.

My $0.02. Regards,
   Nicolas

> drivers next to the device tree binding macro, since this macro is now
> increased by 1 with this change.
> 
> Doing a quick test on the sam9x60 and sama7g54 I did not see any glaring
> issues with this potential solution.
> 
> Best,
> 
> Ryan
> 
> 
> On 9/19/24 05:39, Alexander Dahl wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hello Claudiu,
>>
>> after being busy with other things, I'm back looking at this series.
>> As Nicolas pointed out [1], we need three clocks for the OTPC to work,
>> quote:
>>
>>    "for all the products, the main RC oscillator, the OTPC peripheral
>>    clock and the MCKx clocks associated to OTP must be enabled."
>>
>> I have a problem with making the main_rc_osc accessible for both
>> SAM9X60 and SAMA7G5 here, see below.
>>
>> Am Wed, Aug 21, 2024 at 12:59:40PM +0200 schrieb Alexander Dahl:
>>> SAM9X60 Datasheet (DS60001579G) Section "23.4 Product Dependencies"
>>> says:
>>>
>>>      "The OTPC is clocked through the Power Management Controller (PMC).
>>>      The user must power on the main RC oscillator and enable the
>>>      peripheral clock of the OTPC prior to reading or writing the OTP
>>>      memory."
>>>
>>> The code for enabling/disabling that clock is already present, it was
>>> just not possible to hook into DT anymore, after at91 clk devicetree
>>> binding rework back in 2018 for kernel v4.19.
>>>
>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>> ---
>>>   drivers/clk/at91/sam9x60.c       | 3 ++-
>>>   include/dt-bindings/clock/at91.h | 1 +
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/at91/sam9x60.c b/drivers/clk/at91/sam9x60.c
>>> index e309cbf3cb9a..4d5ee20b8fc4 100644
>>> --- a/drivers/clk/at91/sam9x60.c
>>> +++ b/drivers/clk/at91/sam9x60.c
>>> @@ -207,7 +207,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>>>        if (IS_ERR(regmap))
>>>                return;
>>>
>>> -     sam9x60_pmc = pmc_data_allocate(PMC_PLLACK + 1,
>>> +     sam9x60_pmc = pmc_data_allocate(PMC_MAIN_RC + 1,
>>>                                        nck(sam9x60_systemck),
>>>                                        nck(sam9x60_periphck),
>>>                                        nck(sam9x60_gck), 8);
>>> @@ -218,6 +218,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>>>                                           50000000);
>>>        if (IS_ERR(hw))
>>>                goto err_free;
>>> +     sam9x60_pmc->chws[PMC_MAIN_RC] = hw;
>>>
>>>        hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
>>>        if (IS_ERR(hw))
>>> diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
>>> index 3e3972a814c1..f957625cb3ac 100644
>>> --- a/include/dt-bindings/clock/at91.h
>>> +++ b/include/dt-bindings/clock/at91.h
>>> @@ -25,6 +25,7 @@
>>>   #define PMC_PLLBCK           8
>>>   #define PMC_AUDIOPLLCK               9
>>>   #define PMC_AUDIOPINCK               10
>>> +#define PMC_MAIN_RC          11
>>>
>>>   /* SAMA7G5 */
>>>   #define PMC_CPUPLL           (PMC_MAIN + 1)
>>
>> There are IDs defined in the devicetree bindings here, which are used
>> both in dts and in driver code as array indexes.  In v1 of the patch
>> series I just added a new last element in the end of the generic list
>> and used that for SAM9X60.
>>
>> For SAMA7G5 those IDs are branched of from PMC_MAIN in between, making
>> SAMA7G5 using a different last element, and different values after
>> PMC_MAIN.
>>
>> Now we need a new ID for main rc osc, but not only for SAM9X60, but
>> also for SAMA7G5.  I'm not sure what the implications would be, if the
>> new ID would be added in between before PMC_MAIN, so all values would
>> change?  Adding it to the end of the lists would probably be safe, but
>> then you would need a diffently named variant for SAMA7G5's different
>> IDs.  I find the current status somewhat unfortunate for future
>> extensions.  How should this new ID be added here?  What would be the
>> way forward?
>>
>> Greets
>> Alex
>>
>> [1] https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u
>>
>>> --
>>> 2.39.2
>>>
>>>
>>
> 

Re: [PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[claudiu beznea]

Hi, Ryan, Alexander,

Sorry for returning late, I took some time to think about it...

On 24.09.2024 18:52, Ryan Wanner wrote:
> Hello Alex,
> 
> I think a possible solution is to put the DT binding ID for main rc oc
> after PMC_MCK and then add 1 to all the other IDs that are not dependent
> on PMC_MAIN, the IDs that are before the branch for the sama7g54.

If I understand correctly, wouldn't this shift also the rest of the IDs
and break the DT ABI?

> 
> One issue I see with this solution is with SoCs that do not want the
> main rc os exported to the DT the driver array might be allocating too
> much memory, this can be solved by removing the +1 that is in the clock
> drivers next to the device tree binding macro, since this macro is now
> increased by 1 with this change.
> 
> Doing a quick test on the sam9x60 and sama7g54 I did not see any glaring
> issues with this potential solution.
> 
> Best,
> 
> Ryan
> 
> 
> On 9/19/24 05:39, Alexander Dahl wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hello Claudiu,
>>
>> after being busy with other things, I'm back looking at this series.
>> As Nicolas pointed out [1], we need three clocks for the OTPC to work,
>> quote:
>>
>>   "for all the products, the main RC oscillator, the OTPC peripheral
>>   clock and the MCKx clocks associated to OTP must be enabled."
>>
>> I have a problem with making the main_rc_osc accessible for both
>> SAM9X60 and SAMA7G5 here, see below.
>>
>> Am Wed, Aug 21, 2024 at 12:59:40PM +0200 schrieb Alexander Dahl:
>>> SAM9X60 Datasheet (DS60001579G) Section "23.4 Product Dependencies"
>>> says:
>>>
>>>     "The OTPC is clocked through the Power Management Controller (PMC).
>>>     The user must power on the main RC oscillator and enable the
>>>     peripheral clock of the OTPC prior to reading or writing the OTP
>>>     memory."
>>>
>>> The code for enabling/disabling that clock is already present, it was
>>> just not possible to hook into DT anymore, after at91 clk devicetree
>>> binding rework back in 2018 for kernel v4.19.
>>>
>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>> ---
>>>  drivers/clk/at91/sam9x60.c       | 3 ++-
>>>  include/dt-bindings/clock/at91.h | 1 +
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/at91/sam9x60.c b/drivers/clk/at91/sam9x60.c
>>> index e309cbf3cb9a..4d5ee20b8fc4 100644
>>> --- a/drivers/clk/at91/sam9x60.c
>>> +++ b/drivers/clk/at91/sam9x60.c
>>> @@ -207,7 +207,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>>>       if (IS_ERR(regmap))
>>>               return;
>>>
>>> -     sam9x60_pmc = pmc_data_allocate(PMC_PLLACK + 1,
>>> +     sam9x60_pmc = pmc_data_allocate(PMC_MAIN_RC + 1,
>>>                                       nck(sam9x60_systemck),
>>>                                       nck(sam9x60_periphck),
>>>                                       nck(sam9x60_gck), 8);
>>> @@ -218,6 +218,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>>>                                          50000000);
>>>       if (IS_ERR(hw))
>>>               goto err_free;
>>> +     sam9x60_pmc->chws[PMC_MAIN_RC] = hw;
>>>
>>>       hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
>>>       if (IS_ERR(hw))
>>> diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
>>> index 3e3972a814c1..f957625cb3ac 100644
>>> --- a/include/dt-bindings/clock/at91.h
>>> +++ b/include/dt-bindings/clock/at91.h
>>> @@ -25,6 +25,7 @@
>>>  #define PMC_PLLBCK           8
>>>  #define PMC_AUDIOPLLCK               9
>>>  #define PMC_AUDIOPINCK               10
>>> +#define PMC_MAIN_RC          11
>>>
>>>  /* SAMA7G5 */
>>>  #define PMC_CPUPLL           (PMC_MAIN + 1)
>>
>> There are IDs defined in the devicetree bindings here, which are used
>> both in dts and in driver code as array indexes.  In v1 of the patch
>> series I just added a new last element in the end of the generic list
>> and used that for SAM9X60.
>>
>> For SAMA7G5 those IDs are branched of from PMC_MAIN in between, making
>> SAMA7G5 using a different last element, and different values after
>> PMC_MAIN.

Looking at it now, I think it was a bad decision to do this branch.
Thinking at it maybe it would be better to have per SoC specific bindings
to avoid this kind of issue in future. The PMC IP b/w different SAM SoCs is
anyway different and, as it happens now, we may reach to a point where, due
to issues in datasheet, or whatever human errors, we may reach this problem
again.

So, what do you think about having separate binding files for each SoC?

Another option would be to xlate the clocks not by directly indexing in
struct pmc_data::chws but by matching the driver clock ID and DT provided
id. This will increase the lookup time, from O(1) to O(N), N being 13 for
SAMA7G5, 15 for SAM9X7 and SAMA7D55. And will need adjustment at least for
SAM9X{60, 7} and SAMA7{G5, D55}. With this the of_clk_hw_pmc_get() will be
something like:

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 5aa9c1f1c886..22191d1ca78b 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -52,8 +52,10 @@ struct clk_hw *of_clk_hw_pmc_get(struct of_phandle_args
*clkspec, void *data)

        switch (type) {
        case PMC_TYPE_CORE:
-               if (idx < pmc_data->ncore)
-                       return pmc_data->chws[idx];
+               for (int i = 0; i < pmc_data->ncore; i++) {
+                       if (pmc_data->chws.idx[i] == i)
+                               return pmc_data->chws.hws[i];
+               }
                break;


diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
index 4fb29ca111f7..f7e88f9872dc 100644
--- a/drivers/clk/at91/pmc.h
+++ b/drivers/clk/at91/pmc.h
@@ -19,7 +19,10 @@ extern spinlock_t pmc_pcr_lock;

 struct pmc_data {
        unsigned int ncore;
-       struct clk_hw **chws;
+       struct {
+               struct clk_hw **hws;
+               int *idx;
+       } chws;

Thank you,
Claudiu Beznea

>>
>> Now we need a new ID for main rc osc, but not only for SAM9X60, but
>> also for SAMA7G5.  I'm not sure what the implications would be, if the
>> new ID would be added in between before PMC_MAIN, so all values would
>> change?  Adding it to the end of the lists would probably be safe, but
>> then you would need a diffently named variant for SAMA7G5's different
>> IDs.  I find the current status somewhat unfortunate for future
>> extensions.  How should this new ID be added here?  What would be the
>> way forward?
>>
>> Greets
>> Alex
>>
>> [1] https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u
>>
>>> --
>>> 2.39.2
>>>
>>>
>>
> 

Re: [PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[Ryan Wanner]

On 9/26/24 00:42, claudiu beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi, Ryan, Alexander,
> 
> Sorry for returning late, I took some time to think about it...
> 
> On 24.09.2024 18:52, Ryan Wanner wrote:
>> Hello Alex,
>>
>> I think a possible solution is to put the DT binding ID for main rc oc
>> after PMC_MCK and then add 1 to all the other IDs that are not dependent
>> on PMC_MAIN, the IDs that are before the branch for the sama7g54.
> 
> If I understand correctly, wouldn't this shift also the rest of the IDs
> and break the DT ABI?
> 
>>
>> One issue I see with this solution is with SoCs that do not want the
>> main rc os exported to the DT the driver array might be allocating too
>> much memory, this can be solved by removing the +1 that is in the clock
>> drivers next to the device tree binding macro, since this macro is now
>> increased by 1 with this change.
>>
>> Doing a quick test on the sam9x60 and sama7g54 I did not see any glaring
>> issues with this potential solution.
>>
>> Best,
>>
>> Ryan
>>
>>
>> On 9/19/24 05:39, Alexander Dahl wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hello Claudiu,
>>>
>>> after being busy with other things, I'm back looking at this series.
>>> As Nicolas pointed out [1], we need three clocks for the OTPC to work,
>>> quote:
>>>
>>>   "for all the products, the main RC oscillator, the OTPC peripheral
>>>   clock and the MCKx clocks associated to OTP must be enabled."
>>>
>>> I have a problem with making the main_rc_osc accessible for both
>>> SAM9X60 and SAMA7G5 here, see below.
>>>
>>> Am Wed, Aug 21, 2024 at 12:59:40PM +0200 schrieb Alexander Dahl:
>>>> SAM9X60 Datasheet (DS60001579G) Section "23.4 Product Dependencies"
>>>> says:
>>>>
>>>>     "The OTPC is clocked through the Power Management Controller (PMC).
>>>>     The user must power on the main RC oscillator and enable the
>>>>     peripheral clock of the OTPC prior to reading or writing the OTP
>>>>     memory."
>>>>
>>>> The code for enabling/disabling that clock is already present, it was
>>>> just not possible to hook into DT anymore, after at91 clk devicetree
>>>> binding rework back in 2018 for kernel v4.19.
>>>>
>>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>>> ---
>>>>  drivers/clk/at91/sam9x60.c       | 3 ++-
>>>>  include/dt-bindings/clock/at91.h | 1 +
>>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/at91/sam9x60.c b/drivers/clk/at91/sam9x60.c
>>>> index e309cbf3cb9a..4d5ee20b8fc4 100644
>>>> --- a/drivers/clk/at91/sam9x60.c
>>>> +++ b/drivers/clk/at91/sam9x60.c
>>>> @@ -207,7 +207,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>>>>       if (IS_ERR(regmap))
>>>>               return;
>>>>
>>>> -     sam9x60_pmc = pmc_data_allocate(PMC_PLLACK + 1,
>>>> +     sam9x60_pmc = pmc_data_allocate(PMC_MAIN_RC + 1,
>>>>                                       nck(sam9x60_systemck),
>>>>                                       nck(sam9x60_periphck),
>>>>                                       nck(sam9x60_gck), 8);
>>>> @@ -218,6 +218,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
>>>>                                          50000000);
>>>>       if (IS_ERR(hw))
>>>>               goto err_free;
>>>> +     sam9x60_pmc->chws[PMC_MAIN_RC] = hw;
>>>>
>>>>       hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
>>>>       if (IS_ERR(hw))
>>>> diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
>>>> index 3e3972a814c1..f957625cb3ac 100644
>>>> --- a/include/dt-bindings/clock/at91.h
>>>> +++ b/include/dt-bindings/clock/at91.h
>>>> @@ -25,6 +25,7 @@
>>>>  #define PMC_PLLBCK           8
>>>>  #define PMC_AUDIOPLLCK               9
>>>>  #define PMC_AUDIOPINCK               10
>>>> +#define PMC_MAIN_RC          11
>>>>
>>>>  /* SAMA7G5 */
>>>>  #define PMC_CPUPLL           (PMC_MAIN + 1)
>>>
>>> There are IDs defined in the devicetree bindings here, which are used
>>> both in dts and in driver code as array indexes.  In v1 of the patch
>>> series I just added a new last element in the end of the generic list
>>> and used that for SAM9X60.
>>>
>>> For SAMA7G5 those IDs are branched of from PMC_MAIN in between, making
>>> SAMA7G5 using a different last element, and different values after
>>> PMC_MAIN.
> 
> Looking at it now, I think it was a bad decision to do this branch.
> Thinking at it maybe it would be better to have per SoC specific bindings
> to avoid this kind of issue in future. The PMC IP b/w different SAM SoCs is
> anyway different and, as it happens now, we may reach to a point where, due
> to issues in datasheet, or whatever human errors, we may reach this problem
> again.
> 
> So, what do you think about having separate binding files for each SoC?

I think the simplest way to do this is having a separate file for the
SAMA7 SoC clock bindings. To me it looks like the split is for the SAMA7
SoCs only, so having a separate file will be the best solution as it
will mean less duplicate code and still keeping the O(1) look up time.

Best,
Ryan
> 
> Another option would be to xlate the clocks not by directly indexing in
> struct pmc_data::chws but by matching the driver clock ID and DT provided
> id. This will increase the lookup time, from O(1) to O(N), N being 13 for
> SAMA7G5, 15 for SAM9X7 and SAMA7D55. And will need adjustment at least for
> SAM9X{60, 7} and SAMA7{G5, D55}. With this the of_clk_hw_pmc_get() will be
> something like:
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 5aa9c1f1c886..22191d1ca78b 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -52,8 +52,10 @@ struct clk_hw *of_clk_hw_pmc_get(struct of_phandle_args
> *clkspec, void *data)
> 
>         switch (type) {
>         case PMC_TYPE_CORE:
> -               if (idx < pmc_data->ncore)
> -                       return pmc_data->chws[idx];
> +               for (int i = 0; i < pmc_data->ncore; i++) {
> +                       if (pmc_data->chws.idx[i] == i)
> +                               return pmc_data->chws.hws[i];
> +               }
>                 break;
> 
> 
> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
> index 4fb29ca111f7..f7e88f9872dc 100644
> --- a/drivers/clk/at91/pmc.h
> +++ b/drivers/clk/at91/pmc.h
> @@ -19,7 +19,10 @@ extern spinlock_t pmc_pcr_lock;
> 
>  struct pmc_data {
>         unsigned int ncore;
> -       struct clk_hw **chws;
> +       struct {
> +               struct clk_hw **hws;
> +               int *idx;
> +       } chws;
> 
> Thank you,
> Claudiu Beznea
> 
>>>
>>> Now we need a new ID for main rc osc, but not only for SAM9X60, but
>>> also for SAMA7G5.  I'm not sure what the implications would be, if the
>>> new ID would be added in between before PMC_MAIN, so all values would
>>> change?  Adding it to the end of the lists would probably be safe, but
>>> then you would need a diffently named variant for SAMA7G5's different
>>> IDs.  I find the current status somewhat unfortunate for future
>>> extensions.  How should this new ID be added here?  What would be the
>>> way forward?
>>>
>>> Greets
>>> Alex
>>>
>>> [1] https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u
>>>
>>>> --
>>>> 2.39.2
>>>>
>>>>
>>>
>>

Re: [PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[Alexander Dahl]

Hei hei,

I'm currently reworking this series.  Everything else is more or less
clear to me, but this core clock array index stuff makes knots in my
head, see below.  (I keep all the context, because it's been a while …)

Am Tue, Oct 01, 2024 at 08:04:55AM -0700 schrieb Ryan Wanner:
> On 9/26/24 00:42, claudiu beznea wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Hi, Ryan, Alexander,
> > 
> > Sorry for returning late, I took some time to think about it...
> > 
> > On 24.09.2024 18:52, Ryan Wanner wrote:
> >> Hello Alex,
> >>
> >> I think a possible solution is to put the DT binding ID for main rc oc
> >> after PMC_MCK and then add 1 to all the other IDs that are not dependent
> >> on PMC_MAIN, the IDs that are before the branch for the sama7g54.
> > 
> > If I understand correctly, wouldn't this shift also the rest of the IDs
> > and break the DT ABI?
> > 
> >>
> >> One issue I see with this solution is with SoCs that do not want the
> >> main rc os exported to the DT the driver array might be allocating too
> >> much memory, this can be solved by removing the +1 that is in the clock
> >> drivers next to the device tree binding macro, since this macro is now
> >> increased by 1 with this change.
> >>
> >> Doing a quick test on the sam9x60 and sama7g54 I did not see any glaring
> >> issues with this potential solution.
> >>
> >> Best,
> >>
> >> Ryan
> >>
> >>
> >> On 9/19/24 05:39, Alexander Dahl wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> Hello Claudiu,
> >>>
> >>> after being busy with other things, I'm back looking at this series.
> >>> As Nicolas pointed out [1], we need three clocks for the OTPC to work,
> >>> quote:
> >>>
> >>>   "for all the products, the main RC oscillator, the OTPC peripheral
> >>>   clock and the MCKx clocks associated to OTP must be enabled."
> >>>
> >>> I have a problem with making the main_rc_osc accessible for both
> >>> SAM9X60 and SAMA7G5 here, see below.
> >>>
> >>> Am Wed, Aug 21, 2024 at 12:59:40PM +0200 schrieb Alexander Dahl:
> >>>> SAM9X60 Datasheet (DS60001579G) Section "23.4 Product Dependencies"
> >>>> says:
> >>>>
> >>>>     "The OTPC is clocked through the Power Management Controller (PMC).
> >>>>     The user must power on the main RC oscillator and enable the
> >>>>     peripheral clock of the OTPC prior to reading or writing the OTP
> >>>>     memory."
> >>>>
> >>>> The code for enabling/disabling that clock is already present, it was
> >>>> just not possible to hook into DT anymore, after at91 clk devicetree
> >>>> binding rework back in 2018 for kernel v4.19.
> >>>>
> >>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> >>>> ---
> >>>>  drivers/clk/at91/sam9x60.c       | 3 ++-
> >>>>  include/dt-bindings/clock/at91.h | 1 +
> >>>>  2 files changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/clk/at91/sam9x60.c b/drivers/clk/at91/sam9x60.c
> >>>> index e309cbf3cb9a..4d5ee20b8fc4 100644
> >>>> --- a/drivers/clk/at91/sam9x60.c
> >>>> +++ b/drivers/clk/at91/sam9x60.c
> >>>> @@ -207,7 +207,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
> >>>>       if (IS_ERR(regmap))
> >>>>               return;
> >>>>
> >>>> -     sam9x60_pmc = pmc_data_allocate(PMC_PLLACK + 1,
> >>>> +     sam9x60_pmc = pmc_data_allocate(PMC_MAIN_RC + 1,
> >>>>                                       nck(sam9x60_systemck),
> >>>>                                       nck(sam9x60_periphck),
> >>>>                                       nck(sam9x60_gck), 8);
> >>>> @@ -218,6 +218,7 @@ static void __init sam9x60_pmc_setup(struct device_node *np)
> >>>>                                          50000000);
> >>>>       if (IS_ERR(hw))
> >>>>               goto err_free;
> >>>> +     sam9x60_pmc->chws[PMC_MAIN_RC] = hw;
> >>>>
> >>>>       hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
> >>>>       if (IS_ERR(hw))
> >>>> diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
> >>>> index 3e3972a814c1..f957625cb3ac 100644
> >>>> --- a/include/dt-bindings/clock/at91.h
> >>>> +++ b/include/dt-bindings/clock/at91.h
> >>>> @@ -25,6 +25,7 @@
> >>>>  #define PMC_PLLBCK           8
> >>>>  #define PMC_AUDIOPLLCK               9
> >>>>  #define PMC_AUDIOPINCK               10
> >>>> +#define PMC_MAIN_RC          11
> >>>>
> >>>>  /* SAMA7G5 */
> >>>>  #define PMC_CPUPLL           (PMC_MAIN + 1)
> >>>
> >>> There are IDs defined in the devicetree bindings here, which are used
> >>> both in dts and in driver code as array indexes.  In v1 of the patch
> >>> series I just added a new last element in the end of the generic list
> >>> and used that for SAM9X60.
> >>>
> >>> For SAMA7G5 those IDs are branched of from PMC_MAIN in between, making
> >>> SAMA7G5 using a different last element, and different values after
> >>> PMC_MAIN.
> > 
> > Looking at it now, I think it was a bad decision to do this branch.
> > Thinking at it maybe it would be better to have per SoC specific bindings
> > to avoid this kind of issue in future. The PMC IP b/w different SAM SoCs is
> > anyway different and, as it happens now, we may reach to a point where, due
> > to issues in datasheet, or whatever human errors, we may reach this problem
> > again.
> > 
> > So, what do you think about having separate binding files for each SoC?
> 
> I think the simplest way to do this is having a separate file for the
> SAMA7 SoC clock bindings. To me it looks like the split is for the SAMA7
> SoCs only, so having a separate file will be the best solution as it
> will mean less duplicate code and still keeping the O(1) look up time.

This is not true, at least not anymore.  I try to wrap it up.

We have 13 different drivers now for 15 different compatibles/SoC
variants:

at91rm9200.c
at91sam9g45.c
at91sam9n12.c
at91sam9rl.c
at91sam9x5.c
at91sam9260.c   -> also for 9261 and 9263
sam9x7.c
sam9x60.c
sama5d2.c
sama5d3.c
sama5d4.c
sama7d65.c
sama7g5.c

Those use different sets of core clocks allocated by
pmc_data_allocate() and with different maximum index aka array size.
No driver uses all members of pmc_data->chws, each leaves more or less
many holes, each hole around 44 bytes AFAICT.

(I have a spreadsheet for this now if anyone is interested.)

I need to add a new clock (main rc oscillator) for using the OTPC, a
block available on sam9x60, sam9x7, sama7g5 and sama7d65.  The max
indexes for those are:

sam9x60:    PMC_PLLACK = 7                      (using 4, wasting 4)
sam9x7:     PMC_LVDSPLL = PMC_MAIN + 12 = 15    (using 8, wasting 8)
sama7d65:   PMC_INDEX_MAX = 25                  (using 12, wasting 13, defined in source!)
sama7g5:    PMC_MCK1 = PMC_MAIN + 10 = 13       (using 8, wasting 6)

Note: sam9x7 uses `PMC_AUDIOPMCPLL (PMC_MAIN + 6)` and above _and_
PMC_PLLACK which is after or lets say in between the "PMC_MAIN +
branchoff thing", making things even more messy.

I could just add PMC_MAIN_RC = PMC_MAIN + 15 = 18 now, this would be
more or less okay for the newer SoCs I guess, but it would mean 19
array members for sam9x60 of which only five are used, so 14 * 44 byte
wasted.

Over 600 byte wasted for a solution which already is quite messy?!

If there are no objections, I'm going to make this binding splitup now.
I see 2 variants:

1. full splitup per SoC/driver?
2. opportunistic approach keeping the old stuff up to sam9x60 and
sama5, and just create new headers for sam9x7 and shared
sama7g5/sama7d65?

Question is about naming the symbols then.  I guess it would be a bad
idea to define the same thing twice with different values like this?
Have this in one file for sam9x60:

    #define PMC_MAIN_RC 10

and this in a different file for sama7:

    #define PMC_MAIN_RC (PMC_MAIN + 15)


So I rather introduce some new headers including different new
per SoC values like this?  For sam9x60:

    #define SAM9X60_PMC_MAIN_RC 10

For sam9x7:

    #define SAM9X7_PMC_MAIN_RC (PMC_MAIN + 13)

For sama7*:

    #define SAMA7_PMC_MAIN_RC (PMC_MAIN + 15)


And then keep all the old names for now?  Should some definitions be
moved to the new headers already?  Add some comments maybe?  Or
duplicate the old definitions like this?
Keep in generic header:

    #define PMC_MCK3 (PMC_MAIN + 13)

Add in separate header:

    /* same as PMC_MCK3 */
    #define SAMA7_PMC_MCK3 16

I'll come up with something and you can review the patch series then.
Just let me know what you think, I mean Microchip, it's your SoCs and
drivers, I just need the UID on sam9x60, I did not intend to climb
down this rabbit hole in the first place. ;-)

Greets
Alex

> 
> Best,
> Ryan
> > 
> > Another option would be to xlate the clocks not by directly indexing in
> > struct pmc_data::chws but by matching the driver clock ID and DT provided
> > id. This will increase the lookup time, from O(1) to O(N), N being 13 for
> > SAMA7G5, 15 for SAM9X7 and SAMA7D55. And will need adjustment at least for
> > SAM9X{60, 7} and SAMA7{G5, D55}. With this the of_clk_hw_pmc_get() will be
> > something like:
> > 
> > diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> > index 5aa9c1f1c886..22191d1ca78b 100644
> > --- a/drivers/clk/at91/pmc.c
> > +++ b/drivers/clk/at91/pmc.c
> > @@ -52,8 +52,10 @@ struct clk_hw *of_clk_hw_pmc_get(struct of_phandle_args
> > *clkspec, void *data)
> > 
> >         switch (type) {
> >         case PMC_TYPE_CORE:
> > -               if (idx < pmc_data->ncore)
> > -                       return pmc_data->chws[idx];
> > +               for (int i = 0; i < pmc_data->ncore; i++) {
> > +                       if (pmc_data->chws.idx[i] == i)
> > +                               return pmc_data->chws.hws[i];
> > +               }
> >                 break;
> > 
> > 
> > diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
> > index 4fb29ca111f7..f7e88f9872dc 100644
> > --- a/drivers/clk/at91/pmc.h
> > +++ b/drivers/clk/at91/pmc.h
> > @@ -19,7 +19,10 @@ extern spinlock_t pmc_pcr_lock;
> > 
> >  struct pmc_data {
> >         unsigned int ncore;
> > -       struct clk_hw **chws;
> > +       struct {
> > +               struct clk_hw **hws;
> > +               int *idx;
> > +       } chws;
> > 
> > Thank you,
> > Claudiu Beznea
> > 
> >>>
> >>> Now we need a new ID for main rc osc, but not only for SAM9X60, but
> >>> also for SAMA7G5.  I'm not sure what the implications would be, if the
> >>> new ID would be added in between before PMC_MAIN, so all values would
> >>> change?  Adding it to the end of the lists would probably be safe, but
> >>> then you would need a diffently named variant for SAMA7G5's different
> >>> IDs.  I find the current status somewhat unfortunate for future
> >>> extensions.  How should this new ID be added here?  What would be the
> >>> way forward?
> >>>
> >>> Greets
> >>> Alex
> >>>
> >>> [1] https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u
> >>>
> >>>> --
> >>>> 2.39.2
> >>>>
> >>>>
> >>>
> >>
> 
>