Re: [PATCH v3 1/3] starfive: pci: Add StarFive JH7110 pcie driver

Hello!

On Wednesday 29 March 2023 18:01:41 Minda Chen wrote:

From: Mason Huo mason.huo@starfivetech.com

Add pcie driver for StarFive JH7110, the driver depends on starfive gpio, pinctrl, clk and reset driver to do init.

Several devices are tested: a) M.2 NVMe SSD b) Realtek 8169 Ethernet adapter.

Signed-off-by: Mason Huo mason.huo@starfivetech.com Signed-off-by: Minda Chen minda.chen@starfivetech.com

---

drivers/pci/Kconfig | 11 + drivers/pci/Makefile | 1 + drivers/pci/pcie_starfive_jh7110.c | 463 +++++++++++++++++++++++++++++ 3 files changed, 475 insertions(+) create mode 100644 drivers/pci/pcie_starfive_jh7110.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index ef328d2652..e7b0ff5bc3 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -374,4 +374,15 @@ config PCIE_UNIPHIER Say Y here if you want to enable PCIe controller support on UniPhier SoCs.

+config PCIE_STARFIVE_JH7110

• bool "Enable Starfive JH7110 PCIe driver"
• depends on STARFIVE_JH7110
• depends on PINCTRL_STARFIVE_JH7110
• depends on CLK_JH7110
• depends on RESET_JH7110

There is no direct hard dependency on on these 4 symbols (or at least I do not see any in include header files). So to allow compile time checks, change it to just soft dependency by switching from "depends" to "imply" keyword.

• default y
• help

•  Say Y here if you want to enable PCIe controller support on
  

•  StarFive JH7110 SoC.
  

endif

...

diff --git a/drivers/pci/pcie_starfive_jh7110.c b/drivers/pci/pcie_starfive_jh7110.c new file mode 100644 index 0000000000..1005ed9919 --- /dev/null +++ b/drivers/pci/pcie_starfive_jh7110.c

...

+static int starfive_pcie_off_conf(pci_dev_t bdf, uint offset) +{

• unsigned int bus = PCI_BUS(bdf);
• unsigned int dev = PCI_DEV(bdf);
• unsigned int func = PCI_FUNC(bdf);
• return PCIE_ECAM_OFFSET(bus, dev, func, offset);

This function is used just on one place, it is pretty straightforward and nothing starfive_pcie-specific. Just directly inline it into starfive_pcie_conf_address() function?

int where = PCIE_ECAM_OFFSET(PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), offset);

+}

+static int starfive_pcie_addr_valid(pci_dev_t bdf, int first_busno) +{

• /*

• * Single device limitation.
  

• * For JH7110 SoC limitation, one bus can only connnect one device.
  

• * And PCIe controller contain HW issue that non-root host bridge
  

• * bus emumerate duplicate devices.
  

• * Only can access device 0 in Bus 1 host bridge.
  

• */
  

• if ((PCI_BUS(bdf) != first_busno) && (PCI_DEV(bdf) > 0))

• return 0;
  

In v2 discussion you wrote that the issue with duplicate devices is on the bus 1, which is secondary bus - the bus behind the root port.

So check against root port bus (first_busno) is not correct, you need to check it against bus behind the root port (secondary bus).

Current check completely filters all devices with numbers higher than 0 behind the secondary bus, which completely breaks support for PCIe switches. Moreover behind PCIe switch can be anything with more complicated topology.

That secondary bus number is dynamically allocated / assigned by U-Boot core code, so you need to check PCI_SECONDARY_BUS register of the root port.

In pci_mvebu.c is this value cached in "->sec_busno" member. You can reuse this pattern.

You probably want this kind of check:

if (PCI_BUS(bdf) == priv->sec_busno && PCI_DEV(bdf) > 0) return false; else return true;

Also comment "non-root host bridge bus" is strange. Bus of the host bridge is always the root one. You probably want to write the bus immediately behind the root bus / host bridge OR the secondary bus of the host bridge.

• return 1;

+}

