| Message ID | 1383317673-14704-1-git-send-email-jonas.jensen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Jonas, 2013/11/1 Jonas Jensen <jonas.jensen@gmail.com>: > The MOXA UC-711X hardware(s) has an ethernet controller that seem > to be developed internally. The IC used is "RTL8201CP". > > This patch adds an MDIO driver and also patches realtek to include > RTL8201CP PHY driver. > > Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com> > --- > > Notes: > The hardware does not use a separate IRQ for PHY. > > The link state change interrupt can instead be caught by MAC but the > current drivers/of/of_mdio.c does not allow it to be handled in MAC. > > Applies to next-20131031 > > .../devicetree/bindings/net/moxa,moxart-mdio.txt | 19 ++ > drivers/net/phy/Kconfig | 7 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/mdio-moxart.c | 201 +++++++++++++++++++++ > drivers/net/phy/realtek.c | 15 ++ > 5 files changed, 243 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > create mode 100644 drivers/net/phy/mdio-moxart.c > > diff --git a/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt b/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > new file mode 100644 > index 0000000..de0b90c > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > @@ -0,0 +1,19 @@ > +* MOXA ART MDIO Ethernet Controller interface > + > +Required properties: > +- compatible: should be "moxa,moxart-mdio". > +- reg: address and length of the register set for the device. > + > +Example: > +mdio1: mdio@92000090 { > + compatible = "moxa,moxart-mdio"; > + reg = <0x92000090 0x8>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethphy1: ethernet-phy@1 { > + device_type = "ethernet-phy"; > + compatible = "moxa,moxart-rtl8201cp"; > + reg = <1>; > + }; > +}; > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 342561a..9b5d46c 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -154,6 +154,13 @@ config MDIO_SUN4I > interface units of the Allwinner SoC that have an EMAC (A10, > A12, A10s, etc.) > > +config MDIO_MOXART > + tristate "MOXA ART MDIO interface support" > + depends on ARCH_MOXART > + help > + This driver supports the MDIO interface found in the network > + interface units of the MOXA ART SoC > + > config MDIO_BUS_MUX > tristate > depends on OF_MDIO > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index 23a2ab2..9013dfa 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_MDIO_BUS_MUX) += mdio-mux.o > obj-$(CONFIG_MDIO_BUS_MUX_GPIO) += mdio-mux-gpio.o > obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o > obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o > +obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o > diff --git a/drivers/net/phy/mdio-moxart.c b/drivers/net/phy/mdio-moxart.c > new file mode 100644 > index 0000000..ad5d0f8 > --- /dev/null > +++ b/drivers/net/phy/mdio-moxart.c > @@ -0,0 +1,201 @@ > +/* MOXA ART Ethernet (RTL8201CP) MDIO interface driver > + * > + * Copyright (C) 2013 Jonas Jensen <jonas.jensen@gmail.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/delay.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of_address.h> > +#include <linux/of_mdio.h> > +#include <linux/phy.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > + > +#define REG_PHY_CTRL 0 > +#define REG_PHY_WRITE_DATA 4 > + > +/* REG_PHY_CTRL */ > +#define MIIWR BIT(27) /* init write sequence (auto cleared)*/ > +#define MIIRD BIT(26) > +#define REGAD_MASK 0x3e00000 > +#define PHYAD_MASK 0x1f0000 > +#define MIIRDATA_MASK 0xffff > + > +/* REG_PHY_WRITE_DATA */ > +#define MIIWDATA_MASK 0xffff > + > +struct moxart_mdio_data { > + void __iomem *base; > +}; > + > +static int moxart_mdio_read(struct mii_bus *bus, int mii_id, int regnum) > +{ > + struct moxart_mdio_data *data = bus->priv; > + u32 ctrl = 0; > + unsigned int count = 5; > + > + dev_dbg(&bus->dev, "%s\n", __func__); > + > + ctrl |= MIIRD | ((mii_id << 16) & PHYAD_MASK) | > + ((regnum << 21) & REGAD_MASK); > + > + writel(ctrl, data->base + REG_PHY_CTRL); > + > + do { > + ctrl = readl(data->base + REG_PHY_CTRL); > + > + if (!(ctrl & MIIRD)) > + return ctrl & MIIRDATA_MASK; > + > + mdelay(10); > + count--; > + } while (count > 0); > + > + dev_err(&bus->dev, "%s timed out\n", __func__); I would keep these as a debugging aid and not spawn error messages on the console by default. > + > + return -ETIMEDOUT; > +} > + > +static int moxart_mdio_write(struct mii_bus *bus, int mii_id, > + int regnum, u16 value) > +{ > + struct moxart_mdio_data *data = bus->priv; > + u32 ctrl = 0; > + unsigned int count = 5; > + > + dev_dbg(&bus->dev, "%s\n", __func__); > + > + ctrl |= MIIWR | ((mii_id << 16) & PHYAD_MASK) | > + ((regnum << 21) & REGAD_MASK); > + > + value &= MIIWDATA_MASK; > + > + writel(value, data->base + REG_PHY_WRITE_DATA); > + writel(ctrl, data->base + REG_PHY_CTRL); > + > + do { > + ctrl = readl(data->base + REG_PHY_CTRL); > + > + if (!(ctrl & MIIWR)) > + return 0; > + > + mdelay(10); > + count--; > + } while (count > 0); > + > + dev_err(&bus->dev, "%s timed out\n", __func__); Ditto. > + > + return -ETIMEDOUT; > +} > + > +static int moxart_mdio_reset(struct mii_bus *bus) > +{ > + int data, i; > + > + for (i = 0; i < PHY_MAX_ADDR; i++) { > + data = moxart_mdio_read(bus, i, MII_BMCR); > + if (data < 0) > + continue; > + > + data |= BMCR_RESET; > + if (moxart_mdio_write(bus, i, MII_BMCR, data) < 0) > + continue; > + } > + > + return 0; > +} > + > +static int moxart_mdio_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct mii_bus *bus; > + struct moxart_mdio_data *data; > + struct resource *res; > + int ret, i; > + > + bus = mdiobus_alloc_size(sizeof(*data)); > + if (!bus) > + return -ENOMEM; > + > + bus->name = "MOXA ART Ethernet MII"; > + bus->read = &moxart_mdio_read; > + bus->write = &moxart_mdio_write; > + bus->reset = &moxart_mdio_reset; > + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev)); If you only support device tree probing, this might be fine, but you might want to be safe in case someone does a !OF instantiation and also use pdev->id as an additional unique identifier, so something like: %s-%d-mii, pdev->name, pdev->id will work. > + bus->parent = &pdev->dev; > + > + bus->irq = devm_kzalloc(&pdev->dev, sizeof(int) * PHY_MAX_ADDR, > + GFP_KERNEL); > + if (!bus->irq) { > + ret = -ENOMEM; > + goto err_out_free_mdiobus; > + } > + > + /* Setting PHY_IGNORE_INTERRUPT here even if it has no effect, > + * of_mdiobus_register() sets these PHY_POLL. > + * Ideally, the interrupt from MAC controller could be used to > + * detect link state changes, not polling, i.e. if there was > + * a way phy_driver could set PHY_HAS_INTERRUPT but have that > + * interrupt handled in ethernet drivercode. > + */ > + for (i = 0; i < PHY_MAX_ADDR; i++) > + bus->irq[i] = PHY_IGNORE_INTERRUPT; This type of configuration where the PHY interrupt is actually serviced by a link interrupt bit in the Ethernet MAC driver now works since 5ea94e768 ("phy: add phy_mac_interrupt() to use with PHY_IGNORE_INTERRUPT") so setting PHY_IGNORE_INTERRUPT is the right way to signal this and this will no longer make the PHY library poll for the link state. > + > + data = bus->priv; > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->base)) { > + ret = PTR_ERR(data->base); > + goto err_out_free_mdiobus; > + } > + > + ret = of_mdiobus_register(bus, np); > + if (ret < 0) > + return ret; You still need to call mdiobus_free() here in case registration fails. > + > + platform_set_drvdata(pdev, bus); > + > + return 0; > + > +err_out_free_mdiobus: > + mdiobus_free(bus); > + return ret; > +} > + > +static int moxart_mdio_remove(struct platform_device *pdev) > +{ > + struct mii_bus *bus = platform_get_drvdata(pdev); > + > + mdiobus_unregister(bus); > + mdiobus_free(bus); > + > + return 0; > +} > + > +static const struct of_device_id moxart_mdio_dt_ids[] = { > + { .compatible = "moxa,moxart-mdio" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, moxart_mdio_dt_ids); > + > +static struct platform_driver moxart_mdio_driver = { > + .probe = moxart_mdio_probe, > + .remove = moxart_mdio_remove, > + .driver = { > + .name = "moxart-mdio", > + .of_match_table = moxart_mdio_dt_ids, > + }, > +}; > + > +module_platform_driver(moxart_mdio_driver); > + > +MODULE_DESCRIPTION("MOXA ART MDIO interface driver"); > +MODULE_AUTHOR("Jonas Jensen <jonas.jensen@gmail.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index 138de83..fa1d69a 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -64,6 +64,18 @@ static int rtl8211e_config_intr(struct phy_device *phydev) > return err; > } > > +/* RTL8201CP */ > +static struct phy_driver rtl8201cp_driver = { > + .phy_id = 0x00008201, > + .name = "RTL8201CP Ethernet", > + .phy_id_mask = 0x0000ffff, > + .features = PHY_BASIC_FEATURES, If this PHY is internal to your SoC (same die/package) you should also add PHY_IS_INTERNAL to get a consistent ethtool reporting (among others).
Hi Jonas, On Friday 01 November 2013 08:24 PM, Jonas Jensen wrote: > The MOXA UC-711X hardware(s) has an ethernet controller that seem > to be developed internally. The IC used is "RTL8201CP". > > This patch adds an MDIO driver and also patches realtek to include > RTL8201CP PHY driver. > > Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com> Added netdev mailing list and David Miller as I don't maintain ethernet PHYs. Thanks Kishon > --- > > Notes: > The hardware does not use a separate IRQ for PHY. > > The link state change interrupt can instead be caught by MAC but the > current drivers/of/of_mdio.c does not allow it to be handled in MAC. > > Applies to next-20131031 > > .../devicetree/bindings/net/moxa,moxart-mdio.txt | 19 ++ > drivers/net/phy/Kconfig | 7 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/mdio-moxart.c | 201 +++++++++++++++++++++ > drivers/net/phy/realtek.c | 15 ++ > 5 files changed, 243 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > create mode 100644 drivers/net/phy/mdio-moxart.c > > diff --git a/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt b/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > new file mode 100644 > index 0000000..de0b90c > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > @@ -0,0 +1,19 @@ > +* MOXA ART MDIO Ethernet Controller interface > + > +Required properties: > +- compatible: should be "moxa,moxart-mdio". > +- reg: address and length of the register set for the device. > + > +Example: > +mdio1: mdio@92000090 { > + compatible = "moxa,moxart-mdio"; > + reg = <0x92000090 0x8>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethphy1: ethernet-phy@1 { > + device_type = "ethernet-phy"; > + compatible = "moxa,moxart-rtl8201cp"; > + reg = <1>; > + }; > +}; > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 342561a..9b5d46c 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -154,6 +154,13 @@ config MDIO_SUN4I > interface units of the Allwinner SoC that have an EMAC (A10, > A12, A10s, etc.) > > +config MDIO_MOXART > + tristate "MOXA ART MDIO interface support" > + depends on ARCH_MOXART > + help > + This driver supports the MDIO interface found in the network > + interface units of the MOXA ART SoC > + > config MDIO_BUS_MUX > tristate > depends on OF_MDIO > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index 23a2ab2..9013dfa 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_MDIO_BUS_MUX) += mdio-mux.o > obj-$(CONFIG_MDIO_BUS_MUX_GPIO) += mdio-mux-gpio.o > obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o > obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o > +obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o > diff --git a/drivers/net/phy/mdio-moxart.c b/drivers/net/phy/mdio-moxart.c > new file mode 100644 > index 0000000..ad5d0f8 > --- /dev/null > +++ b/drivers/net/phy/mdio-moxart.c > @@ -0,0 +1,201 @@ > +/* MOXA ART Ethernet (RTL8201CP) MDIO interface driver > + * > + * Copyright (C) 2013 Jonas Jensen <jonas.jensen@gmail.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/delay.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of_address.h> > +#include <linux/of_mdio.h> > +#include <linux/phy.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > + > +#define REG_PHY_CTRL 0 > +#define REG_PHY_WRITE_DATA 4 > + > +/* REG_PHY_CTRL */ > +#define MIIWR BIT(27) /* init write sequence (auto cleared)*/ > +#define MIIRD BIT(26) > +#define REGAD_MASK 0x3e00000 > +#define PHYAD_MASK 0x1f0000 > +#define MIIRDATA_MASK 0xffff > + > +/* REG_PHY_WRITE_DATA */ > +#define MIIWDATA_MASK 0xffff > + > +struct moxart_mdio_data { > + void __iomem *base; > +}; > + > +static int moxart_mdio_read(struct mii_bus *bus, int mii_id, int regnum) > +{ > + struct moxart_mdio_data *data = bus->priv; > + u32 ctrl = 0; > + unsigned int count = 5; > + > + dev_dbg(&bus->dev, "%s\n", __func__); > + > + ctrl |= MIIRD | ((mii_id << 16) & PHYAD_MASK) | > + ((regnum << 21) & REGAD_MASK); > + > + writel(ctrl, data->base + REG_PHY_CTRL); > + > + do { > + ctrl = readl(data->base + REG_PHY_CTRL); > + > + if (!(ctrl & MIIRD)) > + return ctrl & MIIRDATA_MASK; > + > + mdelay(10); > + count--; > + } while (count > 0); > + > + dev_err(&bus->dev, "%s timed out\n", __func__); > + > + return -ETIMEDOUT; > +} > + > +static int moxart_mdio_write(struct mii_bus *bus, int mii_id, > + int regnum, u16 value) > +{ > + struct moxart_mdio_data *data = bus->priv; > + u32 ctrl = 0; > + unsigned int count = 5; > + > + dev_dbg(&bus->dev, "%s\n", __func__); > + > + ctrl |= MIIWR | ((mii_id << 16) & PHYAD_MASK) | > + ((regnum << 21) & REGAD_MASK); > + > + value &= MIIWDATA_MASK; > + > + writel(value, data->base + REG_PHY_WRITE_DATA); > + writel(ctrl, data->base + REG_PHY_CTRL); > + > + do { > + ctrl = readl(data->base + REG_PHY_CTRL); > + > + if (!(ctrl & MIIWR)) > + return 0; > + > + mdelay(10); > + count--; > + } while (count > 0); > + > + dev_err(&bus->dev, "%s timed out\n", __func__); > + > + return -ETIMEDOUT; > +} > + > +static int moxart_mdio_reset(struct mii_bus *bus) > +{ > + int data, i; > + > + for (i = 0; i < PHY_MAX_ADDR; i++) { > + data = moxart_mdio_read(bus, i, MII_BMCR); > + if (data < 0) > + continue; > + > + data |= BMCR_RESET; > + if (moxart_mdio_write(bus, i, MII_BMCR, data) < 0) > + continue; > + } > + > + return 0; > +} > + > +static int moxart_mdio_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct mii_bus *bus; > + struct moxart_mdio_data *data; > + struct resource *res; > + int ret, i; > + > + bus = mdiobus_alloc_size(sizeof(*data)); > + if (!bus) > + return -ENOMEM; > + > + bus->name = "MOXA ART Ethernet MII"; > + bus->read = &moxart_mdio_read; > + bus->write = &moxart_mdio_write; > + bus->reset = &moxart_mdio_reset; > + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev)); > + bus->parent = &pdev->dev; > + > + bus->irq = devm_kzalloc(&pdev->dev, sizeof(int) * PHY_MAX_ADDR, > + GFP_KERNEL); > + if (!bus->irq) { > + ret = -ENOMEM; > + goto err_out_free_mdiobus; > + } > + > + /* Setting PHY_IGNORE_INTERRUPT here even if it has no effect, > + * of_mdiobus_register() sets these PHY_POLL. > + * Ideally, the interrupt from MAC controller could be used to > + * detect link state changes, not polling, i.e. if there was > + * a way phy_driver could set PHY_HAS_INTERRUPT but have that > + * interrupt handled in ethernet drivercode. > + */ > + for (i = 0; i < PHY_MAX_ADDR; i++) > + bus->irq[i] = PHY_IGNORE_INTERRUPT; > + > + data = bus->priv; > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->base)) { > + ret = PTR_ERR(data->base); > + goto err_out_free_mdiobus; > + } > + > + ret = of_mdiobus_register(bus, np); > + if (ret < 0) > + return ret; > + > + platform_set_drvdata(pdev, bus); > + > + return 0; > + > +err_out_free_mdiobus: > + mdiobus_free(bus); > + return ret; > +} > + > +static int moxart_mdio_remove(struct platform_device *pdev) > +{ > + struct mii_bus *bus = platform_get_drvdata(pdev); > + > + mdiobus_unregister(bus); > + mdiobus_free(bus); > + > + return 0; > +} > + > +static const struct of_device_id moxart_mdio_dt_ids[] = { > + { .compatible = "moxa,moxart-mdio" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, moxart_mdio_dt_ids); > + > +static struct platform_driver moxart_mdio_driver = { > + .probe = moxart_mdio_probe, > + .remove = moxart_mdio_remove, > + .driver = { > + .name = "moxart-mdio", > + .of_match_table = moxart_mdio_dt_ids, > + }, > +}; > + > +module_platform_driver(moxart_mdio_driver); > + > +MODULE_DESCRIPTION("MOXA ART MDIO interface driver"); > +MODULE_AUTHOR("Jonas Jensen <jonas.jensen@gmail.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index 138de83..fa1d69a 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -64,6 +64,18 @@ static int rtl8211e_config_intr(struct phy_device *phydev) > return err; > } > > +/* RTL8201CP */ > +static struct phy_driver rtl8201cp_driver = { > + .phy_id = 0x00008201, > + .name = "RTL8201CP Ethernet", > + .phy_id_mask = 0x0000ffff, > + .features = PHY_BASIC_FEATURES, > + .flags = PHY_HAS_INTERRUPT, > + .config_aneg = &genphy_config_aneg, > + .read_status = &genphy_read_status, > + .driver = { .owner = THIS_MODULE,}, > +}; > + > /* RTL8211B */ > static struct phy_driver rtl8211b_driver = { > .phy_id = 0x001cc912, > @@ -98,6 +110,9 @@ static int __init realtek_init(void) > { > int ret; > > + ret = phy_driver_register(&rtl8201cp_driver); > + if (ret < 0) > + return -ENODEV; > ret = phy_driver_register(&rtl8211b_driver); > if (ret < 0) > return -ENODEV; >
On Fri, 1 Nov 2013 15:54:33 +0100, Jonas Jensen <jonas.jensen@gmail.com> wrote: > The MOXA UC-711X hardware(s) has an ethernet controller that seem > to be developed internally. The IC used is "RTL8201CP". > > This patch adds an MDIO driver and also patches realtek to include > RTL8201CP PHY driver. > > Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com> > --- > > Notes: > The hardware does not use a separate IRQ for PHY. > > The link state change interrupt can instead be caught by MAC but the > current drivers/of/of_mdio.c does not allow it to be handled in MAC. > > Applies to next-20131031 > > .../devicetree/bindings/net/moxa,moxart-mdio.txt | 19 ++ > drivers/net/phy/Kconfig | 7 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/mdio-moxart.c | 201 +++++++++++++++++++++ > drivers/net/phy/realtek.c | 15 ++ > 5 files changed, 243 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > create mode 100644 drivers/net/phy/mdio-moxart.c > > diff --git a/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt b/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > new file mode 100644 > index 0000000..de0b90c > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt > @@ -0,0 +1,19 @@ > +* MOXA ART MDIO Ethernet Controller interface > + > +Required properties: > +- compatible: should be "moxa,moxart-mdio". > +- reg: address and length of the register set for the device. > + > +Example: > +mdio1: mdio@92000090 { > + compatible = "moxa,moxart-mdio"; > + reg = <0x92000090 0x8>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethphy1: ethernet-phy@1 { > + device_type = "ethernet-phy"; > + compatible = "moxa,moxart-rtl8201cp"; > + reg = <1>; > + }; > +}; Given that the binding is trivial and doesn't add any custom properties, and it really looks like the MDIO block is part of the moxa,moxart-mac device, why not merely add the following blub to Documentation/devicetree/bindings/net/moxa,moxart-mac.txt: --- Integrated MDIO bus node: - compatible: "moxa,moxart-mdio" - Inherets from MDIO bus node binding[1] [1] Documentation/devicetree/bindings/net/phy.txt --- And at the same time add the following blurb to the top of Documentation/devicetree/bindings/net/phy.txt: --- MDIO Bus Nodes MDIO bus nodes describe an MDIO bus. It is a container for PHY nodes as described below. Required properties: - #address-cells = <1>; - #size-cells = <0>; ---- And then update the example to read: --- mdio { #address-cells = <1>; #size-cells = <0>; ethernet-phy@0 { compatible = "...", "ethernet-phy-ieee802.3-c22"; reg = <0>; interrupts = <24 0>; } ethernet-phy@1 { compatible = "..."; reg = <1>; interrupts = <35 1>; } } --- That will break out a lot of the boilerplate that we don't really need in every single binding file. g. > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 342561a..9b5d46c 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -154,6 +154,13 @@ config MDIO_SUN4I > interface units of the Allwinner SoC that have an EMAC (A10, > A12, A10s, etc.) > > +config MDIO_MOXART > + tristate "MOXA ART MDIO interface support" > + depends on ARCH_MOXART > + help > + This driver supports the MDIO interface found in the network > + interface units of the MOXA ART SoC > + > config MDIO_BUS_MUX > tristate > depends on OF_MDIO > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index 23a2ab2..9013dfa 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_MDIO_BUS_MUX) += mdio-mux.o > obj-$(CONFIG_MDIO_BUS_MUX_GPIO) += mdio-mux-gpio.o > obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o > obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o > +obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o > diff --git a/drivers/net/phy/mdio-moxart.c b/drivers/net/phy/mdio-moxart.c > new file mode 100644 > index 0000000..ad5d0f8 > --- /dev/null > +++ b/drivers/net/phy/mdio-moxart.c > @@ -0,0 +1,201 @@ > +/* MOXA ART Ethernet (RTL8201CP) MDIO interface driver > + * > + * Copyright (C) 2013 Jonas Jensen <jonas.jensen@gmail.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/delay.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of_address.h> > +#include <linux/of_mdio.h> > +#include <linux/phy.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > + > +#define REG_PHY_CTRL 0 > +#define REG_PHY_WRITE_DATA 4 > + > +/* REG_PHY_CTRL */ > +#define MIIWR BIT(27) /* init write sequence (auto cleared)*/ > +#define MIIRD BIT(26) > +#define REGAD_MASK 0x3e00000 > +#define PHYAD_MASK 0x1f0000 > +#define MIIRDATA_MASK 0xffff > + > +/* REG_PHY_WRITE_DATA */ > +#define MIIWDATA_MASK 0xffff > + > +struct moxart_mdio_data { > + void __iomem *base; > +}; > + > +static int moxart_mdio_read(struct mii_bus *bus, int mii_id, int regnum) > +{ > + struct moxart_mdio_data *data = bus->priv; > + u32 ctrl = 0; > + unsigned int count = 5; > + > + dev_dbg(&bus->dev, "%s\n", __func__); > + > + ctrl |= MIIRD | ((mii_id << 16) & PHYAD_MASK) | > + ((regnum << 21) & REGAD_MASK); > + > + writel(ctrl, data->base + REG_PHY_CTRL); > + > + do { > + ctrl = readl(data->base + REG_PHY_CTRL); > + > + if (!(ctrl & MIIRD)) > + return ctrl & MIIRDATA_MASK; > + > + mdelay(10); > + count--; > + } while (count > 0); > + > + dev_err(&bus->dev, "%s timed out\n", __func__); > + > + return -ETIMEDOUT; > +} > + > +static int moxart_mdio_write(struct mii_bus *bus, int mii_id, > + int regnum, u16 value) > +{ > + struct moxart_mdio_data *data = bus->priv; > + u32 ctrl = 0; > + unsigned int count = 5; > + > + dev_dbg(&bus->dev, "%s\n", __func__); > + > + ctrl |= MIIWR | ((mii_id << 16) & PHYAD_MASK) | > + ((regnum << 21) & REGAD_MASK); > + > + value &= MIIWDATA_MASK; > + > + writel(value, data->base + REG_PHY_WRITE_DATA); > + writel(ctrl, data->base + REG_PHY_CTRL); > + > + do { > + ctrl = readl(data->base + REG_PHY_CTRL); > + > + if (!(ctrl & MIIWR)) > + return 0; > + > + mdelay(10); > + count--; > + } while (count > 0); > + > + dev_err(&bus->dev, "%s timed out\n", __func__); > + > + return -ETIMEDOUT; > +} > + > +static int moxart_mdio_reset(struct mii_bus *bus) > +{ > + int data, i; > + > + for (i = 0; i < PHY_MAX_ADDR; i++) { > + data = moxart_mdio_read(bus, i, MII_BMCR); > + if (data < 0) > + continue; > + > + data |= BMCR_RESET; > + if (moxart_mdio_write(bus, i, MII_BMCR, data) < 0) > + continue; > + } > + > + return 0; > +} > + > +static int moxart_mdio_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct mii_bus *bus; > + struct moxart_mdio_data *data; > + struct resource *res; > + int ret, i; > + > + bus = mdiobus_alloc_size(sizeof(*data)); > + if (!bus) > + return -ENOMEM; > + > + bus->name = "MOXA ART Ethernet MII"; > + bus->read = &moxart_mdio_read; > + bus->write = &moxart_mdio_write; > + bus->reset = &moxart_mdio_reset; > + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev)); > + bus->parent = &pdev->dev; > + > + bus->irq = devm_kzalloc(&pdev->dev, sizeof(int) * PHY_MAX_ADDR, > + GFP_KERNEL); > + if (!bus->irq) { > + ret = -ENOMEM; > + goto err_out_free_mdiobus; > + } > + > + /* Setting PHY_IGNORE_INTERRUPT here even if it has no effect, > + * of_mdiobus_register() sets these PHY_POLL. > + * Ideally, the interrupt from MAC controller could be used to > + * detect link state changes, not polling, i.e. if there was > + * a way phy_driver could set PHY_HAS_INTERRUPT but have that > + * interrupt handled in ethernet drivercode. > + */ > + for (i = 0; i < PHY_MAX_ADDR; i++) > + bus->irq[i] = PHY_IGNORE_INTERRUPT; > + > + data = bus->priv; > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->base)) { > + ret = PTR_ERR(data->base); > + goto err_out_free_mdiobus; > + } > + > + ret = of_mdiobus_register(bus, np); > + if (ret < 0) > + return ret; > + > + platform_set_drvdata(pdev, bus); > + > + return 0; > + > +err_out_free_mdiobus: > + mdiobus_free(bus); > + return ret; > +} > + > +static int moxart_mdio_remove(struct platform_device *pdev) > +{ > + struct mii_bus *bus = platform_get_drvdata(pdev); > + > + mdiobus_unregister(bus); > + mdiobus_free(bus); > + > + return 0; > +} > + > +static const struct of_device_id moxart_mdio_dt_ids[] = { > + { .compatible = "moxa,moxart-mdio" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, moxart_mdio_dt_ids); > + > +static struct platform_driver moxart_mdio_driver = { > + .probe = moxart_mdio_probe, > + .remove = moxart_mdio_remove, > + .driver = { > + .name = "moxart-mdio", > + .of_match_table = moxart_mdio_dt_ids, > + }, > +}; > + > +module_platform_driver(moxart_mdio_driver); > + > +MODULE_DESCRIPTION("MOXA ART MDIO interface driver"); > +MODULE_AUTHOR("Jonas Jensen <jonas.jensen@gmail.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index 138de83..fa1d69a 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -64,6 +64,18 @@ static int rtl8211e_config_intr(struct phy_device *phydev) > return err; > } > > +/* RTL8201CP */ > +static struct phy_driver rtl8201cp_driver = { > + .phy_id = 0x00008201, > + .name = "RTL8201CP Ethernet", > + .phy_id_mask = 0x0000ffff, > + .features = PHY_BASIC_FEATURES, > + .flags = PHY_HAS_INTERRUPT, > + .config_aneg = &genphy_config_aneg, > + .read_status = &genphy_read_status, > + .driver = { .owner = THIS_MODULE,}, > +}; > + > /* RTL8211B */ > static struct phy_driver rtl8211b_driver = { > .phy_id = 0x001cc912, > @@ -98,6 +110,9 @@ static int __init realtek_init(void) > { > int ret; > > + ret = phy_driver_register(&rtl8201cp_driver); > + if (ret < 0) > + return -ENODEV; > ret = phy_driver_register(&rtl8211b_driver); > if (ret < 0) > return -ENODEV; > -- > 1.8.2.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Thanks for the replies. On 1 November 2013 18:01, Florian Fainelli <florian@openwrt.org> wrote: >> + dev_err(&bus->dev, "%s timed out\n", __func__); > > I would keep these as a debugging aid and not spawn error messages on > the console by default. Done. >> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev)); > > If you only support device tree probing, this might be fine, but you > might want to be safe in case someone does a !OF instantiation and > also use pdev->id as an additional unique identifier, so something > like: > > %s-%d-mii, pdev->name, pdev->id > > will work. Done. >> + /* Setting PHY_IGNORE_INTERRUPT here even if it has no effect, >> + * of_mdiobus_register() sets these PHY_POLL. >> + * Ideally, the interrupt from MAC controller could be used to >> + * detect link state changes, not polling, i.e. if there was >> + * a way phy_driver could set PHY_HAS_INTERRUPT but have that >> + * interrupt handled in ethernet drivercode. >> + */ >> + for (i = 0; i < PHY_MAX_ADDR; i++) >> + bus->irq[i] = PHY_IGNORE_INTERRUPT; > > This type of configuration where the PHY interrupt is actually > serviced by a link interrupt bit in the Ethernet MAC driver now works > since 5ea94e768 ("phy: add phy_mac_interrupt() to use with > PHY_IGNORE_INTERRUPT") so setting PHY_IGNORE_INTERRUPT is the right > way to signal this and this will no longer make the PHY library poll > for the link state. Yes, I tried using phy_mac_interrupt() but had some difficulties (I'll explain below). It seemed to be what I want and is wrapped with EXPORT_SYMBOL(). However, as of next-20131104 I don't see how this works for DT probed devices (those that set PHY_IGNORE_INTERRUPT). As I tried to explain in my comment, of_mdiobus_register() assigns PHY_POLL to the IRQ array: drivers/of/of_mdio.c .. int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np) { .. /* Clear all the IRQ properties */ if (mdio->irq) for (i=0; i<PHY_MAX_ADDR; i++) mdio->irq[i] = PHY_POLL; .. for_each_available_child_of_node(np, child) { .. if (mdio->irq) { mdio->irq[addr] = irq_of_parse_and_map(child, 0); if (!mdio->irq[addr]) mdio->irq[addr] = PHY_POLL; } .. PHY_IGNORE_INTERRUPT || PHY_POLL is not a valid interrupt. The library ends up toggling (polling) between PHY_RUNNING and PHY_CHANGELINK: drivers/net/phy/phy.c: .. void phy_state_machine(struct work_struct *work) { .. case PHY_RUNNING: pr_info("%s: %s: PHY_RUNNING\n", __func__, dev_name(&phydev->dev)); /* Only register a CHANGE if we are * polling or ignoring interrupts */ if (!phy_interrupt_is_valid(phydev)) phydev->state = PHY_CHANGELINK; break; .. include/linux/phy.h: .. static inline bool phy_interrupt_is_valid(struct phy_device *phydev) { return phydev->irq != PHY_POLL && phydev->irq != PHY_IGNORE_INTERRUPT; } .. This is why I ended up setting PHY_IGNORE_INTERRUPT and the comment about its effectiveness. Polling works but the extra reads on the bus seem unnecessary. Ideas how they can be eliminated are appreciated. Another problem, when phy_mac_interrupt() is called from NAPI it looks like it's trying to take a lock it already has. I tried moving it out of poll, placing it directly in IRQ handler, with the same result: [ 18.230000] moxart-ethernet 90900000.mac eth0: moxart_poll: PHYSTS_CHG [ 18.240000] [ 18.240000] ================================= [ 18.240000] [ INFO: inconsistent lock state ] [ 18.240000] 3.12.0-rc7-next-20131104+ #1067 Not tainted [ 18.240000] --------------------------------- [ 18.240000] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage. [ 18.240000] kworker/0:1/123 [HC0[0]:SC0[0]:HE1:SE1] takes: [ 18.240000] ((&dev->phy_queue)){+.?...}, at: [<c0028c08>] process_one_work+0x13c/0x430 [ 18.240000] {IN-SOFTIRQ-W} state was registered at: [ 18.240000] [<c0055648>] mark_lock+0x144/0x670 [ 18.240000] [<c005777c>] __lock_acquire+0x5e4/0x1c24 [ 18.240000] [<c00592cc>] lock_acquire+0x6c/0x80 [ 18.240000] [<c002a024>] flush_work+0x44/0x278 [ 18.240000] [<c002a2e0>] __cancel_work_timer+0x88/0x124 [ 18.240000] [<c002a390>] cancel_work_sync+0x14/0x18 [ 18.240000] [<c01b0190>] phy_mac_interrupt+0x20/0x40 [ 18.240000] [<c01b3450>] moxart_poll+0x2b4/0x4b4 [ 18.240000] [<c01fe3c8>] net_rx_action+0x130/0x22c [ 18.240000] [<c00174cc>] __do_softirq+0xe8/0x238 [ 18.240000] [<c0017a2c>] irq_exit+0xac/0xfc [ 18.240000] [<c0009b40>] handle_IRQ+0x3c/0x8c [ 18.240000] [<c0008534>] handle_irq+0x98/0xa8 [ 18.240000] [<c000c478>] __irq_svc+0x38/0x68 [ 18.240000] [<c00487ec>] rcu_idle_exit+0x78/0xdc [ 18.240000] [<c0040618>] cpu_startup_entry+0x88/0x130 [ 18.240000] [<c026d3c8>] rest_init+0xb8/0xe0 [ 18.240000] [<c033ea1c>] start_kernel+0x298/0x2dc [ 18.240000] irq event stamp: 2659 [ 18.240000] hardirqs last enabled at (2659): [<c0276298>] _raw_spin_unlock_irq+0x2c/0x60 [ 18.240000] hardirqs last disabled at (2658): [<c02760f0>] _raw_spin_lock_irq+0x28/0x78 [ 18.240000] softirqs last enabled at (2654): [<c0017568>] __do_softirq+0x184/0x238 [ 18.240000] softirqs last disabled at (2637): [<c0017a2c>] irq_exit+0xac/0xfc [ 18.240000] [ 18.240000] other info that might help us debug this: [ 18.240000] Possible unsafe locking scenario: [ 18.240000] [ 18.240000] CPU0 [ 18.240000] ---- [ 18.240000] lock([ 18.240000] moxart-ethernet 90900000.mac eth0: TX ring end reached [ 18.240000] (&dev->phy_queue)); [ 18.240000] <Interrupt> [ 18.240000] lock((&dev->phy_queue)); [ 18.240000] [ 18.240000] *** DEADLOCK *** [ 18.240000] [ 18.240000] 1 lock held by kworker/0:1/123: [ 18.240000] #0: (events){.+.+..}, at: [<c0028c08>] process_one_work+0x13c/0x430 [ 18.240000] [ 18.240000] stack backtrace: [ 18.240000] CPU: 0 PID: 123 Comm: kworker/0:1 Not tainted 3.12.0-rc7-next-20131104+ #1067 [ 18.240000] Workqueue: events phy_change [ 18.240000] [<c000d214>] (unwind_backtrace+0x0/0xf4) from [<c000b964>] (show_stack+0x18/0x1c) [ 18.240000] [<c000b964>] (show_stack+0x18/0x1c) from [<c0271614>] (dump_stack+0x20/0x28) [ 18.240000] [<c0271614>] (dump_stack+0x20/0x28) from [<c026fee0>] (print_usage_bug.part.26+0x220/0x288) [ 18.240000] [<c026fee0>] (print_usage_bug.part.26+0x220/0x288) from [<c0055b3c>] (mark_lock+0x638/0x670) [ 18.240000] [<c0055b3c>] (mark_lock+0x638/0x670) from [<c00577c4>] (__lock_acquire+0x62c/0x1c24) [ 18.240000] [<c00577c4>] (__lock_acquire+0x62c/0x1c24) from [<c00592cc>] (lock_acquire+0x6c/0x80) [ 18.240000] [<c00592cc>] (lock_acquire+0x6c/0x80) from [<c0028c70>] (process_one_work+0x1a4/0x430) [ 18.240000] [<c0028c70>] (process_one_work+0x1a4/0x430) from [<c0029304>] (worker_thread+0x13c/0x3dc) [ 18.240000] [<c0029304>] (worker_thread+0x13c/0x3dc) from [<c002f500>] (kthread+0xb8/0xd4) [ 18.240000] [<c002f500>] (kthread+0xb8/0xd4) from [<c0009360>] (ret_from_fork+0x14/0x34) [ 18.610000] libphy: phy_change >> + ret = of_mdiobus_register(bus, np); >> + if (ret < 0) >> + return ret; > > You still need to call mdiobus_free() here in case registration fails. Done. >> +/* RTL8201CP */ >> +static struct phy_driver rtl8201cp_driver = { >> + .phy_id = 0x00008201, >> + .name = "RTL8201CP Ethernet", >> + .phy_id_mask = 0x0000ffff, >> + .features = PHY_BASIC_FEATURES, > > If this PHY is internal to your SoC (same die/package) you should also > add PHY_IS_INTERNAL to get a consistent ethtool reporting (among > others). I was wondering about that, and now I'm sure it should not be set, the physical chip is separate, as can be seen here: https://lh4.googleusercontent.com/-A-2FXDrObU8/UMcMc_K2vEI/AAAAAAAABwg/ldaLZ7ps1P4/w1331-h998-no/UC-7112-LX-picture4.jpg Best regards, Jonas
2013/11/4 Jonas Jensen <jonas.jensen@gmail.com>: > Thanks for the replies. > > On 1 November 2013 18:01, Florian Fainelli <florian@openwrt.org> wrote: >>> + dev_err(&bus->dev, "%s timed out\n", __func__); >> >> I would keep these as a debugging aid and not spawn error messages on >> the console by default. > > Done. > > >>> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev)); >> >> If you only support device tree probing, this might be fine, but you >> might want to be safe in case someone does a !OF instantiation and >> also use pdev->id as an additional unique identifier, so something >> like: >> >> %s-%d-mii, pdev->name, pdev->id >> >> will work. > > Done. > > >>> + /* Setting PHY_IGNORE_INTERRUPT here even if it has no effect, >>> + * of_mdiobus_register() sets these PHY_POLL. >>> + * Ideally, the interrupt from MAC controller could be used to >>> + * detect link state changes, not polling, i.e. if there was >>> + * a way phy_driver could set PHY_HAS_INTERRUPT but have that >>> + * interrupt handled in ethernet drivercode. >>> + */ >>> + for (i = 0; i < PHY_MAX_ADDR; i++) >>> + bus->irq[i] = PHY_IGNORE_INTERRUPT; >> >> This type of configuration where the PHY interrupt is actually >> serviced by a link interrupt bit in the Ethernet MAC driver now works >> since 5ea94e768 ("phy: add phy_mac_interrupt() to use with >> PHY_IGNORE_INTERRUPT") so setting PHY_IGNORE_INTERRUPT is the right >> way to signal this and this will no longer make the PHY library poll >> for the link state. > > Yes, I tried using phy_mac_interrupt() but had some difficulties (I'll > explain below). It seemed to be what I want and is wrapped with > EXPORT_SYMBOL(). > > However, as of next-20131104 I don't see how this works for DT probed > devices (those that set PHY_IGNORE_INTERRUPT). > > As I tried to explain in my comment, of_mdiobus_register() assigns > PHY_POLL to the IRQ array: > > drivers/of/of_mdio.c > .. > int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np) > { > .. > /* Clear all the IRQ properties */ > if (mdio->irq) > for (i=0; i<PHY_MAX_ADDR; i++) > mdio->irq[i] = PHY_POLL; You are right, I missed that part. We can't really assign a dedicated interrupt line for your PHY, so we need another way of dealing with these PHY interrupts present at the Ethernet MAC level. [snip] > This is why I ended up setting PHY_IGNORE_INTERRUPT and the comment > about its effectiveness. Polling works but the extra reads on the bus > seem unnecessary. > Ideas how they can be eliminated are appreciated. As of today, the only way to work around it is not to use of_mdiobus_register() and use mdiobus_register() directly which will allow you to properly describe such a configuration. > > > Another problem, when phy_mac_interrupt() is called from NAPI it looks > like it's trying to take a lock it already has. I tried moving it out > of poll, placing it directly in IRQ handler, with the same result: > > [ 18.230000] moxart-ethernet 90900000.mac eth0: moxart_poll: PHYSTS_CHG > [ 18.240000] > [ 18.240000] ================================= > [ 18.240000] [ INFO: inconsistent lock state ] > [ 18.240000] 3.12.0-rc7-next-20131104+ #1067 Not tainted > [ 18.240000] --------------------------------- > [ 18.240000] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage. > [ 18.240000] kworker/0:1/123 [HC0[0]:SC0[0]:HE1:SE1] takes: > [ 18.240000] ((&dev->phy_queue)){+.?...}, at: [<c0028c08>] > process_one_work+0x13c/0x430 > [ 18.240000] {IN-SOFTIRQ-W} state was registered at: > [ 18.240000] [<c0055648>] mark_lock+0x144/0x670 > [ 18.240000] [<c005777c>] __lock_acquire+0x5e4/0x1c24 > [ 18.240000] [<c00592cc>] lock_acquire+0x6c/0x80 > [ 18.240000] [<c002a024>] flush_work+0x44/0x278 > [ 18.240000] [<c002a2e0>] __cancel_work_timer+0x88/0x124 > [ 18.240000] [<c002a390>] cancel_work_sync+0x14/0x18 > [ 18.240000] [<c01b0190>] phy_mac_interrupt+0x20/0x40 > [ 18.240000] [<c01b3450>] moxart_poll+0x2b4/0x4b4 > [ 18.240000] [<c01fe3c8>] net_rx_action+0x130/0x22c > [ 18.240000] [<c00174cc>] __do_softirq+0xe8/0x238 > [ 18.240000] [<c0017a2c>] irq_exit+0xac/0xfc > [ 18.240000] [<c0009b40>] handle_IRQ+0x3c/0x8c > [ 18.240000] [<c0008534>] handle_irq+0x98/0xa8 > [ 18.240000] [<c000c478>] __irq_svc+0x38/0x68 > [ 18.240000] [<c00487ec>] rcu_idle_exit+0x78/0xdc > [ 18.240000] [<c0040618>] cpu_startup_entry+0x88/0x130 > [ 18.240000] [<c026d3c8>] rest_init+0xb8/0xe0 > [ 18.240000] [<c033ea1c>] start_kernel+0x298/0x2dc > [ 18.240000] irq event stamp: 2659 > [ 18.240000] hardirqs last enabled at (2659): [<c0276298>] > _raw_spin_unlock_irq+0x2c/0x60 > [ 18.240000] hardirqs last disabled at (2658): [<c02760f0>] > _raw_spin_lock_irq+0x28/0x78 > [ 18.240000] softirqs last enabled at (2654): [<c0017568>] > __do_softirq+0x184/0x238 > [ 18.240000] softirqs last disabled at (2637): [<c0017a2c>] irq_exit+0xac/0xfc > [ 18.240000] > [ 18.240000] other info that might help us debug this: > [ 18.240000] Possible unsafe locking scenario: > [ 18.240000] > [ 18.240000] CPU0 > [ 18.240000] ---- > [ 18.240000] lock([ 18.240000] moxart-ethernet 90900000.mac > eth0: TX ring end reached > [ 18.240000] (&dev->phy_queue)); > [ 18.240000] <Interrupt> > [ 18.240000] lock((&dev->phy_queue)); > [ 18.240000] > [ 18.240000] *** DEADLOCK *** > [ 18.240000] > [ 18.240000] 1 lock held by kworker/0:1/123: > [ 18.240000] #0: (events){.+.+..}, at: [<c0028c08>] > process_one_work+0x13c/0x430 > [ 18.240000] > [ 18.240000] stack backtrace: > [ 18.240000] CPU: 0 PID: 123 Comm: kworker/0:1 Not tainted > 3.12.0-rc7-next-20131104+ #1067 > [ 18.240000] Workqueue: events phy_change > [ 18.240000] [<c000d214>] (unwind_backtrace+0x0/0xf4) from > [<c000b964>] (show_stack+0x18/0x1c) > [ 18.240000] [<c000b964>] (show_stack+0x18/0x1c) from [<c0271614>] > (dump_stack+0x20/0x28) > [ 18.240000] [<c0271614>] (dump_stack+0x20/0x28) from [<c026fee0>] > (print_usage_bug.part.26+0x220/0x288) > [ 18.240000] [<c026fee0>] (print_usage_bug.part.26+0x220/0x288) from > [<c0055b3c>] (mark_lock+0x638/0x670) > [ 18.240000] [<c0055b3c>] (mark_lock+0x638/0x670) from [<c00577c4>] > (__lock_acquire+0x62c/0x1c24) > [ 18.240000] [<c00577c4>] (__lock_acquire+0x62c/0x1c24) from > [<c00592cc>] (lock_acquire+0x6c/0x80) > [ 18.240000] [<c00592cc>] (lock_acquire+0x6c/0x80) from [<c0028c70>] > (process_one_work+0x1a4/0x430) > [ 18.240000] [<c0028c70>] (process_one_work+0x1a4/0x430) from > [<c0029304>] (worker_thread+0x13c/0x3dc) > [ 18.240000] [<c0029304>] (worker_thread+0x13c/0x3dc) from > [<c002f500>] (kthread+0xb8/0xd4) > [ 18.240000] [<c002f500>] (kthread+0xb8/0xd4) from [<c0009360>] > (ret_from_fork+0x14/0x34) > [ 18.610000] libphy: phy_change Ok, I will take a look into this, I realize that I was calling this from a workqueue which won't show the locking problem, thanks for the report. > > >>> + ret = of_mdiobus_register(bus, np); >>> + if (ret < 0) >>> + return ret; >> >> You still need to call mdiobus_free() here in case registration fails. > > Done. > > >>> +/* RTL8201CP */ >>> +static struct phy_driver rtl8201cp_driver = { >>> + .phy_id = 0x00008201, >>> + .name = "RTL8201CP Ethernet", >>> + .phy_id_mask = 0x0000ffff, >>> + .features = PHY_BASIC_FEATURES, >> >> If this PHY is internal to your SoC (same die/package) you should also >> add PHY_IS_INTERNAL to get a consistent ethtool reporting (among >> others). > > I was wondering about that, and now I'm sure it should not be set, the > physical chip is separate, as can be seen here: > > https://lh4.googleusercontent.com/-A-2FXDrObU8/UMcMc_K2vEI/AAAAAAAABwg/ldaLZ7ps1P4/w1331-h998-no/UC-7112-LX-picture4.jpg Right, then make that a separate patch instead. -- Florian
On Mon, 4 Nov 2013 09:51:24 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote: > 2013/11/4 Jonas Jensen <jonas.jensen@gmail.com>: > > This is why I ended up setting PHY_IGNORE_INTERRUPT and the comment > > about its effectiveness. Polling works but the extra reads on the bus > > seem unnecessary. > > Ideas how they can be eliminated are appreciated. > > As of today, the only way to work around it is not to use > of_mdiobus_register() and use mdiobus_register() directly which will > allow you to properly describe such a configuration. of_mdiobus_register() can be changed. One option: If the caller knows what it wants to happen in the case of no interrupt, then it can be added as an argument to of_mdiobus_register(). No need to work around this in an ugly way. Is setting the irq to PHY_POLL something that should be done for the entire bus, or only for a handful of PHYs? g.
diff --git a/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt b/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt new file mode 100644 index 0000000..de0b90c --- /dev/null +++ b/Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt @@ -0,0 +1,19 @@ +* MOXA ART MDIO Ethernet Controller interface + +Required properties: +- compatible: should be "moxa,moxart-mdio". +- reg: address and length of the register set for the device. + +Example: +mdio1: mdio@92000090 { + compatible = "moxa,moxart-mdio"; + reg = <0x92000090 0x8>; + #address-cells = <1>; + #size-cells = <0>; + + ethphy1: ethernet-phy@1 { + device_type = "ethernet-phy"; + compatible = "moxa,moxart-rtl8201cp"; + reg = <1>; + }; +}; diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 342561a..9b5d46c 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -154,6 +154,13 @@ config MDIO_SUN4I interface units of the Allwinner SoC that have an EMAC (A10, A12, A10s, etc.) +config MDIO_MOXART + tristate "MOXA ART MDIO interface support" + depends on ARCH_MOXART + help + This driver supports the MDIO interface found in the network + interface units of the MOXA ART SoC + config MDIO_BUS_MUX tristate depends on OF_MDIO diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 23a2ab2..9013dfa 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -31,3 +31,4 @@ obj-$(CONFIG_MDIO_BUS_MUX) += mdio-mux.o obj-$(CONFIG_MDIO_BUS_MUX_GPIO) += mdio-mux-gpio.o obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o +obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o diff --git a/drivers/net/phy/mdio-moxart.c b/drivers/net/phy/mdio-moxart.c new file mode 100644 index 0000000..ad5d0f8 --- /dev/null +++ b/drivers/net/phy/mdio-moxart.c @@ -0,0 +1,201 @@ +/* MOXA ART Ethernet (RTL8201CP) MDIO interface driver + * + * Copyright (C) 2013 Jonas Jensen <jonas.jensen@gmail.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/delay.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of_address.h> +#include <linux/of_mdio.h> +#include <linux/phy.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> + +#define REG_PHY_CTRL 0 +#define REG_PHY_WRITE_DATA 4 + +/* REG_PHY_CTRL */ +#define MIIWR BIT(27) /* init write sequence (auto cleared)*/ +#define MIIRD BIT(26) +#define REGAD_MASK 0x3e00000 +#define PHYAD_MASK 0x1f0000 +#define MIIRDATA_MASK 0xffff + +/* REG_PHY_WRITE_DATA */ +#define MIIWDATA_MASK 0xffff + +struct moxart_mdio_data { + void __iomem *base; +}; + +static int moxart_mdio_read(struct mii_bus *bus, int mii_id, int regnum) +{ + struct moxart_mdio_data *data = bus->priv; + u32 ctrl = 0; + unsigned int count = 5; + + dev_dbg(&bus->dev, "%s\n", __func__); + + ctrl |= MIIRD | ((mii_id << 16) & PHYAD_MASK) | + ((regnum << 21) & REGAD_MASK); + + writel(ctrl, data->base + REG_PHY_CTRL); + + do { + ctrl = readl(data->base + REG_PHY_CTRL); + + if (!(ctrl & MIIRD)) + return ctrl & MIIRDATA_MASK; + + mdelay(10); + count--; + } while (count > 0); + + dev_err(&bus->dev, "%s timed out\n", __func__); + + return -ETIMEDOUT; +} + +static int moxart_mdio_write(struct mii_bus *bus, int mii_id, + int regnum, u16 value) +{ + struct moxart_mdio_data *data = bus->priv; + u32 ctrl = 0; + unsigned int count = 5; + + dev_dbg(&bus->dev, "%s\n", __func__); + + ctrl |= MIIWR | ((mii_id << 16) & PHYAD_MASK) | + ((regnum << 21) & REGAD_MASK); + + value &= MIIWDATA_MASK; + + writel(value, data->base + REG_PHY_WRITE_DATA); + writel(ctrl, data->base + REG_PHY_CTRL); + + do { + ctrl = readl(data->base + REG_PHY_CTRL); + + if (!(ctrl & MIIWR)) + return 0; + + mdelay(10); + count--; + } while (count > 0); + + dev_err(&bus->dev, "%s timed out\n", __func__); + + return -ETIMEDOUT; +} + +static int moxart_mdio_reset(struct mii_bus *bus) +{ + int data, i; + + for (i = 0; i < PHY_MAX_ADDR; i++) { + data = moxart_mdio_read(bus, i, MII_BMCR); + if (data < 0) + continue; + + data |= BMCR_RESET; + if (moxart_mdio_write(bus, i, MII_BMCR, data) < 0) + continue; + } + + return 0; +} + +static int moxart_mdio_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct mii_bus *bus; + struct moxart_mdio_data *data; + struct resource *res; + int ret, i; + + bus = mdiobus_alloc_size(sizeof(*data)); + if (!bus) + return -ENOMEM; + + bus->name = "MOXA ART Ethernet MII"; + bus->read = &moxart_mdio_read; + bus->write = &moxart_mdio_write; + bus->reset = &moxart_mdio_reset; + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev)); + bus->parent = &pdev->dev; + + bus->irq = devm_kzalloc(&pdev->dev, sizeof(int) * PHY_MAX_ADDR, + GFP_KERNEL); + if (!bus->irq) { + ret = -ENOMEM; + goto err_out_free_mdiobus; + } + + /* Setting PHY_IGNORE_INTERRUPT here even if it has no effect, + * of_mdiobus_register() sets these PHY_POLL. + * Ideally, the interrupt from MAC controller could be used to + * detect link state changes, not polling, i.e. if there was + * a way phy_driver could set PHY_HAS_INTERRUPT but have that + * interrupt handled in ethernet drivercode. + */ + for (i = 0; i < PHY_MAX_ADDR; i++) + bus->irq[i] = PHY_IGNORE_INTERRUPT; + + data = bus->priv; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + data->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(data->base)) { + ret = PTR_ERR(data->base); + goto err_out_free_mdiobus; + } + + ret = of_mdiobus_register(bus, np); + if (ret < 0) + return ret; + + platform_set_drvdata(pdev, bus); + + return 0; + +err_out_free_mdiobus: + mdiobus_free(bus); + return ret; +} + +static int moxart_mdio_remove(struct platform_device *pdev) +{ + struct mii_bus *bus = platform_get_drvdata(pdev); + + mdiobus_unregister(bus); + mdiobus_free(bus); + + return 0; +} + +static const struct of_device_id moxart_mdio_dt_ids[] = { + { .compatible = "moxa,moxart-mdio" }, + { } +}; +MODULE_DEVICE_TABLE(of, moxart_mdio_dt_ids); + +static struct platform_driver moxart_mdio_driver = { + .probe = moxart_mdio_probe, + .remove = moxart_mdio_remove, + .driver = { + .name = "moxart-mdio", + .of_match_table = moxart_mdio_dt_ids, + }, +}; + +module_platform_driver(moxart_mdio_driver); + +MODULE_DESCRIPTION("MOXA ART MDIO interface driver"); +MODULE_AUTHOR("Jonas Jensen <jonas.jensen@gmail.com>"); +MODULE_LICENSE("GPL"); diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index 138de83..fa1d69a 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -64,6 +64,18 @@ static int rtl8211e_config_intr(struct phy_device *phydev) return err; } +/* RTL8201CP */ +static struct phy_driver rtl8201cp_driver = { + .phy_id = 0x00008201, + .name = "RTL8201CP Ethernet", + .phy_id_mask = 0x0000ffff, + .features = PHY_BASIC_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .config_aneg = &genphy_config_aneg, + .read_status = &genphy_read_status, + .driver = { .owner = THIS_MODULE,}, +}; + /* RTL8211B */ static struct phy_driver rtl8211b_driver = { .phy_id = 0x001cc912, @@ -98,6 +110,9 @@ static int __init realtek_init(void) { int ret; + ret = phy_driver_register(&rtl8201cp_driver); + if (ret < 0) + return -ENODEV; ret = phy_driver_register(&rtl8211b_driver); if (ret < 0) return -ENODEV;
The MOXA UC-711X hardware(s) has an ethernet controller that seem to be developed internally. The IC used is "RTL8201CP". This patch adds an MDIO driver and also patches realtek to include RTL8201CP PHY driver. Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com> --- Notes: The hardware does not use a separate IRQ for PHY. The link state change interrupt can instead be caught by MAC but the current drivers/of/of_mdio.c does not allow it to be handled in MAC. Applies to next-20131031 .../devicetree/bindings/net/moxa,moxart-mdio.txt | 19 ++ drivers/net/phy/Kconfig | 7 + drivers/net/phy/Makefile | 1 + drivers/net/phy/mdio-moxart.c | 201 +++++++++++++++++++++ drivers/net/phy/realtek.c | 15 ++ 5 files changed, 243 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/moxa,moxart-mdio.txt create mode 100644 drivers/net/phy/mdio-moxart.c