* Re: [RFC PATCH 4/4] bus: ti-sysc: Ensure PRU-ICSS doesn't break suspend/resume
2019-04-02 16:57 ` [RFC PATCH 4/4] bus: ti-sysc: Ensure PRU-ICSS doesn't break suspend/resume Tony Lindgren
@ 2019-04-03 8:46 ` Roger Quadros
0 siblings, 0 replies; 2+ messages in thread
From: Roger Quadros @ 2019-04-03 8:46 UTC (permalink / raw)
To: Tony Lindgren, s-anna; +Cc: nsekhar, linux-kernel, linux-omap
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
^ permalink raw reply [flat|nested] 2+ messages in thread