Add StarFive JH7110 SoC PCIe controller platform
driver codes, and add host init/deinit to pcie-plda-host.c
Signed-off-by: Minda Chen <[email protected]>
Co-developed-by: Kevin Xie <[email protected]>
Reviewed-by: Mason Huo <[email protected]>
---
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 <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/pci/starfive*
+F: drivers/pci/controller/plda/pcie-starfive.c
+
PCIE DRIVER FOR ST SPEAR13XX
M: Pratyush Anand <[email protected]>
L: [email protected]
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;
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;
/* 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;
/* 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);
+
+ 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);
+ 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);
+ 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);
+}
+#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;
+
+ 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;
+
+ 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");
+ 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 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;
+
+ ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
+ if (ret) {
+ dev_err(dev, "failed to enable clocks\n");
+ return ret;
+ }
+
+ ret = reset_control_deassert(pcie->resets);
+ if (ret) {
+ clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
+ dev_err(dev, "failed to 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;
+ 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;
+}
+
+static int starfive_pcie_host_wait_for_link(struct starfive_jh7110_pcie *pcie)
+{
+ struct device *dev = pcie->plda.dev;
+ 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;
+
+ 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"},
+ { /* 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
+ },
+ .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 StarFive JH7110 SoC PCIe controller platform
> driver codes, and add host init/deinit to pcie-plda-host.c
>
> Signed-off-by: Minda Chen <[email protected]>
> Co-developed-by: Kevin Xie <[email protected]>
> Reviewed-by: Mason Huo <[email protected]>
> ---
> 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 <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: Documentation/devicetree/bindings/pci/starfive*
> +F: drivers/pci/controller/plda/pcie-starfive.c
> +
> PCIE DRIVER FOR ST SPEAR13XX
> M: Pratyush Anand <[email protected]>
> L: [email protected]
> 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
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 <[email protected]>
> > Co-developed-by: Kevin Xie <[email protected]>
> > Reviewed-by: Mason Huo <[email protected]>
> > ---
> > 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 <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/pci/starfive*
> > +F: drivers/pci/controller/plda/pcie-starfive.c
> > +
> > PCIE DRIVER FOR ST SPEAR13XX
> > M: Pratyush Anand <[email protected]>
> > L: [email protected]
> > 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 <[email protected]>
>> Co-developed-by: Kevin Xie <[email protected]>
>> Reviewed-by: Mason Huo <[email protected]>
>> ---
>> 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 <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/pci/starfive*
>> +F: drivers/pci/controller/plda/pcie-starfive.c
>> +
>> PCIE DRIVER FOR ST SPEAR13XX
>> M: Pratyush Anand <[email protected]>
>> L: [email protected]
>> 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 <[email protected]>
> >> Co-developed-by: Kevin Xie <[email protected]>
> >> Reviewed-by: Mason Huo <[email protected]>
> >> ---
> >> 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 <[email protected]>
> >> +L: [email protected]
> >> +S: Maintained
> >> +F: Documentation/devicetree/bindings/pci/starfive*
> >> +F: drivers/pci/controller/plda/pcie-starfive.c
> >> +
> >> PCIE DRIVER FOR ST SPEAR13XX
> >> M: Pratyush Anand <[email protected]>
> >> L: [email protected]
> >> 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 <[email protected]>
>> >> Co-developed-by: Kevin Xie <[email protected]>
>> >> Reviewed-by: Mason Huo <[email protected]>
>> >> ---
>> >> 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 <[email protected]>
>> >> +L: [email protected]
>> >> +S: Maintained
>> >> +F: Documentation/devicetree/bindings/pci/starfive*
>> >> +F: drivers/pci/controller/plda/pcie-starfive.c
>> >> +
>> >> PCIE DRIVER FOR ST SPEAR13XX
>> >> M: Pratyush Anand <[email protected]>
>> >> L: [email protected]
>> >> 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 <[email protected]>
> >> >> Co-developed-by: Kevin Xie <[email protected]>
> >> >> Reviewed-by: Mason Huo <[email protected]>
> >> >> ---
> >> >> 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 <[email protected]>
> >> >> +L: [email protected]
> >> >> +S: Maintained
> >> >> +F: Documentation/devicetree/bindings/pci/starfive*
> >> >> +F: drivers/pci/controller/plda/pcie-starfive.c
> >> >> +
> >> >> PCIE DRIVER FOR ST SPEAR13XX
> >> >> M: Pratyush Anand <[email protected]>
> >> >> L: [email protected]
> >> >> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv