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

[Ryan Wanner]

On 9/26/24 00:42, claudiu beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi, Ryan, Alexander,
> 
> Sorry for returning late, I took some time to think about it...
> 
> On 24.09.2024 18: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.
> 
> If I understand correctly, wouldn't this shift also the rest of the IDs
> and break the DT ABI?
> 
>>
>> 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
>> 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@xxxxxxxxxxx>
>>>> ---
>>>>  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.
> 
> Looking at it now, I think it was a bad decision to do this branch.
> Thinking at it maybe it would be better to have per SoC specific bindings
> to avoid this kind of issue in future. The PMC IP b/w different SAM SoCs is
> anyway different and, as it happens now, we may reach to a point where, due
> to issues in datasheet, or whatever human errors, we may reach this problem
> again.
> 
> So, what do you think about having separate binding files for each SoC?

I think the simplest way to do this is having a separate file for the
SAMA7 SoC clock bindings. To me it looks like the split is for the SAMA7
SoCs only, so having a separate file will be the best solution as it
will mean less duplicate code and still keeping the O(1) look up time.

Best,
Ryan
> 
> Another option would be to xlate the clocks not by directly indexing in
> struct pmc_data::chws but by matching the driver clock ID and DT provided
> id. This will increase the lookup time, from O(1) to O(N), N being 13 for
> SAMA7G5, 15 for SAM9X7 and SAMA7D55. And will need adjustment at least for
> SAM9X{60, 7} and SAMA7{G5, D55}. With this the of_clk_hw_pmc_get() will be
> something like:
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 5aa9c1f1c886..22191d1ca78b 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -52,8 +52,10 @@ struct clk_hw *of_clk_hw_pmc_get(struct of_phandle_args
> *clkspec, void *data)
> 
>         switch (type) {
>         case PMC_TYPE_CORE:
> -               if (idx < pmc_data->ncore)
> -                       return pmc_data->chws[idx];
> +               for (int i = 0; i < pmc_data->ncore; i++) {
> +                       if (pmc_data->chws.idx[i] == i)
> +                               return pmc_data->chws.hws[i];
> +               }
>                 break;
> 
> 
> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
> index 4fb29ca111f7..f7e88f9872dc 100644
> --- a/drivers/clk/at91/pmc.h
> +++ b/drivers/clk/at91/pmc.h
> @@ -19,7 +19,10 @@ extern spinlock_t pmc_pcr_lock;
> 
>  struct pmc_data {
>         unsigned int ncore;
> -       struct clk_hw **chws;
> +       struct {
> +               struct clk_hw **hws;
> +               int *idx;
> +       } chws;
> 
> Thank you,
> Claudiu Beznea
> 
>>>
>>> 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@xxxxxxxxxxxxx/T/#u
>>>
>>>> --
>>>> 2.39.2
>>>>
>>>>
>>>
>>