[2/2] fsl/pci: The new pci suspend/resume implementation

[Dongsheng.Wang at freescale.com]

Hi Scott,

I will send v2 patch to fix your comment. Thanks for your review. :)

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, March 20, 2014 5:01 AM
> To: Wang Dongsheng-B40534
> Cc: bhelgaas at google.com; rjw at rjwysocki.net; roy.zang at freescale.com;
> galak at codeaurora.org; linux-pci at vger.kernel.org; linuxppc-dev at lists.ozlabs.org
> Subject: Re: [2/2] fsl/pci: The new pci suspend/resume implementation
> 
> On Tue, Jan 07, 2014 at 04:04:08PM +0800, Dongsheng Wang wrote:
> > From: Wang Dongsheng <dongsheng.wang at freescale.com>
> >
> > The new suspend/resume implementation, send pme turnoff message in
> > suspend, and send pme exit message in resume.
> >
> > Add a PME handler, to response PME & message interrupt.
> >
> > Change platform_driver->suspend/resume to syscore->suspend/resume.
> > pci-driver will call back EP device, to save EP state in
> > pci_pm_suspend_noirq, so we need to keep the link, until
> > pci_pm_suspend_noirq finish.
> >
> > Signed-off-by: Wang Dongsheng <dongsheng.wang at freescale.com>
> 
> Is this patch OK to go in without patch 1/2?  It's not clear whether that was
> deemed incorrect (as in new patch coming) or unnecessary.
> 

Yes, I will abandon 1/2. And send this as a independent patch.

> It would also be good if you submit with the explanation from
> http://www.spinics.net/lists/linux-pci/msg27844.html in the commit message.
> 

Thanks.

> > -static int fsl_pci_probe(struct platform_device *pdev)
> > +#ifdef CONFIG_PM
> > +static irqreturn_t fsl_pci_pme_handle(int irq, void *dev_id)
> >  {
> > -	int ret;
> > -	struct device_node *node;
> > +	struct pci_controller *hose = dev_id;
> > +	struct ccsr_pci __iomem *pci = hose->private_data;
> > +	u32 dr;
> >
> > -	node = pdev->dev.of_node;
> > -	ret = fsl_add_bridge(pdev, fsl_pci_primary == node);
> > +	dr = in_be32(&pci->pex_pme_mes_dr);
> > +	if (dr)
> > +		out_be32(&pci->pex_pme_mes_dr, dr);
> > +	else
> > +		return IRQ_NONE;
> >
> > -	mpc85xx_pci_err_probe(pdev);
> > +	return IRQ_HANDLED;
> > +}
> 
> Why do you put some of the HANDLED path in the if statement, and some outside?
> 
> Just do:
> 
> if (!dr)
> 	return IRQ_NONE;
> 
> out_be32(...);
> return IRQ_HANDLED;
> 

Right. :)

> > +static int fsl_pci_pme_probe(struct pci_controller *hose) {
> > +	struct ccsr_pci __iomem *pci;
> > +	struct pci_dev *dev = hose->bus->self;
> > +	u16 pms;
> > +	int pme_irq;
> > +	int res;
> > +
> > +	/* PME Disable */
> > +	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pms);
> > +	pms &= ~PCI_PM_CTRL_PME_ENABLE;
> > +	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pms);
> > +
> > +	pme_irq = irq_of_parse_and_map(hose->dn, 0);
> > +	if (!pme_irq) {
> > +		pr_warn("Failed to map PME interrupt.\n");
> 
> dev_err()
> 
> > +
> > +		return -ENXIO;
> > +	}
> > +
> > +	res = devm_request_irq(hose->parent, pme_irq,
> > +			fsl_pci_pme_handle,
> > +			IRQF_DISABLED | IRQF_SHARED,
> > +			"[PCI] PME", hose);
> 
> IRQF_DISABLED is a deprecated no-op.
> 
> > +	if (res < 0) {
> > +		pr_warn("Unable to requiest irq %d for PME\n", pme_irq);
> 
> dev_err() etc.
> 

Ok, I will use it.

Regards,
-Dongsheng

> -Scott