Date:   Thu, 9 Mar 2023 19:48:20 +0000
From:   Conor Dooley <conor@...nel.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Janne Grunau <j@...nau.net>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Marc Zyngier <maz@...nel.org>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Rob Herring <robh@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Sven Peter <sven@...npeter.dev>, linux-pci@...r.kernel.org,
        asahi@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Daire McNamara <daire.mcnamara@...rochip.com>,
        Conor Dooley <conor.dooley@...rochip.com>,
        stable@...r.kernel.org
Subject: Re: [PATCH v2] PCI: apple: Set only available ports up

On Thu, Mar 09, 2023 at 10:39:35AM -0600, Bjorn Helgaas wrote:
> [+cc Daire, Conor for apple/microchip use of ECAM .init() method]

It's probably really a Daire question, although he is off in the weeds
at the moment..
> 
> On Thu, Mar 09, 2023 at 02:36:24PM +0100, Janne Grunau wrote:
> > Fixes following warning inside of_irq_parse_raw() called from the common
> > PCI device probe path.
> > 
> >   /soc/pcie@...000000/pci@1,0 interrupt-map failed, using interrupt-controller
> >   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
> 
> Based on this commit log, I assume this patch only fixes the warning,
> and the system *works* just fine either way.  If that's the case, it's
> debatable whether it meets the stable kernel criteria, although the
> documented criteria are much stricter than what happens in practice.
> 
> >   ...
> >   Call trace:
> >    of_irq_parse_raw+0x5fc/0x724
> >    of_irq_parse_and_map_pci+0x128/0x1d8
> >    pci_assign_irq+0xc8/0x140
> >    pci_device_probe+0x70/0x188
> >    really_probe+0x178/0x418
> >    __driver_probe_device+0x120/0x188
> >    driver_probe_device+0x48/0x22c
> >    __device_attach_driver+0x134/0x1d8
> >    bus_for_each_drv+0x8c/0xd8
> >    __device_attach+0xdc/0x1d0
> >    device_attach+0x20/0x2c
> >    pci_bus_add_device+0x5c/0xc0
> >    pci_bus_add_devices+0x58/0x88
> >    pci_host_probe+0x124/0x178
> >    pci_host_common_probe+0x124/0x198 [pci_host_common]
> >    apple_pcie_probe+0x108/0x16c [pcie_apple]
> >    platform_probe+0xb4/0xdc
> > 
> > This became apparent after disabling unused PCIe ports in the Apple
> > silicon device trees instead of deleting them.
> > 
> > Use for_each_available_child_of_node instead of for_each_child_of_node
> > which takes the "status" property into account.
> > 
> > Link: https://lore.kernel.org/asahi/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/
> > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/
> > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> > Cc: stable@...r.kernel.org
> > Reviewed-by: Marc Zyngier <maz@...nel.org>
> > Signed-off-by: Janne Grunau <j@...nau.net>
> > ---
> > Changes in v2:
> > - rewritten commit message with more details and corrections
> > - collected Marc's "Reviewed-by:"
> > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net
> > ---
> >  drivers/pci/controller/pcie-apple.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index 66f37e403a09..f8670a032f7a 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> >  	cfg->priv = pcie;
> >  	INIT_LIST_HEAD(&pcie->ports);
> >  
> > -	for_each_child_of_node(dev->of_node, of_port) {
> > +	for_each_available_child_of_node(dev->of_node, of_port) {
> >  		ret = apple_pcie_setup_port(pcie, of_port);
> >  		if (ret) {
> >  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
> 
> Is this change still needed after 6fffbc7ae137 ("PCI: Honor firmware's
> device disabled status")?  This is a generic problem, and it would be
> a lot nicer if we had a generic solution.  But I assume it *is* still
> needed because Rob gave his Reviewed-by.
> 
> Not related to this patch, but this function looks funny to me.  Most
> pci_ecam_ops.init functions just set up ECAM-related things.
> 
> In addition to ECAM stuff, apple_pcie_init() and mc_platform_init()
> also initialize IRQs, clocks, and resets.
> 
> Maybe we shoehorn the IRQ, clock, reset setup into pci_ecam_ops.init
> because we lack a generic hook for doing those things, but it seems a
> little muddy conceptually.

"We", and that's very much the royal variety, were in the middle of
re-working some of this stuff actually.

At the moment we're using pci_host_common_probe() as
platform_driver.probe, but Daire has a patchset where he introduced an
mc_host_probe() instead, which sets up the clocks.
The interrupt setup is still done in the pci_ecam_ops.init function
though, although with a comment added noting that "Address translation
is up; safe to enable interrupts". The most relevant patch in that
series is:
https://lore.kernel.org/linux-pci/20230111125323.1911373-9-daire.mcnamara@microchip.com/

Robin/Lorenzo had some comments about the dt parsing that needed
investigation & Daire's off in the weeds, but hopefully we'll get another
version of that stuff out once he gets back.
That'll at least move the clock setup out of the ecam bits and provide a
reason for why we're doing the interrupt setup in pci_ecam_ops.init.

FWIW, doing it this way was review feedback at some stage during the
upstreaming process for the driver:
https://lore.kernel.org/linux-pci/20200709200055.GA763222@bogus/
Originally, Daire was doing this stuff in platform_device.probe but that
was 3 years ago, so maybe the winds have changed since then!

Hope that helps?
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists