| Message ID | 20211029200319.23475-8-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: brcmstb: have host-bridge turn on sub-device power | expand |
Hi Jim,
I love your patch! Yet something to improve:
[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on robh/for-next v5.15-rc7 next-20211029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jim-Quinlan/PCI-brcmstb-have-host-bridge-turn-on-sub-device-power/20211030-040531
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/00b5df640b36d92065547cd17d317a1b9f241124
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jim-Quinlan/PCI-brcmstb-have-host-bridge-turn-on-sub-device-power/20211030-040531
git checkout 00b5df640b36d92065547cd17d317a1b9f241124
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "pci_set_of_node" [drivers/pci/controller/pcie-brcmstb.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, Oct 29, 2021 at 04:03:15PM -0400, Jim Quinlan wrote: > This Broadcom STB PCIe RC driver has one port and connects directly to one > device, be it a switch or an endpoint. We want to be able to turn on/off > any regulators for that device. Control of regulators is needed because of > the chicken-and-egg situation: although the regulator is "owned" by the > device and would be best handled by its driver, the device cannot be > discovered and probed unless its regulator is already turned on. Reviwed-by: Mark Brown <broonie@kernel.org>
On Fri, Oct 29, 2021 at 04:03:15PM -0400, Jim Quinlan wrote: > This Broadcom STB PCIe RC driver has one port and connects directly to one > device, be it a switch or an endpoint. We want to be able to turn on/off > any regulators for that device. Control of regulators is needed because of > the chicken-and-egg situation: although the regulator is "owned" by the > device and would be best handled by its driver, the device cannot be > discovered and probed unless its regulator is already turned on. I think this can be done in a much more simple way that avoids the prior patches using the pci_ops.add_bus() (and remove_bus()) hook. add_bus is called before the core scans a child bus. In the handler, you just need to get the bridge device, then the bridge DT node, and then get the regulators and enable. Given we're talking about standard properties in a standard (bridge) node, I think the implementation for .add_bus should be common (drivers/pci/of.c). It doesn't scale to be doing this in every host bridge driver. Rob
On Tue, Nov 2, 2021 at 12:00 PM Rob Herring <robh@kernel.org> wrote: > > On Fri, Oct 29, 2021 at 04:03:15PM -0400, Jim Quinlan wrote: > > This Broadcom STB PCIe RC driver has one port and connects directly to one > > device, be it a switch or an endpoint. We want to be able to turn on/off > > any regulators for that device. Control of regulators is needed because of > > the chicken-and-egg situation: although the regulator is "owned" by the > > device and would be best handled by its driver, the device cannot be > > discovered and probed unless its regulator is already turned on. > > I think this can be done in a much more simple way that avoids the > prior patches using the pci_ops.add_bus() (and remove_bus()) hook. > add_bus is called before the core scans a child bus. In the handler, you > just need to get the bridge device, then the bridge DT node, and then > get the regulators and enable. Hi Rob, In reply to my bindings commit you wanted to put the "xxx-supply" property(s) under the bridge node rather than under the pci-ep node. This not only makes sense but also removes the burden of prematurely creating the struct device *ptr as the bridge device has already been created. However, there is still an issue: if the pcie-link is not successful, we want the bus enumeration to stop and not read the vendor/dev id of the EP. Our controller has the disadvantage of causing an abort when accessing config space when the link is not established. Other controllers kindly return 0xffffffff as the data. Doing something like this gets around the issue: static struct pci_bus *pci_alloc_child_bus(...) { /* ... */ add_dev: /* ... */ if (child->ops->add_bus) { ret = child->ops->add_bus(child); + if (ret == -ENOLINK) + return NULL; if (WARN_ON(ret < 0)) dev_err(&child->dev, "failed to add bus: %d\n", ret); } Is this acceptable? Other suggestions? > > Given we're talking about standard properties in a standard (bridge) > node, I think the implementation for .add_bus should be common > (drivers/pci/of.c). It doesn't scale to be doing this in every host > bridge driver. Are you saying that the bridge DT node should have a property such as "get-and-turn-on-subdev-regulators;" which would invoke what I'm now calling brcm_pcie_add_bus()? The problem with this is that our host bridge needs to be the agent freeing the regulators. IIRC correctly, when the regulators were freed by the EP device -- or now the bridge in this case -- we got panics when doing unbinds. I will go back and get the details on this, but I'm wondering if our controller has arcane but necessary requirements outside of what a general mechanism could provide. Thanks, Jim > > Rob
On Tue, Nov 2, 2021 at 5:36 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > On Tue, Nov 2, 2021 at 12:00 PM Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Oct 29, 2021 at 04:03:15PM -0400, Jim Quinlan wrote: > > > This Broadcom STB PCIe RC driver has one port and connects directly to one > > > device, be it a switch or an endpoint. We want to be able to turn on/off > > > any regulators for that device. Control of regulators is needed because of > > > the chicken-and-egg situation: although the regulator is "owned" by the > > > device and would be best handled by its driver, the device cannot be > > > discovered and probed unless its regulator is already turned on. > > > > I think this can be done in a much more simple way that avoids the > > prior patches using the pci_ops.add_bus() (and remove_bus()) hook. > > add_bus is called before the core scans a child bus. In the handler, you > > just need to get the bridge device, then the bridge DT node, and then > > get the regulators and enable. > Hi Rob, > In reply to my bindings commit you wanted to put the "xxx-supply" > property(s) under the > bridge node rather than under the pci-ep node. This not only makes > sense but also removes > the burden of prematurely creating the struct device *ptr as the > bridge device has > already been created. > > However, there is still an issue: if the pcie-link is not > successful, we want the bus enumeration > to stop and not read the vendor/dev id of the EP. Our controller has > the disadvantage of causing > an abort when accessing config space when the link is not established. Other > controllers kindly return 0xffffffff as the data. > > Doing something like this gets around the issue: > > static struct pci_bus *pci_alloc_child_bus(...) > { > /* ... */ > add_dev: > /* ... */ > if (child->ops->add_bus) { > ret = child->ops->add_bus(child); > + if (ret == -ENOLINK) > + return NULL; > if (WARN_ON(ret < 0)) > dev_err(&child->dev, "failed to add bus: %d\n", ret); > } > > Is this acceptable? Other suggestions? Acceptable yes once we agree on error code to return. I'd just do -ENODEV. > > Given we're talking about standard properties in a standard (bridge) > > node, I think the implementation for .add_bus should be common > > (drivers/pci/of.c). It doesn't scale to be doing this in every host > > bridge driver. > Are you saying that the bridge DT node should have a property such as > "get-and-turn-on-subdev-regulators;" which would invoke what I'm now > calling brcm_pcie_add_bus()? No! Define a common function that host drivers can opt in to by setting their .add_bus() hook to or calling from their own add_bus function. Ideally, it would work on Rockchip too as it's the same supplies. However, that would require some reworking of the link initialization and PERST handling. As Bjorn has mentioned, all that should be per RP, not per host bridge anyways. I'm taking it one step further and saying it should be per PCI bridge. Hikey for example needs PERST handling on bridges behind a switch. Rob
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index ba4d6daf312c..6635e143cfcb 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -24,6 +24,7 @@ #include <linux/pci.h> #include <linux/pci-ecam.h> #include <linux/printk.h> +#include <linux/regulator/consumer.h> #include <linux/reset.h> #include <linux/sizes.h> #include <linux/slab.h> @@ -192,6 +193,12 @@ static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val); +static const char * const supplies[] = { + "vpcie3v3", + "vpcie3v3aux", + "vpcie12v", +}; + enum { RGR1_SW_INIT_1, EXT_CFG_INDEX, @@ -295,8 +302,38 @@ 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_bulk_data supplies[ARRAY_SIZE(supplies)]; + unsigned int num_supplies; }; +static int brcm_regulators_on(struct brcm_pcie *pcie) +{ + struct device *dev = pcie->dev; + int ret; + + if (!pcie->num_supplies) + return 0; + ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies); + if (ret) + dev_err(dev, "failed to enable EP regulators\n"); + + return ret; +} + +static int brcm_regulators_off(struct brcm_pcie *pcie) +{ + struct device *dev = pcie->dev; + int ret; + + if (!pcie->num_supplies) + return 0; + ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies); + if (ret) + dev_err(dev, "failed to disable EP regulators\n"); + + return ret; +} + /* * 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 @@ -1148,6 +1185,59 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) pcie->bridge_sw_init_set(pcie, 1); } +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie, struct pci_dev *dev) +{ + const unsigned int ns = ARRAY_SIZE(supplies); + struct device_node *dn; + struct property *pp; + unsigned int i; + int ret; + + /* This is for Broadcom STB/CM chips only */ + if (pcie->type == BCM2711) + return 0; + + pci_set_of_node(dev); + dn = dev->dev.of_node; + if (!dn) + return 0; + + for_each_property_of_node(dn, pp) { + for (i = 0; i < ns; i++) { + char prop_name[64]; /* 64 is max size of property name */ + + snprintf(prop_name, 64, "%s-supply", supplies[i]); + if (strcmp(prop_name, pp->name) == 0) + break; + } + if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies)) + continue; + + pcie->supplies[pcie->num_supplies++].supply = supplies[i]; + } + + if (pcie->num_supplies == 0) + return 0; + + /* + * We set the name ahead of time as the regulator core expects + * it to exist when regulator_bulk_get() is called. + */ + dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", + pci_domain_nr(dev->bus), + dev->bus->number, PCI_SLOT(dev->devfn), + PCI_FUNC(dev->devfn)); + /* + * We cannot use devm_regulator_bulk_get() because the + * downstream device may be removed w/o the regulator + * first being disabled by the host bridge. + */ + ret = regulator_bulk_get(&dev->dev, pcie->num_supplies, + pcie->supplies); + + return ret; +} + static int brcm_pcie_suspend(struct device *dev) { struct brcm_pcie *pcie = dev_get_drvdata(dev); @@ -1158,7 +1248,7 @@ static int brcm_pcie_suspend(struct device *dev) reset_control_rearm(pcie->rescal); clk_disable_unprepare(pcie->clk); - return ret; + return brcm_regulators_off(pcie); } static int brcm_pcie_resume(struct device *dev) @@ -1174,6 +1264,9 @@ static int brcm_pcie_resume(struct device *dev) ret = reset_control_reset(pcie->rescal); if (ret) goto err_disable_clk; + ret = brcm_regulators_on(pcie); + if (ret) + goto err_reset; ret = brcm_phy_start(pcie); if (ret) @@ -1217,6 +1310,10 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) brcm_phy_stop(pcie); reset_control_rearm(pcie->rescal); clk_disable_unprepare(pcie->clk); + if (pcie->num_supplies) { + brcm_regulators_off(pcie); + regulator_bulk_free(pcie->num_supplies, pcie->supplies); + } } static int brcm_pcie_remove(struct platform_device *pdev) @@ -1241,6 +1338,57 @@ static const struct of_device_id brcm_pcie_match[] = { {}, }; +static bool brcm_pcie_pci_subdev_can_prepare(struct pci_bus *bus, int devfn) +{ + /* + * We only care about a device that is directly connected + * to the root complex, ie bus == 1 and slot == 0. + */ + return (bus->number == 1 && PCI_SLOT(devfn) == 0); +} + +static int brcm_pcie_pci_subdev_prepare(struct pci_bus *bus, int devfn, + struct pci_host_bridge *bridge, + struct pci_dev *pdev) +{ + struct brcm_pcie *pcie; + int ret = 0; + + pcie = (struct brcm_pcie *) bridge->sysdata; + ret = brcm_pcie_get_regulators(pcie, pdev); + if (ret) { + pcie->num_supplies = 0; + if (ret != -EPROBE_DEFER) + dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret); + return ret; + } + + ret = brcm_regulators_on(pcie); + if (ret) + goto err_out0; + + ret = brcm_pcie_linkup(pcie); + if (ret) + goto err_out1; + + /* + * dev_set_name() was called in brcm_get_regulators(). Free the + * string it allocated as it will be called again when + * pci_setup_device() is invoked. + */ + kfree_const(pdev->dev.kobj.name); + pdev->dev.kobj.name = NULL; + + return 0; + +err_out1: + brcm_regulators_off(pcie); +err_out0: + regulator_bulk_free(pcie->num_supplies, pcie->supplies); + pcie->num_supplies = 0; + return ret; +} + static int brcm_pcie_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node, *msi_np; @@ -1327,12 +1475,24 @@ static int brcm_pcie_probe(struct platform_device *pdev) } } + bridge->pci_subdev_can_prepare = brcm_pcie_pci_subdev_can_prepare; + bridge->pci_subdev_prepare = brcm_pcie_pci_subdev_prepare; bridge->ops = &brcm_pcie_ops; bridge->sysdata = pcie; platform_set_drvdata(pdev, pcie); - return pci_host_probe(bridge); + ret = pci_host_probe(bridge); + if (!ret && !brcm_pcie_link_up(pcie)) + ret = -ENODEV; + + if (ret) { + brcm_pcie_remove(pdev); + return ret; + } + + return 0; + fail: __brcm_pcie_remove(pcie); return ret;
This Broadcom STB PCIe RC driver has one port and connects directly to one device, be it a switch or an endpoint. We want to be able to turn on/off any regulators for that device. Control of regulators is needed because of the chicken-and-egg situation: although the regulator is "owned" by the device and would be best handled by its driver, the device cannot be discovered and probed unless its regulator is already turned on. Signed-off-by: Jim Quinlan <jim2101024@gmail.com> --- drivers/pci/controller/pcie-brcmstb.c | 164 +++++++++++++++++++++++++- 1 file changed, 162 insertions(+), 2 deletions(-)