| Message ID | 20220728054202.6981-2-yu.tu@amlogic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add S4 SoC clock controller driver | expand |
On 28/07/2022 07:42, Yu Tu wrote: > Add new clock controller compatible and dt-bindings header for the > Everything-Else domain of the S4 SoC. > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > diff --git a/MAINTAINERS b/MAINTAINERS > index c1abc53f9e91..f872d0c0c253 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* > F: drivers/clk/meson/ > F: include/dt-bindings/clock/gxbb* > F: include/dt-bindings/clock/meson* > +F: include/dt-bindings/clock/s4-clkc.h > > ARM/Amlogic Meson SoC Crypto Drivers > M: Corentin Labbe <clabbe@baylibre.com> > diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h > new file mode 100644 > index 000000000000..b686c8877419 > --- /dev/null > +++ b/include/dt-bindings/clock/s4-clkc.h Filename with vendor prefix, so: amlogic,s4-clkc.h > @@ -0,0 +1,146 @@ > +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ > +/* > + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. > + * Author: Yu Tu <yu.tu@amlogic.com> > + */ > + > +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H > +#define _DT_BINDINGS_CLOCK_S4_CLKC_H > + > +/* > + * CLKID index values > + */ > + > +#define CLKID_FIXED_PLL 1 > +#define CLKID_FCLK_DIV2 3 > +#define CLKID_FCLK_DIV3 5 > +#define CLKID_FCLK_DIV4 7 > +#define CLKID_FCLK_DIV5 9 > +#define CLKID_FCLK_DIV7 11 Why these aren't continuous? IDs are expected to be incremented by 1. > + > +#endif /* _DT_BINDINGS_CLOCK_S4_CLKC_H */ Best regards, Krzysztof
On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 28/07/2022 07:42, Yu Tu wrote: >> Add new clock controller compatible and dt-bindings header for the >> Everything-Else domain of the S4 SoC. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> > > > >> diff --git a/MAINTAINERS b/MAINTAINERS >> index c1abc53f9e91..f872d0c0c253 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* >> F: drivers/clk/meson/ >> F: include/dt-bindings/clock/gxbb* >> F: include/dt-bindings/clock/meson* >> +F: include/dt-bindings/clock/s4-clkc.h >> >> ARM/Amlogic Meson SoC Crypto Drivers >> M: Corentin Labbe <clabbe@baylibre.com> >> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h >> new file mode 100644 >> index 000000000000..b686c8877419 >> --- /dev/null >> +++ b/include/dt-bindings/clock/s4-clkc.h > > Filename with vendor prefix, so: > amlogic,s4-clkc.h > >> @@ -0,0 +1,146 @@ >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >> +/* >> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >> + * Author: Yu Tu <yu.tu@amlogic.com> >> + */ >> + >> +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H >> +#define _DT_BINDINGS_CLOCK_S4_CLKC_H >> + >> +/* >> + * CLKID index values >> + */ >> + >> +#define CLKID_FIXED_PLL 1 >> +#define CLKID_FCLK_DIV2 3 >> +#define CLKID_FCLK_DIV3 5 >> +#define CLKID_FCLK_DIV4 7 >> +#define CLKID_FCLK_DIV5 9 >> +#define CLKID_FCLK_DIV7 11 > > Why these aren't continuous? IDs are expected to be incremented by 1. > All clocks have IDs, it is one big table in the driver, but we are not exposing them all. For example, with composite 'mux / div / gate' assembly, we usually need only the leaf. Same has been done for the other AML controllers: For ex: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h >> + >> +#endif /* _DT_BINDINGS_CLOCK_S4_CLKC_H */ > > > Best regards, > Krzysztof
On 28/07/2022 10:50, Jerome Brunet wrote: > > On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 28/07/2022 07:42, Yu Tu wrote: >>> Add new clock controller compatible and dt-bindings header for the >>> Everything-Else domain of the S4 SoC. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> >> >> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index c1abc53f9e91..f872d0c0c253 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* >>> F: drivers/clk/meson/ >>> F: include/dt-bindings/clock/gxbb* >>> F: include/dt-bindings/clock/meson* >>> +F: include/dt-bindings/clock/s4-clkc.h >>> >>> ARM/Amlogic Meson SoC Crypto Drivers >>> M: Corentin Labbe <clabbe@baylibre.com> >>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h >>> new file mode 100644 >>> index 000000000000..b686c8877419 >>> --- /dev/null >>> +++ b/include/dt-bindings/clock/s4-clkc.h >> >> Filename with vendor prefix, so: >> amlogic,s4-clkc.h >> >>> @@ -0,0 +1,146 @@ >>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>> +/* >>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>> + * Author: Yu Tu <yu.tu@amlogic.com> >>> + */ >>> + >>> +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H >>> +#define _DT_BINDINGS_CLOCK_S4_CLKC_H >>> + >>> +/* >>> + * CLKID index values >>> + */ >>> + >>> +#define CLKID_FIXED_PLL 1 >>> +#define CLKID_FCLK_DIV2 3 >>> +#define CLKID_FCLK_DIV3 5 >>> +#define CLKID_FCLK_DIV4 7 >>> +#define CLKID_FCLK_DIV5 9 >>> +#define CLKID_FCLK_DIV7 11 >> >> Why these aren't continuous? IDs are expected to be incremented by 1. >> > > All clocks have IDs, it is one big table in the driver, but we are not exposing them all. > For example, with composite 'mux / div / gate' assembly, we usually need > only the leaf. I understand you do not expose them all, but that is not the reason to increment ID by 2 or 3... Otherwise these are not IDs and you are not expected to put register offsets into the bindings (you do not bindings in such case). > Same has been done for the other AML controllers: > For ex: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h This cannot be fixed now, but it is very poor argument. Like saying "we had a bug in other driver, so we implemented the bug here as well". Best regards, Krzysztof
On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 28/07/2022 10:50, Jerome Brunet wrote: >> >> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >> >>> On 28/07/2022 07:42, Yu Tu wrote: [...] >>>> +/* >>>> + * CLKID index values >>>> + */ >>>> + >>>> +#define CLKID_FIXED_PLL 1 >>>> +#define CLKID_FCLK_DIV2 3 >>>> +#define CLKID_FCLK_DIV3 5 >>>> +#define CLKID_FCLK_DIV4 7 >>>> +#define CLKID_FCLK_DIV5 9 >>>> +#define CLKID_FCLK_DIV7 11 >>> >>> Why these aren't continuous? IDs are expected to be incremented by 1. >>> >> >> All clocks have IDs, it is one big table in the driver, but we are not exposing them all. >> For example, with composite 'mux / div / gate' assembly, we usually need >> only the leaf. > > I understand you do not expose them all, but that is not the reason to > increment ID by 2 or 3... Otherwise these are not IDs and you are not > expected to put register offsets into the bindings (you do not bindings > in such case). Why is it not an IDs if it not continuous in the bindings ? If there is technical reason, we'll probably end up exposing everything. It would not be a dramatic change. I asked for this over v1 because we have done that is the past and I think it makes sense. I'm happy to be convinced to do things differently. Just looking for the technical reason that require contiuous exposed IDs. The other IDs exists, but we do not expose them as bindings. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125 > > >> Same has been done for the other AML controllers: >> For ex: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h > > This cannot be fixed now, but it is very poor argument. Like saying "we > had a bug in other driver, so we implemented the bug here as well". I agree, "done before" is not a good argument. I was trying to provide a better picutre. I'm just surprised to have this new requirement that IDs have to be incremented by 1 (in the bindings) and I'd like to understand why what we had done could be considered a bug now. For example the simple-reset driver compute the reset offset from the IDs: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/reset-simple.c There might be holes in the IDs if not all bits have reset maps. I don't think that would be a bug either. > > Best regards, > Krzysztof
On 28/07/2022 11:09, Jerome Brunet wrote: > > On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 28/07/2022 10:50, Jerome Brunet wrote: >>> >>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 28/07/2022 07:42, Yu Tu wrote: > [...] >>>>> +/* >>>>> + * CLKID index values >>>>> + */ >>>>> + >>>>> +#define CLKID_FIXED_PLL 1 >>>>> +#define CLKID_FCLK_DIV2 3 >>>>> +#define CLKID_FCLK_DIV3 5 >>>>> +#define CLKID_FCLK_DIV4 7 >>>>> +#define CLKID_FCLK_DIV5 9 >>>>> +#define CLKID_FCLK_DIV7 11 >>>> >>>> Why these aren't continuous? IDs are expected to be incremented by 1. >>>> >>> >>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all. >>> For example, with composite 'mux / div / gate' assembly, we usually need >>> only the leaf. >> >> I understand you do not expose them all, but that is not the reason to >> increment ID by 2 or 3... Otherwise these are not IDs and you are not >> expected to put register offsets into the bindings (you do not bindings >> in such case). > > Why is it not an IDs if it not continuous in the bindings ? > > If there is technical reason, we'll probably end up exposing everything. It > would not be a dramatic change. I asked for this over v1 because we have > done that is the past and I think it makes sense. > > I'm happy to be convinced to do things differently. Just looking for the > technical reason that require contiuous exposed IDs. > > The other IDs exists, but we do not expose them as bindings. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125 https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/ https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/ https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/ The IDs are abstract numbers, where the number does not matter because it is not tied to driver implementation or device programming model. The driver maps ID to respective clock. Using some meaningful numbers as these IDs, means you tied bindings to your implementation and any change in implementation requires change in the bindings. This contradicts the idea of bindings. > >> >> >>> Same has been done for the other AML controllers: >>> For ex: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h >> >> This cannot be fixed now, but it is very poor argument. Like saying "we >> had a bug in other driver, so we implemented the bug here as well". > > I agree, "done before" is not a good argument. I was trying to provide a > better picutre. I'm just surprised to have this new requirement that IDs > have to be incremented by 1 (in the bindings) and I'd like to understand > why what we had done could be considered a bug now. It was always, just no one ever enforced it. And almost all clock and reset providers follow it. There are just literally few exceptions. > For example the simple-reset driver compute the reset offset from the IDs: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/reset-simple.c This is one of the exceptions where it actually made sense, but I would argue it still contradicts the bindings. You have now binding which is tied to both Linux implementation and to device programming model. However fixing it would require creating huge mapping tables for each SoC, so obviously this exception is quite reasonable. Clock drivers require tables and translation anyway. Almost all clock drivers did it, so such exception is not justified. > There might be holes in the IDs if not all bits have reset maps. > I don't think that would be a bug either. Bug was of course highly exaggerated example. :) Best regards, Krzysztof
On Thu 28 Jul 2022 at 11:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 28/07/2022 11:09, Jerome Brunet wrote: >> >> On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >> >>> On 28/07/2022 10:50, Jerome Brunet wrote: >>>> >>>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>>> On 28/07/2022 07:42, Yu Tu wrote: >> [...] >>>>>> +/* >>>>>> + * CLKID index values >>>>>> + */ >>>>>> + >>>>>> +#define CLKID_FIXED_PLL 1 >>>>>> +#define CLKID_FCLK_DIV2 3 >>>>>> +#define CLKID_FCLK_DIV3 5 >>>>>> +#define CLKID_FCLK_DIV4 7 >>>>>> +#define CLKID_FCLK_DIV5 9 >>>>>> +#define CLKID_FCLK_DIV7 11 >>>>> >>>>> Why these aren't continuous? IDs are expected to be incremented by 1. >>>>> >>>> >>>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all. >>>> For example, with composite 'mux / div / gate' assembly, we usually need >>>> only the leaf. >>> >>> I understand you do not expose them all, but that is not the reason to >>> increment ID by 2 or 3... Otherwise these are not IDs and you are not >>> expected to put register offsets into the bindings (you do not bindings >>> in such case). >> >> Why is it not an IDs if it not continuous in the bindings ? >> >> If there is technical reason, we'll probably end up exposing everything. It >> would not be a dramatic change. I asked for this over v1 because we have >> done that is the past and I think it makes sense. >> >> I'm happy to be convinced to do things differently. Just looking for the >> technical reason that require contiuous exposed IDs. >> >> The other IDs exists, but we do not expose them as bindings. >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125 > > https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/ > > https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/ > > https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/ > > The IDs are abstract numbers, where the number does not matter because > it is not tied to driver implementation or device programming model. The > driver maps ID to respective clock. > > Using some meaningful numbers as these IDs, means you tied bindings to > your implementation and any change in implementation requires change in > the bindings. This contradicts the idea of bindings. > I totally agree. Bindings ID are abstract numbers. We do follow that. We even document it: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n118 It is just a choice to not expose some IDs. It is not tied to the implementation at all. I think we actually follow the rules and the idea behind it. We can expose then all If you still think what we are doing is not appropriate. I'd like things to be consistent though. So if the decision is to expose everything, I'll probably end up doing the same for the old SoCs.
Hi Krzysztof, Thanks for your reply. On 2022/7/28 16:41, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 28/07/2022 07:42, Yu Tu wrote: >> Add new clock controller compatible and dt-bindings header for the >> Everything-Else domain of the S4 SoC. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> > > > >> diff --git a/MAINTAINERS b/MAINTAINERS >> index c1abc53f9e91..f872d0c0c253 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* >> F: drivers/clk/meson/ >> F: include/dt-bindings/clock/gxbb* >> F: include/dt-bindings/clock/meson* >> +F: include/dt-bindings/clock/s4-clkc.h >> >> ARM/Amlogic Meson SoC Crypto Drivers >> M: Corentin Labbe <clabbe@baylibre.com> >> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h >> new file mode 100644 >> index 000000000000..b686c8877419 >> --- /dev/null >> +++ b/include/dt-bindings/clock/s4-clkc.h > > Filename with vendor prefix, so: > amlogic,s4-clkc.h It's fine with me. It's mainly Jerome's opinion. > >> @@ -0,0 +1,146 @@ >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >> +/* >> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >> + * Author: Yu Tu <yu.tu@amlogic.com> >> + */ >> + >> +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H >> +#define _DT_BINDINGS_CLOCK_S4_CLKC_H >> + >> +/* >> + * CLKID index values >> + */ >> + >> +#define CLKID_FIXED_PLL 1 >> +#define CLKID_FCLK_DIV2 3 >> +#define CLKID_FCLK_DIV3 5 >> +#define CLKID_FCLK_DIV4 7 >> +#define CLKID_FCLK_DIV5 9 >> +#define CLKID_FCLK_DIV7 11 > > Why these aren't continuous? IDs are expected to be incremented by 1. > >> + >> +#endif /* _DT_BINDINGS_CLOCK_S4_CLKC_H */ > > > Best regards, > Krzysztof > > .
On 28/07/2022 11:54, Jerome Brunet wrote: > > On Thu 28 Jul 2022 at 11:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 28/07/2022 11:09, Jerome Brunet wrote: >>> >>> On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 28/07/2022 10:50, Jerome Brunet wrote: >>>>> >>>>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>>> >>>>>> On 28/07/2022 07:42, Yu Tu wrote: >>> [...] >>>>>>> +/* >>>>>>> + * CLKID index values >>>>>>> + */ >>>>>>> + >>>>>>> +#define CLKID_FIXED_PLL 1 >>>>>>> +#define CLKID_FCLK_DIV2 3 >>>>>>> +#define CLKID_FCLK_DIV3 5 >>>>>>> +#define CLKID_FCLK_DIV4 7 >>>>>>> +#define CLKID_FCLK_DIV5 9 >>>>>>> +#define CLKID_FCLK_DIV7 11 >>>>>> >>>>>> Why these aren't continuous? IDs are expected to be incremented by 1. >>>>>> >>>>> >>>>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all. >>>>> For example, with composite 'mux / div / gate' assembly, we usually need >>>>> only the leaf. >>>> >>>> I understand you do not expose them all, but that is not the reason to >>>> increment ID by 2 or 3... Otherwise these are not IDs and you are not >>>> expected to put register offsets into the bindings (you do not bindings >>>> in such case). >>> >>> Why is it not an IDs if it not continuous in the bindings ? >>> >>> If there is technical reason, we'll probably end up exposing everything. It >>> would not be a dramatic change. I asked for this over v1 because we have >>> done that is the past and I think it makes sense. >>> >>> I'm happy to be convinced to do things differently. Just looking for the >>> technical reason that require contiuous exposed IDs. >>> >>> The other IDs exists, but we do not expose them as bindings. >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125 >> >> https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/ >> >> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/ >> >> https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/ >> >> The IDs are abstract numbers, where the number does not matter because >> it is not tied to driver implementation or device programming model. The >> driver maps ID to respective clock. >> >> Using some meaningful numbers as these IDs, means you tied bindings to >> your implementation and any change in implementation requires change in >> the bindings. This contradicts the idea of bindings. >> > > I totally agree. Bindings ID are abstract numbers. > We do follow that. We even document it: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n118 > > It is just a choice to not expose some IDs. > It is not tied to the implementation at all. > I think we actually follow the rules and the idea behind it. > > We can expose then all If you still think what we are doing is not appropriate. No, no need. You are right and I took your not-by-one-increment-ID by other approaches I saw. The IDs do not have to be incremental, they should not be tied to programming model. You have it done and documented, so thanks for explanation: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 28/07/2022 12:05, Yu Tu wrote: > Hi Krzysztof, > Thanks for your reply. > > On 2022/7/28 16:41, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> >> On 28/07/2022 07:42, Yu Tu wrote: >>> Add new clock controller compatible and dt-bindings header for the >>> Everything-Else domain of the S4 SoC. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> >> >> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index c1abc53f9e91..f872d0c0c253 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* >>> F: drivers/clk/meson/ >>> F: include/dt-bindings/clock/gxbb* >>> F: include/dt-bindings/clock/meson* >>> +F: include/dt-bindings/clock/s4-clkc.h >>> >>> ARM/Amlogic Meson SoC Crypto Drivers >>> M: Corentin Labbe <clabbe@baylibre.com> >>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h >>> new file mode 100644 >>> index 000000000000..b686c8877419 >>> --- /dev/null >>> +++ b/include/dt-bindings/clock/s4-clkc.h >> >> Filename with vendor prefix, so: >> amlogic,s4-clkc.h > It's fine with me. It's mainly Jerome's opinion. To clarify: I understand such naming might bring inconsistency, but we want to bring some order in the bindings directories. They keep growing and at some point the model names might start conflicting. Best regards, Krzysztof
On 2022/7/28 18:09, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 28/07/2022 12:05, Yu Tu wrote: >> Hi Krzysztof, >> Thanks for your reply. >> >> On 2022/7/28 16:41, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On 28/07/2022 07:42, Yu Tu wrote: >>>> Add new clock controller compatible and dt-bindings header for the >>>> Everything-Else domain of the S4 SoC. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> >>> >>> >>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>> index c1abc53f9e91..f872d0c0c253 100644 >>>> --- a/MAINTAINERS >>>> +++ b/MAINTAINERS >>>> @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* >>>> F: drivers/clk/meson/ >>>> F: include/dt-bindings/clock/gxbb* >>>> F: include/dt-bindings/clock/meson* >>>> +F: include/dt-bindings/clock/s4-clkc.h >>>> >>>> ARM/Amlogic Meson SoC Crypto Drivers >>>> M: Corentin Labbe <clabbe@baylibre.com> >>>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h >>>> new file mode 100644 >>>> index 000000000000..b686c8877419 >>>> --- /dev/null >>>> +++ b/include/dt-bindings/clock/s4-clkc.h >>> >>> Filename with vendor prefix, so: >>> amlogic,s4-clkc.h >> It's fine with me. It's mainly Jerome's opinion. > > To clarify: I understand such naming might bring inconsistency, but we > want to bring some order in the bindings directories. They keep growing > and at some point the model names might start conflicting. If Jerome agrees, I will change it according to your opinion and make another edition. > > > Best regards, > Krzysztof > > .
On Thu 28 Jul 2022 at 18:19, Yu Tu <yu.tu@amlogic.com> wrote: > On 2022/7/28 18:09, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> On 28/07/2022 12:05, Yu Tu wrote: >>> Hi Krzysztof, >>> Thanks for your reply. >>> >>> On 2022/7/28 16:41, Krzysztof Kozlowski wrote: >>>> [ EXTERNAL EMAIL ] >>>> >>>> On 28/07/2022 07:42, Yu Tu wrote: >>>>> Add new clock controller compatible and dt-bindings header for the >>>>> Everything-Else domain of the S4 SoC. >>>>> >>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> >>>> >>>> >>>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>>> index c1abc53f9e91..f872d0c0c253 100644 >>>>> --- a/MAINTAINERS >>>>> +++ b/MAINTAINERS >>>>> @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* >>>>> F: drivers/clk/meson/ >>>>> F: include/dt-bindings/clock/gxbb* >>>>> F: include/dt-bindings/clock/meson* >>>>> +F: include/dt-bindings/clock/s4-clkc.h >>>>> ARM/Amlogic Meson SoC Crypto Drivers >>>>> M: Corentin Labbe <clabbe@baylibre.com> >>>>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h >>>>> new file mode 100644 >>>>> index 000000000000..b686c8877419 >>>>> --- /dev/null >>>>> +++ b/include/dt-bindings/clock/s4-clkc.h >>>> >>>> Filename with vendor prefix, so: >>>> amlogic,s4-clkc.h >>> It's fine with me. It's mainly Jerome's opinion. >> To clarify: I understand such naming might bring inconsistency, but we >> want to bring some order in the bindings directories. They keep growing >> and at some point the model names might start conflicting. > If Jerome agrees, I will change it according to your opinion and make > another edition. I'm aligned with Krzysztof on this. Please add the vendor prefix. It was mistake to omit the vendor prefix. Unfortunately, I don't think we can fix the old bindings now. > >> >> Best regards, >> Krzysztof >> .
On 2022/7/28 19:48, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Thu 28 Jul 2022 at 18:19, Yu Tu <yu.tu@amlogic.com> wrote: > >> On 2022/7/28 18:09, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> On 28/07/2022 12:05, Yu Tu wrote: >>>> Hi Krzysztof, >>>> Thanks for your reply. >>>> >>>> On 2022/7/28 16:41, Krzysztof Kozlowski wrote: >>>>> [ EXTERNAL EMAIL ] >>>>> >>>>> On 28/07/2022 07:42, Yu Tu wrote: >>>>>> Add new clock controller compatible and dt-bindings header for the >>>>>> Everything-Else domain of the S4 SoC. >>>>>> >>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>> >>>>> >>>>> >>>>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>>>> index c1abc53f9e91..f872d0c0c253 100644 >>>>>> --- a/MAINTAINERS >>>>>> +++ b/MAINTAINERS >>>>>> @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* >>>>>> F: drivers/clk/meson/ >>>>>> F: include/dt-bindings/clock/gxbb* >>>>>> F: include/dt-bindings/clock/meson* >>>>>> +F: include/dt-bindings/clock/s4-clkc.h >>>>>> ARM/Amlogic Meson SoC Crypto Drivers >>>>>> M: Corentin Labbe <clabbe@baylibre.com> >>>>>> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h >>>>>> new file mode 100644 >>>>>> index 000000000000..b686c8877419 >>>>>> --- /dev/null >>>>>> +++ b/include/dt-bindings/clock/s4-clkc.h >>>>> >>>>> Filename with vendor prefix, so: >>>>> amlogic,s4-clkc.h >>>> It's fine with me. It's mainly Jerome's opinion. >>> To clarify: I understand such naming might bring inconsistency, but we >>> want to bring some order in the bindings directories. They keep growing >>> and at some point the model names might start conflicting. >> If Jerome agrees, I will change it according to your opinion and make >> another edition. > > I'm aligned with Krzysztof on this. Please add the vendor prefix. I will add it in next version. > > It was mistake to omit the vendor prefix. Unfortunately, I don't think > we can fix the old bindings now. > >> >>> >>> Best regards, >>> Krzysztof >>> . > > .
diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt index 7ccecd5c02c1..301b43dea912 100644 --- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt +++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt @@ -12,6 +12,7 @@ Required Properties: "amlogic,g12a-clkc" for G12A SoC. "amlogic,g12b-clkc" for G12B SoC. "amlogic,sm1-clkc" for SM1 SoC. + "amlogic,s4-clkc" for S4 SoC. - clocks : list of clock phandle, one for each entry clock-names. - clock-names : should contain the following: * "xtal": the platform xtal diff --git a/MAINTAINERS b/MAINTAINERS index c1abc53f9e91..f872d0c0c253 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1775,6 +1775,7 @@ F: Documentation/devicetree/bindings/clock/amlogic* F: drivers/clk/meson/ F: include/dt-bindings/clock/gxbb* F: include/dt-bindings/clock/meson* +F: include/dt-bindings/clock/s4-clkc.h ARM/Amlogic Meson SoC Crypto Drivers M: Corentin Labbe <clabbe@baylibre.com> diff --git a/include/dt-bindings/clock/s4-clkc.h b/include/dt-bindings/clock/s4-clkc.h new file mode 100644 index 000000000000..b686c8877419 --- /dev/null +++ b/include/dt-bindings/clock/s4-clkc.h @@ -0,0 +1,146 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ +/* + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. + * Author: Yu Tu <yu.tu@amlogic.com> + */ + +#ifndef _DT_BINDINGS_CLOCK_S4_CLKC_H +#define _DT_BINDINGS_CLOCK_S4_CLKC_H + +/* + * CLKID index values + */ + +#define CLKID_FIXED_PLL 1 +#define CLKID_FCLK_DIV2 3 +#define CLKID_FCLK_DIV3 5 +#define CLKID_FCLK_DIV4 7 +#define CLKID_FCLK_DIV5 9 +#define CLKID_FCLK_DIV7 11 +#define CLKID_FCLK_DIV2P5 13 +#define CLKID_GP0_PLL 15 +#define CLKID_HIFI_PLL 17 +#define CLKID_HDMI_PLL 20 +#define CLKID_MPLL_50M 22 +#define CLKID_MPLL0 25 +#define CLKID_MPLL1 27 +#define CLKID_MPLL2 29 +#define CLKID_MPLL3 31 +#define CLKID_RTC_CLK 36 +#define CLKID_SYS_CLK_B_GATE 39 +#define CLKID_SYS_CLK_A_GATE 42 +#define CLKID_SYS_CLK 43 +#define CLKID_CECA_32K_CLKOUT 48 +#define CLKID_CECB_32K_CLKOUT 53 +#define CLKID_SC_CLK_GATE 56 +#define CLKID_12_24M_CLK_SEL 59 +#define CLKID_VID_PLL 62 +#define CLKID_VCLK 69 +#define CLKID_VCLK2 70 +#define CLKID_VCLK_DIV1 71 +#define CLKID_VCLK2_DIV1 76 +#define CLKID_VCLK_DIV2 81 +#define CLKID_VCLK_DIV4 82 +#define CLKID_VCLK_DIV6 83 +#define CLKID_VCLK_DIV12 84 +#define CLKID_VCLK2_DIV2 85 +#define CLKID_VCLK2_DIV4 86 +#define CLKID_VCLK2_DIV6 87 +#define CLKID_VCLK2_DIV12 88 +#define CLKID_CTS_ENCI 93 +#define CLKID_CTS_ENCP 94 +#define CLKID_CTS_VDAC 95 +#define CLKID_HDMI 99 +#define CLKID_TS_CLK_GATE 101 +#define CLKID_MALI_0 104 +#define CLKID_MALI_1 107 +#define CLKID_MALI 108 +#define CLKID_VDEC_P0 111 +#define CLKID_VDEC_P1 114 +#define CLKID_VDEC_SEL 115 +#define CLKID_HEVCF_P0 118 +#define CLKID_HEVCF_P1 121 +#define CLKID_HEVCF_SEL 122 +#define CLKID_VPU_0 125 +#define CLKID_VPU_1 128 +#define CLKID_VPU 129 +#define CLKID_VPU_CLKB_TMP 132 +#define CLKID_VPU_CLKB 134 +#define CLKID_VPU_CLKC_P0 137 +#define CLKID_VPU_CLKC_P1 140 +#define CLKID_VPU_CLKC_SEL 141 +#define CLKID_VAPB_0 144 +#define CLKID_VAPB_1 147 +#define CLKID_VAPB 148 +#define CLKID_GE2D 149 +#define CLKID_VDIN_MEAS_GATE 152 +#define CLKID_SD_EMMC_C_CLK 155 +#define CLKID_SD_EMMC_A_CLK 158 +#define CLKID_SD_EMMC_B_CLK 161 +#define CLKID_SPICC0_GATE 164 +#define CLKID_PWM_A_GATE 167 +#define CLKID_PWM_B_GATE 170 +#define CLKID_PWM_C_GATE 173 +#define CLKID_PWM_D_GATE 176 +#define CLKID_PWM_E_GATE 179 +#define CLKID_PWM_F_GATE 182 +#define CLKID_PWM_G_GATE 185 +#define CLKID_PWM_H_GATE 188 +#define CLKID_PWM_I_GATE 191 +#define CLKID_PWM_J_GATE 194 +#define CLKID_SARADC_GATE 197 +#define CLKID_GEN_GATE 200 +#define CLKID_DDR 201 +#define CLKID_DOS 202 +#define CLKID_ETHPHY 203 +#define CLKID_MALI_GATE 204 +#define CLKID_AOCPU 205 +#define CLKID_AUCPU 206 +#define CLKID_CEC 207 +#define CLKID_SD_EMMC_A 208 +#define CLKID_SD_EMMC_B 209 +#define CLKID_NAND 210 +#define CLKID_SMARTCARD 211 +#define CLKID_ACODEC 212 +#define CLKID_SPIFC 213 +#define CLKID_MSR_CLK 214 +#define CLKID_IR_CTRL 215 +#define CLKID_AUDIO 216 +#define CLKID_ETH 217 +#define CLKID_UART_A 218 +#define CLKID_UART_B 219 +#define CLKID_UART_C 220 +#define CLKID_UART_D 221 +#define CLKID_UART_E 222 +#define CLKID_AIFIFO 223 +#define CLKID_TS_DDR 224 +#define CLKID_TS_PLL 225 +#define CLKID_G2D 226 +#define CLKID_SPICC0 227 +#define CLKID_SPICC1 228 +#define CLKID_USB 229 +#define CLKID_I2C_M_A 230 +#define CLKID_I2C_M_B 231 +#define CLKID_I2C_M_C 232 +#define CLKID_I2C_M_D 233 +#define CLKID_I2C_M_E 234 +#define CLKID_HDMITX_APB 235 +#define CLKID_I2C_S_A 236 +#define CLKID_USB1_TO_DDR 237 +#define CLKID_HDCP22 238 +#define CLKID_MMC_APB 239 +#define CLKID_RSA 240 +#define CLKID_CPU_DEBUG 241 +#define CLKID_VPU_INTR 242 +#define CLKID_DEMOD 243 +#define CLKID_SAR_ADC 244 +#define CLKID_GIC 245 +#define CLKID_PWM_AB 246 +#define CLKID_PWM_CD 247 +#define CLKID_PWM_EF 248 +#define CLKID_PWM_GH 249 +#define CLKID_PWM_IJ 250 +#define CLKID_HDCP22_ESMCLK_GATE 253 +#define CLKID_HDCP22_SKPCLK_GATE 256 + +#endif /* _DT_BINDINGS_CLOCK_S4_CLKC_H */
Add new clock controller compatible and dt-bindings header for the Everything-Else domain of the S4 SoC. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- .../bindings/clock/amlogic,gxbb-clkc.txt | 1 + MAINTAINERS | 1 + include/dt-bindings/clock/s4-clkc.h | 146 ++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 include/dt-bindings/clock/s4-clkc.h