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@xxxxxxxxxxx>
> > > Signed-off-by: Krzysztof Wilczyński <kwilczynski@xxxxxxxxxx>
> > > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > > ---
> > > 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
--
மணிவண்ணன் சதாசிவம்
