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

From: Rob Herring
Date: Wed Nov 17 2021 - 17:47:01 EST

On Tue, Nov 16, 2021 at 2:56 PM Florian Fainelli <f.fainelli@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxx> 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