Endless "supply vcc not found, using dummy regulator"
diff mbox

Message ID 20160523134737.GA21683@sesse.net
State Not Applicable
Headers show

Commit Message

Steinar H. Gunderson May 23, 2016, 1:47 p.m. UTC
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(+)


/* Steinar */

Comments

Steinar H. Gunderson May 23, 2016, 2:40 p.m. UTC | #1
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 */
Mark Brown May 23, 2016, 4:24 p.m. UTC | #2
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.
Steinar H. Gunderson May 23, 2016, 5:06 p.m. UTC | #3
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 */
Steinar H. Gunderson May 23, 2016, 5:46 p.m. UTC | #4
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 */
Mark Brown May 23, 2016, 6:44 p.m. UTC | #5
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.
Mark Brown May 23, 2016, 6:56 p.m. UTC | #6
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.
Krzysztof Kozlowski May 24, 2016, 3:06 p.m. UTC | #7
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
--
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
Steinar H. Gunderson May 24, 2016, 3:26 p.m. UTC | #8
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 */
Mark Brown May 24, 2016, 3:39 p.m. UTC | #9
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.
Mark Brown May 24, 2016, 3:45 p.m. UTC | #10
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.
Krzysztof Kozlowski May 24, 2016, 3:53 p.m. UTC | #11
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
--
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
Steinar H. Gunderson May 24, 2016, 4:44 p.m. UTC | #12
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 */
Steinar H. Gunderson May 24, 2016, 7:24 p.m. UTC | #13
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 */
Anand Moon May 25, 2016, 12:16 p.m. UTC | #14
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
--
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
Steinar H. Gunderson May 25, 2016, 5:52 p.m. UTC | #15
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 */
Steinar H. Gunderson May 26, 2016, 12:57 p.m. UTC | #16
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 */
Vivek Gautam May 27, 2016, 9:32 a.m. UTC | #17
On Thu, May 26, 2016 at 6:27 PM, Steinar H. Gunderson
<sgunderson@bigfoot.com> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steinar H. Gunderson May 27, 2016, 9:39 a.m. UTC | #18
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 */
Vivek Gautam May 27, 2016, 9:43 a.m. UTC | #19
On Fri, May 27, 2016 at 3:09 PM, Steinar H. Gunderson
<sgunderson@bigfoot.com> 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/

Patch
diff mbox

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);