| Message ID | 20211103184939.45263-6-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 |
On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote: > + 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]; > + } Why are we doing this? If the DT omits the supplies the framework will provide dummy supplies so there is no need to open code handling for supplies not being present at all in client drivers. Just unconditionally ask for all the supplies.
On Wed, Nov 3, 2021 at 3:45 PM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote: > > > + 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]; > > + } > > Why are we doing this? If the DT omits the supplies the framework will > provide dummy supplies so there is no need to open code handling for > supplies not being present at all in client drivers. Just > unconditionally ask for all the supplies. I did it to squelch the "supply xxxxx not found, using dummy regulator" output. I'll change it. Regards, Jim
On Wed, Nov 3, 2021 at 4:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > On Wed, Nov 3, 2021 at 3:45 PM Mark Brown <broonie@kernel.org> wrote: > > > > On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote: > > > > > + 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]; > > > + } > > > > Why are we doing this? If the DT omits the supplies the framework will > > provide dummy supplies so there is no need to open code handling for > > supplies not being present at all in client drivers. Just > > unconditionally ask for all the supplies. > > I did it to squelch the "supply xxxxx not found, using dummy > regulator" output. I'll change it. Now I remember: if I know there are no vpciexxx-supplly props in the DT, I can skip executing all of the buik regulator calls entirely, as well as walking the PCI bus as in brcm_regulators_off(). Do you consider this a valid reason? Jim Jim Jim > Regards, Jim
On Wed, Nov 03, 2021 at 04:34:34PM -0400, Jim Quinlan wrote: > On Wed, Nov 3, 2021 at 4:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > I did it to squelch the "supply xxxxx not found, using dummy > > regulator" output. I'll change it. > Now I remember: if I know there are no vpciexxx-supplly props in the > DT, I can skip executing all of the buik regulator calls entirely, as > well as walking the PCI bus as in brcm_regulators_off(). > Do you consider this a valid reason? No, the whole point in the core code providing dummy supplies is that it removes the complexity introduced by client drivers trying to guess if there's supplies available or not. If they do that then we end up with a bunch of code duplication and issues if there's any changes or extensions to the generic bindings.
On Wed, Nov 3, 2021 at 5:52 PM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Nov 03, 2021 at 04:34:34PM -0400, Jim Quinlan wrote: > > On Wed, Nov 3, 2021 at 4:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > > I did it to squelch the "supply xxxxx not found, using dummy > > > regulator" output. I'll change it. > > > Now I remember: if I know there are no vpciexxx-supplly props in the > > DT, I can skip executing all of the buik regulator calls entirely, as > > well as walking the PCI bus as in brcm_regulators_off(). > > > Do you consider this a valid reason? > > No, the whole point in the core code providing dummy supplies is that it > removes the complexity introduced by client drivers trying to guess if > there's supplies available or not. If they do that then we end up with > a bunch of code duplication and issues if there's any changes or > extensions to the generic bindings. Ok, will change it. Thanks, Jim
On Wed, Nov 3, 2021 at 1:49 PM Jim Quinlan <jim2101024@gmail.com> 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. > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 156 +++++++++++++++++++++++++- > 1 file changed, 154 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index ba4d6daf312c..aaf6a4cbeb78 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> > @@ -191,6 +192,15 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, > 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 int brcm_pcie_add_bus(struct pci_bus *bus); > +static void brcm_pcie_remove_bus(struct pci_bus *bus); > +static bool brcm_pcie_link_up(struct brcm_pcie *pcie); > + > +static const char * const supplies[] = { > + "vpcie3v3", > + "vpcie3v3aux", > + "vpcie12v", Common DT properties, so they should be in common DT code. > +}; > > enum { > RGR1_SW_INIT_1, > @@ -295,8 +305,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; Humm, this will need to be stored somewhere, but the host bridge is not the right place. That doesn't scale to more than 1 bridge/bus. I'm not exactly sure where though. pci_bus.self->sys_data, pci_bus.self->dev.driver_data, pci_bus->sysdata, pci_bus->dev.driver_data are possible options. Bjorn? However, given suspend/resume hooks are also needed, maybe portdrv_pci.c driver is the better spot for all this? The host bridge wouldn't be in control of opting in, but presence of a DT node ptr for the port device may be sufficient. Rob
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index ba4d6daf312c..aaf6a4cbeb78 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> @@ -191,6 +192,15 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, 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 int brcm_pcie_add_bus(struct pci_bus *bus); +static void brcm_pcie_remove_bus(struct pci_bus *bus); +static bool brcm_pcie_link_up(struct brcm_pcie *pcie); + +static const char * const supplies[] = { + "vpcie3v3", + "vpcie3v3aux", + "vpcie12v", +}; enum { RGR1_SW_INIT_1, @@ -295,8 +305,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 @@ -722,6 +762,8 @@ static struct pci_ops brcm_pcie_ops = { .map_bus = brcm_pcie_map_conf, .read = pci_generic_config_read, .write = pci_generic_config_write, + .add_bus = brcm_pcie_add_bus, + .remove_bus = brcm_pcie_remove_bus, }; static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val) @@ -1148,6 +1190,47 @@ 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 device *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; + + dn = 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; + + ret = devm_regulator_bulk_get(dev, pcie->num_supplies, + pcie->supplies); + if (ret) + dev_err(dev, "failed to get EP regulators\n"); + + return ret; +} + static int brcm_pcie_suspend(struct device *dev) { struct brcm_pcie *pcie = dev_get_drvdata(dev); @@ -1158,7 +1241,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 +1257,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) @@ -1241,6 +1327,62 @@ static const struct of_device_id brcm_pcie_match[] = { {}, }; +static int brcm_pcie_add_bus(struct pci_bus *bus) +{ + struct pci_host_bridge *hb = pci_find_host_bridge(bus); + struct brcm_pcie *pcie = (struct brcm_pcie *) hb->sysdata; + struct device *dev = &bus->dev; + int ret; + + /* + * We only care about a device that is directly connected + * to the root complex, ie bus == 1 and slot == 0. + */ + if (bus->number != 1) + return 0; + + ret = brcm_pcie_get_regulators(pcie, dev); + if (ret) + goto err_out; + + ret = brcm_regulators_on(pcie); + if (ret) + goto err_out; + + ret = brcm_pcie_linkup(pcie); + if (ret) + goto err_out; + + return 0; + +err_out: + /* + * If we have failed there is no point to return the exact reason, + * as currently it will cause a WARNING() from + * pci_alloc_child_bus(). We return -ENOLINK to the caller, which + * unlike any other errors, will stop the PCIe tree exploration. + */ + return -ENOLINK; +} + +static void brcm_pcie_remove_bus(struct pci_bus *bus) +{ + struct pci_host_bridge *hb = pci_find_host_bridge(bus); + struct brcm_pcie *pcie = (struct brcm_pcie *) hb->sysdata; + struct device *dev = &bus->dev; + int ret; + + if (bus->number != 1) + return; + + if (pcie->num_supplies > 0) { + ret = brcm_regulators_off(pcie, true); + if (ret) + dev_err(dev, "Could not turn of regulators\n"); + pcie->num_supplies = 0; + } +} + static int brcm_pcie_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node, *msi_np; @@ -1332,7 +1474,17 @@ static int brcm_pcie_probe(struct platform_device *pdev) 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 | 156 +++++++++++++++++++++++++- 1 file changed, 154 insertions(+), 2 deletions(-)