[PATCH 1/1] net/macb: add DT support

[PATCH 1/1] net/macb: add DT support

[Jean-Christophe PLAGNIOL-VILLARD]

allow the DT to pass the mac address and the phy mode

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/net/macb.txt |   22 ++++++++
 drivers/net/ethernet/cadence/macb.c            |   65 +++++++++++++++++++++---
 drivers/net/ethernet/cadence/macb.h            |    2 +
 3 files changed, 81 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/macb.txt

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
new file mode 100644
index 0000000..2b727ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -0,0 +1,22 @@
+* Cadence EMACB
+
+Implemeted on Atmel AT91 & AVR32 SoC
+
+Required properties:
+- compatible : Should be "atmel,macb" for Atmel
+- reg : Address and length of the register set for the device
+- interrupts : Should contain macb interrupt
+- phy-mode : String, operation mode of the PHY interface.
+  Supported values are: "mii", "rmii",
+
+Optional properties:
+- local-mac-address : 6 bytes, mac address
+
+Examples:
+
+	macb0: macb@fffc4000 {
+		compatible = "atmel,macb";
+		reg = <oxfffc4000 0x4000>;
+		interrupts = <21>;
+		phy-mode = "mii";
+	};
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index a437b46..2c345bc 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -20,6 +20,9 @@
 #include <linux/etherdevice.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
 #include <linux/phy.h>
 
 #include <mach/board.h>
@@ -81,6 +84,20 @@ static void __init macb_get_hwaddr(struct macb *bp)
 	addr[4] = top & 0xff;
 	addr[5] = (top >> 8) & 0xff;
 
+#ifdef CONFIG_OF
+	/*
+	 * 2) from device tree data
+	 */
+	if (!is_valid_ether_addr(addr)) {
+		struct device_node *np = bp->pdev->dev.of_node;
+		if (np) {
+			const char *mac = of_get_mac_address(np);
+			if (mac)
+				memcpy(addr, mac, sizeof(addr));
+		}
+	}
+#endif
+
 	if (is_valid_ether_addr(addr)) {
 		memcpy(bp->dev->dev_addr, addr, sizeof(addr));
 	} else {
@@ -191,7 +208,6 @@ static int macb_mii_probe(struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
 	struct phy_device *phydev;
-	struct eth_platform_data *pdata;
 	int ret;
 
 	phydev = phy_find_first(bp->mii_bus);
@@ -200,14 +216,11 @@ static int macb_mii_probe(struct net_device *dev)
 		return -1;
 	}
 
-	pdata = bp->pdev->dev.platform_data;
 	/* TODO : add pin_irq */
 
 	/* attach the mac to the phy */
 	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, 0,
-				 pdata && pdata->is_rmii ?
-				 PHY_INTERFACE_MODE_RMII :
-				 PHY_INTERFACE_MODE_MII);
+				 bp->phy_interface);
 	if (ret) {
 		printk(KERN_ERR "%s: Could not attach to PHY\n", dev->name);
 		return ret;
@@ -1117,6 +1130,30 @@ static const struct net_device_ops macb_netdev_ops = {
 #endif
 };
 
+#if defined(CONFIG_OF)
+static const struct of_device_id macb_dt_ids[] = {
+	{ .compatible = "atmel,macb" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, macb_dt_ids);
+
+static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	if (np)
+		return of_get_phy_mode(np);
+
+	return -ENODEV;
+}
+#else
+static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+#endif
+
 static int __init macb_probe(struct platform_device *pdev)
 {
 	struct eth_platform_data *pdata;
@@ -1210,20 +1247,31 @@ static int __init macb_probe(struct platform_device *pdev)
 	macb_writel(bp, NCFGR, config);
 
 	macb_get_hwaddr(bp);
-	pdata = pdev->dev.platform_data;
 
-	if (pdata && pdata->is_rmii)
+	err = macb_get_phy_mode_dt(pdev);
+	if (err < 0) {
+		pdata = pdev->dev.platform_data;
+		if (pdata && pdata->is_rmii)
+			bp->phy_interface = PHY_INTERFACE_MODE_RMII;
+		else
+			bp->phy_interface = PHY_INTERFACE_MODE_MII;
+	} else {
+		bp->phy_interface = err;
+	}
+
+	if (bp->phy_interface == PHY_INTERFACE_MODE_RMII) {
 #if defined(CONFIG_ARCH_AT91)
 		macb_writel(bp, USRIO, (MACB_BIT(RMII) | MACB_BIT(CLKEN)) );
 #else
 		macb_writel(bp, USRIO, 0);
 #endif
-	else
+	} else {
 #if defined(CONFIG_ARCH_AT91)
 		macb_writel(bp, USRIO, MACB_BIT(CLKEN));
 #else
 		macb_writel(bp, USRIO, MACB_BIT(MII));
 #endif
+	}
 
 	bp->tx_pending = DEF_TX_RING_PENDING;
 
@@ -1344,6 +1392,7 @@ static struct platform_driver macb_driver = {
 	.driver		= {
 		.name		= "macb",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(macb_dt_ids),
 	},
 };
 
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index d3212f6..8342744 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -389,6 +389,8 @@ struct macb {
 	unsigned int 		link;
 	unsigned int 		speed;
 	unsigned int 		duplex;
+
+	phy_interface_t		phy_interface;
 };
 
 #endif /* _MACB_H */
-- 
1.7.7

Re: [PATCH 1/1] net/macb: add DT support

[Jamie Iles]

Hi Jean-Christophe,

On Fri, Nov 18, 2011 at 03:29:25PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> allow the DT to pass the mac address and the phy mode
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Jamie Iles <jamie@jamieiles.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>

This looks OK to me in principle.  I can't easily test this at the 
moment, but as I don't have a DT platform that has the clk framework up 
and running.  A couple of nits/questions inline, but thanks for doing 
this!

Jamie

> ---
>  Documentation/devicetree/bindings/net/macb.txt |   22 ++++++++
>  drivers/net/ethernet/cadence/macb.c            |   65 +++++++++++++++++++++---
>  drivers/net/ethernet/cadence/macb.h            |    2 +
>  3 files changed, 81 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/macb.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
> new file mode 100644
> index 0000000..2b727ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/macb.txt
> @@ -0,0 +1,22 @@
> +* Cadence EMACB
> +
> +Implemeted on Atmel AT91 & AVR32 SoC

I think something along the lines of "Binding for the Cadence MACB 
Ethernet controller" rather than listing specific parts might be 
clearer.

> +
> +Required properties:
> +- compatible : Should be "atmel,macb" for Atmel
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain macb interrupt
> +- phy-mode : String, operation mode of the PHY interface.
> +  Supported values are: "mii", "rmii",
> +
> +Optional properties:
> +- local-mac-address : 6 bytes, mac address
> +
> +Examples:
> +
> +	macb0: macb@fffc4000 {

Rob pointed out to me a little while ago that the preferred naming from 
the ePAPR document would be:

	macb0: ethernet@fffc4000

so it might be worth being consistent here.

> +		compatible = "atmel,macb";

This should be "cdns,macb" as it isn't Atmel specific.  I believe cdns 
is the correct stock ticker symbol for Cadence.

> +		reg = <oxfffc4000 0x4000>;
> +		interrupts = <21>;
> +		phy-mode = "mii";
> +	};
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index a437b46..2c345bc 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -20,6 +20,9 @@
>  #include <linux/etherdevice.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_net.h>
>  #include <linux/phy.h>
>  
>  #include <mach/board.h>
> @@ -81,6 +84,20 @@ static void __init macb_get_hwaddr(struct macb *bp)
>  	addr[4] = top & 0xff;
>  	addr[5] = (top >> 8) & 0xff;
>  
> +#ifdef CONFIG_OF
> +	/*
> +	 * 2) from device tree data
> +	 */
> +	if (!is_valid_ether_addr(addr)) {
> +		struct device_node *np = bp->pdev->dev.of_node;
> +		if (np) {
> +			const char *mac = of_get_mac_address(np);
> +			if (mac)
> +				memcpy(addr, mac, sizeof(addr));
> +		}
> +	}
> +#endif

I'm a bit conflicted here.  I think we should always use the MAC address 
from the device tree if it is present even if the current MAC address is 
valid.

> +
>  	if (is_valid_ether_addr(addr)) {
>  		memcpy(bp->dev->dev_addr, addr, sizeof(addr));
>  	} else {
> @@ -191,7 +208,6 @@ static int macb_mii_probe(struct net_device *dev)
>  {
>  	struct macb *bp = netdev_priv(dev);
>  	struct phy_device *phydev;
> -	struct eth_platform_data *pdata;
>  	int ret;
>  
>  	phydev = phy_find_first(bp->mii_bus);
> @@ -200,14 +216,11 @@ static int macb_mii_probe(struct net_device *dev)
>  		return -1;
>  	}
>  
> -	pdata = bp->pdev->dev.platform_data;
>  	/* TODO : add pin_irq */
>  
>  	/* attach the mac to the phy */
>  	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, 0,
> -				 pdata && pdata->is_rmii ?
> -				 PHY_INTERFACE_MODE_RMII :
> -				 PHY_INTERFACE_MODE_MII);
> +				 bp->phy_interface);
>  	if (ret) {
>  		printk(KERN_ERR "%s: Could not attach to PHY\n", dev->name);
>  		return ret;
> @@ -1117,6 +1130,30 @@ static const struct net_device_ops macb_netdev_ops = {
>  #endif
>  };
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id macb_dt_ids[] = {
> +	{ .compatible = "atmel,macb" },

cdns,macb again.

> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, macb_dt_ids);
> +
> +static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	if (np)
> +		return of_get_phy_mode(np);
> +
> +	return -ENODEV;
> +}
> +#else
> +static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  static int __init macb_probe(struct platform_device *pdev)
>  {
>  	struct eth_platform_data *pdata;
> @@ -1210,20 +1247,31 @@ static int __init macb_probe(struct platform_device *pdev)
>  	macb_writel(bp, NCFGR, config);
>  
>  	macb_get_hwaddr(bp);
> -	pdata = pdev->dev.platform_data;
>  
> -	if (pdata && pdata->is_rmii)
> +	err = macb_get_phy_mode_dt(pdev);
> +	if (err < 0) {
> +		pdata = pdev->dev.platform_data;
> +		if (pdata && pdata->is_rmii)
> +			bp->phy_interface = PHY_INTERFACE_MODE_RMII;
> +		else
> +			bp->phy_interface = PHY_INTERFACE_MODE_MII;
> +	} else {
> +		bp->phy_interface = err;
> +	}
> +
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_RMII) {
>  #if defined(CONFIG_ARCH_AT91)
>  		macb_writel(bp, USRIO, (MACB_BIT(RMII) | MACB_BIT(CLKEN)) );
>  #else
>  		macb_writel(bp, USRIO, 0);
>  #endif
> -	else
> +	} else {
>  #if defined(CONFIG_ARCH_AT91)
>  		macb_writel(bp, USRIO, MACB_BIT(CLKEN));
>  #else
>  		macb_writel(bp, USRIO, MACB_BIT(MII));
>  #endif
> +	}
>  
>  	bp->tx_pending = DEF_TX_RING_PENDING;
>  
> @@ -1344,6 +1392,7 @@ static struct platform_driver macb_driver = {
>  	.driver		= {
>  		.name		= "macb",
>  		.owner	= THIS_MODULE,
> +		.of_match_table	= of_match_ptr(macb_dt_ids),
>  	},
>  };
>  
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index d3212f6..8342744 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -389,6 +389,8 @@ struct macb {
>  	unsigned int 		link;
>  	unsigned int 		speed;
>  	unsigned int 		duplex;
> +
> +	phy_interface_t		phy_interface;
>  };
>  
>  #endif /* _MACB_H */
> -- 
> 1.7.7
> 

Re: [PATCH 1/1] net/macb: add DT support

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:58 Fri 18 Nov     , Jamie Iles wrote:
> Hi Jean-Christophe,
> 
> On Fri, Nov 18, 2011 at 03:29:25PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > allow the DT to pass the mac address and the phy mode
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: Jamie Iles <jamie@jamieiles.com>
> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> This looks OK to me in principle.  I can't easily test this at the 
> moment, but as I don't have a DT platform that has the clk framework up 
> and running.  A couple of nits/questions inline, but thanks for doing 
> this!
> 
> Jamie
> 
> > ---
> >  Documentation/devicetree/bindings/net/macb.txt |   22 ++++++++
> >  drivers/net/ethernet/cadence/macb.c            |   65 +++++++++++++++++++++---
> >  drivers/net/ethernet/cadence/macb.h            |    2 +
> >  3 files changed, 81 insertions(+), 8 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/macb.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
> > new file mode 100644
> > index 0000000..2b727ec
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/macb.txt
> > @@ -0,0 +1,22 @@
> > +* Cadence EMACB
> > +
> > +Implemeted on Atmel AT91 & AVR32 SoC
> 
> I think something along the lines of "Binding for the Cadence MACB 
> Ethernet controller" rather than listing specific parts might be 
> clearer.
I prefer as we will have implementation detail in the binding
> 
> > +
> > +Required properties:
> > +- compatible : Should be "atmel,macb" for Atmel
> > +- reg : Address and length of the register set for the device
> > +- interrupts : Should contain macb interrupt
> > +- phy-mode : String, operation mode of the PHY interface.
> > +  Supported values are: "mii", "rmii",
> > +
> > +Optional properties:
> > +- local-mac-address : 6 bytes, mac address
> > +
> > +Examples:
> > +
> > +	macb0: macb@fffc4000 {
> 
> Rob pointed out to me a little while ago that the preferred naming from 
> the ePAPR document would be:
> 
> 	macb0: ethernet@fffc4000
> 
> so it might be worth being consistent here.
ok
> 
> > +		compatible = "atmel,macb";
> 
> This should be "cdns,macb" as it isn't Atmel specific.  I believe cdns 
> is the correct stock ticker symbol for Cadence.
here I put "atmel,macb" on purpose to specify the difference of the IP between
the soc, in fact it should have been atmel-at91,macb

> 
> > +		reg = <oxfffc4000 0x4000>;
> > +		interrupts = <21>;
> > +		phy-mode = "mii";
> > +	};
> > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> > index a437b46..2c345bc 100644
> > --- a/drivers/net/ethernet/cadence/macb.c
> > +++ b/drivers/net/ethernet/cadence/macb.c
> > @@ -20,6 +20,9 @@
> >  #include <linux/etherdevice.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_net.h>
> >  #include <linux/phy.h>
> >  
> >  #include <mach/board.h>
> > @@ -81,6 +84,20 @@ static void __init macb_get_hwaddr(struct macb *bp)
> >  	addr[4] = top & 0xff;
> >  	addr[5] = (top >> 8) & 0xff;
> >  
> > +#ifdef CONFIG_OF
> > +	/*
> > +	 * 2) from device tree data
> > +	 */
> > +	if (!is_valid_ether_addr(addr)) {
> > +		struct device_node *np = bp->pdev->dev.of_node;
> > +		if (np) {
> > +			const char *mac = of_get_mac_address(np);
> > +			if (mac)
> > +				memcpy(addr, mac, sizeof(addr));
> > +		}
> > +	}
> > +#endif
> 
> I'm a bit conflicted here.  I think we should always use the MAC address 
> from the device tree if it is present even if the current MAC address is 
> valid.
if the mac is already programmed in the register we just keep it
I prefer this way if the bootloader set it we keep it

Best Regards,
J.

Re: [PATCH 1/1] net/macb: add DT support

[Jamie Iles]

On Sun, Nov 20, 2011 at 05:47:40PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 15:58 Fri 18 Nov     , Jamie Iles wrote:
> > Hi Jean-Christophe,
> > 
> > On Fri, Nov 18, 2011 at 03:29:25PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > allow the DT to pass the mac address and the phy mode
> > > 
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > Cc: Jamie Iles <jamie@jamieiles.com>
> > > Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> > 
> > This looks OK to me in principle.  I can't easily test this at the 
> > moment, but as I don't have a DT platform that has the clk framework up 
> > and running.  A couple of nits/questions inline, but thanks for doing 
> > this!
> > 
> > Jamie
> > 
> > > ---
> > >  Documentation/devicetree/bindings/net/macb.txt |   22 ++++++++
> > >  drivers/net/ethernet/cadence/macb.c            |   65 +++++++++++++++++++++---
> > >  drivers/net/ethernet/cadence/macb.h            |    2 +
> > >  3 files changed, 81 insertions(+), 8 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/net/macb.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
> > > new file mode 100644
> > > index 0000000..2b727ec
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/macb.txt
> > > @@ -0,0 +1,22 @@
> > > +* Cadence EMACB
> > > +
> > > +Implemeted on Atmel AT91 & AVR32 SoC
> > 
> > I think something along the lines of "Binding for the Cadence MACB 
> > Ethernet controller" rather than listing specific parts might be 
> > clearer.
> I prefer as we will have implementation detail in the binding

I can't see any Atmel specific implementation detail here though so lets 
keep it generic for now.  There isn't a benefit to keeping a list of 
SoC's that the device is implemented in here as it'll only become out of 
date.  We need to make it easy for other vendors to reuse the binding + 
driver.

> > > +		compatible = "atmel,macb";
> > 
> > This should be "cdns,macb" as it isn't Atmel specific.  I believe cdns 
> > is the correct stock ticker symbol for Cadence.
> here I put "atmel,macb" on purpose to specify the difference of the IP between
> the soc, in fact it should have been atmel-at91,macb

Well if we really can't detect the difference from the revision register 
then we should have "cdns,macb" *and* "atmel,at91-macb" at least then 
where platforms could claim compatibility as:

	compatible = "atmel,at91-macb", "cdns,macb";

If we consider that another vendor integrates the Cadence IP, then it 
makes much more sense to claim compatibility with a Cadence string 
rather than an Atmel one...

> > 
> > > +		reg = <oxfffc4000 0x4000>;
> > > +		interrupts = <21>;
> > > +		phy-mode = "mii";
> > > +	};
> > > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> > > index a437b46..2c345bc 100644
> > > --- a/drivers/net/ethernet/cadence/macb.c
> > > +++ b/drivers/net/ethernet/cadence/macb.c
> > > @@ -20,6 +20,9 @@
> > >  #include <linux/etherdevice.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_net.h>
> > >  #include <linux/phy.h>
> > >  
> > >  #include <mach/board.h>
> > > @@ -81,6 +84,20 @@ static void __init macb_get_hwaddr(struct macb *bp)
> > >  	addr[4] = top & 0xff;
> > >  	addr[5] = (top >> 8) & 0xff;
> > >  
> > > +#ifdef CONFIG_OF
> > > +	/*
> > > +	 * 2) from device tree data
> > > +	 */
> > > +	if (!is_valid_ether_addr(addr)) {
> > > +		struct device_node *np = bp->pdev->dev.of_node;
> > > +		if (np) {
> > > +			const char *mac = of_get_mac_address(np);
> > > +			if (mac)
> > > +				memcpy(addr, mac, sizeof(addr));
> > > +		}
> > > +	}
> > > +#endif
> > 
> > I'm a bit conflicted here.  I think we should always use the MAC address 
> > from the device tree if it is present even if the current MAC address is 
> > valid.
> if the mac is already programmed in the register we just keep it
> I prefer this way if the bootloader set it we keep it

