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 <[email protected]>
> > Cc: Jamie Iles <[email protected]>
> > Cc: Nicolas Ferre <[email protected]>
>
> 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.
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
