| Message ID | 20250210164506.495747-2-ada@thorsis.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Microchip OTPC driver on SAM9X60 exposing UIDxR as additional nvmem device | expand |
On 10/02/2025 17:44, Alexander Dahl wrote: > Before adding even more new indexes creating more holes in the > clk at91 drivers pmc_data->chws arrays, split this up. > > This is a partial split up only for SoCs affected by upcoming changes > and by that PMC_MAIN + x hack, others could follow by the same scheme. > > Binding splitup was proposed for several reasons: > > 1) keep the driver code simple, readable, and efficient > 2) avoid accidental array index duplication > 3) avoid memory waste by creating more and more unused array members. > > Old values are kept to not break dts, and to maintain dt ABI. > > Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u > Signed-off-by: Alexander Dahl <ada@thorsis.com> > --- > > Notes: > v2: > - new patch, not present in v1 > > .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++ > .../dt-bindings/clock/microchip,sam9x7-pmc.h | 25 +++++++++++++++ > .../clock/microchip,sama7d65-pmc.h | 32 +++++++++++++++++++ > .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++ > 4 files changed, 100 insertions(+) > create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h > create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h > create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h > create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h > > diff --git a/include/dt-bindings/clock/microchip,sam9x60-pmc.h b/include/dt-bindings/clock/microchip,sam9x60-pmc.h > new file mode 100644 > index 0000000000000..e01e867e8c4da > --- /dev/null > +++ b/include/dt-bindings/clock/microchip,sam9x60-pmc.h > @@ -0,0 +1,19 @@ > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ > +/* > + * The constants defined in this header are being used in dts and in > + * at91 sam9x60 clock driver. > + */ > + > +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H > +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H > + > +#include <dt-bindings/clock/at91.h> > + > +/* old from before bindings splitup */ > +#define SAM9X60_PMC_MCK PMC_MCK /* 1 */ > +#define SAM9X60_PMC_UTMI PMC_UTMI /* 2 */ > +#define SAM9X60_PMC_MAIN PMC_MAIN /* 3 */ > + > +#define SAM9X60_PMC_PLLACK PMC_PLLACK /* 7 */ IIUC, you want to have bindings per SoC, so why not adding proper constants here instead of including entire old binding header? The binding header should be entirely abandoned later. Best regards, Krzysztof
Hello everyone, Am Mon, Feb 10, 2025 at 06:05:31PM +0100 schrieb Krzysztof Kozlowski: > On 10/02/2025 17:44, Alexander Dahl wrote: > > Before adding even more new indexes creating more holes in the > > clk at91 drivers pmc_data->chws arrays, split this up. > > > > This is a partial split up only for SoCs affected by upcoming changes > > and by that PMC_MAIN + x hack, others could follow by the same scheme. > > > > Binding splitup was proposed for several reasons: > > > > 1) keep the driver code simple, readable, and efficient > > 2) avoid accidental array index duplication > > 3) avoid memory waste by creating more and more unused array members. > > > > Old values are kept to not break dts, and to maintain dt ABI. > > > > Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u > > Signed-off-by: Alexander Dahl <ada@thorsis.com> > > --- > > > > Notes: > > v2: > > - new patch, not present in v1 > > > > .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++ > > .../dt-bindings/clock/microchip,sam9x7-pmc.h | 25 +++++++++++++++ > > .../clock/microchip,sama7d65-pmc.h | 32 +++++++++++++++++++ > > .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++ > > 4 files changed, 100 insertions(+) > > create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h > > create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h > > create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h > > create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h > > > > diff --git a/include/dt-bindings/clock/microchip,sam9x60-pmc.h b/include/dt-bindings/clock/microchip,sam9x60-pmc.h > > new file mode 100644 > > index 0000000000000..e01e867e8c4da > > --- /dev/null > > +++ b/include/dt-bindings/clock/microchip,sam9x60-pmc.h > > @@ -0,0 +1,19 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ > > +/* > > + * The constants defined in this header are being used in dts and in > > + * at91 sam9x60 clock driver. > > + */ > > + > > +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H > > +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H > > + > > +#include <dt-bindings/clock/at91.h> > > + > > +/* old from before bindings splitup */ > > +#define SAM9X60_PMC_MCK PMC_MCK /* 1 */ > > +#define SAM9X60_PMC_UTMI PMC_UTMI /* 2 */ > > +#define SAM9X60_PMC_MAIN PMC_MAIN /* 3 */ > > + > > +#define SAM9X60_PMC_PLLACK PMC_PLLACK /* 7 */ > > IIUC, you want to have bindings per SoC, so why not adding proper > constants here instead of including entire old binding header? The > binding header should be entirely abandoned later. Which binding header should be abandoned entirely? The bindings per SoC idea was proposed in series v1 feedback. I'm neither a binding nor a clock expeert. As far as I understood it's important to keep the exact same values as before to not change any ABI. The non SoC specific values are still used on older SoCs of the at91 family, so this is why I used the old constants for now. These PMC indexes are not the only definitions in dt-bindings/clock/at91.h however, there are more which are not SoC specific. I'd like some thoughts from the Microchip maintainers here, what's their idea on how to proceed with the at91 clock stuff? This works for me, but the current state is more or less still an idea as base for discussion. Please don't make it overly complicated, this is not the primary focus of my work. Thanks and greets Alex > > > Best regards, > Krzysztof
On 11/02/2025 08:16, Alexander Dahl wrote: > Hello everyone, > > Am Mon, Feb 10, 2025 at 06:05:31PM +0100 schrieb Krzysztof Kozlowski: >> On 10/02/2025 17:44, Alexander Dahl wrote: >>> Before adding even more new indexes creating more holes in the >>> clk at91 drivers pmc_data->chws arrays, split this up. >>> >>> This is a partial split up only for SoCs affected by upcoming changes >>> and by that PMC_MAIN + x hack, others could follow by the same scheme. >>> >>> Binding splitup was proposed for several reasons: >>> >>> 1) keep the driver code simple, readable, and efficient >>> 2) avoid accidental array index duplication >>> 3) avoid memory waste by creating more and more unused array members. >>> >>> Old values are kept to not break dts, and to maintain dt ABI. >>> >>> Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u >>> Signed-off-by: Alexander Dahl <ada@thorsis.com> >>> --- >>> >>> Notes: >>> v2: >>> - new patch, not present in v1 >>> >>> .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++ >>> .../dt-bindings/clock/microchip,sam9x7-pmc.h | 25 +++++++++++++++ >>> .../clock/microchip,sama7d65-pmc.h | 32 +++++++++++++++++++ >>> .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++ >>> 4 files changed, 100 insertions(+) >>> create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h >>> create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h >>> create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h >>> create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h >>> >>> diff --git a/include/dt-bindings/clock/microchip,sam9x60-pmc.h b/include/dt-bindings/clock/microchip,sam9x60-pmc.h >>> new file mode 100644 >>> index 0000000000000..e01e867e8c4da >>> --- /dev/null >>> +++ b/include/dt-bindings/clock/microchip,sam9x60-pmc.h >>> @@ -0,0 +1,19 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ >>> +/* >>> + * The constants defined in this header are being used in dts and in >>> + * at91 sam9x60 clock driver. >>> + */ >>> + >>> +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H >>> +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H >>> + >>> +#include <dt-bindings/clock/at91.h> >>> + >>> +/* old from before bindings splitup */ >>> +#define SAM9X60_PMC_MCK PMC_MCK /* 1 */ >>> +#define SAM9X60_PMC_UTMI PMC_UTMI /* 2 */ >>> +#define SAM9X60_PMC_MAIN PMC_MAIN /* 3 */ >>> + >>> +#define SAM9X60_PMC_PLLACK PMC_PLLACK /* 7 */ >> >> IIUC, you want to have bindings per SoC, so why not adding proper >> constants here instead of including entire old binding header? The >> binding header should be entirely abandoned later. > > Which binding header should be abandoned entirely? The one you claim to split. I assume it is the same one included here. > > The bindings per SoC idea was proposed in series v1 feedback. I'm > neither a binding nor a clock expeert. As far as I understood it's > important to keep the exact same values as before to not change any Yes, I did not propose to change any IDs. > ABI. The non SoC specific values are still used on older SoCs of the > at91 family, so this is why I used the old constants for now. > > These PMC indexes are not the only definitions in > dt-bindings/clock/at91.h however, there are more which are not SoC > specific. > > I'd like some thoughts from the Microchip maintainers here, > what's their idea on how to proceed with the at91 clock stuff? > > This works for me, but the current state is more or less still an idea > as base for discussion. Please don't make it overly complicated, this > is not the primary focus of my work. You made this binding more complicated than it should be - instead of simple list of clocks you include some other file and split between old and new. Best regards, Krzysztof
Hi, Alexander, On 10.02.2025 18:44, Alexander Dahl wrote: > Before adding even more new indexes creating more holes in the > clk at91 drivers pmc_data->chws arrays, split this up. > > This is a partial split up only for SoCs affected by upcoming changes > and by that PMC_MAIN + x hack, others could follow by the same scheme. > > Binding splitup was proposed for several reasons: > > 1) keep the driver code simple, readable, and efficient > 2) avoid accidental array index duplication > 3) avoid memory waste by creating more and more unused array members. > > Old values are kept to not break dts, and to maintain dt ABI. > > Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u > Signed-off-by: Alexander Dahl <ada@thorsis.com> > --- > > Notes: > v2: > - new patch, not present in v1 > > .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++ > .../dt-bindings/clock/microchip,sam9x7-pmc.h | 25 +++++++++++++++ > .../clock/microchip,sama7d65-pmc.h | 32 +++++++++++++++++++ > .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++ > 4 files changed, 100 insertions(+) > create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h > create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h > create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h > create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h > [ ...] > diff --git a/include/dt-bindings/clock/microchip,sama7g5-pmc.h b/include/dt-bindings/clock/microchip,sama7g5-pmc.h > new file mode 100644 > index 0000000000000..ad69ccdf9dc78 > --- /dev/null > +++ b/include/dt-bindings/clock/microchip,sama7g5-pmc.h > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ > +/* > + * The constants defined in this header are being used in dts and in > + * at91 sama7g5 clock driver. > + */ > + > +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H > +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H > + > +#include <dt-bindings/clock/at91.h> > + > +/* old from before bindings splitup */ > +#define SAMA7G5_PMC_MCK0 PMC_MCK /* 1 */ > +#define SAMA7G5_PMC_UTMI PMC_UTMI /* 2 */ > +#define SAMA7G5_PMC_MAIN PMC_MAIN /* 3 */ > +#define SAMA7G5_PMC_CPUPLL PMC_CPUPLL /* 4 */ > +#define SAMA7G5_PMC_SYSPLL PMC_SYSPLL /* 5 */ > + > +#define SAMA7G5_PMC_AUDIOPMCPLL PMC_AUDIOPMCPLL /* 9 */ > +#define SAMA7G5_PMC_AUDIOIOPLL PMC_AUDIOIOPLL /* 10 */ > + > +#define SAMA7G5_PMC_MCK1 PMC_MCK1 /* 13 */ > + > +#endif I would have expected this to be something like: #ifndef __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ #define __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ /* Core clocks. */ #define SAMA7G5_MCK0 1 #define SAMA7G5_UTMI 2 #define SAMA7G5_MAIN 3 #define SAMA7G5_CPUPLL 4 #define SAMA7G5_SYSPLL 5 #define SAMA7G5_DDRPLL 6 #define SAMA7G5_IMGPLL 7 #define SAMA7G5_BAUDPLL 8 // ... #define SAMA7G5_MCK1 13 #endif /* __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ */ Same for the other affected SoCs. The content of include/dt-bindings/clock/at91.h would be limited eventually only to the PMC clock types. The other "#define PMC_*" defines will eventually go to SoC specific bindings. "#define AT91_PMC_*" seems to not belong here anyway and these would in the end removed, as well. Thank you, Claudiu
Hello Claudiu, Am Mon, Feb 17, 2025 at 11:11:44AM +0200 schrieb Claudiu Beznea: > Hi, Alexander, > > On 10.02.2025 18:44, Alexander Dahl wrote: > > Before adding even more new indexes creating more holes in the > > clk at91 drivers pmc_data->chws arrays, split this up. > > > > This is a partial split up only for SoCs affected by upcoming changes > > and by that PMC_MAIN + x hack, others could follow by the same scheme. > > > > Binding splitup was proposed for several reasons: > > > > 1) keep the driver code simple, readable, and efficient > > 2) avoid accidental array index duplication > > 3) avoid memory waste by creating more and more unused array members. > > > > Old values are kept to not break dts, and to maintain dt ABI. > > > > Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u > > Signed-off-by: Alexander Dahl <ada@thorsis.com> > > --- > > > > Notes: > > v2: > > - new patch, not present in v1 > > > > .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++ > > .../dt-bindings/clock/microchip,sam9x7-pmc.h | 25 +++++++++++++++ > > .../clock/microchip,sama7d65-pmc.h | 32 +++++++++++++++++++ > > .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++ > > 4 files changed, 100 insertions(+) > > create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h > > create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h > > create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h > > create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h > > > > [ ...] > > > diff --git a/include/dt-bindings/clock/microchip,sama7g5-pmc.h b/include/dt-bindings/clock/microchip,sama7g5-pmc.h > > new file mode 100644 > > index 0000000000000..ad69ccdf9dc78 > > --- /dev/null > > +++ b/include/dt-bindings/clock/microchip,sama7g5-pmc.h > > @@ -0,0 +1,24 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ > > +/* > > + * The constants defined in this header are being used in dts and in > > + * at91 sama7g5 clock driver. > > + */ > > + > > +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H > > +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H > > + > > +#include <dt-bindings/clock/at91.h> > > + > > +/* old from before bindings splitup */ > > +#define SAMA7G5_PMC_MCK0 PMC_MCK /* 1 */ > > +#define SAMA7G5_PMC_UTMI PMC_UTMI /* 2 */ > > +#define SAMA7G5_PMC_MAIN PMC_MAIN /* 3 */ > > +#define SAMA7G5_PMC_CPUPLL PMC_CPUPLL /* 4 */ > > +#define SAMA7G5_PMC_SYSPLL PMC_SYSPLL /* 5 */ > > + > > +#define SAMA7G5_PMC_AUDIOPMCPLL PMC_AUDIOPMCPLL /* 9 */ > > +#define SAMA7G5_PMC_AUDIOIOPLL PMC_AUDIOIOPLL /* 10 */ > > + > > +#define SAMA7G5_PMC_MCK1 PMC_MCK1 /* 13 */ > > + > > +#endif > > I would have expected this to be something like: > > #ifndef __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ > #define __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ > > /* Core clocks. */ > #define SAMA7G5_MCK0 1 > #define SAMA7G5_UTMI 2 > #define SAMA7G5_MAIN 3 > #define SAMA7G5_CPUPLL 4 > #define SAMA7G5_SYSPLL 5 > #define SAMA7G5_DDRPLL 6 > #define SAMA7G5_IMGPLL 7 > #define SAMA7G5_BAUDPLL 8 Okay no reference to the old header, but numbers. Got that. I'm not sure where you got the 7 and 8 from here, according to my analysis, sama7g5 does not use those. > > // ... > > #define SAMA7G5_MCK1 13 > > #endif /* __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ */ > > Same for the other affected SoCs. > > The content of include/dt-bindings/clock/at91.h would be limited eventually > only to the PMC clock types. What does this mean? The clocks split out are no PMC clocks? Then the old PMC_MAIN etc. definitions were named wrong? All or only some of them? Or is this different between older and newer SoC variants of the at91 family? From a quick glance in the SAM9X60 datasheet for example the clock generator provides MD_SLCK, TD_SLCK, MAINCK, and PLL clocks, while the PMC provides MCK, USB clocks, GCLK, PCK, and the peripheral clocks. The chws array in drivers/clk/at91/sam9x60.c however gets main_rc_osc (from clock generator), mainck (clock generator), pllack (clock generator), upllck (clock generator, UTMI), but also mck (from PMC). This creates the impression things are mixed up here. I find all this quite confusing to be honest. > The other "#define PMC_*" defines will eventually go to SoC specific > bindings. "#define AT91_PMC_*" seems to not belong here anyway and these > would in the end removed, as well. Okay, you seem to have an idea how this should look like in the long run. Are there any plans at Microchip or at91 clock maintainer side to clean this up in the near future? I would like to rather put my small changes for otpc on top of a clean tree, instead of trying to clean up clock drivers and bindings for a whole family of SoCs and boards, where I can test only one of them. O:-) Greets Alex
Hi, Alexander, On 17.02.2025 11:47, Alexander Dahl wrote: > Hello Claudiu, > > Am Mon, Feb 17, 2025 at 11:11:44AM +0200 schrieb Claudiu Beznea: >> Hi, Alexander, >> >> On 10.02.2025 18:44, Alexander Dahl wrote: >>> Before adding even more new indexes creating more holes in the >>> clk at91 drivers pmc_data->chws arrays, split this up. >>> >>> This is a partial split up only for SoCs affected by upcoming changes >>> and by that PMC_MAIN + x hack, others could follow by the same scheme. >>> >>> Binding splitup was proposed for several reasons: >>> >>> 1) keep the driver code simple, readable, and efficient >>> 2) avoid accidental array index duplication >>> 3) avoid memory waste by creating more and more unused array members. >>> >>> Old values are kept to not break dts, and to maintain dt ABI. >>> >>> Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u >>> Signed-off-by: Alexander Dahl <ada@thorsis.com> >>> --- >>> >>> Notes: >>> v2: >>> - new patch, not present in v1 >>> >>> .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++ >>> .../dt-bindings/clock/microchip,sam9x7-pmc.h | 25 +++++++++++++++ >>> .../clock/microchip,sama7d65-pmc.h | 32 +++++++++++++++++++ >>> .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++ >>> 4 files changed, 100 insertions(+) >>> create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h >>> create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h >>> create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h >>> create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h >>> >> >> [ ...] >> >>> diff --git a/include/dt-bindings/clock/microchip,sama7g5-pmc.h b/include/dt-bindings/clock/microchip,sama7g5-pmc.h >>> new file mode 100644 >>> index 0000000000000..ad69ccdf9dc78 >>> --- /dev/null >>> +++ b/include/dt-bindings/clock/microchip,sama7g5-pmc.h >>> @@ -0,0 +1,24 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ >>> +/* >>> + * The constants defined in this header are being used in dts and in >>> + * at91 sama7g5 clock driver. >>> + */ >>> + >>> +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H >>> +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H >>> + >>> +#include <dt-bindings/clock/at91.h> >>> + >>> +/* old from before bindings splitup */ >>> +#define SAMA7G5_PMC_MCK0 PMC_MCK /* 1 */ >>> +#define SAMA7G5_PMC_UTMI PMC_UTMI /* 2 */ >>> +#define SAMA7G5_PMC_MAIN PMC_MAIN /* 3 */ >>> +#define SAMA7G5_PMC_CPUPLL PMC_CPUPLL /* 4 */ >>> +#define SAMA7G5_PMC_SYSPLL PMC_SYSPLL /* 5 */ >>> + >>> +#define SAMA7G5_PMC_AUDIOPMCPLL PMC_AUDIOPMCPLL /* 9 */ >>> +#define SAMA7G5_PMC_AUDIOIOPLL PMC_AUDIOIOPLL /* 10 */ >>> + >>> +#define SAMA7G5_PMC_MCK1 PMC_MCK1 /* 13 */ >>> + >>> +#endif >> >> I would have expected this to be something like: >> >> #ifndef __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ >> #define __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ >> >> /* Core clocks. */ >> #define SAMA7G5_MCK0 1 >> #define SAMA7G5_UTMI 2 >> #define SAMA7G5_MAIN 3 >> #define SAMA7G5_CPUPLL 4 >> #define SAMA7G5_SYSPLL 5 >> #define SAMA7G5_DDRPLL 6 >> #define SAMA7G5_IMGPLL 7 >> #define SAMA7G5_BAUDPLL 8 > > Okay no reference to the old header, but numbers. Got that. > > I'm not sure where you got the 7 and 8 from here, according to my > analysis, sama7g5 does not use those. From include/dt-bindings/clock/at91.sh #define PMC_IMGPLL (PMC_MAIN + 4) #define PMC_BAUDPLL (PMC_MAIN + 5) > >> >> // ... >> >> #define SAMA7G5_MCK1 13 >> >> #endif /* __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ */ >> >> Same for the other affected SoCs. >> >> The content of include/dt-bindings/clock/at91.h would be limited eventually >> only to the PMC clock types. > > What does this mean? The clocks split out are no PMC clocks? Still PMC clocks. Keeping the types in separate header allows keeping the code PMC code common for all SoCs. Then the newly added headers will be used only in the SoC DTes and SoC clock driver (e.g. in your case drivers/clk/at91/sam9x60.c) > Then > the old PMC_MAIN etc. definitions were named wrong? All or only some > of them? Or is this different between older and newer SoC variants of > the at91 family? > > From a quick glance in the SAM9X60 datasheet for example the clock > generator provides MD_SLCK, TD_SLCK, MAINCK, and PLL clocks, while the > PMC provides MCK, USB clocks, GCLK, PCK, and the peripheral clocks. drivers splits this into: - core clocks - peripheral clocks - generic clocks - system clocks - programmable It's how the code sees it, just a logical split. Thank you, Claudiu > > The chws array in drivers/clk/at91/sam9x60.c however gets main_rc_osc > (from clock generator), mainck (clock generator), pllack (clock > generator), upllck (clock generator, UTMI), but also mck (from PMC). > > This creates the impression things are mixed up here. I find all this > quite confusing to be honest. > >> The other "#define PMC_*" defines will eventually go to SoC specific >> bindings. "#define AT91_PMC_*" seems to not belong here anyway and these >> would in the end removed, as well. > > Okay, you seem to have an idea how this should look like in the long > run. Are there any plans at Microchip or at91 clock maintainer side > to clean this up in the near future? > > I would like to rather put my small changes for otpc on top of a clean > tree, instead of trying to clean up clock drivers and bindings for a > whole family of SoCs and boards, where I can test only one of them. > O:-) > > Greets > Alex >
Hello Claudiu, Am Wed, Feb 19, 2025 at 10:51:40AM +0200 schrieb Claudiu Beznea: > Hi, Alexander, > > On 17.02.2025 11:47, Alexander Dahl wrote: > > Hello Claudiu, > > > > Am Mon, Feb 17, 2025 at 11:11:44AM +0200 schrieb Claudiu Beznea: > >> Hi, Alexander, > >> > >> On 10.02.2025 18:44, Alexander Dahl wrote: > >>> Before adding even more new indexes creating more holes in the > >>> clk at91 drivers pmc_data->chws arrays, split this up. > >>> > >>> This is a partial split up only for SoCs affected by upcoming changes > >>> and by that PMC_MAIN + x hack, others could follow by the same scheme. > >>> > >>> Binding splitup was proposed for several reasons: > >>> > >>> 1) keep the driver code simple, readable, and efficient > >>> 2) avoid accidental array index duplication > >>> 3) avoid memory waste by creating more and more unused array members. > >>> > >>> Old values are kept to not break dts, and to maintain dt ABI. > >>> > >>> Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u > >>> Signed-off-by: Alexander Dahl <ada@thorsis.com> > >>> --- > >>> > >>> Notes: > >>> v2: > >>> - new patch, not present in v1 > >>> > >>> .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++ > >>> .../dt-bindings/clock/microchip,sam9x7-pmc.h | 25 +++++++++++++++ > >>> .../clock/microchip,sama7d65-pmc.h | 32 +++++++++++++++++++ > >>> .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++ > >>> 4 files changed, 100 insertions(+) > >>> create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h > >>> create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h > >>> create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h > >>> create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h > >>> > >> > >> [ ...] > >> > >>> diff --git a/include/dt-bindings/clock/microchip,sama7g5-pmc.h b/include/dt-bindings/clock/microchip,sama7g5-pmc.h > >>> new file mode 100644 > >>> index 0000000000000..ad69ccdf9dc78 > >>> --- /dev/null > >>> +++ b/include/dt-bindings/clock/microchip,sama7g5-pmc.h > >>> @@ -0,0 +1,24 @@ > >>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ > >>> +/* > >>> + * The constants defined in this header are being used in dts and in > >>> + * at91 sama7g5 clock driver. > >>> + */ > >>> + > >>> +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H > >>> +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H > >>> + > >>> +#include <dt-bindings/clock/at91.h> > >>> + > >>> +/* old from before bindings splitup */ > >>> +#define SAMA7G5_PMC_MCK0 PMC_MCK /* 1 */ > >>> +#define SAMA7G5_PMC_UTMI PMC_UTMI /* 2 */ > >>> +#define SAMA7G5_PMC_MAIN PMC_MAIN /* 3 */ > >>> +#define SAMA7G5_PMC_CPUPLL PMC_CPUPLL /* 4 */ > >>> +#define SAMA7G5_PMC_SYSPLL PMC_SYSPLL /* 5 */ > >>> + > >>> +#define SAMA7G5_PMC_AUDIOPMCPLL PMC_AUDIOPMCPLL /* 9 */ > >>> +#define SAMA7G5_PMC_AUDIOIOPLL PMC_AUDIOIOPLL /* 10 */ > >>> + > >>> +#define SAMA7G5_PMC_MCK1 PMC_MCK1 /* 13 */ > >>> + > >>> +#endif > >> > >> I would have expected this to be something like: > >> > >> #ifndef __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ > >> #define __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ > >> > >> /* Core clocks. */ > >> #define SAMA7G5_MCK0 1 > >> #define SAMA7G5_UTMI 2 > >> #define SAMA7G5_MAIN 3 > >> #define SAMA7G5_CPUPLL 4 > >> #define SAMA7G5_SYSPLL 5 > >> #define SAMA7G5_DDRPLL 6 > >> #define SAMA7G5_IMGPLL 7 > >> #define SAMA7G5_BAUDPLL 8 > > > > Okay no reference to the old header, but numbers. Got that. > > > > I'm not sure where you got the 7 and 8 from here, according to my > > analysis, sama7g5 does not use those. > > From include/dt-bindings/clock/at91.sh > > #define PMC_IMGPLL (PMC_MAIN + 4) > > #define PMC_BAUDPLL (PMC_MAIN + 5) Okay fine, but those defines are not used anywhere in the whole kernel as of v6.14-rc3, also not by sama7g5 clock driver. Are those used in a different version or tree? Should I rebase? Or do you want the whole SAMA7G5 section moved away from include/dt-bindings/clock/at91.sh already? Then this would be four steps, right? 1. introduce the new defines 2. use the new defines in driver 3. use the new defines in dt 4. remove the old defines (Same for SAM9X7 and SAMA7D65 sections?) Or is this out of scope for this series? > > > > > >> > >> // ... > >> > >> #define SAMA7G5_MCK1 13 > >> > >> #endif /* __DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H__ */ > >> > >> Same for the other affected SoCs. > >> > >> The content of include/dt-bindings/clock/at91.h would be limited eventually > >> only to the PMC clock types. > > > > What does this mean? The clocks split out are no PMC clocks? > > Still PMC clocks. Keeping the types in separate header allows keeping the > code PMC code common for all SoCs. Then the newly added headers will be > used only in the SoC DTes and SoC clock driver (e.g. in your case > drivers/clk/at91/sam9x60.c) I understand this as: PMC_TYPE_CORE, PMC_TYPE_SYSTEM, etc. will stay in include/dt-bindings/clock/at91.h as they are, but the IDs for core clocks I started to split out will be removed eventually? Then I would have to change my patch to slightly rename the new defines and use the static numbers instead of referencing the old defines, but besides that it's fine? Correct me, if I understood that wrong. Thanks and Greets Alex > > > Then > > the old PMC_MAIN etc. definitions were named wrong? All or only some > > of them? Or is this different between older and newer SoC variants of > > the at91 family? > > > > From a quick glance in the SAM9X60 datasheet for example the clock > > generator provides MD_SLCK, TD_SLCK, MAINCK, and PLL clocks, while the > > PMC provides MCK, USB clocks, GCLK, PCK, and the peripheral clocks. > > drivers splits this into: > - core clocks > - peripheral clocks > - generic clocks > - system clocks > - programmable > > It's how the code sees it, just a logical split. > > Thank you, > Claudiu > > > > > The chws array in drivers/clk/at91/sam9x60.c however gets main_rc_osc > > (from clock generator), mainck (clock generator), pllack (clock > > generator), upllck (clock generator, UTMI), but also mck (from PMC). > > > > This creates the impression things are mixed up here. I find all this > > quite confusing to be honest. > > > >> The other "#define PMC_*" defines will eventually go to SoC specific > >> bindings. "#define AT91_PMC_*" seems to not belong here anyway and these > >> would in the end removed, as well. > > > > Okay, you seem to have an idea how this should look like in the long > > run. Are there any plans at Microchip or at91 clock maintainer side > > to clean this up in the near future? > > > > I would like to rather put my small changes for otpc on top of a clean > > tree, instead of trying to clean up clock drivers and bindings for a > > whole family of SoCs and boards, where I can test only one of them. > > O:-) > > > > Greets > > Alex > > >
diff --git a/include/dt-bindings/clock/microchip,sam9x60-pmc.h b/include/dt-bindings/clock/microchip,sam9x60-pmc.h new file mode 100644 index 0000000000000..e01e867e8c4da --- /dev/null +++ b/include/dt-bindings/clock/microchip,sam9x60-pmc.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ +/* + * The constants defined in this header are being used in dts and in + * at91 sam9x60 clock driver. + */ + +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H + +#include <dt-bindings/clock/at91.h> + +/* old from before bindings splitup */ +#define SAM9X60_PMC_MCK PMC_MCK /* 1 */ +#define SAM9X60_PMC_UTMI PMC_UTMI /* 2 */ +#define SAM9X60_PMC_MAIN PMC_MAIN /* 3 */ + +#define SAM9X60_PMC_PLLACK PMC_PLLACK /* 7 */ + +#endif diff --git a/include/dt-bindings/clock/microchip,sam9x7-pmc.h b/include/dt-bindings/clock/microchip,sam9x7-pmc.h new file mode 100644 index 0000000000000..2df1ff97a5b18 --- /dev/null +++ b/include/dt-bindings/clock/microchip,sam9x7-pmc.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ +/* + * The constants defined in this header are being used in dts and in + * at91 sam9x7 clock driver. + */ + +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X7_PMC_H +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X7_PMC_H + +#include <dt-bindings/clock/at91.h> + +/* old from before bindings splitup */ +#define SAM9X7_PMC_MCK PMC_MCK /* 1 */ +#define SAM9X7_PMC_UTMI PMC_UTMI /* 2 */ +#define SAM9X7_PMC_MAIN PMC_MAIN /* 3 */ + +#define SAM9X7_PMC_PLLACK PMC_PLLACK /* 7 */ + +#define SAM9X7_PMC_AUDIOPMCPLL PMC_AUDIOPMCPLL /* 9 */ +#define SAM9X7_PMC_AUDIOIOPLL PMC_AUDIOIOPLL /* 10 */ + +#define SAM9X7_PMC_PLLADIV2 PMC_PLLADIV2 /* 14 */ +#define SAM9X7_PMC_LVDSPLL PMC_LVDSPLL /* 15 */ + +#endif diff --git a/include/dt-bindings/clock/microchip,sama7d65-pmc.h b/include/dt-bindings/clock/microchip,sama7d65-pmc.h new file mode 100644 index 0000000000000..f5be643be9b36 --- /dev/null +++ b/include/dt-bindings/clock/microchip,sama7d65-pmc.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ +/* + * The constants defined in this header are being used in dts and in + * at91 sama7d65 clock driver. + */ + +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7D65_PMC_H +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7D65_PMC_H + +#include <dt-bindings/clock/at91.h> + +/* old from before bindings splitup */ +#define SAMA7D65_PMC_MCK0 PMC_MCK /* 1 */ +#define SAMA7D65_PMC_UTMI PMC_UTMI /* 2 */ +#define SAMA7D65_PMC_MAIN PMC_MAIN /* 3 */ +#define SAMA7D65_PMC_CPUPLL PMC_CPUPLL /* 4 */ +#define SAMA7D65_PMC_SYSPLL PMC_SYSPLL /* 5 */ + +#define SAMA7D65_PMC_BAUDPLL PMC_BAUDPLL /* 8 */ +#define SAMA7D65_PMC_AUDIOPMCPLL PMC_AUDIOPMCPLL /* 9 */ +#define SAMA7D65_PMC_AUDIOIOPLL PMC_AUDIOIOPLL /* 10 */ +#define SAMA7D65_PMC_ETHPLL PMC_ETHPLL /* 11 */ + +#define SAMA7D65_PMC_MCK1 PMC_MCK1 /* 13 */ + +#define SAMA7D65_PMC_LVDSPLL PMC_LVDSPLL /* 15 */ +#define SAMA7D65_PMC_MCK3 PMC_MCK3 /* 16 */ +#define SAMA7D65_PMC_MCK5 PMC_MCK5 /* 17 */ + +#define SAMA7D65_PMC_INDEX_MAX 25 + +#endif diff --git a/include/dt-bindings/clock/microchip,sama7g5-pmc.h b/include/dt-bindings/clock/microchip,sama7g5-pmc.h new file mode 100644 index 0000000000000..ad69ccdf9dc78 --- /dev/null +++ b/include/dt-bindings/clock/microchip,sama7g5-pmc.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ +/* + * The constants defined in this header are being used in dts and in + * at91 sama7g5 clock driver. + */ + +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAMA7G5_PMC_H + +#include <dt-bindings/clock/at91.h> + +/* old from before bindings splitup */ +#define SAMA7G5_PMC_MCK0 PMC_MCK /* 1 */ +#define SAMA7G5_PMC_UTMI PMC_UTMI /* 2 */ +#define SAMA7G5_PMC_MAIN PMC_MAIN /* 3 */ +#define SAMA7G5_PMC_CPUPLL PMC_CPUPLL /* 4 */ +#define SAMA7G5_PMC_SYSPLL PMC_SYSPLL /* 5 */ + +#define SAMA7G5_PMC_AUDIOPMCPLL PMC_AUDIOPMCPLL /* 9 */ +#define SAMA7G5_PMC_AUDIOIOPLL PMC_AUDIOIOPLL /* 10 */ + +#define SAMA7G5_PMC_MCK1 PMC_MCK1 /* 13 */ + +#endif
Before adding even more new indexes creating more holes in the clk at91 drivers pmc_data->chws arrays, split this up. This is a partial split up only for SoCs affected by upcoming changes and by that PMC_MAIN + x hack, others could follow by the same scheme. Binding splitup was proposed for several reasons: 1) keep the driver code simple, readable, and efficient 2) avoid accidental array index duplication 3) avoid memory waste by creating more and more unused array members. Old values are kept to not break dts, and to maintain dt ABI. Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u Signed-off-by: Alexander Dahl <ada@thorsis.com> --- Notes: v2: - new patch, not present in v1 .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++ .../dt-bindings/clock/microchip,sam9x7-pmc.h | 25 +++++++++++++++ .../clock/microchip,sama7d65-pmc.h | 32 +++++++++++++++++++ .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++ 4 files changed, 100 insertions(+) create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h