| Message ID | 20211126104252.5443-4-pali@kernel.org |
|---|---|
| State | Accepted |
| Commit | 247ffc6b36ac2605d54fe31628ae6b827603d0ac |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Common U-Boot macros for PCI Configuration Mechanism #1 | expand |
Hi Pali, On Fri, 26 Nov 2021 at 03:43, Pali Rohár <pali@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@kernel.org> > --- > drivers/pci/pci_mpc85xx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Simon Glass <sjg@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? > 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
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@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@kernel.org> > > --- > > drivers/pci/pci_mpc85xx.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > Reviewed-by: Simon Glass <sjg@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
On Fri, Nov 26, 2021 at 11:42:43AM +0100, Pali Rohár 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@kernel.org> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
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); out_be32(priv->cfg_addr, addr); sync(); out_le32(priv->cfg_data, pci_conv_size_to_32(0, value, offset, size));
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@kernel.org> --- drivers/pci/pci_mpc85xx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)