All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: Endless "supply vcc not found, using dummy regulator"
       [not found] <20160515173900.GC21194@sesse.net>
@ 2016-05-21 14:43 ` Steinar H. Gunderson
  2016-05-23 13:47   ` Steinar H. Gunderson
  0 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-21 14:43 UTC (permalink / raw)
  To: linux-samsung-soc

On Sun, May 15, 2016 at 07:39:00PM +0200, Steinar H. Gunderson wrote:
> [   47.161428] exynos-dwc3 usb@12000000: no suspend clk specified               
> [   47.162811] usb_phy_generic.49646.auto supply vcc not found, using dummy regulator 
> [   47.163532] usb_phy_generic.49647.auto supply vcc not found, using dummy regulator
> 
> They appear to go on forever, blocking boot -- until I set the log level to 0
> to suppress them (using magic SysRq over the serial console), nothing else seems
> to happen on the machine. (loglevel=0 on the kernel command line also works
> fine.)

Hi,

I still have this problem.

I've noticed that somehow, there's a huge amount of USB PHYs being autodetected;
probably related to this issue.

  sesse@soldroid:~$ ls -ld /sys/devices/platform/usb_phy* | wc -l
  4288

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-21 14:43 ` Endless "supply vcc not found, using dummy regulator" Steinar H. Gunderson
@ 2016-05-23 13:47   ` Steinar H. Gunderson
  2016-05-23 14:40     ` Steinar H. Gunderson
  0 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-23 13:47 UTC (permalink / raw)
  To: linux-samsung-soc; +Cc: 823552, broonie

On Sat, May 21, 2016 at 04:43:08PM +0200, Steinar H. Gunderson wrote:
> I still have this problem.
> 
> I've noticed that somehow, there's a huge amount of USB PHYs being autodetected;
> probably related to this issue.
> 
>   sesse@soldroid:~$ ls -ld /sys/devices/platform/usb_phy* | wc -l
>   4288

I've tracked this down partially. It has to do with probe deferral;
for whatever reason, the vdd33 regulator isn't available when the dwc3 driver
comes up, and this causes the probe to defer. For each and every such
deferral, these messages are printed by the regulator subsystem as part of
things attempted probed before vdd33. I don't understand entirely why
it tries 2000+ times before it succeeds, but then I don't understand probe
deferral very well to begin with. I also don't know if there's a way to avoid
these messages for each time, but it seems this is a common problem:

  https://lkml.org/lkml/2016/3/16/269

In this case, it's not just an annoyance, though; they're so many that they
keep the system from booting unless loglevel is turned down. Cc-ing Mark in
case he has any insights (I hope I have the right email address).

The reason for the huge amount of PHYs is that the driver doesn't clean them
up on failure (including probe deferral), so they leak. This is easy to fix;
I'm attaching a small fix below.

>From 6df2ebcbaae74577d49dbbc41e28d52084a124b0 Mon Sep 17 00:00:00 2001
From: "Steinar H. Gunderson" <sesse@google.com>
Date: Mon, 23 May 2016 15:44:37 +0200
Subject: [PATCH 1/1] dwc3-exynos: Clean up phys on probe failure.

Otherwise, they would leak, which is especially bad given probe deferral
(one could end up with thousands of dead phys after a normal boot)

Signed-off-by: Steinar H. Gunderson <sesse@google.com>
---
 drivers/usb/dwc3/dwc3-exynos.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index dd5cb55..4104ef5 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -202,6 +202,8 @@ err4:
 err3:
 	regulator_disable(exynos->vdd33);
 err2:
+	platform_device_unregister(exynos->usb2_phy);
+	platform_device_unregister(exynos->usb3_phy);
 	clk_disable_unprepare(exynos->axius_clk);
 	clk_disable_unprepare(exynos->susp_clk);
 	clk_disable_unprepare(exynos->clk);

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-23 13:47   ` Steinar H. Gunderson
@ 2016-05-23 14:40     ` Steinar H. Gunderson
  2016-05-23 16:24       ` Mark Brown
  0 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-23 14:40 UTC (permalink / raw)
  To: linux-samsung-soc; +Cc: 823552, broonie

On Mon, May 23, 2016 at 03:47:37PM +0200, Steinar H. Gunderson wrote:
> I don't understand entirely why it tries 2000+ times before it succeeds

Now I do; the initramfs doesn't include i2c-exynos5, and before that is
loaded, s2mps11 (the regulator) can't come up either.

So fixing initramfs-tools to include the driver will seemingly fix (or maybe
more work around) the huge amounts of spam, but this is still a larger issue
that needs resolving.

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-23 14:40     ` Steinar H. Gunderson
@ 2016-05-23 16:24       ` Mark Brown
  2016-05-23 17:06         ` Steinar H. Gunderson
  0 siblings, 1 reply; 31+ messages in thread
From: Mark Brown @ 2016-05-23 16:24 UTC (permalink / raw)
  To: Steinar H. Gunderson; +Cc: linux-samsung-soc, 823552

[-- Attachment #1: Type: text/plain, Size: 1987 bytes --]

On Mon, May 23, 2016 at 04:40:47PM +0200, Steinar H. Gunderson wrote:
> On Mon, May 23, 2016 at 03:47:37PM +0200, Steinar H. Gunderson wrote:

> > In this case, it's not just an annoyance, though; they're so many that they
> > keep the system from booting unless loglevel is turned down. Cc-ing Mark in
> > case he has any insights (I hope I have the right email address).

But nobody who works on probe deferral or made any of the suggestions I
mentioned in the message you linked to, nor anyone who works on the
driver you've identified a bug in...  :(

> > I don't understand entirely why it tries 2000+ times before it succeeds

> Now I do; the initramfs doesn't include i2c-exynos5, and before that is
> loaded, s2mps11 (the regulator) can't come up either.

> So fixing initramfs-tools to include the driver will seemingly fix (or maybe
> more work around) the huge amounts of spam, but this is still a larger issue
> that needs resolving.

Not really, the issue you're seeing is just a plain resource leak in the
driver that happens to blow up worse than normal in your particular
configuration.  This isn't even something related to probe deferral at
the regulator level, the core is complaining that your system
description is buggy as it has omitted some of the supplies for the
device (notice how it says "using dummy regulator"...).   This is
happening a lot as the DWC3 driver is leaking, it is happening at all
because when the Exynos DWC3 integration creates it PHYs it doesn't map
the supplies through to them (it should be registering a supply alias to
do this).

The patch you linked to was for a completely different error message
which is at least related to probe deferral, though fundamentally unless
we just stop printing diagnostics (which is getting more and more
tempting to be honest) I'm not sure anything is going to fully resolve
leaks like this, the best chance you've got is something that explicitly
looks at the dependencies like Raphael was proposing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-23 16:24       ` Mark Brown
@ 2016-05-23 17:06         ` Steinar H. Gunderson
  2016-05-23 17:46           ` Steinar H. Gunderson
                             ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-23 17:06 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-samsung-soc, 823552

On Mon, May 23, 2016 at 05:24:55PM +0100, Mark Brown wrote:
>>> Cc-ing Mark in case he has any insights (I hope I have the right email
>>> address).
> But nobody who works on probe deferral or made any of the suggestions I
> mentioned in the message you linked to, nor anyone who works on the
> driver you've identified a bug in...  :(

As for the former, I have no idea who they would be, sorry. For the latter,
isn't linux-samsung-soc@ the right place? MAINTAINERS said it was.

