| Message ID | 20231027083811.9200-1-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines | expand |
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. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > > drivers/pci/pci.h | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 39a8932dc340..31da9fde8aca 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -2,6 +2,8 @@ > #ifndef DRIVERS_PCI_H > #define DRIVERS_PCI_H > > +#include <linux/bitfield.h> > +#include <linux/bits.h> > #include <linux/pci.h> > > /* Number of possible devfns: 0.0 to 1f.7 inclusive */ > @@ -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. 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))) 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. Bjorn
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))) 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.) > 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).
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). > >
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
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 39a8932dc340..31da9fde8aca 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -2,6 +2,8 @@ #ifndef DRIVERS_PCI_H #define DRIVERS_PCI_H +#include <linux/bitfield.h> +#include <linux/bits.h> #include <linux/pci.h> /* Number of possible devfns: 0.0 to 1f.7 inclusive */ @@ -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) #define PCI_CONF1_ADDRESS(bus, dev, func, reg) \ @@ -827,9 +825,8 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev) * 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 -#define PCI_CONF1_EXT_REG(x) (((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT) +#define PCI_CONF1_EXT_REG_MASK GENMASK(27, 24) +#define PCI_CONF1_EXT_REG(x) FIELD_PREP(PCI_CONF1_EXT_REG_MASK, (x) >> 8) #define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \ (PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
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. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/pci/pci.h | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)