[PATCHv3 08/15] dts: powerpc: p1020rdb: Add eTSEC DT nodes
Z.q. Hou
zhiqiang.hou at nxp.com
Mon Jun 15 10:50:30 CEST 2020
Hi Vladimir,
Thanks a lot for your comments!
> -----Original Message-----
> From: Vladimir Oltean [mailto:olteanv at gmail.com]
> Sent: 2020年6月13日 4:15
> To: Z.q. Hou <zhiqiang.hou at nxp.com>
> Cc: u-boot <u-boot at lists.denx.de>; Priyanka Jain <priyanka.jain at nxp.com>;
> Bin Meng <bmeng.cn at gmail.com>
> Subject: Re: [PATCHv3 08/15] dts: powerpc: p1020rdb: Add eTSEC DT nodes
>
> On Fri, 12 Jun 2020 at 18:23, Zhiqiang Hou <Zhiqiang.Hou at nxp.com> wrote:
> >
> > From: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
> >
> > P1020RDB implements 3 enhanced three-speed Ethernet controllers, and
> > the connection is shown below:
> > eTSEC1: Connected to RGMII PHY VSC7385
>
> As you said in a previous patch, VSC7385 is a switch, not a PHY.
These connection info was copied from the kernel changelog without further polish.
I'll correct it in next version.
>
> > eTSEC2: Connected to SGMII PHY VSC8221
> > eTSEC3: Connected to SGMII PHY AR8021
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
> > ---
> > V3:
> > - Rebase the patch, no change intended.
> >
> > arch/powerpc/dts/p1020-post.dtsi | 16 +++++++
> > arch/powerpc/dts/p1020rdb-pc.dts | 1 +
> > arch/powerpc/dts/p1020rdb-pc.dtsi | 55
> ++++++++++++++++++++++++
> > arch/powerpc/dts/p1020rdb-pc_36b.dts | 1 +
> > arch/powerpc/dts/p1020rdb-pd.dts | 57
> +++++++++++++++++++++++++
> > arch/powerpc/dts/pq3-etsec2-0.dtsi | 35 +++++++++++++++
> > arch/powerpc/dts/pq3-etsec2-1.dtsi | 35 +++++++++++++++
> > arch/powerpc/dts/pq3-etsec2-2.dtsi | 35 +++++++++++++++
> > arch/powerpc/dts/pq3-etsec2-grp2-0.dtsi | 16 +++++++
> > arch/powerpc/dts/pq3-etsec2-grp2-1.dtsi | 16 +++++++
> > arch/powerpc/dts/pq3-etsec2-grp2-2.dtsi | 16 +++++++
> > 11 files changed, 283 insertions(+)
> > create mode 100644 arch/powerpc/dts/p1020rdb-pc.dtsi create mode
> > 100644 arch/powerpc/dts/pq3-etsec2-0.dtsi
> > create mode 100644 arch/powerpc/dts/pq3-etsec2-1.dtsi
> > create mode 100644 arch/powerpc/dts/pq3-etsec2-2.dtsi
> > create mode 100644 arch/powerpc/dts/pq3-etsec2-grp2-0.dtsi
> > create mode 100644 arch/powerpc/dts/pq3-etsec2-grp2-1.dtsi
> > create mode 100644 arch/powerpc/dts/pq3-etsec2-grp2-2.dtsi
> >
> > diff --git a/arch/powerpc/dts/p1020-post.dtsi
> > b/arch/powerpc/dts/p1020-post.dtsi
> > index 1dce8e86e9..2c0aa7a5c3 100644
> > --- a/arch/powerpc/dts/p1020-post.dtsi
> > +++ b/arch/powerpc/dts/p1020-post.dtsi
> > @@ -46,8 +46,24 @@
> >
> > /include/ "pq3-i2c-0.dtsi"
> > /include/ "pq3-i2c-1.dtsi"
> > +
> > +/include/ "pq3-etsec2-0.dtsi"
>
> Could you keep the indentation level of this include the same as the others?
I don't think it is a good style to add indentation for the /include/ lines, I don't
know why Biwen added it for these 2, and not for others.
I can help to remove the indentation for the i2c include entries.
>
> > + enet0: enet0_grp2: ethernet at b0000 {
>
> I don't understand why you're doing it like this (i.e. specifying the label here,
> and using it in pq3-etsec2-grp2-0.dtsi, vs just specifying the full path in
> pq3-etsec2-grp2-0.dtsi).
These DT nodes was copied from the Linux.
>
> > + };
> > +
> > +/include/ "pq3-etsec2-1.dtsi"
> > + enet1: enet1_grp2: ethernet at b1000 {
> > + };
> > +
> > +/include/ "pq3-etsec2-2.dtsi"
> > + enet2: enet2_grp2: ethernet at b2000 {
> > + };
> > };
> >
> > +/include/ "pq3-etsec2-grp2-0.dtsi"
> > +/include/ "pq3-etsec2-grp2-1.dtsi"
> > +/include/ "pq3-etsec2-grp2-2.dtsi"
> > +
> > /* PCIe controller base address 0x9000 */
> > &pci1 {
> > compatible = "fsl,pcie-p1_p2", "fsl,pcie-fsl-qoriq"; diff
> > --git a/arch/powerpc/dts/p1020rdb-pc.dts
> > b/arch/powerpc/dts/p1020rdb-pc.dts
> > index 7ebaa619df..715330dc50 100644
> > --- a/arch/powerpc/dts/p1020rdb-pc.dts
> > +++ b/arch/powerpc/dts/p1020rdb-pc.dts
> > @@ -32,4 +32,5 @@
> > };
> > };
> >
> > +/include/ "p1020rdb-pc.dtsi"
> > /include/ "p1020-post.dtsi"
> > diff --git a/arch/powerpc/dts/p1020rdb-pc.dtsi
> > b/arch/powerpc/dts/p1020rdb-pc.dtsi
> > new file mode 100644
> > index 0000000000..6bf424fd3f
> > --- /dev/null
> > +++ b/arch/powerpc/dts/p1020rdb-pc.dtsi
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0+ OR X11
> > +/*
> > + * P1020 RDB-PC Device Tree Source stub (no addresses or top-level
> > +ranges)
> > + *
> > + * Copyright 2012 Freescale Semiconductor Inc.
> > + * Copyright 2020 NXP
> > + */
> > +
> > +&soc {
> > + mdio at 24000 {
> > + phy0: ethernet-phy at 0 {
> > + interrupt-parent = <&mpic>;
> > + interrupts = <3 1 0 0>;
> > + reg = <0x0>;
> > + };
> > +
> > + phy1: ethernet-phy at 1 {
> > + interrupt-parent = <&mpic>;
> > + interrupts = <2 1 0 0>;
> > + reg = <0x1>;
> > + };
> > +
> > + tbi0: tbi-phy at 11 {
> > + device_type = "tbi-phy";
> > + reg = <0x11>;
> > + };
> > + };
> > +
> > + mdio at 25000 {
> > + tbi1: tbi-phy at 11 {
> > + reg = <0x11>;
> > + device_type = "tbi-phy";
> > + };
> > + };
> > +
> > + enet0: ethernet at b0000 {
> > + phy-connection-type = "rgmii-id";
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + };
> > +
> > + };
> > +
> > + enet1: ethernet at b1000 {
> > + phy-handle = <&phy0>;
> > + tbi-handle = <&tbi1>;
> > + phy-connection-type = "sgmii";
> > + };
> > +
> > + enet2: ethernet at b2000 {
> > + phy-handle = <&phy1>;
> > + phy-connection-type = "rgmii-id";
> > + };
> > +};
> > diff --git a/arch/powerpc/dts/p1020rdb-pc_36b.dts
> > b/arch/powerpc/dts/p1020rdb-pc_36b.dts
> > index c0e5ef4cf4..7680b7c7e1 100644
> > --- a/arch/powerpc/dts/p1020rdb-pc_36b.dts
> > +++ b/arch/powerpc/dts/p1020rdb-pc_36b.dts
> > @@ -32,4 +32,5 @@
> > };
> > };
> >
> > +/include/ "p1020rdb-pc.dtsi"
> > /include/ "p1020-post.dtsi"
> > diff --git a/arch/powerpc/dts/p1020rdb-pd.dts
> > b/arch/powerpc/dts/p1020rdb-pd.dts
> > index 21174a09be..7868c9b95c 100644
> > --- a/arch/powerpc/dts/p1020rdb-pd.dts
> > +++ b/arch/powerpc/dts/p1020rdb-pd.dts
> > @@ -17,6 +17,63 @@
> >
> > soc: soc at ffe00000 {
> > ranges = <0x0 0x0 0xffe00000 0x100000>;
> > +
> > + mdio at 24000 {
> > + phy0: ethernet-phy at 0 {
> > + interrupts = <3 1 0 0>;
> > + reg = <0x0>;
> > + };
> > +
> > + phy1: ethernet-phy at 1 {
> > + interrupts = <2 1 0 0>;
> > + reg = <0x1>;
> > + };
> > + };
> > +
> > + mdio at 25000 {
> > + tbi1: tbi-phy at 11 {
> > + reg = <0x11>;
> > + device_type = "tbi-phy";
> > + };
> > + };
> > +
> > + mdio at 26000 {
> > + tbi2: tbi-phy at 11 {
> > + reg = <0x11>;
> > + device_type = "tbi-phy";
> > + };
> > + };
> > +
> > + ptp_clock at b0e00 {
> > + compatible = "fsl,etsec-ptp";
> > + reg = <0xb0e00 0xb0>;
> > + interrupts = <68 2 0 0 69 2 0 0>;
> > + fsl,tclk-period = <10>;
> > + fsl,tmr-prsc = <2>;
> > + fsl,tmr-add = <0x80000016>;
> > + fsl,tmr-fiper1 = <999999990>;
> > + fsl,tmr-fiper2 = <99990>;
> > + fsl,max-adj = <199999999>;
> > + };
>
> I'm pretty sure the PTP clock is not needed in U-Boot? Doesn't checkpatch
> give warnings about undocumented compatible string?
OK, the PTP node will be removed in next version.
Checkpatch tool does not report.
>
> > +
> > + enet0: ethernet at b0000 {
> > + phy-connection-type = "rgmii-id";
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + };
> > + };
> > +
> > + enet1: ethernet at b1000 {
> > + phy-handle = <&phy0>;
> > + tbi-handle = <&tbi1>;
> > + phy-connection-type = "sgmii";
> > + };
> > +
> > + enet2: ethernet at b2000 {
> > + phy-handle = <&phy1>;
> > + phy-connection-type = "rgmii-id";
> > + };
> > };
> >
> > pci1: pcie at ffe09000 {
> > diff --git a/arch/powerpc/dts/pq3-etsec2-0.dtsi
> > b/arch/powerpc/dts/pq3-etsec2-0.dtsi
> > new file mode 100644
> > index 0000000000..f9d3d04650
> > --- /dev/null
> > +++ b/arch/powerpc/dts/pq3-etsec2-0.dtsi
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0+ OR X11
> > +/*
> > + * PQ3 eTSEC2 device tree stub [ @ offsets 0x24000/0xb0000 ]
> > + *
> > + * Copyright 2011 Freescale Semiconductor Inc.
>
> Why the 2011 Freescale copyright?
As I had said these are copied from Linux, I think it is better to keep the copyright entry.
>
> > + * Copyright 2020 NXP
> > + */
> > +
> > +mdio at 24000 {
>
> You could have labeled the MDIO buses here, and just refer to the label in the
> board dtsi files. I'm not sure but you may need to use #include instead of
> /include/ for that.
I trend to align with Linux DT nodes to reduce the risk.
>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "fsl,etsec2-mdio";
> > + reg = <0x24000 0x1000 0xb0030 0x4>; };
> > +
> > +ethernet at b0000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + device_type = "network";
> > + model = "eTSEC";
> > + compatible = "fsl,etsec2";
> > + reg = <0xb0000 0x1000>;
> > + fsl,num_rx_queues = <0x8>;
> > + fsl,num_tx_queues = <0x8>;
> > + fsl,magic-packet;
> > + local-mac-address = [ 00 00 00 00 00 00 ];
>
> I don't think this is needed.
>
> > + ranges;
> > +
> > + queue-group at b0000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xb0000 0x1000>;
> > + interrupts = <29 2 0 0 30 2 0 0 34 2 0 0>;
>
> What do these numbers represent?
> I'm not sure it is in the best interest of maintaining this driver to import so
> many DT bindings from Linux which are unused. It is highly unlikely that
> anyone will make use of them in the future.
I want to align with Linux DT nodes, though I know current uboot doesn't use
interrupt, but I don't trend to change them unless necessary.
Thanks,
Zhiqiang
>
> > + };
> > +};
> > diff --git a/arch/powerpc/dts/pq3-etsec2-1.dtsi
> > b/arch/powerpc/dts/pq3-etsec2-1.dtsi
> > new file mode 100644
> > index 0000000000..6c01481909
> > --- /dev/null
> > +++ b/arch/powerpc/dts/pq3-etsec2-1.dtsi
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0+ OR X11
> > +/*
> > + * PQ3 eTSEC2 device tree stub [ @ offsets 0x25000/0xb1000 ]
> > + *
> > + * Copyright 2011 Freescale Semiconductor Inc.
> > + * Copyright 2020 NXP
> > + */
> > +
> > +mdio at 25000 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "fsl,etsec2-tbi";
> > + reg = <0x25000 0x1000 0xb1030 0x4>; };
> > +
> > +ethernet at b1000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + device_type = "network";
> > + model = "eTSEC";
> > + compatible = "fsl,etsec2";
> > + reg = <0xb1000 0x1000>;
> > + fsl,num_rx_queues = <0x8>;
> > + fsl,num_tx_queues = <0x8>;
> > + fsl,magic-packet;
> > + local-mac-address = [ 00 00 00 00 00 00 ];
> > + ranges;
> > +
> > + queue-group at b1000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xb1000 0x1000>;
> > + interrupts = <35 2 0 0 36 2 0 0 40 2 0 0>;
> > + };
> > +};
> > diff --git a/arch/powerpc/dts/pq3-etsec2-2.dtsi
> > b/arch/powerpc/dts/pq3-etsec2-2.dtsi
> > new file mode 100644
> > index 0000000000..2a597c0db6
> > --- /dev/null
> > +++ b/arch/powerpc/dts/pq3-etsec2-2.dtsi
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0+ OR X11
> > +/*
> > + * PQ3 eTSEC2 device tree stub [ @ offsets 0x26000/0xb2000 ]
> > + *
> > + * Copyright 2011 Freescale Semiconductor Inc.
> > + * Copyright 2020 NXP
> > + */
> > +
> > +mdio at 26000 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "fsl,etsec2-tbi";
> > + reg = <0x26000 0x1000 0xb1030 0x4>; };
> > +
> > +ethernet at b2000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + device_type = "network";
> > + model = "eTSEC";
> > + compatible = "fsl,etsec2";
> > + reg = <0xb2000 0x1000>;
> > + fsl,num_rx_queues = <0x8>;
> > + fsl,num_tx_queues = <0x8>;
> > + fsl,magic-packet;
> > + local-mac-address = [ 00 00 00 00 00 00 ];
> > + ranges;
> > +
> > + queue-group at b2000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xb2000 0x1000>;
> > + interrupts = <31 2 0 0 32 2 0 0 33 2 0 0>;
> > + };
> > +};
> > diff --git a/arch/powerpc/dts/pq3-etsec2-grp2-0.dtsi
> > b/arch/powerpc/dts/pq3-etsec2-grp2-0.dtsi
> > new file mode 100644
> > index 0000000000..16752a7c45
> > --- /dev/null
> > +++ b/arch/powerpc/dts/pq3-etsec2-grp2-0.dtsi
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0+ OR X11
> > +/*
> > + * PQ3 eTSEC2 Group 2 device tree stub [ @ offsets 0xb4000 ]
> > + *
> > + * Copyright 2011 Freescale Semiconductor Inc.
> > + * Copyright 2020 NXP
> > + */
> > +
> > +&enet0_grp2 {
> > + queue-group at b4000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xb4000 0x1000>;
> > + interrupts = <17 2 0 0 18 2 0 0 24 2 0 0>;
> > + };
> > +};
> > diff --git a/arch/powerpc/dts/pq3-etsec2-grp2-1.dtsi
> > b/arch/powerpc/dts/pq3-etsec2-grp2-1.dtsi
> > new file mode 100644
> > index 0000000000..0464938424
> > --- /dev/null
> > +++ b/arch/powerpc/dts/pq3-etsec2-grp2-1.dtsi
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0+ OR X11
> > +/*
> > + * PQ3 eTSEC2 Group 2 device tree stub [ @ offsets 0xb5000 ]
> > + *
> > + * Copyright 2011 Freescale Semiconductor Inc.
> > + * Copyright 2020 NXP
> > + */
> > +
> > +&enet1_grp2 {
> > + queue-group at b5000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xb5000 0x1000>;
> > + interrupts = <51 2 0 0 52 2 0 0 67 2 0 0>;
> > + };
> > +};
> > diff --git a/arch/powerpc/dts/pq3-etsec2-grp2-2.dtsi
> > b/arch/powerpc/dts/pq3-etsec2-grp2-2.dtsi
> > new file mode 100644
> > index 0000000000..fe8003c44a
> > --- /dev/null
> > +++ b/arch/powerpc/dts/pq3-etsec2-grp2-2.dtsi
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0+ OR X11
> > +/*
> > + * PQ3 eTSEC2 Group 2 device tree stub [ @ offsets 0xb6000 ]
> > + *
> > + * Copyright 2011 Freescale Semiconductor Inc.
> > + * Copyright 2020 NXP
> > + */
> > +
> > +&enet2_grp2 {
> > + queue-group at b6000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xb6000 0x1000>;
> > + interrupts = <25 2 0 0 26 2 0 0 27 2 0 0>;
> > + };
> > +};
> > --
> > 2.25.1
> >
More information about the U-Boot
mailing list