| Message ID | 20230915102243.59775-1-minda.chen@starfivetech.com |
|---|---|
| Headers | show |
| Series | Refactoring Microchip PCIe driver and add StarFive PCIe | expand |
On 2023/9/15 18:22, Minda Chen wrote: > This patchset final purpose is add PCIe driver for StarFive JH7110 SoC. > JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the > same IP and have commit their codes, which are mixed with PLDA > controller codes and Microchip platform codes. > > For re-use the PLDA controller codes, I request refactoring microchip > codes, move PLDA common codes to PLDA files. > Desigware and Cadence is good example for refactoring codes. > > So first step is extract the PLDA common codes from microchip, and > refactoring the microchip codes.(patch1 - 16) > Then, add Starfive codes. (patch17 - 19) > > This patchset is base on v6.6-rc1 > > patch1 is move PLDA XpressRICH PCIe host common properties dt-binding > docs from microchip,pcie-host.yaml > patch2 is move PolarFire codes to PLDA directory. > patch3 is move PLDA IP register macros to plda-pcie.h > patch4 is rename data structure in microchip codes. > patch5 is rename two setup functions in microchip codes, prepare to move > to common file. > patch6 is change the arguments of plda_pcie_setup_iomems() > patch7 is move the two setup functions to common file pcie-plda-host.c > patch8 is Add PLDA event interrupt codes and IRQ domain ops. > patch9 is rename the IRQ related functions, prepare to move to > pcie-plda-host. > patch10 - 14 is modify the event codes, preparing for support starfive > and microchip two platforms. > patch15 is move IRQ related functions to pcie-plda-host.c > patch16 is set plda_event_handler to static. > patch17 is add StarFive JH7110 PCIe dt-binding doc. > patch18 is add StarFive JH7110 Soc PCIe codes. > patch19 is Starfive dts config > > previous version: > v1:https://patchwork.kernel.org/project/linux-pci/cover/20230719102057.22329-1-minda.chen@starfivetech.com/ > v2:https://patchwork.kernel.org/project/linux-pci/cover/20230727103949.26149-1-minda.chen@starfivetech.com/ > v3:https://patchwork.kernel.org/project/linux-pci/cover/20230814082016.104181-1-minda.chen@starfivetech.com/ > v4:https://patchwork.kernel.org/project/linux-pci/cover/20230825090129.65721-1-minda.chen@starfivetech.com/ > v5:https://patchwork.kernel.org/project/linux-pci/cover/20230907091058.125630-1-minda.chen@starfivetech.com/ > > change: > v6: > v5 patch 4 split to patch 4 -6. New patches just contain one > function modification. It is more reguluar. > patch 7: Just move the two setup functions only > patch 8 : draw a graph of PLDA local register, make it easier to > review the codes. > v5 patch 7 split to patch 9- 14. Each patch just contain one > function modification. It is more regular. > patch 9: rename IRQ related functions. > patch 10 - 14 : modify the events codes, total five patch. > patch 15: move IRQ related functions to pcie-plda-host.c > patch 16: Add new patch 16. > patch 18- 19 using "linux,pci-domain" dts setting. > Hi Bjorn I have noticed that the previous patches of refactoring codes is not so regular( the patched of modify Microchip' codes), and you don't give any comment to the patches. Now this verison is more regular and easier to review. Could you please review the driver codes? Hi Conor and Daire Thanks for reviewing. Now I split more patches and make it easier to review.
On Fri, Sep 15, 2023 at 06:22:43PM +0800, Minda Chen wrote: > Add PCIe dts configuraion for JH7110 SoC platform. > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > Reviewed-by: Hal Feng <hal.feng@starfivetech.com> > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> Not really important, but checkpatch whinges about the double signoff here, since both are you"
On Fri, Sep 15, 2023 at 06:22:26PM +0800, Minda Chen wrote: > For Microchip Polarfire PCIe host is PLDA XpressRich IP, > move to plda directory. Prepare for refactor the codes. > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > --- > MAINTAINERS | 2 +- > drivers/pci/controller/Kconfig | 9 +-------- > drivers/pci/controller/Makefile | 2 +- > drivers/pci/controller/plda/Kconfig | 14 ++++++++++++++ > drivers/pci/controller/plda/Makefile | 2 ++ > .../controller/{ => plda}/pcie-microchip-host.c | 2 +- > 6 files changed, 20 insertions(+), 11 deletions(-) > create mode 100644 drivers/pci/controller/plda/Kconfig > create mode 100644 drivers/pci/controller/plda/Makefile > rename drivers/pci/controller/{ => plda}/pcie-microchip-host.c (99%) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 90f13281d297..b1050804e6d2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16647,7 +16647,7 @@ M: Daire McNamara <daire.mcnamara@microchip.com> > L: linux-pci@vger.kernel.org > S: Supported > F: Documentation/devicetree/bindings/pci/microchip* > -F: drivers/pci/controller/*microchip* > +F: drivers/pci/controller/plda/*microchip* The riscv patchwork automation is complaining that you have added maintainers pattern errors with this patch. If you run ./scripts/get_maintainer.pl --self-test=patterns it'll tell you what the bad pattern is. Not sure why I never actually dumped the output of that test into the failure report, so I can't immediately tell you what is wrong.
On Fri, Sep 15, 2023 at 06:22:32PM +0800, Minda Chen wrote: > For PolarFire implements non-PLDA local interrupt events, most of > event interrupt process codes can not be re-used. PLDA implements > new codes and IRQ domain ops like PolarFire. > > plda_get_events() adds a new IRQ num to event num mapping codes for > PLDA local event except DMA engine interrupt events. The DMA engine > interrupt events are implemented by vendors. > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> Perhaps not important as they will go away in the next patch, but for this patch the riscv patchwork stuff noticed: drivers/pci/controller/plda/pcie-plda-host.c:114:36: warning: unused variable 'plda_evt_dom_ops' [-Wunused-const-variable] drivers/pci/controller/plda/pcie-plda-host.c:118:36: warning: unused variable 'plda_event_ops' [-Wunused-const-variable] Cheers, Conor. > --- > drivers/pci/controller/plda/pcie-plda-host.c | 99 ++++++++++++++++++++ > drivers/pci/controller/plda/pcie-plda.h | 33 +++++++ > 2 files changed, 132 insertions(+) > > diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c > index f0c7636f1f64..f29ff0f00dda 100644 > --- a/drivers/pci/controller/plda/pcie-plda-host.c > +++ b/drivers/pci/controller/plda/pcie-plda-host.c > @@ -20,6 +20,105 @@ > > #include "pcie-plda.h" > > +irqreturn_t plda_event_handler(int irq, void *dev_id) > +{ > + return IRQ_HANDLED; > +} > + > +static u32 plda_get_events(struct plda_pcie_rp *port) > +{ > + u32 events, val, origin; > + > + origin = readl_relaxed(port->bridge_addr + ISTATUS_LOCAL); > + > + /* Error events and doorbell events */ > + events = (origin >> A_ATR_EVT_POST_ERR_SHIFT) & 0xff; > + > + /* INTx events */ > + if (origin & PM_MSI_INT_INTX_MASK) > + events |= BIT(EVENT_PM_MSI_INT_INTX); > + > + /* MSI event and sys events */ > + val = (origin >> PM_MSI_INT_MSI_SHIFT) & 0xf; > + events |= val << EVENT_PM_MSI_INT_MSI; > + > + return events; > +} > + > +static u32 plda_hwirq_to_mask(int hwirq) > +{ > + u32 mask; > + > + if (hwirq < EVENT_PM_MSI_INT_INTX) > + mask = BIT(hwirq + A_ATR_EVT_POST_ERR_SHIFT); > + else if (hwirq == EVENT_PM_MSI_INT_INTX) > + mask = PM_MSI_INT_INTX_MASK; > + else > + mask = BIT(hwirq + PM_MSI_TO_MASK_OFFSET); > + > + return mask; > +} > + > +static void plda_ack_event_irq(struct irq_data *data) > +{ > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > + > + writel_relaxed(plda_hwirq_to_mask(data->hwirq), > + port->bridge_addr + ISTATUS_LOCAL); > +} > + > +static void plda_mask_event_irq(struct irq_data *data) > +{ > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > + u32 mask, val; > + > + mask = plda_hwirq_to_mask(data->hwirq); > + > + raw_spin_lock(&port->lock); > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); > + val &= ~mask; > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); > + raw_spin_unlock(&port->lock); > +} > + > +static void plda_unmask_event_irq(struct irq_data *data) > +{ > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > + u32 mask, val; > + > + mask = plda_hwirq_to_mask(data->hwirq); > + > + raw_spin_lock(&port->lock); > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); > + val |= mask; > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); > + raw_spin_unlock(&port->lock); > +} > + > +static struct irq_chip plda_event_irq_chip = { > + .name = "PLDA PCIe EVENT", > + .irq_ack = plda_ack_event_irq, > + .irq_mask = plda_mask_event_irq, > + .irq_unmask = plda_unmask_event_irq, > +}; > + > +static int plda_pcie_event_map(struct irq_domain *domain, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_chip_and_handler(irq, &plda_event_irq_chip, handle_level_irq); > + irq_set_chip_data(irq, domain->host_data); > + > + return 0; > +} > + > +static const struct irq_domain_ops plda_evt_dom_ops = { > + .map = plda_pcie_event_map, > +}; > + > +static const struct plda_event_ops plda_event_ops = { > + .get_events = plda_get_events, > +}; > + > void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > phys_addr_t axi_addr, phys_addr_t pci_addr, > size_t size) > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > index 3deefd35fa5a..32a913d4101f 100644 > --- a/drivers/pci/controller/plda/pcie-plda.h > +++ b/drivers/pci/controller/plda/pcie-plda.h > @@ -102,6 +102,38 @@ > #define EVENT_PM_MSI_INT_SYS_ERR 12 > #define NUM_PLDA_EVENTS 13 > > +/* > + * PLDA local interrupt register > + * > + * 31 27 23 15 7 0 > + * +--+--+--+-+------+-+-+-+-+-+-+-+-+-----------+-----------+ > + * |12|11|10|9| intx |7|6|5|4|3|2|1|0| DMA error | DMA end | > + * +--+--+--+-+------+-+-+-+-+-+-+-+-+-----------+-----------+ > + * 0: AXI post error > + * 1: AXI fetch error > + * 2: AXI discard error > + * 3: AXI doorbell > + * 4: PCIe post error > + * 5: PCIe fetch error > + * 6: PCIe discard error > + * 7: PCIe doorbell > + * 8: 4 INTx interruts > + * 9: MSI interrupt > + * 10: AER event > + * 11: PM/LTR/Hotplug > + * 12: System error > + * DMA error : reserved for vendor implement > + * DMA end : reserved for vendor implement > + */ > + > +#define PM_MSI_TO_MASK_OFFSET 19 > + > +struct plda_pcie_rp; > + > +struct plda_event_ops { > + u32 (*get_events)(struct plda_pcie_rp *pcie); > +}; > + > struct plda_msi { > struct mutex lock; /* Protect used bitmap */ > struct irq_domain *msi_domain; > @@ -120,6 +152,7 @@ struct plda_pcie_rp { > void __iomem *bridge_addr; > }; > > +irqreturn_t plda_event_handler(int irq, void *dev_id); > void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > phys_addr_t axi_addr, phys_addr_t pci_addr, > size_t size); > -- > 2.17.1 >
On 2023/9/16 8:09, Conor Dooley wrote: > On Fri, Sep 15, 2023 at 06:22:26PM +0800, Minda Chen wrote: >> For Microchip Polarfire PCIe host is PLDA XpressRich IP, >> move to plda directory. Prepare for refactor the codes. >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> MAINTAINERS | 2 +- >> drivers/pci/controller/Kconfig | 9 +-------- >> drivers/pci/controller/Makefile | 2 +- >> drivers/pci/controller/plda/Kconfig | 14 ++++++++++++++ >> drivers/pci/controller/plda/Makefile | 2 ++ >> .../controller/{ => plda}/pcie-microchip-host.c | 2 +- >> 6 files changed, 20 insertions(+), 11 deletions(-) >> create mode 100644 drivers/pci/controller/plda/Kconfig >> create mode 100644 drivers/pci/controller/plda/Makefile >> rename drivers/pci/controller/{ => plda}/pcie-microchip-host.c (99%) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 90f13281d297..b1050804e6d2 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -16647,7 +16647,7 @@ M: Daire McNamara <daire.mcnamara@microchip.com> >> L: linux-pci@vger.kernel.org >> S: Supported >> F: Documentation/devicetree/bindings/pci/microchip* >> -F: drivers/pci/controller/*microchip* >> +F: drivers/pci/controller/plda/*microchip* > > The riscv patchwork automation is complaining that you have added > maintainers pattern errors with this patch. If you run > ./scripts/get_maintainer.pl --self-test=patterns > it'll tell you what the bad pattern is. Not sure why I never actually > dumped the output of that test into the failure report, so I can't > immediately tell you what is wrong. Okay. I will check this.
On 2023/9/16 8:07, Conor Dooley wrote: > On Fri, Sep 15, 2023 at 06:22:43PM +0800, Minda Chen wrote: >> Add PCIe dts configuraion for JH7110 SoC platform. >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > > Not really important, but checkpatch whinges about the double signoff > here, since both are you" Thanks. I don't check this carefully.
Minda Chen wrote: > Add StarFive JH7110 SoC PCIe controller platform > driver codes, and add host init/deinit to pcie-plda-host.c > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > Co-developed-by: Kevin Xie <kevin.xie@starfivetech.com> > Reviewed-by: Mason Huo <mason.huo@starfivetech.com> > --- > MAINTAINERS | 7 + > drivers/pci/controller/plda/Kconfig | 12 + > drivers/pci/controller/plda/Makefile | 1 + > drivers/pci/controller/plda/pcie-plda-host.c | 121 ++++- > drivers/pci/controller/plda/pcie-plda.h | 91 +++- > drivers/pci/controller/plda/pcie-starfive.c | 461 +++++++++++++++++++ > 6 files changed, 688 insertions(+), 5 deletions(-) > create mode 100644 drivers/pci/controller/plda/pcie-starfive.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index ebdb65e195ee..4632cd411a3d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16679,6 +16679,13 @@ S: Maintained > F: Documentation/devicetree/bindings/pci/socionext,uniphier-pcie* > F: drivers/pci/controller/dwc/pcie-uniphier* > > +PCIE DRIVER FOR STARFIVE JH71x0 > +M: Kevin Xie <kevin.xie@starfivetech.com> > +L: linux-pci@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/pci/starfive* > +F: drivers/pci/controller/plda/pcie-starfive.c > + > PCIE DRIVER FOR ST SPEAR13XX > M: Pratyush Anand <pratyush.anand@gmail.com> > L: linux-pci@vger.kernel.org > diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig > index e54a82ee94f5..8de661730aa5 100644 > --- a/drivers/pci/controller/plda/Kconfig > +++ b/drivers/pci/controller/plda/Kconfig > @@ -15,4 +15,16 @@ config PCIE_MICROCHIP_HOST > Say Y here if you want kernel to support the Microchip AXI PCIe > Host Bridge driver. > > +config PCIE_STARFIVE_HOST > + tristate "StarFive PCIe host controller" > + depends on OF && PCI_MSI > + select PCIE_PLDA_HOST > + help > + Say Y here if you want to support the StarFive PCIe controller > + in host mode. StarFive PCIe controller uses PLDA PCIe > + core. > + If you choose to build this driver as module it will > + be dynamically linked and module will be called > + pcie-starfive.ko > + > endmenu > diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile > index 4340ab007f44..0ac6851bed48 100644 > --- a/drivers/pci/controller/plda/Makefile > +++ b/drivers/pci/controller/plda/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o > obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o > +obj-$(CONFIG_PCIE_STARFIVE_HOST) += pcie-starfive.o > diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c > index 26e6cfd2eaba..8b32df477ad8 100644 > --- a/drivers/pci/controller/plda/pcie-plda-host.c > +++ b/drivers/pci/controller/plda/pcie-plda-host.c > @@ -20,6 +20,15 @@ > > #include "pcie-plda.h" > > +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, > + int where) > +{ > + struct plda_pcie_rp *pcie = bus->sysdata; > + > + return pcie->config_base + PCIE_ECAM_OFFSET(bus->number, devfn, where); > +} > +EXPORT_SYMBOL_GPL(plda_pcie_map_bus); > + > static void plda_handle_msi(struct irq_desc *desc) > { > struct plda_pcie_rp *port = irq_desc_get_handler_data(desc); > @@ -370,6 +379,12 @@ static const struct plda_event_ops plda_event_ops = { > .get_events = plda_get_events, > }; > > +static const struct plda_event plda_default_event = { > + .event_ops = &plda_event_ops, > + .intx_event = EVENT_PM_MSI_INT_INTX, > + .msi_event = EVENT_PM_MSI_INT_MSI > +}; > + > static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port, > const struct irq_domain_ops *ops) > { > @@ -416,7 +431,6 @@ int plda_init_interrupts(struct platform_device *pdev, > const struct plda_event *event) > { > struct device *dev = &pdev->dev; > - int irq; > int i, intx_irq, msi_irq, event_irq; > int ret; > const struct irq_domain_ops *irq_dom_ops; > @@ -434,8 +448,8 @@ int plda_init_interrupts(struct platform_device *pdev, > return ret; > } > > - irq = platform_get_irq(pdev, 0); > - if (irq < 0) > + port->irq = platform_get_irq(pdev, 0); > + if (port->irq < 0) > return -ENODEV; Here you're dropping the local variable irq and just use port->irq directly.. > > port->event_ops = event->event_ops; > @@ -465,6 +479,7 @@ int plda_init_interrupts(struct platform_device *pdev, > dev_err(dev, "failed to map INTx interrupt\n"); > return -ENXIO; > } > + port->intx_irq = intx_irq; ..but for intx_irq.. > > /* Plug the INTx chained handler */ > irq_set_chained_handler_and_data(intx_irq, plda_handle_intx, port); > @@ -473,12 +488,13 @@ int plda_init_interrupts(struct platform_device *pdev, > event->msi_event); > if (!msi_irq) > return -ENXIO; > + port->msi_irq = msi_irq; ..and msi_irq you're keeping the local variable. I think you should drop the local variables here too, or at least do the same for all of them. > > /* Plug the MSI chained handler */ > irq_set_chained_handler_and_data(msi_irq, plda_handle_msi, port); > > /* Plug the main event chained handler */ > - irq_set_chained_handler_and_data(irq, plda_handle_event, port); > + irq_set_chained_handler_and_data(port->irq, plda_handle_event, port); > > return 0; > } > @@ -544,3 +560,100 @@ int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > return 0; > } > EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); > + > +static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie) > +{ > + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL); > + irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL); > + irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL); > + > + irq_domain_remove(pcie->msi.msi_domain); > + irq_domain_remove(pcie->msi.dev_domain); > + > + irq_domain_remove(pcie->intx_domain); > + irq_domain_remove(pcie->event_domain); > +} > + > +int plda_pcie_host_init(struct plda_pcie_rp *port, struct pci_ops *ops) > +{ > + struct resource *cfg_res; > + struct device *dev = port->dev; > + int ret; > + struct pci_host_bridge *bridge; > + struct platform_device *pdev = to_platform_device(dev); This looks weird. Please order initialized variables first and maybe even do reverse christmas tree order. Eg. longeist lines first. > + > + port->bridge_addr = > + devm_platform_ioremap_resource_byname(pdev, "apb"); > + > + if (IS_ERR(port->bridge_addr)) > + return dev_err_probe(dev, PTR_ERR(port->bridge_addr), > + "failed to map reg memory\n"); > + > + cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); > + if (!cfg_res) > + return dev_err_probe(dev, -ENODEV, > + "failed to get config memory\n"); > + > + port->config_base = devm_ioremap_resource(dev, cfg_res); > + if (IS_ERR(port->config_base)) > + return dev_err_probe(dev, PTR_ERR(port->config_base), > + "failed to map config memory\n"); > + > + port->phy = devm_phy_optional_get(dev, NULL); > + if (IS_ERR(port->phy)) > + return dev_err_probe(dev, PTR_ERR(port->phy), > + "failed to get pcie phy\n"); > + > + bridge = devm_pci_alloc_host_bridge(dev, 0); > + if (!bridge) > + return dev_err_probe(dev, -ENOMEM, > + "failed to alloc bridge\n"); > + > + if (port->host_ops && port->host_ops->host_init) { > + ret = port->host_ops->host_init(port); > + if (ret) > + return ret; > + } > + > + port->bridge = bridge; > + plda_pcie_setup_window(port->bridge_addr, 0, cfg_res->start, 0, > + resource_size(cfg_res)); > + plda_pcie_setup_iomems(bridge, port); > + plda_set_default_msi(&port->msi); > + ret = plda_init_interrupts(pdev, port, &plda_default_event); > + if (ret) > + goto err_host; > + > + /* Set default bus ops */ > + bridge->ops = ops; > + bridge->sysdata = port; > + > + ret = pci_host_probe(bridge); > + if (ret < 0) { > + dev_err(dev, "failed to pci host probe: %d\n", ret); I think you mean: dev_err_probe(dev, ret, "failed to probe pci host\n"); > + goto err_probe; > + } > + > + return ret; > + > +err_probe: > + plda_pcie_irq_domain_deinit(port); > +err_host: > + if (port->host_ops && port->host_ops->host_deinit) > + port->host_ops->host_deinit(port); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(plda_pcie_host_init); > + > +void plda_pcie_host_deinit(struct plda_pcie_rp *port) > +{ > + pci_stop_root_bus(port->bridge->bus); > + pci_remove_root_bus(port->bridge->bus); > + > + plda_pcie_irq_domain_deinit(port); > + > + if (port->host_ops && port->host_ops->host_deinit) > + port->host_ops->host_deinit(port); > +} > +EXPORT_SYMBOL_GPL(plda_pcie_host_deinit); > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > index b7420598a326..e571047d7944 100644 > --- a/drivers/pci/controller/plda/pcie-plda.h > +++ b/drivers/pci/controller/plda/pcie-plda.h > @@ -6,14 +6,24 @@ > #ifndef _PCIE_PLDA_H > #define _PCIE_PLDA_H > > +#include <linux/phy/phy.h> > + > /* Number of MSI IRQs */ > #define PLDA_MAX_NUM_MSI_IRQS 32 > > /* PCIe Bridge Phy Regs */ > +#define GEN_SETTINGS 0x80 > +#define RP_ENABLE 1 > +#define PCIE_PCI_IDS_DW1 0x9c > +#define IDS_CLASS_CODE_SHIFT 16 > #define PCIE_PCI_IRQ_DW0 0xa8 > #define MSIX_CAP_MASK BIT(31) > #define NUM_MSI_MSGS_MASK GENMASK(6, 4) > #define NUM_MSI_MSGS_SHIFT 4 > +#define PCI_MISC 0xb4 > +#define PHY_FUNCTION_DIS BIT(15) > +#define PCIE_WINROM 0xfc > +#define PREF_MEM_WIN_64_SUPPORT BIT(3) > > #define IMASK_LOCAL 0x180 > #define DMA_END_ENGINE_0_MASK 0x00000000u > @@ -64,6 +74,8 @@ > #define ISTATUS_HOST 0x18c > #define IMSI_ADDR 0x190 > #define ISTATUS_MSI 0x194 > +#define PMSG_SUPPORT_RX 0x3f0 > +#define PMSG_LTR_SUPPORT BIT(2) > > /* PCIe Master table init defines */ > #define ATR0_PCIE_WIN0_SRCADDR_PARAM 0x600u > @@ -85,6 +97,8 @@ > #define PCIE_TX_RX_INTERFACE 0x00000000u > #define PCIE_CONFIG_INTERFACE 0x00000001u > > +#define CONFIG_SPACE_ADDR 0x1000u > + > #define ATR_ENTRY_SIZE 32 > > #define EVENT_A_ATR_EVT_POST_ERR 0 > @@ -134,6 +148,11 @@ struct plda_event_ops { > u32 (*get_events)(struct plda_pcie_rp *pcie); > }; > > +struct plda_pcie_host_ops { > + int (*host_init)(struct plda_pcie_rp *pcie); > + void (*host_deinit)(struct plda_pcie_rp *pcie); > +}; > + > struct plda_msi { > struct mutex lock; /* Protect used bitmap */ > struct irq_domain *msi_domain; > @@ -145,12 +164,19 @@ struct plda_msi { > > struct plda_pcie_rp { > struct device *dev; > + struct pci_host_bridge *bridge; > struct irq_domain *intx_domain; > struct irq_domain *event_domain; > raw_spinlock_t lock; > struct plda_msi msi; > const struct plda_event_ops *event_ops; > + const struct plda_pcie_host_ops *host_ops; > + struct phy *phy; > void __iomem *bridge_addr; > + void __iomem *config_base; > + int irq; > + int msi_irq; > + int intx_irq; > int num_events; > }; > > @@ -163,6 +189,7 @@ struct plda_event { > int msi_event; > }; > > +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, int where); > int plda_init_interrupts(struct platform_device *pdev, > struct plda_pcie_rp *port, > const struct plda_event *event); > @@ -171,4 +198,66 @@ void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > size_t size); > int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > struct plda_pcie_rp *port); > -#endif > +int plda_pcie_host_init(struct plda_pcie_rp *pcie, struct pci_ops *ops); > +void plda_pcie_host_deinit(struct plda_pcie_rp *pcie); > + > +static inline void plda_set_default_msi(struct plda_msi *msi) > +{ > + msi->vector_phy = IMSI_ADDR; > + msi->num_vectors = PLDA_MAX_NUM_MSI_IRQS; > +} > + > +static inline void plda_pcie_enable_root_port(struct plda_pcie_rp *plda) > +{ > + u32 value; > + > + value = readl_relaxed(plda->bridge_addr + GEN_SETTINGS); > + value |= RP_ENABLE; > + writel_relaxed(value, plda->bridge_addr + GEN_SETTINGS); > +} > + > +static inline void plda_pcie_set_standard_class(struct plda_pcie_rp *plda) > +{ > + u32 value; > + > + value = readl_relaxed(plda->bridge_addr + PCIE_PCI_IDS_DW1); > + value &= 0xff; > + value |= (PCI_CLASS_BRIDGE_PCI << IDS_CLASS_CODE_SHIFT); This looks suspicious. You're clearing the lower 8 bits, but setting bits 16 and up. Also the parentheses are not strictly necessary. > + writel_relaxed(value, plda->bridge_addr + PCIE_PCI_IDS_DW1); > +} > + > +static inline void plda_pcie_set_pref_win_64bit(struct plda_pcie_rp *plda) > +{ > + u32 value; > + > + value = readl_relaxed(plda->bridge_addr + PCIE_WINROM); > + value |= PREF_MEM_WIN_64_SUPPORT; > + writel_relaxed(value, plda->bridge_addr + PCIE_WINROM); > +} > + > +static inline void plda_pcie_disable_ltr(struct plda_pcie_rp *plda) > +{ > + u32 value; > + > + value = readl_relaxed(plda->bridge_addr + PMSG_SUPPORT_RX); > + value &= ~PMSG_LTR_SUPPORT; > + writel_relaxed(value, plda->bridge_addr + PMSG_SUPPORT_RX); > +} > + > +static inline void plda_pcie_disable_func(struct plda_pcie_rp *plda) > +{ > + u32 value; > + > + value = readl_relaxed(plda->bridge_addr + PCI_MISC); > + value |= PHY_FUNCTION_DIS; > + writel_relaxed(value, plda->bridge_addr + PCI_MISC); > +} > + > +static inline void plda_pcie_write_rc_bar(struct plda_pcie_rp *plda, u64 val) > +{ > + void __iomem *addr = plda->bridge_addr + CONFIG_SPACE_ADDR; > + > + writel_relaxed(val & 0xffffffff, addr + PCI_BASE_ADDRESS_0); > + writel_relaxed(val >> 32, addr + PCI_BASE_ADDRESS_1); Consider using lower_32_bits(val) and upper_32_bits(val) here. > +} > +#endif /* _PCIE_PLDA_H */ > diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c > new file mode 100644 > index 000000000000..42099fef0d7d > --- /dev/null > +++ b/drivers/pci/controller/plda/pcie-starfive.c > @@ -0,0 +1,461 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * PCIe host controller driver for StarFive JH7110 Soc. > + * > + * Copyright (C) 2023 StarFive Technology Co., Ltd. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/of_pci.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/reset.h> > +#include "../../pci.h" > + > +#include "pcie-plda.h" > + > +#define PCIE_FUNC_NUM 4 > + > +/* system control */ > +#define STG_SYSCON_PCIE0_BASE 0x48 > +#define STG_SYSCON_PCIE1_BASE 0x1f8 > + > +#define STG_SYSCON_ARFUNC_OFFSET 0x78 > +#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK GENMASK(22, 8) > +#define STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(x) FIELD_PREP(GENMASK(20, 17), x) > +#define STG_SYSCON_AWFUNC_OFFSET 0x7c > +#define STG_SYSCON_AXI4_SLVL_AWFUNC_MASK GENMASK(14, 0) > +#define STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(x) FIELD_PREP(GENMASK(12, 9), x) > +#define STG_SYSCON_CLKREQ BIT(22) > +#define STG_SYSCON_CKREF_SRC_MASK GENMASK(19, 18) > +#define STG_SYSCON_RP_NEP_OFFSET 0xe8 > +#define STG_SYSCON_K_RP_NEP BIT(8) > +#define STG_SYSCON_LNKSTA_OFFSET 0x170 > +#define DATA_LINK_ACTIVE BIT(5) > + > +/* Parameters for the waiting for link up routine */ > +#define LINK_WAIT_MAX_RETRIES 10 > +#define LINK_WAIT_USLEEP_MIN 90000 > +#define LINK_WAIT_USLEEP_MAX 100000 > + > +struct starfive_jh7110_pcie { > + struct plda_pcie_rp plda; > + struct reset_control *resets; > + struct clk_bulk_data *clks; > + struct regmap *reg_syscon; > + struct gpio_desc *power_gpio; > + struct gpio_desc *reset_gpio; > + > + u32 stg_arfun; > + u32 stg_awfun; > + u32 stg_rp_nep; > + u32 stg_lnksta; > + > + int num_clks; > +}; > + > +/* > + * The BAR0/1 of bridge should be hidden during enumeration to > + * avoid the sizing and resource allocation by PCIe core. > + */ > +static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int devfn, > + int offset) > +{ > + if (pci_is_root_bus(bus) && !devfn && > + (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1)) > + return true; > + > + return false; > +} > + > +static int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn, > + int where, int size, u32 value) > +{ > + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > + return PCIBIOS_BAD_REGISTER_NUMBER; > + > + return pci_generic_config_write(bus, devfn, where, size, value); > +} > + > +static int starfive_pcie_config_read(struct pci_bus *bus, unsigned int devfn, > + int where, int size, u32 *value) > +{ > + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > + return PCIBIOS_BAD_REGISTER_NUMBER; > + > + return pci_generic_config_read(bus, devfn, where, size, value); > +} > + > +static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie, struct device *dev) > +{ > + int domain_nr, stg_pcie_base; > + > + pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks); > + if (pcie->num_clks < 0) > + return dev_err_probe(dev, -ENODEV, > + "failed to get pcie clocks\n"); > + > + pcie->resets = devm_reset_control_array_get_exclusive(dev); > + if (IS_ERR(pcie->resets)) > + return dev_err_probe(dev, PTR_ERR(pcie->resets), > + "failed to get pcie resets"); > + > + pcie->reg_syscon = > + syscon_regmap_lookup_by_phandle(dev->of_node, > + "starfive,stg-syscon"); > + > + if (IS_ERR(pcie->reg_syscon)) > + return dev_err_probe(dev, PTR_ERR(pcie->reg_syscon), > + "failed to parse starfive,stg-syscon\n"); > + > + domain_nr = of_get_pci_domain_nr(dev->of_node); > + > + if (domain_nr < 0 || domain_nr > 1) > + return dev_err_probe(dev, -ENODEV, > + "failed to get valid pcie id\n"); > + > + if (domain_nr == 0) > + stg_pcie_base = STG_SYSCON_PCIE0_BASE; > + else > + stg_pcie_base = STG_SYSCON_PCIE1_BASE; It seems you only need the domain_nr for the determining this offset into the stg_syscon. Since this is already specific to the JH7110 SoC, wouldn't it be safer to match on the 0x2b000000/0x2c000000 or 0x940000000/0x9c0000000 address of the peripheral instead? > + > + pcie->stg_arfun = stg_pcie_base + STG_SYSCON_ARFUNC_OFFSET; > + pcie->stg_awfun = stg_pcie_base + STG_SYSCON_AWFUNC_OFFSET; > + pcie->stg_rp_nep = stg_pcie_base + STG_SYSCON_RP_NEP_OFFSET; > + pcie->stg_lnksta = stg_pcie_base + STG_SYSCON_LNKSTA_OFFSET; These offsets are all the same for both pcie0 and pcie1 so why do you need a variable for each? Eg. why not just if (domain_nr == 0) pcie->stg_base = STG_SYSCON_PCIE0_BASE; else pcie->stg_base = STG_SYSCON_PCIE1_BASE; ..and then add add those static offsets where needed? > + > + pcie->reset_gpio = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); > + if (IS_ERR(pcie->reset_gpio)) { > + dev_warn(dev, "failed to get perst-gpio\n"); Why is this a warn when you actually error out? I think you mean return dev_err_probe(dev, PTR_ERR(pcie->reset_gpio, "failed to get reset GPIO\n"); > + return -EINVAL; > + } > + > + pcie->power_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW); > + if (IS_ERR(pcie->power_gpio)) { > + dev_warn(dev, "failed to get power-gpio\n"); > + return -EINVAL; return dev_err_probe(dev, PTR_ERR(pcie->power_gpio), "failed to get power GPIO\n"); > + } > + > + return 0; > +} > + > +static struct pci_ops starfive_pcie_ops = { > + .map_bus = plda_pcie_map_bus, > + .read = starfive_pcie_config_read, > + .write = starfive_pcie_config_write, > +}; > + > +static int starfive_pcie_clk_rst_init(struct starfive_jh7110_pcie *pcie) > +{ > + int ret; > + struct device *dev = pcie->plda.dev; Again, please order initialized variables first. > + > + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); > + if (ret) { > + dev_err(dev, "failed to enable clocks\n"); > + return ret; return dev_err_probe(dev, ret, "failed to enable clocks\n"); > + } > + > + ret = reset_control_deassert(pcie->resets); > + if (ret) { > + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > + dev_err(dev, "failed to resets\n"); I think you mean return dev_err_probe(dev, ret, "failed to deassert resets\n"); > + } > + > + return ret; > +} > + > +static void starfive_pcie_clk_rst_deinit(struct starfive_jh7110_pcie *pcie) > +{ > + reset_control_assert(pcie->resets); > + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > +} > + > +static bool starfive_pcie_link_up(struct plda_pcie_rp *plda) > +{ > + struct starfive_jh7110_pcie *pcie = > + container_of(plda, struct starfive_jh7110_pcie, plda); > + struct device *dev = pcie->plda.dev; This is only used once below, so just use pcie->plda.dev directly there. > + int ret; > + u32 stg_reg_val; > + > + ret = regmap_read(pcie->reg_syscon, pcie->stg_lnksta, &stg_reg_val); > + if (ret) { > + dev_err(dev, "failed to read link status\n"); > + return false; > + } > + > + if (stg_reg_val & DATA_LINK_ACTIVE) > + return true; > + > + return false; These last 4 lines is just "return stg_reg_val & DATA_LINK_ACTIVE;" right? > +} > + > +static int starfive_pcie_host_wait_for_link(struct starfive_jh7110_pcie *pcie) > +{ > + struct device *dev = pcie->plda.dev; Only used once, just inline. > + int retries; > + > + /* Check if the link is up or not */ > + for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { > + if (starfive_pcie_link_up(&pcie->plda)) { > + dev_info(dev, "port link up\n"); > + return 0; > + } > + usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > + } > + > + return -ETIMEDOUT; > +} > + > +static int starfive_pcie_enable_phy(struct device *dev, struct plda_pcie_rp *pcie) > +{ > + int ret; > + > + if (!pcie->phy) > + return 0; > + > + ret = phy_init(pcie->phy); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to initialize pcie phy\n"); > + > + ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE); > + if (ret) { > + dev_err(dev, "failed to set pcie mode\n"); > + goto err_phy_on; > + } > + > + ret = phy_power_on(pcie->phy); > + if (ret) { > + dev_err(dev, "failed to power on pcie phy\n"); > + goto err_phy_on; > + } > + > + return 0; > + > +err_phy_on: > + phy_exit(pcie->phy); > + return ret; > +} > + > +static void starfive_pcie_disable_phy(struct plda_pcie_rp *pcie) > +{ > + phy_power_off(pcie->phy); > + phy_exit(pcie->phy); > +} > + > +static void starfive_pcie_host_deinit(struct plda_pcie_rp *plda) > +{ > + struct starfive_jh7110_pcie *pcie = > + container_of(plda, struct starfive_jh7110_pcie, plda); > + > + starfive_pcie_clk_rst_deinit(pcie); > + if (pcie->power_gpio) > + gpiod_set_value_cansleep(pcie->power_gpio, 0); > + starfive_pcie_disable_phy(plda); > +} > + > +static int starfive_pcie_host_init(struct plda_pcie_rp *plda) > +{ > + int i; > + struct starfive_jh7110_pcie *pcie = > + container_of(plda, struct starfive_jh7110_pcie, plda); > + struct device *dev = plda->dev; > + int ret; Weird ordering again. > + > + ret = starfive_pcie_enable_phy(dev, plda); > + if (ret) > + return ret; > + > + regmap_update_bits(pcie->reg_syscon, pcie->stg_rp_nep, > + STG_SYSCON_K_RP_NEP, STG_SYSCON_K_RP_NEP); > + > + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > + STG_SYSCON_CKREF_SRC_MASK, > + FIELD_PREP(STG_SYSCON_CKREF_SRC_MASK, 2)); > + > + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > + STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ); > + > + ret = starfive_pcie_clk_rst_init(pcie); > + if (ret) > + return ret; > + > + if (pcie->power_gpio) > + gpiod_set_value_cansleep(pcie->power_gpio, 1); > + > + if (pcie->reset_gpio) > + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > + > + /* Disable physical functions except #0 */ > + for (i = 1; i < PCIE_FUNC_NUM; i++) { > + regmap_update_bits(pcie->reg_syscon, > + pcie->stg_arfun, > + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, > + STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(i)); > + > + regmap_update_bits(pcie->reg_syscon, > + pcie->stg_awfun, > + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, > + STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(i)); > + > + plda_pcie_disable_func(plda); > + } > + > + regmap_update_bits(pcie->reg_syscon, pcie->stg_arfun, > + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, 0); > + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, 0); > + > + plda_pcie_enable_root_port(plda); > + plda_pcie_write_rc_bar(plda, 0); > + > + /* PCIe PCI Standard Configuration Identification Settings. */ > + plda_pcie_set_standard_class(plda); > + > + /* > + * The LTR message forwarding of PCIe Message Reception was set by core > + * as default, but the forward id & addr are also need to be reset. > + * If we do not disable LTR message forwarding here, or set a legal > + * forwarding address, the kernel will get stuck after this driver probe. > + * To workaround, disable the LTR message forwarding support on > + * PCIe Message Reception. > + */ > + plda_pcie_disable_ltr(plda); > + > + /* Prefetchable memory window 64-bit addressing support */ > + plda_pcie_set_pref_win_64bit(plda); > + > + /* Ensure that PERST has been asserted for at least 100 ms, > + * the sleep value is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4) > + */ > + msleep(100); > + if (pcie->reset_gpio) > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > + > + /* As the requirement in PCIe base spec r6.0, system (<=5GT/s) must > + * wait a minimum of 100 ms following exit from a conventional reset > + * before sending a configuration request to the device. > + */ > + msleep(100); > + > + if (starfive_pcie_host_wait_for_link(pcie)) > + dev_info(dev, "port link down\n"); > + > + return ret; > +} > + > +static const struct plda_pcie_host_ops sf_host_ops = { > + .host_init = starfive_pcie_host_init, > + .host_deinit = starfive_pcie_host_deinit, > +}; > + > +static int starfive_pcie_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct starfive_jh7110_pcie *pcie; > + struct plda_pcie_rp *plda; > + int ret; > + > + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > + if (!pcie) > + return -ENOMEM; > + > + plda = &pcie->plda; > + plda->dev = dev; > + > + ret = starfive_pcie_parse_dt(pcie, dev); > + if (ret) > + return ret; > + > + plda->host_ops = &sf_host_ops; > + plda->num_events = NUM_PLDA_EVENTS; > + ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops); > + if (ret) > + return ret; > + > + pm_runtime_enable(&pdev->dev); > + pm_runtime_get_sync(&pdev->dev); > + platform_set_drvdata(pdev, pcie); > + > + return 0; > +} > + > +static void starfive_pcie_remove(struct platform_device *pdev) > +{ > + struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev); > + > + plda_pcie_host_deinit(&pcie->plda); > + platform_set_drvdata(pdev, NULL); > +} > + > +static int starfive_pcie_suspend_noirq(struct device *dev) > +{ > + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > + > + if (!pcie) > + return 0; > + > + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > + starfive_pcie_disable_phy(&pcie->plda); > + > + return 0; > +} > + > +static int starfive_pcie_resume_noirq(struct device *dev) > +{ > + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > + int ret; > + > + ret = starfive_pcie_enable_phy(dev, &pcie->plda); > + if (ret) > + return ret; > + > + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); > + if (ret) { > + dev_err(dev, "failed to enable clocks\n"); > + starfive_pcie_disable_phy(&pcie->plda); > + return ret; > + } > + > + return ret; > +} > + > +static const struct dev_pm_ops starfive_pcie_pm_ops = { > + NOIRQ_SYSTEM_SLEEP_PM_OPS(starfive_pcie_suspend_noirq, > + starfive_pcie_resume_noirq) > +}; > + > +static const struct of_device_id starfive_pcie_of_match[] = { > + { .compatible = "starfive,jh7110-pcie"}, Missing space before } > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, starfive_pcie_of_match); > + > +static struct platform_driver starfive_pcie_driver = { > + .driver = { > + .name = "pcie-starfive", > + .of_match_table = of_match_ptr(starfive_pcie_of_match), > +#ifdef CONFIG_PM_SLEEP > + .pm = &starfive_pcie_pm_ops, > +#endif This can be just .pm = pm_sleep_ptr(&starfive_pcie_pm_ops), > + }, > + .probe = starfive_pcie_probe, > + .remove_new = starfive_pcie_remove, > +}; > +module_platform_driver(starfive_pcie_driver); > + > +MODULE_DESCRIPTION("StarFive JH7110 PCIe host driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.17.1
Minda Chen wrote: > Add PCIe dts configuraion for JH7110 SoC platform. > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > Reviewed-by: Hal Feng <hal.feng@starfivetech.com> > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > --- > .../jh7110-starfive-visionfive-2.dtsi | 63 +++++++++++++ > arch/riscv/boot/dts/starfive/jh7110.dtsi | 88 +++++++++++++++++++ > 2 files changed, 151 insertions(+) > > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > index d79f94432b27..8c84852f1c06 100644 > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > @@ -402,6 +402,53 @@ > }; > }; > > + pcie0_pins: pcie0-0 { > + wake-pins { > + pinmux = <GPIOMUX(32, GPOUT_LOW, > + GPOEN_DISABLE, > + GPI_NONE)>; > + bias-pull-up; > + drive-strength = <2>; > + input-enable; > + input-schmitt-disable; > + slew-rate = <0>; > + }; > + clkreq-pins { > + pinmux = <GPIOMUX(27, GPOUT_LOW, > + GPOEN_DISABLE, > + GPI_NONE)>; > + bias-pull-down; > + drive-strength = <2>; > + input-enable; > + input-schmitt-disable; > + slew-rate = <0>; > + }; > + }; > + > + pcie1_pins: pcie1-0 { > + wake-pins { > + pinmux = <GPIOMUX(21, GPOUT_LOW, > + GPOEN_DISABLE, > + GPI_NONE)>; > + bias-pull-up; > + drive-strength = <2>; > + input-enable; > + input-schmitt-disable; > + slew-rate = <0>; > + }; > + > + clkreq-pins { > + pinmux = <GPIOMUX(29, GPOUT_LOW, > + GPOEN_DISABLE, > + GPI_NONE)>; > + bias-pull-down; > + drive-strength = <2>; > + input-enable; > + input-schmitt-disable; > + slew-rate = <0>; > + }; > + }; > + > spi0_pins: spi0-0 { > mosi-pins { > pinmux = <GPIOMUX(52, GPOUT_SYS_SPI0_TXD, > @@ -499,6 +546,22 @@ > }; > }; > > +&pcie0 { > + pinctrl-names = "default"; > + perst-gpios = <&sysgpio 26 GPIO_ACTIVE_LOW>; > + pinctrl-0 = <&pcie0_pins>; > + phys = <&pciephy0>; > + status = "okay"; > +}; > + > +&pcie1 { > + pinctrl-names = "default"; > + perst-gpios = <&sysgpio 28 GPIO_ACTIVE_LOW>; > + pinctrl-0 = <&pcie1_pins>; > + phys = <&pciephy1>; > + status = "okay"; > +}; These nodes are out of place. The order is - root node - clocks sorted alphabetically - other node references sorted alphabetically > &tdm { > pinctrl-names = "default"; > pinctrl-0 = <&tdm_pins>; > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi > index e85464c328d0..97fe5a242d60 100644 > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi > @@ -1045,5 +1045,93 @@ > #reset-cells = <1>; > power-domains = <&pwrc JH7110_PD_VOUT>; > }; > + > + pcie0: pcie@940000000 { > + compatible = "starfive,jh7110-pcie"; > + reg = <0x9 0x40000000 0x0 0x1000000>, > + <0x0 0x2b000000 0x0 0x100000>; > + reg-names = "cfg", "apb"; > + linux,pci-domain = <0>; > + #address-cells = <3>; > + #size-cells = <2>; > + #interrupt-cells = <1>; > + ranges = <0x82000000 0x0 0x30000000 0x0 0x30000000 0x0 0x08000000>, > + <0xc3000000 0x9 0x00000000 0x9 0x00000000 0x0 0x40000000>; > + interrupts = <56>; > + interrupt-parent = <&plic>; Is interrupt-parent not inherited from the soc bus like other peripherals? > + interrupt-map-mask = <0x0 0x0 0x0 0x7>; > + interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc0 0x1>, > + <0x0 0x0 0x0 0x2 &pcie_intc0 0x2>, > + <0x0 0x0 0x0 0x3 &pcie_intc0 0x3>, > + <0x0 0x0 0x0 0x4 &pcie_intc0 0x4>; > + msi-controller; > + device_type = "pci"; > + starfive,stg-syscon = <&stg_syscon>; > + bus-range = <0x0 0xff>; > + clocks = <&syscrg JH7110_SYSCLK_NOC_BUS_STG_AXI>, > + <&stgcrg JH7110_STGCLK_PCIE0_TL>, > + <&stgcrg JH7110_STGCLK_PCIE0_AXI_MST0>, > + <&stgcrg JH7110_STGCLK_PCIE0_APB>; > + clock-names = "noc", "tl", "axi_mst0", "apb"; > + resets = <&stgcrg JH7110_STGRST_PCIE0_AXI_MST0>, > + <&stgcrg JH7110_STGRST_PCIE0_AXI_SLV0>, > + <&stgcrg JH7110_STGRST_PCIE0_AXI_SLV>, > + <&stgcrg JH7110_STGRST_PCIE0_BRG>, > + <&stgcrg JH7110_STGRST_PCIE0_CORE>, > + <&stgcrg JH7110_STGRST_PCIE0_APB>; > + reset-names = "mst0", "slv0", "slv", "brg", > + "core", "apb"; > + status = "disabled"; > + > + pcie_intc0: interrupt-controller { > + #address-cells = <0>; > + #interrupt-cells = <1>; > + interrupt-controller; > + }; > + }; > + > + pcie1: pcie@9c0000000 { > + compatible = "starfive,jh7110-pcie"; > + reg = <0x9 0xc0000000 0x0 0x1000000>, > + <0x0 0x2c000000 0x0 0x100000>; > + reg-names = "cfg", "apb"; > + linux,pci-domain = <1>; > + #address-cells = <3>; > + #size-cells = <2>; > + #interrupt-cells = <1>; > + ranges = <0x82000000 0x0 0x38000000 0x0 0x38000000 0x0 0x08000000>, > + <0xc3000000 0x9 0x80000000 0x9 0x80000000 0x0 0x40000000>; > + interrupts = <57>; > + interrupt-parent = <&plic>; ditto. > + interrupt-map-mask = <0x0 0x0 0x0 0x7>; > + interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc1 0x1>, > + <0x0 0x0 0x0 0x2 &pcie_intc1 0x2>, > + <0x0 0x0 0x0 0x3 &pcie_intc1 0x3>, > + <0x0 0x0 0x0 0x4 &pcie_intc1 0x4>; > + msi-controller; > + device_type = "pci"; > + starfive,stg-syscon = <&stg_syscon>; > + bus-range = <0x0 0xff>; > + clocks = <&syscrg JH7110_SYSCLK_NOC_BUS_STG_AXI>, > + <&stgcrg JH7110_STGCLK_PCIE1_TL>, > + <&stgcrg JH7110_STGCLK_PCIE1_AXI_MST0>, > + <&stgcrg JH7110_STGCLK_PCIE1_APB>; > + clock-names = "noc", "tl", "axi_mst0", "apb"; > + resets = <&stgcrg JH7110_STGRST_PCIE1_AXI_MST0>, > + <&stgcrg JH7110_STGRST_PCIE1_AXI_SLV0>, > + <&stgcrg JH7110_STGRST_PCIE1_AXI_SLV>, > + <&stgcrg JH7110_STGRST_PCIE1_BRG>, > + <&stgcrg JH7110_STGRST_PCIE1_CORE>, > + <&stgcrg JH7110_STGRST_PCIE1_APB>; > + reset-names = "mst0", "slv0", "slv", "brg", > + "core", "apb"; > + status = "disabled"; > + > + pcie_intc1: interrupt-controller { > + #address-cells = <0>; > + #interrupt-cells = <1>; > + interrupt-controller; > + }; > + }; > }; > }; > -- > 2.17.1
Minda Chen wrote: > Move setup functions to common pcie-plda-host.c. > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > --- > drivers/pci/controller/plda/Kconfig | 4 + > drivers/pci/controller/plda/Makefile | 1 + > .../pci/controller/plda/pcie-microchip-host.c | 59 ------------- > drivers/pci/controller/plda/pcie-plda-host.c | 82 +++++++++++++++++++ > drivers/pci/controller/plda/pcie-plda.h | 6 ++ > 5 files changed, 93 insertions(+), 59 deletions(-) > create mode 100644 drivers/pci/controller/plda/pcie-plda-host.c > > diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig > index 5cb3be4fc98c..e54a82ee94f5 100644 > --- a/drivers/pci/controller/plda/Kconfig > +++ b/drivers/pci/controller/plda/Kconfig > @@ -3,10 +3,14 @@ > menu "PLDA-based PCIe controllers" > depends on PCI > > +config PCIE_PLDA_HOST > + bool > + > config PCIE_MICROCHIP_HOST > tristate "Microchip AXI PCIe controller" > depends on PCI_MSI && OF > select PCI_HOST_COMMON > + select PCIE_PLDA_HOST > help > Say Y here if you want kernel to support the Microchip AXI PCIe > Host Bridge driver. > diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile > index e1a265cbf91c..4340ab007f44 100644 > --- a/drivers/pci/controller/plda/Makefile > +++ b/drivers/pci/controller/plda/Makefile > @@ -1,2 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o > obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c > index 1d253acd6bc2..ac7126b0bacf 100644 > --- a/drivers/pci/controller/plda/pcie-microchip-host.c > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c > @@ -837,65 +837,6 @@ static int mc_pcie_init_irq_domains(struct plda_pcie_rp *port) > return mc_allocate_msi_domains(port); > } > > -static void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > - phys_addr_t axi_addr, phys_addr_t pci_addr, > - size_t size) > -{ > - u32 atr_sz = ilog2(size) - 1; > - u32 val; > - > - if (index == 0) > - val = PCIE_CONFIG_INTERFACE; > - else > - val = PCIE_TX_RX_INTERFACE; > - > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_TRSL_PARAM); > - > - val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) | > - ATR_IMPL_ENABLE; > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_SRCADDR_PARAM); > - > - val = upper_32_bits(axi_addr); > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_SRC_ADDR); > - > - val = lower_32_bits(pci_addr); > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_TRSL_ADDR_LSB); > - > - val = upper_32_bits(pci_addr); > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_TRSL_ADDR_UDW); > - > - val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > - val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); > - writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > - writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); > -} > - > -static int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > - struct plda_pcie_rp *port) > -{ > - void __iomem *bridge_base_addr = port->bridge_addr; > - struct resource_entry *entry; > - u64 pci_addr; > - u32 index = 1; > - > - resource_list_for_each_entry(entry, &bridge->windows) { > - if (resource_type(entry->res) == IORESOURCE_MEM) { > - pci_addr = entry->res->start - entry->offset; > - plda_pcie_setup_window(bridge_base_addr, index, > - entry->res->start, pci_addr, > - resource_size(entry->res)); > - index++; > - } > - } > - > - return 0; > -} > - > static inline void mc_clear_secs(struct mc_pcie *port) > { > void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR; > diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c > new file mode 100644 > index 000000000000..f0c7636f1f64 > --- /dev/null > +++ b/drivers/pci/controller/plda/pcie-plda-host.c > @@ -0,0 +1,82 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PLDA PCIe XpressRich host controller driver > + * > + * Copyright (C) 2023 Microchip Co. Ltd > + * StarFive Co. Ltd. > + * > + * Author: Daire McNamara <daire.mcnamara@microchip.com> > + * Author: Minda Chen <minda.chen@starfivetech.com> > + */ > + > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> > +#include <linux/msi.h> > +#include <linux/of_address.h> > +#include <linux/of_pci.h> > +#include <linux/pci_regs.h> > +#include <linux/pci-ecam.h> > +#include <linux/platform_device.h> > + > +#include "pcie-plda.h" > + > +void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > + phys_addr_t axi_addr, phys_addr_t pci_addr, > + size_t size) > +{ > + u32 atr_sz = ilog2(size) - 1; > + u32 val; > + > + if (index == 0) > + val = PCIE_CONFIG_INTERFACE; > + else > + val = PCIE_TX_RX_INTERFACE; > + > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_TRSL_PARAM); > + > + val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) | > + ATR_IMPL_ENABLE; > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_SRCADDR_PARAM); > + > + val = upper_32_bits(axi_addr); > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_SRC_ADDR); > + > + val = lower_32_bits(pci_addr); > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_TRSL_ADDR_LSB); > + > + val = upper_32_bits(pci_addr); > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_TRSL_ADDR_UDW); > + > + val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > + val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); I know this code is just a straight copy, but for future cleanups are we guaranteed that this field is always zero? Otherwise it looks a little suspicious to do read-modify-write, but just set the (0x25 << 1) bits without clearing the field first. > + writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > + writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); > +} > +EXPORT_SYMBOL_GPL(plda_pcie_setup_window); > + > +int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > + struct plda_pcie_rp *port) > +{ > + void __iomem *bridge_base_addr = port->bridge_addr; > + struct resource_entry *entry; > + u64 pci_addr; > + u32 index = 1; > + > + resource_list_for_each_entry(entry, &bridge->windows) { > + if (resource_type(entry->res) == IORESOURCE_MEM) { > + pci_addr = entry->res->start - entry->offset; > + plda_pcie_setup_window(bridge_base_addr, index, > + entry->res->start, pci_addr, > + resource_size(entry->res)); > + index++; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > index d04a571404b9..3deefd35fa5a 100644 > --- a/drivers/pci/controller/plda/pcie-plda.h > +++ b/drivers/pci/controller/plda/pcie-plda.h > @@ -119,4 +119,10 @@ struct plda_pcie_rp { > struct plda_msi msi; > void __iomem *bridge_addr; > }; > + > +void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > + phys_addr_t axi_addr, phys_addr_t pci_addr, > + size_t size); > +int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > + struct plda_pcie_rp *port); > #endif > -- > 2.17.1
Emil Renner Berthing wrote: > Minda Chen wrote: > > Add StarFive JH7110 SoC PCIe controller platform > > driver codes, and add host init/deinit to pcie-plda-host.c > > > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > > Co-developed-by: Kevin Xie <kevin.xie@starfivetech.com> > > Reviewed-by: Mason Huo <mason.huo@starfivetech.com> > > --- > > MAINTAINERS | 7 + > > drivers/pci/controller/plda/Kconfig | 12 + > > drivers/pci/controller/plda/Makefile | 1 + > > drivers/pci/controller/plda/pcie-plda-host.c | 121 ++++- > > drivers/pci/controller/plda/pcie-plda.h | 91 +++- > > drivers/pci/controller/plda/pcie-starfive.c | 461 +++++++++++++++++++ > > 6 files changed, 688 insertions(+), 5 deletions(-) > > create mode 100644 drivers/pci/controller/plda/pcie-starfive.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index ebdb65e195ee..4632cd411a3d 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16679,6 +16679,13 @@ S: Maintained > > F: Documentation/devicetree/bindings/pci/socionext,uniphier-pcie* > > F: drivers/pci/controller/dwc/pcie-uniphier* > > > > +PCIE DRIVER FOR STARFIVE JH71x0 > > +M: Kevin Xie <kevin.xie@starfivetech.com> > > +L: linux-pci@vger.kernel.org > > +S: Maintained > > +F: Documentation/devicetree/bindings/pci/starfive* > > +F: drivers/pci/controller/plda/pcie-starfive.c > > + > > PCIE DRIVER FOR ST SPEAR13XX > > M: Pratyush Anand <pratyush.anand@gmail.com> > > L: linux-pci@vger.kernel.org > > diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig > > index e54a82ee94f5..8de661730aa5 100644 > > --- a/drivers/pci/controller/plda/Kconfig > > +++ b/drivers/pci/controller/plda/Kconfig > > @@ -15,4 +15,16 @@ config PCIE_MICROCHIP_HOST > > Say Y here if you want kernel to support the Microchip AXI PCIe > > Host Bridge driver. > > > > +config PCIE_STARFIVE_HOST > > + tristate "StarFive PCIe host controller" > > + depends on OF && PCI_MSI > > + select PCIE_PLDA_HOST > > + help > > + Say Y here if you want to support the StarFive PCIe controller > > + in host mode. StarFive PCIe controller uses PLDA PCIe > > + core. > > + If you choose to build this driver as module it will > > + be dynamically linked and module will be called > > + pcie-starfive.ko > > + > > endmenu > > diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile > > index 4340ab007f44..0ac6851bed48 100644 > > --- a/drivers/pci/controller/plda/Makefile > > +++ b/drivers/pci/controller/plda/Makefile > > @@ -1,3 +1,4 @@ > > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o > > obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o > > +obj-$(CONFIG_PCIE_STARFIVE_HOST) += pcie-starfive.o > > diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c > > index 26e6cfd2eaba..8b32df477ad8 100644 > > --- a/drivers/pci/controller/plda/pcie-plda-host.c > > +++ b/drivers/pci/controller/plda/pcie-plda-host.c > > @@ -20,6 +20,15 @@ > > > > #include "pcie-plda.h" > > > > +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, > > + int where) > > +{ > > + struct plda_pcie_rp *pcie = bus->sysdata; > > + > > + return pcie->config_base + PCIE_ECAM_OFFSET(bus->number, devfn, where); > > +} > > +EXPORT_SYMBOL_GPL(plda_pcie_map_bus); > > + > > static void plda_handle_msi(struct irq_desc *desc) > > { > > struct plda_pcie_rp *port = irq_desc_get_handler_data(desc); > > @@ -370,6 +379,12 @@ static const struct plda_event_ops plda_event_ops = { > > .get_events = plda_get_events, > > }; > > > > +static const struct plda_event plda_default_event = { > > + .event_ops = &plda_event_ops, > > + .intx_event = EVENT_PM_MSI_INT_INTX, > > + .msi_event = EVENT_PM_MSI_INT_MSI > > +}; > > + > > static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port, > > const struct irq_domain_ops *ops) > > { > > @@ -416,7 +431,6 @@ int plda_init_interrupts(struct platform_device *pdev, > > const struct plda_event *event) > > { > > struct device *dev = &pdev->dev; > > - int irq; > > int i, intx_irq, msi_irq, event_irq; > > int ret; > > const struct irq_domain_ops *irq_dom_ops; > > @@ -434,8 +448,8 @@ int plda_init_interrupts(struct platform_device *pdev, > > return ret; > > } > > > > - irq = platform_get_irq(pdev, 0); > > - if (irq < 0) > > + port->irq = platform_get_irq(pdev, 0); > > + if (port->irq < 0) > > return -ENODEV; > > Here you're dropping the local variable irq and just use port->irq directly.. > > > > > port->event_ops = event->event_ops; > > @@ -465,6 +479,7 @@ int plda_init_interrupts(struct platform_device *pdev, > > dev_err(dev, "failed to map INTx interrupt\n"); > > return -ENXIO; > > } > > + port->intx_irq = intx_irq; > > ..but for intx_irq.. > > > > > /* Plug the INTx chained handler */ > > irq_set_chained_handler_and_data(intx_irq, plda_handle_intx, port); > > @@ -473,12 +488,13 @@ int plda_init_interrupts(struct platform_device *pdev, > > event->msi_event); > > if (!msi_irq) > > return -ENXIO; > > + port->msi_irq = msi_irq; > > ..and msi_irq you're keeping the local variable. I think you should drop > the local variables here too, or at least do the same for all of them. > > > > > /* Plug the MSI chained handler */ > > irq_set_chained_handler_and_data(msi_irq, plda_handle_msi, port); > > > > /* Plug the main event chained handler */ > > - irq_set_chained_handler_and_data(irq, plda_handle_event, port); > > + irq_set_chained_handler_and_data(port->irq, plda_handle_event, port); > > > > return 0; > > } > > @@ -544,3 +560,100 @@ int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > > return 0; > > } > > EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); > > + > > +static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie) > > +{ > > + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL); > > + irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL); > > + irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL); > > + > > + irq_domain_remove(pcie->msi.msi_domain); > > + irq_domain_remove(pcie->msi.dev_domain); > > + > > + irq_domain_remove(pcie->intx_domain); > > + irq_domain_remove(pcie->event_domain); > > +} > > + > > +int plda_pcie_host_init(struct plda_pcie_rp *port, struct pci_ops *ops) > > +{ > > + struct resource *cfg_res; > > + struct device *dev = port->dev; > > + int ret; > > + struct pci_host_bridge *bridge; > > + struct platform_device *pdev = to_platform_device(dev); > > This looks weird. Please order initialized variables first and maybe even do > reverse christmas tree order. Eg. longeist lines first. > > > + > > + port->bridge_addr = > > + devm_platform_ioremap_resource_byname(pdev, "apb"); > > + > > + if (IS_ERR(port->bridge_addr)) > > + return dev_err_probe(dev, PTR_ERR(port->bridge_addr), > > + "failed to map reg memory\n"); > > + > > + cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); > > + if (!cfg_res) > > + return dev_err_probe(dev, -ENODEV, > > + "failed to get config memory\n"); > > + > > + port->config_base = devm_ioremap_resource(dev, cfg_res); > > + if (IS_ERR(port->config_base)) > > + return dev_err_probe(dev, PTR_ERR(port->config_base), > > + "failed to map config memory\n"); > > + > > + port->phy = devm_phy_optional_get(dev, NULL); > > + if (IS_ERR(port->phy)) > > + return dev_err_probe(dev, PTR_ERR(port->phy), > > + "failed to get pcie phy\n"); > > + > > + bridge = devm_pci_alloc_host_bridge(dev, 0); > > + if (!bridge) > > + return dev_err_probe(dev, -ENOMEM, > > + "failed to alloc bridge\n"); > > + > > + if (port->host_ops && port->host_ops->host_init) { > > + ret = port->host_ops->host_init(port); > > + if (ret) > > + return ret; > > + } > > + > > + port->bridge = bridge; > > + plda_pcie_setup_window(port->bridge_addr, 0, cfg_res->start, 0, > > + resource_size(cfg_res)); > > + plda_pcie_setup_iomems(bridge, port); > > + plda_set_default_msi(&port->msi); > > + ret = plda_init_interrupts(pdev, port, &plda_default_event); > > + if (ret) > > + goto err_host; > > + > > + /* Set default bus ops */ > > + bridge->ops = ops; > > + bridge->sysdata = port; > > + > > + ret = pci_host_probe(bridge); > > + if (ret < 0) { > > + dev_err(dev, "failed to pci host probe: %d\n", ret); > > I think you mean: > > dev_err_probe(dev, ret, "failed to probe pci host\n"); > > > + goto err_probe; > > + } > > + > > + return ret; > > + > > +err_probe: > > + plda_pcie_irq_domain_deinit(port); > > +err_host: > > + if (port->host_ops && port->host_ops->host_deinit) > > + port->host_ops->host_deinit(port); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(plda_pcie_host_init); > > + > > +void plda_pcie_host_deinit(struct plda_pcie_rp *port) > > +{ > > + pci_stop_root_bus(port->bridge->bus); > > + pci_remove_root_bus(port->bridge->bus); > > + > > + plda_pcie_irq_domain_deinit(port); > > + > > + if (port->host_ops && port->host_ops->host_deinit) > > + port->host_ops->host_deinit(port); > > +} > > +EXPORT_SYMBOL_GPL(plda_pcie_host_deinit); > > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > > index b7420598a326..e571047d7944 100644 > > --- a/drivers/pci/controller/plda/pcie-plda.h > > +++ b/drivers/pci/controller/plda/pcie-plda.h > > @@ -6,14 +6,24 @@ > > #ifndef _PCIE_PLDA_H > > #define _PCIE_PLDA_H > > > > +#include <linux/phy/phy.h> > > + > > /* Number of MSI IRQs */ > > #define PLDA_MAX_NUM_MSI_IRQS 32 > > > > /* PCIe Bridge Phy Regs */ > > +#define GEN_SETTINGS 0x80 > > +#define RP_ENABLE 1 > > +#define PCIE_PCI_IDS_DW1 0x9c > > +#define IDS_CLASS_CODE_SHIFT 16 > > #define PCIE_PCI_IRQ_DW0 0xa8 > > #define MSIX_CAP_MASK BIT(31) > > #define NUM_MSI_MSGS_MASK GENMASK(6, 4) > > #define NUM_MSI_MSGS_SHIFT 4 > > +#define PCI_MISC 0xb4 > > +#define PHY_FUNCTION_DIS BIT(15) > > +#define PCIE_WINROM 0xfc > > +#define PREF_MEM_WIN_64_SUPPORT BIT(3) > > > > #define IMASK_LOCAL 0x180 > > #define DMA_END_ENGINE_0_MASK 0x00000000u > > @@ -64,6 +74,8 @@ > > #define ISTATUS_HOST 0x18c > > #define IMSI_ADDR 0x190 > > #define ISTATUS_MSI 0x194 > > +#define PMSG_SUPPORT_RX 0x3f0 > > +#define PMSG_LTR_SUPPORT BIT(2) > > > > /* PCIe Master table init defines */ > > #define ATR0_PCIE_WIN0_SRCADDR_PARAM 0x600u > > @@ -85,6 +97,8 @@ > > #define PCIE_TX_RX_INTERFACE 0x00000000u > > #define PCIE_CONFIG_INTERFACE 0x00000001u > > > > +#define CONFIG_SPACE_ADDR 0x1000u > > + > > #define ATR_ENTRY_SIZE 32 > > > > #define EVENT_A_ATR_EVT_POST_ERR 0 > > @@ -134,6 +148,11 @@ struct plda_event_ops { > > u32 (*get_events)(struct plda_pcie_rp *pcie); > > }; > > > > +struct plda_pcie_host_ops { > > + int (*host_init)(struct plda_pcie_rp *pcie); > > + void (*host_deinit)(struct plda_pcie_rp *pcie); > > +}; > > + > > struct plda_msi { > > struct mutex lock; /* Protect used bitmap */ > > struct irq_domain *msi_domain; > > @@ -145,12 +164,19 @@ struct plda_msi { > > > > struct plda_pcie_rp { > > struct device *dev; > > + struct pci_host_bridge *bridge; > > struct irq_domain *intx_domain; > > struct irq_domain *event_domain; > > raw_spinlock_t lock; > > struct plda_msi msi; > > const struct plda_event_ops *event_ops; > > + const struct plda_pcie_host_ops *host_ops; > > + struct phy *phy; > > void __iomem *bridge_addr; > > + void __iomem *config_base; > > + int irq; > > + int msi_irq; > > + int intx_irq; > > int num_events; > > }; > > > > @@ -163,6 +189,7 @@ struct plda_event { > > int msi_event; > > }; > > > > +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, int where); > > int plda_init_interrupts(struct platform_device *pdev, > > struct plda_pcie_rp *port, > > const struct plda_event *event); > > @@ -171,4 +198,66 @@ void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > > size_t size); > > int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > > struct plda_pcie_rp *port); > > -#endif > > +int plda_pcie_host_init(struct plda_pcie_rp *pcie, struct pci_ops *ops); > > +void plda_pcie_host_deinit(struct plda_pcie_rp *pcie); > > + > > +static inline void plda_set_default_msi(struct plda_msi *msi) > > +{ > > + msi->vector_phy = IMSI_ADDR; > > + msi->num_vectors = PLDA_MAX_NUM_MSI_IRQS; > > +} > > + > > +static inline void plda_pcie_enable_root_port(struct plda_pcie_rp *plda) > > +{ > > + u32 value; > > + > > + value = readl_relaxed(plda->bridge_addr + GEN_SETTINGS); > > + value |= RP_ENABLE; > > + writel_relaxed(value, plda->bridge_addr + GEN_SETTINGS); > > +} > > + > > +static inline void plda_pcie_set_standard_class(struct plda_pcie_rp *plda) > > +{ > > + u32 value; > > + > > + value = readl_relaxed(plda->bridge_addr + PCIE_PCI_IDS_DW1); > > + value &= 0xff; > > + value |= (PCI_CLASS_BRIDGE_PCI << IDS_CLASS_CODE_SHIFT); > > This looks suspicious. You're clearing the lower 8 bits, but setting > bits 16 and up. Also the parentheses are not strictly necessary. Sorry, I just noticed I read this wrong. You're clearing *everything but* the lower 8 bits. This may be the right thing to do, but then please add a comment explaining why and define a mask with a name showing what those lower 8 bits mean. > > + writel_relaxed(value, plda->bridge_addr + PCIE_PCI_IDS_DW1); > > +} > > + > > +static inline void plda_pcie_set_pref_win_64bit(struct plda_pcie_rp *plda) > > +{ > > + u32 value; > > + > > + value = readl_relaxed(plda->bridge_addr + PCIE_WINROM); > > + value |= PREF_MEM_WIN_64_SUPPORT; > > + writel_relaxed(value, plda->bridge_addr + PCIE_WINROM); > > +} > > + > > +static inline void plda_pcie_disable_ltr(struct plda_pcie_rp *plda) > > +{ > > + u32 value; > > + > > + value = readl_relaxed(plda->bridge_addr + PMSG_SUPPORT_RX); > > + value &= ~PMSG_LTR_SUPPORT; > > + writel_relaxed(value, plda->bridge_addr + PMSG_SUPPORT_RX); > > +} > > + > > +static inline void plda_pcie_disable_func(struct plda_pcie_rp *plda) > > +{ > > + u32 value; > > + > > + value = readl_relaxed(plda->bridge_addr + PCI_MISC); > > + value |= PHY_FUNCTION_DIS; > > + writel_relaxed(value, plda->bridge_addr + PCI_MISC); > > +} > > + > > +static inline void plda_pcie_write_rc_bar(struct plda_pcie_rp *plda, u64 val) > > +{ > > + void __iomem *addr = plda->bridge_addr + CONFIG_SPACE_ADDR; > > + > > + writel_relaxed(val & 0xffffffff, addr + PCI_BASE_ADDRESS_0); > > + writel_relaxed(val >> 32, addr + PCI_BASE_ADDRESS_1); > > Consider using lower_32_bits(val) and upper_32_bits(val) here. > > > +} > > +#endif /* _PCIE_PLDA_H */ > > diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c > > new file mode 100644 > > index 000000000000..42099fef0d7d > > --- /dev/null > > +++ b/drivers/pci/controller/plda/pcie-starfive.c > > @@ -0,0 +1,461 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * PCIe host controller driver for StarFive JH7110 Soc. > > + * > > + * Copyright (C) 2023 StarFive Technology Co., Ltd. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/module.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > +#include <linux/of_pci.h> > > +#include <linux/pci.h> > > +#include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/regmap.h> > > +#include <linux/reset.h> > > +#include "../../pci.h" > > + > > +#include "pcie-plda.h" > > + > > +#define PCIE_FUNC_NUM 4 > > + > > +/* system control */ > > +#define STG_SYSCON_PCIE0_BASE 0x48 > > +#define STG_SYSCON_PCIE1_BASE 0x1f8 > > + > > +#define STG_SYSCON_ARFUNC_OFFSET 0x78 > > +#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK GENMASK(22, 8) > > +#define STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(x) FIELD_PREP(GENMASK(20, 17), x) > > +#define STG_SYSCON_AWFUNC_OFFSET 0x7c > > +#define STG_SYSCON_AXI4_SLVL_AWFUNC_MASK GENMASK(14, 0) > > +#define STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(x) FIELD_PREP(GENMASK(12, 9), x) > > +#define STG_SYSCON_CLKREQ BIT(22) > > +#define STG_SYSCON_CKREF_SRC_MASK GENMASK(19, 18) > > +#define STG_SYSCON_RP_NEP_OFFSET 0xe8 > > +#define STG_SYSCON_K_RP_NEP BIT(8) > > +#define STG_SYSCON_LNKSTA_OFFSET 0x170 > > +#define DATA_LINK_ACTIVE BIT(5) > > + > > +/* Parameters for the waiting for link up routine */ > > +#define LINK_WAIT_MAX_RETRIES 10 > > +#define LINK_WAIT_USLEEP_MIN 90000 > > +#define LINK_WAIT_USLEEP_MAX 100000 > > + > > +struct starfive_jh7110_pcie { > > + struct plda_pcie_rp plda; > > + struct reset_control *resets; > > + struct clk_bulk_data *clks; > > + struct regmap *reg_syscon; > > + struct gpio_desc *power_gpio; > > + struct gpio_desc *reset_gpio; > > + > > + u32 stg_arfun; > > + u32 stg_awfun; > > + u32 stg_rp_nep; > > + u32 stg_lnksta; > > + > > + int num_clks; > > +}; > > + > > +/* > > + * The BAR0/1 of bridge should be hidden during enumeration to > > + * avoid the sizing and resource allocation by PCIe core. > > + */ > > +static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int devfn, > > + int offset) > > +{ > > + if (pci_is_root_bus(bus) && !devfn && > > + (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1)) > > + return true; > > + > > + return false; > > +} > > + > > +static int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn, > > + int where, int size, u32 value) > > +{ > > + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > + > > + return pci_generic_config_write(bus, devfn, where, size, value); > > +} > > + > > +static int starfive_pcie_config_read(struct pci_bus *bus, unsigned int devfn, > > + int where, int size, u32 *value) > > +{ > > + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > + > > + return pci_generic_config_read(bus, devfn, where, size, value); > > +} > > + > > +static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie, struct device *dev) > > +{ > > + int domain_nr, stg_pcie_base; > > + > > + pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks); > > + if (pcie->num_clks < 0) > > + return dev_err_probe(dev, -ENODEV, > > + "failed to get pcie clocks\n"); > > + > > + pcie->resets = devm_reset_control_array_get_exclusive(dev); > > + if (IS_ERR(pcie->resets)) > > + return dev_err_probe(dev, PTR_ERR(pcie->resets), > > + "failed to get pcie resets"); > > + > > + pcie->reg_syscon = > > + syscon_regmap_lookup_by_phandle(dev->of_node, > > + "starfive,stg-syscon"); > > + > > + if (IS_ERR(pcie->reg_syscon)) > > + return dev_err_probe(dev, PTR_ERR(pcie->reg_syscon), > > + "failed to parse starfive,stg-syscon\n"); > > + > > + domain_nr = of_get_pci_domain_nr(dev->of_node); > > + > > + if (domain_nr < 0 || domain_nr > 1) > > + return dev_err_probe(dev, -ENODEV, > > + "failed to get valid pcie id\n"); > > + > > + if (domain_nr == 0) > > + stg_pcie_base = STG_SYSCON_PCIE0_BASE; > > + else > > + stg_pcie_base = STG_SYSCON_PCIE1_BASE; > > It seems you only need the domain_nr for the determining this offset into the > stg_syscon. Since this is already specific to the JH7110 SoC, wouldn't it be > safer to match on the 0x2b000000/0x2c000000 or 0x940000000/0x9c0000000 > address of the peripheral instead? > > > + > > + pcie->stg_arfun = stg_pcie_base + STG_SYSCON_ARFUNC_OFFSET; > > + pcie->stg_awfun = stg_pcie_base + STG_SYSCON_AWFUNC_OFFSET; > > + pcie->stg_rp_nep = stg_pcie_base + STG_SYSCON_RP_NEP_OFFSET; > > + pcie->stg_lnksta = stg_pcie_base + STG_SYSCON_LNKSTA_OFFSET; > > These offsets are all the same for both pcie0 and pcie1 so why do you need a > variable for each? Eg. why not just > > if (domain_nr == 0) > pcie->stg_base = STG_SYSCON_PCIE0_BASE; > else > pcie->stg_base = STG_SYSCON_PCIE1_BASE; > > ..and then add add those static offsets where needed? > > > + > > + pcie->reset_gpio = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); > > + if (IS_ERR(pcie->reset_gpio)) { > > + dev_warn(dev, "failed to get perst-gpio\n"); > > Why is this a warn when you actually error out? I think you mean > > return dev_err_probe(dev, PTR_ERR(pcie->reset_gpio, "failed to get > reset GPIO\n"); > > > + return -EINVAL; > > + } > > + > > + pcie->power_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW); > > + if (IS_ERR(pcie->power_gpio)) { > > + dev_warn(dev, "failed to get power-gpio\n"); > > + return -EINVAL; > > return dev_err_probe(dev, PTR_ERR(pcie->power_gpio), "failed to get > power GPIO\n"); > > > + } > > + > > + return 0; > > +} > > + > > +static struct pci_ops starfive_pcie_ops = { > > + .map_bus = plda_pcie_map_bus, > > + .read = starfive_pcie_config_read, > > + .write = starfive_pcie_config_write, > > +}; > > + > > +static int starfive_pcie_clk_rst_init(struct starfive_jh7110_pcie *pcie) > > +{ > > + int ret; > > + struct device *dev = pcie->plda.dev; > > Again, please order initialized variables first. > > > + > > + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); > > + if (ret) { > > + dev_err(dev, "failed to enable clocks\n"); > > + return ret; > > return dev_err_probe(dev, ret, "failed to enable clocks\n"); > > > + } > > + > > + ret = reset_control_deassert(pcie->resets); > > + if (ret) { > > + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > > + dev_err(dev, "failed to resets\n"); > > I think you mean > return dev_err_probe(dev, ret, "failed to deassert resets\n"); > > > + } > > + > > + return ret; > > +} > > + > > +static void starfive_pcie_clk_rst_deinit(struct starfive_jh7110_pcie *pcie) > > +{ > > + reset_control_assert(pcie->resets); > > + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > > +} > > + > > +static bool starfive_pcie_link_up(struct plda_pcie_rp *plda) > > +{ > > + struct starfive_jh7110_pcie *pcie = > > + container_of(plda, struct starfive_jh7110_pcie, plda); > > + struct device *dev = pcie->plda.dev; > > This is only used once below, so just use pcie->plda.dev directly there. > > > + int ret; > > + u32 stg_reg_val; > > + > > + ret = regmap_read(pcie->reg_syscon, pcie->stg_lnksta, &stg_reg_val); > > + if (ret) { > > + dev_err(dev, "failed to read link status\n"); > > + return false; > > + } > > + > > + if (stg_reg_val & DATA_LINK_ACTIVE) > > + return true; > > + > > + return false; > > These last 4 lines is just "return stg_reg_val & DATA_LINK_ACTIVE;" right? > > > +} > > + > > +static int starfive_pcie_host_wait_for_link(struct starfive_jh7110_pcie *pcie) > > +{ > > + struct device *dev = pcie->plda.dev; > > Only used once, just inline. > > > + int retries; > > + > > + /* Check if the link is up or not */ > > + for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { > > + if (starfive_pcie_link_up(&pcie->plda)) { > > + dev_info(dev, "port link up\n"); > > + return 0; > > + } > > + usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > > + } > > + > > + return -ETIMEDOUT; > > +} > > + > > +static int starfive_pcie_enable_phy(struct device *dev, struct plda_pcie_rp *pcie) > > +{ > > + int ret; > > + > > + if (!pcie->phy) > > + return 0; > > + > > + ret = phy_init(pcie->phy); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "failed to initialize pcie phy\n"); > > + > > + ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE); > > + if (ret) { > > + dev_err(dev, "failed to set pcie mode\n"); > > + goto err_phy_on; > > + } > > + > > + ret = phy_power_on(pcie->phy); > > + if (ret) { > > + dev_err(dev, "failed to power on pcie phy\n"); > > + goto err_phy_on; > > + } > > + > > + return 0; > > + > > +err_phy_on: > > + phy_exit(pcie->phy); > > + return ret; > > +} > > + > > +static void starfive_pcie_disable_phy(struct plda_pcie_rp *pcie) > > +{ > > + phy_power_off(pcie->phy); > > + phy_exit(pcie->phy); > > +} > > + > > +static void starfive_pcie_host_deinit(struct plda_pcie_rp *plda) > > +{ > > + struct starfive_jh7110_pcie *pcie = > > + container_of(plda, struct starfive_jh7110_pcie, plda); > > + > > + starfive_pcie_clk_rst_deinit(pcie); > > + if (pcie->power_gpio) > > + gpiod_set_value_cansleep(pcie->power_gpio, 0); > > + starfive_pcie_disable_phy(plda); > > +} > > + > > +static int starfive_pcie_host_init(struct plda_pcie_rp *plda) > > +{ > > + int i; > > + struct starfive_jh7110_pcie *pcie = > > + container_of(plda, struct starfive_jh7110_pcie, plda); > > + struct device *dev = plda->dev; > > + int ret; > > Weird ordering again. > > > + > > + ret = starfive_pcie_enable_phy(dev, plda); > > + if (ret) > > + return ret; > > + > > + regmap_update_bits(pcie->reg_syscon, pcie->stg_rp_nep, > > + STG_SYSCON_K_RP_NEP, STG_SYSCON_K_RP_NEP); > > + > > + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > > + STG_SYSCON_CKREF_SRC_MASK, > > + FIELD_PREP(STG_SYSCON_CKREF_SRC_MASK, 2)); > > + > > + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > > + STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ); > > + > > + ret = starfive_pcie_clk_rst_init(pcie); > > + if (ret) > > + return ret; > > + > > + if (pcie->power_gpio) > > + gpiod_set_value_cansleep(pcie->power_gpio, 1); > > + > > + if (pcie->reset_gpio) > > + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > > + > > + /* Disable physical functions except #0 */ > > + for (i = 1; i < PCIE_FUNC_NUM; i++) { > > + regmap_update_bits(pcie->reg_syscon, > > + pcie->stg_arfun, > > + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, > > + STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(i)); > > + > > + regmap_update_bits(pcie->reg_syscon, > > + pcie->stg_awfun, > > + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, > > + STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(i)); > > + > > + plda_pcie_disable_func(plda); > > + } > > + > > + regmap_update_bits(pcie->reg_syscon, pcie->stg_arfun, > > + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, 0); > > + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > > + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, 0); > > + > > + plda_pcie_enable_root_port(plda); > > + plda_pcie_write_rc_bar(plda, 0); > > + > > + /* PCIe PCI Standard Configuration Identification Settings. */ > > + plda_pcie_set_standard_class(plda); > > + > > + /* > > + * The LTR message forwarding of PCIe Message Reception was set by core > > + * as default, but the forward id & addr are also need to be reset. > > + * If we do not disable LTR message forwarding here, or set a legal > > + * forwarding address, the kernel will get stuck after this driver probe. > > + * To workaround, disable the LTR message forwarding support on > > + * PCIe Message Reception. > > + */ > > + plda_pcie_disable_ltr(plda); > > + > > + /* Prefetchable memory window 64-bit addressing support */ > > + plda_pcie_set_pref_win_64bit(plda); > > + > > + /* Ensure that PERST has been asserted for at least 100 ms, > > + * the sleep value is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4) > > + */ > > + msleep(100); > > + if (pcie->reset_gpio) > > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > > + > > + /* As the requirement in PCIe base spec r6.0, system (<=5GT/s) must > > + * wait a minimum of 100 ms following exit from a conventional reset > > + * before sending a configuration request to the device. > > + */ > > + msleep(100); > > + > > + if (starfive_pcie_host_wait_for_link(pcie)) > > + dev_info(dev, "port link down\n"); > > + > > + return ret; > > +} > > + > > +static const struct plda_pcie_host_ops sf_host_ops = { > > + .host_init = starfive_pcie_host_init, > > + .host_deinit = starfive_pcie_host_deinit, > > +}; > > + > > +static int starfive_pcie_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct starfive_jh7110_pcie *pcie; > > + struct plda_pcie_rp *plda; > > + int ret; > > + > > + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > > + if (!pcie) > > + return -ENOMEM; > > + > > + plda = &pcie->plda; > > + plda->dev = dev; > > + > > + ret = starfive_pcie_parse_dt(pcie, dev); > > + if (ret) > > + return ret; > > + > > + plda->host_ops = &sf_host_ops; > > + plda->num_events = NUM_PLDA_EVENTS; > > + ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops); > > + if (ret) > > + return ret; > > + > > + pm_runtime_enable(&pdev->dev); > > + pm_runtime_get_sync(&pdev->dev); > > + platform_set_drvdata(pdev, pcie); > > + > > + return 0; > > +} > > + > > +static void starfive_pcie_remove(struct platform_device *pdev) > > +{ > > + struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev); > > + > > + plda_pcie_host_deinit(&pcie->plda); > > + platform_set_drvdata(pdev, NULL); > > +} > > + > > +static int starfive_pcie_suspend_noirq(struct device *dev) > > +{ > > + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > > + > > + if (!pcie) > > + return 0; > > + > > + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > > + starfive_pcie_disable_phy(&pcie->plda); > > + > > + return 0; > > +} > > + > > +static int starfive_pcie_resume_noirq(struct device *dev) > > +{ > > + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = starfive_pcie_enable_phy(dev, &pcie->plda); > > + if (ret) > > + return ret; > > + > > + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); > > + if (ret) { > > + dev_err(dev, "failed to enable clocks\n"); > > + starfive_pcie_disable_phy(&pcie->plda); > > + return ret; > > + } > > + > > + return ret; > > +} > > + > > +static const struct dev_pm_ops starfive_pcie_pm_ops = { > > + NOIRQ_SYSTEM_SLEEP_PM_OPS(starfive_pcie_suspend_noirq, > > + starfive_pcie_resume_noirq) > > +}; > > + > > +static const struct of_device_id starfive_pcie_of_match[] = { > > + { .compatible = "starfive,jh7110-pcie"}, > > Missing space before } > > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, starfive_pcie_of_match); > > + > > +static struct platform_driver starfive_pcie_driver = { > > + .driver = { > > + .name = "pcie-starfive", > > + .of_match_table = of_match_ptr(starfive_pcie_of_match), > > +#ifdef CONFIG_PM_SLEEP > > + .pm = &starfive_pcie_pm_ops, > > +#endif > > This can be just .pm = pm_sleep_ptr(&starfive_pcie_pm_ops), > > > + }, > > + .probe = starfive_pcie_probe, > > + .remove_new = starfive_pcie_remove, > > +}; > > +module_platform_driver(starfive_pcie_driver); > > + > > +MODULE_DESCRIPTION("StarFive JH7110 PCIe host driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.17.1
On 2023/9/18 6:13, Emil Renner Berthing wrote: > Minda Chen wrote: >> Add StarFive JH7110 SoC PCIe controller platform >> driver codes, and add host init/deinit to pcie-plda-host.c >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> Co-developed-by: Kevin Xie <kevin.xie@starfivetech.com> >> Reviewed-by: Mason Huo <mason.huo@starfivetech.com> >> --- >> MAINTAINERS | 7 + >> drivers/pci/controller/plda/Kconfig | 12 + >> drivers/pci/controller/plda/Makefile | 1 + >> drivers/pci/controller/plda/pcie-plda-host.c | 121 ++++- >> drivers/pci/controller/plda/pcie-plda.h | 91 +++- >> drivers/pci/controller/plda/pcie-starfive.c | 461 +++++++++++++++++++ >> 6 files changed, 688 insertions(+), 5 deletions(-) >> create mode 100644 drivers/pci/controller/plda/pcie-starfive.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index ebdb65e195ee..4632cd411a3d 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -16679,6 +16679,13 @@ S: Maintained >> F: Documentation/devicetree/bindings/pci/socionext,uniphier-pcie* >> F: drivers/pci/controller/dwc/pcie-uniphier* >> >> +PCIE DRIVER FOR STARFIVE JH71x0 >> +M: Kevin Xie <kevin.xie@starfivetech.com> >> +L: linux-pci@vger.kernel.org >> +S: Maintained >> +F: Documentation/devicetree/bindings/pci/starfive* >> +F: drivers/pci/controller/plda/pcie-starfive.c >> + >> PCIE DRIVER FOR ST SPEAR13XX >> M: Pratyush Anand <pratyush.anand@gmail.com> >> L: linux-pci@vger.kernel.org >> diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig >> index e54a82ee94f5..8de661730aa5 100644 >> --- a/drivers/pci/controller/plda/Kconfig >> +++ b/drivers/pci/controller/plda/Kconfig >> @@ -15,4 +15,16 @@ config PCIE_MICROCHIP_HOST >> Say Y here if you want kernel to support the Microchip AXI PCIe >> Host Bridge driver. >> >> +config PCIE_STARFIVE_HOST >> + tristate "StarFive PCIe host controller" >> + depends on OF && PCI_MSI >> + select PCIE_PLDA_HOST >> + help >> + Say Y here if you want to support the StarFive PCIe controller >> + in host mode. StarFive PCIe controller uses PLDA PCIe >> + core. >> + If you choose to build this driver as module it will >> + be dynamically linked and module will be called >> + pcie-starfive.ko >> + >> endmenu >> diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile >> index 4340ab007f44..0ac6851bed48 100644 >> --- a/drivers/pci/controller/plda/Makefile >> +++ b/drivers/pci/controller/plda/Makefile >> @@ -1,3 +1,4 @@ >> # SPDX-License-Identifier: GPL-2.0 >> obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o >> obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o >> +obj-$(CONFIG_PCIE_STARFIVE_HOST) += pcie-starfive.o >> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c >> index 26e6cfd2eaba..8b32df477ad8 100644 >> --- a/drivers/pci/controller/plda/pcie-plda-host.c >> +++ b/drivers/pci/controller/plda/pcie-plda-host.c >> @@ -20,6 +20,15 @@ >> >> #include "pcie-plda.h" >> >> +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, >> + int where) >> +{ >> + struct plda_pcie_rp *pcie = bus->sysdata; >> + >> + return pcie->config_base + PCIE_ECAM_OFFSET(bus->number, devfn, where); >> +} >> +EXPORT_SYMBOL_GPL(plda_pcie_map_bus); >> + >> static void plda_handle_msi(struct irq_desc *desc) >> { >> struct plda_pcie_rp *port = irq_desc_get_handler_data(desc); >> @@ -370,6 +379,12 @@ static const struct plda_event_ops plda_event_ops = { >> .get_events = plda_get_events, >> }; >> >> +static const struct plda_event plda_default_event = { >> + .event_ops = &plda_event_ops, >> + .intx_event = EVENT_PM_MSI_INT_INTX, >> + .msi_event = EVENT_PM_MSI_INT_MSI >> +}; >> + >> static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port, >> const struct irq_domain_ops *ops) >> { >> @@ -416,7 +431,6 @@ int plda_init_interrupts(struct platform_device *pdev, >> const struct plda_event *event) >> { >> struct device *dev = &pdev->dev; >> - int irq; >> int i, intx_irq, msi_irq, event_irq; >> int ret; >> const struct irq_domain_ops *irq_dom_ops; >> @@ -434,8 +448,8 @@ int plda_init_interrupts(struct platform_device *pdev, >> return ret; >> } >> >> - irq = platform_get_irq(pdev, 0); >> - if (irq < 0) >> + port->irq = platform_get_irq(pdev, 0); >> + if (port->irq < 0) >> return -ENODEV; > > Here you're dropping the local variable irq and just use port->irq directly.. > >> >> port->event_ops = event->event_ops; >> @@ -465,6 +479,7 @@ int plda_init_interrupts(struct platform_device *pdev, >> dev_err(dev, "failed to map INTx interrupt\n"); >> return -ENXIO; >> } >> + port->intx_irq = intx_irq; > > ..but for intx_irq.. > >> >> /* Plug the INTx chained handler */ >> irq_set_chained_handler_and_data(intx_irq, plda_handle_intx, port); >> @@ -473,12 +488,13 @@ int plda_init_interrupts(struct platform_device *pdev, >> event->msi_event); >> if (!msi_irq) >> return -ENXIO; >> + port->msi_irq = msi_irq; > > ..and msi_irq you're keeping the local variable. I think you should drop > the local variables here too, or at least do the same for all of them. > OK, remove all the local variables is better. >> >> /* Plug the MSI chained handler */ >> irq_set_chained_handler_and_data(msi_irq, plda_handle_msi, port); >> >> /* Plug the main event chained handler */ >> - irq_set_chained_handler_and_data(irq, plda_handle_event, port); >> + irq_set_chained_handler_and_data(port->irq, plda_handle_event, port); >> >> return 0; >> } >> @@ -544,3 +560,100 @@ int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> return 0; >> } >> EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); >> + >> +static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie) >> +{ >> + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL); >> + irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL); >> + irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL); >> + >> + irq_domain_remove(pcie->msi.msi_domain); >> + irq_domain_remove(pcie->msi.dev_domain); >> + >> + irq_domain_remove(pcie->intx_domain); >> + irq_domain_remove(pcie->event_domain); >> +} >> + >> +int plda_pcie_host_init(struct plda_pcie_rp *port, struct pci_ops *ops) >> +{ >> + struct resource *cfg_res; >> + struct device *dev = port->dev; >> + int ret; >> + struct pci_host_bridge *bridge; >> + struct platform_device *pdev = to_platform_device(dev); > > This looks weird. Please order initialized variables first and maybe even do > reverse christmas tree order. Eg. longeist lines first. > OK, The same with other places. >> + >> + port->bridge_addr = >> + devm_platform_ioremap_resource_byname(pdev, "apb"); >> + >> + if (IS_ERR(port->bridge_addr)) >> + return dev_err_probe(dev, PTR_ERR(port->bridge_addr), >> + "failed to map reg memory\n"); >> + >> + cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); >> + if (!cfg_res) >> + return dev_err_probe(dev, -ENODEV, >> + "failed to get config memory\n"); >> + >> + port->config_base = devm_ioremap_resource(dev, cfg_res); >> + if (IS_ERR(port->config_base)) >> + return dev_err_probe(dev, PTR_ERR(port->config_base), >> + "failed to map config memory\n"); >> + >> + port->phy = devm_phy_optional_get(dev, NULL); >> + if (IS_ERR(port->phy)) >> + return dev_err_probe(dev, PTR_ERR(port->phy), >> + "failed to get pcie phy\n"); >> + >> + bridge = devm_pci_alloc_host_bridge(dev, 0); >> + if (!bridge) >> + return dev_err_probe(dev, -ENOMEM, >> + "failed to alloc bridge\n"); >> + >> + if (port->host_ops && port->host_ops->host_init) { >> + ret = port->host_ops->host_init(port); >> + if (ret) >> + return ret; >> + } >> + >> + port->bridge = bridge; >> + plda_pcie_setup_window(port->bridge_addr, 0, cfg_res->start, 0, >> + resource_size(cfg_res)); >> + plda_pcie_setup_iomems(bridge, port); >> + plda_set_default_msi(&port->msi); >> + ret = plda_init_interrupts(pdev, port, &plda_default_event); >> + if (ret) >> + goto err_host; >> + >> + /* Set default bus ops */ >> + bridge->ops = ops; >> + bridge->sysdata = port; >> + >> + ret = pci_host_probe(bridge); >> + if (ret < 0) { >> + dev_err(dev, "failed to pci host probe: %d\n", ret); > > I think you mean: > > dev_err_probe(dev, ret, "failed to probe pci host\n"); > I check with dev_err_probe, It is better than dev_err, I will replace this. >> + goto err_probe; >> + } >> + >> + return ret; >> + >> +err_probe: >> + plda_pcie_irq_domain_deinit(port); >> +err_host: >> + if (port->host_ops && port->host_ops->host_deinit) >> + port->host_ops->host_deinit(port); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(plda_pcie_host_init); >> + >> +void plda_pcie_host_deinit(struct plda_pcie_rp *port) >> +{ >> + pci_stop_root_bus(port->bridge->bus); >> + pci_remove_root_bus(port->bridge->bus); >> + >> + plda_pcie_irq_domain_deinit(port); >> + >> + if (port->host_ops && port->host_ops->host_deinit) >> + port->host_ops->host_deinit(port); >> +} >> +EXPORT_SYMBOL_GPL(plda_pcie_host_deinit); >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h >> index b7420598a326..e571047d7944 100644 >> --- a/drivers/pci/controller/plda/pcie-plda.h >> +++ b/drivers/pci/controller/plda/pcie-plda.h >> @@ -6,14 +6,24 @@ >> #ifndef _PCIE_PLDA_H >> #define _PCIE_PLDA_H >> >> +#include <linux/phy/phy.h> >> + >> /* Number of MSI IRQs */ >> #define PLDA_MAX_NUM_MSI_IRQS 32 >> >> /* PCIe Bridge Phy Regs */ >> +#define GEN_SETTINGS 0x80 >> +#define RP_ENABLE 1 >> +#define PCIE_PCI_IDS_DW1 0x9c >> +#define IDS_CLASS_CODE_SHIFT 16 >> #define PCIE_PCI_IRQ_DW0 0xa8 >> #define MSIX_CAP_MASK BIT(31) >> #define NUM_MSI_MSGS_MASK GENMASK(6, 4) >> #define NUM_MSI_MSGS_SHIFT 4 >> +#define PCI_MISC 0xb4 >> +#define PHY_FUNCTION_DIS BIT(15) >> +#define PCIE_WINROM 0xfc >> +#define PREF_MEM_WIN_64_SUPPORT BIT(3) >> >> #define IMASK_LOCAL 0x180 >> #define DMA_END_ENGINE_0_MASK 0x00000000u >> @@ -64,6 +74,8 @@ >> #define ISTATUS_HOST 0x18c >> #define IMSI_ADDR 0x190 >> #define ISTATUS_MSI 0x194 >> +#define PMSG_SUPPORT_RX 0x3f0 >> +#define PMSG_LTR_SUPPORT BIT(2) >> >> /* PCIe Master table init defines */ >> #define ATR0_PCIE_WIN0_SRCADDR_PARAM 0x600u >> @@ -85,6 +97,8 @@ >> #define PCIE_TX_RX_INTERFACE 0x00000000u >> #define PCIE_CONFIG_INTERFACE 0x00000001u >> >> +#define CONFIG_SPACE_ADDR 0x1000u >> + >> #define ATR_ENTRY_SIZE 32 >> >> #define EVENT_A_ATR_EVT_POST_ERR 0 >> @@ -134,6 +148,11 @@ struct plda_event_ops { >> u32 (*get_events)(struct plda_pcie_rp *pcie); >> }; >> >> +struct plda_pcie_host_ops { >> + int (*host_init)(struct plda_pcie_rp *pcie); >> + void (*host_deinit)(struct plda_pcie_rp *pcie); >> +}; >> + >> struct plda_msi { >> struct mutex lock; /* Protect used bitmap */ >> struct irq_domain *msi_domain; >> @@ -145,12 +164,19 @@ struct plda_msi { >> >> struct plda_pcie_rp { >> struct device *dev; >> + struct pci_host_bridge *bridge; >> struct irq_domain *intx_domain; >> struct irq_domain *event_domain; >> raw_spinlock_t lock; >> struct plda_msi msi; >> const struct plda_event_ops *event_ops; >> + const struct plda_pcie_host_ops *host_ops; >> + struct phy *phy; >> void __iomem *bridge_addr; >> + void __iomem *config_base; >> + int irq; >> + int msi_irq; >> + int intx_irq; >> int num_events; >> }; >> >> @@ -163,6 +189,7 @@ struct plda_event { >> int msi_event; >> }; >> >> +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, int where); >> int plda_init_interrupts(struct platform_device *pdev, >> struct plda_pcie_rp *port, >> const struct plda_event *event); >> @@ -171,4 +198,66 @@ void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> size_t size); >> int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> struct plda_pcie_rp *port); >> -#endif >> +int plda_pcie_host_init(struct plda_pcie_rp *pcie, struct pci_ops *ops); >> +void plda_pcie_host_deinit(struct plda_pcie_rp *pcie); >> + >> +static inline void plda_set_default_msi(struct plda_msi *msi) >> +{ >> + msi->vector_phy = IMSI_ADDR; >> + msi->num_vectors = PLDA_MAX_NUM_MSI_IRQS; >> +} >> + >> +static inline void plda_pcie_enable_root_port(struct plda_pcie_rp *plda) >> +{ >> + u32 value; >> + >> + value = readl_relaxed(plda->bridge_addr + GEN_SETTINGS); >> + value |= RP_ENABLE; >> + writel_relaxed(value, plda->bridge_addr + GEN_SETTINGS); >> +} >> + >> +static inline void plda_pcie_set_standard_class(struct plda_pcie_rp *plda) >> +{ >> + u32 value; >> + >> + value = readl_relaxed(plda->bridge_addr + PCIE_PCI_IDS_DW1); >> + value &= 0xff; >> + value |= (PCI_CLASS_BRIDGE_PCI << IDS_CLASS_CODE_SHIFT); > > This looks suspicious. You're clearing the lower 8 bits, but setting > bits 16 and up. Also the parentheses are not strictly necessary. > I will add comments to this. >> + writel_relaxed(value, plda->bridge_addr + PCIE_PCI_IDS_DW1); >> +} >> + >> +static inline void plda_pcie_set_pref_win_64bit(struct plda_pcie_rp *plda) >> +{ >> + u32 value; >> + >> + value = readl_relaxed(plda->bridge_addr + PCIE_WINROM); >> + value |= PREF_MEM_WIN_64_SUPPORT; >> + writel_relaxed(value, plda->bridge_addr + PCIE_WINROM); >> +} >> + >> +static inline void plda_pcie_disable_ltr(struct plda_pcie_rp *plda) >> +{ >> + u32 value; >> + >> + value = readl_relaxed(plda->bridge_addr + PMSG_SUPPORT_RX); >> + value &= ~PMSG_LTR_SUPPORT; >> + writel_relaxed(value, plda->bridge_addr + PMSG_SUPPORT_RX); >> +} >> + >> +static inline void plda_pcie_disable_func(struct plda_pcie_rp *plda) >> +{ >> + u32 value; >> + >> + value = readl_relaxed(plda->bridge_addr + PCI_MISC); >> + value |= PHY_FUNCTION_DIS; >> + writel_relaxed(value, plda->bridge_addr + PCI_MISC); >> +} >> + >> +static inline void plda_pcie_write_rc_bar(struct plda_pcie_rp *plda, u64 val) >> +{ >> + void __iomem *addr = plda->bridge_addr + CONFIG_SPACE_ADDR; >> + >> + writel_relaxed(val & 0xffffffff, addr + PCI_BASE_ADDRESS_0); >> + writel_relaxed(val >> 32, addr + PCI_BASE_ADDRESS_1); > > Consider using lower_32_bits(val) and upper_32_bits(val) here. > >> +} >> +#endif /* _PCIE_PLDA_H */ >> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c >> new file mode 100644 >> index 000000000000..42099fef0d7d >> --- /dev/null >> +++ b/drivers/pci/controller/plda/pcie-starfive.c >> @@ -0,0 +1,461 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * PCIe host controller driver for StarFive JH7110 Soc. >> + * >> + * Copyright (C) 2023 StarFive Technology Co., Ltd. >> + */ >> + >> +#include <linux/bitfield.h> >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/module.h> >> +#include <linux/of_address.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_pci.h> >> +#include <linux/pci.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/regmap.h> >> +#include <linux/reset.h> >> +#include "../../pci.h" >> + >> +#include "pcie-plda.h" >> + >> +#define PCIE_FUNC_NUM 4 >> + >> +/* system control */ >> +#define STG_SYSCON_PCIE0_BASE 0x48 >> +#define STG_SYSCON_PCIE1_BASE 0x1f8 >> + >> +#define STG_SYSCON_ARFUNC_OFFSET 0x78 >> +#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK GENMASK(22, 8) >> +#define STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(x) FIELD_PREP(GENMASK(20, 17), x) >> +#define STG_SYSCON_AWFUNC_OFFSET 0x7c >> +#define STG_SYSCON_AXI4_SLVL_AWFUNC_MASK GENMASK(14, 0) >> +#define STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(x) FIELD_PREP(GENMASK(12, 9), x) >> +#define STG_SYSCON_CLKREQ BIT(22) >> +#define STG_SYSCON_CKREF_SRC_MASK GENMASK(19, 18) >> +#define STG_SYSCON_RP_NEP_OFFSET 0xe8 >> +#define STG_SYSCON_K_RP_NEP BIT(8) >> +#define STG_SYSCON_LNKSTA_OFFSET 0x170 >> +#define DATA_LINK_ACTIVE BIT(5) >> + >> +/* Parameters for the waiting for link up routine */ >> +#define LINK_WAIT_MAX_RETRIES 10 >> +#define LINK_WAIT_USLEEP_MIN 90000 >> +#define LINK_WAIT_USLEEP_MAX 100000 >> + >> +struct starfive_jh7110_pcie { >> + struct plda_pcie_rp plda; >> + struct reset_control *resets; >> + struct clk_bulk_data *clks; >> + struct regmap *reg_syscon; >> + struct gpio_desc *power_gpio; >> + struct gpio_desc *reset_gpio; >> + >> + u32 stg_arfun; >> + u32 stg_awfun; >> + u32 stg_rp_nep; >> + u32 stg_lnksta; >> + >> + int num_clks; >> +}; >> + >> +/* >> + * The BAR0/1 of bridge should be hidden during enumeration to >> + * avoid the sizing and resource allocation by PCIe core. >> + */ >> +static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int devfn, >> + int offset) >> +{ >> + if (pci_is_root_bus(bus) && !devfn && >> + (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1)) >> + return true; >> + >> + return false; >> +} >> + >> +static int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn, >> + int where, int size, u32 value) >> +{ >> + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) >> + return PCIBIOS_BAD_REGISTER_NUMBER; >> + >> + return pci_generic_config_write(bus, devfn, where, size, value); >> +} >> + >> +static int starfive_pcie_config_read(struct pci_bus *bus, unsigned int devfn, >> + int where, int size, u32 *value) >> +{ >> + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) >> + return PCIBIOS_BAD_REGISTER_NUMBER; >> + >> + return pci_generic_config_read(bus, devfn, where, size, value); >> +} >> + >> +static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie, struct device *dev) >> +{ >> + int domain_nr, stg_pcie_base; >> + >> + pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks); >> + if (pcie->num_clks < 0) >> + return dev_err_probe(dev, -ENODEV, >> + "failed to get pcie clocks\n"); >> + >> + pcie->resets = devm_reset_control_array_get_exclusive(dev); >> + if (IS_ERR(pcie->resets)) >> + return dev_err_probe(dev, PTR_ERR(pcie->resets), >> + "failed to get pcie resets"); >> + >> + pcie->reg_syscon = >> + syscon_regmap_lookup_by_phandle(dev->of_node, >> + "starfive,stg-syscon"); >> + >> + if (IS_ERR(pcie->reg_syscon)) >> + return dev_err_probe(dev, PTR_ERR(pcie->reg_syscon), >> + "failed to parse starfive,stg-syscon\n"); >> + >> + domain_nr = of_get_pci_domain_nr(dev->of_node); >> + >> + if (domain_nr < 0 || domain_nr > 1) >> + return dev_err_probe(dev, -ENODEV, >> + "failed to get valid pcie id\n"); >> + >> + if (domain_nr == 0) >> + stg_pcie_base = STG_SYSCON_PCIE0_BASE; >> + else >> + stg_pcie_base = STG_SYSCON_PCIE1_BASE; > > It seems you only need the domain_nr for the determining this offset into the > stg_syscon. Since this is already specific to the JH7110 SoC, wouldn't it be > safer to match on the 0x2b000000/0x2c000000 or 0x940000000/0x9c0000000 > address of the peripheral instead? > Maybe this codes not just bind to JH7110 SoC. >> + >> + pcie->stg_arfun = stg_pcie_base + STG_SYSCON_ARFUNC_OFFSET; >> + pcie->stg_awfun = stg_pcie_base + STG_SYSCON_AWFUNC_OFFSET; >> + pcie->stg_rp_nep = stg_pcie_base + STG_SYSCON_RP_NEP_OFFSET; >> + pcie->stg_lnksta = stg_pcie_base + STG_SYSCON_LNKSTA_OFFSET; > > These offsets are all the same for both pcie0 and pcie1 so why do you need a > variable for each? Eg. why not just > > if (domain_nr == 0) > pcie->stg_base = STG_SYSCON_PCIE0_BASE; > else > pcie->stg_base = STG_SYSCON_PCIE1_BASE; > > ..and then add add those static offsets where needed? > OK. I will try to modify this. >> + >> + pcie->reset_gpio = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); >> + if (IS_ERR(pcie->reset_gpio)) { >> + dev_warn(dev, "failed to get perst-gpio\n"); > > Why is this a warn when you actually error out? I think you mean > > return dev_err_probe(dev, PTR_ERR(pcie->reset_gpio, "failed to get > reset GPIO\n"); > >> + return -EINVAL; >> + } >> + >> + pcie->power_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW); >> + if (IS_ERR(pcie->power_gpio)) { >> + dev_warn(dev, "failed to get power-gpio\n"); >> + return -EINVAL; > > return dev_err_probe(dev, PTR_ERR(pcie->power_gpio), "failed to get > power GPIO\n"); > >> + } >> + >> + return 0; >> +} >> + >> +static struct pci_ops starfive_pcie_ops = { >> + .map_bus = plda_pcie_map_bus, >> + .read = starfive_pcie_config_read, >> + .write = starfive_pcie_config_write, >> +}; >> + >> +static int starfive_pcie_clk_rst_init(struct starfive_jh7110_pcie *pcie) >> +{ >> + int ret; >> + struct device *dev = pcie->plda.dev; > > Again, please order initialized variables first. > >> + >> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); >> + if (ret) { >> + dev_err(dev, "failed to enable clocks\n"); >> + return ret; > > return dev_err_probe(dev, ret, "failed to enable clocks\n"); > >> + } >> + >> + ret = reset_control_deassert(pcie->resets); >> + if (ret) { >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); >> + dev_err(dev, "failed to resets\n"); > > I think you mean > return dev_err_probe(dev, ret, "failed to deassert resets\n"); > >> + } >> + >> + return ret; >> +} >> + >> +static void starfive_pcie_clk_rst_deinit(struct starfive_jh7110_pcie *pcie) >> +{ >> + reset_control_assert(pcie->resets); >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); >> +} >> + >> +static bool starfive_pcie_link_up(struct plda_pcie_rp *plda) >> +{ >> + struct starfive_jh7110_pcie *pcie = >> + container_of(plda, struct starfive_jh7110_pcie, plda); >> + struct device *dev = pcie->plda.dev; > > This is only used once below, so just use pcie->plda.dev directly there. > OK, thanks >> + int ret; >> + u32 stg_reg_val; >> + >> + ret = regmap_read(pcie->reg_syscon, pcie->stg_lnksta, &stg_reg_val); >> + if (ret) { >> + dev_err(dev, "failed to read link status\n"); >> + return false; >> + } >> + >> + if (stg_reg_val & DATA_LINK_ACTIVE) >> + return true; >> + >> + return false; > > These last 4 lines is just "return stg_reg_val & DATA_LINK_ACTIVE;" right? > OK >> +} >> + >> +static int starfive_pcie_host_wait_for_link(struct starfive_jh7110_pcie *pcie) >> +{ >> + struct device *dev = pcie->plda.dev; > > Only used once, just inline. > >> + int retries; >> + >> + /* Check if the link is up or not */ >> + for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { >> + if (starfive_pcie_link_up(&pcie->plda)) { >> + dev_info(dev, "port link up\n"); >> + return 0; >> + } >> + usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); >> + } >> + >> + return -ETIMEDOUT; >> +} >> + >> +static int starfive_pcie_enable_phy(struct device *dev, struct plda_pcie_rp *pcie) >> +{ >> + int ret; >> + >> + if (!pcie->phy) >> + return 0; >> + >> + ret = phy_init(pcie->phy); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "failed to initialize pcie phy\n"); >> + >> + ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE); >> + if (ret) { >> + dev_err(dev, "failed to set pcie mode\n"); >> + goto err_phy_on; >> + } >> + >> + ret = phy_power_on(pcie->phy); >> + if (ret) { >> + dev_err(dev, "failed to power on pcie phy\n"); >> + goto err_phy_on; >> + } >> + >> + return 0; >> + >> +err_phy_on: >> + phy_exit(pcie->phy); >> + return ret; >> +} >> + >> +static void starfive_pcie_disable_phy(struct plda_pcie_rp *pcie) >> +{ >> + phy_power_off(pcie->phy); >> + phy_exit(pcie->phy); >> +} >> + >> +static void starfive_pcie_host_deinit(struct plda_pcie_rp *plda) >> +{ >> + struct starfive_jh7110_pcie *pcie = >> + container_of(plda, struct starfive_jh7110_pcie, plda); >> + >> + starfive_pcie_clk_rst_deinit(pcie); >> + if (pcie->power_gpio) >> + gpiod_set_value_cansleep(pcie->power_gpio, 0); >> + starfive_pcie_disable_phy(plda); >> +} >> + >> +static int starfive_pcie_host_init(struct plda_pcie_rp *plda) >> +{ >> + int i; >> + struct starfive_jh7110_pcie *pcie = >> + container_of(plda, struct starfive_jh7110_pcie, plda); >> + struct device *dev = plda->dev; >> + int ret; > > Weird ordering again. > >> + >> + ret = starfive_pcie_enable_phy(dev, plda); >> + if (ret) >> + return ret; >> + >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_rp_nep, >> + STG_SYSCON_K_RP_NEP, STG_SYSCON_K_RP_NEP); >> + >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, >> + STG_SYSCON_CKREF_SRC_MASK, >> + FIELD_PREP(STG_SYSCON_CKREF_SRC_MASK, 2)); >> + >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, >> + STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ); >> + >> + ret = starfive_pcie_clk_rst_init(pcie); >> + if (ret) >> + return ret; >> + >> + if (pcie->power_gpio) >> + gpiod_set_value_cansleep(pcie->power_gpio, 1); >> + >> + if (pcie->reset_gpio) >> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); >> + >> + /* Disable physical functions except #0 */ >> + for (i = 1; i < PCIE_FUNC_NUM; i++) { >> + regmap_update_bits(pcie->reg_syscon, >> + pcie->stg_arfun, >> + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, >> + STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(i)); >> + >> + regmap_update_bits(pcie->reg_syscon, >> + pcie->stg_awfun, >> + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, >> + STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(i)); >> + >> + plda_pcie_disable_func(plda); >> + } >> + >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_arfun, >> + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, 0); >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, >> + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, 0); >> + >> + plda_pcie_enable_root_port(plda); >> + plda_pcie_write_rc_bar(plda, 0); >> + >> + /* PCIe PCI Standard Configuration Identification Settings. */ >> + plda_pcie_set_standard_class(plda); >> + >> + /* >> + * The LTR message forwarding of PCIe Message Reception was set by core >> + * as default, but the forward id & addr are also need to be reset. >> + * If we do not disable LTR message forwarding here, or set a legal >> + * forwarding address, the kernel will get stuck after this driver probe. >> + * To workaround, disable the LTR message forwarding support on >> + * PCIe Message Reception. >> + */ >> + plda_pcie_disable_ltr(plda); >> + >> + /* Prefetchable memory window 64-bit addressing support */ >> + plda_pcie_set_pref_win_64bit(plda); >> + >> + /* Ensure that PERST has been asserted for at least 100 ms, >> + * the sleep value is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4) >> + */ >> + msleep(100); >> + if (pcie->reset_gpio) >> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); >> + >> + /* As the requirement in PCIe base spec r6.0, system (<=5GT/s) must >> + * wait a minimum of 100 ms following exit from a conventional reset >> + * before sending a configuration request to the device. >> + */ >> + msleep(100); >> + >> + if (starfive_pcie_host_wait_for_link(pcie)) >> + dev_info(dev, "port link down\n"); >> + >> + return ret; >> +} >> + >> +static const struct plda_pcie_host_ops sf_host_ops = { >> + .host_init = starfive_pcie_host_init, >> + .host_deinit = starfive_pcie_host_deinit, >> +}; >> + >> +static int starfive_pcie_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct starfive_jh7110_pcie *pcie; >> + struct plda_pcie_rp *plda; >> + int ret; >> + >> + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); >> + if (!pcie) >> + return -ENOMEM; >> + >> + plda = &pcie->plda; >> + plda->dev = dev; >> + >> + ret = starfive_pcie_parse_dt(pcie, dev); >> + if (ret) >> + return ret; >> + >> + plda->host_ops = &sf_host_ops; >> + plda->num_events = NUM_PLDA_EVENTS; >> + ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops); >> + if (ret) >> + return ret; >> + >> + pm_runtime_enable(&pdev->dev); >> + pm_runtime_get_sync(&pdev->dev); >> + platform_set_drvdata(pdev, pcie); >> + >> + return 0; >> +} >> + >> +static void starfive_pcie_remove(struct platform_device *pdev) >> +{ >> + struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev); >> + >> + plda_pcie_host_deinit(&pcie->plda); >> + platform_set_drvdata(pdev, NULL); >> +} >> + >> +static int starfive_pcie_suspend_noirq(struct device *dev) >> +{ >> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); >> + >> + if (!pcie) >> + return 0; >> + >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); >> + starfive_pcie_disable_phy(&pcie->plda); >> + >> + return 0; >> +} >> + >> +static int starfive_pcie_resume_noirq(struct device *dev) >> +{ >> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = starfive_pcie_enable_phy(dev, &pcie->plda); >> + if (ret) >> + return ret; >> + >> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); >> + if (ret) { >> + dev_err(dev, "failed to enable clocks\n"); >> + starfive_pcie_disable_phy(&pcie->plda); >> + return ret; >> + } >> + >> + return ret; >> +} >> + >> +static const struct dev_pm_ops starfive_pcie_pm_ops = { >> + NOIRQ_SYSTEM_SLEEP_PM_OPS(starfive_pcie_suspend_noirq, >> + starfive_pcie_resume_noirq) >> +}; >> + >> +static const struct of_device_id starfive_pcie_of_match[] = { >> + { .compatible = "starfive,jh7110-pcie"}, > > Missing space before } > >> + { /* sentinel */ } >> +}; >> +MODULE_DEVICE_TABLE(of, starfive_pcie_of_match); >> + >> +static struct platform_driver starfive_pcie_driver = { >> + .driver = { >> + .name = "pcie-starfive", >> + .of_match_table = of_match_ptr(starfive_pcie_of_match), >> +#ifdef CONFIG_PM_SLEEP >> + .pm = &starfive_pcie_pm_ops, >> +#endif > > This can be just .pm = pm_sleep_ptr(&starfive_pcie_pm_ops), > OK >> + }, >> + .probe = starfive_pcie_probe, >> + .remove_new = starfive_pcie_remove, >> +}; >> +module_platform_driver(starfive_pcie_driver); >> + >> +MODULE_DESCRIPTION("StarFive JH7110 PCIe host driver"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.17.1
Minda Chen wrote: > > > On 2023/9/18 6:13, Emil Renner Berthing wrote: > > Minda Chen wrote: > >> Add StarFive JH7110 SoC PCIe controller platform > >> driver codes, and add host init/deinit to pcie-plda-host.c > >> > >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > >> Co-developed-by: Kevin Xie <kevin.xie@starfivetech.com> > >> Reviewed-by: Mason Huo <mason.huo@starfivetech.com> > >> --- > >> MAINTAINERS | 7 + > >> drivers/pci/controller/plda/Kconfig | 12 + > >> drivers/pci/controller/plda/Makefile | 1 + > >> drivers/pci/controller/plda/pcie-plda-host.c | 121 ++++- > >> drivers/pci/controller/plda/pcie-plda.h | 91 +++- > >> drivers/pci/controller/plda/pcie-starfive.c | 461 +++++++++++++++++++ > >> 6 files changed, 688 insertions(+), 5 deletions(-) > >> create mode 100644 drivers/pci/controller/plda/pcie-starfive.c > >> > >> diff --git a/MAINTAINERS b/MAINTAINERS > >> index ebdb65e195ee..4632cd411a3d 100644 > >> --- a/MAINTAINERS > >> +++ b/MAINTAINERS > >> @@ -16679,6 +16679,13 @@ S: Maintained > >> F: Documentation/devicetree/bindings/pci/socionext,uniphier-pcie* > >> F: drivers/pci/controller/dwc/pcie-uniphier* > >> > >> +PCIE DRIVER FOR STARFIVE JH71x0 > >> +M: Kevin Xie <kevin.xie@starfivetech.com> > >> +L: linux-pci@vger.kernel.org > >> +S: Maintained > >> +F: Documentation/devicetree/bindings/pci/starfive* > >> +F: drivers/pci/controller/plda/pcie-starfive.c > >> + > >> PCIE DRIVER FOR ST SPEAR13XX > >> M: Pratyush Anand <pratyush.anand@gmail.com> > >> L: linux-pci@vger.kernel.org > >> diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig > >> index e54a82ee94f5..8de661730aa5 100644 > >> --- a/drivers/pci/controller/plda/Kconfig > >> +++ b/drivers/pci/controller/plda/Kconfig > >> @@ -15,4 +15,16 @@ config PCIE_MICROCHIP_HOST > >> Say Y here if you want kernel to support the Microchip AXI PCIe > >> Host Bridge driver. > >> > >> +config PCIE_STARFIVE_HOST > >> + tristate "StarFive PCIe host controller" > >> + depends on OF && PCI_MSI > >> + select PCIE_PLDA_HOST > >> + help > >> + Say Y here if you want to support the StarFive PCIe controller > >> + in host mode. StarFive PCIe controller uses PLDA PCIe > >> + core. > >> + If you choose to build this driver as module it will > >> + be dynamically linked and module will be called > >> + pcie-starfive.ko > >> + > >> endmenu > >> diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile > >> index 4340ab007f44..0ac6851bed48 100644 > >> --- a/drivers/pci/controller/plda/Makefile > >> +++ b/drivers/pci/controller/plda/Makefile > >> @@ -1,3 +1,4 @@ > >> # SPDX-License-Identifier: GPL-2.0 > >> obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o > >> obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o > >> +obj-$(CONFIG_PCIE_STARFIVE_HOST) += pcie-starfive.o > >> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c > >> index 26e6cfd2eaba..8b32df477ad8 100644 > >> --- a/drivers/pci/controller/plda/pcie-plda-host.c > >> +++ b/drivers/pci/controller/plda/pcie-plda-host.c > >> @@ -20,6 +20,15 @@ > >> > >> #include "pcie-plda.h" > >> > >> +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, > >> + int where) > >> +{ > >> + struct plda_pcie_rp *pcie = bus->sysdata; > >> + > >> + return pcie->config_base + PCIE_ECAM_OFFSET(bus->number, devfn, where); > >> +} > >> +EXPORT_SYMBOL_GPL(plda_pcie_map_bus); > >> + > >> static void plda_handle_msi(struct irq_desc *desc) > >> { > >> struct plda_pcie_rp *port = irq_desc_get_handler_data(desc); > >> @@ -370,6 +379,12 @@ static const struct plda_event_ops plda_event_ops = { > >> .get_events = plda_get_events, > >> }; > >> > >> +static const struct plda_event plda_default_event = { > >> + .event_ops = &plda_event_ops, > >> + .intx_event = EVENT_PM_MSI_INT_INTX, > >> + .msi_event = EVENT_PM_MSI_INT_MSI > >> +}; > >> + > >> static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port, > >> const struct irq_domain_ops *ops) > >> { > >> @@ -416,7 +431,6 @@ int plda_init_interrupts(struct platform_device *pdev, > >> const struct plda_event *event) > >> { > >> struct device *dev = &pdev->dev; > >> - int irq; > >> int i, intx_irq, msi_irq, event_irq; > >> int ret; > >> const struct irq_domain_ops *irq_dom_ops; > >> @@ -434,8 +448,8 @@ int plda_init_interrupts(struct platform_device *pdev, > >> return ret; > >> } > >> > >> - irq = platform_get_irq(pdev, 0); > >> - if (irq < 0) > >> + port->irq = platform_get_irq(pdev, 0); > >> + if (port->irq < 0) > >> return -ENODEV; > > > > Here you're dropping the local variable irq and just use port->irq directly.. > > > >> > >> port->event_ops = event->event_ops; > >> @@ -465,6 +479,7 @@ int plda_init_interrupts(struct platform_device *pdev, > >> dev_err(dev, "failed to map INTx interrupt\n"); > >> return -ENXIO; > >> } > >> + port->intx_irq = intx_irq; > > > > ..but for intx_irq.. > > > >> > >> /* Plug the INTx chained handler */ > >> irq_set_chained_handler_and_data(intx_irq, plda_handle_intx, port); > >> @@ -473,12 +488,13 @@ int plda_init_interrupts(struct platform_device *pdev, > >> event->msi_event); > >> if (!msi_irq) > >> return -ENXIO; > >> + port->msi_irq = msi_irq; > > > > ..and msi_irq you're keeping the local variable. I think you should drop > > the local variables here too, or at least do the same for all of them. > > > OK, remove all the local variables is better. > >> > >> /* Plug the MSI chained handler */ > >> irq_set_chained_handler_and_data(msi_irq, plda_handle_msi, port); > >> > >> /* Plug the main event chained handler */ > >> - irq_set_chained_handler_and_data(irq, plda_handle_event, port); > >> + irq_set_chained_handler_and_data(port->irq, plda_handle_event, port); > >> > >> return 0; > >> } > >> @@ -544,3 +560,100 @@ int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > >> return 0; > >> } > >> EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); > >> + > >> +static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie) > >> +{ > >> + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL); > >> + irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL); > >> + irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL); > >> + > >> + irq_domain_remove(pcie->msi.msi_domain); > >> + irq_domain_remove(pcie->msi.dev_domain); > >> + > >> + irq_domain_remove(pcie->intx_domain); > >> + irq_domain_remove(pcie->event_domain); > >> +} > >> + > >> +int plda_pcie_host_init(struct plda_pcie_rp *port, struct pci_ops *ops) > >> +{ > >> + struct resource *cfg_res; > >> + struct device *dev = port->dev; > >> + int ret; > >> + struct pci_host_bridge *bridge; > >> + struct platform_device *pdev = to_platform_device(dev); > > > > This looks weird. Please order initialized variables first and maybe even do > > reverse christmas tree order. Eg. longeist lines first. > > > OK, The same with other places. > >> + > >> + port->bridge_addr = > >> + devm_platform_ioremap_resource_byname(pdev, "apb"); > >> + > >> + if (IS_ERR(port->bridge_addr)) > >> + return dev_err_probe(dev, PTR_ERR(port->bridge_addr), > >> + "failed to map reg memory\n"); > >> + > >> + cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); > >> + if (!cfg_res) > >> + return dev_err_probe(dev, -ENODEV, > >> + "failed to get config memory\n"); > >> + > >> + port->config_base = devm_ioremap_resource(dev, cfg_res); > >> + if (IS_ERR(port->config_base)) > >> + return dev_err_probe(dev, PTR_ERR(port->config_base), > >> + "failed to map config memory\n"); > >> + > >> + port->phy = devm_phy_optional_get(dev, NULL); > >> + if (IS_ERR(port->phy)) > >> + return dev_err_probe(dev, PTR_ERR(port->phy), > >> + "failed to get pcie phy\n"); > >> + > >> + bridge = devm_pci_alloc_host_bridge(dev, 0); > >> + if (!bridge) > >> + return dev_err_probe(dev, -ENOMEM, > >> + "failed to alloc bridge\n"); > >> + > >> + if (port->host_ops && port->host_ops->host_init) { > >> + ret = port->host_ops->host_init(port); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> + port->bridge = bridge; > >> + plda_pcie_setup_window(port->bridge_addr, 0, cfg_res->start, 0, > >> + resource_size(cfg_res)); > >> + plda_pcie_setup_iomems(bridge, port); > >> + plda_set_default_msi(&port->msi); > >> + ret = plda_init_interrupts(pdev, port, &plda_default_event); > >> + if (ret) > >> + goto err_host; > >> + > >> + /* Set default bus ops */ > >> + bridge->ops = ops; > >> + bridge->sysdata = port; > >> + > >> + ret = pci_host_probe(bridge); > >> + if (ret < 0) { > >> + dev_err(dev, "failed to pci host probe: %d\n", ret); > > > > I think you mean: > > > > dev_err_probe(dev, ret, "failed to probe pci host\n"); > > > I check with dev_err_probe, It is better than dev_err, I will replace this. > >> + goto err_probe; > >> + } > >> + > >> + return ret; > >> + > >> +err_probe: > >> + plda_pcie_irq_domain_deinit(port); > >> +err_host: > >> + if (port->host_ops && port->host_ops->host_deinit) > >> + port->host_ops->host_deinit(port); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(plda_pcie_host_init); > >> + > >> +void plda_pcie_host_deinit(struct plda_pcie_rp *port) > >> +{ > >> + pci_stop_root_bus(port->bridge->bus); > >> + pci_remove_root_bus(port->bridge->bus); > >> + > >> + plda_pcie_irq_domain_deinit(port); > >> + > >> + if (port->host_ops && port->host_ops->host_deinit) > >> + port->host_ops->host_deinit(port); > >> +} > >> +EXPORT_SYMBOL_GPL(plda_pcie_host_deinit); > >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > >> index b7420598a326..e571047d7944 100644 > >> --- a/drivers/pci/controller/plda/pcie-plda.h > >> +++ b/drivers/pci/controller/plda/pcie-plda.h > >> @@ -6,14 +6,24 @@ > >> #ifndef _PCIE_PLDA_H > >> #define _PCIE_PLDA_H > >> > >> +#include <linux/phy/phy.h> > >> + > >> /* Number of MSI IRQs */ > >> #define PLDA_MAX_NUM_MSI_IRQS 32 > >> > >> /* PCIe Bridge Phy Regs */ > >> +#define GEN_SETTINGS 0x80 > >> +#define RP_ENABLE 1 > >> +#define PCIE_PCI_IDS_DW1 0x9c > >> +#define IDS_CLASS_CODE_SHIFT 16 > >> #define PCIE_PCI_IRQ_DW0 0xa8 > >> #define MSIX_CAP_MASK BIT(31) > >> #define NUM_MSI_MSGS_MASK GENMASK(6, 4) > >> #define NUM_MSI_MSGS_SHIFT 4 > >> +#define PCI_MISC 0xb4 > >> +#define PHY_FUNCTION_DIS BIT(15) > >> +#define PCIE_WINROM 0xfc > >> +#define PREF_MEM_WIN_64_SUPPORT BIT(3) > >> > >> #define IMASK_LOCAL 0x180 > >> #define DMA_END_ENGINE_0_MASK 0x00000000u > >> @@ -64,6 +74,8 @@ > >> #define ISTATUS_HOST 0x18c > >> #define IMSI_ADDR 0x190 > >> #define ISTATUS_MSI 0x194 > >> +#define PMSG_SUPPORT_RX 0x3f0 > >> +#define PMSG_LTR_SUPPORT BIT(2) > >> > >> /* PCIe Master table init defines */ > >> #define ATR0_PCIE_WIN0_SRCADDR_PARAM 0x600u > >> @@ -85,6 +97,8 @@ > >> #define PCIE_TX_RX_INTERFACE 0x00000000u > >> #define PCIE_CONFIG_INTERFACE 0x00000001u > >> > >> +#define CONFIG_SPACE_ADDR 0x1000u > >> + > >> #define ATR_ENTRY_SIZE 32 > >> > >> #define EVENT_A_ATR_EVT_POST_ERR 0 > >> @@ -134,6 +148,11 @@ struct plda_event_ops { > >> u32 (*get_events)(struct plda_pcie_rp *pcie); > >> }; > >> > >> +struct plda_pcie_host_ops { > >> + int (*host_init)(struct plda_pcie_rp *pcie); > >> + void (*host_deinit)(struct plda_pcie_rp *pcie); > >> +}; > >> + > >> struct plda_msi { > >> struct mutex lock; /* Protect used bitmap */ > >> struct irq_domain *msi_domain; > >> @@ -145,12 +164,19 @@ struct plda_msi { > >> > >> struct plda_pcie_rp { > >> struct device *dev; > >> + struct pci_host_bridge *bridge; > >> struct irq_domain *intx_domain; > >> struct irq_domain *event_domain; > >> raw_spinlock_t lock; > >> struct plda_msi msi; > >> const struct plda_event_ops *event_ops; > >> + const struct plda_pcie_host_ops *host_ops; > >> + struct phy *phy; > >> void __iomem *bridge_addr; > >> + void __iomem *config_base; > >> + int irq; > >> + int msi_irq; > >> + int intx_irq; > >> int num_events; > >> }; > >> > >> @@ -163,6 +189,7 @@ struct plda_event { > >> int msi_event; > >> }; > >> > >> +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, int where); > >> int plda_init_interrupts(struct platform_device *pdev, > >> struct plda_pcie_rp *port, > >> const struct plda_event *event); > >> @@ -171,4 +198,66 @@ void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > >> size_t size); > >> int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > >> struct plda_pcie_rp *port); > >> -#endif > >> +int plda_pcie_host_init(struct plda_pcie_rp *pcie, struct pci_ops *ops); > >> +void plda_pcie_host_deinit(struct plda_pcie_rp *pcie); > >> + > >> +static inline void plda_set_default_msi(struct plda_msi *msi) > >> +{ > >> + msi->vector_phy = IMSI_ADDR; > >> + msi->num_vectors = PLDA_MAX_NUM_MSI_IRQS; > >> +} > >> + > >> +static inline void plda_pcie_enable_root_port(struct plda_pcie_rp *plda) > >> +{ > >> + u32 value; > >> + > >> + value = readl_relaxed(plda->bridge_addr + GEN_SETTINGS); > >> + value |= RP_ENABLE; > >> + writel_relaxed(value, plda->bridge_addr + GEN_SETTINGS); > >> +} > >> + > >> +static inline void plda_pcie_set_standard_class(struct plda_pcie_rp *plda) > >> +{ > >> + u32 value; > >> + > >> + value = readl_relaxed(plda->bridge_addr + PCIE_PCI_IDS_DW1); > >> + value &= 0xff; > >> + value |= (PCI_CLASS_BRIDGE_PCI << IDS_CLASS_CODE_SHIFT); > > > > This looks suspicious. You're clearing the lower 8 bits, but setting > > bits 16 and up. Also the parentheses are not strictly necessary. > > > I will add comments to this. > >> + writel_relaxed(value, plda->bridge_addr + PCIE_PCI_IDS_DW1); > >> +} > >> + > >> +static inline void plda_pcie_set_pref_win_64bit(struct plda_pcie_rp *plda) > >> +{ > >> + u32 value; > >> + > >> + value = readl_relaxed(plda->bridge_addr + PCIE_WINROM); > >> + value |= PREF_MEM_WIN_64_SUPPORT; > >> + writel_relaxed(value, plda->bridge_addr + PCIE_WINROM); > >> +} > >> + > >> +static inline void plda_pcie_disable_ltr(struct plda_pcie_rp *plda) > >> +{ > >> + u32 value; > >> + > >> + value = readl_relaxed(plda->bridge_addr + PMSG_SUPPORT_RX); > >> + value &= ~PMSG_LTR_SUPPORT; > >> + writel_relaxed(value, plda->bridge_addr + PMSG_SUPPORT_RX); > >> +} > >> + > >> +static inline void plda_pcie_disable_func(struct plda_pcie_rp *plda) > >> +{ > >> + u32 value; > >> + > >> + value = readl_relaxed(plda->bridge_addr + PCI_MISC); > >> + value |= PHY_FUNCTION_DIS; > >> + writel_relaxed(value, plda->bridge_addr + PCI_MISC); > >> +} > >> + > >> +static inline void plda_pcie_write_rc_bar(struct plda_pcie_rp *plda, u64 val) > >> +{ > >> + void __iomem *addr = plda->bridge_addr + CONFIG_SPACE_ADDR; > >> + > >> + writel_relaxed(val & 0xffffffff, addr + PCI_BASE_ADDRESS_0); > >> + writel_relaxed(val >> 32, addr + PCI_BASE_ADDRESS_1); > > > > Consider using lower_32_bits(val) and upper_32_bits(val) here. > > > >> +} > >> +#endif /* _PCIE_PLDA_H */ > >> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c > >> new file mode 100644 > >> index 000000000000..42099fef0d7d > >> --- /dev/null > >> +++ b/drivers/pci/controller/plda/pcie-starfive.c > >> @@ -0,0 +1,461 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> +/* > >> + * PCIe host controller driver for StarFive JH7110 Soc. > >> + * > >> + * Copyright (C) 2023 StarFive Technology Co., Ltd. > >> + */ > >> + > >> +#include <linux/bitfield.h> > >> +#include <linux/clk.h> > >> +#include <linux/delay.h> > >> +#include <linux/gpio/consumer.h> > >> +#include <linux/interrupt.h> > >> +#include <linux/kernel.h> > >> +#include <linux/mfd/syscon.h> > >> +#include <linux/module.h> > >> +#include <linux/of_address.h> > >> +#include <linux/of_irq.h> > >> +#include <linux/of_pci.h> > >> +#include <linux/pci.h> > >> +#include <linux/platform_device.h> > >> +#include <linux/pm_runtime.h> > >> +#include <linux/regmap.h> > >> +#include <linux/reset.h> > >> +#include "../../pci.h" > >> + > >> +#include "pcie-plda.h" > >> + > >> +#define PCIE_FUNC_NUM 4 > >> + > >> +/* system control */ > >> +#define STG_SYSCON_PCIE0_BASE 0x48 > >> +#define STG_SYSCON_PCIE1_BASE 0x1f8 > >> + > >> +#define STG_SYSCON_ARFUNC_OFFSET 0x78 > >> +#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK GENMASK(22, 8) > >> +#define STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(x) FIELD_PREP(GENMASK(20, 17), x) > >> +#define STG_SYSCON_AWFUNC_OFFSET 0x7c > >> +#define STG_SYSCON_AXI4_SLVL_AWFUNC_MASK GENMASK(14, 0) > >> +#define STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(x) FIELD_PREP(GENMASK(12, 9), x) > >> +#define STG_SYSCON_CLKREQ BIT(22) > >> +#define STG_SYSCON_CKREF_SRC_MASK GENMASK(19, 18) > >> +#define STG_SYSCON_RP_NEP_OFFSET 0xe8 > >> +#define STG_SYSCON_K_RP_NEP BIT(8) > >> +#define STG_SYSCON_LNKSTA_OFFSET 0x170 > >> +#define DATA_LINK_ACTIVE BIT(5) > >> + > >> +/* Parameters for the waiting for link up routine */ > >> +#define LINK_WAIT_MAX_RETRIES 10 > >> +#define LINK_WAIT_USLEEP_MIN 90000 > >> +#define LINK_WAIT_USLEEP_MAX 100000 > >> + > >> +struct starfive_jh7110_pcie { > >> + struct plda_pcie_rp plda; > >> + struct reset_control *resets; > >> + struct clk_bulk_data *clks; > >> + struct regmap *reg_syscon; > >> + struct gpio_desc *power_gpio; > >> + struct gpio_desc *reset_gpio; > >> + > >> + u32 stg_arfun; > >> + u32 stg_awfun; > >> + u32 stg_rp_nep; > >> + u32 stg_lnksta; > >> + > >> + int num_clks; > >> +}; > >> + > >> +/* > >> + * The BAR0/1 of bridge should be hidden during enumeration to > >> + * avoid the sizing and resource allocation by PCIe core. > >> + */ > >> +static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int devfn, > >> + int offset) > >> +{ > >> + if (pci_is_root_bus(bus) && !devfn && > >> + (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1)) > >> + return true; > >> + > >> + return false; > >> +} > >> + > >> +static int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn, > >> + int where, int size, u32 value) > >> +{ > >> + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > >> + return PCIBIOS_BAD_REGISTER_NUMBER; > >> + > >> + return pci_generic_config_write(bus, devfn, where, size, value); > >> +} > >> + > >> +static int starfive_pcie_config_read(struct pci_bus *bus, unsigned int devfn, > >> + int where, int size, u32 *value) > >> +{ > >> + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > >> + return PCIBIOS_BAD_REGISTER_NUMBER; > >> + > >> + return pci_generic_config_read(bus, devfn, where, size, value); > >> +} > >> + > >> +static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie, struct device *dev) > >> +{ > >> + int domain_nr, stg_pcie_base; > >> + > >> + pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks); > >> + if (pcie->num_clks < 0) > >> + return dev_err_probe(dev, -ENODEV, > >> + "failed to get pcie clocks\n"); > >> + > >> + pcie->resets = devm_reset_control_array_get_exclusive(dev); > >> + if (IS_ERR(pcie->resets)) > >> + return dev_err_probe(dev, PTR_ERR(pcie->resets), > >> + "failed to get pcie resets"); > >> + > >> + pcie->reg_syscon = > >> + syscon_regmap_lookup_by_phandle(dev->of_node, > >> + "starfive,stg-syscon"); > >> + > >> + if (IS_ERR(pcie->reg_syscon)) > >> + return dev_err_probe(dev, PTR_ERR(pcie->reg_syscon), > >> + "failed to parse starfive,stg-syscon\n"); > >> + > >> + domain_nr = of_get_pci_domain_nr(dev->of_node); > >> + > >> + if (domain_nr < 0 || domain_nr > 1) > >> + return dev_err_probe(dev, -ENODEV, > >> + "failed to get valid pcie id\n"); > >> + > >> + if (domain_nr == 0) > >> + stg_pcie_base = STG_SYSCON_PCIE0_BASE; > >> + else > >> + stg_pcie_base = STG_SYSCON_PCIE1_BASE; > > > > It seems you only need the domain_nr for the determining this offset into the > > stg_syscon. Since this is already specific to the JH7110 SoC, wouldn't it be > > safer to match on the 0x2b000000/0x2c000000 or 0x940000000/0x9c0000000 > > address of the peripheral instead? > > > Maybe this codes not just bind to JH7110 SoC. Right, but in that case the STG_SYSCON_PCIE{0,1}_BASE offsets will also be different, so you'd need to get them from device tree match data anyway, and then you might as well have an { address, offset } table. > >> + > >> + pcie->stg_arfun = stg_pcie_base + STG_SYSCON_ARFUNC_OFFSET; > >> + pcie->stg_awfun = stg_pcie_base + STG_SYSCON_AWFUNC_OFFSET; > >> + pcie->stg_rp_nep = stg_pcie_base + STG_SYSCON_RP_NEP_OFFSET; > >> + pcie->stg_lnksta = stg_pcie_base + STG_SYSCON_LNKSTA_OFFSET; > > > > These offsets are all the same for both pcie0 and pcie1 so why do you need a > > variable for each? Eg. why not just > > > > if (domain_nr == 0) > > pcie->stg_base = STG_SYSCON_PCIE0_BASE; > > else > > pcie->stg_base = STG_SYSCON_PCIE1_BASE; > > > > ..and then add add those static offsets where needed? > > > OK. I will try to modify this. > >> + > >> + pcie->reset_gpio = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); > >> + if (IS_ERR(pcie->reset_gpio)) { > >> + dev_warn(dev, "failed to get perst-gpio\n"); > > > > Why is this a warn when you actually error out? I think you mean > > > > return dev_err_probe(dev, PTR_ERR(pcie->reset_gpio, "failed to get > > reset GPIO\n"); > > > >> + return -EINVAL; > >> + } > >> + > >> + pcie->power_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW); > >> + if (IS_ERR(pcie->power_gpio)) { > >> + dev_warn(dev, "failed to get power-gpio\n"); > >> + return -EINVAL; > > > > return dev_err_probe(dev, PTR_ERR(pcie->power_gpio), "failed to get > > power GPIO\n"); > > > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static struct pci_ops starfive_pcie_ops = { > >> + .map_bus = plda_pcie_map_bus, > >> + .read = starfive_pcie_config_read, > >> + .write = starfive_pcie_config_write, > >> +}; > >> + > >> +static int starfive_pcie_clk_rst_init(struct starfive_jh7110_pcie *pcie) > >> +{ > >> + int ret; > >> + struct device *dev = pcie->plda.dev; > > > > Again, please order initialized variables first. > > > >> + > >> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); > >> + if (ret) { > >> + dev_err(dev, "failed to enable clocks\n"); > >> + return ret; > > > > return dev_err_probe(dev, ret, "failed to enable clocks\n"); > > > >> + } > >> + > >> + ret = reset_control_deassert(pcie->resets); > >> + if (ret) { > >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > >> + dev_err(dev, "failed to resets\n"); > > > > I think you mean > > return dev_err_probe(dev, ret, "failed to deassert resets\n"); > > > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static void starfive_pcie_clk_rst_deinit(struct starfive_jh7110_pcie *pcie) > >> +{ > >> + reset_control_assert(pcie->resets); > >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > >> +} > >> + > >> +static bool starfive_pcie_link_up(struct plda_pcie_rp *plda) > >> +{ > >> + struct starfive_jh7110_pcie *pcie = > >> + container_of(plda, struct starfive_jh7110_pcie, plda); > >> + struct device *dev = pcie->plda.dev; > > > > This is only used once below, so just use pcie->plda.dev directly there. > > > OK, thanks > >> + int ret; > >> + u32 stg_reg_val; > >> + > >> + ret = regmap_read(pcie->reg_syscon, pcie->stg_lnksta, &stg_reg_val); > >> + if (ret) { > >> + dev_err(dev, "failed to read link status\n"); > >> + return false; > >> + } > >> + > >> + if (stg_reg_val & DATA_LINK_ACTIVE) > >> + return true; > >> + > >> + return false; > > > > These last 4 lines is just "return stg_reg_val & DATA_LINK_ACTIVE;" right? > > > OK > >> +} > >> + > >> +static int starfive_pcie_host_wait_for_link(struct starfive_jh7110_pcie *pcie) > >> +{ > >> + struct device *dev = pcie->plda.dev; > > > > Only used once, just inline. > > > >> + int retries; > >> + > >> + /* Check if the link is up or not */ > >> + for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { > >> + if (starfive_pcie_link_up(&pcie->plda)) { > >> + dev_info(dev, "port link up\n"); > >> + return 0; > >> + } > >> + usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > >> + } > >> + > >> + return -ETIMEDOUT; > >> +} > >> + > >> +static int starfive_pcie_enable_phy(struct device *dev, struct plda_pcie_rp *pcie) > >> +{ > >> + int ret; > >> + > >> + if (!pcie->phy) > >> + return 0; > >> + > >> + ret = phy_init(pcie->phy); > >> + if (ret) > >> + return dev_err_probe(dev, ret, > >> + "failed to initialize pcie phy\n"); > >> + > >> + ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE); > >> + if (ret) { > >> + dev_err(dev, "failed to set pcie mode\n"); > >> + goto err_phy_on; > >> + } > >> + > >> + ret = phy_power_on(pcie->phy); > >> + if (ret) { > >> + dev_err(dev, "failed to power on pcie phy\n"); > >> + goto err_phy_on; > >> + } > >> + > >> + return 0; > >> + > >> +err_phy_on: > >> + phy_exit(pcie->phy); > >> + return ret; > >> +} > >> + > >> +static void starfive_pcie_disable_phy(struct plda_pcie_rp *pcie) > >> +{ > >> + phy_power_off(pcie->phy); > >> + phy_exit(pcie->phy); > >> +} > >> + > >> +static void starfive_pcie_host_deinit(struct plda_pcie_rp *plda) > >> +{ > >> + struct starfive_jh7110_pcie *pcie = > >> + container_of(plda, struct starfive_jh7110_pcie, plda); > >> + > >> + starfive_pcie_clk_rst_deinit(pcie); > >> + if (pcie->power_gpio) > >> + gpiod_set_value_cansleep(pcie->power_gpio, 0); > >> + starfive_pcie_disable_phy(plda); > >> +} > >> + > >> +static int starfive_pcie_host_init(struct plda_pcie_rp *plda) > >> +{ > >> + int i; > >> + struct starfive_jh7110_pcie *pcie = > >> + container_of(plda, struct starfive_jh7110_pcie, plda); > >> + struct device *dev = plda->dev; > >> + int ret; > > > > Weird ordering again. > > > >> + > >> + ret = starfive_pcie_enable_phy(dev, plda); > >> + if (ret) > >> + return ret; > >> + > >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_rp_nep, > >> + STG_SYSCON_K_RP_NEP, STG_SYSCON_K_RP_NEP); > >> + > >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > >> + STG_SYSCON_CKREF_SRC_MASK, > >> + FIELD_PREP(STG_SYSCON_CKREF_SRC_MASK, 2)); > >> + > >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > >> + STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ); > >> + > >> + ret = starfive_pcie_clk_rst_init(pcie); > >> + if (ret) > >> + return ret; > >> + > >> + if (pcie->power_gpio) > >> + gpiod_set_value_cansleep(pcie->power_gpio, 1); > >> + > >> + if (pcie->reset_gpio) > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > >> + > >> + /* Disable physical functions except #0 */ > >> + for (i = 1; i < PCIE_FUNC_NUM; i++) { > >> + regmap_update_bits(pcie->reg_syscon, > >> + pcie->stg_arfun, > >> + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, > >> + STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(i)); > >> + > >> + regmap_update_bits(pcie->reg_syscon, > >> + pcie->stg_awfun, > >> + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, > >> + STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(i)); > >> + > >> + plda_pcie_disable_func(plda); > >> + } > >> + > >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_arfun, > >> + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, 0); > >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > >> + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, 0); > >> + > >> + plda_pcie_enable_root_port(plda); > >> + plda_pcie_write_rc_bar(plda, 0); > >> + > >> + /* PCIe PCI Standard Configuration Identification Settings. */ > >> + plda_pcie_set_standard_class(plda); > >> + > >> + /* > >> + * The LTR message forwarding of PCIe Message Reception was set by core > >> + * as default, but the forward id & addr are also need to be reset. > >> + * If we do not disable LTR message forwarding here, or set a legal > >> + * forwarding address, the kernel will get stuck after this driver probe. > >> + * To workaround, disable the LTR message forwarding support on > >> + * PCIe Message Reception. > >> + */ > >> + plda_pcie_disable_ltr(plda); > >> + > >> + /* Prefetchable memory window 64-bit addressing support */ > >> + plda_pcie_set_pref_win_64bit(plda); > >> + > >> + /* Ensure that PERST has been asserted for at least 100 ms, > >> + * the sleep value is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4) > >> + */ > >> + msleep(100); > >> + if (pcie->reset_gpio) > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > >> + > >> + /* As the requirement in PCIe base spec r6.0, system (<=5GT/s) must > >> + * wait a minimum of 100 ms following exit from a conventional reset > >> + * before sending a configuration request to the device. > >> + */ > >> + msleep(100); > >> + > >> + if (starfive_pcie_host_wait_for_link(pcie)) > >> + dev_info(dev, "port link down\n"); > >> + > >> + return ret; > >> +} > >> + > >> +static const struct plda_pcie_host_ops sf_host_ops = { > >> + .host_init = starfive_pcie_host_init, > >> + .host_deinit = starfive_pcie_host_deinit, > >> +}; > >> + > >> +static int starfive_pcie_probe(struct platform_device *pdev) > >> +{ > >> + struct device *dev = &pdev->dev; > >> + struct starfive_jh7110_pcie *pcie; > >> + struct plda_pcie_rp *plda; > >> + int ret; > >> + > >> + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > >> + if (!pcie) > >> + return -ENOMEM; > >> + > >> + plda = &pcie->plda; > >> + plda->dev = dev; > >> + > >> + ret = starfive_pcie_parse_dt(pcie, dev); > >> + if (ret) > >> + return ret; > >> + > >> + plda->host_ops = &sf_host_ops; > >> + plda->num_events = NUM_PLDA_EVENTS; > >> + ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops); > >> + if (ret) > >> + return ret; > >> + > >> + pm_runtime_enable(&pdev->dev); > >> + pm_runtime_get_sync(&pdev->dev); > >> + platform_set_drvdata(pdev, pcie); > >> + > >> + return 0; > >> +} > >> + > >> +static void starfive_pcie_remove(struct platform_device *pdev) > >> +{ > >> + struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev); > >> + > >> + plda_pcie_host_deinit(&pcie->plda); > >> + platform_set_drvdata(pdev, NULL); > >> +} > >> + > >> +static int starfive_pcie_suspend_noirq(struct device *dev) > >> +{ > >> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > >> + > >> + if (!pcie) > >> + return 0; > >> + > >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > >> + starfive_pcie_disable_phy(&pcie->plda); > >> + > >> + return 0; > >> +} > >> + > >> +static int starfive_pcie_resume_noirq(struct device *dev) > >> +{ > >> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > >> + int ret; > >> + > >> + ret = starfive_pcie_enable_phy(dev, &pcie->plda); > >> + if (ret) > >> + return ret; > >> + > >> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); > >> + if (ret) { > >> + dev_err(dev, "failed to enable clocks\n"); > >> + starfive_pcie_disable_phy(&pcie->plda); > >> + return ret; > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static const struct dev_pm_ops starfive_pcie_pm_ops = { > >> + NOIRQ_SYSTEM_SLEEP_PM_OPS(starfive_pcie_suspend_noirq, > >> + starfive_pcie_resume_noirq) > >> +}; > >> + > >> +static const struct of_device_id starfive_pcie_of_match[] = { > >> + { .compatible = "starfive,jh7110-pcie"}, > > > > Missing space before } > > > >> + { /* sentinel */ } > >> +}; > >> +MODULE_DEVICE_TABLE(of, starfive_pcie_of_match); > >> + > >> +static struct platform_driver starfive_pcie_driver = { > >> + .driver = { > >> + .name = "pcie-starfive", > >> + .of_match_table = of_match_ptr(starfive_pcie_of_match), > >> +#ifdef CONFIG_PM_SLEEP > >> + .pm = &starfive_pcie_pm_ops, > >> +#endif > > > > This can be just .pm = pm_sleep_ptr(&starfive_pcie_pm_ops), > > > OK > >> + }, > >> + .probe = starfive_pcie_probe, > >> + .remove_new = starfive_pcie_remove, > >> +}; > >> +module_platform_driver(starfive_pcie_driver); > >> + > >> +MODULE_DESCRIPTION("StarFive JH7110 PCIe host driver"); > >> +MODULE_LICENSE("GPL v2"); > >> -- > >> 2.17.1
On 2023/9/19 19:34, Emil Renner Berthing wrote: > Minda Chen wrote: >> >> >> On 2023/9/18 6:13, Emil Renner Berthing wrote: >> > Minda Chen wrote: >> >> Add StarFive JH7110 SoC PCIe controller platform >> >> driver codes, and add host init/deinit to pcie-plda-host.c >> >> >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> >> Co-developed-by: Kevin Xie <kevin.xie@starfivetech.com> >> >> Reviewed-by: Mason Huo <mason.huo@starfivetech.com> >> >> --- >> >> MAINTAINERS | 7 + >> >> drivers/pci/controller/plda/Kconfig | 12 + >> >> drivers/pci/controller/plda/Makefile | 1 + >> >> drivers/pci/controller/plda/pcie-plda-host.c | 121 ++++- >> >> drivers/pci/controller/plda/pcie-plda.h | 91 +++- >> >> drivers/pci/controller/plda/pcie-starfive.c | 461 +++++++++++++++++++ >> >> 6 files changed, 688 insertions(+), 5 deletions(-) >> >> create mode 100644 drivers/pci/controller/plda/pcie-starfive.c >> >> >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> >> index ebdb65e195ee..4632cd411a3d 100644 >> >> --- a/MAINTAINERS >> >> +++ b/MAINTAINERS >> >> @@ -16679,6 +16679,13 @@ S: Maintained >> >> F: Documentation/devicetree/bindings/pci/socionext,uniphier-pcie* >> >> F: drivers/pci/controller/dwc/pcie-uniphier* >> >> >> >> +PCIE DRIVER FOR STARFIVE JH71x0 >> >> +M: Kevin Xie <kevin.xie@starfivetech.com> >> >> +L: linux-pci@vger.kernel.org >> >> +S: Maintained >> >> +F: Documentation/devicetree/bindings/pci/starfive* >> >> +F: drivers/pci/controller/plda/pcie-starfive.c >> >> + >> >> PCIE DRIVER FOR ST SPEAR13XX >> >> M: Pratyush Anand <pratyush.anand@gmail.com> >> >> L: linux-pci@vger.kernel.org >> >> diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig >> >> index e54a82ee94f5..8de661730aa5 100644 >> >> --- a/drivers/pci/controller/plda/Kconfig >> >> +++ b/drivers/pci/controller/plda/Kconfig >> >> @@ -15,4 +15,16 @@ config PCIE_MICROCHIP_HOST >> >> Say Y here if you want kernel to support the Microchip AXI PCIe >> >> Host Bridge driver. >> >> >> >> +config PCIE_STARFIVE_HOST >> >> + tristate "StarFive PCIe host controller" >> >> + depends on OF && PCI_MSI >> >> + select PCIE_PLDA_HOST >> >> + help >> >> + Say Y here if you want to support the StarFive PCIe controller >> >> + in host mode. StarFive PCIe controller uses PLDA PCIe >> >> + core. >> >> + If you choose to build this driver as module it will >> >> + be dynamically linked and module will be called >> >> + pcie-starfive.ko >> >> + >> >> endmenu >> >> diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile >> >> index 4340ab007f44..0ac6851bed48 100644 >> >> --- a/drivers/pci/controller/plda/Makefile >> >> +++ b/drivers/pci/controller/plda/Makefile >> >> @@ -1,3 +1,4 @@ >> >> # SPDX-License-Identifier: GPL-2.0 >> >> obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o >> >> obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o >> >> +obj-$(CONFIG_PCIE_STARFIVE_HOST) += pcie-starfive.o >> >> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c >> >> index 26e6cfd2eaba..8b32df477ad8 100644 >> >> --- a/drivers/pci/controller/plda/pcie-plda-host.c >> >> +++ b/drivers/pci/controller/plda/pcie-plda-host.c >> >> @@ -20,6 +20,15 @@ >> >> >> >> #include "pcie-plda.h" >> >> >> >> +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, >> >> + int where) >> >> +{ >> >> + struct plda_pcie_rp *pcie = bus->sysdata; >> >> + >> >> + return pcie->config_base + PCIE_ECAM_OFFSET(bus->number, devfn, where); >> >> +} >> >> +EXPORT_SYMBOL_GPL(plda_pcie_map_bus); >> >> + >> >> static void plda_handle_msi(struct irq_desc *desc) >> >> { >> >> struct plda_pcie_rp *port = irq_desc_get_handler_data(desc); >> >> @@ -370,6 +379,12 @@ static const struct plda_event_ops plda_event_ops = { >> >> .get_events = plda_get_events, >> >> }; >> >> >> >> +static const struct plda_event plda_default_event = { >> >> + .event_ops = &plda_event_ops, >> >> + .intx_event = EVENT_PM_MSI_INT_INTX, >> >> + .msi_event = EVENT_PM_MSI_INT_MSI >> >> +}; >> >> + >> >> static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port, >> >> const struct irq_domain_ops *ops) >> >> { >> >> @@ -416,7 +431,6 @@ int plda_init_interrupts(struct platform_device *pdev, >> >> const struct plda_event *event) >> >> { >> >> struct device *dev = &pdev->dev; >> >> - int irq; >> >> int i, intx_irq, msi_irq, event_irq; >> >> int ret; >> >> const struct irq_domain_ops *irq_dom_ops; >> >> @@ -434,8 +448,8 @@ int plda_init_interrupts(struct platform_device *pdev, >> >> return ret; >> >> } >> >> >> >> - irq = platform_get_irq(pdev, 0); >> >> - if (irq < 0) >> >> + port->irq = platform_get_irq(pdev, 0); >> >> + if (port->irq < 0) >> >> return -ENODEV; >> > >> > Here you're dropping the local variable irq and just use port->irq directly.. >> > >> >> >> >> port->event_ops = event->event_ops; >> >> @@ -465,6 +479,7 @@ int plda_init_interrupts(struct platform_device *pdev, >> >> dev_err(dev, "failed to map INTx interrupt\n"); >> >> return -ENXIO; >> >> } >> >> + port->intx_irq = intx_irq; >> > >> > ..but for intx_irq.. >> > >> >> >> >> /* Plug the INTx chained handler */ >> >> irq_set_chained_handler_and_data(intx_irq, plda_handle_intx, port); >> >> @@ -473,12 +488,13 @@ int plda_init_interrupts(struct platform_device *pdev, >> >> event->msi_event); >> >> if (!msi_irq) >> >> return -ENXIO; >> >> + port->msi_irq = msi_irq; >> > >> > ..and msi_irq you're keeping the local variable. I think you should drop >> > the local variables here too, or at least do the same for all of them. >> > >> OK, remove all the local variables is better. >> >> >> >> /* Plug the MSI chained handler */ >> >> irq_set_chained_handler_and_data(msi_irq, plda_handle_msi, port); >> >> >> >> /* Plug the main event chained handler */ >> >> - irq_set_chained_handler_and_data(irq, plda_handle_event, port); >> >> + irq_set_chained_handler_and_data(port->irq, plda_handle_event, port); >> >> >> >> return 0; >> >> } >> >> @@ -544,3 +560,100 @@ int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> >> return 0; >> >> } >> >> EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); >> >> + >> >> +static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie) >> >> +{ >> >> + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL); >> >> + irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL); >> >> + irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL); >> >> + >> >> + irq_domain_remove(pcie->msi.msi_domain); >> >> + irq_domain_remove(pcie->msi.dev_domain); >> >> + >> >> + irq_domain_remove(pcie->intx_domain); >> >> + irq_domain_remove(pcie->event_domain); >> >> +} >> >> + >> >> +int plda_pcie_host_init(struct plda_pcie_rp *port, struct pci_ops *ops) >> >> +{ >> >> + struct resource *cfg_res; >> >> + struct device *dev = port->dev; >> >> + int ret; >> >> + struct pci_host_bridge *bridge; >> >> + struct platform_device *pdev = to_platform_device(dev); >> > >> > This looks weird. Please order initialized variables first and maybe even do >> > reverse christmas tree order. Eg. longeist lines first. >> > >> OK, The same with other places. >> >> + >> >> + port->bridge_addr = >> >> + devm_platform_ioremap_resource_byname(pdev, "apb"); >> >> + >> >> + if (IS_ERR(port->bridge_addr)) >> >> + return dev_err_probe(dev, PTR_ERR(port->bridge_addr), >> >> + "failed to map reg memory\n"); >> >> + >> >> + cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); >> >> + if (!cfg_res) >> >> + return dev_err_probe(dev, -ENODEV, >> >> + "failed to get config memory\n"); >> >> + >> >> + port->config_base = devm_ioremap_resource(dev, cfg_res); >> >> + if (IS_ERR(port->config_base)) >> >> + return dev_err_probe(dev, PTR_ERR(port->config_base), >> >> + "failed to map config memory\n"); >> >> + >> >> + port->phy = devm_phy_optional_get(dev, NULL); >> >> + if (IS_ERR(port->phy)) >> >> + return dev_err_probe(dev, PTR_ERR(port->phy), >> >> + "failed to get pcie phy\n"); >> >> + >> >> + bridge = devm_pci_alloc_host_bridge(dev, 0); >> >> + if (!bridge) >> >> + return dev_err_probe(dev, -ENOMEM, >> >> + "failed to alloc bridge\n"); >> >> + >> >> + if (port->host_ops && port->host_ops->host_init) { >> >> + ret = port->host_ops->host_init(port); >> >> + if (ret) >> >> + return ret; >> >> + } >> >> + >> >> + port->bridge = bridge; >> >> + plda_pcie_setup_window(port->bridge_addr, 0, cfg_res->start, 0, >> >> + resource_size(cfg_res)); >> >> + plda_pcie_setup_iomems(bridge, port); >> >> + plda_set_default_msi(&port->msi); >> >> + ret = plda_init_interrupts(pdev, port, &plda_default_event); >> >> + if (ret) >> >> + goto err_host; >> >> + >> >> + /* Set default bus ops */ >> >> + bridge->ops = ops; >> >> + bridge->sysdata = port; >> >> + >> >> + ret = pci_host_probe(bridge); >> >> + if (ret < 0) { >> >> + dev_err(dev, "failed to pci host probe: %d\n", ret); >> > >> > I think you mean: >> > >> > dev_err_probe(dev, ret, "failed to probe pci host\n"); >> > >> I check with dev_err_probe, It is better than dev_err, I will replace this. >> >> + goto err_probe; >> >> + } >> >> + >> >> + return ret; >> >> + >> >> +err_probe: >> >> + plda_pcie_irq_domain_deinit(port); >> >> +err_host: >> >> + if (port->host_ops && port->host_ops->host_deinit) >> >> + port->host_ops->host_deinit(port); >> >> + >> >> + return ret; >> >> +} >> >> +EXPORT_SYMBOL_GPL(plda_pcie_host_init); >> >> + >> >> +void plda_pcie_host_deinit(struct plda_pcie_rp *port) >> >> +{ >> >> + pci_stop_root_bus(port->bridge->bus); >> >> + pci_remove_root_bus(port->bridge->bus); >> >> + >> >> + plda_pcie_irq_domain_deinit(port); >> >> + >> >> + if (port->host_ops && port->host_ops->host_deinit) >> >> + port->host_ops->host_deinit(port); >> >> +} >> >> +EXPORT_SYMBOL_GPL(plda_pcie_host_deinit); >> >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h >> >> index b7420598a326..e571047d7944 100644 >> >> --- a/drivers/pci/controller/plda/pcie-plda.h >> >> +++ b/drivers/pci/controller/plda/pcie-plda.h >> >> @@ -6,14 +6,24 @@ >> >> #ifndef _PCIE_PLDA_H >> >> #define _PCIE_PLDA_H >> >> >> >> +#include <linux/phy/phy.h> >> >> + >> >> /* Number of MSI IRQs */ >> >> #define PLDA_MAX_NUM_MSI_IRQS 32 >> >> >> >> /* PCIe Bridge Phy Regs */ >> >> +#define GEN_SETTINGS 0x80 >> >> +#define RP_ENABLE 1 >> >> +#define PCIE_PCI_IDS_DW1 0x9c >> >> +#define IDS_CLASS_CODE_SHIFT 16 >> >> #define PCIE_PCI_IRQ_DW0 0xa8 >> >> #define MSIX_CAP_MASK BIT(31) >> >> #define NUM_MSI_MSGS_MASK GENMASK(6, 4) >> >> #define NUM_MSI_MSGS_SHIFT 4 >> >> +#define PCI_MISC 0xb4 >> >> +#define PHY_FUNCTION_DIS BIT(15) >> >> +#define PCIE_WINROM 0xfc >> >> +#define PREF_MEM_WIN_64_SUPPORT BIT(3) >> >> >> >> #define IMASK_LOCAL 0x180 >> >> #define DMA_END_ENGINE_0_MASK 0x00000000u >> >> @@ -64,6 +74,8 @@ >> >> #define ISTATUS_HOST 0x18c >> >> #define IMSI_ADDR 0x190 >> >> #define ISTATUS_MSI 0x194 >> >> +#define PMSG_SUPPORT_RX 0x3f0 >> >> +#define PMSG_LTR_SUPPORT BIT(2) >> >> >> >> /* PCIe Master table init defines */ >> >> #define ATR0_PCIE_WIN0_SRCADDR_PARAM 0x600u >> >> @@ -85,6 +97,8 @@ >> >> #define PCIE_TX_RX_INTERFACE 0x00000000u >> >> #define PCIE_CONFIG_INTERFACE 0x00000001u >> >> >> >> +#define CONFIG_SPACE_ADDR 0x1000u >> >> + >> >> #define ATR_ENTRY_SIZE 32 >> >> >> >> #define EVENT_A_ATR_EVT_POST_ERR 0 >> >> @@ -134,6 +148,11 @@ struct plda_event_ops { >> >> u32 (*get_events)(struct plda_pcie_rp *pcie); >> >> }; >> >> >> >> +struct plda_pcie_host_ops { >> >> + int (*host_init)(struct plda_pcie_rp *pcie); >> >> + void (*host_deinit)(struct plda_pcie_rp *pcie); >> >> +}; >> >> + >> >> struct plda_msi { >> >> struct mutex lock; /* Protect used bitmap */ >> >> struct irq_domain *msi_domain; >> >> @@ -145,12 +164,19 @@ struct plda_msi { >> >> >> >> struct plda_pcie_rp { >> >> struct device *dev; >> >> + struct pci_host_bridge *bridge; >> >> struct irq_domain *intx_domain; >> >> struct irq_domain *event_domain; >> >> raw_spinlock_t lock; >> >> struct plda_msi msi; >> >> const struct plda_event_ops *event_ops; >> >> + const struct plda_pcie_host_ops *host_ops; >> >> + struct phy *phy; >> >> void __iomem *bridge_addr; >> >> + void __iomem *config_base; >> >> + int irq; >> >> + int msi_irq; >> >> + int intx_irq; >> >> int num_events; >> >> }; >> >> >> >> @@ -163,6 +189,7 @@ struct plda_event { >> >> int msi_event; >> >> }; >> >> >> >> +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, int where); >> >> int plda_init_interrupts(struct platform_device *pdev, >> >> struct plda_pcie_rp *port, >> >> const struct plda_event *event); >> >> @@ -171,4 +198,66 @@ void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> >> size_t size); >> >> int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> >> struct plda_pcie_rp *port); >> >> -#endif >> >> +int plda_pcie_host_init(struct plda_pcie_rp *pcie, struct pci_ops *ops); >> >> +void plda_pcie_host_deinit(struct plda_pcie_rp *pcie); >> >> + >> >> +static inline void plda_set_default_msi(struct plda_msi *msi) >> >> +{ >> >> + msi->vector_phy = IMSI_ADDR; >> >> + msi->num_vectors = PLDA_MAX_NUM_MSI_IRQS; >> >> +} >> >> + >> >> +static inline void plda_pcie_enable_root_port(struct plda_pcie_rp *plda) >> >> +{ >> >> + u32 value; >> >> + >> >> + value = readl_relaxed(plda->bridge_addr + GEN_SETTINGS); >> >> + value |= RP_ENABLE; >> >> + writel_relaxed(value, plda->bridge_addr + GEN_SETTINGS); >> >> +} >> >> + >> >> +static inline void plda_pcie_set_standard_class(struct plda_pcie_rp *plda) >> >> +{ >> >> + u32 value; >> >> + >> >> + value = readl_relaxed(plda->bridge_addr + PCIE_PCI_IDS_DW1); >> >> + value &= 0xff; >> >> + value |= (PCI_CLASS_BRIDGE_PCI << IDS_CLASS_CODE_SHIFT); >> > >> > This looks suspicious. You're clearing the lower 8 bits, but setting >> > bits 16 and up. Also the parentheses are not strictly necessary. >> > >> I will add comments to this. >> >> + writel_relaxed(value, plda->bridge_addr + PCIE_PCI_IDS_DW1); >> >> +} >> >> + >> >> +static inline void plda_pcie_set_pref_win_64bit(struct plda_pcie_rp *plda) >> >> +{ >> >> + u32 value; >> >> + >> >> + value = readl_relaxed(plda->bridge_addr + PCIE_WINROM); >> >> + value |= PREF_MEM_WIN_64_SUPPORT; >> >> + writel_relaxed(value, plda->bridge_addr + PCIE_WINROM); >> >> +} >> >> + >> >> +static inline void plda_pcie_disable_ltr(struct plda_pcie_rp *plda) >> >> +{ >> >> + u32 value; >> >> + >> >> + value = readl_relaxed(plda->bridge_addr + PMSG_SUPPORT_RX); >> >> + value &= ~PMSG_LTR_SUPPORT; >> >> + writel_relaxed(value, plda->bridge_addr + PMSG_SUPPORT_RX); >> >> +} >> >> + >> >> +static inline void plda_pcie_disable_func(struct plda_pcie_rp *plda) >> >> +{ >> >> + u32 value; >> >> + >> >> + value = readl_relaxed(plda->bridge_addr + PCI_MISC); >> >> + value |= PHY_FUNCTION_DIS; >> >> + writel_relaxed(value, plda->bridge_addr + PCI_MISC); >> >> +} >> >> + >> >> +static inline void plda_pcie_write_rc_bar(struct plda_pcie_rp *plda, u64 val) >> >> +{ >> >> + void __iomem *addr = plda->bridge_addr + CONFIG_SPACE_ADDR; >> >> + >> >> + writel_relaxed(val & 0xffffffff, addr + PCI_BASE_ADDRESS_0); >> >> + writel_relaxed(val >> 32, addr + PCI_BASE_ADDRESS_1); >> > >> > Consider using lower_32_bits(val) and upper_32_bits(val) here. >> > >> >> +} >> >> +#endif /* _PCIE_PLDA_H */ >> >> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c >> >> new file mode 100644 >> >> index 000000000000..42099fef0d7d >> >> --- /dev/null >> >> +++ b/drivers/pci/controller/plda/pcie-starfive.c >> >> @@ -0,0 +1,461 @@ >> >> +// SPDX-License-Identifier: GPL-2.0+ >> >> +/* >> >> + * PCIe host controller driver for StarFive JH7110 Soc. >> >> + * >> >> + * Copyright (C) 2023 StarFive Technology Co., Ltd. >> >> + */ >> >> + >> >> +#include <linux/bitfield.h> >> >> +#include <linux/clk.h> >> >> +#include <linux/delay.h> >> >> +#include <linux/gpio/consumer.h> >> >> +#include <linux/interrupt.h> >> >> +#include <linux/kernel.h> >> >> +#include <linux/mfd/syscon.h> >> >> +#include <linux/module.h> >> >> +#include <linux/of_address.h> >> >> +#include <linux/of_irq.h> >> >> +#include <linux/of_pci.h> >> >> +#include <linux/pci.h> >> >> +#include <linux/platform_device.h> >> >> +#include <linux/pm_runtime.h> >> >> +#include <linux/regmap.h> >> >> +#include <linux/reset.h> >> >> +#include "../../pci.h" >> >> + >> >> +#include "pcie-plda.h" >> >> + >> >> +#define PCIE_FUNC_NUM 4 >> >> + >> >> +/* system control */ >> >> +#define STG_SYSCON_PCIE0_BASE 0x48 >> >> +#define STG_SYSCON_PCIE1_BASE 0x1f8 >> >> + >> >> +#define STG_SYSCON_ARFUNC_OFFSET 0x78 >> >> +#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK GENMASK(22, 8) >> >> +#define STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(x) FIELD_PREP(GENMASK(20, 17), x) >> >> +#define STG_SYSCON_AWFUNC_OFFSET 0x7c >> >> +#define STG_SYSCON_AXI4_SLVL_AWFUNC_MASK GENMASK(14, 0) >> >> +#define STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(x) FIELD_PREP(GENMASK(12, 9), x) >> >> +#define STG_SYSCON_CLKREQ BIT(22) >> >> +#define STG_SYSCON_CKREF_SRC_MASK GENMASK(19, 18) >> >> +#define STG_SYSCON_RP_NEP_OFFSET 0xe8 >> >> +#define STG_SYSCON_K_RP_NEP BIT(8) >> >> +#define STG_SYSCON_LNKSTA_OFFSET 0x170 >> >> +#define DATA_LINK_ACTIVE BIT(5) >> >> + >> >> +/* Parameters for the waiting for link up routine */ >> >> +#define LINK_WAIT_MAX_RETRIES 10 >> >> +#define LINK_WAIT_USLEEP_MIN 90000 >> >> +#define LINK_WAIT_USLEEP_MAX 100000 >> >> + >> >> +struct starfive_jh7110_pcie { >> >> + struct plda_pcie_rp plda; >> >> + struct reset_control *resets; >> >> + struct clk_bulk_data *clks; >> >> + struct regmap *reg_syscon; >> >> + struct gpio_desc *power_gpio; >> >> + struct gpio_desc *reset_gpio; >> >> + >> >> + u32 stg_arfun; >> >> + u32 stg_awfun; >> >> + u32 stg_rp_nep; >> >> + u32 stg_lnksta; >> >> + >> >> + int num_clks; >> >> +}; >> >> + >> >> +/* >> >> + * The BAR0/1 of bridge should be hidden during enumeration to >> >> + * avoid the sizing and resource allocation by PCIe core. >> >> + */ >> >> +static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int devfn, >> >> + int offset) >> >> +{ >> >> + if (pci_is_root_bus(bus) && !devfn && >> >> + (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1)) >> >> + return true; >> >> + >> >> + return false; >> >> +} >> >> + >> >> +static int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn, >> >> + int where, int size, u32 value) >> >> +{ >> >> + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) >> >> + return PCIBIOS_BAD_REGISTER_NUMBER; >> >> + >> >> + return pci_generic_config_write(bus, devfn, where, size, value); >> >> +} >> >> + >> >> +static int starfive_pcie_config_read(struct pci_bus *bus, unsigned int devfn, >> >> + int where, int size, u32 *value) >> >> +{ >> >> + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) >> >> + return PCIBIOS_BAD_REGISTER_NUMBER; >> >> + >> >> + return pci_generic_config_read(bus, devfn, where, size, value); >> >> +} >> >> + >> >> +static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie, struct device *dev) >> >> +{ >> >> + int domain_nr, stg_pcie_base; >> >> + >> >> + pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks); >> >> + if (pcie->num_clks < 0) >> >> + return dev_err_probe(dev, -ENODEV, >> >> + "failed to get pcie clocks\n"); >> >> + >> >> + pcie->resets = devm_reset_control_array_get_exclusive(dev); >> >> + if (IS_ERR(pcie->resets)) >> >> + return dev_err_probe(dev, PTR_ERR(pcie->resets), >> >> + "failed to get pcie resets"); >> >> + >> >> + pcie->reg_syscon = >> >> + syscon_regmap_lookup_by_phandle(dev->of_node, >> >> + "starfive,stg-syscon"); >> >> + >> >> + if (IS_ERR(pcie->reg_syscon)) >> >> + return dev_err_probe(dev, PTR_ERR(pcie->reg_syscon), >> >> + "failed to parse starfive,stg-syscon\n"); >> >> + >> >> + domain_nr = of_get_pci_domain_nr(dev->of_node); >> >> + >> >> + if (domain_nr < 0 || domain_nr > 1) >> >> + return dev_err_probe(dev, -ENODEV, >> >> + "failed to get valid pcie id\n"); >> >> + >> >> + if (domain_nr == 0) >> >> + stg_pcie_base = STG_SYSCON_PCIE0_BASE; >> >> + else >> >> + stg_pcie_base = STG_SYSCON_PCIE1_BASE; >> > >> > It seems you only need the domain_nr for the determining this offset into the >> > stg_syscon. Since this is already specific to the JH7110 SoC, wouldn't it be >> > safer to match on the 0x2b000000/0x2c000000 or 0x940000000/0x9c0000000 >> > address of the peripheral instead? >> > >> Maybe this codes not just bind to JH7110 SoC. > > Right, but in that case the STG_SYSCON_PCIE{0,1}_BASE offsets will also be > different, so you'd need to get them from device tree match data anyway, and > then you might as well have an { address, offset } table. > Previous I discuss this with Conor，The match data is not added now. The match data will be added While adding different device tree compatible. Now just one compatible. >> >> + >> >> + pcie->stg_arfun = stg_pcie_base + STG_SYSCON_ARFUNC_OFFSET; >> >> + pcie->stg_awfun = stg_pcie_base + STG_SYSCON_AWFUNC_OFFSET; >> >> + pcie->stg_rp_nep = stg_pcie_base + STG_SYSCON_RP_NEP_OFFSET; >> >> + pcie->stg_lnksta = stg_pcie_base + STG_SYSCON_LNKSTA_OFFSET; >> > >> > These offsets are all the same for both pcie0 and pcie1 so why do you need a >> > variable for each? Eg. why not just >> > >> > if (domain_nr == 0) >> > pcie->stg_base = STG_SYSCON_PCIE0_BASE; >> > else >> > pcie->stg_base = STG_SYSCON_PCIE1_BASE; >> > >> > ..and then add add those static offsets where needed? >> > >> OK. I will try to modify this. >> >> + >> >> + pcie->reset_gpio = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); >> >> + if (IS_ERR(pcie->reset_gpio)) { >> >> + dev_warn(dev, "failed to get perst-gpio\n"); >> > >> > Why is this a warn when you actually error out? I think you mean >> > >> > return dev_err_probe(dev, PTR_ERR(pcie->reset_gpio, "failed to get >> > reset GPIO\n"); >> > >> >> + return -EINVAL; >> >> + } >> >> + >> >> + pcie->power_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW); >> >> + if (IS_ERR(pcie->power_gpio)) { >> >> + dev_warn(dev, "failed to get power-gpio\n"); >> >> + return -EINVAL; >> > >> > return dev_err_probe(dev, PTR_ERR(pcie->power_gpio), "failed to get >> > power GPIO\n"); >> > >> >> + } >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static struct pci_ops starfive_pcie_ops = { >> >> + .map_bus = plda_pcie_map_bus, >> >> + .read = starfive_pcie_config_read, >> >> + .write = starfive_pcie_config_write, >> >> +}; >> >> + >> >> +static int starfive_pcie_clk_rst_init(struct starfive_jh7110_pcie *pcie) >> >> +{ >> >> + int ret; >> >> + struct device *dev = pcie->plda.dev; >> > >> > Again, please order initialized variables first. >> > >> >> + >> >> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); >> >> + if (ret) { >> >> + dev_err(dev, "failed to enable clocks\n"); >> >> + return ret; >> > >> > return dev_err_probe(dev, ret, "failed to enable clocks\n"); >> > >> >> + } >> >> + >> >> + ret = reset_control_deassert(pcie->resets); >> >> + if (ret) { >> >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); >> >> + dev_err(dev, "failed to resets\n"); >> > >> > I think you mean >> > return dev_err_probe(dev, ret, "failed to deassert resets\n"); >> > >> >> + } >> >> + >> >> + return ret; >> >> +} >> >> + >> >> +static void starfive_pcie_clk_rst_deinit(struct starfive_jh7110_pcie *pcie) >> >> +{ >> >> + reset_control_assert(pcie->resets); >> >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); >> >> +} >> >> + >> >> +static bool starfive_pcie_link_up(struct plda_pcie_rp *plda) >> >> +{ >> >> + struct starfive_jh7110_pcie *pcie = >> >> + container_of(plda, struct starfive_jh7110_pcie, plda); >> >> + struct device *dev = pcie->plda.dev; >> > >> > This is only used once below, so just use pcie->plda.dev directly there. >> > >> OK, thanks >> >> + int ret; >> >> + u32 stg_reg_val; >> >> + >> >> + ret = regmap_read(pcie->reg_syscon, pcie->stg_lnksta, &stg_reg_val); >> >> + if (ret) { >> >> + dev_err(dev, "failed to read link status\n"); >> >> + return false; >> >> + } >> >> + >> >> + if (stg_reg_val & DATA_LINK_ACTIVE) >> >> + return true; >> >> + >> >> + return false; >> > >> > These last 4 lines is just "return stg_reg_val & DATA_LINK_ACTIVE;" right? >> > >> OK >> >> +} >> >> + >> >> +static int starfive_pcie_host_wait_for_link(struct starfive_jh7110_pcie *pcie) >> >> +{ >> >> + struct device *dev = pcie->plda.dev; >> > >> > Only used once, just inline. >> > >> >> + int retries; >> >> + >> >> + /* Check if the link is up or not */ >> >> + for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { >> >> + if (starfive_pcie_link_up(&pcie->plda)) { >> >> + dev_info(dev, "port link up\n"); >> >> + return 0; >> >> + } >> >> + usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); >> >> + } >> >> + >> >> + return -ETIMEDOUT; >> >> +} >> >> + >> >> +static int starfive_pcie_enable_phy(struct device *dev, struct plda_pcie_rp *pcie) >> >> +{ >> >> + int ret; >> >> + >> >> + if (!pcie->phy) >> >> + return 0; >> >> + >> >> + ret = phy_init(pcie->phy); >> >> + if (ret) >> >> + return dev_err_probe(dev, ret, >> >> + "failed to initialize pcie phy\n"); >> >> + >> >> + ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE); >> >> + if (ret) { >> >> + dev_err(dev, "failed to set pcie mode\n"); >> >> + goto err_phy_on; >> >> + } >> >> + >> >> + ret = phy_power_on(pcie->phy); >> >> + if (ret) { >> >> + dev_err(dev, "failed to power on pcie phy\n"); >> >> + goto err_phy_on; >> >> + } >> >> + >> >> + return 0; >> >> + >> >> +err_phy_on: >> >> + phy_exit(pcie->phy); >> >> + return ret; >> >> +} >> >> + >> >> +static void starfive_pcie_disable_phy(struct plda_pcie_rp *pcie) >> >> +{ >> >> + phy_power_off(pcie->phy); >> >> + phy_exit(pcie->phy); >> >> +} >> >> + >> >> +static void starfive_pcie_host_deinit(struct plda_pcie_rp *plda) >> >> +{ >> >> + struct starfive_jh7110_pcie *pcie = >> >> + container_of(plda, struct starfive_jh7110_pcie, plda); >> >> + >> >> + starfive_pcie_clk_rst_deinit(pcie); >> >> + if (pcie->power_gpio) >> >> + gpiod_set_value_cansleep(pcie->power_gpio, 0); >> >> + starfive_pcie_disable_phy(plda); >> >> +} >> >> + >> >> +static int starfive_pcie_host_init(struct plda_pcie_rp *plda) >> >> +{ >> >> + int i; >> >> + struct starfive_jh7110_pcie *pcie = >> >> + container_of(plda, struct starfive_jh7110_pcie, plda); >> >> + struct device *dev = plda->dev; >> >> + int ret; >> > >> > Weird ordering again. >> > >> >> + >> >> + ret = starfive_pcie_enable_phy(dev, plda); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_rp_nep, >> >> + STG_SYSCON_K_RP_NEP, STG_SYSCON_K_RP_NEP); >> >> + >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, >> >> + STG_SYSCON_CKREF_SRC_MASK, >> >> + FIELD_PREP(STG_SYSCON_CKREF_SRC_MASK, 2)); >> >> + >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, >> >> + STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ); >> >> + >> >> + ret = starfive_pcie_clk_rst_init(pcie); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + if (pcie->power_gpio) >> >> + gpiod_set_value_cansleep(pcie->power_gpio, 1); >> >> + >> >> + if (pcie->reset_gpio) >> >> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); >> >> + >> >> + /* Disable physical functions except #0 */ >> >> + for (i = 1; i < PCIE_FUNC_NUM; i++) { >> >> + regmap_update_bits(pcie->reg_syscon, >> >> + pcie->stg_arfun, >> >> + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, >> >> + STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(i)); >> >> + >> >> + regmap_update_bits(pcie->reg_syscon, >> >> + pcie->stg_awfun, >> >> + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, >> >> + STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(i)); >> >> + >> >> + plda_pcie_disable_func(plda); >> >> + } >> >> + >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_arfun, >> >> + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, 0); >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, >> >> + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, 0); >> >> + >> >> + plda_pcie_enable_root_port(plda); >> >> + plda_pcie_write_rc_bar(plda, 0); >> >> + >> >> + /* PCIe PCI Standard Configuration Identification Settings. */ >> >> + plda_pcie_set_standard_class(plda); >> >> + >> >> + /* >> >> + * The LTR message forwarding of PCIe Message Reception was set by core >> >> + * as default, but the forward id & addr are also need to be reset. >> >> + * If we do not disable LTR message forwarding here, or set a legal >> >> + * forwarding address, the kernel will get stuck after this driver probe. >> >> + * To workaround, disable the LTR message forwarding support on >> >> + * PCIe Message Reception. >> >> + */ >> >> + plda_pcie_disable_ltr(plda); >> >> + >> >> + /* Prefetchable memory window 64-bit addressing support */ >> >> + plda_pcie_set_pref_win_64bit(plda); >> >> + >> >> + /* Ensure that PERST has been asserted for at least 100 ms, >> >> + * the sleep value is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4) >> >> + */ >> >> + msleep(100); >> >> + if (pcie->reset_gpio) >> >> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); >> >> + >> >> + /* As the requirement in PCIe base spec r6.0, system (<=5GT/s) must >> >> + * wait a minimum of 100 ms following exit from a conventional reset >> >> + * before sending a configuration request to the device. >> >> + */ >> >> + msleep(100); >> >> + >> >> + if (starfive_pcie_host_wait_for_link(pcie)) >> >> + dev_info(dev, "port link down\n"); >> >> + >> >> + return ret; >> >> +} >> >> + >> >> +static const struct plda_pcie_host_ops sf_host_ops = { >> >> + .host_init = starfive_pcie_host_init, >> >> + .host_deinit = starfive_pcie_host_deinit, >> >> +}; >> >> + >> >> +static int starfive_pcie_probe(struct platform_device *pdev) >> >> +{ >> >> + struct device *dev = &pdev->dev; >> >> + struct starfive_jh7110_pcie *pcie; >> >> + struct plda_pcie_rp *plda; >> >> + int ret; >> >> + >> >> + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); >> >> + if (!pcie) >> >> + return -ENOMEM; >> >> + >> >> + plda = &pcie->plda; >> >> + plda->dev = dev; >> >> + >> >> + ret = starfive_pcie_parse_dt(pcie, dev); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + plda->host_ops = &sf_host_ops; >> >> + plda->num_events = NUM_PLDA_EVENTS; >> >> + ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + pm_runtime_enable(&pdev->dev); >> >> + pm_runtime_get_sync(&pdev->dev); >> >> + platform_set_drvdata(pdev, pcie); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static void starfive_pcie_remove(struct platform_device *pdev) >> >> +{ >> >> + struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev); >> >> + >> >> + plda_pcie_host_deinit(&pcie->plda); >> >> + platform_set_drvdata(pdev, NULL); >> >> +} >> >> + >> >> +static int starfive_pcie_suspend_noirq(struct device *dev) >> >> +{ >> >> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); >> >> + >> >> + if (!pcie) >> >> + return 0; >> >> + >> >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); >> >> + starfive_pcie_disable_phy(&pcie->plda); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static int starfive_pcie_resume_noirq(struct device *dev) >> >> +{ >> >> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); >> >> + int ret; >> >> + >> >> + ret = starfive_pcie_enable_phy(dev, &pcie->plda); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); >> >> + if (ret) { >> >> + dev_err(dev, "failed to enable clocks\n"); >> >> + starfive_pcie_disable_phy(&pcie->plda); >> >> + return ret; >> >> + } >> >> + >> >> + return ret; >> >> +} >> >> + >> >> +static const struct dev_pm_ops starfive_pcie_pm_ops = { >> >> + NOIRQ_SYSTEM_SLEEP_PM_OPS(starfive_pcie_suspend_noirq, >> >> + starfive_pcie_resume_noirq) >> >> +}; >> >> + >> >> +static const struct of_device_id starfive_pcie_of_match[] = { >> >> + { .compatible = "starfive,jh7110-pcie"}, >> > >> > Missing space before } >> > >> >> + { /* sentinel */ } >> >> +}; >> >> +MODULE_DEVICE_TABLE(of, starfive_pcie_of_match); >> >> + >> >> +static struct platform_driver starfive_pcie_driver = { >> >> + .driver = { >> >> + .name = "pcie-starfive", >> >> + .of_match_table = of_match_ptr(starfive_pcie_of_match), >> >> +#ifdef CONFIG_PM_SLEEP >> >> + .pm = &starfive_pcie_pm_ops, >> >> +#endif >> > >> > This can be just .pm = pm_sleep_ptr(&starfive_pcie_pm_ops), >> > >> OK >> >> + }, >> >> + .probe = starfive_pcie_probe, >> >> + .remove_new = starfive_pcie_remove, >> >> +}; >> >> +module_platform_driver(starfive_pcie_driver); >> >> + >> >> +MODULE_DESCRIPTION("StarFive JH7110 PCIe host driver"); >> >> +MODULE_LICENSE("GPL v2"); >> >> -- >> >> 2.17.1
Minda Chen wrote: > > > On 2023/9/19 19:34, Emil Renner Berthing wrote: > > Minda Chen wrote: > >> > >> > >> On 2023/9/18 6:13, Emil Renner Berthing wrote: > >> > Minda Chen wrote: > >> >> Add StarFive JH7110 SoC PCIe controller platform > >> >> driver codes, and add host init/deinit to pcie-plda-host.c > >> >> > >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > >> >> Co-developed-by: Kevin Xie <kevin.xie@starfivetech.com> > >> >> Reviewed-by: Mason Huo <mason.huo@starfivetech.com> > >> >> --- > >> >> MAINTAINERS | 7 + > >> >> drivers/pci/controller/plda/Kconfig | 12 + > >> >> drivers/pci/controller/plda/Makefile | 1 + > >> >> drivers/pci/controller/plda/pcie-plda-host.c | 121 ++++- > >> >> drivers/pci/controller/plda/pcie-plda.h | 91 +++- > >> >> drivers/pci/controller/plda/pcie-starfive.c | 461 +++++++++++++++++++ > >> >> 6 files changed, 688 insertions(+), 5 deletions(-) > >> >> create mode 100644 drivers/pci/controller/plda/pcie-starfive.c > >> >> > >> >> diff --git a/MAINTAINERS b/MAINTAINERS > >> >> index ebdb65e195ee..4632cd411a3d 100644 > >> >> --- a/MAINTAINERS > >> >> +++ b/MAINTAINERS > >> >> @@ -16679,6 +16679,13 @@ S: Maintained > >> >> F: Documentation/devicetree/bindings/pci/socionext,uniphier-pcie* > >> >> F: drivers/pci/controller/dwc/pcie-uniphier* > >> >> > >> >> +PCIE DRIVER FOR STARFIVE JH71x0 > >> >> +M: Kevin Xie <kevin.xie@starfivetech.com> > >> >> +L: linux-pci@vger.kernel.org > >> >> +S: Maintained > >> >> +F: Documentation/devicetree/bindings/pci/starfive* > >> >> +F: drivers/pci/controller/plda/pcie-starfive.c > >> >> + > >> >> PCIE DRIVER FOR ST SPEAR13XX > >> >> M: Pratyush Anand <pratyush.anand@gmail.com> > >> >> L: linux-pci@vger.kernel.org > >> >> diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig > >> >> index e54a82ee94f5..8de661730aa5 100644 > >> >> --- a/drivers/pci/controller/plda/Kconfig > >> >> +++ b/drivers/pci/controller/plda/Kconfig > >> >> @@ -15,4 +15,16 @@ config PCIE_MICROCHIP_HOST > >> >> Say Y here if you want kernel to support the Microchip AXI PCIe > >> >> Host Bridge driver. > >> >> > >> >> +config PCIE_STARFIVE_HOST > >> >> + tristate "StarFive PCIe host controller" > >> >> + depends on OF && PCI_MSI > >> >> + select PCIE_PLDA_HOST > >> >> + help > >> >> + Say Y here if you want to support the StarFive PCIe controller > >> >> + in host mode. StarFive PCIe controller uses PLDA PCIe > >> >> + core. > >> >> + If you choose to build this driver as module it will > >> >> + be dynamically linked and module will be called > >> >> + pcie-starfive.ko > >> >> + > >> >> endmenu > >> >> diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile > >> >> index 4340ab007f44..0ac6851bed48 100644 > >> >> --- a/drivers/pci/controller/plda/Makefile > >> >> +++ b/drivers/pci/controller/plda/Makefile > >> >> @@ -1,3 +1,4 @@ > >> >> # SPDX-License-Identifier: GPL-2.0 > >> >> obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o > >> >> obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o > >> >> +obj-$(CONFIG_PCIE_STARFIVE_HOST) += pcie-starfive.o > >> >> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c > >> >> index 26e6cfd2eaba..8b32df477ad8 100644 > >> >> --- a/drivers/pci/controller/plda/pcie-plda-host.c > >> >> +++ b/drivers/pci/controller/plda/pcie-plda-host.c > >> >> @@ -20,6 +20,15 @@ > >> >> > >> >> #include "pcie-plda.h" > >> >> > >> >> +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, > >> >> + int where) > >> >> +{ > >> >> + struct plda_pcie_rp *pcie = bus->sysdata; > >> >> + > >> >> + return pcie->config_base + PCIE_ECAM_OFFSET(bus->number, devfn, where); > >> >> +} > >> >> +EXPORT_SYMBOL_GPL(plda_pcie_map_bus); > >> >> + > >> >> static void plda_handle_msi(struct irq_desc *desc) > >> >> { > >> >> struct plda_pcie_rp *port = irq_desc_get_handler_data(desc); > >> >> @@ -370,6 +379,12 @@ static const struct plda_event_ops plda_event_ops = { > >> >> .get_events = plda_get_events, > >> >> }; > >> >> > >> >> +static const struct plda_event plda_default_event = { > >> >> + .event_ops = &plda_event_ops, > >> >> + .intx_event = EVENT_PM_MSI_INT_INTX, > >> >> + .msi_event = EVENT_PM_MSI_INT_MSI > >> >> +}; > >> >> + > >> >> static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port, > >> >> const struct irq_domain_ops *ops) > >> >> { > >> >> @@ -416,7 +431,6 @@ int plda_init_interrupts(struct platform_device *pdev, > >> >> const struct plda_event *event) > >> >> { > >> >> struct device *dev = &pdev->dev; > >> >> - int irq; > >> >> int i, intx_irq, msi_irq, event_irq; > >> >> int ret; > >> >> const struct irq_domain_ops *irq_dom_ops; > >> >> @@ -434,8 +448,8 @@ int plda_init_interrupts(struct platform_device *pdev, > >> >> return ret; > >> >> } > >> >> > >> >> - irq = platform_get_irq(pdev, 0); > >> >> - if (irq < 0) > >> >> + port->irq = platform_get_irq(pdev, 0); > >> >> + if (port->irq < 0) > >> >> return -ENODEV; > >> > > >> > Here you're dropping the local variable irq and just use port->irq directly.. > >> > > >> >> > >> >> port->event_ops = event->event_ops; > >> >> @@ -465,6 +479,7 @@ int plda_init_interrupts(struct platform_device *pdev, > >> >> dev_err(dev, "failed to map INTx interrupt\n"); > >> >> return -ENXIO; > >> >> } > >> >> + port->intx_irq = intx_irq; > >> > > >> > ..but for intx_irq.. > >> > > >> >> > >> >> /* Plug the INTx chained handler */ > >> >> irq_set_chained_handler_and_data(intx_irq, plda_handle_intx, port); > >> >> @@ -473,12 +488,13 @@ int plda_init_interrupts(struct platform_device *pdev, > >> >> event->msi_event); > >> >> if (!msi_irq) > >> >> return -ENXIO; > >> >> + port->msi_irq = msi_irq; > >> > > >> > ..and msi_irq you're keeping the local variable. I think you should drop > >> > the local variables here too, or at least do the same for all of them. > >> > > >> OK, remove all the local variables is better. > >> >> > >> >> /* Plug the MSI chained handler */ > >> >> irq_set_chained_handler_and_data(msi_irq, plda_handle_msi, port); > >> >> > >> >> /* Plug the main event chained handler */ > >> >> - irq_set_chained_handler_and_data(irq, plda_handle_event, port); > >> >> + irq_set_chained_handler_and_data(port->irq, plda_handle_event, port); > >> >> > >> >> return 0; > >> >> } > >> >> @@ -544,3 +560,100 @@ int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > >> >> return 0; > >> >> } > >> >> EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); > >> >> + > >> >> +static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie) > >> >> +{ > >> >> + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL); > >> >> + irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL); > >> >> + irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL); > >> >> + > >> >> + irq_domain_remove(pcie->msi.msi_domain); > >> >> + irq_domain_remove(pcie->msi.dev_domain); > >> >> + > >> >> + irq_domain_remove(pcie->intx_domain); > >> >> + irq_domain_remove(pcie->event_domain); > >> >> +} > >> >> + > >> >> +int plda_pcie_host_init(struct plda_pcie_rp *port, struct pci_ops *ops) > >> >> +{ > >> >> + struct resource *cfg_res; > >> >> + struct device *dev = port->dev; > >> >> + int ret; > >> >> + struct pci_host_bridge *bridge; > >> >> + struct platform_device *pdev = to_platform_device(dev); > >> > > >> > This looks weird. Please order initialized variables first and maybe even do > >> > reverse christmas tree order. Eg. longeist lines first. > >> > > >> OK, The same with other places. > >> >> + > >> >> + port->bridge_addr = > >> >> + devm_platform_ioremap_resource_byname(pdev, "apb"); > >> >> + > >> >> + if (IS_ERR(port->bridge_addr)) > >> >> + return dev_err_probe(dev, PTR_ERR(port->bridge_addr), > >> >> + "failed to map reg memory\n"); > >> >> + > >> >> + cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); > >> >> + if (!cfg_res) > >> >> + return dev_err_probe(dev, -ENODEV, > >> >> + "failed to get config memory\n"); > >> >> + > >> >> + port->config_base = devm_ioremap_resource(dev, cfg_res); > >> >> + if (IS_ERR(port->config_base)) > >> >> + return dev_err_probe(dev, PTR_ERR(port->config_base), > >> >> + "failed to map config memory\n"); > >> >> + > >> >> + port->phy = devm_phy_optional_get(dev, NULL); > >> >> + if (IS_ERR(port->phy)) > >> >> + return dev_err_probe(dev, PTR_ERR(port->phy), > >> >> + "failed to get pcie phy\n"); > >> >> + > >> >> + bridge = devm_pci_alloc_host_bridge(dev, 0); > >> >> + if (!bridge) > >> >> + return dev_err_probe(dev, -ENOMEM, > >> >> + "failed to alloc bridge\n"); > >> >> + > >> >> + if (port->host_ops && port->host_ops->host_init) { > >> >> + ret = port->host_ops->host_init(port); > >> >> + if (ret) > >> >> + return ret; > >> >> + } > >> >> + > >> >> + port->bridge = bridge; > >> >> + plda_pcie_setup_window(port->bridge_addr, 0, cfg_res->start, 0, > >> >> + resource_size(cfg_res)); > >> >> + plda_pcie_setup_iomems(bridge, port); > >> >> + plda_set_default_msi(&port->msi); > >> >> + ret = plda_init_interrupts(pdev, port, &plda_default_event); > >> >> + if (ret) > >> >> + goto err_host; > >> >> + > >> >> + /* Set default bus ops */ > >> >> + bridge->ops = ops; > >> >> + bridge->sysdata = port; > >> >> + > >> >> + ret = pci_host_probe(bridge); > >> >> + if (ret < 0) { > >> >> + dev_err(dev, "failed to pci host probe: %d\n", ret); > >> > > >> > I think you mean: > >> > > >> > dev_err_probe(dev, ret, "failed to probe pci host\n"); > >> > > >> I check with dev_err_probe, It is better than dev_err, I will replace this. > >> >> + goto err_probe; > >> >> + } > >> >> + > >> >> + return ret; > >> >> + > >> >> +err_probe: > >> >> + plda_pcie_irq_domain_deinit(port); > >> >> +err_host: > >> >> + if (port->host_ops && port->host_ops->host_deinit) > >> >> + port->host_ops->host_deinit(port); > >> >> + > >> >> + return ret; > >> >> +} > >> >> +EXPORT_SYMBOL_GPL(plda_pcie_host_init); > >> >> + > >> >> +void plda_pcie_host_deinit(struct plda_pcie_rp *port) > >> >> +{ > >> >> + pci_stop_root_bus(port->bridge->bus); > >> >> + pci_remove_root_bus(port->bridge->bus); > >> >> + > >> >> + plda_pcie_irq_domain_deinit(port); > >> >> + > >> >> + if (port->host_ops && port->host_ops->host_deinit) > >> >> + port->host_ops->host_deinit(port); > >> >> +} > >> >> +EXPORT_SYMBOL_GPL(plda_pcie_host_deinit); > >> >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > >> >> index b7420598a326..e571047d7944 100644 > >> >> --- a/drivers/pci/controller/plda/pcie-plda.h > >> >> +++ b/drivers/pci/controller/plda/pcie-plda.h > >> >> @@ -6,14 +6,24 @@ > >> >> #ifndef _PCIE_PLDA_H > >> >> #define _PCIE_PLDA_H > >> >> > >> >> +#include <linux/phy/phy.h> > >> >> + > >> >> /* Number of MSI IRQs */ > >> >> #define PLDA_MAX_NUM_MSI_IRQS 32 > >> >> > >> >> /* PCIe Bridge Phy Regs */ > >> >> +#define GEN_SETTINGS 0x80 > >> >> +#define RP_ENABLE 1 > >> >> +#define PCIE_PCI_IDS_DW1 0x9c > >> >> +#define IDS_CLASS_CODE_SHIFT 16 > >> >> #define PCIE_PCI_IRQ_DW0 0xa8 > >> >> #define MSIX_CAP_MASK BIT(31) > >> >> #define NUM_MSI_MSGS_MASK GENMASK(6, 4) > >> >> #define NUM_MSI_MSGS_SHIFT 4 > >> >> +#define PCI_MISC 0xb4 > >> >> +#define PHY_FUNCTION_DIS BIT(15) > >> >> +#define PCIE_WINROM 0xfc > >> >> +#define PREF_MEM_WIN_64_SUPPORT BIT(3) > >> >> > >> >> #define IMASK_LOCAL 0x180 > >> >> #define DMA_END_ENGINE_0_MASK 0x00000000u > >> >> @@ -64,6 +74,8 @@ > >> >> #define ISTATUS_HOST 0x18c > >> >> #define IMSI_ADDR 0x190 > >> >> #define ISTATUS_MSI 0x194 > >> >> +#define PMSG_SUPPORT_RX 0x3f0 > >> >> +#define PMSG_LTR_SUPPORT BIT(2) > >> >> > >> >> /* PCIe Master table init defines */ > >> >> #define ATR0_PCIE_WIN0_SRCADDR_PARAM 0x600u > >> >> @@ -85,6 +97,8 @@ > >> >> #define PCIE_TX_RX_INTERFACE 0x00000000u > >> >> #define PCIE_CONFIG_INTERFACE 0x00000001u > >> >> > >> >> +#define CONFIG_SPACE_ADDR 0x1000u > >> >> + > >> >> #define ATR_ENTRY_SIZE 32 > >> >> > >> >> #define EVENT_A_ATR_EVT_POST_ERR 0 > >> >> @@ -134,6 +148,11 @@ struct plda_event_ops { > >> >> u32 (*get_events)(struct plda_pcie_rp *pcie); > >> >> }; > >> >> > >> >> +struct plda_pcie_host_ops { > >> >> + int (*host_init)(struct plda_pcie_rp *pcie); > >> >> + void (*host_deinit)(struct plda_pcie_rp *pcie); > >> >> +}; > >> >> + > >> >> struct plda_msi { > >> >> struct mutex lock; /* Protect used bitmap */ > >> >> struct irq_domain *msi_domain; > >> >> @@ -145,12 +164,19 @@ struct plda_msi { > >> >> > >> >> struct plda_pcie_rp { > >> >> struct device *dev; > >> >> + struct pci_host_bridge *bridge; > >> >> struct irq_domain *intx_domain; > >> >> struct irq_domain *event_domain; > >> >> raw_spinlock_t lock; > >> >> struct plda_msi msi; > >> >> const struct plda_event_ops *event_ops; > >> >> + const struct plda_pcie_host_ops *host_ops; > >> >> + struct phy *phy; > >> >> void __iomem *bridge_addr; > >> >> + void __iomem *config_base; > >> >> + int irq; > >> >> + int msi_irq; > >> >> + int intx_irq; > >> >> int num_events; > >> >> }; > >> >> > >> >> @@ -163,6 +189,7 @@ struct plda_event { > >> >> int msi_event; > >> >> }; > >> >> > >> >> +void __iomem *plda_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, int where); > >> >> int plda_init_interrupts(struct platform_device *pdev, > >> >> struct plda_pcie_rp *port, > >> >> const struct plda_event *event); > >> >> @@ -171,4 +198,66 @@ void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > >> >> size_t size); > >> >> int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > >> >> struct plda_pcie_rp *port); > >> >> -#endif > >> >> +int plda_pcie_host_init(struct plda_pcie_rp *pcie, struct pci_ops *ops); > >> >> +void plda_pcie_host_deinit(struct plda_pcie_rp *pcie); > >> >> + > >> >> +static inline void plda_set_default_msi(struct plda_msi *msi) > >> >> +{ > >> >> + msi->vector_phy = IMSI_ADDR; > >> >> + msi->num_vectors = PLDA_MAX_NUM_MSI_IRQS; > >> >> +} > >> >> + > >> >> +static inline void plda_pcie_enable_root_port(struct plda_pcie_rp *plda) > >> >> +{ > >> >> + u32 value; > >> >> + > >> >> + value = readl_relaxed(plda->bridge_addr + GEN_SETTINGS); > >> >> + value |= RP_ENABLE; > >> >> + writel_relaxed(value, plda->bridge_addr + GEN_SETTINGS); > >> >> +} > >> >> + > >> >> +static inline void plda_pcie_set_standard_class(struct plda_pcie_rp *plda) > >> >> +{ > >> >> + u32 value; > >> >> + > >> >> + value = readl_relaxed(plda->bridge_addr + PCIE_PCI_IDS_DW1); > >> >> + value &= 0xff; > >> >> + value |= (PCI_CLASS_BRIDGE_PCI << IDS_CLASS_CODE_SHIFT); > >> > > >> > This looks suspicious. You're clearing the lower 8 bits, but setting > >> > bits 16 and up. Also the parentheses are not strictly necessary. > >> > > >> I will add comments to this. > >> >> + writel_relaxed(value, plda->bridge_addr + PCIE_PCI_IDS_DW1); > >> >> +} > >> >> + > >> >> +static inline void plda_pcie_set_pref_win_64bit(struct plda_pcie_rp *plda) > >> >> +{ > >> >> + u32 value; > >> >> + > >> >> + value = readl_relaxed(plda->bridge_addr + PCIE_WINROM); > >> >> + value |= PREF_MEM_WIN_64_SUPPORT; > >> >> + writel_relaxed(value, plda->bridge_addr + PCIE_WINROM); > >> >> +} > >> >> + > >> >> +static inline void plda_pcie_disable_ltr(struct plda_pcie_rp *plda) > >> >> +{ > >> >> + u32 value; > >> >> + > >> >> + value = readl_relaxed(plda->bridge_addr + PMSG_SUPPORT_RX); > >> >> + value &= ~PMSG_LTR_SUPPORT; > >> >> + writel_relaxed(value, plda->bridge_addr + PMSG_SUPPORT_RX); > >> >> +} > >> >> + > >> >> +static inline void plda_pcie_disable_func(struct plda_pcie_rp *plda) > >> >> +{ > >> >> + u32 value; > >> >> + > >> >> + value = readl_relaxed(plda->bridge_addr + PCI_MISC); > >> >> + value |= PHY_FUNCTION_DIS; > >> >> + writel_relaxed(value, plda->bridge_addr + PCI_MISC); > >> >> +} > >> >> + > >> >> +static inline void plda_pcie_write_rc_bar(struct plda_pcie_rp *plda, u64 val) > >> >> +{ > >> >> + void __iomem *addr = plda->bridge_addr + CONFIG_SPACE_ADDR; > >> >> + > >> >> + writel_relaxed(val & 0xffffffff, addr + PCI_BASE_ADDRESS_0); > >> >> + writel_relaxed(val >> 32, addr + PCI_BASE_ADDRESS_1); > >> > > >> > Consider using lower_32_bits(val) and upper_32_bits(val) here. > >> > > >> >> +} > >> >> +#endif /* _PCIE_PLDA_H */ > >> >> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c > >> >> new file mode 100644 > >> >> index 000000000000..42099fef0d7d > >> >> --- /dev/null > >> >> +++ b/drivers/pci/controller/plda/pcie-starfive.c > >> >> @@ -0,0 +1,461 @@ > >> >> +// SPDX-License-Identifier: GPL-2.0+ > >> >> +/* > >> >> + * PCIe host controller driver for StarFive JH7110 Soc. > >> >> + * > >> >> + * Copyright (C) 2023 StarFive Technology Co., Ltd. > >> >> + */ > >> >> + > >> >> +#include <linux/bitfield.h> > >> >> +#include <linux/clk.h> > >> >> +#include <linux/delay.h> > >> >> +#include <linux/gpio/consumer.h> > >> >> +#include <linux/interrupt.h> > >> >> +#include <linux/kernel.h> > >> >> +#include <linux/mfd/syscon.h> > >> >> +#include <linux/module.h> > >> >> +#include <linux/of_address.h> > >> >> +#include <linux/of_irq.h> > >> >> +#include <linux/of_pci.h> > >> >> +#include <linux/pci.h> > >> >> +#include <linux/platform_device.h> > >> >> +#include <linux/pm_runtime.h> > >> >> +#include <linux/regmap.h> > >> >> +#include <linux/reset.h> > >> >> +#include "../../pci.h" > >> >> + > >> >> +#include "pcie-plda.h" > >> >> + > >> >> +#define PCIE_FUNC_NUM 4 > >> >> + > >> >> +/* system control */ > >> >> +#define STG_SYSCON_PCIE0_BASE 0x48 > >> >> +#define STG_SYSCON_PCIE1_BASE 0x1f8 > >> >> + > >> >> +#define STG_SYSCON_ARFUNC_OFFSET 0x78 > >> >> +#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK GENMASK(22, 8) > >> >> +#define STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(x) FIELD_PREP(GENMASK(20, 17), x) > >> >> +#define STG_SYSCON_AWFUNC_OFFSET 0x7c > >> >> +#define STG_SYSCON_AXI4_SLVL_AWFUNC_MASK GENMASK(14, 0) > >> >> +#define STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(x) FIELD_PREP(GENMASK(12, 9), x) > >> >> +#define STG_SYSCON_CLKREQ BIT(22) > >> >> +#define STG_SYSCON_CKREF_SRC_MASK GENMASK(19, 18) > >> >> +#define STG_SYSCON_RP_NEP_OFFSET 0xe8 > >> >> +#define STG_SYSCON_K_RP_NEP BIT(8) > >> >> +#define STG_SYSCON_LNKSTA_OFFSET 0x170 > >> >> +#define DATA_LINK_ACTIVE BIT(5) > >> >> + > >> >> +/* Parameters for the waiting for link up routine */ > >> >> +#define LINK_WAIT_MAX_RETRIES 10 > >> >> +#define LINK_WAIT_USLEEP_MIN 90000 > >> >> +#define LINK_WAIT_USLEEP_MAX 100000 > >> >> + > >> >> +struct starfive_jh7110_pcie { > >> >> + struct plda_pcie_rp plda; > >> >> + struct reset_control *resets; > >> >> + struct clk_bulk_data *clks; > >> >> + struct regmap *reg_syscon; > >> >> + struct gpio_desc *power_gpio; > >> >> + struct gpio_desc *reset_gpio; > >> >> + > >> >> + u32 stg_arfun; > >> >> + u32 stg_awfun; > >> >> + u32 stg_rp_nep; > >> >> + u32 stg_lnksta; > >> >> + > >> >> + int num_clks; > >> >> +}; > >> >> + > >> >> +/* > >> >> + * The BAR0/1 of bridge should be hidden during enumeration to > >> >> + * avoid the sizing and resource allocation by PCIe core. > >> >> + */ > >> >> +static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int devfn, > >> >> + int offset) > >> >> +{ > >> >> + if (pci_is_root_bus(bus) && !devfn && > >> >> + (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1)) > >> >> + return true; > >> >> + > >> >> + return false; > >> >> +} > >> >> + > >> >> +static int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn, > >> >> + int where, int size, u32 value) > >> >> +{ > >> >> + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > >> >> + return PCIBIOS_BAD_REGISTER_NUMBER; > >> >> + > >> >> + return pci_generic_config_write(bus, devfn, where, size, value); > >> >> +} > >> >> + > >> >> +static int starfive_pcie_config_read(struct pci_bus *bus, unsigned int devfn, > >> >> + int where, int size, u32 *value) > >> >> +{ > >> >> + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > >> >> + return PCIBIOS_BAD_REGISTER_NUMBER; > >> >> + > >> >> + return pci_generic_config_read(bus, devfn, where, size, value); > >> >> +} > >> >> + > >> >> +static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie, struct device *dev) > >> >> +{ > >> >> + int domain_nr, stg_pcie_base; > >> >> + > >> >> + pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks); > >> >> + if (pcie->num_clks < 0) > >> >> + return dev_err_probe(dev, -ENODEV, > >> >> + "failed to get pcie clocks\n"); > >> >> + > >> >> + pcie->resets = devm_reset_control_array_get_exclusive(dev); > >> >> + if (IS_ERR(pcie->resets)) > >> >> + return dev_err_probe(dev, PTR_ERR(pcie->resets), > >> >> + "failed to get pcie resets"); > >> >> + > >> >> + pcie->reg_syscon = > >> >> + syscon_regmap_lookup_by_phandle(dev->of_node, > >> >> + "starfive,stg-syscon"); > >> >> + > >> >> + if (IS_ERR(pcie->reg_syscon)) > >> >> + return dev_err_probe(dev, PTR_ERR(pcie->reg_syscon), > >> >> + "failed to parse starfive,stg-syscon\n"); > >> >> + > >> >> + domain_nr = of_get_pci_domain_nr(dev->of_node); > >> >> + > >> >> + if (domain_nr < 0 || domain_nr > 1) > >> >> + return dev_err_probe(dev, -ENODEV, > >> >> + "failed to get valid pcie id\n"); > >> >> + > >> >> + if (domain_nr == 0) > >> >> + stg_pcie_base = STG_SYSCON_PCIE0_BASE; > >> >> + else > >> >> + stg_pcie_base = STG_SYSCON_PCIE1_BASE; > >> > > >> > It seems you only need the domain_nr for the determining this offset into the > >> > stg_syscon. Since this is already specific to the JH7110 SoC, wouldn't it be > >> > safer to match on the 0x2b000000/0x2c000000 or 0x940000000/0x9c0000000 > >> > address of the peripheral instead? > >> > > >> Maybe this codes not just bind to JH7110 SoC. > > > > Right, but in that case the STG_SYSCON_PCIE{0,1}_BASE offsets will also be > > different, so you'd need to get them from device tree match data anyway, and > > then you might as well have an { address, offset } table. > > Previous I discuss this with Conor，The match data is not added now. > The match data will be added While adding different device tree compatible. > Now just one compatible. Exactly. So for now there should be now problem matching on the peripheral address. > >> >> + > >> >> + pcie->stg_arfun = stg_pcie_base + STG_SYSCON_ARFUNC_OFFSET; > >> >> + pcie->stg_awfun = stg_pcie_base + STG_SYSCON_AWFUNC_OFFSET; > >> >> + pcie->stg_rp_nep = stg_pcie_base + STG_SYSCON_RP_NEP_OFFSET; > >> >> + pcie->stg_lnksta = stg_pcie_base + STG_SYSCON_LNKSTA_OFFSET; > >> > > >> > These offsets are all the same for both pcie0 and pcie1 so why do you need a > >> > variable for each? Eg. why not just > >> > > >> > if (domain_nr == 0) > >> > pcie->stg_base = STG_SYSCON_PCIE0_BASE; > >> > else > >> > pcie->stg_base = STG_SYSCON_PCIE1_BASE; > >> > > >> > ..and then add add those static offsets where needed? > >> > > >> OK. I will try to modify this. > >> >> + > >> >> + pcie->reset_gpio = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); > >> >> + if (IS_ERR(pcie->reset_gpio)) { > >> >> + dev_warn(dev, "failed to get perst-gpio\n"); > >> > > >> > Why is this a warn when you actually error out? I think you mean > >> > > >> > return dev_err_probe(dev, PTR_ERR(pcie->reset_gpio, "failed to get > >> > reset GPIO\n"); > >> > > >> >> + return -EINVAL; > >> >> + } > >> >> + > >> >> + pcie->power_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW); > >> >> + if (IS_ERR(pcie->power_gpio)) { > >> >> + dev_warn(dev, "failed to get power-gpio\n"); > >> >> + return -EINVAL; > >> > > >> > return dev_err_probe(dev, PTR_ERR(pcie->power_gpio), "failed to get > >> > power GPIO\n"); > >> > > >> >> + } > >> >> + > >> >> + return 0; > >> >> +} > >> >> + > >> >> +static struct pci_ops starfive_pcie_ops = { > >> >> + .map_bus = plda_pcie_map_bus, > >> >> + .read = starfive_pcie_config_read, > >> >> + .write = starfive_pcie_config_write, > >> >> +}; > >> >> + > >> >> +static int starfive_pcie_clk_rst_init(struct starfive_jh7110_pcie *pcie) > >> >> +{ > >> >> + int ret; > >> >> + struct device *dev = pcie->plda.dev; > >> > > >> > Again, please order initialized variables first. > >> > > >> >> + > >> >> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); > >> >> + if (ret) { > >> >> + dev_err(dev, "failed to enable clocks\n"); > >> >> + return ret; > >> > > >> > return dev_err_probe(dev, ret, "failed to enable clocks\n"); > >> > > >> >> + } > >> >> + > >> >> + ret = reset_control_deassert(pcie->resets); > >> >> + if (ret) { > >> >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > >> >> + dev_err(dev, "failed to resets\n"); > >> > > >> > I think you mean > >> > return dev_err_probe(dev, ret, "failed to deassert resets\n"); > >> > > >> >> + } > >> >> + > >> >> + return ret; > >> >> +} > >> >> + > >> >> +static void starfive_pcie_clk_rst_deinit(struct starfive_jh7110_pcie *pcie) > >> >> +{ > >> >> + reset_control_assert(pcie->resets); > >> >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > >> >> +} > >> >> + > >> >> +static bool starfive_pcie_link_up(struct plda_pcie_rp *plda) > >> >> +{ > >> >> + struct starfive_jh7110_pcie *pcie = > >> >> + container_of(plda, struct starfive_jh7110_pcie, plda); > >> >> + struct device *dev = pcie->plda.dev; > >> > > >> > This is only used once below, so just use pcie->plda.dev directly there. > >> > > >> OK, thanks > >> >> + int ret; > >> >> + u32 stg_reg_val; > >> >> + > >> >> + ret = regmap_read(pcie->reg_syscon, pcie->stg_lnksta, &stg_reg_val); > >> >> + if (ret) { > >> >> + dev_err(dev, "failed to read link status\n"); > >> >> + return false; > >> >> + } > >> >> + > >> >> + if (stg_reg_val & DATA_LINK_ACTIVE) > >> >> + return true; > >> >> + > >> >> + return false; > >> > > >> > These last 4 lines is just "return stg_reg_val & DATA_LINK_ACTIVE;" right? > >> > > >> OK > >> >> +} > >> >> + > >> >> +static int starfive_pcie_host_wait_for_link(struct starfive_jh7110_pcie *pcie) > >> >> +{ > >> >> + struct device *dev = pcie->plda.dev; > >> > > >> > Only used once, just inline. > >> > > >> >> + int retries; > >> >> + > >> >> + /* Check if the link is up or not */ > >> >> + for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { > >> >> + if (starfive_pcie_link_up(&pcie->plda)) { > >> >> + dev_info(dev, "port link up\n"); > >> >> + return 0; > >> >> + } > >> >> + usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > >> >> + } > >> >> + > >> >> + return -ETIMEDOUT; > >> >> +} > >> >> + > >> >> +static int starfive_pcie_enable_phy(struct device *dev, struct plda_pcie_rp *pcie) > >> >> +{ > >> >> + int ret; > >> >> + > >> >> + if (!pcie->phy) > >> >> + return 0; > >> >> + > >> >> + ret = phy_init(pcie->phy); > >> >> + if (ret) > >> >> + return dev_err_probe(dev, ret, > >> >> + "failed to initialize pcie phy\n"); > >> >> + > >> >> + ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE); > >> >> + if (ret) { > >> >> + dev_err(dev, "failed to set pcie mode\n"); > >> >> + goto err_phy_on; > >> >> + } > >> >> + > >> >> + ret = phy_power_on(pcie->phy); > >> >> + if (ret) { > >> >> + dev_err(dev, "failed to power on pcie phy\n"); > >> >> + goto err_phy_on; > >> >> + } > >> >> + > >> >> + return 0; > >> >> + > >> >> +err_phy_on: > >> >> + phy_exit(pcie->phy); > >> >> + return ret; > >> >> +} > >> >> + > >> >> +static void starfive_pcie_disable_phy(struct plda_pcie_rp *pcie) > >> >> +{ > >> >> + phy_power_off(pcie->phy); > >> >> + phy_exit(pcie->phy); > >> >> +} > >> >> + > >> >> +static void starfive_pcie_host_deinit(struct plda_pcie_rp *plda) > >> >> +{ > >> >> + struct starfive_jh7110_pcie *pcie = > >> >> + container_of(plda, struct starfive_jh7110_pcie, plda); > >> >> + > >> >> + starfive_pcie_clk_rst_deinit(pcie); > >> >> + if (pcie->power_gpio) > >> >> + gpiod_set_value_cansleep(pcie->power_gpio, 0); > >> >> + starfive_pcie_disable_phy(plda); > >> >> +} > >> >> + > >> >> +static int starfive_pcie_host_init(struct plda_pcie_rp *plda) > >> >> +{ > >> >> + int i; > >> >> + struct starfive_jh7110_pcie *pcie = > >> >> + container_of(plda, struct starfive_jh7110_pcie, plda); > >> >> + struct device *dev = plda->dev; > >> >> + int ret; > >> > > >> > Weird ordering again. > >> > > >> >> + > >> >> + ret = starfive_pcie_enable_phy(dev, plda); > >> >> + if (ret) > >> >> + return ret; > >> >> + > >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_rp_nep, > >> >> + STG_SYSCON_K_RP_NEP, STG_SYSCON_K_RP_NEP); > >> >> + > >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > >> >> + STG_SYSCON_CKREF_SRC_MASK, > >> >> + FIELD_PREP(STG_SYSCON_CKREF_SRC_MASK, 2)); > >> >> + > >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > >> >> + STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ); > >> >> + > >> >> + ret = starfive_pcie_clk_rst_init(pcie); > >> >> + if (ret) > >> >> + return ret; > >> >> + > >> >> + if (pcie->power_gpio) > >> >> + gpiod_set_value_cansleep(pcie->power_gpio, 1); > >> >> + > >> >> + if (pcie->reset_gpio) > >> >> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > >> >> + > >> >> + /* Disable physical functions except #0 */ > >> >> + for (i = 1; i < PCIE_FUNC_NUM; i++) { > >> >> + regmap_update_bits(pcie->reg_syscon, > >> >> + pcie->stg_arfun, > >> >> + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, > >> >> + STG_SYSCON_AXI4_SLVL_PHY_ARFUNC(i)); > >> >> + > >> >> + regmap_update_bits(pcie->reg_syscon, > >> >> + pcie->stg_awfun, > >> >> + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, > >> >> + STG_SYSCON_AXI4_SLVL_PHY_AWFUNC(i)); > >> >> + > >> >> + plda_pcie_disable_func(plda); > >> >> + } > >> >> + > >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_arfun, > >> >> + STG_SYSCON_AXI4_SLVL_ARFUNC_MASK, 0); > >> >> + regmap_update_bits(pcie->reg_syscon, pcie->stg_awfun, > >> >> + STG_SYSCON_AXI4_SLVL_AWFUNC_MASK, 0); > >> >> + > >> >> + plda_pcie_enable_root_port(plda); > >> >> + plda_pcie_write_rc_bar(plda, 0); > >> >> + > >> >> + /* PCIe PCI Standard Configuration Identification Settings. */ > >> >> + plda_pcie_set_standard_class(plda); > >> >> + > >> >> + /* > >> >> + * The LTR message forwarding of PCIe Message Reception was set by core > >> >> + * as default, but the forward id & addr are also need to be reset. > >> >> + * If we do not disable LTR message forwarding here, or set a legal > >> >> + * forwarding address, the kernel will get stuck after this driver probe. > >> >> + * To workaround, disable the LTR message forwarding support on > >> >> + * PCIe Message Reception. > >> >> + */ > >> >> + plda_pcie_disable_ltr(plda); > >> >> + > >> >> + /* Prefetchable memory window 64-bit addressing support */ > >> >> + plda_pcie_set_pref_win_64bit(plda); > >> >> + > >> >> + /* Ensure that PERST has been asserted for at least 100 ms, > >> >> + * the sleep value is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4) > >> >> + */ > >> >> + msleep(100); > >> >> + if (pcie->reset_gpio) > >> >> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > >> >> + > >> >> + /* As the requirement in PCIe base spec r6.0, system (<=5GT/s) must > >> >> + * wait a minimum of 100 ms following exit from a conventional reset > >> >> + * before sending a configuration request to the device. > >> >> + */ > >> >> + msleep(100); > >> >> + > >> >> + if (starfive_pcie_host_wait_for_link(pcie)) > >> >> + dev_info(dev, "port link down\n"); > >> >> + > >> >> + return ret; > >> >> +} > >> >> + > >> >> +static const struct plda_pcie_host_ops sf_host_ops = { > >> >> + .host_init = starfive_pcie_host_init, > >> >> + .host_deinit = starfive_pcie_host_deinit, > >> >> +}; > >> >> + > >> >> +static int starfive_pcie_probe(struct platform_device *pdev) > >> >> +{ > >> >> + struct device *dev = &pdev->dev; > >> >> + struct starfive_jh7110_pcie *pcie; > >> >> + struct plda_pcie_rp *plda; > >> >> + int ret; > >> >> + > >> >> + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > >> >> + if (!pcie) > >> >> + return -ENOMEM; > >> >> + > >> >> + plda = &pcie->plda; > >> >> + plda->dev = dev; > >> >> + > >> >> + ret = starfive_pcie_parse_dt(pcie, dev); > >> >> + if (ret) > >> >> + return ret; > >> >> + > >> >> + plda->host_ops = &sf_host_ops; > >> >> + plda->num_events = NUM_PLDA_EVENTS; > >> >> + ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops); > >> >> + if (ret) > >> >> + return ret; > >> >> + > >> >> + pm_runtime_enable(&pdev->dev); > >> >> + pm_runtime_get_sync(&pdev->dev); > >> >> + platform_set_drvdata(pdev, pcie); > >> >> + > >> >> + return 0; > >> >> +} > >> >> + > >> >> +static void starfive_pcie_remove(struct platform_device *pdev) > >> >> +{ > >> >> + struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev); > >> >> + > >> >> + plda_pcie_host_deinit(&pcie->plda); > >> >> + platform_set_drvdata(pdev, NULL); > >> >> +} > >> >> + > >> >> +static int starfive_pcie_suspend_noirq(struct device *dev) > >> >> +{ > >> >> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > >> >> + > >> >> + if (!pcie) > >> >> + return 0; > >> >> + > >> >> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); > >> >> + starfive_pcie_disable_phy(&pcie->plda); > >> >> + > >> >> + return 0; > >> >> +} > >> >> + > >> >> +static int starfive_pcie_resume_noirq(struct device *dev) > >> >> +{ > >> >> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > >> >> + int ret; > >> >> + > >> >> + ret = starfive_pcie_enable_phy(dev, &pcie->plda); > >> >> + if (ret) > >> >> + return ret; > >> >> + > >> >> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks); > >> >> + if (ret) { > >> >> + dev_err(dev, "failed to enable clocks\n"); > >> >> + starfive_pcie_disable_phy(&pcie->plda); > >> >> + return ret; > >> >> + } > >> >> + > >> >> + return ret; > >> >> +} > >> >> + > >> >> +static const struct dev_pm_ops starfive_pcie_pm_ops = { > >> >> + NOIRQ_SYSTEM_SLEEP_PM_OPS(starfive_pcie_suspend_noirq, > >> >> + starfive_pcie_resume_noirq) > >> >> +}; > >> >> + > >> >> +static const struct of_device_id starfive_pcie_of_match[] = { > >> >> + { .compatible = "starfive,jh7110-pcie"}, > >> > > >> > Missing space before } > >> > > >> >> + { /* sentinel */ } > >> >> +}; > >> >> +MODULE_DEVICE_TABLE(of, starfive_pcie_of_match); > >> >> + > >> >> +static struct platform_driver starfive_pcie_driver = { > >> >> + .driver = { > >> >> + .name = "pcie-starfive", > >> >> + .of_match_table = of_match_ptr(starfive_pcie_of_match), > >> >> +#ifdef CONFIG_PM_SLEEP > >> >> + .pm = &starfive_pcie_pm_ops, > >> >> +#endif > >> > > >> > This can be just .pm = pm_sleep_ptr(&starfive_pcie_pm_ops), > >> > > >> OK > >> >> + }, > >> >> + .probe = starfive_pcie_probe, > >> >> + .remove_new = starfive_pcie_remove, > >> >> +}; > >> >> +module_platform_driver(starfive_pcie_driver); > >> >> + > >> >> +MODULE_DESCRIPTION("StarFive JH7110 PCIe host driver"); > >> >> +MODULE_LICENSE("GPL v2"); > >> >> -- > >> >> 2.17.1 > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Sat, Sep 16, 2023 at 01:11:29AM +0100, Conor Dooley wrote: > On Fri, Sep 15, 2023 at 06:22:32PM +0800, Minda Chen wrote: > > For PolarFire implements non-PLDA local interrupt events, most of > > event interrupt process codes can not be re-used. PLDA implements > > new codes and IRQ domain ops like PolarFire. > > > > plda_get_events() adds a new IRQ num to event num mapping codes for > > PLDA local event except DMA engine interrupt events. The DMA engine > > interrupt events are implemented by vendors. > > > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > > Perhaps not important as they will go away in the next patch, but for > this patch the riscv patchwork stuff noticed: > drivers/pci/controller/plda/pcie-plda-host.c:114:36: warning: unused variable 'plda_evt_dom_ops' [-Wunused-const-variable] > drivers/pci/controller/plda/pcie-plda-host.c:118:36: warning: unused variable 'plda_event_ops' [-Wunused-const-variable] Details like this *are* important because fixing them makes the individual patches more readable, thanks for noticing! Bjorn
On 2023/9/21 23:08, Bjorn Helgaas wrote: > On Sat, Sep 16, 2023 at 01:11:29AM +0100, Conor Dooley wrote: >> On Fri, Sep 15, 2023 at 06:22:32PM +0800, Minda Chen wrote: >> > For PolarFire implements non-PLDA local interrupt events, most of >> > event interrupt process codes can not be re-used. PLDA implements >> > new codes and IRQ domain ops like PolarFire. >> > >> > plda_get_events() adds a new IRQ num to event num mapping codes for >> > PLDA local event except DMA engine interrupt events. The DMA engine >> > interrupt events are implemented by vendors. >> > >> > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> >> Perhaps not important as they will go away in the next patch, but for >> this patch the riscv patchwork stuff noticed: >> drivers/pci/controller/plda/pcie-plda-host.c:114:36: warning: unused variable 'plda_evt_dom_ops' [-Wunused-const-variable] >> drivers/pci/controller/plda/pcie-plda-host.c:118:36: warning: unused variable 'plda_event_ops' [-Wunused-const-variable] > > Details like this *are* important because fixing them makes the > individual patches more readable, thanks for noticing! > > Bjorn OK, I will solve this.
On 2023/9/18 6:45, Emil Renner Berthing wrote: > Minda Chen wrote: >> Move setup functions to common pcie-plda-host.c. >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> drivers/pci/controller/plda/Kconfig | 4 + >> drivers/pci/controller/plda/Makefile | 1 + >> .../pci/controller/plda/pcie-microchip-host.c | 59 ------------- >> drivers/pci/controller/plda/pcie-plda-host.c | 82 +++++++++++++++++++ >> drivers/pci/controller/plda/pcie-plda.h | 6 ++ >> 5 files changed, 93 insertions(+), 59 deletions(-) >> create mode 100644 drivers/pci/controller/plda/pcie-plda-host.c >> >> diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig >> index 5cb3be4fc98c..e54a82ee94f5 100644 >> --- a/drivers/pci/controller/plda/Kconfig >> +++ b/drivers/pci/controller/plda/Kconfig >> @@ -3,10 +3,14 @@ >> menu "PLDA-based PCIe controllers" >> depends on PCI >> >> +config PCIE_PLDA_HOST >> + bool >> + >> config PCIE_MICROCHIP_HOST >> tristate "Microchip AXI PCIe controller" >> depends on PCI_MSI && OF >> select PCI_HOST_COMMON >> + select PCIE_PLDA_HOST >> help >> Say Y here if you want kernel to support the Microchip AXI PCIe >> Host Bridge driver. >> diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile >> index e1a265cbf91c..4340ab007f44 100644 >> --- a/drivers/pci/controller/plda/Makefile >> +++ b/drivers/pci/controller/plda/Makefile >> @@ -1,2 +1,3 @@ >> # SPDX-License-Identifier: GPL-2.0 >> +obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o >> obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o >> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c >> index 1d253acd6bc2..ac7126b0bacf 100644 >> --- a/drivers/pci/controller/plda/pcie-microchip-host.c >> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c >> @@ -837,65 +837,6 @@ static int mc_pcie_init_irq_domains(struct plda_pcie_rp *port) >> return mc_allocate_msi_domains(port); >> } >> >> -static void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> - phys_addr_t axi_addr, phys_addr_t pci_addr, >> - size_t size) >> -{ >> - u32 atr_sz = ilog2(size) - 1; >> - u32 val; >> - >> - if (index == 0) >> - val = PCIE_CONFIG_INTERFACE; >> - else >> - val = PCIE_TX_RX_INTERFACE; >> - >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_TRSL_PARAM); >> - >> - val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) | >> - ATR_IMPL_ENABLE; >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_SRCADDR_PARAM); >> - >> - val = upper_32_bits(axi_addr); >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_SRC_ADDR); >> - >> - val = lower_32_bits(pci_addr); >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_TRSL_ADDR_LSB); >> - >> - val = upper_32_bits(pci_addr); >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_TRSL_ADDR_UDW); >> - >> - val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); >> - val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); >> - writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); >> - writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); >> -} >> - >> -static int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> - struct plda_pcie_rp *port) >> -{ >> - void __iomem *bridge_base_addr = port->bridge_addr; >> - struct resource_entry *entry; >> - u64 pci_addr; >> - u32 index = 1; >> - >> - resource_list_for_each_entry(entry, &bridge->windows) { >> - if (resource_type(entry->res) == IORESOURCE_MEM) { >> - pci_addr = entry->res->start - entry->offset; >> - plda_pcie_setup_window(bridge_base_addr, index, >> - entry->res->start, pci_addr, >> - resource_size(entry->res)); >> - index++; >> - } >> - } >> - >> - return 0; >> -} >> - >> static inline void mc_clear_secs(struct mc_pcie *port) >> { >> void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR; >> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c >> new file mode 100644 >> index 000000000000..f0c7636f1f64 >> --- /dev/null >> +++ b/drivers/pci/controller/plda/pcie-plda-host.c >> @@ -0,0 +1,82 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PLDA PCIe XpressRich host controller driver >> + * >> + * Copyright (C) 2023 Microchip Co. Ltd >> + * StarFive Co. Ltd. >> + * >> + * Author: Daire McNamara <daire.mcnamara@microchip.com> >> + * Author: Minda Chen <minda.chen@starfivetech.com> >> + */ >> + >> +#include <linux/irqchip/chained_irq.h> >> +#include <linux/irqdomain.h> >> +#include <linux/msi.h> >> +#include <linux/of_address.h> >> +#include <linux/of_pci.h> >> +#include <linux/pci_regs.h> >> +#include <linux/pci-ecam.h> >> +#include <linux/platform_device.h> >> + >> +#include "pcie-plda.h" >> + >> +void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> + phys_addr_t axi_addr, phys_addr_t pci_addr, >> + size_t size) >> +{ >> + u32 atr_sz = ilog2(size) - 1; >> + u32 val; >> + >> + if (index == 0) >> + val = PCIE_CONFIG_INTERFACE; >> + else >> + val = PCIE_TX_RX_INTERFACE; >> + >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_TRSL_PARAM); >> + >> + val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) | >> + ATR_IMPL_ENABLE; >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_SRCADDR_PARAM); >> + >> + val = upper_32_bits(axi_addr); >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_SRC_ADDR); >> + >> + val = lower_32_bits(pci_addr); >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_TRSL_ADDR_LSB); >> + >> + val = upper_32_bits(pci_addr); >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_TRSL_ADDR_UDW); >> + >> + val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); >> + val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); > > I know this code is just a straight copy, but for future cleanups are we > guaranteed that this field is always zero? > > Otherwise it looks a little suspicious to do read-modify-write, but just > set the (0x25 << 1) bits without clearing the field first. > The reset value is zero. And this is called once. Maybe I can add a patch to fix this. >> + writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); >> + writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); >> +} >> +EXPORT_SYMBOL_GPL(plda_pcie_setup_window); >> + >> +int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> + struct plda_pcie_rp *port) >> +{ >> + void __iomem *bridge_base_addr = port->bridge_addr; >> + struct resource_entry *entry; >> + u64 pci_addr; >> + u32 index = 1; >> + >> + resource_list_for_each_entry(entry, &bridge->windows) { >> + if (resource_type(entry->res) == IORESOURCE_MEM) { >> + pci_addr = entry->res->start - entry->offset; >> + plda_pcie_setup_window(bridge_base_addr, index, >> + entry->res->start, pci_addr, >> + resource_size(entry->res)); >> + index++; >> + } >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h >> index d04a571404b9..3deefd35fa5a 100644 >> --- a/drivers/pci/controller/plda/pcie-plda.h >> +++ b/drivers/pci/controller/plda/pcie-plda.h >> @@ -119,4 +119,10 @@ struct plda_pcie_rp { >> struct plda_msi msi; >> void __iomem *bridge_addr; >> }; >> + >> +void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> + phys_addr_t axi_addr, phys_addr_t pci_addr, >> + size_t size); >> +int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> + struct plda_pcie_rp *port); >> #endif >> -- >> 2.17.1
This patchset final purpose is add PCIe driver for StarFive JH7110 SoC. JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the same IP and have commit their codes, which are mixed with PLDA controller codes and Microchip platform codes. For re-use the PLDA controller codes, I request refactoring microchip codes, move PLDA common codes to PLDA files. Desigware and Cadence is good example for refactoring codes. So first step is extract the PLDA common codes from microchip, and refactoring the microchip codes.(patch1 - 16) Then, add Starfive codes. (patch17 - 19) This patchset is base on v6.6-rc1 patch1 is move PLDA XpressRICH PCIe host common properties dt-binding docs from microchip,pcie-host.yaml patch2 is move PolarFire codes to PLDA directory. patch3 is move PLDA IP register macros to plda-pcie.h patch4 is rename data structure in microchip codes. patch5 is rename two setup functions in microchip codes, prepare to move to common file. patch6 is change the arguments of plda_pcie_setup_iomems() patch7 is move the two setup functions to common file pcie-plda-host.c patch8 is Add PLDA event interrupt codes and IRQ domain ops. patch9 is rename the IRQ related functions, prepare to move to pcie-plda-host. patch10 - 14 is modify the event codes, preparing for support starfive and microchip two platforms. patch15 is move IRQ related functions to pcie-plda-host.c patch16 is set plda_event_handler to static. patch17 is add StarFive JH7110 PCIe dt-binding doc. patch18 is add StarFive JH7110 Soc PCIe codes. patch19 is Starfive dts config previous version: v1:https://patchwork.kernel.org/project/linux-pci/cover/20230719102057.22329-1-minda.chen@starfivetech.com/ v2:https://patchwork.kernel.org/project/linux-pci/cover/20230727103949.26149-1-minda.chen@starfivetech.com/ v3:https://patchwork.kernel.org/project/linux-pci/cover/20230814082016.104181-1-minda.chen@starfivetech.com/ v4:https://patchwork.kernel.org/project/linux-pci/cover/20230825090129.65721-1-minda.chen@starfivetech.com/ v5:https://patchwork.kernel.org/project/linux-pci/cover/20230907091058.125630-1-minda.chen@starfivetech.com/ change: v6: v5 patch 4 split to patch 4 -6. New patches just contain one function modification. It is more reguluar. patch 7: Just move the two setup functions only patch 8 : draw a graph of PLDA local register, make it easier to review the codes. v5 patch 7 split to patch 9- 14. Each patch just contain one function modification. It is more regular. patch 9: rename IRQ related functions. patch 10 - 14 : modify the events codes, total five patch. patch 15: move IRQ related functions to pcie-plda-host.c patch 16: Add new patch 16. patch 18- 19 using "linux,pci-domain" dts setting. v5: patch 9 -14: - Some variables names changed (evt->event). - plda_handle_event() using a unify callback function to get events num. - Add plda_event_ops data structure. patch 18: plda_event_ops changed which is related to patch 6- 8 changed. v4: patch 3: Copy the interrupt events macros to pcie-plda-host.c patch 13: get_events() change in patch 7. Patch 8 is just move the codes. other change: All the functions in commit message add (). v3: patch 2- 16: - splite refactoring patches to multiple patch. - rename plda_pcie to plda_pcie_rp. Maybe other vendor will upstream PLDA ep codes. patch 17: - Remove the redundant reference. - move the offset value to codes in starfive,stg-syscon - change reset-gpio to prest-gpio. patch18: - Add 100ms delay after preset for PCIe 6.0 spec. - stg-syscon related modification. patch19: - Add dts configure. v2: patch1: - squash dt-bindings patches to patch1 - add 'required' list. - plda doc rename to plda,xpressrich-axi-common.yaml patch2 - 16: - squash the microchip modification patch. patch17: - remove the plda common required property. patch18: - Sync the hide rc bar ops with config read function. - Revert the T_PVPERL to 100ms and add comments for the source. - Replace the link check function by the standard link ops. - Convert to new pm ops marcos. - Some formats modification. - pcie-plda-host modification merge to patch4. other: - remove the pcie-plda-plat.c - remove the starfive dts patch first. for it depends on stg clock and syscon setting. Minda Chen (19): dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties PCI: microchip: Move pcie-microchip-host.c to plda directory PCI: microchip: Move PLDA IP register macros to pcie-plda.h PCI: microchip: Rename data structure PCI: microchip: Rename two setup functions PCI: microchip: Change the argument of plda_pcie_setup_iomems() PCI: plda: Move the setup functions to pcie-plda-host.c PCI: plda: Add event interrupt codes and IRQ domain ops PCI: microchip: Rename interrupt related functions PCI: microchip: Add num_events field to struct plda_pcie_rp PCI: microchip: Add request_event_irq() callback function PCI: microchip: Add INTx and MSI event num to struct plda_event PCI: microchip: Add get_events() callback function PCI: microchip: Add event IRQ domain ops to plda_event struct PCI: microchip: Move IRQ functions to pcie-plda-host.c PCI: plda: Set plda_event_handler() to static dt-bindings: PCI: Add StarFive JH7110 PCIe controller PCI: starfive: Add JH7110 PCIe controller riscv: dts: starfive: add PCIe dts configuration for JH7110 .../bindings/pci/microchip,pcie-host.yaml | 55 +- .../pci/plda,xpressrich3-axi-common.yaml | 75 ++ .../bindings/pci/starfive,jh7110-pcie.yaml | 120 ++++ MAINTAINERS | 17 +- .../jh7110-starfive-visionfive-2.dtsi | 63 ++ arch/riscv/boot/dts/starfive/jh7110.dtsi | 88 +++ drivers/pci/controller/Kconfig | 9 +- drivers/pci/controller/Makefile | 2 +- drivers/pci/controller/plda/Kconfig | 30 + drivers/pci/controller/plda/Makefile | 4 + .../{ => plda}/pcie-microchip-host.c | 603 ++-------------- drivers/pci/controller/plda/pcie-plda-host.c | 659 ++++++++++++++++++ drivers/pci/controller/plda/pcie-plda.h | 263 +++++++ drivers/pci/controller/plda/pcie-starfive.c | 461 ++++++++++++ 14 files changed, 1845 insertions(+), 604 deletions(-) create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich3-axi-common.yaml create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml create mode 100644 drivers/pci/controller/plda/Kconfig create mode 100644 drivers/pci/controller/plda/Makefile rename drivers/pci/controller/{ => plda}/pcie-microchip-host.c (54%) create mode 100644 drivers/pci/controller/plda/pcie-plda-host.c create mode 100644 drivers/pci/controller/plda/pcie-plda.h create mode 100644 drivers/pci/controller/plda/pcie-starfive.c base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d