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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux