| Message ID | 20170821234818.4755-6-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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
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
* 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
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 --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, }, }, };
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(-)