| Message ID | 20210401212148.47033-4-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: brcmstb: add slot0 device regulators and panic handler | expand |
On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote: > + /* Look for specific pcie regulators in the RC DT node. */ > + for_each_property_of_node(np, pp) { > + for (i = 0; i < ns; i++) > + if (strcmp(supplies[i], pp->name) == 0) This is broken, the driver knows which supplies are expected, the device can't function without these supplies so the driver should just unconditionally request them like any other supply.
On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote: > > > + /* Look for specific pcie regulators in the RC DT node. */ > > + for_each_property_of_node(np, pp) { > > + for (i = 0; i < ns; i++) > > + if (strcmp(supplies[i], pp->name) == 0) > > This is broken, the driver knows which supplies are expected, the device > can't function without these supplies so the driver should just > unconditionally request them like any other supply. Hi Mark, Some boards require the regulators, some do not. So the driver is only sure what the names may be if they are present. If I put these names in my struct regulator_bulk_data array and do a devm_regulator_bulk_get(), I will get the following for the boards that do not need the regulators (e.g. the RPi SOC): [ 6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found, using dummy regulator [ 6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found, using dummy regulator IIRC you consider this a debug feature? Be that as it may, these lines will confuse our customers and I'd like that they not be printed if possible. So I ask you to allow the code as is. If you still insist, I will change and resubmit. Regards, Jim Quinlan Broadcom STB
On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote: > On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote: > > This is broken, the driver knows which supplies are expected, the device > > can't function without these supplies so the driver should just > > unconditionally request them like any other supply. > Some boards require the regulators, some do not. So the driver is No, some boards have the supplies described in firmware and some do not. > only sure what the names may be if they are present. If I put these > names in my struct regulator_bulk_data array and do a > devm_regulator_bulk_get(), I will get the following for the boards > that do not need the regulators (e.g. the RPi SOC): > > [ 6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found, > using dummy regulator > [ 6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found, > using dummy regulator Sure, those are just warnings. > IIRC you consider this a debug feature? Be that as it may, these > lines will confuse our customers and I'd like that they not be printed > if possible. You can stop the warnings by updating your firmware to more completely describe the system - ideally all the supplies in the system would be described for future proofing. Or if this is a custom software stack just delete whatever error checking and warnings you like. The warnings are there in case we've not got something mapped properly (eg, if there were a typo in a property name) and things stop working, it's not great to just ignore errors. > So I ask you to allow the code as is. If you still insist, I will > change and resubmit. Remove it, conditional code like this is just as bad in this driver as it is in every other one.
On Tue, Apr 6, 2021 at 1:23 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote: > > On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote: > > > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote: > > > > This is broken, the driver knows which supplies are expected, the device > > > can't function without these supplies so the driver should just > > > unconditionally request them like any other supply. > > > Some boards require the regulators, some do not. So the driver is > > No, some boards have the supplies described in firmware and some do not. True. > > > only sure what the names may be if they are present. If I put these > > names in my struct regulator_bulk_data array and do a > > devm_regulator_bulk_get(), I will get the following for the boards > > that do not need the regulators (e.g. the RPi SOC): > > > > [ 6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found, > > using dummy regulator > > [ 6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found, > > using dummy regulator > > Sure, those are just warnings. > > > IIRC you consider this a debug feature? Be that as it may, these > > lines will confuse our customers and I'd like that they not be printed > > if possible. > > You can stop the warnings by updating your firmware to more completely > describe the system - ideally all the supplies in the system would be > described for future proofing. Or if this is a custom software stack > just delete whatever error checking and warnings you like. The warnings > are there in case we've not got something mapped properly (eg, if there > were a typo in a property name) and things stop working, it's not great > to just ignore errors. A lot of this is really not under our control. > > > So I ask you to allow the code as is. If you still insist, I will > > change and resubmit. > > Remove it, conditional code like this is just as bad in this driver as > it is in every other one. I will remove this and resubmit. Thanks, Jim Quinlan Broadcom STB
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 4ce1f3a60574..1b0de0c7da60 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]) @@ -192,6 +194,11 @@ 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[] = { + "vpcie12v-supply", + "vpcie3v3-supply", +}; + enum { RGR1_SW_INIT_1, EXT_CFG_INDEX, @@ -295,8 +302,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 @@ -1112,9 +1138,10 @@ static inline int brcm_phy_start(struct brcm_pcie *pcie) return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0; } -static inline int brcm_phy_stop(struct brcm_pcie *pcie) +static inline void brcm_phy_stop(struct brcm_pcie *pcie) { - return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0; + if (pcie->rescal) + brcm_phy_cntl(pcie, 0); } static void brcm_pcie_turn_off(struct brcm_pcie *pcie) @@ -1141,16 +1168,45 @@ 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 *np = pcie->np; + struct property *pp; + const unsigned int ns = ARRAY_SIZE(supplies); + unsigned int i; + int ret = 0; + + /* Look for specific pcie regulators in the RC DT node. */ + for_each_property_of_node(np, pp) { + for (i = 0; i < ns; i++) + if (strcmp(supplies[i], pp->name) == 0) + break; + if (i >= ns) + continue; + + if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS) + pcie->supplies[pcie->num_supplies++].supply + = supplies[i]; + else + dev_warn(pcie->dev, "No room for supply %s\n", + supplies[i]); + } + + if (pcie->num_supplies) + ret = devm_regulator_bulk_get(pcie->dev, pcie->num_supplies, + pcie->supplies); + return ret; +} + 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) @@ -1165,6 +1221,10 @@ static int brcm_pcie_resume(struct device *dev) if (ret) return ret; + ret = brcm_set_regulators(pcie, true); + if (ret) + return ret; + ret = brcm_phy_start(pcie); if (ret) goto err; @@ -1201,6 +1261,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) @@ -1291,6 +1352,18 @@ static int brcm_pcie_probe(struct platform_device *pdev) return ret; } + ret = brcm_pcie_get_regulators(pcie); + if (ret) { + pcie->num_supplies = 0; + if (ret != -EPROBE_DEFER) + 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;
This Broadcom STB 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 | 83 +++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 5 deletions(-)