[PATCH u-boot-next 03/12] pci: mpc85xx: Use PCI_CONF1_EXT_ADDRESS() macro

[Pali Rohár]

On Tuesday 28 December 2021 01:32:05 Simon Glass wrote:
> Hi Pali,
> 
> On Fri, 26 Nov 2021 at 03:43, Pali Rohár <pali at kernel.org> wrote:
> >
> > PCI mpc85xx driver uses extended format of Config Address for PCI
> > Configuration Mechanism #1.
> >
> > So use new U-Boot macro PCI_CONF1_EXT_ADDRESS().
> >
> > Signed-off-by: Pali Rohár <pali at kernel.org>
> > ---
> >  drivers/pci/pci_mpc85xx.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> nit below
> 
> >
> > diff --git a/drivers/pci/pci_mpc85xx.c b/drivers/pci/pci_mpc85xx.c
> > index 574cb784a893..1e180ee289b0 100644
> > --- a/drivers/pci/pci_mpc85xx.c
> > +++ b/drivers/pci/pci_mpc85xx.c
> > @@ -23,7 +23,7 @@ static int mpc85xx_pci_dm_read_config(const struct udevice *dev, pci_dev_t bdf,
> >         struct mpc85xx_pci_priv *priv = dev_get_priv(dev);
> >         u32 addr;
> >
> > -       addr = bdf | (offset & 0xfc) | ((offset & 0xf00) << 16) | 0x80000000;
> > +       addr = PCI_CONF1_EXT_ADDRESS(PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), offset);
> >         out_be32(priv->cfg_addr, addr);
> >         sync();
> >         *value = pci_conv_32_to_size(in_le32(priv->cfg_data), offset, size);
> > @@ -38,7 +38,7 @@ static int mpc85xx_pci_dm_write_config(struct udevice *dev, pci_dev_t bdf,
> >         struct mpc85xx_pci_priv *priv = dev_get_priv(dev);
> >         u32 addr;
> >
> > -       addr = bdf | (offset & 0xfc) | ((offset & 0xf00) << 16) | 0x80000000;
> > +       addr = PCI_CONF1_EXT_ADDRESS(PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), offset);
> 
> It seems annoying to have to separate these out just to have the macro
> put them back together. Perhaps add a version of the macro that takes
> bdf as a parameter?

Hello!

In most cases specifying the bus value directly from the "bdf" variable
is in U-Boot incorrect. It is because since new DM PCI API "bdf"
parameter in read_config and write_config callbacks is relative to the
DM PCI root bus. And it needs to be translated from DM PCI numbering to
the numbering on PCI bus. In most cases for single-domain or
single-host-bridge or single-PCIe-root-port buses is root bus number
zero and therefore translation is 1:1. Also it applies for the first
registered host bridge into DM. But my understanding of DM API is that
DM drivers should not expects order of loading and neither that exactly
one DM driver from PCI category would be loaded.

So in my opinion, passing bus number from "bdf" variable together with
device number from "bdf" variable to PCI_CONF1_EXT_ADDRESS() is
something which should be avoided in all new drivers. And so separating
bus and device is a good idea.

Lot of these existing PCI drivers were converted from old driver to DM
model without thinking about multidomain/multi-host-bridge support
(as I guess all these old drivers are running on platforms without
multidomain support), so nobody noticed any issue.

But nowadays, it is common that on ARM SoCs are PCIe ports connected to
different PCIe Root Complexes which represents different PCIe
controllers and therefore also different PCI domains. So I think it is a
good idea to have separated bus and domain and do not provide macro
which takes directly "bdf" as it could lead to issues that new drivers
would not provide correct PCIe multi-port support.

Look into pci-aardvark.c or pci_mvebu.c, correct way is to calculate bus
number as: int busno = PCI_BUS(bdf) - dev_seq(bus); (unless driver or HW
does not expect any more complicated translation or mapping).

> >         out_be32(priv->cfg_addr, addr);
> >         sync();
> >         out_le32(priv->cfg_data, pci_conv_size_to_32(0, value, offset, size));
> > --
> > 2.20.1
> >
> 
> Regards,
> Simon