| Message ID | 20250210164506.495747-9-ada@thorsis.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Microchip OTPC driver on SAM9X60 exposing UIDxR as additional nvmem device | expand |
On 10/02/2025 17:44, Alexander Dahl wrote: > The OTPC requires both the peripheral clock through PMC and the main RC > oscillator. Seemed to work without explicitly enabling those clocks on > sama7g5 before, but did not on sam9x60. > > Older datasheets were not clear and explicit about this, but recent are, > e.g. SAMA7G5 series datasheet (DS60001765B), > section 30.4.1 Power Management: > >> 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. > > Link: https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u > Signed-off-by: Alexander Dahl <ada@thorsis.com> > --- > > Notes: > v2: > - new patch, not present in v1 > > .../nvmem/microchip,sama7g5-otpc.yaml | 28 +++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml > index 9a7aaf64eef32..1fa40610888f3 100644 > --- a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml > +++ b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml > @@ -29,6 +29,16 @@ properties: > reg: > maxItems: 1 > > + clocks: > + items: > + - description: main rc oscillator > + - description: otpc peripheral clock > + > + clock-names: > + items: > + - const: main_rc_osc osc > + - const: otpc_clk otpc or bus or whatever logically this is > + > required: > - compatible > - reg > @@ -37,6 +47,8 @@ unevaluatedProperties: false > > examples: > - | > + #include <dt-bindings/clock/at91.h> > + #include <dt-bindings/clock/microchip,sama7g5-pmc.h> > #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h> > > otpc: efuse@e8c00000 { > @@ -44,10 +56,26 @@ examples: > reg = <0xe8c00000 0xec>; > #address-cells = <1>; > #size-cells = <1>; > + clocks = <&pmc PMC_TYPE_CORE SAMA7G5_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 67>; > + clock-names = "main_rc_osc", "otpc_clk"; > > temperature_calib: calib@1 { > reg = <OTP_PKT(1) 76>; > }; > }; > > + - | > + #include <dt-bindings/clock/at91.h> > + #include <dt-bindings/clock/microchip,sam9x60-pmc.h> > + #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h> > + > + efuse@eff00000 { > + compatible = "microchip,sam9x60-otpc", "syscon"; > + reg = <0xeff00000 0xec>; No need for new example with difference in what exactly? Even compatible was not added here... > + #address-cells = <1>; > + #size-cells = <1>; > + clocks = <&pmc PMC_TYPE_CORE SAM9X60_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 46>; > + clock-names = "main_rc_osc", "otpc_clk"; > + }; > + > ... Best regards, Krzysztof
Hello Krzysztof, Am Mon, Feb 10, 2025 at 05:59:52PM +0100 schrieb Krzysztof Kozlowski: > On 10/02/2025 17:44, Alexander Dahl wrote: > > The OTPC requires both the peripheral clock through PMC and the main RC > > oscillator. Seemed to work without explicitly enabling those clocks on > > sama7g5 before, but did not on sam9x60. > > > > Older datasheets were not clear and explicit about this, but recent are, > > e.g. SAMA7G5 series datasheet (DS60001765B), > > section 30.4.1 Power Management: > > > >> 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. > > > > Link: https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u > > Signed-off-by: Alexander Dahl <ada@thorsis.com> > > --- > > > > Notes: > > v2: > > - new patch, not present in v1 > > > > .../nvmem/microchip,sama7g5-otpc.yaml | 28 +++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml > > index 9a7aaf64eef32..1fa40610888f3 100644 > > --- a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml > > +++ b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml > > @@ -29,6 +29,16 @@ properties: > > reg: > > maxItems: 1 > > > > + clocks: > > + items: > > + - description: main rc oscillator > > + - description: otpc peripheral clock > > + > > + clock-names: > > + items: > > + - const: main_rc_osc > > osc On at91 SoCs main oscillator and main RC oscillator are two different things, and those are different clocks in Linux as well. This clock is named "main_rc_osc" in the clock driver. In drivers/clk/at91/sam9x60.c this clock is added like this: hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000, 50000000); The datasheet makes it explicit, it's exactly the main rc oscillator clock required for the OTPC to work, no other clock. So why name this "osc" only then? This is confusing at best. > > > + - const: otpc_clk > > otpc or bus or whatever logically this is Okay the "_clk" suffix is redundant. Since the peripheral clock for the OTPC is required here, I would go with "otpc" only then. > > > + > > required: > > - compatible > > - reg > > @@ -37,6 +47,8 @@ unevaluatedProperties: false > > > > examples: > > - | > > + #include <dt-bindings/clock/at91.h> > > + #include <dt-bindings/clock/microchip,sama7g5-pmc.h> > > #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h> > > > > otpc: efuse@e8c00000 { > > @@ -44,10 +56,26 @@ examples: > > reg = <0xe8c00000 0xec>; > > #address-cells = <1>; > > #size-cells = <1>; > > + clocks = <&pmc PMC_TYPE_CORE SAMA7G5_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 67>; > > + clock-names = "main_rc_osc", "otpc_clk"; > > > > temperature_calib: calib@1 { > > reg = <OTP_PKT(1) 76>; > > }; > > }; > > > > + - | > > + #include <dt-bindings/clock/at91.h> > > + #include <dt-bindings/clock/microchip,sam9x60-pmc.h> > > + #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h> > > + > > + efuse@eff00000 { > > + compatible = "microchip,sam9x60-otpc", "syscon"; > > + reg = <0xeff00000 0xec>; > > No need for new example with difference in what exactly? Even compatible > was not added here... Different compatible, different clocks, no sub nodes, different peripheral clock id … From a human doc readers I'd like another example, but fine, we can drop it if it adds too much redundancy. Greets Alex > > > + #address-cells = <1>; > > + #size-cells = <1>; > > + clocks = <&pmc PMC_TYPE_CORE SAM9X60_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 46>; > > + clock-names = "main_rc_osc", "otpc_clk"; > > + }; > > + > > ... > > > Best regards, > Krzysztof
On 11/02/2025 08:05, Alexander Dahl wrote: > Hello Krzysztof, > > Am Mon, Feb 10, 2025 at 05:59:52PM +0100 schrieb Krzysztof Kozlowski: >> On 10/02/2025 17:44, Alexander Dahl wrote: >>> The OTPC requires both the peripheral clock through PMC and the main RC >>> oscillator. Seemed to work without explicitly enabling those clocks on >>> sama7g5 before, but did not on sam9x60. >>> >>> Older datasheets were not clear and explicit about this, but recent are, >>> e.g. SAMA7G5 series datasheet (DS60001765B), >>> section 30.4.1 Power Management: >>> >>>> 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. >>> >>> Link: https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u >>> Signed-off-by: Alexander Dahl <ada@thorsis.com> >>> --- >>> >>> Notes: >>> v2: >>> - new patch, not present in v1 >>> >>> .../nvmem/microchip,sama7g5-otpc.yaml | 28 +++++++++++++++++++ >>> 1 file changed, 28 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml >>> index 9a7aaf64eef32..1fa40610888f3 100644 >>> --- a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml >>> +++ b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml >>> @@ -29,6 +29,16 @@ properties: >>> reg: >>> maxItems: 1 >>> >>> + clocks: >>> + items: >>> + - description: main rc oscillator >>> + - description: otpc peripheral clock >>> + >>> + clock-names: >>> + items: >>> + - const: main_rc_osc >> >> osc > > On at91 SoCs main oscillator and main RC oscillator are two different > things, and those are different clocks in Linux as well. This clock > is named "main_rc_osc" in the clock driver. In It does not matter, you could have 5 oscillators. This device takes one. What is this clock from the point of view of device? > drivers/clk/at91/sam9x60.c this clock is added like this: > > hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000, 50000000); > > The datasheet makes it explicit, it's exactly the main rc oscillator > clock required for the OTPC to work, no other clock. You speak about source clock but this has to be the clock here, in the device context. > > So why name this "osc" only then? This is confusing at best. > >> >>> + - const: otpc_clk >> >> otpc or bus or whatever logically this is > > Okay the "_clk" suffix is redundant. Since the peripheral clock for > the OTPC is required here, I would go with "otpc" only then. > >> >>> + >>> required: >>> - compatible >>> - reg >>> @@ -37,6 +47,8 @@ unevaluatedProperties: false >>> >>> examples: >>> - | >>> + #include <dt-bindings/clock/at91.h> >>> + #include <dt-bindings/clock/microchip,sama7g5-pmc.h> >>> #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h> >>> >>> otpc: efuse@e8c00000 { >>> @@ -44,10 +56,26 @@ examples: >>> reg = <0xe8c00000 0xec>; >>> #address-cells = <1>; >>> #size-cells = <1>; >>> + clocks = <&pmc PMC_TYPE_CORE SAMA7G5_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 67>; >>> + clock-names = "main_rc_osc", "otpc_clk"; >>> >>> temperature_calib: calib@1 { >>> reg = <OTP_PKT(1) 76>; >>> }; >>> }; >>> >>> + - | >>> + #include <dt-bindings/clock/at91.h> >>> + #include <dt-bindings/clock/microchip,sam9x60-pmc.h> >>> + #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h> >>> + >>> + efuse@eff00000 { >>> + compatible = "microchip,sam9x60-otpc", "syscon"; >>> + reg = <0xeff00000 0xec>; >> >> No need for new example with difference in what exactly? Even compatible >> was not added here... > > Different compatible, different clocks, no sub nodes, different > peripheral clock id … From a human doc readers I'd like another Clocks are the same. Their actual value does not matter. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml index 9a7aaf64eef32..1fa40610888f3 100644 --- a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml +++ b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml @@ -29,6 +29,16 @@ properties: reg: maxItems: 1 + clocks: + items: + - description: main rc oscillator + - description: otpc peripheral clock + + clock-names: + items: + - const: main_rc_osc + - const: otpc_clk + required: - compatible - reg @@ -37,6 +47,8 @@ unevaluatedProperties: false examples: - | + #include <dt-bindings/clock/at91.h> + #include <dt-bindings/clock/microchip,sama7g5-pmc.h> #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h> otpc: efuse@e8c00000 { @@ -44,10 +56,26 @@ examples: reg = <0xe8c00000 0xec>; #address-cells = <1>; #size-cells = <1>; + clocks = <&pmc PMC_TYPE_CORE SAMA7G5_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 67>; + clock-names = "main_rc_osc", "otpc_clk"; temperature_calib: calib@1 { reg = <OTP_PKT(1) 76>; }; }; + - | + #include <dt-bindings/clock/at91.h> + #include <dt-bindings/clock/microchip,sam9x60-pmc.h> + #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h> + + efuse@eff00000 { + compatible = "microchip,sam9x60-otpc", "syscon"; + reg = <0xeff00000 0xec>; + #address-cells = <1>; + #size-cells = <1>; + clocks = <&pmc PMC_TYPE_CORE SAM9X60_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 46>; + clock-names = "main_rc_osc", "otpc_clk"; + }; + ...