[03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node
diff mbox

Message ID 20150730001124.4012.31980.stgit@noble
State New
Headers show

Commit Message

NeilBrown July 30, 2015, 12:11 a.m. UTC
Now that twl4030_bci_probe can safely return -EPROBE_DEFER,
do so when devm_usb_get_phy_by_node returns that error.

Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/power/twl4030_charger.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tony Lindgren Aug. 18, 2015, 8:07 a.m. UTC | #1
* NeilBrown <neil@brown.name> [150729 17:29]:
> Now that twl4030_bci_probe can safely return -EPROBE_DEFER,
> do so when devm_usb_get_phy_by_node returns that error.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  drivers/power/twl4030_charger.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
> index 045238370d3f..ffc123fb7158 100644
> --- a/drivers/power/twl4030_charger.c
> +++ b/drivers/power/twl4030_charger.c
> @@ -636,9 +636,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  
>  		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>  						  NULL, "ti,twl4030-usb");
> -		if (phynode)
> +		if (phynode) {
>  			bci->transceiver = devm_usb_get_phy_by_node(
>  				bci->dev, phynode, &bci->usb_nb);
> +			if (IS_ERR(bci->transceiver) &&
> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +		}
>  	}

Neil, the return with -EPROBE_DEFER here causes flakeyness booting
for me somehow at least on my logicpd-torpedo-37xx-devkit using
omap2plus_defconfig.

It seems that the twl4030_bci_probe keeps looping or something about
1/3 of the boots and that probably prevents the other twl modules
from loading? Reverting this patch alone seems to fix the issue.

I don't think I have a battery wired on this board the USB is wired
the same way as on beagle-xm. So I'd assume also flakeyness on beagle
xm with this patch.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
NeilBrown Aug. 19, 2015, 12:28 a.m. UTC | #2
On Tue, 18 Aug 2015 01:07:58 -0700 Tony Lindgren <tony@atomide.com>
wrote:

> * NeilBrown <neil@brown.name> [150729 17:29]:
> > Now that twl4030_bci_probe can safely return -EPROBE_DEFER,
> > do so when devm_usb_get_phy_by_node returns that error.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  drivers/power/twl4030_charger.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
> > index 045238370d3f..ffc123fb7158 100644
> > --- a/drivers/power/twl4030_charger.c
> > +++ b/drivers/power/twl4030_charger.c
> > @@ -636,9 +636,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
> >  
> >  		phynode = of_find_compatible_node(bci->dev->of_node->parent,
> >  						  NULL, "ti,twl4030-usb");
> > -		if (phynode)
> > +		if (phynode) {
> >  			bci->transceiver = devm_usb_get_phy_by_node(
> >  				bci->dev, phynode, &bci->usb_nb);
> > +			if (IS_ERR(bci->transceiver) &&
> > +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> > +		}
> >  	}
> 
> Neil, the return with -EPROBE_DEFER here causes flakeyness booting
> for me somehow at least on my logicpd-torpedo-37xx-devkit using
> omap2plus_defconfig.
> 
> It seems that the twl4030_bci_probe keeps looping or something about
> 1/3 of the boots and that probably prevents the other twl modules
> from loading? Reverting this patch alone seems to fix the issue.
> 
> I don't think I have a battery wired on this board the USB is wired
> the same way as on beagle-xm. So I'd assume also flakeyness on beagle
> xm with this patch.
> 
> Regards,
> 
> Tony

What dts file are you using?
I'm guessing that it doesn't have something like:

&usb_otg_hs {
        interface-type = <0>;
        usb-phy = <&usb2_phy>;
        phys = <&usb2_phy>;
        phy-names = "usb2-phy";
        mode = <3>;
        power = <50>;
};

? i.e. with a usb-phy=<&usb2_phy> ?

What if you add

&usb2_phy {
 status = "disabled";
}

to your dts file?

Should the 'status' be disabled in twl4030.dtsi, and then marked OK in
any dts that uses it?  I'm not at all clear on how 'status' is meant to
be used.

Thanks,
NeilBrown

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Aug. 19, 2015, 6:25 a.m. UTC | #3
* NeilBrown <neil@brown.name> [150818 17:32]:
> On Tue, 18 Aug 2015 01:07:58 -0700 Tony Lindgren <tony@atomide.com>
> wrote:
> 
> > * NeilBrown <neil@brown.name> [150729 17:29]:
> > > Now that twl4030_bci_probe can safely return -EPROBE_DEFER,
> > > do so when devm_usb_get_phy_by_node returns that error.
> > > 
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > >  drivers/power/twl4030_charger.c |    6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
> > > index 045238370d3f..ffc123fb7158 100644
> > > --- a/drivers/power/twl4030_charger.c
> > > +++ b/drivers/power/twl4030_charger.c
> > > @@ -636,9 +636,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
> > >  
> > >  		phynode = of_find_compatible_node(bci->dev->of_node->parent,
> > >  						  NULL, "ti,twl4030-usb");
> > > -		if (phynode)
> > > +		if (phynode) {
> > >  			bci->transceiver = devm_usb_get_phy_by_node(
> > >  				bci->dev, phynode, &bci->usb_nb);
> > > +			if (IS_ERR(bci->transceiver) &&
> > > +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
> > > +				return -EPROBE_DEFER;
> > > +		}
> > >  	}
> > 
> > Neil, the return with -EPROBE_DEFER here causes flakeyness booting
> > for me somehow at least on my logicpd-torpedo-37xx-devkit using
> > omap2plus_defconfig.
> > 
> > It seems that the twl4030_bci_probe keeps looping or something about
> > 1/3 of the boots and that probably prevents the other twl modules
> > from loading? Reverting this patch alone seems to fix the issue.
> > 
> > I don't think I have a battery wired on this board the USB is wired
> > the same way as on beagle-xm. So I'd assume also flakeyness on beagle
> > xm with this patch.
> 
> What dts file are you using?
> I'm guessing that it doesn't have something like:
> 
> &usb_otg_hs {
>         interface-type = <0>;
>         usb-phy = <&usb2_phy>;
>         phys = <&usb2_phy>;
>         phy-names = "usb2-phy";
>         mode = <3>;
>         power = <50>;
> };

It's the logicpd-torpedo-37xx-devkit.dts like I mentioned above,
and yes it has the same entry above like beagle xm.
 
> ? i.e. with a usb-phy=<&usb2_phy> ?
> 
> What if you add
> 
> &usb2_phy {
>  status = "disabled";
> }
> 
> to your dts file?

Yes then the PHY driver won't get probed and modprobe of the
charger won't hang with -EPROBE_DEFER.. The USB is working on this
one like on beagle xm so that's not a solution.
 
> Should the 'status' be disabled in twl4030.dtsi, and then marked OK in
> any dts that uses it?  I'm not at all clear on how 'status' is meant to
> be used.

Well status = "disabled" makes kernel completely ignore the
hardware, so the struct device is never created. I'd stay away from
using that in general for proper runtime idling of devices.. The
hardware is there for sure :)

Can you check if something like the the following allows you to
reproduce the hang of modprobe twl4030_charger:

# rmmod phy-twl4030-usb
# modprobe twl4030_charger

So just don't probe phy-twl4030-usb before twl4030_charger and
then modprobe of twl4030_charger hangs?

That is with proper usb2_phy configured in .dts file and not
set to status = "disabled".

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Aug. 27, 2015, 8:51 p.m. UTC | #4
On Wed, Jul 29, 2015 at 5:11 PM, NeilBrown <neil@brown.name> wrote:
> Now that twl4030_bci_probe can safely return -EPROBE_DEFER,
> do so when devm_usb_get_phy_by_node returns that error.
>
> Signed-off-by: NeilBrown <neil@brown.name>

This patch has hit linux-next in the form of coommit 3fc3895e4fe1
(twl4030_charger: correctly handle -EPROBE_DEFER from
devm_usb_get_phy_by_node) and kernelci.org found a regression on
omap3-beagle-xm[1].  Bisecting[2] this boot failure pointed at this
commit, and I verified that reverting it on top of next-20150827 gets
the board booting again.  I haven't debugged any further.

Kevin

[1] http://storage.kernelci.org/next/next-20150827/arm-omap2plus_defconfig/lab-khilman/boot-omap3-beagle-xm.html
[2] https://ci.linaro.org/view/people/job/tbaker-boot-bisect-bot/88/console
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Sept. 2, 2015, 3:25 a.m. UTC | #5
ping... this boot failure has now landed in mainline

