Re: [PATCH v6 2/6] dt-bindings: Add the rzn1-clocks.h file
Hi Michel, On Wed, May 23, 2018 at 10:17 AM, M Pwrote: > 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
Morning Geert, On Wed, 23 May 2018 at 08:26, Geert Uytterhoevenwrote: > 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
On Tue, 22 May 2018 at 19:44, Geert Uytterhoevenwrote: > 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
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