[PATCH 0/2] Add fwnode helper functions to MDIO bus driver

[PATCH 0/2] Add fwnode helper functions to MDIO bus driver

[Vikas Singh]

This patch add helper functions to handle fwnodes on MDIO bus in case of
ACPI probing. These helper functions will be used in DPAA MAC driver.

The patches are in below logical order:
1. Add helper function to attach MAC with PHY
2. Associate device node with fixed PHY by extending "fixed_phy_status"

Vikas Singh (2):
  net: phy: Add fwnode helper functions
  net: phy: Associate device node with fixed PHY

 drivers/net/phy/fixed_phy.c |  2 ++
 drivers/net/phy/mdio_bus.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h        |  4 +++
 include/linux/phy_fixed.h   |  2 ++
 4 files changed, 74 insertions(+)

-- 
2.7.4

[PATCH 1/2] net: phy: Add fwnode helper functions

[Vikas Singh]

Add support of fwnode helper functions to MDIO bus driver.
1. fwnode_phy_find_device() to find phy_device associated to a fwnod
2. fwnode_phy_connect() to attach the mac to the phy

Signed-off-by: Vikas Singh <vikas.singh@puresoftware.com>
---
 drivers/net/phy/mdio_bus.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h       |  4 +++
 2 files changed, 70 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 46b3370..ab8fcea 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -40,6 +40,72 @@
 
 #include "mdio-boardinfo.h"
 
+/* Helper function for fwnode_phy_find_device */
+static int fwnode_phy_match(struct device *dev, const void *phy_fwnode)
+{
+	return dev->fwnode == phy_fwnode;
+}
+
+/**
+ * fwnode_phy_find_device - find the phy_device associated to fwnode
+ * @phy_fwnode: Pointer to the PHY's fwnode
+ *
+ * If successful, returns a pointer to the phy_device with the
+ * embedded struct device refcount incremented by one, NULL on
+ * failure.
+ */
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	struct device *d;
+	struct mdio_device *mdiodev;
+
+	if (!phy_fwnode)
+		return NULL;
+
+	d = bus_find_device(&mdio_bus_type, NULL, phy_fwnode, fwnode_phy_match);
+	if (d) {
+		mdiodev = to_mdio_device(d);
+		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+			return to_phy_device(d);
+		put_device(d);
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(fwnode_phy_find_device);
+
+/**
+ * fwnode_phy_connect - Connect to the phy described in the device tree
+ * @dev: pointer to net_device claiming the phy
+ * @phy_fwnode: Pointer to fwnode for the PHY
+ * @hndlr: Link state callback for the network device
+ * @flags: flags to pass to the PHY
+ * @iface: PHY data interface type
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure. The
+ * refcount must be dropped by calling phy_disconnect() or phy_detach().
+ */
+struct phy_device *fwnode_phy_connect(
+		struct net_device *dev, struct fwnode_handle *phy_fwnode,
+		void (*hndlr)(struct net_device *), u32 flags, u32 iface)
+{
+	struct phy_device *phy_dev;
+
+	phy_dev = fwnode_phy_find_device(phy_fwnode);
+	if (!phy_dev)
+		return NULL;
+
+	phy_dev->dev_flags = flags;
+
+	/* If in case we fail to attach to PHY,then phy_dev must be NULL */
+	if (phy_connect_direct(dev, phy_dev, hndlr, iface))
+		return NULL;
+
+	return phy_dev;
+}
+EXPORT_SYMBOL(fwnode_phy_connect);
+
 static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
 {
 	/* Deassert the optional reset signal */
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 898cbf0..501da6a 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -362,6 +362,10 @@ int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
 bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
+struct phy_device *fwnode_phy_connect(
+		struct net_device *dev, struct fwnode_handle *phy_fwnode,
+		void (*hndlr)(struct net_device *), u32 flags, u32 iface);
 
 /**
  * mdio_module_driver() - Helper macro for registering mdio drivers
-- 
2.7.4

Re: [PATCH 1/2] net: phy: Add fwnode helper functions

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2806 bytes --]

Hi Vikas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.8-rc7 next-20200731]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vikas-Singh/Add-fwnode-helper-functions-to-MDIO-bus-driver/20200728-201610
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 92ed301919932f777713b9172e525674157e983d
config: h8300-randconfig-s031-20200731 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-115-g5fc204f2-dirty
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   h8300-linux-ld: drivers/net/phy/mdio_bus.o: in function `fwnode_phy_connect':
>> drivers/net/phy/mdio_bus.c:102: undefined reference to `phy_connect_direct'

vim +102 drivers/net/phy/mdio_bus.c

    76	
    77	/**
    78	 * fwnode_phy_connect - Connect to the phy described in the device tree
    79	 * @dev: pointer to net_device claiming the phy
    80	 * @phy_fwnode: Pointer to fwnode for the PHY
    81	 * @hndlr: Link state callback for the network device
    82	 * @flags: flags to pass to the PHY
    83	 * @iface: PHY data interface type
    84	 *
    85	 * If successful, returns a pointer to the phy_device with the embedded
    86	 * struct device refcount incremented by one, or NULL on failure. The
    87	 * refcount must be dropped by calling phy_disconnect() or phy_detach().
    88	 */
    89	struct phy_device *fwnode_phy_connect(
    90			struct net_device *dev, struct fwnode_handle *phy_fwnode,
    91			void (*hndlr)(struct net_device *), u32 flags, u32 iface)
    92	{
    93		struct phy_device *phy_dev;
    94	
    95		phy_dev = fwnode_phy_find_device(phy_fwnode);
    96		if (!phy_dev)
    97			return NULL;
    98	
    99		phy_dev->dev_flags = flags;
   100	
   101		/* If in case we fail to attach to PHY,then phy_dev must be NULL */
 > 102		if (phy_connect_direct(dev, phy_dev, hndlr, iface))
   103			return NULL;
   104	
   105		return phy_dev;
   106	}
   107	EXPORT_SYMBOL(fwnode_phy_connect);
   108	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25209 bytes --]

[PATCH 2/2] net: phy: Associate device node with fixed PHY

[Vikas Singh]

This patch will extend "struct fixed_phy_status" by adding new
"struct device *dev" member entry in it.
This change will help to handle the fixed phy registration in
ACPI probe case for fwnodes.

Signed-off-by: Vikas Singh <vikas.singh@puresoftware.com>
---
 drivers/net/phy/fixed_phy.c | 2 ++
 include/linux/phy_fixed.h   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index c4641b1..011c033 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -267,6 +267,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
 		phy->duplex = status->duplex;
 		phy->pause = status->pause;
 		phy->asym_pause = status->asym_pause;
+		if (!np)
+			phy->mdio.dev.fwnode = status->dev->fwnode;
 	}
 
 	of_node_get(np);
diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index 52bc8e4..155fea6 100644
--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h
@@ -8,6 +8,8 @@ struct fixed_phy_status {
 	int duplex;
 	int pause;
 	int asym_pause;
+	/* Associated device node */
+	struct device *dev;
 };
 
 struct device_node;
-- 
2.7.4

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Andrew Lunn]

On Tue, Jul 28, 2020 at 05:43:20PM +0530, Vikas Singh wrote:
> This patch will extend "struct fixed_phy_status" by adding new
> "struct device *dev" member entry in it.
> This change will help to handle the fixed phy registration in
> ACPI probe case for fwnodes.

Hi Vikas

Please could you tell us more about your use cases. It seems that
using ACPI on ARM is limited to SBSA/SBSA system. It is not clear to
me why you would need a fixed-link PHY on such a system.

   Andrew

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Andrew Lunn]

On Wed, Jul 29, 2020 at 07:34:19PM +0530, Vikas Singh wrote:
> Hi Andrew,
> 
> I understand your concern But my use case is pretty simple and straightforward.
> As you are already aware of the fact that operating systems running on standard
> server hardware requires standard firmware interfaces to be present in order to
> boot
> and function correctly.
> Here SBBR describes these firmware requirements which covers UEFI & ACPI.
> Therefore I would like to provide dual boot support to such systems which means
> supporting ACPI along with existing DT.
> Now kernels "__fixed_phy_registe() " being a wonderful implementation catters
> almost every thing required for both ACPI & DT
> But fails to register fixed PHYs in case of ACPI because it has no clue of the
> fwnode (of_node in case of DT) to attach with a PHY device.

You failed to answer my question. Why do you need a fixed PHY? Please
could you point me at your DT files so i can at least understand your
hardware.

	Andrew

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Russell King - ARM Linux admin]

On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> Hi Andrew,
> 
> Please refer to the "fman" node under
> linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> I have two 10G ethernet interfaces out of which one is of fixed-link.

Please do not top post.

How does XGMII (which is a 10G only interface) work at 1G speed?  Is
what is in DT itself a hack because fixed-phy doesn't support 10G
modes?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Andrew Lunn]

On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM Linux admin wrote:
> On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > Hi Andrew,
> > 
> > Please refer to the "fman" node under
> > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > I have two 10G ethernet interfaces out of which one is of fixed-link.
> 
> Please do not top post.
> 
> How does XGMII (which is a 10G only interface) work at 1G speed?  Is
> what is in DT itself a hack because fixed-phy doesn't support 10G
> modes?

My gut feeling is there is some hack going on here, which is why i'm
being persistent at trying to understand what is actually going on
here.

So Vikas, as Russell pointed out, fixed-link is limited to 1G. It
seems odd you are running a 10G link at 1G. It is also unclear what
you have on the other end of that fixed link? Is it an SFP and you are
afraid of the work needed to get phylink working with ACPI? Is it an
Ethernet switch, and you are afraid of the work needed to get DSA
working with ACPI?

Looking at
https://www.nxp.com/docs/en/quick-reference-guide/LS1046AQRS.pdf

I see a XFI/2-5G SGMII port connected to a PHY, which i guess is

       ethernet@f0000 { /* 10GEC1 */
                phy-handle = <&aqr106_phy>;
                phy-connection-type = "xgmii";
        };

and
                aqr106_phy: ethernet-phy@0 {
                        compatible = "ethernet-phy-ieee802.3-c45";
                        interrupts = <0 131 4>;
                        reg = <0x0>;
                };

Which leaves an XFI interface connected to a retimer and then to an
SFP cage? Is this where you are using fixed-link?

	Andrew

RE: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Andrew Lunn
> Sent: 01 August 2020 18:11
> To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Cc: Vikas Singh <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> hkallweit1@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)
> <calvin.johnson@oss.nxp.com>; kuldip dwivedi
> <kuldip.dwivedi@puresoftware.com>; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; Vikas Singh <vikas.singh@nxp.com>
> Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> 
> On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM Linux admin
> wrote:
> > On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > > Hi Andrew,
> > >
> > > Please refer to the "fman" node under
> > > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > > I have two 10G ethernet interfaces out of which one is of fixed-link.
> >
> > Please do not top post.
> >
> > How does XGMII (which is a 10G only interface) work at 1G speed?  Is
> > what is in DT itself a hack because fixed-phy doesn't support 10G
> > modes?
> 
> My gut feeling is there is some hack going on here, which is why i'm
> being persistent at trying to understand what is actually going on
> here.

Hi Andrew,

That platform used 1G fixed link there since there was no support for
10G fixed link at the time. PHYlib could have tolerated 10G speed there
With a one-liner. I understand that PHYLink is working to describe this
Better, but it was not there at that time. Adding the dependency on
PHYLink was not desirable as most of the users for the DPAA 1 platforms
were targeting kernels before the PHYLink introduction (and last I've
looked, it's still under development, with unstable APIs so we'll
take a look at this later, when it settles).

> So Vikas, as Russell pointed out, fixed-link is limited to 1G. It
> seems odd you are running a 10G link at 1G. It is also unclear what
> you have on the other end of that fixed link? Is it an SFP and you are
> afraid of the work needed to get phylink working with ACPI? Is it an
> Ethernet switch, and you are afraid of the work needed to get DSA
> working with ACPI?
> 
> Looking at
> https://www.nxp.com/docs/en/quick-reference-guide/LS1046AQRS.pdf
> 
> I see a XFI/2-5G SGMII port connected to a PHY, which i guess is
> 
>        ethernet@f0000 { /* 10GEC1 */
>                 phy-handle = <&aqr106_phy>;
>                 phy-connection-type = "xgmii";
>         };
> 
> and
>                 aqr106_phy: ethernet-phy@0 {
>                         compatible = "ethernet-phy-ieee802.3-c45";
>                         interrupts = <0 131 4>;
>                         reg = <0x0>;
>                 };
> 
> Which leaves an XFI interface connected to a retimer and then to an
> SFP cage? Is this where you are using fixed-link?
> 
> 	Andrew

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Russell King - ARM Linux admin]

On Mon, Aug 03, 2020 at 08:33:19AM +0000, Madalin Bucur (OSS) wrote:
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > Behalf Of Andrew Lunn
> > Sent: 01 August 2020 18:11
> > To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > Cc: Vikas Singh <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> > hkallweit1@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)
> > <calvin.johnson@oss.nxp.com>; kuldip dwivedi
> > <kuldip.dwivedi@puresoftware.com>; Madalin Bucur (OSS)
> > <madalin.bucur@oss.nxp.com>; Vikas Singh <vikas.singh@nxp.com>
> > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> > 
> > On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM Linux admin
> > wrote:
> > > On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > > > Hi Andrew,
> > > >
> > > > Please refer to the "fman" node under
> > > > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > > > I have two 10G ethernet interfaces out of which one is of fixed-link.
> > >
> > > Please do not top post.
> > >
> > > How does XGMII (which is a 10G only interface) work at 1G speed?  Is
> > > what is in DT itself a hack because fixed-phy doesn't support 10G
> > > modes?
> > 
> > My gut feeling is there is some hack going on here, which is why i'm
> > being persistent at trying to understand what is actually going on
> > here.
> 
> Hi Andrew,
> 
> That platform used 1G fixed link there since there was no support for
> 10G fixed link at the time. PHYlib could have tolerated 10G speed there
> With a one-liner.

That statement is false.  It is not a "one liner".  fixed-phy exposes
the settings to userspace as a Clause 22 PHY register set, and the
Clause 22 register set does not support 10G.  So, a "one liner" would
just be yet another hack.  Adding Clause 45 PHY emulation support
would be a huge task.

> I understand that PHYLink is working to describe this
> Better, but it was not there at that time. Adding the dependency on
> PHYLink was not desirable as most of the users for the DPAA 1 platforms
> were targeting kernels before the PHYLink introduction (and last I've
> looked, it's still under development, with unstable APIs so we'll
> take a look at this later, when it settles).

I think you need to read Documentation/process/stable-api-nonsense.rst
particularly the section "Stable Kernel Source Interfaces".

phylink is going to be under development for quite some time to come
as requirements evolve.  For example, when support for QSFP interfaces
is eventually worked out, I suspect there will need to be some further
changes to the driver interface.  This is completely normal.

Now, as to the stability of the phylink API to drivers, it has in fact
been very stable - it has only changed over the course of this year to
support split PCS, a necessary step for DPAA2 and a few others.  It has
been around in mainline for two years, and has been around much longer
than that, and during that time it has been in mainline, the MAC facing
interface has not changed until recently.

So, I find your claim to be quite unreasonable.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Sent: 03 August 2020 12:07
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> <vikas.singh@nxp.com>
> Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> 
> On Mon, Aug 03, 2020 at 08:33:19AM +0000, Madalin Bucur (OSS) wrote:
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > > Behalf Of Andrew Lunn
> > > Sent: 01 August 2020 18:11
> > > To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > Cc: Vikas Singh <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> > > hkallweit1@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)
> > > <calvin.johnson@oss.nxp.com>; kuldip dwivedi
> > > <kuldip.dwivedi@puresoftware.com>; Madalin Bucur (OSS)
> > > <madalin.bucur@oss.nxp.com>; Vikas Singh <vikas.singh@nxp.com>
> > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed
> PHY
> > >
> > > On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM Linux
> admin
> > > wrote:
> > > > On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > > > > Hi Andrew,
> > > > >
> > > > > Please refer to the "fman" node under
> > > > > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > > > > I have two 10G ethernet interfaces out of which one is of fixed-
> link.
> > > >
> > > > Please do not top post.
> > > >
> > > > How does XGMII (which is a 10G only interface) work at 1G speed?  Is
> > > > what is in DT itself a hack because fixed-phy doesn't support 10G
> > > > modes?
> > >
> > > My gut feeling is there is some hack going on here, which is why i'm
> > > being persistent at trying to understand what is actually going on
> > > here.
> >
> > Hi Andrew,
> >
> > That platform used 1G fixed link there since there was no support for
> > 10G fixed link at the time. PHYlib could have tolerated 10G speed there
> > With a one-liner.
> 
> That statement is false.  It is not a "one liner".  fixed-phy exposes
> the settings to userspace as a Clause 22 PHY register set, and the
> Clause 22 register set does not support 10G.  So, a "one liner" would
> just be yet another hack.  Adding Clause 45 PHY emulation support
> would be a huge task.
> 
> > I understand that PHYLink is working to describe this
> > Better, but it was not there at that time. Adding the dependency on
> > PHYLink was not desirable as most of the users for the DPAA 1 platforms
> > were targeting kernels before the PHYLink introduction (and last I've
> > looked, it's still under development, with unstable APIs so we'll
> > take a look at this later, when it settles).
> 
> I think you need to read Documentation/process/stable-api-nonsense.rst
> particularly the section "Stable Kernel Source Interfaces".
> 
> phylink is going to be under development for quite some time to come
> as requirements evolve.  For example, when support for QSFP interfaces
> is eventually worked out, I suspect there will need to be some further
> changes to the driver interface.  This is completely normal.
> 
> Now, as to the stability of the phylink API to drivers, it has in fact
> been very stable - it has only changed over the course of this year to
> support split PCS, a necessary step for DPAA2 and a few others.  It has
> been around in mainline for two years, and has been around much longer
> than that, and during that time it has been in mainline, the MAC facing
> interface has not changed until recently.
> 
> So, I find your claim to be quite unreasonable.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

I see you agree that there were and there will be many changes for a while,
It's not a complaint, I know hot it works, it's just a decision based on
required effort vs features offered vs user requirements. Lately it's been
time consuming to try to fix things in this area.

Regards
Madalin

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Andrew Lunn]

> I see you agree that there were and there will be many changes for a while,
> It's not a complaint, I know hot it works, it's just a decision based on
> required effort vs features offered vs user requirements. Lately it's been
> time consuming to try to fix things in this area.

So the conclusion to all this that you are unwilling to use the
correct API for this, which would be phylink, and the SFP code.  So:

NACK

	Andrew

RE: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 03 August 2020 15:58
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>; Vikas Singh
> <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> <vikas.singh@nxp.com>
> Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> 
> > I see you agree that there were and there will be many changes for a
> while,
> > It's not a complaint, I know hot it works, it's just a decision based on
> > required effort vs features offered vs user requirements. Lately it's
> been
> > time consuming to try to fix things in this area.
> 
> So the conclusion to all this that you are unwilling to use the
> correct API for this, which would be phylink, and the SFP code.  So:
> 
> NACK
> 
> 	Andrew

You've rejected a generic change - ACPI support for fixed link.
The discussion above is just an example of how it could have been (mis-)used.
Are you rejecting the general case or just the particular one?

Madalin

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Andrew Lunn]

On Mon, Aug 03, 2020 at 02:33:56PM +0000, Madalin Bucur (OSS) wrote:
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: 03 August 2020 15:58
> > To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> > Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>; Vikas Singh
> > <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> > netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> > kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> > <vikas.singh@nxp.com>
> > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> > 
> > > I see you agree that there were and there will be many changes for a
> > while,
> > > It's not a complaint, I know hot it works, it's just a decision based on
> > > required effort vs features offered vs user requirements. Lately it's
> > been
> > > time consuming to try to fix things in this area.
> > 
> > So the conclusion to all this that you are unwilling to use the
> > correct API for this, which would be phylink, and the SFP code.  So:
> > 
> > NACK
> > 
> > 	Andrew
> 
> You've rejected a generic change - ACPI support for fixed link.
> The discussion above is just an example of how it could have been (mis-)used.
> Are you rejecting the general case or just the particular one?

So far, nobody has corrected me that the MAC is not connected to an
SFP socket. So i see two sorts of abuse going on here:

1) Using a fixed link with a hack to allow 10G. phylink allows 10G
   fixed links without an hacks.

2) Using a fixed link when not even appropriate since phylink should
   be used to control the SFP.

Now, you can do whatever you want in your Vendor Crap tree. But there
is no reason mainline should help you support your vendor crap tree.

To make progress here, you need to add an in tree user of this generic
change. And since that means an ACPI user, you need to follow what has
been set out in the other thread. You need an ACPI maintainer to ACK
it. And to get an ACPI maintainer to ACK it, you need a specification,
and proof it is being used. And to get my ACK, it needs to be valid
use of it as well.

	Andrew

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Russell King - ARM Linux admin]

On Mon, Aug 03, 2020 at 05:00:51PM +0200, Andrew Lunn wrote:
> On Mon, Aug 03, 2020 at 02:33:56PM +0000, Madalin Bucur (OSS) wrote:
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: 03 August 2020 15:58
> > > To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> > > Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>; Vikas Singh
> > > <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> > > netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> > > kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> > > <vikas.singh@nxp.com>
> > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> > > 
> > > > I see you agree that there were and there will be many changes for a
> > > while,
> > > > It's not a complaint, I know hot it works, it's just a decision based on
> > > > required effort vs features offered vs user requirements. Lately it's
> > > been
> > > > time consuming to try to fix things in this area.
> > > 
> > > So the conclusion to all this that you are unwilling to use the
> > > correct API for this, which would be phylink, and the SFP code.  So:
> > > 
> > > NACK
> > > 
> > > 	Andrew
> > 
> > You've rejected a generic change - ACPI support for fixed link.
> > The discussion above is just an example of how it could have been (mis-)used.
> > Are you rejecting the general case or just the particular one?
> 
> So far, nobody has corrected me that the MAC is not connected to an
> SFP socket. So i see two sorts of abuse going on here:
> 
> 1) Using a fixed link with a hack to allow 10G. phylink allows 10G
>    fixed links without an hacks.
> 
> 2) Using a fixed link when not even appropriate since phylink should
>    be used to control the SFP.
> 
> Now, you can do whatever you want in your Vendor Crap tree. But there
> is no reason mainline should help you support your vendor crap tree.

+1 for everything above.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Russell King - ARM Linux admin]

On Mon, Aug 03, 2020 at 11:45:55AM +0000, Madalin Bucur (OSS) wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > Sent: 03 August 2020 12:07
> > To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> > Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> > <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> > netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> > kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> > <vikas.singh@nxp.com>
> > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> > 
> > On Mon, Aug 03, 2020 at 08:33:19AM +0000, Madalin Bucur (OSS) wrote:
> > > > -----Original Message-----
> > > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > > > Behalf Of Andrew Lunn
> > > > Sent: 01 August 2020 18:11
> > > > To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > > Cc: Vikas Singh <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> > > > hkallweit1@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)
> > > > <calvin.johnson@oss.nxp.com>; kuldip dwivedi
> > > > <kuldip.dwivedi@puresoftware.com>; Madalin Bucur (OSS)
> > > > <madalin.bucur@oss.nxp.com>; Vikas Singh <vikas.singh@nxp.com>
> > > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed
> > PHY
> > > >
> > > > On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM Linux
> > admin
> > > > wrote:
> > > > > On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > > > > > Hi Andrew,
> > > > > >
> > > > > > Please refer to the "fman" node under
> > > > > > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > > > > > I have two 10G ethernet interfaces out of which one is of fixed-
> > link.
> > > > >
> > > > > Please do not top post.
> > > > >
> > > > > How does XGMII (which is a 10G only interface) work at 1G speed?  Is
> > > > > what is in DT itself a hack because fixed-phy doesn't support 10G
> > > > > modes?
> > > >
> > > > My gut feeling is there is some hack going on here, which is why i'm
> > > > being persistent at trying to understand what is actually going on
> > > > here.
> > >
> > > Hi Andrew,
> > >
> > > That platform used 1G fixed link there since there was no support for
> > > 10G fixed link at the time. PHYlib could have tolerated 10G speed there
> > > With a one-liner.
> > 
> > That statement is false.  It is not a "one liner".  fixed-phy exposes
> > the settings to userspace as a Clause 22 PHY register set, and the
> > Clause 22 register set does not support 10G.  So, a "one liner" would
> > just be yet another hack.  Adding Clause 45 PHY emulation support
> > would be a huge task.
> > 
> > > I understand that PHYLink is working to describe this
> > > Better, but it was not there at that time. Adding the dependency on
> > > PHYLink was not desirable as most of the users for the DPAA 1 platforms
> > > were targeting kernels before the PHYLink introduction (and last I've
> > > looked, it's still under development, with unstable APIs so we'll
> > > take a look at this later, when it settles).
> > 
> > I think you need to read Documentation/process/stable-api-nonsense.rst
> > particularly the section "Stable Kernel Source Interfaces".
> > 
> > phylink is going to be under development for quite some time to come
> > as requirements evolve.  For example, when support for QSFP interfaces
> > is eventually worked out, I suspect there will need to be some further
> > changes to the driver interface.  This is completely normal.
> > 
> > Now, as to the stability of the phylink API to drivers, it has in fact
> > been very stable - it has only changed over the course of this year to
> > support split PCS, a necessary step for DPAA2 and a few others.  It has
> > been around in mainline for two years, and has been around much longer
> > than that, and during that time it has been in mainline, the MAC facing
> > interface has not changed until recently.
> > 
> > So, I find your claim to be quite unreasonable.
> 
> I see you agree that there were and there will be many changes for a while,
> It's not a complaint, I know hot it works, it's just a decision based on
> required effort vs features offered vs user requirements. Lately it's been
> time consuming to try to fix things in this area.

No, it hasn't been time consuming.  The only API changes as far as
drivers are concerned have been:

1. the change to the mac_link_up() prototype to move the setup of the
   final link parameters out of mac_config() - and almost all of the
   updates to users were done by me.

2. the addition of split PCS support, introducing new interfaces, has
   had minimal impact on those drivers that updated in step (1).

There have been no other changes as far as users are concerned.

Some of the difficulty with (1) has been that users of phylink appeared
initially with no proper review, and consequently they got quite a lot
wrong.  The most common error has been using state->speed, state->duplex
in mac_config() methods irrespective of the AN mode, which has _always_
since before phylink was merged into mainline, been totally unreliable.

That leads me on to the other visible "changes" for users are concerned,
which may be interpreted as interface changes, but are not; they have
all been clarifications to the documentation, to strengthen things such
as "do not use state->speed and state->duplex in mac_config() for
various specific AN modes".  Nothing has actually changed with any of
those clarifications.

For example, if in in-band mode, and mac_config() uses state->speed
and state->duplex, then it doesn't matter which version of phylink
you're using, if someone issues ethtool -s ethN ..., then state->speed
and state->duplex will be their respective UNKNOWN values, and if you're
using these in that situation, you will mis-program the MAC.

Again, that is not something that has changed.  Ever.  But the
documentation has because people just don't seem to get it, and I seemed
to be constantly repeating myself in review after review on the same
points.

So, your assertion that the phylink API is not stable is false.  It
has been remarkably stable over the two years that it has been around.
It is only natural that as the technology that a piece of code supports
evolves, so the code evolves with it.  That is exactly what has happened
this year with the two changes I mention above.

Now, if you've found it time consuming to "fix things" (unspecified what
"things" are) then I assert that what has needed to be fixed are things
that NXP have got wrong.  Such as the rtnl cockups.  Such as abusing
state->speed and state->duplex.  None of that is because the interface
is unstable - they are down to buggy implementation on NXPs part.

Essentially, what I'm saying is that your attempt to paint phylink as
being painful on the basis of interface changes is totally and utterly
wrong and is just an excuse to justify abusing the fixed-link code and
specifying things that are clearly incorrect via DT.

I will accept that the interface that had existed up until the
mac_link_up() change was confusing - it clearly was due to the number
of people getting mac_config() implementations wrong.  That is actually
another of the reasons why the mac_link_up() change was made.  These
problems are _only_ found by people making use of it.  If people don't
use stuff, then problems aren't found, and nothing changes.

So, I think you can expect a NAK for the patch at the top of this
thread on the basis that it is perpetuating an abuse not only the
legacy fixed-link code, but also DT.  However, I will leave Andrew to
make that call.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Sent: 03 August 2020 17:10
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> <vikas.singh@nxp.com>
> Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> 
> On Mon, Aug 03, 2020 at 11:45:55AM +0000, Madalin Bucur (OSS) wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > Sent: 03 August 2020 12:07
> > > To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> > > Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> > > <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> hkallweit1@gmail.com;
> > > netdev@vger.kernel.org; Calvin Johnson (OSS)
> <calvin.johnson@oss.nxp.com>;
> > > kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> > > <vikas.singh@nxp.com>
> > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed
> PHY
> > >
> > > On Mon, Aug 03, 2020 at 08:33:19AM +0000, Madalin Bucur (OSS) wrote:
> > > > > -----Original Message-----
> > > > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org>
> On
> > > > > Behalf Of Andrew Lunn
> > > > > Sent: 01 August 2020 18:11
> > > > > To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > > > Cc: Vikas Singh <vikas.singh@puresoftware.com>;
> f.fainelli@gmail.com;
> > > > > hkallweit1@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)
> > > > > <calvin.johnson@oss.nxp.com>; kuldip dwivedi
> > > > > <kuldip.dwivedi@puresoftware.com>; Madalin Bucur (OSS)
> > > > > <madalin.bucur@oss.nxp.com>; Vikas Singh <vikas.singh@nxp.com>
> > > > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with
> fixed
> > > PHY
> > > > >
> > > > > On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM Linux
> > > admin
> > > > > wrote:
> > > > > > On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > > > > > > Hi Andrew,
> > > > > > >
> > > > > > > Please refer to the "fman" node under
> > > > > > > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > > > > > > I have two 10G ethernet interfaces out of which one is of
> fixed-
> > > link.
> > > > > >
> > > > > > Please do not top post.
> > > > > >
> > > > > > How does XGMII (which is a 10G only interface) work at 1G speed?
> Is
> > > > > > what is in DT itself a hack because fixed-phy doesn't support
> 10G
> > > > > > modes?
> > > > >
> > > > > My gut feeling is there is some hack going on here, which is why
> i'm
> > > > > being persistent at trying to understand what is actually going on
> > > > > here.
> > > >
> > > > Hi Andrew,
> > > >
> > > > That platform used 1G fixed link there since there was no support
> for
> > > > 10G fixed link at the time. PHYlib could have tolerated 10G speed
> there
> > > > With a one-liner.
> > >
> > > That statement is false.  It is not a "one liner".  fixed-phy exposes
> > > the settings to userspace as a Clause 22 PHY register set, and the
> > > Clause 22 register set does not support 10G.  So, a "one liner" would
> > > just be yet another hack.  Adding Clause 45 PHY emulation support
> > > would be a huge task.
> > >
> > > > I understand that PHYLink is working to describe this
> > > > Better, but it was not there at that time. Adding the dependency on
> > > > PHYLink was not desirable as most of the users for the DPAA 1
> platforms
> > > > were targeting kernels before the PHYLink introduction (and last
> I've
> > > > looked, it's still under development, with unstable APIs so we'll
> > > > take a look at this later, when it settles).
> > >
> > > I think you need to read Documentation/process/stable-api-nonsense.rst
> > > particularly the section "Stable Kernel Source Interfaces".
> > >
> > > phylink is going to be under development for quite some time to come
> > > as requirements evolve.  For example, when support for QSFP interfaces
> > > is eventually worked out, I suspect there will need to be some further
> > > changes to the driver interface.  This is completely normal.
> > >
> > > Now, as to the stability of the phylink API to drivers, it has in fact
> > > been very stable - it has only changed over the course of this year to
> > > support split PCS, a necessary step for DPAA2 and a few others.  It
> has
> > > been around in mainline for two years, and has been around much longer
> > > than that, and during that time it has been in mainline, the MAC
> facing
> > > interface has not changed until recently.
> > >
> > > So, I find your claim to be quite unreasonable.
> >
> > I see you agree that there were and there will be many changes for a
> while,
> > It's not a complaint, I know hot it works, it's just a decision based on
> > required effort vs features offered vs user requirements. Lately it's
> been
> > time consuming to try to fix things in this area.
> 
> No, it hasn't been time consuming.  The only API changes as far as
> drivers are concerned have been:
> 
> 1. the change to the mac_link_up() prototype to move the setup of the
>    final link parameters out of mac_config() - and almost all of the
>    updates to users were done by me.
> 
> 2. the addition of split PCS support, introducing new interfaces, has
>    had minimal impact on those drivers that updated in step (1).
> 
> There have been no other changes as far as users are concerned.
> 
> Some of the difficulty with (1) has been that users of phylink appeared
> initially with no proper review, and consequently they got quite a lot
> wrong.  The most common error has been using state->speed, state->duplex
> in mac_config() methods irrespective of the AN mode, which has _always_
> since before phylink was merged into mainline, been totally unreliable.
> 
> That leads me on to the other visible "changes" for users are concerned,
> which may be interpreted as interface changes, but are not; they have
> all been clarifications to the documentation, to strengthen things such
> as "do not use state->speed and state->duplex in mac_config() for
> various specific AN modes".  Nothing has actually changed with any of
> those clarifications.
> 
> For example, if in in-band mode, and mac_config() uses state->speed
> and state->duplex, then it doesn't matter which version of phylink
> you're using, if someone issues ethtool -s ethN ..., then state->speed
> and state->duplex will be their respective UNKNOWN values, and if you're
> using these in that situation, you will mis-program the MAC.
> 
> Again, that is not something that has changed.  Ever.  But the
> documentation has because people just don't seem to get it, and I seemed
> to be constantly repeating myself in review after review on the same
> points.
> 
> So, your assertion that the phylink API is not stable is false.  It
> has been remarkably stable over the two years that it has been around.
> It is only natural that as the technology that a piece of code supports
> evolves, so the code evolves with it.  That is exactly what has happened
> this year with the two changes I mention above.
> 
> Now, if you've found it time consuming to "fix things" (unspecified what
> "things" are) then I assert that what has needed to be fixed are things
> that NXP have got wrong.  Such as the rtnl cockups.  Such as abusing
> state->speed and state->duplex.  None of that is because the interface
> is unstable - they are down to buggy implementation on NXPs part.
> 
> Essentially, what I'm saying is that your attempt to paint phylink as
> being painful on the basis of interface changes is totally and utterly
> wrong and is just an excuse to justify abusing the fixed-link code and
> specifying things that are clearly incorrect via DT.

