[PATCH v3 0/8] Add clock config and pm support to bcm iProc mdio mux

[PATCH v3 0/8] Add clock config and pm support to bcm iProc mdio mux

[Arun Parameswaran]

Hi,
The patchset is based on David Miller's "net-next" repo.

The patchset extends the Broadcom iProc mdio mux to add support for
suspend/resume and the ability to configure the internal clock
divider. The patchset also sets the scan control register to
disable external master access.

The base address of the mdio-mux-bcm-iproc is modified to point to the
start of the mdio block's address space, to be able to access all the
mdio's registers. The missing registers are required to configure the
internal clock divider registers in some of the Broadcom SoC's.

Changes from v2:
 - Addressed Andrew's comments:
     - Moved to using devm_mdiobus_alloc. Added this as a separate patch.
     - Changed to reverse christmas tree order for variable declaration in
       the clock patch
 - Addressed Florian's comments:
     - Removed null checks for the clock before calling unprepare in
       both clock and pm patches.
     - Added check for EPROBE_DEFER when fetching the clock in the clock
       patch.
 - The patch to use the devm API has been added before the clock & pm
   patches. This patch is now patch '5' in the series.
 - Added reviewed-by tags to commit messages of patches which remain
   unmodified from v2.
 - Modified PM patch to use platform_get_drvdata() in suspend/resume
   API's, similar to the recent fix that went in for the remove()
   api.

Changes from v1:
 - Addressed Andrew's comments.
   - Reworked the patches to be based on 'net-next'
   - Removed 'fixes' from the commit messages, the changes are related
     to the new features being added.
   - Maintained backward compatibility to older dt-blob's specifying
     base addresse with an offset. The correction is applied in the
     driver and a message is printed to update the dt-blob.
   - Re-worked and re-ordered the last four patches (4-7).
     - Added setting of the scan control register as a new patch
     - Added a call to 'clk_prepare_enable()' in the patch that adds
       the clock config support, removed the debug message when clock
       is not passed.
     - Simplified the pm support patch (removed the array used for the
       save/restore logic).

Thanks

Arun Parameswaran (8):
  dt-bindings: net: Fix Broadcom iProc mdio mux driver base address
  net: phy: Fix the register offsets in Broadcom iProc mdio mux driver
  arm64: dts: Fix the base address of the Broadcom iProc mdio mux
  net: phy: Disable external master access in bcm mdio mux driver
  net: phy: Use devm api for mdio bus allocation in bcm iproc mdio mux
  dt-bindings: net: Add clock handle to Broadcom iProc mdio mux
  net: phy: Add support to configure clock in Broadcom iProc mdio mux
  net: phy: Add pm support to Broadcom iProc mdio mux driver

 .../bindings/net/brcm,mdio-mux-iproc.txt           |   7 +-
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi   |   4 +-
 .../arm64/boot/dts/broadcom/stingray/stingray.dtsi |   4 +-
 drivers/net/phy/mdio-mux-bcm-iproc.c               | 111 +++++++++++++++++++--
 4 files changed, 109 insertions(+), 17 deletions(-)

-- 
1.9.1

[PATCH v3 1/8] dt-bindings: net: Fix Broadcom iProc mdio mux driver base address

[Arun Parameswaran]

Modify the base address of the Broadcom iProc MDIO mux driver to
point to the start of the block's register address space.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
index dfe287a..dc8aa68 100644
--- a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
+++ b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
@@ -18,9 +18,9 @@ at- Documentation/devicetree/bindings/net/mdio-mux.txt
 
 
 for example:
