[PATCH v6 1/2] net: mdio: add ipq8064 mdio driver

[PATCH v6 1/2] net: mdio: add ipq8064 mdio driver

[Ansuel Smith]

Currently ipq806x soc use generic bitbang driver to
comunicate with the gmac ethernet interface.
Add a dedicated driver created by chunkeey to fix this.

Co-developed-by: Christian Lamparter <chunkeey@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/phy/Kconfig        |   8 ++
 drivers/net/phy/Makefile       |   1 +
 drivers/net/phy/mdio-ipq8064.c | 166 +++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 drivers/net/phy/mdio-ipq8064.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 9dabe03a668c..ec2a5493a7e8 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -157,6 +157,14 @@ config MDIO_I2C
 
 	  This is library mode.
 
+config MDIO_IPQ8064
+	tristate "Qualcomm IPQ8064 MDIO interface support"
+	depends on HAS_IOMEM && OF_MDIO
+	depends on MFD_SYSCON
+	help
+	  This driver supports the MDIO interface found in the network
+	  interface units of the IPQ8064 SoC
+
 config MDIO_MOXART
 	tristate "MOXA ART MDIO interface support"
 	depends on ARCH_MOXART || COMPILE_TEST
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index fe5badf13b65..8f02bd2089f3 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
 obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
 obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
 obj-$(CONFIG_MDIO_I2C)		+= mdio-i2c.o
+obj-$(CONFIG_MDIO_IPQ8064)	+= mdio-ipq8064.o
 obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
 obj-$(CONFIG_MDIO_MSCC_MIIM)	+= mdio-mscc-miim.o
 obj-$(CONFIG_MDIO_OCTEON)	+= mdio-octeon.o
