回复: [PATCH v4 1/6] PCI: starfive: Use regulator APIs instead of GPIO APIs to enable the 3V3 power supply of PCIe slots

Kevin Xie
2025-12-02T01:46:13Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 5073 bytes --]

> Krzysztof Kozlowski <krzk+dt@kernel.org>; Palmer Dabbelt
> <palmer@dabbelt.com>; Paul Walmsley <pjw@kernel.org>; Albert Ou
> <aou@eecs.berkeley.edu>; Rafael J . Wysocki <rafael@kernel.org>; Viresh
> Kumar <viresh.kumar@linaro.org>; Lorenzo Pieralisi <lpieralisi@kernel.org>;
> Krzysztof Wilczy¨½ski <kwilczynski@kernel.org>; Manivannan Sadhasivam
> <mani@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Emil Renner
> Berthing <emil.renner.berthing@canonical.com>; Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com>; E Shattow <e@freeshell.de>;
> devicetree@vger.kernel.org; linux-pci@vger.kernel.org;
> linux-riscv@lists.infradead.org; linux-kernel@vger.kernel.org; Kevin Xie
> <kevin.xie@starfivetech.com>
> Ö÷Ìâ: Re: [PATCH v4 1/6] PCI: starfive: Use regulator APIs instead of GPIO APIs
> to enable the 3V3 power supply of PCIe slots
>
> [+cc Kevin, pcie-starfive.c maintainer; will need his ack]
>
> Subject line is excessively long.
>
> On Tue, Nov 25, 2025 at 03:55:59PM +0800, Hal Feng wrote:
> > The "enable-gpio" property is not documented in the dt-bindings and
> > using GPIO APIs is not a standard method to enable or disable PCIe
> > slot power, so use regulator APIs to replace them.
>
> I can't tell from this whether existing DTs will continue to work after this
> change. It looks like previously we looked for an "enable-gpios" or
> "enable-gpio" property and now we'll look for a "vpcie3v3-supply" regulator
> property.
>
> I don't see "enable-gpios" or "enable-gpio" mentioned in any of the DT patches
> in this series, so maybe that property was never actually used before, and the
> code for pcie->power_gpio was actually dead?
>

pcie->power_gpio is used in the our JH7110 EVB, it share the same pcie controller
driver with VisionFive2 board. Although JH7110 was not upstreamed, we still hope
to maintain the compatibility of the driver.

> Please add something here about how we know this won't break any existing
> setups using DTs that are already in the field.
>
> > Tested-by: Matthias Brugger <mbrugger@suse.com>
> > Fixes: 39b91eb40c6a ("PCI: starfive: Add JH7110 PCIe controller")
>
> Based on the cover letter, it looks like the point of this is to add support for a
> new device, which I don't think really qualifies as a "fix".
>
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> > drivers/pci/controller/plda/pcie-starfive.c | 25
> > ++++++++++++---------
> > 1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/controller/plda/pcie-starfive.c
> > b/drivers/pci/controller/plda/pcie-starfive.c
> > index 3caf53c6c082..298036c3e7f9 100644
> > --- a/drivers/pci/controller/plda/pcie-starfive.c
> > +++ b/drivers/pci/controller/plda/pcie-starfive.c
> > @@ -55,7 +55,7 @@ struct starfive_jh7110_pcie {
> > struct reset_control *resets;
> > struct clk_bulk_data *clks;
> > struct regmap *reg_syscon;
> > - struct gpio_desc *power_gpio;
> > + struct regulator *vpcie3v3;
> > struct gpio_desc *reset_gpio;
> > struct phy *phy;
> >
> > @@ -153,11 +153,13 @@ static int starfive_pcie_parse_dt(struct
> starfive_jh7110_pcie *pcie,
> > return dev_err_probe(dev, PTR_ERR(pcie->reset_gpio),
> > "failed to get perst-gpio\n");
> >
> > - pcie->power_gpio = devm_gpiod_get_optional(dev, "enable",
> > - GPIOD_OUT_LOW);
> > - if (IS_ERR(pcie->power_gpio))
> > - return dev_err_probe(dev, PTR_ERR(pcie->power_gpio),
> > - "failed to get power-gpio\n");
> > + pcie->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> > + if (IS_ERR(pcie->vpcie3v3)) {
> > + if (PTR_ERR(pcie->vpcie3v3) != -ENODEV)
> > + return dev_err_probe(dev, PTR_ERR(pcie->vpcie3v3),
> > + "failed to get vpcie3v3 regulator\n");
> > + pcie->vpcie3v3 = NULL;
> > + }
> >
> > return 0;
> > }
> > @@ -270,8 +272,8 @@ static void starfive_pcie_host_deinit(struct
> plda_pcie_rp *plda)
> > container_of(plda, struct starfive_jh7110_pcie, plda);
> >
> > starfive_pcie_clk_rst_deinit(pcie);
> > - if (pcie->power_gpio)
> > - gpiod_set_value_cansleep(pcie->power_gpio, 0);
> > + if (pcie->vpcie3v3)
> > + regulator_disable(pcie->vpcie3v3);
> > starfive_pcie_disable_phy(pcie);
> > }
> >
> > @@ -304,8 +306,11 @@ static int starfive_pcie_host_init(struct
> plda_pcie_rp *plda)
> > if (ret)
> > return ret;
> >
> > - if (pcie->power_gpio)
> > - gpiod_set_value_cansleep(pcie->power_gpio, 1);
> > + if (pcie->vpcie3v3) {
> > + ret = regulator_enable(pcie->vpcie3v3);
> > + if (ret)
> > + dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
> > + }
> >
> > if (pcie->reset_gpio)
> > gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> > --
> > 2.43.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]