On 2023/3/30 1:27, Pali Rohár wrote:
> 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.
>
ok， I will change this.
>> + 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);
>
ok, thanks
>> +}
>> +
>> +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;
>
OK, thanks， I will record secondary busno according PCI_SECONDARY_BUS register
> 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.
>
ok
>> + 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.
>
ok
>> +
>> + *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?
>
This register is not read-only register, consist resion ID and class code.
Bit [39:32]: Revision ID
Bit [63:40]: Class code
And the register reset value is zero, Our PCIe version is 2.0. So set value 2.
Maybe I will add comment to this.
>> +
>> + /*
>> + * 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.
>
ok
>> +
>> + 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;
>> +}
