[PATCH net-next 0/2] Add mii_bus to ixgbe driver for dsa devs

[PATCH net-next 0/2] Add mii_bus to ixgbe driver for dsa devs

[Steve Douthit]

Most dsa devices currently expect to get a pointer to a mii_bus from
platform data of device tree information.

The first patch exposes a mii_bus for ixgbe devices.

Currently the ixgbe driver only allows accesses to the probed PHY
address via ioctls. The second patch changes that behavior to allow
access to all addresses.  This should be useful for doing switch peeks &
pokes from userspace for development, test, or if theres no dsa driver
for a particular switch yet.

I've tested the clause 22 portion of this code on a board that has an
Intel C3xxx series SoC connected to a Marvell Peridot switch.  I don't
have any clause 45 devices or other ixgbe devices to test with.

Stephen Douthit (2):
  ixgbe: register a mdiobus
  ixgbe: use mii_bus to handle MII related ioctls

 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  35 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c  | 192 ++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h  |   2 +
 4 files changed, 216 insertions(+), 15 deletions(-)

-- 
2.17.2

[PATCH net-next 1/2] ixgbe: register a mdiobus

[Steve Douthit]

Most dsa devices expect a 'struct mii_bus' pointer to talk to switches
via the MII interface.

Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   6 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c  | 192 ++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h  |   2 +
 4 files changed, 202 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 143bdd5ee2a0..08d85e336bd4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -12,6 +12,7 @@
 #include <linux/aer.h>
 #include <linux/if_vlan.h>
 #include <linux/jiffies.h>
+#include <linux/phy.h>
 
 #include <linux/timecounter.h>
 #include <linux/net_tstamp.h>
@@ -561,6 +562,7 @@ struct ixgbe_adapter {
 	struct net_device *netdev;
 	struct bpf_prog *xdp_prog;
 	struct pci_dev *pdev;
+	struct mii_bus *mii_bus;
 
 	unsigned long state;
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 49a4ea38eb07..c2141fe35685 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -39,6 +39,7 @@
 #include "ixgbe.h"
 #include "ixgbe_common.h"
 #include "ixgbe_dcb_82599.h"
+#include "ixgbe_phy.h"
 #include "ixgbe_sriov.h"
 #include "ixgbe_model.h"
 #include "ixgbe_txrx_common.h"
@@ -11120,6 +11121,9 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			IXGBE_LINK_SPEED_10GB_FULL | IXGBE_LINK_SPEED_1GB_FULL,
 			true);
 
+	if (ixgbe_mii_bus_init(hw))
+		e_err(probe, "failed to create mii_bus\n");
+
 	return 0;
 
 err_register:
@@ -11170,6 +11174,8 @@ static void ixgbe_remove(struct pci_dev *pdev)
 	set_bit(__IXGBE_REMOVING, &adapter->state);
 	cancel_work_sync(&adapter->service_task);
 
+	if (adapter->mii_bus)
+		mdiobus_unregister(adapter->mii_bus);
 
 #ifdef CONFIG_IXGBE_DCA
 	if (adapter->flags & IXGBE_FLAG_DCA_ENABLED) {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
index 919a7af84b42..75db340963ad 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
@@ -658,6 +658,198 @@ s32 ixgbe_write_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
 	return status;
 }
 
+/**
+ *  ixgbe_msca - Write the command register and poll for completion/timeout
+ *  @hw: pointer to hardware structure
+ *  @cmd: command register value to write
+ **/
+static s32 ixgbe_msca_cmd(struct ixgbe_hw *hw, u32 cmd)
+{
+	u32 i;
+
+	IXGBE_WRITE_REG(hw, IXGBE_MSCA, cmd);
+
+	for (i = 0; i < IXGBE_MDIO_COMMAND_TIMEOUT; i++) {
+		udelay(10);
+		cmd = IXGBE_READ_REG(hw, IXGBE_MSCA);
+		if (!(cmd & IXGBE_MSCA_MDI_COMMAND))
+			return 0;
+	}
+
+	return -ETIMEDOUT;
+}
+
+/**
+ *  ixgbe_msca - Use device_id to figure out if MDIO bus is shared between MACs.
+ *  The embedded x550(s) in the C3000 line of SoCs only have a single mii_bus
+ *  shared between all MACs, and that introduces some new mask flags that need
+ *  to be passed to the *_swfw_sync callbacks.
+ *  @hw: pointer to hardware structure
+ **/
+static bool ixgbe_is_shared_mii(struct ixgbe_hw *hw)
+{
+	switch (hw->device_id) {
+	case IXGBE_DEV_ID_X550EM_A_KR:
+	case IXGBE_DEV_ID_X550EM_A_KR_L:
+	case IXGBE_DEV_ID_X550EM_A_SFP_N:
+	case IXGBE_DEV_ID_X550EM_A_SGMII:
+	case IXGBE_DEV_ID_X550EM_A_SGMII_L:
+	case IXGBE_DEV_ID_X550EM_A_10G_T:
+	case IXGBE_DEV_ID_X550EM_A_SFP:
+	case IXGBE_DEV_ID_X550EM_A_1G_T:
+	case IXGBE_DEV_ID_X550EM_A_1G_T_L:
+		return true;
+	}
+
+	return false;
+}
+
+/**
+ *  ixgbe_mii_bus_read - Read a clause 22/45 register
+ *  @hw: pointer to hardware structure
+ *  @addr: address
+ *  @regnum: register number
+ **/
+static s32 ixgbe_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
+{
+	struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
+	struct ixgbe_hw *hw = &adapter->hw;
+	u32 hwaddr, cmd, gssr = hw->phy.phy_semaphore_mask;
+	s32 data;
+
+	if (ixgbe_is_shared_mii(hw)) {
+		gssr |= IXGBE_GSSR_TOKEN_SM;
+		if (hw->bus.lan_id)
+			gssr |= IXGBE_GSSR_PHY1_SM;
+		else
+			gssr |= IXGBE_GSSR_PHY0_SM;
+	}
+
+	if (hw->mac.ops.acquire_swfw_sync(hw, gssr))
+		return -EBUSY;
+
+	hwaddr = addr << IXGBE_MSCA_PHY_ADDR_SHIFT;
+	if (regnum & MII_ADDR_C45) {
+		hwaddr |= regnum & GENMASK(21, 0);
+		cmd = hwaddr | IXGBE_MSCA_ADDR_CYCLE | IXGBE_MSCA_MDI_COMMAND;
+	} else {
+		hwaddr |= (regnum & GENMASK(5, 0)) << IXGBE_MSCA_DEV_TYPE_SHIFT;
+		cmd = hwaddr | IXGBE_MSCA_OLD_PROTOCOL |
+			IXGBE_MSCA_READ_AUTOINC | IXGBE_MSCA_MDI_COMMAND;
+	}
+
+	data = ixgbe_msca_cmd(hw, cmd);
+	if (data < 0)
+		goto mii_bus_read_done;
+
+	/* For a clause 45 access the address cycle just completed, we still
+	 * need to do the read command, otherwise just get the data
+	 */
+	if (!(regnum & MII_ADDR_C45))
+		goto do_mii_bus_read;
+
+	cmd = hwaddr | IXGBE_MSCA_READ | IXGBE_MSCA_MDI_COMMAND;
+	data = ixgbe_msca_cmd(hw, cmd);
+	if (data < 0)
+		goto mii_bus_read_done;
+
+do_mii_bus_read:
+	data = IXGBE_READ_REG(hw, IXGBE_MSRWD);
+	data = (data >> IXGBE_MSRWD_READ_DATA_SHIFT) & GENMASK(16, 0);
+
+mii_bus_read_done:
+	hw->mac.ops.release_swfw_sync(hw, gssr);
+	return data;
+}
+
+/**
+ *  ixgbe_mii_bus_write - Write a clause 22/45 register
+ *  @hw: pointer to hardware structure
+ *  @addr: address
+ *  @regnum: register number
+ *  @val: value to write
+ **/
+static s32 ixgbe_mii_bus_write(struct mii_bus *bus, int addr, int regnum,
+			       u16 val)
+{
+	struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
+	struct ixgbe_hw *hw = &adapter->hw;
+	u32 hwaddr, cmd, gssr = hw->phy.phy_semaphore_mask;
+	s32 err;
+
+	if (ixgbe_is_shared_mii(hw)) {
+		gssr |= IXGBE_GSSR_TOKEN_SM;
+		if (hw->bus.lan_id)
+			gssr |= IXGBE_GSSR_PHY1_SM;
+		else
+			gssr |= IXGBE_GSSR_PHY0_SM;
+	}
+
+	if (hw->mac.ops.acquire_swfw_sync(hw, gssr))
+		return -EBUSY;
+
+	IXGBE_WRITE_REG(hw, IXGBE_MSRWD, (u32)val);
+
+	hwaddr = addr << IXGBE_MSCA_PHY_ADDR_SHIFT;
+	if (regnum & MII_ADDR_C45) {
+		hwaddr |= regnum & GENMASK(21, 0);
+		cmd = hwaddr | IXGBE_MSCA_ADDR_CYCLE | IXGBE_MSCA_MDI_COMMAND;
+	} else {
+		hwaddr |= (regnum & GENMASK(5, 0)) << IXGBE_MSCA_DEV_TYPE_SHIFT;
+		cmd = hwaddr | IXGBE_MSCA_OLD_PROTOCOL | IXGBE_MSCA_WRITE |
+			IXGBE_MSCA_MDI_COMMAND;
+	}
+
+	/* For clause 45 this is an address cycle, for clause 22 this is the
+	 * entire transaction
+	 */
+	err = ixgbe_msca_cmd(hw, cmd);
+	if (err < 0 || !(regnum & MII_ADDR_C45))
+		goto mii_bus_write_done;
+
+	cmd = hwaddr | IXGBE_MSCA_WRITE | IXGBE_MSCA_MDI_COMMAND;
+	err = ixgbe_msca_cmd(hw, cmd);
+
+mii_bus_write_done:
+	hw->mac.ops.release_swfw_sync(hw, gssr);
+	return err;
+}
+
+/**
+ * ixgbe_mii_bus_init - mii_bus structure setup
+ * @hw: pointer to hardware structure
+ *
+ * Returns 0 on success, negative on failure
+ *
+ * ixgbe_mii_bus_init initializes a mii_bus structure in adapter
+ **/
+s32 ixgbe_mii_bus_init(struct ixgbe_hw *hw)
+{
+	struct ixgbe_adapter *adapter = hw->back;
+	struct pci_dev *pdev = adapter->pdev;
+	struct mii_bus *bus = adapter->mii_bus;
+	struct device *dev = &adapter->netdev->dev;
+
+	adapter->mii_bus = devm_mdiobus_alloc(dev);
+	if (!adapter->mii_bus)
+		return -ENOMEM;
+
+	bus = adapter->mii_bus;
+
+	/* Use the position of the device in the PCI hierarchy as the id */
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mdio-%s", ixgbe_driver_name,
+		 pci_name(pdev->bus->self));
+
+	bus->name = "ixgbe-mdio";
+	bus->read = &ixgbe_mii_bus_read;
+	bus->write = &ixgbe_mii_bus_write;
+	bus->priv = adapter;
+	bus->parent = dev;
+	bus->phy_mask = 0xffffffff;
+
+	return mdiobus_register(bus);
+}
+
 /**
  *  ixgbe_setup_phy_link_generic - Set and restart autoneg
  *  @hw: pointer to hardware structure
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
index 64e44e01c973..214b01085718 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
@@ -120,6 +120,8 @@
 /* SFP+ SFF-8472 Compliance code */
 #define IXGBE_SFF_SFF_8472_UNSUP      0x00
 
