| Message ID | 20200915112038.30219-2-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: ti: Add USB support for J7200 EVM | expand |
Hi, Sorry for the delay. On 2020-09-15 13:20, Roger Quadros wrote: > Each SERDES lane mux can select upto 4 different IPs. > There are 4 lanes in each J7200 SERDES. Define all > the possible functions in this file. > > Cc: Peter Rosin <peda@axentia.se> > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > include/dt-bindings/mux/mux-j7200-wiz.h | 29 +++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h > > diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h b/include/dt-bindings/mux/mux-j7200-wiz.h > new file mode 100644 > index 000000000000..b091b1185a36 > --- /dev/null > +++ b/include/dt-bindings/mux/mux-j7200-wiz.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * This header provides constants for J7200 WIZ. > + */ > + > +#ifndef _DT_BINDINGS_J7200_WIZ > +#define _DT_BINDINGS_J7200_WIZ > + > +#define SERDES0_LANE0_QSGMII_LANE3 0x0 > +#define SERDES0_LANE0_PCIE1_LANE0 0x1 > +#define SERDES0_LANE0_IP3_UNUSED 0x2 > +#define SERDES0_LANE0_IP4_UNUSED 0x3 > + > +#define SERDES0_LANE1_QSGMII_LANE4 0x0 > +#define SERDES0_LANE1_PCIE1_LANE1 0x1 > +#define SERDES0_LANE1_IP3_UNUSED 0x2 > +#define SERDES0_LANE1_IP4_UNUSED 0x3 > + > +#define SERDES0_LANE2_QSGMII_LANE1 0x0 > +#define SERDES0_LANE2_PCIE1_LANE2 0x1 > +#define SERDES0_LANE2_IP3_UNUSED 0x2 > +#define SERDES0_LANE2_IP4_UNUSED 0x3 > + > +#define SERDES0_LANE3_QSGMII_LANE2 0x0 > +#define SERDES0_LANE3_PCIE1_LANE3 0x1 > +#define SERDES0_LANE3_USB 0x2 > +#define SERDES0_LANE3_IP4_UNUSED 0x3 > + > +#endif /* _DT_BINDINGS_J7200_WIZ */ Should not the defines start with J7200_WIZ? SERDES0 seems like a too generic prefix, at least to me. Cheers, Peter
On 06:52-20200916, Peter Rosin wrote: > Hi, > > Sorry for the delay. > > On 2020-09-15 13:20, Roger Quadros wrote: > > Each SERDES lane mux can select upto 4 different IPs. > > There are 4 lanes in each J7200 SERDES. Define all > > the possible functions in this file. > > > > Cc: Peter Rosin <peda@axentia.se> > > Signed-off-by: Roger Quadros <rogerq@ti.com> > > --- > > include/dt-bindings/mux/mux-j7200-wiz.h | 29 +++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h > > > > diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h b/include/dt-bindings/mux/mux-j7200-wiz.h > > new file mode 100644 > > index 000000000000..b091b1185a36 > > --- /dev/null > > +++ b/include/dt-bindings/mux/mux-j7200-wiz.h > > @@ -0,0 +1,29 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * This header provides constants for J7200 WIZ. > > + */ > > + > > +#ifndef _DT_BINDINGS_J7200_WIZ > > +#define _DT_BINDINGS_J7200_WIZ > > + > > +#define SERDES0_LANE0_QSGMII_LANE3 0x0 > > +#define SERDES0_LANE0_PCIE1_LANE0 0x1 > > +#define SERDES0_LANE0_IP3_UNUSED 0x2 > > +#define SERDES0_LANE0_IP4_UNUSED 0x3 > > + > > +#define SERDES0_LANE1_QSGMII_LANE4 0x0 > > +#define SERDES0_LANE1_PCIE1_LANE1 0x1 > > +#define SERDES0_LANE1_IP3_UNUSED 0x2 > > +#define SERDES0_LANE1_IP4_UNUSED 0x3 > > + > > +#define SERDES0_LANE2_QSGMII_LANE1 0x0 > > +#define SERDES0_LANE2_PCIE1_LANE2 0x1 > > +#define SERDES0_LANE2_IP3_UNUSED 0x2 > > +#define SERDES0_LANE2_IP4_UNUSED 0x3 > > + > > +#define SERDES0_LANE3_QSGMII_LANE2 0x0 > > +#define SERDES0_LANE3_PCIE1_LANE3 0x1 > > +#define SERDES0_LANE3_USB 0x2 > > +#define SERDES0_LANE3_IP4_UNUSED 0x3 > > + > > +#endif /* _DT_BINDINGS_J7200_WIZ */ > > Should not the defines start with J7200_WIZ? SERDES0 seems like a too > generic prefix, at least to me. Thanks, good point. I am not sure if WIZ should even be used.. It is a TI internal prefix for various serdes solutions, but I agree that SERDES0 is too generic a terminology. That said, we should cleanup include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing j7200 changes.
Hi! On 2020-09-16 17:45, Nishanth Menon wrote: > On 06:52-20200916, Peter Rosin wrote: >> Hi, >> >> Sorry for the delay. >> >> On 2020-09-15 13:20, Roger Quadros wrote: >>> Each SERDES lane mux can select upto 4 different IPs. >>> There are 4 lanes in each J7200 SERDES. Define all >>> the possible functions in this file. >>> >>> Cc: Peter Rosin <peda@axentia.se> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> --- *snip* >> Should not the defines start with J7200_WIZ? SERDES0 seems like a too >> generic prefix, at least to me. > > Thanks, good point. I am not sure if WIZ should even be used.. It is > a TI internal prefix for various serdes solutions, but I agree that > SERDES0 is too generic a terminology. That said, we should cleanup > include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing > j7200 changes. Right. As maintainer for the directory in question, I should have been on Cc for that series too, but it appears I wasn't. Hence, I didn't notice that file until now when I went looking for it. Why wasn't I on Cc? Cheers, Peter
Nishanth, On 16/09/20 9:15 pm, Nishanth Menon wrote: > On 06:52-20200916, Peter Rosin wrote: >> Hi, >> >> Sorry for the delay. >> >> On 2020-09-15 13:20, Roger Quadros wrote: >>> Each SERDES lane mux can select upto 4 different IPs. >>> There are 4 lanes in each J7200 SERDES. Define all >>> the possible functions in this file. >>> >>> Cc: Peter Rosin <peda@axentia.se> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> --- >>> include/dt-bindings/mux/mux-j7200-wiz.h | 29 +++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h >>> >>> diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h b/include/dt-bindings/mux/mux-j7200-wiz.h >>> new file mode 100644 >>> index 000000000000..b091b1185a36 >>> --- /dev/null >>> +++ b/include/dt-bindings/mux/mux-j7200-wiz.h >>> @@ -0,0 +1,29 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * This header provides constants for J7200 WIZ. >>> + */ >>> + >>> +#ifndef _DT_BINDINGS_J7200_WIZ >>> +#define _DT_BINDINGS_J7200_WIZ >>> + >>> +#define SERDES0_LANE0_QSGMII_LANE3 0x0 >>> +#define SERDES0_LANE0_PCIE1_LANE0 0x1 >>> +#define SERDES0_LANE0_IP3_UNUSED 0x2 >>> +#define SERDES0_LANE0_IP4_UNUSED 0x3 >>> + >>> +#define SERDES0_LANE1_QSGMII_LANE4 0x0 >>> +#define SERDES0_LANE1_PCIE1_LANE1 0x1 >>> +#define SERDES0_LANE1_IP3_UNUSED 0x2 >>> +#define SERDES0_LANE1_IP4_UNUSED 0x3 >>> + >>> +#define SERDES0_LANE2_QSGMII_LANE1 0x0 >>> +#define SERDES0_LANE2_PCIE1_LANE2 0x1 >>> +#define SERDES0_LANE2_IP3_UNUSED 0x2 >>> +#define SERDES0_LANE2_IP4_UNUSED 0x3 >>> + >>> +#define SERDES0_LANE3_QSGMII_LANE2 0x0 >>> +#define SERDES0_LANE3_PCIE1_LANE3 0x1 >>> +#define SERDES0_LANE3_USB 0x2 >>> +#define SERDES0_LANE3_IP4_UNUSED 0x3 >>> + >>> +#endif /* _DT_BINDINGS_J7200_WIZ */ >> >> Should not the defines start with J7200_WIZ? SERDES0 seems like a too >> generic prefix, at least to me. > > Thanks, good point. I am not sure if WIZ should even be used.. It is > a TI internal prefix for various serdes solutions, but I agree that > SERDES0 is too generic a terminology. That said, we should cleanup > include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing > j7200 changes. WIZ is defined in public TRM (https://www.ti.com/lit/pdf/spruiu1). " The SERDES mux (WIZ) module supports the following features: *) Multiplexes device interfaces onto a single SERDES lane (Tx and Rx) *) Provides registers to implement SERDES control and status functions and alignment delays . . " Thanks Kishon
Hi Peter & Nishanth, On 16/09/2020 18:45, Nishanth Menon wrote: > On 06:52-20200916, Peter Rosin wrote: >> Hi, >> >> Sorry for the delay. >> >> On 2020-09-15 13:20, Roger Quadros wrote: >>> Each SERDES lane mux can select upto 4 different IPs. >>> There are 4 lanes in each J7200 SERDES. Define all >>> the possible functions in this file. >>> >>> Cc: Peter Rosin <peda@axentia.se> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> --- >>> include/dt-bindings/mux/mux-j7200-wiz.h | 29 +++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h >>> >>> diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h b/include/dt-bindings/mux/mux-j7200-wiz.h >>> new file mode 100644 >>> index 000000000000..b091b1185a36 >>> --- /dev/null >>> +++ b/include/dt-bindings/mux/mux-j7200-wiz.h >>> @@ -0,0 +1,29 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * This header provides constants for J7200 WIZ. >>> + */ >>> + >>> +#ifndef _DT_BINDINGS_J7200_WIZ >>> +#define _DT_BINDINGS_J7200_WIZ >>> + >>> +#define SERDES0_LANE0_QSGMII_LANE3 0x0 >>> +#define SERDES0_LANE0_PCIE1_LANE0 0x1 >>> +#define SERDES0_LANE0_IP3_UNUSED 0x2 >>> +#define SERDES0_LANE0_IP4_UNUSED 0x3 >>> + >>> +#define SERDES0_LANE1_QSGMII_LANE4 0x0 >>> +#define SERDES0_LANE1_PCIE1_LANE1 0x1 >>> +#define SERDES0_LANE1_IP3_UNUSED 0x2 >>> +#define SERDES0_LANE1_IP4_UNUSED 0x3 >>> + >>> +#define SERDES0_LANE2_QSGMII_LANE1 0x0 >>> +#define SERDES0_LANE2_PCIE1_LANE2 0x1 >>> +#define SERDES0_LANE2_IP3_UNUSED 0x2 >>> +#define SERDES0_LANE2_IP4_UNUSED 0x3 >>> + >>> +#define SERDES0_LANE3_QSGMII_LANE2 0x0 >>> +#define SERDES0_LANE3_PCIE1_LANE3 0x1 >>> +#define SERDES0_LANE3_USB 0x2 >>> +#define SERDES0_LANE3_IP4_UNUSED 0x3 >>> + >>> +#endif /* _DT_BINDINGS_J7200_WIZ */ >> >> Should not the defines start with J7200_WIZ? SERDES0 seems like a too >> generic prefix, at least to me. > > Thanks, good point. I am not sure if WIZ should even be used.. It is > a TI internal prefix for various serdes solutions, but I agree that > SERDES0 is too generic a terminology. That said, we should cleanup > include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing > j7200 changes. > I'm planning to put all TI SERDES definitions in one header file "ti-serdes-mux.h" and add SOC specific prefixes to the macros. This will mean some churn in the existing DT files. (only 2 so far) Are you guys OK if I do the change in one patch to avoid a broken build in between. You guys can then decide whose tree it goes through. The new SoC addition will be separate of course. cheers, -roger
On 15:00-20200917, Roger Quadros wrote: > Hi Peter & Nishanth, > > On 16/09/2020 18:45, Nishanth Menon wrote: > > On 06:52-20200916, Peter Rosin wrote: > > > Hi, > > > > > > Sorry for the delay. > > > > > > On 2020-09-15 13:20, Roger Quadros wrote: > > > > Each SERDES lane mux can select upto 4 different IPs. > > > > There are 4 lanes in each J7200 SERDES. Define all > > > > the possible functions in this file. > > > > > > > > Cc: Peter Rosin <peda@axentia.se> > > > > Signed-off-by: Roger Quadros <rogerq@ti.com> > > > > --- > > > > include/dt-bindings/mux/mux-j7200-wiz.h | 29 +++++++++++++++++++++++++ > > > > 1 file changed, 29 insertions(+) > > > > create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h > > > > > > > > diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h b/include/dt-bindings/mux/mux-j7200-wiz.h > > > > new file mode 100644 > > > > index 000000000000..b091b1185a36 > > > > --- /dev/null > > > > +++ b/include/dt-bindings/mux/mux-j7200-wiz.h > > > > @@ -0,0 +1,29 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +/* > > > > + * This header provides constants for J7200 WIZ. > > > > + */ > > > > + > > > > +#ifndef _DT_BINDINGS_J7200_WIZ > > > > +#define _DT_BINDINGS_J7200_WIZ > > > > + > > > > +#define SERDES0_LANE0_QSGMII_LANE3 0x0 > > > > +#define SERDES0_LANE0_PCIE1_LANE0 0x1 > > > > +#define SERDES0_LANE0_IP3_UNUSED 0x2 > > > > +#define SERDES0_LANE0_IP4_UNUSED 0x3 > > > > + > > > > +#define SERDES0_LANE1_QSGMII_LANE4 0x0 > > > > +#define SERDES0_LANE1_PCIE1_LANE1 0x1 > > > > +#define SERDES0_LANE1_IP3_UNUSED 0x2 > > > > +#define SERDES0_LANE1_IP4_UNUSED 0x3 > > > > + > > > > +#define SERDES0_LANE2_QSGMII_LANE1 0x0 > > > > +#define SERDES0_LANE2_PCIE1_LANE2 0x1 > > > > +#define SERDES0_LANE2_IP3_UNUSED 0x2 > > > > +#define SERDES0_LANE2_IP4_UNUSED 0x3 > > > > + > > > > +#define SERDES0_LANE3_QSGMII_LANE2 0x0 > > > > +#define SERDES0_LANE3_PCIE1_LANE3 0x1 > > > > +#define SERDES0_LANE3_USB 0x2 > > > > +#define SERDES0_LANE3_IP4_UNUSED 0x3 > > > > + > > > > +#endif /* _DT_BINDINGS_J7200_WIZ */ > > > > > > Should not the defines start with J7200_WIZ? SERDES0 seems like a too > > > generic prefix, at least to me. > > > > Thanks, good point. I am not sure if WIZ should even be used.. It is > > a TI internal prefix for various serdes solutions, but I agree that > > SERDES0 is too generic a terminology. That said, we should cleanup > > include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing > > j7200 changes. > > > > I'm planning to put all TI SERDES definitions in one header file "ti-serdes-mux.h" > and add SOC specific prefixes to the macros. > > This will mean some churn in the existing DT files. (only 2 so far) Please check bindings and examples if any reference as well. Those changes will need to be considered as well. > > Are you guys OK if I do the change in one patch to avoid a broken build in between. > You guys can then decide whose tree it goes through. > > The new SoC addition will be separate of course. If Peter acks and is OK with the changes, then based on Peter's opinion, I'd rather take the changes via SoC tree for 5.10+ for maintaining bisectability. I prefer we name it ti-serdes-mux or something that Peter is OK with as well. reasons: i) "wiz" is yet another TLA deal even if documented in public TI TRM in some remote chapter, other non-TI folks are going to go scratching their heads.. ii) There is no way this can scale with one header per SoC!
On 11:45-20200917, Peter Rosin wrote: [...] > > >> Should not the defines start with J7200_WIZ? SERDES0 seems like a too > >> generic prefix, at least to me. > > > > Thanks, good point. I am not sure if WIZ should even be used.. It is > > a TI internal prefix for various serdes solutions, but I agree that > > SERDES0 is too generic a terminology. That said, we should cleanup > > include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing > > j7200 changes. > > Right. As maintainer for the directory in question, I should have > been on Cc for that series too, but it appears I wasn't. Hence, I yes, you should have been. The following commit introduced it. commit b766e3b0d5f6 ("arm64: dts: ti: k3-j721e-main: Add system controller node and SERDES lane mux") > didn't notice that file until now when I went looking for it. Why > wasn't I on Cc? Got through the SoC tree - an oversight on our part[1] and should'nt have, Apologies on the bad call. I would like to propose the following: a) The header should be renamed to be something more human friendly. b) The header should be renamed to be something TI specific and NOT per TI SoC. c) The macros need renaming to be less generic as it stands right now. If you ack the changes, I am guessing that the changes will impact dts a lot and would rather take the cleanups through SoC tree to maintain bisectability? OR I can pick on an immutable tag from you with just the header file change and pick on the dts - but I doubt that would be bisectable. Just worried that I have picked a bunch of cleanups already on the dts for 5.10, and would like to avoid a merge conflict. your thoughts? [1] https://lore.kernel.org/linux-devicetree/20200709231933.GA1083562@bogus/
On 15:47-20200917, Kishon Vijay Abraham I wrote: [..] > > Thanks, good point. I am not sure if WIZ should even be used.. It is > > a TI internal prefix for various serdes solutions, but I agree that > > SERDES0 is too generic a terminology. That said, we should cleanup > > include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing > > j7200 changes. > > WIZ is defined in public TRM (https://www.ti.com/lit/pdf/spruiu1). > " Maybe give the TRM team a feedback to use a little more human readable naming? if I just ctrl+f "wiz" in the 10,000 page trm the first match is: "CA bits can be swizzled between any bit positions via the DDRSS_PHY_1053[23-0] PHY_ADR_ADDR_SEL_0 field" (it is not even in the table of contents?) However, if I search for serdes, and then go down to page 7773, "WIZ: The WIZ acts as a wrapper for the SerDes, and can both send control signals to and report status signals from the SerDes, and muxes SerDes to peripherals" just call it ti-k3-serdes-mux (since there are other TI serdes muxes..)? You dont want to be on the brunt of something like [1] caused by, your's truely.. Also this is never going to scale with the number of devices we are spinning out.. one header per SoC? makes no sense to me. [1] https://groups.google.com/forum/#!msg/linux.kernel/fcntv48yoOc/35bAdI1eaiUJ
Hi! On 2020-09-17 14:00, Roger Quadros wrote: > Hi Peter & Nishanth, > > On 16/09/2020 18:45, Nishanth Menon wrote: >> On 06:52-20200916, Peter Rosin wrote: >>> Hi, >>> >>> Sorry for the delay. >>> >>> On 2020-09-15 13:20, Roger Quadros wrote: >>>> Each SERDES lane mux can select upto 4 different IPs. >>>> There are 4 lanes in each J7200 SERDES. Define all >>>> the possible functions in this file. >>>> >>>> Cc: Peter Rosin <peda@axentia.se> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>> --- >>>> include/dt-bindings/mux/mux-j7200-wiz.h | 29 +++++++++++++++++++++++++ >>>> 1 file changed, 29 insertions(+) >>>> create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h >>>> >>>> diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h b/include/dt-bindings/mux/mux-j7200-wiz.h >>>> new file mode 100644 >>>> index 000000000000..b091b1185a36 >>>> --- /dev/null >>>> +++ b/include/dt-bindings/mux/mux-j7200-wiz.h >>>> @@ -0,0 +1,29 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +/* >>>> + * This header provides constants for J7200 WIZ. >>>> + */ >>>> + >>>> +#ifndef _DT_BINDINGS_J7200_WIZ >>>> +#define _DT_BINDINGS_J7200_WIZ >>>> + >>>> +#define SERDES0_LANE0_QSGMII_LANE3 0x0 >>>> +#define SERDES0_LANE0_PCIE1_LANE0 0x1 >>>> +#define SERDES0_LANE0_IP3_UNUSED 0x2 >>>> +#define SERDES0_LANE0_IP4_UNUSED 0x3 >>>> + >>>> +#define SERDES0_LANE1_QSGMII_LANE4 0x0 >>>> +#define SERDES0_LANE1_PCIE1_LANE1 0x1 >>>> +#define SERDES0_LANE1_IP3_UNUSED 0x2 >>>> +#define SERDES0_LANE1_IP4_UNUSED 0x3 >>>> + >>>> +#define SERDES0_LANE2_QSGMII_LANE1 0x0 >>>> +#define SERDES0_LANE2_PCIE1_LANE2 0x1 >>>> +#define SERDES0_LANE2_IP3_UNUSED 0x2 >>>> +#define SERDES0_LANE2_IP4_UNUSED 0x3 >>>> + >>>> +#define SERDES0_LANE3_QSGMII_LANE2 0x0 >>>> +#define SERDES0_LANE3_PCIE1_LANE3 0x1 >>>> +#define SERDES0_LANE3_USB 0x2 >>>> +#define SERDES0_LANE3_IP4_UNUSED 0x3 >>>> + >>>> +#endif /* _DT_BINDINGS_J7200_WIZ */ >>> >>> Should not the defines start with J7200_WIZ? SERDES0 seems like a too >>> generic prefix, at least to me. >> >> Thanks, good point. I am not sure if WIZ should even be used.. It is >> a TI internal prefix for various serdes solutions, but I agree that >> SERDES0 is too generic a terminology. That said, we should cleanup >> include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing >> j7200 changes. >> > > I'm planning to put all TI SERDES definitions in one header file "ti-serdes-mux.h" > and add SOC specific prefixes to the macros. > > This will mean some churn in the existing DT files. (only 2 so far) > > Are you guys OK if I do the change in one patch to avoid a broken build in between. > You guys can then decide whose tree it goes through. > > The new SoC addition will be separate of course. We should get these changes done before 5.9 is released. Not breaking the build for each intermediate step is always a priority. Also, renaming mux-j721e-wiz.h to ti-serdes-mux.h and renaming the macros could be seen as orthogonal, and it is certainly possible to do that as two patches without breaking the build in between. It would just need changes on both sides of the interface in both patches. But I wouldn't worry about separating this into two patches, just do a rename patch and be done with it. Then follow up with additions for j7200. However, now that we are renaming things anyway, do we really need "mux" in the name of the file itself? I personally find .../dt-dbindings/mux/ti-serdes.h descriptive enough. Cheers, Peter
On 14:37-20200917, Peter Rosin wrote: [...] > >>> Should not the defines start with J7200_WIZ? SERDES0 seems like a too > >>> generic prefix, at least to me. > >> > >> Thanks, good point. I am not sure if WIZ should even be used.. It is > >> a TI internal prefix for various serdes solutions, but I agree that > >> SERDES0 is too generic a terminology. That said, we should cleanup > >> include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing > >> j7200 changes. > >> > > > > I'm planning to put all TI SERDES definitions in one header file "ti-serdes-mux.h" > > and add SOC specific prefixes to the macros. > > > > This will mean some churn in the existing DT files. (only 2 so far) > > > > Are you guys OK if I do the change in one patch to avoid a broken build in between. > > You guys can then decide whose tree it goes through. > > > > The new SoC addition will be separate of course. > > We should get these changes done before 5.9 is released. OK. > Not breaking the build for each intermediate step is always a priority. > Also, renaming mux-j721e-wiz.h to ti-serdes-mux.h and renaming the macros > could be seen as orthogonal, and it is certainly possible to do that > as two patches without breaking the build in between. It would just need > changes on both sides of the interface in both patches. But I wouldn't > worry about separating this into two patches, just do a rename patch and > be done with it. Then follow up with additions for j7200. > > However, now that we are renaming things anyway, do we really need "mux" > in the name of the file itself? > I personally find .../dt-dbindings/mux/ti-serdes.h descriptive enough. yep, OK with me.
On 2020-09-17 14:27, Nishanth Menon wrote: > On 11:45-20200917, Peter Rosin wrote: > [...] >> >>>> Should not the defines start with J7200_WIZ? SERDES0 seems like a too >>>> generic prefix, at least to me. >>> >>> Thanks, good point. I am not sure if WIZ should even be used.. It is >>> a TI internal prefix for various serdes solutions, but I agree that >>> SERDES0 is too generic a terminology. That said, we should cleanup >>> include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing >>> j7200 changes. >> >> Right. As maintainer for the directory in question, I should have >> been on Cc for that series too, but it appears I wasn't. Hence, I > > yes, you should have been. The following commit introduced it. > > commit b766e3b0d5f6 ("arm64: dts: ti: k3-j721e-main: Add system controller > node and SERDES lane mux") > >> didn't notice that file until now when I went looking for it. Why >> wasn't I on Cc? > > Got through the SoC tree - an oversight on our part[1] and should'nt have, > Apologies on the bad call. > > I would like to propose the following: > a) The header should be renamed to be something more human friendly. > b) The header should be renamed to be something TI specific and NOT per > TI SoC. > c) The macros need renaming to be less generic as it stands right now. > > > If you ack the changes, I am guessing that the changes will impact dts > a lot and would rather take the cleanups through SoC tree to maintain > bisectability? OR I can pick on an immutable tag from you with just the > header file change and pick on the dts - but I doubt that would be > bisectable. Just worried that I have picked a bunch of cleanups already > on the dts for 5.10, and would like to avoid a merge conflict. [Our mails crossed.] I do not have a tree and dt-patches should normally not go *through* me. But I'd still like to see what's happening. I did not realize this was going to cause a *lot* of churn in the dt files. How bad can it be when the file is new in this cycle? And is it worth it? But it seems you all see problems with the current naming and in that case it must surely be better to fix it early? And if it's a lot, maybe it needs to be more than one patch? I get the feeling this will need to be taken care of by someone other than me, because I'm just the maintainer of a very small subsystem and I don't normally have to deal with "big" issues involving several trees. I would be a bottleneck. Cheers, Peter
On 14:53-20200917, Peter Rosin wrote: [..] > I do not have a tree and dt-patches should normally not go *through* me. yep. > But I'd still like to see what's happening. > I did not realize this was going to cause a *lot* of churn in the dt > files. How bad can it be when the file is new in this cycle? And is it > worth it? But it seems you all see problems with the current naming and > in that case it must surely be better to fix it early? yes - I think I can figure it out. > > And if it's a lot, maybe it needs to be more than one patch? I get the > feeling this will need to be taken care of by someone other than me, > because I'm just the maintainer of a very small subsystem and I don't > normally have to deal with "big" issues involving several trees. I would > be a bottleneck. No a pain, I think I can help get it through, may be, I worry too much.. Lets see how the cleanup series looks like. Roger: I am not touching the j7200 series till we cleanup j721e as discussed in this thread. please use 5.9-rc1 as your baseline for cleanup. Once available, we can figure out how to get 5.10-rc1 staged items later. Window is narrowing rather soon, so appreciate a quick cleaup :).
diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h b/include/dt-bindings/mux/mux-j7200-wiz.h new file mode 100644 index 000000000000..b091b1185a36 --- /dev/null +++ b/include/dt-bindings/mux/mux-j7200-wiz.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * This header provides constants for J7200 WIZ. + */ + +#ifndef _DT_BINDINGS_J7200_WIZ +#define _DT_BINDINGS_J7200_WIZ + +#define SERDES0_LANE0_QSGMII_LANE3 0x0 +#define SERDES0_LANE0_PCIE1_LANE0 0x1 +#define SERDES0_LANE0_IP3_UNUSED 0x2 +#define SERDES0_LANE0_IP4_UNUSED 0x3 + +#define SERDES0_LANE1_QSGMII_LANE4 0x0 +#define SERDES0_LANE1_PCIE1_LANE1 0x1 +#define SERDES0_LANE1_IP3_UNUSED 0x2 +#define SERDES0_LANE1_IP4_UNUSED 0x3 + +#define SERDES0_LANE2_QSGMII_LANE1 0x0 +#define SERDES0_LANE2_PCIE1_LANE2 0x1 +#define SERDES0_LANE2_IP3_UNUSED 0x2 +#define SERDES0_LANE2_IP4_UNUSED 0x3 + +#define SERDES0_LANE3_QSGMII_LANE2 0x0 +#define SERDES0_LANE3_PCIE1_LANE3 0x1 +#define SERDES0_LANE3_USB 0x2 +#define SERDES0_LANE3_IP4_UNUSED 0x3 + +#endif /* _DT_BINDINGS_J7200_WIZ */
Each SERDES lane mux can select upto 4 different IPs. There are 4 lanes in each J7200 SERDES. Define all the possible functions in this file. Cc: Peter Rosin <peda@axentia.se> Signed-off-by: Roger Quadros <rogerq@ti.com> --- include/dt-bindings/mux/mux-j7200-wiz.h | 29 +++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h