Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 14 Feb 2022 10:05:56 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Ben Dooks <ben.dooks@codethink.co.uk>
Cc:     linux-kernel@vger.kernel.org, bhelgaas@google.comv,
        linux-pci@vger.kernel.org, paul.walmsley@sifive.com,
        greentime.hu@sifive.com, david.abdurachmanov@gmail.com,
        Rob Herring <robh@kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Subject: Re: [PATCH 1/2] PCI: fu740: fix finding GPIOs
Message-ID: <20220214160556.GA9253@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220214082144.1176084-2-ben.dooks@codethink.co.uk>
Precedence: bulk

[+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.

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
>