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

[Pali Rohár]

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 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() ?

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