| Message ID | 20200213025930.27943-3-jaedon.shin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcmstb: Add Broadcom STB support | expand |
On 2/12/2020 6:59 PM, Jaedon Shin wrote: > ARM-based Broadcom STB SoCs have GPIO-based voltage regulator for PCIe > turning off/on power supplies. > > Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com> > --- > drivers/gpio/gpio-brcmstb.c | 13 ++++- > drivers/pci/controller/pcie-brcmstb.c | 76 +++++++++++++++++++++++++++ > 2 files changed, 88 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c > index 05e3f99ae59c..0cee5fcd2782 100644 > --- a/drivers/gpio/gpio-brcmstb.c > +++ b/drivers/gpio/gpio-brcmstb.c > @@ -777,7 +777,18 @@ static struct platform_driver brcmstb_gpio_driver = { > .remove = brcmstb_gpio_remove, > .shutdown = brcmstb_gpio_shutdown, > }; > -module_platform_driver(brcmstb_gpio_driver); > + > +static int __init brcmstb_gpio_init(void) > +{ > + return platform_driver_register(&brcmstb_gpio_driver); > +} > +subsys_initcall(brcmstb_gpio_init); > + > +static void __exit brcmstb_gpio_exit(void) > +{ > + platform_driver_unregister(&brcmstb_gpio_driver); > +} > +module_exit(brcmstb_gpio_exit); We do this in the downstream tree, but there is no reason, we should just deal with EPROBE_DEFER being returned from the regulator subsystem until the GPIO provide is available. [snip] > +static void brcm_pcie_regulator_init(struct brcm_pcie *pcie) > +{ > + struct device_node *np = pcie->dev->of_node; > + struct device *dev = pcie->dev; > + const char *name; > + struct regulator *reg; > + int i; > + > + pcie->num_regs = of_property_count_strings(np, "supply-names"); > + if (pcie->num_regs <= 0) { > + pcie->num_regs = 0; > + return; > + } > + > + pcie->regs = devm_kcalloc(dev, pcie->num_regs, sizeof(pcie->regs[0]), > + GFP_KERNEL); > + if (!pcie->regs) { > + pcie->num_regs = 0; > + return; > + } > + > + for (i = 0; i < pcie->num_regs; i++) { > + if (of_property_read_string_index(np, "supply-names", i, &name)) > + continue; > + > + reg = devm_regulator_get_optional(dev, name); > + if (IS_ERR(reg)) > + continue; You need to handle EPROBE_DEFER here and propagate that back to the caller to defer the entire driver from probing until the regulator providers are available. > + > + pcie->regs[i] = reg; > + } > +} > +#else > +static inline void brcm_pcie_regulator_enable(struct brcm_pcie *pcie) { } > +static inline void brcm_pcie_regulator_disable(struct brcm_pcie *pcie) { } > +static inline void brcm_pcie_regulator_init(struct brcm_pcie *pcie) { } > +#endif > + > /* > * 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 > @@ -898,6 +970,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) > { > brcm_msi_remove(pcie); > brcm_pcie_turn_off(pcie); > + brcm_pcie_regulator_disable(pcie); > clk_disable_unprepare(pcie->clk); > clk_put(pcie->clk); > } > @@ -955,6 +1028,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) > return ret; > } > > + brcm_pcie_regulator_init(pcie); > + brcm_pcie_regulator_enable(pcie); And deal with errors here. > + > ret = brcm_pcie_setup(pcie); > if (ret) > goto fail; >
On Wed, Feb 12, 2020 at 9:59 PM Jaedon Shin <jaedon.shin@gmail.com> wrote: > > ARM-based Broadcom STB SoCs have GPIO-based voltage regulator for PCIe > turning off/on power supplies. > > Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com> > --- > drivers/gpio/gpio-brcmstb.c | 13 ++++- > drivers/pci/controller/pcie-brcmstb.c | 76 +++++++++++++++++++++++++++ > 2 files changed, 88 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c > index 05e3f99ae59c..0cee5fcd2782 100644 > --- a/drivers/gpio/gpio-brcmstb.c > +++ b/drivers/gpio/gpio-brcmstb.c > @@ -777,7 +777,18 @@ static struct platform_driver brcmstb_gpio_driver = { > .remove = brcmstb_gpio_remove, > .shutdown = brcmstb_gpio_shutdown, > }; > -module_platform_driver(brcmstb_gpio_driver); > + > +static int __init brcmstb_gpio_init(void) > +{ > + return platform_driver_register(&brcmstb_gpio_driver); > +} > +subsys_initcall(brcmstb_gpio_init); > + > +static void __exit brcmstb_gpio_exit(void) > +{ > + platform_driver_unregister(&brcmstb_gpio_driver); > +} > +module_exit(brcmstb_gpio_exit); > > MODULE_AUTHOR("Gregory Fong"); > MODULE_DESCRIPTION("Driver for Broadcom BRCMSTB SoC UPG GPIO"); > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 34581a6a7313..0e0ca39a680b 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/sizes.h> > #include <linux/slab.h> > #include <linux/string.h> > @@ -173,8 +174,79 @@ struct brcm_pcie { > int gen; > u64 msi_target_addr; > struct brcm_msi *msi; > +#ifdef CONFIG_REGULATOR > + int num_regs; > + struct regulator **regs; > +#endif > }; Hi, Just a nit but I would lean towards using 'regulator' as opposed to 'reg', as the 'reg' term's common association is 'register'. You don't seem to be anywhere near the 80-col limit on your code so that doesn't seem to be an issue. Thanks, Jim > > +#ifdef CONFIG_REGULATOR > +static void brcm_pcie_regulator_enable(struct brcm_pcie *pcie) > +{ > + int i, ret; > + > + for (i = 0; i < pcie->num_regs; i++) { > + if (!pcie->regs[i]) > + continue; > + > + ret = regulator_enable(pcie->regs[i]); > + if (ret) > + dev_err(pcie->dev, "Failed to enable regulator\n"); > + } > +} > + > +static void brcm_pcie_regulator_disable(struct brcm_pcie *pcie) > +{ > + int i, ret; > + > + for (i = 0; i < pcie->num_regs; i++) { > + if (!pcie->regs[i]) > + continue; > + > + ret = regulator_disable(pcie->regs[i]); > + if (ret) > + dev_err(pcie->dev, "Failed to disable regulator\n"); > + } > +} > + > +static void brcm_pcie_regulator_init(struct brcm_pcie *pcie) > +{ > + struct device_node *np = pcie->dev->of_node; > + struct device *dev = pcie->dev; > + const char *name; > + struct regulator *reg; > + int i; > + > + pcie->num_regs = of_property_count_strings(np, "supply-names"); > + if (pcie->num_regs <= 0) { > + pcie->num_regs = 0; > + return; > + } > + > + pcie->regs = devm_kcalloc(dev, pcie->num_regs, sizeof(pcie->regs[0]), > + GFP_KERNEL); > + if (!pcie->regs) { > + pcie->num_regs = 0; > + return; > + } > + > + for (i = 0; i < pcie->num_regs; i++) { > + if (of_property_read_string_index(np, "supply-names", i, &name)) > + continue; > + > + reg = devm_regulator_get_optional(dev, name); > + if (IS_ERR(reg)) > + continue; > + > + pcie->regs[i] = reg; > + } > +} > +#else > +static inline void brcm_pcie_regulator_enable(struct brcm_pcie *pcie) { } > +static inline void brcm_pcie_regulator_disable(struct brcm_pcie *pcie) { } > +static inline void brcm_pcie_regulator_init(struct brcm_pcie *pcie) { } > +#endif > + > /* > * 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 > @@ -898,6 +970,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) > { > brcm_msi_remove(pcie); > brcm_pcie_turn_off(pcie); > + brcm_pcie_regulator_disable(pcie); > clk_disable_unprepare(pcie->clk); > clk_put(pcie->clk); > } > @@ -955,6 +1028,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) > return ret; > } > > + brcm_pcie_regulator_init(pcie); > + brcm_pcie_regulator_enable(pcie); > + > ret = brcm_pcie_setup(pcie); > if (ret) > goto fail; > -- > 2.21.0 >
Hi Jaedon, thanks for your patch! On Thu, Feb 13, 2020 at 3:59 AM Jaedon Shin <jaedon.shin@gmail.com> wrote: > +#ifdef CONFIG_REGULATOR > + int num_regs; > + struct regulator **regs; > +#endif Is this #ifdef:in necessary? Since the regulator framework provides stubs if compiled out, I think you can just include all code unconditionally and it will work fine anyway. > +static void brcm_pcie_regulator_enable(struct brcm_pcie *pcie) > +static void brcm_pcie_regulator_disable(struct brcm_pcie *pcie) > +static void brcm_pcie_regulator_init(struct brcm_pcie *pcie) I would replace the word "regulator" with "power" here to indicate what it is about (easier to read). > + struct device_node *np = pcie->dev->of_node; > + struct device *dev = pcie->dev; > + const char *name; > + struct regulator *reg; > + int i; > + > + pcie->num_regs = of_property_count_strings(np, "supply-names"); > + if (pcie->num_regs <= 0) { > + pcie->num_regs = 0; > + return; > + } > + > + pcie->regs = devm_kcalloc(dev, pcie->num_regs, sizeof(pcie->regs[0]), > + GFP_KERNEL); > + if (!pcie->regs) { > + pcie->num_regs = 0; > + return; > + } > + > + for (i = 0; i < pcie->num_regs; i++) { > + if (of_property_read_string_index(np, "supply-names", i, &name)) > + continue; > + > + reg = devm_regulator_get_optional(dev, name); > + if (IS_ERR(reg)) > + continue; > + > + pcie->regs[i] = reg; > + } > +} So what this does is just grab any regulators, no matter what they are named, and enable them? The swiss army knife used is the raw of_* parsing functions. I don't think that is very good practice. First define very cleanly what regulators exist in the device tree bindings. If the set of regulators differ between variants, then key that with the compatible value. Then explicitly grab the resources by name, using the regulator_bulk_get() API, which will transparently grab the regulators for you from the device tree. Then use regulator_bulk_[enable|disable] to enable/disable the regulators. git grep in the kernel tree for good examples! Also involve the regulator maintainer in the review. (I added him on the To: line.) Yours, Linus Walleij
On Thu Feb 13, 2020 at 11:59 AM, Jaedon Shin wrote: > @@ -173,8 +174,79 @@ struct brcm_pcie { > int gen; > u64 msi_target_addr; > struct brcm_msi *msi; > +#ifdef CONFIG_REGULATOR Correct me if I'm wrong, but I don't think these #ifdefs are necessary (same below). The regulator code defines empty functions and relevant structures even when not enabled. Regards, Nicolas
On Fri, Feb 14, 2020 at 11:06:51AM +0100, Linus Walleij wrote: > So what this does is just grab any regulators, no matter what they are > named, and enable them? The swiss army knife used is the raw > of_* parsing functions. > I don't think that is very good practice. This is a really bad idea, yes.
On Wed, Feb 12, 2020 at 7:58 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > On 2/12/2020 6:59 PM, Jaedon Shin wrote: > > ARM-based Broadcom STB SoCs have GPIO-based voltage regulator for PCIe > > turning off/on power supplies. > > > > Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com> > > --- > > drivers/gpio/gpio-brcmstb.c | 13 ++++- > > drivers/pci/controller/pcie-brcmstb.c | 76 +++++++++++++++++++++++++++ > > 2 files changed, 88 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c > > index 05e3f99ae59c..0cee5fcd2782 100644 > > --- a/drivers/gpio/gpio-brcmstb.c > > +++ b/drivers/gpio/gpio-brcmstb.c > > @@ -777,7 +777,18 @@ static struct platform_driver brcmstb_gpio_driver = { > > .remove = brcmstb_gpio_remove, > > .shutdown = brcmstb_gpio_shutdown, > > }; > > -module_platform_driver(brcmstb_gpio_driver); > > + > > +static int __init brcmstb_gpio_init(void) > > +{ > > + return platform_driver_register(&brcmstb_gpio_driver); > > +} > > +subsys_initcall(brcmstb_gpio_init); > > + > > +static void __exit brcmstb_gpio_exit(void) > > +{ > > + platform_driver_unregister(&brcmstb_gpio_driver); > > +} > > +module_exit(brcmstb_gpio_exit); > > We do this in the downstream tree, but there is no reason, we should > just deal with EPROBE_DEFER being returned from the regulator subsystem > until the GPIO provide is available. > Agreed, also see this thread from January 2016: https://lore.kernel.org/linux-mips/568EAA99.1020603@gmail.com/ Best regards, Gregory
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c index 05e3f99ae59c..0cee5fcd2782 100644 --- a/drivers/gpio/gpio-brcmstb.c +++ b/drivers/gpio/gpio-brcmstb.c @@ -777,7 +777,18 @@ static struct platform_driver brcmstb_gpio_driver = { .remove = brcmstb_gpio_remove, .shutdown = brcmstb_gpio_shutdown, }; -module_platform_driver(brcmstb_gpio_driver); + +static int __init brcmstb_gpio_init(void) +{ + return platform_driver_register(&brcmstb_gpio_driver); +} +subsys_initcall(brcmstb_gpio_init); + +static void __exit brcmstb_gpio_exit(void) +{ + platform_driver_unregister(&brcmstb_gpio_driver); +} +module_exit(brcmstb_gpio_exit); MODULE_AUTHOR("Gregory Fong"); MODULE_DESCRIPTION("Driver for Broadcom BRCMSTB SoC UPG GPIO"); diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 34581a6a7313..0e0ca39a680b 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/sizes.h> #include <linux/slab.h> #include <linux/string.h> @@ -173,8 +174,79 @@ struct brcm_pcie { int gen; u64 msi_target_addr; struct brcm_msi *msi; +#ifdef CONFIG_REGULATOR + int num_regs; + struct regulator **regs; +#endif }; +#ifdef CONFIG_REGULATOR +static void brcm_pcie_regulator_enable(struct brcm_pcie *pcie) +{ + int i, ret; + + for (i = 0; i < pcie->num_regs; i++) { + if (!pcie->regs[i]) + continue; + + ret = regulator_enable(pcie->regs[i]); + if (ret) + dev_err(pcie->dev, "Failed to enable regulator\n"); + } +} + +static void brcm_pcie_regulator_disable(struct brcm_pcie *pcie) +{ + int i, ret; + + for (i = 0; i < pcie->num_regs; i++) { + if (!pcie->regs[i]) + continue; + + ret = regulator_disable(pcie->regs[i]); + if (ret) + dev_err(pcie->dev, "Failed to disable regulator\n"); + } +} + +static void brcm_pcie_regulator_init(struct brcm_pcie *pcie) +{ + struct device_node *np = pcie->dev->of_node; + struct device *dev = pcie->dev; + const char *name; + struct regulator *reg; + int i; + + pcie->num_regs = of_property_count_strings(np, "supply-names"); + if (pcie->num_regs <= 0) { + pcie->num_regs = 0; + return; + } + + pcie->regs = devm_kcalloc(dev, pcie->num_regs, sizeof(pcie->regs[0]), + GFP_KERNEL); + if (!pcie->regs) { + pcie->num_regs = 0; + return; + } + + for (i = 0; i < pcie->num_regs; i++) { + if (of_property_read_string_index(np, "supply-names", i, &name)) + continue; + + reg = devm_regulator_get_optional(dev, name); + if (IS_ERR(reg)) + continue; + + pcie->regs[i] = reg; + } +} +#else +static inline void brcm_pcie_regulator_enable(struct brcm_pcie *pcie) { } +static inline void brcm_pcie_regulator_disable(struct brcm_pcie *pcie) { } +static inline void brcm_pcie_regulator_init(struct brcm_pcie *pcie) { } +#endif + /* * 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 @@ -898,6 +970,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) { brcm_msi_remove(pcie); brcm_pcie_turn_off(pcie); + brcm_pcie_regulator_disable(pcie); clk_disable_unprepare(pcie->clk); clk_put(pcie->clk); } @@ -955,6 +1028,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) return ret; } + brcm_pcie_regulator_init(pcie); + brcm_pcie_regulator_enable(pcie); + ret = brcm_pcie_setup(pcie); if (ret) goto fail;
ARM-based Broadcom STB SoCs have GPIO-based voltage regulator for PCIe turning off/on power supplies. Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com> --- drivers/gpio/gpio-brcmstb.c | 13 ++++- drivers/pci/controller/pcie-brcmstb.c | 76 +++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-)