[PATCH u-boot-next 01/12] pci: Add standard PCI Config Address macros

[Simon Glass]

Hi Paul,

On Fri, 26 Nov 2021 at 03:43, Pali Rohár <pali at 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 at kernel.org>
> ---
>  include/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>

Reviewed-by: Simon Glass <sjg at 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