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