Re: [PATCH v21 13/16] PCI: dwc: rcar-gen4: Add R-Car Gen4 PCIe controller support

[Manivannan Sadhasivam <mani@kernel.org>]

On Wed, Oct 11, 2023 at 01:18:11AM +0000, Yoshihiro Shimoda wrote:
> Hello Manivannan,
> 
> > From: Manivannan Sadhasivam, Sent: Tuesday, October 10, 2023 9:04 PM
> > 
> > On Fri, Sep 22, 2023 at 03:53:28PM +0900, Yoshihiro Shimoda wrote:
> > > Add R-Car Gen4 PCIe controller support for host mode.
> > >
> > > This controller is based on Synopsys DesignWare PCIe. However, this
> > > particular controller has a number of vendor-specific registers, and as
> > > such, requires initialization code like mode setting and retraining and
> > > so on.
> > >
> > > [kwilczynski: commit log]
> > > Link:
> <snip URL>
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
> > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig          |  14 +
> > >  drivers/pci/controller/dwc/Makefile         |   1 +
> > >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 380 ++++++++++++++++++++
> > >  3 files changed, 395 insertions(+)
> > >  create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > >
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index ab96da43e0c2..bc69fcab2e2a 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -415,4 +415,18 @@ config PCIE_VISCONTI_HOST
> > >  	  Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
> > >  	  This driver supports TMPV7708 SoC.
> > >
> > > +config PCIE_RCAR_GEN4
> > > +	tristate
> > > +
> > > +config PCIE_RCAR_GEN4_HOST
> > > +	tristate "Renesas R-Car Gen4 PCIe controller (host mode)"
> > > +	depends on ARCH_RENESAS || COMPILE_TEST
> > > +	depends on PCI_MSI
> > > +	select PCIE_DW_HOST
> > > +	select PCIE_RCAR_GEN4
> > > +	help
> > > +	  Say Y here if you want PCIe controller (host mode) on R-Car Gen4 SoCs.
> > > +	  To compile this driver as a module, choose M here: the module will be
> > > +	  called pcie-rcar-gen4.ko. This uses the DesignWare core.
> > > +
> > >  endmenu
> > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > index bf5c311875a1..bac103faa523 100644
> > > --- a/drivers/pci/controller/dwc/Makefile
> > > +++ b/drivers/pci/controller/dwc/Makefile
> > > @@ -26,6 +26,7 @@ obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
> > >  obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> > >  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> > >  obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
> > > +obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
> > >
> > >  # The following drivers are for devices that use the generic ACPI
> > >  # pci_root.c driver but don't support standard ECAM config access.
> > > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > new file mode 100644
> > > index 000000000000..dfff6bb18932
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c

[...]

> > > +/* Host mode */
> > > +static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
> > > +{
> > > +	struct dw_pcie *dw = to_dw_pcie_from_pp(pp);
> > > +	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > > +	int ret;
> > > +	u32 val;
> > > +
> > > +	gpiod_set_value_cansleep(dw->pe_rst, 1);
> > > +
> > > +	ret = rcar_gen4_pcie_common_init(rcar);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > > +	 * Rev.5.20a, we should disable two BARs to avoid unnecessary memory
> > > +	 * assignment during device enumeration.
> > > +	 */
> > > +	dw_pcie_writel_dbi2(dw, PCI_BASE_ADDRESS_0, 0x0);
> > > +	dw_pcie_writel_dbi2(dw, PCI_BASE_ADDRESS_1, 0x0);
> > 
> > If this is DWC specific, can we move it to the common code?
> 
> Hmm, it seems so. However, I didn't find any similar code on other DWC drivers.
> So, for now, moving it to the common code is not needed, I think.
> 

No. If this is as per the DWC spec, then it should be part of the common code.

> > > +
> > > +	/* Enable MSI interrupt signal */
> > > +	val = readl(rcar->base + PCIEINTSTS0EN);
> > > +	val |= MSI_CTRL_INT;
> > > +	writel(val, rcar->base + PCIEINTSTS0EN);
> > > +
> > 
> > Above should be guarded with:
> > 
> > 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > 	...
> > 	}
> 
> Since this driver depends on PCI_MSI by Kconfig, such a IS_ENABLED is not
> needed. This is from your suggestion ;) [2]
> 
> [2] https://lore.kernel.org/linux-devicetree/20230724122820.GM6291@thinkpad/
> 

heh... thanks for reminding me ;)

- Mani

-- 
மணிவண்ணன் சதாசிவம்