[PATCH 1/2] net: phy: micrel: Add KS8851 PHY support

[PATCH 1/2] net: phy: micrel: Add KS8851 PHY support

[Marek Vasut]

The KS8851 has a reduced internal PHY, which is accessible through its
registers at offset 0xe4. The PHY is compatible with KS886x PHY present
in Micrel switches, except the PHY ID Low/High registers are swapped.
Add PHY ID for this KS8851 PHY and use custom PHY ID mask due to the swap.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Lukas Wunner <lukas@wunner.de>
To: netdev@vger.kernel.org
---
 drivers/net/phy/micrel.c   | 9 +++++++++
 include/linux/micrel_phy.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 54e0d75203da..ca6da128e37a 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1386,6 +1386,14 @@ static struct phy_driver ksphy_driver[] = {
 	.read_status	= ksz8873mll_read_status,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
+}, {
+	.phy_id		= PHY_ID_KSZ8851,
+	.phy_id_mask	= 0xfff000ff,
+	.name		= "Micrel KSZ8851 Ethernet MAC",
+	/* PHY_BASIC_FEATURES */
+	.config_init	= kszphy_config_init,
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
 }, {
 	.phy_id		= PHY_ID_KSZ886X,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
@@ -1432,6 +1440,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
 	{ PHY_ID_KSZ8061, MICREL_PHY_ID_MASK },
 	{ PHY_ID_KSZ8081, MICREL_PHY_ID_MASK },
 	{ PHY_ID_KSZ8873MLL, MICREL_PHY_ID_MASK },
+	{ PHY_ID_KSZ8851, 0xfff000ff },
 	{ PHY_ID_KSZ886X, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
 	{ }
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 416ee6dd2574..1c26e4ac0dc9 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -29,6 +29,8 @@
 #define PHY_ID_KSZ9131		0x00221640
 #define PHY_ID_LAN8814		0x00221660
 
+/* The PHY ID Low/High registers are swapped on KSZ8851 */
+#define PHY_ID_KSZ8851		0x14300022
 #define PHY_ID_KSZ886X		0x00221430
 #define PHY_ID_KSZ8863		0x00221435
 
-- 
2.29.2

[PATCH 2/2] net: ks8851: Register MDIO bus and the internal PHY

[Marek Vasut]

The KS8851 has a reduced internal PHY, which is accessible through its
registers at offset 0xe4. The PHY is compatible with KS886x PHY present
in Micrel switches, except the PHY ID Low/High registers are swapped.
Register MDIO bus so this PHY can be detected and probed by phylib.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Lukas Wunner <lukas@wunner.de>
To: netdev@vger.kernel.org
---
 drivers/net/ethernet/micrel/ks8851.h        |  1 +
 drivers/net/ethernet/micrel/ks8851_common.c | 69 ++++++++++++++++++++-
 2 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index 2b319e451121..9bed9e024d81 100644
--- a/drivers/net/ethernet/micrel/ks8851.h
+++ b/drivers/net/ethernet/micrel/ks8851.h
@@ -403,6 +403,7 @@ struct ks8851_net {
 	struct regulator	*vdd_reg;
 	struct regulator	*vdd_io;
 	int			gpio;
+	struct mii_bus		*mii_bus;
 
 	void			(*lock)(struct ks8851_net *ks,
 					unsigned long *flags);
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index 6fc7483aea03..c4f5cb9768ef 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -23,6 +23,7 @@
 
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
+#include <linux/of_mdio.h>
 #include <linux/of_net.h>
 
 #include "ks8851.h"
@@ -983,6 +984,24 @@ static void ks8851_phy_write(struct net_device *dev,
 	}
 }
 
+static int ks8851_mdio_read(struct mii_bus *bus, int phy_id, int reg)
+{
+	struct ks8851_net *ks = bus->priv;
+
+	if (phy_id != 0)
+		return 0xffffffff;
+
+	return ks8851_phy_read(ks->netdev, phy_id, reg);
+}
+
+static int ks8851_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 val)
+{
+	struct ks8851_net *ks = bus->priv;
+
+	ks8851_phy_write(ks->netdev, phy_id, reg, val);
+	return 0;
+}
+
 /**
  * ks8851_read_selftest - read the selftest memory info.
  * @ks: The device state
@@ -1046,6 +1065,42 @@ int ks8851_resume(struct device *dev)
 }
 #endif
 
+static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev)
+{
+	struct mii_bus *mii_bus;
+	int ret;
+
+	mii_bus = mdiobus_alloc();
+	if (!mii_bus)
+		return -ENOMEM;
+
+	mii_bus->name = "ks8851_eth_mii";
+	mii_bus->read = ks8851_mdio_read;
+	mii_bus->write = ks8851_mdio_write;
+	mii_bus->priv = ks;
+	mii_bus->parent = dev;
+	mii_bus->phy_mask = ~BIT(0);
+	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
+
+	ret = mdiobus_register(mii_bus);
+	if (ret)
+		goto err_mdiobus_register;
+
+	ks->mii_bus = mii_bus;
+
+	return 0;
+
+err_mdiobus_register:
+	mdiobus_free(mii_bus);
+	return ret;
+}
+
+static void ks8851_unregister_mdiobus(struct ks8851_net *ks)
+{
+	mdiobus_unregister(ks->mii_bus);
+	mdiobus_free(ks->mii_bus);
+}
+
 int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 			int msg_en)
 {
@@ -1104,6 +1159,8 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 
 	INIT_WORK(&ks->rxctrl_work, ks8851_rxctrl_work);
 
+	SET_NETDEV_DEV(netdev, dev);
+
 	/* setup EEPROM state */
 	ks->eeprom.data = ks;
 	ks->eeprom.width = PCI_EEPROM_WIDTH_93C46;
@@ -1120,6 +1177,10 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 
 	dev_info(dev, "message enable is %d\n", msg_en);
 
+	ret = ks8851_register_mdiobus(ks, dev);
+	if (ret)
+		goto err_mdio;
+
 	/* set the default message enable */
 	ks->msg_enable = netif_msg_init(msg_en, NETIF_MSG_DRV |
 						NETIF_MSG_PROBE |
@@ -1128,7 +1189,6 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 	skb_queue_head_init(&ks->txq);
 
 	netdev->ethtool_ops = &ks8851_ethtool_ops;
-	SET_NETDEV_DEV(netdev, dev);
 
 	dev_set_drvdata(dev, ks);
 
@@ -1156,7 +1216,7 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 	ret = register_netdev(netdev);
 	if (ret) {
 		dev_err(dev, "failed to register network device\n");
-		goto err_netdev;
+		goto err_id;
 	}
 
 	netdev_info(netdev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
@@ -1165,8 +1225,9 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 
 	return 0;
 
-err_netdev:
 err_id:
+	ks8851_unregister_mdiobus(ks);
+err_mdio:
 	if (gpio_is_valid(gpio))
 		gpio_set_value(gpio, 0);
 	regulator_disable(ks->vdd_reg);
@@ -1180,6 +1241,8 @@ int ks8851_remove_common(struct device *dev)
 {
 	struct ks8851_net *priv = dev_get_drvdata(dev);
 
+	ks8851_unregister_mdiobus(priv);
+
 	if (netif_msg_drv(priv))
 		dev_info(dev, "remove\n");
 
-- 
2.29.2

Re: [PATCH 2/2] net: ks8851: Register MDIO bus and the internal PHY

[Andrew Lunn]

> +static int ks8851_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> +{
> +	struct ks8851_net *ks = bus->priv;
> +
> +	if (phy_id != 0)
> +		return 0xffffffff;
> +

Please check for C45 and return -EOPNOTSUPP.

       Andrew

Re: [PATCH 2/2] net: ks8851: Register MDIO bus and the internal PHY

[Marek Vasut]

On 12/30/20 5:00 PM, Andrew Lunn wrote:
>> +static int ks8851_mdio_read(struct mii_bus *bus, int phy_id, int reg)
>> +{
>> +	struct ks8851_net *ks = bus->priv;
>> +
>> +	if (phy_id != 0)
>> +		return 0xffffffff;
>> +
> 
> Please check for C45 and return -EOPNOTSUPP.

The ks8851_reg_read() does all the register checking already.

Re: [PATCH 2/2] net: ks8851: Register MDIO bus and the internal PHY

[Andrew Lunn]

On Sun, Jan 03, 2021 at 01:58:09PM +0100, Marek Vasut wrote:
> On 12/30/20 5:00 PM, Andrew Lunn wrote:
> > > +static int ks8851_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> > > +{
> > > +	struct ks8851_net *ks = bus->priv;
> > > +
> > > +	if (phy_id != 0)
> > > +		return 0xffffffff;
> > > +
> > 
> > Please check for C45 and return -EOPNOTSUPP.
> 
> The ks8851_reg_read() does all the register checking already.

Not really.

static int ks8851_phy_read(struct net_device *dev, int phy_addr, int reg)
{
	struct ks8851_net *ks = netdev_priv(dev);
	unsigned long flags;
	int ksreg;
	int result;

	ksreg = ks8851_phy_reg(reg);
	if (!ksreg)
		return 0x0;	/* no error return allowed, so use zero */

	ks8851_lock(ks, &flags);
	result = ks8851_rdreg16(ks, ksreg);
	ks8851_unlock(ks, &flags);

	return result;
}

static int ks8851_phy_reg(int reg)
{
	switch (reg) {
	case MII_BMCR:
		return KS_P1MBCR;
	case MII_BMSR:
		return KS_P1MBSR;
	case MII_PHYSID1:
		return KS_PHY1ILR;
	case MII_PHYSID2:
		return KS_PHY1IHR;
	case MII_ADVERTISE:
		return KS_P1ANAR;
	case MII_LPA:
		return KS_P1ANLPR;
	}

	return 0x0;
}

So a C45 reg will cause 0 to be returned, not -EOPNOTSUPP.

   Andrew

Re: [PATCH 2/2] net: ks8851: Register MDIO bus and the internal PHY

[kernel test robot]

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

Hi Marek,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master linus/master v5.11-rc1 next-20201223]
[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/Marek-Vasut/net-phy-micrel-Add-KS8851-PHY-support/20201230-210003
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 4bfc4714849d005e6835bcffa3c29ebd6e5ee35d
config: alpha-allmodconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/799aae00b2ec29ee4a86a2d8b8a8b0c1b816cb84
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Vasut/net-phy-micrel-Add-KS8851-PHY-support/20201230-210003
        git checkout 799aae00b2ec29ee4a86a2d8b8a8b0c1b816cb84
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha 

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

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/micrel/ks8851_common.c: In function 'ks8851_register_mdiobus':
>> drivers/net/ethernet/micrel/ks8851_common.c:1082:22: warning: conversion from 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from '18446744073709551614' to '4294967294' [-Woverflow]
    1082 |  mii_bus->phy_mask = ~BIT(0);
         |                      ^


vim +1082 drivers/net/ethernet/micrel/ks8851_common.c

  1067	
  1068	static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev)
  1069	{
  1070		struct mii_bus *mii_bus;
  1071		int ret;
  1072	
  1073		mii_bus = mdiobus_alloc();
  1074		if (!mii_bus)
  1075			return -ENOMEM;
  1076	
  1077		mii_bus->name = "ks8851_eth_mii";
  1078		mii_bus->read = ks8851_mdio_read;
  1079		mii_bus->write = ks8851_mdio_write;
  1080		mii_bus->priv = ks;
  1081		mii_bus->parent = dev;
> 1082		mii_bus->phy_mask = ~BIT(0);
  1083		snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
  1084	
  1085		ret = mdiobus_register(mii_bus);
  1086		if (ret)
  1087			goto err_mdiobus_register;
  1088	
  1089		ks->mii_bus = mii_bus;
  1090	
  1091		return 0;
  1092	
  1093	err_mdiobus_register:
  1094		mdiobus_free(mii_bus);
  1095		return ret;
  1096	}
  1097	

---
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: 67188 bytes --]

Re: [PATCH 1/2] net: phy: micrel: Add KS8851 PHY support

[Andrew Lunn]

On Wed, Dec 30, 2020 at 01:53:57PM +0100, Marek Vasut wrote:
> The KS8851 has a reduced internal PHY, which is accessible through its
> registers at offset 0xe4. The PHY is compatible with KS886x PHY present
> in Micrel switches, except the PHY ID Low/High registers are swapped.

Can you intercept the reads in the KS8851 driver and swap them back
again? The mv88e6xxx driver does something similar. The mv88e6393
family of switches have PHYs with the Marvell OUI but no device ID. So
the code traps these reads and provides an ID.

    Andrew

Re: [PATCH 1/2] net: phy: micrel: Add KS8851 PHY support

[Marek Vasut]

On 12/30/20 4:43 PM, Andrew Lunn wrote:
> On Wed, Dec 30, 2020 at 01:53:57PM +0100, Marek Vasut wrote:
>> The KS8851 has a reduced internal PHY, which is accessible through its
>> registers at offset 0xe4. The PHY is compatible with KS886x PHY present
>> in Micrel switches, except the PHY ID Low/High registers are swapped.
> 
> Can you intercept the reads in the KS8851 driver and swap them back
> again? The mv88e6xxx driver does something similar. The mv88e6393
> family of switches have PHYs with the Marvell OUI but no device ID. So
> the code traps these reads and provides an ID.

I would prefer to keep this as-is, since then the PHY driver can match 
on these swapped IDs and discern the PHY from PHY present in the KS886x 
switch.

Re: [PATCH 1/2] net: phy: micrel: Add KS8851 PHY support

[Andrew Lunn]

On Sun, Jan 03, 2021 at 01:55:22PM +0100, Marek Vasut wrote:
> On 12/30/20 4:43 PM, Andrew Lunn wrote:
> > On Wed, Dec 30, 2020 at 01:53:57PM +0100, Marek Vasut wrote:
> > > The KS8851 has a reduced internal PHY, which is accessible through its
> > > registers at offset 0xe4. The PHY is compatible with KS886x PHY present
> > > in Micrel switches, except the PHY ID Low/High registers are swapped.
> > 
> > Can you intercept the reads in the KS8851 driver and swap them back
> > again? The mv88e6xxx driver does something similar. The mv88e6393
> > family of switches have PHYs with the Marvell OUI but no device ID. So
> > the code traps these reads and provides an ID.
> 
> I would prefer to keep this as-is, since then the PHY driver can match on
> these swapped IDs and discern the PHY from PHY present in the KS886x switch.

The problem is, this ID contains an OUI. Well, part of an OUI, the top
two bits of the OUI are removed, leaving 22 bits of a 24 bit
OUI. These OUIs are assigned to companies, well organisations. If you
look at the reversed PHY ID, what are the 4 possible OUIs? Who are
they assigned to? Are they ever likely to manufacture a PHY?

	 Andrew

Re: [PATCH 1/2] net: phy: micrel: Add KS8851 PHY support

[Marek Vasut]

On 1/3/21 5:55 PM, Andrew Lunn wrote:
> On Sun, Jan 03, 2021 at 01:55:22PM +0100, Marek Vasut wrote:
>> On 12/30/20 4:43 PM, Andrew Lunn wrote:
>>> On Wed, Dec 30, 2020 at 01:53:57PM +0100, Marek Vasut wrote:
>>>> The KS8851 has a reduced internal PHY, which is accessible through its
>>>> registers at offset 0xe4. The PHY is compatible with KS886x PHY present
>>>> in Micrel switches, except the PHY ID Low/High registers are swapped.
>>>
>>> Can you intercept the reads in the KS8851 driver and swap them back
>>> again? The mv88e6xxx driver does something similar. The mv88e6393
>>> family of switches have PHYs with the Marvell OUI but no device ID. So
>>> the code traps these reads and provides an ID.
>>
>> I would prefer to keep this as-is, since then the PHY driver can match on
>> these swapped IDs and discern the PHY from PHY present in the KS886x switch.
> 
> The problem is, this ID contains an OUI. Well, part of an OUI, the top
> two bits of the OUI are removed, leaving 22 bits of a 24 bit
> OUI. These OUIs are assigned to companies, well organisations. If you
> look at the reversed PHY ID, what are the 4 possible OUIs? Who are
> they assigned to? Are they ever likely to manufacture a PHY?

Are these OUIs used for anything else than identifying the PHY though ?