Thank you for the distilled phylink history, it may be easier to comprehend
with these details. I was not referring to phylink, but PHY related issues
on the DPAA 1 platforms.

> I will accept that the interface that had existed up until the
> mac_link_up() change was confusing - it clearly was due to the number
> of people getting mac_config() implementations wrong.  That is actually
> another of the reasons why the mac_link_up() change was made.  These
> problems are _only_ found by people making use of it.  If people don't
> use stuff, then problems aren't found, and nothing changes.
> 
> So, I think you can expect a NAK for the patch at the top of this
> thread on the basis that it is perpetuating an abuse not only the
> legacy fixed-link code, but also DT.  However, I will leave Andrew to
> make that call.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Russell King - ARM Linux admin]

On Mon, Aug 03, 2020 at 02:47:41PM +0000, Madalin Bucur (OSS) wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > Sent: 03 August 2020 17:10
> > To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> > Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> > <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> > netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> > kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> > <vikas.singh@nxp.com>
> > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> > 
> > On Mon, Aug 03, 2020 at 11:45:55AM +0000, Madalin Bucur (OSS) wrote:
> > > > -----Original Message-----
> > > > From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > > Sent: 03 August 2020 12:07
> > > > To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> > > > Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> > > > <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> > hkallweit1@gmail.com;
> > > > netdev@vger.kernel.org; Calvin Johnson (OSS)
> > <calvin.johnson@oss.nxp.com>;
> > > > kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> > > > <vikas.singh@nxp.com>
> > > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed
> > PHY
> > > >
> > > > On Mon, Aug 03, 2020 at 08:33:19AM +0000, Madalin Bucur (OSS) wrote:
> > > > > > -----Original Message-----
> > > > > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org>
> > On
> > > > > > Behalf Of Andrew Lunn
> > > > > > Sent: 01 August 2020 18:11
> > > > > > To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > > > > Cc: Vikas Singh <vikas.singh@puresoftware.com>;
> > f.fainelli@gmail.com;
> > > > > > hkallweit1@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)
> > > > > > <calvin.johnson@oss.nxp.com>; kuldip dwivedi
> > > > > > <kuldip.dwivedi@puresoftware.com>; Madalin Bucur (OSS)
> > > > > > <madalin.bucur@oss.nxp.com>; Vikas Singh <vikas.singh@nxp.com>
> > > > > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with
> > fixed
> > > > PHY
> > > > > >
> > > > > > On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM Linux
> > > > admin
> > > > > > wrote:
> > > > > > > On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > > > > > > > Hi Andrew,
> > > > > > > >
> > > > > > > > Please refer to the "fman" node under
> > > > > > > > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > > > > > > > I have two 10G ethernet interfaces out of which one is of
> > fixed-
> > > > link.
> > > > > > >
> > > > > > > Please do not top post.
> > > > > > >
> > > > > > > How does XGMII (which is a 10G only interface) work at 1G speed?
> > Is
> > > > > > > what is in DT itself a hack because fixed-phy doesn't support
> > 10G
> > > > > > > modes?
> > > > > >
> > > > > > My gut feeling is there is some hack going on here, which is why
> > i'm
> > > > > > being persistent at trying to understand what is actually going on
> > > > > > here.
> > > > >
> > > > > Hi Andrew,
> > > > >
> > > > > That platform used 1G fixed link there since there was no support
> > for
> > > > > 10G fixed link at the time. PHYlib could have tolerated 10G speed
> > there
> > > > > With a one-liner.
> > > >
> > > > That statement is false.  It is not a "one liner".  fixed-phy exposes
> > > > the settings to userspace as a Clause 22 PHY register set, and the
> > > > Clause 22 register set does not support 10G.  So, a "one liner" would
> > > > just be yet another hack.  Adding Clause 45 PHY emulation support
> > > > would be a huge task.
> > > >
> > > > > I understand that PHYLink is working to describe this
> > > > > Better, but it was not there at that time. Adding the dependency on
> > > > > PHYLink was not desirable as most of the users for the DPAA 1
> > platforms
> > > > > were targeting kernels before the PHYLink introduction (and last
> > I've
> > > > > looked, it's still under development, with unstable APIs so we'll
> > > > > take a look at this later, when it settles).
> > > >
> > > > I think you need to read Documentation/process/stable-api-nonsense.rst
> > > > particularly the section "Stable Kernel Source Interfaces".
> > > >
> > > > phylink is going to be under development for quite some time to come
> > > > as requirements evolve.  For example, when support for QSFP interfaces
> > > > is eventually worked out, I suspect there will need to be some further
> > > > changes to the driver interface.  This is completely normal.
> > > >
> > > > Now, as to the stability of the phylink API to drivers, it has in fact
> > > > been very stable - it has only changed over the course of this year to
> > > > support split PCS, a necessary step for DPAA2 and a few others.  It
> > has
> > > > been around in mainline for two years, and has been around much longer
> > > > than that, and during that time it has been in mainline, the MAC
> > facing
> > > > interface has not changed until recently.
> > > >
> > > > So, I find your claim to be quite unreasonable.
> > >
> > > I see you agree that there were and there will be many changes for a
> > while,
> > > It's not a complaint, I know hot it works, it's just a decision based on
> > > required effort vs features offered vs user requirements. Lately it's
> > been
> > > time consuming to try to fix things in this area.
> > 
> > No, it hasn't been time consuming.  The only API changes as far as
> > drivers are concerned have been:
> > 
> > 1. the change to the mac_link_up() prototype to move the setup of the
> >    final link parameters out of mac_config() - and almost all of the
> >    updates to users were done by me.
> > 
> > 2. the addition of split PCS support, introducing new interfaces, has
> >    had minimal impact on those drivers that updated in step (1).
> > 
> > There have been no other changes as far as users are concerned.
> > 
> > Some of the difficulty with (1) has been that users of phylink appeared
> > initially with no proper review, and consequently they got quite a lot
> > wrong.  The most common error has been using state->speed, state->duplex
> > in mac_config() methods irrespective of the AN mode, which has _always_
> > since before phylink was merged into mainline, been totally unreliable.
> > 
> > That leads me on to the other visible "changes" for users are concerned,
> > which may be interpreted as interface changes, but are not; they have
> > all been clarifications to the documentation, to strengthen things such
> > as "do not use state->speed and state->duplex in mac_config() for
> > various specific AN modes".  Nothing has actually changed with any of
> > those clarifications.
> > 
> > For example, if in in-band mode, and mac_config() uses state->speed
> > and state->duplex, then it doesn't matter which version of phylink
> > you're using, if someone issues ethtool -s ethN ..., then state->speed
> > and state->duplex will be their respective UNKNOWN values, and if you're
> > using these in that situation, you will mis-program the MAC.
> > 
> > Again, that is not something that has changed.  Ever.  But the
> > documentation has because people just don't seem to get it, and I seemed
> > to be constantly repeating myself in review after review on the same
> > points.
> > 
> > So, your assertion that the phylink API is not stable is false.  It
> > has been remarkably stable over the two years that it has been around.
> > It is only natural that as the technology that a piece of code supports
> > evolves, so the code evolves with it.  That is exactly what has happened
> > this year with the two changes I mention above.
> > 
> > Now, if you've found it time consuming to "fix things" (unspecified what
> > "things" are) then I assert that what has needed to be fixed are things
> > that NXP have got wrong.  Such as the rtnl cockups.  Such as abusing
> > state->speed and state->duplex.  None of that is because the interface
> > is unstable - they are down to buggy implementation on NXPs part.
> > 
> > Essentially, what I'm saying is that your attempt to paint phylink as
> > being painful on the basis of interface changes is totally and utterly
> > wrong and is just an excuse to justify abusing the fixed-link code and
> > specifying things that are clearly incorrect via DT.
> 
> Thank you for the distilled phylink history, it may be easier to comprehend
> with these details. I was not referring to phylink, but PHY related issues
> on the DPAA 1 platforms.

