ARM: OMAP: core/hwmod: first set of fixes for 3.5-rc

Enable the AESS auto-gating control bit during AESS hwmod setup. This fixes the following boot warning on OMAP4:

omap_hwmod: aess: _wait_target_disable failed

Without this patch, the AESS IP block does not indicate to the PRCM that it is idle after it is reset. This prevents some types of SoC power management until something sets the auto-gating control bit.

It seems that most/many IP blocks need their custom reset hacks, and it's not limited to just few instances?

AFAIK there's no need to reset the IP blocks before the driver init, it's really needed for PM. So it's not needed early on, and it's OK to require running the driver init for driver modules that are not in use to reset them properly. After all, the hardware is on the device, even if it's not being used.

Quoted 2 lines · Tony Lindgren

It seems that most/many IP blocks need their custom reset hacks, and it's not limited to just few instances?

Only four out of the fifty-seven omap_hwmod_classes defined in mach-omap2/omap_hwmod_44xx_data.c after this series have custom reset functions used:

$ fgrep 'struct omap_hwmod_class ' arch/arm/mach-omap2/omap_hwmod_44xx_data.c | wc -l
57
$ fgrep '.reset' arch/arm/mach-omap2/omap_hwmod_44xx_data.c | wc -l
4

That's 7% of the classes. In terms of the total number of IP block instances that use custom reset functions viewed against the total number of instances on the chip, the percentage is even smaller.

Quoted 5 lines

AFAIK there's no need to reset the IP blocks before the driver init, it's really needed for PM. So it's not needed early on, and it's OK to require running the driver init for driver modules that are not in use to reset them properly. After all, the hardware is on the device, even if it's not being used.

I don't think I'm following you. It's not just PM; the problem is also with kexec or buggy bootloaders. If an IP block isn't reset when the kernel boots, and is doing DMA or anything else that could affect the reset of the system, it could easily cause unpredictable behavior or crashes in unrelated kernel code.

It's also worth mentioning that many IP blocks, such as AESS, don't have Linux drivers.

OK so that's not too bad then. But there's also the omap2_wd_timer_disable pre_shutdown too. And there's also the sysconfig autoidle bit for each driver that we're tweaking in the bus level code?

If we can remove the ioremapping and accessing driver registers in the bus level code things get much simpler for the bus level code.

Quoted 3 lines · Tony Lindgren

OK so that's not too bad then. But there's also the omap2_wd_timer_disable pre_shutdown too. And there's also the sysconfig autoidle bit for each driver that we're tweaking in the bus level code?

I think I lost your point here. The ioremap() issue is separate from the reset functions, etc., in my view. Moving the reset functions out to drivers/ seems potentially more reasonable than dropping the ioremap().

Quoted 2 lines

If we can remove the ioremapping and accessing driver registers in the bus level code things get much simpler for the bus level code.

That's like saying if PCI Configuration Header handling were to be moved into the driver code, then the PCI bus-level code would be much simpler :-)

The hwmod code ioremaps the device registers to handle the integration-level registers at the beginning of the device's address space. These registers can be thought of as part of the PRCM, not part of the IP block. It would have been better if TI had put these integration registers in a separate address space like PCI does. But we are stuck with the existing hardware design. The integration registers also differ from chip to chip even with the same underlying IP block, see for example the 32k sync timer.

The main reasons why these integration registers are handled now in common code are:

1. to avoid duplicating integration code between lots of different drivers that is unrelated to the driver itself, such as bus-level reset

2. to ensure consistency of the OCP registers with the rest of the PM state

3. to avoid callbacks into drivers that might otherwise be needed for bitfields like CLOCKACTIVITY

4. to make it easier to debug integration problems with drivers

If we don't handle those registers in common code, the number of SoC integration workarounds that need to be placed into the drivers will increase. For example, when OMAP4 added the smart-idle-with-wakeup and smart-standby-with-wakeup OCP idle modes, only a couple of files needed to be changed. If those integration-level details were still in the drivers, a large number of files would need to be changed. And $DEITY help us if the code sequence for dealing with those bits were to ever change in the future - we'd need to change a bunch of drivers, rather than just one or two files. Also some people are going to need to audit the driver code from an integration level pretty carefully for PM to work consistently.

I suppose one option, if we were to have a real omap_device, would be to define callbacks for each driver to implement that would read and write the OCP header registers. Then the omap_bus code could call those callbacks to handle the OCP register accesses, when called from the driver's PM runtime calls. Adds another layer of indirection, but would localize IP block register accesses to the IP block's driver.

...

