Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Pankaj Bansal <pankaj.bansal@nxp.com>
To: Leo Li <leoyang.li@nxp.com>, Rob Herring <robh+dt@kernel.org>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Shawn Guo <shawnguo@kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: RE: [PATCH v3] arm64: dts: lx2160aqds: Add mdio mux nodes
Date: Mon, 18 Feb 2019 05:49:18 +0000
Message-ID: <VI1PR0401MB2496C24647AB1DF2EC7BA266F1630@VI1PR0401MB2496.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <CADRPPNTua4AmYC6_AkDYKbyOXXzpR+7a1nLTe=_Qn-KhXck=8w@mail.gmail.com>



> -----Original Message-----
> From: Li Yang [mailto:leoyang.li@nxp.com]
> Sent: Friday, 15 February, 2019 04:03 AM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Rob Herring <robh+dt@kernel.org>
> 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 v3] arm64: dts: lx2160aqds: Add mdio mux nodes
> 
> On Tue, Feb 12, 2019 at 12:01 PM Li Yang <leoyang.li@nxp.com> wrote:
> >
> > On Mon, Feb 11, 2019 at 9:28 PM Pankaj Bansal <pankaj.bansal@nxp.com>
> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Leo Li
> > > > Sent: Tuesday, 12 February, 2019 02:14 AM
> > > > To: Shawn Guo <shawnguo@kernel.org>; Pankaj Bansal
> > > > <pankaj.bansal@nxp.com>
> > > > Cc: Andrew Lunn <andrew@lunn.ch>; Florian Fainelli
> > > > <f.fainelli@gmail.com>; netdev@vger.kernel.org;
> > > > linux-arm-kernel@lists.infradead.org
> > > > Subject: RE: [PATCH v3] arm64: dts: lx2160aqds: Add mdio mux nodes
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Shawn Guo <shawnguo@kernel.org>
> > > > > Sent: Sunday, February 10, 2019 9:00 PM
> > > > > To: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > > Cc: Leo Li <leoyang.li@nxp.com>; Andrew Lunn <andrew@lunn.ch>;
> > > > > Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org;
> > > > > linux-arm- kernel@lists.infradead.org
> > > > > Subject: Re: [PATCH v3] arm64: dts: lx2160aqds: Add mdio mux
> > > > > nodes
> > > > >
> > > > > On Wed, Feb 06, 2019 at 09:40:33AM +0000, Pankaj Bansal 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:
> > > > > >     V3:
> > > > > >     - Add status = disabled in soc file and status = okay in board file
> > > > > >       for external MDIO nodes
> > > > > >     - Add interrupts property in external mdio nodes in soc file
> > > > > >     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   | 123 +++++++++++++++++
> > > > > >  .../boot/dts/freescale/fsl-lx2160a.dtsi      |  22 +++
> > > > > >  2 files changed, 145 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..079264b391a2 100644
> > > > > > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts
> > > > > > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-qds.dts
> > > > > > @@ -35,6 +35,14 @@
> > > > > >   status = "okay";
> > > > > >  };
> > > > > >
> > > > > > +&emdio1 {
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&emdio2 {
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > >  &esdhc0 {
> > > > > >   status = "okay";
> > > > > >  };
> > > > > > @@ -46,6 +54,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>;
> > > > > > +                 };
> > > > >
> > > > > Please have a newline between nodes.  It doesn't deserve a
> > > > > respin though.  I can fix them up when applying if Leo is fine with this
> version.
> > > >
> > > > I think there should be a compatible string defined for the
> > > > binding of parent node mdio-mux, probably "mdio-mux-regmap", and
> > > > be used here in the device tree.
> > >
> > > I have two concerns :
> > > 1. The regmap is linux s/w construct, while device tree is h/w representation
> and is s/w agnostic. can we use regmap in device tree?
> >
> > Well, if we want to avoid using the regmap name, we probably can try
> > "mdio-mux-reg" or "mdio-mux-syscon"?  With further search I also found
> > a more generic mux binding at Documentation/devicetree/bindings/mux/,
> > would be even better if we can use that to describe the mux.
> 
> 
> To make it more clear, with the use of
> Documentation/devicetree/bindings/mux/ binding, I think it would end up with
> something like this:
> 
> i2c {
>  fpga {
>   compatible = "fsl,lx2160aqds-fpga", "syscon";
>   ....
>   mux: mux-controller {
>    compatible = "mmio-mux"

The FPGA in LX2160AQDS is not mmio controlled but an I2c controlled FPGA.
As such there is no binding yet for mux nodes controlled by SPI or I2C devices.
Even the syscon binding is for MMIO controlled devices.
I have added such binding at https://patchwork.ozlabs.org/patch/1043790/
Let's see if this gets accepted or not? After that I will resend this patch.

>    ...
>   }
>  }
> ...
> }
> 
> mdio-mux {
>  compatible = "mdio-mux"
>  mux-controls = <&mux 0>;
>  ....
>  mdio {
>   phy {
>   }
>   ...
>  }
>  mdio {
>   ...
>  }
>  ...
> }
> 
> 
> }"
> 
> >
> > > 2. By convention the device tree compatible binding is defined as
> "<manufacturer>,<model>" e.g. "fsl,mpc8349-uart". The mdio-mux node and it's
> sub nodes are a generic representation of mdio mux and it is not dependent on a
> particular manufacturer device. How to define the compatible in this case?
> >
> > The manufacturer prefix is for vendor specific bindings.  If the
> > binding a suppose to be generic, we don't need the vendor prefix.
> >
> > >
> > > >
> > > > >
> > > > > Shawn
> > > > >
> > > > > > +                 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..7def5252ac1a 100644
> > > > > > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> > > > > > @@ -762,5 +762,27 @@
> > > > > >                                <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>;
> > > > > > +                 interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > +                 #address-cells = <1>;
> > > > > > +                 #size-cells = <0>;
> > > > > > +                 little-endian;  /* force the driver in LE mode */
> > > > > > +                 status = "disabled";
> > > > > > +         };
> > > > > > +
> > > > > > +         /* WRIOP0: 0x8b8_0000, E-MDIO2: 0x1_7000 */
> > > > > > +         emdio2: mdio@8b97000 {
> > > > > > +                 compatible = "fsl,fman-memac-mdio";
> > > > > > +                 reg = <0x0 0x8b97000 0x0 0x1000>;
> > > > > > +                 interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > +                 #address-cells = <1>;
> > > > > > +                 #size-cells = <0>;
> > > > > > +                 little-endian;  /* force the driver in LE mode */
> > > > > > +                 status = "disabled";
> > > > > > +         };
> > > > > >   };
> > > > > >  };
> > > > > > --
> > > > > > 2.17.1
> > > > > >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

      reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-06  9:40 Pankaj Bansal
2019-02-11  3:00 ` Shawn Guo
2019-02-11 20:43   ` Leo Li
2019-02-12  3:26     ` Pankaj Bansal
2019-02-12 18:01       ` Li Yang
2019-02-14 22:32         ` Li Yang
2019-02-18  5:49           ` Pankaj Bansal [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR0401MB2496C24647AB1DF2EC7BA266F1630@VI1PR0401MB2496.eurprd04.prod.outlook.com \
    --to=pankaj.bansal@nxp.com \
    --cc=andrew@lunn.ch \
    --cc=f.fainelli@gmail.com \
    --cc=leoyang.li@nxp.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=netdev@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox