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

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

[Jian Yang]

These series patches is based on linux-next, tag: next-20231009.
Add support for controlling power supplies and reset GPIO of a downstream
component in Mediatek's PCIe GEN3 controller driver.

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.

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   | 93 ++++++++++++++++++-
 2 files changed, 122 insertions(+), 1 deletion(-)

-- 
2.18.0

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

[AngeloGioacchino Del Regno]

Il 09/10/23 10:49, 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>
> ---
>   .../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..eb4ad98549d1 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 ]
>   
> +  pcie1v8-supply:

There are another two controllers having such regulators and they all have the
same name for those supplies; Can you please change the names to be consistent
with the other controllers?

vpcie1v8
vpcie3v3
vpcie12v

Regards,
Angelo

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

[AngeloGioacchino Del Regno]

Il 09/10/23 10:49, 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 flow.
> 
> 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 a optional feature.
> 
> Signed-off-by: jian.yang <jian.yang@mediatek.com>
> ---
>   drivers/pci/controller/pcie-mediatek-gen3.c | 93 ++++++++++++++++++++-
>   1 file changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index e0e27645fdf4..ad4b25c34f5d 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[] = {
> +	"pcie1v8",
> +	"pcie3v3",
> +	"pcie12v",
> +};

Please....

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 ret, i;

Since you anyway have to send a v4, can you please also order these by name?

int i, ret;

>   
>   	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
>   	if (!regs)
> @@ -809,14 +825,86 @@ 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)) {
> +		ret = PTR_ERR(pcie->dsc_reset);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to request DSC reset gpio\n");

dev_err_probe() does exactly what you're doing here, but it's shorter :-)

> +
> +		return ret;
> +	}
> +
>   	return 0;
>   }
>   
> +static int mtk_pcie_dsc_power_up(struct mtk_gen3_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	int ret;
> +
> +	/*
> +	 * Skip downstream component's power-up flow if it was kept power-on

* 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 power-on if it need to wake up the

* Keep downstream component powered on if it is capable of waking up
* the system from suspend

> +	 * 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);
> +}
> +

Regards,
Angelo

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

[Jian Yang (杨戬)]

Hi Angelo,

Thanks for your comment.

On Tue, 2023-10-10 at 11:45 +0200, AngeloGioacchino Del Regno wrote:
> Il 09/10/23 10:49, 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>
> > ---
> >   .../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..eb4ad98549d1 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 ]
> >   
> > +  pcie1v8-supply:
> 
> There are another two controllers having such regulators and they all
> have the
> same name for those supplies; Can you please change the names to be
> consistent
> with the other controllers?
> 
> vpcie1v8
> vpcie3v3
> vpcie12v
> 
I will rename them in next patch version.

> Regards,
> Angelo
> 
> 
Best regards,
Jian Yang

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

[Jian Yang (杨戬)]

Hi Angelo,

On Tue, 2023-10-10 at 11:51 +0200, AngeloGioacchino Del Regno wrote:
> Il 09/10/23 10:49, 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 flow.
> > 
> > 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 a optional feature.
> > 
> > Signed-off-by: jian.yang <jian.yang@mediatek.com>
> > ---
> >   drivers/pci/controller/pcie-mediatek-gen3.c | 93
> > ++++++++++++++++++++-
> >   1 file changed, 92 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index e0e27645fdf4..ad4b25c34f5d 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[] = {
> > +	"pcie1v8",
> > +	"pcie3v3",
> > +	"pcie12v",
> > +};
> 
> Please....
> 
> static const char *const dsc_power_supplies[] = {
> 	"vpcie1v8",
> 	"vpcie3v3",
> 	"vpcie12v",
> };
> 

OK.

> > +
> >   /**
> >    * 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 ret, i;
> 
> Since you anyway have to send a v4, can you please also order these
> by name?
> 
> int i, ret;
> 

I will re-order them in next version.

> >   
> >   	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "pcie-mac");
> >   	if (!regs)
> > @@ -809,14 +825,86 @@ 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)) {
> > +		ret = PTR_ERR(pcie->dsc_reset);
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(dev, "failed to request DSC reset
> > gpio\n");
> 
> dev_err_probe() does exactly what you're doing here, but it's shorter
> :-)
> 

Got it

> > +
> > +		return ret;
> > +	}
> > +
> >   	return 0;
> >   }
> >   
> > +static int mtk_pcie_dsc_power_up(struct mtk_gen3_pcie *pcie)
> > +{
> > +	struct device *dev = pcie->dev;
> > +	int ret;
> > +
> > +	/*
> > +	 * Skip downstream component's power-up flow if it was kept
> > power-on
> 
> * Skip powering up the downstream component if it was kept powered on
> 

Thanks for correcting that.

> 
> > +	 * 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 power-on if it need to wake up the
> 
> * Keep downstream component powered on if it is capable of waking up
> * the system from suspend
> 

Thanks for correcting and sorry for my English :)

> > +	 * 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);
> > +}
> > +
> 
> Regards,
> Angelo
> 

Best regards,
Jian Yang