PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

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 <[email protected]>

---
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(-)

drivers/pci/controller/pcie-brcmstb.c +0 -0

index 1fc7bd49a7ad..0c49fc65792c 100644

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 <[email protected]>

---
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(-)

drivers/pci/controller/pcie-brcmstb.c +1 -1

index 1fc7bd49a7ad..0c49fc65792c 100644
@@ -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);

Quoted 5 lines · Andy Shevchenko

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()...

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Andy Shevchenko

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.

Quoted 4 lines

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)? :-)

Quoted 2 lines

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.

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Andy Shevchenko

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 22 lines · Andy Shevchenko

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Andy Shevchenko

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

Quoted 2 lines

Personally I don't think that abusing BIT() in the context of setting multiple bits is any better than abusing __GENMASK()...

Quoted 3 lines

Quoted 3 lines

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.

Quoted 30 lines · Rob Herring

+Marc Z

Quoted 25 lines · Andy Shevchenko

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Andy Shevchenko

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

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().

Quoted 35 lines · Florian Fainelli

Quoted 33 lines · Rob Herring

+Marc Z

Quoted 10 lines · Andy Shevchenko

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Andy Shevchenko

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.

Quoted 15 lines

Quoted 6 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 4 lines

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:

Quoted 31 lines · Florian Fainelli

Quoted 30 lines · Rob Herring

+Marc Z

Quoted 25 lines · Andy Shevchenko

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Andy Shevchenko

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 3 lines

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:

drivers/pci/controller/pcie-brcmstb.c +2 -0

index 1fc7bd49a7ad..41404b268fa3 100644
@@ -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

drivers/pci/controller/pcie-brcmstb.c +10 -2

                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))

drivers/pci/controller/pcie-brcmstb.c +3 -0

+}
+
 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)

drivers/pci/controller/pcie-brcmstb.c +0 -3

        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

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 <[email protected]>

---
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(-)

drivers/pci/controller/pcie-brcmstb.c +1 -1

index 1fc7bd49a7ad..0c49fc65792c 100644
@@ -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);

Quoted 5 lines · Andy Shevchenko

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.

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.

[...]

Quoted 6 lines · Andy Shevchenko

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.

Quoted 3 lines

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

Quoted 13 lines · Lorenzo Pieralisi

Quoted 9 lines · Andy Shevchenko

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!

Quoted 2 lines

[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 <[email protected]>
Subject: [PATCH] PCI: brcmstb: Do not use __GENMASK