Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support
@ 2020-02-13  2:59 Jaedon Shin
  2020-02-13  2:59 ` [PATCH 1/3] PCI: brcmstb: Enable ARCH_BRCMSTB Jaedon Shin
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Jaedon Shin @ 2020-02-13  2:59 UTC (permalink / raw)
  To: Nicolas Saenz Julienne, Florian Fainelli, bcm-kernel-feedback-list
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci, Jaedon Shin

This series enables the ARM based Broadcom STB SoCs and supports GPIO
based regulators for its power supplies. and this has an improvement on
devm_ APIS.

Jaedon Shin (3):
  PCI: brcmstb: Enable ARCH_BRCMSTB
  PCI: brcmstb: Add regulator support
  PCI: brcmstb: Drop clk_put when probe fails and remove

 .../bindings/pci/brcm,stb-pcie.yaml           |  8 +-
 drivers/gpio/gpio-brcmstb.c                   | 13 +++-
 drivers/pci/controller/Kconfig                |  2 +-
 drivers/pci/controller/pcie-brcmstb.c         | 78 ++++++++++++++++++-
 4 files changed, 97 insertions(+), 4 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/3] PCI: brcmstb: Enable ARCH_BRCMSTB
  2020-02-13  2:59 [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Jaedon Shin
@ 2020-02-13  2:59 ` Jaedon Shin
  2020-02-13  3:58   ` Florian Fainelli
  2020-02-13  2:59 ` [PATCH 2/3] PCI: brcmstb: Add regulator support Jaedon Shin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Jaedon Shin @ 2020-02-13  2:59 UTC (permalink / raw)
  To: Nicolas Saenz Julienne, Florian Fainelli, bcm-kernel-feedback-list
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci, Jaedon Shin

Add PCIe support for ARM-based Broadcom STB SoCs.

Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
---
 Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 8 +++++++-
 drivers/pci/controller/Kconfig                           | 2 +-
 drivers/pci/controller/pcie-brcmstb.c                    | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 77d3e81a437b..fb1a78606f78 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -14,7 +14,13 @@ allOf:
 
 properties:
   compatible:
-    const: brcm,bcm2711-pcie # The Raspberry Pi 4
+    oneOf:
+      - description:
+          BCM2711 based Boards
+        const: brcm,bcm2711-pcie
+      - description:
+          ARM-based BCM7XXX Broadcom STB Boards
+        const: brcm,bcm7445-pcie
 
   reg:
     maxItems: 1
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 20bf00f587bd..c60a27cff81a 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -254,7 +254,7 @@ config VMD
 
 config PCIE_BRCMSTB
 	tristate "Broadcom Brcmstb PCIe host controller"
-	depends on ARCH_BCM2835 || COMPILE_TEST
+	depends on ARCH_BCM2835 || ARCH_BRCMSTB || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
 	help
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index d20aabc26273..34581a6a7313 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -996,6 +996,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie" },
+	{ .compatible = "brcm,bcm7445-pcie" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, brcm_pcie_match);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 2/3] PCI: brcmstb: Add regulator support
  2020-02-13  2:59 [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Jaedon Shin
  2020-02-13  2:59 ` [PATCH 1/3] PCI: brcmstb: Enable ARCH_BRCMSTB Jaedon Shin
@ 2020-02-13  2:59 ` Jaedon Shin
  2020-02-13  3:58   ` Florian Fainelli
                     ` (3 more replies)
  2020-02-13  2:59 ` [PATCH 3/3] PCI: brcmstb: Drop clk_put when probe fails and remove Jaedon Shin
  2020-02-13  3:54 ` [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Florian Fainelli
  3 siblings, 4 replies; 17+ messages in thread
From: Jaedon Shin @ 2020-02-13  2:59 UTC (permalink / raw)
  To: Nicolas Saenz Julienne, Florian Fainelli, bcm-kernel-feedback-list
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci, Jaedon Shin

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
 };
 
+#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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/3] PCI: brcmstb: Drop clk_put when probe fails and remove
  2020-02-13  2:59 [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Jaedon Shin
  2020-02-13  2:59 ` [PATCH 1/3] PCI: brcmstb: Enable ARCH_BRCMSTB Jaedon Shin
  2020-02-13  2:59 ` [PATCH 2/3] PCI: brcmstb: Add regulator support Jaedon Shin
@ 2020-02-13  2:59 ` Jaedon Shin
  2020-02-13  4:01   ` Florian Fainelli
  2020-02-14 10:55   ` Nicolas Saenz Julienne
  2020-02-13  3:54 ` [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Florian Fainelli
  3 siblings, 2 replies; 17+ messages in thread
From: Jaedon Shin @ 2020-02-13  2:59 UTC (permalink / raw)
  To: Nicolas Saenz Julienne, Florian Fainelli, bcm-kernel-feedback-list
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci, Jaedon Shin

devm_clk_get* APIs are device managed and get freed automatically when
the device detaches. so there is no reason to explicitly call clk_put()
in probe or remove functions.

Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 0e0ca39a680b..3e48d9e238bb 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -972,7 +972,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_pcie_turn_off(pcie);
 	brcm_pcie_regulator_disable(pcie);
 	clk_disable_unprepare(pcie->clk);
-	clk_put(pcie->clk);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
-- 
2.21.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support
  2020-02-13  2:59 [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Jaedon Shin
                   ` (2 preceding siblings ...)
  2020-02-13  2:59 ` [PATCH 3/3] PCI: brcmstb: Drop clk_put when probe fails and remove Jaedon Shin
@ 2020-02-13  3:54 ` Florian Fainelli
  2020-02-13  5:15   ` Jaedon Shin
  3 siblings, 1 reply; 17+ messages in thread
From: Florian Fainelli @ 2020-02-13  3:54 UTC (permalink / raw)
  To: Jaedon Shin, Nicolas Saenz Julienne, bcm-kernel-feedback-list,
	Jim Quinlan
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci

+Jim,

On 2/12/2020 6:59 PM, Jaedon Shin wrote:
> This series enables the ARM based Broadcom STB SoCs and supports GPIO
> based regulators for its power supplies. and this has an improvement on
> devm_ APIS.

Which ARM-based SoCs did you try this on? We still have an issue with
the multiple dma-ranges that must be handled to support 7445 with memory
in the extension regions as well as 7278 and 7216.

See comments in specific patches.

> 
> Jaedon Shin (3):
>   PCI: brcmstb: Enable ARCH_BRCMSTB
>   PCI: brcmstb: Add regulator support
>   PCI: brcmstb: Drop clk_put when probe fails and remove
> 
>  .../bindings/pci/brcm,stb-pcie.yaml           |  8 +-
>  drivers/gpio/gpio-brcmstb.c                   | 13 +++-
>  drivers/pci/controller/Kconfig                |  2 +-
>  drivers/pci/controller/pcie-brcmstb.c         | 78 ++++++++++++++++++-
>  4 files changed, 97 insertions(+), 4 deletions(-)
> 

-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] PCI: brcmstb: Enable ARCH_BRCMSTB
  2020-02-13  2:59 ` [PATCH 1/3] PCI: brcmstb: Enable ARCH_BRCMSTB Jaedon Shin
@ 2020-02-13  3:58   ` Florian Fainelli
  0 siblings, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2020-02-13  3:58 UTC (permalink / raw)
  To: Jaedon Shin, Nicolas Saenz Julienne, bcm-kernel-feedback-list,
	Jim Quinlan
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci



On 2/12/2020 6:59 PM, Jaedon Shin wrote:
> Add PCIe support for ARM-based Broadcom STB SoCs.
> 
> Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
> ---
>  Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 8 +++++++-
>  drivers/pci/controller/Kconfig                           | 2 +-
>  drivers/pci/controller/pcie-brcmstb.c                    | 1 +
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 77d3e81a437b..fb1a78606f78 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -14,7 +14,13 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: brcm,bcm2711-pcie # The Raspberry Pi 4
> +    oneOf:
> +      - description:
> +          BCM2711 based Boards
> +        const: brcm,bcm2711-pcie
> +      - description:
> +          ARM-based BCM7XXX Broadcom STB Boards
> +        const: brcm,bcm7445-pcie

You would also need to document the regulator properties as optional here.

>  
>    reg:
>      maxItems: 1
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 20bf00f587bd..c60a27cff81a 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -254,7 +254,7 @@ config VMD
>  
>  config PCIE_BRCMSTB
>  	tristate "Broadcom Brcmstb PCIe host controller"
> -	depends on ARCH_BCM2835 || COMPILE_TEST
> +	depends on ARCH_BCM2835 || ARCH_BRCMSTB || COMPILE_TEST

This part looks entirely reasonable.
-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] PCI: brcmstb: Add regulator support
  2020-02-13  2:59 ` [PATCH 2/3] PCI: brcmstb: Add regulator support Jaedon Shin
@ 2020-02-13  3:58   ` Florian Fainelli
  2020-02-20 11:07     ` Gregory Fong
  2020-02-13 15:25   ` Jim Quinlan
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Florian Fainelli @ 2020-02-13  3:58 UTC (permalink / raw)
  To: Jaedon Shin, Nicolas Saenz Julienne, bcm-kernel-feedback-list,
	Jim Quinlan
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci



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;
> 

-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] PCI: brcmstb: Drop clk_put when probe fails and remove
  2020-02-13  2:59 ` [PATCH 3/3] PCI: brcmstb: Drop clk_put when probe fails and remove Jaedon Shin
@ 2020-02-13  4:01   ` Florian Fainelli
  2020-02-14 10:55   ` Nicolas Saenz Julienne
  1 sibling, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2020-02-13  4:01 UTC (permalink / raw)
  To: Jaedon Shin, Nicolas Saenz Julienne, bcm-kernel-feedback-list,
	Jim Quinlan
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci



On 2/12/2020 6:59 PM, Jaedon Shin wrote:
> devm_clk_get* APIs are device managed and get freed automatically when
> the device detaches. so there is no reason to explicitly call clk_put()
> in probe or remove functions.
> 
> Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>

Fixes: c0452137034b ("PCI: brcmstb: Add Broadcom STB PCIe host
controller driver")
Acked-by: Florian Fainelli <f.fainelli@gmail.com>


> ---
>  drivers/pci/controller/pcie-brcmstb.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 0e0ca39a680b..3e48d9e238bb 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -972,7 +972,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>  	brcm_pcie_turn_off(pcie);
>  	brcm_pcie_regulator_disable(pcie);
>  	clk_disable_unprepare(pcie->clk);
> -	clk_put(pcie->clk);
>  }
>  
>  static int brcm_pcie_remove(struct platform_device *pdev)
> 

-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support
  2020-02-13  3:54 ` [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Florian Fainelli
@ 2020-02-13  5:15   ` Jaedon Shin
  2020-02-13 15:54     ` Jim Quinlan
  0 siblings, 1 reply; 17+ messages in thread
From: Jaedon Shin @ 2020-02-13  5:15 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Nicolas Saenz Julienne, bcm-kernel-feedback-list, Jim Quinlan,
	Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci


> 2020. 2. 13. 오후 12:54, Florian Fainelli <f.fainelli@gmail.com> 작성:
> 
> +Jim,
> 
> On 2/12/2020 6:59 PM, Jaedon Shin wrote:
>> This series enables the ARM based Broadcom STB SoCs and supports GPIO
>> based regulators for its power supplies. and this has an improvement on
>> devm_ APIS.
> 
> Which ARM-based SoCs did you try this on? We still have an issue with
> the multiple dma-ranges that must be handled to support 7445 with memory
> in the extension regions as well as 7278 and 7216.


I'm using BCM72604 which has only one memc. and device is Qualcomm Atheros
QCA6174 Wi-Fi module.

> 
> See comments in specific patches.
> 
>> 
>> Jaedon Shin (3):
>>  PCI: brcmstb: Enable ARCH_BRCMSTB
>>  PCI: brcmstb: Add regulator support
>>  PCI: brcmstb: Drop clk_put when probe fails and remove
>> 
>> .../bindings/pci/brcm,stb-pcie.yaml           |  8 +-
>> drivers/gpio/gpio-brcmstb.c                   | 13 +++-
>> drivers/pci/controller/Kconfig                |  2 +-
>> drivers/pci/controller/pcie-brcmstb.c         | 78 ++++++++++++++++++-
>> 4 files changed, 97 insertions(+), 4 deletions(-)
>> 
> 
> -- 
> Florian


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] PCI: brcmstb: Add regulator support
  2020-02-13  2:59 ` [PATCH 2/3] PCI: brcmstb: Add regulator support Jaedon Shin
  2020-02-13  3:58   ` Florian Fainelli
@ 2020-02-13 15:25   ` Jim Quinlan
  2020-02-14 10:06   ` Linus Walleij
  2020-02-14 11:01   ` Nicolas Saenz Julienne
  3 siblings, 0 replies; 17+ messages in thread
From: Jim Quinlan @ 2020-02-13 15:25 UTC (permalink / raw)
  To: Jaedon Shin
  Cc: Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Lorenzo Pieralisi, Andrew Murray, Gregory Fong,
	Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-arm-kernel,
	linux-pci

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
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support
  2020-02-13  5:15   ` Jaedon Shin
@ 2020-02-13 15:54     ` Jim Quinlan
  2020-02-14  2:16       ` Jaedon Shin
  0 siblings, 1 reply; 17+ messages in thread
From: Jim Quinlan @ 2020-02-13 15:54 UTC (permalink / raw)
  To: Jaedon Shin
  Cc: Florian Fainelli, Nicolas Saenz Julienne,
	bcm-kernel-feedback-list, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Lorenzo Pieralisi, Andrew Murray, Gregory Fong,
	Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-arm-kernel,
	linux-pci

On Thu, Feb 13, 2020 at 12:15 AM Jaedon Shin <jaedon.shin@gmail.com> wrote:
>
>
> > 2020. 2. 13. 오후 12:54, Florian Fainelli <f.fainelli@gmail.com> 작성:
> >
> > +Jim,
> >
> > On 2/12/2020 6:59 PM, Jaedon Shin wrote:
> >> This series enables the ARM based Broadcom STB SoCs and supports GPIO
> >> based regulators for its power supplies. and this has an improvement on
> >> devm_ APIS.
> >
> > Which ARM-based SoCs did you try this on? We still have an issue with
> > the multiple dma-ranges that must be handled to support 7445 with memory
> > in the extension regions as well as 7278 and 7216.
>
>
> I'm using BCM72604 which has only one memc. and device is Qualcomm Atheros
> QCA6174 Wi-Fi module.
>
You apparently have a simple enough memory configuration such that the
current upstream driver will work.  But this will not work for a
7445-based chip or other BCM7xxx chips that either have a memory
region extension or a non-zero system memory offset.

IIRC, similar regulator code was submitted before as part of the
complete driver that could handle our curious DMA situation, but was
rejected because someone objected to us using a  generic list of
regulators without specific explanation for each in the device tree
documentations.  I hope you have better luck :-)

Thanks,
Jim
> >
> > See comments in specific patches.
> >
> >>
> >> Jaedon Shin (3):
> >>  PCI: brcmstb: Enable ARCH_BRCMSTB
> >>  PCI: brcmstb: Add regulator support
> >>  PCI: brcmstb: Drop clk_put when probe fails and remove
> >>
> >> .../bindings/pci/brcm,stb-pcie.yaml           |  8 +-
> >> drivers/gpio/gpio-brcmstb.c                   | 13 +++-
> >> drivers/pci/controller/Kconfig                |  2 +-
> >> drivers/pci/controller/pcie-brcmstb.c         | 78 ++++++++++++++++++-
> >> 4 files changed, 97 insertions(+), 4 deletions(-)
> >>
> >
> > --
> > Florian
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support
  2020-02-13 15:54     ` Jim Quinlan
@ 2020-02-14  2:16       ` Jaedon Shin
  0 siblings, 0 replies; 17+ messages in thread
From: Jaedon Shin @ 2020-02-14  2:16 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Florian Fainelli, Nicolas Saenz Julienne,
	bcm-kernel-feedback-list, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Lorenzo Pieralisi, Andrew Murray, Gregory Fong,
	Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-arm-kernel,
	linux-pci



> 2020. 2. 14. 오전 12:54, Jim Quinlan <james.quinlan@Broadcom.com> 작성:
> 
> On Thu, Feb 13, 2020 at 12:15 AM Jaedon Shin <jaedon.shin@gmail.com> wrote:
>> 
>> 
>>> 2020. 2. 13. 오후 12:54, Florian Fainelli <f.fainelli@gmail.com> 작성:
>>> 
>>> +Jim,
>>> 
>>> On 2/12/2020 6:59 PM, Jaedon Shin wrote:
>>>> This series enables the ARM based Broadcom STB SoCs and supports GPIO
>>>> based regulators for its power supplies. and this has an improvement on
>>>> devm_ APIS.
>>> 
>>> Which ARM-based SoCs did you try this on? We still have an issue with
>>> the multiple dma-ranges that must be handled to support 7445 with memory
>>> in the extension regions as well as 7278 and 7216.
>> 
>> 
>> I'm using BCM72604 which has only one memc. and device is Qualcomm Atheros
>> QCA6174 Wi-Fi module.
>> 
> You apparently have a simple enough memory configuration such that the
> current upstream driver will work.  But this will not work for a
> 7445-based chip or other BCM7xxx chips that either have a memory
> region extension or a non-zero system memory offset.

