| Message ID | 20120607061308.25532.19767.stgit@dusk |
|---|---|
| State | New |
| Headers | show |
* 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
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.
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
* 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
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...
* 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
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
* 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
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.
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
* 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
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
+ 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
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
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
* 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
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
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
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
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
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
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
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
* 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
* 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
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
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
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
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); + /*-------------------------------------------------------------------------*/ /*