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

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:

quoted

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

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <redacted>
Cc: Jamie Iles <redacted>
Cc: Nicolas Ferre <redacted>

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

quoted

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

quoted

+
+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

quoted

+		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

quoted

+		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.