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

login
register
mail settings
Submitter Jean-Christophe PLAGNIOL-VILLARD
Date Nov. 18, 2011, 2:29 p.m.
Message ID <1321626565-11261-1-git-send-email-plagnioj@jcrosoft.com>
Download mbox | patch
Permalink /patch/126422/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jean-Christophe PLAGNIOL-VILLARD - Nov. 18, 2011, 2:29 p.m.
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
Jamie Iles - Nov. 18, 2011, 3:58 p.m.
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
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean-Christophe PLAGNIOL-VILLARD - Nov. 20, 2011, 4:47 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jamie Iles - Nov. 20, 2011, 5:11 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Ferre - Nov. 21, 2011, 10:08 a.m.
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 - Nov. 21, 2011, 11:08 a.m.
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 - Dec. 2, 2011, 3:30 p.m.
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,
Jamie Iles - Dec. 2, 2011, 3:38 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 */