Sigh.  No, you were referring to phylink.  This is what you said:

> I understand that PHYLink is working to describe this
> Better, but it was not there at that time. Adding the dependency on
> PHYLink was not desirable as most of the users for the DPAA 1 platforms
> were targeting kernels before the PHYLink introduction (and last I've
> looked, it's still under development, with unstable APIs so we'll
> take a look at this later, when it settles).

This discussion stems from your misconception and incorrect statements
concerning phylink, which I am correcting in this discussion.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 2/2] net: phy: Associate device node with fixed PHY

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Sent: 03 August 2020 19:54
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> <vikas.singh@nxp.com>
> Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
> 
> On Mon, Aug 03, 2020 at 02:47:41PM +0000, Madalin Bucur (OSS) wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > Sent: 03 August 2020 17:10
> > > To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> > > Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> > > <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> hkallweit1@gmail.com;
> > > netdev@vger.kernel.org; Calvin Johnson (OSS)
> <calvin.johnson@oss.nxp.com>;
> > > kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> > > <vikas.singh@nxp.com>
> > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed
> PHY
> > >
> > > On Mon, Aug 03, 2020 at 11:45:55AM +0000, Madalin Bucur (OSS) wrote:
> > > > > -----Original Message-----
> > > > > From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > > > Sent: 03 August 2020 12:07
> > > > > To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> > > > > Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> > > > > <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> > > hkallweit1@gmail.com;
> > > > > netdev@vger.kernel.org; Calvin Johnson (OSS)
> > > <calvin.johnson@oss.nxp.com>;
> > > > > kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> > > > > <vikas.singh@nxp.com>
> > > > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with
> fixed
> > > PHY
> > > > >
> > > > > On Mon, Aug 03, 2020 at 08:33:19AM +0000, Madalin Bucur (OSS)
> wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: netdev-owner@vger.kernel.org <netdev-
> owner@vger.kernel.org>
> > > On
> > > > > > > Behalf Of Andrew Lunn
> > > > > > > Sent: 01 August 2020 18:11
> > > > > > > To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > > > > > Cc: Vikas Singh <vikas.singh@puresoftware.com>;
> > > f.fainelli@gmail.com;
> > > > > > > hkallweit1@gmail.com; netdev@vger.kernel.org; Calvin Johnson
> (OSS)
> > > > > > > <calvin.johnson@oss.nxp.com>; kuldip dwivedi
> > > > > > > <kuldip.dwivedi@puresoftware.com>; Madalin Bucur (OSS)
> > > > > > > <madalin.bucur@oss.nxp.com>; Vikas Singh <vikas.singh@nxp.com>
> > > > > > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with
> > > fixed
> > > > > PHY
> > > > > > >
> > > > > > > On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM
> Linux
> > > > > admin
> > > > > > > wrote:
> > > > > > > > On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > > > > > > > > Hi Andrew,
> > > > > > > > >
> > > > > > > > > Please refer to the "fman" node under
> > > > > > > > > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > > > > > > > > I have two 10G ethernet interfaces out of which one is of
> > > fixed-
> > > > > link.
> > > > > > > >
> > > > > > > > Please do not top post.
> > > > > > > >
> > > > > > > > How does XGMII (which is a 10G only interface) work at 1G
> speed?
> > > Is
> > > > > > > > what is in DT itself a hack because fixed-phy doesn't
> support
> > > 10G
> > > > > > > > modes?
> > > > > > >
> > > > > > > My gut feeling is there is some hack going on here, which is
> why
> > > i'm
> > > > > > > being persistent at trying to understand what is actually
> going on
> > > > > > > here.
> > > > > >
> > > > > > Hi Andrew,
> > > > > >
> > > > > > That platform used 1G fixed link there since there was no
> support
> > > for
> > > > > > 10G fixed link at the time. PHYlib could have tolerated 10G
> speed
> > > there
> > > > > > With a one-liner.
> > > > >
> > > > > That statement is false.  It is not a "one liner".  fixed-phy
> exposes
> > > > > the settings to userspace as a Clause 22 PHY register set, and the
> > > > > Clause 22 register set does not support 10G.  So, a "one liner"
> would
> > > > > just be yet another hack.  Adding Clause 45 PHY emulation support
> > > > > would be a huge task.
> > > > >
> > > > > > I understand that PHYLink is working to describe this
> > > > > > Better, but it was not there at that time. Adding the dependency
> on
> > > > > > PHYLink was not desirable as most of the users for the DPAA 1
> > > platforms
> > > > > > were targeting kernels before the PHYLink introduction (and last
> > > I've
> > > > > > looked, it's still under development, with unstable APIs so
> we'll
> > > > > > take a look at this later, when it settles).
> > > > >
> > > > > I think you need to read Documentation/process/stable-api-
> nonsense.rst
> > > > > particularly the section "Stable Kernel Source Interfaces".
> > > > >
> > > > > phylink is going to be under development for quite some time to
> come
> > > > > as requirements evolve.  For example, when support for QSFP
> interfaces
> > > > > is eventually worked out, I suspect there will need to be some
> further
> > > > > changes to the driver interface.  This is completely normal.
> > > > >
> > > > > Now, as to the stability of the phylink API to drivers, it has in
> fact
> > > > > been very stable - it has only changed over the course of this
> year to
> > > > > support split PCS, a necessary step for DPAA2 and a few others.
> It
> > > has
> > > > > been around in mainline for two years, and has been around much
> longer
> > > > > than that, and during that time it has been in mainline, the MAC
> > > facing
> > > > > interface has not changed until recently.
> > > > >
> > > > > So, I find your claim to be quite unreasonable.
> > > >
> > > > I see you agree that there were and there will be many changes for a
> > > while,
> > > > It's not a complaint, I know hot it works, it's just a decision
> based on
> > > > required effort vs features offered vs user requirements. Lately
> it's
> > > been
> > > > time consuming to try to fix things in this area.
> > >
> > > No, it hasn't been time consuming.  The only API changes as far as
> > > drivers are concerned have been:
> > >
> > > 1. the change to the mac_link_up() prototype to move the setup of the
> > >    final link parameters out of mac_config() - and almost all of the
> > >    updates to users were done by me.
> > >
> > > 2. the addition of split PCS support, introducing new interfaces, has
> > >    had minimal impact on those drivers that updated in step (1).
> > >
> > > There have been no other changes as far as users are concerned.
> > >
> > > Some of the difficulty with (1) has been that users of phylink
> appeared
> > > initially with no proper review, and consequently they got quite a lot
> > > wrong.  The most common error has been using state->speed, state-
> >duplex
> > > in mac_config() methods irrespective of the AN mode, which has
> _always_
> > > since before phylink was merged into mainline, been totally unreliable.
> > >
> > > That leads me on to the other visible "changes" for users are
> concerned,
> > > which may be interpreted as interface changes, but are not; they have
> > > all been clarifications to the documentation, to strengthen things
> such
> > > as "do not use state->speed and state->duplex in mac_config() for
> > > various specific AN modes".  Nothing has actually changed with any of
> > > those clarifications.
> > >
> > > For example, if in in-band mode, and mac_config() uses state->speed
> > > and state->duplex, then it doesn't matter which version of phylink
> > > you're using, if someone issues ethtool -s ethN ..., then state->speed
> > > and state->duplex will be their respective UNKNOWN values, and if
> you're
> > > using these in that situation, you will mis-program the MAC.
> > >
> > > Again, that is not something that has changed.  Ever.  But the
> > > documentation has because people just don't seem to get it, and I
> seemed
> > > to be constantly repeating myself in review after review on the same
> > > points.
> > >
> > > So, your assertion that the phylink API is not stable is false.  It
> > > has been remarkably stable over the two years that it has been around.
> > > It is only natural that as the technology that a piece of code
> supports
> > > evolves, so the code evolves with it.  That is exactly what has
> happened
> > > this year with the two changes I mention above.
> > >
> > > Now, if you've found it time consuming to "fix things" (unspecified
> what
> > > "things" are) then I assert that what has needed to be fixed are
> things
> > > that NXP have got wrong.  Such as the rtnl cockups.  Such as abusing
> > > state->speed and state->duplex.  None of that is because the interface
> > > is unstable - they are down to buggy implementation on NXPs part.
> > >
> > > Essentially, what I'm saying is that your attempt to paint phylink as
> > > being painful on the basis of interface changes is totally and utterly
> > > wrong and is just an excuse to justify abusing the fixed-link code and
> > > specifying things that are clearly incorrect via DT.
> >
> > Thank you for the distilled phylink history, it may be easier to
> comprehend
> > with these details. I was not referring to phylink, but PHY related
> issues
> > on the DPAA 1 platforms.
> 
> Sigh.  No, you were referring to phylink.  This is what you said:
> 
> > I understand that PHYLink is working to describe this
> > Better, but it was not there at that time. Adding the dependency on
> > PHYLink was not desirable as most of the users for the DPAA 1 platforms
> > were targeting kernels before the PHYLink introduction (and last I've
> > looked, it's still under development, with unstable APIs so we'll
> > take a look at this later, when it settles).
> 
> This discussion stems from your misconception and incorrect statements
> concerning phylink, which I am correcting in this discussion.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

