Hi Michel,

Cc'ing linux-clk

On Tue, Apr 10, 2018 at 6:56 AM, Michel Pollet
<michel.pol...@bp.renesas.com> 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.pol...@bp.renesas.com> 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,

>> 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.

Reply via email to