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

[Pali Rohár]

Hello!

On Tuesday 11 April 2023 09:02:07 Minda Chen wrote:
> +int starfive_pcie_config_write(struct udevice *udev, pci_dev_t bdf,
> +			       uint offset, ulong value,
> +			       enum pci_size_t size)
> +{
> +	struct starfive_pcie *priv = dev_get_priv(udev);
> +	int ret;
> +
> +	ret = pci_generic_mmap_write_config(udev, starfive_pcie_conf_address,
> +					    bdf, offset, value, size);
> +
> +	if (!ret && offset == PCI_SECONDARY_BUS) {
> +		priv->sec_busno = value & 0xff;
> +		debug("Secondary bus number was changed to %d\n",
> +		      priv->sec_busno);
> +	}

This block of code contains two issues:

1) If secondary bus is changed by the 16-bit or 32-bit write operation
   then this condition does not catch it.

2) priv->sec_busno is used just for checking if driver is going to
   access device on secondary bus of the Root Port. But this code
   updates priv->sec_busno also for write to _any_ device on any bus,
   not just when updating Root Port device. So it breaks support for
   non-trivial PCIe hierarchy which contains e.g. PCIe switch (e.g. when
   changing configuration of the virtual PCI-to-PCI bridge device of
   PCIe switch, which is behind the secondary bus of the Root Port).

So you need something like this:

    if (!ret &&
        PCI_BUS(bdf) == dev_seq(udev) && PCI_DEV(bdf) == 0 && PCI_FUNC(bdf) == 0 &&
        (offset == PCI_SECONDARY_BUS || (offset == PCI_PRIMARY_BUS && size != PCI_SIZE_8)) {
        priv->sec_busno = ((offset == PCI_PRIMARY_BUS) ? (value >> 8) : value) & 0xff;
        debug("Secondary bus number was changed to %d\n", pcie->sec_busno);
    }

You have to update priv->sec_busno only when write request is for the
Root Port. And you need to catch also 16-bit or 32-bit write operation
to the PCI_PRIMARY_BUS register. It is because PCI_SECONDARY_BUS reg
is (PCI_PRIMARY_BUS+2) and (PCI_SECONDARY_BUS & ~3) == PCI_PRIMARY_BUS

> +	return ret;
> +}