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? > 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. > > The v6.7 merge window just opened, and I won't start merging v6.8 > > material until v6.7-rc1 (probably Nov 12), so no hurry on this. > > Yes I knew I was sending it quite late because I tried to meet your > request in getting it all done in the same merge window (which I > obviously failed but it isn't the end of the world). > > -- i.
