[PATCH v7 2/4] PCI: dwc: rockchip: add legacy interrupt support

[Peter Geis]

On Mon, Apr 18, 2022 at 8:34 AM Marc Zyngier <maz at kernel.org> wrote:
>
> On Mon, 18 Apr 2022 12:37:00 +0100,
> Peter Geis <pgwipeout at gmail.com> wrote:
> >
> > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <maz at kernel.org> wrote:
> > >
> > > On Sat, 16 Apr 2022 14:24:26 +0100,
> > > Peter Geis <pgwipeout at gmail.com> wrote:
> > > >
> > > > Okay, that makes sense. I'm hitting the entire block when it should be
> > > > the individual IRQ.
> > > > I also notice some drivers protect this with a spinlock while others
> > > > do not, how should this be handled?
> > >
> > > It obviously depends on how the HW. works. If this is a shared
> > > register using a RMW sequence, then you need some form of mutual
> > > exclusion in order to preserve the atomicity of the update.
> > >
> > > If the HW supports updating the masks using a set of hot bits (with
> > > separate clear/set registers), than there is no need for locking.  In
> > > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd
> > > "write-enable" feature which can probably be used to implement a
> > > lockless access, something like:
> > >
> > >         void mask(struct irq_data *d)
> > >         {
> > >                 u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq);
> >
> > This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code.
> > I believe I can safely drop the spinlock when enabling/disabling
> > individual interrupts.
>
> Yes.
>
> >
> > >                 writel_relaxed(val, ...);
> > >         }
> > >
> > >         void mask(struct irq_data *d)
> > >         {
> > >                 u32 val = BIT(d->hwirq + 16);
> > >                 writel_relaxed(val, ...);
> > >         }
> > >
> > > Another thing is that it is completely unclear to me what initialises
> > > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY).
> > > Are you relying on the firmware to do that for you?
> >
> > There is no dedicated mask or enable/disable for the legacy interrupt
> > line (unless it's undocumented).
>
> I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers,
> which control the INTx (although the latter seems to default to some
> reserved values). I don't see where you initialise them to a state
> where they are enabled and masked, which should be the initial state
> once this driver has probed. The output interrupt itself is obviously
> controlled by the GIC driver.

PCIE_CLIENT_INTR_MASK_LEGACY is the register I use here to mask/unmask
the interrupts.
It defaults to all masked on reset.
The current rk3568 trm v1.1 does not reference an INTR_EN_LEGACY register.

>
> > It appears to be enabled via an "or" function with the emulated interrupts.
> > As far as I can tell this is common for dw-pcie, looking at the other drivers.
>
> I think we're talking past each other. I'm solely concerned with the
> initialisation of the input control registers, for which I see no code
> in this patch.

Downstream points to the mask/unmask functions for the enable/disable
functions, which would be superfluous here as mainline defaults to
that anyways if they are null.

I've double checked and downstream only uses the mask register, enable
and routing config appears to be left as is from reset.
I'm rather concerned about the lack of any obvious way to control
routing, nor an ack mechanism for the irq.
I see other implementations reference the core registers or vendor
defined registers for these functions.
Unfortunately the rk3568 trm does not include the core register
definitions, and the designware documentation appears to be behind a
paywall/nda.

I suspect most of the confusion here boils down to a lack of
documentation, but it's entirely possible I am simply not
understanding the question.
I'm already aware of other functions that I need documentation for
that is currently unavailable.

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Thank you for your time,
Peter