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

[Pali Rohár]

Hello!

On Wednesday 29 March 2023 18:01:41 Minda Chen wrote:
> From: Mason Huo <mason.huo at 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 at starfivetech.com>
> Signed-off-by: Minda Chen <minda.chen at 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;
> +}