| Message ID | 1321626565-11261-1-git-send-email-plagnioj@jcrosoft.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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,
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 */
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,
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
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 */
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