| Message ID | 20210326191906.43567-3-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: brcmstb: add EP regulators and panic handler | expand |
On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote: > Control of EP regulators by the RC is needed because of the chicken-and-egg Can you expand "EP"? Not sure if this refers to "endpoint" or something else. If this refers to a device in a slot, I guess it isn't necessarily a PCIe *endpoint*; it could also be a switch upstream port. > 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 <jim2101024@gmail.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++- > 1 file changed, 87 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index e330e6811f0b..b76ec7d9af32 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> > @@ -169,6 +170,7 @@ > #define SSC_STATUS_SSC_MASK 0x400 > #define SSC_STATUS_PLL_LOCK_MASK 0x800 > #define PCIE_BRCM_MAX_MEMC 3 > +#define PCIE_BRCM_MAX_EP_REGULATORS 4 > > #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX]) > #define DATA_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_DATA]) > @@ -295,8 +297,27 @@ 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[PCIE_BRCM_MAX_EP_REGULATORS]; > + unsigned int num_supplies; > }; > > +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on) > +{ > + struct device *dev = pcie->dev; > + int ret; > + > + if (!pcie->num_supplies) > + return 0; > + if (on) > + ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies); > + else > + ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies); > + if (ret) > + dev_err(dev, "failed to %s EP regulators\n", > + on ? "enable" : "disable"); > + 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 > @@ -1141,16 +1162,63 @@ 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_node *child, *parent = pcie->np; > + const unsigned int max_name_len = 64 + 4; > + struct property *pp; > + > + /* Look for regulator supply property in the EP device subnodes */ > + for_each_available_child_of_node(parent, child) { > + /* > + * Do a santiy test to ensure that this is an EP node s/santiy/sanity/ > + * (e.g. node name: "pci-ep@0,0"). The slot number > + * should always be 0 as our controller only has a single > + * port. > + */ > + const char *p = strstr(child->full_name, "@0"); > + > + if (!p || (p[2] && p[2] != ',')) > + continue; > + > + /* Now look for regulator supply properties */ > + for_each_property_of_node(child, pp) { > + int i, n = strnlen(pp->name, max_name_len); > + > + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7)) > + continue; > + > + /* Make sure this is not a duplicate */ > + for (i = 0; i < pcie->num_supplies; i++) > + if (strncmp(pcie->supplies[i].supply, > + pp->name, max_name_len) == 0) > + continue; > + > + if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS) > + pcie->supplies[pcie->num_supplies++].supply = pp->name; > + else > + dev_warn(pcie->dev, "No room for EP supply %s\n", > + pp->name); > + } > + } > + /* > + * Get the regulators that the EP devices require. We cannot use > + * pcie->dev as the device argument in regulator_bulk_get() since > + * it will not find the regulators. Instead, use NULL and the > + * regulators are looked up by their name. The comment doesn't explain the interesting part of why you need NULL instead of "pcie->dev". I assume it has something to do with the platform topology and its DT description. This appears to be the only instance in the whole kernel of a use of regulator_bulk_get() or devm_regulator_bulk_get() with NULL. That definitely warrants a comment, so I'm glad you've got something here. The regulator_bulk_get() function comment doesn't mention the possibility of "dev == NULL", although regulator_dev_lookup(), create_regulator(), device_link_add() do check for it being NULL, so I guess it's not a surprise. We may call dev_err(NULL), which I think will *work* without crashing even though it will look like a mistake on the output. > + */ > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies); devm_regulator_bulk_get()? > +} > + > static int brcm_pcie_suspend(struct device *dev) > { > struct brcm_pcie *pcie = dev_get_drvdata(dev); > - int ret; > > brcm_pcie_turn_off(pcie); > - ret = brcm_phy_stop(pcie); > + brcm_phy_stop(pcie); If we no longer care whether brcm_phy_stop() returns an error, nobody looks at the return value and it could be void. > clk_disable_unprepare(pcie->clk); > > - return ret; > + return brcm_set_regulators(pcie, false); > } > > static int brcm_pcie_resume(struct device *dev) > @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev) > base = pcie->base; > clk_prepare_enable(pcie->clk); > > + ret = brcm_set_regulators(pcie, true); > + if (ret) > + return ret; > + > ret = brcm_phy_start(pcie); > if (ret) > goto err; > @@ -1199,6 +1271,8 @@ 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); > + regulator_bulk_free(pcie->num_supplies, pcie->supplies); > } > > static int brcm_pcie_remove(struct platform_device *pdev) > @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev) > return ret; > } > > + ret = brcm_pcie_get_regulators(pcie); > + if (ret) { > + dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret); > + goto fail; > + } > + > + ret = brcm_set_regulators(pcie, true); > + if (ret) > + goto fail; > + > ret = brcm_pcie_setup(pcie); > if (ret) > goto fail; > -- > 2.17.1 >
On Fri, Mar 26, 2021 at 4:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote: > > Control of EP regulators by the RC is needed because of the chicken-and-egg > > Can you expand "EP"? Not sure if this refers to "endpoint" or > something else. Yes I meant "endpoint" -- I will expand it. > > If this refers to a device in a slot, I guess it isn't necessarily aWe only support this feature for endpoint devices; it they hav > PCIe *endpoint*; it could also be a switch upstream port. True; to be precise I mean the device directly connected to the single RC port. > > > 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 <jim2101024@gmail.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++- > > 1 file changed, 87 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index e330e6811f0b..b76ec7d9af32 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> > > @@ -169,6 +170,7 @@ > > #define SSC_STATUS_SSC_MASK 0x400 > > #define SSC_STATUS_PLL_LOCK_MASK 0x800 > > #define PCIE_BRCM_MAX_MEMC 3 > > +#define PCIE_BRCM_MAX_EP_REGULATORS 4 > > > > #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX]) > > #define DATA_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_DATA]) > > @@ -295,8 +297,27 @@ 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[PCIE_BRCM_MAX_EP_REGULATORS]; > > + unsigned int num_supplies; > > }; > > > > +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on) > > +{ > > + struct device *dev = pcie->dev; > > + int ret; > > + > > + if (!pcie->num_supplies) > > + return 0; > > + if (on) > > + ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies); > > + else > > + ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies); > > + if (ret) > > + dev_err(dev, "failed to %s EP regulators\n", > > + on ? "enable" : "disable"); > > + 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 > > @@ -1141,16 +1162,63 @@ 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_node *child, *parent = pcie->np; > > + const unsigned int max_name_len = 64 + 4; > > + struct property *pp; > > + > > + /* Look for regulator supply property in the EP device subnodes */ > > + for_each_available_child_of_node(parent, child) { > > + /* > > + * Do a santiy test to ensure that this is an EP node > > s/santiy/sanity/ > > > + * (e.g. node name: "pci-ep@0,0"). The slot number > > + * should always be 0 as our controller only has a single > > + * port. > > + */ > > + const char *p = strstr(child->full_name, "@0"); > > + > > + if (!p || (p[2] && p[2] != ',')) > > + continue; > > + > > + /* Now look for regulator supply properties */ > > + for_each_property_of_node(child, pp) { > > + int i, n = strnlen(pp->name, max_name_len); > > + > > + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7)) > > + continue; > > + > > + /* Make sure this is not a duplicate */ > > + for (i = 0; i < pcie->num_supplies; i++) > > + if (strncmp(pcie->supplies[i].supply, > > + pp->name, max_name_len) == 0) > > + continue; > > + > > + if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS) > > + pcie->supplies[pcie->num_supplies++].supply = pp->name; > > + else > > + dev_warn(pcie->dev, "No room for EP supply %s\n", > > + pp->name); > > + } > > + } > > + /* > > + * Get the regulators that the EP devices require. We cannot use > > + * pcie->dev as the device argument in regulator_bulk_get() since > > + * it will not find the regulators. Instead, use NULL and the > > + * regulators are looked up by their name. > > The comment doesn't explain the interesting part of why you need NULL > instead of "pcie->dev". I assume it has something to do with the > platform topology and its DT description. > > This appears to be the only instance in the whole kernel of a use of > regulator_bulk_get() or devm_regulator_bulk_get() with NULL. That > definitely warrants a comment, so I'm glad you've got something here. > > The regulator_bulk_get() function comment doesn't mention the > possibility of "dev == NULL", although regulator_dev_lookup(), > create_regulator(), device_link_add() do check for it being NULL, so I > guess it's not a surprise. We may call dev_err(NULL), which I thinkWe only support this feature for endpoint devices; it they hav > will *work* without crashing even though it will look like a mistake > on the output. Folks wanted me to put the "supply" in the endpoint subnode. After looking at the regulator code I assumed that using the pcie->dev in this call would not work as the supply property is not in its DT node. Turns out it works fine; I will fix it. > > > + */ > > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies); > > devm_regulator_bulk_get()? Yep. > > > +} > > + > > static int brcm_pcie_suspend(struct device *dev) > > { > > struct brcm_pcie *pcie = dev_get_drvdata(dev); > > - int ret; > > > > brcm_pcie_turn_off(pcie); > > - ret = brcm_phy_stop(pcie); > > + brcm_phy_stop(pcie);We only support this feature for endpoint devices; it they hav > > If we no longer care whether brcm_phy_stop() returns an error, nobody > looks at the return value and it could be void. Will fix. Thanks, Jim Quinlan Broadcom STB > > > clk_disable_unprepare(pcie->clk); > > > > - return ret; > > + return brcm_set_regulators(pcie, false); > > } > > > > static int brcm_pcie_resume(struct device *dev) > > @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev) > > base = pcie->base; > > clk_prepare_enable(pcie->clk); > > > > + ret = brcm_set_regulators(pcie, true); > > + if (ret) > > + return ret; > > + > > ret = brcm_phy_start(pcie); > > if (ret) > > goto err; > > @@ -1199,6 +1271,8 @@ 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); > > + regulator_bulk_free(pcie->num_supplies, pcie->supplies); > > } > > > > static int brcm_pcie_remove(struct platform_device *pdev) > > @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > return ret; > > } > > > > + ret = brcm_pcie_get_regulators(pcie); > > + if (ret) { > > + dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret); > > + goto fail; > > + } > > + > > + ret = brcm_set_regulators(pcie, true); > > + if (ret) > > + goto fail; > > + > > ret = brcm_pcie_setup(pcie); > > if (ret) > > goto fail; > > -- > > 2.17.1 > >
On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote: > 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. Ideally the driver core would have a way for buses to register devices pre physical enumeration and give drivers a callback that could be used to do whatever is needed to trigger enumeration, letting the bus then match newly physically enumerated devices with the software enumerated struct devices. Soundwire has something in this area for a bus level solution.
On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote: > + /* Now look for regulator supply properties */ > + for_each_property_of_node(child, pp) { > + int i, n = strnlen(pp->name, max_name_len); > + > + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7)) > + continue; Here you are figuring out a device local supply name... > + /* > + * Get the regulators that the EP devices require. We cannot use > + * pcie->dev as the device argument in regulator_bulk_get() since > + * it will not find the regulators. Instead, use NULL and the > + * regulators are looked up by their name. > + */ > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies); ...and here you are trying to look up that device local name in the global namespace. That's not going to work well, the global names that supplies are labelled with may be completely different to what the chip designer called them and there could easily be naming collisions between different chips.
On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org> w./lib/python3.6/site-packages/dtschema/schemasrote: > > On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote: > > > + /* Now look for regulator supply properties */ > > + for_each_property_of_node(child, pp) { > > + int i, n = strnlen(pp->name, max_name_len); > > + > > + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7)) > > + continue; > > Here you are figuring out a device local supply name... > > > + /* > > + * Get the regulators that the EP devianswerces require. We cannot use > > + * pcie->dev as the device argument in regulator_bulk_get() since > > + * it will not find the regulators. Instead, use NULL and the > > + * regulators are looked up by their name. > > + */ > > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies); > > ...and here you are trying to look up that device local name in the > global namespace. That's not going to work well, the global names that > supplies are labelled with may be completely different to what the chip > designer called them and there could easily be naming collisions between > different chips. Hello Mark, I am re-submitting this pullreq using "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the NULL for the device and if so does this fix it? If not, what do you suggest that I do? Thanks, Jim
On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote: > On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org> > > Here you are figuring out a device local supply name... > > > + /* > > > + * Get the regulators that the EP devianswerces require. We cannot use > > > + * pcie->dev as the device argument in regulator_bulk_get() since > > > + * it will not find the regulators. Instead, use NULL and the > > > + * regulators are looked up by their name. > > > + */ > > > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies); > > ...and here you are trying to look up that device local name in the > > global namespace. That's not going to work well, the global names that > > supplies are labelled with may be completely different to what the chip > > designer called them and there could easily be naming collisions between > > different chips. > "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the > NULL for the device and if so does this fix it? If not, what do you > suggest that I do? If you use the struct device for the PCIe controller then that's going to first try the PCIe controller then the global namespace so you still have all the same problems. You really need to use the struct device for the device that is being supplied not some random other struct device you happened to find in the system. As I said in my earlier reply I think either the driver core or PCI needs something like Soundwire has which lets it create struct devices for things that have been enumerated via software but not enumerated by hardware and a callback or something which lets those devices take whatever steps are needed to trigger probe.
/* Pmap_idx to avs pmap number */
const uint8_t pmap_idx_to_avs_id[20];
On Mon, Mar 29, 2021 at 1:16 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> > On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org>
>
> > > Here you are figuring out a device local supply name...
>
> > > > + /*
> > > > + * Get the regulators that the EP devianswerces require. We cannot use
> > > > + * pcie->dev as the device argument in regulator_bulk_get() since
> > > > + * it will not find the regulators. Instead, use NULL and the
> > > > + * regulators are looked up by their name.
> > > > + */
> > > > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> > > ...and here you are trying to look up that device local name in the
> > > global namespace. That's not going to work well, the global names that
> > > supplies are labelled with may be completely different to what the chip
> > > designer called them and there could easily be naming collisions between
> > > different chips.
>
> > "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> > NULL for the device and if so does this fix it? If not, what do you
> > suggest that I do?
>
> If you use the struct device for the PCIe controller then that's going
> to first try the PCIe controller then the global namespace so you still
> have all the same problems. You really need to use the struct device
> for the device that is being supplied not some random other struct
> device you happened to find in the system.
Hello Mark,
I'm not concerned about a namespace collision and I don't think you
should be concerned either. First, this driver is for Broadcom STB
PCIe chips and boards, and we also deliver the DT to the customers.
We typically do not have any other regulators in the DT besides the
ones I am proposing. For example, the 7216 SOC DT has 0 other
regulators -- no namespace collision possible. Our DT-generating
scripts also flag namespace issues. I admit that this driver is also
used by RPi chips, but I can easily exclude this feature from the RPI
if Nicolas has any objection.
Further, if you want, I can restrict the search to the two regulators
I am proposing to add to pci-bus.yaml: "vpcie12v-supply" and
"vpcie3v3-supply".
Is the above enough to alleviate your concerns about global namespace collision?
>
> As I said in my earlier reply I think either the driver core or PCI
> needs something like Soundwire has which lets it create struct devices
> for things that have been enumerated via software but not enumerated by
> hardware and a callback or something which lets those devices take
> whatever steps are needed to trigger probe.
Can you please elaborate this further and in detail? This sounds like
a decent-size undertaking, and the last time I followed a review
suggestion that was similar in spirit to this, the pullreq was
ironically NAKed by the person who suggested it. Do you really think
it is necessary to construct such a subsystem just to avoid the very
remote possibility of a namespace collision which is our (Broadcom
STB) responsibility and consequence to avoid?
Regards,
Jim Quinlan
Broadcom STB
On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote: > I'm not concerned about a namespace collision and I don't think you > should be concerned either. First, this driver is for Broadcom STB > PCIe chips and boards, and we also deliver the DT to the customers. > We typically do not have any other regulators in the DT besides the > ones I am proposing. For example, the 7216 SOC DT has 0 other You may not describe these regulators in the DT but you must have other regulators in your system, most devices need power to operate. In any case "this works for me with my DT on my system and nobody will ever change our reference design" isn't really a great approach, frankly it's not a claim I entirely believe and even if it turns out to be true for your systems if we establish this as being how regulators work for soldered down PCI devices everyone else is going to want to do the same thing, most likely making the same claims for how much control they have over the systems things will run on. > regulators -- no namespace collision possible. Our DT-generating > scripts also flag namespace issues. I admit that this driver is also > used by RPi chips, but I can easily exclude this feature from the RPI > if Nicolas has any objection. That's certainly an issue, obviously the RPI is the sort of system where I'd imagine this would be particularly useful. > Further, if you want, I can restrict the search to the two regulators > I am proposing to add to pci-bus.yaml: "vpcie12v-supply" and > "vpcie3v3-supply". No, that doesn't help - what happens if someone uses separate regulators for different PCI devices? The reason the supplies are device namespaced is that each device can look up it's own supplies and label them how it wants without reference to anything else on the board. Alternatively what happens if some device has another supply it needs to power on (eg, something that wants a clean LDO output for analogue use)? > Is the above enough to alleviate your concerns about global namespace collision? Not really. TBH it looks like this driver is keeping the regulators enabled all the time except for suspend and resume anyway, if that's all that's going on I'd have thought that you wouldn't need any explicit management in the driver anyway? Just mark the regualtors as always on and set up an appropriate suspend mode configuration and everything should work without the drivers doing anything. Unless your PMIC isn't able to provide separate suspend mode configuration for the regulators?
On 3/29/21 1:45 PM, Mark Brown wrote: > On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote: > >> I'm not concerned about a namespace collision and I don't think you >> should be concerned either. First, this driver is for Broadcom STB >> PCIe chips and boards, and we also deliver the DT to the customers. >> We typically do not have any other regulators in the DT besides the >> ones I am proposing. For example, the 7216 SOC DT has 0 other > > You may not describe these regulators in the DT but you must have other > regulators in your system, most devices need power to operate. In any > case "this works for me with my DT on my system and nobody will ever > change our reference design" isn't really a great approach, frankly it's > not a claim I entirely believe and even if it turns out to be true for > your systems if we establish this as being how regulators work for > soldered down PCI devices everyone else is going to want to do the same > thing, most likely making the same claims for how much control they have > over the systems things will run on. > >> regulators -- no namespace collision possible. Our DT-generating >> scripts also flag namespace issues. I admit that this driver is also >> used by RPi chips, but I can easily exclude this feature from the RPI >> if Nicolas has any objection. > > That's certainly an issue, obviously the RPI is the sort of system where > I'd imagine this would be particularly useful. > >> Further, if you want, I can restrict the search to the two regulators >> I am proposing to add to pci-bus.yaml: "vpcie12v-supply" and >> "vpcie3v3-supply". > > No, that doesn't help - what happens if someone uses separate regulators > for different PCI devices? The reason the supplies are device namespaced > is that each device can look up it's own supplies and label them how it > wants without reference to anything else on the board. Alternatively > what happens if some device has another supply it needs to power on (eg, > something that wants a clean LDO output for analogue use)? > >> Is the above enough to alleviate your concerns about global namespace collision? > > Not really. TBH it looks like this driver is keeping the regulators > enabled all the time except for suspend and resume anyway, if that's all > that's going on I'd have thought that you wouldn't need any explicit > management in the driver anyway? Just mark the regualtors as always on > and set up an appropriate suspend mode configuration and everything > should work without the drivers doing anything. Unless your PMIC isn't > able to provide separate suspend mode configuration for the regulators? > We have typically GPIO-controlled and PMIC (via SCMI) controlled regulators. During PCIe enumeration we need these regulators turned on to be successful in training the PCIe link and discover the end-point attached, so there an always on regulator would work. When we enter a system suspend state however there are really two cases: - the end-point supports Wake-on (typically wake-on-WLAN) and we need its power supplied kept on to support that - the end-point does not support or participate in any wake-up, there we want to turn its supplies off to save power How would we go about supporting such an use case with an always on regulator?
On Mon, Mar 29, 2021 at 02:09:58PM -0700, Florian Fainelli wrote: > On 3/29/21 1:45 PM, Mark Brown wrote: > > management in the driver anyway? Just mark the regualtors as always on > > and set up an appropriate suspend mode configuration and everything > > should work without the drivers doing anything. Unless your PMIC isn't > > able to provide separate suspend mode configuration for the regulators? > We have typically GPIO-controlled and PMIC (via SCMI) controlled > regulators. During PCIe enumeration we need these regulators turned on > to be successful in training the PCIe link and discover the end-point > attached, so there an always on regulator would work. > When we enter a system suspend state however there are really two cases: > - the end-point supports Wake-on (typically wake-on-WLAN) and we need > its power supplied kept on to support that > - the end-point does not support or participate in any wake-up, there we > want to turn its supplies off to save power > How would we go about supporting such an use case with an always on > regulator? With a PMIC most PMICs have a system suspend mode with separate regulator configuration for that and there's seprate regulator API control for those, including DT bindings. If that needs runtime configuration for something hidden by SCMI I'd hope the SCMI regulator stuff has facilities for that, if not then I guess a spec extension is needed. If you want to dynamically select if something is on during suspend there's not really a way around regulator API support. For a GPIO regulator you probably need something that does a disable on the way down, assuming that the GPIO/pin controller doesn't end up having it's own suspend mode control that ends up powering things off anyway. With GPIOs pinctrl on the pins rather than exposing as a regulator might be enough.
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index e330e6811f0b..b76ec7d9af32 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> @@ -169,6 +170,7 @@ #define SSC_STATUS_SSC_MASK 0x400 #define SSC_STATUS_PLL_LOCK_MASK 0x800 #define PCIE_BRCM_MAX_MEMC 3 +#define PCIE_BRCM_MAX_EP_REGULATORS 4 #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX]) #define DATA_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_DATA]) @@ -295,8 +297,27 @@ 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[PCIE_BRCM_MAX_EP_REGULATORS]; + unsigned int num_supplies; }; +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on) +{ + struct device *dev = pcie->dev; + int ret; + + if (!pcie->num_supplies) + return 0; + if (on) + ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies); + else + ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies); + if (ret) + dev_err(dev, "failed to %s EP regulators\n", + on ? "enable" : "disable"); + 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 @@ -1141,16 +1162,63 @@ 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_node *child, *parent = pcie->np; + const unsigned int max_name_len = 64 + 4; + struct property *pp; + + /* Look for regulator supply property in the EP device subnodes */ + for_each_available_child_of_node(parent, child) { + /* + * Do a santiy test to ensure that this is an EP node + * (e.g. node name: "pci-ep@0,0"). The slot number + * should always be 0 as our controller only has a single + * port. + */ + const char *p = strstr(child->full_name, "@0"); + + if (!p || (p[2] && p[2] != ',')) + continue; + + /* Now look for regulator supply properties */ + for_each_property_of_node(child, pp) { + int i, n = strnlen(pp->name, max_name_len); + + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7)) + continue; + + /* Make sure this is not a duplicate */ + for (i = 0; i < pcie->num_supplies; i++) + if (strncmp(pcie->supplies[i].supply, + pp->name, max_name_len) == 0) + continue; + + if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS) + pcie->supplies[pcie->num_supplies++].supply = pp->name; + else + dev_warn(pcie->dev, "No room for EP supply %s\n", + pp->name); + } + } + /* + * Get the regulators that the EP devices require. We cannot use + * pcie->dev as the device argument in regulator_bulk_get() since + * it will not find the regulators. Instead, use NULL and the + * regulators are looked up by their name. + */ + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies); +} + static int brcm_pcie_suspend(struct device *dev) { struct brcm_pcie *pcie = dev_get_drvdata(dev); - int ret; brcm_pcie_turn_off(pcie); - ret = brcm_phy_stop(pcie); + brcm_phy_stop(pcie); clk_disable_unprepare(pcie->clk); - return ret; + return brcm_set_regulators(pcie, false); } static int brcm_pcie_resume(struct device *dev) @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev) base = pcie->base; clk_prepare_enable(pcie->clk); + ret = brcm_set_regulators(pcie, true); + if (ret) + return ret; + ret = brcm_phy_start(pcie); if (ret) goto err; @@ -1199,6 +1271,8 @@ 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); + regulator_bulk_free(pcie->num_supplies, pcie->supplies); } static int brcm_pcie_remove(struct platform_device *pdev) @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev) return ret; } + ret = brcm_pcie_get_regulators(pcie); + if (ret) { + dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret); + goto fail; + } + + ret = brcm_set_regulators(pcie, true); + if (ret) + goto fail; + ret = brcm_pcie_setup(pcie); if (ret) goto fail;
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 <jim2101024@gmail.com> --- drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 3 deletions(-)