[PATCH v4 0/2] PCI: mediatek-gen3: Support controlling power supplies

[PATCH v4 0/2] PCI: mediatek-gen3: Support controlling power supplies

[Jian Yang]

Add support for controlling power supplies and reset GPIO of a downstream
component in Mediatek's PCIe GEN3 controller driver.

Changes in v4:
1. Rename power supplies properties in DT binding and driver.
2. Reorder variables alphabetically.
3. Use 'dev_err_probe' to do some error handling stuff.

Changes in v3:
1. Modify description of power supply properties in DT binding.
2. Remove unused header files.
3. Use 'device_wakeup_path' to determine whether the downstream component
needs to skip the reset process in system suspend scenarios.

Changes in v2:
1. Remove an unnecessary property in dt-bindings file.
2. Use the flag 'GPIOD_OUT_LOW' to set initial state of a downstream
component's reset GPIO.
3. Keep downstream component powered on in suspend state if it is capable
of waking up the system.

jian.yang (2):
  dt-bindings: PCI: mediatek-gen3: Add support for controlling power and
    reset
  PCI: mediatek-gen3: Add power and reset control feature for downstream
    component

 .../bindings/pci/mediatek-pcie-gen3.yaml      | 30 +++++++
 drivers/pci/controller/pcie-mediatek-gen3.c   | 89 ++++++++++++++++++-
 2 files changed, 118 insertions(+), 1 deletion(-)

-- 
2.18.0

[PATCH v4 1/2] dt-bindings: PCI: mediatek-gen3: Add support for controlling power and reset

[Jian Yang]

From: "jian.yang" <jian.yang@mediatek.com>

Add new properties to support control power supplies and reset pin of
a downstream component.

Signed-off-by: jian.yang <jian.yang@mediatek.com>
---
 .../bindings/pci/mediatek-pcie-gen3.yaml      | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
index 7e8c7a2a5f9b..a4f6b48d57fa 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -84,6 +84,26 @@ properties:
     items:
       enum: [ phy, mac ]
 
+  vpcie1v8-supply:
+    description:
+      The regulator phandle that provides 1.8V power from root port to a
+      downstream component.
+
+  vpcie3v3-supply:
+    description:
+      The regulator phandle that provides 3.3V power from root port to a
+      downstream component.
+
+  vpcie12v-supply:
+    description:
+      The regulator phandle that provides 12V power from root port to a
+      downstream component.
+
+  dsc-reset-gpios:
+    description:
+      The extra reset pin other than PERST# required by a downstream component.
+    maxItems: 1
+
   clocks:
     minItems: 4
     maxItems: 6
@@ -238,5 +258,15 @@ examples:
                       #interrupt-cells = <1>;
                       interrupt-controller;
             };
+
+            pcie@0 {
+                device_type = "pci";
+                reg = <0x0 0x0 0x0 0x0 0x0>;
+                vpcie3v3-supply = <&pcie3v3_regulator>;
+
+                #address-cells = <3>;
+                #size-cells = <2>;
+                ranges;
+            };
         };
     };
-- 
2.18.0

[PATCH v4 2/2] PCI: mediatek-gen3: Add power and reset control feature for downstream component

[Jian Yang]

From: "jian.yang" <jian.yang@mediatek.com>

Make MediaTek's controller driver capable of controlling power
supplies and reset pin of a downstream component in power-on and
power-off process.

Some downstream components (e.g., a WIFI chip) may need an extra
reset other than PERST# and their power supplies, depending on
the requirements of platform, may need to controlled by their
parent's driver. To meet the requirements described above, I add this
feature to MediaTek's PCIe controller driver as an optional feature.

Signed-off-by: jian.yang <jian.yang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 89 ++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index e0e27645fdf4..51d30331db5a 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -8,6 +8,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/iopoll.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
@@ -20,6 +21,8 @@
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeup.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 
 #include "../pci.h"
@@ -100,6 +103,13 @@
 #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
 #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
 
+/* Downstream Component power supplies used by MediaTek PCIe */
+static const char *const dsc_power_supplies[] = {
+	"vpcie1v8",
+	"vpcie3v3",
+	"vpcie12v",
+};
+
 /**
  * struct mtk_msi_set - MSI information for each set
  * @base: IO mapped register base
@@ -122,6 +132,9 @@ struct mtk_msi_set {
  * @phy: PHY controller block
  * @clks: PCIe clocks
  * @num_clks: PCIe clocks count for this port
+ * @supplies: Downstream Component power supplies
+ * @num_supplies: Downstream Component power supplies count
+ * @dsc_reset: The GPIO pin to reset Downstream component
  * @irq: PCIe controller interrupt number
  * @saved_irq_state: IRQ enable state saved at suspend time
  * @irq_lock: lock protecting IRQ register access
@@ -141,6 +154,9 @@ struct mtk_gen3_pcie {
 	struct phy *phy;
 	struct clk_bulk_data *clks;
 	int num_clks;
+	struct regulator_bulk_data *supplies;
+	int num_supplies;
+	struct gpio_desc *dsc_reset;
 
 	int irq;
 	u32 saved_irq_state;
@@ -763,7 +779,7 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct resource *regs;
-	int ret;
+	int i, ret;
 
 	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
 	if (!regs)
@@ -809,14 +825,82 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 		return pcie->num_clks;
 	}
 
+	pcie->num_supplies = ARRAY_SIZE(dsc_power_supplies);
+	pcie->supplies = devm_kcalloc(dev, pcie->num_supplies,
+				      sizeof(*pcie->supplies),
+				      GFP_KERNEL);
+	if (!pcie->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < pcie->num_supplies; i++)
+		pcie->supplies[i].supply = dsc_power_supplies[i];
+
+	ret = devm_regulator_bulk_get(dev, pcie->num_supplies, pcie->supplies);
+	if (ret)
+		return ret;
+
+	pcie->dsc_reset = devm_gpiod_get_optional(dev, "dsc-reset",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(pcie->dsc_reset))
+		return dev_err_probe(dev, PTR_ERR(pcie->dsc_reset),
+				     "failed to request DSC reset gpio\n");
+
 	return 0;
 }
 
+static int mtk_pcie_dsc_power_up(struct mtk_gen3_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	int ret;
+
+	/*
+	 * Skip powering up the downstream component if it was kept powered on
+	 * while system entered suspend state
+	 */
+	if (device_wakeup_path(dev))
+		return 0;
+
+	/* Assert Downstream Component reset */
+	if (pcie->dsc_reset)
+		gpiod_set_value_cansleep(pcie->dsc_reset, 1);
+
+	ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
+	if (ret)
+		dev_err(dev, "failed to enable DSC power supplies: %d\n", ret);
+
+	/* De-assert Downstream Component reset */
+	if (pcie->dsc_reset)
+		gpiod_set_value_cansleep(pcie->dsc_reset, 0);
+
+	return ret;
+}
+
+static void mtk_pcie_dsc_power_down(struct mtk_gen3_pcie *pcie)
+{
+	/*
+	 * Keep downstream component powered on if it is capable of waking up
+	 * the system in suspend state
+	 */
+	if (device_wakeup_path(pcie->dev))
+		return;
+
+	/* Assert Downstream Component reset */
+	if (pcie->dsc_reset)
+		gpiod_set_value_cansleep(pcie->dsc_reset, 1);
+
+	regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
+}
+
 static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 	int err;
 
+	/* Downstream Component power up before RC */
+	err = mtk_pcie_dsc_power_up(pcie);
+	if (err)
+		return err;
+
 	/* PHY power on and enable pipe clock */
 	reset_control_deassert(pcie->phy_reset);
 
@@ -855,6 +939,7 @@ static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
 	phy_exit(pcie->phy);
 err_phy_init:
 	reset_control_assert(pcie->phy_reset);
+	mtk_pcie_dsc_power_down(pcie);
 
 	return err;
 }
@@ -870,6 +955,8 @@ static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie)
 	phy_power_off(pcie->phy);
 	phy_exit(pcie->phy);
 	reset_control_assert(pcie->phy_reset);
+
+	mtk_pcie_dsc_power_down(pcie);
 }
 
 static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
-- 
2.18.0

Re: [PATCH v4 1/2] dt-bindings: PCI: mediatek-gen3: Add support for controlling power and reset

[Krzysztof Kozlowski]

On 06/11/2023 07:12, Jian Yang wrote:
> From: "jian.yang" <jian.yang@mediatek.com>
> 
> Add new properties to support control power supplies and reset pin of
> a downstream component.
> 
> Signed-off-by: jian.yang <jian.yang@mediatek.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

> ---
>  .../bindings/pci/mediatek-pcie-gen3.yaml      | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> index 7e8c7a2a5f9b..a4f6b48d57fa 100644
> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -84,6 +84,26 @@ properties:
>      items:
>        enum: [ phy, mac ]
>  
> +  vpcie1v8-supply:
> +    description:
> +      The regulator phandle that provides 1.8V power from root port to a
> +      downstream component.
> +
> +  vpcie3v3-supply:
> +    description:
> +      The regulator phandle that provides 3.3V power from root port to a
> +      downstream component.
> +
> +  vpcie12v-supply:
> +    description:
> +      The regulator phandle that provides 12V power from root port to a
> +      downstream component.
> +
> +  dsc-reset-gpios:
> +    description:
> +      The extra reset pin other than PERST# required by a downstream component.
> +    maxItems: 1

