Re: [PATCH v6 2/6] dt-bindings: Add the rzn1-clocks.h file

[Geert Uytterhoeven]

Hi Michel,

On Wed, May 23, 2018 at 10:17 AM, M P  wrote:
> On Wed, 23 May 2018 at 08:26, Geert Uytterhoeven 
> wrote:
>> 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:
>> >> > --- /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?

Yes, all of these are desirable.

Now, given the clock definitions for RZ/N1[DSL] are the same (although some
don't exist on some variants), you could keep on using RZN1_CLK_FOO for the
names of the defines, and store them in a common file, included by the
soc-specific file. But please make clear the common file cannot be included
directly, so the filename does not become part of the DT ABI, and you are
shielded from future marketing silliness (e.g. next quarter's RZ/N1X being
totally different).

E.g.:

include/dt-bindings/clock/r9a06g033-clocks.h:

#ifndef __DT_BINDINGS_R9A06G033_CLOCK_H__
#define __DT_BINDINGS_R9A06G033_CLOCK_H__

#include "rzn1-clocks.h"

// ... additions and fixups, if needed

   #endif // __DT_BINDINGS_R9A06G033_CLOCK_H__

include/dt-bindings/clock/rzn1-clocks.h:

#ifndef __DT_BINDINGS_RZN1_CLOCK_H__
#define __DT_BINDINGS_RZN1_CLOCK_H__

#ifndef __DT_BINDINGS_R9A06G033_CLOCK_H__
#error Do not include rzn1-clocks.h directly
#endif

#define RZN1_CLK_FOO0
#define RZN1_CLK_BAR1
...

#endif // __DT_BINDINGS_RZN1_CLOCK_H__

As for the driver, I think you should make sure the driver instantiates no
clocks that don't exist on the actual SoC it runs on.

BTW, someone asked why I didn't use the same definitions for R-Car M3-W and
R-Car H3 (there were only a few differences), and didn't use common
definitions for all R-Car Gen3 SoCs.
I doubted a long time that we made the right decision, but then V3M and V3H
appeared in later revisions of the datasheet, and D3 and E3 later,
reconfirming our decision.

Even for compatible values for IP cores within the same family: there are
differences, but usually we only discover them after a while.
In theory all the same IP cores in R-Car Gen3 SoCs are the same (except for
obvious differences like clocks and pinctrl), in practice they are not.
So it may sound silly to have SoC-specific compatible values for the gross
of the IP cores, but it would be worse if we got bitten later by not having
them.

BTW, you're too quick reposting your series. I can't finish my reviews in
time ;-)
Please give people a reasonable amount of time to handle them.
Thanks!

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond 

Re: [PATCH v6 2/6] dt-bindings: Add the rzn1-clocks.h file

[M P]

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 what, again?

What about *making up names* -- The 'family names' can/will change -- the
part numbers are *too limited in scope* -- why not just make up names? does
it matter as long as it's close to reality and are documented? I dunno,
"rzn1_18" or "rzn1_mk1" and so we have a way out when they release a new
one next year? It seems to be working fine for cars 

Re: [PATCH v6 2/6] dt-bindings: Add the rzn1-clocks.h file

[M P]

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 2/6] dt-bindings: Add the rzn1-clocks.h file

[Rob Herring]

On Tue, May 22, 2018 at 11:01:22AM +0100, Michel Pollet wrote:
> This adds the constants necessary to use the renesas,rzn1-clocks driver.
> 
> Signed-off-by: Michel Pollet 
> ---
>  include/dt-bindings/clock/rzn1-clocks.h | 187 
> 
>  1 file changed, 187 insertions(+)
>  create mode 100644 include/dt-bindings/clock/rzn1-clocks.h

Reviewed-by: Rob Herring 

[PATCH v6 2/6] dt-bindings: Add the rzn1-clocks.h file

[Michel Pollet]

This adds the constants necessary to use the renesas,rzn1-clocks driver.

Signed-off-by: Michel Pollet 
---
 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
