Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1433234 > unrolled thread

[RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

Started byJon Mason <jon.mason@broadcom.com>
First post2016-06-28 21:40 +0200
Last post2016-06-29 22:40 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file Jon Mason <jon.mason@broadcom.com> - 2016-06-28 21:40 +0200
    Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a  separate file Andrew Lunn <andrew@lunn.ch> - 2016-06-28 22:10 +0200
    Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a  separate file Andrew Lunn <andrew@lunn.ch> - 2016-06-29 16:20 +0200
      Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a  separate file Florian Fainelli <f.fainelli@gmail.com> - 2016-06-29 20:40 +0200
        Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a  separate file Andrew Lunn <andrew@lunn.ch> - 2016-06-29 20:50 +0200
          Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a  separate file Jon Mason <jon.mason@broadcom.com> - 2016-06-29 22:10 +0200
            Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a  separate file Andrew Lunn <andrew@lunn.ch> - 2016-06-29 22:20 +0200
              Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a  separate file Jon Mason <jon.mason@broadcom.com> - 2016-06-29 22:40 +0200

#1433234 — [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

FromJon Mason <jon.mason@broadcom.com>
Date2016-06-28 21:40 +0200
Subject[RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file
Message-ID<rP71w-2K4-5@gated-at.bofh.it>
Move the BCMA MDIO phy into a separate file, as it is very tightly
coupled with the BCMA bus.  This will help with the upcoming BCMA
removal from the bgmac driver.  Optimally, this should be moved into
phy drivers, but it is too tightly coupled with the bgmac driver to
effectively move it without more changes to the driver.

Note: the phy_reset was intentionally removed, as the mdio phy subsystem
automatically resets the phy if a reset function pointer is present.  In
addition to the moving of the driver, this reset function is added.

Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
 drivers/net/ethernet/broadcom/Makefile          |   2 +-
 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 264 ++++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bgmac.c           | 246 +++-------------------
 drivers/net/ethernet/broadcom/bgmac.h           |   3 +
 4 files changed, 298 insertions(+), 217 deletions(-)
 create mode 100644 drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c

diff --git a/drivers/net/ethernet/broadcom/Makefile b/drivers/net/ethernet/broadcom/Makefile
index 00584d7..f559794 100644
--- a/drivers/net/ethernet/broadcom/Makefile
+++ b/drivers/net/ethernet/broadcom/Makefile
@@ -10,6 +10,6 @@ obj-$(CONFIG_CNIC) += cnic.o
 obj-$(CONFIG_BNX2X) += bnx2x/
 obj-$(CONFIG_SB1250_MAC) += sb1250-mac.o
 obj-$(CONFIG_TIGON3) += tg3.o
-obj-$(CONFIG_BGMAC) += bgmac.o
+obj-$(CONFIG_BGMAC) += bgmac.o bgmac-bcma-mdio.o
 obj-$(CONFIG_SYSTEMPORT) += bcmsysport.o
 obj-$(CONFIG_BNXT) += bnxt/
diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
new file mode 100644
index 0000000..1e65349
--- /dev/null
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c
@@ -0,0 +1,264 @@
+/*
+ * Driver for (BCM4706)? GBit MAC core on BCMA bus.
+ *
+ * Copyright (C) 2012 Rafał Miłecki <zajec5@gmail.com>
+ *
+ * Licensed under the GNU/GPL. See COPYING for details.
+ */
+
+#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
+
+#include <linux/bcma/bcma.h>
+#include <linux/brcmphy.h>
+#include "bgmac.h"
+
+struct bcma_mdio {
+	struct bcma_device *core;
+	u8 phyaddr;
+};
+
+static bool bcma_mdio_wait_value(struct bcma_device *core, u16 reg, u32 mask,
+				 u32 value, int timeout)
+{
+	u32 val;
+	int i;
+
+	for (i = 0; i < timeout / 10; i++) {
+		val = bcma_read32(core, reg);
+		if ((val & mask) == value)
+			return true;
+		udelay(10);
+	}
+	dev_err(&core->dev, "Timeout waiting for reg 0x%X\n", reg);
+	return false;
+}
+
+/**************************************************
+ * PHY ops
+ **************************************************/
+
+static u16 bcma_mdio_phy_read(struct bcma_mdio *bcma_mdio, u8 phyaddr, u8 reg)
+{
+	struct bcma_device *core;
+	u16 phy_access_addr;
+	u16 phy_ctl_addr;
+	u32 tmp;
+
+	BUILD_BUG_ON(BGMAC_PA_DATA_MASK != BCMA_GMAC_CMN_PA_DATA_MASK);
+	BUILD_BUG_ON(BGMAC_PA_ADDR_MASK != BCMA_GMAC_CMN_PA_ADDR_MASK);
+	BUILD_BUG_ON(BGMAC_PA_ADDR_SHIFT != BCMA_GMAC_CMN_PA_ADDR_SHIFT);
+	BUILD_BUG_ON(BGMAC_PA_REG_MASK != BCMA_GMAC_CMN_PA_REG_MASK);
+	BUILD_BUG_ON(BGMAC_PA_REG_SHIFT != BCMA_GMAC_CMN_PA_REG_SHIFT);
+	BUILD_BUG_ON(BGMAC_PA_WRITE != BCMA_GMAC_CMN_PA_WRITE);
+	BUILD_BUG_ON(BGMAC_PA_START != BCMA_GMAC_CMN_PA_START);
+	BUILD_BUG_ON(BGMAC_PC_EPA_MASK != BCMA_GMAC_CMN_PC_EPA_MASK);
+	BUILD_BUG_ON(BGMAC_PC_MCT_MASK != BCMA_GMAC_CMN_PC_MCT_MASK);
+	BUILD_BUG_ON(BGMAC_PC_MCT_SHIFT != BCMA_GMAC_CMN_PC_MCT_SHIFT);
+	BUILD_BUG_ON(BGMAC_PC_MTE != BCMA_GMAC_CMN_PC_MTE);
+
+	if (bcma_mdio->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
+		core = bcma_mdio->core->bus->drv_gmac_cmn.core;
+		phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
+		phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
+	} else {
+		core = bcma_mdio->core;
+		phy_access_addr = BGMAC_PHY_ACCESS;
+		phy_ctl_addr = BGMAC_PHY_CNTL;
+	}
+
+	tmp = bcma_read32(core, phy_ctl_addr);
+	tmp &= ~BGMAC_PC_EPA_MASK;
+	tmp |= phyaddr;
+	bcma_write32(core, phy_ctl_addr, tmp);
+
+	tmp = BGMAC_PA_START;
+	tmp |= phyaddr << BGMAC_PA_ADDR_SHIFT;
+	tmp |= reg << BGMAC_PA_REG_SHIFT;
+	bcma_write32(core, phy_access_addr, tmp);
+
+	if (!bcma_mdio_wait_value(core, phy_access_addr, BGMAC_PA_START, 0,
+				  1000)) {
+		dev_err(&core->dev, "Reading PHY %d register 0x%X failed\n",
+			phyaddr, reg);
+		return 0xffff;
+	}
+
+	return bcma_read32(core, phy_access_addr) & BGMAC_PA_DATA_MASK;
+}
+
+/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphywr */
+static int bcma_mdio_phy_write(struct bcma_mdio *bcma_mdio, u8 phyaddr, u8 reg,
+			       u16 value)
+{
+	struct bcma_device *core;
+	u16 phy_access_addr;
+	u16 phy_ctl_addr;
+	u32 tmp;
+
+	if (bcma_mdio->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
+		core = bcma_mdio->core->bus->drv_gmac_cmn.core;
+		phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
+		phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
+	} else {
+		core = bcma_mdio->core;
+		phy_access_addr = BGMAC_PHY_ACCESS;
+		phy_ctl_addr = BGMAC_PHY_CNTL;
+	}
+
+	tmp = bcma_read32(core, phy_ctl_addr);
+	tmp &= ~BGMAC_PC_EPA_MASK;
+	tmp |= phyaddr;
+	bcma_write32(core, phy_ctl_addr, tmp);
+
+	bcma_write32(bcma_mdio->core, BGMAC_INT_STATUS, BGMAC_IS_MDIO);
+	if (bcma_read32(bcma_mdio->core, BGMAC_INT_STATUS) & BGMAC_IS_MDIO)
+		dev_warn(&core->dev, "Error setting MDIO int\n");
+
+	tmp = BGMAC_PA_START;
+	tmp |= BGMAC_PA_WRITE;
+	tmp |= phyaddr << BGMAC_PA_ADDR_SHIFT;
+	tmp |= reg << BGMAC_PA_REG_SHIFT;
+	tmp |= value;
+	bcma_write32(core, phy_access_addr, tmp);
+
+	if (!bcma_mdio_wait_value(core, phy_access_addr, BGMAC_PA_START, 0,
+				  1000)) {
+		dev_err(&core->dev, "Writing to PHY %d register 0x%X failed\n",
+			phyaddr, reg);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyinit */
+static void bcma_mdio_phy_init(struct bcma_mdio *bcma_mdio)
+{
+	struct bcma_chipinfo *ci = &bcma_mdio->core->bus->chipinfo;
+	u8 i;
+
+	if (ci->id == BCMA_CHIP_ID_BCM5356) {
+		for (i = 0; i < 5; i++) {
+			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x008b);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x15, 0x0100);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x12, 0x2aaa);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);
+		}
+	}
+	if ((ci->id == BCMA_CHIP_ID_BCM5357 && ci->pkg != 10) ||
+	    (ci->id == BCMA_CHIP_ID_BCM4749 && ci->pkg != 10) ||
+	    (ci->id == BCMA_CHIP_ID_BCM53572 && ci->pkg != 9)) {
+		struct bcma_drv_cc *cc = &bcma_mdio->core->bus->drv_cc;
+
+		bcma_chipco_chipctl_maskset(cc, 2, ~0xc0000000, 0);
+		bcma_chipco_chipctl_maskset(cc, 4, ~0x80000000, 0);
+		for (i = 0; i < 5; i++) {
+			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x5284);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x0010);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x5296);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x1073);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x9073);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x52b6);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x9273);
+			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);
+		}
+	}
+}
+
+/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyreset */
+static int bcma_mdio_phy_reset(struct mii_bus *bus)
+{
+	struct bcma_mdio *bcma_mdio = bus->priv;
+	u8 phyaddr = bcma_mdio->phyaddr;
+
+	if (bcma_mdio->phyaddr == BGMAC_PHY_NOREGS)
+		return 0;
+
+	bcma_mdio_phy_write(bcma_mdio, phyaddr, MII_BMCR, BMCR_RESET);
+	udelay(100);
+	if (bcma_mdio_phy_read(bcma_mdio, phyaddr, MII_BMCR) & BMCR_RESET)
+		dev_err(&bcma_mdio->core->dev, "PHY reset failed\n");
+	bcma_mdio_phy_init(bcma_mdio);
+
+	return 0;
+}
+
+/**************************************************
+ * MII
+ **************************************************/
+
+static int bcma_mdio_mii_read(struct mii_bus *bus, int mii_id, int regnum)
+{
+	return bcma_mdio_phy_read(bus->priv, mii_id, regnum);
+}
+
+static int bcma_mdio_mii_write(struct mii_bus *bus, int mii_id, int regnum,
+			       u16 value)
+{
+	return bcma_mdio_phy_write(bus->priv, mii_id, regnum, value);
+}
+
+struct mii_bus *bcma_mdio_mii_register(struct bcma_device *core, u8 phyaddr)
+{
+	struct bcma_mdio *bcma_mdio;
+	struct mii_bus *mii_bus;
+	int err;
+
+	bcma_mdio = kzalloc(sizeof(*bcma_mdio), GFP_KERNEL);
+	if (!bcma_mdio)
+		return ERR_PTR(-ENOMEM);
+
+	mii_bus = mdiobus_alloc();
+	if (!mii_bus) {
+		err = -ENOMEM;
+		goto err;
+	}
+
+	mii_bus->name = "bcma_mdio mii bus";
+	sprintf(mii_bus->id, "%s-%d-%d", "bcma_mdio", core->bus->num,
+		core->core_unit);
+	mii_bus->priv = bcma_mdio;
+	mii_bus->read = bcma_mdio_mii_read;
+	mii_bus->write = bcma_mdio_mii_write;
+	mii_bus->reset = bcma_mdio_phy_reset;
+	mii_bus->parent = &core->dev;
+	mii_bus->phy_mask = ~(1 << phyaddr);
+
+	bcma_mdio->core = core;
+	bcma_mdio->phyaddr = phyaddr;
+
+	err = mdiobus_register(mii_bus);
+	if (err) {
+		dev_err(&core->dev, "Registration of mii bus failed\n");
+		goto err_free_bus;
+	}
+
+	return mii_bus;
+
+err_free_bus:
+	mdiobus_free(mii_bus);
+err:
+	kfree(bcma_mdio);
+	return ERR_PTR(err);
+}
+
+void bcma_mdio_mii_unregister(struct mii_bus *mii_bus)
+{
+	struct bcma_mdio *bcma_mdio;
+
+	if (!mii_bus)
+		return;
+
+	bcma_mdio = mii_bus->priv;
+
+	mdiobus_unregister(mii_bus);
+	mdiobus_free(mii_bus);
+	kfree(bcma_mdio);
+}
+
+MODULE_AUTHOR("Rafał Miłecki");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index b5c35bc..88b9513 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -755,150 +755,6 @@ error:
 	return err;
 }
 
-/**************************************************
- * PHY ops
- **************************************************/
-
-static u16 bgmac_phy_read(struct bgmac *bgmac, u8 phyaddr, u8 reg)
-{
-	struct bcma_device *core;
-	u16 phy_access_addr;
-	u16 phy_ctl_addr;
-	u32 tmp;
-
-	BUILD_BUG_ON(BGMAC_PA_DATA_MASK != BCMA_GMAC_CMN_PA_DATA_MASK);
-	BUILD_BUG_ON(BGMAC_PA_ADDR_MASK != BCMA_GMAC_CMN_PA_ADDR_MASK);
-	BUILD_BUG_ON(BGMAC_PA_ADDR_SHIFT != BCMA_GMAC_CMN_PA_ADDR_SHIFT);
-	BUILD_BUG_ON(BGMAC_PA_REG_MASK != BCMA_GMAC_CMN_PA_REG_MASK);
-	BUILD_BUG_ON(BGMAC_PA_REG_SHIFT != BCMA_GMAC_CMN_PA_REG_SHIFT);
-	BUILD_BUG_ON(BGMAC_PA_WRITE != BCMA_GMAC_CMN_PA_WRITE);
-	BUILD_BUG_ON(BGMAC_PA_START != BCMA_GMAC_CMN_PA_START);
-	BUILD_BUG_ON(BGMAC_PC_EPA_MASK != BCMA_GMAC_CMN_PC_EPA_MASK);
-	BUILD_BUG_ON(BGMAC_PC_MCT_MASK != BCMA_GMAC_CMN_PC_MCT_MASK);
-	BUILD_BUG_ON(BGMAC_PC_MCT_SHIFT != BCMA_GMAC_CMN_PC_MCT_SHIFT);
-	BUILD_BUG_ON(BGMAC_PC_MTE != BCMA_GMAC_CMN_PC_MTE);
-
-	if (bgmac->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
-		core = bgmac->core->bus->drv_gmac_cmn.core;
-		phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
-		phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
-	} else {
-		core = bgmac->core;
-		phy_access_addr = BGMAC_PHY_ACCESS;
-		phy_ctl_addr = BGMAC_PHY_CNTL;
-	}
-
-	tmp = bcma_read32(core, phy_ctl_addr);
-	tmp &= ~BGMAC_PC_EPA_MASK;
-	tmp |= phyaddr;
-	bcma_write32(core, phy_ctl_addr, tmp);
-
-	tmp = BGMAC_PA_START;
-	tmp |= phyaddr << BGMAC_PA_ADDR_SHIFT;
-	tmp |= reg << BGMAC_PA_REG_SHIFT;
-	bcma_write32(core, phy_access_addr, tmp);
-
-	if (!bgmac_wait_value(core, phy_access_addr, BGMAC_PA_START, 0, 1000)) {
-		dev_err(bgmac->dev, "Reading PHY %d register 0x%X failed\n",
-			phyaddr, reg);
-		return 0xffff;
-	}
-
-	return bcma_read32(core, phy_access_addr) & BGMAC_PA_DATA_MASK;
-}
-
-/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphywr */
-static int bgmac_phy_write(struct bgmac *bgmac, u8 phyaddr, u8 reg, u16 value)
-{
-	struct bcma_device *core;
-	u16 phy_access_addr;
-	u16 phy_ctl_addr;
-	u32 tmp;
-
-	if (bgmac->core->id.id == BCMA_CORE_4706_MAC_GBIT) {
-		core = bgmac->core->bus->drv_gmac_cmn.core;
-		phy_access_addr = BCMA_GMAC_CMN_PHY_ACCESS;
-		phy_ctl_addr = BCMA_GMAC_CMN_PHY_CTL;
-	} else {
-		core = bgmac->core;
-		phy_access_addr = BGMAC_PHY_ACCESS;
-		phy_ctl_addr = BGMAC_PHY_CNTL;
-	}
-
-	tmp = bcma_read32(core, phy_ctl_addr);
-	tmp &= ~BGMAC_PC_EPA_MASK;
-	tmp |= phyaddr;
-	bcma_write32(core, phy_ctl_addr, tmp);
-
-	bgmac_write(bgmac, BGMAC_INT_STATUS, BGMAC_IS_MDIO);
-	if (bgmac_read(bgmac, BGMAC_INT_STATUS) & BGMAC_IS_MDIO)
-		dev_warn(bgmac->dev, "Error setting MDIO int\n");
-
-	tmp = BGMAC_PA_START;
-	tmp |= BGMAC_PA_WRITE;
-	tmp |= phyaddr << BGMAC_PA_ADDR_SHIFT;
-	tmp |= reg << BGMAC_PA_REG_SHIFT;
-	tmp |= value;
-	bcma_write32(core, phy_access_addr, tmp);
-
-	if (!bgmac_wait_value(core, phy_access_addr, BGMAC_PA_START, 0, 1000)) {
-		dev_err(bgmac->dev, "Writing to PHY %d register 0x%X failed\n",
-			phyaddr, reg);
-		return -ETIMEDOUT;
-	}
-
-	return 0;
-}
-
-/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyinit */
-static void bgmac_phy_init(struct bgmac *bgmac)
-{
-	struct bcma_chipinfo *ci = &bgmac->core->bus->chipinfo;
-	struct bcma_drv_cc *cc = &bgmac->core->bus->drv_cc;
-	u8 i;
-
-	if (ci->id == BCMA_CHIP_ID_BCM5356) {
-		for (i = 0; i < 5; i++) {
-			bgmac_phy_write(bgmac, i, 0x1f, 0x008b);
-			bgmac_phy_write(bgmac, i, 0x15, 0x0100);
-			bgmac_phy_write(bgmac, i, 0x1f, 0x000f);
-			bgmac_phy_write(bgmac, i, 0x12, 0x2aaa);
-			bgmac_phy_write(bgmac, i, 0x1f, 0x000b);
-		}
-	}
-	if ((ci->id == BCMA_CHIP_ID_BCM5357 && ci->pkg != 10) ||
-	    (ci->id == BCMA_CHIP_ID_BCM4749 && ci->pkg != 10) ||
-	    (ci->id == BCMA_CHIP_ID_BCM53572 && ci->pkg != 9)) {
-		bcma_chipco_chipctl_maskset(cc, 2, ~0xc0000000, 0);
-		bcma_chipco_chipctl_maskset(cc, 4, ~0x80000000, 0);
-		for (i = 0; i < 5; i++) {
-			bgmac_phy_write(bgmac, i, 0x1f, 0x000f);
-			bgmac_phy_write(bgmac, i, 0x16, 0x5284);
-			bgmac_phy_write(bgmac, i, 0x1f, 0x000b);
-			bgmac_phy_write(bgmac, i, 0x17, 0x0010);
-			bgmac_phy_write(bgmac, i, 0x1f, 0x000f);
-			bgmac_phy_write(bgmac, i, 0x16, 0x5296);
-			bgmac_phy_write(bgmac, i, 0x17, 0x1073);
-			bgmac_phy_write(bgmac, i, 0x17, 0x9073);
-			bgmac_phy_write(bgmac, i, 0x16, 0x52b6);
-			bgmac_phy_write(bgmac, i, 0x17, 0x9273);
-			bgmac_phy_write(bgmac, i, 0x1f, 0x000b);
-		}
-	}
-}
-
-/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyreset */
-static void bgmac_phy_reset(struct bgmac *bgmac)
-{
-	if (bgmac->phyaddr == BGMAC_PHY_NOREGS)
-		return;
-
-	bgmac_phy_write(bgmac, bgmac->phyaddr, MII_BMCR, BMCR_RESET);
-	udelay(100);
-	if (bgmac_phy_read(bgmac, bgmac->phyaddr, MII_BMCR) & BMCR_RESET)
-		dev_err(bgmac->dev, "PHY reset failed\n");
-	bgmac_phy_init(bgmac);
-}
 
 /**************************************************
  * Chip ops
@@ -1155,7 +1011,8 @@ static void bgmac_chip_reset(struct bgmac *bgmac)
 	else
 		bgmac_set(bgmac, BGMAC_PHY_CNTL, BGMAC_PC_MTE);
 	bgmac_miiconfig(bgmac);
-	bgmac_phy_init(bgmac);
+	if (bgmac->mii_bus)
+		bgmac->mii_bus->reset(bgmac->mii_bus);
 
 	netdev_reset_queue(bgmac->net_dev);
 }
@@ -1532,17 +1389,6 @@ static const struct ethtool_ops bgmac_ethtool_ops = {
  * MII
  **************************************************/
 
-static int bgmac_mii_read(struct mii_bus *bus, int mii_id, int regnum)
-{
-	return bgmac_phy_read(bus->priv, mii_id, regnum);
-}
-
-static int bgmac_mii_write(struct mii_bus *bus, int mii_id, int regnum,
-			   u16 value)
-{
-	return bgmac_phy_write(bus->priv, mii_id, regnum, value);
-}
-
 static void bgmac_adjust_link(struct net_device *net_dev)
 {
 	struct bgmac *bgmac = netdev_priv(net_dev);
@@ -1567,7 +1413,7 @@ static void bgmac_adjust_link(struct net_device *net_dev)
 	}
 }
 
-static int bgmac_fixed_phy_register(struct bgmac *bgmac)
+static int bgmac_phy_connect_direct(struct bgmac *bgmac)
 {
 	struct fixed_phy_status fphy_status = {
 		.link = 1,
@@ -1593,70 +1439,24 @@ static int bgmac_fixed_phy_register(struct bgmac *bgmac)
 	return err;
 }
 
-static int bgmac_mii_register(struct bgmac *bgmac)
+static int bgmac_phy_connect(struct bgmac *bgmac)
 {
-	struct mii_bus *mii_bus;
 	struct phy_device *phy_dev;
 	char bus_id[MII_BUS_ID_SIZE + 3];
-	int err = 0;
-
-	if (bgmac_is_bcm4707_family(bgmac))
-		return bgmac_fixed_phy_register(bgmac);
-
-	mii_bus = mdiobus_alloc();
-	if (!mii_bus)
-		return -ENOMEM;
-
-	mii_bus->name = "bgmac mii bus";
-	sprintf(mii_bus->id, "%s-%d-%d", "bgmac", bgmac->core->bus->num,
-		bgmac->core->core_unit);
-	mii_bus->priv = bgmac;
-	mii_bus->read = bgmac_mii_read;
-	mii_bus->write = bgmac_mii_write;
-	mii_bus->parent = &bgmac->core->dev;
-	mii_bus->phy_mask = ~(1 << bgmac->phyaddr);
-
-	err = mdiobus_register(mii_bus);
-	if (err) {
-		dev_err(bgmac->dev, "Registration of mii bus failed\n");
-		goto err_free_bus;
-	}
-
-	bgmac->mii_bus = mii_bus;
 
 	/* Connect to the PHY */
-	snprintf(bus_id, sizeof(bus_id), PHY_ID_FMT, mii_bus->id,
+	snprintf(bus_id, sizeof(bus_id), PHY_ID_FMT, bgmac->mii_bus->id,
 		 bgmac->phyaddr);
 	phy_dev = phy_connect(bgmac->net_dev, bus_id, &bgmac_adjust_link,
 			      PHY_INTERFACE_MODE_MII);
 	if (IS_ERR(phy_dev)) {
 		dev_err(bgmac->dev, "PHY connecton failed\n");
-		err = PTR_ERR(phy_dev);
-		goto err_unregister_bus;
+		return PTR_ERR(phy_dev);
 	}
 
-	return err;
-
-err_unregister_bus:
-	mdiobus_unregister(mii_bus);
-err_free_bus:
-	mdiobus_free(mii_bus);
-	return err;
-}
-
-static void bgmac_mii_unregister(struct bgmac *bgmac)
-{
-	struct mii_bus *mii_bus = bgmac->mii_bus;
-
-	mdiobus_unregister(mii_bus);
-	mdiobus_free(mii_bus);
+	return 0;
 }
 
-/**************************************************
- * BCMA bus ops
- **************************************************/
-
-/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipattach */
 static int bgmac_probe(struct bcma_device *core)
 {
 	struct net_device *net_dev;
@@ -1777,9 +1577,6 @@ static int bgmac_probe(struct bcma_device *core)
 	if (bcm47xx_nvram_getenv("et0_no_txint", NULL, 0) == 0)
 		bgmac->int_mask &= ~BGMAC_IS_TX_MASK;
 
-	/* TODO: reset the external phy. Specs are needed */
-	bgmac_phy_reset(bgmac);
-
 	bgmac->has_robosw = !!(core->bus->sprom.boardflags_lo &
 			       BGMAC_BFL_ENETROBO);
 	if (bgmac->has_robosw)
@@ -1790,10 +1587,25 @@ static int bgmac_probe(struct bcma_device *core)
 
 	netif_napi_add(net_dev, &bgmac->napi, bgmac_poll, BGMAC_WEIGHT);
 
-	err = bgmac_mii_register(bgmac);
+	if (!bgmac_is_bcm4707_family(bgmac)) {
+		struct mii_bus *mii_bus;
+
+		mii_bus = bcma_mdio_mii_register(core, bgmac->phyaddr);
+		if (!IS_ERR(mii_bus)) {
+			err = PTR_ERR(mii_bus);
+			goto err_dma_free;
+		}
+
+		bgmac->mii_bus = mii_bus;
+	}
+
+	if (!bgmac->mii_bus)
+		err = bgmac_phy_connect_direct(bgmac);
+	else
+		err = bgmac_phy_connect(bgmac);
 	if (err) {
 		dev_err(bgmac->dev, "Cannot connect to phy\n");
-		goto err_dma_free;
+		goto err_mii_unregister;
 	}
 
 	net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
@@ -1803,18 +1615,19 @@ static int bgmac_probe(struct bcma_device *core)
 	err = register_netdev(bgmac->net_dev);
 	if (err) {
 		dev_err(bgmac->dev, "Cannot register net device\n");
-		goto err_mii_unregister;
+		goto err_phy_disconnect;
 	}
 
 	netif_carrier_off(net_dev);
 
 	return 0;
 
+err_phy_disconnect:
+	phy_disconnect(net_dev->phydev);
 err_mii_unregister:
-	bgmac_mii_unregister(bgmac);
+	bcma_mdio_mii_unregister(bgmac->mii_bus);
 err_dma_free:
 	bgmac_dma_free(bgmac);
-
 err_netdev_free:
 	bcma_set_drvdata(core, NULL);
 	free_netdev(net_dev);
@@ -1827,7 +1640,8 @@ static void bgmac_remove(struct bcma_device *core)
 	struct bgmac *bgmac = bcma_get_drvdata(core);
 
 	unregister_netdev(bgmac->net_dev);
-	bgmac_mii_unregister(bgmac);
+	phy_disconnect(bgmac->net_dev->phydev);
+	bcma_mdio_mii_unregister(bgmac->mii_bus);
 	netif_napi_del(&bgmac->napi);
 	bgmac_dma_free(bgmac);
 	bcma_set_drvdata(core, NULL);
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index fd20018..191e64a 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -456,6 +456,9 @@ struct bgmac {
 	bool loopback;
 };
 
+struct mii_bus *bcma_mdio_mii_register(struct bcma_device *core, u8 phyaddr);
+void bcma_mdio_mii_unregister(struct mii_bus *mii_bus);
+
 static inline u32 bgmac_read(struct bgmac *bgmac, u16 offset)
 {
 	return bcma_read32(bgmac->core, offset);
-- 
1.9.1

[toc] | [next] | [standalone]

#1433266 — Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

FromAndrew Lunn <andrew@lunn.ch>
Date2016-06-28 22:10 +0200
SubjectRe: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file
Message-ID<rP7ux-39z-13@gated-at.bofh.it>
In reply to#1433234
On Tue, Jun 28, 2016 at 03:34:40PM -0400, Jon Mason wrote:
> Move the BCMA MDIO phy into a separate file, as it is very tightly
> coupled with the BCMA bus.  This will help with the upcoming BCMA
> removal from the bgmac driver.  Optimally, this should be moved into
> phy drivers, but it is too tightly coupled with the bgmac driver to
> effectively move it without more changes to the driver.

It is quite common to have the MII bus driver as a sub driver of the
MAC driver, if they are tightly coupled. The MII drivers in
drivers/net/phy are all independent of the MAC driver and use a
different space in the register sets. This does not seem the case
here, so i think the split you have made is O.K, and i don't see a
need to move it into phy.

     Andrew

[toc] | [prev] | [next] | [standalone]

#1433788 — Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

FromAndrew Lunn <andrew@lunn.ch>
Date2016-06-29 16:20 +0200
SubjectRe: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file
Message-ID<rPovo-5ep-21@gated-at.bofh.it>
In reply to#1433234
Hi Jon

I know you are just refactoring code, but at some point it would be
good to take a closer look at this MDIO bus driver.

The MDIO bus driver should be generic, allowing access to all 32
addresses on the bus, if that makes sense. You could for example have
a B53 switch hanging off the MDIO bus.... The basic read/write
functions seem to allow this.

> +/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyinit */
> +static void bcma_mdio_phy_init(struct bcma_mdio *bcma_mdio)
> +{
> +	struct bcma_chipinfo *ci = &bcma_mdio->core->bus->chipinfo;
> +	u8 i;
> +
> +	if (ci->id == BCMA_CHIP_ID_BCM5356) {
> +		for (i = 0; i < 5; i++) {
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x008b);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x15, 0x0100);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x12, 0x2aaa);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);

It would be good to document what these writes are doing. Also, could
they be moved into the phy driver? Does the PHY implement an ID in
registers 2 and 3 that can be used to determine these are needed?

Also, why is it iterating over 5 PHYs? Is this actually a switch with
5 ports?

> +		}
> +	}
> +	if ((ci->id == BCMA_CHIP_ID_BCM5357 && ci->pkg != 10) ||
> +	    (ci->id == BCMA_CHIP_ID_BCM4749 && ci->pkg != 10) ||
> +	    (ci->id == BCMA_CHIP_ID_BCM53572 && ci->pkg != 9)) {
> +		struct bcma_drv_cc *cc = &bcma_mdio->core->bus->drv_cc;
> +
> +		bcma_chipco_chipctl_maskset(cc, 2, ~0xc0000000, 0);
> +		bcma_chipco_chipctl_maskset(cc, 4, ~0x80000000, 0);
> +		for (i = 0; i < 5; i++) {
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x5284);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x0010);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x5296);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x1073);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x9073);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x52b6);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x9273);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);

Same comment here.

> +/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyreset */
> +static int bcma_mdio_phy_reset(struct mii_bus *bus)
> +{
> +	struct bcma_mdio *bcma_mdio = bus->priv;
> +	u8 phyaddr = bcma_mdio->phyaddr;
> +
> +	if (bcma_mdio->phyaddr == BGMAC_PHY_NOREGS)
> +		return 0;
> +
> +	bcma_mdio_phy_write(bcma_mdio, phyaddr, MII_BMCR, BMCR_RESET);
> +	udelay(100);
> +	if (bcma_mdio_phy_read(bcma_mdio, phyaddr, MII_BMCR) & BMCR_RESET)
> +		dev_err(&bcma_mdio->core->dev, "PHY reset failed\n");
> +	bcma_mdio_phy_init(bcma_mdio);

This appears to be resetting one PHY. What about the others? Why not
put this in the PHY driver?

> +struct mii_bus *bcma_mdio_mii_register(struct bcma_device *core, u8 phyaddr)
> +{
> +	struct bcma_mdio *bcma_mdio;
> +	struct mii_bus *mii_bus;
> +	int err;
> +
> +	bcma_mdio = kzalloc(sizeof(*bcma_mdio), GFP_KERNEL);
> +	if (!bcma_mdio)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mii_bus = mdiobus_alloc();
> +	if (!mii_bus) {
> +		err = -ENOMEM;
> +		goto err;
> +	}
> +
> +	mii_bus->name = "bcma_mdio mii bus";
> +	sprintf(mii_bus->id, "%s-%d-%d", "bcma_mdio", core->bus->num,
> +		core->core_unit);
> +	mii_bus->priv = bcma_mdio;
> +	mii_bus->read = bcma_mdio_mii_read;
> +	mii_bus->write = bcma_mdio_mii_write;
> +	mii_bus->reset = bcma_mdio_phy_reset;
> +	mii_bus->parent = &core->dev;
> +	mii_bus->phy_mask = ~(1 << phyaddr);

Is there a need to limit this to just the one PHY?

> +
> +	bcma_mdio->core = core;
> +	bcma_mdio->phyaddr = phyaddr;
> +
> +	err = mdiobus_register(mii_bus);

Something which appears to be missing from the later patch which adds
device tree support. If you have a device node pointer, you should use
of_mdiobus_register() and document in the binding that PHYs should be
listed in the usual way.

       Andrew

[toc] | [prev] | [next] | [standalone]

#1433966 — Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

FromFlorian Fainelli <f.fainelli@gmail.com>
Date2016-06-29 20:40 +0200
SubjectRe: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file
Message-ID<rPsz0-7FB-35@gated-at.bofh.it>
In reply to#1433788
On 06/29/2016 07:13 AM, Andrew Lunn wrote:
> Hi Jon
> 
> I know you are just refactoring code, but at some point it would be
> good to take a closer look at this MDIO bus driver.
> 
> The MDIO bus driver should be generic, allowing access to all 32
> addresses on the bus, if that makes sense. You could for example have
> a B53 switch hanging off the MDIO bus.... The basic read/write
> functions seem to allow this.

That's the case on MIPS-based SoCs actually.

> 
>> +/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyinit */
>> +static void bcma_mdio_phy_init(struct bcma_mdio *bcma_mdio)
>> +{
>> +	struct bcma_chipinfo *ci = &bcma_mdio->core->bus->chipinfo;
>> +	u8 i;
>> +
>> +	if (ci->id == BCMA_CHIP_ID_BCM5356) {
>> +		for (i = 0; i < 5; i++) {
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x008b);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x15, 0x0100);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x12, 0x2aaa);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);
> 
> It would be good to document what these writes are doing. Also, could
> they be moved into the phy driver? Does the PHY implement an ID in
> registers 2 and 3 that can be used to determine these are needed?
> 
> Also, why is it iterating over 5 PHYs? Is this actually a switch with
> 5 ports?

Yes, this is applying workarounds to the integrated PHYs, the plan is to
move that to a proper PHY driver so we can get rid of that.

> 
>> +		}
>> +	}
>> +	if ((ci->id == BCMA_CHIP_ID_BCM5357 && ci->pkg != 10) ||
>> +	    (ci->id == BCMA_CHIP_ID_BCM4749 && ci->pkg != 10) ||
>> +	    (ci->id == BCMA_CHIP_ID_BCM53572 && ci->pkg != 9)) {
>> +		struct bcma_drv_cc *cc = &bcma_mdio->core->bus->drv_cc;
>> +
>> +		bcma_chipco_chipctl_maskset(cc, 2, ~0xc0000000, 0);
>> +		bcma_chipco_chipctl_maskset(cc, 4, ~0x80000000, 0);
>> +		for (i = 0; i < 5; i++) {
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x5284);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x0010);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x5296);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x1073);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x9073);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x52b6);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x9273);
>> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);
> 
> Same comment here.
> 
>> +/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyreset */
>> +static int bcma_mdio_phy_reset(struct mii_bus *bus)
>> +{
>> +	struct bcma_mdio *bcma_mdio = bus->priv;
>> +	u8 phyaddr = bcma_mdio->phyaddr;
>> +
>> +	if (bcma_mdio->phyaddr == BGMAC_PHY_NOREGS)
>> +		return 0;
>> +
>> +	bcma_mdio_phy_write(bcma_mdio, phyaddr, MII_BMCR, BMCR_RESET);
>> +	udelay(100);
>> +	if (bcma_mdio_phy_read(bcma_mdio, phyaddr, MII_BMCR) & BMCR_RESET)
>> +		dev_err(&bcma_mdio->core->dev, "PHY reset failed\n");
>> +	bcma_mdio_phy_init(bcma_mdio);
> 
> This appears to be resetting one PHY. What about the others? Why not
> put this in the PHY driver?

Probably should be there, keep in mind this driver was mostly reverse
engineered, from an internal code base which usually did not plan on
utilizing existing APIs.

> 
>> +struct mii_bus *bcma_mdio_mii_register(struct bcma_device *core, u8 phyaddr)
>> +{
>> +	struct bcma_mdio *bcma_mdio;
>> +	struct mii_bus *mii_bus;
>> +	int err;
>> +
>> +	bcma_mdio = kzalloc(sizeof(*bcma_mdio), GFP_KERNEL);
>> +	if (!bcma_mdio)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	mii_bus = mdiobus_alloc();
>> +	if (!mii_bus) {
>> +		err = -ENOMEM;
>> +		goto err;
>> +	}
>> +
>> +	mii_bus->name = "bcma_mdio mii bus";
>> +	sprintf(mii_bus->id, "%s-%d-%d", "bcma_mdio", core->bus->num,
>> +		core->core_unit);
>> +	mii_bus->priv = bcma_mdio;
>> +	mii_bus->read = bcma_mdio_mii_read;
>> +	mii_bus->write = bcma_mdio_mii_write;
>> +	mii_bus->reset = bcma_mdio_phy_reset;
>> +	mii_bus->parent = &core->dev;
>> +	mii_bus->phy_mask = ~(1 << phyaddr);
> 
> Is there a need to limit this to just the one PHY?

Usually this is only relevant for an external PHY, and Jon is just
moving code around without making functional changes to existing BCMA
code, so all of that can be changed at a later time.

> 
>> +
>> +	bcma_mdio->core = core;
>> +	bcma_mdio->phyaddr = phyaddr;
>> +
>> +	err = mdiobus_register(mii_bus);
> 
> Something which appears to be missing from the later patch which adds
> device tree support. If you have a device node pointer, you should use
> of_mdiobus_register() and document in the binding that PHYs should be
> listed in the usual way.

This is just moving the code around for now.

I don't think of_mdiobus_register() is going to be used at all, because
on the ARM-based BCM5301X SoCs that use Device Tree, we have yet to see
a platform which requires this MDIO bus driver, all PHYs are internal,
and hanging off the switch register access block.

On MIPS-based BCM53xx/47xx, we need this driver to drive the external
switches, but those are not Device Tree aware, nor is there a need to
make that happen.

And, to re-iterate all of your points are valid, but this is premature
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1433968 — Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

FromAndrew Lunn <andrew@lunn.ch>
Date2016-06-29 20:50 +0200
SubjectRe: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file
Message-ID<rPsIF-7IY-1@gated-at.bofh.it>
In reply to#1433966
On Wed, Jun 29, 2016 at 11:35:28AM -0700, Florian Fainelli wrote:
> On 06/29/2016 07:13 AM, Andrew Lunn wrote:
> > Hi Jon
> > 
> > I know you are just refactoring code, but at some point it would be
> > good to take a closer look at this MDIO bus driver.
 
> And, to re-iterate all of your points are valid, but this is premature

We agree then :-)

   Andrew

[toc] | [prev] | [next] | [standalone]

#1434024 — Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

FromJon Mason <jon.mason@broadcom.com>
Date2016-06-29 22:10 +0200
SubjectRe: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file
Message-ID<rPtY6-d1-9@gated-at.bofh.it>
In reply to#1433968
On Wed, Jun 29, 2016 at 2:46 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Jun 29, 2016 at 11:35:28AM -0700, Florian Fainelli wrote:
>> On 06/29/2016 07:13 AM, Andrew Lunn wrote:
>> > Hi Jon
>> >
>> > I know you are just refactoring code, but at some point it would be
>> > good to take a closer look at this MDIO bus driver.
>
>> And, to re-iterate all of your points are valid, but this is premature
>
> We agree then :-)
>
>    Andrew

I also agree with all of your points, but hope this is not something
that would prevent this patch series from being acceptable.  It could
be quite an effort to try and properly document the code and what it
is doing, get HW to verify the assumptions from the documentation (as
the MIPS HW was done by a different group, no hardware is available,
etc), relocate the code to the drivers/net/phy and add a standard
interface.  From your original comment, I believe that you agree that
this additional effort is not necessary at this point in time.

Thanks,
Jon

[toc] | [prev] | [next] | [standalone]

#1434033 — Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

FromAndrew Lunn <andrew@lunn.ch>
Date2016-06-29 22:20 +0200
SubjectRe: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file
Message-ID<rPu7M-go-25@gated-at.bofh.it>
In reply to#1434024
On Wed, Jun 29, 2016 at 04:08:20PM -0400, Jon Mason wrote:
> On Wed, Jun 29, 2016 at 2:46 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Wed, Jun 29, 2016 at 11:35:28AM -0700, Florian Fainelli wrote:
> >> On 06/29/2016 07:13 AM, Andrew Lunn wrote:
> >> > Hi Jon
> >> >
> >> > I know you are just refactoring code, but at some point it would be
> >> > good to take a closer look at this MDIO bus driver.
> >
> >> And, to re-iterate all of your points are valid, but this is premature
> >
> > We agree then :-)
> >
> >    Andrew
> 
> I also agree with all of your points, but hope this is not something
> that would prevent this patch series from being acceptable.

No, it is acceptable as is.

However, it would be nice the clean up the mess, especially if you are
planning on using this code with new platforms, not just legacy stuff
which is bit rotting.

      Andrew

[toc] | [prev] | [next] | [standalone]

#1434042 — Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

FromJon Mason <jon.mason@broadcom.com>
Date2016-06-29 22:40 +0200
SubjectRe: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file
Message-ID<rPur7-mO-7@gated-at.bofh.it>
In reply to#1434033
On Wed, Jun 29, 2016 at 4:15 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Jun 29, 2016 at 04:08:20PM -0400, Jon Mason wrote:
>> On Wed, Jun 29, 2016 at 2:46 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > On Wed, Jun 29, 2016 at 11:35:28AM -0700, Florian Fainelli wrote:
>> >> On 06/29/2016 07:13 AM, Andrew Lunn wrote:
>> >> > Hi Jon
>> >> >
>> >> > I know you are just refactoring code, but at some point it would be
>> >> > good to take a closer look at this MDIO bus driver.
>> >
>> >> And, to re-iterate all of your points are valid, but this is premature
>> >
>> > We agree then :-)
>> >
>> >    Andrew
>>
>> I also agree with all of your points, but hope this is not something
>> that would prevent this patch series from being acceptable.
>
> No, it is acceptable as is.
>
> However, it would be nice the clean up the mess, especially if you are
> planning on using this code with new platforms, not just legacy stuff
> which is bit rotting.

I don't believe we'll be using the BCMA infrastructure going forward.
We'll be doing our own MDIO Phys, adding them to the device tree, and
referencing them through the DT/OF method.  This is the current plan
for NSP, NS2, Cygnus, and other upcoming platforms.  If someone can
find an elegant way of splitting off the Phy code from the BCMA core,
then this can be better than it is now.  But, it will take someone
with docs, time, and hardware to give this code the audit and cleanup
it really needs.  BTW, my last comment isn't intended as a slight
against the authors of this code.  It is unfortunate that they were
put in a position of having to reverse engineer drivers for this HW.

Thanks,
Jon

>
>       Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web