| Message ID | 20220411085916.941433-6-conor.dooley@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More PolarFire SoC Fixes for 5.18 | expand |
On 11/04/2022 10:59, Conor Dooley wrote: > The RTC reference and MSSPLL were previously not documented or defined, > as they were unused. Add their defines to the PolarFire SoC header. > > Fixes: 2145bb687e3f ("dt-bindings: clk: microchip: Add Microchip PolarFire host binding") > Reviewed-by: Daire McNamara <daire.mcnamara@microchip.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > include/dt-bindings/clock/microchip,mpfs-clock.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/include/dt-bindings/clock/microchip,mpfs-clock.h b/include/dt-bindings/clock/microchip,mpfs-clock.h > index 73f2a9324857..3cba46b9191f 100644 > --- a/include/dt-bindings/clock/microchip,mpfs-clock.h > +++ b/include/dt-bindings/clock/microchip,mpfs-clock.h > @@ -1,15 +1,18 @@ > /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > /* > * Daire McNamara,<daire.mcnamara@microchip.com> > - * Copyright (C) 2020 Microchip Technology Inc. All rights reserved. > + * Copyright (C) 2020-2022 Microchip Technology Inc. All rights reserved. > */ > > #ifndef _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ > #define _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ > > +#define CLK_MSSPLL 34 You have some weird order here. Shouldn't it be under CLK_RTCREF? > + > #define CLK_CPU 0 > #define CLK_AXI 1 > #define CLK_AHB 2 > +#define CLK_RTCREF 33 > > #define CLK_ENVM 3 > #define CLK_MAC0 4 Best regards, Krzysztof
On 12/04/2022 11:47, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 11/04/2022 10:59, Conor Dooley wrote: >> The RTC reference and MSSPLL were previously not documented or defined, >> as they were unused. Add their defines to the PolarFire SoC header. >> >> Fixes: 2145bb687e3f ("dt-bindings: clk: microchip: Add Microchip PolarFire host binding") >> Reviewed-by: Daire McNamara <daire.mcnamara@microchip.com> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> include/dt-bindings/clock/microchip,mpfs-clock.h | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/include/dt-bindings/clock/microchip,mpfs-clock.h b/include/dt-bindings/clock/microchip,mpfs-clock.h >> index 73f2a9324857..3cba46b9191f 100644 >> --- a/include/dt-bindings/clock/microchip,mpfs-clock.h >> +++ b/include/dt-bindings/clock/microchip,mpfs-clock.h >> @@ -1,15 +1,18 @@ >> /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ >> /* >> * Daire McNamara,<daire.mcnamara@microchip.com> >> - * Copyright (C) 2020 Microchip Technology Inc. All rights reserved. >> + * Copyright (C) 2020-2022 Microchip Technology Inc. All rights reserved. >> */ >> >> #ifndef _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ >> #define _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ >> >> +#define CLK_MSSPLL 34 > > You have some weird order here. Shouldn't it be under CLK_RTCREF? Yeah numerically weirdly ordered - I grouped the clocks by type: MSSPLL is a pll, CPU/AXI/AHB/RTC are all dividers & the rest are on/off toggles. I'd've prefered to have renumbered the whole list, but that didn't feel like a good idea. Additionally MSSPLL is the source for CLK_{CPI,AXI,AHB} so I put it at the top. I have no particular preference, so if you want them reordered so that MSSPLL is under RTCREF just say the word :) Thanks, Conor. > >> + >> #define CLK_CPU 0 >> #define CLK_AXI 1 >> #define CLK_AHB 2 >> +#define CLK_RTCREF 33 >> >> #define CLK_ENVM 3 >> #define CLK_MAC0 4 > > > Best regards, > Krzysztof
On 12/04/2022 14:04, Conor.Dooley@microchip.com wrote: > On 12/04/2022 11:47, Krzysztof Kozlowski wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 11/04/2022 10:59, Conor Dooley wrote: >>> The RTC reference and MSSPLL were previously not documented or defined, >>> as they were unused. Add their defines to the PolarFire SoC header. >>> >>> Fixes: 2145bb687e3f ("dt-bindings: clk: microchip: Add Microchip PolarFire host binding") >>> Reviewed-by: Daire McNamara <daire.mcnamara@microchip.com> >>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >>> --- >>> include/dt-bindings/clock/microchip,mpfs-clock.h | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/dt-bindings/clock/microchip,mpfs-clock.h b/include/dt-bindings/clock/microchip,mpfs-clock.h >>> index 73f2a9324857..3cba46b9191f 100644 >>> --- a/include/dt-bindings/clock/microchip,mpfs-clock.h >>> +++ b/include/dt-bindings/clock/microchip,mpfs-clock.h >>> @@ -1,15 +1,18 @@ >>> /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ >>> /* >>> * Daire McNamara,<daire.mcnamara@microchip.com> >>> - * Copyright (C) 2020 Microchip Technology Inc. All rights reserved. >>> + * Copyright (C) 2020-2022 Microchip Technology Inc. All rights reserved. >>> */ >>> >>> #ifndef _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ >>> #define _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ >>> >>> +#define CLK_MSSPLL 34 >> >> You have some weird order here. Shouldn't it be under CLK_RTCREF? > > Yeah numerically weirdly ordered - I grouped the clocks by type: > MSSPLL is a pll, CPU/AXI/AHB/RTC are all dividers & the rest are on/off > toggles. I'd've prefered to have renumbered the whole list, but that > didn't feel like a good idea. > > Additionally MSSPLL is the source for CLK_{CPI,AXI,AHB} so I put it at > the top. I have no particular preference, so if you want them reordered > so that MSSPLL is under RTCREF just say the word :) Hm, are these in the same clock controller (device, not driver)? If yes, then please order them numerically. Pretty often one binding header have IDs for several clock controllers, so then it's a different case. Best regards, Krzysztof
On 12/04/2022 12:10, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 12/04/2022 14:04, Conor.Dooley@microchip.com wrote: >> On 12/04/2022 11:47, Krzysztof Kozlowski wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On 11/04/2022 10:59, Conor Dooley wrote: >>>> The RTC reference and MSSPLL were previously not documented or defined, >>>> as they were unused. Add their defines to the PolarFire SoC header. >>>> >>>> Fixes: 2145bb687e3f ("dt-bindings: clk: microchip: Add Microchip PolarFire host binding") >>>> Reviewed-by: Daire McNamara <daire.mcnamara@microchip.com> >>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >>>> --- >>>> include/dt-bindings/clock/microchip,mpfs-clock.h | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/dt-bindings/clock/microchip,mpfs-clock.h b/include/dt-bindings/clock/microchip,mpfs-clock.h >>>> index 73f2a9324857..3cba46b9191f 100644 >>>> --- a/include/dt-bindings/clock/microchip,mpfs-clock.h >>>> +++ b/include/dt-bindings/clock/microchip,mpfs-clock.h >>>> @@ -1,15 +1,18 @@ >>>> /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ >>>> /* >>>> * Daire McNamara,<daire.mcnamara@microchip.com> >>>> - * Copyright (C) 2020 Microchip Technology Inc. All rights reserved. >>>> + * Copyright (C) 2020-2022 Microchip Technology Inc. All rights reserved. >>>> */ >>>> >>>> #ifndef _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ >>>> #define _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ >>>> >>>> +#define CLK_MSSPLL 34 >>> >>> You have some weird order here. Shouldn't it be under CLK_RTCREF? >> >> Yeah numerically weirdly ordered - I grouped the clocks by type: >> MSSPLL is a pll, CPU/AXI/AHB/RTC are all dividers & the rest are on/off >> toggles. I'd've prefered to have renumbered the whole list, but that >> didn't feel like a good idea. >> >> Additionally MSSPLL is the source for CLK_{CPI,AXI,AHB} so I put it at >> the top. I have no particular preference, so if you want them reordered >> so that MSSPLL is under RTCREF just say the word :) > > Hm, are these in the same clock controller (device, not driver)? If yes, > then please order them numerically. Pretty often one binding header have > IDs for several clock controllers, so then it's a different case. Not *quite* sure what you mean by device. There is only one SoC that this header applies to, but in the actual design the MSSPLL is in one block, the RTC divider in another and CLK_CPU -> CLK_CFM in a third.
On 12/04/2022 14:26, Conor.Dooley@microchip.com wrote: >>> Additionally MSSPLL is the source for CLK_{CPI,AXI,AHB} so I put it at >>> the top. I have no particular preference, so if you want them reordered >>> so that MSSPLL is under RTCREF just say the word :) >> >> Hm, are these in the same clock controller (device, not driver)? If yes, >> then please order them numerically. Pretty often one binding header have >> IDs for several clock controllers, so then it's a different case. > > Not *quite* sure what you mean by device. There is only one SoC that > this header applies to, but in the actual design the MSSPLL is in one > block, the RTC divider in another and CLK_CPU -> CLK_CFM in a third. By device I meant here part of Soc responsible for clocks which could be called a self-containing block. Pretty often such block maps to a Linux "struct device" or some wrapper around it (e.g. clock-controller device). For example such "self-containing block" has device node in DTS. Judging by your description, these will be different blocks / device nodes in DTS? Best regards, Krzysztof
On 12/04/2022 18:10, Krzysztof Kozlowski wrote: > On 12/04/2022 14:26, Conor.Dooley@microchip.com wrote: >>>> Additionally MSSPLL is the source for CLK_{CPI,AXI,AHB} so I put it at >>>> the top. I have no particular preference, so if you want them reordered >>>> so that MSSPLL is under RTCREF just say the word :) >>> >>> Hm, are these in the same clock controller (device, not driver)? If yes, >>> then please order them numerically. Pretty often one binding header have >>> IDs for several clock controllers, so then it's a different case. >> >> Not *quite* sure what you mean by device. There is only one SoC that >> this header applies to, but in the actual design the MSSPLL is in one >> block, the RTC divider in another and CLK_CPU -> CLK_CFM in a third. > > By device I meant here part of Soc responsible for clocks which could be > called a self-containing block. Pretty often such block maps to a Linux > "struct device" or some wrapper around it (e.g. clock-controller > device). For example such "self-containing block" has device node in DTS. > > Judging by your description, these will be different blocks / device > nodes in DTS? The way it's implemented is a bit interconnected and none of the three blocks would satisfy a "self contained" constraint. Eg. The rtcref divider's control reg sits between two registers responsible for the CLK_CPU -> CLK_CFM clocks but it's input clock mux is in the same sub-block as the MSSPLL. I guess its better put that each of the three are sub-blocks of a self contained clock controller for the mss core complex. There are several other clock domains on the chip which would have distinct clock controllers & may be added to this header in the future, if letting Linux control them makes any sense. For example, clocks in (and used for the clocking of) the fpga fabric. This controller is a single node in the device tree. Sounds like reordering it numerically makes the most sense then - I'll resend tomorrow if that's okay. Thanks, Conor.
On 12/04/2022 20:29, Conor Dooley wrote: > The way it's implemented is a bit interconnected and none of the three > blocks would satisfy a "self contained" constraint. Eg. The rtcref > divider's control reg sits between two registers responsible for the > CLK_CPU -> CLK_CFM clocks but it's input clock mux is in the same > sub-block as the MSSPLL. > > I guess its better put that each of the three are sub-blocks of a self > contained clock controller for the mss core complex. There are several > other clock domains on the chip which would have distinct clock > controllers & may be added to this header in the future, if letting > Linux control them makes any sense. For example, clocks in (and used for > the clocking of) the fpga fabric. > > This controller is a single node in the device tree. Sounds like > reordering it numerically makes the most sense then - I'll resend > tomorrow if that's okay. Yes. Best regards, Krzysztof
diff --git a/include/dt-bindings/clock/microchip,mpfs-clock.h b/include/dt-bindings/clock/microchip,mpfs-clock.h index 73f2a9324857..3cba46b9191f 100644 --- a/include/dt-bindings/clock/microchip,mpfs-clock.h +++ b/include/dt-bindings/clock/microchip,mpfs-clock.h @@ -1,15 +1,18 @@ /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ /* * Daire McNamara,<daire.mcnamara@microchip.com> - * Copyright (C) 2020 Microchip Technology Inc. All rights reserved. + * Copyright (C) 2020-2022 Microchip Technology Inc. All rights reserved. */ #ifndef _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ #define _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ +#define CLK_MSSPLL 34 + #define CLK_CPU 0 #define CLK_AXI 1 #define CLK_AHB 2 +#define CLK_RTCREF 33 #define CLK_ENVM 3 #define CLK_MAC0 4
The RTC reference and MSSPLL were previously not documented or defined, as they were unused. Add their defines to the PolarFire SoC header. Fixes: 2145bb687e3f ("dt-bindings: clk: microchip: Add Microchip PolarFire host binding") Reviewed-by: Daire McNamara <daire.mcnamara@microchip.com> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- include/dt-bindings/clock/microchip,mpfs-clock.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)