>> So fixing initramfs-tools to include the driver will seemingly fix (or maybe
>> more work around) the huge amounts of spam, but this is still a larger issue
>> that needs resolving.
> Not really, the issue you're seeing is just a plain resource leak in the
> driver that happens to blow up worse than normal in your particular
> configuration.  This isn't even something related to probe deferral at
> the regulator level, the core is complaining that your system
> description is buggy as it has omitted some of the supplies for the
> device (notice how it says "using dummy regulator"...).   This is
> happening a lot as the DWC3 driver is leaking, it is happening at all
> because when the Exynos DWC3 integration creates it PHYs it doesn't map
> the supplies through to them (it should be registering a supply alias to
> do this).

So there are multiple issues in play here. First of all, the leak is bad,
but it doesn't affect the issue with the system not booting. If I set
loglevel=0, it boots with or without the leak patch; however, it still sends
a gazillion error lines to dmesg (with or without the deferral).

Second, there's the issue that the driver takes so long to load in the first
place. This is because the regulator isn't up and doesn't come up before
after initramfs is done. This is a bug in Debian's initramfs-tools, but
hopefully easily remedied.

Then, there's the issue of why the messages come for each deferred probe
attempt. It seems from your message this is about something in the
declaration of the device tree; I don't understand the nuances here, but I
suppose it's pretty easy?

Fourth, there's the question of why there are thousands of probe attempts;
it shouldn't be even if the regulator takes a long time to come up. I guess
this is what your original message was about, and fixing that would also
reduce the amount of logging a ton (plus presumably speed up boot by wasting
less CPU on repeated probing). But as I understand you, it's not strictly
necessary to actually fix this issue?

Fifth and finally, there's the question of whether we can suppress
diagnostics on a probe that ends up being deferred. It would also solve the
problem here, although perhaps less elegantly than fixing issues #3 or
#4 (or to a lesser degree, #2), either of which will make my system boot. :-)

> The patch you linked to was for a completely different error message
> which is at least related to probe deferral

Yes, it's a different error message, but points to the same issue as #4
above, no?

> though fundamentally unless
> we just stop printing diagnostics (which is getting more and more
> tempting to be honest) I'm not sure anything is going to fully resolve
> leaks like this, the best chance you've got is something that explicitly
> looks at the dependencies like Raphael was proposing.

What do you mean by “leaks” here?

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-23 17:06         ` Steinar H. Gunderson
@ 2016-05-23 17:46           ` Steinar H. Gunderson
  2016-05-23 18:56             ` Mark Brown
  2016-05-23 18:44           ` Mark Brown
  2016-05-24 15:06           ` Krzysztof Kozlowski
  2 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-23 17:46 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-samsung-soc, 823552

On Mon, May 23, 2016 at 07:06:17PM +0200, Steinar H. Gunderson wrote:
> Then, there's the issue of why the messages come for each deferred probe
> attempt. It seems from your message this is about something in the
> declaration of the device tree; I don't understand the nuances here, but I
> suppose it's pretty easy?

FWIW, from reading drivers/usb/phy/phy-generic.c, it looks like vcc-supply on
the USB phy is supposed to be optional.

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-23 17:06         ` Steinar H. Gunderson
  2016-05-23 17:46           ` Steinar H. Gunderson
@ 2016-05-23 18:44           ` Mark Brown
  2016-05-24 15:06           ` Krzysztof Kozlowski
  2 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2016-05-23 18:44 UTC (permalink / raw)
  To: Steinar H. Gunderson; +Cc: linux-samsung-soc, 823552

[-- Attachment #1: Type: text/plain, Size: 5316 bytes --]

On Mon, May 23, 2016 at 07:06:17PM +0200, Steinar H. Gunderson wrote:
> On Mon, May 23, 2016 at 05:24:55PM +0100, Mark Brown wrote:

> >>> Cc-ing Mark in case he has any insights (I hope I have the right email
> >>> address).

> > But nobody who works on probe deferral or made any of the suggestions I
> > mentioned in the message you linked to, nor anyone who works on the
> > driver you've identified a bug in...  :(

> As for the former, I have no idea who they would be, sorry. For the latter,

There's the people I mentioned in the e-mail you linked to for
exmaple...  you could also look at git annotate for the probe deferral
code and see who worked on it, or do whatever it is lead to you finding
me.

> isn't linux-samsung-soc@ the right place? MAINTAINERS said it was.

That's just the list, not people.  You really want to see who's working
on the driver and talk to them, you can't guarantee that everyone reads
the lists sufficiently well that they will notice some random post that
doesn't even mention the driver in the subject line (it's probably a
good idea to submit that patch BTW).  Note also that if MAINTAINERS has
multiple hits you want to pay attention to them.

> Then, there's the issue of why the messages come for each deferred probe
> attempt. It seems from your message this is about something in the
> declaration of the device tree; I don't understand the nuances here, but I
> suppose it's pretty easy?

No.  This is nothing to do with the device tree.  The dwc3-exynos driver
is continually creating new, partially specified devices.  Since each of
these new devices has the same problem they all get the same warning
reported for them.

The regulator API has no idea that this is anything to do with deferred
probe, it's printing a warning message because something asked for a
supply that not mapped at all which is a potential concern if we explode
later due to this being an error in the code.  All the regulator API can
realistically do is remove all diagnostics in case someone triggers them
with deferred probe but that's probably not enormously helpful to people
if they run into an error directly triggered at the regulator level who
might like some diagnostics to help them figure out what went wrong.

> Fourth, there's the question of why there are thousands of probe attempts;
> it shouldn't be even if the regulator takes a long time to come up. I guess
> this is what your original message was about, and fixing that would also
> reduce the amount of logging a ton (plus presumably speed up boot by wasting
> less CPU on repeated probing). But as I understand you, it's not strictly
> necessary to actually fix this issue?

No, this is the way probe deferral is supposed to work.  It is a bit
wasteful of CPU but realistically it's not that much and it's really
simple to implement robustly unlike anything that involves actually
looking at dependencies.  Some breakage in your system is triggering
vastly more retries than are normal, even a hundred rounds would be
*extremely* high for the initial boot.

We're doing repeated probes because the way deferred probe works is that
every time a new device binds we start a new retry of all deferred
devices to see if that new device unblocked anything (if multiple
devices progress in one run it should only schedule one new run).  The
reason you're seeing the spectacular volume is that every time we retry
we add more devices (notice that you're not complaining about the log
message generated by the underlying Designware controller failing to get
the supply it requests which will print once per probe but rather by the
PHY devices it spams in).

The fact that the Exynos driver is instantiating subdevices before it's
even acquired its own resources is probably not helping here of course,
it's more than likely that at least some of those resources need to be
passed on to the child devices and of course if the children did
actually instantiate that'd trigger continual probe deferral runs.
Looking at the code I've got a horrible feeling that might be the root
cause here, it's a single regulator that's being requested so the
diagnostics should be being printed in the caller but that just silently
passes back failures to get supplies (which is why it wasn't immediately
obvious that that was failing).

> > The patch you linked to was for a completely different error message
> > which is at least related to probe deferral

> Yes, it's a different error message, but points to the same issue as #4
> above, no?

Not from the point of the view of the regulator API and really as far as
I can tell this is just a driver specific issue.  The regulator API has
no idea this is anything to do with deferred probe.

> > though fundamentally unless
> > we just stop printing diagnostics (which is getting more and more
> > tempting to be honest) I'm not sure anything is going to fully resolve
> > leaks like this, the best chance you've got is something that explicitly
> > looks at the dependencies like Raphael was proposing.

> What do you mean by “leaks” here?

Resources that are not cleaned up are said to be leaks, as you identifed
the dwc3-exynos code isn't cleaning up the child devices it creates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-23 17:46           ` Steinar H. Gunderson
@ 2016-05-23 18:56             ` Mark Brown
  0 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2016-05-23 18:56 UTC (permalink / raw)
  To: Steinar H. Gunderson; +Cc: linux-samsung-soc, 823552

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

On Mon, May 23, 2016 at 07:46:43PM +0200, Steinar H. Gunderson wrote:
> On Mon, May 23, 2016 at 07:06:17PM +0200, Steinar H. Gunderson wrote:

> > Then, there's the issue of why the messages come for each deferred probe
> > attempt. It seems from your message this is about something in the
> > declaration of the device tree; I don't understand the nuances here, but I
> > suppose it's pretty easy?

> FWIW, from reading drivers/usb/phy/phy-generic.c, it looks like vcc-supply on
> the USB phy is supposed to be optional.

No, not unless the device can operate without power which seems
improbable.  Optional supplies are for supplies which may be physically
absent, not to shut up warnings from partially specified system
descriptions.  If there is an optional supply with no configuration of
the device to operate in a different mode without that supply it is most
likely that the API is being abused.  The API is there to support users
with things like optional external reference voltages that may be
missing in some system designs, it's not there to support broken system
integrations.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-23 17:06         ` Steinar H. Gunderson
  2016-05-23 17:46           ` Steinar H. Gunderson
  2016-05-23 18:44           ` Mark Brown
@ 2016-05-24 15:06           ` Krzysztof Kozlowski
  2016-05-24 15:26             ` Steinar H. Gunderson
  2016-05-24 15:39             ` Mark Brown
  2 siblings, 2 replies; 31+ messages in thread
From: Krzysztof Kozlowski @ 2016-05-24 15:06 UTC (permalink / raw)
  To: Steinar H. Gunderson; +Cc: Mark Brown, linux-samsung-soc, 823552

On Mon, May 23, 2016 at 7:06 PM, Steinar H. Gunderson
<sgunderson@bigfoot.com> wrote:
> On Mon, May 23, 2016 at 05:24:55PM +0100, Mark Brown wrote:
>>>> Cc-ing Mark in case he has any insights (I hope I have the right email
>>>> address).
>> But nobody who works on probe deferral or made any of the suggestions I
>> mentioned in the message you linked to, nor anyone who works on the
>> driver you've identified a bug in...  :(
>
> As for the former, I have no idea who they would be, sorry. For the latter,
> isn't linux-samsung-soc@ the right place? MAINTAINERS said it was.

I looked quickly through the thread and I am not sure what is exactly
your problem. I understood that the Exynos dwc3 driver is leaking the
PHY. That is a good catch but your patch is not sufficient. You should
clean up starting from devm_clk_get() error. Unless you are fixing
this for different kernel version.

Please send an appropriate separate patch for fixing this. Your email
did not reach people, I think. Just make a patch, test it,
scripts/checkpatch and scripts/get_maintainers. It is a fix so also a
candidate for this release cycle. if you do not plan to send a patch,
I can prepare on my own with your "Reported-by" tag but actually this
is your finding so credits (and effort) should go to you. :)

What other problems exactly do you have? Late binding of S2MPS11
regulator driver? That does not look like a problem. If it is built as
a module then it should be loaded, probably from initramfs because
these are regulators.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-24 15:06           ` Krzysztof Kozlowski
@ 2016-05-24 15:26             ` Steinar H. Gunderson
  2016-05-24 15:45               ` Mark Brown
  2016-05-24 15:53               ` Krzysztof Kozlowski
  2016-05-24 15:39             ` Mark Brown
  1 sibling, 2 replies; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-24 15:26 UTC (permalink / raw)
  To: Krzysztof Kozlowski; +Cc: Mark Brown, linux-samsung-soc, 823552

On Tue, May 24, 2016 at 05:06:43PM +0200, Krzysztof Kozlowski wrote:
> I looked quickly through the thread and I am not sure what is exactly
> your problem.

My immediate problem is that the repeated (deferred) probing is causing so
much logging that the system doesn't actually boot. The root causes are a bit
more complex and muddy (see my previous email for a breakdown).

> I understood that the Exynos dwc3 driver is leaking the
> PHY. That is a good catch but your patch is not sufficient. You should
> clean up starting from devm_clk_get() error. Unless you are fixing
> this for different kernel version.

I have zero idea how all of this is supposed to be connected, much less how
DWC3 works or what the driver is supposed to do. I'm just an end user trying
to figure out why my system didn't boot. :-)

Which devm_clk_get() error are you talking about? The one with susp_clk?
That's an interesting case because a) nothing actually uses susp_clk
(it's dead code, presumably waiting for further patches), and b) there was a
commit not too long ago (42f69a02) upgrading the message from dev_dbg to
dev_info for reasons I don't understand, making the problem worse.
(The commit message had an argument that I could accept for changing _to_
dbg, but not the other way round.)

> Please send an appropriate separate patch for fixing this. Your email
> did not reach people, I think.

I only sent one patch so far, namely the leak fix.

> What other problems exactly do you have? Late binding of S2MPS11
> regulator driver? That does not look like a problem. If it is built as
> a module then it should be loaded, probably from initramfs because
> these are regulators.

In this specific case, Debian's initramfs has neglected to include the i2c
driver in the initramfs, so the regulator doesn't come up until the boot is
out of the initramfs. That's probably a bug in its own right, and fixing it
reduces the amount of messages by a _lot_, but even so, it sounds to me like
the kernel should be able to boot without that fix.

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-24 15:06           ` Krzysztof Kozlowski
  2016-05-24 15:26             ` Steinar H. Gunderson
@ 2016-05-24 15:39             ` Mark Brown
  1 sibling, 0 replies; 31+ messages in thread
From: Mark Brown @ 2016-05-24 15:39 UTC (permalink / raw)
  To: Krzysztof Kozlowski; +Cc: Steinar H. Gunderson, linux-samsung-soc, 823552

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

On Tue, May 24, 2016 at 05:06:43PM +0200, Krzysztof Kozlowski wrote:

> What other problems exactly do you have? Late binding of S2MPS11
> regulator driver? That does not look like a problem. If it is built as
> a module then it should be loaded, probably from initramfs because
> these are regulators.

AFAICT the fact that every time the dwc3-exynos driver probes it creates
a new child device which successfully instantiates means that we end up
constantly running deferred probes.  It should only create the child
devices if it managed to get the other resources, that way we don't get
the constant deferred probe retries.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-24 15:26             ` Steinar H. Gunderson
@ 2016-05-24 15:45               ` Mark Brown
  2016-05-24 15:53               ` Krzysztof Kozlowski
  1 sibling, 0 replies; 31+ messages in thread
From: Mark Brown @ 2016-05-24 15:45 UTC (permalink / raw)
  To: Steinar H. Gunderson; +Cc: Krzysztof Kozlowski, linux-samsung-soc, 823552

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Tue, May 24, 2016 at 05:26:38PM +0200, Steinar H. Gunderson wrote:
> On Tue, May 24, 2016 at 05:06:43PM +0200, Krzysztof Kozlowski wrote:

> > Please send an appropriate separate patch for fixing this. Your email
> > did not reach people, I think.

> I only sent one patch so far, namely the leak fix.

You buried it in the middle of another mail and didn't CC any of the
maintainers - he's asking you to submit it using the process in
SubmittingPatches, the USB maintainers won't have seen this discussion
and may not notice a patch buried in the middle of a message in the
middle of a thread even if they see it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-24 15:26             ` Steinar H. Gunderson
  2016-05-24 15:45               ` Mark Brown
@ 2016-05-24 15:53               ` Krzysztof Kozlowski
  2016-05-24 16:44                 ` Steinar H. Gunderson
       [not found]                 ` <574478FE.4070800-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  1 sibling, 2 replies; 31+ messages in thread
From: Krzysztof Kozlowski @ 2016-05-24 15:53 UTC (permalink / raw)
  To: Steinar H. Gunderson; +Cc: Mark Brown, linux-samsung-soc, 823552

On 05/24/2016 05:26 PM, Steinar H. Gunderson wrote:
> On Tue, May 24, 2016 at 05:06:43PM +0200, Krzysztof Kozlowski wrote:
>> I looked quickly through the thread and I am not sure what is exactly
>> your problem.
> 
> My immediate problem is that the repeated (deferred) probing is causing so
> much logging that the system doesn't actually boot. The root causes are a bit
> more complex and muddy (see my previous email for a breakdown).

Ah, I got it.

> 
>> I understood that the Exynos dwc3 driver is leaking the
>> PHY. That is a good catch but your patch is not sufficient. You should
>> clean up starting from devm_clk_get() error. Unless you are fixing
>> this for different kernel version.
> 
> I have zero idea how all of this is supposed to be connected, much less how
> DWC3 works or what the driver is supposed to do. I'm just an end user trying
> to figure out why my system didn't boot. :-)
> 
> Which devm_clk_get() error are you talking about? The one with susp_clk?

Now I saw your original report on Debian bugzilla. Let's stick to v4.5.
The platform_device_alloc() is done in dwc3_exynos_register_phys(). So
on error paths you have clean up starting from next error:

       ret = dwc3_exynos_register_phys(exynos);
        if (ret) {
                dev_err(dev, "couldn't register PHYs\n");
                return ret;
        }

        exynos->dev     = dev;

        exynos->clk = devm_clk_get(dev, "usbdrd30");
        if (IS_ERR(exynos->clk)) {
+		// On each error path since here we need to
+		// revert work done by dwc3_exynos_register_phys()
                dev_err(dev, "couldn't get clock\n");
                return -EINVAL;
        }
        clk_prepare_enable(exynos->clk);


> That's an interesting case because a) nothing actually uses susp_clk
> (it's dead code, presumably waiting for further patches),

It does not look like dead code because it is enabled.

> and b) there was a
> commit not too long ago (42f69a02) upgrading the message from dev_dbg to
> dev_info for reasons I don't understand, making the problem worse.
> (The commit message had an argument that I could accept for changing _to_
> dbg, but not the other way round.

I don't get the rationale behind that change neither...


>> Please send an appropriate separate patch for fixing this. Your email
>> did not reach people, I think.
> 
> I only sent one patch so far, namely the leak fix.

Yeah, but you did not send it to appropriate people. get_maintainer.pl
will point you (Felipe Balbi handles the patches for this driver).

> 
>> What other problems exactly do you have? Late binding of S2MPS11
>> regulator driver? That does not look like a problem. If it is built as
>> a module then it should be loaded, probably from initramfs because
>> these are regulators.
> 
> In this specific case, Debian's initramfs has neglected to include the i2c
> driver in the initramfs, so the regulator doesn't come up until the boot is
> out of the initramfs. That's probably a bug in its own right, and fixing it
> reduces the amount of messages by a _lot_, but even so, it sounds to me like
> the kernel should be able to boot without that fix.

Apparently the s2mps11 regulator driver can be built as module... but is
not a commonly tested configuration. In our testing configs (exynos and
multi_v7) it is built-in. Actually most of PMICs are built in.

Additionally, if you look at the driver, its init was moved earlier from
module_init() to subsys_initcall(). This is kind of manual probe
ordering, used mostly because some depending drivers did not support
probe deferral.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-24 15:53               ` Krzysztof Kozlowski
@ 2016-05-24 16:44                 ` Steinar H. Gunderson
       [not found]                 ` <574478FE.4070800-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  1 sibling, 0 replies; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-24 16:44 UTC (permalink / raw)
  To: Krzysztof Kozlowski; +Cc: Mark Brown, linux-samsung-soc, 823552

On Tue, May 24, 2016 at 05:53:34PM +0200, Krzysztof Kozlowski wrote:
>> Which devm_clk_get() error are you talking about? The one with susp_clk?
> Now I saw your original report on Debian bugzilla. Let's stick to v4.5.

I'm actually developing on 4.6, but sure. The differences are small from what
I can see.

>         exynos->clk = devm_clk_get(dev, "usbdrd30");
>         if (IS_ERR(exynos->clk)) {
> +		// On each error path since here we need to
> +		// revert work done by dwc3_exynos_register_phys()
>                 dev_err(dev, "couldn't get clock\n");
>                 return -EINVAL;
>         }
>         clk_prepare_enable(exynos->clk);

OK, sounds like these should be changed to the common goto pattern to save on
the repeated cleanup. I'll have a stab at that later today.

>> That's an interesting case because a) nothing actually uses susp_clk
>> (it's dead code, presumably waiting for further patches),
> It does not look like dead code because it is enabled.

But not a single DT seems to set such a suspend clock, from what I can see?

> Yeah, but you did not send it to appropriate people. get_maintainer.pl
> will point you (Felipe Balbi handles the patches for this driver).

Ack.

> Apparently the s2mps11 regulator driver can be built as module... but is
> not a commonly tested configuration. In our testing configs (exynos and
> multi_v7) it is built-in. Actually most of PMICs are built in.

I don't have a lot of control over how Debian chooses to build kernels --
from what I gather from previous conversations, the kernel team are unhappy
about building things into the kernel if they can be modules. And in this
case, it wasn't even about the s2mps11 module, it was just that it didn't
have an I2C bus ready for init.

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2] dwc3-exynos: Fix deferred probing storm.
  2016-05-24 19:24                   ` [PATCH] " Steinar H. Gunderson
@ 2016-05-24 18:13                     ` Steinar H. Gunderson
  2016-05-24 18:13                     ` [PATCH] " Steinar H. Gunderson
  2016-05-25 12:16                     ` [PATCH] Re: Endless "supply vcc not found, using dummy regulator" Anand Moon
  2 siblings, 0 replies; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-24 18:13 UTC (permalink / raw)
  To: k.kozlowski
  Cc: broonie, linux-samsung-soc, 823552, balbi, linux-usb, stable,
	gautam.vivek

dwc3-exynos has two problems during init if the regulators are slow
to come up (for instance if the I2C bus driver is not on the initramfs)
and return probe deferral. First, every time this happens, the driver
leaks the USB phys created; they need to be deallocated on error.

Second, since the phy devices are created before the regulators fail,
this means that there's a new device to re-trigger deferred probing,
which causes it to essentially go into a busy loop of re-probing the
device until the regulators come up.

Move the phy creation to after the regulators have succeeded, and also
fix cleanup on failure. On my ODROID XU4 system (with Debian's initramfs
which doesn't contain the I2C driver), this reduces the number of probe
attempts (for each of the two controllers) from more than 2000 to eight.

Signed-off-by: Steinar H. Gunderson <sesse@google.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Vivek Gautam <gautam.vivek@samsung.com>
Fixes: d720f057fda4 ("usb: dwc3: exynos: add nop transceiver support")
Cc: <stable@vger.kernel.org>
---
 drivers/usb/dwc3/dwc3-exynos.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index dd5cb55..2f1fb7e 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -128,12 +128,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, exynos);
 
-	ret = dwc3_exynos_register_phys(exynos);
-	if (ret) {
-		dev_err(dev, "couldn't register PHYs\n");
-		return ret;
-	}
-
 	exynos->dev	= dev;
 
 	exynos->clk = devm_clk_get(dev, "usbdrd30");
@@ -183,20 +177,29 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 		goto err3;
 	}
 
+	ret = dwc3_exynos_register_phys(exynos);
+	if (ret) {
+		dev_err(dev, "couldn't register PHYs\n");
+		goto err4;
+	}
+
 	if (node) {
 		ret = of_platform_populate(node, NULL, NULL, dev);
 		if (ret) {
 			dev_err(dev, "failed to add dwc3 core\n");
-			goto err4;
+			goto err5;
 		}
 	} else {
 		dev_err(dev, "no device node, failed to add dwc3 core\n");
 		ret = -ENODEV;
-		goto err4;
+		goto err5;
 	}
 
 	return 0;
 
+err5:
+	platform_device_unregister(exynos->usb2_phy);
+	platform_device_unregister(exynos->usb3_phy);
 err4:
 	regulator_disable(exynos->vdd10);
 err3:
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH] dwc3-exynos: Fix deferred probing storm.
  2016-05-24 19:24                   ` [PATCH] " Steinar H. Gunderson
  2016-05-24 18:13                     ` [PATCH v2] dwc3-exynos: Fix deferred probing storm Steinar H. Gunderson
@ 2016-05-24 18:13                     ` Steinar H. Gunderson
  2016-05-25  7:54                       ` Krzysztof Kozlowski
  2016-05-27  9:53                       ` Vivek Gautam
  2016-05-25 12:16                     ` [PATCH] Re: Endless "supply vcc not found, using dummy regulator" Anand Moon
  2 siblings, 2 replies; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-24 18:13 UTC (permalink / raw)
  To: k.kozlowski; +Cc: broonie, linux-samsung-soc, 823552, balbi, linux-usb

dwc3-exynos has two problems during init if the regulators are slow
to come up (for instance if the I2C bus driver is not on the initramfs)
and return probe deferral. First, every time this happens, the driver
leaks the USB phys created; they need to be deallocated on error.

Second, since the phy devices are created before the regulators fail,
this means that there's a new device to re-trigger deferred probing,
which causes it to essentially go into a busy loop of re-probing the
device until the regulators come up.

Move the phy creation to after the regulators have succeeded, and also
fix cleanup on failure. On my ODROID XU4 system (with Debian's initramfs
which doesn't contain the I2C driver), this reduces the number of probe
attempts (for each of the two controllers) from more than 2000 to eight.

Reported-by: Steinar H. Gunderson <sgunderson@bigfoot.com>
Signed-off-by: Steinar H. Gunderson <sesse@google.com>
---
 drivers/usb/dwc3/dwc3-exynos.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index dd5cb55..2f1fb7e 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -128,12 +128,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, exynos);
 
-	ret = dwc3_exynos_register_phys(exynos);
-	if (ret) {
-		dev_err(dev, "couldn't register PHYs\n");
-		return ret;
-	}
-
 	exynos->dev	= dev;
 
 	exynos->clk = devm_clk_get(dev, "usbdrd30");
@@ -183,20 +177,29 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 		goto err3;
 	}
 
+	ret = dwc3_exynos_register_phys(exynos);
+	if (ret) {
+		dev_err(dev, "couldn't register PHYs\n");
+		goto err4;
+	}
+
 	if (node) {
 		ret = of_platform_populate(node, NULL, NULL, dev);
 		if (ret) {
 			dev_err(dev, "failed to add dwc3 core\n");
-			goto err4;
+			goto err5;
 		}
 	} else {
 		dev_err(dev, "no device node, failed to add dwc3 core\n");
 		ret = -ENODEV;
-		goto err4;
+		goto err5;
 	}
 
 	return 0;
 
+err5:
+	platform_device_unregister(exynos->usb2_phy);
+	platform_device_unregister(exynos->usb3_phy);
 err4:
 	regulator_disable(exynos->vdd10);
 err3:
-- 
2.8.0.rc3.226.g39d4020

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH] Re: Endless "supply vcc not found, using dummy regulator"
       [not found]                 ` <574478FE.4070800-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2016-05-24 19:24                   ` Steinar H. Gunderson
  2016-05-24 18:13                     ` [PATCH v2] dwc3-exynos: Fix deferred probing storm Steinar H. Gunderson
                                       ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-24 19:24 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Mark Brown, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	823552-61a8vm9lEZVf4u+23C9RwQ, balbi-DgEjT+Ai2ygdnm+yROfE0A,
	linux-usb-u79uwXL29TY76Z2rM5mHXA

On Tue, May 24, 2016 at 05:53:34PM +0200, Krzysztof Kozlowski wrote:
>         exynos->clk = devm_clk_get(dev, "usbdrd30");
>         if (IS_ERR(exynos->clk)) {
> +		// On each error path since here we need to
> +		// revert work done by dwc3_exynos_register_phys()
>                 dev_err(dev, "couldn't get clock\n");
>                 return -EINVAL;
>         }
>         clk_prepare_enable(exynos->clk);

OK, so I took Mark's advice and moved the phy instantiation towards the end
of the function (after the regulators have successfully come up). It reduced
the number of probes, even with the original initramfs, dramatically, so
it seems to work quite well. It also reduces the text for each deferred probe
by a lot, since we no longer have the dummy regulator message for each one
(only the message about “no suspend clk specified” is left). Finally, this
arrangement reduced the need for extra error handling to a minimum.

Cc-ing Felipe and and linux-usb@, and adding the patch as a reply to this
message.

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] dwc3-exynos: Fix deferred probing storm.
  2016-05-24 18:13                     ` [PATCH] " Steinar H. Gunderson
@ 2016-05-25  7:54                       ` Krzysztof Kozlowski
  2016-05-27  9:53                       ` Vivek Gautam
  1 sibling, 0 replies; 31+ messages in thread
From: Krzysztof Kozlowski @ 2016-05-25  7:54 UTC (permalink / raw)
  To: Steinar H. Gunderson; +Cc: broonie, linux-samsung-soc, 823552, balbi, linux-usb

On 05/24/2016 08:13 PM, Steinar H. Gunderson wrote:
> dwc3-exynos has two problems during init if the regulators are slow
> to come up (for instance if the I2C bus driver is not on the initramfs)
> and return probe deferral. First, every time this happens, the driver
> leaks the USB phys created; they need to be deallocated on error.
> 
> Second, since the phy devices are created before the regulators fail,
> this means that there's a new device to re-trigger deferred probing,
> which causes it to essentially go into a busy loop of re-probing the
> device until the regulators come up.
> 
> Move the phy creation to after the regulators have succeeded, and also
> fix cleanup on failure. On my ODROID XU4 system (with Debian's initramfs
> which doesn't contain the I2C driver), this reduces the number of probe
> attempts (for each of the two controllers) from more than 2000 to eight.
> 
> Reported-by: Steinar H. Gunderson <sgunderson@bigfoot.com>
> Signed-off-by: Steinar H. Gunderson <sesse@google.com>

This is the same person so no need for "Reported-by". The reported-by is
used if someone else submits patch after your bug report.

Please (when resubmitting or applying) add following tags:

Fixes: d720f057fda4 ("usb: dwc3: exynos: add nop transceiver support")
Cc: <stable@vger.kernel.org>

This will help downstream (like Debian) using stable kernels.

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

> ---
>  drivers/usb/dwc3/dwc3-exynos.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index dd5cb55..2f1fb7e 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -128,12 +128,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, exynos);
>  
> -	ret = dwc3_exynos_register_phys(exynos);
> -	if (ret) {
> -		dev_err(dev, "couldn't register PHYs\n");
> -		return ret;
> -	}
> -
>  	exynos->dev	= dev;
>  
>  	exynos->clk = devm_clk_get(dev, "usbdrd30");
> @@ -183,20 +177,29 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  		goto err3;
>  	}
>  
> +	ret = dwc3_exynos_register_phys(exynos);
> +	if (ret) {
> +		dev_err(dev, "couldn't register PHYs\n");
> +		goto err4;
> +	}
> +
>  	if (node) {
>  		ret = of_platform_populate(node, NULL, NULL, dev);
>  		if (ret) {
>  			dev_err(dev, "failed to add dwc3 core\n");
> -			goto err4;
> +			goto err5;
>  		}
>  	} else {
>  		dev_err(dev, "no device node, failed to add dwc3 core\n");
>  		ret = -ENODEV;
> -		goto err4;
> +		goto err5;
>  	}
>  
>  	return 0;
>  
> +err5:
> +	platform_device_unregister(exynos->usb2_phy);
> +	platform_device_unregister(exynos->usb3_phy);
>  err4:
>  	regulator_disable(exynos->vdd10);
>  err3:
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-24 19:24                   ` [PATCH] " Steinar H. Gunderson
  2016-05-24 18:13                     ` [PATCH v2] dwc3-exynos: Fix deferred probing storm Steinar H. Gunderson
  2016-05-24 18:13                     ` [PATCH] " Steinar H. Gunderson
@ 2016-05-25 12:16                     ` Anand Moon
  2016-05-25 17:52                       ` Steinar H. Gunderson
  2 siblings, 1 reply; 31+ messages in thread
From: Anand Moon @ 2016-05-25 12:16 UTC (permalink / raw)
  To: Steinar H. Gunderson
  Cc: Krzysztof Kozlowski, Mark Brown, linux-samsung-soc, 823552,
	balbi, linux-usb

Hi Steinar,

On 25 May 2016 at 00:54, Steinar H. Gunderson <sgunderson@bigfoot.com> wrote:
> On Tue, May 24, 2016 at 05:53:34PM +0200, Krzysztof Kozlowski wrote:
>>         exynos->clk = devm_clk_get(dev, "usbdrd30");
>>         if (IS_ERR(exynos->clk)) {
>> +             // On each error path since here we need to
>> +             // revert work done by dwc3_exynos_register_phys()
>>                 dev_err(dev, "couldn't get clock\n");
>>                 return -EINVAL;
>>         }
>>         clk_prepare_enable(exynos->clk);
>
> OK, so I took Mark's advice and moved the phy instantiation towards the end
> of the function (after the regulators have successfully come up). It reduced
> the number of probes, even with the original initramfs, dramatically, so
> it seems to work quite well. It also reduces the text for each deferred probe
> by a lot, since we no longer have the dummy regulator message for each one
> (only the message about “no suspend clk specified” is left). Finally, this
> arrangement reduced the need for extra error handling to a minimum.
>
> Cc-ing Felipe and and linux-usb@, and adding the patch as a reply to this
> message.
>
> /* Steinar */
> --
> Homepage: https://www.sesse.net/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Actually their are some missing patches to tune the usb3 phy.

https://lkml.org/lkml/2014/10/31/266

Best Regards
-Anand Moon

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-25 12:16                     ` [PATCH] Re: Endless "supply vcc not found, using dummy regulator" Anand Moon
@ 2016-05-25 17:52                       ` Steinar H. Gunderson
  2016-05-26 12:57                         ` Steinar H. Gunderson
  0 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-25 17:52 UTC (permalink / raw)
  To: Anand Moon
  Cc: Krzysztof Kozlowski, Mark Brown, linux-samsung-soc, 823552,
	balbi, linux-usb

On Wed, May 25, 2016 at 05:46:51PM +0530, Anand Moon wrote:
> Actually their are some missing patches to tune the usb3 phy.
> 
> https://lkml.org/lkml/2014/10/31/266

This explains why the default networking speed refused to go above
~300 Mbit/sec! What happened to the patches, I wonder?

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Re: Endless "supply vcc not found, using dummy regulator"
  2016-05-25 17:52                       ` Steinar H. Gunderson
@ 2016-05-26 12:57                         ` Steinar H. Gunderson
       [not found]                           ` <20160526125757.GA23797-gdzBep0Ce9heoWH0uzbU5w@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-26 12:57 UTC (permalink / raw)
  To: Anand Moon
  Cc: Krzysztof Kozlowski, Mark Brown, linux-samsung-soc, 823552,
	balbi, linux-usb, gautam.vivek

On Wed, May 25, 2016 at 07:52:36PM +0200, Steinar H. Gunderson wrote:
>> Actually their are some missing patches to tune the usb3 phy.
>> 
>> https://lkml.org/lkml/2014/10/31/266
> This explains why the default networking speed refused to go above
> ~300 Mbit/sec! What happened to the patches, I wonder?

Adding Vivek in case he knows. They certainly don't apply anymore, at least.

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Re: Endless "supply vcc not found, using dummy regulator"
       [not found]                           ` <20160526125757.GA23797-gdzBep0Ce9heoWH0uzbU5w@public.gmane.org>
@ 2016-05-27  9:32                             ` Vivek Gautam
       [not found]                               ` <CAFp+6iE_QW0BDO-Z1Ce5zrUJiToWD_8UtnCFnfKLeaYyys7U8A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Vivek Gautam @ 2016-05-27  9:32 UTC (permalink / raw)
  To: Steinar H. Gunderson
  Cc: Anand Moon, Krzysztof Kozlowski, Mark Brown,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	823552-61a8vm9lEZVf4u+23C9RwQ, balbi-DgEjT+Ai2ygdnm+yROfE0A,
	Linux USB Mailing List, Vivek Gautam

On Thu, May 26, 2016 at 6:27 PM, Steinar H. Gunderson
<sgunderson-jG/AHqQBv7lBDgjK7y7TUQ@public.gmane.org> wrote:
> On Wed, May 25, 2016 at 07:52:36PM +0200, Steinar H. Gunderson wrote:
>>> Actually their are some missing patches to tune the usb3 phy.
>>>
>>> https://lkml.org/lkml/2014/10/31/266
>> This explains why the default networking speed refused to go above
>> ~300 Mbit/sec! What happened to the patches, I wonder?
>
> Adding Vivek in case he knows. They certainly don't apply anymore, at least.

Above mentioned patches were not accepted by the maintainers of generic-phy
and usb. I couldn't get any response on them for quite a long time. So, the
patches could never make it to the mainline.
I can try initiating the entire exercise once again and try to get them merged.

>From the thread, i can't make out the configuration of the board you are using.
If the network is coming out of the USB 3.0 phy, then definitely you will see
a performance drop.

AFA dwc3-exynos is concerned, there's definitely a resource leak as
pointed out by you.
Please post the suggested patch, adding required people in CC.

>
> /* Steinar */
> --
> Homepage: https://www.sesse.net/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Re: Endless "supply vcc not found, using dummy regulator"
       [not found]                               ` <CAFp+6iE_QW0BDO-Z1Ce5zrUJiToWD_8UtnCFnfKLeaYyys7U8A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-05-27  9:39                                 ` Steinar H. Gunderson
       [not found]                                   ` <20160527093919.GA34987-gdzBep0Ce9heoWH0uzbU5w@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-27  9:39 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Anand Moon, Krzysztof Kozlowski, Mark Brown,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	823552-61a8vm9lEZVf4u+23C9RwQ, balbi-DgEjT+Ai2ygdnm+yROfE0A,
	Linux USB Mailing List, Vivek Gautam

On Fri, May 27, 2016 at 03:02:48PM +0530, Vivek Gautam wrote:
> Above mentioned patches were not accepted by the maintainers of generic-phy
> and usb. I couldn't get any response on them for quite a long time. So, the
> patches could never make it to the mainline.
> I can try initiating the entire exercise once again and try to get them merged.

That would be nice; having USB3 speeds is certainly attractive.

> AFA dwc3-exynos is concerned, there's definitely a resource leak as
> pointed out by you.
> Please post the suggested patch, adding required people in CC.

http://www.spinics.net/lists/linux-usb/msg141385.html

Is there anyone I should have Cc-ed that I didn't?

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Re: Endless "supply vcc not found, using dummy regulator"
       [not found]                                   ` <20160527093919.GA34987-gdzBep0Ce9heoWH0uzbU5w@public.gmane.org>
@ 2016-05-27  9:43                                     ` Vivek Gautam
  0 siblings, 0 replies; 31+ messages in thread
From: Vivek Gautam @ 2016-05-27  9:43 UTC (permalink / raw)
  To: Steinar H. Gunderson
  Cc: Anand Moon, Krzysztof Kozlowski, Mark Brown,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	823552-61a8vm9lEZVf4u+23C9RwQ, balbi-DgEjT+Ai2ygdnm+yROfE0A,
	Linux USB Mailing List, Vivek Gautam

On Fri, May 27, 2016 at 3:09 PM, Steinar H. Gunderson
<sgunderson-jG/AHqQBv7lBDgjK7y7TUQ@public.gmane.org> wrote:
> On Fri, May 27, 2016 at 03:02:48PM +0530, Vivek Gautam wrote:
>> Above mentioned patches were not accepted by the maintainers of generic-phy
>> and usb. I couldn't get any response on them for quite a long time. So, the
>> patches could never make it to the mainline.
>> I can try initiating the entire exercise once again and try to get them merged.
>
> That would be nice; having USB3 speeds is certainly attractive.
>
>> AFA dwc3-exynos is concerned, there's definitely a resource leak as
>> pointed out by you.
>> Please post the suggested patch, adding required people in CC.
>
> http://www.spinics.net/lists/linux-usb/msg141385.html
>
> Is there anyone I should have Cc-ed that I didn't?

That's alright, i missed noticing your patch.

>
> /* Steinar */
> --
> Homepage: https://www.sesse.net/



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] dwc3-exynos: Fix deferred probing storm.
  2016-05-24 18:13                     ` [PATCH] " Steinar H. Gunderson
  2016-05-25  7:54                       ` Krzysztof Kozlowski
@ 2016-05-27  9:53                       ` Vivek Gautam
  2016-05-27 11:46                         ` Steinar H. Gunderson
  1 sibling, 1 reply; 31+ messages in thread
From: Vivek Gautam @ 2016-05-27  9:53 UTC (permalink / raw)
  To: Steinar H. Gunderson
  Cc: Krzysztof Kozłowski, Mark Brown, linux-samsung-soc, 823552,
	balbi, Linux USB Mailing List

On Tue, May 24, 2016 at 11:43 PM, Steinar H. Gunderson <sesse@google.com> wrote:
> dwc3-exynos has two problems during init if the regulators are slow
> to come up (for instance if the I2C bus driver is not on the initramfs)
> and return probe deferral. First, every time this happens, the driver
> leaks the USB phys created; they need to be deallocated on error.
>
> Second, since the phy devices are created before the regulators fail,
> this means that there's a new device to re-trigger deferred probing,
> which causes it to essentially go into a busy loop of re-probing the
> device until the regulators come up.
>
> Move the phy creation to after the regulators have succeeded, and also
> fix cleanup on failure. On my ODROID XU4 system (with Debian's initramfs
> which doesn't contain the I2C driver), this reduces the number of probe
> attempts (for each of the two controllers) from more than 2000 to eight.
>
> Reported-by: Steinar H. Gunderson <sgunderson@bigfoot.com>
> Signed-off-by: Steinar H. Gunderson <sesse@google.com>
> ---

I don't have any concerns with the patch apart from the ones
Krzysztof has already pointed out.
LGTM.

After fixing the patch,
Reviewed-by: Vivek Gautam <gautam.vivek@samsung.com>

>  drivers/usb/dwc3/dwc3-exynos.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index dd5cb55..2f1fb7e 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -128,12 +128,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, exynos);
>
> -       ret = dwc3_exynos_register_phys(exynos);
> -       if (ret) {
> -               dev_err(dev, "couldn't register PHYs\n");
> -               return ret;
> -       }
> -
>         exynos->dev     = dev;
>
>         exynos->clk = devm_clk_get(dev, "usbdrd30");
> @@ -183,20 +177,29 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>                 goto err3;
>         }
>
> +       ret = dwc3_exynos_register_phys(exynos);
> +       if (ret) {
> +               dev_err(dev, "couldn't register PHYs\n");
> +               goto err4;
> +       }
> +
>         if (node) {
>                 ret = of_platform_populate(node, NULL, NULL, dev);
>                 if (ret) {
>                         dev_err(dev, "failed to add dwc3 core\n");
> -                       goto err4;
> +                       goto err5;
>                 }
>         } else {
>                 dev_err(dev, "no device node, failed to add dwc3 core\n");
>                 ret = -ENODEV;
> -               goto err4;
> +               goto err5;
>         }
>
>         return 0;
>
> +err5:
> +       platform_device_unregister(exynos->usb2_phy);
> +       platform_device_unregister(exynos->usb3_phy);
>  err4:
>         regulator_disable(exynos->vdd10);
>  err3:
> --
> 2.8.0.rc3.226.g39d4020
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] dwc3-exynos: Fix deferred probing storm.
  2016-05-27  9:53                       ` Vivek Gautam
@ 2016-05-27 11:46                         ` Steinar H. Gunderson
  2016-05-27 13:13                           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-27 11:46 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Steinar H. Gunderson, Krzysztof Kozłowski, Mark Brown,
	linux-samsung-soc, 823552, balbi, Linux USB Mailing List

On Fri, May 27, 2016 at 03:23:35PM +0530, Vivek Gautam wrote:
> I don't have any concerns with the patch apart from the ones
> Krzysztof has already pointed out.
> LGTM.

Should I repost the patch, or will people just make these commit message
changes for me? I guess balbi@ is the right person to review and merge,
since he maintains the driver.

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] dwc3-exynos: Fix deferred probing storm.
       [not found]                             ` <5748480D.8000501-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2016-05-27 13:12                               ` Felipe Balbi
       [not found]                                 ` <87bn3rprtw.fsf-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Felipe Balbi @ 2016-05-27 13:12 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Steinar H. Gunderson, Vivek Gautam
  Cc: Steinar H. Gunderson, Mark Brown,
	linux-samsung-soc@vger.kernel.org, 823552-61a8vm9lEZVf4u+23C9RwQ,
	Linux USB Mailing List

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]


Hi,

Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> writes:
> On 05/27/2016 01:46 PM, Steinar H. Gunderson wrote:
>> On Fri, May 27, 2016 at 03:23:35PM +0530, Vivek Gautam wrote:
>>> I don't have any concerns with the patch apart from the ones
>>> Krzysztof has already pointed out.
>>> LGTM.
>> 
>> Should I repost the patch, or will people just make these commit message
>> changes for me? I guess balbi@ is the right person to review and merge,
>> since he maintains the driver.
>
> Although resend is not strictly needed because maintainer might do the
> changes by himself... but he might just miss the comments as well (and
> e.g. use patchwork to apply the patch).
>
> I think it would be useful if you send a v2 with fixes pointed by me and
> accumulated review-tags (mine and Vivek's Reviewed-by).

yes, please do that. Keep in mind, also, that we're still in the middle
of the merge window and nothing will really happen until v4.7-rc1 is
tagged.

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] dwc3-exynos: Fix deferred probing storm.
  2016-05-27 11:46                         ` Steinar H. Gunderson
@ 2016-05-27 13:13                           ` Krzysztof Kozlowski
       [not found]                             ` <5748480D.8000501-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Krzysztof Kozlowski @ 2016-05-27 13:13 UTC (permalink / raw)
  To: Steinar H. Gunderson, Vivek Gautam
  Cc: Steinar H. Gunderson, Mark Brown, linux-samsung-soc, 823552,
	balbi, Linux USB Mailing List

On 05/27/2016 01:46 PM, Steinar H. Gunderson wrote:
> On Fri, May 27, 2016 at 03:23:35PM +0530, Vivek Gautam wrote:
>> I don't have any concerns with the patch apart from the ones
>> Krzysztof has already pointed out.
>> LGTM.
> 
> Should I repost the patch, or will people just make these commit message
> changes for me? I guess balbi@ is the right person to review and merge,
> since he maintains the driver.

Although resend is not strictly needed because maintainer might do the
changes by himself... but he might just miss the comments as well (and
e.g. use patchwork to apply the patch).

I think it would be useful if you send a v2 with fixes pointed by me and
accumulated review-tags (mine and Vivek's Reviewed-by).

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] dwc3-exynos: Fix deferred probing storm.
       [not found]                                 ` <87bn3rprtw.fsf-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
@ 2016-05-27 13:25                                   ` Steinar H. Gunderson
  2016-05-27 13:26                                     ` Felipe Balbi
  0 siblings, 1 reply; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-27 13:25 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Krzysztof Kozlowski, Vivek Gautam, Steinar H. Gunderson,
	Mark Brown, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	823552-61a8vm9lEZVf4u+23C9RwQ, Linux USB Mailing List

On Fri, May 27, 2016 at 04:12:59PM +0300, Felipe Balbi wrote:
> yes, please do that. Keep in mind, also, that we're still in the middle
> of the merge window and nothing will really happen until v4.7-rc1 is
> tagged.

Sent. As a fix, there's a chance it could go into 4.7, right?

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] dwc3-exynos: Fix deferred probing storm.
  2016-05-27 13:25                                   ` Steinar H. Gunderson
@ 2016-05-27 13:26                                     ` Felipe Balbi
  2016-05-30 17:46                                       ` Steinar H. Gunderson
  0 siblings, 1 reply; 31+ messages in thread
From: Felipe Balbi @ 2016-05-27 13:26 UTC (permalink / raw)
  To: Steinar H. Gunderson
  Cc: Krzysztof Kozlowski, Vivek Gautam, Steinar H. Gunderson,
	Mark Brown, linux-samsung-soc, 823552, Linux USB Mailing List

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]


Hi,

"Steinar H. Gunderson" <sgunderson@bigfoot.com> writes:
> On Fri, May 27, 2016 at 04:12:59PM +0300, Felipe Balbi wrote:
>> yes, please do that. Keep in mind, also, that we're still in the middle
>> of the merge window and nothing will really happen until v4.7-rc1 is
>> tagged.
>
> Sent. As a fix, there's a chance it could go into 4.7, right?

yup, shouldn't be a problem. But only after v4.7-rc1 is tagged.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] dwc3-exynos: Fix deferred probing storm.
  2016-05-27 13:26                                     ` Felipe Balbi
@ 2016-05-30 17:46                                       ` Steinar H. Gunderson
  0 siblings, 0 replies; 31+ messages in thread
From: Steinar H. Gunderson @ 2016-05-30 17:46 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Krzysztof Kozlowski, Vivek Gautam, Steinar H. Gunderson,
	Mark Brown, linux-samsung-soc, 823552, Linux USB Mailing List

On Fri, May 27, 2016 at 04:26:42PM +0300, Felipe Balbi wrote:
>> Sent. As a fix, there's a chance it could go into 4.7, right?
> yup, shouldn't be a problem. But only after v4.7-rc1 is tagged.

Seemingly v4.7-rc1 is out today (I was surprised at how quick that was).

/* Steinar */
-- 
Homepage: https://www.sesse.net/

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2016-05-30 17:46 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20160515173900.GC21194@sesse.net>
2016-05-21 14:43 ` Endless "supply vcc not found, using dummy regulator" Steinar H. Gunderson
2016-05-23 13:47   ` Steinar H. Gunderson
2016-05-23 14:40     ` Steinar H. Gunderson
2016-05-23 16:24       ` Mark Brown
2016-05-23 17:06         ` Steinar H. Gunderson
2016-05-23 17:46           ` Steinar H. Gunderson
2016-05-23 18:56             ` Mark Brown
2016-05-23 18:44           ` Mark Brown
2016-05-24 15:06           ` Krzysztof Kozlowski
2016-05-24 15:26             ` Steinar H. Gunderson
2016-05-24 15:45               ` Mark Brown
2016-05-24 15:53               ` Krzysztof Kozlowski
2016-05-24 16:44                 ` Steinar H. Gunderson
     [not found]                 ` <574478FE.4070800-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2016-05-24 19:24                   ` [PATCH] " Steinar H. Gunderson
2016-05-24 18:13                     ` [PATCH v2] dwc3-exynos: Fix deferred probing storm Steinar H. Gunderson
2016-05-24 18:13                     ` [PATCH] " Steinar H. Gunderson
2016-05-25  7:54                       ` Krzysztof Kozlowski
2016-05-27  9:53                       ` Vivek Gautam
2016-05-27 11:46                         ` Steinar H. Gunderson
2016-05-27 13:13                           ` Krzysztof Kozlowski
     [not found]                             ` <5748480D.8000501-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2016-05-27 13:12                               ` Felipe Balbi
     [not found]                                 ` <87bn3rprtw.fsf-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2016-05-27 13:25                                   ` Steinar H. Gunderson
2016-05-27 13:26                                     ` Felipe Balbi
2016-05-30 17:46                                       ` Steinar H. Gunderson
2016-05-25 12:16                     ` [PATCH] Re: Endless "supply vcc not found, using dummy regulator" Anand Moon
2016-05-25 17:52                       ` Steinar H. Gunderson
2016-05-26 12:57                         ` Steinar H. Gunderson
     [not found]                           ` <20160526125757.GA23797-gdzBep0Ce9heoWH0uzbU5w@public.gmane.org>
2016-05-27  9:32                             ` Vivek Gautam
     [not found]                               ` <CAFp+6iE_QW0BDO-Z1Ce5zrUJiToWD_8UtnCFnfKLeaYyys7U8A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-05-27  9:39                                 ` Steinar H. Gunderson
     [not found]                                   ` <20160527093919.GA34987-gdzBep0Ce9heoWH0uzbU5w@public.gmane.org>
2016-05-27  9:43                                     ` Vivek Gautam
2016-05-24 15:39             ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.