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 <[email protected]>
Signed-off-by: Roger Quadros <[email protected]>
---
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 */
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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 <[email protected]>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> 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 <[email protected]>
> > Signed-off-by: Roger Quadros <[email protected]>
> > ---
> > 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.
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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 <[email protected]>
>>> Signed-off-by: Roger Quadros <[email protected]>
>>> ---
*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 <[email protected]>
>>> Signed-off-by: Roger Quadros <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> > > > Signed-off-by: Roger Quadros <[email protected]>
> > > > ---
> > > > 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!
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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 <[email protected]>
>>> Signed-off-by: Roger Quadros <[email protected]>
>>> ---
>>> 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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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/
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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 <[email protected]>
>>>> Signed-off-by: Roger Quadros <[email protected]>
>>>> ---
>>>> 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 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: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.
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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 :).
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D