Re: [PATCH v1 09/12] clk: at91: sam9x60: Allow enabling main_rc_osc through DT

[Nicolas Ferre <nicolas.ferre@microchip.com>]

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