As far as the reset and preconfiguration aspects of the hwmod code go, they just happen to be possible since we're doing the ioremap anyway. It can be ensured that no matter what drivers are present, or what the bootloader or previous OS did or didn't do, a minimal kernel should behave predictably.

It seems like it might be reasonable to move these to some built-in driver shim layer as you suggest in your other E-mail. But that is assuming that it can be made to work without needless layers of indirection. I don't know of any driver that does this now. Maybe you know of one?

From: Tero Kristo <[email protected]>

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.

Quoted 10 lines · Paul Walmsley

From: Tero Kristo <[email protected]>

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

Quoted 4 lines

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.

arch/arm/mach-omap2/usb-fs.c line 50

Quoted 5 lines

arch/arm/mach-omap2/usb-fs.c +5 -0

+/* 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.

arch/arm/mach-omap2/usb-fs.c line 91

Quoted 6 lines

arch/arm/mach-omap2/usb-fs.c +6 -0

+	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.

Hi,

Quoted 16 lines · Tony Lindgren

Quoted 14 lines · Paul Walmsley

From: Tero Kristo <[email protected]>

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.

Quoted 3 lines

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.

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.

Quoted 3 lines · Tony Lindgren

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?

Quoted 6 lines · Paul Walmsley

Quoted 3 lines · Tony Lindgren

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.

Quoted 2 lines

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.

Quoted 7 lines · Tony Lindgren

Quoted 6 lines · Paul Walmsley

Quoted 3 lines · Tony Lindgren

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.

Quoted 6 lines

Quoted 2 lines

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...

Quoted 15 lines · Felipe Balbi

Quoted 13 lines · Tony Lindgren

Quoted 8 lines · Paul Walmsley

Quoted 3 lines · Tony Lindgren

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.

Quoted 6 lines

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.

Quoted 15 lines · Felipe Balbi

Quoted 7 lines · Tony Lindgren

Quoted 6 lines · Paul Walmsley

Quoted 3 lines · Tony Lindgren

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

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.

Quoted 4 lines · Benoit

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.

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.

Quoted 3 lines · Benoit

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.

Quoted 6 lines · Paul Walmsley

Quoted 3 lines · Benoit

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.

Quoted 4 lines

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.

Quoted 7 lines · Benoit

Quoted 6 lines · Paul Walmsley

Quoted 3 lines · Benoit

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

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?

Hi

Quoted 12 lines · Vaibhav

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.

Quoted 6 lines

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" ?

Quoted 2 lines

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.

Quoted 7 lines · Paul Walmsley

Quoted 3 lines · Vaibhav

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.

Quoted 6 lines

Quoted 2 lines

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().

Quoted 7 lines · Benoit

Quoted 6 lines · Paul Walmsley

Quoted 3 lines · Benoit

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines · Paul Walmsley

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.

Quoted 5 lines · Tony Lindgren

Quoted 2 lines · Paul Walmsley

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

Quoted 7 lines

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.

Quoted 3 lines · Paul Walmsley

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.

Quoted 6 lines · Paul Walmsley

Quoted 3 lines · Tony Lindgren

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.

Quoted 2 lines

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.

Quoted 10 lines · Tony Lindgren

Quoted 8 lines · Paul Walmsley

Quoted 3 lines · Tony Lindgren

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?

Quoted 3 lines

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.

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 */
	...
}

Quoted 3 lines · Tony Lindgren

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/.

Quoted 11 lines

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:

Quoted 1 line

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.

Quoted 10 lines

