Il 09/10/23 10:49, Jian Yang ha scritto:
> From: "jian.yang" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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" <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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