Hi Bjorn,
> 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.
> > create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4.h
>
> > +config PCIE_RCAR_GEN4
>
> If you look through drivers/pci/controller/dwc/Kconfig, it's typical
> to use a "_HOST" suffix on the symbol to enable host controller
> drivers. Similarly, "_EP" suffix for endpoint drivers.
I got it. I'll rename this to PCIE_RCAR_GEN4_HOST.
> > + tristate "Renesas R-Car Gen4 PCIe Host controller"
> > + depends on ARCH_RENESAS || COMPILE_TEST
> > + depends on PCI_MSI
> > + select PCIE_DW_HOST
>
> > +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);
> > +
> > + /* Enable MSI interrupt signal */
> > + val = readl(rcar->base + PCIEINTSTS0EN);
> > + val |= MSI_CTRL_INT;
> > + writel(val, rcar->base + PCIEINTSTS0EN);
> > +
> > + 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.
> > + gpiod_set_value_cansleep(dw->pe_rst, 0);
> > +
> > + return 0;
> > +}
>
> > + * Manually initiate the speed change. Return true if the change succeeded,
> > + * false if the change didn't finish within certain periods.
> > + */
> > +static bool rcar_gen4_pcie_speed_change(struct dw_pcie *dw)
>
> This looks like it should return int, e.g., 0 for success, negative
> for failure. Boolean functions ideally would not have side effects
> and the name would be a condition that can be true or false.
I got it. I'll fix it.
> > +{
> > + u32 val;
> > + int i;
> > +
> > + val = dw_pcie_readl_dbi(dw, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > + val &= ~PORT_LOGIC_SPEED_CHANGE;
> > + dw_pcie_writel_dbi(dw, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> > +
> > + val = dw_pcie_readl_dbi(dw, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > + val |= PORT_LOGIC_SPEED_CHANGE;
> > + dw_pcie_writel_dbi(dw, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> > +
> > + for (i = 0; i < RCAR_NUM_SPEED_CHANGE_RETRIES; i++) {
> > + val = dw_pcie_readl_dbi(dw, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > + if (!(val & PORT_LOGIC_SPEED_CHANGE))
> > + return true;
> > + usleep_range(10000, 11000);
>
> Where did these values (num retries and sleep duration) come from?
> Can we include a spec citation for them?
These values came from my investigation. I could not find any spec
from the databook.
> > + }
> > +
> > + return false;
> > +}
> > +
> > +/*
> > + * Enable LTSSM of this controller and manually initiate the speed change.
> > + * Always return true.
>
> This doesn't return "true". It returns *0*, which is a perfectly good
> "success" value, but it isn't "true", which would be a non-zero value.
Oops. I'll fix the comment.
> > + */
> > +static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
> > +{
> > + struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > + int i, changes;
> > +
> > + rcar_gen4_pcie_ltssm_enable(rcar, true);
> > +
> > + /*
> > + * Require direct speed change with retrying here if the link_gen is
> > + * PCIe Gen2 or higher.
> > + */
> > + changes = min_not_zero(dw->link_gen, RCAR_MAX_LINK_SPEED) - 1;
> > +
> > + /*
> > + * Since dw_pcie_setup_rc() sets it once, PCIe Gen2 will be trained.
> > + * So, this needs remaining times for up to PCIe Gen4 if RC mode.
> > + */
> > + if (changes && rcar->mode == DW_PCIE_RC_TYPE)
> > + changes--;
> > +
> > + for (i = 0; i < changes; i++) {
> > + if (!rcar_gen4_pcie_speed_change(dw))
> > + break; /* No error because possible disconnected here if EP mode */
>
> Rest of the file fits in 80 columns, it'd be nice if the comment did
> too.
I got it. I'll fix this somehow.
> > + }
> > +
> > + return 0;
> > +}
>
> > +#define PCIEMSR0 0x0000
> > +#define BIFUR_MOD_SET_ON BIT(0)
> > +#define DEVICE_TYPE_EP 0
> > +#define DEVICE_TYPE_RC BIT(4)
> > +
> > +#define PCIEINTSTS0 0x0084
> > +#define PCIEINTSTS0EN 0x0310
> > +#define MSI_CTRL_INT BIT(26)
> > +#define SMLH_LINK_UP BIT(7)
> > +#define RDLH_LINK_UP BIT(6)
> > +#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)
-----
Best regards,
Yoshihiro Shimoda
