diff mbox series

[5/8] ARM: OMAP4: hwmod_data: Remove modulemode from IPU/DSP hwmods

Message ID 20170821234818.4755-6-s-anna@ti.com
State New
Headers show
Series Add hwmod data for IPU & DSP processors/MMUs | expand

Commit Message

Suman Anna Aug. 21, 2017, 11:48 p.m. UTC 
The .modulemode field is removed from both the IPU and DSP processor
hwmods. This fixes a potential kernel crash during the shutdown sequence
of any of these remoteproc devices, either during a normal teardown or
during a recovery.

The DSP and IPU processor subsystems are represented by multiple hwmods,
one for each of the MMUs present within the subsystem and one for the
processor cores. The processor subsystem is clocked from a single clock
source with separate reset lines for each of the processors and the
MMUs. This clock and reset information is represented in separate hwmods
to allow the management of these entities in different drivers operating
on the corresponding platform devices. This doesn't fit quite well into
the current omap_hwmod layer due to these inter-dependencies.

A remoteproc startup sequence involves enabling and programming of the
MMUs, loading of the firmware into RAM mapped by the MMUs, and releasing
the processors from reset. A shutdown sequence follows a reverse pattern
with the processors put into reset first followed by the unmapping and
disabling of the MMUs. Both the processors and the MMUs are present
within a single clock domain and requires the domain be clocked and
enabled until the last entity. The kernel crash can happen during the
unmapping phase of the MMUs, as the hwmod layer disables the module
during the omap_device_idle processing of the processor hwmod. This
issue is fixed by not defining a modulemode for the IPU/DSP processors,
which results in keeping the module in an activated state. The module
is disabled properly during the omap_device_idle processing of the MMU
hwmod while disabling the MMU.

Signed-off-by: Suman Anna <s-anna@ti.com>

---
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 2 --
 1 file changed, 2 deletions(-)

-- 
2.13.1

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tony Lindgren Aug. 22, 2017, 5:37 p.m. UTC | #1 
* Suman Anna <s-anna@ti.com> [170821 16:48]:
>

> A shutdown sequence follows a reverse pattern

> with the processors put into reset first followed by the unmapping and

> disabling of the MMUs. Both the processors and the MMUs are present

> within a single clock domain and requires the domain be clocked and

> enabled until the last entity. The kernel crash can happen during the

> unmapping phase of the MMUs, as the hwmod layer disables the module

> during the omap_device_idle processing of the processor hwmod. This

> issue is fixed by not defining a modulemode for the IPU/DSP processors,

> which results in keeping the module in an activated state. The module

> is disabled properly during the omap_device_idle processing of the MMU

> hwmod while disabling the MMU.


I think a better way to fix this would be to make sure the module
is enabled during the unmapping phase of the MMUs. If there is no
driver left at that point to call pm_runtime_get() on the module,
do it via pdata-quirks.c using struct iommu_platform_data?

If you can't add the enable/disable around existing assert_reset/
deassert_reset, you can add a new one.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Suman Anna Aug. 22, 2017, 6:44 p.m. UTC | #2 
Hi Tony,

On 08/22/2017 12:37 PM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [170821 16:48]:

>>

>> A shutdown sequence follows a reverse pattern

>> with the processors put into reset first followed by the unmapping and

>> disabling of the MMUs. Both the processors and the MMUs are present

>> within a single clock domain and requires the domain be clocked and

>> enabled until the last entity. The kernel crash can happen during the

>> unmapping phase of the MMUs, as the hwmod layer disables the module

>> during the omap_device_idle processing of the processor hwmod. This

>> issue is fixed by not defining a modulemode for the IPU/DSP processors,

>> which results in keeping the module in an activated state. The module

>> is disabled properly during the omap_device_idle processing of the MMU

>> hwmod while disabling the MMU.

> 

> I think a better way to fix this would be to make sure the module

> is enabled during the unmapping phase of the MMUs. If there is no

> driver left at that point to call pm_runtime_get() on the module,

> do it via pdata-quirks.c using struct iommu_platform_data?


The problem is because there is no reference counting on modulemode
programming unlike clocks or omap_device pm_domain callbacks. The IOMMU
driver already has an active pm_runtime_get() invoked earlier and
invoking another wouldn't result in any change.

> If you can't add the enable/disable around existing assert_reset/

> deassert_reset, you can add a new one.


The remoteproc driver is only dealing with its resets and hwmod while
the IOMMU driver is dealing with its dedicated reset. The PRCM registers
though are a single set between the two.

regards
Suman
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Aug. 22, 2017, 7:24 p.m. UTC | #3 
* Suman Anna <s-anna@ti.com> [170822 11:45]:
> On 08/22/2017 12:37 PM, Tony Lindgren wrote:

> > I think a better way to fix this would be to make sure the module

> > is enabled during the unmapping phase of the MMUs. If there is no

> > driver left at that point to call pm_runtime_get() on the module,

> > do it via pdata-quirks.c using struct iommu_platform_data?

> 

> The problem is because there is no reference counting on modulemode

> programming unlike clocks or omap_device pm_domain callbacks. The IOMMU

> driver already has an active pm_runtime_get() invoked earlier and

> invoking another wouldn't result in any change.


Hmm iommu driver has pm_runtime_get() on which modules? Can you
please point me to that code too so I can follow..

Or is there maybe a single module shared across multiple devices?

If so, we need a minimal module wrapper driver. You can do what we
already do for musb on am335x in drivers/usb/musb/musb_am335x.c.
A single module has two musb instances and a shared cppi41 dma
instance. See also it's related entries in am33xx.dtsi.

Note that the clkctrl clocks are available now as clocks, so they
could be directly enabled for testing. See omap4_tesla_clkctrl_regs
and omap4_ducati_clkctrl_regs if that helps.

> The remoteproc driver is only dealing with its resets and hwmod while

> the IOMMU driver is dealing with its dedicated reset. The PRCM registers

> though are a single set between the two.


Sorry but I'm having hard time following which driver claims
which modules :)

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Suman Anna Aug. 22, 2017, 8:54 p.m. UTC | #4 
Hi Tony,

On 08/22/2017 02:24 PM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [170822 11:45]:

>> On 08/22/2017 12:37 PM, Tony Lindgren wrote:

>>> I think a better way to fix this would be to make sure the module

>>> is enabled during the unmapping phase of the MMUs. If there is no

>>> driver left at that point to call pm_runtime_get() on the module,

>>> do it via pdata-quirks.c using struct iommu_platform_data?

>>

>> The problem is because there is no reference counting on modulemode

>> programming unlike clocks or omap_device pm_domain callbacks. The IOMMU

>> driver already has an active pm_runtime_get() invoked earlier and

>> invoking another wouldn't result in any change.

> 

> Hmm iommu driver has pm_runtime_get() on which modules? Can you

> please point me to that code too so I can follow..


The OMAP IOMMU driver deals with the MMU device only and uses both the
omap_device_{assert/deassert}_hardreset and omap_device_{enable/idle} on
the omap_device associated with the MMU device. The MMU register space
has a SYSCONFIG register (also has softreset that only resets the MMU
block) so we do have a hwmod associated with it to deal with its
setting. Today, the reset API is invoked through pdata-quirks and the
omap_device_{enable/idle} API through the pm_runtime in the IOMMU
driver. Lookup iommu_enable() and iommu_disable() in
drivers/iommu/omap-iommu.c

> Or is there maybe a single module shared across multiple devices?


Yeah kinda. The PRCM registers are just one set, with different bits in
RSTCTRL and RSTST registers dealing with different reset lines. The API
to deal with resets is still using the omap_device API and they are
invoked using pdata-quirks. For OMAP remoteproc driver, we only have one
pdata quirks defined, but the function we plug in invokes both the reset
as well as the omap_device_enable/idle functions (not in mainline yet).
See [1] for reference.

So, we have a unique hwmod/omap_device associated with each of the
individual devices (IOMMU and the processor are represented as a device
each).

> If so, we need a minimal module wrapper driver. You can do what we

> already do for musb on am335x in drivers/usb/musb/musb_am335x.c.

> A single module has two musb instances and a shared cppi41 dma

> instance. See also it's related entries in am33xx.dtsi.

> 

> Note that the clkctrl clocks are available now as clocks, so they

> could be directly enabled for testing. See omap4_tesla_clkctrl_regs

> and omap4_ducati_clkctrl_regs if that helps.


OK, looks like these are recent additions by Tero. It will still be a
shared set between the MMU and remoteproc drivers, but let me look into
how these get integrated. At this point, I don't want to be dealing with
separate clk API in my driver.

>> The remoteproc driver is only dealing with its resets and hwmod while

>> the IOMMU driver is dealing with its dedicated reset. The PRCM registers

>> though are a single set between the two.

> 

> Sorry but I'm having hard time following which driver claims

> which modules :)


OMAP IOMMU driver claims and deals with the MMU device in each of the
DSP and IPU subsystems. The OMAP remoteproc driver deals with the DSP or
the Cortex-M3/M4 cores.

regards
Suman

[1]
http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=blob;f=arch/arm/mach-omap2/remoteproc.c;h=78a4c4746bb568420b251bc42db02849ef439e27;hb=6f7d874549ce847eeb0308704b6a477c308f59e5
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 3e2d792fd9df..d183ffdf37e6 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -550,7 +550,6 @@  static struct omap_hwmod omap44xx_dsp_hwmod = {
 			.clkctrl_offs = OMAP4_CM_TESLA_TESLA_CLKCTRL_OFFSET,
 			.rstctrl_offs = OMAP4_RM_TESLA_RSTCTRL_OFFSET,
 			.context_offs = OMAP4_RM_TESLA_TESLA_CONTEXT_OFFSET,
-			.modulemode   = MODULEMODE_HWCTRL,
 		},
 	},
 };
@@ -1561,7 +1560,6 @@  static struct omap_hwmod omap44xx_ipu_hwmod = {
 			.clkctrl_offs = OMAP4_CM_DUCATI_DUCATI_CLKCTRL_OFFSET,
 			.rstctrl_offs = OMAP4_RM_DUCATI_RSTCTRL_OFFSET,
 			.context_offs = OMAP4_RM_DUCATI_DUCATI_CONTEXT_OFFSET,
-			.modulemode   = MODULEMODE_HWCTRL,
 		},
 	},
 };