[PATCH u-boot-next 00/12] Common U-Boot macros for PCI Configuration Mechanism #1
This patch series add new U-Boot macros for composing Config Address for PCI Configuration Mechanism #1 as defined in PCI Local Bus Specification, including extended variant (which do not have any formal specification) and then use these new macros (PCI_CONF1_ADDRESS and PCI_CONF1_EXT_ADDRESS) in all PCI drivers that use Config Address according to PCI Configuration Mechanism #1.
PCI Configuration Mechanism #1 was originally specified for x86 platforms and it is still supported on x86 together with PCIe ECAM. Nowadays lot of non-ECAM-compliant ARM PCIe controllers use only extended variant of this address construction and some of them requires cleared Enable bit. Extended variant is also supported by x86 AMD Barcelona (and new) CPUs, but U-Boot code does not provide this support yet.
Note that it exists also PCI Configuration Mechanism #2, but this one was removed from PCI Local Bus Specification revision 3.0 and seems that it is not used by any PCI driver in U-Boot. So I have not added macros for this mechanism in this patch series.
Because lot of hardware still use this (rather old) mechanism, relevant U-Boot PCI and PCIe drivers have own ad-hoc code address construction, which is repeated and written in different ways.
This patch series is code cleanup of these PCIe drivers to use common U-Boot macros for PCI Configuration Mechanism #1.
The last change is fixing construction of Config Address in PCI driver sh7751. Construction with this change matches Linux kernel code and also U-Boot code prior commit mentioned in commit message.
I have tested this patch series only for PCI mvebu driver on A385 board and it is working fine. So Please properly review all other changes. Ideally test them.
I have run CI tests with these changes on github and everything passed: https://github.com/u-boot/u-boot/pull/101
Please let me know what do you think about this change.
Pali Rohár (12): pci: Add standard PCI Config Address macros pci: gt64120: Use PCI_CONF1_ADDRESS() macro pci: mpc85xx: Use PCI_CONF1_EXT_ADDRESS() macro pci: msc01: Use PCI_CONF1_ADDRESS() macro pci: mvebu: Use PCI_CONF1_EXT_ADDRESS() macro pci: tegra: Use PCI_CONF1_EXT_ADDRESS() macro pci: fsl: Use PCI_CONF1_EXT_ADDRESS() macro pci: mediatek: Use PCI_CONF1_EXT_ADDRESS() macro pci: sh7780: Use PCI_CONF1_ADDRESS() macro x86: pci: Use PCI_CONF1_ADDRESS() macro m68k: mcf5445x: pci: Use PCI_CONF1_ADDRESS() macro pci: sh7751: Fix access to config space via PCI_CONF1_ADDRESS() macro
arch/m68k/cpu/mcf5445x/pci.c | 7 +++--- arch/x86/cpu/pci.c | 4 ++-- drivers/pci/pci_gt64120.c | 7 ++---- drivers/pci/pci_mpc85xx.c | 4 ++-- drivers/pci/pci_msc01.c | 7 ++---- drivers/pci/pci_mvebu.c | 17 ++++---------- drivers/pci/pci_sh7751.c | 29 ++--------------------- drivers/pci/pci_sh7780.c | 8 +++---- drivers/pci/pci_tegra.c | 11 +++------ drivers/pci/pcie_fsl.c | 10 ++++---- drivers/pci/pcie_mediatek.c | 17 +++++++------- include/gt64120.h | 12 ---------- include/msc01.h | 9 -------- include/pci.h | 45 ++++++++++++++++++++++++++++++++++++ 14 files changed, 83 insertions(+), 104 deletions(-)
Lot of PCI and PCIe controllers are using standard Config Address for PCI Configuration Mechanism #1 or its extended version.
So add PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() macros into U-Boot's pci.h header file which can be suitable for most PCI and PCIe controller drivers. Drivers do not have to invent their own macros and can use these new U-Boot macros.
Signed-off-by: Pali Rohár pali@kernel.org --- include/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
diff --git a/include/pci.h b/include/pci.h index 6c1094d72998..0ea41a7e1ba2 100644 --- a/include/pci.h +++ b/include/pci.h @@ -522,6 +522,51 @@
#include <pci_ids.h>
+/* + * Config Address for PCI Configuration Mechanism #1 + * + * See PCI Local Bus Specification, Revision 3.0, + * Section 3.2.2.3.2, Figure 3-2, p. 50. + */ + +#define PCI_CONF1_BUS_SHIFT 16 /* Bus number */ +#define PCI_CONF1_DEV_SHIFT 11 /* Device number */ +#define PCI_CONF1_FUNC_SHIFT 8 /* Function number */ + +#define PCI_CONF1_BUS_MASK 0xff +#define PCI_CONF1_DEV_MASK 0x1f +#define PCI_CONF1_FUNC_MASK 0x7 +#define PCI_CONF1_REG_MASK 0xfc /* Limit aligned offset to a maximum of 256B */ + +#define PCI_CONF1_ENABLE BIT(31) +#define PCI_CONF1_BUS(x) (((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT) +#define PCI_CONF1_DEV(x) (((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT) +#define PCI_CONF1_FUNC(x) (((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT) +#define PCI_CONF1_REG(x) ((x) & PCI_CONF1_REG_MASK) + +#define PCI_CONF1_ADDRESS(bus, dev, func, reg) \ + (PCI_CONF1_ENABLE | \ + PCI_CONF1_BUS(bus) | \ + PCI_CONF1_DEV(dev) | \ + PCI_CONF1_FUNC(func) | \ + PCI_CONF1_REG(reg)) + +/* + * Extension of PCI Config Address for accessing extended PCIe registers + * + * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs + * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address + * are used for specifying additional 4 high bits of PCI Express register. + */ + +#define PCI_CONF1_EXT_REG_SHIFT 16 +#define PCI_CONF1_EXT_REG_MASK 0xf00 +#define PCI_CONF1_EXT_REG(x) (((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT) + +#define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \ + (PCI_CONF1_ADDRESS(bus, dev, func, reg) | \ + PCI_CONF1_EXT_REG(reg)) + /* * Enhanced Configuration Access Mechanism (ECAM) *
Hi Paul,
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
Lot of PCI and PCIe controllers are using standard Config Address for PCI Configuration Mechanism #1 or its extended version.
So add PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() macros into U-Boot's pci.h header file which can be suitable for most PCI and PCIe controller drivers. Drivers do not have to invent their own macros and can use these new U-Boot macros.
Signed-off-by: Pali Rohár pali@kernel.org
include/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Thoughts below
diff --git a/include/pci.h b/include/pci.h index 6c1094d72998..0ea41a7e1ba2 100644 --- a/include/pci.h +++ b/include/pci.h @@ -522,6 +522,51 @@
#include <pci_ids.h>
+/*
- Config Address for PCI Configuration Mechanism #1
- See PCI Local Bus Specification, Revision 3.0,
- Section 3.2.2.3.2, Figure 3-2, p. 50.
- */
+#define PCI_CONF1_BUS_SHIFT 16 /* Bus number */ +#define PCI_CONF1_DEV_SHIFT 11 /* Device number */ +#define PCI_CONF1_FUNC_SHIFT 8 /* Function number */
+#define PCI_CONF1_BUS_MASK 0xff +#define PCI_CONF1_DEV_MASK 0x1f +#define PCI_CONF1_FUNC_MASK 0x7 +#define PCI_CONF1_REG_MASK 0xfc /* Limit aligned offset to a maximum of 256B */
+#define PCI_CONF1_ENABLE BIT(31) +#define PCI_CONF1_BUS(x) (((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT) +#define PCI_CONF1_DEV(x) (((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT) +#define PCI_CONF1_FUNC(x) (((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT) +#define PCI_CONF1_REG(x) ((x) & PCI_CONF1_REG_MASK)
+#define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
Just for brevity, how about _ADDR() instead of _ADDRESS() ?
(PCI_CONF1_ENABLE | \PCI_CONF1_BUS(bus) | \PCI_CONF1_DEV(dev) | \PCI_CONF1_FUNC(func) | \PCI_CONF1_REG(reg))+/*
- Extension of PCI Config Address for accessing extended PCIe registers
- No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
- or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
- are used for specifying additional 4 high bits of PCI Express register.
- */
+#define PCI_CONF1_EXT_REG_SHIFT 16 +#define PCI_CONF1_EXT_REG_MASK 0xf00
How about s/EXT_REG/EXT/ ?
For shifts and masks we normally like to define the mask in terms of the shift, so:
+#define PCI_CONF1_EXT_REG_MASK (0xf00 << PCI_CONF1_EXT_REG_SHIFT)
+#define PCI_CONF1_EXT_REG(x) (((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT)
+#define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \PCI_CONF1_EXT_REG(reg))/*
- Enhanced Configuration Access Mechanism (ECAM)
-- 2.20.1
Regards, Simon
On Tuesday 28 December 2021 01:32:02 Simon Glass wrote:
Hi Paul,
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
Lot of PCI and PCIe controllers are using standard Config Address for PCI Configuration Mechanism #1 or its extended version.
So add PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() macros into U-Boot's pci.h header file which can be suitable for most PCI and PCIe controller drivers. Drivers do not have to invent their own macros and can use these new U-Boot macros.
Signed-off-by: Pali Rohár pali@kernel.org
include/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Thoughts below
diff --git a/include/pci.h b/include/pci.h index 6c1094d72998..0ea41a7e1ba2 100644 --- a/include/pci.h +++ b/include/pci.h @@ -522,6 +522,51 @@
#include <pci_ids.h>
+/*
- Config Address for PCI Configuration Mechanism #1
- See PCI Local Bus Specification, Revision 3.0,
- Section 3.2.2.3.2, Figure 3-2, p. 50.
- */
+#define PCI_CONF1_BUS_SHIFT 16 /* Bus number */ +#define PCI_CONF1_DEV_SHIFT 11 /* Device number */ +#define PCI_CONF1_FUNC_SHIFT 8 /* Function number */
+#define PCI_CONF1_BUS_MASK 0xff +#define PCI_CONF1_DEV_MASK 0x1f +#define PCI_CONF1_FUNC_MASK 0x7 +#define PCI_CONF1_REG_MASK 0xfc /* Limit aligned offset to a maximum of 256B */
+#define PCI_CONF1_ENABLE BIT(31) +#define PCI_CONF1_BUS(x) (((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT) +#define PCI_CONF1_DEV(x) (((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT) +#define PCI_CONF1_FUNC(x) (((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT) +#define PCI_CONF1_REG(x) ((x) & PCI_CONF1_REG_MASK)
+#define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
Just for brevity, how about _ADDR() instead of _ADDRESS() ?
I do not know... I think both are OK.
(PCI_CONF1_ENABLE | \PCI_CONF1_BUS(bus) | \PCI_CONF1_DEV(dev) | \PCI_CONF1_FUNC(func) | \PCI_CONF1_REG(reg))+/*
- Extension of PCI Config Address for accessing extended PCIe registers
- No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
- or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
- are used for specifying additional 4 high bits of PCI Express register.
- */
+#define PCI_CONF1_EXT_REG_SHIFT 16 +#define PCI_CONF1_EXT_REG_MASK 0xf00
How about s/EXT_REG/EXT/ ?
I'm not sure... Name "EXT" does not say what it is. This macro defines access to the extended part (higher bits) of register value. That is why I called it "EXT_REG" so reader would see that it belongs to plain "REG" macro (which defines lower bits of register value).
For shifts and masks we normally like to define the mask in terms of the shift, so:
+#define PCI_CONF1_EXT_REG_MASK (0xf00 << PCI_CONF1_EXT_REG_SHIFT)
+#define PCI_CONF1_EXT_REG(x) (((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT)
Well, I wanted these PCI_CONF1_* macros to be "compatible" with PCIE_ECAM_* macros which are already in next and which I reused from Linux kernel header files. And these existing macros are using the style which I used also for PCI_CONF1_* macros.
+#define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \PCI_CONF1_EXT_REG(reg))/*
- Enhanced Configuration Access Mechanism (ECAM)
-- 2.20.1
Regards, Simon
On Fri, Nov 26, 2021 at 11:42:41AM +0100, Pali Rohár wrote:
Lot of PCI and PCIe controllers are using standard Config Address for PCI Configuration Mechanism #1 or its extended version.
So add PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() macros into U-Boot's pci.h header file which can be suitable for most PCI and PCIe controller drivers. Drivers do not have to invent their own macros and can use these new U-Boot macros.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
PCI gt64120 driver uses standard format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_ADDRESS() and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org --- drivers/pci/pci_gt64120.c | 7 ++----- include/gt64120.h | 12 ------------ 2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/pci_gt64120.c b/drivers/pci/pci_gt64120.c index 153c65b119a4..2c2a80eeaa06 100644 --- a/drivers/pci/pci_gt64120.c +++ b/drivers/pci/pci_gt64120.c @@ -48,7 +48,7 @@ static int gt_config_access(struct gt64120_pci_controller *gt, { unsigned int bus = PCI_BUS(bdf); unsigned int dev = PCI_DEV(bdf); - unsigned int devfn = PCI_DEV(bdf) << 3 | PCI_FUNC(bdf); + unsigned int func = PCI_FUNC(bdf); u32 intr; u32 addr; u32 val; @@ -65,10 +65,7 @@ static int gt_config_access(struct gt64120_pci_controller *gt, /* Clear cause register bits */ writel(~GT_INTRCAUSE_ABORT_BITS, >->regs->intrcause);
- addr = GT_PCI0_CFGADDR_CONFIGEN_BIT; - addr |= bus << GT_PCI0_CFGADDR_BUSNUM_SHF; - addr |= devfn << GT_PCI0_CFGADDR_FUNCTNUM_SHF; - addr |= (where / 4) << GT_PCI0_CFGADDR_REGNUM_SHF; + addr = PCI_CONF1_ADDRESS(bus, dev, func, where);
/* Setup address */ writel(addr, >->regs->pci0_cfgaddr); diff --git a/include/gt64120.h b/include/gt64120.h index 0b577f3f44b9..b58afe3c4afe 100644 --- a/include/gt64120.h +++ b/include/gt64120.h @@ -491,18 +491,6 @@ #define GT_INTRCAUSE_TARABORT0_BIT GT_INTRCAUSE_TARABORT0_MSK
-#define GT_PCI0_CFGADDR_REGNUM_SHF 2 -#define GT_PCI0_CFGADDR_REGNUM_MSK (MSK(6) << GT_PCI0_CFGADDR_REGNUM_SHF) -#define GT_PCI0_CFGADDR_FUNCTNUM_SHF 8 -#define GT_PCI0_CFGADDR_FUNCTNUM_MSK (MSK(3) << GT_PCI0_CFGADDR_FUNCTNUM_SHF) -#define GT_PCI0_CFGADDR_DEVNUM_SHF 11 -#define GT_PCI0_CFGADDR_DEVNUM_MSK (MSK(5) << GT_PCI0_CFGADDR_DEVNUM_SHF) -#define GT_PCI0_CFGADDR_BUSNUM_SHF 16 -#define GT_PCI0_CFGADDR_BUSNUM_MSK (MSK(8) << GT_PCI0_CFGADDR_BUSNUM_SHF) -#define GT_PCI0_CFGADDR_CONFIGEN_SHF 31 -#define GT_PCI0_CFGADDR_CONFIGEN_MSK (MSK(1) << GT_PCI0_CFGADDR_CONFIGEN_SHF) -#define GT_PCI0_CFGADDR_CONFIGEN_BIT GT_PCI0_CFGADDR_CONFIGEN_MSK - #define GT_PCI0_CMD_MBYTESWAP_SHF 0 #define GT_PCI0_CMD_MBYTESWAP_MSK (MSK(1) << GT_PCI0_CMD_MBYTESWAP_SHF) #define GT_PCI0_CMD_MBYTESWAP_BIT GT_PCI0_CMD_MBYTESWAP_MSK
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
PCI gt64120 driver uses standard format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_ADDRESS() and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org
drivers/pci/pci_gt64120.c | 7 ++----- include/gt64120.h | 12 ------------ 2 files changed, 2 insertions(+), 17 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:42AM +0100, Pali Rohár wrote:
PCI gt64120 driver uses standard format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_ADDRESS() and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
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(-)
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));
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!
PCI msc01 driver uses standard format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org --- drivers/pci/pci_msc01.c | 7 ++----- include/msc01.h | 9 --------- 2 files changed, 2 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/pci_msc01.c b/drivers/pci/pci_msc01.c index 2f1b688fc321..8d363d60498b 100644 --- a/drivers/pci/pci_msc01.c +++ b/drivers/pci/pci_msc01.c @@ -34,16 +34,13 @@ static int msc01_config_access(struct msc01_pci_controller *msc01, void *cfgdata = msc01->base + MSC01_PCI_CFGDATA_OFS; unsigned int bus = PCI_BUS(bdf); unsigned int dev = PCI_DEV(bdf); - unsigned int devfn = PCI_DEV(bdf) << 3 | PCI_FUNC(bdf); + unsigned int func = PCI_FUNC(bdf);
/* clear abort status */ __raw_writel(aborts, intstat);
/* setup address */ - __raw_writel((bus << MSC01_PCI_CFGADDR_BNUM_SHF) | - (dev << MSC01_PCI_CFGADDR_DNUM_SHF) | - (devfn << MSC01_PCI_CFGADDR_FNUM_SHF) | - ((where / 4) << MSC01_PCI_CFGADDR_RNUM_SHF), + __raw_writel((PCI_CONF1_ADDRESS(bus, dev, func, where) & ~PCI_CONF1_ENABLE), msc01->base + MSC01_PCI_CFGADDR_OFS);
/* perform access */ diff --git a/include/msc01.h b/include/msc01.h index ec18a724eb93..20158123494a 100644 --- a/include/msc01.h +++ b/include/msc01.h @@ -71,15 +71,6 @@ #define MSC01_PCI_INTSTAT_MA_SHF 7 #define MSC01_PCI_INTSTAT_MA_MSK (0x1 << MSC01_PCI_INTSTAT_MA_SHF)
-#define MSC01_PCI_CFGADDR_BNUM_SHF 16 -#define MSC01_PCI_CFGADDR_BNUM_MSK (0xff << MSC01_PCI_CFGADDR_BNUM_SHF) -#define MSC01_PCI_CFGADDR_DNUM_SHF 11 -#define MSC01_PCI_CFGADDR_DNUM_MSK (0x1f << MSC01_PCI_CFGADDR_DNUM_SHF) -#define MSC01_PCI_CFGADDR_FNUM_SHF 8 -#define MSC01_PCI_CFGADDR_FNUM_MSK (0x3 << MSC01_PCI_CFGADDR_FNUM_SHF) -#define MSC01_PCI_CFGADDR_RNUM_SHF 2 -#define MSC01_PCI_CFGADDR_RNUM_MSK (0x3f << MSC01_PCI_CFGADDR_RNUM_SHF) - #define MSC01_PCI_HEAD0_VENDORID_SHF 0 #define MSC01_PCI_HEAD0_DEVICEID_SHF 16
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
PCI msc01 driver uses standard format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org
drivers/pci/pci_msc01.c | 7 ++----- include/msc01.h | 9 --------- 2 files changed, 2 insertions(+), 14 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:44AM +0100, Pali Rohár wrote:
PCI msc01 driver uses standard format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
PCI mvebu driver uses extended format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org --- drivers/pci/pci_mvebu.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c index 14cd82db6ff8..aa0d6bd5162c 100644 --- a/drivers/pci/pci_mvebu.c +++ b/drivers/pci/pci_mvebu.c @@ -48,15 +48,6 @@ DECLARE_GLOBAL_DATA_PTR; #define PCIE_WIN5_BASE_OFF 0x1884 #define PCIE_WIN5_REMAP_OFF 0x188c #define PCIE_CONF_ADDR_OFF 0x18f8 -#define PCIE_CONF_ADDR_EN BIT(31) -#define PCIE_CONF_REG(r) ((((r) & 0xf00) << 16) | ((r) & 0xfc)) -#define PCIE_CONF_BUS(b) (((b) & 0xff) << 16) -#define PCIE_CONF_DEV(d) (((d) & 0x1f) << 11) -#define PCIE_CONF_FUNC(f) (((f) & 0x7) << 8) -#define PCIE_CONF_ADDR(b, d, f, reg) \ - (PCIE_CONF_BUS(b) | PCIE_CONF_DEV(d) | \ - PCIE_CONF_FUNC(f) | PCIE_CONF_REG(reg) | \ - PCIE_CONF_ADDR_EN) #define PCIE_CONF_DATA_OFF 0x18fc #define PCIE_MASK_OFF 0x1910 #define PCIE_MASK_ENABLE_INTS (0xf << 24) @@ -188,9 +179,9 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf, * secondary bus with device number 1. */ if (busno == pcie->first_busno) - addr = PCIE_CONF_ADDR(pcie->sec_busno, 1, 0, offset); + addr = PCI_CONF1_EXT_ADDRESS(pcie->sec_busno, 1, 0, offset); else - addr = PCIE_CONF_ADDR(busno, PCI_DEV(bdf), PCI_FUNC(bdf), offset); + addr = PCI_CONF1_EXT_ADDRESS(busno, PCI_DEV(bdf), PCI_FUNC(bdf), offset);
/* write address */ writel(addr, pcie->base + PCIE_CONF_ADDR_OFF); @@ -286,9 +277,9 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, * secondary bus with device number 1. */ if (busno == pcie->first_busno) - addr = PCIE_CONF_ADDR(pcie->sec_busno, 1, 0, offset); + addr = PCI_CONF1_EXT_ADDRESS(pcie->sec_busno, 1, 0, offset); else - addr = PCIE_CONF_ADDR(busno, PCI_DEV(bdf), PCI_FUNC(bdf), offset); + addr = PCI_CONF1_EXT_ADDRESS(busno, PCI_DEV(bdf), PCI_FUNC(bdf), offset);
/* write address */ writel(addr, pcie->base + PCIE_CONF_ADDR_OFF);
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
PCI mvebu driver uses extended format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org
drivers/pci/pci_mvebu.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:45AM +0100, Pali Rohár wrote:
PCI mvebu driver uses extended format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
PCI tegra driver uses extended format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address function.
Signed-off-by: Pali Rohár pali@kernel.org --- drivers/pci/pci_tegra.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/pci_tegra.c b/drivers/pci/pci_tegra.c index 9cb4414836f8..fc05ee00f1fc 100644 --- a/drivers/pci/pci_tegra.c +++ b/drivers/pci/pci_tegra.c @@ -275,13 +275,6 @@ static void rp_writel(struct tegra_pcie_port *port, unsigned long value, writel(value, port->regs.start + offset); }
-static unsigned long tegra_pcie_conf_offset(pci_dev_t bdf, int where) -{ - return ((where & 0xf00) << 16) | (PCI_BUS(bdf) << 16) | - (PCI_DEV(bdf) << 11) | (PCI_FUNC(bdf) << 8) | - (where & 0xfc); -} - static int tegra_pcie_conf_address(struct tegra_pcie *pcie, pci_dev_t bdf, int where, unsigned long *address) { @@ -305,7 +298,9 @@ static int tegra_pcie_conf_address(struct tegra_pcie *pcie, pci_dev_t bdf, return -EFAULT; #endif
- *address = pcie->cs.start + tegra_pcie_conf_offset(bdf, where); + *address = pcie->cs.start + + (PCI_CONF1_EXT_ADDRESS(PCI_BUS(bdf), PCI_DEV(bdf), + PCI_FUNC(bdf), where) & ~PCI_CONF1_ENABLE); return 0; } }
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
PCI tegra driver uses extended format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address function.
Signed-off-by: Pali Rohár pali@kernel.org
drivers/pci/pci_tegra.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:46AM +0100, Pali Rohár wrote:
PCI tegra driver uses extended format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address function.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
PCI fsl 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/pcie_fsl.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pcie_fsl.c b/drivers/pci/pcie_fsl.c index 3c2a2a476111..cc6efdd5b464 100644 --- a/drivers/pci/pcie_fsl.c +++ b/drivers/pci/pcie_fsl.c @@ -58,8 +58,9 @@ static int fsl_pcie_read_config(const struct udevice *bus, pci_dev_t bdf, return 0; }
- bdf = bdf - PCI_BDF(dev_seq(bus), 0, 0); - val = bdf | (offset & 0xfc) | ((offset & 0xf00) << 16) | 0x80000000; + val = PCI_CONF1_EXT_ADDRESS(PCI_BUS(bdf) - dev_seq(bus), + PCI_DEV(bdf), PCI_FUNC(bdf), + offset); out_be32(®s->cfg_addr, val);
sync(); @@ -94,8 +95,9 @@ static int fsl_pcie_write_config(struct udevice *bus, pci_dev_t bdf, if (fsl_pcie_addr_valid(pcie, bdf)) return 0;
- bdf = bdf - PCI_BDF(dev_seq(bus), 0, 0); - val = bdf | (offset & 0xfc) | ((offset & 0xf00) << 16) | 0x80000000; + val = PCI_CONF1_EXT_ADDRESS(PCI_BUS(bdf) - dev_seq(bus), + PCI_DEV(bdf), PCI_FUNC(bdf), + offset); out_be32(®s->cfg_addr, val);
sync();
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
PCI fsl 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/pcie_fsl.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:47AM +0100, Pali Rohár wrote:
PCI fsl 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!
PCI mediatek driver uses extended format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org --- drivers/pci/pcie_mediatek.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/pcie_mediatek.c b/drivers/pci/pcie_mediatek.c index f5556713878f..051a3bc96935 100644 --- a/drivers/pci/pcie_mediatek.c +++ b/drivers/pci/pcie_mediatek.c @@ -41,10 +41,6 @@ #define PCIE_BAR_ENABLE BIT(0) #define PCIE_REVISION_ID BIT(0) #define PCIE_CLASS_CODE (0x60400 << 8) -#define PCIE_CONF_REG(regn) (((regn) & GENMASK(7, 2)) | \ - ((((regn) >> 8) & GENMASK(3, 0)) << 24)) -#define PCIE_CONF_ADDR(regn, bdf) \ - (PCIE_CONF_REG(regn) | (bdf))
/* MediaTek specific configuration registers */ #define PCIE_FTS_NUM 0x70c @@ -147,8 +143,11 @@ static int mtk_pcie_config_address(const struct udevice *udev, pci_dev_t bdf, uint offset, void **paddress) { struct mtk_pcie *pcie = dev_get_priv(udev); + u32 val;
- writel(PCIE_CONF_ADDR(offset, bdf), pcie->base + PCIE_CFG_ADDR); + val = PCI_CONF1_EXT_ADDRESS(PCI_BUS(bdf), PCI_DEV(bdf), + PCI_FUNC(bdf), offset) & ~PCI_CONF1_ENABLE; + writel(val, pcie->base + PCIE_CFG_ADDR); *paddress = pcie->base + PCIE_CFG_DATA + (offset & 3);
return 0; @@ -330,7 +329,6 @@ static void mtk_pcie_port_free(struct mtk_pcie_port *port) static int mtk_pcie_startup_port(struct mtk_pcie_port *port) { struct mtk_pcie *pcie = port->pcie; - u32 slot = PCI_DEV(port->slot << 11); u32 val; int err;
@@ -357,13 +355,14 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port) writel(PCIE_CLASS_CODE | PCIE_REVISION_ID, port->base + PCIE_CLASS);
/* configure FC credit */ - writel(PCIE_CONF_ADDR(PCIE_FC_CREDIT, slot), - pcie->base + PCIE_CFG_ADDR); + val = PCI_CONF1_EXT_ADDRESS(0, port->slot, 0, PCIE_FC_CREDIT) & ~PCI_CONF1_ENABLE; + writel(val, pcie->base + PCIE_CFG_ADDR); clrsetbits_le32(pcie->base + PCIE_CFG_DATA, PCIE_FC_CREDIT_MASK, PCIE_FC_CREDIT_VAL(0x806c));
/* configure RC FTS number to 250 when it leaves L0s */ - writel(PCIE_CONF_ADDR(PCIE_FTS_NUM, slot), pcie->base + PCIE_CFG_ADDR); + val = PCI_CONF1_EXT_ADDRESS(0, port->slot, 0, PCIE_FTS_NUM) & ~PCI_CONF1_ENABLE; + writel(val, pcie->base + PCIE_CFG_ADDR); clrsetbits_le32(pcie->base + PCIE_CFG_DATA, PCIE_FTS_NUM_MASK, PCIE_FTS_NUM_L0(0x50));
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
PCI mediatek driver uses extended format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org
drivers/pci/pcie_mediatek.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:48AM +0100, Pali Rohár wrote:
PCI mediatek driver uses extended format of Config Address for PCI Configuration Mechanism #1 but with cleared Enable bit.
So use new U-Boot macro PCI_CONF1_EXT_ADDRESS() with clearing PCI_CONF1_ENABLE bit and remove old custom driver address macros.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
PCI sh7780 driver uses standard format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org --- drivers/pci/pci_sh7780.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci_sh7780.c b/drivers/pci/pci_sh7780.c index 06d711a6cb9e..7533286c0156 100644 --- a/drivers/pci/pci_sh7780.c +++ b/drivers/pci/pci_sh7780.c @@ -34,9 +34,9 @@ int pci_sh4_read_config_dword(struct pci_controller *hose, pci_dev_t dev, int offset, u32 *value) { - u32 par_data = 0x80000000 | dev; + u32 par_data = PCI_CONF1_ADDRESS(PCI_BUS(dev), PCI_DEV(dev), PCI_FUNC(dev), offset);
- p4_out(par_data | (offset & 0xfc), SH7780_PCIPAR); + p4_out(par_data, SH7780_PCIPAR); *value = p4_in(SH7780_PCIPDR);
return 0; @@ -45,9 +45,9 @@ int pci_sh4_read_config_dword(struct pci_controller *hose, int pci_sh4_write_config_dword(struct pci_controller *hose, pci_dev_t dev, int offset, u32 value) { - u32 par_data = 0x80000000 | dev; + u32 par_data = PCI_CONF1_ADDRESS(PCI_BUS(dev), PCI_DEV(dev), PCI_FUNC(dev), offset);
- p4_out(par_data | (offset & 0xfc), SH7780_PCIPAR); + p4_out(par_data, SH7780_PCIPAR); p4_out(value, SH7780_PCIPDR); return 0; }
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
PCI sh7780 driver uses standard format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org
drivers/pci/pci_sh7780.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:49AM +0100, Pali Rohár wrote:
PCI sh7780 driver uses standard format of Config Address for PCI Configuration Mechanism #1.
So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
x86 platform uses standard format of Config Address for PCI Configuration Mechanism #1. So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org --- arch/x86/cpu/pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/cpu/pci.c b/arch/x86/cpu/pci.c index d4f9290ca73b..8a992ed82339 100644 --- a/arch/x86/cpu/pci.c +++ b/arch/x86/cpu/pci.c @@ -20,7 +20,7 @@ int pci_x86_read_config(pci_dev_t bdf, uint offset, ulong *valuep, enum pci_size_t size) { - outl(bdf | (offset & 0xfc) | PCI_CFG_EN, PCI_REG_ADDR); + outl(PCI_CONF1_ADDRESS(PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), offset), PCI_REG_ADDR); switch (size) { case PCI_SIZE_8: *valuep = inb(PCI_REG_DATA + (offset & 3)); @@ -39,7 +39,7 @@ int pci_x86_read_config(pci_dev_t bdf, uint offset, ulong *valuep, int pci_x86_write_config(pci_dev_t bdf, uint offset, ulong value, enum pci_size_t size) { - outl(bdf | (offset & 0xfc) | PCI_CFG_EN, PCI_REG_ADDR); + outl(PCI_CONF1_ADDRESS(PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), offset), PCI_REG_ADDR); switch (size) { case PCI_SIZE_8: outb(value, PCI_REG_DATA + (offset & 3));
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
x86 platform uses standard format of Config Address for PCI Configuration Mechanism #1. So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org
arch/x86/cpu/pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:50AM +0100, Pali Rohár wrote:
x86 platform uses standard format of Config Address for PCI Configuration Mechanism #1. So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
mcf5445x platform uses standard format of Config Address for PCI Configuration Mechanism #1. So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org --- arch/m68k/cpu/mcf5445x/pci.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/m68k/cpu/mcf5445x/pci.c b/arch/m68k/cpu/mcf5445x/pci.c index af02c4934c97..d487468d0bfa 100644 --- a/arch/m68k/cpu/mcf5445x/pci.c +++ b/arch/m68k/cpu/mcf5445x/pci.c @@ -26,12 +26,11 @@ int pci_##rw##_cfg_##size(struct pci_controller *hose, \ pci_dev_t dev, int offset, type val) \ { \ - u32 addr = 0; \ - u16 cfg_type = 0; \ - addr = ((offset & 0xfc) | cfg_type | (dev) | 0x80000000); \ + u32 addr = PCI_CONF1_ADDRESS(PCI_BUS(dev), PCI_DEV(dev), \ + PCI_FUNC(dev), offset); \ out_be32(hose->cfg_addr, addr); \ cfg_##rw(val, hose->cfg_data + (offset & mask), type, op); \ - out_be32(hose->cfg_addr, addr & 0x7fffffff); \ + out_be32(hose->cfg_addr, addr & ~PCI_CONF1_ENABLE); \ return 0; \ }
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
mcf5445x platform uses standard format of Config Address for PCI Configuration Mechanism #1. So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org
arch/m68k/cpu/mcf5445x/pci.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:51AM +0100, Pali Rohár wrote:
mcf5445x platform uses standard format of Config Address for PCI Configuration Mechanism #1. So use new U-Boot macro PCI_CONF1_ADDRESS().
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
sh7751 platform uses standard format of Config Address for PCI Configuration Mechanism #1.
Commit 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") which did conversion of PCI sh7751 driver to DM, broke access to config space as that commit somehow swapped device and function bits in config address.
Fix all these issues by using new U-Boot macro PCI_CONF1_ADDRESS() which calculates Config Address correctly.
Also remove nonsense function sh7751_pci_addr_valid() which was introduced in commit 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") probably due to workarounded issues with mixing/swapping device and function bits of config address which probably resulted in non-working access to some devices. With correct composing of config address there should not be such issue anymore.
Signed-off-by: Pali Rohár pali@kernel.org Fixes: 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") Cc: Marek Vasut marek.vasut+renesas@gmail.com --- drivers/pci/pci_sh7751.c | 29 ++--------------------------- 1 file changed, 2 insertions(+), 27 deletions(-)
diff --git a/drivers/pci/pci_sh7751.c b/drivers/pci/pci_sh7751.c index e110550c71c8..d514c040344c 100644 --- a/drivers/pci/pci_sh7751.c +++ b/drivers/pci/pci_sh7751.c @@ -74,33 +74,13 @@ #define p4_in(addr) (*addr) #define p4_out(data, addr) (*addr) = (data)
-static int sh7751_pci_addr_valid(pci_dev_t d, uint offset) -{ - if (PCI_FUNC(d)) - return -EINVAL; - - return 0; -} - -static u32 get_bus_address(const struct udevice *dev, pci_dev_t bdf, u32 offset) -{ - return BIT(31) | (PCI_DEV(bdf) << 8) | (offset & ~3); -} - static int sh7751_pci_read_config(const struct udevice *dev, pci_dev_t bdf, uint offset, ulong *value, enum pci_size_t size) { u32 addr, reg; - int ret;
- ret = sh7751_pci_addr_valid(bdf, offset); - if (ret) { - *value = pci_get_ff(size); - return 0; - } - - addr = get_bus_address(dev, bdf, offset); + addr = PCI_CONF1_ADDRESS(PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), offset); p4_out(addr, SH7751_PCIPAR); reg = p4_in(SH7751_PCIPDR); *value = pci_conv_32_to_size(reg, offset, size); @@ -113,13 +93,8 @@ static int sh7751_pci_write_config(struct udevice *dev, pci_dev_t bdf, enum pci_size_t size) { u32 addr, reg, old; - int ret; - - ret = sh7751_pci_addr_valid(bdf, offset); - if (ret) - return ret;
- addr = get_bus_address(dev, bdf, offset); + addr = PCI_CONF1_ADDRESS(PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), offset); p4_out(addr, SH7751_PCIPAR); old = p4_in(SH7751_PCIPDR); reg = pci_conv_size_to_32(old, value, offset, size);
On Fri, 26 Nov 2021 at 03:43, Pali Rohár pali@kernel.org wrote:
sh7751 platform uses standard format of Config Address for PCI Configuration Mechanism #1.
Commit 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") which did conversion of PCI sh7751 driver to DM, broke access to config space as that commit somehow swapped device and function bits in config address.
Fix all these issues by using new U-Boot macro PCI_CONF1_ADDRESS() which calculates Config Address correctly.
Also remove nonsense function sh7751_pci_addr_valid() which was introduced in commit 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") probably due to workarounded issues with mixing/swapping device and function bits of config address which probably resulted in non-working access to some devices. With correct composing of config address there should not be such issue anymore.
Signed-off-by: Pali Rohár pali@kernel.org Fixes: 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") Cc: Marek Vasut marek.vasut+renesas@gmail.com
drivers/pci/pci_sh7751.c | 29 ++--------------------------- 1 file changed, 2 insertions(+), 27 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Nov 26, 2021 at 11:42:52AM +0100, Pali Rohár wrote:
sh7751 platform uses standard format of Config Address for PCI Configuration Mechanism #1.
Commit 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") which did conversion of PCI sh7751 driver to DM, broke access to config space as that commit somehow swapped device and function bits in config address.
Fix all these issues by using new U-Boot macro PCI_CONF1_ADDRESS() which calculates Config Address correctly.
Also remove nonsense function sh7751_pci_addr_valid() which was introduced in commit 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") probably due to workarounded issues with mixing/swapping device and function bits of config address which probably resulted in non-working access to some devices. With correct composing of config address there should not be such issue anymore.
Signed-off-by: Pali Rohár pali@kernel.org Fixes: 72c2f4acd76f ("pci: sh7751: Convert to DM and DT probing") Cc: Marek Vasut marek.vasut+renesas@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
Hello! Could you look and review at this patch series?
On Friday 26 November 2021 11:42:40 Pali Rohár wrote:
This patch series add new U-Boot macros for composing Config Address for PCI Configuration Mechanism #1 as defined in PCI Local Bus Specification, including extended variant (which do not have any formal specification) and then use these new macros (PCI_CONF1_ADDRESS and PCI_CONF1_EXT_ADDRESS) in all PCI drivers that use Config Address according to PCI Configuration Mechanism #1.
PCI Configuration Mechanism #1 was originally specified for x86 platforms and it is still supported on x86 together with PCIe ECAM. Nowadays lot of non-ECAM-compliant ARM PCIe controllers use only extended variant of this address construction and some of them requires cleared Enable bit. Extended variant is also supported by x86 AMD Barcelona (and new) CPUs, but U-Boot code does not provide this support yet.
Note that it exists also PCI Configuration Mechanism #2, but this one was removed from PCI Local Bus Specification revision 3.0 and seems that it is not used by any PCI driver in U-Boot. So I have not added macros for this mechanism in this patch series.
Because lot of hardware still use this (rather old) mechanism, relevant U-Boot PCI and PCIe drivers have own ad-hoc code address construction, which is repeated and written in different ways.
This patch series is code cleanup of these PCIe drivers to use common U-Boot macros for PCI Configuration Mechanism #1.
The last change is fixing construction of Config Address in PCI driver sh7751. Construction with this change matches Linux kernel code and also U-Boot code prior commit mentioned in commit message.
I have tested this patch series only for PCI mvebu driver on A385 board and it is working fine. So Please properly review all other changes. Ideally test them.
I have run CI tests with these changes on github and everything passed: https://github.com/u-boot/u-boot/pull/101
Please let me know what do you think about this change.
Pali Rohár (12): pci: Add standard PCI Config Address macros pci: gt64120: Use PCI_CONF1_ADDRESS() macro pci: mpc85xx: Use PCI_CONF1_EXT_ADDRESS() macro pci: msc01: Use PCI_CONF1_ADDRESS() macro pci: mvebu: Use PCI_CONF1_EXT_ADDRESS() macro pci: tegra: Use PCI_CONF1_EXT_ADDRESS() macro pci: fsl: Use PCI_CONF1_EXT_ADDRESS() macro pci: mediatek: Use PCI_CONF1_EXT_ADDRESS() macro pci: sh7780: Use PCI_CONF1_ADDRESS() macro x86: pci: Use PCI_CONF1_ADDRESS() macro m68k: mcf5445x: pci: Use PCI_CONF1_ADDRESS() macro pci: sh7751: Fix access to config space via PCI_CONF1_ADDRESS() macro
arch/m68k/cpu/mcf5445x/pci.c | 7 +++--- arch/x86/cpu/pci.c | 4 ++-- drivers/pci/pci_gt64120.c | 7 ++---- drivers/pci/pci_mpc85xx.c | 4 ++-- drivers/pci/pci_msc01.c | 7 ++---- drivers/pci/pci_mvebu.c | 17 ++++---------- drivers/pci/pci_sh7751.c | 29 ++--------------------- drivers/pci/pci_sh7780.c | 8 +++---- drivers/pci/pci_tegra.c | 11 +++------ drivers/pci/pcie_fsl.c | 10 ++++---- drivers/pci/pcie_mediatek.c | 17 +++++++------- include/gt64120.h | 12 ---------- include/msc01.h | 9 -------- include/pci.h | 45 ++++++++++++++++++++++++++++++++++++ 14 files changed, 83 insertions(+), 104 deletions(-)
-- 2.20.1
participants (3)
-
Pali Rohár -
Simon Glass -
Tom Rini