| Message ID | 20190402133752.6912-5-rogerq@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | bus: ti-sysc: Add generic enable/disable & PRUSS | expand |
* Roger Quadros <rogerq@ti.com> [190402 13:38]: > The PRU-ICSS subsystem's SYSCONFIG register is similar to > omap4-simple but has 2 special bits STANDBY_INIT and SUB_MWAIT. > > The STANDBY_INIT bit initiates a Standby sequence (when set) and > triggers a MStandby request to the SoC's PRCM module. This same > bit is also used to enable the OCP master ports (when cleared). > > Some PRU applications require the OCP master port access to be > enabled thus keeping it out of standby. So do we need to configure this depending on the application? > During sustem suspend/resume we must ensure that the PRUSS is in > standby else it will break resume. > > NOTE: > 1. This patch only adds the PM callbacks with code to fix the System > Suspend/Resume hang issue on AM33xx/AM437x SoCs, but does not > implement the full context save and restore required for the PRUSS > drivers to work across system suspend/resume when the power domain > is switched off (L4PER domain is switched OFF on AM335x/AM437x > during system suspend/resume, so PRUSS modules do lose context). I think we already restore the interconnect target module access properly on resume. If not we should fix that. Saving and restoring the child device state is up to the device drivers managing the child device(s), and there's not much ti-sysc.c can do about it, right? > @@ -92,6 +93,7 @@ struct sysc { > bool enabled; > bool needs_resume; > bool child_needs_resume; > + bool in_standby; > struct delayed_work idle_work; > }; We should start using bitfields for the bool here, might as well already do it now: unsigned long in_standby:1; See "17) Using bool" in Documentation/process/coding-style.rst. > @@ -1023,6 +1025,21 @@ static int __maybe_unused sysc_noirq_suspend(struct device *dev) > if (ddata->cfg.quirks & SYSC_QUIRK_LEGACY_IDLE) > return 0; > > + if (ddata->cap->type == TI_SYSC_PRUSS) { Should this test be made more generic based on the mstandby bit being configured? And can you please make these into separate functions to avoid cluttering the suspend and resume functions. Something like sysc_handle_mstandby() maybe? > + u32 reg, mask; > + const struct sysc_regbits *regbits = ddata->cap->regbits; > + > + mask = BIT(regbits->standby_init_shift); > + reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]); > + ddata->in_standby = reg & mask; Hmm so could we just assume that the device drivers for child device(s) configure the MSTANDBY bit? Or do we need to manage it in ti-sysc? See for example drivers/usb/musb/omap2430.c omap2430_low_level_init() and omap2430_low_level_exit(). That's a separate register though. Regards, Tony
On 02/04/2019 19:57, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [190402 13:38]: >> The PRU-ICSS subsystem's SYSCONFIG register is similar to >> omap4-simple but has 2 special bits STANDBY_INIT and SUB_MWAIT. >> >> The STANDBY_INIT bit initiates a Standby sequence (when set) and >> triggers a MStandby request to the SoC's PRCM module. This same >> bit is also used to enable the OCP master ports (when cleared). >> >> Some PRU applications require the OCP master port access to be >> enabled thus keeping it out of standby. > > So do we need to configure this depending on the application? Yes. > >> During sustem suspend/resume we must ensure that the PRUSS is in >> standby else it will break resume. >> >> NOTE: >> 1. This patch only adds the PM callbacks with code to fix the System >> Suspend/Resume hang issue on AM33xx/AM437x SoCs, but does not >> implement the full context save and restore required for the PRUSS >> drivers to work across system suspend/resume when the power domain >> is switched off (L4PER domain is switched OFF on AM335x/AM437x >> during system suspend/resume, so PRUSS modules do lose context). > > I think we already restore the interconnect target module access > properly on resume. If not we should fix that. > > Saving and restoring the child device state is up to the device > drivers managing the child device(s), and there's not much ti-sysc.c > can do about it, right? Agreed. In that case this handling should be done by pruss.c and uio_pruss.c in their suspend/resume handlers. Suman, do you agree? > >> @@ -92,6 +93,7 @@ struct sysc { >> bool enabled; >> bool needs_resume; >> bool child_needs_resume; >> + bool in_standby; >> struct delayed_work idle_work; >> }; > > We should start using bitfields for the bool here, might as well > already do it now: > > unsigned long in_standby:1; > > See "17) Using bool" in Documentation/process/coding-style.rst. > >> @@ -1023,6 +1025,21 @@ static int __maybe_unused sysc_noirq_suspend(struct device *dev) >> if (ddata->cfg.quirks & SYSC_QUIRK_LEGACY_IDLE) >> return 0; >> >> + if (ddata->cap->type == TI_SYSC_PRUSS) { > > Should this test be made more generic based on the mstandby > bit being configured? No other module uses this bit. It is specific to PRUSS. > > And can you please make these into separate functions to > avoid cluttering the suspend and resume functions. Something > like sysc_handle_mstandby() maybe? > >> + u32 reg, mask; >> + const struct sysc_regbits *regbits = ddata->cap->regbits; >> + >> + mask = BIT(regbits->standby_init_shift); >> + reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]); >> + ddata->in_standby = reg & mask; > > Hmm so could we just assume that the device drivers for child device(s) > configure the MSTANDBY bit? Or do we need to manage it in ti-sysc? > I'm in favor of managing it in the child device driver. Let's see if Suman has any concerns. > See for example drivers/usb/musb/omap2430.c omap2430_low_level_init() > and omap2430_low_level_exit(). That's a separate register though. > -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c index e4ab4d422ea5..9c94ce08dd36 100644 --- a/drivers/bus/ti-sysc.c +++ b/drivers/bus/ti-sysc.c @@ -71,6 +71,7 @@ static const char * const clock_names[SYSC_MAX_CLOCKS] = { * @name: name if available * @revision: interconnect target module revision * @needs_resume: runtime resume needed on resume from suspend + * @in_standby: flag used by PRUSS type during suspend/resume */ struct sysc { struct device *dev; @@ -92,6 +93,7 @@ struct sysc { bool enabled; bool needs_resume; bool child_needs_resume; + bool in_standby; struct delayed_work idle_work; }; @@ -1023,6 +1025,21 @@ static int __maybe_unused sysc_noirq_suspend(struct device *dev) if (ddata->cfg.quirks & SYSC_QUIRK_LEGACY_IDLE) return 0; + if (ddata->cap->type == TI_SYSC_PRUSS) { + u32 reg, mask; + const struct sysc_regbits *regbits = ddata->cap->regbits; + + mask = BIT(regbits->standby_init_shift); + reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]); + ddata->in_standby = reg & mask; + + /* initiate MStandby */ + if (!ddata->in_standby) { + reg |= mask; + sysc_write(ddata, ddata->offsets[SYSC_SYSCONFIG], reg); + } + } + return pm_runtime_force_suspend(dev); } @@ -1035,6 +1052,25 @@ static int __maybe_unused sysc_noirq_resume(struct device *dev) if (ddata->cfg.quirks & SYSC_QUIRK_LEGACY_IDLE) return 0; + if (ddata->cap->type == TI_SYSC_PRUSS && !ddata->in_standby) { + u32 reg; + const struct sysc_regbits *regbits = ddata->cap->regbits; + + /* re-enable OCP master ports/disable MStandby */ + reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]); + reg &= ~BIT(regbits->standby_init_shift); + sysc_write(ddata, ddata->offsets[SYSC_SYSCONFIG], reg); + ddata->in_standby = 0; + + /* wait till ready for transactions - delay is arbitrary */ + usleep_range(50, 100); + reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]); + if (reg & BIT(regbits->sub_mwait_shift)) { + dev_err(dev, "timeout waiting for SUB_MWAIT_READY\n"); + return -ETIMEDOUT; + } + } + return pm_runtime_force_resume(dev); }
The PRU-ICSS subsystem's SYSCONFIG register is similar to omap4-simple but has 2 special bits STANDBY_INIT and SUB_MWAIT. The STANDBY_INIT bit initiates a Standby sequence (when set) and triggers a MStandby request to the SoC's PRCM module. This same bit is also used to enable the OCP master ports (when cleared). Some PRU applications require the OCP master port access to be enabled thus keeping it out of standby. During sustem suspend/resume we must ensure that the PRUSS is in standby else it will break resume. NOTE: 1. This patch only adds the PM callbacks with code to fix the System Suspend/Resume hang issue on AM33xx/AM437x SoCs, but does not implement the full context save and restore required for the PRUSS drivers to work across system suspend/resume when the power domain is switched off (L4PER domain is switched OFF on AM335x/AM437x during system suspend/resume, so PRUSS modules do lose context). 2. The PRUSS driver functionality on AM57xx SoCs is not affected that much because the PER power domain to which the PRUSS IPs belong is not switched OFF during suspend/resume. Based on work by Suman Anna. Cc: Suman Anna <s-anna@ti.com> Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/bus/ti-sysc.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki