diff mbox series

[v3,1/6] dt-bindings: mux-j7200-wiz: Add lane function defines

Message ID 20200915112038.30219-2-rogerq@ti.com
State Changes Requested, archived
Headers show
Series arm64: dts: ti: Add USB support for J7200 EVM | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Roger Quadros Sept. 15, 2020, 11:20 a.m. UTC 
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

Comments

Peter Rosin Sept. 16, 2020, 4:52 a.m. UTC | #1 
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
Nishanth Menon Sept. 16, 2020, 3:45 p.m. UTC | #2 
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.
Peter Rosin Sept. 17, 2020, 9:45 a.m. UTC | #3 
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
Kishon Vijay Abraham I Sept. 17, 2020, 10:17 a.m. UTC | #4 
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
Roger Quadros Sept. 17, 2020, noon UTC | #5 
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
Nishanth Menon Sept. 17, 2020, 12:14 p.m. UTC | #6 
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!
Nishanth Menon Sept. 17, 2020, 12:27 p.m. UTC | #7 
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/
Nishanth Menon Sept. 17, 2020, 12:36 p.m. UTC | #8 
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
Peter Rosin Sept. 17, 2020, 12:37 p.m. UTC | #9 
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
Nishanth Menon Sept. 17, 2020, 12:51 p.m. UTC | #10 
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.
Peter Rosin Sept. 17, 2020, 12:53 p.m. UTC | #11 
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
Nishanth Menon Sept. 17, 2020, 1 p.m. UTC | #12 
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 mbox series

Patch

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 */