Re: [PATCH 1/2] PCI: fu740: fix finding GPIOs

[Rob Herring <robh@kernel.org>]

On Mon, Feb 14, 2022 at 10:05 AM Bjorn Helgaas <helgaas@kernel.org> 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".

This is purely a kernel issue. The gpio API appends '-gpios' (and a
fallback to '-gpio') to the name passed. So passing 'reset-gpios'
looks for 'reset-gpios-gpios' property. This happens enough, there
should probably be a check for this mistake.

>
> 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.
>
> s/pcie/PCIe/
> s/GPOOs/GPIOs/
> "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".
>
> If we *do* want to change the code, please change the error messages
> to match.
>
> > 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.
>
> s/u-boot/U-Boot/
> s/funciton/function/
>
> 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?
>
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> >  drivers/pci/controller/dwc/pcie-fu740.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> > index 00cde9a248b5..842b7202b96e 100644
> > --- a/drivers/pci/controller/dwc/pcie-fu740.c
> > +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> > @@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev)
> >               return PTR_ERR(afp->mgmt_base);
> >
> >       /* Fetch GPIOs */
> > -     afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW);
> > +     afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> >       if (IS_ERR(afp->reset))
> >               return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n");
> >
> > -     afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW);
> > +     afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW);
> >       if (IS_ERR(afp->pwren))
> >               return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n");
> >
> > --
> > 2.34.1
> >