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