Hi Michel, Cc'ing linux-clk On Tue, Apr 10, 2018 at 6:56 AM, Michel Pollet <michel.pollet@xxxxxxxxxxxxxx> wrote: > Hi Geert, > > On 10 April 2018 11:08, Geert wrote: >> >> Hi Michel, >> >> On Tue, Apr 10, 2018 at 11:56 AM, Michel Pollet >> <michel.pollet@xxxxxxxxxxxxxx> wrote: >> > In the current SDK for the RZ/N1, we made a clock architecture that is >> entirely device-tree based. >> > The clock hierarchy is quite complex and was machine generated from >> > design documents, and some exceptions and grouping were added to the >> 'main' family rzn1.dtsi... >> > >> > Apart from a few fixed-clock nodes, all of the other nodes are >> > 'special' and require a driver. All of these drivers are sub-drivers to a 'main' >> clock driver. That has been working for 2 years already. >> > >> > One extra note: we don't 'own' all of these clocks, part of the >> > clocks/dividers can be enable/disabled by the CM3 core. >> > >> > Now, For upstreaming, I'm going to have to change that, since already >> > the 'clock' bits are going to go under the MFD sysctrl node. However >> > I'm trying to figure out if we can still use our rzn1-clocks.dtsi in >> > some form, as well as my drivers, or so I have to convert it to a C table in >> some way. >> > >> > Also note that all the clock refer to SYSCTRL registers/bits using >> > constant names from a header file, not hex constants etc. >> > >> > I would appreciate any ideas/suggestions before I commit blindly to a >> path... >> > >> > Here is the main autogenerated clock file: >> > https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable/arch/arm/boo >> > t/dts/rzn1-clocks.dtsi Here's the extra clock{} node in the main >> > rzn1.dtsi >> > https://github.com/renesas- >> rz/rzn1_linux/blob/89d6c9be056a462b95d52172 >> > 21d70d6e5c25dfc2/arch/arm/boot/dts/rzn1.dtsi#L70 >> >> Describing the full clock hierarchy in DT is no longer recommended. >> The modern way is to have a single clock provider node in DT, and have the >> driver register all clocks. >> >> Compare e.g. the single clock-controller node in >> arch/arm/boot/dts/r8a7791.dtsi in v4.15 (used with the {r8a7791,renesas}- >> cpg-mssr.c driver, with the complex clocks node in v4.14, used with a lot of >> subdrivers, and requiring continuous maintenance. >> >> So I think you're best of creating (generating) a C table instead, and keep the >> DT simple and obviously correct. > > So, do I understand correctly that I could duplicate the tree as a C structure, and have my > 'clock' driver instantiate all my sub drivers as a clock tree directly? > > I looked into r8a7791's code, but your clock tree is 'flat' from what I see there, you don't have > the weird and fun clock dependencies we have. Nor any of the special cases we have; also, > you have a single clock driver in there it seems, so it is a pretty simple case where your > indexing method works... > > So I can definitely have a C struct to instantiate the table, but mostly that C table will be > duplicating the device tree hierarchy completely, and I'll still need as many sub-drivers > as I already have... The only change is really that that table will be in a .c -- is that > exactly what you want? > > Here are the current drivers I used. > https://github.com/renesas-rz/rzn1_linux/tree/rzn1-stable/drivers/clk/rzn1 > > There is: > + renesas,rzn1-clock-group -- many drivers only support claiming a single clock; the > clock-group driver allows me to group clocks together so they are enabled as ones > -- that way I don't have to patch the drivers. > + renesas,rzn1-clock-gate -- this one is more or less the same as yours. Enables/disable > a clock. It's parent will provide the rate. > + renesas,rzn1-clock-divider -- this one has a register with min,max, and an optional > table of 'fixed' values if the range is not linear. > + renesas,rzn1-clock-bitselect -- this one is fun, a single bit can change not only the > parent clock of *several IPs at the same time* but also the gate they have to use... > + renesas,rzn1-clock-dualgate -- this one works with the previous one, use gate 0 or > 1 depending on one bit... > > So, should I just jam all of these together with a struct hierarchy in a single driver, and > use an index? Keep your various .c files that define the operations of those different clk types. You want to keep your clk_ops implementations separate from your clk data for a given SoC. > I don't have too much trouble with the table as I can generate it as well, > I'm just making sure it's what you want... For a given SoC implementation you should have a driver that defines the clk data for all the clocks in that clock controller. This driver will of course reference the clk_ops implementations defined in the separate .c files mentioned above. A table is a fine way to do that. Best regards, Mike > >> >> Gr{oetje,eeting}s, >> >> Geert > > Thanks! > Michel > PS: I didn't make the hardware :-) > > > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
