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] [day] [month] [year] [list]
Message-ID: <CADRPPNS3j49UKkjodKky4cyodnyhWsUu8WahwZ9BwXShK+Nakg@mail.gmail.com>
Date:   Thu, 7 Feb 2019 17:24:33 -0600
From:   Li Yang <leoyang.li@....com>
To:     Pankaj Bansal <pankaj.bansal@....com>
Cc:     Andrew Lunn <andrew@...n.ch>, 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

On Wed, Feb 6, 2019 at 10:44 PM Pankaj Bansal <pankaj.bansal@....com> wrote:
>
>
>
> > -----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.

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