Re: [PATCH v2 08/16] dt-bindings: nvmem: microchip-otpc: Add required clocks

[Krzysztof Kozlowski <krzk@kernel.org>]

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