| Message ID | 20211019095858.21316-1-Meng.Li@windriver.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pci: pcie-rcar: add regulators support | expand |
Hi Meng, On Tue, Oct 19, 2021 at 11:59 AM Meng Li <Meng.Li@windriver.com> wrote: > From: Andrey Gusakov <andrey.gusakov@cogentembedded.com> > > Add PCIe regulators for KingFisher board. > > Signed-off-by: Meng Li <Meng.Li@windriver.com> Thanks for your patch! > arch/arm64/boot/dts/renesas/ulcb-kf.dtsi | 47 +++++++++++++++++ > drivers/pci/controller/pcie-rcar-host.c | 64 ++++++++++++++++++++++++ Please split patches touching both DT and driver sources. > --- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi > +++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi > @@ -259,6 +303,9 @@ > > &pciec1 { > status = "okay"; > + > + pcie3v3-supply = <&mpcie_3v3>; > + pcie1v8-supply = <&mpcie_1v8>; This needs an update to the R-Car PCIe DT bindings first. > --- a/drivers/pci/controller/pcie-rcar-host.c > +++ b/drivers/pci/controller/pcie-rcar-host.c > @@ -893,6 +896,36 @@ static const struct of_device_id rcar_pcie_of_match[] = { > {}, > }; > > +static int rcar_pcie_set_vpcie(struct rcar_pcie_host *host) > +{ > + struct device *dev = host->pcie.dev; > + int err; > + > + if (!IS_ERR(host->pcie3v3)) { > + err = regulator_enable(host->pcie3v3); This will crash if host->pcie3v3 is NULL (optional regulator not present). Probably you just want to check for non-NULL (see below). > + if (err) { > + dev_err(dev, "fail to enable vpcie3v3 regulator\n"); > + goto err_out; > + } > + } > + > + if (!IS_ERR(host->pcie1v8)) { > + err = regulator_enable(host->pcie1v8); Likewise. > + if (err) { > + dev_err(dev, "fail to enable vpcie1v8 regulator\n"); > + goto err_disable_3v3; > + } > + } > + > + return 0; > + > +err_disable_3v3: > + if (!IS_ERR(host->pcie3v3)) Likewise. > + regulator_disable(host->pcie3v3); > +err_out: > + return err; > +} > + > static int rcar_pcie_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -911,6 +944,31 @@ static int rcar_pcie_probe(struct platform_device *pdev) > pcie->dev = dev; > platform_set_drvdata(pdev, host); > > + host->pcie3v3 = devm_regulator_get_optional(dev, "pcie3v3"); > + if (IS_ERR(host->pcie3v3)) { > + if (PTR_ERR(host->pcie3v3) == -EPROBE_DEFER) { > + pci_free_host_bridge(bridge); Please drop this. As the bridge was allocated using devm_pci_alloc_host_bridge(), freeing it manually will cause a double free. > + return -EPROBE_DEFER; > + } > + dev_info(dev, "no pcie3v3 regulator found\n"); devm_regulator_get_optional() returns NULL if the regulator was not found. Hence if IS_ERR() is true, this indicates a real error, which you should handle: if (IS_ERR(host->pcie3v3)) return PTR_ERR(host->pcie3v3); > + } > + > + host->pcie1v8 = devm_regulator_get_optional(dev, "pcie1v8"); > + if (IS_ERR(host->pcie1v8)) { > + if (PTR_ERR(host->pcie1v8) == -EPROBE_DEFER) { > + pci_free_host_bridge(bridge); > + return -EPROBE_DEFER; > + } > + dev_info(dev, "no pcie1v8 regulator found\n"); Likewise. > + } > + > + err = rcar_pcie_set_vpcie(host); > + if (err) { > + dev_err(dev, "failed to set pcie regulators\n"); > + pci_free_host_bridge(bridge); Please drop this to avoid double free. > + return err; > + } > + > pm_runtime_enable(pcie->dev); > err = pm_runtime_get_sync(pcie->dev); > if (err < 0) { > @@ -985,6 +1043,12 @@ static int rcar_pcie_probe(struct platform_device *pdev) > irq_dispose_mapping(host->msi.irq1); > > err_pm_put: > + if(!IS_ERR(host->pcie3v3)) if (host->pcie3v3) > + if (regulator_is_enabled(host->pcie3v3)) If you get here, the regulator should be enabled? > + regulator_disable(host->pcie3v3); > + if(!IS_ERR(host->pcie1v8)) if (host->pcie1v8) > + if (regulator_is_enabled(host->pcie1v8)) > + regulator_disable(host->pcie1v8); Please move this below the call to pm_runtime_disable(), to preserve symmetry. > pm_runtime_put(dev); > pm_runtime_disable(dev); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Tuesday, October 19, 2021 6:20 PM > To: Li, Meng <Meng.Li@windriver.com> > Cc: Magnus Damm <magnus.damm@gmail.com>; Rob Herring > <robh+dt@kernel.org>; Marek Vasut <marek.vasut+renesas@gmail.com>; > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>; Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com>; Krzysztof Wilczyński <kw@linux.com>; Bjorn > Helgaas <bhelgaas@google.com>; Liam Girdwood <lgirdwood@gmail.com>; > Mark Brown <broonie@kernel.org>; Linux-Renesas <linux-renesas- > soc@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED DEVICE > TREE BINDINGS <devicetree@vger.kernel.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; linux-pci <linux-pci@vger.kernel.org> > Subject: Re: [PATCH] pci: pcie-rcar: add regulators support > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > Hi Meng, > > On Tue, Oct 19, 2021 at 11:59 AM Meng Li <Meng.Li@windriver.com> wrote: > > From: Andrey Gusakov <andrey.gusakov@cogentembedded.com> > > > > Add PCIe regulators for KingFisher board. > > > > Signed-off-by: Meng Li <Meng.Li@windriver.com> > > Thanks for your patch! > > > arch/arm64/boot/dts/renesas/ulcb-kf.dtsi | 47 +++++++++++++++++ > > drivers/pci/controller/pcie-rcar-host.c | 64 ++++++++++++++++++++++++ > > Please split patches touching both DT and driver sources. > > > --- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi > > +++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi > > > @@ -259,6 +303,9 @@ > > > > &pciec1 { > > status = "okay"; > > + > > + pcie3v3-supply = <&mpcie_3v3>; > > + pcie1v8-supply = <&mpcie_1v8>; > > This needs an update to the R-Car PCIe DT bindings first. > > > --- a/drivers/pci/controller/pcie-rcar-host.c > > +++ b/drivers/pci/controller/pcie-rcar-host.c > > > @@ -893,6 +896,36 @@ static const struct of_device_id > rcar_pcie_of_match[] = { > > {}, > > }; > > > > +static int rcar_pcie_set_vpcie(struct rcar_pcie_host *host) { > > + struct device *dev = host->pcie.dev; > > + int err; > > + > > + if (!IS_ERR(host->pcie3v3)) { > > + err = regulator_enable(host->pcie3v3); > > This will crash if host->pcie3v3 is NULL (optional regulator not present). > Probably you just want to check for non-NULL (see below). > > > + if (err) { > > + dev_err(dev, "fail to enable vpcie3v3 regulator\n"); > > + goto err_out; > > + } > > + } > > + > > + if (!IS_ERR(host->pcie1v8)) { > > + err = regulator_enable(host->pcie1v8); > > Likewise. > > > + if (err) { > > + dev_err(dev, "fail to enable vpcie1v8 regulator\n"); > > + goto err_disable_3v3; > > + } > > + } > > + > > + return 0; > > + > > +err_disable_3v3: > > + if (!IS_ERR(host->pcie3v3)) > > Likewise. > > > + regulator_disable(host->pcie3v3); > > +err_out: > > + return err; > > +} > > + > > static int rcar_pcie_probe(struct platform_device *pdev) { > > struct device *dev = &pdev->dev; @@ -911,6 +944,31 @@ static > > int rcar_pcie_probe(struct platform_device *pdev) > > pcie->dev = dev; > > platform_set_drvdata(pdev, host); > > > > + host->pcie3v3 = devm_regulator_get_optional(dev, "pcie3v3"); > > + if (IS_ERR(host->pcie3v3)) { > > + if (PTR_ERR(host->pcie3v3) == -EPROBE_DEFER) { > > + pci_free_host_bridge(bridge); > > Please drop this. As the bridge was allocated using > devm_pci_alloc_host_bridge(), freeing it manually will cause a double free. > > > + return -EPROBE_DEFER; > > + } > > + dev_info(dev, "no pcie3v3 regulator found\n"); > > devm_regulator_get_optional() returns NULL if the regulator was not found. > Hence if IS_ERR() is true, this indicates a real error, which you should handle: > > if (IS_ERR(host->pcie3v3)) > return PTR_ERR(host->pcie3v3); > > > + } > > + > > + host->pcie1v8 = devm_regulator_get_optional(dev, "pcie1v8"); > > + if (IS_ERR(host->pcie1v8)) { > > + if (PTR_ERR(host->pcie1v8) == -EPROBE_DEFER) { > > + pci_free_host_bridge(bridge); > > + return -EPROBE_DEFER; > > + } > > + dev_info(dev, "no pcie1v8 regulator found\n"); > > Likewise. > > > + } > > + > > + err = rcar_pcie_set_vpcie(host); > > + if (err) { > > + dev_err(dev, "failed to set pcie regulators\n"); > > + pci_free_host_bridge(bridge); > > Please drop this to avoid double free. > > > + return err; > > + } > > + > > pm_runtime_enable(pcie->dev); > > err = pm_runtime_get_sync(pcie->dev); > > if (err < 0) { > > @@ -985,6 +1043,12 @@ static int rcar_pcie_probe(struct platform_device > *pdev) > > irq_dispose_mapping(host->msi.irq1); > > > > err_pm_put: > > + if(!IS_ERR(host->pcie3v3)) > > if (host->pcie3v3) > > > + if (regulator_is_enabled(host->pcie3v3)) > > If you get here, the regulator should be enabled? > > > + regulator_disable(host->pcie3v3); > > + if(!IS_ERR(host->pcie1v8)) > > if (host->pcie1v8) > > > + if (regulator_is_enabled(host->pcie1v8)) > > + regulator_disable(host->pcie1v8); > > Please move this below the call to pm_runtime_disable(), to preserve > symmetry. > > > pm_runtime_put(dev); > > pm_runtime_disable(dev); > > Gr{oetje,eeting}s, > Thanks for your professional suggest. I will improve patches and send v2 in later Thanks, Limeng > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Tue, Oct 19, 2021 at 05:58:58PM +0800, Meng Li wrote: > From: Andrey Gusakov <andrey.gusakov@cogentembedded.com> > > Add PCIe regulators for KingFisher board. > > Signed-off-by: Meng Li <Meng.Li@windriver.com> > --- You've not provided a Signed-off-by for Andrey, please see Documentation/process/submitting-patches.rst for details on what this is and why it's important. > + host->pcie3v3 = devm_regulator_get_optional(dev, "pcie3v3"); > + host->pcie1v8 = devm_regulator_get_optional(dev, "pcie1v8"); Unless PCIe works without these supplies (which are in my understanding mandatory according to the spec) these should not be optional, this API is for supplies that may be physically absent.
On Tue, Oct 19, 2021 at 05:58:58PM +0800, Meng Li wrote: > From: Andrey Gusakov <andrey.gusakov@cogentembedded.com> > > Add PCIe regulators for KingFisher board. Please pay attention to the existing code and history. Your current subject line is: pci: pcie-rcar: add regulators support which looks nothing like the history: $ git log --oneline drivers/pci/controller/pcie-rcar-host.c 861e133ba268 ("PCI: rcar-host: Remove unneeded includes") a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook") d21faba11693 ("PCI: Bulk conversion to generic_handle_domain_irq()") 83ed8d4fa656 ("PCI: rcar: Convert to MSI domains") 93cd1bb4862d ("PCI: rcar: Don't allocate extra memory for the MSI capture address") c4e0fec2f7ee ("PCI: rcar: Always allocate MSI addresses in 32bit space") 6e8e137abeab ("PCI: rcar: Drop unused members from struct rcar_pcie_host") b64aa11eb2dd ("PCI: Set bridge map_irq and swizzle_irq to default functions") 669cbc708122 ("PCI: Move DT resource setup into devm_pci_alloc_host_bridge()") b411b2e1adb9 ("PCI: rcar: Use struct pci_host_bridge.windows list directly") 61f11f8250e2 ("PCI: rcar: Use devm_pci_alloc_host_bridge()") 4f5c883d7815 ("PCI: Move setting pci_host_bridge.busnr out of host drivers") 6176a5f32751 ("PCI: rcar: Use pci_is_root_bus() to check if bus is root bus") 6a589900d050 ("PCI: Set default bridge parent device") a68e06e729b1 ("PCI: rcar: Fix runtime PM imbalance on error") 56d292348470 ("PCI: rcar: Use pci_host_probe() to register host") 78a0d7f2f5a3 ("PCI: rcar: Move shareable code to a common file") a18f4b6ea50b ("PCI: rcar: Rename pcie-rcar.c to pcie-rcar-host.c") You could use something like: PCI: rcar-host: Add regulator support for KingFisher > + host->pcie3v3 = devm_regulator_get_optional(dev, "pcie3v3"); > + if (IS_ERR(host->pcie3v3)) { +1 to Geert's comments. Sprinkling IS_ERR() everywhere is kind of ugly. host->pcie3v3 should be NULL if not present. Bjorn
diff --git a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi index 8986a7e6e099..82e463c32a37 100644 --- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi +++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi @@ -50,6 +50,25 @@ startup-delay-us = <70000>; enable-active-high; }; + + mpcie_3v3: regulator-mpcie_3v3 { + compatible = "regulator-fixed"; + regulator-name = "mPCIe 3v3"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + gpio = <&gpio_exp_77 14 GPIO_ACTIVE_HIGH>; + enable-active-high; + }; + + mpcie_1v8: regulator-mpcie_1v8 { + compatible = "regulator-fixed"; + regulator-name = "mPCIe 1v8"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + gpio = <&gpio_exp_77 15 GPIO_ACTIVE_HIGH>; + startup-delay-us = <200000>; + enable-active-high; + }; }; &can0 { @@ -241,6 +260,31 @@ interrupt-controller; interrupt-parent = <&gpio5>; interrupts = <9 IRQ_TYPE_EDGE_FALLING>; + + mpcie_wake { + gpio-hog; + gpios = <0 GPIO_ACTIVE_HIGH>; + output-low; + line-name = "mPCIe WAKE#"; + }; + mpcie_wdisable { + gpio-hog; + gpios = <1 GPIO_ACTIVE_HIGH>; + output-high; + line-name = "mPCIe W_DISABLE"; + }; + mpcie_clreq { + gpio-hog; + gpios = <2 GPIO_ACTIVE_HIGH>; + input; + line-name = "mPCIe CLKREQ#"; + }; + mpcie_ovc { + gpio-hog; + gpios = <3 GPIO_ACTIVE_HIGH>; + input; + line-name = "mPCIe OVC"; + }; }; }; @@ -259,6 +303,9 @@ &pciec1 { status = "okay"; + + pcie3v3-supply = <&mpcie_3v3>; + pcie1v8-supply = <&mpcie_1v8>; }; &pfc { diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c index cdc0963f154e..bf1e4007876e 100644 --- a/drivers/pci/controller/pcie-rcar-host.c +++ b/drivers/pci/controller/pcie-rcar-host.c @@ -14,6 +14,7 @@ #include <linux/clk.h> #include <linux/clk-provider.h> #include <linux/delay.h> +#include <linux/regulator/consumer.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/irqdomain.h> @@ -54,6 +55,8 @@ struct rcar_pcie_host { struct rcar_pcie pcie; struct phy *phy; struct clk *bus_clk; + struct regulator *pcie3v3; /* 3.3V power supply */ + struct regulator *pcie1v8; /* 1.8V power supply */ struct rcar_msi msi; int (*phy_init_fn)(struct rcar_pcie_host *host); }; @@ -893,6 +896,36 @@ static const struct of_device_id rcar_pcie_of_match[] = { {}, }; +static int rcar_pcie_set_vpcie(struct rcar_pcie_host *host) +{ + struct device *dev = host->pcie.dev; + int err; + + if (!IS_ERR(host->pcie3v3)) { + err = regulator_enable(host->pcie3v3); + if (err) { + dev_err(dev, "fail to enable vpcie3v3 regulator\n"); + goto err_out; + } + } + + if (!IS_ERR(host->pcie1v8)) { + err = regulator_enable(host->pcie1v8); + if (err) { + dev_err(dev, "fail to enable vpcie1v8 regulator\n"); + goto err_disable_3v3; + } + } + + return 0; + +err_disable_3v3: + if (!IS_ERR(host->pcie3v3)) + regulator_disable(host->pcie3v3); +err_out: + return err; +} + static int rcar_pcie_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -911,6 +944,31 @@ static int rcar_pcie_probe(struct platform_device *pdev) pcie->dev = dev; platform_set_drvdata(pdev, host); + host->pcie3v3 = devm_regulator_get_optional(dev, "pcie3v3"); + if (IS_ERR(host->pcie3v3)) { + if (PTR_ERR(host->pcie3v3) == -EPROBE_DEFER) { + pci_free_host_bridge(bridge); + return -EPROBE_DEFER; + } + dev_info(dev, "no pcie3v3 regulator found\n"); + } + + host->pcie1v8 = devm_regulator_get_optional(dev, "pcie1v8"); + if (IS_ERR(host->pcie1v8)) { + if (PTR_ERR(host->pcie1v8) == -EPROBE_DEFER) { + pci_free_host_bridge(bridge); + return -EPROBE_DEFER; + } + dev_info(dev, "no pcie1v8 regulator found\n"); + } + + err = rcar_pcie_set_vpcie(host); + if (err) { + dev_err(dev, "failed to set pcie regulators\n"); + pci_free_host_bridge(bridge); + return err; + } + pm_runtime_enable(pcie->dev); err = pm_runtime_get_sync(pcie->dev); if (err < 0) { @@ -985,6 +1043,12 @@ static int rcar_pcie_probe(struct platform_device *pdev) irq_dispose_mapping(host->msi.irq1); err_pm_put: + if(!IS_ERR(host->pcie3v3)) + if (regulator_is_enabled(host->pcie3v3)) + regulator_disable(host->pcie3v3); + if(!IS_ERR(host->pcie1v8)) + if (regulator_is_enabled(host->pcie1v8)) + regulator_disable(host->pcie1v8); pm_runtime_put(dev); pm_runtime_disable(dev);