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

[Jamie Iles <jamie@jamieiles.com>]

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