| Message ID | 1528187462-47093-3-git-send-email-michel.pollet@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Michel, On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet <michel.pollet@bp.renesas.com> wrote: > The Renesas R9A06G032 SYSCTRL node description. > > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com> > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.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." The rest looks fine to me. Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, On 11 June 2018 11:01, Geert wrote: > Hi Michel, > > On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet > <michel.pollet@bp.renesas.com> wrote: > > The Renesas R9A06G032 SYSCTRL node description. > > > > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com> > > > --- /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. > > The rest looks fine to me. > Thanks! 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... Cheers, Michel > > 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 Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
Hi Michel, On Wed, Jun 13, 2018 at 8:38 AM Michel Pollet <michel.pollet@bp.renesas.com> wrote: > On 11 June 2018 11:01, Geert wrote: > > On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet > > <michel.pollet@bp.renesas.com> wrote: > > > The Renesas R9A06G032 SYSCTRL node description. > > > > > > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com> > > > > > --- /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. 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? Thanks! Gr{oetje,eeting}s, Geert
hi Geert, On Wed, 13 Jun 2018 at 08:17, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Michel, > > On Wed, Jun 13, 2018 at 8:38 AM Michel Pollet > <michel.pollet@bp.renesas.com> wrote: > > On 11 June 2018 11:01, Geert wrote: > > > On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet > > > <michel.pollet@bp.renesas.com> wrote: > > > > The Renesas R9A06G032 SYSCTRL node description. > > > > > > > > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com> > > > > > > > --- /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
Hi Michel, On Wed, Jun 13, 2018 at 10:53 AM M P <buserror@gmail.com> wrote: > On Wed, 13 Jun 2018 at 08:17, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, Jun 13, 2018 at 8:38 AM Michel Pollet > > <michel.pollet@bp.renesas.com> wrote: > > > On 11 June 2018 11:01, Geert wrote: > > > > On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet > > > > <michel.pollet@bp.renesas.com> wrote: > > > > > The Renesas R9A06G032 SYSCTRL node description. > > > > > > > > > > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com> > > > > > > > > > --- /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? IC. Still, DT describes the hardware, not (the limitations of) the software implementation. So you can have the external clock inputs in the DT bindings and in the DTS, but ignore complicated routing and source selection in the driver. > > > 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? clk-rcar-gen2.c is an old (early DT CCF) driver. Please check out renesas-cpg-mssr.c for something newer. Having a platform_driver means you're also having a struct device, and can do power management, if needed. > Just curious, sometime I lose track of that the goalpost is -- for > years I understood > that having OF was simpler and better all around? "having OF" does not mean CLK_OF_DECLARE(). Those *_OF_DECLARE() macros impose a very strict initialization order, which is not always suitable. Hence the trend to move to plain platform drivers. 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
diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt new file mode 100644 index 0000000..6aee360 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.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 + +Examples +-------- + + - SYSCTRL node: + + sysctrl: system-controller@4000c000 { + compatible = "renesas,r9a06g032-sysctrl"; + reg = <0x4000c000 0x1000>; + #clock-cells = <1>; + }; + + - Other nodes can use the clocks provided by SYSCTRL as in: + + #include <dt-bindings/clock/r9a06g032-sysctrl.h> + uart0: serial@40060000 { + compatible = "snps,dw-apb-uart"; + reg = <0x40060000 0x400>; + interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>; + reg-shift = <2>; + reg-io-width = <4>; + clocks = <&sysctrl R9A06G032_CLK_UART0>; + clock-names = "baudclk"; + };
The Renesas R9A06G032 SYSCTRL node description. Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com> --- .../bindings/clock/renesas,r9a06g032-sysctrl.txt | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt