Message-ID: <20210616013136.tsxf3tpekczivztb@toshiba.co.jp>
Date:   Wed, 16 Jun 2021 10:31:36 +0900
From:   Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        linux-pci@...r.kernel.org, punit1.agrawal@...hiba.co.jp,
        yuji2.ishikawa@...hiba.co.jp, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org,
        Kishon Vijay Abraham I <kishon@...com>
Subject: Re: [PATCH v3 2/3] PCI: Visconti: Add Toshiba Visconti PCIe host
 controller driver

Hi, 

Thanks for your reivew.


On Mon, May 24, 2021 at 01:58:39PM -0500, Bjorn Helgaas wrote:
> [+cc Kishon for cpu_addr_fixup() question]
> 
> Please make the subject "PCI: visconti: Add ..." since the driver
> names are usually lower-case.  When referring to the hardware itself,
> use "Visconti", of course.


OK, I will do so from next time.

> 
> On Mon, May 24, 2021 at 03:30:03PM +0900, Nobuhiro Iwamatsu wrote:
> > Add support to PCIe RC controller on Toshiba Visconti ARM SoCs. PCIe
> > controller is based of Synopsys DesignWare PCIe core.
> > 
> > This patch does not yet use the clock framework to control the clock.
> > This will be replaced in the future.
> > 
> > v2 -> v3:
> >   - Update subject.
> >   - Wrap description in 75 columns.
> >   - Change config name to PCIE_VISCONTI_HOST.
> >   - Update Kconfig text.
> >   - Drop blank lines.
> >   - Adjusted to 80 columns.
> >   - Drop inline from functions for register access.
> >   - Changed function name from visconti_pcie_check_link_status to
> >     visconti_pcie_link_up.
> >   - Update to using dw_pcie_host_init().
> >   - Reorder these in the order of use in visconti_pcie_establish_link.
> >   - Rewrite visconti_pcie_host_init() without dw_pcie_setup_rc().
> >   - Change function name from  visconti_device_turnon() to
> >     visconti_pcie_power_on().
> >   - Unify formats such as dev_err().
> >   - Drop error label in visconti_add_pcie_port().
> > 
> > v1 -> v2:
> >   - Fix typo in commit message.
> >   - Drop "depends on OF && HAS_IOMEM" from Kconfig.
> >   - Stop using the pointer of struct dw_pcie.
> >   - Use _relaxed variant.
> >   - Drop dw_pcie_wait_for_link.
> >   - Drop dbi resource processing.
> >   - Drop MSI IRQ initialization processing.
> 
> Thanks for the changelog.  Please move it after the "---" line for
> future versions.  That way it won't appear in the commit log when this
> is merged.  The notes about v1->v2, v2->v3, etc are useful during
> review, but not after this is merged.


Simliar to the above your comment, I will do so from next time.

> 
> > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@...hiba.co.jp>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>
> > ---
> >  drivers/pci/controller/dwc/Kconfig         |   9 +
> >  drivers/pci/controller/dwc/Makefile        |   1 +
> >  drivers/pci/controller/dwc/pcie-visconti.c | 369 +++++++++++++++++++++
> >  3 files changed, 379 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-visconti.c
> > 
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index 423d35872ce4..7c3dcb86fcad 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -286,6 +286,15 @@ config PCIE_TEGRA194_EP
> >  	  in order to enable device-specific features PCIE_TEGRA194_EP must be
> >  	  selected. This uses the DesignWare core.
> >  
> > +config PCIE_VISCONTI_HOST
> > +	bool "Toshiba Visconti PCIe controllers"
> > +	depends on ARCH_VISCONTI || COMPILE_TEST
> > +	depends on PCI_MSI_IRQ_DOMAIN
> > +	select PCIE_DW_HOST
> > +	help
> > +	  Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
> > +	  This driver supports TMPV7708 SoC.
> > +

<snip>

> > diff --git a/drivers/pci/controller/dwc/pcie-visconti.c b/drivers/pci/controller/dwc/pcie-visconti.c
> > new file mode 100644
> > index 000000000000..b764334f32e6
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-visconti.c
> > @@ -0,0 +1,369 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DWC PCIe RC driver for Toshiba Visconti ARM SoC
> > + *
> > + * Copyright (C) 2021 Toshiba Electronic Device & Storage Corporation
> > + * Copyright (C) 2021 TOSHIBA CORPORATION
> > + *
> > + * Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>
> > + */

<snip>

> > +static u64 visconti_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
> > +{
> > +	return pci_addr - PCIE_BUS_OFFSET;
> 
> This is called from __dw_pcie_prog_outbound_atu() as:
> 
>   cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> 
> so I think the parameter here should be *cpu_addr*, not pci_addr.

I see, I rename the variable to cpu_addr.

> 
> dra7xx and artpec6 also call it "pci_addr", which is at best
> confusing.
> 
> I'm also confused about exactly what .cpu_addr_fixup() does.  Is it
> applying an offset that cannot be deduced from the DT description?  If
> so, *should* this offset be described in DT?
>

This depends on Visconti5's PCIe hardware implementation.
It depends on the specification that the CPU bus connected to PCIe outputs
with 0x40000000 added as an offset. This is fixed.
I will add this description as comment.

> > +}
> > +
> > +static const struct dw_pcie_ops dw_pcie_ops = {
> > +	.cpu_addr_fixup = visconti_pcie_cpu_addr_fixup,
> > +	.link_up = visconti_pcie_link_up,
> > +	.start_link = visconti_pcie_start_link,
> > +	.stop_link = visconti_pcie_stop_link,
> > +};
> > +
> > +static int visconti_pcie_power_on(struct visconti_pcie *pcie)
> > +{
> > +	void __iomem *addr;
> > +	int err;
> > +	u32 val;
> > +

<snip>

> > +static int
> > +visconti_add_pcie_port(struct visconti_pcie *pcie, struct platform_device *pdev)
> > +{
> > +	struct dw_pcie *pci = &pcie->pci;
> > +	struct pcie_port *pp = &pci->pp;
> > +	struct device *dev = &pdev->dev;
> > +
> > +	pp->irq = platform_get_irq_byname(pdev, "intr");
> > +	if (pp->irq < 0) {
> > +		dev_err(dev, "Interrupt intr is missing");
> > +		return pp->irq;
> > +	}
> 
> Looks like most drivers use "pp->irq = platform_get_irq(pdev, 0);"
> Is there a reason for this to be different?

This driver has two interrupts, msi and usual interrupt.
MSI is handled by the dwc framework, but the rest is handled by the driver.
When When I get the IRQ using index, it may not be processed correctly
depending on how to write DT, so I use platform_get_irq_byname() to check by name.

> 
> > +	pp->ops = &visconti_pcie_host_ops;
> > +
> > +	pci->link_gen = of_pci_get_max_link_speed(pdev->dev.of_node);
> > +	if (pci->link_gen < 0 || pci->link_gen > 3) {
> > +		pci->link_gen = 3;
> > +		dev_dbg(dev, "Applied default link speed\n");
> > +	}
> > +
> > +	dev_dbg(dev, "Link speed Gen %d", pci->link_gen);
> > +
> > +	return dw_pcie_host_init(pp);
> > +}
> > +
> > +static int visconti_pcie_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct visconti_pcie *pcie;
> > +	struct pcie_port *pp;
> > +	struct dw_pcie *pci;
> > +	int ret;
> > +
> > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> > +	if (ret)
> > +		return ret;
> 
> Somewhat unusual in PCIe controller drivers.  Is there something
> unusual about the Visconti hardware?
> 
> Also looks a little suspicious since the sequence is:
> 
>   visconti_pcie_probe
>     dma_set_mask_and_coherent(DMA_BIT_MASK(36))
>     visconti_add_pcie_port
>       dw_pcie_host_init
>         if (pci_msi_enabled())
>           dma_set_mask(DMA_BIT_MASK(32))
> 
> so dw_pcie_host_init() will override part of what we're setting here.
> 

Indeed, this process is unnecessary. I will drop.

> > +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > +	if (!pcie)
> > +		return -ENOMEM;
> > +
> > +	pci = &pcie->pci;
> > +	pp = &pci->pp;
> > +	pp->num_vectors = MAX_MSI_IRQS;
> 
> Is this necessary?  I can't tell that this driver even implements MSI
> support.  It looks like tegra194 is the only other driver that sets
> num_vectors itself.

This device uses MSI. And I double-checked about interrupts. This device
does not need to do this, as it has a maximum of 32 interrupt vectors.
I will delete it. Thank you for pointing out.

> 
> > +	pci->dev = dev;
> > +	pci->ops = &dw_pcie_ops;
> > +
> > +	ret = visconti_get_resources(pdev, pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	platform_set_drvdata(pdev, pcie);
> > +
> > +	return visconti_add_pcie_port(pcie, pdev);
> > +}
> > +
> > +static const struct of_device_id visconti_pcie_match[] = {
> > +	{ .compatible = "toshiba,visconti-pcie" },
> > +	{},
> > +};
> > +
> > +static struct platform_driver visconti_pcie_driver = {
> > +	.probe = visconti_pcie_probe,
> > +	.driver = {
> > +		.name = "visconti-pcie",
> > +		.of_match_table = visconti_pcie_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +};
> > +
> > +builtin_platform_driver(visconti_pcie_driver);
> > -- 
> > 2.31.1
> > 
>

Best regards,
  Nobuhiro

Powered by blists - more mailing lists