| Message ID | 20230825093219.2685912-17-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand |
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. > 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. > + 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. > + 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. > +{ > + 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? > + } > + > + 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. > + */ > +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. > + } > + > + 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.
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. > +config PCIE_RCAR_GEN4 > + tristate "Renesas R-Car Gen4 PCIe Host controller" The config prompt that matches the other drivers would be: tristate "Renesas R-Car Gen4 PCIe controller (host mode)" Similarly for the endpoint driver.
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@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. > > 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
Hi Bjorn, > From: Bjorn Helgaas, Sent: Friday, September 15, 2023 1:59 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. > > > +config PCIE_RCAR_GEN4 > > + tristate "Renesas R-Car Gen4 PCIe Host controller" > > The config prompt that matches the other drivers would be: > > tristate "Renesas R-Car Gen4 PCIe controller (host mode)" > > Similarly for the endpoint driver. I got it. I'll fix them. Best regards, Yoshihiro Shimoda
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
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@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. 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
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index ab96da43e0c2..3884a67e4d56 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -415,4 +415,14 @@ config PCIE_VISCONTI_HOST Say Y here if you want PCIe controller support on Toshiba Visconti SoC. This driver supports TMPV7708 SoC. +config PCIE_RCAR_GEN4 + tristate "Renesas R-Car Gen4 PCIe Host controller" + depends on ARCH_RENESAS || COMPILE_TEST + depends on PCI_MSI + select PCIE_DW_HOST + help + Say Y here if you want PCIe host controller support on R-Car Gen4 SoCs. + To compile this driver as a module, choose M here: the module will be + called pcie-rcar-gen4-host.ko. This uses the DesignWare core. + endmenu diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index bf5c311875a1..ab2c6bc16216 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -26,6 +26,8 @@ obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o +pcie-rcar-gen4-host-objs := pcie-rcar-gen4.o pcie-rcar-gen4-host-drv.o +obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4-host.o # The following drivers are for devices that use the generic ACPI # pci_root.c driver but don't support standard ECAM config access. diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4-host-drv.c b/drivers/pci/controller/dwc/pcie-rcar-gen4-host-drv.c new file mode 100644 index 000000000000..bf62804a84dc --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4-host-drv.c @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * PCIe host controller driver for Renesas R-Car Gen4 Series SoCs + * Copyright (C) 2022-2023 Renesas Electronics Corporation + */ + +#include <linux/delay.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/pci.h> +#include <linux/platform_device.h> + +#include "pcie-rcar-gen4.h" +#include "pcie-designware.h" + +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 */ + + gpiod_set_value_cansleep(dw->pe_rst, 0); + + return 0; +} + +static void rcar_gen4_pcie_host_deinit(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); + + gpiod_set_value_cansleep(dw->pe_rst, 1); + rcar_gen4_pcie_common_deinit(rcar); +} + +static const struct dw_pcie_host_ops rcar_gen4_pcie_host_ops = { + .host_init = rcar_gen4_pcie_host_init, + .host_deinit = rcar_gen4_pcie_host_deinit, +}; + +static int rcar_gen4_add_dw_pcie_rp(struct rcar_gen4_pcie *rcar) +{ + struct dw_pcie_rp *pp = &rcar->dw.pp; + + pp->num_vectors = MAX_MSI_IRQS; + pp->ops = &rcar_gen4_pcie_host_ops; + rcar->mode = DW_PCIE_RC_TYPE; + + return dw_pcie_host_init(pp); +} + +static void rcar_gen4_remove_dw_pcie_rp(struct rcar_gen4_pcie *rcar) +{ + dw_pcie_host_deinit(&rcar->dw.pp); +} + +static int rcar_gen4_pcie_probe(struct platform_device *pdev) +{ + struct rcar_gen4_pcie *rcar; + int err; + + rcar = rcar_gen4_pcie_devm_alloc(pdev); + if (IS_ERR(rcar)) + return PTR_ERR(rcar); + + err = rcar_gen4_pcie_get_resources(rcar); + if (err) + return err; + + err = rcar_gen4_pcie_prepare(rcar); + if (err) + return err; + + err = rcar_gen4_add_dw_pcie_rp(rcar); + if (err) + goto err_unprepare; + + return 0; + +err_unprepare: + rcar_gen4_pcie_unprepare(rcar); + + return err; +} + +static void rcar_gen4_pcie_remove(struct platform_device *pdev) +{ + struct rcar_gen4_pcie *rcar = platform_get_drvdata(pdev); + + rcar_gen4_remove_dw_pcie_rp(rcar); + rcar_gen4_pcie_unprepare(rcar); +} + +static const struct of_device_id rcar_gen4_pcie_of_match[] = { + { .compatible = "renesas,rcar-gen4-pcie", }, + {}, +}; +MODULE_DEVICE_TABLE(of, rcar_gen4_pcie_of_match); + +static struct platform_driver rcar_gen4_pcie_driver = { + .driver = { + .name = "pcie-rcar-gen4", + .of_match_table = rcar_gen4_pcie_of_match, + .probe_type = PROBE_PREFER_ASYNCHRONOUS, + }, + .probe = rcar_gen4_pcie_probe, + .remove_new = rcar_gen4_pcie_remove, +}; +module_platform_driver(rcar_gen4_pcie_driver); + +MODULE_DESCRIPTION("Renesas R-Car Gen4 PCIe host controller driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c new file mode 100644 index 000000000000..6da4d816891b --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c @@ -0,0 +1,227 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * PCIe host/endpoint controller driver for Renesas R-Car Gen4 Series SoCs + * Copyright (C) 2022-2023 Renesas Electronics Corporation + */ + +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/of_device.h> +#include <linux/pci.h> +#include <linux/pm_runtime.h> +#include <linux/reset.h> + +#include "pcie-rcar-gen4.h" +#include "pcie-designware.h" + +/* Renesas-specific */ +#define PCIERSTCTRL1 0x0014 +#define APP_HOLD_PHY_RST BIT(16) +#define APP_LTSSM_ENABLE BIT(0) + +#define RCAR_NUM_SPEED_CHANGE_RETRIES 10 +#define RCAR_MAX_LINK_SPEED 4 + +static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar, + bool enable) +{ + u32 val; + + val = readl(rcar->base + PCIERSTCTRL1); + if (enable) { + val |= APP_LTSSM_ENABLE; + val &= ~APP_HOLD_PHY_RST; + } else { + /* + * Since the datasheet of R-Car doesn't mention how to assert + * the APP_HOLD_PHY_RST, don't assert it again. Otherwise, + * hang-up issue happened in the dw_edma_core_off() when + * the controller didn't detect a PCI device. + */ + val &= ~APP_LTSSM_ENABLE; + } + writel(val, rcar->base + PCIERSTCTRL1); +} + +static int rcar_gen4_pcie_link_up(struct dw_pcie *dw) +{ + struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw); + u32 val, mask; + + val = readl(rcar->base + PCIEINTSTS0); + mask = RDLH_LINK_UP | SMLH_LINK_UP; + + return (val & mask) == mask; +} + +/* + * 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) +{ + 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); + } + + return false; +} + +/* + * Enable LTSSM of this controller and manually initiate the speed change. + * Always return true. + */ +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 */ + } + + return 0; +} + +static void rcar_gen4_pcie_stop_link(struct dw_pcie *dw) +{ + struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw); + + rcar_gen4_pcie_ltssm_enable(rcar, false); +} + +int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar) +{ + struct dw_pcie *dw = &rcar->dw; + u32 val; + int ret; + + ret = clk_bulk_prepare_enable(DW_PCIE_NUM_CORE_CLKS, dw->core_clks); + if (ret) { + dev_err(dw->dev, "Failed to enable ref clocks\n"); + return ret; + } + + if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc)) + reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc); + + val = readl(rcar->base + PCIEMSR0); + if (rcar->mode == DW_PCIE_RC_TYPE) { + val |= DEVICE_TYPE_RC; + } else if (rcar->mode == DW_PCIE_EP_TYPE) { + val |= DEVICE_TYPE_EP; + } else { + ret = -EINVAL; + goto err_unprepare; + } + + if (dw->num_lanes < 4) + val |= BIFUR_MOD_SET_ON; + + writel(val, rcar->base + PCIEMSR0); + + ret = reset_control_deassert(dw->core_rsts[DW_PCIE_PWR_RST].rstc); + if (ret) + goto err_unprepare; + + return 0; + +err_unprepare: + clk_bulk_disable_unprepare(DW_PCIE_NUM_CORE_CLKS, dw->core_clks); + + return ret; +} + +void rcar_gen4_pcie_common_deinit(struct rcar_gen4_pcie *rcar) +{ + struct dw_pcie *dw = &rcar->dw; + + reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc); + clk_bulk_disable_unprepare(DW_PCIE_NUM_CORE_CLKS, dw->core_clks); +} + +int rcar_gen4_pcie_prepare(struct rcar_gen4_pcie *rcar) +{ + struct device *dev = rcar->dw.dev; + int err; + + pm_runtime_enable(dev); + err = pm_runtime_resume_and_get(dev); + if (err < 0) { + dev_err(dev, "Failed to resume/get Runtime PM\n"); + pm_runtime_disable(dev); + } + + return err; +} + +void rcar_gen4_pcie_unprepare(struct rcar_gen4_pcie *rcar) +{ + struct device *dev = rcar->dw.dev; + + pm_runtime_put(dev); + pm_runtime_disable(dev); +} + +int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar) +{ + /* Renesas-specific registers */ + rcar->base = devm_platform_ioremap_resource_byname(rcar->pdev, "app"); + + return PTR_ERR_OR_ZERO(rcar->base); +} + +static const struct dw_pcie_ops dw_pcie_ops = { + .start_link = rcar_gen4_pcie_start_link, + .stop_link = rcar_gen4_pcie_stop_link, + .link_up = rcar_gen4_pcie_link_up, +}; + +struct rcar_gen4_pcie *rcar_gen4_pcie_devm_alloc(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rcar_gen4_pcie *rcar; + + rcar = devm_kzalloc(dev, sizeof(*rcar), GFP_KERNEL); + if (!rcar) + return ERR_PTR(-ENOMEM); + + rcar->dw.ops = &dw_pcie_ops; + rcar->dw.dev = dev; + rcar->pdev = pdev; + dw_pcie_cap_set(&rcar->dw, EDMA_UNROLL); + dw_pcie_cap_set(&rcar->dw, REQ_RES); + platform_set_drvdata(pdev, rcar); + + return rcar; +} diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.h b/drivers/pci/controller/dwc/pcie-rcar-gen4.h new file mode 100644 index 000000000000..7cbb3767c2d4 --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * PCIe host/endpoint controller driver for Renesas R-Car Gen4 Series SoCs + * Copyright (C) 2022-2023 Renesas Electronics Corporation + */ + +#ifndef _PCIE_RCAR_GEN4_H_ +#define _PCIE_RCAR_GEN4_H_ + +#include <linux/io.h> +#include <linux/pci.h> + +#include "pcie-designware.h" + +/* Renesas-specific */ +#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) + +struct rcar_gen4_pcie { + struct dw_pcie dw; + void __iomem *base; + struct platform_device *pdev; + enum dw_pcie_device_mode mode; +}; +#define to_rcar_gen4_pcie(_dw) container_of(_dw, struct rcar_gen4_pcie, dw) + +int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar); +void rcar_gen4_pcie_common_deinit(struct rcar_gen4_pcie *rcar); +int rcar_gen4_pcie_prepare(struct rcar_gen4_pcie *rcar); +void rcar_gen4_pcie_unprepare(struct rcar_gen4_pcie *rcar); +int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar); +struct rcar_gen4_pcie *rcar_gen4_pcie_devm_alloc(struct platform_device *pdev); + +#endif /* _PCIE_RCAR_GEN4_H_ */