"Lately it's been time consuming to try to fix things in this area."

area != phylink

Otherwise, yes, I may have some misconceptions in regards to phylink, not
having paid the time to understand the details.

Madalin

[PATCH 0/2] Add fwnode helper functions to MDIO bus driver

[Vikas Singh]

This patch add helper functions to handle fwnodes on MDIO bus in case of
ACPI probing. These helper functions will be used in DPAA MAC driver.

The patches are in below logical order:
1. Add helper function to attach MAC with PHY
2. Associate device node with fixed PHY by extending "fixed_phy_status"

Vikas Singh (2):
  net: phy: Add fwnode helper functions
  net: phy: Associate device node with fixed PHY

 drivers/net/phy/fixed_phy.c |  2 ++
 drivers/net/phy/mdio_bus.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h        |  4 +++
 include/linux/phy_fixed.h   |  2 ++
 4 files changed, 74 insertions(+)

-- 
2.7.4


-- 




*Disclaimer* -The information transmitted is intended solely for the 
individual
or entity to which it is addressed and may contain confidential 
and/or
privileged material. Any review, re-transmission, dissemination or 
other use of
or taking action in reliance upon this information by persons 
or entities other
than the intended recipient is prohibited. If you have 
received this email in
error please contact the sender and delete the 
material from any computer. In
such instances you are further prohibited 
from reproducing, disclosing,
distributing or taking any action in reliance 
on it.As a recipient of this email,
you are responsible for screening its 
contents and the contents of any
attachments for the presence of viruses. 
No liability is accepted for any
damages caused by any virus transmitted by 
this email.

[PATCH 2/2] net: phy: Associate device node with fixed PHY

[Vikas Singh]

This patch will extend "struct fixed_phy_status" by adding new
"struct device *dev" member entry in it.
This change will help to handle the fixed phy registration in
ACPI probe case for fwnodes.

Signed-off-by: Vikas Singh <vikas.singh@puresoftware.com>
---
 drivers/net/phy/fixed_phy.c | 2 ++
 include/linux/phy_fixed.h   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index c4641b1..011c033 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -267,6 +267,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
 		phy->duplex = status->duplex;
 		phy->pause = status->pause;
 		phy->asym_pause = status->asym_pause;
+		if (!np)
+			phy->mdio.dev.fwnode = status->dev->fwnode;
 	}
 
 	of_node_get(np);
diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index 52bc8e4..155fea6 100644
--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h
@@ -8,6 +8,8 @@ struct fixed_phy_status {
 	int duplex;
 	int pause;
 	int asym_pause;
+	/* Associated device node */
+	struct device *dev;
 };
 
 struct device_node;
-- 
2.7.4


-- 




*Disclaimer* -The information transmitted is intended solely for the 
individual
or entity to which it is addressed and may contain confidential 
and/or
privileged material. Any review, re-transmission, dissemination or 
other use of
or taking action in reliance upon this information by persons 
or entities other
than the intended recipient is prohibited. If you have 
received this email in
error please contact the sender and delete the 
material from any computer. In
such instances you are further prohibited 
from reproducing, disclosing,
distributing or taking any action in reliance 
on it.As a recipient of this email,
you are responsible for screening its 
contents and the contents of any
attachments for the presence of viruses. 
No liability is accepted for any
damages caused by any virus transmitted by 
this email.