[PATCH 0/2] PCI: mediatek-gen3: Support controlling power and reset of downstream componennt

[PATCH 0/2] PCI: mediatek-gen3: Support controlling power and reset of downstream componennt

[Jian Yang]

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

These series patches add support for controlling power supplies and reset
GPIO of a downstream component in MediaTek's PCIe GEN3 controller driver.

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

 .../bindings/pci/mediatek-pcie-gen3.yaml      |  23 ++++
 drivers/pci/controller/pcie-mediatek-gen3.c   | 114 +++++++++++++++++-
 2 files changed, 136 insertions(+), 1 deletion(-)

-- 
2.18.0

[PATCH 1/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 flow.

Some downstream components (e.g., a WIFI chip) may need an extra
reset other than of 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 a optional feature.

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

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index b8612ce5f4d0..5ddbc2a0c020 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -8,6 +8,8 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/iopoll.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
@@ -15,11 +17,13 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/msi.h>
+#include <linux/of_gpio.h>
 #include <linux/pci.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 
 #include "../pci.h"
@@ -100,6 +104,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[] = {
+	"pcie1v8",
+	"pcie3v3",
+	"pcie12v",
+};
+
 /**
  * struct mtk_msi_set - MSI information for each set
  * @base: IO mapped register base
@@ -122,6 +133,10 @@ 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
+ * @dsc_reset_delay_ms: Delay in ms before the deassertion of reset GPIO
  * @irq: PCIe controller interrupt number
  * @saved_irq_state: IRQ enable state saved at suspend time
  * @irq_lock: lock protecting IRQ register access
@@ -141,6 +156,10 @@ 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;
+	u32 dsc_reset_delay_ms;
 
 	int irq;
 	u32 saved_irq_state;
@@ -763,7 +782,9 @@ 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;
+	enum of_gpio_flags flags;
+	enum gpiod_flags dsc_reset_init_flags;
+	int ret, i;
 
 	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
 	if (!regs)
@@ -809,14 +830,103 @@ 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;
+
+	ret = of_get_named_gpio_flags(dev->of_node, "dsc-reset-gpios", 0,
+				      &flags);
+	if (ret < 0) {
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		/*
+		 * It's okay that the reset GPIO of a downstream component not
+		 * defined in related devicetree node since it's an optional
+		 * property.
+		 */
+		return 0;
+	}
+
+	dsc_reset_init_flags = (flags & OF_GPIO_ACTIVE_LOW) ? GPIOD_OUT_HIGH :
+			       GPIOD_OUT_LOW;
+	pcie->dsc_reset = devm_gpiod_get_optional(dev, "dsc-reset",
+						  dsc_reset_init_flags);
+	if (IS_ERR(pcie->dsc_reset)) {
+		ret = PTR_ERR(pcie->dsc_reset);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to request DSC reset gpio\n");
+
+		return ret;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "dsc-reset-msleep",
+				   &pcie->dsc_reset_delay_ms);
+	if (ret) {
+		dev_info(dev, "Failed to get delay time of DSC, set it to default 5ms\n");
+		pcie->dsc_reset_delay_ms = 5;
+	}
+
 	return 0;
 }
 
