Patchwork [04/11] ARM: OMAP2+: usb_host_fs: add custom reset for usb_host_fs (fsusb)

login
register
mail settings
Submitter Paul Walmsley
Date June 7, 2012, 6:13 a.m.
Message ID <20120607061308.25532.19767.stgit@dusk>
Download mbox | patch
Permalink /patch/163485/
State New
Headers show

Comments

Paul Walmsley - June 7, 2012, 6:13 a.m.
From: Tero Kristo <t-kristo@ti.com>

Add a custom reset function for the usb_host_fs/fsusb IP block, and
connect it to the OMAP4 FSUSB block.

This is the first of two fixes required to get rid of the boot
warning:

omap_hwmod: usb_host_fs: _wait_target_disable failed

and to allow the module to idle.

It may be necessary to use this reset method for OMAP2xxx SoCs as
well; this is left for a future patch.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
[paul@pwsan.com: rewrote the custom reset function, documented it and
 updated the commit message, and moved the code to mach-omap2/fs-usb.c]
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: BenoƮt Cousson <b-cousson@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
---
 arch/arm/mach-omap2/Makefile               |    4 --
 arch/arm/mach-omap2/cm.h                   |    8 +++-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    1 
 arch/arm/mach-omap2/usb-fs.c               |   62 ++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/usb.h      |    3 +
 5 files changed, 73 insertions(+), 5 deletions(-)
Tony Lindgren - June 7, 2012, 7:31 a.m.
* Paul Walmsley <paul@pwsan.com> [120606 23:26]:
> From: Tero Kristo <t-kristo@ti.com>
> 
> Add a custom reset function for the usb_host_fs/fsusb IP block, and
> connect it to the OMAP4 FSUSB block.
> 
> This is the first of two fixes required to get rid of the boot
> warning:
> 
> omap_hwmod: usb_host_fs: _wait_target_disable failed
> 
> and to allow the module to idle.
> 
> It may be necessary to use this reset method for OMAP2xxx SoCs as
> well; this is left for a future patch.

Here too I think driver like features like this should live in the
driver init for omap OHCI driver. In the likely case that FS OHCI is
not in use on the board, the OHCI glue can just reset it.
 
> +/* HCCOMMANDSTATUS: the register offset of the HCCOMMANDSTATUS register */
> +#define HCCOMMANDSTATUS			0x0008
> +
> +/* HCCOMMANDSTATUS_HCR: the bitmask of the host controller reset flag */
> +#define HCCOMMANDSTATUS_HCR_MASK	(1 << 0)

I don't think the bus layer code should need to know anything about driver
specific registers.

> +	omap_hwmod_write(HCCOMMANDSTATUS_HCR_MASK, oh, HCCOMMANDSTATUS);
> +
> +	omap_test_timeout(!(omap_hwmod_read(oh, HCCOMMANDSTATUS)
> +			    & HCCOMMANDSTATUS_HCR_MASK),
> +			  MAX_MODULE_SOFTRESET_WAIT, c);
> +

These should be accessed by the driver in a standard way after ioremapping
the device.

Tony
Felipe Balbi - June 7, 2012, 7:33 a.m.
Hi,

On Thu, Jun 07, 2012 at 12:31:13AM -0700, Tony Lindgren wrote:
> * Paul Walmsley <paul@pwsan.com> [120606 23:26]:
> > From: Tero Kristo <t-kristo@ti.com>
> > 
> > Add a custom reset function for the usb_host_fs/fsusb IP block, and
> > connect it to the OMAP4 FSUSB block.
> > 
> > This is the first of two fixes required to get rid of the boot
> > warning:
> > 
> > omap_hwmod: usb_host_fs: _wait_target_disable failed
> > 
> > and to allow the module to idle.
> > 
> > It may be necessary to use this reset method for OMAP2xxx SoCs as
> > well; this is left for a future patch.
> 
> Here too I think driver like features like this should live in the
> driver init for omap OHCI driver. In the likely case that FS OHCI is
> not in use on the board, the OHCI glue can just reset it.

this I don't agree. It means we will have to add some OHCI code even
when OHCI is disabled.

> > +/* HCCOMMANDSTATUS: the register offset of the HCCOMMANDSTATUS register */
> > +#define HCCOMMANDSTATUS			0x0008
> > +
> > +/* HCCOMMANDSTATUS_HCR: the bitmask of the host controller reset flag */
> > +#define HCCOMMANDSTATUS_HCR_MASK	(1 << 0)
> 
> I don't think the bus layer code should need to know anything about driver
> specific registers.
> 
> > +	omap_hwmod_write(HCCOMMANDSTATUS_HCR_MASK, oh, HCCOMMANDSTATUS);
> > +
> > +	omap_test_timeout(!(omap_hwmod_read(oh, HCCOMMANDSTATUS)
> > +			    & HCCOMMANDSTATUS_HCR_MASK),
> > +			  MAX_MODULE_SOFTRESET_WAIT, c);
> > +
> 
> These should be accessed by the driver in a standard way after ioremapping
> the device.

agree.
Paul Walmsley - June 7, 2012, 7:40 a.m.
On Thu, 7 Jun 2012, Tony Lindgren wrote:

> Here too I think driver like features like this should live in the
> driver init for omap OHCI driver. In the likely case that FS OHCI is
> not in use on the board, the OHCI glue can just reset it.

What if the driver is not compiled into the kernel, but instead is built 
as a loadable module?


- Paul
Tony Lindgren - June 7, 2012, 7:51 a.m.
* Paul Walmsley <paul@pwsan.com> [120607 00:44]:
> On Thu, 7 Jun 2012, Tony Lindgren wrote:
> 
> > Here too I think driver like features like this should live in the
> > driver init for omap OHCI driver. In the likely case that FS OHCI is
> > not in use on the board, the OHCI glue can just reset it.
> 
> What if the driver is not compiled into the kernel, but instead is built 
> as a loadable module?

You can still have a core piece of the driver that's always built in, such
as omap-ohci-common. But it should live under drivers, not in the bus level
code. Or you can insmod/rmmod it to reset things properly.

Regards,

Tony
Felipe Balbi - June 7, 2012, 7:55 a.m.
On Thu, Jun 07, 2012 at 12:51:58AM -0700, Tony Lindgren wrote:
> * Paul Walmsley <paul@pwsan.com> [120607 00:44]:
> > On Thu, 7 Jun 2012, Tony Lindgren wrote:
> > 
> > > Here too I think driver like features like this should live in the
> > > driver init for omap OHCI driver. In the likely case that FS OHCI is
> > > not in use on the board, the OHCI glue can just reset it.
> > 
> > What if the driver is not compiled into the kernel, but instead is built 
> > as a loadable module?
> 
> You can still have a core piece of the driver that's always built in, such
> as omap-ohci-common. But it should live under drivers, not in the bus level
> code. Or you can insmod/rmmod it to reset things properly.

that's such a hack... both solutions are quite hacky. The only problem
here is because some bootloaders are leaving controller in an unknown
state and I guess to be completely safe, resets should be done before
any driver kicks in.

But, driver will probably reset again the IP block during probe...
Tony Lindgren - June 7, 2012, 8 a.m.
* Felipe Balbi <balbi@ti.com> [120607 00:39]:
> Hi,
> 
> On Thu, Jun 07, 2012 at 12:31:13AM -0700, Tony Lindgren wrote:
> > * Paul Walmsley <paul@pwsan.com> [120606 23:26]:
> > > From: Tero Kristo <t-kristo@ti.com>
> > > 
> > > Add a custom reset function for the usb_host_fs/fsusb IP block, and
> > > connect it to the OMAP4 FSUSB block.
> > > 
> > > This is the first of two fixes required to get rid of the boot
> > > warning:
> > > 
> > > omap_hwmod: usb_host_fs: _wait_target_disable failed
> > > 
> > > and to allow the module to idle.
> > > 
> > > It may be necessary to use this reset method for OMAP2xxx SoCs as
> > > well; this is left for a future patch.
> > 
> > Here too I think driver like features like this should live in the
> > driver init for omap OHCI driver. In the likely case that FS OHCI is
> > not in use on the board, the OHCI glue can just reset it.
> 
> this I don't agree. It means we will have to add some OHCI code even
> when OHCI is disabled.

That's because the bus level code alone is not enough for the reset and
driver features are needed to reset it. Nothing wrong with the code itself,
it should be just something that's part of the driver rather than the bus
level code. It could be always built in for sure even if it lives under
drivers.

Tony
Cousson, Benoit - June 7, 2012, 8:02 a.m.
On 6/7/2012 9:55 AM, Felipe Balbi wrote:
> On Thu, Jun 07, 2012 at 12:51:58AM -0700, Tony Lindgren wrote:
>> * Paul Walmsley<paul@pwsan.com>  [120607 00:44]:
>>> On Thu, 7 Jun 2012, Tony Lindgren wrote:
>>>
>>>> Here too I think driver like features like this should live in the
>>>> driver init for omap OHCI driver. In the likely case that FS OHCI is
>>>> not in use on the board, the OHCI glue can just reset it.
>>>
>>> What if the driver is not compiled into the kernel, but instead is built
>>> as a loadable module?
>>
>> You can still have a core piece of the driver that's always built in, such
>> as omap-ohci-common. But it should live under drivers, not in the bus level
>> code. Or you can insmod/rmmod it to reset things properly.
>
> that's such a hack... both solutions are quite hacky. The only problem
> here is because some bootloaders are leaving controller in an unknown
> state and I guess to be completely safe, resets should be done before
> any driver kicks in.
>
> But, driver will probably reset again the IP block during probe...

Ideally it should be done only in the probe if needed. In the case of 
the DSS, the bootloader can init it with a splash screen and we do not 
want to blindly reset it and thus produce some ugly artifact on the screen.

In fact we should delay the reset to the very last moment and 
potentially reset the IPs not under driver control later after a couple 
of second for example.
It will avoid reseting every IP that will be handled properly by drivers.

Regards,
Benoit
Tony Lindgren - June 7, 2012, 8:10 a.m.
* Cousson, Benoit <b-cousson@ti.com> [120607 01:07]:
> On 6/7/2012 9:55 AM, Felipe Balbi wrote:
> >On Thu, Jun 07, 2012 at 12:51:58AM -0700, Tony Lindgren wrote:
> >>* Paul Walmsley<paul@pwsan.com>  [120607 00:44]:
> >>>On Thu, 7 Jun 2012, Tony Lindgren wrote:
> >>>
> >>>>Here too I think driver like features like this should live in the
> >>>>driver init for omap OHCI driver. In the likely case that FS OHCI is
> >>>>not in use on the board, the OHCI glue can just reset it.
> >>>
> >>>What if the driver is not compiled into the kernel, but instead is built
> >>>as a loadable module?
> >>
> >>You can still have a core piece of the driver that's always built in, such
> >>as omap-ohci-common. But it should live under drivers, not in the bus level
> >>code. Or you can insmod/rmmod it to reset things properly.
> >
> >that's such a hack... both solutions are quite hacky. The only problem
> >here is because some bootloaders are leaving controller in an unknown
> >state and I guess to be completely safe, resets should be done before
> >any driver kicks in.
> >
> >But, driver will probably reset again the IP block during probe...

Well I see two advantages moving the reset and idle responsibility to
the drivers:

1. We can avoid ioremapping the devices in the bus level code and
   simplify things

2. We don't need to duplicate driver code into the bus level code
 
> Ideally it should be done only in the probe if needed. In the case
> of the DSS, the bootloader can init it with a splash screen and we
> do not want to blindly reset it and thus produce some ugly artifact
> on the screen.
> 
> In fact we should delay the reset to the very last moment and
> potentially reset the IPs not under driver control later after a
> couple of second for example.
> It will avoid reseting every IP that will be handled properly by drivers.

Good point.

Regards,

Tony
Felipe Balbi - June 7, 2012, 8:14 a.m.
On Thu, Jun 07, 2012 at 10:02:51AM +0200, Cousson, Benoit wrote:
> On 6/7/2012 9:55 AM, Felipe Balbi wrote:
> >On Thu, Jun 07, 2012 at 12:51:58AM -0700, Tony Lindgren wrote:
> >>* Paul Walmsley<paul@pwsan.com>  [120607 00:44]:
> >>>On Thu, 7 Jun 2012, Tony Lindgren wrote:
> >>>
> >>>>Here too I think driver like features like this should live in the
> >>>>driver init for omap OHCI driver. In the likely case that FS OHCI is
> >>>>not in use on the board, the OHCI glue can just reset it.
> >>>
> >>>What if the driver is not compiled into the kernel, but instead is built
> >>>as a loadable module?
> >>
> >>You can still have a core piece of the driver that's always built in, such
> >>as omap-ohci-common. But it should live under drivers, not in the bus level
> >>code. Or you can insmod/rmmod it to reset things properly.
> >
> >that's such a hack... both solutions are quite hacky. The only problem
> >here is because some bootloaders are leaving controller in an unknown
> >state and I guess to be completely safe, resets should be done before
> >any driver kicks in.
> >
> >But, driver will probably reset again the IP block during probe...
> 
> Ideally it should be done only in the probe if needed. In the case of
> the DSS, the bootloader can init it with a splash screen and we do
> not want to blindly reset it and thus produce some ugly artifact on
> the screen.
> 
> In fact we should delay the reset to the very last moment and
> potentially reset the IPs not under driver control later after a
> couple of second for example.
> It will avoid reseting every IP that will be handled properly by drivers.

you could have a late_initcall() that will iterate over hwmods and reset
the ones which aren't used. That would mean adding some extra code to
omap_device_build_ss() which would set a flag on each hwmod, or
something similar.
Paul Walmsley - June 7, 2012, 10:20 a.m.
On Thu, 7 Jun 2012, Tony Lindgren wrote:

> * Paul Walmsley <paul@pwsan.com> [120607 00:44]:
> > On Thu, 7 Jun 2012, Tony Lindgren wrote:
> > 
> > > Here too I think driver like features like this should live in the
> > > driver init for omap OHCI driver. In the likely case that FS OHCI is
> > > not in use on the board, the OHCI glue can just reset it.
> > 
> > What if the driver is not compiled into the kernel, but instead is built 
> > as a loadable module?
> 
> You can still have a core piece of the driver that's always built in, such
> as omap-ohci-common. But it should live under drivers, not in the bus level
> code. Or you can insmod/rmmod it to reset things properly.

Do you know of any device drivers that do this now, with a core built-in 
piece separate from a dynamically loadable part?

Seems like it would be tricky to avoid linking in the entire driver, due 
to the symbol dependencies.  Either that, or an extra, largely useless, 
layer of indirection would be needed in the shim layer.


- Paul
Tony Lindgren - June 7, 2012, 10:52 a.m.
* Paul Walmsley <paul@pwsan.com> [120607 03:24]:
> On Thu, 7 Jun 2012, Tony Lindgren wrote:
> 
> > * Paul Walmsley <paul@pwsan.com> [120607 00:44]:
> > > On Thu, 7 Jun 2012, Tony Lindgren wrote:
> > > 
> > > > Here too I think driver like features like this should live in the
> > > > driver init for omap OHCI driver. In the likely case that FS OHCI is
> > > > not in use on the board, the OHCI glue can just reset it.
> > > 
> > > What if the driver is not compiled into the kernel, but instead is built 
> > > as a loadable module?
> > 
> > You can still have a core piece of the driver that's always built in, such
> > as omap-ohci-common. But it should live under drivers, not in the bus level
> > code. Or you can insmod/rmmod it to reset things properly.
> 
> Do you know of any device drivers that do this now, with a core built-in 
> piece separate from a dynamically loadable part?

Hmm yeah good point, only driver frameworks tend to do that. It would require
the module registering with the core driver.
 
> Seems like it would be tricky to avoid linking in the entire driver, due 
> to the symbol dependencies.  Either that, or an extra, largely useless, 
> layer of indirection would be needed in the shim layer.

Yes probably better approach would be to only build in the reset and idle
part of the driver in the minimal case. And that too can get messy as the
makefiles may not even be included.

Until we have something generic in place to deal with stuff like unused driver
reset and idle, how about we set up the driver specific reset parts as inline
functions in the driver header?

That way the hwmod code can include those functions using the driver register
defines. Something like:

static inline int xyz_driver_reset(void __iomem *base, int flags)
{
	...
}

Then instead of having a separate platform init file for each driver,
we could just have a list of reset functions:

static int hwmod_xyz_driver_reset(void __iomem *base, int flags)
{
	int res;

	/* do bus related reset here */
	...

	/* call the driver reset */
	res = xyz_driver_reset(base, flags)


	/* do more bus related reset here */
	...
}

Regards,

Tony
Paul Walmsley - June 7, 2012, 10:52 a.m.
On Thu, 7 Jun 2012, Cousson, Benoit wrote:

> In fact we should delay the reset to the very last moment and 
> potentially reset the IPs not under driver control later after a couple 
> of second for example. It will avoid reseting every IP that will be 
> handled properly by drivers.

We discussed this a couple of years ago on the list and aligned on late 
and lazy resets, from my recollection.  The main reason why it wasn't 
implemented was because there were some drivers that were not yet PM 
runtime-converted.  This would have caused crashes, since the core code 
would have no idea that the non-PM-runtime drivers had initialized their 
devices, and so the core just went ahead and reset those anyway.

It might actually be safe now to switch to the late reset arrangement, 
depending on whether the rest of the drivers have been PM 
runtime-converted by now.

Of course, it would all be moot if the reset is moved away from the hwmod 
code into the drivers.


- Paul
Cousson, Benoit - June 7, 2012, 12:30 p.m.
+ Ohad

On 6/7/2012 12:52 PM, Paul Walmsley wrote:
> On Thu, 7 Jun 2012, Cousson, Benoit wrote:
>
>> In fact we should delay the reset to the very last moment and
>> potentially reset the IPs not under driver control later after a couple
>> of second for example. It will avoid reseting every IP that will be
>> handled properly by drivers.
>
> We discussed this a couple of years ago on the list and aligned on late
> and lazy resets, from my recollection.

Indeed, what I did not mention is that potentially the whole device init 
should be done ondemand as well. Meaning the whole hwmod setup phase 
should be done only when the driver will probe the device.

> The main reason why it wasn't
> implemented was because there were some drivers that were not yet PM
> runtime-converted.  This would have caused crashes, since the core code
> would have no idea that the non-PM-runtime drivers had initialized their
> devices, and so the core just went ahead and reset those anyway.
>
> It might actually be safe now to switch to the late reset arrangement,
> depending on whether the rest of the drivers have been PM
> runtime-converted by now.
>
> Of course, it would all be moot if the reset is moved away from the hwmod
> code into the drivers.

I do think that moving that to driver code seems much better since all 
these customs reset code (I2C, DSS, USB...) does anyway require access 
to the device itself. The is driver responsibility to do that.

So ideally it should not be in OMAP architecture code.

Regards,
Benoit
Paul Walmsley - June 7, 2012, 10:05 p.m.
On Thu, 7 Jun 2012, Tony Lindgren wrote:

> Until we have something generic in place to deal with stuff like unused driver
> reset and idle, how about we set up the driver specific reset parts as inline
> functions in the driver header?

You're referring to the driver integration header files in 
arch/arm/plat-omap/include/ and arch/arm/mach-omap2/, right?  That would 
avoid the need for "sideways" includes from drivers/.

> That way the hwmod code can include those functions using the driver register
> defines. Something like:
> 
> static inline int xyz_driver_reset(void __iomem *base, int flags)
> {
> 	...
> }
> 
> Then instead of having a separate platform init file for each driver,
> we could just have a list of reset functions:
> 
> static int hwmod_xyz_driver_reset(void __iomem *base, int flags)

This should probably be passed a struct omap_hwmod * instead of base so 
it can call the existing hwmod bus related reset functions like 
omap_hwmod_softreset().  Or were you thinking about open-coding those into 
this reset function?

Just as an aside, this function will probably need to be marked 
__maybe_unused, so the compiler doesn't warn when other files include this 
header, but don't call the function.

> {
> 	int res;
> 
> 	/* do bus related reset here */
> 	...
>
> 	/* call the driver reset */
> 	res = xyz_driver_reset(base, flags)
> 
> 
> 	/* do more bus related reset here */
> 	...
> }

That's fine with me.  It doesn't matter to me where that code lives as 
long as it makes technical sense.

I'm hoping that in the near future that we can get rid of these 
hwmod_xyz_driver_reset() functions.  Instead it should be possible to have 
the hwmod reset code call functions like xyz_driver_reset_pre_wait() and 
xyz_driver_reset_post_ocpreset() via optional function pointers at 
different points in the reset process.  That should allow us to remove the 
omap_hwmod function calls from the I2C, MSDI, etc. reset functions, and 
also remove some needlessly duplicated code.


- Paul
Paul Walmsley - June 8, 2012, 1:11 a.m.
On Thu, 7 Jun 2012, Cousson, Benoit wrote:

> Indeed, what I did not mention is that potentially the whole device init
> should be done ondemand as well. Meaning the whole hwmod setup phase should be
> done only when the driver will probe the device.

That means if no driver exists for an IP block, or if the driver isn't 
using PM runtime, the IP block won't be reset.  And somehow we still are 
missing drivers in mainline.  We also still have drivers that aren't yet 
PM runtime converted.


- Paul
Tony Lindgren - June 8, 2012, 6:38 a.m.
* Paul Walmsley <paul@pwsan.com> [120607 15:09]:
> On Thu, 7 Jun 2012, Tony Lindgren wrote:
> 
> > Until we have something generic in place to deal with stuff like unused driver
> > reset and idle, how about we set up the driver specific reset parts as inline
> > functions in the driver header?
> 
> You're referring to the driver integration header files in 
> arch/arm/plat-omap/include/ and arch/arm/mach-omap2/, right?  That would 
> avoid the need for "sideways" includes from drivers/.
> 
> > That way the hwmod code can include those functions using the driver register
> > defines. Something like:
> > 
> > static inline int xyz_driver_reset(void __iomem *base, int flags)
> > {
> > 	...
> > }
> > 
> > Then instead of having a separate platform init file for each driver,
> > we could just have a list of reset functions:
> > 
> > static int hwmod_xyz_driver_reset(void __iomem *base, int flags)
> 
> This should probably be passed a struct omap_hwmod * instead of base so 
> it can call the existing hwmod bus related reset functions like 
> omap_hwmod_softreset().  Or were you thinking about open-coding those into 
> this reset function?

Oh OK yeah makes sense as that's hwmod internal function. Then the driver
specific part should use just void __iomem *base and use readl/writel and
live under include/linux/platform_data/omap-usb.h.

> Just as an aside, this function will probably need to be marked 
> __maybe_unused, so the compiler doesn't warn when other files include this 
> header, but don't call the function.

Yeah probably.
 
> > {
> > 	int res;
> > 
> > 	/* do bus related reset here */
> > 	...
> >
> > 	/* call the driver reset */
> > 	res = xyz_driver_reset(base, flags)
> > 
> > 
> > 	/* do more bus related reset here */
> > 	...
> > }
> 
> That's fine with me.  It doesn't matter to me where that code lives as 
> long as it makes technical sense.

