Re: [PATCH v11 04/11] PCI: brcmstb: Add suspend and resume pm_ops

[Florian Fainelli <f.fainelli@gmail.com>]

On 9/10/2020 8:56 AM, Rob Herring wrote:
> On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote:
>> From: Jim Quinlan <jquinlan@broadcom.com>
>>
>> Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend
>> and resume.  Now the PCIe driver may do so as well.
>>
>> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
>> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>> index c2b3d2946a36..3d588ab7a6dd 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
>>   	brcm_pcie_bridge_sw_init_set(pcie, 1);
>>   }
>>   
>> +static int brcm_pcie_suspend(struct device *dev)
>> +{
>> +	struct brcm_pcie *pcie = dev_get_drvdata(dev);
>> +
>> +	brcm_pcie_turn_off(pcie);
>> +	clk_disable_unprepare(pcie->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int brcm_pcie_resume(struct device *dev)
>> +{
>> +	struct brcm_pcie *pcie = dev_get_drvdata(dev);
>> +	void __iomem *base;
>> +	u32 tmp;
>> +	int ret;
>> +
>> +	base = pcie->base;
>> +	clk_prepare_enable(pcie->clk);
>> +
>> +	/* Take bridge out of reset so we can access the SERDES reg */
>> +	brcm_pcie_bridge_sw_init_set(pcie, 0);
>> +
>> +	/* SERDES_IDDQ = 0 */
>> +	tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
>> +	u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK);
>> +	writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
>> +
>> +	/* wait for serdes to be stable */
>> +	udelay(100);
> 
> Really needs to be a spinloop?
> 
>> +
>> +	ret = brcm_pcie_setup(pcie);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (pcie->msi)
>> +		brcm_msi_set_regs(pcie->msi);
>> +
>> +	return 0;
>> +}
>> +
>>   static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>>   {
>>   	brcm_msi_remove(pcie);
>> @@ -1087,12 +1128,18 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>   
>>   MODULE_DEVICE_TABLE(of, brcm_pcie_match);
>>   
>> +static const struct dev_pm_ops brcm_pcie_pm_ops = {
>> +	.suspend_noirq = brcm_pcie_suspend,
>> +	.resume_noirq = brcm_pcie_resume,
> 
> Why do you need interrupts disabled? There's 39 cases of .suspend_noirq
> and 1352 of .suspend in the tree.
> 
> Is doing a clk unprepare even safe in .suspend_noirq? IIRC,
> prepare/unprepare can sleep.

Yes, it is safe, provided that your clock provider (clk-scmi.c in our 
case) supports it, too. In our case the underlying mailbox driver has 
its interrupts flagged with IRQF_NOSUSPEND such that they can still be 
processed at _noirq time.

I think the rationale was to ensure that this would be done much later 
after other subsystem have been made quiescent, but given the Linux 
device driver model, the PCI bridge should be suspended after all 
pci_device child device, so it should be safe not to use _noirq.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel