Re: [PATCH] arm64: dts: ti: k3-j721e: Rename mux header and update macro names

From: Peter Rosin
Date: Thu Sep 17 2020 - 16:44:22 EST

Hi!

A few questions. I don't really care, but if someone does then at least I
highlighted few thing out.

On 2020-09-17 18:16, Roger Quadros wrote:
> We intend to use one header file for SERDES MUX for all
> TI SoCs so rename the header file.
>
> The exsting macros are too generic. Prefix them with SoC name.
>
> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> ---
> .../dts/ti/k3-j721e-common-proc-board.dts | 11 ++--
> arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 13 +++--
> include/dt-bindings/mux/mux-j721e-wiz.h | 53 ------------------
> include/dt-bindings/mux/ti-serdes.h | 55 +++++++++++++++++++
> 4 files changed, 68 insertions(+), 64 deletions(-)
> delete mode 100644 include/dt-bindings/mux/mux-j721e-wiz.h
> create mode 100644 include/dt-bindings/mux/ti-serdes.h
>
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> index 8bc1e6ecc50e..493f64ee7a2a 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> @@ -404,11 +404,12 @@
> };
>
> &serdes_ln_ctrl {
> - idle-states = <SERDES0_LANE0_PCIE0_LANE0>, <SERDES0_LANE1_PCIE0_LANE1>,
> - <SERDES1_LANE0_PCIE1_LANE0>, <SERDES1_LANE1_PCIE1_LANE1>,
> - <SERDES2_LANE0_PCIE2_LANE0>, <SERDES2_LANE1_PCIE2_LANE1>,
> - <SERDES3_LANE0_USB3_0_SWAP>, <SERDES3_LANE1_USB3_0>,
> - <SERDES4_LANE0_EDP_LANE0>, <SERDES4_LANE1_EDP_LANE1>, <SERDES4_LANE2_EDP_LANE2>, <SERDES4_LANE3_EDP_LANE3>;
> + idle-states = <J721E_SERDES0_LANE0_PCIE0_LANE0>, <J721E_SERDES0_LANE1_PCIE0_LANE1>,
> + <J721E_SERDES1_LANE0_PCIE1_LANE0>, <J721E_SERDES1_LANE1_PCIE1_LANE1>,
> + <J721E_SERDES2_LANE0_PCIE2_LANE0>, <J721E_SERDES2_LANE1_PCIE2_LANE1>,
> + <J721E_SERDES3_LANE0_USB3_0_SWAP>, <J721E_SERDES3_LANE1_USB3_0>,
> + <J721E_SERDES4_LANE0_EDP_LANE0>, <J721E_SERDES4_LANE1_EDP_LANE1>,
> + <J721E_SERDES4_LANE2_EDP_LANE2>, <J721E_SERDES4_LANE3_EDP_LANE3>;
> };
>
> &serdes_wiz3 {
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> index d14060207f00..924c51146d01 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> @@ -6,7 +6,7 @@
> */
> #include <dt-bindings/phy/phy.h>
> #include <dt-bindings/mux/mux.h>
> -#include <dt-bindings/mux/mux-j721e-wiz.h>
> +#include <dt-bindings/mux/ti-serdes.h>
>
> &cbass_main {
> msmc_ram: sram@70000000 {
> @@ -38,11 +38,12 @@
> <0x40b0 0x3>, <0x40b4 0x3>, /* SERDES3 lane0/1 select */
> <0x40c0 0x3>, <0x40c4 0x3>, <0x40c8 0x3>, <0x40cc 0x3>;
> /* SERDES4 lane0/1/2/3 select */
> - idle-states = <SERDES0_LANE0_PCIE0_LANE0>, <SERDES0_LANE1_PCIE0_LANE1>,
> - <SERDES1_LANE0_PCIE1_LANE0>, <SERDES1_LANE1_PCIE1_LANE1>,
> - <SERDES2_LANE0_PCIE2_LANE0>, <SERDES2_LANE1_PCIE2_LANE1>,
> - <MUX_IDLE_AS_IS>, <SERDES3_LANE1_USB3_0>,
> - <SERDES4_LANE0_EDP_LANE0>, <SERDES4_LANE1_EDP_LANE1>, <SERDES4_LANE2_EDP_LANE2>, <SERDES4_LANE3_EDP_LANE3>;
> + idle-states = <J721E_SERDES0_LANE0_PCIE0_LANE0>, <J721E_SERDES0_LANE1_PCIE0_LANE1>,
> + <J721E_SERDES1_LANE0_PCIE1_LANE0>, <J721E_SERDES1_LANE1_PCIE1_LANE1>,
> + <J721E_SERDES2_LANE0_PCIE2_LANE0>, <J721E_SERDES2_LANE1_PCIE2_LANE1>,
> + <MUX_IDLE_AS_IS>, <J721E_SERDES3_LANE1_USB3_0>,
> + <J721E_SERDES4_LANE0_EDP_LANE0>, <J721E_SERDES4_LANE1_EDP_LANE1>,
> + <J721E_SERDES4_LANE2_EDP_LANE2>, <J721E_SERDES4_LANE3_EDP_LANE3>;
> };
>
> usb_serdes_mux: mux-controller@4000 {
> diff --git a/include/dt-bindings/mux/mux-j721e-wiz.h b/include/dt-bindings/mux/mux-j721e-wiz.h
> deleted file mode 100644
> index fd1c4ea9fc7f..000000000000
> --- a/include/dt-bindings/mux/mux-j721e-wiz.h
> +++ /dev/null
> @@ -1,53 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/*
> - * This header provides constants for J721E WIZ.
> - */
> -
> -#ifndef _DT_BINDINGS_J721E_WIZ
> -#define _DT_BINDINGS_J721E_WIZ
> -
> -#define SERDES0_LANE0_QSGMII_LANE1 0x0
> -#define SERDES0_LANE0_PCIE0_LANE0 0x1
> -#define SERDES0_LANE0_USB3_0_SWAP 0x2
> -
> -#define SERDES0_LANE1_QSGMII_LANE2 0x0
> -#define SERDES0_LANE1_PCIE0_LANE1 0x1
> -#define SERDES0_LANE1_USB3_0 0x2
> -
> -#define SERDES1_LANE0_QSGMII_LANE3 0x0
> -#define SERDES1_LANE0_PCIE1_LANE0 0x1
> -#define SERDES1_LANE0_USB3_1_SWAP 0x2
> -#define SERDES1_LANE0_SGMII_LANE0 0x3
> -
> -#define SERDES1_LANE1_QSGMII_LANE4 0x0
> -#define SERDES1_LANE1_PCIE1_LANE1 0x1
> -#define SERDES1_LANE1_USB3_1 0x2
> -#define SERDES1_LANE1_SGMII_LANE1 0x3
> -
> -#define SERDES2_LANE0_PCIE2_LANE0 0x1
> -#define SERDES2_LANE0_SGMII_LANE0 0x3
> -#define SERDES2_LANE0_USB3_1_SWAP 0x2
> -
> -#define SERDES2_LANE1_PCIE2_LANE1 0x1
> -#define SERDES2_LANE1_USB3_1 0x2
> -#define SERDES2_LANE1_SGMII_LANE1 0x3
> -
> -#define SERDES3_LANE0_PCIE3_LANE0 0x1
> -#define SERDES3_LANE0_USB3_0_SWAP 0x2
> -
> -#define SERDES3_LANE1_PCIE3_LANE1 0x1
> -#define SERDES3_LANE1_USB3_0 0x2
> -
> -#define SERDES4_LANE0_EDP_LANE0 0x0
> -#define SERDES4_LANE0_QSGMII_LANE5 0x2
> -
> -#define SERDES4_LANE1_EDP_LANE1 0x0
> -#define SERDES4_LANE1_QSGMII_LANE6 0x2
> -
> -#define SERDES4_LANE2_EDP_LANE2 0x0
> -#define SERDES4_LANE2_QSGMII_LANE7 0x2
> -
> -#define SERDES4_LANE3_EDP_LANE3 0x0
> -#define SERDES4_LANE3_QSGMII_LANE8 0x2
> -
> -#endif /* _DT_BINDINGS_J721E_WIZ */
> diff --git a/include/dt-bindings/mux/ti-serdes.h b/include/dt-bindings/mux/ti-serdes.h
> new file mode 100644
> index 000000000000..3e1f2d243e4a
> --- /dev/null
> +++ b/include/dt-bindings/mux/ti-serdes.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides constants for SERDES MUX for TI SoCs
> + */
> +
> +#ifndef _DT_BINDINGS_TI_SERDES_MUX
> +#define _DT_BINDINGS_TI_SERDES_MUX

I would have spelled this _DT_BINDINGS_MUX_TI_SERDES. But as stated, it
doesn't really matter to me.

> +
> +/* J721E */
> +
> +#define J721E_SERDES0_LANE0_QSGMII_LANE1 0x0
> +#define J721E_SERDES0_LANE0_PCIE0_LANE0 0x1
> +#define J721E_SERDES0_LANE0_USB3_0_SWAP 0x2
> +
> +#define J721E_SERDES0_LANE1_QSGMII_LANE2 0x0
> +#define J721E_SERDES0_LANE1_PCIE0_LANE1 0x1
> +#define J721E_SERDES0_LANE1_USB3_0 0x2
> +
> +#define J721E_SERDES1_LANE0_QSGMII_LANE3 0x0
> +#define J721E_SERDES1_LANE0_PCIE1_LANE0 0x1
> +#define J721E_SERDES1_LANE0_USB3_1_SWAP 0x2
> +#define J721E_SERDES1_LANE0_SGMII_LANE0 0x3
> +
> +#define J721E_SERDES1_LANE1_QSGMII_LANE4 0x0
> +#define J721E_SERDES1_LANE1_PCIE1_LANE1 0x1
> +#define J721E_SERDES1_LANE1_USB3_1 0x2
> +#define J721E_SERDES1_LANE1_SGMII_LANE1 0x3
> +
> +#define J721E_SERDES2_LANE0_PCIE2_LANE0 0x1
> +#define J721E_SERDES2_LANE0_SGMII_LANE0 0x3
> +#define J721E_SERDES2_LANE0_USB3_1_SWAP 0x2
> +
> +#define J721E_SERDES2_LANE1_PCIE2_LANE1 0x1
> +#define J721E_SERDES2_LANE1_USB3_1 0x2
> +#define J721E_SERDES2_LANE1_SGMII_LANE1 0x3
> +
> +#define J721E_SERDES3_LANE0_PCIE3_LANE0 0x1
> +#define J721E_SERDES3_LANE0_USB3_0_SWAP 0x2
> +
> +#define J721E_SERDES3_LANE1_PCIE3_LANE1 0x1
> +#define J721E_SERDES3_LANE1_USB3_0 0x2
> +
> +#define J721E_SERDES4_LANE0_EDP_LANE0 0x0
> +#define J721E_SERDES4_LANE0_QSGMII_LANE5 0x2
> +
> +#define J721E_SERDES4_LANE1_EDP_LANE1 0x0
> +#define J721E_SERDES4_LANE1_QSGMII_LANE6 0x2
> +
> +#define J721E_SERDES4_LANE2_EDP_LANE2 0x0
> +#define J721E_SERDES4_LANE2_QSGMII_LANE7 0x2
> +
> +#define J721E_SERDES4_LANE3_EDP_LANE3 0x0
> +#define J721E_SERDES4_LANE3_QSGMII_LANE8 0x2
> +
> +#endif /* _DT_BINDINGS_TI_SERDES_MUX */
>

The J7200-series listed *all* possible mux values, some with names
like BLA_BLA_UNUSED. Why is that different here? Some dt-files using
the J7200 then ended up using these "unused" entries (for idle-states)
so maybe thoes values are useful here as well? The choice of using
_UNUSED for entries that end up being used can of course also be
debated :-)

If it is ill-adviced to use the values not listed above, that's
another matter of course...

I don't know the answer to the above, and will not be impacted in the
least, I'm just throwing out questions. So, either way:

Acked-by: Peter Rosin <peda@xxxxxxxxxx>

Cheers,
Peter