On Thu, Aug 27, 2015 at 1:51 PM, Kevin Hilman <khilman@kernel.org> wrote:
> On Wed, Jul 29, 2015 at 5:11 PM, NeilBrown <neil@brown.name> wrote:
>> Now that twl4030_bci_probe can safely return -EPROBE_DEFER,
>> do so when devm_usb_get_phy_by_node returns that error.
>>
>> Signed-off-by: NeilBrown <neil@brown.name>
>
> This patch has hit linux-next in the form of coommit 3fc3895e4fe1
> (twl4030_charger: correctly handle -EPROBE_DEFER from
> devm_usb_get_phy_by_node) and kernelci.org found a regression on
> omap3-beagle-xm[1].  Bisecting[2] this boot failure pointed at this
> commit, and I verified that reverting it on top of next-20150827 gets
> the board booting again.  I haven't debugged any further.
>
> Kevin
>
> [1] http://storage.kernelci.org/next/next-20150827/arm-omap2plus_defconfig/lab-khilman/boot-omap3-beagle-xm.html
> [2] https://ci.linaro.org/view/people/job/tbaker-boot-bisect-bot/88/console
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
NeilBrown Sept. 2, 2015, 6:19 a.m. UTC | #6
Kevin Hilman <khilman@kernel.org> writes:

> ping... this boot failure has now landed in mainline

sorry, I'm on leave at the moment and travelling so I'm unlikely to be
able to look at this properly.  I should be able to examine this issue
before the end of the month but cannot promise sooner than that (though
it is not impossible).

Maybe it would be best to just revert it until a proper analysis can be
done.

NeilBrown


>
> On Thu, Aug 27, 2015 at 1:51 PM, Kevin Hilman <khilman@kernel.org> wrote:
>> On Wed, Jul 29, 2015 at 5:11 PM, NeilBrown <neil@brown.name> wrote:
>>> Now that twl4030_bci_probe can safely return -EPROBE_DEFER,
>>> do so when devm_usb_get_phy_by_node returns that error.
>>>
>>> Signed-off-by: NeilBrown <neil@brown.name>
>>
>> This patch has hit linux-next in the form of coommit 3fc3895e4fe1
>> (twl4030_charger: correctly handle -EPROBE_DEFER from
>> devm_usb_get_phy_by_node) and kernelci.org found a regression on
>> omap3-beagle-xm[1].  Bisecting[2] this boot failure pointed at this
>> commit, and I verified that reverting it on top of next-20150827 gets
>> the board booting again.  I haven't debugged any further.
>>
>> Kevin
>>
>> [1] http://storage.kernelci.org/next/next-20150827/arm-omap2plus_defconfig/lab-khilman/boot-omap3-beagle-xm.html
>> [2] https://ci.linaro.org/view/people/job/tbaker-boot-bisect-bot/88/console
Tony Lindgren Sept. 2, 2015, 1:07 p.m. UTC | #7
* Neil Brown <neil@brown.name> [150901 23:23]:
> Kevin Hilman <khilman@kernel.org> writes:
> 
> > ping... this boot failure has now landed in mainline
> 
> sorry, I'm on leave at the moment and travelling so I'm unlikely to be
> able to look at this properly.  I should be able to examine this issue
> before the end of the month but cannot promise sooner than that (though
> it is not impossible).
> 
> Maybe it would be best to just revert it until a proper analysis can be
> done.

OK yeah let's revert this one for now until we know what goes wrong.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Sept. 8, 2015, 6:32 p.m. UTC | #8
On Wed, Sep 2, 2015 at 6:07 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Neil Brown <neil@brown.name> [150901 23:23]:
>> Kevin Hilman <khilman@kernel.org> writes:
>>
>> > ping... this boot failure has now landed in mainline
>>
>> sorry, I'm on leave at the moment and travelling so I'm unlikely to be
>> able to look at this properly.  I should be able to examine this issue
>> before the end of the month but cannot promise sooner than that (though
>> it is not impossible).
>>
>> Maybe it would be best to just revert it until a proper analysis can be
>> done.
>
> OK yeah let's revert this one for now until we know what goes wrong.

Looks like this is still in mainline.

Tony, can you revert?

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Sept. 8, 2015, 8:14 p.m. UTC | #9
* Kevin Hilman <khilman@kernel.org> [150908 11:36]:
> On Wed, Sep 2, 2015 at 6:07 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Neil Brown <neil@brown.name> [150901 23:23]:
> >> Kevin Hilman <khilman@kernel.org> writes:
> >>
> >> > ping... this boot failure has now landed in mainline
> >>
> >> sorry, I'm on leave at the moment and travelling so I'm unlikely to be
> >> able to look at this properly.  I should be able to examine this issue
> >> before the end of the month but cannot promise sooner than that (though
> >> it is not impossible).
> >>
> >> Maybe it would be best to just revert it until a proper analysis can be
> >> done.
> >
> > OK yeah let's revert this one for now until we know what goes wrong.
> 
> Looks like this is still in mainline.
> 
> Tony, can you revert?

Probably best that Sebastian does it as there's another fix needed
too. It seems the following are needed:

- Revert $subject patch 3fc3895e4fe1 ("twl4030_charger: correctly
  handle -EPROBE_DEFER from devm_usb_get_phy_by_node")

- Apply compile fix "[PATCH] twl4030_charger: fix another compile
  error"

Just in case Sebastian is travelling or something.. Sebastian,
can you please confirm?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel Sept. 10, 2015, 8:08 a.m. UTC | #10
Hi,

On Tue, Sep 08, 2015 at 01:14:17PM -0700, Tony Lindgren wrote:
> * Kevin Hilman <khilman@kernel.org> [150908 11:36]:
> > On Wed, Sep 2, 2015 at 6:07 AM, Tony Lindgren <tony@atomide.com> wrote:
> > > * Neil Brown <neil@brown.name> [150901 23:23]:
> > >> Kevin Hilman <khilman@kernel.org> writes:
> > >>
> > >> > ping... this boot failure has now landed in mainline
> > >>
> > >> sorry, I'm on leave at the moment and travelling so I'm unlikely to be
> > >> able to look at this properly.  I should be able to examine this issue
> > >> before the end of the month but cannot promise sooner than that (though
> > >> it is not impossible).
> > >>
> > >> Maybe it would be best to just revert it until a proper analysis can be
> > >> done.
> > >
> > > OK yeah let's revert this one for now until we know what goes wrong.
> > 
> > Looks like this is still in mainline.
> > 
> > Tony, can you revert?
> 
> Probably best that Sebastian does it as there's another fix needed
> too. It seems the following are needed:
> 
> - Revert $subject patch 3fc3895e4fe1 ("twl4030_charger: correctly
>   handle -EPROBE_DEFER from devm_usb_get_phy_by_node")
> 
> - Apply compile fix "[PATCH] twl4030_charger: fix another compile
>   error"
> 
> Just in case Sebastian is travelling or something..

Right, I was on vacation without internet access until now.

> Sebastian, can you please confirm?

Sounds fine to me. I will take care of it once I've slept a bit.

-- Sebastian
Sebastian Reichel Sept. 10, 2015, 8:27 p.m. UTC | #11
Hi,

On Thu, Sep 10, 2015 at 10:08:50AM +0200, Sebastian Reichel wrote:
> > Probably best that Sebastian does it as there's another fix needed
> > too. It seems the following are needed:
> > 
> > - Revert $subject patch 3fc3895e4fe1 ("twl4030_charger: correctly
> >   handle -EPROBE_DEFER from devm_usb_get_phy_by_node")
> > 
> > - Apply compile fix "[PATCH] twl4030_charger: fix another compile
> >   error"

I just added these to my next branch:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=next&id=aefc574bbbbe74bb891ba392d98f2d59a417c774
https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=next&id=e11fc21e756e662e465cac3da6547d438d0b1446

-- Sebastian
Tony Lindgren Sept. 10, 2015, 8:43 p.m. UTC | #12
* Sebastian Reichel <sre@kernel.org> [150910 13:31]:
> Hi,
> 
> On Thu, Sep 10, 2015 at 10:08:50AM +0200, Sebastian Reichel wrote:
> > > Probably best that Sebastian does it as there's another fix needed
> > > too. It seems the following are needed:
> > > 
> > > - Revert $subject patch 3fc3895e4fe1 ("twl4030_charger: correctly
> > >   handle -EPROBE_DEFER from devm_usb_get_phy_by_node")
> > > 
> > > - Apply compile fix "[PATCH] twl4030_charger: fix another compile
> > >   error"
> 
> I just added these to my next branch:
> 
> https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=next&id=aefc574bbbbe74bb891ba392d98f2d59a417c774
> https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=next&id=e11fc21e756e662e465cac3da6547d438d0b1446

Thanks!

Tony

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index 045238370d3f..ffc123fb7158 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -636,9 +636,13 @@  static int twl4030_bci_probe(struct platform_device *pdev)
 
 		phynode = of_find_compatible_node(bci->dev->of_node->parent,
 						  NULL, "ti,twl4030-usb");
-		if (phynode)
+		if (phynode) {
 			bci->transceiver = devm_usb_get_phy_by_node(
 				bci->dev, phynode, &bci->usb_nb);
+			if (IS_ERR(bci->transceiver) &&
+			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+		}
 	}
 
 	/* Enable interrupts now. */