{
	int res;

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

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

Quoted 3 lines

	/* 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.

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.

Quoted 3 lines · Tony Lindgren

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.

Kevin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c ("ARM: OMAP2/3: hwmod data: Add 32k-sync timer data to hwmod database") broke CORE idle on OMAP3. This prevents device low power states.

The root cause is that the 32K sync timer IP block does not support smart-idle mode[1], and so the hwmod code keeps the IP block in no-idle mode while it is active. This in turn prevents the WKUP clockdomain from transitioning to idle. There is a hardcoded sleep dependency that prevents the CORE_L3 and CORE_CM clockdomains from transitioning to idle when the WKUP clockdomain is active[2], so the chip cannot enter any device low power states.

It turns out that there is no need to take the 32k sync timer out of idle. The IP block itself probably does not have any native idle handling at all, due to its simplicity. Furthermore, the PRCM will never request target idle for this IP block while the kernel is running, due to the sleep dependency that prevents the WKUP clockdomain from idling while the CORE_L3 clockdomain is active. So we can safely leave the 32k sync timer in target-no-idle mode, even while we continue to access it.

This workaround is implemented by programming the force-idle mode for any IP block that only supports the force-idle and no-idle modes. If an IP block is ever released that doesn't support smart-idle and requires no-idle mode to be programmed while it's in use, we'll have to change this behavior.

Isn't this impact AM33xx devices, where we do not support smart idle mode??? Anyway, I will also test it on both OMAP3EVM and AM33xx platform now...

Hi

Quoted 2 lines · Vaibhav

Isn't this impact AM33xx devices, where we do not support smart idle mode??? Anyway, I will also test it on both OMAP3EVM and AM33xx platform now...

Thanks, please let me know how your tests go. If it doesn't work, we'll go back to the flag-based approach in the patch I posted already.

I couldn't finish my testing today, got into continuous meetings.

Tomorrow, I will test it and update you on this.

Hi

Quoted 1 line · Vaibhav

I couldn't finish my testing today, got into continuous meetings.

No worries, I understand.

Quoted 1 line

Tomorrow, I will test it and update you on this.

That would be great.

I took a look at SPRUH73F and sure enough, at least one module (CONTROL) doesn't support smart-idle -- per Table 14-217 "CONTROL Register Field Descriptions". I'd guess that the PRCM won't idle-req this IP block until the kernel is not running, so we might be able to get away with the existing approach; but the TRM also says:

"By definition, initiator may generate read/write transaction as long as it is out of IDLE state."

Which pretty much matches my understanding too of the implicit interface contract here.

So I think we'd better go back to the flag approach as implemented in this patch:

http://www.spinics.net/lists/arm-kernel/msg176836.html

The WBU 32k sync timer's behavior is what relies on quirks of the integration that are hard to identify via other means, so it seems to be safest to tag it explicitly.

Quoted 24 lines · Paul Walmsley

Hi

Quoted 1 line · Vaibhav

I couldn't finish my testing today, got into continuous meetings.

No worries, I understand.

Quoted 1 line

Tomorrow, I will test it and update you on this.

That would be great.

I took a look at SPRUH73F and sure enough, at least one module (CONTROL) doesn't support smart-idle -- per Table 14-217 "CONTROL Register Field Descriptions". I'd guess that the PRCM won't idle-req this IP block until the kernel is not running, so we might be able to get away with the existing approach; but the TRM also says:

"By definition, initiator may generate read/write transaction as long as it is out of IDLE state."

Which pretty much matches my understanding too of the implicit interface contract here.

Quoted 9 lines

So I think we'd better go back to the flag approach as implemented in this patch:

http://www.spinics.net/lists/arm-kernel/msg176836.html

The WBU 32k sync timer's behavior is what relies on quirks of the integration that are hard to identify via other means, so it seems to be safest to tag it explicitly.

Paul,

I tested it on AM335x platform just now, it booted up to the Linux prompt, but I am sure it is going to impact low power state usecases on AM33xx.

So, I also feel that, flag based approach should be used here.

{

arch/arm/mach-omap2/omap_hwmod.c +4 -2

@@ -1141,8 +1143,26 @@ static void _enable_sysc(struct omap_hwmod *oh)
 	sf = oh->class->sysc->sysc_flags;
 
 	if (sf & SYSC_HAS_SIDLEMODE) {
-		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
-			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
+		if (oh->flags & HWMOD_SWSUP_SIDLE) {
+			/*
+			 * IP blocks without smart idle should be left
+			 * in force-idle.  Currently this only applies

There's a bug in this patch, see below.

There should by binary or, not logical here.

Until the OMAP4 code is converted to disable the use of the clock framework-based clockdomain enable/disable sequence, any clock used as a hwmod main_clk must have a clockdomain associated with it. This patch populates some clock structure clockdomain names to resolve the following warnings during kernel init:

omap_hwmod: dpll_mpu_m2_ck: missing clockdomain for dpll_mpu_m2_ck.
omap_hwmod: trace_clk_div_ck: missing clockdomain for trace_clk_div_ck.
omap_hwmod: l3_div_ck: missing clockdomain for l3_div_ck.
omap_hwmod: ddrphy_ck: missing clockdomain for ddrphy_ck.

Signed-off-by Paul Walmsley<[email protected]>

Cc Rajendra Nayak<[email protected]>

Cc Beno?t Cousson<[email protected]>

---
  arch/arm/mach-omap2/clock44xx_data.c |    5 +++++
  1 file changed, 5 insertions(+)

arch/arm/mach-omap2/clock44xx_data.c +5 -0

index 2172f66..e2b701e 100644
@@ -84,6 +84,7 @@ static struct clk slimbus_clk = {
 
  static struct clk sys_32k_ck = {
  	.name		= "sys_32k_ck",
+	.clkdm_name	= "prm_clkdm",
  	.rate		= 32768,
  	.ops		=&clkops_null,
  };

@@ -512,6 +513,7 @@ static struct clk ddrphy_ck = {
  	.name		= "ddrphy_ck",
  	.parent		=&dpll_core_m2_ck,
  	.ops		=&clkops_null,
+	.clkdm_name	= "l3_emif_clkdm",
  	.fixed_div	= 2,
  	.recalc		=&omap_fixed_divisor_recalc,
  };

@@ -769,6 +771,7 @@ static const struct clksel dpll_mpu_m2_div[] = {
  static struct clk dpll_mpu_m2_ck = {
  	.name		= "dpll_mpu_m2_ck",
  	.parent		=&dpll_mpu_ck,
+	.clkdm_name	= "cm_clkdm",
  	.clksel		= dpll_mpu_m2_div,
  	.clksel_reg	= OMAP4430_CM_DIV_M2_DPLL_MPU,
  	.clksel_mask	= OMAP4430_DPLL_CLKOUT_DIV_MASK,

@@ -1149,6 +1152,7 @@ static const struct clksel l3_div_div[] = {
  static struct clk l3_div_ck = {
  	.name		= "l3_div_ck",
  	.parent		=&div_core_ck,
+	.clkdm_name	= "cm_clkdm",
  	.clksel		= l3_div_div,
  	.clksel_reg	= OMAP4430_CM_CLKSEL_CORE,
  	.clksel_mask	= OMAP4430_CLKSEL_L3_MASK,

@@ -2824,6 +2828,7 @@ static const struct clksel trace_clk_div_div[] = {
  static struct clk trace_clk_div_ck = {
  	.name		= "trace_clk_div_ck",
  	.parent		=&pmd_trace_clk_mux_ck,
+	.clkdm_name	= "emu_sys_clkdm",
  	.clksel		= trace_clk_div_div,
  	.clksel_reg	= OMAP4430_CM_EMU_DEBUGSS_CLKCTRL,
  	.clksel_mask	= OMAP4430_CLKSEL_PMD_TRACE_CLK_MASK,

Quoted 5 lines · Paul Walmsley

Until the OMAP4 code is converted to disable the use of the clock framework-based clockdomain enable/disable sequence, any clock used as a hwmod main_clk must have a clockdomain associated with it. This patch populates some clock structure clockdomain names to resolve the following warnings during kernel init:

But these associations are useless if the clock is not a 'gate' clock, except for getting rid of these warnings. Maybe we should make hwmod understand that not all clocks need to have a clockdomain associated with it and stop complaining.

Quoted 11 lines · Rajendra Nayak

Quoted 5 lines · Paul Walmsley

Until the OMAP4 code is converted to disable the use of the clock framework-based clockdomain enable/disable sequence, any clock used as a hwmod main_clk must have a clockdomain associated with it. This patch populates some clock structure clockdomain names to resolve the following warnings during kernel init:

But these associations are useless if the clock is not a 'gate' clock, except for getting rid of these warnings. Maybe we should make hwmod understand that not all clocks need to have a clockdomain associated with it and stop complaining.

In retrospect, I think I should have made clockdomains mandatory for all clocks for OMAP4 back then, rather than only allowing them for some clocks. As I understand it, that would have saved a lot of time and debugging frustration on the bug fixed by commit 6c4a057bffe9823221eab547e11fac181dc18a2b ("ARM: OMAP4: clock data: Force a DPLL clkdm/pwrdm ON before a relock"). Oh well.

- Paul

Tested this set on top of v3.5-rc1 with omap3 / omap4 suspend + my omap4 core retention / dev-off patches. There are a couple of minor issues, like the bug in patch 5, and the fact that counter_32k hwmod data is broken for omap4. This fix is needed on omap4 to fix the counter_32k, if this is not done, device off does not work:

arch/arm/mach-omap2/omap_hwmod_44xx_data.c +0 -0

index 66da92d..6ad64c6 100644

Tested this set on top of v3.5-rc1 with omap3 / omap4 suspend + my omap4 core retention / dev-off patches. There are a couple of minor issues, like the bug in patch 5, and the fact that counter_32k hwmod data is broken for omap4. This fix is needed on omap4 to fix the counter_32k, if this is not done, device off does not work:

Hi Tero,

On Fri, 8 Jun 2012, Tero Kristo wrote:

Quoted 5 lines

Tested this set on top of v3.5-rc1 with omap3 / omap4 suspend + my omap4 core retention / dev-off patches. There are a couple of minor issues, like the bug in patch 5, and the fact that counter_32k hwmod data is broken for omap4. This fix is needed on omap4 to fix the counter_32k, if this is not done, device off does not work:

Thanks for the testing and the reminder. Will send the next version of this soon to address more comments from Tony.