+static int mtk_pcie_dsc_power_up(struct mtk_gen3_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	int ret;
+
+	/* 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) {
+		/*
+		 * Wait for a short time before we de-assert the reset GPIO.
+		 * Depends on the requirement of a specific Downstream
+		 * Component.
+		 */
+		usleep_range(1000 * pcie->dsc_reset_delay_ms,
+			     1000 * pcie->dsc_reset_delay_ms + 100);
+		gpiod_set_value_cansleep(pcie->dsc_reset, 0);
+	}
+
+	return ret;
+}
+
+static void mtk_pcie_dsc_power_down(struct mtk_gen3_pcie *pcie)
+{
+	/* 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 +965,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 +981,7 @@ 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

[PATCH 2/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      | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
index 7e8c7a2a5f9b..46149cc63989 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -84,6 +84,29 @@ properties:
     items:
       enum: [ phy, mac ]
 
+  pcie1v8-supply:
+    description:
+      The regulator phandle that provides 1.8V power to downstream component.
+
+  pcie3v3-supply:
+    description:
+      The regulator phandle that provides 3.3V power to downstream component.
+
+  pcie12v-supply:
+    description:
+      The regulator phandle that provides 12V power to downstream component.
+
+  dsc-reset-gpios:
+    description:
+      The reset GPIO of a downstream component.
+    maxItems: 1
+
+  dsc-reset-msleep:
+    description:
+      The delay time between assertion and de-assertion of a downstream
+      component's reset GPIO.
+    maxItems: 1
+
   clocks:
     minItems: 4
     maxItems: 6
-- 
2.18.0

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

[Krzysztof Kozlowski]

On 11/01/2023 04:28, 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.  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      | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> index 7e8c7a2a5f9b..46149cc63989 100644
> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -84,6 +84,29 @@ properties:
>      items:
>        enum: [ phy, mac ]
>  
> +  pcie1v8-supply:
> +    description:
> +      The regulator phandle that provides 1.8V power to downstream component.
> +
> +  pcie3v3-supply:
> +    description:
> +      The regulator phandle that provides 3.3V power to downstream component.
> +
> +  pcie12v-supply:
> +    description:
> +      The regulator phandle that provides 12V power to downstream component.
> +
> +  dsc-reset-gpios:
> +    description:
> +      The reset GPIO of a downstream component.

Why you cannot use standard reset-gpios property?

> +    maxItems: 1
> +
> +  dsc-reset-msleep:

Wrong property unit/suffix.

> +    description:
> +      The delay time between assertion and de-assertion of a downstream
> +      component's reset GPIO.

Why this should be a property of DT?

> +    maxItems: 1

maxItems of what?

> +
>    clocks:
>      minItems: 4
>      maxItems: 6

Best regards,
Krzysztof

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

[Jian Yang (杨戬)]

Dear Krzysztof,

Sorry for the late response and thanks for your comment.

On Wed, 2023-01-11 at 10:30 +0100, Krzysztof Kozlowski wrote:
> On 11/01/2023 04:28, 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.  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      | 23
> > +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-
> > gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-
> > gen3.yaml
> > index 7e8c7a2a5f9b..46149cc63989 100644
> > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > @@ -84,6 +84,29 @@ properties:
> >      items:
> >        enum: [ phy, mac ]
> >  
> > +  pcie1v8-supply:
> > +    description:
> > +      The regulator phandle that provides 1.8V power to downstream
> > component.
> > +
> > +  pcie3v3-supply:
> > +    description:
> > +      The regulator phandle that provides 3.3V power to downstream
> > component.
> > +
> > +  pcie12v-supply:
> > +    description:
> > +      The regulator phandle that provides 12V power to downstream
> > component.
> > +
> > +  dsc-reset-gpios:
> > +    description:
> > +      The reset GPIO of a downstream component.
> 
> Why you cannot use standard reset-gpios property?

The "dsc-reset-gpios" represents an extra reset pin other than PERST#
required by a PCIe downstream device. But the "reset-gpios", described
in "pci.txt", represents the PERST#. So I tend to add a new property to
meet this requirement.

> 
> > +    maxItems: 1
> > +
> > +  dsc-reset-msleep:
> 
> Wrong property unit/suffix.

Thanks for the correction. I will modify it in v2 patch.

> 
> > +    description:
> > +      The delay time between assertion and de-assertion of a
> > downstream
> > +      component's reset GPIO.
> 
> Why this should be a property of DT?

Same as the reason I described above. I suppose we need to add a
property to let user determine the delay time due to differences
in requirements between various devices.

> 
> > +    maxItems: 1
> 
> maxItems of what?

Seems unnecessary to add a "maxItems" here. Also I will remove it in v2
patch. Thanks a lot.

Best regards,
Jian Yang

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

[Krzysztof Kozlowski]

On 03/02/2023 10:38, Jian Yang (杨戬) wrote:
>>> +  pcie12v-supply:
>>> +    description:
>>> +      The regulator phandle that provides 12V power to downstream
>>> component.
>>> +
>>> +  dsc-reset-gpios:
>>> +    description:
>>> +      The reset GPIO of a downstream component.
>>
>> Why you cannot use standard reset-gpios property?
> 
> The "dsc-reset-gpios" represents an extra reset pin other than PERST#
> required by a PCIe downstream device. But the "reset-gpios", described
> in "pci.txt", represents the PERST#. So I tend to add a new property to
> meet this requirement.

OK

>>
>>> +    description:
>>> +      The delay time between assertion and de-assertion of a
>>> downstream
>>> +      component's reset GPIO.
>>
>> Why this should be a property of DT?
> 
> Same as the reason I described above. I suppose we need to add a
> property to let user determine the delay time due to differences
> in requirements between various devices.

No, I don't think we want individual properties like that. There is
ongoing discussion about this:
https://lore.kernel.org/all/20221214095342.937303-1-alexander.stein@ew.tq-group.com/

Feedback is welcomed - there. Don't create your own half-baked delays
for different hardware designs.

Best regards,
Krzysztof

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

[Jian Yang (杨戬)]

Dear Krzysztof,

On Fri, 2023-02-03 at 13:32 +0100, Krzysztof Kozlowski wrote:
> On 03/02/2023 10:38, Jian Yang (杨戬) wrote:
> > > > +  pcie12v-supply:
> > > > +    description:
> > > > +      The regulator phandle that provides 12V power to
> > > > downstream
> > > > component.
> > > > +
> > > > +  dsc-reset-gpios:
> > > > +    description:
> > > > +      The reset GPIO of a downstream component.
> > > 
> > > Why you cannot use standard reset-gpios property?
> > 
> > The "dsc-reset-gpios" represents an extra reset pin other than
> > PERST#
> > required by a PCIe downstream device. But the "reset-gpios",
> > described
> > in "pci.txt", represents the PERST#. So I tend to add a new
> > property to
> > meet this requirement.
> 
> OK
> 
> > > 
> > > > +    description:
> > > > +      The delay time between assertion and de-assertion of a
> > > > downstream
> > > > +      component's reset GPIO.
> > > 
> > > Why this should be a property of DT?
> > 
> > Same as the reason I described above. I suppose we need to add a
> > property to let user determine the delay time due to differences
> > in requirements between various devices.
> 
> No, I don't think we want individual properties like that. There is
> ongoing discussion about this:
> 
https://lore.kernel.org/all/20221214095342.937303-1-alexander.stein@ew.tq-group.com/
> 
> Feedback is welcomed - there. Don't create your own half-baked delays
> for different hardware designs.

Thanks for your helpful suggestion. I will remove that property of
delay-time in V2 patch and let the work-in-progress "GPIO delay" driver
to handle this requirement.

Best regards,
Jian Yang

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

[Rob Herring]

On Tue, Jan 10, 2023 at 9:28 PM Jian Yang <jian.yang@mediatek.com> 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      | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> index 7e8c7a2a5f9b..46149cc63989 100644
> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -84,6 +84,29 @@ properties:
>      items:
>        enum: [ phy, mac ]
>
> +  pcie1v8-supply:
> +    description:
> +      The regulator phandle that provides 1.8V power to downstream component.
> +
> +  pcie3v3-supply:
> +    description:
> +      The regulator phandle that provides 3.3V power to downstream component.
> +
> +  pcie12v-supply:
> +    description:
> +      The regulator phandle that provides 12V power to downstream component.

While in some bindings we've allowed these in the host bridge node,
that is a mistake. These should be in the root port node. You probably
don't have one in DT, so add one.

> +
> +  dsc-reset-gpios:
> +    description:
> +      The reset GPIO of a downstream component.
> +    maxItems: 1
> +
> +  dsc-reset-msleep:

Doesn't the PCI spec define this time? We're talking about PERST#, right?

> +    description:
> +      The delay time between assertion and de-assertion of a downstream
> +      component's reset GPIO.
> +    maxItems: 1
> +
>    clocks:
>      minItems: 4
>      maxItems: 6
> --
> 2.18.0
>

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

[Jian Yang (杨戬)]

Dear Rob,

Thanks for your comment.

On Fri, 2023-02-03 at 10:07 -0600, Rob Herring wrote:
> On Tue, Jan 10, 2023 at 9:28 PM Jian Yang <jian.yang@mediatek.com>
> 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      | 23
> > +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-
> > gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-
> > gen3.yaml
> > index 7e8c7a2a5f9b..46149cc63989 100644
> > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > @@ -84,6 +84,29 @@ properties:
> >      items:
> >        enum: [ phy, mac ]
> > 
> > +  pcie1v8-supply:
> > +    description:
> > +      The regulator phandle that provides 1.8V power to downstream
> > component.
> > +
> > +  pcie3v3-supply:
> > +    description:
> > +      The regulator phandle that provides 3.3V power to downstream
> > component.
> > +
> > +  pcie12v-supply:
> > +    description:
> > +      The regulator phandle that provides 12V power to downstream
> > component.
> 
> While in some bindings we've allowed these in the host bridge node,
> that is a mistake. These should be in the root port node. You
> probably
> don't have one in DT, so add one.

In Mediatek's PCIe structure, there is only one root port under a host
bridge. I am wondering if you think it's necessary to add a root port
node in our host bridge node based on that structure?

And I'm a bit confused about how to declare a property which should be
added in a root port node. I would be grateful if you could provide me
some good example about it.

> > +
> > +  dsc-reset-gpios:
> > +    description:
> > +      The reset GPIO of a downstream component.
> > +    maxItems: 1
> > +
> > +  dsc-reset-msleep:
> 
> Doesn't the PCI spec define this time? We're talking about PERST#,
> right?

The "dsc-reset-gpios" represents an extra reset pin other than PERST#
required by a downstream component. I tried to add a property here so
that users can control the delay time between the assertion and
deassertion according to their requirement, but Krzysztof mentioned
that there is an ongoing discussion about a "GPIO delay" driver can
handle this. So I will remove the "dsc-reset-msleep" in V2 patch.

> > +    description:
> > +      The delay time between assertion and de-assertion of a
> > downstream
> > +      component's reset GPIO.
> > +    maxItems: 1
> > +
> >    clocks:
> >      minItems: 4
> >      maxItems: 6
> > --
> > 2.18.0
> > 

Best regards,
Jian Yang