linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-kernel@vger.kernel.org,
	Florian Fainelli <f.fainelli@gmail.com>,
	bcm-kernel-feedback-list@broadcom.com,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Andrew Murray <amurray@thegoodpenguin.co.uk>,
	linux-usb@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
	tim.gover@raspberrypi.org, linux-pci@vger.kernel.org,
	wahrenst@gmx.net, sergei.shtylyov@cogentembedded.com
Subject: Re: [PATCH v6 3/4] PCI: brcmstb: Wait for Raspberry Pi's firmware when present
Date: Thu, 02 Apr 2020 16:27:23 +0200	[thread overview]
Message-ID: <47c543e2144d5247743548b00d1931e9fc217f43.camel@suse.de> (raw)
In-Reply-To: <20200401204149.GA131584@google.com>

[-- Attachment #1: Type: text/plain, Size: 3502 bytes --]

Hi Bjorn,

On Wed, 2020-04-01 at 15:41 -0500, Bjorn Helgaas wrote:
> On Tue, Mar 24, 2020 at 07:28:11PM +0100, Nicolas Saenz Julienne wrote:
> > xHCI's PCI fixup, run at the end of pcie-brcmstb's probe, depends on
> 
> Is there a function name for this fixup that you can mention?

Yes, rpi_firmware_init_vl805(), I'll update the description.

> > RPi4's VideoCore firmware interface to be up and running. It's possible
> > for both initializations to race, so make sure it's available prior to
> > starting.
> 
> I guess "both initializations" means brcm_pcie_probe() and something
> else?  It'd be nice to include that function name here, too.

Noted, I'll be more explicit on the next version of the series. More in depth
explanation below.

> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c
> > b/drivers/pci/controller/pcie-brcmstb.c
> > index 3a10e678c7f4..a3d3070a5832 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -28,6 +28,8 @@
> >  #include <linux/string.h>
> >  #include <linux/types.h>
> >  
> > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > +
> >  #include "../pci.h"
> >  
> >  /* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */
> > @@ -917,11 +919,24 @@ static int brcm_pcie_probe(struct platform_device
> > *pdev)
> >  {
> >  	struct device_node *np = pdev->dev.of_node, *msi_np;
> >  	struct pci_host_bridge *bridge;
> > +	struct device_node *fw_np;
> >  	struct brcm_pcie *pcie;
> >  	struct pci_bus *child;
> >  	struct resource *res;
> >  	int ret;
> >  
> > +	/*
> > +	 * We have to wait for the Raspberry Pi's firmware interface to be up
> > +	 * as some PCI fixups depend on it.
> 
> It'd be nice to know the nature of this dependency between the
> firmware interface and the fixups.  This may be useful for future
> maintenance.  E.g., if PCI config access doesn't work until the
> firmware interface is up, that would affect almost everything.  But
> you say "some PCI fixups", so I suppose the actual dependency is
> probably something else.

Sorry it wasn't clear enough, I'll redo this comment. Also note that the PCIe
bus and the XHCI chip are hardwired, so that's the only device that'll ever be
available on the bus.

VIA805's XHCI firmware has to be loaded trough RPi's firmware mailbox in
between the PCIe bus probe and the subsequent USB probe. Note that a PCI reset
clears the firmware. The only mechanism available in between the two operations
are PCI Fixups. These are limited in their own way, as I can't return
-EPROBE_DEFER if the firmware interface isn't available yet. Hence the need for
an explicit dependency between pcie-brcmstb and raspberrypi's firmware mailbox
device.

Your concern here showcases this series' limitations. From a high level
perspective it's not clear to me who should be responsible for downloading the
firmware. And I get the feeling I'm abusing PCI fixups. I haven't found any
smart way to deal with this three way dependency of platform/non-platform
devices. I even looked into adding -EPROBE_DEFER support to fixups, but I fear
that would entail moving them into the core device definition.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2020-04-02 14:27 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 18:28 [PATCH v6 0/4] USB: pci-quirks: Add Raspberry Pi 4 quirk Nicolas Saenz Julienne
2020-03-24 18:28 ` [PATCH v6 1/4] soc: bcm2835: Sync xHCI reset firmware property with downstream Nicolas Saenz Julienne
2020-04-02 18:01   ` Bjorn Helgaas
2020-04-04 20:11     ` Florian Fainelli
2020-03-24 18:28 ` [PATCH v6 2/4] firmware: raspberrypi: Introduce vl805 init routine Nicolas Saenz Julienne
2020-04-01 20:37   ` Bjorn Helgaas
2020-04-02 11:32     ` Nicolas Saenz Julienne
2020-04-02 19:40       ` Bjorn Helgaas
2020-04-04 18:56         ` Nicolas Saenz Julienne
2020-03-24 18:28 ` [PATCH v6 3/4] PCI: brcmstb: Wait for Raspberry Pi's firmware when present Nicolas Saenz Julienne
2020-04-01 20:41   ` Bjorn Helgaas
2020-04-02 14:27     ` Nicolas Saenz Julienne [this message]
2020-04-02 19:38       ` Bjorn Helgaas
2020-04-04 19:20         ` Nicolas Saenz Julienne
2020-04-04 20:09           ` Florian Fainelli
2020-03-24 18:28 ` [PATCH v6 4/4] USB: pci-quirks: Add Raspberry Pi 4 quirk Nicolas Saenz Julienne

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=47c543e2144d5247743548b00d1931e9fc217f43.camel@suse.de \
    --to=nsaenzjulienne@suse.de \
    --cc=amurray@thegoodpenguin.co.uk \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=f.fainelli@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=helgaas@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=sergei.shtylyov@cogentembedded.com \
    --cc=tim.gover@raspberrypi.org \
    --cc=wahrenst@gmx.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).