But I don't think that makes sense - if there is a MAC address in the 
DT, which is an optional property then the DT author must want to set 
the MAC address from the DT.  We should really prefer an explicit 
assignment over an implicit one.

Jamie

Re: [PATCH 1/1] net/macb: add DT support

[Nicolas Ferre]

On 11/20/2011 06:11 PM, Jamie Iles :
> On Sun, Nov 20, 2011 at 05:47:40PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 15:58 Fri 18 Nov     , Jamie Iles wrote:
>>> Hi Jean-Christophe,
>>>
>>> On Fri, Nov 18, 2011 at 03:29:25PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> allow the DT to pass the mac address and the phy mode
>>>>
>>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>>> Cc: Jamie Iles <jamie@jamieiles.com>
>>>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>
>>> This looks OK to me in principle.  I can't easily test this at the 
>>> moment, but as I don't have a DT platform that has the clk framework up 
>>> and running.  A couple of nits/questions inline, but thanks for doing 
>>> this!
>>>
>>> Jamie
>>>
>>>> ---
>>>>  Documentation/devicetree/bindings/net/macb.txt |   22 ++++++++
>>>>  drivers/net/ethernet/cadence/macb.c            |   65 +++++++++++++++++++++---
>>>>  drivers/net/ethernet/cadence/macb.h            |    2 +
>>>>  3 files changed, 81 insertions(+), 8 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/net/macb.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
>>>> new file mode 100644
>>>> index 0000000..2b727ec
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/macb.txt
>>>> @@ -0,0 +1,22 @@
>>>> +* Cadence EMACB
>>>> +
>>>> +Implemeted on Atmel AT91 & AVR32 SoC
>>>
>>> I think something along the lines of "Binding for the Cadence MACB 
>>> Ethernet controller" rather than listing specific parts might be 
>>> clearer.
>> I prefer as we will have implementation detail in the binding
> 
> I can't see any Atmel specific implementation detail here though so lets 
> keep it generic for now.  There isn't a benefit to keeping a list of 
> SoC's that the device is implemented in here as it'll only become out of 
> date.  We need to make it easy for other vendors to reuse the binding + 
> driver.

Yes, now that Jamie has made the driver generic, we should not advertise
for specific SoC...

>>>> +		compatible = "atmel,macb";
>>>
>>> This should be "cdns,macb" as it isn't Atmel specific.  I believe cdns 
>>> is the correct stock ticker symbol for Cadence.
>> here I put "atmel,macb" on purpose to specify the difference of the IP between
>> the soc, in fact it should have been atmel-at91,macb

No, before comma means "manufacturer".

> Well if we really can't detect the difference from the revision register 
> then we should have "cdns,macb" *and* "atmel,at91-macb" at least then 
> where platforms could claim compatibility as:
> 
> 	compatible = "atmel,at91-macb", "cdns,macb";
> 
> If we consider that another vendor integrates the Cadence IP, then it 
> makes much more sense to claim compatibility with a Cadence string 
> rather than an Atmel one...

Yes, it seems that you manage to use the revision register to identify
the IP. So here again, maybe the generic compatible string is enough...


>>>
>>>> +		reg = <oxfffc4000 0x4000>;
>>>> +		interrupts = <21>;
>>>> +		phy-mode = "mii";
>>>> +	};
>>>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>>>> index a437b46..2c345bc 100644
>>>> --- a/drivers/net/ethernet/cadence/macb.c
>>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>>> @@ -20,6 +20,9 @@
>>>>  #include <linux/etherdevice.h>
>>>>  #include <linux/dma-mapping.h>
>>>>  #include <linux/platform_device.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/of_net.h>
>>>>  #include <linux/phy.h>
>>>>  
>>>>  #include <mach/board.h>
>>>> @@ -81,6 +84,20 @@ static void __init macb_get_hwaddr(struct macb *bp)
>>>>  	addr[4] = top & 0xff;
>>>>  	addr[5] = (top >> 8) & 0xff;
>>>>  
>>>> +#ifdef CONFIG_OF
>>>> +	/*
>>>> +	 * 2) from device tree data
>>>> +	 */
>>>> +	if (!is_valid_ether_addr(addr)) {
>>>> +		struct device_node *np = bp->pdev->dev.of_node;
>>>> +		if (np) {
>>>> +			const char *mac = of_get_mac_address(np);
>>>> +			if (mac)
>>>> +				memcpy(addr, mac, sizeof(addr));
>>>> +		}
>>>> +	}
>>>> +#endif
>>>
>>> I'm a bit conflicted here.  I think we should always use the MAC address 
>>> from the device tree if it is present even if the current MAC address is 
>>> valid.
>> if the mac is already programmed in the register we just keep it
>> I prefer this way if the bootloader set it we keep it
> 
> But I don't think that makes sense - if there is a MAC address in the 
> DT, which is an optional property then the DT author must want to set 
> the MAC address from the DT.  We should really prefer an explicit 
> assignment over an implicit one.

Yes, that seems sensible.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH 1/1] net/macb: add DT support

[Nicolas Ferre]

On 11/18/2011 03:29 PM, Jean-Christophe PLAGNIOL-VILLARD :
> allow the DT to pass the mac address and the phy mode
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Jamie Iles <jamie@jamieiles.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  Documentation/devicetree/bindings/net/macb.txt |   22 ++++++++
>  drivers/net/ethernet/cadence/macb.c            |   65 +++++++++++++++++++++---
>  drivers/net/ethernet/cadence/macb.h            |    2 +
>  3 files changed, 81 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/macb.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
> new file mode 100644
> index 0000000..2b727ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/macb.txt
> @@ -0,0 +1,22 @@
> +* Cadence EMACB
> +
> +Implemeted on Atmel AT91 & AVR32 SoC

Ditto with Jamie's comments.

> +
> +Required properties:
> +- compatible : Should be "atmel,macb" for Atmel
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain macb interrupt
> +- phy-mode : String, operation mode of the PHY interface.
> +  Supported values are: "mii", "rmii",
> +
> +Optional properties:
> +- local-mac-address : 6 bytes, mac address
> +
> +Examples:
> +
> +	macb0: macb@fffc4000 {
> +		compatible = "atmel,macb";

Ditto.

> +		reg = <oxfffc4000 0x4000>;
> +		interrupts = <21>;
> +		phy-mode = "mii";
> +	};
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index a437b46..2c345bc 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -20,6 +20,9 @@
>  #include <linux/etherdevice.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_net.h>
>  #include <linux/phy.h>
>  
>  #include <mach/board.h>
> @@ -81,6 +84,20 @@ static void __init macb_get_hwaddr(struct macb *bp)
>  	addr[4] = top & 0xff;
>  	addr[5] = (top >> 8) & 0xff;
>  
> +#ifdef CONFIG_OF
> +	/*
> +	 * 2) from device tree data
> +	 */
> +	if (!is_valid_ether_addr(addr)) {
> +		struct device_node *np = bp->pdev->dev.of_node;
> +		if (np) {
> +			const char *mac = of_get_mac_address(np);
> +			if (mac)


Maybe here also, check is_valid_ether_addr(mac): it seems safer.


> +				memcpy(addr, mac, sizeof(addr));
> +		}
> +	}
> +#endif
> +
>  	if (is_valid_ether_addr(addr)) {
>  		memcpy(bp->dev->dev_addr, addr, sizeof(addr));
>  	} else {
> @@ -191,7 +208,6 @@ static int macb_mii_probe(struct net_device *dev)
>  {
>  	struct macb *bp = netdev_priv(dev);
>  	struct phy_device *phydev;
> -	struct eth_platform_data *pdata;
>  	int ret;
>  
>  	phydev = phy_find_first(bp->mii_bus);
> @@ -200,14 +216,11 @@ static int macb_mii_probe(struct net_device *dev)
>  		return -1;
>  	}
>  
> -	pdata = bp->pdev->dev.platform_data;
>  	/* TODO : add pin_irq */
>  
>  	/* attach the mac to the phy */
>  	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, 0,
> -				 pdata && pdata->is_rmii ?
> -				 PHY_INTERFACE_MODE_RMII :
> -				 PHY_INTERFACE_MODE_MII);
> +				 bp->phy_interface);
>  	if (ret) {
>  		printk(KERN_ERR "%s: Could not attach to PHY\n", dev->name);
>  		return ret;
> @@ -1117,6 +1130,30 @@ static const struct net_device_ops macb_netdev_ops = {
>  #endif
>  };
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id macb_dt_ids[] = {
> +	{ .compatible = "atmel,macb" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, macb_dt_ids);
> +
> +static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	if (np)
> +		return of_get_phy_mode(np);
> +
> +	return -ENODEV;
> +}
> +#else
> +static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  static int __init macb_probe(struct platform_device *pdev)
>  {
>  	struct eth_platform_data *pdata;
> @@ -1210,20 +1247,31 @@ static int __init macb_probe(struct platform_device *pdev)
>  	macb_writel(bp, NCFGR, config);
>  
>  	macb_get_hwaddr(bp);
> -	pdata = pdev->dev.platform_data;
>  
> -	if (pdata && pdata->is_rmii)
> +	err = macb_get_phy_mode_dt(pdev);
> +	if (err < 0) {
> +		pdata = pdev->dev.platform_data;
> +		if (pdata && pdata->is_rmii)
> +			bp->phy_interface = PHY_INTERFACE_MODE_RMII;
> +		else
> +			bp->phy_interface = PHY_INTERFACE_MODE_MII;
> +	} else {
> +		bp->phy_interface = err;
> +	}
> +
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_RMII) {
>  #if defined(CONFIG_ARCH_AT91)
>  		macb_writel(bp, USRIO, (MACB_BIT(RMII) | MACB_BIT(CLKEN)) );
>  #else
>  		macb_writel(bp, USRIO, 0);
>  #endif
> -	else
> +	} else {
>  #if defined(CONFIG_ARCH_AT91)
>  		macb_writel(bp, USRIO, MACB_BIT(CLKEN));
>  #else
>  		macb_writel(bp, USRIO, MACB_BIT(MII));
>  #endif
> +	}
>  
>  	bp->tx_pending = DEF_TX_RING_PENDING;
>  
> @@ -1344,6 +1392,7 @@ static struct platform_driver macb_driver = {
>  	.driver		= {
>  		.name		= "macb",
>  		.owner	= THIS_MODULE,
> +		.of_match_table	= of_match_ptr(macb_dt_ids),
>  	},
>  };
>  
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index d3212f6..8342744 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -389,6 +389,8 @@ struct macb {
>  	unsigned int 		link;
>  	unsigned int 		speed;
>  	unsigned int 		duplex;
> +
> +	phy_interface_t		phy_interface;
>  };
>  
>  #endif /* _MACB_H */


-- 
Nicolas Ferre

Re: [PATCH 1/1] net/macb: add DT support

[Nicolas Ferre]

On 11/20/2011 06:11 PM, Jamie Iles :
> On Sun, Nov 20, 2011 at 05:47:40PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 15:58 Fri 18 Nov     , Jamie Iles wrote:
>>> Hi Jean-Christophe,
>>>
>>> On Fri, Nov 18, 2011 at 03:29:25PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> allow the DT to pass the mac address and the phy mode
>>>>
>>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com>
>>>> Cc: Jamie Iles<jamie@jamieiles.com>
>>>> Cc: Nicolas Ferre<nicolas.ferre@atmel.com>
>>>
>>> This looks OK to me in principle.  I can't easily test this at the
>>> moment, but as I don't have a DT platform that has the clk framework up
>>> and running.  A couple of nits/questions inline, but thanks for doing
>>> this!
>>>
>>> Jamie
>>>
>>>> ---
>>>>   Documentation/devicetree/bindings/net/macb.txt |   22 ++++++++
>>>>   drivers/net/ethernet/cadence/macb.c            |   65 +++++++++++++++++++++---
>>>>   drivers/net/ethernet/cadence/macb.h            |    2 +
>>>>   3 files changed, 81 insertions(+), 8 deletions(-)
>>>>   create mode 100644 Documentation/devicetree/bindings/net/macb.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
>>>> new file mode 100644
>>>> index 0000000..2b727ec
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/macb.txt
>>>> @@ -0,0 +1,22 @@
>>>> +* Cadence EMACB
>>>> +
>>>> +Implemeted on Atmel AT91&  AVR32 SoC
>>>
>>> I think something along the lines of "Binding for the Cadence MACB
>>> Ethernet controller" rather than listing specific parts might be
>>> clearer.
>> I prefer as we will have implementation detail in the binding
>
> I can't see any Atmel specific implementation detail here though so lets
> keep it generic for now.  There isn't a benefit to keeping a list of
> SoC's that the device is implemented in here as it'll only become out of
> date.  We need to make it easy for other vendors to reuse the binding +
> driver.
>
>>>> +		compatible = "atmel,macb";
>>>
>>> This should be "cdns,macb" as it isn't Atmel specific.  I believe cdns
>>> is the correct stock ticker symbol for Cadence.
>> here I put "atmel,macb" on purpose to specify the difference of the IP between
>> the soc, in fact it should have been atmel-at91,macb
>
> Well if we really can't detect the difference from the revision register
> then we should have "cdns,macb" *and* "atmel,at91-macb" at least then
> where platforms could claim compatibility as:
>
> 	compatible = "atmel,at91-macb", "cdns,macb";

re-thinking about this I propose that we go for the following compatible 
string for macb:

- compatible: Should be "cdns,<chip>-macb"

And as the first SoC that have embedded an emacb that is compatible with 
current 10/100 AT91 usage is AVR32 at32ap7000... We may end up with 
"cdns,at32ap7000-macb" compatible string. The first ones with different 
synthesis parameters where at91sam9260/3 so I may also add:
"cdns,at91sam9260-macb".
Then you will have to add the first SoC that uses the gigabit version of 
the macb...
What do you think about that?

BTW, "cdns" seems not included in the vendor-prefixes.txt file yet...

Bye,
-- 
Nicolas Ferre

Re: [PATCH 1/1] net/macb: add DT support

[Jamie Iles]

On Fri, Dec 02, 2011 at 04:30:36PM +0100, Nicolas Ferre wrote:
> On 11/20/2011 06:11 PM, Jamie Iles :
> >On Sun, Nov 20, 2011 at 05:47:40PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>On 15:58 Fri 18 Nov     , Jamie Iles wrote:
[...]
> >>>>+		compatible = "atmel,macb";
> >>>
> >>>This should be "cdns,macb" as it isn't Atmel specific.  I believe cdns
> >>>is the correct stock ticker symbol for Cadence.
> >>here I put "atmel,macb" on purpose to specify the difference of the IP between
> >>the soc, in fact it should have been atmel-at91,macb
> >
> >Well if we really can't detect the difference from the revision register
> >then we should have "cdns,macb" *and* "atmel,at91-macb" at least then
> >where platforms could claim compatibility as:
> >
> >	compatible = "atmel,at91-macb", "cdns,macb";
> 
> re-thinking about this I propose that we go for the following
> compatible string for macb:
> 
> - compatible: Should be "cdns,<chip>-macb"
> 
> And as the first SoC that have embedded an emacb that is compatible
> with current 10/100 AT91 usage is AVR32 at32ap7000... We may end up
> with "cdns,at32ap7000-macb" compatible string. The first ones with
> different synthesis parameters where at91sam9260/3 so I may also
> add:
> "cdns,at91sam9260-macb".
> Then you will have to add the first SoC that uses the gigabit
> version of the macb...
> What do you think about that?

Sure, that works for me, though I guess this is a much more general 
thing than this one binding, but that does make sense to me.  I think 
that keeping a general "cdns,macb" _too_ still makes sense though as 
lots of it may well be detectable and it will probably be difficult for 
one SoC vendor to know whether their IP instantiation really is the same 
as another vendors...  Either way I don't have a strong opinion on that.

> BTW, "cdns" seems not included in the vendor-prefixes.txt file yet...

No, that one is missing.  If you want to add it then feel free, if not 
I'll add it to my list of patches to do!

Jamie

[PATCH] net/macb: add DT support

[Nicolas Ferre]

From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Allow the device tree to provide the mac address and the phy mode.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
[nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change "compatible" node property, doc and DT hwaddr]
Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Cc: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---
 Documentation/devicetree/bindings/net/macb.txt |   22 +++++++
 drivers/net/ethernet/cadence/macb.c            |   71 +++++++++++++++++++++---
 drivers/net/ethernet/cadence/macb.h            |    2 +
 3 files changed, 87 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/macb.txt

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
new file mode 100644
index 0000000..7f0b90a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -0,0 +1,22 @@
+* Cadence MACB Ethernet controller
+
+Required properties:
+- compatible: Should be "cdns,[<chip>-]macb"
+  Use "cdns,at91sam9260-macb" Atmel at91sam9260 and at91sam9263 SoCs.
+  Use "cdns,at32ap7000-macb" for other 10/100 usage or use the generic form: "cdns,macb"
+- reg: Address and length of the register set for the device
+- interrupts: Should contain macb interrupt
+- phy-mode: String, operation mode of the PHY interface.
+  Supported values are: "mii", "rmii", "gmii", "rgmii".
+
+Optional properties:
+- local-mac-address: 6 bytes, mac address
+
+Examples:
+
+	macb0: ethernet@fffc4000 {
+		compatible = "cdns,at32ap7000-macb";
+		reg = <0xfffc4000 0x4000>;
+		interrupts = <21>;
+		phy-mode = "rmii";
+	};
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 64d6146..103c6e6 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -23,6 +23,8 @@
 #include <linux/platform_data/macb.h>
 #include <linux/platform_device.h>
 #include <linux/phy.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
 
 #include "macb.h"
 
@@ -191,7 +193,6 @@ static int macb_mii_probe(struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
 	struct phy_device *phydev;
-	struct macb_platform_data *pdata;
 	int ret;
 
 	phydev = phy_find_first(bp->mii_bus);
@@ -200,14 +201,11 @@ static int macb_mii_probe(struct net_device *dev)
 		return -1;
 	}
 
-	pdata = bp->pdev->dev.platform_data;
 	/* TODO : add pin_irq */
 
 	/* attach the mac to the phy */
 	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, 0,
-				 pdata && pdata->is_rmii ?
-				 PHY_INTERFACE_MODE_RMII :
-				 PHY_INTERFACE_MODE_MII);
+				 bp->phy_interface);
 	if (ret) {
 		netdev_err(dev, "Could not attach to PHY\n");
 		return ret;
@@ -1244,6 +1242,50 @@ static const struct net_device_ops macb_netdev_ops = {
 #endif
 };
 
+#if defined(CONFIG_OF)
+static const struct of_device_id macb_dt_ids[] = {
+	{ .compatible = "cdns,at32ap7000-macb" },
+	{ .compatible = "cdns,at91sam9260-macb" },
+	{ .compatible = "cdns,macb" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, macb_dt_ids);
+
+static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	if (np)
+		return of_get_phy_mode(np);
+
+	return -ENODEV;
+}
+
+static int __devinit macb_get_hwaddr_dt(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	if (np) {
+		const char *mac = of_get_mac_address(np);
+		if (mac) {
+			memcpy(pdev->dev->dev_addr, mac, ETH_ALEN);
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+#else
+static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+static int __devinit macb_get_hwaddr_dt(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+#endif
+
 static int __init macb_probe(struct platform_device *pdev)
 {
 	struct macb_platform_data *pdata;
@@ -1318,10 +1360,22 @@ static int __init macb_probe(struct platform_device *pdev)
 	config |= macb_dbw(bp);
 	macb_writel(bp, NCFGR, config);
 
-	macb_get_hwaddr(bp);
-	pdata = pdev->dev.platform_data;
+	err = macb_get_hwaddr_dt(pdev);
+	if (err < 0)
+		macb_get_hwaddr(bp);
+
+	err = macb_get_phy_mode_dt(pdev);
+	if (err < 0) {
+		pdata = pdev->dev.platform_data;
+		if (pdata && pdata->is_rmii)
+			bp->phy_interface = PHY_INTERFACE_MODE_RMII;
+		else
+			bp->phy_interface = PHY_INTERFACE_MODE_MII;
+	} else {
+		bp->phy_interface = err;
+	}
 
-	if (pdata && pdata->is_rmii)
+	if (bp->phy_interface == PHY_INTERFACE_MODE_RMII)
 #if defined(CONFIG_ARCH_AT91)
 		macb_or_gem_writel(bp, USRIO, (MACB_BIT(RMII) |
 					       MACB_BIT(CLKEN)));
@@ -1444,6 +1498,7 @@ static struct platform_driver macb_driver = {
 	.driver		= {
 		.name		= "macb",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(macb_dt_ids),
 	},
 };
 
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 1931078..335e288 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -532,6 +532,8 @@ struct macb {
 	unsigned int 		link;
 	unsigned int 		speed;
 	unsigned int 		duplex;
+
+	phy_interface_t		phy_interface;
 };
 
 static inline bool macb_is_gem(struct macb *bp)
-- 
1.7.5.4

Re: [PATCH] net/macb: add DT support

[Jamie Iles]

Hi Nicolas,

On Fri, Dec 02, 2011 at 06:14:10PM +0100, Nicolas Ferre wrote:
> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> 
> Allow the device tree to provide the mac address and the phy mode.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change "compatible" node property, doc and DT hwaddr]
> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> Cc: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>

Looks nice to me.  There's a patch below to add the GEM stuff to the 
binding too if you want to role that in.

Acked-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>

8<----

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index 7f0b90a..e09e3fb 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -1,9 +1,11 @@
-* Cadence MACB Ethernet controller
+* Cadence MACB/GEM Ethernet controller
 
 Required properties:
-- compatible: Should be "cdns,[<chip>-]macb"
+- compatible: Should be "cdns,[<chip>-]{macb,gem}"
   Use "cdns,at91sam9260-macb" Atmel at91sam9260 and at91sam9263 SoCs.
   Use "cdns,at32ap7000-macb" for other 10/100 usage or use the generic form: "cdns,macb"
+  Use "cnds,pc302-gem" for Picochip picoXcell pc302 and later devices based on
+  the Cadence GEM, or the generic form "cdns,gem".
 - reg: Address and length of the register set for the device
 - interrupts: Should contain macb interrupt
 - phy-mode: String, operation mode of the PHY interface.
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 103c6e6..89060e6 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -1247,6 +1247,8 @@ static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,at32ap7000-macb" },
 	{ .compatible = "cdns,at91sam9260-macb" },
 	{ .compatible = "cdns,macb" },
+	{ .compatible = "cdns,pc302-gem" },
+	{ .compatible = "cdns,gem" },
 	{ /* sentinel */ }
 };

[PATCH v2] net/macb: add DT support

[Nicolas Ferre]

From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Allow the device tree to provide the mac address and the phy mode.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
[nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change "compatible" node property, doc and DT hwaddr]
Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Cc: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---
v2: modify macb_get_hwaddr_dt() parameter

 Documentation/devicetree/bindings/net/macb.txt |   22 +++++++
 drivers/net/ethernet/cadence/macb.c            |   71 +++++++++++++++++++++---
 drivers/net/ethernet/cadence/macb.h            |    2 +
 3 files changed, 87 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/macb.txt

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
new file mode 100644
index 0000000..7f0b90a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -0,0 +1,22 @@
+* Cadence MACB Ethernet controller
+
+Required properties:
+- compatible: Should be "cdns,[<chip>-]macb"
+  Use "cdns,at91sam9260-macb" Atmel at91sam9260 and at91sam9263 SoCs.
+  Use "cdns,at32ap7000-macb" for other 10/100 usage or use the generic form: "cdns,macb"
+- reg: Address and length of the register set for the device
+- interrupts: Should contain macb interrupt
+- phy-mode: String, operation mode of the PHY interface.
+  Supported values are: "mii", "rmii", "gmii", "rgmii".
+
+Optional properties:
+- local-mac-address: 6 bytes, mac address
+
+Examples:
+
+	macb0: ethernet@fffc4000 {
+		compatible = "cdns,at32ap7000-macb";
+		reg = <0xfffc4000 0x4000>;
+		interrupts = <21>;
+		phy-mode = "rmii";
+	};
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 64d6146..5e75855 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -23,6 +23,8 @@
 #include <linux/platform_data/macb.h>
 #include <linux/platform_device.h>
 #include <linux/phy.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
 
 #include "macb.h"
 
@@ -191,7 +193,6 @@ static int macb_mii_probe(struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
 	struct phy_device *phydev;
-	struct macb_platform_data *pdata;
 	int ret;
 
 	phydev = phy_find_first(bp->mii_bus);
@@ -200,14 +201,11 @@ static int macb_mii_probe(struct net_device *dev)
 		return -1;
 	}
 
-	pdata = bp->pdev->dev.platform_data;
 	/* TODO : add pin_irq */
 
 	/* attach the mac to the phy */
 	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, 0,
-				 pdata && pdata->is_rmii ?
-				 PHY_INTERFACE_MODE_RMII :
-				 PHY_INTERFACE_MODE_MII);
+				 bp->phy_interface);
 	if (ret) {
 		netdev_err(dev, "Could not attach to PHY\n");
 		return ret;
@@ -1244,6 +1242,50 @@ static const struct net_device_ops macb_netdev_ops = {
 #endif
 };
 
+#if defined(CONFIG_OF)
+static const struct of_device_id macb_dt_ids[] = {
+	{ .compatible = "cdns,at32ap7000-macb" },
+	{ .compatible = "cdns,at91sam9260-macb" },
+	{ .compatible = "cdns,macb" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, macb_dt_ids);
+
+static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	if (np)
+		return of_get_phy_mode(np);
+
+	return -ENODEV;
+}
+
+static int __devinit macb_get_hwaddr_dt(struct macb *bp)
+{
+	struct device_node *np = bp->pdev->dev.of_node;
+	if (np) {
+		const char *mac = of_get_mac_address(np);
+		if (mac) {
+			memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+#else
+static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+static int __devinit macb_get_hwaddr_dt(struct macb *bp)
+{
+	return -ENODEV;
+}
+#endif
+
 static int __init macb_probe(struct platform_device *pdev)
 {
 	struct macb_platform_data *pdata;
@@ -1318,10 +1360,22 @@ static int __init macb_probe(struct platform_device *pdev)
 	config |= macb_dbw(bp);
 	macb_writel(bp, NCFGR, config);
 
-	macb_get_hwaddr(bp);
-	pdata = pdev->dev.platform_data;
+	err = macb_get_hwaddr_dt(bp);
+	if (err < 0)
+		macb_get_hwaddr(bp);
+
+	err = macb_get_phy_mode_dt(pdev);
+	if (err < 0) {
+		pdata = pdev->dev.platform_data;
+		if (pdata && pdata->is_rmii)
+			bp->phy_interface = PHY_INTERFACE_MODE_RMII;
+		else
+			bp->phy_interface = PHY_INTERFACE_MODE_MII;
+	} else {
+		bp->phy_interface = err;
+	}
 
-	if (pdata && pdata->is_rmii)
+	if (bp->phy_interface == PHY_INTERFACE_MODE_RMII)
 #if defined(CONFIG_ARCH_AT91)
 		macb_or_gem_writel(bp, USRIO, (MACB_BIT(RMII) |
 					       MACB_BIT(CLKEN)));
@@ -1444,6 +1498,7 @@ static struct platform_driver macb_driver = {
 	.driver		= {
 		.name		= "macb",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(macb_dt_ids),
 	},
 };
 
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 1931078..335e288 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -532,6 +532,8 @@ struct macb {
 	unsigned int 		link;
 	unsigned int 		speed;
 	unsigned int 		duplex;
+
+	phy_interface_t		phy_interface;
 };
 
 static inline bool macb_is_gem(struct macb *bp)
-- 
1.7.5.4

[PATCH] ARM: at91/net: add macb ethernet controller in 9g45 DT

[Nicolas Ferre]

Add the Cadence macb ethernet controller in at91sam9g45 .dtsi and
enable it in at91sam9m10g45ek board device tree file.

Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/at91sam9g45.dtsi     |    7 +++++++
 arch/arm/boot/dts/at91sam9m10g45ek.dts |    6 ++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index e89b1d7..67f94d3 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -101,6 +101,13 @@
 				atmel,use-dma-tx;
 				status = "disabled";
 			};
+
+			macb0: ethernet@fffbc000 {
+				compatible = "cdns,at32ap7000-macb", "cdns,macb";
+				reg = <0xfffbc000 0x100>;
+				interrupts = <25 4>;
+				status = "disabled";
+			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/at91sam9m10g45ek.dts b/arch/arm/boot/dts/at91sam9m10g45ek.dts
index 85b34f5..17377a2 100644
--- a/arch/arm/boot/dts/at91sam9m10g45ek.dts
+++ b/arch/arm/boot/dts/at91sam9m10g45ek.dts
@@ -30,6 +30,12 @@
 			usart1: serial@fff90000 {
 				status = "okay";
 			};
+
+			macb0: ethernet@fffbc000 {
+				local-mac-address = [3a 0e 03 04 05 06];
+				phy-mode = "rmii";
+				status = "okay";
+			};
 		};
 	};
 };
-- 
1.7.5.4

Re: [PATCH] net/macb: add DT support

[Nicolas Ferre]

On 12/02/2011 06:28 PM, Jamie Iles :
> Hi Nicolas,
>
> On Fri, Dec 02, 2011 at 06:14:10PM +0100, Nicolas Ferre wrote:
>> From: Jean-Christophe PLAGNIOL-VILLARD<plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>>
>> Allow the device tree to provide the mac address and the phy mode.
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change "compatible" node property, doc and DT hwaddr]
>> Signed-off-by: Nicolas Ferre<nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> Cc: Jamie Iles<jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
>
> Looks nice to me.  There's a patch below to add the GEM stuff to the
> binding too if you want to role that in.

Yes, I will push it in a "v3" (I was busy correcting a bug and did not 
see your answer before sending v2...).

> Acked-by: Jamie Iles<jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
>
> 8<----
>
> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
> index 7f0b90a..e09e3fb 100644
> --- a/Documentation/devicetree/bindings/net/macb.txt
> +++ b/Documentation/devicetree/bindings/net/macb.txt
> @@ -1,9 +1,11 @@
> -* Cadence MACB Ethernet controller
> +* Cadence MACB/GEM Ethernet controller
>
>   Required properties:
> -- compatible: Should be "cdns,[<chip>-]macb"
> +- compatible: Should be "cdns,[<chip>-]{macb,gem}"
>     Use "cdns,at91sam9260-macb" Atmel at91sam9260 and at91sam9263 SoCs.
>     Use "cdns,at32ap7000-macb" for other 10/100 usage or use the generic form: "cdns,macb"
> +  Use "cnds,pc302-gem" for Picochip picoXcell pc302 and later devices based on
> +  the Cadence GEM, or the generic form "cdns,gem".
>   - reg: Address and length of the register set for the device
>   - interrupts: Should contain macb interrupt
>   - phy-mode: String, operation mode of the PHY interface.
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 103c6e6..89060e6 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1247,6 +1247,8 @@ static const struct of_device_id macb_dt_ids[] = {
>   	{ .compatible = "cdns,at32ap7000-macb" },
>   	{ .compatible = "cdns,at91sam9260-macb" },
>   	{ .compatible = "cdns,macb" },
> +	{ .compatible = "cdns,pc302-gem" },
> +	{ .compatible = "cdns,gem" },
>   	{ /* sentinel */ }
>   };

BTW, I think we may also modify the MII/RMII selection code for adding 
gigabit selection... but maybe you already have the patches?

Best regards,
-- 
Nicolas Ferre

Re: [PATCH v2] net/macb: add DT support

[David Miller]

From: Nicolas Ferre <nicolas.ferre@atmel.com>
Date: Fri,  2 Dec 2011 18:43:02 +0100

> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> 
> Allow the device tree to provide the mac address and the phy mode.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> [nicolas.ferre@atmel.com: change "compatible" node property, doc and DT hwaddr]
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Jamie Iles <jamie@jamieiles.com>
> ---
> v2: modify macb_get_hwaddr_dt() parameter

You'll have to respin these two macb patches, as they don't apply properly
to the net-next tree which is where they should be targetted.

Thanks.

Re: [PATCH] ARM: at91/net: add macb ethernet controller in 9g45 DT

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:50 Fri 02 Dec     , Nicolas Ferre wrote:
> Add the Cadence macb ethernet controller in at91sam9g45 .dtsi and
> enable it in at91sam9m10g45ek board device tree file.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  arch/arm/boot/dts/at91sam9g45.dtsi     |    7 +++++++
>  arch/arm/boot/dts/at91sam9m10g45ek.dts |    6 ++++++
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
> index e89b1d7..67f94d3 100644
> --- a/arch/arm/boot/dts/at91sam9g45.dtsi
> +++ b/arch/arm/boot/dts/at91sam9g45.dtsi
> @@ -101,6 +101,13 @@
>  				atmel,use-dma-tx;
>  				status = "disabled";
>  			};
> +
> +			macb0: ethernet@fffbc000 {
> +				compatible = "cdns,at32ap7000-macb", "cdns,macb";
> +				reg = <0xfffbc000 0x100>;
> +				interrupts = <25 4>;
why?
> +				status = "disabled";
> +			};
>  		};
>  	};
>  };
> diff --git a/arch/arm/boot/dts/at91sam9m10g45ek.dts b/arch/arm/boot/dts/at91sam9m10g45ek.dts
> index 85b34f5..17377a2 100644
> --- a/arch/arm/boot/dts/at91sam9m10g45ek.dts
> +++ b/arch/arm/boot/dts/at91sam9m10g45ek.dts
> @@ -30,6 +30,12 @@
>  			usart1: serial@fff90000 {
>  				status = "okay";
>  			};
> +
> +			macb0: ethernet@fffbc000 {
> +				local-mac-address = [3a 0e 03 04 05 06];
please drop this is not supposed to be in the dts but updated for each board

Best Regards,
J.

Re: [PATCH v2] net/macb: add DT support

[Nicolas Ferre]

On 12/02/2011 06:58 PM, David Miller :
> From: Nicolas Ferre<nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> Date: Fri,  2 Dec 2011 18:43:02 +0100
>
>> From: Jean-Christophe PLAGNIOL-VILLARD<plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>>
>> Allow the device tree to provide the mac address and the phy mode.
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change "compatible" node property, doc and DT hwaddr]
>> Signed-off-by: Nicolas Ferre<nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> Cc: Jamie Iles<jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
>> ---
>> v2: modify macb_get_hwaddr_dt() parameter
>
> You'll have to respin these two macb patches, as they don't apply properly
> to the net-next tree which is where they should be targetted.

