| Message ID | 20190204141641.18272-1-pankaj.bansal@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: dts: lx2160aqds: Add mdio mux nodes | expand |
From: Pankaj Bansal <pankaj.bansal@nxp.com> Date: Mon, 4 Feb 2019 08:51:57 +0000 > The two external MDIO buses used to communicate with phy devices that are > external to SOC are muxed in LX2160AQDS board. > > These buses can be routed to any one of the eight IO slots on LX2160AQDS > board depending on value in fpga register 0x54. > > Additionally the external MDIO1 is used to communicate to the onboard > RGMII phy devices. > > The mdio1 is controlled by bits 4-7 of fpga register and mdio2 is > controlled by bits 0-3 of fpga register. > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com> Am I applying this to my networking tree or are the ARM folks taking this?
Hi Shawn/Leo, If you have no more comments, can you please merge this path in your branch? in same branch in which you have accepted LX2160AQDS board patches. Regards, Pankaj Bansal > -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Monday, 4 February, 2019 10:21 PM > To: Pankaj Bansal <pankaj.bansal@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; andrew@lunn.ch; > f.fainelli@gmail.com; netdev@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes > > From: Pankaj Bansal <pankaj.bansal@nxp.com> > Date: Mon, 4 Feb 2019 08:51:57 +0000 > > > The two external MDIO buses used to communicate with phy devices that > > are external to SOC are muxed in LX2160AQDS board. > > > > These buses can be routed to any one of the eight IO slots on > > LX2160AQDS board depending on value in fpga register 0x54. > > > > Additionally the external MDIO1 is used to communicate to the onboard > > RGMII phy devices. > > > > The mdio1 is controlled by bits 4-7 of fpga register and mdio2 is > > controlled by bits 0-3 of fpga register. > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com> > > Am I applying this to my networking tree or are the ARM folks taking this?
On Mon, Feb 4, 2019 at 2:53 AM Pankaj Bansal <pankaj.bansal@nxp.com> wrote: > > The two external MDIO buses used to communicate with phy devices that are > external to SOC are muxed in LX2160AQDS board. > > These buses can be routed to any one of the eight IO slots on LX2160AQDS > board depending on value in fpga register 0x54. > > Additionally the external MDIO1 is used to communicate to the onboard > RGMII phy devices. > > The mdio1 is controlled by bits 4-7 of fpga register and mdio2 is > controlled by bits 0-3 of fpga register. > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com> > --- > > Notes: > V2: > - removed unnecassary TODO statements > - removed device_type from mdio nodes > - change the case of hex number to lowercase > - removed board specific comments from soc file There are still some comments from V1 haven't been addressed. > > .../boot/dts/freescale/fsl-lx2160a-qds.dts | 115 +++++++++++++++++ > .../boot/dts/freescale/fsl-lx2160a.dtsi | 18 +++ > 2 files changed, 133 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > index 99a22abbe725..2c3020a72d41 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > @@ -46,6 +46,121 @@ > &i2c0 { > status = "okay"; > > + fpga@66 { > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c"; > + reg = <0x66>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + mdio-mux-1@54 { Still no compatible string defined for the node. Probably should be "mdio-mux-mmioreg", "mdio-mux" > + mdio-parent-bus = <&emdio1>; > + reg = <0x54>; /* BRDCFG4 */ > + mux-mask = <0xf8>; /* EMI1_MDIO */ > + #address-cells=<1>; > + #size-cells = <0>; > + > + mdio@0 { > + reg = <0x00>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@40 { > + reg = <0x40>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@c0 { > + reg = <0xc0>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@c8 { > + reg = <0xc8>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@d0 { > + reg = <0xd0>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@d8 { > + reg = <0xd8>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@e0 { > + reg = <0xe0>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@e8 { > + reg = <0xe8>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@f0 { > + reg = <0xf0>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@f8 { > + reg = <0xf8>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + }; > + > + mdio-mux-2@54 { > + mdio-parent-bus = <&emdio2>; > + reg = <0x54>; /* BRDCFG4 */ > + mux-mask = <0x07>; /* EMI2_MDIO */ > + #address-cells=<1>; > + #size-cells = <0>; > + > + mdio@0 { > + reg = <0x00>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@1 { > + reg = <0x01>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@2 { > + reg = <0x02>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@3 { > + reg = <0x03>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@4 { > + reg = <0x04>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@5 { > + reg = <0x05>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@6 { > + reg = <0x06>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + mdio@7 { > + reg = <0x07>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + }; > + }; > + > i2c-mux@77 { > compatible = "nxp,pca9547"; > reg = <0x77>; > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > index a79f5c1ea56d..a74045ad22ad 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > @@ -762,5 +762,23 @@ > <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>; > dma-coherent; > }; > + > + /* WRIOP0: 0x8b8_0000, E-MDIO1: 0x1_6000 */ > + emdio1: mdio@8b96000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8b96000 0x0 0x1000>; > + #address-cells = <1>; > + #size-cells = <0>; > + little-endian; /* force the driver in LE mode */ Still doesn't fully align with the binding at "Documentation/devicetree/bindings/powerpc/fsl/fman.txt". It should either have the "interrupts" property for external MDIO or "fsl,fman-internal-mdio" for internal MDIO. > + }; > + > + /* WRIOP0: 0x8b8_0000, E-MDIO2: 0x1_7000 */ > + emdio2: mdio@8b97000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8b97000 0x0 0x1000>; > + #address-cells = <1>; > + #size-cells = <0>; > + little-endian; /* force the driver in LE mode */ > + }; > }; > }; > -- > 2.17.1 >
On Tue, Feb 5, 2019 at 6:26 AM Pankaj Bansal <pankaj.bansal@nxp.com> wrote: > > Hi Shawn/Leo, > > If you have no more comments, can you please merge this path in your branch? > in same branch in which you have accepted LX2160AQDS board patches. It can be merged through the ARM SoC tree. But there are still comments remain to be addressed. > > Regards, > Pankaj Bansal > > > -----Original Message----- > > From: David Miller [mailto:davem@davemloft.net] > > Sent: Monday, 4 February, 2019 10:21 PM > > To: Pankaj Bansal <pankaj.bansal@nxp.com> > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; andrew@lunn.ch; > > f.fainelli@gmail.com; netdev@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org > > Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes > > > > From: Pankaj Bansal <pankaj.bansal@nxp.com> > > Date: Mon, 4 Feb 2019 08:51:57 +0000 > > > > > The two external MDIO buses used to communicate with phy devices that > > > are external to SOC are muxed in LX2160AQDS board. > > > > > > These buses can be routed to any one of the eight IO slots on > > > LX2160AQDS board depending on value in fpga register 0x54. > > > > > > Additionally the external MDIO1 is used to communicate to the onboard > > > RGMII phy devices. > > > > > > The mdio1 is controlled by bits 4-7 of fpga register and mdio2 is > > > controlled by bits 0-3 of fpga register. > > > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com> > > > > Am I applying this to my networking tree or are the ARM folks taking this?
> -----Original Message----- > From: Li Yang [mailto:leoyang.li@nxp.com] > Sent: Wednesday, 6 February, 2019 12:07 AM > To: Pankaj Bansal <pankaj.bansal@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Andrew Lunn <andrew@lunn.ch>; > Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes > > On Mon, Feb 4, 2019 at 2:53 AM Pankaj Bansal <pankaj.bansal@nxp.com> > wrote: > > > > The two external MDIO buses used to communicate with phy devices that > > are external to SOC are muxed in LX2160AQDS board. > > > > These buses can be routed to any one of the eight IO slots on > > LX2160AQDS board depending on value in fpga register 0x54. > > > > Additionally the external MDIO1 is used to communicate to the onboard > > RGMII phy devices. > > > > The mdio1 is controlled by bits 4-7 of fpga register and mdio2 is > > controlled by bits 0-3 of fpga register. > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com> > > --- > > > > Notes: > > V2: > > - removed unnecassary TODO statements > > - removed device_type from mdio nodes > > - change the case of hex number to lowercase > > - removed board specific comments from soc file > > There are still some comments from V1 haven't been addressed. > > > > > .../boot/dts/freescale/fsl-lx2160a-qds.dts | 115 +++++++++++++++++ > > .../boot/dts/freescale/fsl-lx2160a.dtsi | 18 +++ > > 2 files changed, 133 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > > b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > > index 99a22abbe725..2c3020a72d41 100644 > > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > > @@ -46,6 +46,121 @@ > > &i2c0 { > > status = "okay"; > > > > + fpga@66 { > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c"; > > + reg = <0x66>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + mdio-mux-1@54 { > > Still no compatible string defined for the node. Probably should be "mdio-mux- > mmioreg", "mdio-mux" it is not a specific device. MDIO mux is meant to be controlled by some registers of parent device (FPGA). Therefore, IMO this should not be a device and there should not be any "compatible" property for it. > > > + mdio-parent-bus = <&emdio1>; > > + reg = <0x54>; /* BRDCFG4 */ > > + mux-mask = <0xf8>; /* EMI1_MDIO */ > > + #address-cells=<1>; > > + #size-cells = <0>; > > + > > + mdio@0 { > > + reg = <0x00>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@40 { > > + reg = <0x40>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@c0 { > > + reg = <0xc0>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@c8 { > > + reg = <0xc8>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@d0 { > > + reg = <0xd0>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@d8 { > > + reg = <0xd8>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@e0 { > > + reg = <0xe0>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@e8 { > > + reg = <0xe8>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@f0 { > > + reg = <0xf0>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@f8 { > > + reg = <0xf8>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + }; > > + > > + mdio-mux-2@54 { > > + mdio-parent-bus = <&emdio2>; > > + reg = <0x54>; /* BRDCFG4 */ > > + mux-mask = <0x07>; /* EMI2_MDIO */ > > + #address-cells=<1>; > > + #size-cells = <0>; > > + > > + mdio@0 { > > + reg = <0x00>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@1 { > > + reg = <0x01>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@2 { > > + reg = <0x02>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@3 { > > + reg = <0x03>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@4 { > > + reg = <0x04>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@5 { > > + reg = <0x05>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@6 { > > + reg = <0x06>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + mdio@7 { > > + reg = <0x07>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + }; > > + }; > > + > > i2c-mux@77 { > > compatible = "nxp,pca9547"; > > reg = <0x77>; > > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > > b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > > index a79f5c1ea56d..a74045ad22ad 100644 > > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > > @@ -762,5 +762,23 @@ > > <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>; > > dma-coherent; > > }; > > + > > + /* WRIOP0: 0x8b8_0000, E-MDIO1: 0x1_6000 */ > > + emdio1: mdio@8b96000 { > > + compatible = "fsl,fman-memac-mdio"; > > + reg = <0x0 0x8b96000 0x0 0x1000>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + little-endian; /* force the driver in LE mode > > + */ > > Still doesn't fully align with the binding at > "Documentation/devicetree/bindings/powerpc/fsl/fman.txt". > > It should either have the "interrupts" property for external MDIO or "fsl,fman- > internal-mdio" for internal MDIO. I see that for DPAA1 based SOCs, there was definitely an interrupt property in external MDIO node. BUT, for DPAA2 based devices none of the SOC has this property. I am looking further into this. > > > + }; > > + > > + /* WRIOP0: 0x8b8_0000, E-MDIO2: 0x1_7000 */ > > + emdio2: mdio@8b97000 { > > + compatible = "fsl,fman-memac-mdio"; > > + reg = <0x0 0x8b97000 0x0 0x1000>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + little-endian; /* force the driver in LE mode */ > > + }; > > }; > > }; > > -- > > 2.17.1 > >
> -----Original Message----- > From: Pankaj Bansal > Sent: Tuesday, February 5, 2019 10:02 PM > To: Leo Li <leoyang.li@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Andrew Lunn <andrew@lunn.ch>; > Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: RE: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes > > > > > -----Original Message----- > > From: Li Yang [mailto:leoyang.li@nxp.com] > > Sent: Wednesday, 6 February, 2019 12:07 AM > > To: Pankaj Bansal <pankaj.bansal@nxp.com> > > Cc: Shawn Guo <shawnguo@kernel.org>; Andrew Lunn > <andrew@lunn.ch>; > > Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org; > > linux-arm- kernel@lists.infradead.org > > Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes > > > > On Mon, Feb 4, 2019 at 2:53 AM Pankaj Bansal <pankaj.bansal@nxp.com> > > wrote: > > > > > > The two external MDIO buses used to communicate with phy devices > > > that are external to SOC are muxed in LX2160AQDS board. > > > > > > These buses can be routed to any one of the eight IO slots on > > > LX2160AQDS board depending on value in fpga register 0x54. > > > > > > Additionally the external MDIO1 is used to communicate to the > > > onboard RGMII phy devices. > > > > > > The mdio1 is controlled by bits 4-7 of fpga register and mdio2 is > > > controlled by bits 0-3 of fpga register. > > > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com> > > > --- > > > > > > Notes: > > > V2: > > > - removed unnecassary TODO statements > > > - removed device_type from mdio nodes > > > - change the case of hex number to lowercase > > > - removed board specific comments from soc file > > > > There are still some comments from V1 haven't been addressed. > > > > > > > > .../boot/dts/freescale/fsl-lx2160a-qds.dts | 115 +++++++++++++++++ > > > .../boot/dts/freescale/fsl-lx2160a.dtsi | 18 +++ > > > 2 files changed, 133 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > > > b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > > > index 99a22abbe725..2c3020a72d41 100644 > > > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > > > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts > > > @@ -46,6 +46,121 @@ > > > &i2c0 { > > > status = "okay"; > > > > > > + fpga@66 { > > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c"; > > > + reg = <0x66>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + mdio-mux-1@54 { > > > > Still no compatible string defined for the node. Probably should be > > "mdio-mux- mmioreg", "mdio-mux" > > it is not a specific device. MDIO mux is meant to be controlled by some > registers of parent device (FPGA). > Therefore, IMO this should not be a device and there should not be any > "compatible" property for it. If it is not a device why we are defining a device node for it? It is probably not a physical device per se, but it can be considered a virtual device provided by FPGA. This also bring up another question that why this device cannot reuse the existing drivers/net/phy/mdio-mux-mmioreg.c driver? If we think regmap is a better solution, shall we replace the mmioreg driver with the regmap driver? > > > > > > + mdio-parent-bus = <&emdio1>; > > > + reg = <0x54>; /* BRDCFG4 */ > > > + mux-mask = <0xf8>; /* EMI1_MDIO */ > > > + #address-cells=<1>; > > > + #size-cells = <0>; > > > + > > > + mdio@0 { > > > + reg = <0x00>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@40 { > > > + reg = <0x40>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@c0 { > > > + reg = <0xc0>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@c8 { > > > + reg = <0xc8>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@d0 { > > > + reg = <0xd0>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@d8 { > > > + reg = <0xd8>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@e0 { > > > + reg = <0xe0>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@e8 { > > > + reg = <0xe8>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@f0 { > > > + reg = <0xf0>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@f8 { > > > + reg = <0xf8>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + }; > > > + > > > + mdio-mux-2@54 { > > > + mdio-parent-bus = <&emdio2>; > > > + reg = <0x54>; /* BRDCFG4 */ > > > + mux-mask = <0x07>; /* EMI2_MDIO */ > > > + #address-cells=<1>; > > > + #size-cells = <0>; > > > + > > > + mdio@0 { > > > + reg = <0x00>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@1 { > > > + reg = <0x01>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@2 { > > > + reg = <0x02>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@3 { > > > + reg = <0x03>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@4 { > > > + reg = <0x04>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@5 { > > > + reg = <0x05>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@6 { > > > + reg = <0x06>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + mdio@7 { > > > + reg = <0x07>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + }; > > > + }; > > > + }; > > > + > > > i2c-mux@77 { > > > compatible = "nxp,pca9547"; > > > reg = <0x77>; > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > > > b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > > > index a79f5c1ea56d..a74045ad22ad 100644 > > > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > > > @@ -762,5 +762,23 @@ > > > <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>; > > > dma-coherent; > > > }; > > > + > > > + /* WRIOP0: 0x8b8_0000, E-MDIO1: 0x1_6000 */ > > > + emdio1: mdio@8b96000 { > > > + compatible = "fsl,fman-memac-mdio"; > > > + reg = <0x0 0x8b96000 0x0 0x1000>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + little-endian; /* force the driver in LE > > > + mode */ > > > > Still doesn't fully align with the binding at > > "Documentation/devicetree/bindings/powerpc/fsl/fman.txt". > > > > It should either have the "interrupts" property for external MDIO or > > "fsl,fman- internal-mdio" for internal MDIO. > > I see that for DPAA1 based SOCs, there was definitely an interrupt property > in external MDIO node. > BUT, for DPAA2 based devices none of the SOC has this property. I am > looking further into this. > > > > > > + }; > > > + > > > + /* WRIOP0: 0x8b8_0000, E-MDIO2: 0x1_7000 */ > > > + emdio2: mdio@8b97000 { > > > + compatible = "fsl,fman-memac-mdio"; > > > + reg = <0x0 0x8b97000 0x0 0x1000>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + little-endian; /* force the driver in LE mode */ > > > + }; > > > }; > > > }; > > > -- > > > 2.17.1 > > >
> > > > &i2c0 { > > > > status = "okay"; > > > > > > > > + fpga@66 { > > > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c"; > > > > + reg = <0x66>; > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + mdio-mux-1@54 { > > > > > > Still no compatible string defined for the node. Probably should be > > > "mdio-mux- mmioreg", "mdio-mux" > > > > it is not a specific device. MDIO mux is meant to be controlled by some > > registers of parent device (FPGA). > > Therefore, IMO this should not be a device and there should not be any > > "compatible" property for it. > If it is not a device why we are defining a device node for it? It > is probably not a physical device per se, but it can be considered a > virtual device provided by FPGA. It is a physical device. But it happens to be embedded inside another device. And that embedded is not performed as a bus with devices on it, so the device tree concepts don't fit directly. > This also bring up another question that why this device cannot > reuse the existing drivers/net/phy/mdio-mux-mmioreg.c driver? Because it is on an i2c bus, not an mmio bus. > If we think regmap is a better solution, shall we replace the > mmioreg driver with the regmap driver? regmap can be used with mmio. But for a single MMIO register it is a huge framework. So it makes sense to keep mdio-mux-mmioreg simple. If however the device is already using regmap, adding one more register is very little overhead. And it might be possible to use this new mux with an mmio regmap, or an spi regmap, etc. So we seem to be covering the best of both worlds. Andrew
On Wed, Feb 6, 2019 at 3:46 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > &i2c0 { > > > > > status = "okay"; > > > > > > > > > > + fpga@66 { > > > > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c"; > > > > > + reg = <0x66>; > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + mdio-mux-1@54 { > > > > > > > > Still no compatible string defined for the node. Probably should be > > > > "mdio-mux- mmioreg", "mdio-mux" > > > > > > it is not a specific device. MDIO mux is meant to be controlled by some > > > registers of parent device (FPGA). > > > Therefore, IMO this should not be a device and there should not be any > > > "compatible" property for it. > > > If it is not a device why we are defining a device node for it? It > > is probably not a physical device per se, but it can be considered a > > virtual device provided by FPGA. > > It is a physical device. But it happens to be embedded inside another > device. And that embedded is not performed as a bus with devices on > it, so the device tree concepts don't fit directly. Whether or not it is populated as a bus(which probably should as the FPGA does contain many different functions and these functions like the mdio-mux we are discussing about could have separate drivers), the node should have a new binding documentation similar to the mdio_mux_mmioreg binding or even covers the mmioreg too. And the best way to match the node with the binding is through compatible strings IMO. This is why I'm asking the node to have a compatible string. > > > This also bring up another question that why this device cannot > > reuse the existing drivers/net/phy/mdio-mux-mmioreg.c driver? > > Because it is on an i2c bus, not an mmio bus. Oops, I missed that. > > > If we think regmap is a better solution, shall we replace the > > mmioreg driver with the regmap driver? > > regmap can be used with mmio. But for a single MMIO register it is a > huge framework. So it makes sense to keep mdio-mux-mmioreg simple. > > If however the device is already using regmap, adding one more > register is very little overhead. And it might be possible to use this > new mux with an mmio regmap, or an spi regmap, etc. So we seem to be > covering the best of both worlds. Ya. It would be ideal if the new driver can cover the legacy mdio-mux-mmioreg case too. Regards, Leo
> -----Original Message----- > From: Li Yang [mailto:leoyang.li@nxp.com] > Sent: Thursday, 7 February, 2019 05:09 AM > To: Andrew Lunn <andrew@lunn.ch> > Cc: Pankaj Bansal <pankaj.bansal@nxp.com>; Shawn Guo > <shawnguo@kernel.org>; Florian Fainelli <f.fainelli@gmail.com>; > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Rob Herring > <robh+dt@kernel.org> > Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes > > On Wed, Feb 6, 2019 at 3:46 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > &i2c0 { > > > > > > status = "okay"; > > > > > > > > > > > > + fpga@66 { > > > > > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c"; > > > > > > + reg = <0x66>; > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + mdio-mux-1@54 { > > > > > > > > > > Still no compatible string defined for the node. Probably > > > > > should be > > > > > "mdio-mux- mmioreg", "mdio-mux" > > > > > > > > it is not a specific device. MDIO mux is meant to be controlled by > > > > some registers of parent device (FPGA). > > > > Therefore, IMO this should not be a device and there should not be > > > > any "compatible" property for it. > > > > > If it is not a device why we are defining a device node for it? It > > > is probably not a physical device per se, but it can be considered a > > > virtual device provided by FPGA. > > > > It is a physical device. But it happens to be embedded inside another > > device. And that embedded is not performed as a bus with devices on > > it, so the device tree concepts don't fit directly. > > Whether or not it is populated as a bus(which probably should as the FPGA does > contain many different functions and these functions like the mdio-mux we are > discussing about could have separate drivers), the node should have a new > binding documentation similar to the mdio_mux_mmioreg binding or even > covers the mmioreg too. And the best way to match the node with the binding > is through compatible strings IMO. This is why I'm asking the node to have a > compatible string. The mdio_mux is NOT a device. FPGA is a device that provides the mdio mux functionality (among other functions). The mdio mux is controlled via some bits In one of the FPGA register. In my previous approach, I also used a compatible field for mdio_mux node in FPGA. https://www.mail-archive.com/netdev@vger.kernel.org/msg252744.html The FPGA driver would create as many platform devices for each subnode, and those devices Would attach to mdio_mux_regmap driver based on compatible field. BUT, this platform device creation is the problem. Since mdio_mux is not an actual device, how can We create a platform device for it? Which is why I removed the compatible field from mdio_mux nodes. The FPGA driver detects the mdio_mux Using their name i.e. " mdio-mux-1@54". Like this: for_each_child_of_node(dev->of_node, child) { if (!of_node_name_prefix(child, "mdio-mux")) Refer : https://patchwork.kernel.org/patch/10797227/ > > > > > > This also bring up another question that why this device cannot > > > reuse the existing drivers/net/phy/mdio-mux-mmioreg.c driver? > > > > Because it is on an i2c bus, not an mmio bus. > > Oops, I missed that. > > > > > > If we think regmap is a better solution, shall we replace the > > > mmioreg driver with the regmap driver? > > > > regmap can be used with mmio. But for a single MMIO register it is a > > huge framework. So it makes sense to keep mdio-mux-mmioreg simple. > > > > If however the device is already using regmap, adding one more > > register is very little overhead. And it might be possible to use this > > new mux with an mmio regmap, or an spi regmap, etc. So we seem to be > > covering the best of both worlds. > > Ya. It would be ideal if the new driver can cover the legacy mdio-mux-mmioreg > case too. Refer : https://patchwork.kernel.org/patch/10797227/ The mdio_mux_regmap can be used by any FPGA be it i2c connected or MMIO based. > > Regards, > Leo
On Wed, Feb 6, 2019 at 10:44 PM Pankaj Bansal <pankaj.bansal@nxp.com> wrote: > > > > > -----Original Message----- > > From: Li Yang [mailto:leoyang.li@nxp.com] > > Sent: Thursday, 7 February, 2019 05:09 AM > > To: Andrew Lunn <andrew@lunn.ch> > > Cc: Pankaj Bansal <pankaj.bansal@nxp.com>; Shawn Guo > > <shawnguo@kernel.org>; Florian Fainelli <f.fainelli@gmail.com>; > > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Rob Herring > > <robh+dt@kernel.org> > > Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes > > > > On Wed, Feb 6, 2019 at 3:46 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > > > &i2c0 { > > > > > > > status = "okay"; > > > > > > > > > > > > > > + fpga@66 { > > > > > > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c"; > > > > > > > + reg = <0x66>; > > > > > > > + #address-cells = <1>; > > > > > > > + #size-cells = <0>; > > > > > > > + > > > > > > > + mdio-mux-1@54 { > > > > > > > > > > > > Still no compatible string defined for the node. Probably > > > > > > should be > > > > > > "mdio-mux- mmioreg", "mdio-mux" > > > > > > > > > > it is not a specific device. MDIO mux is meant to be controlled by > > > > > some registers of parent device (FPGA). > > > > > Therefore, IMO this should not be a device and there should not be > > > > > any "compatible" property for it. > > > > > > > If it is not a device why we are defining a device node for it? It > > > > is probably not a physical device per se, but it can be considered a > > > > virtual device provided by FPGA. > > > > > > It is a physical device. But it happens to be embedded inside another > > > device. And that embedded is not performed as a bus with devices on > > > it, so the device tree concepts don't fit directly. > > > > Whether or not it is populated as a bus(which probably should as the FPGA does > > contain many different functions and these functions like the mdio-mux we are > > discussing about could have separate drivers), the node should have a new > > binding documentation similar to the mdio_mux_mmioreg binding or even > > covers the mmioreg too. And the best way to match the node with the binding > > is through compatible strings IMO. This is why I'm asking the node to have a > > compatible string. > > The mdio_mux is NOT a device. FPGA is a device that provides the mdio mux functionality > (among other functions). The mdio mux is controlled via some bits In one of the FPGA register. With modern chips, it is likely to have multiple functions in a single physical device that are covered by multiple subsystems. We shouldn't limit the concept of device to only physical devices. > > In my previous approach, I also used a compatible field for mdio_mux node in FPGA. > https://www.mail-archive.com/netdev@vger.kernel.org/msg252744.html > The FPGA driver would create as many platform devices for each subnode, and those devices > Would attach to mdio_mux_regmap driver based on compatible field. > > BUT, this platform device creation is the problem. Since mdio_mux is not an actual device, how can > We create a platform device for it? Like I said platform device doesn't have to be a physically separate device. In this specific case, I think a multi-function device(MFD) will be the best fit for this FPGA device. The framework will help to create sub-devices and help to share the regmap to all the sub-function drivers. Please check existing MFD drivers for more details. > > Which is why I removed the compatible field from mdio_mux nodes. The FPGA driver detects the mdio_mux > Using their name i.e. " mdio-mux-1@54". Like this: > for_each_child_of_node(dev->of_node, child) { > if (!of_node_name_prefix(child, "mdio-mux")) > > Refer : https://patchwork.kernel.org/patch/10797227/ > > > > > > > > > > This also bring up another question that why this device cannot > > > > reuse the existing drivers/net/phy/mdio-mux-mmioreg.c driver? > > > > > > Because it is on an i2c bus, not an mmio bus. > > > > Oops, I missed that. > > > > > > > > > If we think regmap is a better solution, shall we replace the > > > > mmioreg driver with the regmap driver? > > > > > > regmap can be used with mmio. But for a single MMIO register it is a > > > huge framework. So it makes sense to keep mdio-mux-mmioreg simple. > > > > > > If however the device is already using regmap, adding one more > > > register is very little overhead. And it might be possible to use this > > > new mux with an mmio regmap, or an spi regmap, etc. So we seem to be > > > covering the best of both worlds. > > > > Ya. It would be ideal if the new driver can cover the legacy mdio-mux-mmioreg > > case too. > > Refer : https://patchwork.kernel.org/patch/10797227/ > The mdio_mux_regmap can be used by any FPGA be it i2c connected or MMIO based. > > > > > Regards, > > Leo
diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts index 99a22abbe725..2c3020a72d41 100644 --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts @@ -46,6 +46,121 @@ &i2c0 { status = "okay"; + fpga@66 { + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c"; + reg = <0x66>; + #address-cells = <1>; + #size-cells = <0>; + + mdio-mux-1@54 { + mdio-parent-bus = <&emdio1>; + reg = <0x54>; /* BRDCFG4 */ + mux-mask = <0xf8>; /* EMI1_MDIO */ + #address-cells=<1>; + #size-cells = <0>; + + mdio@0 { + reg = <0x00>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@40 { + reg = <0x40>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@c0 { + reg = <0xc0>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@c8 { + reg = <0xc8>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@d0 { + reg = <0xd0>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@d8 { + reg = <0xd8>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@e0 { + reg = <0xe0>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@e8 { + reg = <0xe8>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@f0 { + reg = <0xf0>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@f8 { + reg = <0xf8>; + #address-cells = <1>; + #size-cells = <0>; + }; + }; + + mdio-mux-2@54 { + mdio-parent-bus = <&emdio2>; + reg = <0x54>; /* BRDCFG4 */ + mux-mask = <0x07>; /* EMI2_MDIO */ + #address-cells=<1>; + #size-cells = <0>; + + mdio@0 { + reg = <0x00>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@1 { + reg = <0x01>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@2 { + reg = <0x02>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@3 { + reg = <0x03>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@4 { + reg = <0x04>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@5 { + reg = <0x05>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@6 { + reg = <0x06>; + #address-cells = <1>; + #size-cells = <0>; + }; + mdio@7 { + reg = <0x07>; + #address-cells = <1>; + #size-cells = <0>; + }; + }; + }; + i2c-mux@77 { compatible = "nxp,pca9547"; reg = <0x77>; diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi index a79f5c1ea56d..a74045ad22ad 100644 --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi @@ -762,5 +762,23 @@ <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>; dma-coherent; }; + + /* WRIOP0: 0x8b8_0000, E-MDIO1: 0x1_6000 */ + emdio1: mdio@8b96000 { + compatible = "fsl,fman-memac-mdio"; + reg = <0x0 0x8b96000 0x0 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + little-endian; /* force the driver in LE mode */ + }; + + /* WRIOP0: 0x8b8_0000, E-MDIO2: 0x1_7000 */ + emdio2: mdio@8b97000 { + compatible = "fsl,fman-memac-mdio"; + reg = <0x0 0x8b97000 0x0 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + little-endian; /* force the driver in LE mode */ + }; }; };
The two external MDIO buses used to communicate with phy devices that are external to SOC are muxed in LX2160AQDS board. These buses can be routed to any one of the eight IO slots on LX2160AQDS board depending on value in fpga register 0x54. Additionally the external MDIO1 is used to communicate to the onboard RGMII phy devices. The mdio1 is controlled by bits 4-7 of fpga register and mdio2 is controlled by bits 0-3 of fpga register. Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com> --- Notes: V2: - removed unnecassary TODO statements - removed device_type from mdio nodes - change the case of hex number to lowercase - removed board specific comments from soc file .../boot/dts/freescale/fsl-lx2160a-qds.dts | 115 +++++++++++++++++ .../boot/dts/freescale/fsl-lx2160a.dtsi | 18 +++ 2 files changed, 133 insertions(+)