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@xxxxxxxxxxxxx/T/#u >>> Signed-off-by: Alexander Dahl <ada@xxxxxxxxxxx> >>> --- >>> >>> 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