OK good. That way we can separate the driver specific part from the bus
code. And the driver maintainers can review the driver specific part of the
idle/reset function. And maybe at some point we'll have device_reset and
device_idle functions and some generic framework in place for it..
 
> I'm hoping that in the near future that we can get rid of these 
> hwmod_xyz_driver_reset() functions.  Instead it should be possible to have 
> the hwmod reset code call functions like xyz_driver_reset_pre_wait() and 
> xyz_driver_reset_post_ocpreset() via optional function pointers at 
> different points in the reset process.  That should allow us to remove the 
> omap_hwmod function calls from the I2C, MSDI, etc. reset functions, and 
> also remove some needlessly duplicated code.

Sounds good to me. Also sounds like that does not need changes to the
driver specific xyz_driver_reset functions.

Regards,

Tony
Cousson, Benoit - June 8, 2012, 1:13 p.m.
On 6/8/2012 3:11 AM, Paul Walmsley wrote:
> On Thu, 7 Jun 2012, Cousson, Benoit wrote:
>
>> Indeed, what I did not mention is that potentially the whole device init
>> should be done ondemand as well. Meaning the whole hwmod setup phase should be
>> done only when the driver will probe the device.
>
> That means if no driver exists for an IP block, or if the driver isn't
> using PM runtime, the IP block won't be reset.  And somehow we still are
> missing drivers in mainline.  We also still have drivers that aren't yet
> PM runtime converted.

No the point is still the same as before. You let the drivers do the job 
if they are there, and then do a pass at very late time during the boot 
process to handle the ones that were not probed by any driver.

At least you will avoid the enable -> reset -> idle -> enable sequence 
we are doing right now for most of the active drivers when it is not 
necessary.

Regards,
Benoit
Paul Walmsley - June 8, 2012, 1:28 p.m.
On Fri, 8 Jun 2012, Cousson, Benoit wrote:

> On 6/8/2012 3:11 AM, Paul Walmsley wrote:
> > On Thu, 7 Jun 2012, Cousson, Benoit wrote:
> > 
> > > Indeed, what I did not mention is that potentially the whole device 
> > > init should be done ondemand as well. Meaning the whole hwmod setup 
> > > phase should be done only when the driver will probe the device.
> > 
> > That means if no driver exists for an IP block, or if the driver isn't
> > using PM runtime, the IP block won't be reset.  And somehow we still are
> > missing drivers in mainline.  We also still have drivers that aren't yet
> > PM runtime converted.
> 
> No the point is still the same as before. You let the drivers do the job if
> they are there, and then do a pass at very late time during the boot process
> to handle the ones that were not probed by any driver.

Ah, I see what you mean.  Above you wrote that the the hwmod setup phase 
would be done only when the driver will probe the device.  But you also 
mean that it should also be done for the remaining devices before starting 
userspace.

> At least you will avoid the enable -> reset -> idle -> enable sequence 
> we are doing right now for most of the active drivers when it is not 
> necessary.

It must not be widely known, but early reset was implemented 
intentionally.  The goal was to keep any configuration damage from 
out-of-date or broken bootloaders or previous OSes to a minimum length of 
time during the boot process.

I don't really have a huge problem with switching to a late reset, 
but there are disadvantages to it.


- Paul
hvaibhav@ti.com - June 8, 2012, 7:32 p.m.
On Fri, Jun 08, 2012 at 18:58:51, Paul Walmsley wrote:
> On Fri, 8 Jun 2012, Cousson, Benoit wrote:
> 
> > On 6/8/2012 3:11 AM, Paul Walmsley wrote:
> > > On Thu, 7 Jun 2012, Cousson, Benoit wrote:
> > > 
> > > > Indeed, what I did not mention is that potentially the whole device 
> > > > init should be done ondemand as well. Meaning the whole hwmod setup 
> > > > phase should be done only when the driver will probe the device.
> > > 
> > > That means if no driver exists for an IP block, or if the driver isn't
> > > using PM runtime, the IP block won't be reset.  And somehow we still are
> > > missing drivers in mainline.  We also still have drivers that aren't yet
> > > PM runtime converted.
> > 
> > No the point is still the same as before. You let the drivers do the job if
> > they are there, and then do a pass at very late time during the boot process
> > to handle the ones that were not probed by any driver.
> 
> Ah, I see what you mean.  Above you wrote that the the hwmod setup phase 
> would be done only when the driver will probe the device.  But you also 
> mean that it should also be done for the remaining devices before starting 
> userspace.
> 
> > At least you will avoid the enable -> reset -> idle -> enable sequence 
> > we are doing right now for most of the active drivers when it is not 
> > necessary.
> 
> It must not be widely known, but early reset was implemented 
> intentionally.  The goal was to keep any configuration damage from 
> out-of-date or broken bootloaders or previous OSes to a minimum length of 
> time during the boot process.
> 
> I don't really have a huge problem with switching to a late reset, 
> but there are disadvantages to it.
> 
> 

I was reading all these discussion and was holding myself back, so that I 
digest before adding another flavor to this discussion,

In case of AM33xx, recently I came across similar issue (rather more than this) with CPSW module.

The issue is,

We have observed that, in order to disable the CPSW module (MODULEMODE=0x0),
We have to assert OCP level reset, before disabling it; else module enters into unknown state, where register status shows, MODULEMODE turns 0x0, but idlests says module is busy.

This has to be done everytime you try to disable the module.

The worst part here is, CPSW has 4 submodules (SS, SL1, SL2, CPDMA),
We have to assert reset signal to each submodules.

So the approach I had taken is, I had implemented almost similar function specific to cpsw and introduced flag called HWMOD_SWSUP_RESET_BEFORE_IDLE.


Now the question here would be, should we consider this IP bug or 
integration bug? If it is integration bug, then isn't this device issue?

Thanks,
Vaibhav
Paul Walmsley - June 8, 2012, 11:10 p.m.
Hi

On Fri, 8 Jun 2012, Hiremath, Vaibhav wrote:

> In case of AM33xx, recently I came across similar issue (rather more 
> than this) with CPSW module.
> 
> The issue is,
> 
> We have observed that, in order to disable the CPSW module 
> (MODULEMODE=0x0), We have to assert OCP level reset, before disabling 
> it; else module enters into unknown state, where register status shows, 
> MODULEMODE turns 0x0, but idlests says module is busy.
> 
> This has to be done everytime you try to disable the module.
> 
> The worst part here is, CPSW has 4 submodules (SS, SL1, SL2, CPDMA),
> We have to assert reset signal to each submodules.
> 
> So the approach I had taken is, I had implemented almost similar 
> function specific to cpsw and introduced flag called 
> HWMOD_SWSUP_RESET_BEFORE_IDLE.

Why "SWSUP" ? 

> Now the question here would be, should we consider this IP bug or 
> integration bug? If it is integration bug, then isn't this device issue?

I don't know if it's a bug in either place, but it sounds like something 
that needs to be handled in the _am335x_disable_module() code in 
mach-omap2/omap_hwmod.c.


- Paul
Paul Walmsley - June 9, 2012, 1:31 a.m.
On Thu, 7 Jun 2012, Tony Lindgren wrote:

> Oh OK yeah makes sense as that's hwmod internal function. Then the driver
> specific part should use just void __iomem *base and use readl/writel and
> live under include/linux/platform_data/omap-usb.h.

This sounds like something that might be flame-bait, since these functions 
aren't platform_data.

How about putting these functions in arch/arm/plat-omap/include/plat?  
Drivers are able to include those files easily.


- Paul
hvaibhav@ti.com - June 9, 2012, 8:39 a.m.
On Sat, Jun 09, 2012 at 04:40:03, Paul Walmsley wrote:
> Hi
> 
> On Fri, 8 Jun 2012, Hiremath, Vaibhav wrote:
> 
> > In case of AM33xx, recently I came across similar issue (rather more 
> > than this) with CPSW module.
> > 
> > The issue is,
> > 
> > We have observed that, in order to disable the CPSW module 
> > (MODULEMODE=0x0), We have to assert OCP level reset, before disabling 
> > it; else module enters into unknown state, where register status shows, 
> > MODULEMODE turns 0x0, but idlests says module is busy.
> > 
> > This has to be done everytime you try to disable the module.
> > 
> > The worst part here is, CPSW has 4 submodules (SS, SL1, SL2, CPDMA),
> > We have to assert reset signal to each submodules.
> > 
> > So the approach I had taken is, I had implemented almost similar 
> > function specific to cpsw and introduced flag called 
> > HWMOD_SWSUP_RESET_BEFORE_IDLE.
> 
> Why "SWSUP" ? 
> 

Since it was SW initiated reset assertion so I added this prefix.

> > Now the question here would be, should we consider this IP bug or 
> > integration bug? If it is integration bug, then isn't this device issue?
> 
> I don't know if it's a bug in either place, but it sounds like something 
> that needs to be handled in the _am335x_disable_module() code in 
> mach-omap2/omap_hwmod.c.
> 

Yes, certainly it should be part of _am335x_disable_module().

Thanks,
Vaibhav
Paul Walmsley - June 9, 2012, 4:05 p.m.
On Sat, 9 Jun 2012, Hiremath, Vaibhav wrote:

> On Sat, Jun 09, 2012 at 04:40:03, Paul Walmsley wrote:
> > On Fri, 8 Jun 2012, Hiremath, Vaibhav wrote:
> > 
> > > So the approach I had taken is, I had implemented almost similar 
> > > function specific to cpsw and introduced flag called 
> > > HWMOD_SWSUP_RESET_BEFORE_IDLE.
> > 
> > Why "SWSUP" ? 
> 
> Since it was SW initiated reset assertion so I added this prefix.

Maybe just use HWMOD_RESET_BEFORE_IDLE.  The code use SWSUP to mean 
"software-supervised" in the context of idle.

> > > Now the question here would be, should we consider this IP bug or 
> > > integration bug? If it is integration bug, then isn't this device issue?
> > 
> > I don't know if it's a bug in either place, but it sounds like something 
> > that needs to be handled in the _am335x_disable_module() code in 
> > mach-omap2/omap_hwmod.c.
> 
> Yes, certainly it should be part of _am335x_disable_module().

Great. Care to send a patch?


- Paul
Tony Lindgren - June 11, 2012, 6:15 a.m.
* Paul Walmsley <paul@pwsan.com> [120608 06:33]:
> On Fri, 8 Jun 2012, Cousson, Benoit wrote:
> 
> > On 6/8/2012 3:11 AM, Paul Walmsley wrote:
> > > On Thu, 7 Jun 2012, Cousson, Benoit wrote:
> > > 
> > > > Indeed, what I did not mention is that potentially the whole device 
> > > > init should be done ondemand as well. Meaning the whole hwmod setup 
> > > > phase should be done only when the driver will probe the device.
> > > 
> > > That means if no driver exists for an IP block, or if the driver isn't
> > > using PM runtime, the IP block won't be reset.  And somehow we still are
> > > missing drivers in mainline.  We also still have drivers that aren't yet
> > > PM runtime converted.
> > 
> > No the point is still the same as before. You let the drivers do the job if
> > they are there, and then do a pass at very late time during the boot process
> > to handle the ones that were not probed by any driver.
> 
> Ah, I see what you mean.  Above you wrote that the the hwmod setup phase 
> would be done only when the driver will probe the device.  But you also 
> mean that it should also be done for the remaining devices before starting 
> userspace.
> 
> > At least you will avoid the enable -> reset -> idle -> enable sequence 
> > we are doing right now for most of the active drivers when it is not 
> > necessary.
> 
> It must not be widely known, but early reset was implemented 
> intentionally.  The goal was to keep any configuration damage from 
> out-of-date or broken bootloaders or previous OSes to a minimum length of 
> time during the boot process.

