mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Add Davicom phy + reset-gpios
@ 2018-08-20 14:52 Sam Ravnborg
  2018-08-20 14:56 ` [PATCH v3 1/3] phylib: add Davicom PHY support Sam Ravnborg
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sam Ravnborg @ 2018-08-20 14:52 UTC (permalink / raw)
  To: Barebox List; +Cc: Sam Ravnborg

Changes in v3:
- Consider the v3 a new implmentation - almost nothing was
  left from v2. Thus also invalidate any former review - sorry.
- Dropped the need for the mdio {} node, as this node is
  only used when there is dedicated HW support for the mdio.
- Added so PHY child nodes to ethernet nodes are registered,
  and as part of registration the PHY nodes are reset if
  the reset-gpios property is present.
- Further dropped that the parsed info is stored in the bus,
  as we only do reset once, thus there is no need to
  save the info from the DT
- Futher dropped helper function to reset. They was not needed
- Named the gpio with the name of the PHY node
  in the DT. This makes it unique and easier to recognize.

Changes in v2:
- Added patch to enable Davicom PHY on at91sam9263ek - evaluation kit
- Fix so we do reset before comunicating with the PHY
- Rename to mdio_reset()
- Reference correct binding file in commit log (mdio.txt)
- Tested on at91sam9263ek
  The at91sam9263ek kit do not require the reset like my
  proprietary board, so no DT changes required

Intro:
The following patches was necessary to get networking
operational on my proprietary target.
The target is at91sam9263 based with a Davicom PHY.

The Davicom PHY is a straight copy form the Linux
kernel with the interrupt routine removed and
minor adjustments to the rest.

The davicom PHY would not work until it had seen a reset
cycle - which I think may be an artifact of the board design.

To fix the reset issue I have implemented support for the
reset-gpios binding (see net/phy.txt bindings).
A minimal implmentation was done, just enough to get
my target running.

I could have implemented something in macb -
but I preferred the more generic solution.

Also included are a patch that for the at91sam9263ek
evaluation board. The patch adds several extra tools
that are usefull for testing, and enable the Davicom PHY.

        Sam

Sam Ravnborg (3):
      phylib: add Davicom PHY support
      phylib: add support for reset-gpios
      at91sam9263ek: add PHY, miitool etc. to config


 arch/arm/configs/at91sam9263ek_defconfig |   12 ++
 drivers/net/phy/Kconfig                  |    5 +
 drivers/net/phy/Makefile                 |    1 
 drivers/net/phy/davicom.c                |  140 +++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c               |   82 ++++++++++++++++++
 5 files changed, 239 insertions(+), 1 deletion(-)

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 1/3] phylib: add Davicom PHY support
  2018-08-20 14:52 [PATCH v3 0/3] Add Davicom phy + reset-gpios Sam Ravnborg
@ 2018-08-20 14:56 ` Sam Ravnborg
  2018-08-20 14:56 ` [PATCH v3 2/3] phylib: add support for reset-gpios Sam Ravnborg
  2018-08-20 14:56 ` [PATCH v3 3/3] at91sam9263ek: add PHY, miitool etc. to config Sam Ravnborg
  2 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2018-08-20 14:56 UTC (permalink / raw)
  To: Barebox List; +Cc: Sam Ravnborg

Based on driver from Linux kernel 4.18.0-rc4

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/net/phy/Kconfig   |   5 ++
 drivers/net/phy/Makefile  |   1 +
 drivers/net/phy/davicom.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 drivers/net/phy/davicom.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index cda752b65..79fb917ee 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -18,6 +18,11 @@ config AT803X_PHY
 	---help---
 	  Currently supports the AT8030, AT8031 and AT8035 PHYs.
 
+config DAVICOM_PHY
+	bool "Driver for Davicom PHYs"
+	---help---
+	  Currently supports dm9161e and dm9131
+
 config LXT_PHY
 	bool "Driver for the Intel LXT PHYs"
 	---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 30b20f8ee..4424054d9 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -1,6 +1,7 @@
 obj-y += phy.o mdio_bus.o
 obj-$(CONFIG_AR8327N_PHY)	+= ar8327.o
 obj-$(CONFIG_AT803X_PHY)	+= at803x.o
+obj-$(CONFIG_DAVICOM_PHY)	+= davicom.o
 obj-$(CONFIG_LXT_PHY)		+= lxt.o
 obj-$(CONFIG_MARVELL_PHY)	+= marvell.o
 obj-$(CONFIG_MICREL_PHY)	+= micrel.o
diff --git a/drivers/net/phy/davicom.c b/drivers/net/phy/davicom.c
new file mode 100644
index 000000000..8a784b1e5
--- /dev/null
+++ b/drivers/net/phy/davicom.c
@@ -0,0 +1,140 @@
+/*
+ * drivers/net/phy/davicom.c
+ *
+ * Driver for Davicom PHYs
+ *
+ * Author: Andy Fleming
+ *
+ * Copyright (c) 2004 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <common.h>
+#include <init.h>
+#include <linux/mii.h>
+#include <linux/ethtool.h>
+#include <linux/phy.h>
+
+#define MII_DM9161_SCR		0x10
+#define MII_DM9161_SCR_INIT	0x0610
+#define MII_DM9161_SCR_RMII	0x0100
+
+/* DM9161 Interrupt Register */
+#define MII_DM9161_INTR	0x15
+#define MII_DM9161_INTR_PEND		0x8000
+#define MII_DM9161_INTR_DPLX_MASK	0x0800
+#define MII_DM9161_INTR_SPD_MASK	0x0400
+#define MII_DM9161_INTR_LINK_MASK	0x0200
+#define MII_DM9161_INTR_MASK		0x0100
+#define MII_DM9161_INTR_DPLX_CHANGE	0x0010
+#define MII_DM9161_INTR_SPD_CHANGE	0x0008
+#define MII_DM9161_INTR_LINK_CHANGE	0x0004
+#define MII_DM9161_INTR_INIT 		0x0000
+#define MII_DM9161_INTR_STOP	\
+(MII_DM9161_INTR_DPLX_MASK | MII_DM9161_INTR_SPD_MASK \
+ | MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)
+
+/* DM9161 10BT Configuration/Status */
+#define MII_DM9161_10BTCSR	0x12
+#define MII_DM9161_10BTCSR_INIT	0x7800
+
+MODULE_DESCRIPTION("Davicom PHY driver");
+MODULE_AUTHOR("Andy Fleming");
+MODULE_LICENSE("GPL");
+
+
+static int dm9161_config_aneg(struct phy_device *phydev)
+{
+	int err;
+
+	/* Isolate the PHY */
+	err = phy_write(phydev, MII_BMCR, BMCR_ISOLATE);
+
+	if (err < 0)
+		return err;
+
+	/* Configure the new settings */
+	err = genphy_config_aneg(phydev);
+
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int dm9161_config_init(struct phy_device *phydev)
+{
+	int err, temp;
+
+	/* Isolate the PHY */
+	err = phy_write(phydev, MII_BMCR, BMCR_ISOLATE);
+
+	if (err < 0)
+		return err;
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_MII:
+		temp = MII_DM9161_SCR_INIT;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		temp =  MII_DM9161_SCR_INIT | MII_DM9161_SCR_RMII;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Do not bypass the scrambler/descrambler */
+	err = phy_write(phydev, MII_DM9161_SCR, temp);
+	if (err < 0)
+		return err;
+
+	/* Clear 10BTCSR to default */
+	err = phy_write(phydev, MII_DM9161_10BTCSR, MII_DM9161_10BTCSR_INIT);
+
+	if (err < 0)
+		return err;
+
+	/* Reconnect the PHY, and enable Autonegotiation */
+	return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
+}
+
+static struct phy_driver dm91xx_driver[] = {
+{
+	.phy_id		= 0x0181b880,
+	.drv.name	= "Davicom DM9161E",
+	.phy_id_mask	= 0x0ffffff0,
+	.features	= PHY_BASIC_FEATURES,
+	.config_init	= dm9161_config_init,
+	.config_aneg	= dm9161_config_aneg,
+}, {
+	.phy_id		= 0x0181b8b0,
+	.drv.name	= "Davicom DM9161B/C",
+	.phy_id_mask	= 0x0ffffff0,
+	.features	= PHY_BASIC_FEATURES,
+	.config_init	= dm9161_config_init,
+	.config_aneg	= dm9161_config_aneg,
+}, {
+	.phy_id		= 0x0181b8a0,
+	.drv.name	= "Davicom DM9161A",
+	.phy_id_mask	= 0x0ffffff0,
+	.features	= PHY_BASIC_FEATURES,
+	.config_init	= dm9161_config_init,
+	.config_aneg	= dm9161_config_aneg,
+}, {
+	.phy_id		= 0x00181b80,
+	.drv.name	= "Davicom DM9131",
+	.phy_id_mask	= 0x0ffffff0,
+	.features	= PHY_BASIC_FEATURES,
+} };
+
+static int dm9161_init(void)
+{
+	return phy_drivers_register(dm91xx_driver,
+		ARRAY_SIZE(dm91xx_driver));
+}
+fs_initcall(dm9161_init);
-- 
2.12.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 2/3] phylib: add support for reset-gpios
  2018-08-20 14:52 [PATCH v3 0/3] Add Davicom phy + reset-gpios Sam Ravnborg
  2018-08-20 14:56 ` [PATCH v3 1/3] phylib: add Davicom PHY support Sam Ravnborg
@ 2018-08-20 14:56 ` Sam Ravnborg
  2018-08-21  7:50   ` Sascha Hauer
  2018-08-20 14:56 ` [PATCH v3 3/3] at91sam9263ek: add PHY, miitool etc. to config Sam Ravnborg
  2 siblings, 1 reply; 8+ messages in thread
From: Sam Ravnborg @ 2018-08-20 14:56 UTC (permalink / raw)
  To: Barebox List; +Cc: Sam Ravnborg

Add minimal support for reset-gpios in the PHY node.

Example DT that uses this:

	macb0: ethernet@fffbc000 {
		phy-mode = "rmii";
		#address-cells = <1>;
		#size-cells = <0>;

		ethphy0: ethernet-phy@1 {
			reg = <3>;
			reset-gpios = <&pioE 17 GPIO_ACTIVE_LOW>;
			reset-assert-us = <1000>;
			reset-deassert-us = <2000>;
		};
	};

The reset is required to get the Davicom PHY operational on
my proprietary board, and is assumed relavant for other boards too.

The PHY is reset when we read the info from DT,
before the phy_id is retreived.

The bindings are documented in dts/Bindings/net/phy.txt.

A side-effect of this patch is that we may see
phy devices created from the DT due to the extra call to:

    of_mdiobus_register()

with the ethernet node as argument.

The logic to determine if a child node is a PHY node
is a simpler version compared to the one used in the kernel.
The kernel have a whitelist of compatible strings
and will also reconize compatible = ethernet-phy-idAAAA.BBBB.
If these are required then they can be added later.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/net/phy/mdio_bus.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index d209716a1..c479c25bd 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -17,14 +17,19 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <common.h>
+#include <of_gpio.h>
 #include <driver.h>
 #include <init.h>
+#include <gpio.h>
 #include <clock.h>
 #include <net.h>
 #include <errno.h>
 #include <linux/phy.h>
 #include <linux/err.h>
 
+#define DEFAULT_GPIO_RESET_ASSERT       1000      /* us */
+#define DEFAULT_GPIO_RESET_DEASSERT     1000      /* us */
+
 LIST_HEAD(mii_bus_list);
 
 int mdiobus_detect(struct device_d *dev)
@@ -82,6 +87,76 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 	return 0;
 }
 
+/*
+ * Node is considered a PHY node if:
+ * o Compatible string of "ethernet-phy-ieee802.3-c45"
+ * o Compatible string of "ethernet-phy-ieee802.3-c22"
+ * o No compatibility string
+ *
+ * A device which is not a phy is expected to have a compatible string
+ * indicating what sort of device it is.
+ */
+static bool of_mdiobus_child_is_phy(struct device_node *np)
+{
+	if (of_device_is_compatible(np, "ethernet-phy-ieee802.3-c45"))
+		return true;
+
+	if (of_device_is_compatible(np, "ethernet-phy-ieee802.3-c22"))
+		return true;
+
+	if (!of_find_property(np, "compatible", NULL))
+		return true;
+
+	return false;
+}
+
+/*
+ * Reset the PHY, based on DT info
+ *
+ * If np is a phy node, and the phy node contains a reset-gpios property
+ * then reset the phy.
+ */
+static void of_reset_phy(struct mii_bus *bus, struct device_node *np)
+{
+	enum of_gpio_flags of_flags;
+	u32 reset_deassert_delay;
+	u32 reset_assert_delay;
+	unsigned long flags;
+	int gpio;
+	int ret;
+
+	if (!of_mdiobus_child_is_phy(np))
+		return;
+
+	gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &of_flags);
+	if (!gpio_is_valid(gpio))
+		return;
+
+	flags = GPIOF_OUT_INIT_INACTIVE;
+	if (of_flags & OF_GPIO_ACTIVE_LOW)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	ret = gpio_request_one(gpio, flags, np->name);
+	if (ret) {
+		dev_err(&bus->dev, "failed to request reset gpio for: %s\n",
+			np->name);
+		return;
+	}
+
+	reset_assert_delay = DEFAULT_GPIO_RESET_ASSERT;
+	of_property_read_u32(np, "reset-assert-us", &reset_assert_delay);
+
+	reset_deassert_delay = DEFAULT_GPIO_RESET_DEASSERT;
+	of_property_read_u32(np, "reset-deassert-us", &reset_deassert_delay);
+
+	/* reset the PHY */
+	dev_dbg(&bus->dev, "reset PHY with GPIO: %d\n", gpio);
+	gpio_set_active(gpio, 1);
+	udelay(reset_assert_delay);
+	gpio_set_active(gpio, 0);
+	udelay(reset_deassert_delay);
+}
+
 /**
  * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -111,6 +186,8 @@ static int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 			continue;
 		}
 
+		of_reset_phy(mdio, child);
+
 		of_mdiobus_register_phy(mdio, child, addr);
 	}
 
@@ -154,6 +231,11 @@ int mdiobus_register(struct mii_bus *bus)
 
 	pr_info("%s: probed\n", dev_name(&bus->dev));
 
+	/* register PHY's as child node to the ethernet node */
+	if (bus->parent->device_node)
+		of_mdiobus_register(bus, bus->parent->device_node);
+
+	/* register PHY's as child node to mdio node */
 	if (bus->dev.device_node)
 		of_mdiobus_register(bus, bus->dev.device_node);
 
-- 
2.12.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 3/3] at91sam9263ek: add PHY, miitool etc. to config
  2018-08-20 14:52 [PATCH v3 0/3] Add Davicom phy + reset-gpios Sam Ravnborg
  2018-08-20 14:56 ` [PATCH v3 1/3] phylib: add Davicom PHY support Sam Ravnborg
  2018-08-20 14:56 ` [PATCH v3 2/3] phylib: add support for reset-gpios Sam Ravnborg
@ 2018-08-20 14:56 ` Sam Ravnborg
  2 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2018-08-20 14:56 UTC (permalink / raw)
  To: Barebox List; +Cc: Sam Ravnborg

Added Davicom phy to the config now it is supported.
Add miitool to show the phy info.
Also add a bunch of extra commands that can be useful
when handling the evaluation board.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 arch/arm/configs/at91sam9263ek_defconfig | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/configs/at91sam9263ek_defconfig b/arch/arm/configs/at91sam9263ek_defconfig
index e8ad841fa..45c6f79de 100644
--- a/arch/arm/configs/at91sam9263ek_defconfig
+++ b/arch/arm/configs/at91sam9263ek_defconfig
@@ -18,7 +18,7 @@ CONFIG_BOOTM_OFTREE=y
 CONFIG_BOOTM_OFTREE_UIMAGE=y
 CONFIG_CONSOLE_ACTIVATE_ALL=y
 CONFIG_DEFAULT_ENVIRONMENT_GENERIC=y
-# CONFIG_CMD_ARM_CPUINFO is not set
+CONFIG_CMD_DMESG=y
 CONFIG_LONGHELP=y
 CONFIG_CMD_IOMEM=y
 CONFIG_CMD_MEMINFO=y
@@ -33,22 +33,32 @@ CONFIG_CMD_PRINTENV=y
 CONFIG_CMD_SAVEENV=y
 CONFIG_CMD_SLEEP=y
 CONFIG_CMD_DHCP=y
+CONFIG_CMD_HOST=y
+CONFIG_NET_CMD_IFUP=y
+CONFIG_CMD_MIITOOL=y
 CONFIG_CMD_PING=y
 CONFIG_CMD_TFTP=y
 CONFIG_CMD_EDIT=y
 CONFIG_CMD_SPLASH=y
+CONFIG_CMD_FBTEST=y
 CONFIG_CMD_READLINE=y
 CONFIG_CMD_TIMEOUT=y
 CONFIG_CMD_CLK=y
+CONFIG_CMD_DETECT=y
 CONFIG_CMD_FLASH=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_LED=y
 CONFIG_CMD_LED_TRIGGER=y
+CONFIG_CMD_OF_NODE=y
+CONFIG_CMD_OF_PROPERTY=y
+CONFIG_CMD_OF_DISPLAY_TIMINGS=y
+CONFIG_CMD_OF_FIXUP_STATUS=y
 CONFIG_CMD_OFTREE=y
 CONFIG_NET=y
 CONFIG_NET_NFS=y
 CONFIG_OF_BAREBOX_DRIVERS=y
 CONFIG_DRIVER_NET_MACB=y
+CONFIG_DAVICOM_PHY=y
 # CONFIG_SPI is not set
 CONFIG_MTD=y
 # CONFIG_MTD_OOB_DEVICE is not set
-- 
2.12.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/3] phylib: add support for reset-gpios
  2018-08-20 14:56 ` [PATCH v3 2/3] phylib: add support for reset-gpios Sam Ravnborg
@ 2018-08-21  7:50   ` Sascha Hauer
  2018-08-21 19:46     ` Sam Ravnborg
  0 siblings, 1 reply; 8+ messages in thread
From: Sascha Hauer @ 2018-08-21  7:50 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: Barebox List


Hi Sam,

Looks good generally, but there's one point

On Mon, Aug 20, 2018 at 04:56:11PM +0200, Sam Ravnborg wrote:
>  /**
>   * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
>   * @mdio: pointer to mii_bus structure
> @@ -111,6 +186,8 @@ static int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  			continue;
>  		}
>  
> +		of_reset_phy(mdio, child);
> +
>  		of_mdiobus_register_phy(mdio, child, addr);
>  	}
>  
> @@ -154,6 +231,11 @@ int mdiobus_register(struct mii_bus *bus)
>  
>  	pr_info("%s: probed\n", dev_name(&bus->dev));
>  
> +	/* register PHY's as child node to the ethernet node */
> +	if (bus->parent->device_node)
> +		of_mdiobus_register(bus, bus->parent->device_node);

I am not so convinced of this change. Here you assume that bus->parent
is the ethernet node, but this doesn't necessarily have to be the case
as some mdio controllers are standalone and not part of an ethernet
device (see "virtual,mdio-gpio" to get examples).

So instead of introducing this change I would prefer if you let the code
below trigger:

> +
> +	/* register PHY's as child node to mdio node */
>  	if (bus->dev.device_node)
>  		of_mdiobus_register(bus, bus->dev.device_node);

This means you have to set bus->dev.device_node in your ethernet driver
like some drivers already do:

drivers/net/macb.c:666:                 macb->miibus.dev.device_node = mdiobus;

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/3] phylib: add support for reset-gpios
  2018-08-21  7:50   ` Sascha Hauer
@ 2018-08-21 19:46     ` Sam Ravnborg
  2018-08-22 13:14       ` Ahmad Fatoum
  0 siblings, 1 reply; 8+ messages in thread
From: Sam Ravnborg @ 2018-08-21 19:46 UTC (permalink / raw)
  To: Sascha Hauer, Andrey Smirnov; +Cc: Barebox List

Hi Sasha.

To Andrey - please read comment on mdio support in macb driver.

> > +	/* register PHY's as child node to the ethernet node */
> > +	if (bus->parent->device_node)
> > +		of_mdiobus_register(bus, bus->parent->device_node);
> 
> I am not so convinced of this change. Here you assume that bus->parent
> is the ethernet node, but this doesn't necessarily have to be the case
> as some mdio controllers are standalone and not part of an ethernet
> device (see "virtual,mdio-gpio" to get examples).
> 
> So instead of introducing this change I would prefer if you let the code
> below trigger:
> 
> > +
> > +	/* register PHY's as child node to mdio node */
> >  	if (bus->dev.device_node)
> >  		of_mdiobus_register(bus, bus->dev.device_node);
> 
> This means you have to set bus->dev.device_node in your ethernet driver
> like some drivers already do:
> 
> drivers/net/macb.c:666:                 macb->miibus.dev.device_node = mdiobus;

I have analyzed on the current situation.

In barebox we have two drivers that set miibus.dev.device_node,
both in the case where they have found a child named "mdio".
-> fec-imx.c and macb.c
When reading dts/Bindings/net/fsl-fec.txt then this binding specify
an optional mdio child, so fec-imx.c is fine.
But macb.txt or the general bindigns do not mention
a mdio child.
And the Linux macb_main.c do not support a "mdio" node either.
So my best guess is that the "mdio" support in macb was accidently
ported over from fec-imx when adding DT support.

If we only consider the DT cases we will call mdio_register()
in the following cases:

1) Typical from an ethernet driver:
bus->dev.device_node is NULL
bus->parent->device_node is the ethernet node

2) fec-imx case:
bus->dev.device_node is the "mdio" node, that agin hold the PHY nodes
bus->parent->device_node is the ethernet node

3) mdio drivers:
bus->dev.device_node is the "mdio" node
bus->parent->device_node is equal to bus->dev.device_node


Considering the above I will do the following:
a) Remove "mdio" support from macb.c
b) Modify my patch to something like this:

	if (bus->dev.device_node)
		of_mdiobus_register(bus, bus->dev.device_node);
	else if (bus->parent->device_node)
		of_mdiobus_register(bus, bus->parent->device_node);

With the above I have covered point 1 to 3 above in a simple way.
And this looks a simple and straghtforward way to do it.

I also looked at how the kernel does thing.
In of_mdiobus_register it has:

	for_each_child() {
		if (of_node_is_phy(child))
			of_mdiobus_register_phy()
		else
			of_mdiobus_register_device()
	}

I so far failed to follow where the code will iterate over
the PHY nodes inside the mdio node (that trigger the
of_mdiobus_register_device() call).
So therefore I am reluctant to try to implment this,
and the suggested solution is also more in the line
with a simpler and straightforward approach preferred in barebox.

I no-one else objects and I do not get any better ideas I will
implement and test this tomorrow.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/3] phylib: add support for reset-gpios
  2018-08-21 19:46     ` Sam Ravnborg
@ 2018-08-22 13:14       ` Ahmad Fatoum
  2018-08-22 18:26         ` Sam Ravnborg
  0 siblings, 1 reply; 8+ messages in thread
From: Ahmad Fatoum @ 2018-08-22 13:14 UTC (permalink / raw)
  To: barebox

Hello Sam,

On 08/21/2018 09:46 PM, Sam Ravnborg wrote:
> But macb.txt or the general bindigns do not mention
> a mdio child.
> And the Linux macb_main.c do not support a "mdio" node either.
> So my best guess is that the "mdio" support in macb was accidently
> ported over from fec-imx when adding DT support.
[snip]
> Considering the above I will do the following:
> a) Remove "mdio" support from macb.c

While submitting a macb patch to linux-netdev, I was asked [1] to add
support for a "mdio" subtree in the macb binding. I intend to submit
it when net-next opens for inclusion into Linux v4.20.
That way, macb would look at phys in the list of its children as well
as the children of its "mdio" child.

Just a heads up to avoid removing functionality that would've to be
reinstated later on...

[1]: https://www.spinics.net/lists/netdev/msg519196.html

Cheers
Ahmad

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/3] phylib: add support for reset-gpios
  2018-08-22 13:14       ` Ahmad Fatoum
@ 2018-08-22 18:26         ` Sam Ravnborg
  0 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2018-08-22 18:26 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

Hi Ahmad.

On Wed, Aug 22, 2018 at 03:14:57PM +0200, Ahmad Fatoum wrote:
> Hello Sam,
> 
> On 08/21/2018 09:46 PM, Sam Ravnborg wrote:
> > But macb.txt or the general bindigns do not mention
> > a mdio child.
> > And the Linux macb_main.c do not support a "mdio" node either.
> > So my best guess is that the "mdio" support in macb was accidently
> > ported over from fec-imx when adding DT support.
> [snip]
> > Considering the above I will do the following:
> > a) Remove "mdio" support from macb.c
> 
> While submitting a macb patch to linux-netdev, I was asked [1] to add
> support for a "mdio" subtree in the macb binding. I intend to submit
> it when net-next opens for inclusion into Linux v4.20.
> That way, macb would look at phys in the list of its children as well
> as the children of its "mdio" child.
> 
> Just a heads up to avoid removing functionality that would've to be
> reinstated later on...
> 
> [1]: https://www.spinics.net/lists/netdev/msg519196.html

Thanks for bringing this to my attention.
I will leave the code as-is.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-08-22 18:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-20 14:52 [PATCH v3 0/3] Add Davicom phy + reset-gpios Sam Ravnborg
2018-08-20 14:56 ` [PATCH v3 1/3] phylib: add Davicom PHY support Sam Ravnborg
2018-08-20 14:56 ` [PATCH v3 2/3] phylib: add support for reset-gpios Sam Ravnborg
2018-08-21  7:50   ` Sascha Hauer
2018-08-21 19:46     ` Sam Ravnborg
2018-08-22 13:14       ` Ahmad Fatoum
2018-08-22 18:26         ` Sam Ravnborg
2018-08-20 14:56 ` [PATCH v3 3/3] at91sam9263ek: add PHY, miitool etc. to config Sam Ravnborg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox