diff mbox series

[1/3] net: phy: mdio: add IPQ40xx MDIO driver

Message ID 20200413170107.246509-1-robert.marko@sartura.hr (mailing list archive)
State Superseded
Headers show
Series [1/3] net: phy: mdio: add IPQ40xx MDIO driver | expand

Commit Message

Robert Marko April 13, 2020, 5:01 p.m. UTC 
This patch adds the driver for the MDIO interface
inside of Qualcomm IPQ40xx series SoC-s.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
Cc: Luka Perkov <luka.perkov@sartura.hr>
---
 drivers/net/phy/Kconfig        |   7 ++
 drivers/net/phy/Makefile       |   1 +
 drivers/net/phy/mdio-ipq40xx.c | 180 +++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+)
 create mode 100644 drivers/net/phy/mdio-ipq40xx.c

Comments

Heiner Kallweit April 13, 2020, 5:18 p.m. UTC | #1 
On 13.04.2020 19:01, Robert Marko wrote:
> This patch adds the driver for the MDIO interface
> inside of Qualcomm IPQ40xx series SoC-s.
> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> Cc: Luka Perkov <luka.perkov@sartura.hr>
> ---
>  drivers/net/phy/Kconfig        |   7 ++
>  drivers/net/phy/Makefile       |   1 +
>  drivers/net/phy/mdio-ipq40xx.c | 180 +++++++++++++++++++++++++++++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 drivers/net/phy/mdio-ipq40xx.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 9dabe03a668c..614d08635012 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -157,6 +157,13 @@ config MDIO_I2C
>  
>  	  This is library mode.
>  
> +config MDIO_IPQ40XX
> +	tristate "Qualcomm IPQ40xx MDIO interface"
> +	depends on HAS_IOMEM && OF
> +	help
> +	  This driver supports the MDIO interface found in Qualcomm
> +	  IPQ40xx series Soc-s.
> +
>  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..c89fc187fd74 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_IPQ40XX)	+= mdio-ipq40xx.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-ipq40xx.c b/drivers/net/phy/mdio-ipq40xx.c
> new file mode 100644
> index 000000000000..8068f1e6a077
> --- /dev/null
> +++ b/drivers/net/phy/mdio-ipq40xx.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/* Copyright (c) 2015, The Linux Foundation. All rights reserved. */
> +
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_mdio.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +
> +#define MDIO_CTRL_0_REG		0x40
> +#define MDIO_CTRL_1_REG		0x44
> +#define MDIO_CTRL_2_REG		0x48
> +#define MDIO_CTRL_3_REG		0x4c
> +#define MDIO_CTRL_4_REG		0x50
> +#define MDIO_CTRL_4_ACCESS_BUSY		BIT(16)
> +#define MDIO_CTRL_4_ACCESS_START		BIT(8)
> +#define MDIO_CTRL_4_ACCESS_CODE_READ		0
> +#define MDIO_CTRL_4_ACCESS_CODE_WRITE	1
> +#define CTRL_0_REG_DEFAULT_VALUE	0x150FF
> +
> +#define IPQ40XX_MDIO_RETRY	1000
> +#define IPQ40XX_MDIO_DELAY	10
> +
> +struct ipq40xx_mdio_data {
> +	struct mii_bus	*mii_bus;
> +	void __iomem	*membase;
> +	struct device	*dev;
> +};
> +
> +static int ipq40xx_mdio_wait_busy(struct ipq40xx_mdio_data *am)
> +{
> +	int i;
> +
> +	for (i = 0; i < IPQ40XX_MDIO_RETRY; i++) {
> +		unsigned int busy;
> +
> +		busy = readl(am->membase + MDIO_CTRL_4_REG) &
> +			MDIO_CTRL_4_ACCESS_BUSY;
> +		if (!busy)
> +			return 0;
> +
> +		/* BUSY might take to be cleard by 15~20 times of loop */
> +		udelay(IPQ40XX_MDIO_DELAY);
> +	}
> +
> +	dev_err(am->dev, "%s: MDIO operation timed out\n", am->mii_bus->name);
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int ipq40xx_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> +{
> +	struct ipq40xx_mdio_data *am = bus->priv;
> +	int value = 0;
> +	unsigned int cmd = 0;
> +
> +	lockdep_assert_held(&bus->mdio_lock);
> +
> +	if (ipq40xx_mdio_wait_busy(am))
> +		return -ETIMEDOUT;
> +
> +	/* issue the phy address and reg */
> +	writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
> +
> +	cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_READ;
> +
> +	/* issue read command */
> +	writel(cmd, am->membase + MDIO_CTRL_4_REG);
> +
> +	/* Wait read complete */
> +	if (ipq40xx_mdio_wait_busy(am))
> +		return -ETIMEDOUT;
> +
> +	/* Read data */
> +	value = readl(am->membase + MDIO_CTRL_3_REG);
> +
> +	return value;
> +}
> +
> +static int ipq40xx_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> +							 u16 value)
> +{
> +	struct ipq40xx_mdio_data *am = bus->priv;
> +	unsigned int cmd = 0;
> +
> +	lockdep_assert_held(&bus->mdio_lock);
> +
> +	if (ipq40xx_mdio_wait_busy(am))
> +		return -ETIMEDOUT;
> +
> +	/* issue the phy address and reg */
> +	writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
> +
> +	/* issue write data */
> +	writel(value, am->membase + MDIO_CTRL_2_REG);
> +
> +	cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_WRITE;
> +	/* issue write command */
> +	writel(cmd, am->membase + MDIO_CTRL_4_REG);
> +
> +	/* Wait write complete */
> +	if (ipq40xx_mdio_wait_busy(am))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int ipq40xx_mdio_probe(struct platform_device *pdev)
> +{
> +	struct ipq40xx_mdio_data *am;
> +	struct resource *res;
> +
> +	am = devm_kzalloc(&pdev->dev, sizeof(*am), GFP_KERNEL);
> +	if (!am)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "no iomem resource found\n");
> +		return -ENXIO;
> +	}
> +
> +	am->membase = devm_ioremap_resource(&pdev->dev, res);

You can use devm_platform_ioremap_resource() here.

> +	if (IS_ERR(am->membase)) {
> +		dev_err(&pdev->dev, "unable to ioremap registers\n");
> +		return PTR_ERR(am->membase);
> +	}
> +
> +	am->mii_bus = devm_mdiobus_alloc(&pdev->dev);
> +	if (!am->mii_bus)
> +		return  -ENOMEM;
> +

You could use devm_mdiobus_alloc_size() and omit allocating am
separately.

> +	writel(CTRL_0_REG_DEFAULT_VALUE, am->membase + MDIO_CTRL_0_REG);
> +
> +	am->mii_bus->name = "ipq40xx_mdio";
> +	am->mii_bus->read = ipq40xx_mdio_read;
> +	am->mii_bus->write = ipq40xx_mdio_write;
> +	am->mii_bus->priv = am;
> +	am->mii_bus->parent = &pdev->dev;
> +	snprintf(am->mii_bus->id, MII_BUS_ID_SIZE, "%s", dev_name(&pdev->dev));
> +
> +	am->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, am);
> +
> +	return of_mdiobus_register(am->mii_bus, pdev->dev.of_node);
> +}
> +
> +static int ipq40xx_mdio_remove(struct platform_device *pdev)
> +{
> +	struct ipq40xx_mdio_data *am = platform_get_drvdata(pdev);
> +
> +	mdiobus_unregister(am->mii_bus);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ipq40xx_mdio_dt_ids[] = {
> +	{ .compatible = "qcom,ipq40xx-mdio" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ipq40xx_mdio_dt_ids);
> +
> +static struct platform_driver ipq40xx_mdio_driver = {
> +	.probe = ipq40xx_mdio_probe,
> +	.remove = ipq40xx_mdio_remove,
> +	.driver = {
> +		.name = "ipq40xx-mdio",
> +		.of_match_table = ipq40xx_mdio_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(ipq40xx_mdio_driver);
> +
> +MODULE_DESCRIPTION("IPQ40XX MDIO interface driver");
> +MODULE_AUTHOR("Qualcomm Atheros");
> +MODULE_LICENSE("Dual BSD/GPL");
>
Andrew Lunn April 13, 2020, 6:42 p.m. UTC | #2 
> --- 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_IPQ40XX)	+= mdio-ipq40xx.o
>  obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
>  obj-$(CONFIG_MDIO_MSCC_MIIM)	+= mdio-mscc-miim.o

Hi Robert

That looks odd. What happened to the

obj-$(CONFIG_MDIO_IPQ8064)      += mdio-ipq8064.o

>  obj-$(CONFIG_MDIO_OCTEON)	+= mdio-octeon.o
> diff --git a/drivers/net/phy/mdio-ipq40xx.c b/drivers/net/phy/mdio-ipq40xx.c
> new file mode 100644
> index 000000000000..8068f1e6a077
> --- /dev/null
> +++ b/drivers/net/phy/mdio-ipq40xx.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/* Copyright (c) 2015, The Linux Foundation. All rights reserved. */
> +
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_mdio.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +
> +#define MDIO_CTRL_0_REG		0x40
> +#define MDIO_CTRL_1_REG		0x44
> +#define MDIO_CTRL_2_REG		0x48
> +#define MDIO_CTRL_3_REG		0x4c
> +#define MDIO_CTRL_4_REG		0x50

Can we have better names than as. It seems like 3 is read data, 2 is
write data, etc.

> +#define MDIO_CTRL_4_ACCESS_BUSY		BIT(16)
> +#define MDIO_CTRL_4_ACCESS_START		BIT(8)
> +#define MDIO_CTRL_4_ACCESS_CODE_READ		0
> +#define MDIO_CTRL_4_ACCESS_CODE_WRITE	1
> +#define CTRL_0_REG_DEFAULT_VALUE	0x150FF

No magic numbers please. Try to explain what each of these bits
do. I'm guessing they are clock speed, preamble enable, maybe C22/C45?

> +
> +#define IPQ40XX_MDIO_RETRY	1000
> +#define IPQ40XX_MDIO_DELAY	10
> +
> +struct ipq40xx_mdio_data {
> +	struct mii_bus	*mii_bus;
> +	void __iomem	*membase;
> +	struct device	*dev;
> +};
> +
> +static int ipq40xx_mdio_wait_busy(struct ipq40xx_mdio_data *am)
> +{
> +	int i;
> +
> +	for (i = 0; i < IPQ40XX_MDIO_RETRY; i++) {
> +		unsigned int busy;
> +
> +		busy = readl(am->membase + MDIO_CTRL_4_REG) &
> +			MDIO_CTRL_4_ACCESS_BUSY;
> +		if (!busy)
> +			return 0;
> +
> +		/* BUSY might take to be cleard by 15~20 times of loop */
> +		udelay(IPQ40XX_MDIO_DELAY);
> +	}
> +
> +	dev_err(am->dev, "%s: MDIO operation timed out\n", am->mii_bus->name);

dev_err() should give you enough to identify the device. No need to
print am->mii_bus->name as well.

> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int ipq40xx_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> +{
> +	struct ipq40xx_mdio_data *am = bus->priv;
> +	int value = 0;
> +	unsigned int cmd = 0;
> +
> +	lockdep_assert_held(&bus->mdio_lock);

Do you think the core is broken?

Please check if the request is for a C45 read, and return -EOPNOTSUPP
if so.


> +
> +	if (ipq40xx_mdio_wait_busy(am))
> +		return -ETIMEDOUT;
> +
> +	/* issue the phy address and reg */
> +	writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
> +
> +	cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_READ;
> +
> +	/* issue read command */
> +	writel(cmd, am->membase + MDIO_CTRL_4_REG);
> +
> +	/* Wait read complete */
> +	if (ipq40xx_mdio_wait_busy(am))
> +		return -ETIMEDOUT;
> +
> +	/* Read data */
> +	value = readl(am->membase + MDIO_CTRL_3_REG);
> +
> +	return value;
> +}
> +
> +static int ipq40xx_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> +							 u16 value)
> +{
> +	struct ipq40xx_mdio_data *am = bus->priv;
> +	unsigned int cmd = 0;
> +
> +	lockdep_assert_held(&bus->mdio_lock);
> +
> +	if (ipq40xx_mdio_wait_busy(am))
> +		return -ETIMEDOUT;
> +
> +	/* issue the phy address and reg */
> +	writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
> +
> +	/* issue write data */
> +	writel(value, am->membase + MDIO_CTRL_2_REG);
> +
> +	cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_WRITE;
> +	/* issue write command */
> +	writel(cmd, am->membase + MDIO_CTRL_4_REG);
> +
> +	/* Wait write complete */
> +	if (ipq40xx_mdio_wait_busy(am))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int ipq40xx_mdio_probe(struct platform_device *pdev)
> +{
> +	struct ipq40xx_mdio_data *am;

Why the name am? Generally priv is used. I could also understand bus,
or even data, but am?

   Andrew
Robert Marko April 14, 2020, 6:03 p.m. UTC | #3 
On Mon, Apr 13, 2020 at 8:42 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > --- 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_IPQ40XX)   += mdio-ipq40xx.o
> >  obj-$(CONFIG_MDIO_MOXART)    += mdio-moxart.o
> >  obj-$(CONFIG_MDIO_MSCC_MIIM) += mdio-mscc-miim.o
>
> Hi Robert
>
> That looks odd. What happened to the
>
> obj-$(CONFIG_MDIO_IPQ8064)      += mdio-ipq8064.o
>
Sorry, this was based off kernel 5.6 instead of net-next.
> >  obj-$(CONFIG_MDIO_OCTEON)    += mdio-octeon.o
> > diff --git a/drivers/net/phy/mdio-ipq40xx.c b/drivers/net/phy/mdio-ipq40xx.c
> > new file mode 100644
> > index 000000000000..8068f1e6a077
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-ipq40xx.c
> > @@ -0,0 +1,180 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> > +/* Copyright (c) 2015, The Linux Foundation. All rights reserved. */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define MDIO_CTRL_0_REG              0x40
> > +#define MDIO_CTRL_1_REG              0x44
> > +#define MDIO_CTRL_2_REG              0x48
> > +#define MDIO_CTRL_3_REG              0x4c
> > +#define MDIO_CTRL_4_REG              0x50
>
> Can we have better names than as. It seems like 3 is read data, 2 is
> write data, etc.
I agree these ones don't really tell whats the function.
Unfortunately, I don't have access to documentation and this is all based on
GPL code from Qualcomm's SDK.
So I don't really know whats their purpose.
It has been floating around for years, so I thought that it would be good to
clean it up and upstream.
>
> > +#define MDIO_CTRL_4_ACCESS_BUSY              BIT(16)
> > +#define MDIO_CTRL_4_ACCESS_START             BIT(8)
> > +#define MDIO_CTRL_4_ACCESS_CODE_READ         0
> > +#define MDIO_CTRL_4_ACCESS_CODE_WRITE        1
> > +#define CTRL_0_REG_DEFAULT_VALUE     0x150FF
>
> No magic numbers please. Try to explain what each of these bits
> do. I'm guessing they are clock speed, preamble enable, maybe C22/C45?
Fortunately, it seems that this is a leftover from early preproduction HW as
my test boards work without it.
Something similar was the case in other Qualcomm SDK drivers.
So, this will be dropped in v2.
>
> > +
> > +#define IPQ40XX_MDIO_RETRY   1000
> > +#define IPQ40XX_MDIO_DELAY   10
> > +
> > +struct ipq40xx_mdio_data {
> > +     struct mii_bus  *mii_bus;
> > +     void __iomem    *membase;
> > +     struct device   *dev;
> > +};
> > +
> > +static int ipq40xx_mdio_wait_busy(struct ipq40xx_mdio_data *am)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < IPQ40XX_MDIO_RETRY; i++) {
> > +             unsigned int busy;
> > +
> > +             busy = readl(am->membase + MDIO_CTRL_4_REG) &
> > +                     MDIO_CTRL_4_ACCESS_BUSY;
> > +             if (!busy)
> > +                     return 0;
> > +
> > +             /* BUSY might take to be cleard by 15~20 times of loop */
> > +             udelay(IPQ40XX_MDIO_DELAY);
> > +     }
> > +
> > +     dev_err(am->dev, "%s: MDIO operation timed out\n", am->mii_bus->name);
>
> dev_err() should give you enough to identify the device. No need to
> print am->mii_bus->name as well.
It will be fixed in v2, thanks.
>
> > +
> > +     return -ETIMEDOUT;
> > +}
> > +
> > +static int ipq40xx_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> > +{
> > +     struct ipq40xx_mdio_data *am = bus->priv;
> > +     int value = 0;
> > +     unsigned int cmd = 0;
> > +
> > +     lockdep_assert_held(&bus->mdio_lock);
>
> Do you think the core is broken?
No, this is also an old relic from the SDK driver.
It works without this perfectly fine, so I will drop it from v2.
>
> Please check if the request is for a C45 read, and return -EOPNOTSUPP
> if so.
It will be added to v2, thanks.
>
>
> > +
> > +     if (ipq40xx_mdio_wait_busy(am))
> > +             return -ETIMEDOUT;
> > +
> > +     /* issue the phy address and reg */
> > +     writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
> > +
> > +     cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_READ;
> > +
> > +     /* issue read command */
> > +     writel(cmd, am->membase + MDIO_CTRL_4_REG);
> > +
> > +     /* Wait read complete */
> > +     if (ipq40xx_mdio_wait_busy(am))
> > +             return -ETIMEDOUT;
> > +
> > +     /* Read data */
> > +     value = readl(am->membase + MDIO_CTRL_3_REG);
> > +
> > +     return value;
> > +}
> > +
> > +static int ipq40xx_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> > +                                                      u16 value)
> > +{
> > +     struct ipq40xx_mdio_data *am = bus->priv;
> > +     unsigned int cmd = 0;
> > +
> > +     lockdep_assert_held(&bus->mdio_lock);
> > +
> > +     if (ipq40xx_mdio_wait_busy(am))
> > +             return -ETIMEDOUT;
> > +
> > +     /* issue the phy address and reg */
> > +     writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
> > +
> > +     /* issue write data */
> > +     writel(value, am->membase + MDIO_CTRL_2_REG);
> > +
> > +     cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_WRITE;
> > +     /* issue write command */
> > +     writel(cmd, am->membase + MDIO_CTRL_4_REG);
> > +
> > +     /* Wait write complete */
> > +     if (ipq40xx_mdio_wait_busy(am))
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static int ipq40xx_mdio_probe(struct platform_device *pdev)
> > +{
> > +     struct ipq40xx_mdio_data *am;
>
> Why the name am? Generally priv is used. I could also understand bus,
> or even data, but am?
Like most stuff in this driver, its a leftover from the SDK driver.
I have changed it to priv in v2, also I switched the driver to
devm_mdiobus_alloc_size() to
try and simplify/modernize the driver also.

Thanks for the suggestions.
I will send a v2 soon.

Best regards,
Andrew
>
>    Andrew
Robert Marko April 14, 2020, 6:15 p.m. UTC | #4 
On Mon, Apr 13, 2020 at 7:18 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 13.04.2020 19:01, Robert Marko wrote:
> > This patch adds the driver for the MDIO interface
> > inside of Qualcomm IPQ40xx series SoC-s.
> >
> > Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > Cc: Luka Perkov <luka.perkov@sartura.hr>
> > ---
> >  drivers/net/phy/Kconfig        |   7 ++
> >  drivers/net/phy/Makefile       |   1 +
> >  drivers/net/phy/mdio-ipq40xx.c | 180 +++++++++++++++++++++++++++++++++
> >  3 files changed, 188 insertions(+)
> >  create mode 100644 drivers/net/phy/mdio-ipq40xx.c
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 9dabe03a668c..614d08635012 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -157,6 +157,13 @@ config MDIO_I2C
> >
> >         This is library mode.
> >
> > +config MDIO_IPQ40XX
> > +     tristate "Qualcomm IPQ40xx MDIO interface"
> > +     depends on HAS_IOMEM && OF
> > +     help
> > +       This driver supports the MDIO interface found in Qualcomm
> > +       IPQ40xx series Soc-s.
> > +
> >  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..c89fc187fd74 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_IPQ40XX)   += mdio-ipq40xx.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-ipq40xx.c b/drivers/net/phy/mdio-ipq40xx.c
> > new file mode 100644
> > index 000000000000..8068f1e6a077
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-ipq40xx.c
> > @@ -0,0 +1,180 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> > +/* Copyright (c) 2015, The Linux Foundation. All rights reserved. */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/io.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define MDIO_CTRL_0_REG              0x40
> > +#define MDIO_CTRL_1_REG              0x44
> > +#define MDIO_CTRL_2_REG              0x48
> > +#define MDIO_CTRL_3_REG              0x4c
> > +#define MDIO_CTRL_4_REG              0x50
> > +#define MDIO_CTRL_4_ACCESS_BUSY              BIT(16)
> > +#define MDIO_CTRL_4_ACCESS_START             BIT(8)
> > +#define MDIO_CTRL_4_ACCESS_CODE_READ         0
> > +#define MDIO_CTRL_4_ACCESS_CODE_WRITE        1
> > +#define CTRL_0_REG_DEFAULT_VALUE     0x150FF
> > +
> > +#define IPQ40XX_MDIO_RETRY   1000
> > +#define IPQ40XX_MDIO_DELAY   10
> > +
> > +struct ipq40xx_mdio_data {
> > +     struct mii_bus  *mii_bus;
> > +     void __iomem    *membase;
> > +     struct device   *dev;
> > +};
> > +
> > +static int ipq40xx_mdio_wait_busy(struct ipq40xx_mdio_data *am)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < IPQ40XX_MDIO_RETRY; i++) {
> > +             unsigned int busy;
> > +
> > +             busy = readl(am->membase + MDIO_CTRL_4_REG) &
> > +                     MDIO_CTRL_4_ACCESS_BUSY;
> > +             if (!busy)
> > +                     return 0;
> > +
> > +             /* BUSY might take to be cleard by 15~20 times of loop */
> > +             udelay(IPQ40XX_MDIO_DELAY);
> > +     }
> > +
> > +     dev_err(am->dev, "%s: MDIO operation timed out\n", am->mii_bus->name);
> > +
> > +     return -ETIMEDOUT;
> > +}
> > +
> > +static int ipq40xx_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> > +{
> > +     struct ipq40xx_mdio_data *am = bus->priv;
> > +     int value = 0;
> > +     unsigned int cmd = 0;
> > +
> > +     lockdep_assert_held(&bus->mdio_lock);
> > +
> > +     if (ipq40xx_mdio_wait_busy(am))
> > +             return -ETIMEDOUT;
> > +
> > +     /* issue the phy address and reg */
> > +     writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
> > +
> > +     cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_READ;
> > +
> > +     /* issue read command */
> > +     writel(cmd, am->membase + MDIO_CTRL_4_REG);
> > +
> > +     /* Wait read complete */
> > +     if (ipq40xx_mdio_wait_busy(am))
> > +             return -ETIMEDOUT;
> > +
> > +     /* Read data */
> > +     value = readl(am->membase + MDIO_CTRL_3_REG);
> > +
> > +     return value;
> > +}
> > +
> > +static int ipq40xx_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> > +                                                      u16 value)
> > +{
> > +     struct ipq40xx_mdio_data *am = bus->priv;
> > +     unsigned int cmd = 0;
> > +
> > +     lockdep_assert_held(&bus->mdio_lock);
> > +
> > +     if (ipq40xx_mdio_wait_busy(am))
> > +             return -ETIMEDOUT;
> > +
> > +     /* issue the phy address and reg */
> > +     writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
> > +
> > +     /* issue write data */
> > +     writel(value, am->membase + MDIO_CTRL_2_REG);
> > +
> > +     cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_WRITE;
> > +     /* issue write command */
> > +     writel(cmd, am->membase + MDIO_CTRL_4_REG);
> > +
> > +     /* Wait write complete */
> > +     if (ipq40xx_mdio_wait_busy(am))
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static int ipq40xx_mdio_probe(struct platform_device *pdev)
> > +{
> > +     struct ipq40xx_mdio_data *am;
> > +     struct resource *res;
> > +
> > +     am = devm_kzalloc(&pdev->dev, sizeof(*am), GFP_KERNEL);
> > +     if (!am)
> > +             return -ENOMEM;
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     if (!res) {
> > +             dev_err(&pdev->dev, "no iomem resource found\n");
> > +             return -ENXIO;
> > +     }
> > +
> > +     am->membase = devm_ioremap_resource(&pdev->dev, res);
>
> You can use devm_platform_ioremap_resource() here.
Thanks, its now used in v2.
>
> > +     if (IS_ERR(am->membase)) {
> > +             dev_err(&pdev->dev, "unable to ioremap registers\n");
> > +             return PTR_ERR(am->membase);
> > +     }
> > +
> > +     am->mii_bus = devm_mdiobus_alloc(&pdev->dev);
> > +     if (!am->mii_bus)
> > +             return  -ENOMEM;
> > +
>
> You could use devm_mdiobus_alloc_size() and omit allocating am
> separately.
Thanks, I switched to it in v2 along some other improvements.
>
> > +     writel(CTRL_0_REG_DEFAULT_VALUE, am->membase + MDIO_CTRL_0_REG);
> > +
> > +     am->mii_bus->name = "ipq40xx_mdio";
> > +     am->mii_bus->read = ipq40xx_mdio_read;
> > +     am->mii_bus->write = ipq40xx_mdio_write;
> > +     am->mii_bus->priv = am;
> > +     am->mii_bus->parent = &pdev->dev;
> > +     snprintf(am->mii_bus->id, MII_BUS_ID_SIZE, "%s", dev_name(&pdev->dev));
> > +
> > +     am->dev = &pdev->dev;
> > +     platform_set_drvdata(pdev, am);
> > +
> > +     return of_mdiobus_register(am->mii_bus, pdev->dev.of_node);
> > +}
> > +
> > +static int ipq40xx_mdio_remove(struct platform_device *pdev)
> > +{
> > +     struct ipq40xx_mdio_data *am = platform_get_drvdata(pdev);
> > +
> > +     mdiobus_unregister(am->mii_bus);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id ipq40xx_mdio_dt_ids[] = {
> > +     { .compatible = "qcom,ipq40xx-mdio" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ipq40xx_mdio_dt_ids);
> > +
> > +static struct platform_driver ipq40xx_mdio_driver = {
> > +     .probe = ipq40xx_mdio_probe,
> > +     .remove = ipq40xx_mdio_remove,
> > +     .driver = {
> > +             .name = "ipq40xx-mdio",
> > +             .of_match_table = ipq40xx_mdio_dt_ids,
> > +     },
> > +};
> > +
> > +module_platform_driver(ipq40xx_mdio_driver);
> > +
> > +MODULE_DESCRIPTION("IPQ40XX MDIO interface driver");
> > +MODULE_AUTHOR("Qualcomm Atheros");
> > +MODULE_LICENSE("Dual BSD/GPL");
> >
>
Andrew Lunn April 14, 2020, 6:36 p.m. UTC | #5 
> Unfortunately, I don't have access to documentation and this is all based on
> GPL code from Qualcomm's SDK.
> So I don't really know whats their purpose.

Then please try to reverse engineer it. Just looking at the code, it
seems like one register is read, and the other is write. So use that
in the name.

> No, this is also an old relic from the SDK driver.
> It works without this perfectly fine, so I will drop it from v2.

Part of cleaning up 'Vendor Crap' is throwing out all the stuff like
this. What you end up with should be something you would of been happy
to write yourself.

> > Why the name am? Generally priv is used. I could also understand bus,
> > or even data, but am?
> Like most stuff in this driver, its a leftover from the SDK driver.

I guessed as much. But this is the sort of thing you need to fix when
cleaning up 'vendor crap'.

	 Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 9dabe03a668c..614d08635012 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -157,6 +157,13 @@  config MDIO_I2C
 
 	  This is library mode.
 
+config MDIO_IPQ40XX
+	tristate "Qualcomm IPQ40xx MDIO interface"
+	depends on HAS_IOMEM && OF
+	help
+	  This driver supports the MDIO interface found in Qualcomm
+	  IPQ40xx series Soc-s.
+
 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..c89fc187fd74 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_IPQ40XX)	+= mdio-ipq40xx.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-ipq40xx.c b/drivers/net/phy/mdio-ipq40xx.c
new file mode 100644
index 000000000000..8068f1e6a077
--- /dev/null
+++ b/drivers/net/phy/mdio-ipq40xx.c
@@ -0,0 +1,180 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+/* Copyright (c) 2015, The Linux Foundation. All rights reserved. */
+
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+
+#define MDIO_CTRL_0_REG		0x40
+#define MDIO_CTRL_1_REG		0x44
+#define MDIO_CTRL_2_REG		0x48
+#define MDIO_CTRL_3_REG		0x4c
+#define MDIO_CTRL_4_REG		0x50
+#define MDIO_CTRL_4_ACCESS_BUSY		BIT(16)
+#define MDIO_CTRL_4_ACCESS_START		BIT(8)
+#define MDIO_CTRL_4_ACCESS_CODE_READ		0
+#define MDIO_CTRL_4_ACCESS_CODE_WRITE	1
+#define CTRL_0_REG_DEFAULT_VALUE	0x150FF
+
+#define IPQ40XX_MDIO_RETRY	1000
+#define IPQ40XX_MDIO_DELAY	10
+
+struct ipq40xx_mdio_data {
+	struct mii_bus	*mii_bus;
+	void __iomem	*membase;
+	struct device	*dev;
+};
+
+static int ipq40xx_mdio_wait_busy(struct ipq40xx_mdio_data *am)
+{
+	int i;
+
+	for (i = 0; i < IPQ40XX_MDIO_RETRY; i++) {
+		unsigned int busy;
+
+		busy = readl(am->membase + MDIO_CTRL_4_REG) &
+			MDIO_CTRL_4_ACCESS_BUSY;
+		if (!busy)
+			return 0;
+
+		/* BUSY might take to be cleard by 15~20 times of loop */
+		udelay(IPQ40XX_MDIO_DELAY);
+	}
+
+	dev_err(am->dev, "%s: MDIO operation timed out\n", am->mii_bus->name);
+
+	return -ETIMEDOUT;
+}
+
+static int ipq40xx_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+{
+	struct ipq40xx_mdio_data *am = bus->priv;
+	int value = 0;
+	unsigned int cmd = 0;
+
+	lockdep_assert_held(&bus->mdio_lock);
+
+	if (ipq40xx_mdio_wait_busy(am))
+		return -ETIMEDOUT;
+
+	/* issue the phy address and reg */
+	writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
+
+	cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_READ;
+
+	/* issue read command */
+	writel(cmd, am->membase + MDIO_CTRL_4_REG);
+
+	/* Wait read complete */
+	if (ipq40xx_mdio_wait_busy(am))
+		return -ETIMEDOUT;
+
+	/* Read data */
+	value = readl(am->membase + MDIO_CTRL_3_REG);
+
+	return value;
+}
+
+static int ipq40xx_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
+							 u16 value)
+{
+	struct ipq40xx_mdio_data *am = bus->priv;
+	unsigned int cmd = 0;
+
+	lockdep_assert_held(&bus->mdio_lock);
+
+	if (ipq40xx_mdio_wait_busy(am))
+		return -ETIMEDOUT;
+
+	/* issue the phy address and reg */
+	writel((mii_id << 8) | regnum, am->membase + MDIO_CTRL_1_REG);
+
+	/* issue write data */
+	writel(value, am->membase + MDIO_CTRL_2_REG);
+
+	cmd = MDIO_CTRL_4_ACCESS_START | MDIO_CTRL_4_ACCESS_CODE_WRITE;
+	/* issue write command */
+	writel(cmd, am->membase + MDIO_CTRL_4_REG);
+
+	/* Wait write complete */
+	if (ipq40xx_mdio_wait_busy(am))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int ipq40xx_mdio_probe(struct platform_device *pdev)
+{
+	struct ipq40xx_mdio_data *am;
+	struct resource *res;
+
+	am = devm_kzalloc(&pdev->dev, sizeof(*am), GFP_KERNEL);
+	if (!am)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no iomem resource found\n");
+		return -ENXIO;
+	}
+
+	am->membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(am->membase)) {
+		dev_err(&pdev->dev, "unable to ioremap registers\n");
+		return PTR_ERR(am->membase);
+	}
+
+	am->mii_bus = devm_mdiobus_alloc(&pdev->dev);
+	if (!am->mii_bus)
+		return  -ENOMEM;
+
+	writel(CTRL_0_REG_DEFAULT_VALUE, am->membase + MDIO_CTRL_0_REG);
+
+	am->mii_bus->name = "ipq40xx_mdio";
+	am->mii_bus->read = ipq40xx_mdio_read;
+	am->mii_bus->write = ipq40xx_mdio_write;
+	am->mii_bus->priv = am;
+	am->mii_bus->parent = &pdev->dev;
+	snprintf(am->mii_bus->id, MII_BUS_ID_SIZE, "%s", dev_name(&pdev->dev));
+
+	am->dev = &pdev->dev;
+	platform_set_drvdata(pdev, am);
+
+	return of_mdiobus_register(am->mii_bus, pdev->dev.of_node);
+}
+
+static int ipq40xx_mdio_remove(struct platform_device *pdev)
+{
+	struct ipq40xx_mdio_data *am = platform_get_drvdata(pdev);
+
+	mdiobus_unregister(am->mii_bus);
+
+	return 0;
+}
+
+static const struct of_device_id ipq40xx_mdio_dt_ids[] = {
+	{ .compatible = "qcom,ipq40xx-mdio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ipq40xx_mdio_dt_ids);
+
+static struct platform_driver ipq40xx_mdio_driver = {
+	.probe = ipq40xx_mdio_probe,
+	.remove = ipq40xx_mdio_remove,
+	.driver = {
+		.name = "ipq40xx-mdio",
+		.of_match_table = ipq40xx_mdio_dt_ids,
+	},
+};
+
+module_platform_driver(ipq40xx_mdio_driver);
+
+MODULE_DESCRIPTION("IPQ40XX MDIO interface driver");
+MODULE_AUTHOR("Qualcomm Atheros");
+MODULE_LICENSE("Dual BSD/GPL");