Message-ID: <014b01d5ea5f$de7f6020$9b7e2060$@gmail.com>
Date:   Sun, 23 Feb 2020 16:42:33 +0100
From:   <ansuelsmth@...il.com>
To:     "'Christian Lamparter'" <chunkeey@...il.com>
Cc:     "'Andy Gross'" <agross@...nel.org>,
        "'Bjorn Andersson'" <bjorn.andersson@...aro.org>,
        "'David S. Miller'" <davem@...emloft.net>,
        "'Rob Herring'" <robh+dt@...nel.org>,
        "'Mark Rutland'" <mark.rutland@....com>,
        "'Andrew Lunn'" <andrew@...n.ch>,
        "'Florian Fainelli'" <f.fainelli@...il.com>,
        "'Heiner Kallweit'" <hkallweit1@...il.com>,
        "'Russell King'" <linux@...linux.org.uk>,
        <linux-arm-msm@...r.kernel.org>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: R: [PATCH v6 1/2] net: mdio: add ipq8064 mdio driver

> 
> 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@...il.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@...il.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@...il.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>
> 
> 
> 

Powered by blists - more mailing lists