+s32 ixgbe_mii_bus_init(struct ixgbe_hw *hw);
+
 s32 ixgbe_identify_phy_generic(struct ixgbe_hw *hw);
 s32 ixgbe_reset_phy_generic(struct ixgbe_hw *hw);
 s32 ixgbe_read_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
-- 
2.17.2

[PATCH net-next 2/2] ixgbe: use mii_bus to handle MII related ioctls

[Steve Douthit]

Use the mii_bus callbacks to address the entire clause 22/45 address
space.  Enables userspace to poke switch registers instead of a single
PHY address.

Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 29 +++++++++----------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index c2141fe35685..345ecdddc749 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8786,27 +8786,26 @@ static int
 ixgbe_mdio_read(struct net_device *netdev, int prtad, int devad, u16 addr)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	struct ixgbe_hw *hw = &adapter->hw;
-	u16 value;
-	int rc;
+	struct mii_bus *mii_bus = adapter->mii_bus;
+	int regnum = addr;
 
-	if (prtad != hw->phy.mdio.prtad)
-		return -EINVAL;
-	rc = hw->phy.ops.read_reg(hw, addr, devad, &value);
-	if (!rc)
-		rc = value;
-	return rc;
+	if (devad != MDIO_DEVAD_NONE)
+		regnum |= (devad << 16) | MII_ADDR_C45;
+
+	return mdiobus_read(mii_bus, prtad, regnum);
 }
 
 static int ixgbe_mdio_write(struct net_device *netdev, int prtad, int devad,
 			    u16 addr, u16 value)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	struct ixgbe_hw *hw = &adapter->hw;
+	struct mii_bus *mii_bus = adapter->mii_bus;
+	int regnum = addr;
 
-	if (prtad != hw->phy.mdio.prtad)
-		return -EINVAL;
-	return hw->phy.ops.write_reg(hw, addr, devad, value);
+	if (devad != MDIO_DEVAD_NONE)
+		regnum |= (devad << 16) | MII_ADDR_C45;
+
+	return mdiobus_write(mii_bus, prtad, regnum, value);
 }
 
 static int ixgbe_ioctl(struct net_device *netdev, struct ifreq *req, int cmd)
@@ -8819,7 +8818,7 @@ static int ixgbe_ioctl(struct net_device *netdev, struct ifreq *req, int cmd)
 	case SIOCGHWTSTAMP:
 		return ixgbe_ptp_get_ts_config(adapter, req);
 	case SIOCGMIIPHY:
-		if (!adapter->hw.phy.ops.read_reg)
+		if (!adapter->mii_bus)
 			return -EOPNOTSUPP;
 		/* fall through */
 	default:
@@ -10796,7 +10795,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* ixgbe_identify_phy_generic will set prtad and mmds properly */
 	hw->phy.mdio.prtad = MDIO_PRTAD_NONE;
 	hw->phy.mdio.mmds = 0;
-	hw->phy.mdio.mode_support = MDIO_SUPPORTS_C45 | MDIO_EMULATE_C22;
+	hw->phy.mdio.mode_support = MDIO_SUPPORTS_C45 | MDIO_SUPPORTS_C22;
 	hw->phy.mdio.dev = netdev;
 	hw->phy.mdio.mdio_read = ixgbe_mdio_read;
 	hw->phy.mdio.mdio_write = ixgbe_mdio_write;
-- 
2.17.2

Re: [PATCH net-next 0/2] Add mii_bus to ixgbe driver for dsa devs

[Jeff Kirsher]

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

On Thu, 2018-11-29 at 18:54 +0000, Steve Douthit wrote:
> Most dsa devices currently expect to get a pointer to a mii_bus from
> platform data of device tree information.
> 
> The first patch exposes a mii_bus for ixgbe devices.
> 
> Currently the ixgbe driver only allows accesses to the probed PHY
> address via ioctls. The second patch changes that behavior to allow
> access to all addresses.  This should be useful for doing switch peeks &
> pokes from userspace for development, test, or if theres no dsa driver
> for a particular switch yet.
> 
> I've tested the clause 22 portion of this code on a board that has an
> Intel C3xxx series SoC connected to a Marvell Peridot switch.  I don't
> have any clause 45 devices or other ixgbe devices to test with.
> 
> Stephen Douthit (2):
>   ixgbe: register a mdiobus
>   ixgbe: use mii_bus to handle MII related ioctls
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  35 ++--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c  | 192 ++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h  |   2 +
>  4 files changed, 216 insertions(+), 15 deletions(-)
> 

I will add these changes to my queue, please remember to send patches
against the drivers in drivers/net/ethernet/intel/* to 
intel-wired-lan@lists.osuosl.org  mailing list as well, so that our
patchwork project can track the patches.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 0/2] Add mii_bus to ixgbe driver for dsa devs

[Steve Douthit]

On 11/29/18 2:03 PM, Jeff Kirsher wrote:
> On Thu, 2018-11-29 at 18:54 +0000, Steve Douthit wrote:
>> Most dsa devices currently expect to get a pointer to a mii_bus from
>> platform data of device tree information.
>>
>> The first patch exposes a mii_bus for ixgbe devices.
>>
>> Currently the ixgbe driver only allows accesses to the probed PHY
>> address via ioctls. The second patch changes that behavior to allow
>> access to all addresses.  This should be useful for doing switch peeks &
>> pokes from userspace for development, test, or if theres no dsa driver
>> for a particular switch yet.
>>
>> I've tested the clause 22 portion of this code on a board that has an
>> Intel C3xxx series SoC connected to a Marvell Peridot switch.  I don't
>> have any clause 45 devices or other ixgbe devices to test with.
>>
>> Stephen Douthit (2):
>>    ixgbe: register a mdiobus
>>    ixgbe: use mii_bus to handle MII related ioctls
>>
>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  35 ++--
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c  | 192 ++++++++++++++++++
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h  |   2 +
>>   4 files changed, 216 insertions(+), 15 deletions(-)
>>
> 
> I will add these changes to my queue, please remember to send patches
> against the drivers in drivers/net/ethernet/intel/* to
> intel-wired-lan@lists.osuosl.org  mailing list as well, so that our
> patchwork project can track the patches.

Sorry about that, I'll add it to the CC list for the next rev.

I started adding another board to the platform driver that needs this
code and found a bug.  The new board has more lanes enabled via soft
straps.  That caused sysfs to complain because I ended up with duplicate
bus IDs from this code:

+	/* Use the position of the device in the PCI hierarchy as the id */
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mdio-%s", ixgbe_driver_name,
+		 pci_name(pdev->bus->self));

Please drop this series for now.

Thinking about this some more I probably need to ensure that only a
single mii_bus is registered out of all the discovered x550em_a devices.
More generally there's not always going to be a 1:1 mapping between MACs
and MIIs on these devices and the code should handle that.

I think for the next rev I'll only register a single mii_bus for the
lowest numbered bus:device.function x500em_a device, and skip mii_bus
setup entirely for other device IDs.

Let me know if there's another/better way to tackle that.

Re: [PATCH net-next 0/2] Add mii_bus to ixgbe driver for dsa devs

[Jeff Kirsher]

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

