| Message ID | 1308410354-21387-3-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jun 18, 2011 at 11:19:13PM +0800, Shawn Guo wrote: > It adds device tree data parsing support for fec driver. > > Signed-off-by: Jason Liu <jason.hui@linaro.org> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > Cc: David S. Miller <davem@davemloft.net> > --- > Documentation/devicetree/bindings/net/fsl-fec.txt | 14 ++++++++++ > drivers/net/fec.c | 28 +++++++++++++++++++++ > 2 files changed, 42 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt > new file mode 100644 > index 0000000..705111d > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt > @@ -0,0 +1,14 @@ > +* Freescale Fast Ethernet Controller (FEC) > + > +Required properties: > +- compatible : should be "fsl,<soc>-fec", "fsl,fec" Ditto to comment on last patch. "fsl,fec" is to generic. "fsl,imx51-soc" should be the generic value. Otherwise looks okay to me, and I don't see any problem with queueing it up for v3.1 with that change since it doesn't depend on any other patches. Acked-by: Grant Likely <grant.likely@secretlab.ca> > +- reg : address and length of the register set for the device > +- interrupts : should contain fec interrupt > + > +Example: > + > +fec@83fec000 { > + compatible = "fsl,imx51-fec", "fsl,fec"; > + reg = <0x83fec000 0x4000>; > + interrupts = <87>; > +}; > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > index 885d8ba..ef3d175 100644 > --- a/drivers/net/fec.c > +++ b/drivers/net/fec.c > @@ -44,6 +44,8 @@ > #include <linux/platform_device.h> > #include <linux/phy.h> > #include <linux/fec.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > > #include <asm/cacheflush.h> > > @@ -78,6 +80,26 @@ static struct platform_device_id fec_devtype[] = { > { } > }; > > +#ifdef CONFIG_OF > +static const struct of_device_id fec_dt_ids[] = { > + { .compatible = "fsl,fec", .data = &fec_devtype[0], }, > + {}, > +}; > + > +static const struct of_device_id * > +fec_get_of_device_id(struct platform_device *pdev) > +{ > + return of_match_device(fec_dt_ids, &pdev->dev); > +} > +#else > +#define fec_dt_ids NULL > +static inline struct of_device_id * > +fec_get_of_device_id(struct platform_device *pdev) > +{ > + return NULL; > +} > +#endif > + > static unsigned char macaddr[ETH_ALEN]; > module_param_array(macaddr, byte, NULL, 0); > MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev) > struct net_device *ndev; > int i, irq, ret = 0; > struct resource *r; > + const struct of_device_id *of_id; > + > + of_id = fec_get_of_device_id(pdev); > + if (of_id) > + pdev->id_entry = (struct platform_device_id *) of_id->data; > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!r) > @@ -1531,6 +1558,7 @@ static struct platform_driver fec_driver = { > #ifdef CONFIG_PM > .pm = &fec_pm_ops, > #endif > + .of_match_table = fec_dt_ids, > }, > .id_table = fec_devtype, > .probe = fec_probe, > -- > 1.7.4.1 > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss
On Saturday 18 June 2011 17:19:13 Shawn Guo wrote: > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt > new file mode 100644 > index 0000000..705111d > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt > @@ -0,0 +1,14 @@ > +* Freescale Fast Ethernet Controller (FEC) > + > +Required properties: > +- compatible : should be "fsl,<soc>-fec", "fsl,fec" > +- reg : address and length of the register set for the device > +- interrupts : should contain fec interrupt > + > +Example: > + > +fec@83fec000 { > + compatible = "fsl,imx51-fec", "fsl,fec"; > + reg = <0x83fec000 0x4000>; > + interrupts = <87>; > +}; How about also adding device_type="network" as required here, so you inherit the attributes like "local-mac-address". I would also suggest adding a call to of_get_mac_address() so you can read the address out of the device tree when it is not configured in hardware. Today, the driver relies on a module parameter or platform_data on hardware with a mac address set. The other information that is currently encoded in platform_data is the phy mode. How about adding a property that enables RMII mode when present? Arnd
On Sat, Jun 18, 2011 at 12:27 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Saturday 18 June 2011 17:19:13 Shawn Guo wrote: >> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt >> new file mode 100644 >> index 0000000..705111d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt >> @@ -0,0 +1,14 @@ >> +* Freescale Fast Ethernet Controller (FEC) >> + >> +Required properties: >> +- compatible : should be "fsl,<soc>-fec", "fsl,fec" >> +- reg : address and length of the register set for the device >> +- interrupts : should contain fec interrupt >> + >> +Example: >> + >> +fec@83fec000 { >> + compatible = "fsl,imx51-fec", "fsl,fec"; >> + reg = <0x83fec000 0x4000>; >> + interrupts = <87>; >> +}; > > How about also adding device_type="network" as required here, so you > inherit the attributes like "local-mac-address". local-mac-address should be used regardless. "device_type" only makes sense when a platform uses real OpenFirmware with the runtime services api. It should not be used with the flat tree. > I would also suggest adding a call to of_get_mac_address() so you > can read the address out of the device tree when it is not configured > in hardware. Today, the driver relies on a module parameter or > platform_data on hardware with a mac address set. Yes, of_get_mac_address() is the right thing to do. g.
On Sat, Jun 18, 2011 at 10:22:20AM -0600, Grant Likely wrote: > On Sat, Jun 18, 2011 at 11:19:13PM +0800, Shawn Guo wrote: > > It adds device tree data parsing support for fec driver. > > > > Signed-off-by: Jason Liu <jason.hui@linaro.org> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > Cc: David S. Miller <davem@davemloft.net> > > --- > > Documentation/devicetree/bindings/net/fsl-fec.txt | 14 ++++++++++ > > drivers/net/fec.c | 28 +++++++++++++++++++++ > > 2 files changed, 42 insertions(+), 0 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt > > > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt > > new file mode 100644 > > index 0000000..705111d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt > > @@ -0,0 +1,14 @@ > > +* Freescale Fast Ethernet Controller (FEC) > > + > > +Required properties: > > +- compatible : should be "fsl,<soc>-fec", "fsl,fec" > > Ditto to comment on last patch. "fsl,fec" is to generic. > "fsl,imx51-soc" should be the generic value. > Ditto to the feedback on the last comment. "fsl,imx51-fec" is not a good one to be the compatibility string for imx27 and imx35 fec. > Otherwise looks okay to me, and I don't see any problem with queueing > it up for v3.1 with that change since it doesn't depend on any other > patches. > > Acked-by: Grant Likely <grant.likely@secretlab.ca> >
On Sat, Jun 18, 2011 at 08:27:22PM +0200, Arnd Bergmann wrote: > On Saturday 18 June 2011 17:19:13 Shawn Guo wrote: > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt > > new file mode 100644 > > index 0000000..705111d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt > > @@ -0,0 +1,14 @@ > > +* Freescale Fast Ethernet Controller (FEC) > > + > > +Required properties: > > +- compatible : should be "fsl,<soc>-fec", "fsl,fec" > > +- reg : address and length of the register set for the device > > +- interrupts : should contain fec interrupt > > + > > +Example: > > + > > +fec@83fec000 { > > + compatible = "fsl,imx51-fec", "fsl,fec"; > > + reg = <0x83fec000 0x4000>; > > + interrupts = <87>; > > +}; > > How about also adding device_type="network" as required here, so you > inherit the attributes like "local-mac-address". > > I would also suggest adding a call to of_get_mac_address() so you > can read the address out of the device tree when it is not configured > in hardware. Today, the driver relies on a module parameter or > platform_data on hardware with a mac address set. > > The other information that is currently encoded in platform_data > is the phy mode. How about adding a property that enables RMII mode > when present? > Ah, yes. I missed that. Will add support for local-mac-address and phy-mode. Thanks, Arnd.
Hi Shawn, On 06/19/2011 01:19 AM, Shawn Guo wrote: > It adds device tree data parsing support for fec driver. > > Signed-off-by: Jason Liu<jason.hui@linaro.org> > Signed-off-by: Shawn Guo<shawn.guo@linaro.org> > Cc: David S. Miller<davem@davemloft.net> > --- > Documentation/devicetree/bindings/net/fsl-fec.txt | 14 ++++++++++ > drivers/net/fec.c | 28 +++++++++++++++++++++ > 2 files changed, 42 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt > new file mode 100644 > index 0000000..705111d > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt > @@ -0,0 +1,14 @@ > +* Freescale Fast Ethernet Controller (FEC) > + > +Required properties: > +- compatible : should be "fsl,<soc>-fec", "fsl,fec" > +- reg : address and length of the register set for the device > +- interrupts : should contain fec interrupt > + > +Example: > + > +fec@83fec000 { > + compatible = "fsl,imx51-fec", "fsl,fec"; > + reg =<0x83fec000 0x4000>; > + interrupts =<87>; > +}; > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > index 885d8ba..ef3d175 100644 > --- a/drivers/net/fec.c > +++ b/drivers/net/fec.c > @@ -44,6 +44,8 @@ > #include<linux/platform_device.h> > #include<linux/phy.h> > #include<linux/fec.h> > +#include<linux/of.h> > +#include<linux/of_device.h> > > #include<asm/cacheflush.h> > > @@ -78,6 +80,26 @@ static struct platform_device_id fec_devtype[] = { > { } > }; > > +#ifdef CONFIG_OF > +static const struct of_device_id fec_dt_ids[] = { > + { .compatible = "fsl,fec", .data =&fec_devtype[0], }, > + {}, > +}; > + > +static const struct of_device_id * > +fec_get_of_device_id(struct platform_device *pdev) > +{ > + return of_match_device(fec_dt_ids,&pdev->dev); > +} > +#else > +#define fec_dt_ids NULL > +static inline struct of_device_id * > +fec_get_of_device_id(struct platform_device *pdev) > +{ > + return NULL; > +} > +#endif > + > static unsigned char macaddr[ETH_ALEN]; > module_param_array(macaddr, byte, NULL, 0); > MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev) > struct net_device *ndev; > int i, irq, ret = 0; > struct resource *r; > + const struct of_device_id *of_id; > + > + of_id = fec_get_of_device_id(pdev); fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This use of it will break compilation when this is not defined. Regards Greg > + if (of_id) > + pdev->id_entry = (struct platform_device_id *) of_id->data; > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!r) > @@ -1531,6 +1558,7 @@ static struct platform_driver fec_driver = { > #ifdef CONFIG_PM > .pm =&fec_pm_ops, > #endif > + .of_match_table = fec_dt_ids, > }, > .id_table = fec_devtype, > .probe = fec_probe,
On Sunday 19 June 2011 13:39:32 Greg Ungerer wrote: > > +#ifdef CONFIG_OF > > +static const struct of_device_id fec_dt_ids[] = { > > + { .compatible = "fsl,fec", .data =&fec_devtype[0], }, > > + {}, > > +}; > > + > > +static const struct of_device_id * > > +fec_get_of_device_id(struct platform_device *pdev) > > +{ > > + return of_match_device(fec_dt_ids,&pdev->dev); > > +} > > +#else > > +#define fec_dt_ids NULL > > +static inline struct of_device_id * > > +fec_get_of_device_id(struct platform_device *pdev) > > +{ > > + return NULL; > > +} > > +#endif > > + > > static unsigned char macaddr[ETH_ALEN]; > > module_param_array(macaddr, byte, NULL, 0); > > MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > > @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev) > > struct net_device *ndev; > > int i, irq, ret = 0; > > struct resource *r; > > + const struct of_device_id *of_id; > > + > > + of_id = fec_get_of_device_id(pdev); > > fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This > use of it will break compilation when this is not defined. > Why? Note the #else path defining an empty function. Arnd
On 06/19/2011 07:11 AM, Arnd Bergmann wrote: > On Sunday 19 June 2011 13:39:32 Greg Ungerer wrote: >>> +#ifdef CONFIG_OF >>> +static const struct of_device_id fec_dt_ids[] = { >>> + { .compatible = "fsl,fec", .data =&fec_devtype[0], }, >>> + {}, >>> +}; >>> + >>> +static const struct of_device_id * >>> +fec_get_of_device_id(struct platform_device *pdev) >>> +{ >>> + return of_match_device(fec_dt_ids,&pdev->dev); >>> +} >>> +#else >>> +#define fec_dt_ids NULL >>> +static inline struct of_device_id * >>> +fec_get_of_device_id(struct platform_device *pdev) >>> +{ >>> + return NULL; >>> +} >>> +#endif >>> + >>> static unsigned char macaddr[ETH_ALEN]; >>> module_param_array(macaddr, byte, NULL, 0); >>> MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); >>> @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev) >>> struct net_device *ndev; >>> int i, irq, ret = 0; >>> struct resource *r; >>> + const struct of_device_id *of_id; >>> + >>> + of_id = fec_get_of_device_id(pdev); >> >> fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This >> use of it will break compilation when this is not defined. >> > > > Why? Note the #else path defining an empty function. > None of this is necessary in the first place. Just call of_match_device directly. There is already an empty function to return NULL when CONFIG_OF is not defined. Rob
On Sun, Jun 19, 2011 at 9:09 AM, Rob Herring <robherring2@gmail.com> wrote: > On 06/19/2011 07:11 AM, Arnd Bergmann wrote: >> On Sunday 19 June 2011 13:39:32 Greg Ungerer wrote: >>>> +#ifdef CONFIG_OF >>>> +static const struct of_device_id fec_dt_ids[] = { >>>> + { .compatible = "fsl,fec", .data =&fec_devtype[0], }, >>>> + {}, >>>> +}; >>>> + >>>> +static const struct of_device_id * >>>> +fec_get_of_device_id(struct platform_device *pdev) >>>> +{ >>>> + return of_match_device(fec_dt_ids,&pdev->dev); >>>> +} >>>> +#else >>>> +#define fec_dt_ids NULL >>>> +static inline struct of_device_id * >>>> +fec_get_of_device_id(struct platform_device *pdev) >>>> +{ >>>> + return NULL; >>>> +} >>>> +#endif >>>> + >>>> static unsigned char macaddr[ETH_ALEN]; >>>> module_param_array(macaddr, byte, NULL, 0); >>>> MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); >>>> @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev) >>>> struct net_device *ndev; >>>> int i, irq, ret = 0; >>>> struct resource *r; >>>> + const struct of_device_id *of_id; >>>> + >>>> + of_id = fec_get_of_device_id(pdev); >>> >>> fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This >>> use of it will break compilation when this is not defined. >>> >> >> >> Why? Note the #else path defining an empty function. >> > > None of this is necessary in the first place. Just call of_match_device > directly. There is already an empty function to return NULL when > CONFIG_OF is not defined. Heh, right you are. I should have caught on to that. :-) g.
On 19/06/11 22:11, Arnd Bergmann wrote: > On Sunday 19 June 2011 13:39:32 Greg Ungerer wrote: >>> +#ifdef CONFIG_OF >>> +static const struct of_device_id fec_dt_ids[] = { >>> + { .compatible = "fsl,fec", .data =&fec_devtype[0], }, >>> + {}, >>> +}; >>> + >>> +static const struct of_device_id * >>> +fec_get_of_device_id(struct platform_device *pdev) >>> +{ >>> + return of_match_device(fec_dt_ids,&pdev->dev); >>> +} >>> +#else >>> +#define fec_dt_ids NULL >>> +static inline struct of_device_id * >>> +fec_get_of_device_id(struct platform_device *pdev) >>> +{ >>> + return NULL; >>> +} >>> +#endif >>> + >>> static unsigned char macaddr[ETH_ALEN]; >>> module_param_array(macaddr, byte, NULL, 0); >>> MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); >>> @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev) >>> struct net_device *ndev; >>> int i, irq, ret = 0; >>> struct resource *r; >>> + const struct of_device_id *of_id; >>> + >>> + of_id = fec_get_of_device_id(pdev); >> >> fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This >> use of it will break compilation when this is not defined. >> > > > Why? Note the #else path defining an empty function. Sorry, missed that :-) Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt new file mode 100644 index 0000000..705111d --- /dev/null +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt @@ -0,0 +1,14 @@ +* Freescale Fast Ethernet Controller (FEC) + +Required properties: +- compatible : should be "fsl,<soc>-fec", "fsl,fec" +- reg : address and length of the register set for the device +- interrupts : should contain fec interrupt + +Example: + +fec@83fec000 { + compatible = "fsl,imx51-fec", "fsl,fec"; + reg = <0x83fec000 0x4000>; + interrupts = <87>; +}; diff --git a/drivers/net/fec.c b/drivers/net/fec.c index 885d8ba..ef3d175 100644 --- a/drivers/net/fec.c +++ b/drivers/net/fec.c @@ -44,6 +44,8 @@ #include <linux/platform_device.h> #include <linux/phy.h> #include <linux/fec.h> +#include <linux/of.h> +#include <linux/of_device.h> #include <asm/cacheflush.h> @@ -78,6 +80,26 @@ static struct platform_device_id fec_devtype[] = { { } }; +#ifdef CONFIG_OF +static const struct of_device_id fec_dt_ids[] = { + { .compatible = "fsl,fec", .data = &fec_devtype[0], }, + {}, +}; + +static const struct of_device_id * +fec_get_of_device_id(struct platform_device *pdev) +{ + return of_match_device(fec_dt_ids, &pdev->dev); +} +#else +#define fec_dt_ids NULL +static inline struct of_device_id * +fec_get_of_device_id(struct platform_device *pdev) +{ + return NULL; +} +#endif + static unsigned char macaddr[ETH_ALEN]; module_param_array(macaddr, byte, NULL, 0); MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev) struct net_device *ndev; int i, irq, ret = 0; struct resource *r; + const struct of_device_id *of_id; + + of_id = fec_get_of_device_id(pdev); + if (of_id) + pdev->id_entry = (struct platform_device_id *) of_id->data; r = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!r) @@ -1531,6 +1558,7 @@ static struct platform_driver fec_driver = { #ifdef CONFIG_PM .pm = &fec_pm_ops, #endif + .of_match_table = fec_dt_ids, }, .id_table = fec_devtype, .probe = fec_probe,