| Message ID | 20211115112000.23693-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [v2,1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only | expand |
Hi Andy, > Use BIT() as __GENMASK() is for internal use only. The rationale > of switching to BIT() is to provide better generated code. The > GENMASK() against non-constant numbers may produce an ugly assembler > code. On contrary the BIT() is simply converted to corresponding shift > operation. > > Note, it's the only user of __GENMASK() in the kernel outside of its own realm. > > Fixes: 3baec684a531 ("PCI: brcmstb: Accommodate MSI for older chips") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v2: switched to BIT() and elaborated why, hence not included tag > drivers/pci/controller/pcie-brcmstb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 1fc7bd49a7ad..0c49fc65792c 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -619,7 +619,7 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) > > static void brcm_msi_set_regs(struct brcm_msi *msi) > { > - u32 val = __GENMASK(31, msi->legacy_shift); > + u32 val = ~(BIT(msi->legacy_shift) - 1); Thank you! I also personally prefer using BIT() macro here. Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Krzysztof
On 2021-11-15 11:20, Andy Shevchenko wrote: > Use BIT() as __GENMASK() is for internal use only. The rationale > of switching to BIT() is to provide better generated code. The > GENMASK() against non-constant numbers may produce an ugly assembler > code. On contrary the BIT() is simply converted to corresponding shift > operation. FWIW, If you care about code quality and want the compiler to do the obvious thing, why not specify it as the obvious thing: u32 val = ~0 << msi->legacy_shift; Personally I don't think that abusing BIT() in the context of setting multiple bits is any better than abusing __GENMASK()... Robin. > Note, it's the only user of __GENMASK() in the kernel outside of its own realm. > > Fixes: 3baec684a531 ("PCI: brcmstb: Accommodate MSI for older chips") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v2: switched to BIT() and elaborated why, hence not included tag > drivers/pci/controller/pcie-brcmstb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 1fc7bd49a7ad..0c49fc65792c 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -619,7 +619,7 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) > > static void brcm_msi_set_regs(struct brcm_msi *msi) > { > - u32 val = __GENMASK(31, msi->legacy_shift); > + u32 val = ~(BIT(msi->legacy_shift) - 1); > > writel(val, msi->intr_base + MSI_INT_MASK_CLR); > writel(val, msi->intr_base + MSI_INT_CLR); >
On Mon, Nov 15, 2021 at 4:01 PM Robin Murphy <robin.murphy@arm.com> wrote: > On 2021-11-15 11:20, Andy Shevchenko wrote: > > Use BIT() as __GENMASK() is for internal use only. The rationale > > of switching to BIT() is to provide better generated code. The > > GENMASK() against non-constant numbers may produce an ugly assembler > > code. On contrary the BIT() is simply converted to corresponding shift > > operation. > > FWIW, If you care about code quality and want the compiler to do the > obvious thing, why not specify it as the obvious thing: > > u32 val = ~0 << msi->legacy_shift; Obvious and buggy (from the C standard point of view)? :-) > Personally I don't think that abusing BIT() in the context of setting > multiple bits is any better than abusing __GENMASK()... No, BIT() is not abused here, but __GENMASK(). After all it's up to you, folks, consider that as a bug report.
On Mon, Nov 15, 2021 at 04:14:21PM +0200, Andy Shevchenko wrote: > On Mon, Nov 15, 2021 at 4:01 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2021-11-15 11:20, Andy Shevchenko wrote: > > > Use BIT() as __GENMASK() is for internal use only. The rationale > > > of switching to BIT() is to provide better generated code. The > > > GENMASK() against non-constant numbers may produce an ugly assembler > > > code. On contrary the BIT() is simply converted to corresponding shift > > > operation. > > > > FWIW, If you care about code quality and want the compiler to do the > > obvious thing, why not specify it as the obvious thing: > > > > u32 val = ~0 << msi->legacy_shift; > > Obvious and buggy (from the C standard point of view)? :-) Forgot to mention that BIT() is also makes it easy to avoid such mistake. > > Personally I don't think that abusing BIT() in the context of setting > > multiple bits is any better than abusing __GENMASK()... > > No, BIT() is not abused here, but __GENMASK(). > > After all it's up to you, folks, consider that as a bug report.
+Marc Z On Mon, Nov 15, 2021 at 8:39 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Nov 15, 2021 at 04:14:21PM +0200, Andy Shevchenko wrote: > > On Mon, Nov 15, 2021 at 4:01 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > On 2021-11-15 11:20, Andy Shevchenko wrote: > > > > Use BIT() as __GENMASK() is for internal use only. The rationale > > > > of switching to BIT() is to provide better generated code. The > > > > GENMASK() against non-constant numbers may produce an ugly assembler > > > > code. On contrary the BIT() is simply converted to corresponding shift > > > > operation. > > > > > > FWIW, If you care about code quality and want the compiler to do the > > > obvious thing, why not specify it as the obvious thing: > > > > > > u32 val = ~0 << msi->legacy_shift; > > > > Obvious and buggy (from the C standard point of view)? :-) > > Forgot to mention that BIT() is also makes it easy to avoid such mistake. > > > > Personally I don't think that abusing BIT() in the context of setting > > > multiple bits is any better than abusing __GENMASK()... > > > > No, BIT() is not abused here, but __GENMASK(). > > > > After all it's up to you, folks, consider that as a bug report. Couldn't we get rid of legacy_shift entirely if the legacy case sets up 'hwirq' as 24-31 rather than 0-7? Though the data for the MSI msg uses the hwirq. Rob
On 11/15/21 3:20 AM, Andy Shevchenko wrote: > Use BIT() as __GENMASK() is for internal use only. The rationale > of switching to BIT() is to provide better generated code. The > GENMASK() against non-constant numbers may produce an ugly assembler > code. On contrary the BIT() is simply converted to corresponding shift > operation. The code is not necessarily any different on ARMv8 as far as I can tell, before: static void brcm_msi_set_regs(struct brcm_msi *msi) { u32 val = __GENMASK(31, msi->legacy_shift); 84: b9406402 ldr w2, [x0,#100] 88: d2800021 mov x1, #0x1 // #1 8c: 9ac22021 lsl x1, x1, x2 90: 4b0103e1 neg w1, w1 after: static void brcm_msi_set_regs(struct brcm_msi *msi) { u32 val = ~(BIT(msi->legacy_shift) - 1); 84: b9406402 ldr w2, [x0,#100] 88: d2800021 mov x1, #0x1 // #1 8c: 9ac22021 lsl x1, x1, x2 90: 4b0103e1 neg w1, w1 and the usage of BIT() does not make this any clearer. How about this instead: diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 1fc7bd49a7ad..f832c07337fa 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -144,6 +144,8 @@ #define BRCM_INT_PCI_MSI_NR 32 #define BRCM_INT_PCI_MSI_LEGACY_NR 8 #define BRCM_INT_PCI_MSI_SHIFT 0 +#define BRCM_INT_PCI_MSI_MASK GENMASK(BRCM_INT_PCI_MSI_NR - 1, 0) +#define BRCM_INT_PCI_MSI_LEGACY_MASK GENMASK(31, 32 - BRCM_INT_PCI_MSI_LEGACY_NR) /* MSI target addresses */ #define BRCM_MSI_TARGET_ADDR_LT_4GB 0x0fffffffcULL @@ -619,7 +621,8 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) static void brcm_msi_set_regs(struct brcm_msi *msi) { - u32 val = __GENMASK(31, msi->legacy_shift); + u32 val = msi->legacy ? BRCM_INT_PCI_MSI_MASK : + BRCM_INT_PCI_MSI_LEGACY_MASK; writel(val, msi->intr_base + MSI_INT_MASK_CLR); writel(val, msi->intr_base + MSI_INT_CLR); > > Note, it's the only user of __GENMASK() in the kernel outside of its own realm. > > Fixes: 3baec684a531 ("PCI: brcmstb: Accommodate MSI for older chips") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v2: switched to BIT() and elaborated why, hence not included tag > drivers/pci/controller/pcie-brcmstb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 1fc7bd49a7ad..0c49fc65792c 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -619,7 +619,7 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) > > static void brcm_msi_set_regs(struct brcm_msi *msi) > { > - u32 val = __GENMASK(31, msi->legacy_shift); > + u32 val = ~(BIT(msi->legacy_shift) - 1); > > writel(val, msi->intr_base + MSI_INT_MASK_CLR); > writel(val, msi->intr_base + MSI_INT_CLR); >
On 11/16/21 10:20 AM, Rob Herring wrote: > +Marc Z > > On Mon, Nov 15, 2021 at 8:39 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> >> On Mon, Nov 15, 2021 at 04:14:21PM +0200, Andy Shevchenko wrote: >>> On Mon, Nov 15, 2021 at 4:01 PM Robin Murphy <robin.murphy@arm.com> wrote: >>>> On 2021-11-15 11:20, Andy Shevchenko wrote: >>>>> Use BIT() as __GENMASK() is for internal use only. The rationale >>>>> of switching to BIT() is to provide better generated code. The >>>>> GENMASK() against non-constant numbers may produce an ugly assembler >>>>> code. On contrary the BIT() is simply converted to corresponding shift >>>>> operation. >>>> >>>> FWIW, If you care about code quality and want the compiler to do the >>>> obvious thing, why not specify it as the obvious thing: >>>> >>>> u32 val = ~0 << msi->legacy_shift; >>> >>> Obvious and buggy (from the C standard point of view)? :-) >> >> Forgot to mention that BIT() is also makes it easy to avoid such mistake. >> >>>> Personally I don't think that abusing BIT() in the context of setting >>>> multiple bits is any better than abusing __GENMASK()... >>> >>> No, BIT() is not abused here, but __GENMASK(). >>> >>> After all it's up to you, folks, consider that as a bug report. > > Couldn't we get rid of legacy_shift entirely if the legacy case sets > up 'hwirq' as 24-31 rather than 0-7? Though the data for the MSI msg > uses the hwirq. I personally find it clearer and easier to reason about with the current code though I suppose that with an appropriate xlate method we could sort of set up the hwirq the way we want them to be to avoid any shifting in brcm_pcie_msi_isr().
On 11/16/21 12:41 PM, Florian Fainelli wrote: > On 11/16/21 10:20 AM, Rob Herring wrote: >> +Marc Z >> >> On Mon, Nov 15, 2021 at 8:39 AM Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >>> >>> On Mon, Nov 15, 2021 at 04:14:21PM +0200, Andy Shevchenko wrote: >>>> On Mon, Nov 15, 2021 at 4:01 PM Robin Murphy <robin.murphy@arm.com> wrote: >>>>> On 2021-11-15 11:20, Andy Shevchenko wrote: >>>>>> Use BIT() as __GENMASK() is for internal use only. The rationale >>>>>> of switching to BIT() is to provide better generated code. The >>>>>> GENMASK() against non-constant numbers may produce an ugly assembler >>>>>> code. On contrary the BIT() is simply converted to corresponding shift >>>>>> operation. >>>>> >>>>> FWIW, If you care about code quality and want the compiler to do the >>>>> obvious thing, why not specify it as the obvious thing: >>>>> >>>>> u32 val = ~0 << msi->legacy_shift; >>>> >>>> Obvious and buggy (from the C standard point of view)? :-) >>> >>> Forgot to mention that BIT() is also makes it easy to avoid such mistake. >>> >>>>> Personally I don't think that abusing BIT() in the context of setting >>>>> multiple bits is any better than abusing __GENMASK()... >>>> >>>> No, BIT() is not abused here, but __GENMASK(). >>>> >>>> After all it's up to you, folks, consider that as a bug report. >> >> Couldn't we get rid of legacy_shift entirely if the legacy case sets >> up 'hwirq' as 24-31 rather than 0-7? Though the data for the MSI msg >> uses the hwirq. > > I personally find it clearer and easier to reason about with the current > code though I suppose that with an appropriate xlate method we could > sort of set up the hwirq the way we want them to be to avoid any > shifting in brcm_pcie_msi_isr(). Something like the following maybe? Completely untested as I don't believe I have a device with that legacy controller available at the moment: diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 1fc7bd49a7ad..41404b268fa3 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -144,6 +144,8 @@ #define BRCM_INT_PCI_MSI_NR 32 #define BRCM_INT_PCI_MSI_LEGACY_NR 8 #define BRCM_INT_PCI_MSI_SHIFT 0 +#define BRCM_INT_PCI_MSI_MASK GENMASK(BRCM_INT_PCI_MSI_NR - 1, 0) +#define BRCM_INT_PCI_MSI_LEGACY_MASK GENMASK(31, 32 - BRCM_INT_PCI_MSI_LEGACY_NR) /* MSI target addresses */ #define BRCM_MSI_TARGET_ADDR_LT_4GB 0x0fffffffcULL @@ -269,8 +271,6 @@ struct brcm_msi { /* used indicates which MSI interrupts have been alloc'd */ unsigned long used; bool legacy; - /* Some chips have MSIs in bits [31..24] of a shared register. */ - int legacy_shift; int nr; /* No. of MSI available, depends on chip */ /* This is the base pointer for interrupt status/set/clr regs */ void __iomem *intr_base; @@ -486,7 +486,6 @@ static void brcm_pcie_msi_isr(struct irq_desc *desc) dev = msi->dev; status = readl(msi->intr_base + MSI_INT_STATUS); - status >>= msi->legacy_shift; for_each_set_bit(bit, &status, msi->nr) { int ret; @@ -516,9 +515,8 @@ static int brcm_msi_set_affinity(struct irq_data *irq_data, static void brcm_msi_ack_irq(struct irq_data *data) { struct brcm_msi *msi = irq_data_get_irq_chip_data(data); - const int shift_amt = data->hwirq + msi->legacy_shift; - writel(1 << shift_amt, msi->intr_base + MSI_INT_CLR); + writel(BIT(data->hwirq), msi->intr_base + MSI_INT_CLR); } @@ -573,9 +571,31 @@ static void brcm_irq_domain_free(struct irq_domain *domain, brcm_msi_free(msi, d->hwirq); } +static int brcm_irq_domain_xlate(struct irq_domain *d, + struct device_node *node, + const u32 *intspec, unsigned int intsize, + unsigned long *out_hwirq, + unsigned int *out_type) +{ + struct brcm_msi *msi = d->host_data; + + if (WARN_ON(intsize < 1)) + return -EINVAL; + + if (msi->legacy) { + *out_hwirq = intspec[0] + BRCM_INT_PCI_MSI_SHIFT; + *out_type = IRQ_TYPE_NONE; + return 0; + } + + return irq_domain_xlate_onecell(d, node, intspec, intsize, + out_hwirq, out_type); +} + static const struct irq_domain_ops msi_domain_ops = { .alloc = brcm_irq_domain_alloc, .free = brcm_irq_domain_free, + .xlate = brcm_irq_domain_xlate, }; static int brcm_allocate_domains(struct brcm_msi *msi) @@ -619,7 +639,8 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) static void brcm_msi_set_regs(struct brcm_msi *msi) { - u32 val = __GENMASK(31, msi->legacy_shift); + u32 val = msi->legacy ? BRCM_INT_PCI_MSI_MASK : + BRCM_INT_PCI_MSI_LEGACY_MASK; writel(val, msi->intr_base + MSI_INT_MASK_CLR); writel(val, msi->intr_base + MSI_INT_CLR); @@ -664,11 +685,9 @@ static int brcm_pcie_enable_msi(struct brcm_pcie *pcie) if (msi->legacy) { msi->intr_base = msi->base + PCIE_INTR2_CPU_BASE; msi->nr = BRCM_INT_PCI_MSI_LEGACY_NR; - msi->legacy_shift = 24; } else { msi->intr_base = msi->base + PCIE_MSI_INTR2_BASE; msi->nr = BRCM_INT_PCI_MSI_NR; - msi->legacy_shift = 0; } ret = brcm_allocate_domains(msi);
On Wed, Nov 17, 2021 at 12:42 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 11/16/21 12:41 PM, Florian Fainelli wrote: > > On 11/16/21 10:20 AM, Rob Herring wrote: > >> +Marc Z > >> > >> On Mon, Nov 15, 2021 at 8:39 AM Andy Shevchenko > >> <andriy.shevchenko@linux.intel.com> wrote: > >>> > >>> On Mon, Nov 15, 2021 at 04:14:21PM +0200, Andy Shevchenko wrote: > >>>> On Mon, Nov 15, 2021 at 4:01 PM Robin Murphy <robin.murphy@arm.com> wrote: > >>>>> On 2021-11-15 11:20, Andy Shevchenko wrote: > >>>>>> Use BIT() as __GENMASK() is for internal use only. The rationale > >>>>>> of switching to BIT() is to provide better generated code. The > >>>>>> GENMASK() against non-constant numbers may produce an ugly assembler > >>>>>> code. On contrary the BIT() is simply converted to corresponding shift > >>>>>> operation. > >>>>> > >>>>> FWIW, If you care about code quality and want the compiler to do the > >>>>> obvious thing, why not specify it as the obvious thing: > >>>>> > >>>>> u32 val = ~0 << msi->legacy_shift; > >>>> > >>>> Obvious and buggy (from the C standard point of view)? :-) > >>> > >>> Forgot to mention that BIT() is also makes it easy to avoid such mistake. > >>> > >>>>> Personally I don't think that abusing BIT() in the context of setting > >>>>> multiple bits is any better than abusing __GENMASK()... > >>>> > >>>> No, BIT() is not abused here, but __GENMASK(). > >>>> > >>>> After all it's up to you, folks, consider that as a bug report. > >> > >> Couldn't we get rid of legacy_shift entirely if the legacy case sets > >> up 'hwirq' as 24-31 rather than 0-7? Though the data for the MSI msg > >> uses the hwirq. > > > > I personally find it clearer and easier to reason about with the current > > code though I suppose that with an appropriate xlate method we could > > sort of set up the hwirq the way we want them to be to avoid any > > shifting in brcm_pcie_msi_isr(). > > Something like the following maybe? Completely untested as I don't > believe I have a device with that legacy controller available at the moment: Since it gets rid of __GENMASK (ab)use, I'm fine to see it applied at some point.
On Tue, Nov 16, 2021 at 12:38:39PM -0800, Florian Fainelli wrote: > On 11/15/21 3:20 AM, Andy Shevchenko wrote: > > Use BIT() as __GENMASK() is for internal use only. The rationale > > of switching to BIT() is to provide better generated code. The > > GENMASK() against non-constant numbers may produce an ugly assembler > > code. On contrary the BIT() is simply converted to corresponding shift > > operation. > > The code is not necessarily any different on ARMv8 as far as I can tell, > before: > > static void brcm_msi_set_regs(struct brcm_msi *msi) > { > u32 val = __GENMASK(31, msi->legacy_shift); > 84: b9406402 ldr w2, [x0,#100] > 88: d2800021 mov x1, #0x1 > // #1 > 8c: 9ac22021 lsl x1, x1, x2 > 90: 4b0103e1 neg w1, w1 > > > after: > > static void brcm_msi_set_regs(struct brcm_msi *msi) > { > u32 val = ~(BIT(msi->legacy_shift) - 1); > 84: b9406402 ldr w2, [x0,#100] > 88: d2800021 mov x1, #0x1 > // #1 > 8c: 9ac22021 lsl x1, x1, x2 > 90: 4b0103e1 neg w1, w1 > > and the usage of BIT() does not make this any clearer. While I disagree on the conclusion it's good that assembly isn't bad. Last time I have tried to compile just GENMASK() excerpts for arm32 the non-constant variants were quite bad. And it was obvious win for BIT() over GENMASK(). Actually it maybe that I have tested something like `GENMASK(C1 + var, C2 + var)` vs. `GENMASK(C1, C2) << var` that time.
On Tue, Nov 16, 2021 at 2:56 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 11/16/21 12:41 PM, Florian Fainelli wrote: > > On 11/16/21 10:20 AM, Rob Herring wrote: > >> +Marc Z > >> > >> On Mon, Nov 15, 2021 at 8:39 AM Andy Shevchenko > >> <andriy.shevchenko@linux.intel.com> wrote: > >>> > >>> On Mon, Nov 15, 2021 at 04:14:21PM +0200, Andy Shevchenko wrote: > >>>> On Mon, Nov 15, 2021 at 4:01 PM Robin Murphy <robin.murphy@arm.com> wrote: > >>>>> On 2021-11-15 11:20, Andy Shevchenko wrote: > >>>>>> Use BIT() as __GENMASK() is for internal use only. The rationale > >>>>>> of switching to BIT() is to provide better generated code. The > >>>>>> GENMASK() against non-constant numbers may produce an ugly assembler > >>>>>> code. On contrary the BIT() is simply converted to corresponding shift > >>>>>> operation. > >>>>> > >>>>> FWIW, If you care about code quality and want the compiler to do the > >>>>> obvious thing, why not specify it as the obvious thing: > >>>>> > >>>>> u32 val = ~0 << msi->legacy_shift; > >>>> > >>>> Obvious and buggy (from the C standard point of view)? :-) > >>> > >>> Forgot to mention that BIT() is also makes it easy to avoid such mistake. > >>> > >>>>> Personally I don't think that abusing BIT() in the context of setting > >>>>> multiple bits is any better than abusing __GENMASK()... > >>>> > >>>> No, BIT() is not abused here, but __GENMASK(). > >>>> > >>>> After all it's up to you, folks, consider that as a bug report. > >> > >> Couldn't we get rid of legacy_shift entirely if the legacy case sets > >> up 'hwirq' as 24-31 rather than 0-7? Though the data for the MSI msg > >> uses the hwirq. > > > > I personally find it clearer and easier to reason about with the current > > code though I suppose that with an appropriate xlate method we could > > sort of set up the hwirq the way we want them to be to avoid any > > shifting in brcm_pcie_msi_isr(). > > Something like the following maybe? Completely untested as I don't > believe I have a device with that legacy controller available at the moment: > > diff --git a/drivers/pci/controller/pcie-brcmstb.c > b/drivers/pci/controller/pcie-brcmstb.c > index 1fc7bd49a7ad..41404b268fa3 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -144,6 +144,8 @@ > #define BRCM_INT_PCI_MSI_NR 32 > #define BRCM_INT_PCI_MSI_LEGACY_NR 8 > #define BRCM_INT_PCI_MSI_SHIFT 0 > +#define BRCM_INT_PCI_MSI_MASK GENMASK(BRCM_INT_PCI_MSI_NR - 1, 0) > +#define BRCM_INT_PCI_MSI_LEGACY_MASK GENMASK(31, 32 - > BRCM_INT_PCI_MSI_LEGACY_NR) > > /* MSI target addresses */ > #define BRCM_MSI_TARGET_ADDR_LT_4GB 0x0fffffffcULL > @@ -269,8 +271,6 @@ struct brcm_msi { > /* used indicates which MSI interrupts have been alloc'd */ > unsigned long used; > bool legacy; > - /* Some chips have MSIs in bits [31..24] of a shared register. */ > - int legacy_shift; > int nr; /* No. of MSI available, depends on chip */ Can get rid of this too I think. > /* This is the base pointer for interrupt status/set/clr regs */ > void __iomem *intr_base; > @@ -486,7 +486,6 @@ static void brcm_pcie_msi_isr(struct irq_desc *desc) > dev = msi->dev; > > status = readl(msi->intr_base + MSI_INT_STATUS); > - status >>= msi->legacy_shift; > > for_each_set_bit(bit, &status, msi->nr) { 'nr' needs to be 32 here. > int ret; > @@ -516,9 +515,8 @@ static int brcm_msi_set_affinity(struct irq_data > *irq_data, > static void brcm_msi_ack_irq(struct irq_data *data) > { > struct brcm_msi *msi = irq_data_get_irq_chip_data(data); > - const int shift_amt = data->hwirq + msi->legacy_shift; > > - writel(1 << shift_amt, msi->intr_base + MSI_INT_CLR); > + writel(BIT(data->hwirq), msi->intr_base + MSI_INT_CLR); > } > > > @@ -573,9 +571,31 @@ static void brcm_irq_domain_free(struct irq_domain > *domain, > brcm_msi_free(msi, d->hwirq); > } > > +static int brcm_irq_domain_xlate(struct irq_domain *d, > + struct device_node *node, > + const u32 *intspec, unsigned int intsize, > + unsigned long *out_hwirq, > + unsigned int *out_type) > +{ > + struct brcm_msi *msi = d->host_data; > + > + if (WARN_ON(intsize < 1)) > + return -EINVAL; > + > + if (msi->legacy) { > + *out_hwirq = intspec[0] + BRCM_INT_PCI_MSI_SHIFT; > + *out_type = IRQ_TYPE_NONE; > + return 0; > + } > + > + return irq_domain_xlate_onecell(d, node, intspec, intsize, > + out_hwirq, out_type); When would xlate get called? You don't have an intspec from DT. Wouldn't it be enough to set bits 0-23 in 'used' bitmap so that only 24-31 can be allocated? I'm not really sure though with how all the MSI stuff works. > +} > + > static const struct irq_domain_ops msi_domain_ops = { > .alloc = brcm_irq_domain_alloc, > .free = brcm_irq_domain_free, > + .xlate = brcm_irq_domain_xlate, > }; > > static int brcm_allocate_domains(struct brcm_msi *msi) > @@ -619,7 +639,8 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) > > static void brcm_msi_set_regs(struct brcm_msi *msi) > { > - u32 val = __GENMASK(31, msi->legacy_shift); > + u32 val = msi->legacy ? BRCM_INT_PCI_MSI_MASK : > + BRCM_INT_PCI_MSI_LEGACY_MASK; Perhaps just change legacy to a mask. > > writel(val, msi->intr_base + MSI_INT_MASK_CLR); > writel(val, msi->intr_base + MSI_INT_CLR); > @@ -664,11 +685,9 @@ static int brcm_pcie_enable_msi(struct brcm_pcie *pcie) > if (msi->legacy) { > msi->intr_base = msi->base + PCIE_INTR2_CPU_BASE; > msi->nr = BRCM_INT_PCI_MSI_LEGACY_NR; > - msi->legacy_shift = 24; > } else { > msi->intr_base = msi->base + PCIE_MSI_INTR2_BASE; > msi->nr = BRCM_INT_PCI_MSI_NR; > - msi->legacy_shift = 0; > } > > ret = brcm_allocate_domains(msi); > > -- > Florian
On Mon, 15 Nov 2021 13:20:00 +0200, Andy Shevchenko wrote: > Use BIT() as __GENMASK() is for internal use only. The rationale > of switching to BIT() is to provide better generated code. The > GENMASK() against non-constant numbers may produce an ugly assembler > code. On contrary the BIT() is simply converted to corresponding shift > operation. > > Note, it's the only user of __GENMASK() in the kernel outside of its own realm. > > [...] Applied to pci/brcmstb, thanks! [1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only https://git.kernel.org/lpieralisi/pci/c/6ec6eb949d Thanks, Lorenzo
On Wed, Dec 01, 2021 at 03:53:21PM +0000, Lorenzo Pieralisi wrote: > On Mon, 15 Nov 2021 13:20:00 +0200, Andy Shevchenko wrote: > > Use BIT() as __GENMASK() is for internal use only. The rationale > > of switching to BIT() is to provide better generated code. The > > GENMASK() against non-constant numbers may produce an ugly assembler > > code. On contrary the BIT() is simply converted to corresponding shift > > operation. > > > > Note, it's the only user of __GENMASK() in the kernel outside of its own realm. > > > > [...] > > Applied to pci/brcmstb, thanks! > > [1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only > https://git.kernel.org/lpieralisi/pci/c/6ec6eb949d Thanks, but there is another patch which changes the logic a bit and cleans up more. From: Florian Fainelli <f.fainelli@gmail.com> Subject: [PATCH] PCI: brcmstb: Do not use __GENMASK
On Wed, Dec 01, 2021 at 06:01:46PM +0200, Andy Shevchenko wrote: > On Wed, Dec 01, 2021 at 03:53:21PM +0000, Lorenzo Pieralisi wrote: > > On Mon, 15 Nov 2021 13:20:00 +0200, Andy Shevchenko wrote: > > > Use BIT() as __GENMASK() is for internal use only. The rationale > > > of switching to BIT() is to provide better generated code. The > > > GENMASK() against non-constant numbers may produce an ugly assembler > > > code. On contrary the BIT() is simply converted to corresponding shift > > > operation. > > > > > > Note, it's the only user of __GENMASK() in the kernel outside of its own realm. > > > > > > [...] > > > > Applied to pci/brcmstb, thanks! > > > > [1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only > > https://git.kernel.org/lpieralisi/pci/c/6ec6eb949d > > Thanks, but there is another patch which changes the logic a bit and cleans up > more. > > From: Florian Fainelli <f.fainelli@gmail.com> > Subject: [PATCH] PCI: brcmstb: Do not use __GENMASK Right, I pulled that one instead, thanks for the heads-up. Lorenzo
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 1fc7bd49a7ad..0c49fc65792c 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -619,7 +619,7 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) static void brcm_msi_set_regs(struct brcm_msi *msi) { - u32 val = __GENMASK(31, msi->legacy_shift); + u32 val = ~(BIT(msi->legacy_shift) - 1); writel(val, msi->intr_base + MSI_INT_MASK_CLR); writel(val, msi->intr_base + MSI_INT_CLR);
Use BIT() as __GENMASK() is for internal use only. The rationale of switching to BIT() is to provide better generated code. The GENMASK() against non-constant numbers may produce an ugly assembler code. On contrary the BIT() is simply converted to corresponding shift operation. Note, it's the only user of __GENMASK() in the kernel outside of its own realm. Fixes: 3baec684a531 ("PCI: brcmstb: Accommodate MSI for older chips") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v2: switched to BIT() and elaborated why, hence not included tag drivers/pci/controller/pcie-brcmstb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)