On Thu, 2018-11-29 at 22:19 +0000, Steve Douthit wrote:
> On 11/29/18 2:03 PM, Jeff Kirsher wrote:
> > On Thu, 2018-11-29 at 18:54 +0000, Steve Douthit wrote:
> > > Most dsa devices currently expect to get a pointer to a mii_bus from
> > > platform data of device tree information.
> > > 
> > > The first patch exposes a mii_bus for ixgbe devices.
> > > 
> > > Currently the ixgbe driver only allows accesses to the probed PHY
> > > address via ioctls. The second patch changes that behavior to allow
> > > access to all addresses.  This should be useful for doing switch
> > > peeks &
> > > pokes from userspace for development, test, or if theres no dsa
> > > driver
> > > for a particular switch yet.
> > > 
> > > I've tested the clause 22 portion of this code on a board that has an
> > > Intel C3xxx series SoC connected to a Marvell Peridot switch.  I
> > > don't
> > > have any clause 45 devices or other ixgbe devices to test with.
> > > 
> > > Stephen Douthit (2):
> > >    ixgbe: register a mdiobus
> > >    ixgbe: use mii_bus to handle MII related ioctls
> > > 
> > >   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
> > >   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  35 ++--
> > >   drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c  | 192
> > > ++++++++++++++++++
> > >   drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h  |   2 +
> > >   4 files changed, 216 insertions(+), 15 deletions(-)
> > > 
> > 
> > I will add these changes to my queue, please remember to send patches
> > against the drivers in drivers/net/ethernet/intel/* to
> > intel-wired-lan@lists.osuosl.org  mailing list as well, so that our
> > patchwork project can track the patches.
> 
> Sorry about that, I'll add it to the CC list for the next rev.
> 
> I started adding another board to the platform driver that needs this
> code and found a bug.  The new board has more lanes enabled via soft
> straps.  That caused sysfs to complain because I ended up with duplicate
> bus IDs from this code:
> 
> +	/* Use the position of the device in the PCI hierarchy as the id */
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mdio-%s", ixgbe_driver_name,
> +		 pci_name(pdev->bus->self));
> 
> Please drop this series for now.

Ok, it has been dropped and I will wait for v2.

> Thinking about this some more I probably need to ensure that only a
> single mii_bus is registered out of all the discovered x550em_a devices.
> More generally there's not always going to be a 1:1 mapping between MACs
> and MIIs on these devices and the code should handle that.
> 
> I think for the next rev I'll only register a single mii_bus for the
> lowest numbered bus:device.function x500em_a device, and skip mii_bus
> setup entirely for other device IDs.
> 
> Let me know if there's another/better way to tackle that.

I will bring this up with our 10GbE driver lead, he may have ideas.  Any
patches sent to intel-wired-lan@lists.osuosl.org mailing list will be seen
by our 10GbE driver developers, so they can provide feedback.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

[Andrew Lunn]

Hi Steve

Cool to see another interface being made DSA capable.