-		mdio_mux_iproc: mdio-mux@6602023c {
+		mdio_mux_iproc: mdio-mux@66020000 {
 			compatible = "brcm,mdio-mux-iproc";
-			reg = <0x6602023c 0x14>;
+			reg = <0x66020000 0x250>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-- 
1.9.1

[PATCH v3 2/8] net: phy: Fix the register offsets in Broadcom iProc mdio mux driver

[Arun Parameswaran]

Modify the register offsets in the Broadcom iProc mdio mux to start
from the top of the register address space.

Earlier, the base address pointed to the end of the block's register
space. The base address will now point to the start of the mdio's
address space. The offsets have been fixed to match this.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 0831b71..48bb74a 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -22,7 +22,7 @@
 #include <linux/mdio-mux.h>
 #include <linux/delay.h>
 
-#define MDIO_PARAM_OFFSET		0x00
+#define MDIO_PARAM_OFFSET		0x23c
 #define MDIO_PARAM_MIIM_CYCLE		29
 #define MDIO_PARAM_INTERNAL_SEL		25
 #define MDIO_PARAM_BUS_ID		22
@@ -30,20 +30,22 @@
 #define MDIO_PARAM_PHY_ID		16
 #define MDIO_PARAM_PHY_DATA		0
 
-#define MDIO_READ_OFFSET		0x04
+#define MDIO_READ_OFFSET		0x240
 #define MDIO_READ_DATA_MASK		0xffff
-#define MDIO_ADDR_OFFSET		0x08
+#define MDIO_ADDR_OFFSET		0x244
 
-#define MDIO_CTRL_OFFSET		0x0C
+#define MDIO_CTRL_OFFSET		0x248
 #define MDIO_CTRL_WRITE_OP		0x1
 #define MDIO_CTRL_READ_OP		0x2
 
-#define MDIO_STAT_OFFSET		0x10
+#define MDIO_STAT_OFFSET		0x24c
 #define MDIO_STAT_DONE			1
 
 #define BUS_MAX_ADDR			32
 #define EXT_BUS_START_ADDR		16
 
+#define MDIO_REG_ADDR_SPACE_SIZE	0x250
+
 struct iproc_mdiomux_desc {
 	void *mux_handle;
 	void __iomem *base;
@@ -169,6 +171,14 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 	md->dev = &pdev->dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res->start & 0xfff) {
+		/* For backward compatibility in case the
+		 * base address is specified with an offset.
+		 */
+		dev_info(&pdev->dev, "fix base address in dt-blob\n");
+		res->start &= ~0xfff;
+		res->end = res->start + MDIO_REG_ADDR_SPACE_SIZE - 1;
+	}
 	md->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(md->base)) {
 		dev_err(&pdev->dev, "failed to ioremap register\n");
-- 
1.9.1

[PATCH v3 3/8] arm64: dts: Fix the base address of the Broadcom iProc mdio mux

[Arun Parameswaran]

Modify the base address of the mdio mux driver to point to the
start of the mdio mux block's register address space.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi    | 4 ++--
 arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi b/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
index 4057197..1a406a7 100644
--- a/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
@@ -482,9 +482,9 @@
 			status = "disabled";
 		};
 
-		mdio_mux_iproc: mdio-mux@6602023c {
+		mdio_mux_iproc: mdio-mux@66020000 {
 			compatible = "brcm,mdio-mux-iproc";
-			reg = <0x6602023c 0x14>;
+			reg = <0x66020000 0x250>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
diff --git a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
index b203152..a70e8dd 100644
--- a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
+++ b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
@@ -278,9 +278,9 @@
 
 		#include "stingray-pinctrl.dtsi"
 
-		mdio_mux_iproc: mdio-mux@2023c {
+		mdio_mux_iproc: mdio-mux@20000 {
 			compatible = "brcm,mdio-mux-iproc";
-			reg = <0x0002023c 0x14>;
+			reg = <0x00020000 0x250>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-- 
1.9.1

[PATCH v3 4/8] net: phy: Disable external master access in bcm mdio mux driver

[Arun Parameswaran]

Configure the scan control register in the Broadcom iProc
mdio mux driver to disable access to external master.

In some SoC's, the scan control register defaults to an incorrect
value.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 48bb74a..c36ce4b 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -22,6 +22,9 @@
 #include <linux/mdio-mux.h>
 #include <linux/delay.h>
 
+#define MDIO_SCAN_CTRL_OFFSET		0x008
+#define MDIO_SCAN_CTRL_OVRIDE_EXT_MSTR	28
+
 #define MDIO_PARAM_OFFSET		0x23c
 #define MDIO_PARAM_MIIM_CYCLE		29
 #define MDIO_PARAM_INTERNAL_SEL		25
@@ -53,6 +56,16 @@ struct iproc_mdiomux_desc {
 	struct mii_bus *mii_bus;
 };
 
+static void mdio_mux_iproc_config(struct iproc_mdiomux_desc *md)
+{
+	u32 val;
+
+	/* Disable external mdio master access */
+	val = readl(md->base + MDIO_SCAN_CTRL_OFFSET);
+	val |= BIT(MDIO_SCAN_CTRL_OVRIDE_EXT_MSTR);
+	writel(val, md->base + MDIO_SCAN_CTRL_OFFSET);
+}
+
 static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
 {
 	unsigned int timeout = 1000; /* loop for 1s */
@@ -216,6 +229,8 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 		goto out_register;
 	}
 
+	mdio_mux_iproc_config(md);
+
 	dev_info(md->dev, "iProc mdiomux registered\n");
 	return 0;
 
-- 
1.9.1

[PATCH v3 5/8] net: phy: Use devm api for mdio bus allocation in bcm iproc mdio mux

[Arun Parameswaran]

Use devm_mdiobus_alloc() instead of mdiobus_alloc() in the Broadcom
iProc mdio mux driver.

Also, clear the driver data on error from probe or when remove() is
called.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index c36ce4b..5640d78 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -198,7 +198,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 		return PTR_ERR(md->base);
 	}
 
-	md->mii_bus = mdiobus_alloc();
+	md->mii_bus = devm_mdiobus_alloc(&pdev->dev);
 	if (!md->mii_bus) {
 		dev_err(&pdev->dev, "mdiomux bus alloc failed\n");
 		return -ENOMEM;
@@ -217,7 +217,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 	rc = mdiobus_register(bus);
 	if (rc) {
 		dev_err(&pdev->dev, "mdiomux registration failed\n");
-		goto out;
+		return rc;
 	}
 
 	platform_set_drvdata(pdev, md);
@@ -226,6 +226,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 			   &md->mux_handle, md, md->mii_bus);
 	if (rc) {
 		dev_info(md->dev, "mdiomux initialization failed\n");
+		platform_set_drvdata(pdev, NULL);
 		goto out_register;
 	}
 
@@ -236,8 +237,6 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 
 out_register:
 	mdiobus_unregister(bus);
-out:
-	mdiobus_free(bus);
 	return rc;
 }
 
@@ -247,7 +246,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
 
 	mdio_mux_uninit(md->mux_handle);
 	mdiobus_unregister(md->mii_bus);
-	mdiobus_free(md->mii_bus);
+	platform_set_drvdata(pdev, NULL);
 
 	return 0;
 }
-- 
1.9.1

[PATCH v3 6/8] dt-bindings: net: Add clock handle to Broadcom iProc mdio mux

[Arun Parameswaran]

Add clock phandle, of the core clock driving the mdio block, as an
optional property to the Broadcom iProc mdio mux.

The clock, when specified, will be used to setup the rate adjust registers
in the mdio to derrive the mdio's operating frequency.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
index dc8aa68..b58843f 100644
--- a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
+++ b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
@@ -13,6 +13,9 @@ MDIO multiplexer node:
 Every non-ethernet PHY requires a compatible so that it could be probed based
 on this compatible string.
 
+Optional properties:
+- clocks: phandle of the core clock which drives the mdio block.
+
 Additional information regarding generic multiplexer properties can be found
 at- Documentation/devicetree/bindings/net/mdio-mux.txt
 
-- 
1.9.1

[PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Arun Parameswaran]

Add support to configure the internal rate adjust register based on the
core clock supplied through device tree in the Broadcom iProc mdio mux.

The operating frequency of the mdio mux block is 11MHz. This is derrived
by dividing the clock to the mdio mux with the rate adjust register.

In some SoC's the default values of the rate adjust register do not yield
11MHz. These SoC's are required to specify the clock via the device tree
for proper operation.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 42 ++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 5640d78..5127010 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -13,7 +13,7 @@
  * You should have received a copy of the GNU General Public License
  * version 2 (GPLv2) along with this source code.
  */
-
+#include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/device.h>
 #include <linux/of_mdio.h>
@@ -22,6 +22,10 @@
 #include <linux/mdio-mux.h>
 #include <linux/delay.h>
 
+#define MDIO_RATE_ADJ_EXT_OFFSET	0x000
+#define MDIO_RATE_ADJ_INT_OFFSET	0x004
+#define MDIO_RATE_ADJ_DIVIDENT_SHIFT	16
+
 #define MDIO_SCAN_CTRL_OFFSET		0x008
 #define MDIO_SCAN_CTRL_OVRIDE_EXT_MSTR	28
 
@@ -49,21 +53,38 @@
 
 #define MDIO_REG_ADDR_SPACE_SIZE	0x250
 
+#define MDIO_OPERATING_FREQUENCY	11000000
+#define MDIO_RATE_ADJ_DIVIDENT		1
+
 struct iproc_mdiomux_desc {
 	void *mux_handle;
 	void __iomem *base;
 	struct device *dev;
 	struct mii_bus *mii_bus;
+	struct clk *core_clk;
 };
 
 static void mdio_mux_iproc_config(struct iproc_mdiomux_desc *md)
 {
+	u32 divisor;
 	u32 val;
 
 	/* Disable external mdio master access */
 	val = readl(md->base + MDIO_SCAN_CTRL_OFFSET);
 	val |= BIT(MDIO_SCAN_CTRL_OVRIDE_EXT_MSTR);
 	writel(val, md->base + MDIO_SCAN_CTRL_OFFSET);
+
+	if (md->core_clk) {
+		/* use rate adjust regs to derrive the mdio's operating
+		 * frequency from the specified core clock
+		 */
+		divisor = clk_get_rate(md->core_clk) / MDIO_OPERATING_FREQUENCY;
+		divisor = divisor / (MDIO_RATE_ADJ_DIVIDENT + 1);
+		val = divisor;
+		val |= MDIO_RATE_ADJ_DIVIDENT << MDIO_RATE_ADJ_DIVIDENT_SHIFT;
+		writel(val, md->base + MDIO_RATE_ADJ_EXT_OFFSET);
+		writel(val, md->base + MDIO_RATE_ADJ_INT_OFFSET);
+	}
 }
 
 static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
@@ -204,6 +225,20 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	md->core_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(md->core_clk)) {
+		if (PTR_ERR(md->core_clk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		md->core_clk = NULL;
+	} else {
+		rc = clk_prepare_enable(md->core_clk);
+		if (rc) {
+			dev_err(&pdev->dev, "failed to enable core clk\n");
+			return rc;
+		}
+	}
+
 	bus = md->mii_bus;
 	bus->priv = md;
 	bus->name = "iProc MDIO mux bus";
@@ -217,7 +252,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 	rc = mdiobus_register(bus);
 	if (rc) {
 		dev_err(&pdev->dev, "mdiomux registration failed\n");
-		return rc;
+		goto out_clk;
 	}
 
 	platform_set_drvdata(pdev, md);
@@ -237,6 +272,8 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 
 out_register:
 	mdiobus_unregister(bus);
+out_clk:
+	clk_disable_unprepare(md->core_clk);
 	return rc;
 }
 
@@ -246,6 +283,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
 
 	mdio_mux_uninit(md->mux_handle);
 	mdiobus_unregister(md->mii_bus);
+	clk_disable_unprepare(md->core_clk);
 	platform_set_drvdata(pdev, NULL);
 
 	return 0;
-- 
1.9.1

[PATCH v3 8/8] net: phy: Add pm support to Broadcom iProc mdio mux driver

[Arun Parameswaran]

Add support for suspend and resume to the Broadcom iProc mdio
mux driver.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 5127010..4e2c51b 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -289,6 +289,32 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mdio_mux_iproc_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iproc_mdiomux_desc *md = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(md->core_clk);
+
+	return 0;
+}
+
+static int mdio_mux_iproc_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iproc_mdiomux_desc *md = platform_get_drvdata(pdev);
+
+	clk_prepare_enable(md->core_clk);
+	mdio_mux_iproc_config(md);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mdio_mux_iproc_pm_ops,
+			 mdio_mux_iproc_suspend, mdio_mux_iproc_resume);
+
 static const struct of_device_id mdio_mux_iproc_match[] = {
 	{
 		.compatible = "brcm,mdio-mux-iproc",
@@ -301,6 +327,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
 	.driver = {
 		.name		= "mdio-mux-iproc",
 		.of_match_table = mdio_mux_iproc_match,
+		.pm		= &mdio_mux_iproc_pm_ops,
 	},
 	.probe		= mdio_mux_iproc_probe,
 	.remove		= mdio_mux_iproc_remove,
-- 
1.9.1

Re: [PATCH v3 5/8] net: phy: Use devm api for mdio bus allocation in bcm iproc mdio mux

[Florian Fainelli]

On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
> Use devm_mdiobus_alloc() instead of mdiobus_alloc() in the Broadcom
> iProc mdio mux driver.
> 
> Also, clear the driver data on error from probe or when remove() is
> called.
> 
> Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
> ---
>  drivers/net/phy/mdio-mux-bcm-iproc.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
> index c36ce4b..5640d78 100644
> --- a/drivers/net/phy/mdio-mux-bcm-iproc.c
> +++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
> @@ -198,7 +198,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>  		return PTR_ERR(md->base);
>  	}
>  
> -	md->mii_bus = mdiobus_alloc();
> +	md->mii_bus = devm_mdiobus_alloc(&pdev->dev);
>  	if (!md->mii_bus) {
>  		dev_err(&pdev->dev, "mdiomux bus alloc failed\n");
>  		return -ENOMEM;
> @@ -217,7 +217,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>  	rc = mdiobus_register(bus);
>  	if (rc) {
>  		dev_err(&pdev->dev, "mdiomux registration failed\n");
> -		goto out;
> +		return rc;
>  	}
>  
>  	platform_set_drvdata(pdev, md);
> @@ -226,6 +226,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>  			   &md->mux_handle, md, md->mii_bus);
>  	if (rc) {
>  		dev_info(md->dev, "mdiomux initialization failed\n");
> +		platform_set_drvdata(pdev, NULL);

This is minor, but I don't think this is necessary, the core device
driver layer does that already.

>  		goto out_register;
>  	}
>  
> @@ -236,8 +237,6 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>  
>  out_register:
>  	mdiobus_unregister(bus);
> -out:
> -	mdiobus_free(bus);
>  	return rc;
>  }
>  
> @@ -247,7 +246,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
>  
>  	mdio_mux_uninit(md->mux_handle);
>  	mdiobus_unregister(md->mii_bus);
> -	mdiobus_free(md->mii_bus);
> +	platform_set_drvdata(pdev, NULL);

Likewise.

>  
>  	return 0;
>  }
> 


-- 
Florian

Re: [PATCH v3 8/8] net: phy: Add pm support to Broadcom iProc mdio mux driver

[Florian Fainelli]

On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
> Add support for suspend and resume to the Broadcom iProc mdio
> mux driver.
> 
> Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Florian Fainelli]

On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
> Add support to configure the internal rate adjust register based on the
> core clock supplied through device tree in the Broadcom iProc mdio mux.
> 
> The operating frequency of the mdio mux block is 11MHz. This is derrived
> by dividing the clock to the mdio mux with the rate adjust register.
> 
> In some SoC's the default values of the rate adjust register do not yield
> 11MHz. These SoC's are required to specify the clock via the device tree
> for proper operation.
> 
> Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
> ---

[snip]

>  static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
> @@ -204,6 +225,20 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	md->core_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(md->core_clk)) {
> +		if (PTR_ERR(md->core_clk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		md->core_clk = NULL;

I would simplify this a bit:

	if (IS_ERR(md->core_clk) && PTR_ERR(md->core_clk) == -EPROBE_DEFER)
		return PTR_ERR(md->core_clk);
	else
		md->core_clk = NULL;

	rc = clk_prepare_enable(md->core_clk);

and continue that way.

> +	} else {
> +		rc = clk_prepare_enable(md->core_clk);
> +		if (rc) {
> +			dev_err(&pdev->dev, "failed to enable core clk\n");
> +			return rc;
> +		}
> +	}
> +
>  	bus = md->mii_bus;
>  	bus->priv = md;
>  	bus->name = "iProc MDIO mux bus";
> @@ -217,7 +252,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>  	rc = mdiobus_register(bus);
>  	if (rc) {
>  		dev_err(&pdev->dev, "mdiomux registration failed\n");
> -		return rc;
> +		goto out_clk;
>  	}
>  
>  	platform_set_drvdata(pdev, md);
> @@ -237,6 +272,8 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>  
>  out_register:
>  	mdiobus_unregister(bus);
> +out_clk:
> +	clk_disable_unprepare(md->core_clk);
>  	return rc;
>  }
>  
> @@ -246,6 +283,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
>  
>  	mdio_mux_uninit(md->mux_handle);
>  	mdiobus_unregister(md->mii_bus);
> +	clk_disable_unprepare(md->core_clk);
>  	platform_set_drvdata(pdev, NULL);
>  
>  	return 0;
> 


-- 
Florian

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Russell King - ARM Linux]

On Wed, Aug 01, 2018 at 11:40:33AM -0700, Florian Fainelli wrote:
> On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
> >  static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
> > @@ -204,6 +225,20 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
> >  
> > +	md->core_clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(md->core_clk)) {
> > +		if (PTR_ERR(md->core_clk) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		md->core_clk = NULL;
> 
> I would simplify this a bit:
> 
> 	if (IS_ERR(md->core_clk) && PTR_ERR(md->core_clk) == -EPROBE_DEFER)

Even better is:

	if (md->core_clk == ERR_PTR(-EPROBE_DEFER))

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Arun Parameswaran]

Hi Russell, Florian,

On 18-08-01 11:46 AM, Russell King - ARM Linux wrote:
> On Wed, Aug 01, 2018 at 11:40:33AM -0700, Florian Fainelli wrote:
>> On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
>>>  static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
>>> @@ -204,6 +225,20 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>>>  		return -ENOMEM;
>>>  	}
>>>  
>>> +	md->core_clk = devm_clk_get(&pdev->dev, NULL);
>>> +	if (IS_ERR(md->core_clk)) {
>>> +		if (PTR_ERR(md->core_clk) == -EPROBE_DEFER)
>>> +			return -EPROBE_DEFER;
>>> +
>>> +		md->core_clk = NULL;
>>
>> I would simplify this a bit:
>>
>> 	if (IS_ERR(md->core_clk) && PTR_ERR(md->core_clk) == -EPROBE_DEFER)
> 
> Even better is:
> 
> 	if (md->core_clk == ERR_PTR(-EPROBE_DEFER))
> 

The clock is an optional parameter. But I realize, that I did not handle all
the error cases.

My intention was to proceed with probe if the clock is not found in the DT.
If the clock is specified in the DT, make use of it.

So, should I modify to something like:

md->core_clk = devm_clk_get(&pdev->dev, NULL);
if (md->core_clk == ERR_PTR(-ENOENT)) {
	/* clock is optional, without it the default
	 * rate divider register values will be used
	 */
	md->core_clk = NULL;
} else if (IS_ERR(md->core_clk)) {
	return PTR_ERR(md->core_clk);
} else {
	rc = clk_prepare_enable(md->core_clk);
	...
}

Thanks
Arun

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Andrew Lunn]

> My intention was to proceed with probe if the clock is not found in the DT.
> If the clock is specified in the DT, make use of it.

You might want to look at what phy_optional_get() does.  If the phy
does not exist in DT, it does not return an error, but a NULL phy.
You can pass NULL to all the other calls generic phy_ calls. If there
is a real error, it returns it. That makes the driver code much
simpler.

You might want to consider adding clk_optional_get() and
devm_clk_optional_get().

	Andrew

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Russell King - ARM Linux]

On Wed, Aug 01, 2018 at 10:07:12PM +0200, Andrew Lunn wrote:
> You might want to consider adding clk_optional_get() and
> devm_clk_optional_get().

I think there's attempts to add such APIs but I don't think it's
trivial - it seems to require a _lot_ of discussion.

I think part of that is because of the quirky use of error codes.
If you look at clk_get(), it calls __of_clk_get_by_name() which
returns:

 -ENOENT if DT is disabled
 -ENOENT if the device has no DT node
 -EPROBE_DEFER if the lookup in DT succeeds but there's no registered
   clock
 -EINVAL if the device has a DT node but the lookup of the name
   failed (in otherwords, the optional clock was omitted)
 -ENOENT if the clocks = property has not enough clocks for the
    clock-names property
 -ENOMEM if we fail to allocate the clk
 -ENOENT if __clk_get() fails

or any other error code returned via of_clk_provider's ->get() method.

The use of -EINVAL, one of the most common error codes, makes it
difficult to be sure that the clock is not specified in DT.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Andrew Lunn]

On Wed, Aug 01, 2018 at 09:23:14PM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 01, 2018 at 10:07:12PM +0200, Andrew Lunn wrote:
> > You might want to consider adding clk_optional_get() and
> > devm_clk_optional_get().
> 
> I think there's attempts to add such APIs but I don't think it's
> trivial - it seems to require a _lot_ of discussion.
> 
> I think part of that is because of the quirky use of error codes.
> If you look at clk_get(), it calls __of_clk_get_by_name() which
> returns:
> 
>  -ENOENT if DT is disabled
>  -ENOENT if the device has no DT node
>  -EPROBE_DEFER if the lookup in DT succeeds but there's no registered
>    clock
>  -EINVAL if the device has a DT node but the lookup of the name
>    failed (in otherwords, the optional clock was omitted)
>  -ENOENT if the clocks = property has not enough clocks for the
>     clock-names property
>  -ENOMEM if we fail to allocate the clk
>  -ENOENT if __clk_get() fails

That makes it hard. I added phy_optional_get() early on, when the
error cases were simple. Hopefully they remain simple...

      Andrew

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Arun Parameswaran]

Hi Andrew, Russell,

On 18-08-01 01:38 PM, Andrew Lunn wrote:
> On Wed, Aug 01, 2018 at 09:23:14PM +0100, Russell King - ARM Linux wrote:
>> On Wed, Aug 01, 2018 at 10:07:12PM +0200, Andrew Lunn wrote:
>>> You might want to consider adding clk_optional_get() and
>>> devm_clk_optional_get().
>>
>> I think there's attempts to add such APIs but I don't think it's
>> trivial - it seems to require a _lot_ of discussion.
>>
>> I think part of that is because of the quirky use of error codes.
>> If you look at clk_get(), it calls __of_clk_get_by_name() which
>> returns:
>>
>>  -ENOENT if DT is disabled
>>  -ENOENT if the device has no DT node
>>  -EPROBE_DEFER if the lookup in DT succeeds but there's no registered
>>    clock
>>  -EINVAL if the device has a DT node but the lookup of the name
>>    failed (in otherwords, the optional clock was omitted)
>>  -ENOENT if the clocks = property has not enough clocks for the
>>     clock-names property
>>  -ENOMEM if we fail to allocate the clk
>>  -ENOENT if __clk_get() fails
> 
> That makes it hard. I added phy_optional_get() early on, when the
> error cases were simple. Hopefully they remain simple...
> 
>       Andrew
> 
I traced the devm_clk_get() and ran into various error codes, so I
figured I should return error on all the other errors (like ENOMEM, EINVAL
& EPROBE_DEFER) but 'ENOENT'.

