On Fri, Nov 10, 2017 at 10:53:07PM +0100, Marek Vasut wrote:
> On 11/10/2017 10:09 AM, Simon Horman wrote:
> > On Wed, Nov 08, 2017 at 10:28:06AM +0100, Marek Vasut wrote:
> >> From: Kazufumi Ikeda <kaz-ikeda@xxxxxxxxxxxxx>
> >>
> >> This adds the suspend/resume supports for pcie-rcar. The resume handler
> >> reprogram the hardware based on the software state kept in specific
> >> device structures. Also it doesn't need to save any registers.
> >>
> >> 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
> >> ---
> >> drivers/pci/host/pcie-rcar.c | 86 +++++++++++++++++++++++++++++++++++++++-----
> >> 1 file changed, 78 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> >> index 2b28292de93a..7a9e30185c79 100644
> >> --- a/drivers/pci/host/pcie-rcar.c
> >> +++ b/drivers/pci/host/pcie-rcar.c
> >> @@ -471,6 +471,36 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie)
> >> (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5");
> >> }
> >>
> >> +static int rcar_pcie_hw_enable(struct rcar_pcie *pcie)
> >
> > This function always returns 0 and the value is not checked by the caller.
> > Can we change the return type to void?
>
> Yes, done
>
> >> +{
> >> + struct resource_entry *win;
> >> + LIST_HEAD(res);
> >> + int i = 0;
> >> +
> >> + /* Try setting 5 GT/s link speed */
> >
> > What if it fails?
>
> If it fails, we're back at 2.5 GT/s . The rcar_pcie_force_speedup()
> first checks if the PCIe IP can do 5 GT/s at all. Only if so, tries to
> initiate transition to 5 GT/s operation , checks whether that succeeded
> and if it failed, falls back to 2.5 GT/s .
Thanks, got it.
> >> + rcar_pcie_force_speedup(pcie);
> >> +
> >> + /* Setup PCI resources */
> >> + resource_list_for_each_entry(win, &pcie->resources) {
> >> + struct resource *res = win->res;
> >> +
> >> + if (!res->flags)
> >> + continue;
> >> +
> >> + switch (resource_type(res)) {
> >> + case IORESOURCE_IO:
> >> + case IORESOURCE_MEM:
> >> + rcar_pcie_setup_window(i, pcie, res);
> >> + i++;
> >> + break;
> >> + default:
> >> + continue;
> >
> > Can the default case be omitted?
>
> Sure
>
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int rcar_pcie_enable(struct rcar_pcie *pcie)
> >> {
> >> struct device *dev = pcie->dev;
> >> @@ -872,11 +902,25 @@ static const struct irq_domain_ops msi_domain_ops = {
> >> .map = rcar_msi_map,
> >> };
> >>
> >> +static void rcar_pcie_hw_enable_msi(struct rcar_pcie *pcie)
> >> +{
> >> + struct rcar_msi *msi = &pcie->msi;
> >> + unsigned long base;
> >> +
> >> + /* setup MSI data target */
> >> + base = virt_to_phys((void *)msi->pages);
> >
> > Why do you need to cast to void *?
> > I expect such casting can be done implicitly.
>
> Because __get_free_pages() returns unsigned long and that's what's used
> to assign msi->pages . And virt_to_phys() expects void * instead, thus
> the cast.
Right, but I don't think one should ever need to explicitly cast
to or from void *. What mean is, can you just remove "(void *)" without
changing any behaviour?
>
> >> +
> >> + rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR);
> >> + rcar_pci_write_reg(pcie, 0, PCIEMSIAUR);
> >> +
> >> + /* enable all MSI interrupts */
> >> + rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);
> >> +}
> >> +
> >> static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
> >> {
> >> struct device *dev = pcie->dev;
> >> struct rcar_msi *msi = &pcie->msi;
> >> - unsigned long base;
> >> int err, i;
> >>
> >> mutex_init(&msi->lock);
> >> @@ -915,13 +959,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
> >>
> >> /* setup MSI data target */
> >> msi->pages = __get_free_pages(GFP_KERNEL, 0);
> >> - base = virt_to_phys((void *)msi->pages);
> >> -
> >> - rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR);
> >> - rcar_pci_write_reg(pcie, 0, PCIEMSIAUR);
> >> -
> >> - /* enable all MSI interrupts */
> >> - rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);
> >> + rcar_pcie_hw_enable_msi(pcie);
> >>
> >> return 0;
> >>
> >> @@ -1202,6 +1240,37 @@ static int rcar_pcie_probe(struct platform_device *pdev)
> >> return err;
> >> }
> >>
> >> +static int rcar_pcie_resume(struct device *dev)
> >> +{
> >> + struct rcar_pcie *pcie = dev_get_drvdata(dev);
> >> + unsigned int data;
> >> + int err;
> >> + int (*hw_init_fn)(struct rcar_pcie *);
> >
> > Please sort local variables in reverse xmas tree order.
>
> OK
>
> >> +
> >> + err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node);
> >> + if (err)
> >> + return 0;
> >> +
> >> + /* Failure to get a link might just be that no cards are inserted */
> >> + hw_init_fn = of_device_get_match_data(dev);
> >> + err = hw_init_fn(pcie);
> >> + if (err) {
> >> + dev_info(dev, "PCIe link down\n");
> >> + return 0;
> >> + }
> >> +
> >> + data = rcar_pci_read_reg(pcie, MACSR);
> >> + dev_info(dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f);
> >> +
> >> + /* Enable MSI */
> >> + if (IS_ENABLED(CONFIG_PCI_MSI))
> >> + rcar_pcie_hw_enable_msi(pcie);
> >> +
> >> + rcar_pcie_hw_enable(pcie);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int rcar_pcie_resume_noirq(struct device *dev)
> >> {
> >> struct rcar_pcie *pcie = dev_get_drvdata(dev);
> >> @@ -1218,6 +1287,7 @@ static int rcar_pcie_resume_noirq(struct device *dev)
> >> }
> >>
> >> static const struct dev_pm_ops rcar_pcie_pm_ops = {
> >> + SET_SYSTEM_SLEEP_PM_OPS(NULL, rcar_pcie_resume)
> >> .resume_noirq = rcar_pcie_resume_noirq,
> >> };
> >>
> >> --
> >> 2.11.0
> >>
>
>
> --
> Best regards,
> Marek Vasut
>
