Re: [PATCH v8 2/5] dt-bindings: clock: renesas,r9a06g032-sysctrl: documentation

[M P]

hi Geert,

On Wed, 13 Jun 2018 at 08:17, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Michel,
>
> On Wed, Jun 13, 2018 at 8:38 AM Michel Pollet
> <michel.pollet@xxxxxxxxxxxxxx> wrote:
> > On 11 June 2018 11:01, Geert wrote:
> > > On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet
> > > <michel.pollet@xxxxxxxxxxxxxx> wrote:
> > > > The Renesas R9A06G032 SYSCTRL node description.
> > > >
> > > > Signed-off-by: Michel Pollet <michel.pollet@xxxxxxxxxxxxxx>
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-
> > > sysctr
> > > > +++ l.txt
> > > > @@ -0,0 +1,32 @@
> > > > +* Renesas R9A06G032 SYSCTRL
> > > > +
> > > > +Required Properties:
> > > > +
> > > > +  - compatible: Must be:
> > > > +    - "renesas,r9a06g032-sysctrl"
> > > > +  - reg: Base address and length of the SYSCTRL IO block.
> > > > +  - #clock-cells: Must be 1
> > >
> > > (repeating myself) No clocks/clock-names for the external clock inputs?
> > >
> > > "RZ/N1 has 3 clock sources, 1 reference clock inputs for RGMII, and 2
> > > reference clock outputs for RMII/MII."
> >
> > Well, I'm trying to keep the binding as simple as possible, to dodge any
> > further discussion. Adding these will be possible later, I don't need them
> > for the moment anyway.
>
> Don't you? The external clock inputs are at the root of the clock tree, so I'd
> say you need them. Bolting them on later may become complicated, especially
> if you care about DTB backwards compatibility.

Well the input is fixed frequency -- and there is a clock (gate)
created for it in the driver
(CLK_RGMII_REF) so you can turn it on/off if you like...

The only configurable bit is a *nightmare* as the selection bit for
whether it is coming
from CLKOUT_D8 or from an external input is in a *completely different
IP block*. I'd
need another iomap() and all that stuff, and a custom driver for it.

If I were to implement it, I'd have to add a custom driver for that
clock, use the current
gate as a parent, iomap the selection bit register etc -- basically.
All of that for a use
case of *none ever* as most people would probably tweak that bit in
the bootloader
anyway -- also, I'd have zero capability for testing it.

As far as the output, they also have their own gates (already
created). They are also fixed
frequency and the only thing that matters to them is the pinmux
settings. So (soon) with the
pinmux driver, you can enable that clock properly without a special
driver, just by adding
that gate to your ethernet, and add the pinmux config for it.

So what do I go with that?

>
> The reset controller subsystem is optional anyway, and used only by a small
> number of drivers, so support for resets can definitely be postponed.
>
> > Did you have a chance to review the clock driver proper? I'm pondering
> > sending a v9 since it's been a week (with very minor changes) -- but I
> > don't want to interrupt if you were in the process of reviewing...
>
> I had a quick glance. Looks OK mostly, so it's definitely heading for the right
> direction!
> One remaining question: do you need CLK_OF_DECLARE()?
> Is there any reason your clock driver cannot be a platform driver, which is
> the recommended way?

I copied it straight out of clk-rcar-gen2.c -- I don't 'need' anything more than
instantiating my driver; does it make a difference? ie a one liner macro vs
adding an extra struct, 2 functions to do the same thing?
Just curious, sometime I lose track of that the goalpost is -- for
years I understood
that having OF was simpler and better all around?

Thanks!
Michel