| Message ID | 20230603200243.243878-1-varshini.rajendran@microchip.com |
|---|---|
| Headers | show |
| Series | Add support for sam9x7 SoC family | expand |
Hey Varshini, On Sun, Jun 04, 2023 at 01:32:25AM +0530, Varshini Rajendran wrote: > Document the property clock-names in the schema. > > It fixes the dtbs_warning, s/dtbs_warning/dtbs_check warning/? > 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' Does this fix a warning currently in the tree, or fix a warning introduced by some patches in this series? (Or both?) Cheers, Conor. > > Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> > --- > Documentation/devicetree/bindings/usb/generic-ehci.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/generic-ehci.yaml b/Documentation/devicetree/bindings/usb/generic-ehci.yaml > index 7e486cc6cfb8..542ac26960fc 100644 > --- a/Documentation/devicetree/bindings/usb/generic-ehci.yaml > +++ b/Documentation/devicetree/bindings/usb/generic-ehci.yaml > @@ -102,6 +102,10 @@ properties: > - if a USB DRD channel: first clock should be host and second > one should be peripheral > > + clock-names: > + minItems: 1 > + maxItems: 4 > + > power-domains: > maxItems: 1 > > -- > 2.25.1 >
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 <varshini.rajendran@microchip.com> ... > +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".
On 03/06/2023 22:02, Varshini Rajendran wrote: > Add sam9x60 compatible string support in the schema file > > Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> > --- > .../devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml b/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml > index a46411149571..c70c77a5e8e5 100644 > --- a/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml > +++ b/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml > @@ -20,6 +20,7 @@ properties: > - atmel,at91rm9200-tcb > - atmel,at91sam9x5-tcb > - atmel,sama5d2-tcb > + - microchip,sam9x60-tcb No wildcards. Best regards, Krzysztof
On 03/06/2023 22:02, Varshini Rajendran wrote: > Document the property clock-names in the schema. > > It fixes the dtbs_warning, > 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' You cut too much from the warning. Which target/board? > > Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> > --- > Documentation/devicetree/bindings/usb/generic-ehci.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/generic-ehci.yaml b/Documentation/devicetree/bindings/usb/generic-ehci.yaml > index 7e486cc6cfb8..542ac26960fc 100644 > --- a/Documentation/devicetree/bindings/usb/generic-ehci.yaml > +++ b/Documentation/devicetree/bindings/usb/generic-ehci.yaml > @@ -102,6 +102,10 @@ properties: > - if a USB DRD channel: first clock should be host and second > one should be peripheral > > + clock-names: > + minItems: 1 > + maxItems: 4 Not really, because we want them to be fixed, so you need to list the items. But it seems this is not needed at all... which boards and drivers use names? Best regards, Krzysztof
On 03/06/2023 22:02, Varshini Rajendran wrote: > Add documentation for sam9x7 ethernet interface > > Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> > --- > Documentation/devicetree/bindings/net/cdns,macb.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml > index bef5e0f895be..e4f9e9b353e5 100644 > --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml > +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml > @@ -54,6 +54,7 @@ properties: > - cdns,np4-macb # NP4 SoC devices > - microchip,sama7g5-emac # Microchip SAMA7G5 ethernet interface > - microchip,sama7g5-gem # Microchip SAMA7G5 gigabit ethernet interface > + - microchip,sam9x7-gem # Microchip SAM9X7 gigabit ethernet interface No wildcards in compatibles. Best regards, Krzysztof
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 <varshini.rajendran@microchip.com> > --- > 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? Best regards, Krzysztof
On Mon, Jun 5, 2023, at 08:35, Krzysztof Kozlowski wrote: > On 03/06/2023 22:02, Varshini Rajendran wrote: >> Add sam9x60 compatible string support in the schema file >> >> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> >> --- >> .../devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml b/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml >> index a46411149571..c70c77a5e8e5 100644 >> --- a/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml >> +++ b/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml >> @@ -20,6 +20,7 @@ properties: >> - atmel,at91rm9200-tcb >> - atmel,at91sam9x5-tcb >> - atmel,sama5d2-tcb >> + - microchip,sam9x60-tcb > > No wildcards. sam9x60 is the actual name of the chip, it's no wildcard. For sam9x70, sam9x72 and sam9x75, I think using sam9x7 as the compatible string is probably fine, as long as they are actually the same chip. Again, the 'x' in there is not a wildcard but part of the name. Arnd
On 05/06/2023 09:04, Arnd Bergmann wrote: > On Mon, Jun 5, 2023, at 08:35, Krzysztof Kozlowski wrote: >> On 03/06/2023 22:02, Varshini Rajendran wrote: >>> Add sam9x60 compatible string support in the schema file >>> >>> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> >>> --- >>> .../devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml b/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml >>> index a46411149571..c70c77a5e8e5 100644 >>> --- a/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml >>> +++ b/Documentation/devicetree/bindings/soc/microchip/atmel,at91rm9200-tcb.yaml >>> @@ -20,6 +20,7 @@ properties: >>> - atmel,at91rm9200-tcb >>> - atmel,at91sam9x5-tcb >>> - atmel,sama5d2-tcb >>> + - microchip,sam9x60-tcb >> >> No wildcards. > > sam9x60 is the actual name of the chip, it's no wildcard. For sam9x70, > sam9x72 and sam9x75, I think using sam9x7 as the compatible string > is probably fine, as long as they are actually the same chip. Again, > the 'x' in there is not a wildcard but part of the name. OK, if that's the case. Best regards, Krzysztof
On 03/06/2023 at 23:15, Conor Dooley wrote: > Hey Varshini, > > On Sun, Jun 04, 2023 at 01:32:25AM +0530, Varshini Rajendran wrote: >> Document the property clock-names in the schema. >> >> It fixes the dtbs_warning, > s/dtbs_warning/dtbs_check warning/? > >> 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' > Does this fix a warning currently in the tree, or fix a warning > introduced by some patches in this series? (Or both?) Our USB DT pattern is the same on all our newer SoC, to it mustn't be introduced by the addition of this one. Best regards, Nicolas >> Signed-off-by: Varshini Rajendran<varshini.rajendran@microchip.com> >> --- >> Documentation/devicetree/bindings/usb/generic-ehci.yaml | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/usb/generic-ehci.yaml b/Documentation/devicetree/bindings/usb/generic-ehci.yaml >> index 7e486cc6cfb8..542ac26960fc 100644 >> --- a/Documentation/devicetree/bindings/usb/generic-ehci.yaml >> +++ b/Documentation/devicetree/bindings/usb/generic-ehci.yaml >> @@ -102,6 +102,10 @@ properties: >> - if a USB DRD channel: first clock should be host and second >> one should be peripheral >> >> + clock-names: >> + minItems: 1 >> + maxItems: 4 >> + >> power-domains: >> maxItems: 1 >> >> -- >> 2.25.1
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 <varshini.rajendran@microchip.com> >> --- >> 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. Best regards, Nicolas
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 <varshini.rajendran@microchip.com> > > > --- > > > 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. Cheers, Conor.
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 <varshini.rajendran@microchip.com> >>> --- >>> 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
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 <varshini.rajendran@microchip.com> >>>> --- >>>> 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. >
This patch series adds support for the new SoC family - sam9x7. - The device tree, configs and drivers are added - Clock driver for sam9x7 is added - Support for basic peripherals is added Balamanikandan Gunasundar (1): ARM: configs: at91: Enable csi and isc support Hari Prasath (1): irqchip/atmel-aic5: Add support for sam9x7 aic Nicolas Ferre (1): net: macb: add support for gmac to sam9x7 Varshini Rajendran (18): dt-bindings: microchip: atmel,at91rm9200-tcb: add sam9x60 compatible dt-bindings: usb: ehci: Add atmel at91sam9g45-ehci compatible dt-bindings: usb: generic-ehci: Document clock-names property ARM: dts: at91: sam9x7: add device tree for soc ARM: configs: at91: enable config flags for sam9x7 SoC ARM: configs: at91: add mcan support ARM: at91: pm: add support for sam9x7 soc family ARM: at91: pm: add sam9x7 soc init config ARM: at91: Kconfig: add config flag for SAM9X7 SoC ARM: at91: add support in soc driver for new sam9x7 clk: at91: clk-sam9x60-pll: re-factor to support individual core freq outputs clk: at91: sam9x7: add support for HW PLL freq dividers clk: at91: sam9x7: add sam9x7 pmc driver dt-bindings: irqchip/atmel-aic5: Add support for sam9x7 aic power: reset: at91-poweroff: lookup for proper pmc dt node for sam9x7 power: reset: at91-reset: add reset support for sam9x7 soc power: reset: at91-reset: add sdhwc support for sam9x7 soc dt-bindings: net: cdns,macb: add documentation for sam9x7 ethernet interface .../interrupt-controller/atmel,aic.txt | 2 +- .../devicetree/bindings/net/cdns,macb.yaml | 1 + .../soc/microchip/atmel,at91rm9200-tcb.yaml | 1 + .../devicetree/bindings/usb/generic-ehci.yaml | 5 + arch/arm/boot/dts/sam9x7.dtsi | 1333 +++++++++++++++++ arch/arm/configs/at91_dt_defconfig | 8 + arch/arm/mach-at91/Kconfig | 21 +- arch/arm/mach-at91/Makefile | 1 + arch/arm/mach-at91/generic.h | 2 + arch/arm/mach-at91/pm.c | 35 + arch/arm/mach-at91/sam9x7.c | 34 + drivers/clk/at91/Makefile | 1 + drivers/clk/at91/clk-sam9x60-pll.c | 50 +- drivers/clk/at91/pmc.h | 2 + drivers/clk/at91/sam9x60.c | 7 + drivers/clk/at91/sam9x7.c | 947 ++++++++++++ drivers/clk/at91/sama7g5.c | 7 + drivers/irqchip/irq-atmel-aic5.c | 10 + drivers/net/ethernet/cadence/macb_main.c | 1 + drivers/power/reset/Kconfig | 4 +- drivers/power/reset/at91-sama5d2_shdwc.c | 1 + drivers/soc/atmel/soc.c | 23 + drivers/soc/atmel/soc.h | 9 + 23 files changed, 2489 insertions(+), 16 deletions(-) create mode 100644 arch/arm/boot/dts/sam9x7.dtsi create mode 100644 arch/arm/mach-at91/sam9x7.c create mode 100644 drivers/clk/at91/sam9x7.c