Re: [PATCH v6 2/6] dt-bindings: Add the rzn1-clocks.h file
On Tue, 22 May 2018 at 19:44, Geert Uytterhoeven
wrote:
> Hi Michel,
> On Tue, May 22, 2018 at 12:01 PM, Michel Pollet
> wrote:
> > This adds the constants necessary to use the renesas,rzn1-clocks driver.
> >
> > Signed-off-by: Michel Pollet
> Thanks for your patch!
> > ---
> > include/dt-bindings/clock/rzn1-clocks.h | 187
> > 1 file changed, 187 insertions(+)
> > create mode 100644 include/dt-bindings/clock/rzn1-clocks.h
> >
> > diff --git a/include/dt-bindings/clock/rzn1-clocks.h
b/include/dt-bindings/clock/rzn1-clocks.h
> > new file mode 100644
> > index 000..8a73db2
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/rzn1-clocks.h
> Given this is part of the DT ABI, and there exist multiple different RZ/N1
> SoCs (and there are probably planned more), I wouldn't call this header
> file "rzn1-clocks.h", but e.g. "r9a06g032-clocks.h".
Actually, no, there already are two r906g03X devices that will work
perfectly fine with this driver. We had that discussion before, and you
insist and me removing mentions of the rzn1 everywhere, however, this
applies to *two* devices already, and I'm supposed to upstream support for
them. I can't rename r9g06g032 because it is *inexact* that's why it's
called rzn1. So unless you let me call it r9a06g0xx-clocks.h (which i know
you won't as per multiple previous discussions) this can't be called
r9a06g032 because it won't be fit for my purpose when I try to bring back
the RZ/N1S into the picture. There are minor difference to clocking,
I don't know if Renesas plans to release any more rzn1's in this series,
but my little finger tells me this isn't the case. But regardless of what
we plan, Marketing will screw it up.
Cheers,
Michel
> > @@ -0,0 +1,187 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * RZ/N1 clock IDs
> > + *
> > + * Copyright (C) 2018 Renesas Electronics Europe Limited
> > + *
> > + * Michel Pollet ,
> > + * Derived from zx-reboot.c
> > + */
> > +
> > +#ifndef __DT_BINDINGS_RZN1_CLOCK_H__
> > +#define __DT_BINDINGS_RZN1_CLOCK_H__
> > +
> > +#define RZN1_CLKOUT0
> Similar for the RZN1 prefix.
> I'll look at the actual list of clocks later...
> 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
Re: [PATCH v6 3/6] dt-bindings: clock: renesas,rzn1-clocks: document RZ/N1 clock driver
Hi Rob,
On Tue, 22 May 2018 at 17:09, Rob Herring wrote:
> On Tue, May 22, 2018 at 11:01:23AM +0100, Michel Pollet wrote:
> > The Renesas RZ/N1 Family (Part #R9A06G0xx) requires a driver
> > to provide the SoC clock infrastructure for Linux.
> >
> > This documents the driver bindings.
> >
> > Signed-off-by: Michel Pollet
> > ---
> > .../bindings/clock/renesas,rzn1-clocks.txt | 44
++
> > 1 file changed, 44 insertions(+)
> > create mode 100644
Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
> >
> > diff --git
a/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
b/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
> > new file mode 100644
> > index 000..0c41137
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
> > @@ -0,0 +1,44 @@
> > +* Renesas RZ/N1 Clock Driver
> > +
> > +This driver provides the clock infrastructure used by all the other
drivers.
> Bindings document h/w not drivers.
> > +
> > +One of the 'special' feature of this infrastructure is that Linux
doesn't
> Bindings are not just for Linux.
> > +necessary 'own' all the clocks on the SoC, some other OS runs on
> > +the Cortex-M3 core and that OS can access and claim it's own clocks.
> How is this relevant to the binding?
Well you just said it, if the binding is not just for linux, it's probably
a good idea to mention it somewhere since I can promise you it's *not*
documented in the hardware manual. Whatever this binding is for, it's
relevant to point out it is different from the other clock drivers in the
same directory... ?
> > +
> > +Required Properties:
> > +
> > + - compatible: Must be
> > +- "renesas,r9a06g032-clocks" for the RZ/N1D
> > +and "renesas,rzn1-clocks" as a fallback.
> Is "clocks" how the chip reference manual refers to this block?
No, the block is called 'sysctrl' and has tons of other stuff in there.
I've tried multiple times to get a 'sysctrl' driver in the previous
versions of the patch, in various shape or form, and I can't because I'd be
supposed to 'document' binding for stuff that has no code, no purpose, no
testing, and have all wildly different topics. So, after some more
lengthily discussion with Geert, we've decided to make the 'primary'
feature of that block (clocks) as a driver, and see from there onward.
Thanks for all the other comments, all taken onboard for next version!
Michel
> > + - reg: Base address and length of the memory resource used by the
driver
> > + - #clock-cells: Must be 1
> > +
> > +Examples
> > +
> > +
> > + - Clock driver device node:
> > +
> > + clock: clocks@4000c000 {
> clock-controller@...
> > + compatible = "renesas,r9a06g032-clocks",
> > + "renesas,rzn1-clocks";
> > + reg = <0x4000c000 0x1000>;
> > + status = "okay";
> Don't show status in examples. (Plus, I doubt you ever want to have this
> disabled, so you don't need the property in your dts files either).
> > + #clock-cells = <1>;
> > + };
> > +
> > +
> > + - Other drivers can use the clocks as in:
> s/drivers/nodes/
> > +
> > + uart0: serial@4006 {
> > + compatible = "snps,dw-apb-uart";
> > + reg = <0x4006 0x400>;
> > + interrupts = ;
> > + reg-shift = <2>;
> > + reg-io-width = <4>;
> > + clocks = <&clock RZN1_CLK_UART0>;
> > + clock-names = "baudclk";
> > + };
> > +Note the use of RZN1_CLK_UART0 -- these constants are declared in
> > +the rzn1-clocks.h header file. These are not hardware based
constants
> > +and are Linux specific.
> No, they are not Linux specific. They are part of the DT ABI.
> While it is not a requirement to base them on some h/w numbering, it is
> preferred if you can. That usually only works if you can base them on
> bit positions or register offsets.
> Rob
Re: [PATCH v6 2/6] dt-bindings: Add the rzn1-clocks.h file
Morning Geert, On Wed, 23 May 2018 at 08:26, Geert Uytterhoeven wrote: > Hi Michel, > On Wed, May 23, 2018 at 8:44 AM, M P wrote: > > On Tue, 22 May 2018 at 19:44, Geert Uytterhoeven > > wrote: > >> On Tue, May 22, 2018 at 12:01 PM, Michel Pollet > >> wrote: > >> > This adds the constants necessary to use the renesas,rzn1-clocks driver. > >> > > >> > Signed-off-by: Michel Pollet > >> > --- /dev/null > >> > +++ b/include/dt-bindings/clock/rzn1-clocks.h > > > >> Given this is part of the DT ABI, and there exist multiple different RZ/N1 > >> SoCs (and there are probably planned more), I wouldn't call this header > >> file "rzn1-clocks.h", but e.g. "r9a06g032-clocks.h". > > > > Actually, no, there already are two r906g03X devices that will work > > perfectly fine with this driver. We had that discussion before, and you > > insist and me removing mentions of the rzn1 everywhere, however, this > > applies to *two* devices already, and I'm supposed to upstream support for > > them. I can't rename r9g06g032 because it is *inexact* that's why it's > My worry is not that there are two r906g03X devices that will work fine > with this driver, but that there will be other "rzn1" devices that will not > work with these bindings (the header file is part of the bindings). > Besides, RZ/N1D and RZ/N1S (Which apparently differ in packaging only? > Oh no, RZ/N1D (the larger package) has less QSPI channels than RZ/N1S > (the smaller package)), there's also (at least) RZ/N1L. > > called rzn1. So unless you let me call it r9a06g0xx-clocks.h (which i know > > you won't as per multiple previous discussions) this can't be called > > r9a06g032 because it won't be fit for my purpose when I try to bring back > > the RZ/N1S into the picture. > You can add r9a06g033-clocks.h when adding support for RZ/N1S. So it is now acceptable to duplicate a huge amount of code, and constants when in fact there differences are so minor they would require minimal amount of code to take care of them? That just flies straight against my 30+ years of programming -- We're going to have twice the *identical* code, twice the header, and completely incompatible device tree files -- I mean, *right now* our rzn1.dtsi works *as is* on the 1D and 1S, we've got ONE file to maintain, and you can switch your CPU board from 1D to 1S and your 'board file' can stay the same. Wasn't it the idea of that stuff in the first place? Isn't it in the customer/engineer interest to be able to cross grade from one manufacturer's device *in the same series* to another without having to duplicate his whole board file? > > There are minor difference to clocking, > Aha? Sure, 1S doesn't' have DDR, 1D doesn't have the second QSPI. That's about it (I lie, theres a few other bits I'm sure). It's not like it won't even *work* or anything, the registers are there, the bit positions are there, all is the same, I'm *sure* that's what the compatible="" thing were supposed to be used for, isn't it? Heck, I'm pretty sure there's a register in sysctrl, that tells me that anyway, so I wouldn't even have to have a special compatible= -- I didn't do it since the driver is already so big. > > I don't know if Renesas plans to release any more rzn1's in this series, > > but my little finger tells me this isn't the case. But regardless of what > We thought the same thing when the first RZ member (RZ/A1H) showed up. > Did we know this was not going to be the first SoC of a new RZ family, but > the first SoC of the first subfamily (RZ/A) of the RZ family... And the > various subfamilies bear not much similarity. > > we plan, Marketing will screw it up. > Correct. And to mitigate that, we have no other choice than to use the real > part numbers to differentiate. Once bitten, twice shy. It's not mitigation from where I stand -- it's a gigantic kludge; To handle one exception, you throw away the baby with the bathwater. From where I sit, it's like having to a different screwdriver for the screws on the left of a panel vs the right of the panel. Sorry to come out as pretty miffed -- I've just spent weeks polishing up a driver to make it more or less similar to what they were 10 years ago (whoo look a platform file with a big table in it!), after throwing away all the work I had done to make it all device-tree based and make the code as agnostic as we could -- and now it turns out we need to make it even worse by throwing away the fact it actually *does* work on two SoC -- and that just because... because wha
Re: [PATCH v6 4/6] ARM: dts: Renesas RZ/N1 SoC base device tree file
Hi Geert,
On Wed, 23 May 2018 at 10:12, Geert Uytterhoeven
wrote:
> Hi Michel,
> On Tue, May 22, 2018 at 12:01 PM, Michel Pollet
> wrote:
> > This adds the Renesas RZ/N1D (Part #R9A06G032) SoC bare
> > bone support.
> >
> > This currently only handles generic parts (gic, architected timer)
> > and a UART.
> > For simplicity sake, this also relies on the bootloader to set the
> > pinctrl and clocks.
> >
> > Signed-off-by: Michel Pollet
> Thanks for your patch!
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + cpus {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + clocks = <&clock RZN1_DIV_CA7>;
> I think the clocks property should be moved to the individual CPU nodes.
Ah, I had a look around, and I found some instances that are in the cpu
sub-node, and others that are not -- it seems that having it in the cpu
sub-node would implies it's core specific... here if that clock is changed
both cores would change speed...
Either way, it's not used by the kernel in any way at the moment -- I had
hoped cpufreq or something would claim it, but it's not the case.
Thanks,
Michel
Re: [PATCH v6 4/6] ARM: dts: Renesas RZ/N1 SoC base device tree file
Hi Geert,
On Wed, 23 May 2018 at 12:18, Geert Uytterhoeven
wrote:
> Hi Michel,
> On Wed, May 23, 2018 at 11:20 AM, M P wrote:
> > On Wed, 23 May 2018 at 10:12, Geert Uytterhoeven
> > wrote:
> >> On Tue, May 22, 2018 at 12:01 PM, Michel Pollet
> >> wrote:
> >> > + #address-cells = <1>;
> >> > + #size-cells = <1>;
> >> > +
> >> > + cpus {
> >> > + #address-cells = <1>;
> >> > + #size-cells = <0>;
> >> > + clocks = <&clock RZN1_DIV_CA7>;
> >
> >> I think the clocks property should be moved to the individual CPU
nodes.
> >
> > Ah, I had a look around, and I found some instances that are in the cpu
> > sub-node, and others that are not -- it seems that having it in the cpu
> > sub-node would implies it's core specific... here if that clock is
changed
> > both cores would change speed...
> Assumed the driver code knows to look in the parent node, which I doubt
> the cpufreq code does.
> > Either way, it's not used by the kernel in any way at the moment -- I
had
> > hoped cpufreq or something would claim it, but it's not the case.
> I guess you have to add your main SoC compatible value to the whitelist
> in drivers/cpufreq/cpufreq-dt-platdev.c first.
Most excellent tip here -- I'll add a further patch to enable this, after
this series eventually gets merged...
Cheers,
Michel