+static int starfive_pcie_conf_address(const struct udevice *udev, pci_dev_t bdf,

• 		      uint offset, void **paddr)
  

+{

• struct starfive_pcie *priv = dev_get_priv(udev);
• int where = starfive_pcie_off_conf(bdf, offset);
• if (!starfive_pcie_addr_valid(bdf, priv->first_busno))

• return -EINVAL;
  

Maybe better error code is -ENODEV? As technically access request is valid but there is no device on it.

• *paddr = (void *)(priv->cfg_base + where);
• return 0;

+}

...

+static int starfive_pcie_init_port(struct udevice *dev) +{

• int ret, i;
• unsigned int value;
• struct starfive_pcie *priv = dev_get_priv(dev);
• ret = clk_enable_bulk(&priv->clks);
• if (ret) {

• dev_err(dev, "Failed to enable clks (ret=%d)\n", ret);
  

• return ret;
  

• }
• ret = reset_deassert_bulk(&priv->rsts);
• if (ret) {

• dev_err(dev, "Failed to deassert resets (ret=%d)\n", ret);
  

• goto err_deassert_clk;
  

• }
• dm_gpio_set_value(&priv->reset_gpio, 1);
• /* Disable physical functions except #0 */
• for (i = 1; i < PLDA_FUNC_NUM; i++) {

• regmap_update_bits(priv->regmap,
  

• 		   priv->stg_arfun,
  

• 		   STG_SYSCON_AXI4_SLVL_ARFUNC_MASK,
  

• 		   (i << PLDA_PHY_FUNC_SHIFT) <<
  

• 		   STG_SYSCON_AXI4_SLVL_ARFUNC_SHIFT);
  

• regmap_update_bits(priv->regmap,
  

• 		   priv->stg_awfun,
  

• 		   STG_SYSCON_AXI4_SLVL_AWFUNC_MASK,
  

• 		   i << PLDA_PHY_FUNC_SHIFT);
  

• value = readl(priv->reg_base + PCI_MISC);
  

• value |= PLDA_FUNCTION_DIS;
  

• writel(value, priv->reg_base + PCI_MISC);
  

• }
• /* Disable physical functions */
• regmap_update_bits(priv->regmap,

• 	   priv->stg_arfun,
  

• 	   STG_SYSCON_AXI4_SLVL_ARFUNC_MASK,
  

• 	   0);
  

• regmap_update_bits(priv->regmap,

• 	   priv->stg_awfun,
  

• 	   STG_SYSCON_AXI4_SLVL_AWFUNC_MASK,
  

• 	   0);
  

• /* Enable root port */
• value = readl(priv->reg_base + GEN_SETTINGS);
• value |= PLDA_RP_ENABLE;
• writel(value, priv->reg_base + GEN_SETTINGS);
• /* PCIe PCI Standard Configuration Identification Settings. */
• value = (PCI_CLASS_BRIDGE_PCI_NORMAL << IDS_CLASS_CODE_SHIFT) | IDS_REVISION_ID;
• writel(value, priv->reg_base + PCIE_PCI_IDS);

This looks like configuration of the PCI_CLASS_REVISION read-only register. Is there any reason why you are removing the original "revision" information by hardcoded IDS_REVISION_ID constant?

• /*

• * 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.
  

• */
  

• value = readl(priv->reg_base + PMSG_SUPPORT_RX);
• value &= ~PMSG_LTR_SUPPORT;
• writel(value, priv->reg_base + PMSG_SUPPORT_RX);
• /* Prefetchable memory window 64-bit addressing support */
• value = readl(priv->reg_base + PCIE_WINROM);
• value |= PREF_MEM_WIN_64_SUPPORT;
• writel(value, priv->reg_base + PCIE_WINROM);
• starfive_pcie_atr_init(priv);

This function may fail. But return value is ignored here.

• dm_gpio_set_value(&priv->reset_gpio, 0);
• /* Ensure that PERST in default at least 300 ms */
• mdelay(300);
• return 0;

+err_deassert_clk:

• clk_disable_bulk(&priv->clks);
• return ret;

+}