I fully agree. It's not yet ready to enable ARM based BCM7445 SoCs. We'd better
not add ARCH_BRCMSTB and "brcm,bcm7445-pcie" string until the multi dma-
range problem is solved.

> 
> IIRC, similar regulator code was submitted before as part of the
> complete driver that could handle our curious DMA situation, but was
> rejected because someone objected to us using a  generic list of
> regulators without specific explanation for each in the device tree
> documentations.  I hope you have better luck :-)

I'll refer to.

Thanks,
Jaedon

> 
> Thanks,
> Jim
>>> 
>>> See comments in specific patches.
>>> 
>>>> 
>>>> Jaedon Shin (3):
>>>> PCI: brcmstb: Enable ARCH_BRCMSTB
>>>> PCI: brcmstb: Add regulator support
>>>> PCI: brcmstb: Drop clk_put when probe fails and remove
>>>> 
>>>> .../bindings/pci/brcm,stb-pcie.yaml           |  8 +-
>>>> drivers/gpio/gpio-brcmstb.c                   | 13 +++-
>>>> drivers/pci/controller/Kconfig                |  2 +-
>>>> drivers/pci/controller/pcie-brcmstb.c         | 78 ++++++++++++++++++-
>>>> 4 files changed, 97 insertions(+), 4 deletions(-)
>>>> 
>>> 
>>> --
>>> Florian


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] PCI: brcmstb: Add regulator support
  2020-02-13  2:59 ` [PATCH 2/3] PCI: brcmstb: Add regulator support Jaedon Shin
  2020-02-13  3:58   ` Florian Fainelli
  2020-02-13 15:25   ` Jim Quinlan
@ 2020-02-14 10:06   ` Linus Walleij
  2020-02-14 11:52     ` Mark Brown
  2020-02-14 11:01   ` Nicolas Saenz Julienne
  3 siblings, 1 reply; 17+ messages in thread
