Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220416110507.642398-1-pgwipeout@gmail.com> <20220416110507.642398-3-pgwipeout@gmail.com>
 <308e9c47197d4f7ae5a31cfcb5a10886@kernel.org> <CAMdYzYo+YeAgT92baMOoWpra230wro_WynRcajL-__9RNkeE9Q@mail.gmail.com>
 <87zgkk9gtc.wl-maz@kernel.org> <CAMdYzYo_+7rakc=GCTueEZvH_F4Co6+=eKAUztJaafiDXSKKXQ@mail.gmail.com>
 <87sfqaa7uv.wl-maz@kernel.org> <CAMdYzYouLoYX89EWjQTRAjd-1bvJEJtfYQa2KrtFH22Kp-4Efw@mail.gmail.com>
 <878rs2c8ay.wl-maz@kernel.org>
In-Reply-To: <878rs2c8ay.wl-maz@kernel.org>
From:   Peter Geis <pgwipeout@gmail.com>
Date:   Mon, 18 Apr 2022 20:23:23 -0400
Message-ID: <CAMdYzYqfitnPYSUEeeUB=x9=L9xzAu6o0SBmEdF8WKeb-+b6uQ@mail.gmail.com>
Subject: Re: [PATCH v7 2/4] PCI: dwc: rockchip: add legacy interrupt support
To:     Marc Zyngier <maz@kernel.org>
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Rob Herring <robh@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Heiko Stuebner <heiko@sntech.de>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        PCI <linux-pci@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        arm-mail-list <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Apr 18, 2022 at 6:53 PM Marc Zyngier <maz@kernel.org> wrote:
>
> 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

I don't disagree here, there are plenty of examples of bugs that stem
from this in Rockchip's code.
Working on this series has given me ideas for improvements to the
rk3399 controller as well.

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

Unfortunately this assumption doesn't hold up to testing.
On rk356x this entire register block is 0x0, which if it was
implemented means legacy interrupts would not work, among other
issues.
Even in the rk3588 it's marked as "reserved" which means there's a
good possibility it isn't fully implemented there either.
A number of other blocks in the rk3588 trm are labeled as being
available only after a specific hardware revision.
We are seeing other bugs in the hardware implementation Rockchip has
here, so making assumptions based on other implementations of the DW
IP is unsafe.

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

You'll find no argument from me here, I'm merely using it as an
example of the vendor's implementation.
The only resources I have available are the publically released
documentation and the publically released 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.

The only way to ensure that in a sane way is to trigger the resets at
driver probe.
Can that be safely done without causing other issues with an already
configured card or should I power cycle it as well?
This is starting to feature creep from the original intention of this
series, since a pre-configured controller would affect more than just
interrupts.
If you wish, as a compromise I can ensure all INTx interrupts are
masked at probe (which would hilariously be the opposite of
downstream).


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