diff --git a/drivers/net/phy/mdio-ipq8064.c b/drivers/net/phy/mdio-ipq8064.c
new file mode 100644
index 000000000000..74d6b92a6f48
--- /dev/null
+++ b/drivers/net/phy/mdio-ipq8064.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Qualcomm IPQ8064 MDIO interface driver
+//
+// Copyright (C) 2019 Christian Lamparter <chunkeey@gmail.com>
+
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/syscon.h>
+
+/* MII address register definitions */
+#define MII_ADDR_REG_ADDR                       0x10
+#define MII_BUSY                                BIT(0)
+#define MII_WRITE                               BIT(1)
+#define MII_CLKRANGE_60_100M                    (0 << 2)
+#define MII_CLKRANGE_100_150M                   (1 << 2)
+#define MII_CLKRANGE_20_35M                     (2 << 2)
+#define MII_CLKRANGE_35_60M                     (3 << 2)
+#define MII_CLKRANGE_150_250M                   (4 << 2)
+#define MII_CLKRANGE_250_300M                   (5 << 2)
+#define MII_CLKRANGE_MASK			GENMASK(4, 2)
+#define MII_REG_SHIFT				6
+#define MII_REG_MASK				GENMASK(10, 6)
+#define MII_ADDR_SHIFT				11
+#define MII_ADDR_MASK				GENMASK(15, 11)
+
+#define MII_DATA_REG_ADDR                       0x14
+
+#define MII_MDIO_DELAY_USEC                     (1000)
+#define MII_MDIO_RETRY_MSEC                     (10)
+
+struct ipq8064_mdio {
+	struct regmap *base; /* NSS_GMAC0_BASE */
+};
+
+static int
+ipq8064_mdio_wait_busy(struct ipq8064_mdio *priv)
+{
+	u32 busy;
+
+	return regmap_read_poll_timeout(priv->base, MII_ADDR_REG_ADDR, busy,
+					!(busy & MII_BUSY), MII_MDIO_DELAY_USEC,
+					MII_MDIO_RETRY_MSEC * USEC_PER_MSEC);
+}
+
+static int
+ipq8064_mdio_read(struct mii_bus *bus, int phy_addr, int reg_offset)
+{
+	struct ipq8064_mdio *priv = bus->priv;
+	u32 miiaddr = MII_BUSY | MII_CLKRANGE_250_300M;
+	u32 ret_val;
+	int err;
+
+	/* Reject clause 45 */
+	if (reg_offset & MII_ADDR_C45)
+		return -EOPNOTSUPP;
+
+	miiaddr |= ((phy_addr << MII_ADDR_SHIFT) & MII_ADDR_MASK) |
+		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
+
+	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
+	usleep_range(10, 20);
+
+	err = ipq8064_mdio_wait_busy(priv);
+	if (err)
+		return err;
+
+	regmap_read(priv->base, MII_DATA_REG_ADDR, &ret_val);
+	return (int)ret_val;
+}
+
+static int
+ipq8064_mdio_write(struct mii_bus *bus, int phy_addr, int reg_offset, u16 data)
+{
+	struct ipq8064_mdio *priv = bus->priv;
+	u32 miiaddr = MII_WRITE | MII_BUSY | MII_CLKRANGE_250_300M;
+
+	/* Reject clause 45 */
+	if (reg_offset & MII_ADDR_C45)
+		return -EOPNOTSUPP;
+
+	regmap_write(priv->base, MII_DATA_REG_ADDR, data);
+
+	miiaddr |= ((phy_addr << MII_ADDR_SHIFT) & MII_ADDR_MASK) |
+		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
+
+	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
+	usleep_range(10, 20);
+
+	return ipq8064_mdio_wait_busy(priv);
+}
+
+static int
+ipq8064_mdio_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct ipq8064_mdio *priv;
+	struct mii_bus *bus;
+	int ret;
+
+	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "ipq8064_mdio_bus";
+	bus->read = ipq8064_mdio_read;
+	bus->write = ipq8064_mdio_write;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev));
+	bus->parent = &pdev->dev;
+
+	priv = bus->priv;
+	priv->base = syscon_node_to_regmap(np);
+	if (IS_ERR(priv->base) && priv->base != ERR_PTR(-EPROBE_DEFER))
+		priv->base = syscon_regmap_lookup_by_phandle(np, "master");
+
+	if (priv->base == ERR_PTR(-EPROBE_DEFER)) {
+		return -EPROBE_DEFER;
+	} else if (IS_ERR(priv->base)) {
+		dev_err(&pdev->dev, "error getting syscon regmap, error=%pe\n",
+			priv->base);
+		return PTR_ERR(priv->base);
+	}
+
+	ret = of_mdiobus_register(bus, np);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, bus);
+	return 0;
+}
+
+static int
+ipq8064_mdio_remove(struct platform_device *pdev)
+{
+	struct mii_bus *bus = platform_get_drvdata(pdev);
+
+	mdiobus_unregister(bus);
+
+	return 0;
+}
+
+static const struct of_device_id ipq8064_mdio_dt_ids[] = {
+	{ .compatible = "qcom,ipq8064-mdio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ipq8064_mdio_dt_ids);
+
+static struct platform_driver ipq8064_mdio_driver = {
+	.probe = ipq8064_mdio_probe,
+	.remove = ipq8064_mdio_remove,
+	.driver = {
+		.name = "ipq8064-mdio",
+		.of_match_table = ipq8064_mdio_dt_ids,
+	},
+};
+
+module_platform_driver(ipq8064_mdio_driver);
+
+MODULE_DESCRIPTION("Qualcomm IPQ8064 MDIO interface driver");
+MODULE_AUTHOR("Christian Lamparter <chunkeey@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.0

[PATCH v6 2/2] Documentation: devictree: Add ipq806x mdio bindings

[Ansuel Smith]

Add documentations for ipq806x mdio driver.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 .../bindings/net/qcom,ipq8064-mdio.yaml       | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qcom,ipq8064-mdio.yaml

diff --git a/Documentation/devicetree/bindings/net/qcom,ipq8064-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq8064-mdio.yaml
new file mode 100644
index 000000000000..d2254a5ff2ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qcom,ipq8064-mdio.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/qcom,ipq8064-mdio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm ipq806x MDIO bus controller
+
+maintainers:
+  - Ansuel Smith <ansuelsmth@gmail.com>
+
+description: |+
+  The ipq806x soc have a MDIO dedicated controller that is
+  used to comunicate with the gmac phy conntected.
+  Child nodes of this MDIO bus controller node are standard
+  Ethernet PHY device nodes as described in
+  Documentation/devicetree/bindings/net/phy.txt
+
+allOf:
+  - $ref: "mdio.yaml#"
+
+properties:
+  compatible:
+    const: qcom,ipq8064-mdio
+  reg:
+    maxItems: 1
+    description: address and length of the register set for the device
+  clocks:
+    maxItems: 1
+    description: A reference to the clock supplying the MDIO bus controller
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    mdio0: mdio@37000000 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        compatible = "qcom,ipq8064-mdio", "syscon";
+        reg = <0x37000000 0x200000>;
+        resets = <&gcc GMAC_CORE1_RESET>;
+        reset-names = "stmmaceth";
+        clocks = <&gcc GMAC_CORE1_CLK>;
+
+        switch@10 {
+            compatible = "qca,qca8337";
+            ...
+        }
+    };
-- 
2.25.0

Re: [PATCH v6 1/2] net: mdio: add ipq8064 mdio driver

[Christian Lamparter]

Hi,

On Saturday, 22 February 2020 17:16:26 CET Mr. Smith wrote:
> Currently ipq806x soc use generic bitbang driver to
> comunicate with the gmac ethernet interface.
> Add a dedicated driver created by chunkeey to fix this.

Ok, I'm officially a bit "out-of-the-loop" here (was looking for
a changelog, but I guess I missed that mail). So please excuse me,
if I say something that was already stated. Instead kindly reply
with a text extract (preferably with the link to the mail as I'm not
a regular linux-net subscriber) to that discussion.

> Co-developed-by: Christian Lamparter <chunkeey@gmail.com>
Ahh, I see that
"docs: Clarify the usage and sign-off requirements for Co-developed-by"
did away with the "original author" wording in the submitting-patches doc.
So keeping the original "From:" doesn't matter as such anymore.

Still, for reference (historical digs or if someone wants to take a peek):
Most of this patch comes from a patch named:
700-net-mdio-add-ipq8064-mdio-driver.patch

which is part of a bigger "ipq8064: ipq8064-mdio + dsa test" commit.
This is currently in a staging tree hosted on <git.openwrt.org>.
Here's a direct link: [0] (sadly, this is not self-updating). 

Background: This driver was mainly written to make and test the qca8k
patches I posted to the linux-net ML last year. The Idea was that I
didn't have to deal with the odd random timing issues on the slow debug
kernels, when I was perfing/hammering the device. 
(The IPQ8064 has/had various scaling problems so, this might be already
fixed by some of Mr. Smith's other work for the abandoned IPQ8064).

From what I know, this patch mostly helps/fixes a problem with the
out-of-tree OpenWrt swconfig-based ar8216/ar8236 driver for the
used QCA8337 switch.
This driver really needs the faster, more efficient reads due to having
a statistics worker which is just gobbing cycles because of all the
exclusive-access bit-banging taking place.
(Remember, the worker could read all the phy-counters for each of the 7
ports over gpio-mdio (there have been attempts to make it less hoggy
in the mean time though). While the IPQ8064 SoC has a beefy dual-core
Cortex-A15 with up to 1.4GHz*, this bitbanging will result in a
considerable load on at least one of the cores.)

Mr. Smith knows more about this though, as he has the hardware and
is the upcoming IPQ8064 expert on this. 

From my POV, I never anticipated this hack was up to standards of linux-net.
As there is this ugly dependency on having the "master" MAC (handled by the 
sttmac/dwmac-ipq806x.c) part up and operational all the time (that's why
the "master" property is needed for some devices at least).
 
I had hopes to do this properly and integrate it into
dwmac-ipq806x.c, but this requires much more work and ultimately
"virtual-mdio" was "just good enough™️" for standard, production kernels.
From what I remember the Qualcomm devs themselves never bothered in their
abandoned posts with the DeviceTree for the qcom-ipq8064.dts to include a
standalone mdio driver. Instead the dev went straight to "virtual-mdio".
(but this was too long ago to really remember the details and it's getting late)

But Ok, if linux-net is content with the standalone mdio-ipq8064 approach,
then: Sure!

> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 9dabe03a668c..ec2a5493a7e8 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -157,6 +157,14 @@ config MDIO_I2C
>  
>  	  This is library mode.
>  
> +config MDIO_IPQ8064
> +	tristate "Qualcomm IPQ8064 MDIO interface support"
> +	depends on HAS_IOMEM && OF_MDIO
> +	depends on MFD_SYSCON
> +	help
> +	  This driver supports the MDIO interface found in the network
> +	  interface units of the IPQ8064 SoC
> +
>  config MDIO_MOXART
>  	tristate "MOXA ART MDIO interface support"
>  	depends on ARCH_MOXART || COMPILE_TEST
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index fe5badf13b65..8f02bd2089f3 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
>  obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
>  obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
>  obj-$(CONFIG_MDIO_I2C)		+= mdio-i2c.o
> +obj-$(CONFIG_MDIO_IPQ8064)	+= mdio-ipq8064.o
>  obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
>  obj-$(CONFIG_MDIO_MSCC_MIIM)	+= mdio-mscc-miim.o
>  obj-$(CONFIG_MDIO_OCTEON)	+= mdio-octeon.o
> diff --git a/drivers/net/phy/mdio-ipq8064.c b/drivers/net/phy/mdio-ipq8064.c
> new file mode 100644
> index 000000000000..74d6b92a6f48
> --- /dev/null
> +++ b/drivers/net/phy/mdio-ipq8064.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Qualcomm IPQ8064 MDIO interface driver
> +//
> +// Copyright (C) 2019 Christian Lamparter <chunkeey@gmail.com>
> +
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/of_mdio.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/syscon.h>
> +
> +/* MII address register definitions */
> +#define MII_ADDR_REG_ADDR                       0x10
> +#define MII_BUSY                                BIT(0)
> +#define MII_WRITE                               BIT(1)
> +#define MII_CLKRANGE_60_100M                    (0 << 2)
> +#define MII_CLKRANGE_100_150M                   (1 << 2)
> +#define MII_CLKRANGE_20_35M                     (2 << 2)
> +#define MII_CLKRANGE_35_60M                     (3 << 2)
> +#define MII_CLKRANGE_150_250M                   (4 << 2)
> +#define MII_CLKRANGE_250_300M                   (5 << 2)
> +#define MII_CLKRANGE_MASK			GENMASK(4, 2)
> +#define MII_REG_SHIFT				6
> +#define MII_REG_MASK				GENMASK(10, 6)
> +#define MII_ADDR_SHIFT				11
> +#define MII_ADDR_MASK				GENMASK(15, 11)
Is it just me, or is something weird going on with tabs vs. spaces here
(and below in ipq8064_mdio_wait_busy() )?

> +
> +#define MII_DATA_REG_ADDR                       0x14
> +
> +#define MII_MDIO_DELAY_USEC                     (1000)
> +#define MII_MDIO_RETRY_MSEC                     (10)
> +
> +struct ipq8064_mdio {
> +	struct regmap *base; /* NSS_GMAC0_BASE */
> +};
> +
> +static int
> +ipq8064_mdio_wait_busy(struct ipq8064_mdio *priv)
> +{
> +	u32 busy;
> +
> +	return regmap_read_poll_timeout(priv->base, MII_ADDR_REG_ADDR, busy,
> +					!(busy & MII_BUSY), MII_MDIO_DELAY_USEC,
> +					MII_MDIO_RETRY_MSEC * USEC_PER_MSEC);
Didn't know this macro existed. This look much nicer.


> +}
> +
> +static int
> +ipq8064_mdio_read(struct mii_bus *bus, int phy_addr, int reg_offset)
> +{
> +	struct ipq8064_mdio *priv = bus->priv;
> +	u32 miiaddr = MII_BUSY | MII_CLKRANGE_250_300M;
> +	u32 ret_val;
> +	int err;
> +
> +	/* Reject clause 45 */
> +	if (reg_offset & MII_ADDR_C45)
> +		return -EOPNOTSUPP;
Heh, C45 on IPQ806X? Ok, anyone know the hardware or is this some fancy
forward-thinking future-proofing?
(So it this will not break in the future. Not that the SoC of the
ipq8064 could more than 1GBit/s per port from what I know.)

> +
> +	miiaddr |= ((phy_addr << MII_ADDR_SHIFT) & MII_ADDR_MASK) |
> +		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
> +
> +	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> +	usleep_range(10, 20);
Yeeah, this looks a bit different. I did measure with a for-loop how many
udelay(1); a operation took. I can't remember the exact values (I think it
was "8", so the "
	SLEEPING FOR "A FEW" USECS ( < ~10us? ):
		* Use udelay" from the timers-howto.txt applies, right?

But I know that "8" (again, 8 is the stand-in value) would seemed
too bike-sheddy... And looks like it was since this got changed.

> +
> +	err = ipq8064_mdio_wait_busy(priv);
> +	if (err)
> +		return err;
> +
> +	regmap_read(priv->base, MII_DATA_REG_ADDR, &ret_val);
> +	return (int)ret_val;
> +}
> +
> +static int
> +ipq8064_mdio_write(struct mii_bus *bus, int phy_addr, int reg_offset, u16 data)
> +{
> +	struct ipq8064_mdio *priv = bus->priv;
> +	u32 miiaddr = MII_WRITE | MII_BUSY | MII_CLKRANGE_250_300M;
> +
> +	/* Reject clause 45 */
> +	if (reg_offset & MII_ADDR_C45)
> +		return -EOPNOTSUPP;
> +
> +	regmap_write(priv->base, MII_DATA_REG_ADDR, data);
> +
> +	miiaddr |= ((phy_addr << MII_ADDR_SHIFT) & MII_ADDR_MASK) |
> +		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
> +
> +	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> +	usleep_range(10, 20);
> +
> +	return ipq8064_mdio_wait_busy(priv);
> +}
> +
> +static int
> +ipq8064_mdio_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct ipq8064_mdio *priv;
> +	struct mii_bus *bus;
> +	int ret;
> +
> +	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "ipq8064_mdio_bus";
> +	bus->read = ipq8064_mdio_read;
> +	bus->write = ipq8064_mdio_write;
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev));
> +	bus->parent = &pdev->dev;
> +
> +	priv = bus->priv;
> +	priv->base = syscon_node_to_regmap(np);
> +	if (IS_ERR(priv->base) && priv->base != ERR_PTR(-EPROBE_DEFER))
> +		priv->base = syscon_regmap_lookup_by_phandle(np, "master");
> +
> +	if (priv->base == ERR_PTR(-EPROBE_DEFER)) {
> +		return -EPROBE_DEFER;
> +	} else if (IS_ERR(priv->base)) {
> +		dev_err(&pdev->dev, "error getting syscon regmap, error=%pe\n",
> +			priv->base);
> +		return PTR_ERR(priv->base);
> +	}
> +
> +	ret = of_mdiobus_register(bus, np);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, bus);
> +	return 0;
> +}
> +
> +static int
> +ipq8064_mdio_remove(struct platform_device *pdev)
> +{
> +	struct mii_bus *bus = platform_get_drvdata(pdev);
> +
> +	mdiobus_unregister(bus);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ipq8064_mdio_dt_ids[] = {
> +	{ .compatible = "qcom,ipq8064-mdio" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ipq8064_mdio_dt_ids);
> +
> +static struct platform_driver ipq8064_mdio_driver = {
> +	.probe = ipq8064_mdio_probe,
> +	.remove = ipq8064_mdio_remove,
> +	.driver = {
> +		.name = "ipq8064-mdio",
> +		.of_match_table = ipq8064_mdio_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(ipq8064_mdio_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm IPQ8064 MDIO interface driver");
> +MODULE_AUTHOR("Christian Lamparter <chunkeey@gmail.com>");
@Mr. Smith: Don't you want to add yourself there (and in the boilerplate)
as well then? 

> +MODULE_LICENSE("GPL");
> 

Cheers and good night,
Christian

---
[0] <https://git.openwrt.org/?p=openwrt/staging/chunkeey.git;a=blob;f=target/linux/ipq806x/patches-4.19/700-net-mdio-add-ipq8064-mdio-driver.patch;h=6f25b895cacb34b7fcf3e275c15ab26e25252fa8;hb=1034741b8735608b022d55b08df34d4cff423b46>

R: [PATCH v6 1/2] net: mdio: add ipq8064 mdio driver

[ansuelsmth]

> 
> Hi,
> 
> On Saturday, 22 February 2020 17:16:26 CET Mr. Smith wrote:
> > Currently ipq806x soc use generic bitbang driver to
> > comunicate with the gmac ethernet interface.
> > Add a dedicated driver created by chunkeey to fix this.
> 
> Ok, I'm officially a bit "out-of-the-loop" here (was looking for
> a changelog, but I guess I missed that mail). So please excuse me,
> if I say something that was already stated. Instead kindly reply
> with a text extract (preferably with the link to the mail as I'm not
> a regular linux-net subscriber) to that discussion.
> 
> > Co-developed-by: Christian Lamparter <chunkeey@gmail.com>
> Ahh, I see that
> "docs: Clarify the usage and sign-off requirements for Co-developed-by"
> did away with the "original author" wording in the submitting-patches doc.
> So keeping the original "From:" doesn't matter as such anymore.
> 
> Still, for reference (historical digs or if someone wants to take a peek):
> Most of this patch comes from a patch named:
> 700-net-mdio-add-ipq8064-mdio-driver.patch
> 
> which is part of a bigger "ipq8064: ipq8064-mdio + dsa test" commit.
> This is currently in a staging tree hosted on <git.openwrt.org>.
> Here's a direct link: [0] (sadly, this is not self-updating).
> 
> Background: This driver was mainly written to make and test the qca8k
> patches I posted to the linux-net ML last year. The Idea was that I
> didn't have to deal with the odd random timing issues on the slow debug
> kernels, when I was perfing/hammering the device.
> (The IPQ8064 has/had various scaling problems so, this might be already
> fixed by some of Mr. Smith's other work for the abandoned IPQ8064).
> 
> From what I know, this patch mostly helps/fixes a problem with the
> out-of-tree OpenWrt swconfig-based ar8216/ar8236 driver for the
> used QCA8337 switch.
> This driver really needs the faster, more efficient reads due to having
> a statistics worker which is just gobbing cycles because of all the
> exclusive-access bit-banging taking place.
> (Remember, the worker could read all the phy-counters for each of the 7
> ports over gpio-mdio (there have been attempts to make it less hoggy
> in the mean time though). While the IPQ8064 SoC has a beefy dual-core
> Cortex-A15 with up to 1.4GHz*, this bitbanging will result in a
> considerable load on at least one of the cores.)
> 
> Mr. Smith knows more about this though, as he has the hardware and
> is the upcoming IPQ8064 expert on this.
> 
> From my POV, I never anticipated this hack was up to standards of linux-
> net.
> As there is this ugly dependency on having the "master" MAC (handled by
> the
> sttmac/dwmac-ipq806x.c) part up and operational all the time (that's why
> the "master" property is needed for some devices at least).
> 
> I had hopes to do this properly and integrate it into
> dwmac-ipq806x.c, but this requires much more work and ultimately
> "virtual-mdio" was "just good enough™️" for standard, production kernels.
> From what I remember the Qualcomm devs themselves never bothered in
> their
> abandoned posts with the DeviceTree for the qcom-ipq8064.dts to include a
> standalone mdio driver. Instead the dev went straight to "virtual-mdio".
> (but this was too long ago to really remember the details and it's getting
> late)
> 
> But Ok, if linux-net is content with the standalone mdio-ipq8064 approach,
> then: Sure!
> 
Thx a lot for the clarification, I tested your driver for a long time and now I'm
trying to post some of the patch on Openwrt upstream. 
There is no changelog as the changes is mainly the use of the api instead of
the for loop (for the busy wait) and some define rename. 
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 9dabe03a668c..ec2a5493a7e8 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -157,6 +157,14 @@ config MDIO_I2C
> >
> >  	  This is library mode.
> >
> > +config MDIO_IPQ8064
> > +	tristate "Qualcomm IPQ8064 MDIO interface support"
> > +	depends on HAS_IOMEM && OF_MDIO
> > +	depends on MFD_SYSCON
> > +	help
> > +	  This driver supports the MDIO interface found in the network
> > +	  interface units of the IPQ8064 SoC
> > +
> >  config MDIO_MOXART
> >  	tristate "MOXA ART MDIO interface support"
> >  	depends on ARCH_MOXART || COMPILE_TEST
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index fe5badf13b65..8f02bd2089f3 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -36,6 +36,7 @@ obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-
> cavium.o
> >  obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
> >  obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
> >  obj-$(CONFIG_MDIO_I2C)		+= mdio-i2c.o
> > +obj-$(CONFIG_MDIO_IPQ8064)	+= mdio-ipq8064.o
> >  obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
> >  obj-$(CONFIG_MDIO_MSCC_MIIM)	+= mdio-mscc-miim.o
> >  obj-$(CONFIG_MDIO_OCTEON)	+= mdio-octeon.o
> > diff --git a/drivers/net/phy/mdio-ipq8064.c b/drivers/net/phy/mdio-
> ipq8064.c
> > new file mode 100644
> > index 000000000000..74d6b92a6f48
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-ipq8064.c
> > @@ -0,0 +1,166 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Qualcomm IPQ8064 MDIO interface driver
> > +//
> > +// Copyright (C) 2019 Christian Lamparter <chunkeey@gmail.com>
> > +
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +/* MII address register definitions */
> > +#define MII_ADDR_REG_ADDR                       0x10
> > +#define MII_BUSY                                BIT(0)
> > +#define MII_WRITE                               BIT(1)
> > +#define MII_CLKRANGE_60_100M                    (0 << 2)
> > +#define MII_CLKRANGE_100_150M                   (1 << 2)
> > +#define MII_CLKRANGE_20_35M                     (2 << 2)
> > +#define MII_CLKRANGE_35_60M                     (3 << 2)
> > +#define MII_CLKRANGE_150_250M                   (4 << 2)
> > +#define MII_CLKRANGE_250_300M                   (5 << 2)
> > +#define MII_CLKRANGE_MASK			GENMASK(4, 2)
> > +#define MII_REG_SHIFT				6
> > +#define MII_REG_MASK				GENMASK(10, 6)
> > +#define MII_ADDR_SHIFT				11
> > +#define MII_ADDR_MASK				GENMASK(15, 11)
> Is it just me, or is something weird going on with tabs vs. spaces here
> (and below in ipq8064_mdio_wait_busy() )?
> 
> > +
> > +#define MII_DATA_REG_ADDR                       0x14
> > +
> > +#define MII_MDIO_DELAY_USEC                     (1000)
> > +#define MII_MDIO_RETRY_MSEC                     (10)
> > +
> > +struct ipq8064_mdio {
> > +	struct regmap *base; /* NSS_GMAC0_BASE */
> > +};
> > +
> > +static int
> > +ipq8064_mdio_wait_busy(struct ipq8064_mdio *priv)
> > +{
> > +	u32 busy;
> > +
> > +	return regmap_read_poll_timeout(priv->base,
> MII_ADDR_REG_ADDR, busy,
> > +					!(busy & MII_BUSY),
> MII_MDIO_DELAY_USEC,
> > +					MII_MDIO_RETRY_MSEC *
> USEC_PER_MSEC);
> Didn't know this macro existed. This look much nicer.
> 
> 
> > +}
> > +
> > +static int
> > +ipq8064_mdio_read(struct mii_bus *bus, int phy_addr, int reg_offset)
> > +{
> > +	struct ipq8064_mdio *priv = bus->priv;
> > +	u32 miiaddr = MII_BUSY | MII_CLKRANGE_250_300M;
> > +	u32 ret_val;
> > +	int err;
> > +
> > +	/* Reject clause 45 */
> > +	if (reg_offset & MII_ADDR_C45)
> > +		return -EOPNOTSUPP;
> Heh, C45 on IPQ806X? Ok, anyone know the hardware or is this some fancy
> forward-thinking future-proofing?
> (So it this will not break in the future. Not that the SoC of the
> ipq8064 could more than 1GBit/s per port from what I know.)
> 
Yes in v1 it was asked to add explicit reject for 45 Clause
> > +
> > +	miiaddr |= ((phy_addr << MII_ADDR_SHIFT) & MII_ADDR_MASK) |
> > +		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
> > +
> > +	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> > +	usleep_range(10, 20);
> Yeeah, this looks a bit different. I did measure with a for-loop how many
> udelay(1); a operation took. I can't remember the exact values (I think it
> was "8", so the "
> 	SLEEPING FOR "A FEW" USECS ( < ~10us? ):
> 		* Use udelay" from the timers-howto.txt applies, right?
> 
> But I know that "8" (again, 8 is the stand-in value) would seemed
> too bike-sheddy... And looks like it was since this got changed.
> 
Will check this with more test but shouldn't cause any problem right?
> > +
> > +	err = ipq8064_mdio_wait_busy(priv);
> > +	if (err)
> > +		return err;
> > +
> > +	regmap_read(priv->base, MII_DATA_REG_ADDR, &ret_val);
> > +	return (int)ret_val;
> > +}
> > +
> > +static int
> > +ipq8064_mdio_write(struct mii_bus *bus, int phy_addr, int reg_offset,
> u16 data)
> > +{
> > +	struct ipq8064_mdio *priv = bus->priv;
> > +	u32 miiaddr = MII_WRITE | MII_BUSY | MII_CLKRANGE_250_300M;
> > +
> > +	/* Reject clause 45 */
> > +	if (reg_offset & MII_ADDR_C45)
> > +		return -EOPNOTSUPP;
> > +
> > +	regmap_write(priv->base, MII_DATA_REG_ADDR, data);
> > +
> > +	miiaddr |= ((phy_addr << MII_ADDR_SHIFT) & MII_ADDR_MASK) |
> > +		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
> > +
> > +	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> > +	usleep_range(10, 20);
> > +
> > +	return ipq8064_mdio_wait_busy(priv);
> > +}
> > +
> > +static int
> > +ipq8064_mdio_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct ipq8064_mdio *priv;
> > +	struct mii_bus *bus;
> > +	int ret;
> > +
> > +	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	bus->name = "ipq8064_mdio_bus";
> > +	bus->read = ipq8064_mdio_read;
> > +	bus->write = ipq8064_mdio_write;
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev-
> >dev));
> > +	bus->parent = &pdev->dev;
> > +
> > +	priv = bus->priv;
> > +	priv->base = syscon_node_to_regmap(np);
> > +	if (IS_ERR(priv->base) && priv->base != ERR_PTR(-EPROBE_DEFER))
> > +		priv->base = syscon_regmap_lookup_by_phandle(np,
> "master");
> > +
> > +	if (priv->base == ERR_PTR(-EPROBE_DEFER)) {
> > +		return -EPROBE_DEFER;
> > +	} else if (IS_ERR(priv->base)) {
> > +		dev_err(&pdev->dev, "error getting syscon regmap,
> error=%pe\n",
> > +			priv->base);
> > +		return PTR_ERR(priv->base);
> > +	}
> > +
> > +	ret = of_mdiobus_register(bus, np);
> > +	if (ret)
> > +		return ret;
> > +
> > +	platform_set_drvdata(pdev, bus);
> > +	return 0;
> > +}
> > +
> > +static int
> > +ipq8064_mdio_remove(struct platform_device *pdev)
> > +{
> > +	struct mii_bus *bus = platform_get_drvdata(pdev);
> > +
> > +	mdiobus_unregister(bus);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ipq8064_mdio_dt_ids[] = {
> > +	{ .compatible = "qcom,ipq8064-mdio" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ipq8064_mdio_dt_ids);
> > +
> > +static struct platform_driver ipq8064_mdio_driver = {
> > +	.probe = ipq8064_mdio_probe,
> > +	.remove = ipq8064_mdio_remove,
> > +	.driver = {
> > +		.name = "ipq8064-mdio",
> > +		.of_match_table = ipq8064_mdio_dt_ids,
> > +	},
> > +};
> > +
> > +module_platform_driver(ipq8064_mdio_driver);
> > +
> > +MODULE_DESCRIPTION("Qualcomm IPQ8064 MDIO interface driver");
> > +MODULE_AUTHOR("Christian Lamparter <chunkeey@gmail.com>");
> @Mr. Smith: Don't you want to add yourself there (and in the boilerplate)
> as well then?
> 
Since you mainly write the driver I thought it wasn't good to take credits for it.
If you are find with it, I will add also mine.
> > +MODULE_LICENSE("GPL");
> >
> 
> Cheers and good night,
> Christian
> 
> ---
> [0]
> <https://git.openwrt.org/?p=openwrt/staging/chunkeey.git;a=blob;f=targe
> t/linux/ipq806x/patches-4.19/700-net-mdio-add-ipq8064-mdio-
> driver.patch;h=6f25b895cacb34b7fcf3e275c15ab26e25252fa8;hb=103474
> 1b8735608b022d55b08df34d4cff423b46>
> 
> 
> 

Re: [PATCH v6 1/2] net: mdio: add ipq8064 mdio driver

[Russell King - ARM Linux admin]

On Sun, Feb 23, 2020 at 12:47:40AM +0100, Christian Lamparter wrote:
> > +static int
> > +ipq8064_mdio_read(struct mii_bus *bus, int phy_addr, int reg_offset)
> > +{
> > +	struct ipq8064_mdio *priv = bus->priv;
> > +	u32 miiaddr = MII_BUSY | MII_CLKRANGE_250_300M;
> > +	u32 ret_val;
> > +	int err;
> > +
> > +	/* Reject clause 45 */
> > +	if (reg_offset & MII_ADDR_C45)
> > +		return -EOPNOTSUPP;
> Heh, C45 on IPQ806X? Ok, anyone know the hardware or is this some fancy
> forward-thinking future-proofing?

Quite simply, the driver as written does not support C45, so it should
reject it, rather than truncating the "reg_offset" and issuing C22
cycles instead.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v6 2/2] Documentation: devictree: Add ipq806x mdio bindings

[Rob Herring]

On Sat, 22 Feb 2020 17:16:27 +0100, Ansuel Smith wrote:
> Add documentations for ipq806x mdio driver.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  .../bindings/net/qcom,ipq8064-mdio.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qcom,ipq8064-mdio.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/display/simple-framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node
Error: Documentation/devicetree/bindings/net/qcom,ipq8064-mdio.example.dts:23.28-29 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:300: recipe for target 'Documentation/devicetree/bindings/net/qcom,ipq8064-mdio.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/net/qcom,ipq8064-mdio.example.dt.yaml] Error 1
Makefile:1263: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1242533
Please check and re-submit.