[PATCH] net/fec: add device tree probe support

[PATCH] net/fec: add device tree probe support

[Shawn Guo]

It adds device tree probe support for fec driver.

Signed-off-by: Jason Liu <jason.hui@linaro.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
 drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
 2 files changed, 139 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
new file mode 100644
index 0000000..1dad888
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -0,0 +1,24 @@
+* Freescale Fast Ethernet Controller (FEC)
+
+Required properties:
+- compatible : Should be "fsl,<soc>-fec"
+- reg : Address and length of the register set for the device
+- interrupts : Should contain fec interrupt
+- phy-mode : String, operation mode of the PHY interface.
+  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
+  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
+- gpios : Should specify the gpio for phy reset
+
+Optional properties:
+- local-mac-address : 6 bytes, mac address
+
+Example:
+
+fec@83fec000 {
+	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
+	reg = <0x83fec000 0x4000>;
+	interrupts = <87>;
+	phy-mode = "mii";
+	gpios = <&gpio1 14 0>; /* phy-reset, GPIO2_14 */
+	local-mac-address = [00 04 9F 01 1B B9];
+};
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 7ae3f28..3ee05ff 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -44,6 +44,9 @@
 #include <linux/platform_device.h>
 #include <linux/phy.h>
 #include <linux/fec.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
 
 #include <asm/cacheflush.h>
 
@@ -78,6 +81,17 @@ static struct platform_device_id fec_devtype[] = {
 	{ }
 };
 
+enum fec_type {
+	IMX27_FEC,
+	IMX28_FEC,
+};
+
+static const struct of_device_id fec_dt_ids[] = {
+	{ .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
+	{ .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },
+	{ /* sentinel */ },
+};
+
 static unsigned char macaddr[ETH_ALEN];
 module_param_array(macaddr, byte, NULL, 0);
 MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
@@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 	 */
 	iap = macaddr;
 
+#ifdef CONFIG_OF
+	/*
+	 * 2) from device tree data
+	 */
+	if (!is_valid_ether_addr(iap)) {
+		struct device_node *np = fep->pdev->dev.of_node;
+		if (np) {
+			const char *mac;
+			int err;
+			err = of_property_read_string(np,
+					"local-mac-address", &mac);
+			if (err)
+				iap = (unsigned char *) mac;
+		}
+	}
+#endif
+
 	/*
-	 * 2) from flash or fuse (via platform data)
+	 * 3) from flash or fuse (via platform data)
 	 */
 	if (!is_valid_ether_addr(iap)) {
 #ifdef CONFIG_M5272
@@ -748,7 +779,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 	}
 
 	/*
-	 * 3) FEC mac registers set by bootloader
+	 * 4) FEC mac registers set by bootloader
 	 */
 	if (!is_valid_ether_addr(iap)) {
 		*((unsigned long *) &tmpaddr[0]) =
@@ -1358,6 +1389,72 @@ static int fec_enet_init(struct net_device *ndev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static int __devinit fec_get_phy_mode_dt(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int err, i;
+	const char *pm, *phy_modes[] = {
+		[PHY_INTERFACE_MODE_MII]	= "mii",
+		[PHY_INTERFACE_MODE_GMII]	= "gmii",
+		[PHY_INTERFACE_MODE_SGMII]	= "sgmii",
+		[PHY_INTERFACE_MODE_TBI]	= "tbi",
+		[PHY_INTERFACE_MODE_RMII]	= "rmii",
+		[PHY_INTERFACE_MODE_RGMII]	= "rgmii",
+		[PHY_INTERFACE_MODE_RGMII_ID]	= "rgmii-id",
+		[PHY_INTERFACE_MODE_RGMII_RXID]	= "rgmii-rxid",
+		[PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
+		[PHY_INTERFACE_MODE_RTBI]	= "rtbi",
+	};
+
+	if (np) {
+		err = of_property_read_string(np, "phy-mode", &pm);
+		if (err)
+			return err;
+		for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
+			if (!strcasecmp(pm, phy_modes[i]))
+				return i;
+	}
+
+	return -ENODEV;
+}
+
+static int __devinit fec_reset_phy(struct platform_device *pdev)
+{
+	int err, phy_reset;
+	struct device_node *np = pdev->dev.of_node;
+
+	if (!np)
+		return -ENODEV;
+
+	phy_reset = of_get_gpio(np, 0);
+	err = gpio_request_one(phy_reset, GPIOF_OUT_INIT_LOW, "phy-reset");
+	if (err) {
+		pr_warn("FEC: failed to get gpio phy-reset: %d\n", err);
+		return err;
+	}
+
+	msleep(1);
+	gpio_set_value(phy_reset, 1);
+
+	return 0;
+}
+#else /* CONFIG_OF */
+static inline int fec_get_phy_mode_dt(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+
+static inline int fec_reset_phy(struct platform_device *pdev)
+{
+	/*
+	 * In case of platform probe, the reset has been done
+	 * by machine code.
+	 */
+	return 0;
+}
+#endif /* CONFIG_OF */
+
 static int __devinit
 fec_probe(struct platform_device *pdev)
 {
@@ -1366,6 +1463,11 @@ fec_probe(struct platform_device *pdev)
 	struct net_device *ndev;
 	int i, irq, ret = 0;
 	struct resource *r;
+	const struct of_device_id *of_id;
+
+	of_id = of_match_device(fec_dt_ids, &pdev->dev);
+	if (of_id)
+		pdev->id_entry = (struct platform_device_id *) of_id->data;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r)
@@ -1397,9 +1499,16 @@ fec_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ndev);
 
-	pdata = pdev->dev.platform_data;
-	if (pdata)
-		fep->phy_interface = pdata->phy;
+	fep->phy_interface = fec_get_phy_mode_dt(pdev);
+	if (fep->phy_interface == -ENODEV) {
+		pdata = pdev->dev.platform_data;
+		if (pdata)
+			fep->phy_interface = pdata->phy;
+		else
+			fep->phy_interface = PHY_INTERFACE_MODE_MII;
+	}
+
+	fec_reset_phy(pdev);
 
 	/* This device has up to three irqs on some platforms */
 	for (i = 0; i < 3; i++) {
@@ -1534,6 +1643,7 @@ static struct platform_driver fec_driver = {
 #ifdef CONFIG_PM
 		.pm	= &fec_pm_ops,
 #endif
+		.of_match_table = fec_dt_ids,
 	},
 	.id_table = fec_devtype,
 	.probe	= fec_probe,
-- 
1.7.4.1

[PATCH] net/fec: add device tree probe support

[Shawn Guo]

It adds device tree probe support for fec driver.

Signed-off-by: Jason Liu <jason.hui@linaro.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
 drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
 2 files changed, 139 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
new file mode 100644
index 0000000..1dad888
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -0,0 +1,24 @@
+* Freescale Fast Ethernet Controller (FEC)
+
+Required properties:
+- compatible : Should be "fsl,<soc>-fec"
+- reg : Address and length of the register set for the device
+- interrupts : Should contain fec interrupt
+- phy-mode : String, operation mode of the PHY interface.
+  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
+  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
+- gpios : Should specify the gpio for phy reset
+
+Optional properties:
+- local-mac-address : 6 bytes, mac address
+
+Example:
+
+fec at 83fec000 {
+	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
+	reg = <0x83fec000 0x4000>;
+	interrupts = <87>;
+	phy-mode = "mii";
+	gpios = <&gpio1 14 0>; /* phy-reset, GPIO2_14 */
+	local-mac-address = [00 04 9F 01 1B B9];
+};
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 7ae3f28..3ee05ff 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -44,6 +44,9 @@
 #include <linux/platform_device.h>
 #include <linux/phy.h>
 #include <linux/fec.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
 
 #include <asm/cacheflush.h>
 
@@ -78,6 +81,17 @@ static struct platform_device_id fec_devtype[] = {
 	{ }
 };
 
+enum fec_type {
+	IMX27_FEC,
+	IMX28_FEC,
+};
+
+static const struct of_device_id fec_dt_ids[] = {
+	{ .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
+	{ .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },
+	{ /* sentinel */ },
+};
+
 static unsigned char macaddr[ETH_ALEN];
 module_param_array(macaddr, byte, NULL, 0);
 MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
@@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 	 */
 	iap = macaddr;
 
+#ifdef CONFIG_OF
+	/*
+	 * 2) from device tree data
+	 */
+	if (!is_valid_ether_addr(iap)) {
+		struct device_node *np = fep->pdev->dev.of_node;
+		if (np) {
+			const char *mac;
+			int err;
+			err = of_property_read_string(np,
+					"local-mac-address", &mac);
+			if (err)
+				iap = (unsigned char *) mac;
+		}
+	}
+#endif
+
 	/*
-	 * 2) from flash or fuse (via platform data)
+	 * 3) from flash or fuse (via platform data)
 	 */
 	if (!is_valid_ether_addr(iap)) {
 #ifdef CONFIG_M5272
@@ -748,7 +779,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 	}
 
 	/*
-	 * 3) FEC mac registers set by bootloader
+	 * 4) FEC mac registers set by bootloader
 	 */
 	if (!is_valid_ether_addr(iap)) {
 		*((unsigned long *) &tmpaddr[0]) =
@@ -1358,6 +1389,72 @@ static int fec_enet_init(struct net_device *ndev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static int __devinit fec_get_phy_mode_dt(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int err, i;
+	const char *pm, *phy_modes[] = {
+		[PHY_INTERFACE_MODE_MII]	= "mii",
+		[PHY_INTERFACE_MODE_GMII]	= "gmii",
+		[PHY_INTERFACE_MODE_SGMII]	= "sgmii",
+		[PHY_INTERFACE_MODE_TBI]	= "tbi",
+		[PHY_INTERFACE_MODE_RMII]	= "rmii",
+		[PHY_INTERFACE_MODE_RGMII]	= "rgmii",
+		[PHY_INTERFACE_MODE_RGMII_ID]	= "rgmii-id",
+		[PHY_INTERFACE_MODE_RGMII_RXID]	= "rgmii-rxid",
+		[PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
+		[PHY_INTERFACE_MODE_RTBI]	= "rtbi",
+	};
+
+	if (np) {
+		err = of_property_read_string(np, "phy-mode", &pm);
+		if (err)
+			return err;
+		for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
+			if (!strcasecmp(pm, phy_modes[i]))
+				return i;
+	}
+
+	return -ENODEV;
+}
+
+static int __devinit fec_reset_phy(struct platform_device *pdev)
+{
+	int err, phy_reset;
+	struct device_node *np = pdev->dev.of_node;
+
+	if (!np)
+		return -ENODEV;
+
+	phy_reset = of_get_gpio(np, 0);
+	err = gpio_request_one(phy_reset, GPIOF_OUT_INIT_LOW, "phy-reset");
+	if (err) {
+		pr_warn("FEC: failed to get gpio phy-reset: %d\n", err);
+		return err;
+	}
+
+	msleep(1);
+	gpio_set_value(phy_reset, 1);
+
+	return 0;
+}
+#else /* CONFIG_OF */
+static inline int fec_get_phy_mode_dt(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+
+static inline int fec_reset_phy(struct platform_device *pdev)
+{
+	/*
+	 * In case of platform probe, the reset has been done
+	 * by machine code.
+	 */
+	return 0;
+}
+#endif /* CONFIG_OF */
+
 static int __devinit
 fec_probe(struct platform_device *pdev)
 {
@@ -1366,6 +1463,11 @@ fec_probe(struct platform_device *pdev)
 	struct net_device *ndev;
 	int i, irq, ret = 0;
 	struct resource *r;
+	const struct of_device_id *of_id;
+
+	of_id = of_match_device(fec_dt_ids, &pdev->dev);
+	if (of_id)
+		pdev->id_entry = (struct platform_device_id *) of_id->data;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r)
@@ -1397,9 +1499,16 @@ fec_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ndev);
 
-	pdata = pdev->dev.platform_data;
-	if (pdata)
-		fep->phy_interface = pdata->phy;
+	fep->phy_interface = fec_get_phy_mode_dt(pdev);
+	if (fep->phy_interface == -ENODEV) {
+		pdata = pdev->dev.platform_data;
+		if (pdata)
+			fep->phy_interface = pdata->phy;
+		else
+			fep->phy_interface = PHY_INTERFACE_MODE_MII;
+	}
+
+	fec_reset_phy(pdev);
 
 	/* This device has up to three irqs on some platforms */
 	for (i = 0; i < 3; i++) {
@@ -1534,6 +1643,7 @@ static struct platform_driver fec_driver = {
 #ifdef CONFIG_PM
 		.pm	= &fec_pm_ops,
 #endif
+		.of_match_table = fec_dt_ids,
 	},
 	.id_table = fec_devtype,
 	.probe	= fec_probe,
-- 
1.7.4.1

Re: [PATCH] net/fec: add device tree probe support

[Grant Likely]

On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> It adds device tree probe support for fec driver.
> 
> Signed-off-by: Jason Liu <jason.hui@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Minor comments below.  After addressing them you can add my:

Acked-by: Grant Likely <grant.likely@secretlab.ca>

I don't see any reason not to merge this one in v3.1

g.

> ---
>  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
>  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
>  2 files changed, 139 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> new file mode 100644
> index 0000000..1dad888
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -0,0 +1,24 @@
> +* Freescale Fast Ethernet Controller (FEC)
> +
> +Required properties:
> +- compatible : Should be "fsl,<soc>-fec"
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain fec interrupt
> +- phy-mode : String, operation mode of the PHY interface.
> +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".

We don't have a common binding for this yet.  Should be
"fsl,phy-mode".

> +- gpios : Should specify the gpio for phy reset
> +
> +Optional properties:
> +- local-mac-address : 6 bytes, mac address
> +
> +Example:
> +
> +fec@83fec000 {
> +	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
> +	reg = <0x83fec000 0x4000>;
> +	interrupts = <87>;
> +	phy-mode = "mii";
> +	gpios = <&gpio1 14 0>; /* phy-reset, GPIO2_14 */
> +	local-mac-address = [00 04 9F 01 1B B9];
> +};
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 7ae3f28..3ee05ff 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -44,6 +44,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/phy.h>
>  #include <linux/fec.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>  
>  #include <asm/cacheflush.h>
>  
> @@ -78,6 +81,17 @@ static struct platform_device_id fec_devtype[] = {
>  	{ }
>  };
>  
> +enum fec_type {
> +	IMX27_FEC,
> +	IMX28_FEC,
> +};
> +
> +static const struct of_device_id fec_dt_ids[] = {
> +	{ .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
> +	{ .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },

Personally, I'd drop the fec_devtype array entirely and have discrete
structures for each, but that isn't a big enough objection to withhold
my acked-by.

> +	{ /* sentinel */ },

Drop the trailing comma.

> +};
> +
>  static unsigned char macaddr[ETH_ALEN];
>  module_param_array(macaddr, byte, NULL, 0);
>  MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
> @@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
>  	 */
>  	iap = macaddr;
>  
> +#ifdef CONFIG_OF
> +	/*
> +	 * 2) from device tree data
> +	 */
> +	if (!is_valid_ether_addr(iap)) {
> +		struct device_node *np = fep->pdev->dev.of_node;
> +		if (np) {
> +			const char *mac;
> +			int err;
> +			err = of_property_read_string(np,
> +					"local-mac-address", &mac);
> +			if (err)
> +				iap = (unsigned char *) mac;

There is already a function for doing this.  of_get_mac_address().
And mac address is *not* a string, it is a byte array.

> +		}
> +	}
> +#endif
> +
>  	/*
> -	 * 2) from flash or fuse (via platform data)
> +	 * 3) from flash or fuse (via platform data)
>  	 */
>  	if (!is_valid_ether_addr(iap)) {
>  #ifdef CONFIG_M5272
> @@ -748,7 +779,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
>  	}
>  
>  	/*
> -	 * 3) FEC mac registers set by bootloader
> +	 * 4) FEC mac registers set by bootloader
>  	 */
>  	if (!is_valid_ether_addr(iap)) {
>  		*((unsigned long *) &tmpaddr[0]) =
> @@ -1358,6 +1389,72 @@ static int fec_enet_init(struct net_device *ndev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static int __devinit fec_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int err, i;
> +	const char *pm, *phy_modes[] = {
> +		[PHY_INTERFACE_MODE_MII]	= "mii",
> +		[PHY_INTERFACE_MODE_GMII]	= "gmii",
> +		[PHY_INTERFACE_MODE_SGMII]	= "sgmii",
> +		[PHY_INTERFACE_MODE_TBI]	= "tbi",
> +		[PHY_INTERFACE_MODE_RMII]	= "rmii",
> +		[PHY_INTERFACE_MODE_RGMII]	= "rgmii",
> +		[PHY_INTERFACE_MODE_RGMII_ID]	= "rgmii-id",
> +		[PHY_INTERFACE_MODE_RGMII_RXID]	= "rgmii-rxid",
> +		[PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> +		[PHY_INTERFACE_MODE_RTBI]	= "rtbi",
> +	};

The phymodes table should be defined outside of the function.

> +
> +	if (np) {
> +		err = of_property_read_string(np, "phy-mode", &pm);
> +		if (err)
> +			return err;
> +		for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> +			if (!strcasecmp(pm, phy_modes[i]))
> +				return i;
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int __devinit fec_reset_phy(struct platform_device *pdev)
> +{
> +	int err, phy_reset;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	phy_reset = of_get_gpio(np, 0);
> +	err = gpio_request_one(phy_reset, GPIOF_OUT_INIT_LOW, "phy-reset");
> +	if (err) {
> +		pr_warn("FEC: failed to get gpio phy-reset: %d\n", err);
> +		return err;
> +	}
> +
> +	msleep(1);
> +	gpio_set_value(phy_reset, 1);
> +
> +	return 0;
> +}
> +#else /* CONFIG_OF */
> +static inline int fec_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int fec_reset_phy(struct platform_device *pdev)
> +{
> +	/*
> +	 * In case of platform probe, the reset has been done
> +	 * by machine code.
> +	 */
> +	return 0;
> +}
> +#endif /* CONFIG_OF */
> +
>  static int __devinit
>  fec_probe(struct platform_device *pdev)
>  {
> @@ -1366,6 +1463,11 @@ fec_probe(struct platform_device *pdev)
>  	struct net_device *ndev;
>  	int i, irq, ret = 0;
>  	struct resource *r;
> +	const struct of_device_id *of_id;
> +
> +	of_id = of_match_device(fec_dt_ids, &pdev->dev);
> +	if (of_id)
> +		pdev->id_entry = (struct platform_device_id *) of_id->data;

of_id->data is a void*.  You don't need the cast.

>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!r)
> @@ -1397,9 +1499,16 @@ fec_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, ndev);
>  
> -	pdata = pdev->dev.platform_data;
> -	if (pdata)
> -		fep->phy_interface = pdata->phy;
> +	fep->phy_interface = fec_get_phy_mode_dt(pdev);
> +	if (fep->phy_interface == -ENODEV) {
> +		pdata = pdev->dev.platform_data;
> +		if (pdata)
> +			fep->phy_interface = pdata->phy;
> +		else
> +			fep->phy_interface = PHY_INTERFACE_MODE_MII;
> +	}
> +
> +	fec_reset_phy(pdev);
>  
>  	/* This device has up to three irqs on some platforms */
>  	for (i = 0; i < 3; i++) {
> @@ -1534,6 +1643,7 @@ static struct platform_driver fec_driver = {
>  #ifdef CONFIG_PM
>  		.pm	= &fec_pm_ops,
>  #endif
> +		.of_match_table = fec_dt_ids,
>  	},
>  	.id_table = fec_devtype,
>  	.probe	= fec_probe,
> -- 
> 1.7.4.1
> 

[PATCH] net/fec: add device tree probe support

[Grant Likely]

On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> It adds device tree probe support for fec driver.
> 
> Signed-off-by: Jason Liu <jason.hui@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Minor comments below.  After addressing them you can add my:

Acked-by: Grant Likely <grant.likely@secretlab.ca>

I don't see any reason not to merge this one in v3.1

g.

> ---
>  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
>  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
>  2 files changed, 139 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> new file mode 100644
> index 0000000..1dad888
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -0,0 +1,24 @@
> +* Freescale Fast Ethernet Controller (FEC)
> +
> +Required properties:
> +- compatible : Should be "fsl,<soc>-fec"
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain fec interrupt
> +- phy-mode : String, operation mode of the PHY interface.
> +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".

We don't have a common binding for this yet.  Should be
"fsl,phy-mode".

> +- gpios : Should specify the gpio for phy reset
> +
> +Optional properties:
> +- local-mac-address : 6 bytes, mac address
> +
> +Example:
> +
> +fec at 83fec000 {
> +	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
> +	reg = <0x83fec000 0x4000>;
> +	interrupts = <87>;
> +	phy-mode = "mii";
> +	gpios = <&gpio1 14 0>; /* phy-reset, GPIO2_14 */
> +	local-mac-address = [00 04 9F 01 1B B9];
> +};
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 7ae3f28..3ee05ff 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -44,6 +44,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/phy.h>
>  #include <linux/fec.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>  
>  #include <asm/cacheflush.h>
>  
> @@ -78,6 +81,17 @@ static struct platform_device_id fec_devtype[] = {
>  	{ }
>  };
>  
> +enum fec_type {
> +	IMX27_FEC,
> +	IMX28_FEC,
> +};
> +
> +static const struct of_device_id fec_dt_ids[] = {
> +	{ .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
> +	{ .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },

Personally, I'd drop the fec_devtype array entirely and have discrete
structures for each, but that isn't a big enough objection to withhold
my acked-by.

> +	{ /* sentinel */ },

Drop the trailing comma.

> +};
> +
>  static unsigned char macaddr[ETH_ALEN];
>  module_param_array(macaddr, byte, NULL, 0);
>  MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
> @@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
>  	 */
>  	iap = macaddr;
>  
> +#ifdef CONFIG_OF
> +	/*
> +	 * 2) from device tree data
> +	 */
> +	if (!is_valid_ether_addr(iap)) {
> +		struct device_node *np = fep->pdev->dev.of_node;
> +		if (np) {
> +			const char *mac;
> +			int err;
> +			err = of_property_read_string(np,
> +					"local-mac-address", &mac);
> +			if (err)
> +				iap = (unsigned char *) mac;

There is already a function for doing this.  of_get_mac_address().
And mac address is *not* a string, it is a byte array.

> +		}
> +	}
> +#endif
> +
>  	/*
> -	 * 2) from flash or fuse (via platform data)
> +	 * 3) from flash or fuse (via platform data)
>  	 */
>  	if (!is_valid_ether_addr(iap)) {
>  #ifdef CONFIG_M5272
> @@ -748,7 +779,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
>  	}
>  
>  	/*
> -	 * 3) FEC mac registers set by bootloader
> +	 * 4) FEC mac registers set by bootloader
>  	 */
>  	if (!is_valid_ether_addr(iap)) {
>  		*((unsigned long *) &tmpaddr[0]) =
> @@ -1358,6 +1389,72 @@ static int fec_enet_init(struct net_device *ndev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static int __devinit fec_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int err, i;
> +	const char *pm, *phy_modes[] = {
> +		[PHY_INTERFACE_MODE_MII]	= "mii",
> +		[PHY_INTERFACE_MODE_GMII]	= "gmii",
> +		[PHY_INTERFACE_MODE_SGMII]	= "sgmii",
> +		[PHY_INTERFACE_MODE_TBI]	= "tbi",
> +		[PHY_INTERFACE_MODE_RMII]	= "rmii",
> +		[PHY_INTERFACE_MODE_RGMII]	= "rgmii",
> +		[PHY_INTERFACE_MODE_RGMII_ID]	= "rgmii-id",
> +		[PHY_INTERFACE_MODE_RGMII_RXID]	= "rgmii-rxid",
> +		[PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> +		[PHY_INTERFACE_MODE_RTBI]	= "rtbi",
> +	};

The phymodes table should be defined outside of the function.

> +
> +	if (np) {
> +		err = of_property_read_string(np, "phy-mode", &pm);
> +		if (err)
> +			return err;
> +		for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> +			if (!strcasecmp(pm, phy_modes[i]))
> +				return i;
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int __devinit fec_reset_phy(struct platform_device *pdev)
> +{
> +	int err, phy_reset;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	phy_reset = of_get_gpio(np, 0);
> +	err = gpio_request_one(phy_reset, GPIOF_OUT_INIT_LOW, "phy-reset");
> +	if (err) {
> +		pr_warn("FEC: failed to get gpio phy-reset: %d\n", err);
> +		return err;
> +	}
> +
> +	msleep(1);
> +	gpio_set_value(phy_reset, 1);
> +
> +	return 0;
> +}
> +#else /* CONFIG_OF */
> +static inline int fec_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int fec_reset_phy(struct platform_device *pdev)
> +{
> +	/*
> +	 * In case of platform probe, the reset has been done
> +	 * by machine code.
> +	 */
> +	return 0;
> +}
> +#endif /* CONFIG_OF */
> +
>  static int __devinit
>  fec_probe(struct platform_device *pdev)
>  {
> @@ -1366,6 +1463,11 @@ fec_probe(struct platform_device *pdev)
>  	struct net_device *ndev;
>  	int i, irq, ret = 0;
>  	struct resource *r;
> +	const struct of_device_id *of_id;
> +
> +	of_id = of_match_device(fec_dt_ids, &pdev->dev);
> +	if (of_id)
> +		pdev->id_entry = (struct platform_device_id *) of_id->data;

of_id->data is a void*.  You don't need the cast.

>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!r)
> @@ -1397,9 +1499,16 @@ fec_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, ndev);
>  
> -	pdata = pdev->dev.platform_data;
> -	if (pdata)
> -		fep->phy_interface = pdata->phy;
> +	fep->phy_interface = fec_get_phy_mode_dt(pdev);
> +	if (fep->phy_interface == -ENODEV) {
> +		pdata = pdev->dev.platform_data;
> +		if (pdata)
> +			fep->phy_interface = pdata->phy;
> +		else
> +			fep->phy_interface = PHY_INTERFACE_MODE_MII;
> +	}
> +
> +	fec_reset_phy(pdev);
>  
>  	/* This device has up to three irqs on some platforms */
>  	for (i = 0; i < 3; i++) {
> @@ -1534,6 +1643,7 @@ static struct platform_driver fec_driver = {
>  #ifdef CONFIG_PM
>  		.pm	= &fec_pm_ops,
>  #endif
> +		.of_match_table = fec_dt_ids,
>  	},
>  	.id_table = fec_devtype,
>  	.probe	= fec_probe,
> -- 
> 1.7.4.1
> 

Re: [PATCH] net/fec: add device tree probe support

[Shawn Guo]

On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > It adds device tree probe support for fec driver.
> > 
> > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> 
> Minor comments below.  After addressing them you can add my:
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> 
> I don't see any reason not to merge this one in v3.1
> 
> g.
> 
> > ---
> >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> >  2 files changed, 139 insertions(+), 5 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > new file mode 100644
> > index 0000000..1dad888
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > @@ -0,0 +1,24 @@
> > +* Freescale Fast Ethernet Controller (FEC)
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<soc>-fec"
> > +- reg : Address and length of the register set for the device
> > +- interrupts : Should contain fec interrupt
> > +- phy-mode : String, operation mode of the PHY interface.
> > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> 
> We don't have a common binding for this yet.  Should be
> "fsl,phy-mode".
> 
There is nothing really fsl specific.  How does the following patch
look to you?

---8<---------
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 86f334a..cc117db 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -8,6 +8,49 @@
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
 #include <linux/of_net.h>
+#include <linux/phy.h>
+
+/**
+ * It maps 'enum phy_interface_t' found in include/linux/phy.h
+ * into the device tree binding of 'phy-mode', so that Ethernet
+ * device driver can get phy interface from device tree.
+ */
+static const char *phy_modes[] = {
+       [PHY_INTERFACE_MODE_MII]        = "mii",
+       [PHY_INTERFACE_MODE_GMII]       = "gmii",
+       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
+       [PHY_INTERFACE_MODE_TBI]        = "tbi",
+       [PHY_INTERFACE_MODE_RMII]       = "rmii",
+       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
+       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
+       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
+       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
+       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
+};
+
+/**
+ * of_get_phy_mode - Get phy mode for given device_node
+ * @np:        Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy-mode',
+ * and return its index in phy_modes table, or errno in error case.
+ */
+const int of_get_phy_mode(struct device_node *np)
+{
+       const char *pm;
+       int err, i;
+
+       err = of_property_read_string(np, "phy-mode", &pm);
+       if (err < 0)
+               return err;
+
+       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
+               if (!strcasecmp(pm, phy_modes[i]))
+                       return i;
+
+       return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(of_get_phy_mode);

 /**
  * Search the device tree for the best MAC address to use.  'mac-address' is
------------

[...]
> > @@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> >  	 */
> >  	iap = macaddr;
> >  
> > +#ifdef CONFIG_OF
> > +	/*
> > +	 * 2) from device tree data
> > +	 */
> > +	if (!is_valid_ether_addr(iap)) {
> > +		struct device_node *np = fep->pdev->dev.of_node;
> > +		if (np) {
> > +			const char *mac;
> > +			int err;
> > +			err = of_property_read_string(np,
> > +					"local-mac-address", &mac);
> > +			if (err)
> > +				iap = (unsigned char *) mac;
> 
> There is already a function for doing this.  of_get_mac_address().
> And mac address is *not* a string, it is a byte array.
> 
Yes, I mistakenly converted of_find_property to of_property_read_string.
Anyway, I'm now using of_get_mac_address as you suggested.

-- 
Regards,
Shawn

Re: [PATCH] net/fec: add device tree probe support

[Shawn Guo]

On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > It adds device tree probe support for fec driver.
> > 
> > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> 
> Minor comments below.  After addressing them you can add my:
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> 
> I don't see any reason not to merge this one in v3.1
> 
> g.
> 
> > ---
> >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> >  2 files changed, 139 insertions(+), 5 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > new file mode 100644
> > index 0000000..1dad888
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > @@ -0,0 +1,24 @@
> > +* Freescale Fast Ethernet Controller (FEC)
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<soc>-fec"
> > +- reg : Address and length of the register set for the device
> > +- interrupts : Should contain fec interrupt
> > +- phy-mode : String, operation mode of the PHY interface.
> > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> 
> We don't have a common binding for this yet.  Should be
> "fsl,phy-mode".
> 
There is nothing really fsl specific.  How does the following patch
look to you?

---8<---------
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 86f334a..cc117db 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -8,6 +8,49 @@
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
 #include <linux/of_net.h>
+#include <linux/phy.h>
+
+/**
+ * It maps 'enum phy_interface_t' found in include/linux/phy.h
+ * into the device tree binding of 'phy-mode', so that Ethernet
+ * device driver can get phy interface from device tree.
+ */
+static const char *phy_modes[] = {
+       [PHY_INTERFACE_MODE_MII]        = "mii",
+       [PHY_INTERFACE_MODE_GMII]       = "gmii",
+       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
+       [PHY_INTERFACE_MODE_TBI]        = "tbi",
+       [PHY_INTERFACE_MODE_RMII]       = "rmii",
+       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
+       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
+       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
+       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
+       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
+};
+
+/**
+ * of_get_phy_mode - Get phy mode for given device_node
+ * @np:        Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy-mode',
+ * and return its index in phy_modes table, or errno in error case.
+ */
+const int of_get_phy_mode(struct device_node *np)
+{
+       const char *pm;
+       int err, i;
+
+       err = of_property_read_string(np, "phy-mode", &pm);
+       if (err < 0)
+               return err;
+
+       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
+               if (!strcasecmp(pm, phy_modes[i]))
+                       return i;
+
+       return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(of_get_phy_mode);

 /**
  * Search the device tree for the best MAC address to use.  'mac-address' is
------------

[...]
> > @@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> >  	 */
> >  	iap = macaddr;
> >  
> > +#ifdef CONFIG_OF
> > +	/*
> > +	 * 2) from device tree data
> > +	 */
> > +	if (!is_valid_ether_addr(iap)) {
> > +		struct device_node *np = fep->pdev->dev.of_node;
> > +		if (np) {
> > +			const char *mac;
> > +			int err;
> > +			err = of_property_read_string(np,
> > +					"local-mac-address", &mac);
> > +			if (err)
> > +				iap = (unsigned char *) mac;
> 
> There is already a function for doing this.  of_get_mac_address().
> And mac address is *not* a string, it is a byte array.
> 
Yes, I mistakenly converted of_find_property to of_property_read_string.
Anyway, I'm now using of_get_mac_address as you suggested.

-- 
Regards,
Shawn

[PATCH] net/fec: add device tree probe support

[Shawn Guo]

On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > It adds device tree probe support for fec driver.
> > 
> > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> 
> Minor comments below.  After addressing them you can add my:
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> 
> I don't see any reason not to merge this one in v3.1
> 
> g.
> 
> > ---
> >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> >  2 files changed, 139 insertions(+), 5 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > new file mode 100644
> > index 0000000..1dad888
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > @@ -0,0 +1,24 @@
> > +* Freescale Fast Ethernet Controller (FEC)
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<soc>-fec"
> > +- reg : Address and length of the register set for the device
> > +- interrupts : Should contain fec interrupt
> > +- phy-mode : String, operation mode of the PHY interface.
> > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> 
> We don't have a common binding for this yet.  Should be
> "fsl,phy-mode".
> 
There is nothing really fsl specific.  How does the following patch
look to you?

---8<---------
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 86f334a..cc117db 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -8,6 +8,49 @@
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
 #include <linux/of_net.h>
+#include <linux/phy.h>
+
+/**
+ * It maps 'enum phy_interface_t' found in include/linux/phy.h
+ * into the device tree binding of 'phy-mode', so that Ethernet
+ * device driver can get phy interface from device tree.
+ */
+static const char *phy_modes[] = {
+       [PHY_INTERFACE_MODE_MII]        = "mii",
+       [PHY_INTERFACE_MODE_GMII]       = "gmii",
+       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
+       [PHY_INTERFACE_MODE_TBI]        = "tbi",
+       [PHY_INTERFACE_MODE_RMII]       = "rmii",
+       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
+       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
+       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
+       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
+       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
+};
+
+/**
+ * of_get_phy_mode - Get phy mode for given device_node
+ * @np:        Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy-mode',
+ * and return its index in phy_modes table, or errno in error case.
+ */
+const int of_get_phy_mode(struct device_node *np)
+{
+       const char *pm;
+       int err, i;
+
+       err = of_property_read_string(np, "phy-mode", &pm);
+       if (err < 0)
+               return err;
+
+       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
+               if (!strcasecmp(pm, phy_modes[i]))
+                       return i;
+
+       return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(of_get_phy_mode);

 /**
  * Search the device tree for the best MAC address to use.  'mac-address' is
------------

[...]
> > @@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> >  	 */
> >  	iap = macaddr;
> >  
> > +#ifdef CONFIG_OF
> > +	/*
> > +	 * 2) from device tree data
> > +	 */
> > +	if (!is_valid_ether_addr(iap)) {
> > +		struct device_node *np = fep->pdev->dev.of_node;
> > +		if (np) {
> > +			const char *mac;
> > +			int err;
> > +			err = of_property_read_string(np,
> > +					"local-mac-address", &mac);
> > +			if (err)
> > +				iap = (unsigned char *) mac;
> 
> There is already a function for doing this.  of_get_mac_address().
> And mac address is *not* a string, it is a byte array.
> 
Yes, I mistakenly converted of_find_property to of_property_read_string.
Anyway, I'm now using of_get_mac_address as you suggested.

-- 
Regards,
Shawn

Re: [PATCH] net/fec: add device tree probe support

[Grant Likely]

On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote:
> On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > > It adds device tree probe support for fec driver.
> > > 
> > > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: David S. Miller <davem@davemloft.net>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > 
> > Minor comments below.  After addressing them you can add my:
> > 
> > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> > 
> > I don't see any reason not to merge this one in v3.1
> > 
> > g.
> > 
> > > ---
> > >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> > >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> > >  2 files changed, 139 insertions(+), 5 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > new file mode 100644
> > > index 0000000..1dad888
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > @@ -0,0 +1,24 @@
> > > +* Freescale Fast Ethernet Controller (FEC)
> > > +
> > > +Required properties:
> > > +- compatible : Should be "fsl,<soc>-fec"
> > > +- reg : Address and length of the register set for the device
> > > +- interrupts : Should contain fec interrupt
> > > +- phy-mode : String, operation mode of the PHY interface.
> > > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> > 
> > We don't have a common binding for this yet.  Should be
> > "fsl,phy-mode".
> > 
> There is nothing really fsl specific.

That's not really the point.  It might be that phy-mode as you're
defining it here will not be sufficient for other ethernet devices.
You should avoid creating 'generic' property definitions, unless
you're doing the leg work of figuring out which devices would use this
property and creating multiple users.

So, you may be right that this isn't fsl specific, but it is still
good practise to use that "fsl," prefix.


> How does thn following patch
> look to you?
> 
> ---8<---------
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index 86f334a..cc117db 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -8,6 +8,49 @@
>  #include <linux/etherdevice.h>
>  #include <linux/kernel.h>
>  #include <linux/of_net.h>
> +#include <linux/phy.h>
> +
> +/**
> + * It maps 'enum phy_interface_t' found in include/linux/phy.h
> + * into the device tree binding of 'phy-mode', so that Ethernet
> + * device driver can get phy interface from device tree.
> + */
> +static const char *phy_modes[] = {
> +       [PHY_INTERFACE_MODE_MII]        = "mii",
> +       [PHY_INTERFACE_MODE_GMII]       = "gmii",
> +       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
> +       [PHY_INTERFACE_MODE_TBI]        = "tbi",
> +       [PHY_INTERFACE_MODE_RMII]       = "rmii",
> +       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
> +       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
> +       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
> +       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> +       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
> +};
> +
> +/**
> + * of_get_phy_mode - Get phy mode for given device_node
> + * @np:        Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy-mode',
> + * and return its index in phy_modes table, or errno in error case.
> + */
> +const int of_get_phy_mode(struct device_node *np)
> +{
> +       const char *pm;
> +       int err, i;
> +
> +       err = of_property_read_string(np, "phy-mode", &pm);
> +       if (err < 0)
> +               return err;
> +
> +       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> +               if (!strcasecmp(pm, phy_modes[i]))
> +                       return i;
> +
> +       return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_get_phy_mode);

The code looks fine, but I'm just not keen on this until I see
multiple users.  What are the other DT-aware network drivers doing
right now?

> 
>  /**
>   * Search the device tree for the best MAC address to use.  'mac-address' is
> ------------
> 
> [...]
> > > @@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> > >  	 */
> > >  	iap = macaddr;
> > >  
> > > +#ifdef CONFIG_OF
> > > +	/*
> > > +	 * 2) from device tree data
> > > +	 */
> > > +	if (!is_valid_ether_addr(iap)) {
> > > +		struct device_node *np = fep->pdev->dev.of_node;
> > > +		if (np) {
> > > +			const char *mac;
> > > +			int err;
> > > +			err = of_property_read_string(np,
> > > +					"local-mac-address", &mac);
> > > +			if (err)
> > > +				iap = (unsigned char *) mac;
> > 
> > There is already a function for doing this.  of_get_mac_address().
> > And mac address is *not* a string, it is a byte array.
> > 
> Yes, I mistakenly converted of_find_property to of_property_read_string.
> Anyway, I'm now using of_get_mac_address as you suggested.

:)

g.

[PATCH] net/fec: add device tree probe support

[Grant Likely]

On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote:
> On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > > It adds device tree probe support for fec driver.
> > > 
> > > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: David S. Miller <davem@davemloft.net>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > 
> > Minor comments below.  After addressing them you can add my:
> > 
> > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> > 
> > I don't see any reason not to merge this one in v3.1
> > 
> > g.
> > 
> > > ---
> > >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> > >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> > >  2 files changed, 139 insertions(+), 5 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > new file mode 100644
> > > index 0000000..1dad888
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > @@ -0,0 +1,24 @@
> > > +* Freescale Fast Ethernet Controller (FEC)
> > > +
> > > +Required properties:
> > > +- compatible : Should be "fsl,<soc>-fec"
> > > +- reg : Address and length of the register set for the device
> > > +- interrupts : Should contain fec interrupt
> > > +- phy-mode : String, operation mode of the PHY interface.
> > > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> > 
> > We don't have a common binding for this yet.  Should be
> > "fsl,phy-mode".
> > 
> There is nothing really fsl specific.

That's not really the point.  It might be that phy-mode as you're
defining it here will not be sufficient for other ethernet devices.
You should avoid creating 'generic' property definitions, unless
you're doing the leg work of figuring out which devices would use this
property and creating multiple users.

So, you may be right that this isn't fsl specific, but it is still
good practise to use that "fsl," prefix.


> How does thn following patch
> look to you?
> 
> ---8<---------
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index 86f334a..cc117db 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -8,6 +8,49 @@
>  #include <linux/etherdevice.h>
>  #include <linux/kernel.h>
>  #include <linux/of_net.h>
> +#include <linux/phy.h>
> +
> +/**
> + * It maps 'enum phy_interface_t' found in include/linux/phy.h
> + * into the device tree binding of 'phy-mode', so that Ethernet
> + * device driver can get phy interface from device tree.
> + */
> +static const char *phy_modes[] = {
> +       [PHY_INTERFACE_MODE_MII]        = "mii",
> +       [PHY_INTERFACE_MODE_GMII]       = "gmii",
> +       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
> +       [PHY_INTERFACE_MODE_TBI]        = "tbi",
> +       [PHY_INTERFACE_MODE_RMII]       = "rmii",
> +       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
> +       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
> +       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
> +       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> +       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
> +};
> +
> +/**
> + * of_get_phy_mode - Get phy mode for given device_node
> + * @np:        Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy-mode',
> + * and return its index in phy_modes table, or errno in error case.
> + */
> +const int of_get_phy_mode(struct device_node *np)
> +{
> +       const char *pm;
> +       int err, i;
> +
> +       err = of_property_read_string(np, "phy-mode", &pm);
> +       if (err < 0)
> +               return err;
> +
> +       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> +               if (!strcasecmp(pm, phy_modes[i]))
> +                       return i;
> +
> +       return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_get_phy_mode);

The code looks fine, but I'm just not keen on this until I see
multiple users.  What are the other DT-aware network drivers doing
right now?

> 
>  /**
>   * Search the device tree for the best MAC address to use.  'mac-address' is
> ------------
> 
> [...]
> > > @@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> > >  	 */
> > >  	iap = macaddr;
> > >  
> > > +#ifdef CONFIG_OF
> > > +	/*
> > > +	 * 2) from device tree data
> > > +	 */
> > > +	if (!is_valid_ether_addr(iap)) {
> > > +		struct device_node *np = fep->pdev->dev.of_node;
> > > +		if (np) {
> > > +			const char *mac;
> > > +			int err;
> > > +			err = of_property_read_string(np,
> > > +					"local-mac-address", &mac);
> > > +			if (err)
> > > +				iap = (unsigned char *) mac;
> > 
> > There is already a function for doing this.  of_get_mac_address().
> > And mac address is *not* a string, it is a byte array.
> > 
> Yes, I mistakenly converted of_find_property to of_property_read_string.
> Anyway, I'm now using of_get_mac_address as you suggested.

:)

g.

Re: [PATCH] net/fec: add device tree probe support

[Shawn Guo]

On Sun, Jul 03, 2011 at 11:59:56PM -0600, Grant Likely wrote:
> On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote:
> > On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> > > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > > > It adds device tree probe support for fec driver.
> > > > 
> > > > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: David S. Miller <davem@davemloft.net>
> > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > 
> > > Minor comments below.  After addressing them you can add my:
> > > 
> > > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> > > 
> > > I don't see any reason not to merge this one in v3.1
> > > 
> > > g.
> > > 
> > > > ---
> > > >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> > > >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> > > >  2 files changed, 139 insertions(+), 5 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > new file mode 100644
> > > > index 0000000..1dad888
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > @@ -0,0 +1,24 @@
> > > > +* Freescale Fast Ethernet Controller (FEC)
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should be "fsl,<soc>-fec"
> > > > +- reg : Address and length of the register set for the device
> > > > +- interrupts : Should contain fec interrupt
> > > > +- phy-mode : String, operation mode of the PHY interface.
> > > > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > > > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> > > 
> > > We don't have a common binding for this yet.  Should be
> > > "fsl,phy-mode".
> > > 
> > There is nothing really fsl specific.
> 
> That's not really the point.  It might be that phy-mode as you're
> defining it here will not be sufficient for other ethernet devices.

I'm not defining the phy mode.  Instead, it's just a direct mapping
of 'enum phy_interface_t' found in include/linux/phy.h, which is
something common and generic, I think?

> You should avoid creating 'generic' property definitions, unless
> you're doing the leg work of figuring out which devices would use this
> property and creating multiple users.
> 

Any net device driver currently referring to 'enum phy_interface_t'
is going to need this property when migrating to dt, I guess.

> So, you may be right that this isn't fsl specific, but it is still
> good practise to use that "fsl," prefix.
> 
> 
> > How does thn following patch
> > look to you?
> > 
> > ---8<---------
> > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > index 86f334a..cc117db 100644
> > --- a/drivers/of/of_net.c
> > +++ b/drivers/of/of_net.c
> > @@ -8,6 +8,49 @@
> >  #include <linux/etherdevice.h>
> >  #include <linux/kernel.h>
> >  #include <linux/of_net.h>
> > +#include <linux/phy.h>
> > +
> > +/**
> > + * It maps 'enum phy_interface_t' found in include/linux/phy.h
> > + * into the device tree binding of 'phy-mode', so that Ethernet
> > + * device driver can get phy interface from device tree.
> > + */
> > +static const char *phy_modes[] = {
> > +       [PHY_INTERFACE_MODE_MII]        = "mii",
> > +       [PHY_INTERFACE_MODE_GMII]       = "gmii",
> > +       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
> > +       [PHY_INTERFACE_MODE_TBI]        = "tbi",
> > +       [PHY_INTERFACE_MODE_RMII]       = "rmii",
> > +       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
> > +       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
> > +       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
> > +       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> > +       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
> > +};
> > +
> > +/**
> > + * of_get_phy_mode - Get phy mode for given device_node
> > + * @np:        Pointer to the given device_node
> > + *
> > + * The function gets phy interface string from property 'phy-mode',
> > + * and return its index in phy_modes table, or errno in error case.
> > + */
> > +const int of_get_phy_mode(struct device_node *np)
> > +{
> > +       const char *pm;
> > +       int err, i;
> > +
> > +       err = of_property_read_string(np, "phy-mode", &pm);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> > +               if (!strcasecmp(pm, phy_modes[i]))
> > +                       return i;
> > +
> > +       return -ENODEV;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_phy_mode);
> 
> The code looks fine, but I'm just not keen on this until I see
> multiple users.  What are the other DT-aware network drivers doing
> right now?
> 
You can find a bunch of examples using 'phy-mode' in this way with
the grep below.

$ grep phy-mode arch/powerpc/boot/dts/*

The code I can find is drivers/net/ibm_newemac/core.c, function
emac_init_config.  And I think it should use 'enum phy_interface_t'
than defining its own PHY_MODE_*.

-- 
Regards,
Shawn

[PATCH] net/fec: add device tree probe support

[Shawn Guo]

On Sun, Jul 03, 2011 at 11:59:56PM -0600, Grant Likely wrote:
> On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote:
> > On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> > > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > > > It adds device tree probe support for fec driver.
> > > > 
> > > > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: David S. Miller <davem@davemloft.net>
> > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > 
> > > Minor comments below.  After addressing them you can add my:
> > > 
> > > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> > > 
> > > I don't see any reason not to merge this one in v3.1
> > > 
> > > g.
> > > 
> > > > ---
> > > >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> > > >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> > > >  2 files changed, 139 insertions(+), 5 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > new file mode 100644
> > > > index 0000000..1dad888
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > @@ -0,0 +1,24 @@
> > > > +* Freescale Fast Ethernet Controller (FEC)
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should be "fsl,<soc>-fec"
> > > > +- reg : Address and length of the register set for the device
> > > > +- interrupts : Should contain fec interrupt
> > > > +- phy-mode : String, operation mode of the PHY interface.
> > > > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > > > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> > > 
> > > We don't have a common binding for this yet.  Should be
> > > "fsl,phy-mode".
> > > 
> > There is nothing really fsl specific.
> 
> That's not really the point.  It might be that phy-mode as you're
> defining it here will not be sufficient for other ethernet devices.

I'm not defining the phy mode.  Instead, it's just a direct mapping
of 'enum phy_interface_t' found in include/linux/phy.h, which is
something common and generic, I think?

> You should avoid creating 'generic' property definitions, unless
> you're doing the leg work of figuring out which devices would use this
> property and creating multiple users.
> 

Any net device driver currently referring to 'enum phy_interface_t'
is going to need this property when migrating to dt, I guess.

> So, you may be right that this isn't fsl specific, but it is still
> good practise to use that "fsl," prefix.
> 
> 
> > How does thn following patch
> > look to you?
> > 
> > ---8<---------
> > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > index 86f334a..cc117db 100644
> > --- a/drivers/of/of_net.c
> > +++ b/drivers/of/of_net.c
> > @@ -8,6 +8,49 @@
> >  #include <linux/etherdevice.h>
> >  #include <linux/kernel.h>
> >  #include <linux/of_net.h>
> > +#include <linux/phy.h>
> > +
> > +/**
> > + * It maps 'enum phy_interface_t' found in include/linux/phy.h
> > + * into the device tree binding of 'phy-mode', so that Ethernet
> > + * device driver can get phy interface from device tree.
> > + */
> > +static const char *phy_modes[] = {
> > +       [PHY_INTERFACE_MODE_MII]        = "mii",
> > +       [PHY_INTERFACE_MODE_GMII]       = "gmii",
> > +       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
> > +       [PHY_INTERFACE_MODE_TBI]        = "tbi",
> > +       [PHY_INTERFACE_MODE_RMII]       = "rmii",
> > +       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
> > +       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
> > +       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
> > +       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> > +       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
> > +};
> > +
> > +/**
> > + * of_get_phy_mode - Get phy mode for given device_node
> > + * @np:        Pointer to the given device_node
> > + *
> > + * The function gets phy interface string from property 'phy-mode',
> > + * and return its index in phy_modes table, or errno in error case.
> > + */
> > +const int of_get_phy_mode(struct device_node *np)
> > +{
> > +       const char *pm;
> > +       int err, i;
> > +
> > +       err = of_property_read_string(np, "phy-mode", &pm);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> > +               if (!strcasecmp(pm, phy_modes[i]))
> > +                       return i;
> > +
> > +       return -ENODEV;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_phy_mode);
> 
> The code looks fine, but I'm just not keen on this until I see
> multiple users.  What are the other DT-aware network drivers doing
> right now?
> 
You can find a bunch of examples using 'phy-mode' in this way with
the grep below.

$ grep phy-mode arch/powerpc/boot/dts/*

The code I can find is drivers/net/ibm_newemac/core.c, function
emac_init_config.  And I think it should use 'enum phy_interface_t'
than defining its own PHY_MODE_*.

-- 
Regards,
Shawn

Re: [PATCH] net/fec: add device tree probe support

[Grant Likely]

On Mon, Jul 04, 2011 at 02:25:22PM +0800, Shawn Guo wrote:
> On Sun, Jul 03, 2011 at 11:59:56PM -0600, Grant Likely wrote:
> > On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote:
> > > On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> > > > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > > > > It adds device tree probe support for fec driver.
> > > > > 
> > > > > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > 
> > > > Minor comments below.  After addressing them you can add my:
> > > > 
> > > > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> > > > 
> > > > I don't see any reason not to merge this one in v3.1
> > > > 
> > > > g.
> > > > 
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> > > > >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> > > > >  2 files changed, 139 insertions(+), 5 deletions(-)
> > > > >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > new file mode 100644
> > > > > index 0000000..1dad888
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > @@ -0,0 +1,24 @@
> > > > > +* Freescale Fast Ethernet Controller (FEC)
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : Should be "fsl,<soc>-fec"
> > > > > +- reg : Address and length of the register set for the device
> > > > > +- interrupts : Should contain fec interrupt
> > > > > +- phy-mode : String, operation mode of the PHY interface.
> > > > > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > > > > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> > > > 
> > > > We don't have a common binding for this yet.  Should be
> > > > "fsl,phy-mode".
> > > > 
> > > There is nothing really fsl specific.
> > 
> > That's not really the point.  It might be that phy-mode as you're
> > defining it here will not be sufficient for other ethernet devices.
> 
> I'm not defining the phy mode.  Instead, it's just a direct mapping
> of 'enum phy_interface_t' found in include/linux/phy.h, which is
> something common and generic, I think?
> 
> > You should avoid creating 'generic' property definitions, unless
> > you're doing the leg work of figuring out which devices would use this
> > property and creating multiple users.
> > 
> 
> Any net device driver currently referring to 'enum phy_interface_t'
> is going to need this property when migrating to dt, I guess.
> 
> > So, you may be right that this isn't fsl specific, but it is still
> > good practise to use that "fsl," prefix.
> > 
> > 
> > > How does thn following patch
> > > look to you?
> > > 
> > > ---8<---------
> > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > > index 86f334a..cc117db 100644
> > > --- a/drivers/of/of_net.c
> > > +++ b/drivers/of/of_net.c
> > > @@ -8,6 +8,49 @@
> > >  #include <linux/etherdevice.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/of_net.h>
> > > +#include <linux/phy.h>
> > > +
> > > +/**
> > > + * It maps 'enum phy_interface_t' found in include/linux/phy.h
> > > + * into the device tree binding of 'phy-mode', so that Ethernet
> > > + * device driver can get phy interface from device tree.
> > > + */
> > > +static const char *phy_modes[] = {
> > > +       [PHY_INTERFACE_MODE_MII]        = "mii",
> > > +       [PHY_INTERFACE_MODE_GMII]       = "gmii",
> > > +       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
> > > +       [PHY_INTERFACE_MODE_TBI]        = "tbi",
> > > +       [PHY_INTERFACE_MODE_RMII]       = "rmii",
> > > +       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
> > > +       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
> > > +       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
> > > +       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> > > +       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
> > > +};
> > > +
> > > +/**
> > > + * of_get_phy_mode - Get phy mode for given device_node
> > > + * @np:        Pointer to the given device_node
> > > + *
> > > + * The function gets phy interface string from property 'phy-mode',
> > > + * and return its index in phy_modes table, or errno in error case.
> > > + */
> > > +const int of_get_phy_mode(struct device_node *np)
> > > +{
> > > +       const char *pm;
> > > +       int err, i;
> > > +
> > > +       err = of_property_read_string(np, "phy-mode", &pm);
> > > +       if (err < 0)
> > > +               return err;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> > > +               if (!strcasecmp(pm, phy_modes[i]))
> > > +                       return i;
> > > +
> > > +       return -ENODEV;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_phy_mode);
> > 
> > The code looks fine, but I'm just not keen on this until I see
> > multiple users.  What are the other DT-aware network drivers doing
> > right now?
> > 
> You can find a bunch of examples using 'phy-mode' in this way with
> the grep below.
> 
> $ grep phy-mode arch/powerpc/boot/dts/*
> 
> The code I can find is drivers/net/ibm_newemac/core.c, function
> emac_init_config.  And I think it should use 'enum phy_interface_t'
> than defining its own PHY_MODE_*.

Fair enough.  Go ahead and create the common definition.  Can you
craft a patch to convert the ibm_newemac driver to use your common
function too?

g.

[PATCH] net/fec: add device tree probe support

[Grant Likely]

On Mon, Jul 04, 2011 at 02:25:22PM +0800, Shawn Guo wrote:
> On Sun, Jul 03, 2011 at 11:59:56PM -0600, Grant Likely wrote:
> > On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote:
> > > On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> > > > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > > > > It adds device tree probe support for fec driver.
> > > > > 
> > > > > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > 
> > > > Minor comments below.  After addressing them you can add my:
> > > > 
> > > > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> > > > 
> > > > I don't see any reason not to merge this one in v3.1
> > > > 
> > > > g.
> > > > 
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> > > > >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> > > > >  2 files changed, 139 insertions(+), 5 deletions(-)
> > > > >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > new file mode 100644
> > > > > index 0000000..1dad888
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > @@ -0,0 +1,24 @@
> > > > > +* Freescale Fast Ethernet Controller (FEC)
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : Should be "fsl,<soc>-fec"
> > > > > +- reg : Address and length of the register set for the device
> > > > > +- interrupts : Should contain fec interrupt
> > > > > +- phy-mode : String, operation mode of the PHY interface.
> > > > > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > > > > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> > > > 
> > > > We don't have a common binding for this yet.  Should be
> > > > "fsl,phy-mode".
> > > > 
> > > There is nothing really fsl specific.
> > 
> > That's not really the point.  It might be that phy-mode as you're
> > defining it here will not be sufficient for other ethernet devices.
> 
> I'm not defining the phy mode.  Instead, it's just a direct mapping
> of 'enum phy_interface_t' found in include/linux/phy.h, which is
> something common and generic, I think?
> 
> > You should avoid creating 'generic' property definitions, unless
> > you're doing the leg work of figuring out which devices would use this
> > property and creating multiple users.
> > 
> 
> Any net device driver currently referring to 'enum phy_interface_t'
> is going to need this property when migrating to dt, I guess.
> 
> > So, you may be right that this isn't fsl specific, but it is still
> > good practise to use that "fsl," prefix.
> > 
> > 
> > > How does thn following patch
> > > look to you?
> > > 
> > > ---8<---------
> > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > > index 86f334a..cc117db 100644
> > > --- a/drivers/of/of_net.c
> > > +++ b/drivers/of/of_net.c
> > > @@ -8,6 +8,49 @@
> > >  #include <linux/etherdevice.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/of_net.h>
> > > +#include <linux/phy.h>
> > > +
> > > +/**
> > > + * It maps 'enum phy_interface_t' found in include/linux/phy.h
> > > + * into the device tree binding of 'phy-mode', so that Ethernet
> > > + * device driver can get phy interface from device tree.
> > > + */
> > > +static const char *phy_modes[] = {
> > > +       [PHY_INTERFACE_MODE_MII]        = "mii",
> > > +       [PHY_INTERFACE_MODE_GMII]       = "gmii",
> > > +       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
> > > +       [PHY_INTERFACE_MODE_TBI]        = "tbi",
> > > +       [PHY_INTERFACE_MODE_RMII]       = "rmii",
> > > +       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
> > > +       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
> > > +       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
> > > +       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> > > +       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
> > > +};
> > > +
> > > +/**
> > > + * of_get_phy_mode - Get phy mode for given device_node
> > > + * @np:        Pointer to the given device_node
> > > + *
> > > + * The function gets phy interface string from property 'phy-mode',
> > > + * and return its index in phy_modes table, or errno in error case.
> > > + */
> > > +const int of_get_phy_mode(struct device_node *np)
> > > +{
> > > +       const char *pm;
> > > +       int err, i;
> > > +
> > > +       err = of_property_read_string(np, "phy-mode", &pm);
> > > +       if (err < 0)
> > > +               return err;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> > > +               if (!strcasecmp(pm, phy_modes[i]))
> > > +                       return i;
> > > +
> > > +       return -ENODEV;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_phy_mode);
> > 
> > The code looks fine, but I'm just not keen on this until I see
> > multiple users.  What are the other DT-aware network drivers doing
> > right now?
> > 
> You can find a bunch of examples using 'phy-mode' in this way with
> the grep below.
> 
> $ grep phy-mode arch/powerpc/boot/dts/*
> 
> The code I can find is drivers/net/ibm_newemac/core.c, function
> emac_init_config.  And I think it should use 'enum phy_interface_t'
> than defining its own PHY_MODE_*.

Fair enough.  Go ahead and create the common definition.  Can you
craft a patch to convert the ibm_newemac driver to use your common
function too?

g.