Hi Vladimir, Thanks a lot for your comments!
> -----Original Message----- > From: Vladimir Oltean [mailto:olte...@gmail.com] > Sent: 2020年6月13日 4:15 > To: Z.q. Hou <zhiqiang....@nxp.com> > Cc: u-boot <u-boot@lists.denx.de>; Priyanka Jain <priyanka.j...@nxp.com>; > Bin Meng <bmeng...@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....@nxp.com> wrote: > > > > From: Hou Zhiqiang <zhiqiang....@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....@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@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@b1000 { > > + }; > > + > > +/include/ "pq3-etsec2-2.dtsi" > > + enet2: enet2_grp2: ethernet@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@24000 { > > + phy0: ethernet-phy@0 { > > + interrupt-parent = <&mpic>; > > + interrupts = <3 1 0 0>; > > + reg = <0x0>; > > + }; > > + > > + phy1: ethernet-phy@1 { > > + interrupt-parent = <&mpic>; > > + interrupts = <2 1 0 0>; > > + reg = <0x1>; > > + }; > > + > > + tbi0: tbi-phy@11 { > > + device_type = "tbi-phy"; > > + reg = <0x11>; > > + }; > > + }; > > + > > + mdio@25000 { > > + tbi1: tbi-phy@11 { > > + reg = <0x11>; > > + device_type = "tbi-phy"; > > + }; > > + }; > > + > > + enet0: ethernet@b0000 { > > + phy-connection-type = "rgmii-id"; > > + fixed-link { > > + speed = <1000>; > > + full-duplex; > > + }; > > + > > + }; > > + > > + enet1: ethernet@b1000 { > > + phy-handle = <&phy0>; > > + tbi-handle = <&tbi1>; > > + phy-connection-type = "sgmii"; > > + }; > > + > > + enet2: ethernet@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@ffe00000 { > > ranges = <0x0 0x0 0xffe00000 0x100000>; > > + > > + mdio@24000 { > > + phy0: ethernet-phy@0 { > > + interrupts = <3 1 0 0>; > > + reg = <0x0>; > > + }; > > + > > + phy1: ethernet-phy@1 { > > + interrupts = <2 1 0 0>; > > + reg = <0x1>; > > + }; > > + }; > > + > > + mdio@25000 { > > + tbi1: tbi-phy@11 { > > + reg = <0x11>; > > + device_type = "tbi-phy"; > > + }; > > + }; > > + > > + mdio@26000 { > > + tbi2: tbi-phy@11 { > > + reg = <0x11>; > > + device_type = "tbi-phy"; > > + }; > > + }; > > + > > + ptp_clock@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@b0000 { > > + phy-connection-type = "rgmii-id"; > > + fixed-link { > > + speed = <1000>; > > + full-duplex; > > + }; > > + }; > > + > > + enet1: ethernet@b1000 { > > + phy-handle = <&phy0>; > > + tbi-handle = <&tbi1>; > > + phy-connection-type = "sgmii"; > > + }; > > + > > + enet2: ethernet@b2000 { > > + phy-handle = <&phy1>; > > + phy-connection-type = "rgmii-id"; > > + }; > > }; > > > > pci1: pcie@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@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@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@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@25000 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + compatible = "fsl,etsec2-tbi"; > > + reg = <0x25000 0x1000 0xb1030 0x4>; }; > > + > > +ethernet@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@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@26000 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + compatible = "fsl,etsec2-tbi"; > > + reg = <0x26000 0x1000 0xb1030 0x4>; }; > > + > > +ethernet@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@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@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@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@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 > >
