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