netdev - RE: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 7 Feb 2019 04:42:37 +0000
From:   Pankaj Bansal <pankaj.bansal@....com>
To:     Leo Li <leoyang.li@....com>, Andrew Lunn <andrew@...n.ch>
CC:     Shawn Guo <shawnguo@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Rob Herring <robh+dt@...nel.org>
Subject: RE: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes



> -----Original Message-----
> From: Li Yang [mailto:leoyang.li@....com]
> Sent: Thursday, 7 February, 2019 05:09 AM
> To: Andrew Lunn <andrew@...n.ch>
> Cc: Pankaj Bansal <pankaj.bansal@....com>; Shawn Guo
> <shawnguo@...nel.org>; Florian Fainelli <f.fainelli@...il.com>;
> netdev@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; Rob Herring
> <robh+dt@...nel.org>
> Subject: Re: [PATCH v2] arm64: dts: lx2160aqds: Add mdio mux nodes
> 
> On Wed, Feb 6, 2019 at 3:46 PM Andrew Lunn <andrew@...n.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