Re: [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Nov 03, 2023 at 04:07:19PM +0200, Ilpo Järvinen wrote:
> On Fri, 3 Nov 2023, Ilpo Järvinen wrote:
> 
> > On Tue, 31 Oct 2023, Bjorn Helgaas wrote:
> > > On Fri, Oct 27, 2023 at 11:38:11AM +0300, Ilpo Järvinen wrote:
> > > > Instead of open-coded masking and shifting with PCI_CONF1_* bitfields,
> > > > use GENMASK() and FIELD_PREP(), and then remove the *_SHIFT defines
> > > > that are no longer needed.
> > 
> > > > @@ -797,19 +799,15 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
> > > >   * 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_BUS_MASK	GENMASK(23, 16)
> > > > +#define PCI_CONF1_DEV_MASK	GENMASK(15, 11)
> > > > +#define PCI_CONF1_FUNC_MASK	GENMASK(10, 8)
> > > >  #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_BUS(x)	FIELD_PREP(PCI_CONF1_BUS_MASK, (x))
> > > > +#define PCI_CONF1_DEV(x)	FIELD_PREP(PCI_CONF1_DEV_MASK, (x))
> > > > +#define PCI_CONF1_FUNC(x)	FIELD_PREP(PCI_CONF1_FUNC_MASK, (x))
> > > >  #define PCI_CONF1_REG(x)	((x) & PCI_CONF1_REG_MASK)
> > > 
> > > I love getting rid of the _SHIFT #defines.
> > >
> > > I'm a dinosaur and haven't been completely converted to the wonders of
> > > GENMASK yet.
> > 
> > Okay but would it convince even "a dinosaur" like you :-) if you imagine
> > a Bit Location column in some spec which says:
> > 	23:16
> > 
> > GENMASK just happens to mystically repeat those two numbers:
> > 	GENMASK(23, 16)
> > 
> > Pretty magic, isn't it?
> > 
> > > PCI_CONF1_ADDRESS is the only user of PCI_CONF1_BUS etc,
> > > so I think this would be simpler overall:
> > > 
> > >   #define PCI_CONF1_BUS  0x00ff0000
> > >   #define PCI_CONF1_DEV  0x0000f800
> > >   #define PCI_CONF1_FUNC 0x00000700
> > >   #define PCI_CONF1_REG  0x000000ff
> > > 
> > >   #define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
> > >     (FIELD_PREP(PCI_CONF1_BUS, (bus)) | \
> > >      FIELD_PREP(PCI_CONF1_DEV, (dev)) | \
> > >      FIELD_PREP(PCI_CONF1_FUNC, (func)) | \
> > >      FIELD_PREP(PCI_CONF1_REG, (reg & ~0x3)))
> 
> This ended up not working, because FIELD_PREP() detects ext regs not 
> fitting into PCI_CONF1_REG:
> 	FIELD_PREP(PCI_CONF1_REG, (reg) & ~0x3)
> 
> There are two partially overlapping things here when it comes to reg 
> (addressing side and parameter input side):
> 
> #define PCI_CONF1_REG_ADDR	0x000000ff
> // for PCI_CONF1_EXT_ADDRESS:
> #define PCI_CONF1_EXT_REG_ADDR	0x0f000000
> 
> /* PCI Config register (parameter input side) */
> #define PCI_CONF1_REG		0x0fc
> #define PCI_CONF1_EXT_REG	0xf00
> 
> Would those 4 defines be acceptable? Or should I mark the input side with
> _IN or use different prefix for the defines?

No opinion here yet.  Maybe PCI_CONF1_EXT_ADDRESS() needs to
explicitly pull out the low 8 bits before giving them to
PCI_CONF1_ADDRESS()?

> > Yes, it makes sense to remove the extra layer.
> > 
> > One additional thing, I just noticed lots of arch/ code is duplicating 
> > this calculation so perhaps this should also be moved outside of 
> > drivers/pci/ into include/linux/pci.h ? (Not in the same patch.)
> 
> I also noticed you took PCI_CONF1_ENABLE away from PCI_CONF1_ADDRESS(),
> did you intend for it to be moved at the caller site?
> 
> Moving it outside of PCI_CONF1_ADDRESS() would certainly help reusing this 
> code as notall arch code wants PCI_CONF1_ENABLE I think.

Honestly, I didn't think about PCI_CONF1_ENABLE; I just wanted to show
the idea of using FIELD_PREP() directly in PCI_CONF1_ADDRESS().

I *did* later think that the "& ~0x3" part maybe doesn't belong in
PCI_CONF1_ADDRESS(), since that's really connected with the use of
readl()/writel() in the caller.  Hmmm, I guess only
faraday_raw_pci_read_config() and ixp4xx_config_addr() actually use
this.  Maybe those places should mask out the low bits themselves?

Maybe the same with PCI_CONF1_ENABLE, since ixp4xx_config_addr()
removes that bit anyway?  Not sure.

Bjorn