@@ -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
+#define RZN1_CLK_PLL_USB   1
+#define RZN1_CLK_481   /* AKA CLK_PLL_USB */
+#define RZN1_CLKOUT_D102
+#define RZN1_CLKOUT_D163
+#define RZN1_MSEBIS_CLK3   /* AKA CLKOUT_D16 */
+#define RZN1_MSEBIM_CLK3   /* AKA CLKOUT_D16 */
+#define RZN1_CLKOUT_D160   4
+#define RZN1_CLKOUT_D1OR2  5
+#define RZN1_CLK_DDRPHY_PLLCLK 5   /* AKA CLKOUT_D1OR2 */
+#define RZN1_CLKOUT_D206
+#define RZN1_CLK50 6   /* AKA CLKOUT_D20 */
+#define RZN1_CLKOUT_D407
+#define RZN1_CLK25 7   /* AKA CLKOUT_D40 */
+#define RZN1_CLKOUT_D5 8
+#define RZN1_CLKOUT_D8 9
+#define RZN1_CLK1259   /* AKA CLKOUT_D8 */
+#define RZN1_DIV_ADC   10
+#define RZN1_DIV_I2C   11
+#define RZN1_DIV_NAND  12
+#define RZN1_DIV_P1_PG 13
+#define RZN1_DIV_P2_PG 14
+#define RZN1_DIV_P3_PG 15
+#define RZN1_DIV_P4_PG 16
+#define RZN1_DIV_P5_PG 17
+#define RZN1_CLK_P5_PG117  /* AKA DIV_P5_PG */
+#define RZN1_DIV_P6_PG 18
+#define RZN1_DIV_QSPI0 19
+#define RZN1_DIV_QSPI1 20
+#define RZN1_DIV_REF_SYNC  21
+#define RZN1_CLK_REF_SYNC  21  /* AKA DIV_REF_SYNC */
+#define RZN1_DIV_SDIO0 22
+#define RZN1_DIV_SDIO1 23
+#define RZN1_DIV_SWITCH24
+#define RZN1_DIV_UART  25
+#define RZN1_CLK_25_PG426
+#define RZN1_CLK_25_PG527
+#define RZN1_CLK_25_PG628
+#define RZN1_CLK_25_PG729
+#define RZN1_CLK_25_PG830
+#define RZN1_CLK_ADC   31
+#define RZN1_CLK_ECAT100   32
+#define RZN1_CLK_HSR10033
+#define RZN1_CLK_I2C0  34
+#define RZN1_CLK_I2C1  35
+#define RZN1_CLK_MII_REF   36
+#define RZN1_CLK_NAND  37
+#define RZN1_CLK_NOUSBP2_PG6   38
+#define RZN1_CLK_P1_PG239
+#define RZN1_CLK_P1_PG340
+#define RZN1_CLK_P1_PG441
+#define RZN1_CLK_P4_PG342
+#define RZN1_CLK_P4_PG443
+#define RZN1_CLK_P6_PG144
+#define RZN1_CLK_P6_PG245
+#define RZN1_CLK_P6_PG346
+#define RZN1_CLK_P6_PG447
+#define RZN1_CLK_PCI_USB   48
+#define RZN1_CLK_QSPI0 49
+#define RZN1_CLK_QSPI1 50
+#define RZN1_CLK_RGMII_REF 51
+#define RZN1_CLK_RMII_REF  52
+#define RZN1_CLK_SDIO0 53
+#define RZN1_CLK_SDIO1 54
+#define RZN1_CLK_SERCOS100 55
+#define RZN1_CLK_SLCD  56
+#define RZN1_CLK_SPI0  57
+#define RZN1_CLK_SPI1  58
+#define RZN1_CLK_SPI2  59
+#define RZN1_CLK_SPI3  60
+#define RZN1_CLK_SPI4  61
+#define RZN1_CLK_SPI5  62
+#define RZN1_CLK_SWITCH63
+#define RZN1_DIV_MOTOR 64
+#define RZN1_HCLK_ECAT125  65
+#define RZN1_HCLK_PINCONFIG66
+#define RZN1_HCLK_SERCOS   67
+#define RZN1_HCLK_SGPIO2   68
+#define RZN1_HCLK_SGPIO3   69
+#define RZN1_HCLK_SGPIO4   70
+#define RZN1_HCLK_TIMER0   71
+#define RZN1_HCLK_TIMER1   72
+#define RZN1_HCLK_USBF 73
+#define RZN1_HCLK_USBH 74
+#define RZN1_HCLK_USBPM75
+#define RZN1_CLK_48_PG_F   76
+#define RZN1_CLK_48_PG4