Message-ID: <20220215152014.GA108566@bhelgaas>
Date:   Tue, 15 Feb 2022 09:20:14 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Ben Dooks <ben.dooks@...ethink.co.uk>
Cc:     linux-kernel@...r.kernel.org, bhelgaas@...gle.comv,
        linux-pci@...r.kernel.org, paul.walmsley@...ive.com,
        greentime.hu@...ive.com, david.abdurachmanov@...il.com,
        Rob Herring <robh@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Subject: Re: [PATCH 1/2] PCI: fu740: fix finding GPIOs

On Tue, Feb 15, 2022 at 11:21:03AM +0000, Ben Dooks wrote:
> On 14/02/2022 16:05, Bjorn Helgaas wrote:
> > [+cc Rob for possible DT/kernel match issue,
> > Lorenzo (native host bridge driver maintainer)]
> > 
> > s/fix finding/Fix finding/ (in subject)
> > Or even better, say something specific about the DT properties in
> > question, e.g., look for "reset" instead of "reset-gpios".
> > 
> > On Mon, Feb 14, 2022 at 08:21:43AM +0000, Ben Dooks wrote:
> > > The calls to devm_gpiod_get_optional() have the -gpios at the end of
> > > the name. This means the pcie driver is not finding the necessary
> > > reset or power GPOOs to allow the PCIe devices on the SiFive Unmatched
> > > boards.
> > 
> > "to allow the PCIe devices ...?"  Something is missing from this
> > sentence.  "To allow the devices <to do what>"?  Or maybe the driver
> > needs these GPIOs to power up the PCIe devices?
> > 
> > I guess the implication is that the code looks for "reset-gpios" and
> > "pwren-gpios", but the DT contains "reset" and "pwren"?
> > 
> > But both Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
> > and arch/riscv/boot/dts/sifive/fu740-c000.dtsi actually do contain
> > "reset-gpios" and "pwren-gpios".
> 
> Yes, the issue is the gpio core code adds either "-gpio" or "-gpios"
> itself, thus the find code does not need this. The error messages and
> DT are correct. I'll reword the initial paragraph to note that which
> should also deal with the other comments:
> 
> The calls to devm_gpiod_get_optional() have the -gpios at the end of
> the name but the GPIO core code is already adding the suffix during
> the lookup. This means the PCIe driver is not finding the necessary
> reset or power lines to allow initialisation of the PCIe.

Ah, that makes things make a LOT more sense!  Please add something
about "the '-gpios' suffix" for non-experts like me who don't know the
inner workings of the GPIO core.

The GPIO core seems slightly too helpful here -- we have to know about
that detail to write error messages that make sense.

> > If we *do* want to change the code, please change the error messages
> > to match.
> 
> See above, it is just devm_gpiod_get_optional() parameters at fault.
> 
> > > This has not been a noted bug as the PCIe probe from u-boot has been
> > > required to get the PCIe working due to other issues with the system
> > > setup. It could have been broken since the driver inclusion, and not
> > > been noticed as it is not necessary for the driver to funciton.
> > 
> > Please add a line about what the connection between U-Boot and this
> > issue is, e.g., maybe U-Boot powers up the devices, so we wouldn't
> > notice the kernel's inability to do so?
> 
> Does a reword to the following make better sense:
> 
> This bug has not been noticed as if U-Boot has setup the GPIO lines
> for the hardware when it does the PCIe initialisation (either by
> booting from PCIe or user command to access PCIe) then the PCIe
> will work in Linux. The U-Boot as supplied by SiFive does not by
> default initialise any PCIe component.

Sounds good, thanks.

Bjorn

Powered by blists - more mailing lists