David,

In fact, the patches are designed to be added on top of Jamie's rework 
of the macb driver that is now in Arnd's "arm-soc" git tree (for-next 
branch). You discussed with him here:

http://www.spinics.net/lists/arm-kernel/msg118717.html

Should I go this path also or wait for this material to be merged and 
update on net-next at this time (after 3.3 merge window...)?

Best regards,
-- 
Nicolas Ferre

Re: [PATCH] ARM: at91/net: add macb ethernet controller in 9g45 DT

[Nicolas Ferre]

On 12/03/2011 06:56 AM, Jean-Christophe PLAGNIOL-VILLARD :
> On 18:50 Fri 02 Dec     , Nicolas Ferre wrote:
>> Add the Cadence macb ethernet controller in at91sam9g45 .dtsi and
>> enable it in at91sam9m10g45ek board device tree file.
>>
>> Signed-off-by: Nicolas Ferre<nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>>   arch/arm/boot/dts/at91sam9g45.dtsi     |    7 +++++++
>>   arch/arm/boot/dts/at91sam9m10g45ek.dts |    6 ++++++
>>   2 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
>> index e89b1d7..67f94d3 100644
>> --- a/arch/arm/boot/dts/at91sam9g45.dtsi
>> +++ b/arch/arm/boot/dts/at91sam9g45.dtsi
>> @@ -101,6 +101,13 @@
>>   				atmel,use-dma-tx;
>>   				status = "disabled";
>>   			};
>> +
>> +			macb0: ethernet@fffbc000 {
>> +				compatible = "cdns,at32ap7000-macb", "cdns,macb";
>> +				reg =<0xfffbc000 0x100>;
>> +				interrupts =<25 4>;
> why?

It is the new AIC specification that was reworked in this patch:
https://lkml.org/lkml/2011/12/1/238

>> +				status = "disabled";
>> +			};
>>   		};
>>   	};
>>   };
>> diff --git a/arch/arm/boot/dts/at91sam9m10g45ek.dts b/arch/arm/boot/dts/at91sam9m10g45ek.dts
>> index 85b34f5..17377a2 100644
>> --- a/arch/arm/boot/dts/at91sam9m10g45ek.dts
>> +++ b/arch/arm/boot/dts/at91sam9m10g45ek.dts
>> @@ -30,6 +30,12 @@
>>   			usart1: serial@fff90000 {
>>   				status = "okay";
>>   			};
>> +
>> +			macb0: ethernet@fffbc000 {
>> +				local-mac-address = [3a 0e 03 04 05 06];
> please drop this is not supposed to be in the dts but updated for each board

Yes, sure: I kept in because it was allowing me to test. But for sure, 
this should go away...

Bye,
-- 
Nicolas Ferre

Re: [PATCH] net/macb: add DT support

[Jamie Iles]

On Fri, Dec 02, 2011 at 06:53:58PM +0100, Nicolas Ferre wrote:
> On 12/02/2011 06:28 PM, Jamie Iles :
[...]
> >diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> >index 103c6e6..89060e6 100644
> >--- a/drivers/net/ethernet/cadence/macb.c
> >+++ b/drivers/net/ethernet/cadence/macb.c
> >@@ -1247,6 +1247,8 @@ static const struct of_device_id macb_dt_ids[] = {
> >  	{ .compatible = "cdns,at32ap7000-macb" },
> >  	{ .compatible = "cdns,at91sam9260-macb" },
> >  	{ .compatible = "cdns,macb" },
> >+	{ .compatible = "cdns,pc302-gem" },
> >+	{ .compatible = "cdns,gem" },
> >  	{ /* sentinel */ }
> >  };
> 
> BTW, I think we may also modify the MII/RMII selection code for
> adding gigabit selection... but maybe you already have the patches?

No, I don't have access to a version of the GEM with gigabit support.  
The platforms I was working on used it as a 10/100 controller, but had 
to use the GEM to get hardware timestamping support.

Jamie

Re: [PATCH] net/macb: add DT support

[Nicolas Ferre]

On 12/05/2011 12:48 PM, Jamie Iles :
> On Fri, Dec 02, 2011 at 06:53:58PM +0100, Nicolas Ferre wrote:
>> On 12/02/2011 06:28 PM, Jamie Iles :
> [...]
>>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>>> index 103c6e6..89060e6 100644
>>> --- a/drivers/net/ethernet/cadence/macb.c
>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>> @@ -1247,6 +1247,8 @@ static const struct of_device_id macb_dt_ids[] = {
>>>   	{ .compatible = "cdns,at32ap7000-macb" },
>>>   	{ .compatible = "cdns,at91sam9260-macb" },
>>>   	{ .compatible = "cdns,macb" },
>>> +	{ .compatible = "cdns,pc302-gem" },
>>> +	{ .compatible = "cdns,gem" },
>>>   	{ /* sentinel */ }
>>>   };
>>
>> BTW, I think we may also modify the MII/RMII selection code for
>> adding gigabit selection... but maybe you already have the patches?
>
> No, I don't have access to a version of the GEM with gigabit support.
> The platforms I was working on used it as a 10/100 controller, but had
> to use the GEM to get hardware timestamping support.

Ok, Thanks for your feedback. I send a v3 patch now.

Best regards,
-- 
Nicolas Ferre

[PATCH v3 1/2] net/macb: add DT support for Cadence macb/gem driver

[Nicolas Ferre]

From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Allow the device tree to provide the mac address and the phy mode.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
[nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change "compatible" node property, doc and DT hwaddr]
Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
[jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org: add "gem" compatibility strings and doc]
Acked-by: Jamie Iles<jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---
v3: add "gem" compatibility strings
v2: modify macb_get_hwaddr_dt() parameter

 Documentation/devicetree/bindings/net/macb.txt |   24 ++++++++
 drivers/net/ethernet/cadence/macb.c            |   73 +++++++++++++++++++++---
 drivers/net/ethernet/cadence/macb.h            |    2 +
 3 files changed, 91 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/macb.txt

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
new file mode 100644
index 0000000..2e24f05
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -0,0 +1,24 @@
+* Cadence MACB/GEM Ethernet controller
+
+Required properties:
+- compatible: Should be "cdns,[<chip>-]{macb|gem}"
+  Use "cdns,at91sam9260-macb" Atmel at91sam9260 and at91sam9263 SoCs.
+  Use "cdns,at32ap7000-macb" for other 10/100 usage or use the generic form: "cdns,macb".
+  Use "cnds,pc302-gem" for Picochip picoXcell pc302 and later devices based on
+  the Cadence GEM, or the generic form: "cdns,gem".
+- reg: Address and length of the register set for the device
+- interrupts: Should contain macb interrupt
+- phy-mode: String, operation mode of the PHY interface.
+  Supported values are: "mii", "rmii", "gmii", "rgmii".
+
+Optional properties:
+- local-mac-address: 6 bytes, mac address
+
+Examples:
+
+	macb0: ethernet@fffc4000 {
+		compatible = "cdns,at32ap7000-macb";
+		reg = <0xfffc4000 0x4000>;
+		interrupts = <21>;
+		phy-mode = "rmii";
+	};
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 64d6146..baf1a0d 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -23,6 +23,8 @@
 #include <linux/platform_data/macb.h>
 #include <linux/platform_device.h>
 #include <linux/phy.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
 
 #include "macb.h"
 
@@ -191,7 +193,6 @@ static int macb_mii_probe(struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
 	struct phy_device *phydev;
-	struct macb_platform_data *pdata;
 	int ret;
 
 	phydev = phy_find_first(bp->mii_bus);
@@ -200,14 +201,11 @@ static int macb_mii_probe(struct net_device *dev)
 		return -1;
 	}
 
-	pdata = bp->pdev->dev.platform_data;
 	/* TODO : add pin_irq */
 
 	/* attach the mac to the phy */
 	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, 0,
-				 pdata && pdata->is_rmii ?
-				 PHY_INTERFACE_MODE_RMII :
-				 PHY_INTERFACE_MODE_MII);
+				 bp->phy_interface);
 	if (ret) {
 		netdev_err(dev, "Could not attach to PHY\n");
 		return ret;
@@ -1244,6 +1242,52 @@ static const struct net_device_ops macb_netdev_ops = {
 #endif
 };
 
+#if defined(CONFIG_OF)
+static const struct of_device_id macb_dt_ids[] = {
+	{ .compatible = "cdns,at32ap7000-macb" },
+	{ .compatible = "cdns,at91sam9260-macb" },
+	{ .compatible = "cdns,macb" },
+	{ .compatible = "cdns,pc302-gem" },
+	{ .compatible = "cdns,gem" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, macb_dt_ids);
+
+static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	if (np)
+		return of_get_phy_mode(np);
+
+	return -ENODEV;
+}
+
+static int __devinit macb_get_hwaddr_dt(struct macb *bp)
+{
+	struct device_node *np = bp->pdev->dev.of_node;
+	if (np) {
+		const char *mac = of_get_mac_address(np);
+		if (mac) {
+			memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+#else
+static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+static int __devinit macb_get_hwaddr_dt(struct macb *bp)
+{
+	return -ENODEV;
+}
+#endif
+
 static int __init macb_probe(struct platform_device *pdev)
 {
 	struct macb_platform_data *pdata;
@@ -1318,10 +1362,22 @@ static int __init macb_probe(struct platform_device *pdev)
 	config |= macb_dbw(bp);
 	macb_writel(bp, NCFGR, config);
 
-	macb_get_hwaddr(bp);
-	pdata = pdev->dev.platform_data;
+	err = macb_get_hwaddr_dt(bp);
+	if (err < 0)
+		macb_get_hwaddr(bp);
+
+	err = macb_get_phy_mode_dt(pdev);
+	if (err < 0) {
+		pdata = pdev->dev.platform_data;
+		if (pdata && pdata->is_rmii)
+			bp->phy_interface = PHY_INTERFACE_MODE_RMII;
+		else
+			bp->phy_interface = PHY_INTERFACE_MODE_MII;
+	} else {
+		bp->phy_interface = err;
+	}
 
-	if (pdata && pdata->is_rmii)
+	if (bp->phy_interface == PHY_INTERFACE_MODE_RMII)
 #if defined(CONFIG_ARCH_AT91)
 		macb_or_gem_writel(bp, USRIO, (MACB_BIT(RMII) |
 					       MACB_BIT(CLKEN)));
@@ -1444,6 +1500,7 @@ static struct platform_driver macb_driver = {
 	.driver		= {
 		.name		= "macb",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(macb_dt_ids),
 	},
 };
 
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 1931078..335e288 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -532,6 +532,8 @@ struct macb {
 	unsigned int 		link;
 	unsigned int 		speed;
 	unsigned int 		duplex;
+
+	phy_interface_t		phy_interface;
 };
 
 static inline bool macb_is_gem(struct macb *bp)
-- 
1.7.5.4

[PATCH v3 2/2] ARM: at91/net: add macb ethernet controller in 9g45 DT

[Nicolas Ferre]

Add the Cadence macb ethernet controller in at91sam9g45 .dtsi and
enable it in at91sam9m10g45ek board device tree file.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
v3: - form a thread with "macb DT support" patch
    - remove "local-mac-address" from 9m10g45ek.dts file but use it in
      documentation example.

 Documentation/devicetree/bindings/net/macb.txt |    1 +
 arch/arm/boot/dts/at91sam9g45.dtsi             |    7 +++++++
 arch/arm/boot/dts/at91sam9m10g45ek.dts         |    5 +++++
 3 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index 2e24f05..44afa0e 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -21,4 +21,5 @@ Examples:
 		reg = <0xfffc4000 0x4000>;
 		interrupts = <21>;
 		phy-mode = "rmii";
+		local-mac-address = [3a 0e 03 04 05 06];
 	};
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index e89b1d7..67f94d3 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -101,6 +101,13 @@
 				atmel,use-dma-tx;
 				status = "disabled";
 			};
+
+			macb0: ethernet@fffbc000 {
+				compatible = "cdns,at32ap7000-macb", "cdns,macb";
+				reg = <0xfffbc000 0x100>;
+				interrupts = <25 4>;
+				status = "disabled";
+			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/at91sam9m10g45ek.dts b/arch/arm/boot/dts/at91sam9m10g45ek.dts
index 85b34f5..a387e77 100644
--- a/arch/arm/boot/dts/at91sam9m10g45ek.dts
+++ b/arch/arm/boot/dts/at91sam9m10g45ek.dts
@@ -30,6 +30,11 @@
 			usart1: serial@fff90000 {
 				status = "okay";
 			};
+
+			macb0: ethernet@fffbc000 {
+				phy-mode = "rmii";
+				status = "okay";
+			};
 		};
 	};
 };
-- 
1.7.5.4

Re: [PATCH v3 2/2] ARM: at91/net: add macb ethernet controller in 9g45 DT

[Jean-Christophe PLAGNIOL-VILLARD]

On 12:59 Mon 05 Dec     , Nicolas Ferre wrote:
> Add the Cadence macb ethernet controller in at91sam9g45 .dtsi and
> enable it in at91sam9m10g45ek board device tree file.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Best Regards,
J.

Re: [PATCH v3 1/2] net/macb: add DT support for Cadence macb/gem driver

[Nicolas Ferre]

On 12/05/2011 12:59 PM, Nicolas Ferre :
> From: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com>
>
> Allow the device tree to provide the mac address and the phy mode.
>
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com>
> [nicolas.ferre@atmel.com: change "compatible" node property, doc and DT hwaddr]
> Signed-off-by: Nicolas Ferre<nicolas.ferre@atmel.com>
> [jamie@jamieiles.com: add "gem" compatibility strings and doc]
> Acked-by: Jamie Iles<jamie@jamieiles.com>

David, can I have your acknowledgment on this patch so that I can send 
it through arm-soc git tree? This way it will reside on top of previous 
work by Jamie that is already in arm-soc git (for-next branch).

Thanks, best regards,

> ---
> v3: add "gem" compatibility strings
> v2: modify macb_get_hwaddr_dt() parameter
>
>   Documentation/devicetree/bindings/net/macb.txt |   24 ++++++++
>   drivers/net/ethernet/cadence/macb.c            |   73 +++++++++++++++++++++---
>   drivers/net/ethernet/cadence/macb.h            |    2 +
>   3 files changed, 91 insertions(+), 8 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/net/macb.txt
>
> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
> new file mode 100644
> index 0000000..2e24f05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/macb.txt
> @@ -0,0 +1,24 @@
> +* Cadence MACB/GEM Ethernet controller
> +
> +Required properties:
> +- compatible: Should be "cdns,[<chip>-]{macb|gem}"
> +  Use "cdns,at91sam9260-macb" Atmel at91sam9260 and at91sam9263 SoCs.
> +  Use "cdns,at32ap7000-macb" for other 10/100 usage or use the generic form: "cdns,macb".
> +  Use "cnds,pc302-gem" for Picochip picoXcell pc302 and later devices based on
> +  the Cadence GEM, or the generic form: "cdns,gem".
> +- reg: Address and length of the register set for the device
> +- interrupts: Should contain macb interrupt
> +- phy-mode: String, operation mode of the PHY interface.
> +  Supported values are: "mii", "rmii", "gmii", "rgmii".
> +
> +Optional properties:
> +- local-mac-address: 6 bytes, mac address
> +
> +Examples:
> +
> +	macb0: ethernet@fffc4000 {
> +		compatible = "cdns,at32ap7000-macb";
> +		reg =<0xfffc4000 0x4000>;
> +		interrupts =<21>;
> +		phy-mode = "rmii";
> +	};
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 64d6146..baf1a0d 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -23,6 +23,8 @@
>   #include<linux/platform_data/macb.h>
>   #include<linux/platform_device.h>
>   #include<linux/phy.h>
> +#include<linux/of_device.h>
> +#include<linux/of_net.h>
>
>   #include "macb.h"
>
> @@ -191,7 +193,6 @@ static int macb_mii_probe(struct net_device *dev)
>   {
>   	struct macb *bp = netdev_priv(dev);
>   	struct phy_device *phydev;
> -	struct macb_platform_data *pdata;
>   	int ret;
>
>   	phydev = phy_find_first(bp->mii_bus);
> @@ -200,14 +201,11 @@ static int macb_mii_probe(struct net_device *dev)
>   		return -1;
>   	}
>
> -	pdata = bp->pdev->dev.platform_data;
>   	/* TODO : add pin_irq */
>
>   	/* attach the mac to the phy */
>   	ret = phy_connect_direct(dev, phydev,&macb_handle_link_change, 0,
> -				 pdata&&  pdata->is_rmii ?
> -				 PHY_INTERFACE_MODE_RMII :
> -				 PHY_INTERFACE_MODE_MII);
> +				 bp->phy_interface);
>   	if (ret) {
>   		netdev_err(dev, "Could not attach to PHY\n");
>   		return ret;
> @@ -1244,6 +1242,52 @@ static const struct net_device_ops macb_netdev_ops = {
>   #endif
>   };
>
> +#if defined(CONFIG_OF)
> +static const struct of_device_id macb_dt_ids[] = {
> +	{ .compatible = "cdns,at32ap7000-macb" },
> +	{ .compatible = "cdns,at91sam9260-macb" },
> +	{ .compatible = "cdns,macb" },
> +	{ .compatible = "cdns,pc302-gem" },
> +	{ .compatible = "cdns,gem" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, macb_dt_ids);
> +
> +static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	if (np)
> +		return of_get_phy_mode(np);
> +
> +	return -ENODEV;
> +}
> +
> +static int __devinit macb_get_hwaddr_dt(struct macb *bp)
> +{
> +	struct device_node *np = bp->pdev->dev.of_node;
> +	if (np) {
> +		const char *mac = of_get_mac_address(np);
> +		if (mac) {
> +			memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
> +			return 0;
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +#else
> +static int __devinit macb_get_phy_mode_dt(struct platform_device *pdev)
> +{
> +	return -ENODEV;
> +}
> +static int __devinit macb_get_hwaddr_dt(struct macb *bp)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>   static int __init macb_probe(struct platform_device *pdev)
>   {
>   	struct macb_platform_data *pdata;
> @@ -1318,10 +1362,22 @@ static int __init macb_probe(struct platform_device *pdev)
>   	config |= macb_dbw(bp);
>   	macb_writel(bp, NCFGR, config);
>
> -	macb_get_hwaddr(bp);
> -	pdata = pdev->dev.platform_data;
> +	err = macb_get_hwaddr_dt(bp);
> +	if (err<  0)
> +		macb_get_hwaddr(bp);
> +
> +	err = macb_get_phy_mode_dt(pdev);
> +	if (err<  0) {
> +		pdata = pdev->dev.platform_data;
> +		if (pdata&&  pdata->is_rmii)
> +			bp->phy_interface = PHY_INTERFACE_MODE_RMII;
> +		else
> +			bp->phy_interface = PHY_INTERFACE_MODE_MII;
> +	} else {
> +		bp->phy_interface = err;
> +	}
>
> -	if (pdata&&  pdata->is_rmii)
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_RMII)
>   #if defined(CONFIG_ARCH_AT91)
>   		macb_or_gem_writel(bp, USRIO, (MACB_BIT(RMII) |
>   					       MACB_BIT(CLKEN)));
> @@ -1444,6 +1500,7 @@ static struct platform_driver macb_driver = {
>   	.driver		= {
>   		.name		= "macb",
>   		.owner	= THIS_MODULE,
> +		.of_match_table	= of_match_ptr(macb_dt_ids),
>   	},
>   };
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 1931078..335e288 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -532,6 +532,8 @@ struct macb {
>   	unsigned int 		link;
>   	unsigned int 		speed;
>   	unsigned int 		duplex;
> +
> +	phy_interface_t		phy_interface;
>   };
>
>   static inline bool macb_is_gem(struct macb *bp)


-- 
Nicolas Ferre

Re: [PATCH v3 1/2] net/macb: add DT support for Cadence macb/gem driver

[David Miller]

From: Nicolas Ferre <nicolas.ferre@atmel.com>
Date: Mon,  5 Dec 2011 12:59:52 +0100

> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> 
> Allow the device tree to provide the mac address and the phy mode.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> [nicolas.ferre@atmel.com: change "compatible" node property, doc and DT hwaddr]
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> [jamie@jamieiles.com: add "gem" compatibility strings and doc]
> Acked-by: Jamie Iles<jamie@jamieiles.com>

Acked-by: David S. Miller <davem@davemloft.net>