| Message ID | 2811962.eGX2i5RJbZ@wasted.cogentembedded.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote: > With the 'phylib' now being aware of the "reset-gpios" PHY node property, > there should be no need to frob the PHY reset in this driver anymore... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > drivers/net/ethernet/cadence/macb.c | 17 ----------------- > drivers/net/ethernet/cadence/macb.h | 1 - > 2 files changed, 18 deletions(-) > > Index: net-next/drivers/net/ethernet/cadence/macb.c > =================================================================== > --- net-next.orig/drivers/net/ethernet/cadence/macb.c > +++ net-next/drivers/net/ethernet/cadence/macb.c > @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de > = macb_clk_init; > int (*init)(struct platform_device *) = macb_init; > struct device_node *np = pdev->dev.of_node; > - struct device_node *phy_node; > const struct macb_config *macb_config = NULL; > struct clk *pclk, *hclk = NULL, *tx_clk = NULL; > unsigned int queue_mask, num_queues; > @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de > else > macb_get_hwaddr(bp); > > - /* Power up the PHY if there is a GPIO reset */ > - phy_node = of_get_next_available_child(np, NULL); > - if (phy_node) { > - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); > - > - if (gpio_is_valid(gpio)) { > - bp->reset_gpio = gpio_to_desc(gpio); > - gpiod_direction_output(bp->reset_gpio, 1); Hi Sergei The code you are deleting would of ignored the flags in the gpio property, i.e. active low. The new code in the previous patch does however take the flags into account. Did you check if there are any device trees which have flags, which were never used, but are now going to be used and thus break... Andrew
Hello. On 04/11/2016 05:28 AM, Andrew Lunn wrote: >> With the 'phylib' now being aware of the "reset-gpios" PHY node property, >> there should be no need to frob the PHY reset in this driver anymore... >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> drivers/net/ethernet/cadence/macb.c | 17 ----------------- >> drivers/net/ethernet/cadence/macb.h | 1 - >> 2 files changed, 18 deletions(-) >> >> Index: net-next/drivers/net/ethernet/cadence/macb.c >> =================================================================== >> --- net-next.orig/drivers/net/ethernet/cadence/macb.c >> +++ net-next/drivers/net/ethernet/cadence/macb.c [...] >> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de >> else >> macb_get_hwaddr(bp); >> >> - /* Power up the PHY if there is a GPIO reset */ >> - phy_node = of_get_next_available_child(np, NULL); >> - if (phy_node) { >> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); >> - >> - if (gpio_is_valid(gpio)) { >> - bp->reset_gpio = gpio_to_desc(gpio); >> - gpiod_direction_output(bp->reset_gpio, 1); > > Hi Sergei > > The code you are deleting would of ignored the flags in the gpio > property, i.e. active low. Hm, you're right -- I forgot about that... :-/ > The new code in the previous patch does > however take the flags into account. Did you check if there are any > device trees which have flags, which were never used, but are now > going to be used and thus break... Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. Looks like it needs to be fixed indeed... > Andrew MBR, Sergei
> >The code you are deleting would of ignored the flags in the gpio > >property, i.e. active low. > > Hm, you're right -- I forgot about that... :-/ > > >The new code in the previous patch does > >however take the flags into account. Did you check if there are any > >device trees which have flags, which were never used, but are now > >going to be used and thus break... > > Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. > Looks like it needs to be fixed indeed... And this is where it gets tricky. You are breaking backwards compatibility by now respecting the flag. An old DT blob is not going to work. You potentially need to add a new property and deprecate the old one. Andrew
Hello. On 04/11/2016 09:19 PM, Andrew Lunn wrote: >>> The code you are deleting would of ignored the flags in the gpio >>> property, i.e. active low. >> >> Hm, you're right -- I forgot about that... :-/ >> >>> The new code in the previous patch does >>> however take the flags into account. Did you check if there are any >>> device trees which have flags, which were never used, but are now >>> going to be used and thus break... >> >> Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. >> Looks like it needs to be fixed indeed... >> > And this is where it gets tricky. You are breaking backwards > compatibility by now respecting the flag. An old DT blob is not going > to work. Do we care that much about the DT blobs that are just *wrong*? > You potentially need to add a new property and deprecate the old one. I would like to avoid that... > Andrew MBR, Sergei
On Mon, Apr 11, 2016 at 09:39:02PM +0300, Sergei Shtylyov wrote: > Hello. > > On 04/11/2016 09:19 PM, Andrew Lunn wrote: > > >>>The code you are deleting would of ignored the flags in the gpio > >>>property, i.e. active low. > >> > >> Hm, you're right -- I forgot about that... :-/ > >> > >>>The new code in the previous patch does > >>>however take the flags into account. Did you check if there are any > >>>device trees which have flags, which were never used, but are now > >>>going to be used and thus break... > >> > >> Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. > >>Looks like it needs to be fixed indeed... > >> > >And this is where it gets tricky. You are breaking backwards > >compatibility by now respecting the flag. An old DT blob is not going > >to work. > > Do we care that much about the DT blobs that are just *wrong*? Wrong, but currently works. > >You potentially need to add a new property and deprecate the old one. > > I would like to avoid that... You will need the agreement from the at91-vinco maintainer. Andrew
On 04/11/2016 09:51 PM, Andrew Lunn wrote: >>>>> The code you are deleting would of ignored the flags in the gpio >>>>> property, i.e. active low. >>>> >>>> Hm, you're right -- I forgot about that... :-/ >>>> >>>>> The new code in the previous patch does >>>>> however take the flags into account. Did you check if there are any >>>>> device trees which have flags, which were never used, but are now >>>>> going to be used and thus break... >>>> >>>> Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. >>>> Looks like it needs to be fixed indeed... >>>> >>> And this is where it gets tricky. You are breaking backwards >>> compatibility by now respecting the flag. An old DT blob is not going >>> to work. >> >> Do we care that much about the DT blobs that are just *wrong*? > > Wrong, but currently works. Note that it's not only using GPIO_ACTIVE_HIGH but does that against what the MACB binding documents. >>> You potentially need to add a new property and deprecate the old one. >> >> I would like to avoid that... > > You will need the agreement from the at91-vinco maintainer. I'll try submitting a formal DT patch... > Andrew MBR, Sergei
Le 11/04/2016 04:28, Andrew Lunn a écrit : > On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote: >> With the 'phylib' now being aware of the "reset-gpios" PHY node property, >> there should be no need to frob the PHY reset in this driver anymore... >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> drivers/net/ethernet/cadence/macb.c | 17 ----------------- >> drivers/net/ethernet/cadence/macb.h | 1 - >> 2 files changed, 18 deletions(-) >> >> Index: net-next/drivers/net/ethernet/cadence/macb.c >> =================================================================== >> --- net-next.orig/drivers/net/ethernet/cadence/macb.c >> +++ net-next/drivers/net/ethernet/cadence/macb.c >> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de >> = macb_clk_init; >> int (*init)(struct platform_device *) = macb_init; >> struct device_node *np = pdev->dev.of_node; >> - struct device_node *phy_node; >> const struct macb_config *macb_config = NULL; >> struct clk *pclk, *hclk = NULL, *tx_clk = NULL; >> unsigned int queue_mask, num_queues; >> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de >> else >> macb_get_hwaddr(bp); >> >> - /* Power up the PHY if there is a GPIO reset */ >> - phy_node = of_get_next_available_child(np, NULL); >> - if (phy_node) { >> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); >> - >> - if (gpio_is_valid(gpio)) { >> - bp->reset_gpio = gpio_to_desc(gpio); >> - gpiod_direction_output(bp->reset_gpio, 1); > > Hi Sergei > > The code you are deleting would of ignored the flags in the gpio I don't parse this. The code deleted does take the flag into account. And the DT property associated to it seems correct to me (I mean, with proper flag specification). > property, i.e. active low. The new code in the previous patch does > however take the flags into account. Did you check if there are any > device trees which have flags, which were never used, but are now > going to be used and thus break... Flag was used and you are saying that it's taken into account in new code... So, what's the issue? I see a difference in the way the "value" of gpiod_* functions is used. There may be a misunderstanding here... Bye,
Le 11/04/2016 20:51, Andrew Lunn a écrit : > On Mon, Apr 11, 2016 at 09:39:02PM +0300, Sergei Shtylyov wrote: >> Hello. >> >> On 04/11/2016 09:19 PM, Andrew Lunn wrote: >> >>>>> The code you are deleting would of ignored the flags in the gpio >>>>> property, i.e. active low. >>>> >>>> Hm, you're right -- I forgot about that... :-/ >>>> >>>>> The new code in the previous patch does >>>>> however take the flags into account. Did you check if there are any >>>>> device trees which have flags, which were never used, but are now >>>>> going to be used and thus break... >>>> >>>> Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. >>>> Looks like it needs to be fixed indeed... >>>> >>> And this is where it gets tricky. You are breaking backwards >>> compatibility by now respecting the flag. An old DT blob is not going >>> to work. >> >> Do we care that much about the DT blobs that are just *wrong*? > > Wrong, but currently works. > >>> You potentially need to add a new property and deprecate the old one. >> >> I would like to avoid that... > > You will need the agreement from the at91-vinco maintainer. If the at91-vinco has to be modified, you have my agreement that it can be modified. Bye,
On Tue, Apr 12, 2016 at 11:22:10AM +0200, Nicolas Ferre wrote: > Le 11/04/2016 04:28, Andrew Lunn a écrit : > > On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote: > >> With the 'phylib' now being aware of the "reset-gpios" PHY node property, > >> there should be no need to frob the PHY reset in this driver anymore... > >> > >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >> > >> --- > >> drivers/net/ethernet/cadence/macb.c | 17 ----------------- > >> drivers/net/ethernet/cadence/macb.h | 1 - > >> 2 files changed, 18 deletions(-) > >> > >> Index: net-next/drivers/net/ethernet/cadence/macb.c > >> =================================================================== > >> --- net-next.orig/drivers/net/ethernet/cadence/macb.c > >> +++ net-next/drivers/net/ethernet/cadence/macb.c > >> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de > >> = macb_clk_init; > >> int (*init)(struct platform_device *) = macb_init; > >> struct device_node *np = pdev->dev.of_node; > >> - struct device_node *phy_node; > >> const struct macb_config *macb_config = NULL; > >> struct clk *pclk, *hclk = NULL, *tx_clk = NULL; > >> unsigned int queue_mask, num_queues; > >> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de > >> else > >> macb_get_hwaddr(bp); > >> > >> - /* Power up the PHY if there is a GPIO reset */ > >> - phy_node = of_get_next_available_child(np, NULL); > >> - if (phy_node) { > >> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); > >> - > >> - if (gpio_is_valid(gpio)) { > >> - bp->reset_gpio = gpio_to_desc(gpio); > >> - gpiod_direction_output(bp->reset_gpio, 1); > > > > Hi Sergei > > > > The code you are deleting would of ignored the flags in the gpio > I don't parse this. > > The code deleted does take the flag into account. And the DT property > associated to it seems correct to me (I mean, with proper flag > specification). Hi Nicolas of_get_named_gpio() does not do anything with the flags. So for example, gpios = <&gpio0 12 GPIO_ACTIVE_LOW>; the GPIO_ACTIVE_LOW would be ignored. If you want the flags to be respected, you need to use the gpiod API for all calls, in particular, you need to use something which calls gpiod_get_index(), since that is the only function to call gpiod_parse_flags() to translate GPIO_ACTIVE_LOW into a flag within the gpio descriptor. Andrew
Hello. On 4/12/2016 12:22 PM, Nicolas Ferre wrote: >>> With the 'phylib' now being aware of the "reset-gpios" PHY node property, >>> there should be no need to frob the PHY reset in this driver anymore... >>> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> >>> --- >>> drivers/net/ethernet/cadence/macb.c | 17 ----------------- >>> drivers/net/ethernet/cadence/macb.h | 1 - >>> 2 files changed, 18 deletions(-) >>> >>> Index: net-next/drivers/net/ethernet/cadence/macb.c >>> =================================================================== >>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c >>> +++ net-next/drivers/net/ethernet/cadence/macb.c [...] >>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de >>> else >>> macb_get_hwaddr(bp); >>> >>> - /* Power up the PHY if there is a GPIO reset */ >>> - phy_node = of_get_next_available_child(np, NULL); >>> - if (phy_node) { >>> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); >>> - >>> - if (gpio_is_valid(gpio)) { >>> - bp->reset_gpio = gpio_to_desc(gpio); >>> - gpiod_direction_output(bp->reset_gpio, 1); >> >> Hi Sergei >> >> The code you are deleting would of ignored the flags in the gpio > > I don't parse this. > The code deleted does take the flag into account. Not really -- you need to call of_get_named_gpio_flags() (with a valid last argument) for that. > And the DT property > associated to it seems correct to me (I mean, with proper flag > specification). It apparently is not as it have GPIO_ACTIVE_HIGH and the driver assumes active-low reset signal. [...] > Bye, MBR, Sergei
Le 12/04/2016 15:40, Andrew Lunn a écrit : > On Tue, Apr 12, 2016 at 11:22:10AM +0200, Nicolas Ferre wrote: >> Le 11/04/2016 04:28, Andrew Lunn a écrit : >>> On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote: >>>> With the 'phylib' now being aware of the "reset-gpios" PHY node property, >>>> there should be no need to frob the PHY reset in this driver anymore... >>>> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>> >>>> --- >>>> drivers/net/ethernet/cadence/macb.c | 17 ----------------- >>>> drivers/net/ethernet/cadence/macb.h | 1 - >>>> 2 files changed, 18 deletions(-) >>>> >>>> Index: net-next/drivers/net/ethernet/cadence/macb.c >>>> =================================================================== >>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c >>>> +++ net-next/drivers/net/ethernet/cadence/macb.c >>>> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de >>>> = macb_clk_init; >>>> int (*init)(struct platform_device *) = macb_init; >>>> struct device_node *np = pdev->dev.of_node; >>>> - struct device_node *phy_node; >>>> const struct macb_config *macb_config = NULL; >>>> struct clk *pclk, *hclk = NULL, *tx_clk = NULL; >>>> unsigned int queue_mask, num_queues; >>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de >>>> else >>>> macb_get_hwaddr(bp); >>>> >>>> - /* Power up the PHY if there is a GPIO reset */ >>>> - phy_node = of_get_next_available_child(np, NULL); >>>> - if (phy_node) { >>>> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); >>>> - >>>> - if (gpio_is_valid(gpio)) { >>>> - bp->reset_gpio = gpio_to_desc(gpio); >>>> - gpiod_direction_output(bp->reset_gpio, 1); >>> >>> Hi Sergei >>> >>> The code you are deleting would of ignored the flags in the gpio >> I don't parse this. >> >> The code deleted does take the flag into account. And the DT property >> associated to it seems correct to me (I mean, with proper flag >> specification). > > Hi Nicolas > > of_get_named_gpio() does not do anything with the flags. So for > example, > > gpios = <&gpio0 12 GPIO_ACTIVE_LOW>; > > the GPIO_ACTIVE_LOW would be ignored. If you want the flags to be > respected, you need to use the gpiod API for all calls, in particular, > you need to use something which calls gpiod_get_index(), since that is > the only function to call gpiod_parse_flags() to translate > GPIO_ACTIVE_LOW into a flag within the gpio descriptor. Ok, I remember what confused me now: this code, used to be something around: devm_gpiod_get_optional(&bp->pdev->dev, "phy-reset", GPIOD_OUT_HIGH); before it has been changed to the chunk above... So, yes, the DT flag was not handled anyway... Sorry for the noise and thanks for the clarification. Bye,
Le 12/04/2016 15:54, Sergei Shtylyov a écrit : > Hello. > > On 4/12/2016 12:22 PM, Nicolas Ferre wrote: > >>>> With the 'phylib' now being aware of the "reset-gpios" PHY node property, >>>> there should be no need to frob the PHY reset in this driver anymore... >>>> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>> >>>> --- >>>> drivers/net/ethernet/cadence/macb.c | 17 ----------------- >>>> drivers/net/ethernet/cadence/macb.h | 1 - >>>> 2 files changed, 18 deletions(-) >>>> >>>> Index: net-next/drivers/net/ethernet/cadence/macb.c >>>> =================================================================== >>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c >>>> +++ net-next/drivers/net/ethernet/cadence/macb.c > [...] >>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de >>>> else >>>> macb_get_hwaddr(bp); >>>> >>>> - /* Power up the PHY if there is a GPIO reset */ >>>> - phy_node = of_get_next_available_child(np, NULL); >>>> - if (phy_node) { >>>> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); >>>> - >>>> - if (gpio_is_valid(gpio)) { >>>> - bp->reset_gpio = gpio_to_desc(gpio); >>>> - gpiod_direction_output(bp->reset_gpio, 1); >>> >>> Hi Sergei >>> >>> The code you are deleting would of ignored the flags in the gpio >> >> I don't parse this. > >> The code deleted does take the flag into account. > > Not really -- you need to call of_get_named_gpio_flags() (with a valid > last argument) for that. Yep, >> And the DT property >> associated to it seems correct to me (I mean, with proper flag >> specification). > > It apparently is not as it have GPIO_ACTIVE_HIGH and the driver assumes > active-low reset signal. Yes, logic was inverted and... anyway, the flag never used for real... Thanks Sergei. No problem for me accepting a patch for the at91-vinco.dts. Bye,
Index: net-next/drivers/net/ethernet/cadence/macb.c =================================================================== --- net-next.orig/drivers/net/ethernet/cadence/macb.c +++ net-next/drivers/net/ethernet/cadence/macb.c @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de = macb_clk_init; int (*init)(struct platform_device *) = macb_init; struct device_node *np = pdev->dev.of_node; - struct device_node *phy_node; const struct macb_config *macb_config = NULL; struct clk *pclk, *hclk = NULL, *tx_clk = NULL; unsigned int queue_mask, num_queues; @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de else macb_get_hwaddr(bp); - /* Power up the PHY if there is a GPIO reset */ - phy_node = of_get_next_available_child(np, NULL); - if (phy_node) { - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); - - if (gpio_is_valid(gpio)) { - bp->reset_gpio = gpio_to_desc(gpio); - gpiod_direction_output(bp->reset_gpio, 1); - } - } - of_node_put(phy_node); - err = of_get_phy_mode(np); if (err < 0) { pdata = dev_get_platdata(&pdev->dev); @@ -3054,10 +3041,6 @@ static int macb_remove(struct platform_d mdiobus_unregister(bp->mii_bus); mdiobus_free(bp->mii_bus); - /* Shutdown the PHY if there is a GPIO reset */ - if (bp->reset_gpio) - gpiod_set_value(bp->reset_gpio, 0); - unregister_netdev(dev); clk_disable_unprepare(bp->tx_clk); clk_disable_unprepare(bp->hclk); Index: net-next/drivers/net/ethernet/cadence/macb.h =================================================================== --- net-next.orig/drivers/net/ethernet/cadence/macb.h +++ net-next/drivers/net/ethernet/cadence/macb.h @@ -832,7 +832,6 @@ struct macb { unsigned int dma_burst_length; phy_interface_t phy_interface; - struct gpio_desc *reset_gpio; /* AT91RM9200 transmit */ struct sk_buff *skb; /* holds skb until xmit interrupt completes */
With the 'phylib' now being aware of the "reset-gpios" PHY node property, there should be no need to frob the PHY reset in this driver anymore... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- drivers/net/ethernet/cadence/macb.c | 17 ----------------- drivers/net/ethernet/cadence/macb.h | 1 - 2 files changed, 18 deletions(-)