| Message ID | 20110704055021.GC10245@S2100-06.ap.freescale.net |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote: > On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote: > > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote: > > > It adds device tree probe 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> > > > Cc: Grant Likely <grant.likely@secretlab.ca> > > > > Minor comments below. After addressing them you can add my: > > > > Acked-by: Grant Likely <grant.likely@secretlab.ca> > > > > I don't see any reason not to merge this one in v3.1 > > > > g. > > > > > --- > > > Documentation/devicetree/bindings/net/fsl-fec.txt | 24 ++++ > > > drivers/net/fec.c | 120 ++++++++++++++++++++- > > > 2 files changed, 139 insertions(+), 5 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..1dad888 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt > > > @@ -0,0 +1,24 @@ > > > +* Freescale Fast Ethernet Controller (FEC) > > > + > > > +Required properties: > > > +- compatible : Should be "fsl,<soc>-fec" > > > +- reg : Address and length of the register set for the device > > > +- interrupts : Should contain fec interrupt > > > +- phy-mode : String, operation mode of the PHY interface. > > > + Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii", > > > + "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi". > > > > We don't have a common binding for this yet. Should be > > "fsl,phy-mode". > > > There is nothing really fsl specific. That's not really the point. It might be that phy-mode as you're defining it here will not be sufficient for other ethernet devices. You should avoid creating 'generic' property definitions, unless you're doing the leg work of figuring out which devices would use this property and creating multiple users. So, you may be right that this isn't fsl specific, but it is still good practise to use that "fsl," prefix. > How does thn following patch > look to you? > > ---8<--------- > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c > index 86f334a..cc117db 100644 > --- a/drivers/of/of_net.c > +++ b/drivers/of/of_net.c > @@ -8,6 +8,49 @@ > #include <linux/etherdevice.h> > #include <linux/kernel.h> > #include <linux/of_net.h> > +#include <linux/phy.h> > + > +/** > + * It maps 'enum phy_interface_t' found in include/linux/phy.h > + * into the device tree binding of 'phy-mode', so that Ethernet > + * device driver can get phy interface from device tree. > + */ > +static const char *phy_modes[] = { > + [PHY_INTERFACE_MODE_MII] = "mii", > + [PHY_INTERFACE_MODE_GMII] = "gmii", > + [PHY_INTERFACE_MODE_SGMII] = "sgmii", > + [PHY_INTERFACE_MODE_TBI] = "tbi", > + [PHY_INTERFACE_MODE_RMII] = "rmii", > + [PHY_INTERFACE_MODE_RGMII] = "rgmii", > + [PHY_INTERFACE_MODE_RGMII_ID] = "rgmii-id", > + [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid", > + [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid", > + [PHY_INTERFACE_MODE_RTBI] = "rtbi", > +}; > + > +/** > + * of_get_phy_mode - Get phy mode for given device_node > + * @np: Pointer to the given device_node > + * > + * The function gets phy interface string from property 'phy-mode', > + * and return its index in phy_modes table, or errno in error case. > + */ > +const int of_get_phy_mode(struct device_node *np) > +{ > + const char *pm; > + int err, i; > + > + err = of_property_read_string(np, "phy-mode", &pm); > + if (err < 0) > + return err; > + > + for (i = 0; i < ARRAY_SIZE(phy_modes); i++) > + if (!strcasecmp(pm, phy_modes[i])) > + return i; > + > + return -ENODEV; > +} > +EXPORT_SYMBOL_GPL(of_get_phy_mode); The code looks fine, but I'm just not keen on this until I see multiple users. What are the other DT-aware network drivers doing right now? > > /** > * Search the device tree for the best MAC address to use. 'mac-address' is > ------------ > > [...] > > > @@ -734,8 +748,25 @@ static void __inline__ fec_get_mac(struct net_device *ndev) > > > */ > > > iap = macaddr; > > > > > > +#ifdef CONFIG_OF > > > + /* > > > + * 2) from device tree data > > > + */ > > > + if (!is_valid_ether_addr(iap)) { > > > + struct device_node *np = fep->pdev->dev.of_node; > > > + if (np) { > > > + const char *mac; > > > + int err; > > > + err = of_property_read_string(np, > > > + "local-mac-address", &mac); > > > + if (err) > > > + iap = (unsigned char *) mac; > > > > There is already a function for doing this. of_get_mac_address(). > > And mac address is *not* a string, it is a byte array. > > > Yes, I mistakenly converted of_find_property to of_property_read_string. > Anyway, I'm now using of_get_mac_address as you suggested. :) g.
On Sun, Jul 03, 2011 at 11:59:56PM -0600, Grant Likely wrote: > On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote: > > On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote: > > > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote: > > > > It adds device tree probe 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> > > > > Cc: Grant Likely <grant.likely@secretlab.ca> > > > > > > Minor comments below. After addressing them you can add my: > > > > > > Acked-by: Grant Likely <grant.likely@secretlab.ca> > > > > > > I don't see any reason not to merge this one in v3.1 > > > > > > g. > > > > > > > --- > > > > Documentation/devicetree/bindings/net/fsl-fec.txt | 24 ++++ > > > > drivers/net/fec.c | 120 ++++++++++++++++++++- > > > > 2 files changed, 139 insertions(+), 5 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..1dad888 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt > > > > @@ -0,0 +1,24 @@ > > > > +* Freescale Fast Ethernet Controller (FEC) > > > > + > > > > +Required properties: > > > > +- compatible : Should be "fsl,<soc>-fec" > > > > +- reg : Address and length of the register set for the device > > > > +- interrupts : Should contain fec interrupt > > > > +- phy-mode : String, operation mode of the PHY interface. > > > > + Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii", > > > > + "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi". > > > > > > We don't have a common binding for this yet. Should be > > > "fsl,phy-mode". > > > > > There is nothing really fsl specific. > > That's not really the point. It might be that phy-mode as you're > defining it here will not be sufficient for other ethernet devices. I'm not defining the phy mode. Instead, it's just a direct mapping of 'enum phy_interface_t' found in include/linux/phy.h, which is something common and generic, I think? > You should avoid creating 'generic' property definitions, unless > you're doing the leg work of figuring out which devices would use this > property and creating multiple users. > Any net device driver currently referring to 'enum phy_interface_t' is going to need this property when migrating to dt, I guess. > So, you may be right that this isn't fsl specific, but it is still > good practise to use that "fsl," prefix. > > > > How does thn following patch > > look to you? > > > > ---8<--------- > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c > > index 86f334a..cc117db 100644 > > --- a/drivers/of/of_net.c > > +++ b/drivers/of/of_net.c > > @@ -8,6 +8,49 @@ > > #include <linux/etherdevice.h> > > #include <linux/kernel.h> > > #include <linux/of_net.h> > > +#include <linux/phy.h> > > + > > +/** > > + * It maps 'enum phy_interface_t' found in include/linux/phy.h > > + * into the device tree binding of 'phy-mode', so that Ethernet > > + * device driver can get phy interface from device tree. > > + */ > > +static const char *phy_modes[] = { > > + [PHY_INTERFACE_MODE_MII] = "mii", > > + [PHY_INTERFACE_MODE_GMII] = "gmii", > > + [PHY_INTERFACE_MODE_SGMII] = "sgmii", > > + [PHY_INTERFACE_MODE_TBI] = "tbi", > > + [PHY_INTERFACE_MODE_RMII] = "rmii", > > + [PHY_INTERFACE_MODE_RGMII] = "rgmii", > > + [PHY_INTERFACE_MODE_RGMII_ID] = "rgmii-id", > > + [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid", > > + [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid", > > + [PHY_INTERFACE_MODE_RTBI] = "rtbi", > > +}; > > + > > +/** > > + * of_get_phy_mode - Get phy mode for given device_node > > + * @np: Pointer to the given device_node > > + * > > + * The function gets phy interface string from property 'phy-mode', > > + * and return its index in phy_modes table, or errno in error case. > > + */ > > +const int of_get_phy_mode(struct device_node *np) > > +{ > > + const char *pm; > > + int err, i; > > + > > + err = of_property_read_string(np, "phy-mode", &pm); > > + if (err < 0) > > + return err; > > + > > + for (i = 0; i < ARRAY_SIZE(phy_modes); i++) > > + if (!strcasecmp(pm, phy_modes[i])) > > + return i; > > + > > + return -ENODEV; > > +} > > +EXPORT_SYMBOL_GPL(of_get_phy_mode); > > The code looks fine, but I'm just not keen on this until I see > multiple users. What are the other DT-aware network drivers doing > right now? > You can find a bunch of examples using 'phy-mode' in this way with the grep below. $ grep phy-mode arch/powerpc/boot/dts/* The code I can find is drivers/net/ibm_newemac/core.c, function emac_init_config. And I think it should use 'enum phy_interface_t' than defining its own PHY_MODE_*.
On Mon, Jul 04, 2011 at 02:25:22PM +0800, Shawn Guo wrote: > On Sun, Jul 03, 2011 at 11:59:56PM -0600, Grant Likely wrote: > > On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote: > > > On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote: > > > > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote: > > > > > It adds device tree probe 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> > > > > > Cc: Grant Likely <grant.likely@secretlab.ca> > > > > > > > > Minor comments below. After addressing them you can add my: > > > > > > > > Acked-by: Grant Likely <grant.likely@secretlab.ca> > > > > > > > > I don't see any reason not to merge this one in v3.1 > > > > > > > > g. > > > > > > > > > --- > > > > > Documentation/devicetree/bindings/net/fsl-fec.txt | 24 ++++ > > > > > drivers/net/fec.c | 120 ++++++++++++++++++++- > > > > > 2 files changed, 139 insertions(+), 5 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..1dad888 > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt > > > > > @@ -0,0 +1,24 @@ > > > > > +* Freescale Fast Ethernet Controller (FEC) > > > > > + > > > > > +Required properties: > > > > > +- compatible : Should be "fsl,<soc>-fec" > > > > > +- reg : Address and length of the register set for the device > > > > > +- interrupts : Should contain fec interrupt > > > > > +- phy-mode : String, operation mode of the PHY interface. > > > > > + Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii", > > > > > + "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi". > > > > > > > > We don't have a common binding for this yet. Should be > > > > "fsl,phy-mode". > > > > > > > There is nothing really fsl specific. > > > > That's not really the point. It might be that phy-mode as you're > > defining it here will not be sufficient for other ethernet devices. > > I'm not defining the phy mode. Instead, it's just a direct mapping > of 'enum phy_interface_t' found in include/linux/phy.h, which is > something common and generic, I think? > > > You should avoid creating 'generic' property definitions, unless > > you're doing the leg work of figuring out which devices would use this > > property and creating multiple users. > > > > Any net device driver currently referring to 'enum phy_interface_t' > is going to need this property when migrating to dt, I guess. > > > So, you may be right that this isn't fsl specific, but it is still > > good practise to use that "fsl," prefix. > > > > > > > How does thn following patch > > > look to you? > > > > > > ---8<--------- > > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c > > > index 86f334a..cc117db 100644 > > > --- a/drivers/of/of_net.c > > > +++ b/drivers/of/of_net.c > > > @@ -8,6 +8,49 @@ > > > #include <linux/etherdevice.h> > > > #include <linux/kernel.h> > > > #include <linux/of_net.h> > > > +#include <linux/phy.h> > > > + > > > +/** > > > + * It maps 'enum phy_interface_t' found in include/linux/phy.h > > > + * into the device tree binding of 'phy-mode', so that Ethernet > > > + * device driver can get phy interface from device tree. > > > + */ > > > +static const char *phy_modes[] = { > > > + [PHY_INTERFACE_MODE_MII] = "mii", > > > + [PHY_INTERFACE_MODE_GMII] = "gmii", > > > + [PHY_INTERFACE_MODE_SGMII] = "sgmii", > > > + [PHY_INTERFACE_MODE_TBI] = "tbi", > > > + [PHY_INTERFACE_MODE_RMII] = "rmii", > > > + [PHY_INTERFACE_MODE_RGMII] = "rgmii", > > > + [PHY_INTERFACE_MODE_RGMII_ID] = "rgmii-id", > > > + [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid", > > > + [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid", > > > + [PHY_INTERFACE_MODE_RTBI] = "rtbi", > > > +}; > > > + > > > +/** > > > + * of_get_phy_mode - Get phy mode for given device_node > > > + * @np: Pointer to the given device_node > > > + * > > > + * The function gets phy interface string from property 'phy-mode', > > > + * and return its index in phy_modes table, or errno in error case. > > > + */ > > > +const int of_get_phy_mode(struct device_node *np) > > > +{ > > > + const char *pm; > > > + int err, i; > > > + > > > + err = of_property_read_string(np, "phy-mode", &pm); > > > + if (err < 0) > > > + return err; > > > + > > > + for (i = 0; i < ARRAY_SIZE(phy_modes); i++) > > > + if (!strcasecmp(pm, phy_modes[i])) > > > + return i; > > > + > > > + return -ENODEV; > > > +} > > > +EXPORT_SYMBOL_GPL(of_get_phy_mode); > > > > The code looks fine, but I'm just not keen on this until I see > > multiple users. What are the other DT-aware network drivers doing > > right now? > > > You can find a bunch of examples using 'phy-mode' in this way with > the grep below. > > $ grep phy-mode arch/powerpc/boot/dts/* > > The code I can find is drivers/net/ibm_newemac/core.c, function > emac_init_config. And I think it should use 'enum phy_interface_t' > than defining its own PHY_MODE_*. Fair enough. Go ahead and create the common definition. Can you craft a patch to convert the ibm_newemac driver to use your common function too? g.
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c index 86f334a..cc117db 100644 --- a/drivers/of/of_net.c +++ b/drivers/of/of_net.c @@ -8,6 +8,49 @@ #include <linux/etherdevice.h> #include <linux/kernel.h> #include <linux/of_net.h> +#include <linux/phy.h> + +/** + * It maps 'enum phy_interface_t' found in include/linux/phy.h + * into the device tree binding of 'phy-mode', so that Ethernet + * device driver can get phy interface from device tree. + */ +static const char *phy_modes[] = { + [PHY_INTERFACE_MODE_MII] = "mii", + [PHY_INTERFACE_MODE_GMII] = "gmii", + [PHY_INTERFACE_MODE_SGMII] = "sgmii", + [PHY_INTERFACE_MODE_TBI] = "tbi", + [PHY_INTERFACE_MODE_RMII] = "rmii", + [PHY_INTERFACE_MODE_RGMII] = "rgmii", + [PHY_INTERFACE_MODE_RGMII_ID] = "rgmii-id", + [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid", + [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid", + [PHY_INTERFACE_MODE_RTBI] = "rtbi", +}; + +/** + * of_get_phy_mode - Get phy mode for given device_node + * @np: Pointer to the given device_node + * + * The function gets phy interface string from property 'phy-mode', + * and return its index in phy_modes table, or errno in error case. + */ +const int of_get_phy_mode(struct device_node *np) +{ + const char *pm; + int err, i; + + err = of_property_read_string(np, "phy-mode", &pm); + if (err < 0) + return err; + + for (i = 0; i < ARRAY_SIZE(phy_modes); i++) + if (!strcasecmp(pm, phy_modes[i])) + return i; + + return -ENODEV; +} +EXPORT_SYMBOL_GPL(of_get_phy_mode); /** * Search the device tree for the best MAC address to use. 'mac-address' is