| Message ID | 20220613115712.2831386-5-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | treewide: PCI: renesas: Add R-Car Gen4 PCIe support | expand |
On Mon, Jun 13, 2022 at 08:57:09PM +0900, Yoshihiro Shimoda wrote: > Add R-Car Gen4 PCIe Endpoint support. This controller is based on > Synopsys Designware PCIe. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/pci/controller/dwc/Kconfig | 9 + > drivers/pci/controller/dwc/Makefile | 1 + > .../pci/controller/dwc/pcie-rcar-gen4-ep.c | 253 ++++++++++++++++++ > drivers/pci/controller/dwc/pcie-rcar-gen4.h | 1 + > 4 files changed, 264 insertions(+) > create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index 3ddccc9c38c5..503ead1a4358 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -393,4 +393,13 @@ config PCIE_RCAR_GEN4 > Say Y here if you want PCIe host controller support on R-Car Gen4 SoCs. > This uses the DesignWare core. > > +config PCIE_RCAR_GEN4_EP > + bool "Renesas R-Car Gen4 PCIe Endpoint controller" > + depends on ARCH_RENESAS || COMPILE_TEST > + depends on PCI_ENDPOINT > + select PCIE_DW_EP > + help > + Say Y here if you want PCIe endpoint controller support on R-Car Gen4 > + SoCs. This uses the DesignWare core. > + > endmenu > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > index b3f285e685f9..3d40346efd27 100644 > --- a/drivers/pci/controller/dwc/Makefile > +++ b/drivers/pci/controller/dwc/Makefile > @@ -26,6 +26,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o > obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o > obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o > obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o pcie-rcar-gen4-host.o > +obj-$(CONFIG_PCIE_RCAR_GEN4_EP) += pcie-rcar-gen4.o pcie-rcar-gen4-ep.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-ep.c b/drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c > new file mode 100644 > index 000000000000..622e32c7a410 > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c > @@ -0,0 +1,253 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PCIe Endpoint driver for Renesas R-Car Gen4 Series SoCs > + * Copyright (C) 2022 Renesas Electronics Corporation > + */ > + > +#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" > + > +/* Configuration */ > +#define PCICONF3 0x000c > +#define MULTI_FUNC BIT(23) > + > +struct rcar_gen4_pcie_ep { > + struct rcar_gen4_pcie *pcie; > + struct dw_pcie *pci; Would be better if these are embedded structs rather than pointers. Then it is 1 alloc. Also, 'pci' and 'pcie' aren't very clear. rcar_pcie and pcie perhaps. Or rcar and dw. > + u32 num_lanes; What's wrong with dw_pcie.num_lanes? > +}; > + > +static void rcar_gen4_pcie_ep_init(struct dw_pcie_ep *ep) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + enum pci_barno bar; > + > + for (bar = BAR_0; bar <= BAR_5; bar++) > + dw_pcie_ep_reset_bar(pci, bar); Seems like the core code should be doing this. > +} > + > +static int rcar_gen4_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no, > + enum pci_epc_irq_type type, > + u16 interrupt_num) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + > + switch (type) { > + case PCI_EPC_IRQ_LEGACY: > + return dw_pcie_ep_raise_legacy_irq(ep, func_no); > + case PCI_EPC_IRQ_MSI: > + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > + case PCI_EPC_IRQ_MSIX: > + return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); > + default: > + dev_err(pci->dev, "UNKNOWN IRQ type\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static const struct pci_epc_features rcar_gen4_pcie_epc_features = { > + .linkup_notifier = false, > + .msi_capable = true, > + .msix_capable = false, If this is false, why do you call dw_pcie_ep_raise_msix_irq? > + .align = SZ_1M, > +}; > + > +static const struct pci_epc_features* > +rcar_gen4_pcie_ep_get_features(struct dw_pcie_ep *ep) > +{ > + return &rcar_gen4_pcie_epc_features; > +} > + > +static const struct dw_pcie_ep_ops pcie_ep_ops = { > + .ep_init = rcar_gen4_pcie_ep_init, > + .raise_irq = rcar_gen4_pcie_ep_raise_irq, > + .get_features = rcar_gen4_pcie_ep_get_features, > +}; > + > +static int rcar_gen4_add_pcie_ep(struct rcar_gen4_pcie_ep *pcie_ep, > + struct platform_device *pdev) > +{ > + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; > + struct dw_pcie *pci = pcie->pci; > + struct dw_pcie_ep *ep; > + struct resource *res; > + int ret; > + > + ep = &pci->ep; > + ep->ops = &pcie_ep_ops; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > + if (!res) > + return -EINVAL; Common code handles this. > + > + ep->addr_size = resource_size(res); > + > + ret = dw_pcie_ep_init(ep); > + if (ret) { > + dev_err(&pdev->dev, "failed to initialize endpoint\n"); > + return ret; > + } > + > + pci->ops->start_link(pci); > + > + return 0; > +} > + > +static void rcar_gen4_remove_pcie_ep(struct rcar_gen4_pcie_ep *pcie_ep) > +{ > + dw_pcie_ep_exit(&pcie_ep->pcie->pci->ep); > +} > + > +static void rcar_gen4_pcie_init_ep(struct rcar_gen4_pcie_ep *pcie_ep) > +{ > + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; > + struct dw_pcie *pci = pcie->pci; > + int val; > + > + /* Device type selection - Endpoint */ > + val = rcar_gen4_pcie_readl(pcie, PCIEMSR0); > + val |= DEVICE_TYPE_EP; > + if (pcie_ep->num_lanes < 4) > + val |= BIFUR_MOD_SET_ON; > + rcar_gen4_pcie_writel(pcie, PCIEMSR0, val); > + > + dw_pcie_dbi_ro_wr_en(pci); > + > + /* Single function */ > + val = dw_pcie_readl_dbi(pci, PCICONF3); > + val &= ~MULTI_FUNC; > + dw_pcie_writel_dbi(pci, PCICONF3, val); Common DWC reg/bit? If so, belongs in common header. > + > + /* Disable unused BARs */ > + dw_pcie_writel_dbi(pci, SHADOW_REG(BAR2MASKF), 0x0); > + dw_pcie_writel_dbi(pci, SHADOW_REG(BAR3MASKF), 0x0); Seems like something the common code should do. > + > + /* Set Max Link Width */ > + rcar_gen4_pcie_set_max_link_width(pci, pcie_ep->num_lanes); > + > + dw_pcie_dbi_ro_wr_dis(pci); > +} > + > +static int rcar_gen4_pcie_ep_get_resources(struct rcar_gen4_pcie_ep *pcie_ep, > + struct platform_device *pdev) > +{ > + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; > + struct dw_pcie *pci = pcie->pci; > + struct device *dev = pci->dev; > + struct device_node *np = dev->of_node; > + struct resource *res; > + int err; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); > + pci->dbi_base = devm_pci_remap_cfg_resource(dev, res); > + if (IS_ERR(pci->dbi_base)) > + return PTR_ERR(pci->dbi_base); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu"); > + pci->atu_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(pci->atu_base)) > + return PTR_ERR(pci->atu_base); The common code handles these resources. > + > + /* Renesas-specific registers */ > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "appl"); > + pcie->base = devm_ioremap_resource(dev, res); > + if (IS_ERR(pcie->base)) > + return PTR_ERR(pcie->base); > + > + err = of_property_read_u32(np, "num-lanes", &pcie_ep->num_lanes); Common code does this too. Lots of duplication! Please check. If it's something every DW controller has or might have, then the code for it belongs in the common code. > + if (err < 0) { > + dev_err(dev, "num-lanes not found %d\n", err); > + return err; > + } > + > + return rcar_gen4_pcie_devm_clk_and_reset_get(pcie, dev); > +} > + > +static int rcar_gen4_pcie_ep_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rcar_gen4_pcie_ep *pcie_ep; > + struct rcar_gen4_pcie *pcie; > + int err; > + > + pcie_ep = devm_kzalloc(dev, sizeof(*pcie_ep), GFP_KERNEL); > + if (!pcie_ep) > + return -ENOMEM; > + > + pcie = rcar_gen4_pcie_devm_alloc(dev); > + if (!pcie) > + return -ENOMEM; > + pcie_ep->pcie = pcie; > + > + err = rcar_gen4_pcie_pm_runtime_enable(dev); > + if (err < 0) { > + dev_err(dev, "pm_runtime_get_sync failed\n"); > + return err; > + } > + > + err = rcar_gen4_pcie_ep_get_resources(pcie_ep, pdev); > + if (err < 0) { > + dev_err(dev, "failed to request resource: %d\n", err); > + goto err_pm_put; > + } > + > + pcie->priv = pcie_ep; > + platform_set_drvdata(pdev, pcie); > + > + err = rcar_gen4_pcie_prepare(pcie); > + if (err < 0) > + goto err_pm_put; > + rcar_gen4_pcie_init_ep(pcie_ep); > + > + err = rcar_gen4_add_pcie_ep(pcie_ep, pdev); > + if (err < 0) > + goto err_ep_disable; > + > + return 0; > + > +err_ep_disable: > + rcar_gen4_pcie_unprepare(pcie); > + > +err_pm_put: > + rcar_gen4_pcie_pm_runtime_disable(dev); > + > + return err; > +} > + > +static int rcar_gen4_pcie_ep_remove(struct platform_device *pdev) > +{ > + struct rcar_gen4_pcie *pcie = platform_get_drvdata(pdev); > + struct rcar_gen4_pcie_ep *pcie_ep = pcie->priv; > + > + rcar_gen4_remove_pcie_ep(pcie_ep); > + rcar_gen4_pcie_unprepare(pcie_ep->pcie); > + rcar_gen4_pcie_pm_runtime_disable(&pdev->dev); > + > + return 0; > +} > + > +static const struct of_device_id rcar_gen4_pcie_of_match[] = { > + { .compatible = "renesas,rcar-gen4-pcie-ep", }, > + {}, > +}; > + > +static struct platform_driver rcar_gen4_pcie_ep_driver = { > + .driver = { > + .name = "pcie-rcar-gen4-ep", > + .of_match_table = rcar_gen4_pcie_of_match, > + }, > + .probe = rcar_gen4_pcie_ep_probe, > + .remove = rcar_gen4_pcie_ep_remove, > +}; > +builtin_platform_driver(rcar_gen4_pcie_ep_driver); Not a module or... > + > +MODULE_DESCRIPTION("Renesas R-Car Gen4 PCIe endpoint controller driver"); > +MODULE_LICENSE("GPL v2"); A module? Should be a module if possible. > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.h b/drivers/pci/controller/dwc/pcie-rcar-gen4.h > index bd01d0ffcac9..b6e285d8ebc0 100644 > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.h > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.h > @@ -43,6 +43,7 @@ struct rcar_gen4_pcie { > void __iomem *base; > struct clk *bus_clk; > struct reset_control *rst; > + void *priv; > }; > > extern u32 rcar_gen4_pcie_readl(struct rcar_gen4_pcie *pcie, u32 reg); > -- > 2.25.1 >
Hi Rob, Thank you for your review! > From: Rob Herring, Sent: Tuesday, June 14, 2022 6:51 AM > > On Mon, Jun 13, 2022 at 08:57:09PM +0900, Yoshihiro Shimoda wrote: > > Add R-Car Gen4 PCIe Endpoint support. This controller is based on > > Synopsys Designware PCIe. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/pci/controller/dwc/Kconfig | 9 + > > drivers/pci/controller/dwc/Makefile | 1 + > > .../pci/controller/dwc/pcie-rcar-gen4-ep.c | 253 ++++++++++++++++++ > > drivers/pci/controller/dwc/pcie-rcar-gen4.h | 1 + > > 4 files changed, 264 insertions(+) > > create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c > > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > > index 3ddccc9c38c5..503ead1a4358 100644 > > --- a/drivers/pci/controller/dwc/Kconfig > > +++ b/drivers/pci/controller/dwc/Kconfig > > @@ -393,4 +393,13 @@ config PCIE_RCAR_GEN4 > > Say Y here if you want PCIe host controller support on R-Car Gen4 SoCs. > > This uses the DesignWare core. > > > > +config PCIE_RCAR_GEN4_EP > > + bool "Renesas R-Car Gen4 PCIe Endpoint controller" > > + depends on ARCH_RENESAS || COMPILE_TEST > > + depends on PCI_ENDPOINT > > + select PCIE_DW_EP > > + help > > + Say Y here if you want PCIe endpoint controller support on R-Car Gen4 > > + SoCs. This uses the DesignWare core. > > + > > endmenu > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > > index b3f285e685f9..3d40346efd27 100644 > > --- a/drivers/pci/controller/dwc/Makefile > > +++ b/drivers/pci/controller/dwc/Makefile > > @@ -26,6 +26,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o > > obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o > > obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o > > obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o pcie-rcar-gen4-host.o > > +obj-$(CONFIG_PCIE_RCAR_GEN4_EP) += pcie-rcar-gen4.o pcie-rcar-gen4-ep.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-ep.c b/drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c > > new file mode 100644 > > index 000000000000..622e32c7a410 > > --- /dev/null > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c > > @@ -0,0 +1,253 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * PCIe Endpoint driver for Renesas R-Car Gen4 Series SoCs > > + * Copyright (C) 2022 Renesas Electronics Corporation > > + */ > > + > > +#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" > > + > > +/* Configuration */ > > +#define PCICONF3 0x000c > > +#define MULTI_FUNC BIT(23) > > + > > +struct rcar_gen4_pcie_ep { > > + struct rcar_gen4_pcie *pcie; > > + struct dw_pcie *pci; > > Would be better if these are embedded structs rather than pointers. Then > it is 1 alloc. Also, 'pci' and 'pcie' aren't very clear. rcar_pcie and > pcie perhaps. Or rcar and dw. I got it. I'll fix them. > > + u32 num_lanes; > > What's wrong with dw_pcie.num_lanes? The dw_pcie.num_lanes is set after dw_pcie_ep_init() succeeded. However, this driver would like to refer the num_lanes before dw_pcie_ep_init() to initialize vendor-specific registers. In other words, this value is only needed at that timing. So, we can remove this from struct rcar_gen4_pcie_ep, and just get the num_lanes as a local variable. > > +}; > > + > > +static void rcar_gen4_pcie_ep_init(struct dw_pcie_ep *ep) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + enum pci_barno bar; > > + > > + for (bar = BAR_0; bar <= BAR_5; bar++) > > + dw_pcie_ep_reset_bar(pci, bar); > > Seems like the core code should be doing this. I think so. I'll add such a function into the core core. > > +} > > + > > +static int rcar_gen4_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no, > > + enum pci_epc_irq_type type, > > + u16 interrupt_num) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + > > + switch (type) { > > + case PCI_EPC_IRQ_LEGACY: > > + return dw_pcie_ep_raise_legacy_irq(ep, func_no); > > + case PCI_EPC_IRQ_MSI: > > + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > > + case PCI_EPC_IRQ_MSIX: > > + return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); > > + default: > > + dev_err(pci->dev, "UNKNOWN IRQ type\n"); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static const struct pci_epc_features rcar_gen4_pcie_epc_features = { > > + .linkup_notifier = false, > > + .msi_capable = true, > > + .msix_capable = false, > > If this is false, why do you call dw_pcie_ep_raise_msix_irq? Oops. I'll remove dw_pcie_ep_raise_msix_irq() calling. > > + .align = SZ_1M, > > +}; > > + > > +static const struct pci_epc_features* > > +rcar_gen4_pcie_ep_get_features(struct dw_pcie_ep *ep) > > +{ > > + return &rcar_gen4_pcie_epc_features; > > +} > > + > > +static const struct dw_pcie_ep_ops pcie_ep_ops = { > > + .ep_init = rcar_gen4_pcie_ep_init, > > + .raise_irq = rcar_gen4_pcie_ep_raise_irq, > > + .get_features = rcar_gen4_pcie_ep_get_features, > > +}; > > + > > +static int rcar_gen4_add_pcie_ep(struct rcar_gen4_pcie_ep *pcie_ep, > > + struct platform_device *pdev) > > +{ > > + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; > > + struct dw_pcie *pci = pcie->pci; > > + struct dw_pcie_ep *ep; > > + struct resource *res; > > + int ret; > > + > > + ep = &pci->ep; > > + ep->ops = &pcie_ep_ops; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > + if (!res) > > + return -EINVAL; > > Common code handles this. Yes. I think this driver should use common code somehow. > > + > > + ep->addr_size = resource_size(res); > > + > > + ret = dw_pcie_ep_init(ep); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to initialize endpoint\n"); > > + return ret; > > + } > > + > > + pci->ops->start_link(pci); > > + > > + return 0; > > +} > > + > > +static void rcar_gen4_remove_pcie_ep(struct rcar_gen4_pcie_ep *pcie_ep) > > +{ > > + dw_pcie_ep_exit(&pcie_ep->pcie->pci->ep); > > +} > > + > > +static void rcar_gen4_pcie_init_ep(struct rcar_gen4_pcie_ep *pcie_ep) > > +{ > > + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; > > + struct dw_pcie *pci = pcie->pci; > > + int val; > > + > > + /* Device type selection - Endpoint */ > > + val = rcar_gen4_pcie_readl(pcie, PCIEMSR0); > > + val |= DEVICE_TYPE_EP; > > + if (pcie_ep->num_lanes < 4) > > + val |= BIFUR_MOD_SET_ON; > > + rcar_gen4_pcie_writel(pcie, PCIEMSR0, val); > > + > > + dw_pcie_dbi_ro_wr_en(pci); > > + > > + /* Single function */ > > + val = dw_pcie_readl_dbi(pci, PCICONF3); > > + val &= ~MULTI_FUNC; > > + dw_pcie_writel_dbi(pci, PCICONF3, val); > > Common DWC reg/bit? If so, belongs in common header. This seems PCIe specific register/bit. However, according to the datasheet of this controller, the register/bit is a read-only register. So, I'll drop this if it's true. > > + > > + /* Disable unused BARs */ > > + dw_pcie_writel_dbi(pci, SHADOW_REG(BAR2MASKF), 0x0); > > + dw_pcie_writel_dbi(pci, SHADOW_REG(BAR3MASKF), 0x0); > > Seems like something the common code should do. I got it. > > + > > + /* Set Max Link Width */ > > + rcar_gen4_pcie_set_max_link_width(pci, pcie_ep->num_lanes); > > + > > + dw_pcie_dbi_ro_wr_dis(pci); > > +} > > + > > +static int rcar_gen4_pcie_ep_get_resources(struct rcar_gen4_pcie_ep *pcie_ep, > > + struct platform_device *pdev) > > +{ > > + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; > > + struct dw_pcie *pci = pcie->pci; > > + struct device *dev = pci->dev; > > + struct device_node *np = dev->of_node; > > + struct resource *res; > > + int err; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); > > + pci->dbi_base = devm_pci_remap_cfg_resource(dev, res); > > + if (IS_ERR(pci->dbi_base)) > > + return PTR_ERR(pci->dbi_base); > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu"); > > + pci->atu_base = devm_ioremap_resource(dev, res); > > + if (IS_ERR(pci->atu_base)) > > + return PTR_ERR(pci->atu_base); > > The common code handles these resources. You're correct. I'll use common code somehow. > > + > > + /* Renesas-specific registers */ > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "appl"); > > + pcie->base = devm_ioremap_resource(dev, res); > > + if (IS_ERR(pcie->base)) > > + return PTR_ERR(pcie->base); > > + > > + err = of_property_read_u32(np, "num-lanes", &pcie_ep->num_lanes); > > Common code does this too. Lots of duplication! Please check. If it's > something every DW controller has or might have, then the code for it > belongs in the common code. My headache point is common code will handle both 1) getting resources and 2) DWC common part registers initialization, but this controller requires vendor specification at first. So, I'll modify the common code somehow. > > + if (err < 0) { > > + dev_err(dev, "num-lanes not found %d\n", err); > > + return err; > > + } > > + > > + return rcar_gen4_pcie_devm_clk_and_reset_get(pcie, dev); > > +} > > + > > +static int rcar_gen4_pcie_ep_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct rcar_gen4_pcie_ep *pcie_ep; > > + struct rcar_gen4_pcie *pcie; > > + int err; > > + > > + pcie_ep = devm_kzalloc(dev, sizeof(*pcie_ep), GFP_KERNEL); > > + if (!pcie_ep) > > + return -ENOMEM; > > + > > + pcie = rcar_gen4_pcie_devm_alloc(dev); > > + if (!pcie) > > + return -ENOMEM; > > + pcie_ep->pcie = pcie; > > + > > + err = rcar_gen4_pcie_pm_runtime_enable(dev); > > + if (err < 0) { > > + dev_err(dev, "pm_runtime_get_sync failed\n"); > > + return err; > > + } > > + > > + err = rcar_gen4_pcie_ep_get_resources(pcie_ep, pdev); > > + if (err < 0) { > > + dev_err(dev, "failed to request resource: %d\n", err); > > + goto err_pm_put; > > + } > > + > > + pcie->priv = pcie_ep; > > + platform_set_drvdata(pdev, pcie); > > + > > + err = rcar_gen4_pcie_prepare(pcie); > > + if (err < 0) > > + goto err_pm_put; > > + rcar_gen4_pcie_init_ep(pcie_ep); > > + > > + err = rcar_gen4_add_pcie_ep(pcie_ep, pdev); > > + if (err < 0) > > + goto err_ep_disable; > > + > > + return 0; > > + > > +err_ep_disable: > > + rcar_gen4_pcie_unprepare(pcie); > > + > > +err_pm_put: > > + rcar_gen4_pcie_pm_runtime_disable(dev); > > + > > + return err; > > +} > > + > > +static int rcar_gen4_pcie_ep_remove(struct platform_device *pdev) > > +{ > > + struct rcar_gen4_pcie *pcie = platform_get_drvdata(pdev); > > + struct rcar_gen4_pcie_ep *pcie_ep = pcie->priv; > > + > > + rcar_gen4_remove_pcie_ep(pcie_ep); > > + rcar_gen4_pcie_unprepare(pcie_ep->pcie); > > + rcar_gen4_pcie_pm_runtime_disable(&pdev->dev); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id rcar_gen4_pcie_of_match[] = { > > + { .compatible = "renesas,rcar-gen4-pcie-ep", }, > > + {}, > > +}; > > + > > +static struct platform_driver rcar_gen4_pcie_ep_driver = { > > + .driver = { > > + .name = "pcie-rcar-gen4-ep", > > + .of_match_table = rcar_gen4_pcie_of_match, > > + }, > > + .probe = rcar_gen4_pcie_ep_probe, > > + .remove = rcar_gen4_pcie_ep_remove, > > +}; > > +builtin_platform_driver(rcar_gen4_pcie_ep_driver); > > Not a module or... > > > + > > +MODULE_DESCRIPTION("Renesas R-Car Gen4 PCIe endpoint controller driver"); > > +MODULE_LICENSE("GPL v2"); > > A module? Should be a module if possible. Oops. I'll drop these MODULE_*. Best regards, Yoshihiro Shimoda > > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.h b/drivers/pci/controller/dwc/pcie-rcar-gen4.h > > index bd01d0ffcac9..b6e285d8ebc0 100644 > > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.h > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.h > > @@ -43,6 +43,7 @@ struct rcar_gen4_pcie { > > void __iomem *base; > > struct clk *bus_clk; > > struct reset_control *rst; > > + void *priv; > > }; > > > > extern u32 rcar_gen4_pcie_readl(struct rcar_gen4_pcie *pcie, u32 reg); > > -- > > 2.25.1 > >
On Tue, Jun 14, 2022 at 11:58:46AM +0000, Yoshihiro Shimoda wrote: > Hi Rob, > > Thank you for your review! > > > From: Rob Herring, Sent: Tuesday, June 14, 2022 6:51 AM > > > > On Mon, Jun 13, 2022 at 08:57:09PM +0900, Yoshihiro Shimoda wrote: > > > Add R-Car Gen4 PCIe Endpoint support. This controller is based on > > > Synopsys Designware PCIe. > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > --- [...] > > > + u32 num_lanes; > > > > What's wrong with dw_pcie.num_lanes? > > The dw_pcie.num_lanes is set after dw_pcie_ep_init() succeeded. > However, this driver would like to refer the num_lanes before dw_pcie_ep_init() > to initialize vendor-specific registers. In other words, this value is only > needed at that timing. So, we can remove this from struct rcar_gen4_pcie_ep, > and just get the num_lanes as a local variable. AFAICT, you are just using it to set PCI_EXP_LNKCAP_MLW. That's a common register, so it should be able to set in the common code. [...] > > > +static struct platform_driver rcar_gen4_pcie_ep_driver = { > > > + .driver = { > > > + .name = "pcie-rcar-gen4-ep", > > > + .of_match_table = rcar_gen4_pcie_of_match, > > > + }, > > > + .probe = rcar_gen4_pcie_ep_probe, > > > + .remove = rcar_gen4_pcie_ep_remove, > > > +}; > > > +builtin_platform_driver(rcar_gen4_pcie_ep_driver); > > > > Not a module or... > > > > > + > > > +MODULE_DESCRIPTION("Renesas R-Car Gen4 PCIe endpoint controller driver"); > > > +MODULE_LICENSE("GPL v2"); > > > > A module? Should be a module if possible. > > Oops. I'll drop these MODULE_*. No, is there some reason it can't be a module? Rob
Hi Rob, > From: Rob Herring, Sent: Wednesday, June 15, 2022 4:42 AM > > On Tue, Jun 14, 2022 at 11:58:46AM +0000, Yoshihiro Shimoda wrote: > > Hi Rob, > > > > Thank you for your review! > > > > > From: Rob Herring, Sent: Tuesday, June 14, 2022 6:51 AM > > > > > > On Mon, Jun 13, 2022 at 08:57:09PM +0900, Yoshihiro Shimoda wrote: > > > > Add R-Car Gen4 PCIe Endpoint support. This controller is based on > > > > Synopsys Designware PCIe. > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > --- > > [...] > > > > > + u32 num_lanes; > > > > > > What's wrong with dw_pcie.num_lanes? > > > > The dw_pcie.num_lanes is set after dw_pcie_ep_init() succeeded. > > However, this driver would like to refer the num_lanes before dw_pcie_ep_init() > > to initialize vendor-specific registers. In other words, this value is only > > needed at that timing. So, we can remove this from struct rcar_gen4_pcie_ep, > > and just get the num_lanes as a local variable. > > AFAICT, you are just using it to set PCI_EXP_LNKCAP_MLW. That's a common > register, so it should be able to set in the common code. I see. So, I'll add such a code into the DWC common code because dw_pcie_dbi_ro_wr_en() is required to set PCI_EXP_LNKCAP_MLW. > [...] > > > > > +static struct platform_driver rcar_gen4_pcie_ep_driver = { > > > > + .driver = { > > > > + .name = "pcie-rcar-gen4-ep", > > > > + .of_match_table = rcar_gen4_pcie_of_match, > > > > + }, > > > > + .probe = rcar_gen4_pcie_ep_probe, > > > > + .remove = rcar_gen4_pcie_ep_remove, > > > > +}; > > > > +builtin_platform_driver(rcar_gen4_pcie_ep_driver); > > > > > > Not a module or... > > > > > > > + > > > > +MODULE_DESCRIPTION("Renesas R-Car Gen4 PCIe endpoint controller driver"); > > > > +MODULE_LICENSE("GPL v2"); > > > > > > A module? Should be a module if possible. > > > > Oops. I'll drop these MODULE_*. > > No, is there some reason it can't be a module? Thank you. Now I understood your comment. I think it's possible to be a module. So, I'll use module_platform_driver() instead if possible. Best regards, Yoshihiro Shimoda > Rob
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 3ddccc9c38c5..503ead1a4358 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -393,4 +393,13 @@ config PCIE_RCAR_GEN4 Say Y here if you want PCIe host controller support on R-Car Gen4 SoCs. This uses the DesignWare core. +config PCIE_RCAR_GEN4_EP + bool "Renesas R-Car Gen4 PCIe Endpoint controller" + depends on ARCH_RENESAS || COMPILE_TEST + depends on PCI_ENDPOINT + select PCIE_DW_EP + help + Say Y here if you want PCIe endpoint controller support on R-Car Gen4 + SoCs. This uses the DesignWare core. + endmenu diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index b3f285e685f9..3d40346efd27 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o pcie-rcar-gen4-host.o +obj-$(CONFIG_PCIE_RCAR_GEN4_EP) += pcie-rcar-gen4.o pcie-rcar-gen4-ep.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-ep.c b/drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c new file mode 100644 index 000000000000..622e32c7a410 --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c @@ -0,0 +1,253 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe Endpoint driver for Renesas R-Car Gen4 Series SoCs + * Copyright (C) 2022 Renesas Electronics Corporation + */ + +#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" + +/* Configuration */ +#define PCICONF3 0x000c +#define MULTI_FUNC BIT(23) + +struct rcar_gen4_pcie_ep { + struct rcar_gen4_pcie *pcie; + struct dw_pcie *pci; + u32 num_lanes; +}; + +static void rcar_gen4_pcie_ep_init(struct dw_pcie_ep *ep) +{ + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + enum pci_barno bar; + + for (bar = BAR_0; bar <= BAR_5; bar++) + dw_pcie_ep_reset_bar(pci, bar); +} + +static int rcar_gen4_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no, + enum pci_epc_irq_type type, + u16 interrupt_num) +{ + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + + switch (type) { + case PCI_EPC_IRQ_LEGACY: + return dw_pcie_ep_raise_legacy_irq(ep, func_no); + case PCI_EPC_IRQ_MSI: + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); + case PCI_EPC_IRQ_MSIX: + return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); + default: + dev_err(pci->dev, "UNKNOWN IRQ type\n"); + return -EINVAL; + } + + return 0; +} + +static const struct pci_epc_features rcar_gen4_pcie_epc_features = { + .linkup_notifier = false, + .msi_capable = true, + .msix_capable = false, + .align = SZ_1M, +}; + +static const struct pci_epc_features* +rcar_gen4_pcie_ep_get_features(struct dw_pcie_ep *ep) +{ + return &rcar_gen4_pcie_epc_features; +} + +static const struct dw_pcie_ep_ops pcie_ep_ops = { + .ep_init = rcar_gen4_pcie_ep_init, + .raise_irq = rcar_gen4_pcie_ep_raise_irq, + .get_features = rcar_gen4_pcie_ep_get_features, +}; + +static int rcar_gen4_add_pcie_ep(struct rcar_gen4_pcie_ep *pcie_ep, + struct platform_device *pdev) +{ + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; + struct dw_pcie *pci = pcie->pci; + struct dw_pcie_ep *ep; + struct resource *res; + int ret; + + ep = &pci->ep; + ep->ops = &pcie_ep_ops; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); + if (!res) + return -EINVAL; + + ep->addr_size = resource_size(res); + + ret = dw_pcie_ep_init(ep); + if (ret) { + dev_err(&pdev->dev, "failed to initialize endpoint\n"); + return ret; + } + + pci->ops->start_link(pci); + + return 0; +} + +static void rcar_gen4_remove_pcie_ep(struct rcar_gen4_pcie_ep *pcie_ep) +{ + dw_pcie_ep_exit(&pcie_ep->pcie->pci->ep); +} + +static void rcar_gen4_pcie_init_ep(struct rcar_gen4_pcie_ep *pcie_ep) +{ + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; + struct dw_pcie *pci = pcie->pci; + int val; + + /* Device type selection - Endpoint */ + val = rcar_gen4_pcie_readl(pcie, PCIEMSR0); + val |= DEVICE_TYPE_EP; + if (pcie_ep->num_lanes < 4) + val |= BIFUR_MOD_SET_ON; + rcar_gen4_pcie_writel(pcie, PCIEMSR0, val); + + dw_pcie_dbi_ro_wr_en(pci); + + /* Single function */ + val = dw_pcie_readl_dbi(pci, PCICONF3); + val &= ~MULTI_FUNC; + dw_pcie_writel_dbi(pci, PCICONF3, val); + + /* Disable unused BARs */ + dw_pcie_writel_dbi(pci, SHADOW_REG(BAR2MASKF), 0x0); + dw_pcie_writel_dbi(pci, SHADOW_REG(BAR3MASKF), 0x0); + + /* Set Max Link Width */ + rcar_gen4_pcie_set_max_link_width(pci, pcie_ep->num_lanes); + + dw_pcie_dbi_ro_wr_dis(pci); +} + +static int rcar_gen4_pcie_ep_get_resources(struct rcar_gen4_pcie_ep *pcie_ep, + struct platform_device *pdev) +{ + struct rcar_gen4_pcie *pcie = pcie_ep->pcie; + struct dw_pcie *pci = pcie->pci; + struct device *dev = pci->dev; + struct device_node *np = dev->of_node; + struct resource *res; + int err; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); + pci->dbi_base = devm_pci_remap_cfg_resource(dev, res); + if (IS_ERR(pci->dbi_base)) + return PTR_ERR(pci->dbi_base); + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu"); + pci->atu_base = devm_ioremap_resource(dev, res); + if (IS_ERR(pci->atu_base)) + return PTR_ERR(pci->atu_base); + + /* Renesas-specific registers */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "appl"); + pcie->base = devm_ioremap_resource(dev, res); + if (IS_ERR(pcie->base)) + return PTR_ERR(pcie->base); + + err = of_property_read_u32(np, "num-lanes", &pcie_ep->num_lanes); + if (err < 0) { + dev_err(dev, "num-lanes not found %d\n", err); + return err; + } + + return rcar_gen4_pcie_devm_clk_and_reset_get(pcie, dev); +} + +static int rcar_gen4_pcie_ep_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rcar_gen4_pcie_ep *pcie_ep; + struct rcar_gen4_pcie *pcie; + int err; + + pcie_ep = devm_kzalloc(dev, sizeof(*pcie_ep), GFP_KERNEL); + if (!pcie_ep) + return -ENOMEM; + + pcie = rcar_gen4_pcie_devm_alloc(dev); + if (!pcie) + return -ENOMEM; + pcie_ep->pcie = pcie; + + err = rcar_gen4_pcie_pm_runtime_enable(dev); + if (err < 0) { + dev_err(dev, "pm_runtime_get_sync failed\n"); + return err; + } + + err = rcar_gen4_pcie_ep_get_resources(pcie_ep, pdev); + if (err < 0) { + dev_err(dev, "failed to request resource: %d\n", err); + goto err_pm_put; + } + + pcie->priv = pcie_ep; + platform_set_drvdata(pdev, pcie); + + err = rcar_gen4_pcie_prepare(pcie); + if (err < 0) + goto err_pm_put; + rcar_gen4_pcie_init_ep(pcie_ep); + + err = rcar_gen4_add_pcie_ep(pcie_ep, pdev); + if (err < 0) + goto err_ep_disable; + + return 0; + +err_ep_disable: + rcar_gen4_pcie_unprepare(pcie); + +err_pm_put: + rcar_gen4_pcie_pm_runtime_disable(dev); + + return err; +} + +static int rcar_gen4_pcie_ep_remove(struct platform_device *pdev) +{ + struct rcar_gen4_pcie *pcie = platform_get_drvdata(pdev); + struct rcar_gen4_pcie_ep *pcie_ep = pcie->priv; + + rcar_gen4_remove_pcie_ep(pcie_ep); + rcar_gen4_pcie_unprepare(pcie_ep->pcie); + rcar_gen4_pcie_pm_runtime_disable(&pdev->dev); + + return 0; +} + +static const struct of_device_id rcar_gen4_pcie_of_match[] = { + { .compatible = "renesas,rcar-gen4-pcie-ep", }, + {}, +}; + +static struct platform_driver rcar_gen4_pcie_ep_driver = { + .driver = { + .name = "pcie-rcar-gen4-ep", + .of_match_table = rcar_gen4_pcie_of_match, + }, + .probe = rcar_gen4_pcie_ep_probe, + .remove = rcar_gen4_pcie_ep_remove, +}; +builtin_platform_driver(rcar_gen4_pcie_ep_driver); + +MODULE_DESCRIPTION("Renesas R-Car Gen4 PCIe endpoint controller driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.h b/drivers/pci/controller/dwc/pcie-rcar-gen4.h index bd01d0ffcac9..b6e285d8ebc0 100644 --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.h +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.h @@ -43,6 +43,7 @@ struct rcar_gen4_pcie { void __iomem *base; struct clk *bus_clk; struct reset_control *rst; + void *priv; }; extern u32 rcar_gen4_pcie_readl(struct rcar_gen4_pcie *pcie, u32 reg);
Add R-Car Gen4 PCIe Endpoint support. This controller is based on Synopsys Designware PCIe. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pci/controller/dwc/Kconfig | 9 + drivers/pci/controller/dwc/Makefile | 1 + .../pci/controller/dwc/pcie-rcar-gen4-ep.c | 253 ++++++++++++++++++ drivers/pci/controller/dwc/pcie-rcar-gen4.h | 1 + 4 files changed, 264 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c