Re: RFC: RZ/N1 clock architecture...
Hi Michel,
Cc'ing linux-clk
On Tue, Apr 10, 2018 at 6:56 AM, Michel Pollet
wrote:
> Hi Geert,
>
> On 10 April 2018 11:08, Geert wrote:
>>
>> Hi Michel,
>>
>> On Tue, Apr 10, 2018 at 11:56 AM, Michel Pollet
>> 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
RE: RFC: RZ/N1 clock architecture...
Hi Geert,
On 10 April 2018 11:08, Geert wrote:
>
> Hi Michel,
>
> On Tue, Apr 10, 2018 at 11:56 AM, Michel Pollet
> 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...
Hi Michel,
On Tue, Apr 10, 2018 at 11:56 AM, Michel Pollet
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 -- ge...@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
RFC: RZ/N1 clock architecture...
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.