But, looking at the errors from Russell's email, it looks like I
should look for both 'ENOENT' and 'EINVAL' and consider both these
errors as 'clock not specified' ?

Can I do something like this in the code for this patchset:

md->core_clk = devm_clk_get(&pdev->dev, NULL);
if (md->core_clk == ERR_PTR(-ENOENT) || md->core_clk == ERR_PTR(-EINVAL)) {
	/* clock is optional, without it the default
	 * rate divider register values will be used
	 */
	md->core_clk = NULL;
} else if (IS_ERR(md->core_clk)) {
	return PTR_ERR(md->core_clk);
} else {
	rc = clk_prepare_enable(md->core_clk);
	...
}

Thanks
Arun

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Andrew Lunn]

> md->core_clk = devm_clk_get(&pdev->dev, NULL);
> if (md->core_clk == ERR_PTR(-ENOENT) || md->core_clk == ERR_PTR(-EINVAL)) {
> 	/* clock is optional, without it the default
> 	 * rate divider register values will be used
> 	 */
> 	md->core_clk = NULL;
> } else if (IS_ERR(md->core_clk)) {
> 	return PTR_ERR(md->core_clk);
> } else {
> 	rc = clk_prepare_enable(md->core_clk);
> 	...
> }

As Florian pointed out, the clk_ API is happy to take a NULL pointer
for a clock. So you don't need this last else.

    Andrew

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Arun Parameswaran]

Hi Andrew,

On 18-08-01 02:55 PM, Andrew Lunn wrote:
>> md->core_clk = devm_clk_get(&pdev->dev, NULL);
>> if (md->core_clk == ERR_PTR(-ENOENT) || md->core_clk == ERR_PTR(-EINVAL)) {
>> 	/* clock is optional, without it the default
>> 	 * rate divider register values will be used
>> 	 */
>> 	md->core_clk = NULL;
>> } else if (IS_ERR(md->core_clk)) {
>> 	return PTR_ERR(md->core_clk);
>> } else {
>> 	rc = clk_prepare_enable(md->core_clk);
>> 	...
>> }
> 
> As Florian pointed out, the clk_ API is happy to take a NULL pointer
> for a clock. So you don't need this last else.
> 
>     Andrew
> 
I do return with an error from the probe if the clk_prepare_enable() fails,
so I was calling the prepare with a valid clock.

In the other places where I used the clock, the return values were being
ignored.

The 'else' part should have been, I apologize for the confusion.

if (md->core_clk == ERR_PTR(-ENOENT) || md->core_clk == ERR_PTR(-EINVAL)) {
 	/* clock is optional, without it the default
 	 * rate divider register values will be used
 	 */
 	md->core_clk = NULL;
} else if (IS_ERR(md->core_clk)) {
 	return PTR_ERR(md->core_clk);
} else {
	rc = clk_prepare_enable(md->core_clk);
	if (rc) {
		dev_err(&pdev->dev, "failed to enable core clk\n");
		return rc;
	}
}


Thanks
Arun

Re: [PATCH v3 7/8] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Andrew Lunn]

> > As Florian pointed out, the clk_ API is happy to take a NULL pointer
> > for a clock. So you don't need this last else.
> > 
> >     Andrew
> > 
> I do return with an error from the probe if the clk_prepare_enable() fails,
> so I was calling the prepare with a valid clock.

How many times do we need to say it?

NULL is a valid clock. 

int clk_prepare(struct clk *clk)
{
	if (!clk)
	   return 0;

	   return clk_core_prepare_lock(clk->core);
}

int clk_enable(struct clk *clk)
{
	if (!clk)
	   return 0;

	   return clk_core_enable_lock(clk->core);
}

clk_prepare_enable(NULL) returns 0, which is not an error.

	 Andrew