* [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines
@ 2023-10-27 8:38 Ilpo Järvinen
2023-10-31 20:03 ` Bjorn Helgaas
0 siblings, 1 reply; 5+ messages in thread
From: Ilpo Järvinen @ 2023-10-27 8:38 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci, linux-kernel; +Cc: Ilpo Järvinen
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)
#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) | \
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines
2023-10-27 8:38 [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines Ilpo Järvinen
@ 2023-10-31 20:03 ` Bjorn Helgaas
2023-11-03 13:10 ` Ilpo Järvinen
0 siblings, 1 reply; 5+ messages in thread
From: Bjorn Helgaas @ 2023-10-31 20:03 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Bjorn Helgaas, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines
2023-10-31 20:03 ` Bjorn Helgaas
@ 2023-11-03 13:10 ` Ilpo Järvinen
2023-11-03 14:07 ` Ilpo Järvinen
0 siblings, 1 reply; 5+ messages in thread
From: Ilpo Järvinen @ 2023-11-03 13:10 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Bjorn Helgaas, linux-pci, LKML
[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]
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).
--
i.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines
2023-11-03 13:10 ` Ilpo Järvinen
@ 2023-11-03 14:07 ` Ilpo Järvinen
2023-11-03 15:51 ` Bjorn Helgaas
0 siblings, 1 reply; 5+ messages in thread
From: Ilpo Järvinen @ 2023-11-03 14:07 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Bjorn Helgaas, linux-pci, LKML
[-- Attachment #1: Type: text/plain, Size: 4007 bytes --]
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines
2023-11-03 14:07 ` Ilpo Järvinen
@ 2023-11-03 15:51 ` Bjorn Helgaas
0 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2023-11-03 15:51 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Bjorn Helgaas, linux-pci, LKML
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-11-03 15:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-27 8:38 [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines Ilpo Järvinen
2023-10-31 20:03 ` Bjorn Helgaas
2023-11-03 13:10 ` Ilpo Järvinen
2023-11-03 14:07 ` Ilpo Järvinen
2023-11-03 15:51 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).