[PATCH] usb: misc: usb3503: Force late initialization

[PATCH] usb: misc: usb3503: Force late initialization

[Tushar Behera]

USB3503 chip needs to be reset after the USB PHY controller has been
intiliazed, otherwise it is not detected as plugged in.

Currently there is no other way to ensure that USB3503 chip is probed
after the USB PHY has been initialized, hence the last resort.

Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
---
 drivers/usb/misc/usb3503.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
index a31641e..ad8368d 100644
--- a/drivers/usb/misc/usb3503.c
+++ b/drivers/usb/misc/usb3503.c
@@ -339,7 +339,7 @@ static int __init usb3503_init(void)
 
 	return 0;
 }
-module_init(usb3503_init);
+late_initcall(usb3503_init);
 
 static void __exit usb3503_exit(void)
 {
-- 
1.7.9.5

Re: [PATCH] usb: misc: usb3503: Force late initialization

[Greg KH]

On Wed, Aug 14, 2013 at 03:59:31PM +0530, Tushar Behera wrote:
> USB3503 chip needs to be reset after the USB PHY controller has been
> intiliazed, otherwise it is not detected as plugged in.

Why not?

> Currently there is no other way to ensure that USB3503 chip is probed
> after the USB PHY has been initialized, hence the last resort.

Are you sure that deferred probing doesn't solve this issue?

> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
> ---
>  drivers/usb/misc/usb3503.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
> index a31641e..ad8368d 100644
> --- a/drivers/usb/misc/usb3503.c
> +++ b/drivers/usb/misc/usb3503.c
> @@ -339,7 +339,7 @@ static int __init usb3503_init(void)
>  
>  	return 0;
>  }
> -module_init(usb3503_init);
> +late_initcall(usb3503_init);

I really hate stuff like this, that is what the change to the driver
core for the deferred probe work was for, could you please look into
that instead?

thanks,

greg k-h

Re: [PATCH] usb: misc: usb3503: Force late initialization

[Mark Brown]

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

On Wed, Aug 14, 2013 at 11:22:39AM -0700, Greg KH wrote:
> On Wed, Aug 14, 2013 at 03:59:31PM +0530, Tushar Behera wrote:

> > Currently there is no other way to ensure that USB3503 chip is probed
> > after the USB PHY has been initialized, hence the last resort.

> Are you sure that deferred probing doesn't solve this issue?

In order for deferred probing to help the device would need to acquire
some resource from the parent USB controller once active, allowing it to
defer when it fails to get that resource.  Currently there's nothing
there for that and it's not clear that's sane and sensible (it'd be more
obvious for it to be a child of the USB controller since that's what it
really is).  

We can't just treat it as a PHY (which is the obvious workaroud) since
we do also need to use the built
in PHY in the SoC.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: misc: usb3503: Force late initialization

[Greg KH]

On Wed, Aug 14, 2013 at 08:04:10PM +0100, Mark Brown wrote:
> On Wed, Aug 14, 2013 at 11:22:39AM -0700, Greg KH wrote:
> > On Wed, Aug 14, 2013 at 03:59:31PM +0530, Tushar Behera wrote:
> 
> > > Currently there is no other way to ensure that USB3503 chip is probed
> > > after the USB PHY has been initialized, hence the last resort.
> 
> > Are you sure that deferred probing doesn't solve this issue?
> 
> In order for deferred probing to help the device would need to acquire
> some resource from the parent USB controller once active, allowing it to
> defer when it fails to get that resource.  Currently there's nothing
> there for that and it's not clear that's sane and sensible (it'd be more
> obvious for it to be a child of the USB controller since that's what it
> really is).

Then why not do that?

> We can't just treat it as a PHY (which is the obvious workaroud) since
> we do also need to use the built
> in PHY in the SoC.

There has to be some type of resource that it can grab, as obviously
it's failing to work properly unless that resource is present.  Just
messing with the init order isn't going to solve any problem if the
driver is built as a module, as nothing guarantees module load order.

So this patch wouldn't really solve the problem, only paper over it for
one type of configuration (i.e. driver built into the system), right?

thanks,

greg k-h

Re: [PATCH] usb: misc: usb3503: Force late initialization

[Mark Brown]

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

On Wed, Aug 14, 2013 at 12:15:13PM -0700, Greg KH wrote:
> On Wed, Aug 14, 2013 at 08:04:10PM +0100, Mark Brown wrote:

> > In order for deferred probing to help the device would need to acquire
> > some resource from the parent USB controller once active, allowing it to
> > defer when it fails to get that resource.  Currently there's nothing
> > there for that and it's not clear that's sane and sensible (it'd be more
> > obvious for it to be a child of the USB controller since that's what it
> > really is).

> Then why not do that?

That's why I started the thread about registering devices prior to
learning that they exist; it's a non-trivial bit of work at least from
the everyone agreeing point of view.

> > We can't just treat it as a PHY (which is the obvious workaroud) since
> > we do also need to use the built
> > in PHY in the SoC.

> There has to be some type of resource that it can grab, as obviously
> it's failing to work properly unless that resource is present.  Just

That resource is the USB bus I think (I suspect the issue is that the
fact that power is always present confuses the USB enumeration protocol
if the device gets brought out of reset prior to the bus being live).
The normal way to grab that resource would be to make the device a
device on the bus but currently the only way USB gets children is via
USB enumeration.

> messing with the init order isn't going to solve any problem if the
> driver is built as a module, as nothing guarantees module load order.

Right, and I did discuss that with Tushar elsehwere prior to this being
posted here.  We figured given how cheap and non-invasive the workaround
is it was worth just doing it.

> So this patch wouldn't really solve the problem, only paper over it for
> one type of configuration (i.e. driver built into the system), right?

Yes, though realistically nobody actually does that for the relevant
systems and if they do it's always possible to control the module load
order if you really want to.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: misc: usb3503: Force late initialization

[Greg KH]

On Wed, Aug 14, 2013 at 08:39:50PM +0100, Mark Brown wrote:
> > > We can't just treat it as a PHY (which is the obvious workaroud) since
> > > we do also need to use the built
> > > in PHY in the SoC.
> 
> > There has to be some type of resource that it can grab, as obviously
> > it's failing to work properly unless that resource is present.  Just
> 
> That resource is the USB bus I think (I suspect the issue is that the
> fact that power is always present confuses the USB enumeration protocol
> if the device gets brought out of reset prior to the bus being live).

Which USB bus is it?  How is this device even probed before the device
is found by the bus?

Ah crud, this is an i2c driver, not a USB driver, no wonder I'm
confused...

You are going to have to find some kind of relationship here, not just
by linker order, otherwise you are going to have problems later on.

> The normal way to grab that resource would be to make the device a
> device on the bus but currently the only way USB gets children is via
> USB enumeration.

Perhaps make this device a child of the USB controller in the DT
description?

> > messing with the init order isn't going to solve any problem if the
> > driver is built as a module, as nothing guarantees module load order.
> 
> Right, and I did discuss that with Tushar elsehwere prior to this being
> posted here.  We figured given how cheap and non-invasive the workaround
> is it was worth just doing it.

Nope, sorry, I'm not going to take it.

> > So this patch wouldn't really solve the problem, only paper over it for
> > one type of configuration (i.e. driver built into the system), right?
> 
> Yes, though realistically nobody actually does that for the relevant
> systems and if they do it's always possible to control the module load
> order if you really want to.

But you aren't telling anyone that is what is needed.  How would anyone
know that they need to control the load order?

thanks,

greg k-h

Re: [PATCH] usb: misc: usb3503: Force late initialization

[Mark Brown]

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

On Wed, Aug 14, 2013 at 12:47:44PM -0700, Greg KH wrote:
> On Wed, Aug 14, 2013 at 08:39:50PM +0100, Mark Brown wrote:

> > That resource is the USB bus I think (I suspect the issue is that the
> > fact that power is always present confuses the USB enumeration protocol
> > if the device gets brought out of reset prior to the bus being live).

> Which USB bus is it?  How is this device even probed before the device
> is found by the bus?

> Ah crud, this is an i2c driver, not a USB driver, no wonder I'm
> confused...

Well, in this case it's actually only got the GPIOs wired up so
fortunately we don't care about the I2C case.  This was why I did all
that stuff with the platform device - it was only once I'd done that
that I realised that it wasn't enough just to set the GPIOs but that
they needed to be set at a particular time.  Having to cope with I2C
would indeed be a pain and it's one that I'm not even trying to confront
right now.

> You are going to have to find some kind of relationship here, not just
> by linker order, otherwise you are going to have problems later on.

Oh, clearly - it's not a solution, it's just a really cheap workaround.

> > The normal way to grab that resource would be to make the device a
> > device on the bus but currently the only way USB gets children is via
> > USB enumeration.

> Perhaps make this device a child of the USB controller in the DT
> description?

Yeah, that's the ideal thing and one of the cases I'm thinking of with
the whole thing about telling buses that they have children with device
data before the devices get probed.  People probably also want this to
work for ACPI, or at least would find it convenient to have that - I
first looked at USB due to chatting to one those guys.

> > Right, and I did discuss that with Tushar elsehwere prior to this being
> > posted here.  We figured given how cheap and non-invasive the workaround
> > is it was worth just doing it.

> Nope, sorry, I'm not going to take it.

Fair enough, that was my first reaction to it too but we figured it was
worth considering.

> > > So this patch wouldn't really solve the problem, only paper over it for
> > > one type of configuration (i.e. driver built into the system), right?

> > Yes, though realistically nobody actually does that for the relevant
> > systems and if they do it's always possible to control the module load
> > order if you really want to.

> But you aren't telling anyone that is what is needed.  How would anyone
> know that they need to control the load order?

By debugging it and finding the late initcall.  Worst case it'd work
just as well as it does now, best case it might do better until the
problem actually gets fixed.

Like I say for all practical purposes nobody does that, everyone running
these boards builds everything into the kernel - that's the model for
Android (which is the only thing the board vendor supports) and
realistically it's not a system you build general purpose things for
even in development.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]