[PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[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.

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

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Krzysztof Wilczyński]

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

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Robin Murphy]

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

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Rob Herring]

+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

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Florian Fainelli]

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

-- 
Florian

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Rob Herring]

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

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Florian Fainelli]

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


-- 
Florian

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Lorenzo Pieralisi]

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

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] PCI: brcmstb: Use BIT() as __GENMASK() is for internal use only

[Lorenzo Pieralisi]

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