Re: [PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 10, 2017 at 10:14:33PM +0100, Marek Vasut wrote:
> On 11/10/2017 09:59 AM, Simon Horman wrote:
> > Hi Marek,
> 
> Hi Simon,
> 
> > On Wed, Nov 08, 2017 at 10:28:04AM +0100, Marek Vasut wrote:
> >> From: Kazufumi Ikeda <kaz-ikeda@xxxxxxxxxxxxx>
> >>
> >> Reestablish the PCIe link very early in the resume process in case it
> >> went down to prevent PCI accesses from hanging the bus. Such accesses
> >> can happen early in the PCI resume process, in the resume_noirq, thus
> >> the link must be reestablished in the resume_noirq callback of the
> >> driver.
> >>
> >> Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xxxxxxxxxxxxx>
> >> Signed-off-by: Gaku Inami <gaku.inami.xw@xxxxxxxxxxxxxx>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
> >> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> >> Cc: Simon Horman <horms+renesas@xxxxxxxxxxxx>
> >> Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>
> >> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> > 
> > For patch-sets (with more than one patch) please provide a cover-letter.
> > The --cover-letter option to git format-patch can help.
> > 
> >> ---
> >>  drivers/pci/host/pcie-rcar.c | 31 ++++++++++++++++++++++++++++---
> >>  1 file changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> >> index 889603783f01..aa588a7d4811 100644
> >> --- a/drivers/pci/host/pcie-rcar.c
> >> +++ b/drivers/pci/host/pcie-rcar.c
> >> @@ -43,6 +43,7 @@
> >>  
> >>  /* Transfer control */
> >>  #define PCIETCTLR		0x02000
> >> +#define  DL_DOWN		(1 << 3)
> > 
> > Can you use the BIT() macro here?
> 
> Yes, I'll also add a patch to the series to convert all the others to
> the BIT() macro, to keep things consistent.

Great, thanks.

> >>  #define  CFINIT			1
> >>  #define PCIETSTR		0x02004
> >>  #define  DATA_LINK_ACTIVE	1
> >> @@ -529,7 +530,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
> >>  	phy_wait_for_ack(pcie);
> >>  }
> >>  
> >> -static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
> >> +static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie, int atomic)
> > 
> > As Sergei mentioned bool seems like a more appropriate type for atomic.
> 
> I can actually get rid of this altogether.

Nice :)

> >>  {
> >>  	unsigned int timeout = 10;
> >>  
> >> @@ -537,7 +538,10 @@ static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
> >>  		if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE))
> >>  			return 0;
> >>  
> >> -		msleep(5);
> >> +		if (atomic)
> >> +			mdelay(5);
> >> +		else
> >> +			msleep(5);
> > 
> > If we must delay, then I suppose this is reasonable.
> 
> See above
> 
> >>  	}
> >>  
> >>  	return -ETIMEDOUT;
> >> @@ -595,7 +599,7 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
> >>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> >>  
> >>  	/* This will timeout if we don't have a link. */
> >> -	err = rcar_pcie_wait_for_dl(pcie);
> >> +	err = rcar_pcie_wait_for_dl(pcie, 0);
> >>  	if (err)
> >>  		return err;
> >>  
> >> @@ -1110,6 +1114,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
> >>  	pcie = pci_host_bridge_priv(bridge);
> >>  
> >>  	pcie->dev = dev;
> >> +	platform_set_drvdata(pdev, pcie);
> >>  
> >>  	INIT_LIST_HEAD(&pcie->resources);
> >>  
> >> @@ -1173,10 +1178,30 @@ static int rcar_pcie_probe(struct platform_device *pdev)
> >>  	return err;
> >>  }
> >>  
> >> +static int rcar_pcie_resume_noirq(struct device *dev)
> >> +{
> >> +	struct rcar_pcie *pcie = dev_get_drvdata(dev);
> >> +	u32 val = rcar_pci_read_reg(pcie, PMSR);
> >> +	int ret = 0;
> >> +
> >> +	if ((val == 0) || (rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) {
> > 
> > Please remove the unnecessary parentheses from the line above.
> > 
> > Also, I would prefer if the function returned early.
> > Something like (completely untested!):
> 
> This should work.
> 
> > 	if (rcar_pci_read_reg(pcie, PMSR) &&
> > 	    !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN))
> > 		return 0;
> > 
> > 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> > 	return rcar_pcie_wait_for_dl(pcie, 1);
> > 
> >> +		/* Re-establish the PCIe link */
> >> +		rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> >> +		ret = rcar_pcie_wait_for_dl(pcie, 1);
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static const struct dev_pm_ops rcar_pcie_pm_ops = {
> >> +	.resume_noirq = rcar_pcie_resume_noirq,
> >> +};
> >> +
> >>  static struct platform_driver rcar_pcie_driver = {
> >>  	.driver = {
> >>  		.name = "rcar-pcie",
> >>  		.of_match_table = rcar_pcie_of_match,
> >> +		.pm = &rcar_pcie_pm_ops,
> >>  		.suppress_bind_attrs = true,
> >>  	},
> >>  	.probe = rcar_pcie_probe,
> >> -- 
> >> 2.11.0
> >>
> 
> 
> -- 
> Best regards,
> Marek Vasut
> 



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux