RFC: RZ/N1 clock architecture...

RFC: RZ/N1 clock architecture...

[Michel Pollet]

Hi guys,

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/boot/dts/rzn1-clocks.dtsi
Here's the extra clock{} node in the main rzn1.dtsi
https://github.com/renesas-rz/rzn1_linux/blob/89d6c9be056a462b95d5217221d70d6e5c25dfc2/arch/arm/boot/dts/rzn1.dtsi#L70





Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: RFC: RZ/N1 clock architecture...

[Geert Uytterhoeven]

Hi Michel,

On Tue, Apr 10, 2018 at 11:56 AM, Michel Pollet
<michel.pollet@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/boot/dts/rzn1-clocks.dtsi
> Here's the extra clock{} node in the main rzn1.dtsi
> https://github.com/renesas-rz/rzn1_linux/blob/89d6c9be056a462b95d5217221d70d6e5c25dfc2/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.

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

RE: RFC: RZ/N1 clock architecture...

[Michel Pollet]

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

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

Re: RFC: RZ/N1 clock architecture...

[Michael Turquette]

Hi Michel,

Cc'ing linux-clk

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