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@xxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxx> > > --- > > 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