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

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

[PATCH v1 05/12] ARM: dts: microchip: sam9x60: Add OTPC node

[Alexander Dahl]

See datasheet (DS60001579G) sections "7. Memories" and "23. OTP Memory
Controller (OTPC)" for reference.

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

diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
index 291540e5d81e..2159a6817f44 100644
--- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
+++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
@@ -14,6 +14,7 @@
 #include <dt-bindings/clock/at91.h>
 #include <dt-bindings/mfd/at91-usart.h>
 #include <dt-bindings/mfd/atmel-flexcom.h>
+#include <dt-bindings/nvmem/microchip,sama7g5-otpc.h>
 
 / {
 	#address-cells = <1>;
@@ -156,6 +157,13 @@ sdmmc1: sdio-host@90000000 {
 			status = "disabled";
 		};
 
+		otpc: efuse@eff00000 {
+			compatible = "microchip,sam9x60-otpc", "syscon";
+			reg = <0xeff00000 0xec>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+		};
+
 		apb {
 			compatible = "simple-bus";
 			#address-cells = <1>;
-- 
2.39.2

Re: [PATCH v1 05/12] ARM: dts: microchip: sam9x60: Add OTPC node

[claudiu beznea]

On 21.08.2024 13:59, Alexander Dahl wrote:
> See datasheet (DS60001579G) sections "7. Memories" and "23. OTP Memory
> Controller (OTPC)" for reference.

Please detail here what the patch does and why it is necessary. Sending the
reader to some DS chapters w/o any additional information may be worthless.

> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  arch/arm/boot/dts/microchip/sam9x60.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> index 291540e5d81e..2159a6817f44 100644
> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> @@ -14,6 +14,7 @@
>  #include <dt-bindings/clock/at91.h>
>  #include <dt-bindings/mfd/at91-usart.h>
>  #include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <dt-bindings/nvmem/microchip,sama7g5-otpc.h>
>  
>  / {
>  	#address-cells = <1>;
> @@ -156,6 +157,13 @@ sdmmc1: sdio-host@90000000 {
>  			status = "disabled";
>  		};
>  
> +		otpc: efuse@eff00000 {
> +			compatible = "microchip,sam9x60-otpc", "syscon";
> +			reg = <0xeff00000 0xec>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +		};
> +
>  		apb {
>  			compatible = "simple-bus";
>  			#address-cells = <1>;

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

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

[PATCH v1 10/12] ARM: dts: microchip: sam9x60: Add clock properties to OTPC

[Alexander Dahl]

This will allow to enable the main RC Oscillator from
nvmem_microchip_otpc driver.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---

Notes:
    This requires some DT bindings update, right?

 arch/arm/boot/dts/microchip/sam9x60.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
index 2159a6817f44..4f0651b8cb60 100644
--- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
+++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
@@ -162,6 +162,8 @@ otpc: efuse@eff00000 {
 			reg = <0xeff00000 0xec>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			clocks = <&pmc PMC_TYPE_CORE 11>;
+			clock-names = "main_rc_osc";
 		};
 
 		apb {
-- 
2.39.2

Re: [PATCH v1 10/12] ARM: dts: microchip: sam9x60: Add clock properties to OTPC

[claudiu beznea]

On 21.08.2024 13:59, Alexander Dahl wrote:
> This will allow to enable the main RC Oscillator from
> nvmem_microchip_otpc driver.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
> 
> Notes:
>     This requires some DT bindings update, right?

Yes

> 
>  arch/arm/boot/dts/microchip/sam9x60.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> index 2159a6817f44..4f0651b8cb60 100644
> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> @@ -162,6 +162,8 @@ otpc: efuse@eff00000 {
>  			reg = <0xeff00000 0xec>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> +			clocks = <&pmc PMC_TYPE_CORE 11>;
> +			clock-names = "main_rc_osc";

And this should be squashed w/ patch 05/12.

>  		};
>  
>  		apb {

Re: [PATCH v1 10/12] ARM: dts: microchip: sam9x60: Add clock properties to OTPC

[Alexander Dahl]

Hello Claudiu,

Am Sat, Aug 24, 2024 at 06:57:23PM +0300 schrieb claudiu beznea:
> 
> 
> On 21.08.2024 13:59, Alexander Dahl wrote:
> > This will allow to enable the main RC Oscillator from
> > nvmem_microchip_otpc driver.
> > 
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> > 
> > Notes:
> >     This requires some DT bindings update, right?
> 
> Yes

Okay, I suspected that already.

This raises the question if the main rc osc clock should be mandatory
for SAMA7G5 too, or if SAM9X60 and SAMA7G5 should be handled
differently with regard to that clock?  Affects both binding and
driver then, see NOTE in patch 11, which is directly related to that
question.

> 
> > 
> >  arch/arm/boot/dts/microchip/sam9x60.dtsi | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> > index 2159a6817f44..4f0651b8cb60 100644
> > --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
> > +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> > @@ -162,6 +162,8 @@ otpc: efuse@eff00000 {
> >  			reg = <0xeff00000 0xec>;
> >  			#address-cells = <1>;
> >  			#size-cells = <1>;
> > +			clocks = <&pmc PMC_TYPE_CORE 11>;
> > +			clock-names = "main_rc_osc";
> 
> And this should be squashed w/ patch 05/12.

Okay, this may require patch reordering, we'll see.

Greets
Alex

> 
> >  		};
> >  
> >  		apb {

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