| Message ID | 20240821105943.230281-10-ada@thorsis.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Microchip OTPC driver on SAM9X60 exposing UIDxR as additional nvmem device | expand |
On Wed, Aug 21, 2024 at 12:59:40PM +0200, Alexander Dahl wrote: > 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 + Generally we don't want binding changes in the same patch as a driver change. If your fix was determined to be faulty down the line and reverted, the binding change would remain valid, for example. Can you split it into two patches please, for the next version please? > 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) > -- > 2.39.2 >
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 > >
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 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 >> >> >
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 >>> >>> >> >
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@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. 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? 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@microchip.com/T/#u >> >>> -- >>> 2.39.2 >>> >>> >> >
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@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. > > 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@microchip.com/T/#u >>> >>>> -- >>>> 2.39.2 >>>> >>>> >>> >>
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)
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(-)