How did you implement Rob's feedback? Or did you just ignore it?

This does not look like property of the controller. Aren't you now
trying to implement power-sequencing of devices via properties of host
controller?

Best regards,
Krzysztof

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add power and reset control feature for downstream component

[Krzysztof Kozlowski]

On 06/11/2023 07:12, Jian Yang wrote:
> From: "jian.yang" <jian.yang@mediatek.com>
> 
> Make MediaTek's controller driver capable of controlling power
> supplies and reset pin of a downstream component in power-on and
> power-off process.
> 
> Some downstream components (e.g., a WIFI chip) may need an extra
> reset other than PERST# and their power supplies, depending on
> the requirements of platform, may need to controlled by their
> parent's driver. To meet the requirements described above, I add this
> feature to MediaTek's PCIe controller driver as an optional feature.

NAK, strong NAK. This should be done in a generic way because nothing
here is specific to Mediatek.

You just implement power sequencing of devices through quirks specific
to one controller.

Work with others to provide common solution.
https://lpc.events/event/17/contributions/1507/

Best regards,
Krzysztof

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add power and reset control feature for downstream component

[AngeloGioacchino Del Regno]

Il 06/11/23 07:12, Jian Yang ha scritto:
> From: "jian.yang" <jian.yang@mediatek.com>
> 
> Make MediaTek's controller driver capable of controlling power
> supplies and reset pin of a downstream component in power-on and
> power-off process.
> 
> Some downstream components (e.g., a WIFI chip) may need an extra
> reset other than PERST# and their power supplies, depending on
> the requirements of platform, may need to controlled by their
> parent's driver. To meet the requirements described above, I add this
> feature to MediaTek's PCIe controller driver as an optional feature.
> 
> Signed-off-by: jian.yang <jian.yang@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v4 1/2] dt-bindings: PCI: mediatek-gen3: Add support for controlling power and reset

[AngeloGioacchino Del Regno]

Il 06/11/23 07:12, Jian Yang ha scritto:
> From: "jian.yang" <jian.yang@mediatek.com>
> 
> Add new properties to support control power supplies and reset pin of
> a downstream component.
> 
> Signed-off-by: jian.yang <jian.yang@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add power and reset control feature for downstream component

[AngeloGioacchino Del Regno]

Il 06/11/23 08:53, Krzysztof Kozlowski ha scritto:
> On 06/11/2023 07:12, Jian Yang wrote:
>> From: "jian.yang" <jian.yang@mediatek.com>
>>
>> Make MediaTek's controller driver capable of controlling power
>> supplies and reset pin of a downstream component in power-on and
>> power-off process.
>>
>> Some downstream components (e.g., a WIFI chip) may need an extra
>> reset other than PERST# and their power supplies, depending on
>> the requirements of platform, may need to controlled by their
>> parent's driver. To meet the requirements described above, I add this
>> feature to MediaTek's PCIe controller driver as an optional feature.
> 
> NAK, strong NAK. This should be done in a generic way because nothing
> here is specific to Mediatek.
> 
> You just implement power sequencing of devices through quirks specific
> to one controller.
> 
> Work with others to provide common solution.
> https://lpc.events/event/17/contributions/1507/
> 

I agree that working with everyone else by adding pwrseq is a must, but other
other PCIe controllers are doing the exact same as this patch: if the supply
and gpio names are aligned with the others, why shouldn't we let this in and
then convert this driver, along with the others, to the new pwrseq subsystem
when it's ready?

That, because I expect the pwrseq to require a bit more time before being
ready to get upstream.

P.S.: Check Tegra, Broadcom, RockChip DW, IMX6Q-pcie.

Cheers,
Angelo

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add power and reset control feature for downstream component

[Krzysztof Kozlowski]

On 06/11/2023 09:36, AngeloGioacchino Del Regno wrote:
> Il 06/11/23 08:53, Krzysztof Kozlowski ha scritto:
>> On 06/11/2023 07:12, Jian Yang wrote:
>>> From: "jian.yang" <jian.yang@mediatek.com>
>>>
>>> Make MediaTek's controller driver capable of controlling power
>>> supplies and reset pin of a downstream component in power-on and
>>> power-off process.
>>>
>>> Some downstream components (e.g., a WIFI chip) may need an extra
>>> reset other than PERST# and their power supplies, depending on
>>> the requirements of platform, may need to controlled by their
>>> parent's driver. To meet the requirements described above, I add this
>>> feature to MediaTek's PCIe controller driver as an optional feature.
>>
>> NAK, strong NAK. This should be done in a generic way because nothing
>> here is specific to Mediatek.
>>
>> You just implement power sequencing of devices through quirks specific
>> to one controller.
>>
>> Work with others to provide common solution.
>> https://lpc.events/event/17/contributions/1507/
>>
> 
> I agree that working with everyone else by adding pwrseq is a must, but other
> other PCIe controllers are doing the exact same as this patch: if the supply
> and gpio names are aligned with the others, why shouldn't we let this in and
> then convert this driver, along with the others, to the new pwrseq subsystem
> when it's ready?

Because you already push to the PCI controller bindings new properties
which are not properties of the PCI controller.

> 
> That, because I expect the pwrseq to require a bit more time before being
> ready to get upstream.
> 
> P.S.: Check Tegra, Broadcom, RockChip DW, IMX6Q-pcie.

Every new hack will not make it faster. :( At some point one have to say
- enough of hacks, start doing it properly with upstream.

Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: PCI: mediatek-gen3: Add support for controlling power and reset

[Krzysztof Kozlowski]

On 06/11/2023 07:12, Jian Yang wrote:
> From: "jian.yang" <jian.yang@mediatek.com>
> 
> Add new properties to support control power supplies and reset pin of
> a downstream component.
> 
> Signed-off-by: jian.yang <jian.yang@mediatek.com>
> ---
>  .../bindings/pci/mediatek-pcie-gen3.yaml      | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> index 7e8c7a2a5f9b..a4f6b48d57fa 100644
> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -84,6 +84,26 @@ properties:
>      items:
>        enum: [ phy, mac ]
>  
> +  vpcie1v8-supply:
> +    description:
> +      The regulator phandle that provides 1.8V power from root port to a
> +      downstream component.
> +
> +  vpcie3v3-supply:
> +    description:
> +      The regulator phandle that provides 3.3V power from root port to a
> +      downstream component.

How 3.3V supply can go from root port to downstream? Do you mean that
root port is the regulator itself (regulator provider)?

Sorry, all these supplies look like hacks - stuffing PCI device
properties into the PCI controller node.

Best regards,
Krzysztof

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add power and reset control feature for downstream component

[AngeloGioacchino Del Regno]

Il 06/11/23 09:46, Krzysztof Kozlowski ha scritto:
> On 06/11/2023 09:36, AngeloGioacchino Del Regno wrote:
>> Il 06/11/23 08:53, Krzysztof Kozlowski ha scritto:
>>> On 06/11/2023 07:12, Jian Yang wrote:
>>>> From: "jian.yang" <jian.yang@mediatek.com>
>>>>
>>>> Make MediaTek's controller driver capable of controlling power
>>>> supplies and reset pin of a downstream component in power-on and
>>>> power-off process.
>>>>
>>>> Some downstream components (e.g., a WIFI chip) may need an extra
>>>> reset other than PERST# and their power supplies, depending on
>>>> the requirements of platform, may need to controlled by their
>>>> parent's driver. To meet the requirements described above, I add this
>>>> feature to MediaTek's PCIe controller driver as an optional feature.
>>>
>>> NAK, strong NAK. This should be done in a generic way because nothing
>>> here is specific to Mediatek.
>>>
>>> You just implement power sequencing of devices through quirks specific
>>> to one controller.
>>>
>>> Work with others to provide common solution.
>>> https://lpc.events/event/17/contributions/1507/
>>>
>>
>> I agree that working with everyone else by adding pwrseq is a must, but other
>> other PCIe controllers are doing the exact same as this patch: if the supply
>> and gpio names are aligned with the others, why shouldn't we let this in and
>> then convert this driver, along with the others, to the new pwrseq subsystem
>> when it's ready?
> 
> Because you already push to the PCI controller bindings new properties
> which are not properties of the PCI controller.
> 
>>
>> That, because I expect the pwrseq to require a bit more time before being
>> ready to get upstream.
>>
>> P.S.: Check Tegra, Broadcom, RockChip DW, IMX6Q-pcie.
> 
> Every new hack will not make it faster. :( At some point one have to say
> - enough of hacks, start doing it properly with upstream.
> 

Eh, that's a fair point. I can't really disagree with that.

Cheers,
Angelo