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@xxxxxxxxxxx>
> > > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > > ---
> > > 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
