On Sat, Jun 3, 2023, at 23:23, Conor Dooley wrote:
> On Sat, Jun 03, 2023 at 10:19:50PM +0100, Conor Dooley wrote:
>> Hey Varshini,
>>
>> On Sun, Jun 04, 2023 at 01:32:37AM +0530, Varshini Rajendran wrote:
>> > Document the support added for the Advanced interrupt controller(AIC)
>> > chip in the sam9x7 soc family
>>
>> Please do not add new family based compatibles, but rather use per-soc
>> compatibles instead.
>
> These things leave me penally confused. Afaiu, sam9x60 is a particular
> SoC. sam9x7 is actually a family, containing sam9x70, sam9x72 and
> sam9x75. It would appear to me that each should have its own compatible,
> no?

I think the usual way this works is that the sam9x7 refers to the
SoC design as in what is actually part of the chip, whereas the 70,
72 and 75 models are variants that have a certain subset of the
features enabled.

If that is the case here, then referring to the on-chip parts by
the sam9x7 name makes sense, and this is similar to what we do
on TI AM-series chips.

There is a remaining risk that a there would be a future
sam9x71/73/74/76/... product based on a new chip that uses
incompatible devices, but at that point we can still use the
more specific model number to identify those without being
ambiguous. The same thing can of course happen when a SoC
vendor reuses a specific name of a prior product with an update
chip that has software visible changes.

I'd just leave this up to Varshini and the other at91 maintainers
here, provided they understand the exact risks.

It's different for the parts that are listed as just sam9x60
compatible in the DT, I think those clearly need to have sam9x7
in the compatible list, but could have the sam9x60 identifier
as a fallback if the hardware is compatible.

Arnd

2023-06-04 18:32:37

by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH 14/21] clk: at91: sam9x7: add sam9x7 pmc driver

On Sun, Jun 04, 2023 at 01:32:36AM +0530, Varshini Rajendran wrote:
> Add a driver for the PMC clocks of sam9x7 Soc family
>
> Signed-off-by: Varshini Rajendran <[email protected]>

...

> +static void __init sam9x7_pmc_setup(struct device_node *np)
> +{
> + struct clk_range range = CLK_RANGE(0, 0);
> + const char *td_slck_name, *md_slck_name, *mainxtal_name;
> + struct pmc_data *sam9x7_pmc;
> + const char *parent_names[9];
> + void **alloc_mem = NULL;
> + int alloc_mem_size = 0;
> + struct clk_hw *main_osc_hw;
> + struct regmap *regmap;
> + struct clk_hw *hw;
> + int i, j;
> +
> + i = of_property_match_string(np, "clock-names", "td_slck");
> + if (i < 0)
> + return;
> +
> + td_slck_name = of_clk_get_parent_name(np, i);
> +
> + i = of_property_match_string(np, "clock-names", "md_slck");
> + if (i < 0)
> + return;
> +
> + md_slck_name = of_clk_get_parent_name(np, i);
> +
> + i = of_property_match_string(np, "clock-names", "main_xtal");
> + if (i < 0)
> + return;
> + mainxtal_name = of_clk_get_parent_name(np, i);
> +
> + regmap = device_node_to_regmap(np);
> + if (IS_ERR(regmap))
> + return;
> +
> + sam9x7_pmc = pmc_data_allocate(PMC_PLLACK + 1,
> + nck(sam9x7_systemck),
> + nck(sam9x7_periphck),
> + nck(sam9x7_gck), 8);
> + if (!sam9x7_pmc)
> + return;
> +
> + alloc_mem = kmalloc(sizeof(void *) *
> + (ARRAY_SIZE(sam9x7_gck)),
> + GFP_KERNEL);
> + if (!alloc_mem)
> + goto err_free;
> +
> + hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
> + 50000000);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, 0);
> + if (IS_ERR(hw))
> + goto err_free;
> + main_osc_hw = hw;
> +
> + parent_names[0] = "main_rc_osc";
> + parent_names[1] = "main_osc";
> + hw = at91_clk_register_sam9x5_main(regmap, "mainck", parent_names, 2);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + sam9x7_pmc->chws[PMC_MAIN] = hw;
> +
> + for (i = 0; i < PLL_ID_MAX; i++) {
> + for (j = 0; j < 3; j++) {
> + struct clk_hw *parent_hw;
> +
> + if (!sam9x7_plls[i][j].n)
> + continue;
> +
> + switch (sam9x7_plls[i][j].t) {
> + case PLL_TYPE_FRAC:
> + if (!strcmp(sam9x7_plls[i][j].p, "mainck"))
> + parent_hw = sam9x7_pmc->chws[PMC_MAIN];
> + else if (!strcmp(sam9x7_plls[i][j].p, "main_osc"))
> + parent_hw = main_osc_hw;
> + else
> + parent_hw = __clk_get_hw(of_clk_get_by_name
> + (np, sam9x7_plls[i][j].p));
> +
> + hw = sam9x60_clk_register_frac_pll(regmap,
> + &pmc_pll_lock,
> + sam9x7_plls[i][j].n,
> + sam9x7_plls[i][j].p,
> + parent_hw, i,
> + sam9x7_plls[i][j].c,
> + sam9x7_plls[i][j].l,
> + sam9x7_plls[i][j].f);
> + break;
> +
> + case PLL_TYPE_DIV:
> + hw = sam9x60_clk_register_div_pll(regmap,
> + &pmc_pll_lock,
> + sam9x7_plls[i][j].n,
> + sam9x7_plls[i][j].p, i,
> + sam9x7_plls[i][j].c,
> + sam9x7_plls[i][j].l,
> + sam9x7_plls[i][j].f, 0);
> + break;
> +
> + default:
> + continue;
> + }
> +
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + if (sam9x7_plls[i][j].eid)
> + sam9x7_pmc->chws[sam9x7_plls[i][j].eid] = hw;
> + }
> + }
> +
> + parent_names[0] = md_slck_name;
> + parent_names[1] = "mainck";
> + parent_names[2] = "plla_divpmcck";
> + parent_names[3] = "upll_divpmcck";
> + hw = at91_clk_register_master_pres(regmap, "masterck_pres", 4,
> + parent_names, &sam9x7_master_layout,
> + &mck_characteristics, &mck_lock);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + hw = at91_clk_register_master_div(regmap, "masterck_div",
> + "masterck_pres", &sam9x7_master_layout,
> + &mck_characteristics, &mck_lock,
> + CLK_SET_RATE_GATE, 0);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + sam9x7_pmc->chws[PMC_MCK] = hw;
> +
> + parent_names[0] = "plla_divpmcck";
> + parent_names[1] = "upll_divpmcck";
> + parent_names[2] = "main_osc";
> + hw = sam9x60_clk_register_usb(regmap, "usbck", parent_names, 3);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + parent_names[0] = md_slck_name;
> + parent_names[1] = td_slck_name;
> + parent_names[2] = "mainck";
> + parent_names[3] = "masterck_div";
> + parent_names[4] = "plla_divpmcck";
> + parent_names[5] = "upll_divpmcck";
> + parent_names[6] = "audiopll_divpmcck";
> + for (i = 0; i < 2; i++) {
> + char name[6];
> +
> + snprintf(name, sizeof(name), "prog%d", i);
> +
> + hw = at91_clk_register_programmable(regmap, name,
> + parent_names, 7, i,
> + &sam9x7_programmable_layout,
> + NULL);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + sam9x7_pmc->pchws[i] = hw;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(sam9x7_systemck); i++) {
> + hw = at91_clk_register_system(regmap, sam9x7_systemck[i].n,
> + sam9x7_systemck[i].p,
> + sam9x7_systemck[i].id,
> + sam9x7_systemck[i].flags);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + sam9x7_pmc->shws[sam9x7_systemck[i].id] = hw;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(sam9x7_periphck); i++) {
> + hw = at91_clk_register_sam9x5_peripheral(regmap, &pmc_pcr_lock,
> + &sam9x7_pcr_layout,
> + sam9x7_periphck[i].n,
> + "masterck_div",
> + sam9x7_periphck[i].id,
> + &range, INT_MIN,
> + sam9x7_periphck[i].f);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + sam9x7_pmc->phws[sam9x7_periphck[i].id] = hw;
> + }
> +
> + parent_names[0] = md_slck_name;
> + parent_names[1] = td_slck_name;
> + parent_names[2] = "mainck";
> + parent_names[3] = "masterck_div";
> + for (i = 0; i < ARRAY_SIZE(sam9x7_gck); i++) {
> + u8 num_parents = 4 + sam9x7_gck[i].pp_count;
> + u32 *mux_table;
> +
> + mux_table = kmalloc_array(num_parents, sizeof(*mux_table),
> + GFP_KERNEL);
> + if (!mux_table)
> + goto err_free;
> +
> + SAM9X7_INIT_TABLE(mux_table, 4);
> + SAM9X7_FILL_TABLE(&mux_table[4], sam9x7_gck[i].pp_mux_table,
> + sam9x7_gck[i].pp_count);
> + SAM9X7_FILL_TABLE(&parent_names[4], sam9x7_gck[i].pp,
> + sam9x7_gck[i].pp_count);
> +
> + hw = at91_clk_register_generated(regmap, &pmc_pcr_lock,
> + &sam9x7_pcr_layout,
> + sam9x7_gck[i].n,
> + parent_names, mux_table,
> + num_parents,
> + sam9x7_gck[i].id,
> + &sam9x7_gck[i].r,
> + sam9x7_gck[i].pp_chg_id);
> + if (IS_ERR(hw))
> + goto err_free;
> +
> + sam9x7_pmc->ghws[sam9x7_gck[i].id] = hw;
> + alloc_mem[alloc_mem_size++] = mux_table;
> + }
> +
> + of_clk_add_hw_provider(np, of_clk_hw_pmc_get, sam9x7_pmc);
> +

Hi Varshini,

alloc_mem appears to be leaked here.

> + return;
> +
> +err_free:
> + if (alloc_mem) {
> + for (i = 0; i < alloc_mem_size; i++)
> + kfree(alloc_mem[i]);
> + kfree(alloc_mem);
> + }
> + kfree(sam9x7_pmc);
> +}
> +
> +/* Some clks are used for a clocksource */
> +CLK_OF_DECLARE(sam9x7_pmc, "microchip,sam9x7-pmc", sam9x7_pmc_setup);

I'm sure I'm missing some thing obvious, but I was unable to
find the binding for "microchip,sam9x7-pmc".

2023-06-04 21:48:25

On 05/06/2023 15:04, Nicolas Ferre wrote:
> On 05/06/2023 at 08:43, Krzysztof Kozlowski wrote:
>> On 03/06/2023 22:02, Varshini Rajendran wrote:
>>> Use sam9x7 pmc's compatible to lookup for in the SHDWC driver
>>>
>>> Signed-off-by: Varshini Rajendran <[email protected]>
>>> ---
>>> drivers/power/reset/at91-sama5d2_shdwc.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
>>> index d8ecffe72f16..d0f29b99f25e 100644
>>> --- a/drivers/power/reset/at91-sama5d2_shdwc.c
>>> +++ b/drivers/power/reset/at91-sama5d2_shdwc.c
>>> @@ -326,6 +326,7 @@ static const struct of_device_id at91_pmc_ids[] = {
>>> { .compatible = "atmel,sama5d2-pmc" },
>>> { .compatible = "microchip,sam9x60-pmc" },
>>> { .compatible = "microchip,sama7g5-pmc" },
>>> + { .compatible = "microchip,sam9x7-pmc" },
>>
>> Why do you need new entry if these are compatible?
>
> Yes, PMC is very specific to a SoC silicon. As we must look for it in
> the shutdown controller, I think we need a new entry here.

??? How does it answer to my question at all? What is exactly specific
which warrants new entry?

Best regards,
Krzysztof

2023-06-05 14:07:03

On 05/06/23 6:56 pm, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> Hey,
>
> On Mon, Jun 05, 2023 at 03:04:34PM +0200, Nicolas Ferre wrote:
>> On 05/06/2023 at 08:43, Krzysztof Kozlowski wrote:
>>> On 03/06/2023 22:02, Varshini Rajendran wrote:
>>>> Use sam9x7 pmc's compatible to lookup for in the SHDWC driver
>>>>
>>>> Signed-off-by: Varshini Rajendran <[email protected]>
>>>> ---
>>>> drivers/power/reset/at91-sama5d2_shdwc.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
>>>> index d8ecffe72f16..d0f29b99f25e 100644
>>>> --- a/drivers/power/reset/at91-sama5d2_shdwc.c
>>>> +++ b/drivers/power/reset/at91-sama5d2_shdwc.c
>>>> @@ -326,6 +326,7 @@ static const struct of_device_id at91_pmc_ids[] = {
>>>> { .compatible = "atmel,sama5d2-pmc" },
>>>> { .compatible = "microchip,sam9x60-pmc" },
>>>> { .compatible = "microchip,sama7g5-pmc" },
>>>> + { .compatible = "microchip,sam9x7-pmc" },
>>>
>>> Why do you need new entry if these are compatible?
>>
>> Yes, PMC is very specific to a SoC silicon. As we must look for it in the
>> shutdown controller, I think we need a new entry here.
>
> Copy-pasting this for a wee bit of context as I have two questions.
>
> | static const struct of_device_id at91_shdwc_of_match[] = {
> | {
> | .compatible = "atmel,sama5d2-shdwc",
> | .data = &sama5d2_reg_config,
> | },
> | {
> | .compatible = "microchip,sam9x60-shdwc",
> | .data = &sam9x60_reg_config,
> | },
> | {
> | .compatible = "microchip,sama7g5-shdwc",
> | .data = &sama7g5_reg_config,
> | }, {
> | /*sentinel*/
> | }
> | };
> | MODULE_DEVICE_TABLE(of, at91_shdwc_of_match);
> |
> | static const struct of_device_id at91_pmc_ids[] = {
> | { .compatible = "atmel,sama5d2-pmc" },
> | { .compatible = "microchip,sam9x60-pmc" },
> | { .compatible = "microchip,sama7g5-pmc" },
> | { .compatible = "microchip,sam9x7-pmc" },
> | { /* Sentinel. */ }
> | };
>
> If there's no changes made to the code, other than adding an entry to
> the list of pmc compatibles, then either this has the same as an
> existing SoC, or there is a bug in the patch, since the behaviour of
> the driver will not have changed.
>
> Secondly, this patch only updates the at91_pmc_ids and the dts patch
> contains:
> | shutdown_controller: shdwc@fffffe10 {
> | compatible = "microchip,sam9x60-shdwc";
> | reg = <0xfffffe10 0x10>;
> | clocks = <&clk32k 0>;
> | #address-cells = <1>;
> | #size-cells = <0>;
> | atmel,wakeup-rtc-timer;
> | atmel,wakeup-rtt-timer;
> | status = "disabled";
> | };
>
> ...which would mean that the there's nothing different between the
> programming models for the sam9x60 and sam9x7. If that's the case, the
> dt-binding & dts should list the sam9x60 as a fallback for the sam9x7 &
> there is no change required to the driver. If it's not the case, then
> there's a bug in this patch and the dts one ????
>
> In general, if things are the same as previous products, there's no need
> to change the drivers at all & just add fallback compatibles to the
> bindings and dts. IFF some difference pops up in the future, then the
> sam9x7 compatible will already exist in the dts, and can then be added
> to the driver.

Yes. I totally agree with you. In this patch I have not added a
compatible for the shutdown controller for sam9x7. I have added the
compatible of the pmc used in sam9x7 in the list of compatibles to use
the right pmc driver in order to control the clk disable functions for
the right pmc. The shutdown programming is no different, so no new
compatible for sam9x7 (like microchip,sam9x7-shdwc). But the PMC is
totally different than the other older SoCs, hence I have added the new
compatible microchip,sam9x7-pmc in the list as it is defined in the
drivers/clk/at91/sam9x7.c driver. Hope this is clear.

>
> Cheers,
> Conor.
>

--
Thanks and Regards,
Varshini Rajendran.