| Message ID | 1464625737-6646-3-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Geert, On 30.05.2016 18:28, Geert Uytterhoeven wrote: > Add all R-Car M3-W Clock Pulse Generator Core Clock Outputs, as listed > in Table 8.2b ("List of Clocks [R-Car M3-W]") of the R-Car Gen3 > datasheet (rev. 0.51 + Errata for Rev051 Mar 31 2016). > > Note that internal CPG clocks (S0, S1, S2, S3, SDSRC, and SSPSRC) are > not included, as they are used as internal clock sources only, and never > referenced from DT. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Tested-by: Simon Horman <horms+renesas@verge.net.au> > --- > v2: > - Add Tested-by. > --- > include/dt-bindings/clock/r8a7796-cpg-mssr.h | 69 ++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 include/dt-bindings/clock/r8a7796-cpg-mssr.h > > diff --git a/include/dt-bindings/clock/r8a7796-cpg-mssr.h b/include/dt-bindings/clock/r8a7796-cpg-mssr.h > new file mode 100644 > index 0000000000000000..1e5942695f0dd057 > --- /dev/null > +++ b/include/dt-bindings/clock/r8a7796-cpg-mssr.h > @@ -0,0 +1,69 @@ > +/* > + * Copyright (C) 2016 Renesas Electronics Corp. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#ifndef __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ > +#define __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ > + > +#include <dt-bindings/clock/renesas-cpg-mssr.h> > + > +/* r8a7796 CPG Core Clocks */ > +#define R8A7796_CLK_Z 0 > +#define R8A7796_CLK_Z2 1 > +#define R8A7796_CLK_ZR 2 > +#define R8A7796_CLK_ZG 3 > +#define R8A7796_CLK_ZTR 4 > +#define R8A7796_CLK_ZTRD2 5 > +#define R8A7796_CLK_ZT 6 > +#define R8A7796_CLK_ZX 7 > +#define R8A7796_CLK_S0D1 8 > +#define R8A7796_CLK_S0D2 9 > +#define R8A7796_CLK_S0D3 10 > +#define R8A7796_CLK_S0D4 11 > +#define R8A7796_CLK_S0D6 12 > +#define R8A7796_CLK_S0D8 13 > +#define R8A7796_CLK_S0D12 14 > +#define R8A7796_CLK_S1D1 15 > +#define R8A7796_CLK_S1D2 16 > +#define R8A7796_CLK_S1D4 17 > +#define R8A7796_CLK_S2D1 18 > +#define R8A7796_CLK_S2D2 19 > +#define R8A7796_CLK_S2D4 20 > +#define R8A7796_CLK_S3D1 21 > +#define R8A7796_CLK_S3D2 22 > +#define R8A7796_CLK_S3D4 23 > +#define R8A7796_CLK_LB 24 > +#define R8A7796_CLK_CL 25 > +#define R8A7796_CLK_ZB3 26 > +#define R8A7796_CLK_ZB3D2 27 > +#define R8A7796_CLK_ZB3D4 28 > +#define R8A7796_CLK_CR 29 > +#define R8A7796_CLK_CRD2 30 > +#define R8A7796_CLK_SD0H 31 > +#define R8A7796_CLK_SD0 32 > +#define R8A7796_CLK_SD1H 33 > +#define R8A7796_CLK_SD1 34 > +#define R8A7796_CLK_SD2H 35 > +#define R8A7796_CLK_SD2 36 > +#define R8A7796_CLK_SD3H 37 > +#define R8A7796_CLK_SD3 38 > +#define R8A7796_CLK_SSP2 39 > +#define R8A7796_CLK_SSP1 40 > +#define R8A7796_CLK_SSPRS 41 > +#define R8A7796_CLK_RPC 42 > +#define R8A7796_CLK_RPCD2 43 > +#define R8A7796_CLK_MSO 44 > +#define R8A7796_CLK_CANFD 45 > +#define R8A7796_CLK_HDMI 46 > +#define R8A7796_CLK_CSI0 47 > +#define R8A7796_CLK_CSIREF 48 > +#define R8A7796_CLK_CP 49 > +#define R8A7796_CLK_CPEX 50 > +#define R8A7796_CLK_R 51 > +#define R8A7796_CLK_OSC 52 I think we recently started a discussion to find a more clever way to avoid re-defining (copy & paste) all this R-Car3 clocks (compare [1]) where they are the same over the R-Car3 family while still being able to deal with the differences. Best regards Dirk [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/r8a7795-cpg-mssr.h -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dirk, On Tue, May 31, 2016 at 1:36 AM, Dirk Behme <dirk.behme@gmail.com> wrote: > Hi Geert, > > > On 30.05.2016 18:28, Geert Uytterhoeven wrote: >> >> Add all R-Car M3-W Clock Pulse Generator Core Clock Outputs, as listed >> in Table 8.2b ("List of Clocks [R-Car M3-W]") of the R-Car Gen3 >> datasheet (rev. 0.51 + Errata for Rev051 Mar 31 2016). >> >> Note that internal CPG clocks (S0, S1, S2, S3, SDSRC, and SSPSRC) are >> not included, as they are used as internal clock sources only, and never >> referenced from DT. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> Tested-by: Simon Horman <horms+renesas@verge.net.au> >> --- >> v2: >> - Add Tested-by. >> --- [snip] >> +#include <dt-bindings/clock/renesas-cpg-mssr.h> >> + >> +/* r8a7796 CPG Core Clocks */ >> +#define R8A7796_CLK_Z 0 >> +#define R8A7796_CLK_Z2 1 > > I think we recently started a discussion to find a more clever way to avoid > re-defining (copy & paste) all this R-Car3 clocks (compare [1]) where they > are the same over the R-Car3 family while still being able to deal with the > differences. > > [1] > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/r8a7795-cpg-mssr.h This topic is related to the rather fixed DT ABI for the CPG device. Please see the interface described in the following upstream commits: $ git log --oneline Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt | cat ca00c38 clk: shmobile: cpg-mssr: Update serial port clock in example 3686d3e clk: shmobile: Add new Renesas CPG/MSSR DT bindings $ Some years ago we discussed this consolidation topic for R-Car Gen2 and we revisited when doing various iterations for the R-Car Gen3 CPGs last year. Using a single per-family shared range of clocks was considered both times, however when comparing the pros and cons it became evident that best practice with DT tends to be to not speculate and go with what we know based on existing documentation. Like you may have experienced, the SoC documentation is slightly changing over time. Also the number of SoCs in a certain family is clearly being extended over time. The direction where things are going is not known during initial design phase. Because of this we make use of known-to-be-fixed ids such as SoC-specific part numbers instead of speculating with family DT strings where the meaning tend to change over time. If we would go with consolidating the clock indices from per-SoC to per-family then please note that this does not affect the run time size of the DTB. In the kernel source code we have some level of agreed duplication where DT compat strings and such are chosen to be duplicated per-device instead of trying to make the code more compact. I believe our build machines are snappy enough to handle a couple of KiB of strings worst case. Because of the unchanged DTB size this discussion is purely a matter of how to maintain the code. So the maintainers need to decide what level of duplication is needed. Perhaps this topic should be revisited, and maybe meeting face-to-face would help? Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 30.05.2016 18:36, Dirk Behme wrote: > Hi Geert, > > On 30.05.2016 18:28, Geert Uytterhoeven wrote: >> Add all R-Car M3-W Clock Pulse Generator Core Clock Outputs, as listed >> in Table 8.2b ("List of Clocks [R-Car M3-W]") of the R-Car Gen3 >> datasheet (rev. 0.51 + Errata for Rev051 Mar 31 2016). >> >> Note that internal CPG clocks (S0, S1, S2, S3, SDSRC, and SSPSRC) are >> not included, as they are used as internal clock sources only, and never >> referenced from DT. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> Tested-by: Simon Horman <horms+renesas@verge.net.au> >> --- >> v2: >> - Add Tested-by. >> --- >> include/dt-bindings/clock/r8a7796-cpg-mssr.h | 69 >> ++++++++++++++++++++++++++++ >> 1 file changed, 69 insertions(+) >> create mode 100644 include/dt-bindings/clock/r8a7796-cpg-mssr.h >> >> diff --git a/include/dt-bindings/clock/r8a7796-cpg-mssr.h >> b/include/dt-bindings/clock/r8a7796-cpg-mssr.h >> new file mode 100644 >> index 0000000000000000..1e5942695f0dd057 >> --- /dev/null >> +++ b/include/dt-bindings/clock/r8a7796-cpg-mssr.h >> @@ -0,0 +1,69 @@ >> +/* >> + * Copyright (C) 2016 Renesas Electronics Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> +#ifndef __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ >> +#define __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ >> + >> +#include <dt-bindings/clock/renesas-cpg-mssr.h> >> + >> +/* r8a7796 CPG Core Clocks */ >> +#define R8A7796_CLK_Z 0 >> +#define R8A7796_CLK_Z2 1 >> +#define R8A7796_CLK_ZR 2 >> +#define R8A7796_CLK_ZG 3 >> +#define R8A7796_CLK_ZTR 4 >> +#define R8A7796_CLK_ZTRD2 5 >> +#define R8A7796_CLK_ZT 6 >> +#define R8A7796_CLK_ZX 7 >> +#define R8A7796_CLK_S0D1 8 >> +#define R8A7796_CLK_S0D2 9 >> +#define R8A7796_CLK_S0D3 10 >> +#define R8A7796_CLK_S0D4 11 >> +#define R8A7796_CLK_S0D6 12 >> +#define R8A7796_CLK_S0D8 13 >> +#define R8A7796_CLK_S0D12 14 >> +#define R8A7796_CLK_S1D1 15 >> +#define R8A7796_CLK_S1D2 16 >> +#define R8A7796_CLK_S1D4 17 >> +#define R8A7796_CLK_S2D1 18 >> +#define R8A7796_CLK_S2D2 19 >> +#define R8A7796_CLK_S2D4 20 >> +#define R8A7796_CLK_S3D1 21 >> +#define R8A7796_CLK_S3D2 22 >> +#define R8A7796_CLK_S3D4 23 >> +#define R8A7796_CLK_LB 24 >> +#define R8A7796_CLK_CL 25 >> +#define R8A7796_CLK_ZB3 26 >> +#define R8A7796_CLK_ZB3D2 27 >> +#define R8A7796_CLK_ZB3D4 28 >> +#define R8A7796_CLK_CR 29 >> +#define R8A7796_CLK_CRD2 30 >> +#define R8A7796_CLK_SD0H 31 >> +#define R8A7796_CLK_SD0 32 >> +#define R8A7796_CLK_SD1H 33 >> +#define R8A7796_CLK_SD1 34 >> +#define R8A7796_CLK_SD2H 35 >> +#define R8A7796_CLK_SD2 36 >> +#define R8A7796_CLK_SD3H 37 >> +#define R8A7796_CLK_SD3 38 >> +#define R8A7796_CLK_SSP2 39 >> +#define R8A7796_CLK_SSP1 40 >> +#define R8A7796_CLK_SSPRS 41 >> +#define R8A7796_CLK_RPC 42 >> +#define R8A7796_CLK_RPCD2 43 >> +#define R8A7796_CLK_MSO 44 >> +#define R8A7796_CLK_CANFD 45 >> +#define R8A7796_CLK_HDMI 46 >> +#define R8A7796_CLK_CSI0 47 >> +#define R8A7796_CLK_CSIREF 48 >> +#define R8A7796_CLK_CP 49 >> +#define R8A7796_CLK_CPEX 50 >> +#define R8A7796_CLK_R 51 >> +#define R8A7796_CLK_OSC 52 > > > I think we recently started a discussion to find a more clever way to > avoid re-defining (copy & paste) all this R-Car3 clocks (compare [1]) > where they are the same over the R-Car3 family while still being able to > deal with the differences. > > Best regards > > Dirk > > [1] > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/r8a7795-cpg-mssr.h What's the status of the discussion I mentioned above? Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dirk, On Mon, Jun 6, 2016 at 2:03 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote: > On 30.05.2016 18:36, Dirk Behme wrote: >> On 30.05.2016 18:28, Geert Uytterhoeven wrote: >>> Add all R-Car M3-W Clock Pulse Generator Core Clock Outputs, as listed >>> in Table 8.2b ("List of Clocks [R-Car M3-W]") of the R-Car Gen3 >>> datasheet (rev. 0.51 + Errata for Rev051 Mar 31 2016). >>> >>> Note that internal CPG clocks (S0, S1, S2, S3, SDSRC, and SSPSRC) are >>> not included, as they are used as internal clock sources only, and never >>> referenced from DT. >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> Tested-by: Simon Horman <horms+renesas@verge.net.au> >>> --- >>> v2: >>> - Add Tested-by. >>> --- >>> include/dt-bindings/clock/r8a7796-cpg-mssr.h | 69 >>> ++++++++++++++++++++++++++++ >>> 1 file changed, 69 insertions(+) >>> create mode 100644 include/dt-bindings/clock/r8a7796-cpg-mssr.h >>> >>> diff --git a/include/dt-bindings/clock/r8a7796-cpg-mssr.h >>> b/include/dt-bindings/clock/r8a7796-cpg-mssr.h >>> new file mode 100644 >>> index 0000000000000000..1e5942695f0dd057 >>> --- /dev/null >>> +++ b/include/dt-bindings/clock/r8a7796-cpg-mssr.h >>> @@ -0,0 +1,69 @@ >>> +/* >>> + * Copyright (C) 2016 Renesas Electronics Corp. >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation; either version 2 of the License, or >>> + * (at your option) any later version. >>> + */ >>> +#ifndef __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ >>> +#define __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ >>> + >>> +#include <dt-bindings/clock/renesas-cpg-mssr.h> >>> + >>> +/* r8a7796 CPG Core Clocks */ >>> +#define R8A7796_CLK_Z 0 [...] >> I think we recently started a discussion to find a more clever way to >> avoid re-defining (copy & paste) all this R-Car3 clocks (compare [1]) >> where they are the same over the R-Car3 family while still being able to >> deal with the differences. >> >> Best regards >> >> Dirk >> >> [1] >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/r8a7795-cpg-mssr.h > > What's the status of the discussion I mentioned above? As mentioned in that thread,the CPGs in r8a7795 and r8a7796 provide slightly different sets of clocks. Future members of the R-Car Gen3 family may provide the same or different sets of clocks, we don't know. As Magnus already mentions, we try to stay as close as possible to the datasheet (which is unfortunately a moving target, too). For CPG Core Clocks, the datasheet only provides us with a list of named clocks. There are no fixed numbers. So either we refer to clocks by name, or by coming up with our own numbering scheme (which has to be a stable set of numbers, i.e. append only). For MSSR (Module) Clocks, the datasheet does provide us with numbers (MSTP register index + bit index inside the register). The way the CPG/MSSR drivers handles these clocks was heavily influenced by the experience we gained with the Common Clock Framework and DT on R-Car Gen2. R-Car Gen2 described all clocks and their registers in DT. The goal (utopia?) here was to handle all SoCs from the family with a single driver, provided it was fed with the right description in DT. For CPG Core Clocks, this lead to a mix of: - Nodes for fixed factor clocks, - Nodes for variable factor clocks, specifying a register to operate on, - Special CPG clocks that couldn't be handled by the above, using a common (family-specific) list of definitions for clocks, that had to be extended constantly. For MSSR Clocks (called "MSTP" for historical reasons), each set of 32 clocks had its own node, with multiple registers, and three separate arrays for parent clocks, clock indices, and clock names, that had to be kept in sync. The clock indices were defines, using numbers from the datasheet, but they were still easy to abuse (which register does the define apply to?). As the CCF was quite new and best practices were still under development, all of this was difficult to define up-front. Due to the complexity, it was also hard to review and maintain, leading to many errors. The arbitrary (grown organically) offsets for the various MSSR-related registers also made it hard to ever add module reset support. Hence the call for a new framework, designed in close collaboration with the clock maintainer, and implemented in the CPG/MSSR driver. The goals were: - Make the DT part user friendly, reviewer friendly, and maintainer friendly, as it provides a stable ABI, and thus must be obviously correct from the beginning, - Hide complexity and internals in the driver, as this can be reworked and extended at any time, without breaking the DT ABI, - Hence, describe CPG/MSSR as a single simple block in DT, - Support both new and existing SoCs (PoC was done for r8a7791), - Allow for adding module reset support (the "SR" part) later. Hence for the CPG Core Clocks, we wanted a simple append-only list of defines for all clocks (and only those, as trying to use another clock is a bug) that exist on a specific SoC. This allows to list all existing clocks in the bindings include up-front. A mapping from SoC-specific numbers to family-specific clocks is handled by the tables in the driver, to promote code reuse while keeping specialization. For MSSR Clocks, we solved the disconnection of register and bit indices by going with a single number. We also removed the defines, as it's actually easier to review .dtsi updates if the MSSR clocks are directly referred to by number, than by an intermediate define. I hope this explanation helps in understanding the design choices we made. Given the small amount of work needed to bootstrap r8a7796, I think they still hold on their promises. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 06.06.2016 14:59, Geert Uytterhoeven wrote: > Hi Dirk, > > On Mon, Jun 6, 2016 at 2:03 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote: >> On 30.05.2016 18:36, Dirk Behme wrote: >>> On 30.05.2016 18:28, Geert Uytterhoeven wrote: >>>> Add all R-Car M3-W Clock Pulse Generator Core Clock Outputs, as listed >>>> in Table 8.2b ("List of Clocks [R-Car M3-W]") of the R-Car Gen3 >>>> datasheet (rev. 0.51 + Errata for Rev051 Mar 31 2016). >>>> >>>> Note that internal CPG clocks (S0, S1, S2, S3, SDSRC, and SSPSRC) are >>>> not included, as they are used as internal clock sources only, and never >>>> referenced from DT. >>>> >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>>> Tested-by: Simon Horman <horms+renesas@verge.net.au> >>>> --- >>>> v2: >>>> - Add Tested-by. >>>> --- >>>> include/dt-bindings/clock/r8a7796-cpg-mssr.h | 69 >>>> ++++++++++++++++++++++++++++ >>>> 1 file changed, 69 insertions(+) >>>> create mode 100644 include/dt-bindings/clock/r8a7796-cpg-mssr.h >>>> >>>> diff --git a/include/dt-bindings/clock/r8a7796-cpg-mssr.h >>>> b/include/dt-bindings/clock/r8a7796-cpg-mssr.h >>>> new file mode 100644 >>>> index 0000000000000000..1e5942695f0dd057 >>>> --- /dev/null >>>> +++ b/include/dt-bindings/clock/r8a7796-cpg-mssr.h >>>> @@ -0,0 +1,69 @@ >>>> +/* >>>> + * Copyright (C) 2016 Renesas Electronics Corp. >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License as published by >>>> + * the Free Software Foundation; either version 2 of the License, or >>>> + * (at your option) any later version. >>>> + */ >>>> +#ifndef __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ >>>> +#define __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ >>>> + >>>> +#include <dt-bindings/clock/renesas-cpg-mssr.h> >>>> + >>>> +/* r8a7796 CPG Core Clocks */ >>>> +#define R8A7796_CLK_Z 0 > > [...] > >>> I think we recently started a discussion to find a more clever way to >>> avoid re-defining (copy & paste) all this R-Car3 clocks (compare [1]) >>> where they are the same over the R-Car3 family while still being able to >>> deal with the differences. >>> >>> Best regards >>> >>> Dirk >>> >>> [1] >>> >>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/r8a7795-cpg-mssr.h >> >> What's the status of the discussion I mentioned above? > > As mentioned in that thread,the CPGs in r8a7795 and r8a7796 provide > slightly different sets of clocks. Future members of the R-Car Gen3 > family may provide the same or different sets of clocks, we don't know. > > As Magnus already mentions, we try to stay as close as possible to the > datasheet (which is unfortunately a moving target, too). > > For CPG Core Clocks, the datasheet only provides us with a list of named > clocks. There are no fixed numbers. So either we refer to clocks by > name, or by coming up with our own numbering scheme (which has to be a > stable set of numbers, i.e. append only). > > For MSSR (Module) Clocks, the datasheet does provide us with numbers > (MSTP register index + bit index inside the register). > > The way the CPG/MSSR drivers handles these clocks was heavily influenced > by the experience we gained with the Common Clock Framework and DT on > R-Car Gen2. > > R-Car Gen2 described all clocks and their registers in DT. The goal > (utopia?) here was to handle all SoCs from the family with a single > driver, provided it was fed with the right description in DT. > > For CPG Core Clocks, this lead to a mix of: > - Nodes for fixed factor clocks, > - Nodes for variable factor clocks, specifying a register to operate > on, > - Special CPG clocks that couldn't be handled by the above, using a > common (family-specific) list of definitions for clocks, that had to > be extended constantly. > > For MSSR Clocks (called "MSTP" for historical reasons), each set of 32 > clocks had its own node, with multiple registers, and three separate > arrays for parent clocks, clock indices, and clock names, that had to be > kept in sync. The clock indices were defines, using numbers from the > datasheet, but they were still easy to abuse (which register does the > define apply to?). > > As the CCF was quite new and best practices were still under > development, all of this was difficult to define up-front. > Due to the complexity, it was also hard to review and maintain, leading > to many errors. > The arbitrary (grown organically) offsets for the various MSSR-related > registers also made it hard to ever add module reset support. > > Hence the call for a new framework, designed in close collaboration with the > clock maintainer, and implemented in the CPG/MSSR driver. > The goals were: > - Make the DT part user friendly, reviewer friendly, and maintainer > friendly, as it provides a stable ABI, and thus must be obviously > correct from the beginning, > - Hide complexity and internals in the driver, as this can be reworked > and extended at any time, without breaking the DT ABI, > - Hence, describe CPG/MSSR as a single simple block in DT, > - Support both new and existing SoCs (PoC was done for r8a7791), > - Allow for adding module reset support (the "SR" part) later. > > Hence for the CPG Core Clocks, we wanted a simple append-only list of > defines for all clocks (and only those, as trying to use another clock > is a bug) that exist on a specific SoC. This allows to list all existing > clocks in the bindings include up-front. > A mapping from SoC-specific numbers to family-specific clocks is handled > by the tables in the driver, to promote code reuse while keeping > specialization. > > For MSSR Clocks, we solved the disconnection of register and bit indices > by going with a single number. We also removed the defines, as it's > actually easier to review .dtsi updates if the MSSR clocks are directly > referred to by number, than by an intermediate define. > > I hope this explanation helps in understanding the design choices we > made. Many thanks for taking the time for this long background information! Please note that I don't doubt any of the design choices done. I think I just want to discuss if we have a clever idea to further improve one detail. That is, if we have a clever idea to avoid the copy & paste between the family members using anything like a hierarchical way of defining the clocks in r8a779x-cpg-mssr.h. > Given the small amount of work needed to bootstrap r8a7796, I > think they still hold on their promises. Well, regarding the r8a779x-cpg-mssr.h the small amount of work needed isn't a really good argument if you are good with cp & sed for the copy & paste done ;) What I fear we end up the way we are doing the copy & paste r8a779x-cpg-mssr.h is having 5 or 6 or even more of these files, all > 90% identical. And once you have to change anything, you either have to change all these files. Or you miss anything, ending up with subtle bugs when one SoC does behave differently than an other one. Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dirk, On Tue, Jun 7, 2016 at 9:53 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: > I think I just want to discuss if we have a clever idea to further improve > one detail. That is, if we have a clever idea to avoid the copy & paste > between the family members using anything like a hierarchical way of > defining the clocks in r8a779x-cpg-mssr.h. > >> Given the small amount of work needed to bootstrap r8a7796, I >> think they still hold on their promises. > > Well, regarding the r8a779x-cpg-mssr.h the small amount of work needed isn't > a really good argument if you are good with cp & sed for the copy & paste > done ;) They're not really created by cp & sed, as they must match the table in the datasheet (the latter may have been created by copy & paste though :-) > What I fear we end up the way we are doing the copy & paste > r8a779x-cpg-mssr.h is having 5 or 6 or even more of these files, all > 90% > identical. And once you have to change anything, you either have to change > all these files. Or you miss anything, ending up with subtle bugs when one > SoC does behave differently than an other one. The point is these files are stable ABI: no single value can be changed. No value can be reused. Only new values can be appended at the bottom (if a newer revision of the datasheet documents more clocks than the old version, which happens from time to time). IMHO a hierarchical way of defining the clocks has more opportunity of accidentally referring to a clock that doesn't exist on a particular SoC. Furthermore, r8a779x-cpg-mssr.h is not a good name to be part of a DT binding, due to the wildcard. A future SoC may will match r8a779x and even be called (R-Car <something>3?), while using a completely different CPG block. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/dt-bindings/clock/r8a7796-cpg-mssr.h b/include/dt-bindings/clock/r8a7796-cpg-mssr.h new file mode 100644 index 0000000000000000..1e5942695f0dd057 --- /dev/null +++ b/include/dt-bindings/clock/r8a7796-cpg-mssr.h @@ -0,0 +1,69 @@ +/* + * Copyright (C) 2016 Renesas Electronics Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ +#ifndef __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ +#define __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ + +#include <dt-bindings/clock/renesas-cpg-mssr.h> + +/* r8a7796 CPG Core Clocks */ +#define R8A7796_CLK_Z 0 +#define R8A7796_CLK_Z2 1 +#define R8A7796_CLK_ZR 2 +#define R8A7796_CLK_ZG 3 +#define R8A7796_CLK_ZTR 4 +#define R8A7796_CLK_ZTRD2 5 +#define R8A7796_CLK_ZT 6 +#define R8A7796_CLK_ZX 7 +#define R8A7796_CLK_S0D1 8 +#define R8A7796_CLK_S0D2 9 +#define R8A7796_CLK_S0D3 10 +#define R8A7796_CLK_S0D4 11 +#define R8A7796_CLK_S0D6 12 +#define R8A7796_CLK_S0D8 13 +#define R8A7796_CLK_S0D12 14 +#define R8A7796_CLK_S1D1 15 +#define R8A7796_CLK_S1D2 16 +#define R8A7796_CLK_S1D4 17 +#define R8A7796_CLK_S2D1 18 +#define R8A7796_CLK_S2D2 19 +#define R8A7796_CLK_S2D4 20 +#define R8A7796_CLK_S3D1 21 +#define R8A7796_CLK_S3D2 22 +#define R8A7796_CLK_S3D4 23 +#define R8A7796_CLK_LB 24 +#define R8A7796_CLK_CL 25 +#define R8A7796_CLK_ZB3 26 +#define R8A7796_CLK_ZB3D2 27 +#define R8A7796_CLK_ZB3D4 28 +#define R8A7796_CLK_CR 29 +#define R8A7796_CLK_CRD2 30 +#define R8A7796_CLK_SD0H 31 +#define R8A7796_CLK_SD0 32 +#define R8A7796_CLK_SD1H 33 +#define R8A7796_CLK_SD1 34 +#define R8A7796_CLK_SD2H 35 +#define R8A7796_CLK_SD2 36 +#define R8A7796_CLK_SD3H 37 +#define R8A7796_CLK_SD3 38 +#define R8A7796_CLK_SSP2 39 +#define R8A7796_CLK_SSP1 40 +#define R8A7796_CLK_SSPRS 41 +#define R8A7796_CLK_RPC 42 +#define R8A7796_CLK_RPCD2 43 +#define R8A7796_CLK_MSO 44 +#define R8A7796_CLK_CANFD 45 +#define R8A7796_CLK_HDMI 46 +#define R8A7796_CLK_CSI0 47 +#define R8A7796_CLK_CSIREF 48 +#define R8A7796_CLK_CP 49 +#define R8A7796_CLK_CPEX 50 +#define R8A7796_CLK_R 51 +#define R8A7796_CLK_OSC 52 + +#endif /* __DT_BINDINGS_CLOCK_R8A7796_CPG_MSSR_H__ */
Add all R-Car M3-W Clock Pulse Generator Core Clock Outputs, as listed in Table 8.2b ("List of Clocks [R-Car M3-W]") of the R-Car Gen3 datasheet (rev. 0.51 + Errata for Rev051 Mar 31 2016). Note that internal CPG clocks (S0, S1, S2, S3, SDSRC, and SSPSRC) are not included, as they are used as internal clock sources only, and never referenced from DT. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Tested-by: Simon Horman <horms+renesas@verge.net.au> --- v2: - Add Tested-by. --- include/dt-bindings/clock/r8a7796-cpg-mssr.h | 69 ++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 include/dt-bindings/clock/r8a7796-cpg-mssr.h