> +/**
> + *  ixgbe_msca - Write the command register and poll for completion/timeout
> + *  @hw: pointer to hardware structure
> + *  @cmd: command register value to write
> + **/
> +static s32 ixgbe_msca_cmd(struct ixgbe_hw *hw, u32 cmd)
> +{
> +	u32 i;
> +
> +	IXGBE_WRITE_REG(hw, IXGBE_MSCA, cmd);
> +
> +	for (i = 0; i < IXGBE_MDIO_COMMAND_TIMEOUT; i++) {
> +		udelay(10);
> +		cmd = IXGBE_READ_REG(hw, IXGBE_MSCA);
> +		if (!(cmd & IXGBE_MSCA_MDI_COMMAND))
> +			return 0;
> +	}
> +
> +	return -ETIMEDOUT;

Please consider using readx_poll_timeout()

> +}
> +
> +/**
> + *  ixgbe_msca - Use device_id to figure out if MDIO bus is shared between MACs.
> + *  The embedded x550(s) in the C3000 line of SoCs only have a single mii_bus
> + *  shared between all MACs, and that introduces some new mask flags that need
> + *  to be passed to the *_swfw_sync callbacks.
> + *  @hw: pointer to hardware structure
> + **/
> +static bool ixgbe_is_shared_mii(struct ixgbe_hw *hw)
> +{
> +	switch (hw->device_id) {
> +	case IXGBE_DEV_ID_X550EM_A_KR:
> +	case IXGBE_DEV_ID_X550EM_A_KR_L:
> +	case IXGBE_DEV_ID_X550EM_A_SFP_N:
> +	case IXGBE_DEV_ID_X550EM_A_SGMII:
> +	case IXGBE_DEV_ID_X550EM_A_SGMII_L:
> +	case IXGBE_DEV_ID_X550EM_A_10G_T:
> +	case IXGBE_DEV_ID_X550EM_A_SFP:
> +	case IXGBE_DEV_ID_X550EM_A_1G_T:
> +	case IXGBE_DEV_ID_X550EM_A_1G_T_L:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + *  ixgbe_mii_bus_read - Read a clause 22/45 register
> + *  @hw: pointer to hardware structure
> + *  @addr: address
> + *  @regnum: register number
> + **/
> +static s32 ixgbe_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
> +{
> +	struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	u32 hwaddr, cmd, gssr = hw->phy.phy_semaphore_mask;
> +	s32 data;
> +
> +	if (ixgbe_is_shared_mii(hw)) {
> +		gssr |= IXGBE_GSSR_TOKEN_SM;
> +		if (hw->bus.lan_id)
> +			gssr |= IXGBE_GSSR_PHY1_SM;
> +		else
> +			gssr |= IXGBE_GSSR_PHY0_SM;
> +	}

Could you explain this shared bus a bit more.  If there is only one
physical MDIO bus, you should only register one bus with Linux. So i
would not normally expect to see ixgbe_is_shared_mii() sort of
statements into the read/write functions. That tends to happen at the
point you are registering the MDIO bus, and when looking for the MACs
PHY on the bus.

    Andrew

Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

[Steve Douthit]

On 11/30/18 8:21 AM, Andrew Lunn wrote:
> Hi Steve
> 
> Cool to see another interface being made DSA capable.
> 
>> +/**
>> + *  ixgbe_msca - Write the command register and poll for completion/timeout
>> + *  @hw: pointer to hardware structure
>> + *  @cmd: command register value to write
>> + **/
>> +static s32 ixgbe_msca_cmd(struct ixgbe_hw *hw, u32 cmd)
>> +{
>> +	u32 i;
>> +
>> +	IXGBE_WRITE_REG(hw, IXGBE_MSCA, cmd);
>> +
>> +	for (i = 0; i < IXGBE_MDIO_COMMAND_TIMEOUT; i++) {
>> +		udelay(10);
>> +		cmd = IXGBE_READ_REG(hw, IXGBE_MSCA);
>> +		if (!(cmd & IXGBE_MSCA_MDI_COMMAND))
>> +			return 0;
>> +	}
>> +
>> +	return -ETIMEDOUT;
> 
> Please consider using readx_poll_timeout()

OK

>> +}
>> +
>> +/**
>> + *  ixgbe_msca - Use device_id to figure out if MDIO bus is shared between MACs.
>> + *  The embedded x550(s) in the C3000 line of SoCs only have a single mii_bus
>> + *  shared between all MACs, and that introduces some new mask flags that need
>> + *  to be passed to the *_swfw_sync callbacks.
>> + *  @hw: pointer to hardware structure
>> + **/
>> +static bool ixgbe_is_shared_mii(struct ixgbe_hw *hw)
>> +{
>> +	switch (hw->device_id) {
>> +	case IXGBE_DEV_ID_X550EM_A_KR:
>> +	case IXGBE_DEV_ID_X550EM_A_KR_L:
>> +	case IXGBE_DEV_ID_X550EM_A_SFP_N:
>> +	case IXGBE_DEV_ID_X550EM_A_SGMII:
>> +	case IXGBE_DEV_ID_X550EM_A_SGMII_L:
>> +	case IXGBE_DEV_ID_X550EM_A_10G_T:
>> +	case IXGBE_DEV_ID_X550EM_A_SFP:
>> +	case IXGBE_DEV_ID_X550EM_A_1G_T:
>> +	case IXGBE_DEV_ID_X550EM_A_1G_T_L:
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +/**
>> + *  ixgbe_mii_bus_read - Read a clause 22/45 register
>> + *  @hw: pointer to hardware structure
>> + *  @addr: address
>> + *  @regnum: register number
>> + **/
>> +static s32 ixgbe_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
>> +{
>> +	struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
>> +	struct ixgbe_hw *hw = &adapter->hw;
>> +	u32 hwaddr, cmd, gssr = hw->phy.phy_semaphore_mask;
>> +	s32 data;
>> +
>> +	if (ixgbe_is_shared_mii(hw)) {
>> +		gssr |= IXGBE_GSSR_TOKEN_SM;
>> +		if (hw->bus.lan_id)
>> +			gssr |= IXGBE_GSSR_PHY1_SM;
>> +		else
>> +			gssr |= IXGBE_GSSR_PHY0_SM;
>> +	}
> 
> Could you explain this shared bus a bit more.  If there is only one
> physical MDIO bus, you should only register one bus with Linux. So i
> would not normally expect to see ixgbe_is_shared_mii() sort of
> statements into the read/write functions. That tends to happen at the
> point you are registering the MDIO bus, and when looking for the MACs
> PHY on the bus.

Yep, registering multiple interfaces is wrong.  The first board I tested
against only had a single MAC enabled (they can be disabled/hidden via
straps) so it just happened to work.  Then I moved to a board with all
four interfaces enabled and found this.

The Intel C3xxx family of SoCs have up to four ixgbe MACs.  These are
structured as two devices of two functions each on fixed internal root
ports.

from lspci:
<snip>
            +-16.0-[05]--+-00.0
            |            \-00.1
            +-17.0-[06]--+-00.0
            |            \-00.1
<snip>

Each of those MACs exposes the same set of MDIO control registers as if
there are four interface, but there's really only a single bus in the
hardware for these SoCs.

How about I change:
1) ixgbe_is_shared_mii() -> ixgbe_is_x550em_a()
2) ixgbe_mii_bus_(read|write) -> ixgbe_x550em_a_mii_bus_(read|write) and
    update the phy_semaphore_mask unconditionally
3) Only have ixgbe_mii_bus_init() register something for the x550em_a
    devices for now, and leave handling other platforms for the future.
4) Have ixgbe_mii_bus_init() register only a single mdiobus for func 0
    of the device downstream of the 00:16.0 root port, or if that's been
    disabled then func 0 downstream of the 00:17.0 root port.  All of the
    devices on bus 0 of the SoC are at fixed device/function locations.

Would that be acceptable?

Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

[Andrew Lunn]

> Yep, registering multiple interfaces is wrong.  The first board I tested
> against only had a single MAC enabled (they can be disabled/hidden via
> straps) so it just happened to work.

Hi Steve

Can you hide any/all via straps, or is 00.0 always guaranteed to
exist?
 
> The Intel C3xxx family of SoCs have up to four ixgbe MACs.  These are
> structured as two devices of two functions each on fixed internal root
> ports.
> 
> from lspci:
> <snip>
>             +-16.0-[05]--+-00.0
>             |            \-00.1
>             +-17.0-[06]--+-00.0
>             |            \-00.1
> <snip>

Is there any other hardware resource which is shared between the MAC
interfaces? I'm just wondering if the driver has already solved this
once. Is there an EEPROM per interface for the MAC address, or one
shared EEPROM?

Ah, how about using the 'cards_found' found variable. It is not
perfect, in that it is not decremented in ixgb_remove(), and i wonder
about race conditions since there does not appear to be any lock when
it is incremented. But if cards_found == 0, register the MDIO bus.

   Andrew

Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

[Steve Douthit]

On 11/30/18 11:34 AM, Andrew Lunn wrote:
>> Yep, registering multiple interfaces is wrong.  The first board I tested
>> against only had a single MAC enabled (they can be disabled/hidden via
>> straps) so it just happened to work.
> 
> Hi Steve
> 
> Can you hide any/all via straps, or is 00.0 always guaranteed to
> exist?

You can hide all the devices, but if function 1 is enabled then function
0 must also be enabled, so not all combinations are valid.

>> The Intel C3xxx family of SoCs have up to four ixgbe MACs.  These are
>> structured as two devices of two functions each on fixed internal root
>> ports.
>>
>> from lspci:
>> <snip>
>>              +-16.0-[05]--+-00.0
>>              |            \-00.1
>>              +-17.0-[06]--+-00.0
>>              |            \-00.1
>> <snip>
> 
> Is there any other hardware resource which is shared between the MAC
> interfaces? I'm just wondering if the driver has already solved this
> once. Is there an EEPROM per interface for the MAC address, or one
> shared EEPROM?

NVM is shared, it's actually the same SPI flash that holds the BIOS for
this SoC.  Access is serialized by the swfw_sync semaphore.  I think the
device firmware is automagically handling offset translation.

I don't think that helps for this case.

There might be a better match for shared resources, but nothing springs
to mind.

> Ah, how about using the 'cards_found' found variable. It is not
> perfect, in that it is not decremented in ixgb_remove(), and i wonder
> about race conditions since there does not appear to be any lock when
> it is incremented. But if cards_found == 0, register the MDIO bus.

'cards_found' doesn't exist for the ixgbe driver.  I could add it and
fix the race/decrement issues you mention, but it'd have to be a device
type specific count.  It's still possible there are other non-x550em_a
ixgbe devices in external PCIe slots that have different resource
sharing setups.

It's still a lighter weight solution than poking around the parent bus
so I'll add a 'x550em_a_devs_found' counter to v2.

Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

[Florian Fainelli]

On 11/30/2018 9:34 AM, Steve Douthit wrote:
> On 11/30/18 11:34 AM, Andrew Lunn wrote:
>>> Yep, registering multiple interfaces is wrong.  The first board I tested
>>> against only had a single MAC enabled (they can be disabled/hidden via
>>> straps) so it just happened to work.
>>
>> Hi Steve
>>
>> Can you hide any/all via straps, or is 00.0 always guaranteed to
>> exist?
> 
> You can hide all the devices, but if function 1 is enabled then function
> 0 must also be enabled, so not all combinations are valid.
> 
>>> The Intel C3xxx family of SoCs have up to four ixgbe MACs.  These are
>>> structured as two devices of two functions each on fixed internal root
>>> ports.
>>>
>>> from lspci:
>>> <snip>
>>>              +-16.0-[05]--+-00.0
>>>              |            \-00.1
>>>              +-17.0-[06]--+-00.0
>>>              |            \-00.1
>>> <snip>
>>
>> Is there any other hardware resource which is shared between the MAC
>> interfaces? I'm just wondering if the driver has already solved this
>> once. Is there an EEPROM per interface for the MAC address, or one
>> shared EEPROM?
> 
> NVM is shared, it's actually the same SPI flash that holds the BIOS for
> this SoC.  Access is serialized by the swfw_sync semaphore.  I think the
> device firmware is automagically handling offset translation.
> 
> I don't think that helps for this case.
> 
> There might be a better match for shared resources, but nothing springs
> to mind.
> 
>> Ah, how about using the 'cards_found' found variable. It is not
>> perfect, in that it is not decremented in ixgb_remove(), and i wonder
>> about race conditions since there does not appear to be any lock when
>> it is incremented. But if cards_found == 0, register the MDIO bus.
> 
> 'cards_found' doesn't exist for the ixgbe driver.  I could add it and
> fix the race/decrement issues you mention, but it'd have to be a device
> type specific count.  It's still possible there are other non-x550em_a
> ixgbe devices in external PCIe slots that have different resource
> sharing setups.
> 
> It's still a lighter weight solution than poking around the parent bus
> so I'll add a 'x550em_a_devs_found' counter to v2.
> 

If the MDIO bus block is hardwired to function 0, would it be acceptable
to walk the PCI bus hierarchy from any driver's probe routine where PCI
function != 0 and see if it is claimed/enabled already, and if so, skip
registering the MDIO bus entirely?

There is also possibly a problem if you have a shared MDIO device, and
you turn off/power off the PCI function that does provide it. How can we
make sure it remains alive for other functions to use it?
-- 
Florian

Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

[Steve Douthit]

On 11/30/18 12:43 PM, Florian Fainelli wrote:
> 
> 
> On 11/30/2018 9:34 AM, Steve Douthit wrote:
>> On 11/30/18 11:34 AM, Andrew Lunn wrote:
>>>> Yep, registering multiple interfaces is wrong.  The first board I tested
>>>> against only had a single MAC enabled (they can be disabled/hidden via
>>>> straps) so it just happened to work.
>>>
>>> Hi Steve
>>>
>>> Can you hide any/all via straps, or is 00.0 always guaranteed to
>>> exist?
>>
>> You can hide all the devices, but if function 1 is enabled then function
>> 0 must also be enabled, so not all combinations are valid.
>>
>>>> The Intel C3xxx family of SoCs have up to four ixgbe MACs.  These are
>>>> structured as two devices of two functions each on fixed internal root
>>>> ports.
>>>>
>>>> from lspci:
>>>> <snip>
>>>>               +-16.0-[05]--+-00.0
>>>>               |            \-00.1
>>>>               +-17.0-[06]--+-00.0
>>>>               |            \-00.1
>>>> <snip>
>>>
>>> Is there any other hardware resource which is shared between the MAC
>>> interfaces? I'm just wondering if the driver has already solved this
>>> once. Is there an EEPROM per interface for the MAC address, or one
>>> shared EEPROM?
>>
>> NVM is shared, it's actually the same SPI flash that holds the BIOS for
>> this SoC.  Access is serialized by the swfw_sync semaphore.  I think the
>> device firmware is automagically handling offset translation.
>>
>> I don't think that helps for this case.
>>
>> There might be a better match for shared resources, but nothing springs
>> to mind.
>>
>>> Ah, how about using the 'cards_found' found variable. It is not
>>> perfect, in that it is not decremented in ixgb_remove(), and i wonder
>>> about race conditions since there does not appear to be any lock when
>>> it is incremented. But if cards_found == 0, register the MDIO bus.
>>
>> 'cards_found' doesn't exist for the ixgbe driver.  I could add it and
>> fix the race/decrement issues you mention, but it'd have to be a device
>> type specific count.  It's still possible there are other non-x550em_a
>> ixgbe devices in external PCIe slots that have different resource
>> sharing setups.
>>
>> It's still a lighter weight solution than poking around the parent bus
>> so I'll add a 'x550em_a_devs_found' counter to v2.
>>
> 
> If the MDIO bus block is hardwired to function 0, would it be acceptable
> to walk the PCI bus hierarchy from any driver's probe routine where PCI
> function != 0 and see if it is claimed/enabled already, and if so, skip
> registering the MDIO bus entirely?

It's not really hardwired to any function at the hardware level AFAICT.
Unless by 'hardwired' you mean we always register it to the same devfn?
Walking the bus to find the lowest numbered devfn was my original
suggestion.  I'm good with a PCI walk or a counter to choose which MAC
registers the MDIO bus.  Let me know what the consensus is.

> There is also possibly a problem if you have a shared MDIO device, and
> you turn off/power off the PCI function that does provide it. How can we
> make sure it remains alive for other functions to use it?

I'm not sure this is an issue.  I wouldn't expect the other MACs to lose
the ability to talk to their PHYs.  Those callbacks weren't changed to
use the registered MDIO bus.

Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

[Andrew Lunn]

> 'cards_found' doesn't exist for the ixgbe driver.

Agh, sorry, i was looking at ixgb, not ixgbe.

     Andrew