* [PATCH] phy: Add MOXA RTL8201CP PHY support
2013-11-01 14:54 [PATCH] phy: Add MOXA RTL8201CP PHY support Jonas Jensen
@ 2013-11-01 17:01 ` Florian Fainelli
2013-11-04 13:50 ` Jonas Jensen
2013-11-01 17:17 ` Kishon Vijay Abraham I
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2013-11-01 17:01 UTC (permalink / raw)
To: linux-arm-kernel
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 at 92000090 {
> + compatible = "moxa,moxart-mdio";
> + reg = <0x92000090 0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy1: ethernet-phy at 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).
--
Florian
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH] phy: Add MOXA RTL8201CP PHY support
2013-11-01 17:01 ` Florian Fainelli
@ 2013-11-04 13:50 ` Jonas Jensen
2013-11-04 17:51 ` Florian Fainelli
0 siblings, 1 reply; 9+ messages in thread
From: Jonas Jensen @ 2013-11-04 13:50 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH] phy: Add MOXA RTL8201CP PHY support
2013-11-04 13:50 ` Jonas Jensen
@ 2013-11-04 17:51 ` Florian Fainelli
2013-11-11 15:30 ` Grant Likely
0 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2013-11-04 17:51 UTC (permalink / raw)
To: linux-arm-kernel
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
--
Florian
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH] phy: Add MOXA RTL8201CP PHY support
2013-11-04 17:51 ` Florian Fainelli
@ 2013-11-11 15:30 ` Grant Likely
0 siblings, 0 replies; 9+ messages in thread
From: Grant Likely @ 2013-11-11 15:30 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] phy: Add MOXA RTL8201CP PHY support
2013-11-01 14:54 [PATCH] phy: Add MOXA RTL8201CP PHY support Jonas Jensen
2013-11-01 17:01 ` Florian Fainelli
@ 2013-11-01 17:17 ` Kishon Vijay Abraham I
2013-11-02 18:05 ` Grant Likely
2013-11-05 15:55 ` [PATCH v2] phy: Add MOXA MDIO driver Jonas Jensen
3 siblings, 0 replies; 9+ messages in thread
From: Kishon Vijay Abraham I @ 2013-11-01 17:17 UTC (permalink / raw)
To: linux-arm-kernel
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 at 92000090 {
> + compatible = "moxa,moxart-mdio";
> + reg = <0x92000090 0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy1: ethernet-phy at 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;
>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH] phy: Add MOXA RTL8201CP PHY support
2013-11-01 14:54 [PATCH] phy: Add MOXA RTL8201CP PHY support Jonas Jensen
2013-11-01 17:01 ` Florian Fainelli
2013-11-01 17:17 ` Kishon Vijay Abraham I
@ 2013-11-02 18:05 ` Grant Likely
2013-11-05 15:55 ` [PATCH v2] phy: Add MOXA MDIO driver Jonas Jensen
3 siblings, 0 replies; 9+ messages in thread
From: Grant Likely @ 2013-11-02 18:05 UTC (permalink / raw)
To: linux-arm-kernel
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 at 92000090 {
> + compatible = "moxa,moxart-mdio";
> + reg = <0x92000090 0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy1: ethernet-phy at 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 at 0 {
compatible = "...", "ethernet-phy-ieee802.3-c22";
reg = <0>;
interrupts = <24 0>;
}
ethernet-phy at 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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2] phy: Add MOXA MDIO driver
2013-11-01 14:54 [PATCH] phy: Add MOXA RTL8201CP PHY support Jonas Jensen
` (2 preceding siblings ...)
2013-11-02 18:05 ` Grant Likely
@ 2013-11-05 15:55 ` Jonas Jensen
2013-11-07 20:37 ` David Miller
3 siblings, 1 reply; 9+ messages in thread
From: Jonas Jensen @ 2013-11-05 15:55 UTC (permalink / raw)
To: linux-arm-kernel
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 which handles the MII bus.
Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---
Notes:
Thanks for the feedback.
Besides what is below I plan to submit changes suggested by
Grant as a separate patch (to Documentation/devicetree/bindings/net/phy.txt)
Changes since v1:
1. split changes to drivers/net/phy/realtek.c into separate patch
driver code:
2. use dev_dbg() instead of dev_err()
3. use id "%s-%d-mii" instead of "%s-mii"
4. call mdiobus_free() on of_mdiobus_register() failure
DT binding document:
5. remove it, submitting changes (to Documentation/devicetree/bindings/net/moxa,moxart-mac.txt) along with changes to the driver
Applies to next-20131105
drivers/net/phy/Kconfig | 7 ++
drivers/net/phy/Makefile | 1 +
drivers/net/phy/mdio-moxart.c | 201 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 drivers/net/phy/mdio-moxart.c
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..a5741cb
--- /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_dbg(&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_dbg(&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-%d-mii", pdev->name, pdev->id);
+ 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)
+ goto err_out_free_mdiobus;
+
+ 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");
--
1.8.2.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