These devices should get reset as the device drivers initialize. Some parts
of course need to be initialized properly early like caches and DMA controller.
 
> I don't really have a huge problem with switching to a late reset, 
> but there are disadvantages to it.

I think the early reset actually has more disadvantages to it compared
to driver reset. We don't see any errors when things go wrong, and we
may even kill the only debug console in the reset process.

We are already doing what Benoit describes with clocks where we only
reset the unclaimed ones at late_initcall level, and that has proven
to work well.

Regards,

Tony
Tony Lindgren - June 11, 2012, 6:21 a.m.
* Paul Walmsley <paul@pwsan.com> [120608 18:35]:
> On Thu, 7 Jun 2012, Tony Lindgren wrote:
> 
> > Oh OK yeah makes sense as that's hwmod internal function. Then the driver
> > specific part should use just void __iomem *base and use readl/writel and
> > live under include/linux/platform_data/omap-usb.h.
> 
> This sounds like something that might be flame-bait, since these functions 
> aren't platform_data.

They at least should be as both platform init code and the driver will
potentially use these functions.
 
> How about putting these functions in arch/arm/plat-omap/include/plat?  
> Drivers are able to include those files easily.

We need to pretty much get rid of all those headers and make them driver
specific for the multi-zimage support. Drivers should be arch independent,
and whatever parts need to be shared between platform init code and drivers
should follow the standard platform driver stuff.

The other place would be just the standard driver include at somewhere
like include/linux/usb/omap-usb.h etc.

Regards,

Tony
Paul Walmsley - June 11, 2012, 8:04 a.m.
On Sun, 10 Jun 2012, Tony Lindgren wrote:

> * Paul Walmsley <paul@pwsan.com> [120608 06:33]:
>  
> > I don't really have a huge problem with switching to a late reset, 
> > but there are disadvantages to it.
> 
> I think the early reset actually has more disadvantages to it compared
> to driver reset. We don't see any errors when things go wrong, and we
> may even kill the only debug console in the reset process.
> 
> We are already doing what Benoit describes with clocks where we only
> reset the unclaimed ones at late_initcall level, and that has proven
> to work well.

The difference though between the clock and the IP block init is that 
leaving the clocks on until later has no effect on system stability.  The 
chip simply consumes more energy.

But if the IP blocks are reset later, and the bootloader or previous OS 
gets some register settings wrong, there's a greater risk of system 
instability or unpredictable behavior during the boot process.

I agree that it is probably easier to debug a late reset approach.


- Paul
Cousson, Benoit - June 11, 2012, 9:24 a.m.
Hi Paul,

On 6/11/2012 10:04 AM, Paul Walmsley wrote:
> On Sun, 10 Jun 2012, Tony Lindgren wrote:
>
>> * Paul Walmsley <paul@pwsan.com> [120608 06:33]:
>>
>>> I don't really have a huge problem with switching to a late reset,
>>> but there are disadvantages to it.
>>
>> I think the early reset actually has more disadvantages to it compared
>> to driver reset. We don't see any errors when things go wrong, and we
>> may even kill the only debug console in the reset process.
>>
>> We are already doing what Benoit describes with clocks where we only
>> reset the unclaimed ones at late_initcall level, and that has proven
>> to work well.
>
> The difference though between the clock and the IP block init is that
> leaving the clocks on until later has no effect on system stability.  The
> chip simply consumes more energy.
>
> But if the IP blocks are reset later, and the bootloader or previous OS
> gets some register settings wrong, there's a greater risk of system
> instability or unpredictable behavior during the boot process.

Mmm, I'm not convince by that. If we delay the PM init at the very last 
time, at least after the modules are properly reset and init, I do not 
think we will have more issues than today.

Regards,
Benoit
Paul Walmsley - June 11, 2012, 4:20 p.m.
On Mon, 11 Jun 2012, Cousson, Benoit wrote:

> On 6/11/2012 10:04 AM, Paul Walmsley wrote:
> 
> > But if the IP blocks are reset later, and the bootloader or previous OS
> > gets some register settings wrong, there's a greater risk of system
> > instability or unpredictable behavior during the boot process.
> 
> Mmm, I'm not convince by that. If we delay the PM init at the very last 
> time, at least after the modules are properly reset and init, I do not 
> think we will have more issues than today.

My intent was not to convince, but to explain.

Certainly back in the OMAP3 days there were bootloaders that got SDRAM and 
GPMC timings wrong.  No way did I want to be chasing down kernel "bugs" 
based on those.

Anyway, once people finish fixing the drivers, then we should be able to 
switch to late hwmod reset.


- Paul

Patch

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index bc2ac4f..fc2ff91 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -245,9 +245,7 @@  omap-hsmmc-$(CONFIG_MMC_OMAP_HS)	:= hsmmc.o
 obj-y					+= $(omap-hsmmc-m) $(omap-hsmmc-y)
 
 
-usbfs-$(CONFIG_ARCH_OMAP_OTG)		:= usb-fs.o
-obj-y					+= $(usbfs-m) $(usbfs-y)
-obj-y					+= usb-musb.o
+obj-y					+= usb-fs.o usb-musb.o
 obj-y					+= omap_phy_internal.o
 
 obj-$(CONFIG_MACH_OMAP2_TUSB6010)	+= usb-tusb6010.o
diff --git a/arch/arm/mach-omap2/cm.h b/arch/arm/mach-omap2/cm.h
index a7bc096..99978c7 100644
--- a/arch/arm/mach-omap2/cm.h
+++ b/arch/arm/mach-omap2/cm.h
@@ -1,7 +1,7 @@ 
 /*
  * OMAP2+ Clock Management prototypes
  *
- * Copyright (C) 2007-2009 Texas Instruments, Inc.
+ * Copyright (C) 2007-2012 Texas Instruments, Inc.
  * Copyright (C) 2007-2009 Nokia Corporation
  *
  * Written by Paul Walmsley
@@ -14,6 +14,12 @@ 
 #define __ARCH_ASM_MACH_OMAP2_CM_H
 
 /*
+ * MAX_MODULE_SOFTRESET_TIME: maximum time in microseconds to wait for
+ * an IP block to finish an OCP SOFTRESET.
+ */
+#define MAX_MODULE_SOFTRESET_WAIT	10000
+
+/*
  * MAX_MODULE_READY_TIME: max duration in microseconds to wait for the
  * PRCM to request that a module exit the inactive state in the case of
  * OMAP2 & 3.
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index a93ce48..02daacc 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -3314,6 +3314,7 @@  static struct omap_hwmod_class_sysconfig omap44xx_usb_host_fs_sysc = {
 static struct omap_hwmod_class omap44xx_usb_host_fs_hwmod_class = {
 	.name	= "usb_host_fs",
 	.sysc	= &omap44xx_usb_host_fs_sysc,
+	.reset	= omap_usb_host_fs_reset,
 };
 
 /* usb_host_fs */
diff --git a/arch/arm/mach-omap2/usb-fs.c b/arch/arm/mach-omap2/usb-fs.c
index 1481078..4faf0f7 100644
--- a/arch/arm/mach-omap2/usb-fs.c
+++ b/arch/arm/mach-omap2/usb-fs.c
@@ -1,7 +1,7 @@ 
 /*
  * Platform level USB initialization for FS USB OTG controller on omap1 and 24xx
  *
- * Copyright (C) 2004 Texas Instruments, Inc.
+ * Copyright (C) 2004, 2012 Texas Instruments, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -32,6 +32,8 @@ 
 #include <plat/usb.h>
 #include <plat/board.h>
 
+#include "cm.h"
+#include "common.h"
 #include "control.h"
 #include "mux.h"
 
@@ -41,6 +43,64 @@ 
 #define INT_USB_IRQ_HGEN	INT_24XX_USB_IRQ_HGEN
 #define INT_USB_IRQ_OTG		INT_24XX_USB_IRQ_OTG
 
+/* HCCOMMANDSTATUS: the register offset of the HCCOMMANDSTATUS register */
+#define HCCOMMANDSTATUS			0x0008
+
+/* HCCOMMANDSTATUS_HCR: the bitmask of the host controller reset flag */
+#define HCCOMMANDSTATUS_HCR_MASK	(1 << 0)
+
+/**
+ * omap_usb_host_fs_reset - custom reset function for the FSUSB IP block
+ * @oh: struct omap_hwmod * of the usb_host_fs IP block
+ *
+ * Reset the FSUSB IP block.  This IP block requires a custom
+ * two-stage reset; otherwise the IP block won't idle-ack to the PRCM.
+ * First the OCP SOFTRESET method must be used.  Next, the IP block's
+ * internal reset bit must be toggled.  This will place the OHCI
+ * controller state into UsbSuspend, which allows the IP block to
+ * idle-ack to the PRCM.  Note that the FSUSB still takes almost 4
+ * milliseconds to idle-ack after this function returns.  Returns 0
+ * upon success, -EINVAL if the IP block softreset data wasn't
+ * supplied, or -EBUSY if the IP block reset times out.
+ */
+int omap_usb_host_fs_reset(struct omap_hwmod *oh)
+{
+	int c;
+
+	if (omap_hwmod_softreset(oh))
+		return -EINVAL;
+
+	omap_test_timeout(!(omap_hwmod_read(oh, oh->class->sysc->sysc_offs)
+			   & SYSC_TYPE2_SOFTRESET_MASK),
+			  MAX_MODULE_SOFTRESET_WAIT, c);
+
+	if (c == MAX_MODULE_SOFTRESET_WAIT) {
+		pr_warn("%s: %s: softreset failed (waited %d usec)\n",
+			__func__, oh->name, MAX_MODULE_SOFTRESET_WAIT);
+		return -EBUSY;
+	} else {
+		pr_debug("%s: %s: softreset in %d usec\n", __func__,
+			 oh->name, c);
+	}
+
+	omap_hwmod_write(HCCOMMANDSTATUS_HCR_MASK, oh, HCCOMMANDSTATUS);
+
+	omap_test_timeout(!(omap_hwmod_read(oh, HCCOMMANDSTATUS)
+			    & HCCOMMANDSTATUS_HCR_MASK),
+			  MAX_MODULE_SOFTRESET_WAIT, c);
+
+	if (c == MAX_MODULE_SOFTRESET_WAIT) {
+		pr_warn("%s: %s: host controller reset failed (waited %d usec)\n",
+			__func__, oh->name, MAX_MODULE_SOFTRESET_WAIT);
+		return -EBUSY;
+	} else {
+		pr_debug("%s: %s: host controller reset in %d usec\n", __func__,
+			 oh->name, c);
+	}
+
+	return 0;
+}
+
 #if defined(CONFIG_ARCH_OMAP2)
 
 #ifdef	CONFIG_USB_GADGET_OMAP
diff --git a/arch/arm/plat-omap/include/plat/usb.h b/arch/arm/plat-omap/include/plat/usb.h
index 762eeb0..ac7db50 100644
--- a/arch/arm/plat-omap/include/plat/usb.h
+++ b/arch/arm/plat-omap/include/plat/usb.h
@@ -6,6 +6,7 @@ 
 #include <linux/io.h>
 #include <linux/usb/musb.h>
 #include <plat/board.h>
+#include <plat/omap_hwmod.h>
 
 #define OMAP3_HS_USB_PORTS	3
 
@@ -181,6 +182,8 @@  static inline void omap2_usbfs_init(struct omap_usb_config *pdata)
 }
 #endif
 
+extern int omap_usb_host_fs_reset(struct omap_hwmod *oh);
+
 /*-------------------------------------------------------------------------*/
 
 /*