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

[Marc Zyngier <maz@kernel.org>]

On Mon, 18 Apr 2022 16:13:39 +0100,
Peter Geis <pgwipeout@gmail.com> wrote:
> 
> On Mon, Apr 18, 2022 at 8:34 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Mon, 18 Apr 2022 12:37:00 +0100,
> > Peter Geis <pgwipeout@gmail.com> wrote:
> > >
> > > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Sat, 16 Apr 2022 14:24:26 +0100,
> > > > Peter Geis <pgwipeout@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.

And? Are your really expecting that the firmware that runs before the
kernel will preserve this register and not write anything silly to it
because, oh wait, it wants to use interrupts? Or that nobody will
kexec a secondary kernel from the first one after having used these
interrupts?

Rule #1: Initialise the HW to sensible values
Rule #2: See Rule #1

> The current rk3568 trm v1.1 does not reference an INTR_EN_LEGACY register.

The TRM for RK3588 mentions it, and is the same IP.

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

Yeah, that's completely dumb. But there is no shortage of dumb stuff
in the RK downstream code...

> 
> I've double checked and downstream only uses the mask register, enable
> and routing config appears to be left as is from reset.

And that's a bug.

> I'm rather concerned about the lack of any obvious way to control
> routing, nor an ack mechanism for the irq.

Which routing? Do you mean the affinity? You can't change it, as this
would change the affinity of all interrupts at once.

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

If you use a search engine, you'll find *CONFIDENTIAL* copies of the
DW stuff. The whole thing is a laugh anyway.

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

My only ask is that you properly initialise the HW. This will save
countless amount of head-scratching once you have a decent firmware or
kexec.

	M.

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