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

Krzysztof Kozlowski
2025-02-11T07:50:24Z

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