Re: [PATCH v20 16/19] PCI: rcar-gen4: Add R-Car Gen4 PCIe Host support

From: Bjorn Helgaas <helgaas@kernel.org>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: "jingoohan1@gmail.com" <jingoohan1@gmail.com>,
	"gustavo.pimentel@synopsys.com" <gustavo.pimentel@synopsys.com>,
	"lpieralisi@kernel.org" <lpieralisi@kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"kw@linux.com" <kw@linux.com>,
	"manivannan.sadhasivam@linaro.org"
	<manivannan.sadhasivam@linaro.org>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"kishon@kernel.org" <kishon@kernel.org>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"conor+dt@kernel.org" <conor+dt@kernel.org>,
	"marek.vasut+renesas@gmail.com" <marek.vasut+renesas@gmail.com>,
	"fancer.lancer@gmail.com" <fancer.lancer@gmail.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH v20 16/19] PCI: rcar-gen4: Add R-Car Gen4 PCIe Host support
Date: Fri, 15 Sep 2023 15:38:30 -0500	[thread overview]
Message-ID: <20230915203830.GA105215@bhelgaas> (raw)
In-Reply-To: <TYBPR01MB534112A636FAFB61A4038337D8F6A@TYBPR01MB5341.jpnprd01.prod.outlook.com>

On Fri, Sep 15, 2023 at 09:37:15AM +0000, Yoshihiro Shimoda wrote:
> > From: Bjorn Helgaas, Sent: Friday, September 15, 2023 1:35 AM
> > On Fri, Aug 25, 2023 at 06:32:16PM +0900, Yoshihiro Shimoda wrote:
> > > Add R-Car Gen4 PCIe Host support. This controller is based on
> > > Synopsys DesignWare PCIe, but this controller has vendor-specific
> > > registers so that requires initialization code like mode setting
> > > and retraining and so on.
> > >
> > > To reduce code delta, adds some helper functions which are used by
> > > both the host driver and the endpoint driver (which is added
> > > immediately afterwards) into a separate file.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig            |  10 +
> > >  drivers/pci/controller/dwc/Makefile           |   2 +
> > >  .../controller/dwc/pcie-rcar-gen4-host-drv.c  | 135 +++++++++++
> > >  drivers/pci/controller/dwc/pcie-rcar-gen4.c   | 227 ++++++++++++++++++
> > >  drivers/pci/controller/dwc/pcie-rcar-gen4.h   |  44 ++++
> > >  5 files changed, 418 insertions(+)
> > >  create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4-host-drv.c
> > 
> > Is "pcie-rcar-gen4-host-drv.c" following some pattern?  I don't see
> > "-drv" in any nearby filenames.  Typical names are "-host.c" for host
> > driver and "-ep.c" for endpoint driver.
> 
> This is not following some pattern on pcie subsystem. But, some
> other subsystems have "*drv.c" filenames. Manivannan suggested the
> filenames to rename the module filenames [1].
>
> < Now >
> The source code filenames : pcie-rcar-gen4-{host,ep}-drv.c
> The module filenames : pcie-rcar-gen4-{host,ep}.ko
> 
> < Previous >
> The source code filenames : pcie-rcar-gen4-{host,ep}.c
> The module filenames : pcie-rcar-gen4-{host,ep}-drv.ko
> 
> [1]
> https://lore.kernel.org/linux-pci/20230724122820.GM6291@thinkpad/
> 
> I don't have a strong opinion on which filenames are good.

I have an opinion :)  I think splitting this up into four files is way
more complicated than it needs to be.  This is all for driving a
single piece of hardware, and I don't think there's enough benefit to
split it into separate files.

Most drivers, even those that support both host and endpoint mode, are
in a single file, e.g., pcie-artpec6.c, pci-imx6.c, pcie-keembay.c,
pcie-tegra194.c, pci-dra7xx.c, pci-keystone.c.

That makes the Makefile very simple and there's only one module name
to worry about.

> > > +	msleep(100);	/* pe_rst requires 100msec delay */
> > 
> > Can we include a spec reference for this delay?  Ideally this would be
> > a #define and likely shared across drivers.
> 
> I think so. Some other PCIe drivers also call "msleep(100)".
> So, I'll add a #define into drivers/pci/pci.h.

Yes.  I'm trying to move away from "msleep(100)" for everybody so we
can make sure all the drivers include the appropriate delays and
they're all based on the same reasoning.

> > > +#define PCIEDMAINTSTSEN		0x0314
> > > +#define PCIEDMAINTSTSEN_INIT	GENMASK(15, 0)
> > 
> > These register offsets are hard to decode whenthey'reallruntogether.
> 
> Unfortunately, these registers' offset names are from the datasheet.
> Perhaps, adding full register names as comments like below are helpful:
> -----
> /* PCIe Interrupt Status 0 */
> +#define PCIEINTSTS0		0x0084
> 
> /* PCIe DMA Interrupt Status Enable */
> #define PCIEDMAINTSTSEN		0x0314
> #define PCIEDMAINTSTSEN_INIT	GENMASK(15, 0)

It's good to use names from the datasheet.  The comments should be
enough.

Bjorn