Hello Bjorn,
> From: Bjorn Helgaas, Sent: Saturday, September 16, 2023 5:39 AM
>
> 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.
Thank you for your suggestion! :)
> 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.
I got it. I realized that pcie-tegra194.c and pci-dra7xx.c support
both host and endpoint modes without "-host.c" and "-ep.c" files.
So, I'll merge all four files into one file.
> That makes the Makefile very simple and there's only one module name
> to worry about.
I got it.
> > > > + 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.
I got it.
> > > > +#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.
I got it.
Best regards,
Yoshihiro Shimoda
> Bjorn
