diff mbox series

[v1,2/6] PCI: brcmstb: Add control of EP voltage regulator(s)

Message ID 20201125192424.14440-3-james.quinlan@broadcom.com (mailing list archive)
State Superseded
Headers show
Series PCI: brcmstb: add EP regulators and panic handler | expand

Commit Message

Jim Quinlan Nov. 25, 2020, 7:24 p.m. UTC
Control of EP regulators by the RC is needed because of the chicken-and-egg
situation: although the regulator is "owned" by the EP and would be best
handled on its driver, the EP cannot be discovered and probed unless its
regulator is already turned on.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 66 +++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

Comments

Mark Brown Nov. 26, 2020, 11:49 a.m. UTC | #1
On Wed, Nov 25, 2020 at 02:24:19PM -0500, Jim Quinlan wrote:

> +	for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
> +		ep_reg = devm_regulator_get_optional(dev, ep_regulator_names[i]);
> +		if (IS_ERR(ep_reg)) {

Does PCI allow supplies to be physically absent?  If not then the driver
shouldn't be using regulator_get_optional() and much of the code here
can be deleted.

> +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> +{

This is open coding the regulator bulk APIs.
Jim Quinlan Nov. 27, 2020, 8:26 p.m. UTC | #2
On Thu, Nov 26, 2020 at 6:49 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Nov 25, 2020 at 02:24:19PM -0500, Jim Quinlan wrote:
>
> > +     for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
> > +             ep_reg = devm_regulator_get_optional(dev, ep_regulator_names[i]);
> > +             if (IS_ERR(ep_reg)) {
>
> Does PCI allow supplies to be physically absent?  If not then the driver
> shouldn't be using regulator_get_optional() and much of the code here
> can be deleted.
Hi Mark,

First, as an aside, I'm  a little confused about the purpose of
devm_regulator_get_optional(...);  the other  xxx_get_optional() calls
I am familiar with (eg clock, reset, gpio) return NULL if the desired
item does not exist, and then NULL can be used as a valid pointer for
the rest of the API.  Not so here.

At any rate, our SOCs are placed in a variety of boards which
implement the PCIe RC-EP connection as they wish.  From the PCIe
driver's point of view, the type of power supply that needs to be
turned on is specified in the DT and they cannot be hard coded by the
driver.  I've listed all of the four possibilities; typically one,
maybe two will  be specified, but never all of them.  In addition,
sometimes a regulator is hard wired on and not even controllable by
the PCIe controller.

>
> > +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > +{
>
> This is open coding the regulator bulk APIs.
Except that a bulk regulator "get"  requires that all supplies are
present.  I would have to first scan the node's properties for the
"-supply" properties and fill in the bulk regulator structure.  I'm
fine with doing that.

However, a previous incarnation of this  commit was reviewed by RobH,
and if I understood him correctly he wanted the actual names of the
possible regulators to be used and specified in the bindings doc.   I
just followed the example of "pcie-rockchip-host.c" whose bindings doc
was reviewed by RobH.

Regards,
Jim Quinlan
Broadcom STB
Mark Brown Nov. 30, 2020, 12:06 p.m. UTC | #3
On Fri, Nov 27, 2020 at 03:26:53PM -0500, Jim Quinlan wrote:
> On Thu, Nov 26, 2020 at 6:49 AM Mark Brown <broonie@kernel.org> wrote:

> > Does PCI allow supplies to be physically absent?  If not then the driver
> > shouldn't be using regulator_get_optional() and much of the code here
> > can be deleted.

> First, as an aside, I'm  a little confused about the purpose of
> devm_regulator_get_optional(...);  the other  xxx_get_optional() calls
> I am familiar with (eg clock, reset, gpio) return NULL if the desired
> item does not exist, and then NULL can be used as a valid pointer for
> the rest of the API.  Not so here.

The other APIs that cloned the regulator API don't have the dummy
support that the regulator has and unfortunately changed the sense a bit
there.

> > > +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > > +{

> > This is open coding the regulator bulk APIs.

> Except that a bulk regulator "get"  requires that all supplies are
> present.  I would have to first scan the node's properties for the
> "-supply" properties and fill in the bulk regulator structure.  I'm
> fine with doing that.

No, you should never do that.  If the supplies can be physically absent
then you should use regulator_get_optional() which allows you to do
whatever needs doing to configure the hardware for the missing supply.
If it's just that the supply may not be described in the DT but has to
be there for the device to operate then the code should use the normal
regualtor APIs - a dummy regulator will be provided if there's no supply
described.

> However, a previous incarnation of this  commit was reviewed by RobH,
> and if I understood him correctly he wanted the actual names of the
> possible regulators to be used and specified in the bindings doc.   I
> just followed the example of "pcie-rockchip-host.c" whose bindings doc
> was reviewed by RobH.

That is just plain bad code, the binding may well be fine but I can't
see any excuse for that driver to be using _optional() there.

Another subsystem I'm going to have to keep an eye on :(
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bea86899bd5d..34d6bad07b66 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -23,6 +23,7 @@ 
 #include <linux/of_platform.h>
 #include <linux/pci.h>
 #include <linux/printk.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -210,6 +211,18 @@  enum pcie_type {
 	BCM2711,
 };
 
+enum pcie_regulators {
+	VPCIE12V,
+	VPCIE3V3,
+	VPCIE1V8,
+	VPCIE0V9,
+	PCIE_REGULATORS_MAX,
+};
+
+static const char *ep_regulator_names[PCIE_REGULATORS_MAX] = {
+	"vpcie12v", "vpcie3v3", "vpcie1v8", "vpcie0v9",
+};
+
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_type type;
@@ -287,8 +300,53 @@  struct brcm_pcie {
 	u32			hw_rev;
 	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	struct regulator	*regulators[PCIE_REGULATORS_MAX];
+	int			num_regulators;
 };
 
+static int brcm_parse_regulators(struct brcm_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct regulator *ep_reg;
+	int i;
+
+	for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
+		ep_reg = devm_regulator_get_optional(dev, ep_regulator_names[i]);
+		if (IS_ERR(ep_reg)) {
+			if (PTR_ERR(ep_reg) == -ENODEV)
+				continue;
+			dev_err(dev, "failed to get regulator %s\n", ep_regulator_names[i]);
+			return PTR_ERR(ep_reg);
+		}
+		pcie->regulators[i] = ep_reg;
+		pcie->num_regulators++;
+	}
+	return 0;
+}
+
+static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+	struct device *dev = pcie->dev;
+	int ret, i;
+
+	if (pcie->num_regulators == 0)
+		return;
+
+	for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
+		if (!pcie->regulators[i])
+			continue;
+		if (on) {
+			ret = regulator_enable(pcie->regulators[i]);
+			dev_dbg(dev, "enable regulator %s (%s)\n",
+				ep_regulator_names[i], ret ? "fail" : "pass");
+		} else {
+			ret = regulator_disable(pcie->regulators[i]);
+			dev_dbg(dev, "disable regulator %s (%s)\n",
+				ep_regulator_names[i], ret ? "fail" : "pass");
+		}
+	}
+}
+
 /*
  * This is to convert the size of the inbound "BAR" region to the
  * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1139,6 +1197,7 @@  static int brcm_pcie_suspend(struct device *dev)
 	brcm_pcie_turn_off(pcie);
 	ret = brcm_phy_stop(pcie);
 	clk_disable_unprepare(pcie->clk);
+	brcm_set_regulators(pcie, false);
 
 	return ret;
 }
@@ -1151,6 +1210,7 @@  static int brcm_pcie_resume(struct device *dev)
 	int ret;
 
 	base = pcie->base;
+	brcm_set_regulators(pcie, true);
 	clk_prepare_enable(pcie->clk);
 
 	ret = brcm_phy_start(pcie);
@@ -1189,6 +1249,7 @@  static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_phy_stop(pcie);
 	reset_control_assert(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
+	brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1238,6 +1299,10 @@  static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 
+	ret = brcm_parse_regulators(pcie);
+	if (ret)
+		return ret;
+
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
 		return PTR_ERR(pcie->base);
@@ -1273,6 +1338,7 @@  static int brcm_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	brcm_set_regulators(pcie, true);
 	ret = brcm_pcie_setup(pcie);
 	if (ret)
 		goto fail;