Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

From: Rob Herring <robh@kernel.org>
To: Marc Zyngier <maz@kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	PCI <linux-pci@vger.kernel.org>,
	"Toan Le" <toan@os.amperecomputing.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Stéphane Graber" <stgraber@ubuntu.com>,
	"dann frazier" <dann.frazier@canonical.com>,
	"Android Kernel Team" <kernel-team@android.com>
Subject: Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality
Date: Mon, 21 Mar 2022 13:03:27 -0500	[thread overview]
Message-ID: <CAL_JsqK57KpZmzCE=86dLcHK4Ws_0w0ga4_qoYUe2GwFNpDzRw@mail.gmail.com> (raw)
In-Reply-To: <87h77rxnyl.wl-maz@kernel.org>

On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 21 Mar 2022 15:17:34 +0000,
> Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > unusable.
> > >
> > > It is understood that this systems come with "creative" DTs that don't
> > > match the expectations of modern kernels. However, there is little to
> > > be gained by forcing these changes on users -- the firmware is not
> > > upgradable, and the current owner of the IP will deny that these
> > > machines have ever existed.
> >
> > The gain for fixing this properly is not having drivers do their own
> > dma-ranges parsing. We've seen what happens when drivers do their own
> > parsing of standard properties (e.g. interrupt-map).
>
> We have, and we added the required exceptions for the legacy platforms
> that the code base supported until then. We didn't leave things broken
> just because we didn't like the way things were done a long time ago.
>
> > Currently, we don't have any drivers doing their own parsing:
> >
> > $ git grep of_pci_dma_range_parser_init
> > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > of_pci_range_parser *parser,
> > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > drivers/of/address.c:#define of_dma_range_parser_init
> > of_pci_dma_range_parser_init
> > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > include/linux/of_address.h:extern int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > include/linux/of_address.h:static inline int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> >
> > And we can probably further refactor this to be private to drivers/pci/of.c.
> >
> > For XGene-2 the issue is simply that the driver depends on the order
> > of dma-ranges entries.
> >
> > For XGene-1, I'd still like to understand what the issue is. Reverting
> > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > dump of how the IB registers are initialized in both cases. I'm not
> > saying changing 'dma-ranges' in the firmware is going to be required
> > here. There's a couple of other ways we could fix that without a
> > firmware change, but first I need to understand why it broke.
>
> Reverting 6dce5aa59e0b was enough for me, without changing anything
> else.

Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.

Can you tell me what 'dma-ranges' contains on your system?

> m400 probably uses an even older firmware (AFAIR, it was stuck
> with an ancient version of u-boot that HP never updated, while Mustang
> had a few updates). In any case, that DT cannot be changed.

How is Dann changing it then? I assume he's not changing the firmware,
but overriding it. That could be a possible solution.

Do the DT's in the kernel tree correspond to anything anyone is using?
I ask because at some point someone will need to address all the
warnings they have or we should drop the dts files if they aren't
close to reality. The same thing applies to Seattle BTW.

> > Rob
> >
> > P.S. We're carrying ACPI and DT support for these platforms. It seems
> > the few users are using DT, so can we drop the ACPI support? Or do I
> > need to break it first and wait a year? ;)
>
> I'm not sure people on the list are representative of all the users,
> and I didn't realise the plan was "let's break everything we don't
> like and see if someone wakes up" either. That definitely puts things
> in a different perspective.

I wasn't really suggesting breaking things on purpose. However, there
is a cost to keeping code and it would be nice to know what's being
used or not. The cost isn't *that* big, but it is not zero for what's
not many users.

At least for Stéphane, using ACPI didn't even work. I'm assuming there
is some version of h/w and f/w out there that did work with the ACPI
support in the kernel? That may have never been seen by anyone but APM
and Jon Masters (his Tested-by is on the patch from APM adding ACPI
support). It's not hard to imagine there was a version of firmware
just to shut Jon up.

Rob