resume path in dra7xx and other DW-based drivers

resume path in dra7xx and other DW-based drivers

[Bjorn Helgaas]

Hi Kishon, et al,

Does dra7xx suspend/resume work?  I'm not sure dra7xx_pcie_resume()
and dra7xx_pcie_resume_noirq() restore everything necessary.  For
example, the probe path has this:

  dra7xx_pcie_probe
    dra7xx_add_pcie_port
      dw_pcie_host_init
	dra7xx_pcie_host_init         # .host_init
	  dw_pcie_setup_rc
	    dw_pcie_prog_outbound_atu

so I think it programs the ATU in dw_pcie_setup_rc().  But the resume
path doesn't call dw_pcie_setup_rc(), so I don't see where the ATU
setup would be restored.

Related to this, dra7xx_pcie_resume_noirq() contains phy init and
power-on code that is duplicated in dra7xx_pcie_probe().  It seems
like that ought to be done by a common function instead of being cut
and pasted.

I haven't looked in detail at all the other DW-based drivers; dra7xx
just happens to be first in alphabetical order :)  But I suspect I'd
have the same question about them.

Bjorn

Re: resume path in dra7xx and other DW-based drivers

[Niklas Cassel]

On 12/07/2016 11:02 PM, Bjorn Helgaas wrote:
> Hi Kishon, et al,
>
> Does dra7xx suspend/resume work?  I'm not sure dra7xx_pcie_resume()
> and dra7xx_pcie_resume_noirq() restore everything necessary.  For
> example, the probe path has this:
>
>   dra7xx_pcie_probe
>     dra7xx_add_pcie_port
>       dw_pcie_host_init
> 	dra7xx_pcie_host_init         # .host_init
> 	  dw_pcie_setup_rc
> 	    dw_pcie_prog_outbound_atu
>
> so I think it programs the ATU in dw_pcie_setup_rc().  But the resume
> path doesn't call dw_pcie_setup_rc(), so I don't see where the ATU
> setup would be restored.
>
> Related to this, dra7xx_pcie_resume_noirq() contains phy init and
> power-on code that is duplicated in dra7xx_pcie_probe().  It seems
> like that ought to be done by a common function instead of being cut
> and pasted.
>
> I haven't looked in detail at all the other DW-based drivers; dra7xx
> just happens to be first in alphabetical order :)  But I suspect I'd
> have the same question about them.

There doesn't appear to be any DW-based driver supporting
suspend/resume other than dra7xx :)


>
> Bjorn

Re: resume path in dra7xx and other DW-based drivers

[Kishon Vijay Abraham I]

Hi Bjorn,

On Thursday 08 December 2016 03:32 AM, Bjorn Helgaas wrote:
> Hi Kishon, et al,
> 
> Does dra7xx suspend/resume work?  I'm not sure dra7xx_pcie_resume()
> and dra7xx_pcie_resume_noirq() restore everything necessary.  For
> example, the probe path has this:
> 
>   dra7xx_pcie_probe
>     dra7xx_add_pcie_port
>       dw_pcie_host_init
> 	dra7xx_pcie_host_init         # .host_init
> 	  dw_pcie_setup_rc
> 	    dw_pcie_prog_outbound_atu
> 
> so I think it programs the ATU in dw_pcie_setup_rc().  But the resume
> path doesn't call dw_pcie_setup_rc(), so I don't see where the ATU
> setup would be restored.

DRA7xx only supported shallow power state and not deep power state. In shallow
power state, the register settings are not lost and hence we didn't have to
restore anything.
> 
> Related to this, dra7xx_pcie_resume_noirq() contains phy init and
> power-on code that is duplicated in dra7xx_pcie_probe().  It seems
> like that ought to be done by a common function instead of being cut
> and pasted.

yeah, that has to be cleaned up.

Thanks
Kishon

Re: resume path in dra7xx and other DW-based drivers

[Bjorn Helgaas]

On Thu, Dec 08, 2016 at 02:19:00PM +0530, Kishon Vijay Abraham I wrote:
> Hi Bjorn,
> 
> On Thursday 08 December 2016 03:32 AM, Bjorn Helgaas wrote:
> > Hi Kishon, et al,
> > 
> > Does dra7xx suspend/resume work?  I'm not sure dra7xx_pcie_resume()
> > and dra7xx_pcie_resume_noirq() restore everything necessary.  For
> > example, the probe path has this:
> > 
> >   dra7xx_pcie_probe
> >     dra7xx_add_pcie_port
> >       dw_pcie_host_init
> > 	dra7xx_pcie_host_init         # .host_init
> > 	  dw_pcie_setup_rc
> > 	    dw_pcie_prog_outbound_atu
> > 
> > so I think it programs the ATU in dw_pcie_setup_rc().  But the resume
> > path doesn't call dw_pcie_setup_rc(), so I don't see where the ATU
> > setup would be restored.
> 
> DRA7xx only supported shallow power state and not deep power state. In shallow
> power state, the register settings are not lost and hence we didn't have to
> restore anything.

I'm not really a PM guy, so I'm not familiar with shallow power state.
Could I have discovered from the code somehow that dra7xx suspend to
shallow power state preserves register state?  It's nice if a reader
can verify correctness without knowing the system-specific details.

Bjorn

Re: resume path in dra7xx and other DW-based drivers

[Kishon Vijay Abraham I]

Hi Bjorn,

On Friday 09 December 2016 02:13 AM, Bjorn Helgaas wrote:
> On Thu, Dec 08, 2016 at 02:19:00PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Bjorn,
>>
>> On Thursday 08 December 2016 03:32 AM, Bjorn Helgaas wrote:
>>> Hi Kishon, et al,
>>>
>>> Does dra7xx suspend/resume work?  I'm not sure dra7xx_pcie_resume()
>>> and dra7xx_pcie_resume_noirq() restore everything necessary.  For
>>> example, the probe path has this:
>>>
>>>   dra7xx_pcie_probe
>>>     dra7xx_add_pcie_port
>>>       dw_pcie_host_init
>>> 	dra7xx_pcie_host_init         # .host_init
>>> 	  dw_pcie_setup_rc
>>> 	    dw_pcie_prog_outbound_atu
>>>
>>> so I think it programs the ATU in dw_pcie_setup_rc().  But the resume
>>> path doesn't call dw_pcie_setup_rc(), so I don't see where the ATU
>>> setup would be restored.
>>
>> DRA7xx only supported shallow power state and not deep power state. In shallow
>> power state, the register settings are not lost and hence we didn't have to
>> restore anything.
> 
> I'm not really a PM guy, so I'm not familiar with shallow power state.
> Could I have discovered from the code somehow that dra7xx suspend to
> shallow power state preserves register state?  It's nice if a reader
> can verify correctness without knowing the system-specific details.

Some drivers like drivers/mmc/host/omap_hsmmc.c saves the register contents
(omap_hsmmc_context_save) and then in runtime_resume checks if the register
contents are still same (omap_hsmmc_context_restore). Do you think we have to
do something like that for dra7xx?

omap_hsmmc.c is used by a lot of platforms some of which loses the context
during suspend. Since dra7xx doesn't loose context, not sure if we have to do
something like omap_hsmmc.

Thanks
Kishon

Re: resume path in dra7xx and other DW-based drivers

[Lucas Stach]

Am Dienstag, den 20.12.2016, 11:55 +0530 schrieb Kishon Vijay Abraham I:
> Hi Bjorn,
> 
> On Friday 09 December 2016 02:13 AM, Bjorn Helgaas wrote:
> > On Thu, Dec 08, 2016 at 02:19:00PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi Bjorn,
> >>
> >> On Thursday 08 December 2016 03:32 AM, Bjorn Helgaas wrote:
> >>> Hi Kishon, et al,
> >>>
> >>> Does dra7xx suspend/resume work?  I'm not sure dra7xx_pcie_resume()
> >>> and dra7xx_pcie_resume_noirq() restore everything necessary.  For
> >>> example, the probe path has this:
> >>>
> >>>   dra7xx_pcie_probe
> >>>     dra7xx_add_pcie_port
> >>>       dw_pcie_host_init
> >>> 	dra7xx_pcie_host_init         # .host_init
> >>> 	  dw_pcie_setup_rc
> >>> 	    dw_pcie_prog_outbound_atu
> >>>
> >>> so I think it programs the ATU in dw_pcie_setup_rc().  But the resume
> >>> path doesn't call dw_pcie_setup_rc(), so I don't see where the ATU
> >>> setup would be restored.
> >>
> >> DRA7xx only supported shallow power state and not deep power state. In shallow
> >> power state, the register settings are not lost and hence we didn't have to
> >> restore anything.
> > 
> > I'm not really a PM guy, so I'm not familiar with shallow power state.
> > Could I have discovered from the code somehow that dra7xx suspend to
> > shallow power state preserves register state?  It's nice if a reader
> > can verify correctness without knowing the system-specific details.
> 
> Some drivers like drivers/mmc/host/omap_hsmmc.c saves the register contents
> (omap_hsmmc_context_save) and then in runtime_resume checks if the register
> contents are still same (omap_hsmmc_context_restore). Do you think we have to
> do something like that for dra7xx?
> 
> omap_hsmmc.c is used by a lot of platforms some of which loses the context
> during suspend. Since dra7xx doesn't loose context, not sure if we have to do
> something like omap_hsmmc.

I think we need some DW-PCIe helper functions that save and restore the
register state and can be called from drivers on platforms where the
core losses its state over PM.

I think this should be fully in the control of the individual glue
drivers, as it is really platform dependent. On MX6 the older
generations only support shallow PM states, where we don't need the
save/restore dance, but newer generations could go into deeper idle
where the core looses state.

Regards,
Lucas