| Message ID | 1360803091-26400-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Iwamatsu-san Thank you for this patch. Small comment from me > +#ifdef CONFIG_OF (snip) > +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) > +{ > + int ret; > + struct device_node *np = dev->of_node; > + struct sh_eth_plat_data *pdata; ... > +#else > +static struct sh_eth_plat_data * > +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) > +{ > + return NULL; > +} > +#endif (snip) > static int sh_eth_drv_probe(struct platform_device *pdev) > { ... > +#ifdef CONFIG_OF > + if (np && of_device_is_available(np)) { > + pd = sh_eth_parse_dt(&pdev->dev, ndev); > + if (pdev->dev.platform_data) { > + struct sh_eth_plat_data *tmp = > + pdev->dev.platform_data; > + pd->set_mdio_gate = tmp->set_mdio_gate; > + pd->needs_init = tmp->needs_init; > + } > + } else > +#endif sh_eth_parse_dt() was defined for both CONFIG_OF and !CONFIG_OF. But it is called only from CONFIG_OF ? Best regards --- Kuninori Morimoto -- 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
Hi Iwamatsu-san, On Thu, Feb 14, 2013 at 9:51 AM, Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> wrote: > This adds support of device tree probe for Renesas sh-ether driver. > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > --- > V2: - Removed ether_setup(). > - Fixed typo from "sh-etn" to "sh-eth". > - Removed "needs-init" and sh-eth,endian from definition of DT. > - Changed "sh-eth,edmac-endian" instead of "sh-eth,edmac-big-endain" > in definition of DT. > > Documentation/devicetree/bindings/net/sh_ether.txt | 41 +++++ > drivers/net/ethernet/renesas/sh_eth.c | 156 +++++++++++++++++--- > 2 files changed, 173 insertions(+), 24 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/sh_ether.txt > > diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt > new file mode 100644 > index 0000000..7415e54 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/sh_ether.txt > @@ -0,0 +1,41 @@ > +* Renesas Electronics SuperH EMAC > + > +This file provides information, what the device node > +for the sh_eth interface contains. > + > +Required properties: > +- compatible: "renesas,sh-eth"; > +- interrupt-parent: The phandle for the interrupt controller that > + services interrupts for this device. > +- reg: Offset and length of the register set for the > + device. > +- interrupts: Interrupt mapping for the sh_eth interrupt > + sources (vector id). > +- phy-mode: String, operation mode of the PHY interface. > +- sh-eth,register-type: String, register type of sh_eth. > + Please select "gigabit", "fast-sh4" or > + "fast-sh3-sh2". > +- sh-eth,phy-id: PHY id. > + > +Optional properties: > +- local-mac-address: 6 bytes, mac address > +- sh-eth,no-ether-link: Set link control by software. When device does > + not support ether-link, set. > +- sh-eth,ether-link-active-low: Set link check method. > + When detection of link is treated as active-low, > + set. > +- sh-eth,edmac-big-endian: Set big endian for sh_eth dmac. > + It work as little endian when this is not set. > +- sh-etn,needs-init: Initialization flag. > + When initialize device in probing device, set. I believe the above still says "sh-etn". Also, it is unclear why you want this flag at all since it is not used to describe the hardware. Thanks, / magnus -- 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
(2013/02/14 10:24), Kuninori Morimoto wrote: > > Hi Iwamatsu-san > > Thank you for this patch. > > Small comment from me > >> +#ifdef CONFIG_OF > (snip) >> +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) >> +{ >> + int ret; >> + struct device_node *np = dev->of_node; >> + struct sh_eth_plat_data *pdata; > ... >> +#else >> +static struct sh_eth_plat_data * >> +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) >> +{ >> + return NULL; >> +} >> +#endif > > (snip) > >> static int sh_eth_drv_probe(struct platform_device *pdev) >> { > ... >> +#ifdef CONFIG_OF >> + if (np&& of_device_is_available(np)) { >> + pd = sh_eth_parse_dt(&pdev->dev, ndev); >> + if (pdev->dev.platform_data) { >> + struct sh_eth_plat_data *tmp = >> + pdev->dev.platform_data; >> + pd->set_mdio_gate = tmp->set_mdio_gate; >> + pd->needs_init = tmp->needs_init; >> + } >> + } else >> +#endif > > sh_eth_parse_dt() was defined for both CONFIG_OF and !CONFIG_OF. > But it is called only from CONFIG_OF ? > Because of_device_is_available needs CONFIG_OF. I already send a patch which add empty function of of_device_is_available. If this was apply, this ifdef becomes without need. Best, Nobuhiro -- 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 Thu, Feb 14, 2013 at 10:47 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > Hi Iwamatsu-san, > > On Thu, Feb 14, 2013 at 9:51 AM, Nobuhiro Iwamatsu > <nobuhiro.iwamatsu.yj@renesas.com> wrote: >> This adds support of device tree probe for Renesas sh-ether driver. >> >> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> >> >> --- >> V2: - Removed ether_setup(). >> - Fixed typo from "sh-etn" to "sh-eth". >> - Removed "needs-init" and sh-eth,endian from definition of DT. >> - Changed "sh-eth,edmac-endian" instead of "sh-eth,edmac-big-endain" >> in definition of DT. >> >> Documentation/devicetree/bindings/net/sh_ether.txt | 41 +++++ >> drivers/net/ethernet/renesas/sh_eth.c | 156 +++++++++++++++++--- >> 2 files changed, 173 insertions(+), 24 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/net/sh_ether.txt >> >> diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt >> new file mode 100644 >> index 0000000..7415e54 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/sh_ether.txt >> @@ -0,0 +1,41 @@ >> +* Renesas Electronics SuperH EMAC >> + >> +This file provides information, what the device node >> +for the sh_eth interface contains. >> + >> +Required properties: >> +- compatible: "renesas,sh-eth"; >> +- interrupt-parent: The phandle for the interrupt controller that >> + services interrupts for this device. >> +- reg: Offset and length of the register set for the >> + device. >> +- interrupts: Interrupt mapping for the sh_eth interrupt >> + sources (vector id). >> +- phy-mode: String, operation mode of the PHY interface. >> +- sh-eth,register-type: String, register type of sh_eth. >> + Please select "gigabit", "fast-sh4" or >> + "fast-sh3-sh2". >> +- sh-eth,phy-id: PHY id. >> + >> +Optional properties: >> +- local-mac-address: 6 bytes, mac address >> +- sh-eth,no-ether-link: Set link control by software. When device does >> + not support ether-link, set. >> +- sh-eth,ether-link-active-low: Set link check method. >> + When detection of link is treated as active-low, >> + set. >> +- sh-eth,edmac-big-endian: Set big endian for sh_eth dmac. >> + It work as little endian when this is not set. >> +- sh-etn,needs-init: Initialization flag. >> + When initialize device in probing device, set. > > I believe the above still says "sh-etn". Also, it is unclear why you > want this flag at all since it is not used to describe the hardware. > yes, I forgot to delete this. I will fix. regards, Nobuhiro
On Thu, Feb 14, 2013 at 10:24 AM, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > Hi Iwamatsu-san > > Thank you for this patch. > > Small comment from me > >> +#ifdef CONFIG_OF > (snip) >> +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) >> +{ >> + int ret; >> + struct device_node *np = dev->of_node; >> + struct sh_eth_plat_data *pdata; > ... >> +#else >> +static struct sh_eth_plat_data * >> +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) >> +{ >> + return NULL; >> +} >> +#endif > > (snip) > >> static int sh_eth_drv_probe(struct platform_device *pdev) >> { > ... >> +#ifdef CONFIG_OF >> + if (np && of_device_is_available(np)) { >> + pd = sh_eth_parse_dt(&pdev->dev, ndev); >> + if (pdev->dev.platform_data) { >> + struct sh_eth_plat_data *tmp = >> + pdev->dev.platform_data; >> + pd->set_mdio_gate = tmp->set_mdio_gate; >> + pd->needs_init = tmp->needs_init; >> + } >> + } else >> +#endif > > sh_eth_parse_dt() was defined for both CO NFIG_OF and !CONFIG_OF. > But it is called only from CONFIG_OF ? > Because of_device_is_available depennds CONFIG_OF. I already send a patch which add empty fuction of of_device_is_available. If this was applied, your point does not need. OK, I erase empty sh_eth_parse_dt, and if a patch is applied, I will update this. Regards, Nobuhiro
Hello, I have a couple of comments on the binding and the way it's parsed. On Thu, Feb 14, 2013 at 12:51:31AM +0000, Nobuhiro Iwamatsu wrote: > This adds support of device tree probe for Renesas sh-ether driver. > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > --- > V2: - Removed ether_setup(). > - Fixed typo from "sh-etn" to "sh-eth". > - Removed "needs-init" and sh-eth,endian from definition of DT. > - Changed "sh-eth,edmac-endian" instead of "sh-eth,edmac-big-endain" > in definition of DT. > > Documentation/devicetree/bindings/net/sh_ether.txt | 41 +++++ > drivers/net/ethernet/renesas/sh_eth.c | 156 +++++++++++++++++--- > 2 files changed, 173 insertions(+), 24 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/sh_ether.txt > > diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt > new file mode 100644 > index 0000000..7415e54 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/sh_ether.txt > @@ -0,0 +1,41 @@ > +* Renesas Electronics SuperH EMAC > + > +This file provides information, what the device node > +for the sh_eth interface contains. > + > +Required properties: > +- compatible: "renesas,sh-eth"; > +- interrupt-parent: The phandle for the interrupt controller that > + services interrupts for this device. Is this really a required property? As it's a standard property with a well-defined meaning, is it necessary to document? > +- reg: Offset and length of the register set for the > + device. The example below shows 2 reg values, yet this implies only one is necessary. > +- interrupts: Interrupt mapping for the sh_eth interrupt > + sources (vector id). How many interrupts are expected, what are they, and in what order should they be in? > +- phy-mode: String, operation mode of the PHY interface. What are the valid values for this? If they're defined in another document, it'd be good to reference it. > +- sh-eth,register-type: String, register type of sh_eth. > + Please select "gigabit", "fast-sh4" or > + "fast-sh3-sh2". Why are these not handled as separate versions using the compatible string to differentiate them? [...] > +- sh-eth,phy-id: PHY id. > + > +Optional properties: > +- local-mac-address: 6 bytes, mac address > +- sh-eth,no-ether-link: Set link control by software. When device does > + not support ether-link, set. > +- sh-eth,ether-link-active-low: Set link check method. > + When detection of link is treated as active-low, > + set. > +- sh-eth,edmac-big-endian: Set big endian for sh_eth dmac. > + It work as little endian when this is not set. > +- sh-etn,needs-init: Initialization flag. > + When initialize device in probing device, set. > + > +Example (armadillo800eva): > + sh-eth@e9a00000 { > + compatible = "renesas,sh-eth"; > + interrupt-parent = <&intca>; > + reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>; > + interrupts = <0x500>; > + phy-mode = "mii"; > + sh-eth,register-type = "gigabit"; > + sh-eth,phy-id = <0>; > + }; > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > index 3d70586..15e50b87 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c > @@ -1,7 +1,7 @@ > /* > * SuperH Ethernet device driver > * > - * Copyright (C) 2006-2012 Nobuhiro Iwamatsu > + * Copyright (C) 2006-2013 Nobuhiro Iwamatsu > * Copyright (C) 2008-2012 Renesas Solutions Corp. > * > * This program is free software; you can redistribute it and/or modify it > @@ -31,6 +31,12 @@ > #include <linux/platform_device.h> > #include <linux/mdio-bitbang.h> > #include <linux/netdevice.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_platform.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/of_net.h> > #include <linux/phy.h> > #include <linux/cache.h> > #include <linux/io.h> > @@ -2347,26 +2353,109 @@ static const struct net_device_ops sh_eth_netdev_ops = { > .ndo_change_mtu = eth_change_mtu, > }; > > +#ifdef CONFIG_OF > + > +static int > +sh_eth_of_get_register_type(struct device_node *np) > +{ > + const char *of_str; > + int ret = of_property_read_string(np, "sh-eth,register-type", &of_str); > + if (ret) > + return ret; > + > + if (of_str && !strcmp(of_str, "gigabit")) > + return SH_ETH_REG_GIGABIT; > + This line space between the if and the else should disappear. > + else if (of_str && !strcmp(of_str, "fast-sh4")) > + return SH_ETH_REG_FAST_SH4; > + else if (of_str && !strcmp(of_str, "fast-sh3-sh2")) > + return SH_ETH_REG_FAST_SH3_SH2; > + else > + return -EINVAL; I think this block is better handled using a compatible string. > +} > + > +static struct sh_eth_plat_data * > +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) > +{ > + int ret; > + struct device_node *np = dev->of_node; > + struct sh_eth_plat_data *pdata; > + > + pdata = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data), > + GFP_KERNEL); > + if (!pdata) { > + dev_err(dev, "%s: failed to allocate config data\n", __func__); > + return NULL; > + } > + > + pdata->phy_interface = of_get_phy_mode(np); > + > + of_property_read_u32(np, "sh-eth,phy-id", &pdata->phy); No sanity checking required? Is there a maximum possible PHY id? > + > + if (of_find_property(np, "sh-eth,edmac-big-endian", NULL)) > + pdata->edmac_endian = EDMAC_BIG_ENDIAN; > + else > + pdata->edmac_endian = EDMAC_LITTLE_ENDIAN; You can use of_property_read_bool here. > + > + if (of_find_property(np, "sh-eth,no-ether-link", NULL)) > + pdata->no_ether_link = 1; > + else > + pdata->no_ether_link = 0; This can be: pdata->no_ether_link = of_property_read_bool(np, "sh-eth,no-ether-link"); > + > + if (of_find_property(np, "sh-eth,ether-link-active-low", NULL)) > + pdata->ether_link_active_low = 1; > + else > + pdata->ether_link_active_low = 0; A similar thing can be done here. > + > + ret = sh_eth_of_get_register_type(np); > + if (ret < 0) > + goto error; > + pdata->register_type = ret; This could be done using the compatible string and some auxdata. [...] Thanks, Mark. -- 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 Thu, Feb 14, 2013 at 11:56:57AM +0900, Nobuhiro Iwamatsu wrote: > (2013/02/14 10:24), Kuninori Morimoto wrote: > > > >Hi Iwamatsu-san > > > >Thank you for this patch. > > > >Small comment from me > > > >>+#ifdef CONFIG_OF > >(snip) > >>+sh_eth_parse_dt(struct device *dev, struct net_device *ndev) > >>+{ > >>+ int ret; > >>+ struct device_node *np = dev->of_node; > >>+ struct sh_eth_plat_data *pdata; > >... > >>+#else > >>+static struct sh_eth_plat_data * > >>+sh_eth_parse_dt(struct device *dev, struct net_device *ndev) > >>+{ > >>+ return NULL; > >>+} > >>+#endif > > > >(snip) > > > >> static int sh_eth_drv_probe(struct platform_device *pdev) > >> { > >... > >>+#ifdef CONFIG_OF > >>+ if (np&& of_device_is_available(np)) { > >>+ pd = sh_eth_parse_dt(&pdev->dev, ndev); > >>+ if (pdev->dev.platform_data) { > >>+ struct sh_eth_plat_data *tmp = > >>+ pdev->dev.platform_data; > >>+ pd->set_mdio_gate = tmp->set_mdio_gate; > >>+ pd->needs_init = tmp->needs_init; > >>+ } > >>+ } else > >>+#endif > > > >sh_eth_parse_dt() was defined for both CONFIG_OF and !CONFIG_OF. > >But it is called only from CONFIG_OF ? > > > > Because of_device_is_available needs CONFIG_OF. > I already send a patch which add empty function of of_device_is_available. > If this was apply, this ifdef becomes without need. Hi Iwamatsu-san, could you let me know of the status of that patch? Has it been queued-up or merged? If so, where and when? -- 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
Hi, Simon. This does not apply yet to upstream. I will send a patch without of_device_is_available　function. Best regards, Nobuhiro (2013/02/23 3:49), Simon Horman wrote: > On Thu, Feb 14, 2013 at 11:56:57AM +0900, Nobuhiro Iwamatsu wrote: >> (2013/02/14 10:24), Kuninori Morimoto wrote: >>> >>> Hi Iwamatsu-san >>> >>> Thank you for this patch. >>> >>> Small comment from me >>> >>>> +#ifdef CONFIG_OF >>> (snip) >>>> +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) >>>> +{ >>>> + int ret; >>>> + struct device_node *np = dev->of_node; >>>> + struct sh_eth_plat_data *pdata; >>> ... >>>> +#else >>>> +static struct sh_eth_plat_data * >>>> +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) >>>> +{ >>>> + return NULL; >>>> +} >>>> +#endif >>> >>> (snip) >>> >>>> static int sh_eth_drv_probe(struct platform_device *pdev) >>>> { >>> ... >>>> +#ifdef CONFIG_OF >>>> + if (np&& of_device_is_available(np)) { >>>> + pd = sh_eth_parse_dt(&pdev->dev, ndev); >>>> + if (pdev->dev.platform_data) { >>>> + struct sh_eth_plat_data *tmp = >>>> + pdev->dev.platform_data; >>>> + pd->set_mdio_gate = tmp->set_mdio_gate; >>>> + pd->needs_init = tmp->needs_init; >>>> + } >>>> + } else >>>> +#endif >>> >>> sh_eth_parse_dt() was defined for both CONFIG_OF and !CONFIG_OF. >>> But it is called only from CONFIG_OF ? >>> >> >> Because of_device_is_available needs CONFIG_OF. >> I already send a patch which add empty function of of_device_is_available. >> If this was apply, this ifdef becomes without need. > > Hi Iwamatsu-san, > > could you let me know of the status of that patch? > Has it been queued-up or merged? If so, where and when? > > -- 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 Thu, 14 Feb 2013 11:56:57 +0900, Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> wrote: > (2013/02/14 10:24), Kuninori Morimoto wrote: > >> +#ifdef CONFIG_OF > >> + if (np&& of_device_is_available(np)) { > >> + pd = sh_eth_parse_dt(&pdev->dev, ndev); > >> + if (pdev->dev.platform_data) { > >> + struct sh_eth_plat_data *tmp = > >> + pdev->dev.platform_data; > >> + pd->set_mdio_gate = tmp->set_mdio_gate; > >> + pd->needs_init = tmp->needs_init; > >> + } > >> + } else > >> +#endif > > > > sh_eth_parse_dt() was defined for both CONFIG_OF and !CONFIG_OF. > > But it is called only from CONFIG_OF ? > > > > Because of_device_is_available needs CONFIG_OF. > I already send a patch which add empty function of of_device_is_available. > If this was apply, this ifdef becomes without need. Actually, there shouldn't be any reason for a device driver to call of_device_is_available() on its own node at all. If the device is not available, then a platform_device won't be created. "if (np)" here is sufficient. g. -- 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
(2013/03/04 17:05), Grant Likely wrote: > On Thu, 14 Feb 2013 11:56:57 +0900, Nobuhiro Iwamatsu<nobuhiro.iwamatsu.yj@renesas.com> wrote: >> (2013/02/14 10:24), Kuninori Morimoto wrote: >>>> +#ifdef CONFIG_OF >>>> + if (np&& of_device_is_available(np)) { >>>> + pd = sh_eth_parse_dt(&pdev->dev, ndev); >>>> + if (pdev->dev.platform_data) { >>>> + struct sh_eth_plat_data *tmp = >>>> + pdev->dev.platform_data; >>>> + pd->set_mdio_gate = tmp->set_mdio_gate; >>>> + pd->needs_init = tmp->needs_init; >>>> + } >>>> + } else >>>> +#endif >>> >>> sh_eth_parse_dt() was defined for both CONFIG_OF and !CONFIG_OF. >>> But it is called only from CONFIG_OF ? >>> >> >> Because of_device_is_available needs CONFIG_OF. >> I already send a patch which add empty function of of_device_is_available. >> If this was apply, this ifdef becomes without need. > > Actually, there shouldn't be any reason for a device driver to call > of_device_is_available() on its own node at all. If the device is not > available, then a platform_device won't be created. "if (np)" here is > sufficient. > Yes, you are right. I re-writed code without of_device_is_available(). I will send new patch soon. Best regards, Nobuhiro -- 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 Fri, Feb 15, 2013 at 04:32:51PM +0000, Mark Rutland wrote: > Hello, > > I have a couple of comments on the binding and the way it's parsed. > > On Thu, Feb 14, 2013 at 12:51:31AM +0000, Nobuhiro Iwamatsu wrote: > > This adds support of device tree probe for Renesas sh-ether driver. > > > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > > > --- > > V2: - Removed ether_setup(). > > - Fixed typo from "sh-etn" to "sh-eth". > > - Removed "needs-init" and sh-eth,endian from definition of DT. > > - Changed "sh-eth,edmac-endian" instead of "sh-eth,edmac-big-endain" > > in definition of DT. > > > > Documentation/devicetree/bindings/net/sh_ether.txt | 41 +++++ > > drivers/net/ethernet/renesas/sh_eth.c | 156 +++++++++++++++++--- > > 2 files changed, 173 insertions(+), 24 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/net/sh_ether.txt > > > > diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt > > new file mode 100644 > > index 0000000..7415e54 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/sh_ether.txt > > @@ -0,0 +1,41 @@ > > +* Renesas Electronics SuperH EMAC > > + > > +This file provides information, what the device node > > +for the sh_eth interface contains. > > + > > +Required properties: > > +- compatible: "renesas,sh-eth"; > > +- interrupt-parent: The phandle for the interrupt controller that > > + services interrupts for this device. > > Is this really a required property? > > As it's a standard property with a well-defined meaning, is it necessary to > document? > > > +- reg: Offset and length of the register set for the > > + device. > > The example below shows 2 reg values, yet this implies only one is necessary. > > > +- interrupts: Interrupt mapping for the sh_eth interrupt > > + sources (vector id). > > How many interrupts are expected, what are they, and in what order should they be in? > > > +- phy-mode: String, operation mode of the PHY interface. > > What are the valid values for this? > > If they're defined in another document, it'd be good to reference it. > > > +- sh-eth,register-type: String, register type of sh_eth. > > + Please select "gigabit", "fast-sh4" or > > + "fast-sh3-sh2". > > Why are these not handled as separate versions using the compatible string to > differentiate them? Hi Iwamatasu-san, Could you please address this review? I believe it is still applicable as of v5 which you posted yesterday. -- 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
Hi, On Sat, Feb 16, 2013 at 1:32 AM, Mark Rutland <mark.rutland@arm.com> wrote: > Hello, > > I have a couple of comments on the binding and the way it's parsed. Thank you for your comment. > > On Thu, Feb 14, 2013 at 12:51:31AM +0000, Nobuhiro Iwamatsu wrote: >> This adds support of device tree probe for Renesas sh-ether driver. >> >> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> >> >> --- >> V2: - Removed ether_setup(). >> - Fixed typo from "sh-etn" to "sh-eth". >> - Removed "needs-init" and sh-eth,endian from definition of DT. >> - Changed "sh-eth,edmac-endian" instead of "sh-eth,edmac-big-endain" >> in definition of DT. >> >> Documentation/devicetree/bindings/net/sh_ether.txt | 41 +++++ >> drivers/net/ethernet/renesas/sh_eth.c | 156 +++++++++++++++++--- >> 2 files changed, 173 insertions(+), 24 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/net/sh_ether.txt >> >> diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt >> new file mode 100644 >> index 0000000..7415e54 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/sh_ether.txt >> @@ -0,0 +1,41 @@ >> +* Renesas Electronics SuperH EMAC >> + >> +This file provides information, what the device node >> +for the sh_eth interface contains. >> + >> +Required properties: >> +- compatible: "renesas,sh-eth"; >> +- interrupt-parent: The phandle for the interrupt controller that >> + services interrupts for this device. > > Is this really a required property? > > As it's a standard property with a well-defined meaning, is it necessary to > document? Yes, this is necessary. CPU handled by this device has multiple interrupt controllers. It uses the interrupt vector ID. The interrupt controller may have the same vector id. This is necessary in order to distinguish between them. > >> +- reg: Offset and length of the register set for the >> + device. > > The example below shows 2 reg values, yet this implies only one is necessary. OK, I will add more infomation for first registger and 2nd register. > >> +- interrupts: Interrupt mapping for the sh_eth interrupt >> + sources (vector id). > > How many interrupts are expected, what are they, and in what order should they be in? This should contain sh-eth LAN interrupt line. One value is set up. I will add these. > >> +- phy-mode: String, operation mode of the PHY interface. > > What are the valid values for this? A string that of_get_phy_mode() can understand. I will add more infomation. > > If they're defined in another document, it'd be good to reference it. > >> +- sh-eth,register-type: String, register type of sh_eth. >> + Please select "gigabit", "fast-sh4" or >> + "fast-sh3-sh2". > > Why are these not handled as separate versions using the compatible string to > differentiate them? As you pointed, this line was removed and moved to using compatible string. > > [...] > >> +- sh-eth,phy-id: PHY id. >> + >> +Optional properties: >> +- local-mac-address: 6 bytes, mac address >> +- sh-eth,no-ether-link: Set link control by software. When device does >> + not support ether-link, set. >> +- sh-eth,ether-link-active-low: Set link check method. >> + When detection of link is treated as active-low, >> + set. >> +- sh-eth,edmac-big-endian: Set big endian for sh_eth dmac. >> + It work as little endian when this is not set. >> +- sh-etn,needs-init: Initialization flag. >> + When initialize device in probing device, set. >> + >> +Example (armadillo800eva): >> + sh-eth@e9a00000 { >> + compatible = "renesas,sh-eth"; >> + interrupt-parent = <&intca>; >> + reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>; >> + interrupts = <0x500>; >> + phy-mode = "mii"; >> + sh-eth,register-type = "gigabit"; >> + sh-eth,phy-id = <0>; >> + }; >> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c >> index 3d70586..15e50b87 100644 >> --- a/drivers/net/ethernet/renesas/sh_eth.c >> +++ b/drivers/net/ethernet/renesas/sh_eth.c >> @@ -1,7 +1,7 @@ >> /* >> * SuperH Ethernet device driver >> * >> - * Copyright (C) 2006-2012 Nobuhiro Iwamatsu >> + * Copyright (C) 2006-2013 Nobuhiro Iwamatsu >> * Copyright (C) 2008-2012 Renesas Solutions Corp. >> * >> * This program is free software; you can redistribute it and/or modify it >> @@ -31,6 +31,12 @@ >> #include <linux/platform_device.h> >> #include <linux/mdio-bitbang.h> >> #include <linux/netdevice.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/of_platform.h> >> +#include <linux/of_address.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_net.h> >> #include <linux/phy.h> >> #include <linux/cache.h> >> #include <linux/io.h> >> @@ -2347,26 +2353,109 @@ static const struct net_device_ops sh_eth_netdev_ops = { >> .ndo_change_mtu = eth_change_mtu, >> }; >> >> +#ifdef CONFIG_OF >> + >> +static int >> +sh_eth_of_get_register_type(struct device_node *np) >> +{ >> + const char *of_str; >> + int ret = of_property_read_string(np, "sh-eth,register-type", &of_str); >> + if (ret) >> + return ret; >> + >> + if (of_str && !strcmp(of_str, "gigabit")) >> + return SH_ETH_REG_GIGABIT; >> + > > This line space between the if and the else should disappear. OK, I will fix. > >> + else if (of_str && !strcmp(of_str, "fast-sh4")) >> + return SH_ETH_REG_FAST_SH4; >> + else if (of_str && !strcmp(of_str, "fast-sh3-sh2")) >> + return SH_ETH_REG_FAST_SH3_SH2; >> + else >> + return -EINVAL; > > I think this block is better handled using a compatible string. I see, I will fix and update document. > >> +} >> + >> +static struct sh_eth_plat_data * >> +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) >> +{ >> + int ret; >> + struct device_node *np = dev->of_node; >> + struct sh_eth_plat_data *pdata; >> + >> + pdata = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data), >> + GFP_KERNEL); >> + if (!pdata) { >> + dev_err(dev, "%s: failed to allocate config data\n", __func__); >> + return NULL; >> + } >> + >> + pdata->phy_interface = of_get_phy_mode(np); >> + >> + of_property_read_u32(np, "sh-eth,phy-id", &pdata->phy); > > No sanity checking required? > > Is there a maximum possible PHY id? > OK, I will add sanity checking. >> + >> + if (of_find_property(np, "sh-eth,edmac-big-endian", NULL)) >> + pdata->edmac_endian = EDMAC_BIG_ENDIAN; >> + else >> + pdata->edmac_endian = EDMAC_LITTLE_ENDIAN; > > You can use of_property_read_bool here. > OK, I will fix. >> + >> + if (of_find_property(np, "sh-eth,no-ether-link", NULL)) >> + pdata->no_ether_link = 1; >> + else >> + pdata->no_ether_link = 0; > > This can be: > > pdata->no_ether_link = of_property_read_bool(np, "sh-eth,no-ether-link"); OK, I will fix. > >> + >> + if (of_find_property(np, "sh-eth,ether-link-active-low", NULL)) >> + pdata->ether_link_active_low = 1; >> + else >> + pdata->ether_link_active_low = 0; > > A similar thing can be done here. and this too. > >> + >> + ret = sh_eth_of_get_register_type(np); >> + if (ret < 0) >> + goto error; >> + pdata->register_type = ret; > > This could be done using the compatible string and some auxdata. OK, I will fix. > > [...] > > Thanks, > Mark. > _______________________________________________ Best regards, Nobuhiro
diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt new file mode 100644 index 0000000..7415e54 --- /dev/null +++ b/Documentation/devicetree/bindings/net/sh_ether.txt @@ -0,0 +1,41 @@ +* Renesas Electronics SuperH EMAC + +This file provides information, what the device node +for the sh_eth interface contains. + +Required properties: +- compatible: "renesas,sh-eth"; +- interrupt-parent: The phandle for the interrupt controller that + services interrupts for this device. +- reg: Offset and length of the register set for the + device. +- interrupts: Interrupt mapping for the sh_eth interrupt + sources (vector id). +- phy-mode: String, operation mode of the PHY interface. +- sh-eth,register-type: String, register type of sh_eth. + Please select "gigabit", "fast-sh4" or + "fast-sh3-sh2". +- sh-eth,phy-id: PHY id. + +Optional properties: +- local-mac-address: 6 bytes, mac address +- sh-eth,no-ether-link: Set link control by software. When device does + not support ether-link, set. +- sh-eth,ether-link-active-low: Set link check method. + When detection of link is treated as active-low, + set. +- sh-eth,edmac-big-endian: Set big endian for sh_eth dmac. + It work as little endian when this is not set. +- sh-etn,needs-init: Initialization flag. + When initialize device in probing device, set. + +Example (armadillo800eva): + sh-eth@e9a00000 { + compatible = "renesas,sh-eth"; + interrupt-parent = <&intca>; + reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>; + interrupts = <0x500>; + phy-mode = "mii"; + sh-eth,register-type = "gigabit"; + sh-eth,phy-id = <0>; + }; diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index 3d70586..15e50b87 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -1,7 +1,7 @@ /* * SuperH Ethernet device driver * - * Copyright (C) 2006-2012 Nobuhiro Iwamatsu + * Copyright (C) 2006-2013 Nobuhiro Iwamatsu * Copyright (C) 2008-2012 Renesas Solutions Corp. * * This program is free software; you can redistribute it and/or modify it @@ -31,6 +31,12 @@ #include <linux/platform_device.h> #include <linux/mdio-bitbang.h> #include <linux/netdevice.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_net.h> #include <linux/phy.h> #include <linux/cache.h> #include <linux/io.h> @@ -2347,26 +2353,109 @@ static const struct net_device_ops sh_eth_netdev_ops = { .ndo_change_mtu = eth_change_mtu, }; +#ifdef CONFIG_OF + +static int +sh_eth_of_get_register_type(struct device_node *np) +{ + const char *of_str; + int ret = of_property_read_string(np, "sh-eth,register-type", &of_str); + if (ret) + return ret; + + if (of_str && !strcmp(of_str, "gigabit")) + return SH_ETH_REG_GIGABIT; + + else if (of_str && !strcmp(of_str, "fast-sh4")) + return SH_ETH_REG_FAST_SH4; + else if (of_str && !strcmp(of_str, "fast-sh3-sh2")) + return SH_ETH_REG_FAST_SH3_SH2; + else + return -EINVAL; +} + +static struct sh_eth_plat_data * +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) +{ + int ret; + struct device_node *np = dev->of_node; + struct sh_eth_plat_data *pdata; + + pdata = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data), + GFP_KERNEL); + if (!pdata) { + dev_err(dev, "%s: failed to allocate config data\n", __func__); + return NULL; + } + + pdata->phy_interface = of_get_phy_mode(np); + + of_property_read_u32(np, "sh-eth,phy-id", &pdata->phy); + + if (of_find_property(np, "sh-eth,edmac-big-endian", NULL)) + pdata->edmac_endian = EDMAC_BIG_ENDIAN; + else + pdata->edmac_endian = EDMAC_LITTLE_ENDIAN; + + if (of_find_property(np, "sh-eth,no-ether-link", NULL)) + pdata->no_ether_link = 1; + else + pdata->no_ether_link = 0; + + if (of_find_property(np, "sh-eth,ether-link-active-low", NULL)) + pdata->ether_link_active_low = 1; + else + pdata->ether_link_active_low = 0; + + ret = sh_eth_of_get_register_type(np); + if (ret < 0) + goto error; + pdata->register_type = ret; + +#ifdef CONFIG_OF_NET + if (!is_valid_ether_addr(ndev->dev_addr)) { + const char *macaddr = of_get_mac_address(np); + if (macaddr) + memcpy(pdata->mac_addr, macaddr, ETH_ALEN); + } +#endif + + return pdata; + +error: + devm_kfree(dev, pdata); + return NULL; +} +#else +static struct sh_eth_plat_data * +sh_eth_parse_dt(struct device *dev, struct net_device *ndev) +{ + return NULL; +} +#endif + static int sh_eth_drv_probe(struct platform_device *pdev) { - int ret, devno = 0; + int ret = 0, devno = 0; struct resource *res; struct net_device *ndev = NULL; struct sh_eth_private *mdp = NULL; struct sh_eth_plat_data *pd; + struct device_node *np = pdev->dev.of_node; + + ndev = alloc_etherdev(sizeof(struct sh_eth_private)); + if (!ndev) { + ret = -ENOMEM; + goto out; + } + mdp = netdev_priv(ndev); /* get base addr */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (unlikely(res == NULL)) { dev_err(&pdev->dev, "invalid resource\n"); ret = -EINVAL; - goto out; - } - - ndev = alloc_etherdev(sizeof(struct sh_eth_private)); - if (!ndev) { - ret = -ENOMEM; - goto out; + goto out_release; } /* The sh Ether-specific entries in the device structure. */ @@ -2383,27 +2472,41 @@ static int sh_eth_drv_probe(struct platform_device *pdev) } ndev->irq = ret; - SET_NETDEV_DEV(ndev, &pdev->dev); - - /* Fill in the fields of the device structure with ethernet values. */ - ether_setup(ndev); - - mdp = netdev_priv(ndev); - mdp->num_tx_ring = TX_RING_SIZE; - mdp->num_rx_ring = RX_RING_SIZE; mdp->addr = ioremap(res->start, resource_size(res)); if (mdp->addr == NULL) { ret = -ENOMEM; dev_err(&pdev->dev, "ioremap failed.\n"); goto out_release; } +#ifdef CONFIG_OF + if (np && of_device_is_available(np)) { + pd = sh_eth_parse_dt(&pdev->dev, ndev); + if (pdev->dev.platform_data) { + struct sh_eth_plat_data *tmp = + pdev->dev.platform_data; + pd->set_mdio_gate = tmp->set_mdio_gate; + pd->needs_init = tmp->needs_init; + } + } else +#endif + pd = (struct sh_eth_plat_data *)(pdev->dev.platform_data); + + if (!pd) { + dev_err(&pdev->dev, "no setup data defined\n"); + ret = -EINVAL; + goto out_release; + } + + SET_NETDEV_DEV(ndev, &pdev->dev); + + mdp->num_tx_ring = TX_RING_SIZE; + mdp->num_rx_ring = RX_RING_SIZE; spin_lock_init(&mdp->lock); mdp->pdev = pdev; pm_runtime_enable(&pdev->dev); pm_runtime_resume(&pdev->dev); - pd = (struct sh_eth_plat_data *)(pdev->dev.platform_data); /* get PHY ID */ mdp->phy_id = pd->phy; mdp->phy_interface = pd->phy_interface; @@ -2412,6 +2515,8 @@ static int sh_eth_drv_probe(struct platform_device *pdev) mdp->no_ether_link = pd->no_ether_link; mdp->ether_link_active_low = pd->ether_link_active_low; mdp->reg_offset = sh_eth_get_register_offset(pd->register_type); + /* read and set MAC address */ + read_mac_address(ndev, pd->mac_addr); /* set cpu data */ #if defined(SH_ETH_HAS_BOTH_MODULES) @@ -2429,20 +2534,16 @@ static int sh_eth_drv_probe(struct platform_device *pdev) /* debug message level */ mdp->msg_enable = SH_ETH_DEF_MSG_ENABLE; - /* read and set MAC address */ - read_mac_address(ndev, pd->mac_addr); - /* ioremap the TSU registers */ if (mdp->cd->tsu) { struct resource *rtsu; rtsu = platform_get_resource(pdev, IORESOURCE_MEM, 1); if (!rtsu) { dev_err(&pdev->dev, "Not found TSU resource\n"); - ret = -ENODEV; goto out_release; } mdp->tsu_addr = ioremap(rtsu->start, - resource_size(rtsu)); + resource_size(rtsu)); mdp->port = devno % 2; ndev->features = NETIF_F_HW_VLAN_FILTER; } @@ -2522,17 +2623,24 @@ static int sh_eth_runtime_nop(struct device *dev) return 0; } -static struct dev_pm_ops sh_eth_dev_pm_ops = { +static const struct dev_pm_ops sh_eth_dev_pm_ops = { .runtime_suspend = sh_eth_runtime_nop, .runtime_resume = sh_eth_runtime_nop, }; +static struct of_device_id sh_eth_match[] = { + { .compatible = "renesas,sh-eth",}, + {}, +}; +MODULE_DEVICE_TABLE(of, sh_eth_match); + static struct platform_driver sh_eth_driver = { .probe = sh_eth_drv_probe, .remove = sh_eth_drv_remove, .driver = { .name = CARDNAME, .pm = &sh_eth_dev_pm_ops, + .of_match_table = sh_eth_match, }, };
This adds support of device tree probe for Renesas sh-ether driver. Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> --- V2: - Removed ether_setup(). - Fixed typo from "sh-etn" to "sh-eth". - Removed "needs-init" and sh-eth,endian from definition of DT. - Changed "sh-eth,edmac-endian" instead of "sh-eth,edmac-big-endain" in definition of DT. Documentation/devicetree/bindings/net/sh_ether.txt | 41 +++++ drivers/net/ethernet/renesas/sh_eth.c | 156 +++++++++++++++++--- 2 files changed, 173 insertions(+), 24 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/sh_ether.txt