From: Linus Walleij @ 2020-02-14 10:06 UTC (permalink / raw)
  To: Jaedon Shin, Mark Brown
  Cc: Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Lorenzo Pieralisi, Andrew Murray, Gregory Fong,
	Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Linux ARM,
	linux-pci

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] PCI: brcmstb: Drop clk_put when probe fails and remove
  2020-02-13  2:59 ` [PATCH 3/3] PCI: brcmstb: Drop clk_put when probe fails and remove Jaedon Shin
  2020-02-13  4:01   ` Florian Fainelli
@ 2020-02-14 10:55   ` Nicolas Saenz Julienne
  1 sibling, 0 replies; 17+ messages in thread
From: Nicolas Saenz Julienne @ 2020-02-14 10:55 UTC (permalink / raw)
  To: Jaedon Shin, Florian Fainelli, bcm-kernel-feedback-list
  Cc: Mark Rutland, Lorenzo Pieralisi, linux-gpio, linux-pci,
	Linus Walleij, Jaedon Shin, Bartosz Golaszewski, Rob Herring,
	Gregory Fong, Bjorn Helgaas, linux-arm-kernel, Andrew Murray

On Thu Feb 13, 2020 at 11:59 AM, Jaedon Shin wrote:
> devm_clk_get* APIs are device managed and get freed automatically when
> the device detaches. so there is no reason to explicitly call clk_put()
> in probe or remove functions.
>
> Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
> ---

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!

> drivers/pci/controller/pcie-brcmstb.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c
> b/drivers/pci/controller/pcie-brcmstb.c
> index 0e0ca39a680b..3e48d9e238bb 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -972,7 +972,6 @@ static void __brcm_pcie_remove(struct brcm_pcie
> *pcie)
> brcm_pcie_turn_off(pcie);
> brcm_pcie_regulator_disable(pcie);
> clk_disable_unprepare(pcie->clk);
> - clk_put(pcie->clk);
> }
>  
> static int brcm_pcie_remove(struct platform_device *pdev)
> --
> 2.21.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] PCI: brcmstb: Add regulator support
  2020-02-13  2:59 ` [PATCH 2/3] PCI: brcmstb: Add regulator support Jaedon Shin
                     ` (2 preceding siblings ...)
  2020-02-14 10:06   ` Linus Walleij
@ 2020-02-14 11:01   ` Nicolas Saenz Julienne
  3 siblings, 0 replies; 17+ messages in thread
From: Nicolas Saenz Julienne @ 2020-02-14 11:01 UTC (permalink / raw)
  To: Jaedon Shin, Florian Fainelli, bcm-kernel-feedback-list
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Lorenzo Pieralisi,
	Andrew Murray, Gregory Fong, Linus Walleij, Bartosz Golaszewski,
	linux-gpio, linux-arm-kernel, linux-pci, Jaedon Shin

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] PCI: brcmstb: Add regulator support
  2020-02-14 10:06   ` Linus Walleij
@ 2020-02-14 11:52     ` Mark Brown
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Brown @ 2020-02-14 11:52 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Jaedon Shin, Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Lorenzo Pieralisi, Andrew Murray, Gregory Fong,
	Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Linux ARM,
	linux-pci

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] PCI: brcmstb: Add regulator support
  2020-02-13  3:58   ` Florian Fainelli
@ 2020-02-20 11:07     ` Gregory Fong
  0 siblings, 0 replies; 17+ messages in thread
From: Gregory Fong @ 2020-02-20 11:07 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Jaedon Shin, Nicolas Saenz Julienne, bcm-kernel-feedback-list,
	Jim Quinlan, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Lorenzo Pieralisi, Andrew Murray, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, linux-arm-kernel, linux-pci

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13  2:59 [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Jaedon Shin
2020-02-13  2:59 ` [PATCH 1/3] PCI: brcmstb: Enable ARCH_BRCMSTB Jaedon Shin
2020-02-13  3:58   ` Florian Fainelli
2020-02-13  2:59 ` [PATCH 2/3] PCI: brcmstb: Add regulator support Jaedon Shin
2020-02-13  3:58   ` Florian Fainelli
2020-02-20 11:07     ` Gregory Fong
2020-02-13 15:25   ` Jim Quinlan
2020-02-14 10:06   ` Linus Walleij
2020-02-14 11:52     ` Mark Brown
2020-02-14 11:01   ` Nicolas Saenz Julienne
2020-02-13  2:59 ` [PATCH 3/3] PCI: brcmstb: Drop clk_put when probe fails and remove Jaedon Shin
2020-02-13  4:01   ` Florian Fainelli
2020-02-14 10:55   ` Nicolas Saenz Julienne
2020-02-13  3:54 ` [PATCH 0/3] PCI: brcmstb: Add Broadcom STB support Florian Fainelli
2020-02-13  5:15   ` Jaedon Shin
2020-02-13 15:54     ` Jim Quinlan
2020-02-14  2:16       ` Jaedon Shin

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git