diff mbox series

[8/8] PCI: qcom: Add support to PCIe slot power supplies

Message ID 20240827063631.3932971-9-quic_qianyu@quicinc.com
State New
Headers show
Series Add support for PCIe3 on x1e80100 | expand

Commit Message

Qiang Yu Aug. 27, 2024, 6:36 a.m. UTC
On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
support to use 3.3v, 3.3v aux and 12v regulators.

Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 52 +++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 2 deletions(-)

Comments

Konrad Dybcio Aug. 27, 2024, 11:02 a.m. UTC | #1
On 27.08.2024 8:36 AM, Qiang Yu wrote:
> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
> support to use 3.3v, 3.3v aux and 12v regulators.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 52 +++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6f953e32d990..59fb415dfeeb 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -248,6 +248,8 @@ struct qcom_pcie_cfg {
>  	bool no_l0s;
>  };
>  
> +#define QCOM_PCIE_SLOT_MAX_SUPPLIES			3
> +
>  struct qcom_pcie {
>  	struct dw_pcie *pci;
>  	void __iomem *parf;			/* DT parf */
> @@ -260,6 +262,7 @@ struct qcom_pcie {
>  	struct icc_path *icc_cpu;
>  	const struct qcom_pcie_cfg *cfg;
>  	struct dentry *debugfs;
> +	struct regulator_bulk_data slot_supplies[QCOM_PCIE_SLOT_MAX_SUPPLIES];
>  	bool suspended;
>  	bool use_pm_opp;
>  };
> @@ -1174,6 +1177,41 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
>  	return !!(val & PCI_EXP_LNKSTA_DLLLA);
>  }
>  
> +static int qcom_pcie_enable_slot_supplies(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(pcie->slot_supplies),
> +				    pcie->slot_supplies);
> +	if (ret < 0)
> +		dev_err(pci->dev, "Failed to enable slot regulators\n");

return dev_err_probe would be a good call.. probably more so below,
but won't hurt to use here too

> +
> +	return ret;
> +}
> +
> +static void qcom_pcie_disable_slot_supplies(struct qcom_pcie *pcie)
> +{
> +	regulator_bulk_disable(ARRAY_SIZE(pcie->slot_supplies),
> +			       pcie->slot_supplies);
> +}

This I feel like is overly abstracted

Konrad
Dmitry Baryshkov Aug. 27, 2024, 11:44 a.m. UTC | #2
On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
>
> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
> support to use 3.3v, 3.3v aux and 12v regulators.

First of all, I don't see corresponding bindings change.

Second, these supplies power up the slot, not the host controller
itself. As such these supplies do not belong to the host controller
entry. Please consider using the pwrseq framework instead.

>
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 52 +++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6f953e32d990..59fb415dfeeb 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -248,6 +248,8 @@ struct qcom_pcie_cfg {
>         bool no_l0s;
>  };
>
> +#define QCOM_PCIE_SLOT_MAX_SUPPLIES                    3
> +
>  struct qcom_pcie {
>         struct dw_pcie *pci;
>         void __iomem *parf;                     /* DT parf */
> @@ -260,6 +262,7 @@ struct qcom_pcie {
>         struct icc_path *icc_cpu;
>         const struct qcom_pcie_cfg *cfg;
>         struct dentry *debugfs;
> +       struct regulator_bulk_data slot_supplies[QCOM_PCIE_SLOT_MAX_SUPPLIES];
>         bool suspended;
>         bool use_pm_opp;
>  };
> @@ -1174,6 +1177,41 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
>         return !!(val & PCI_EXP_LNKSTA_DLLLA);
>  }
>
> +static int qcom_pcie_enable_slot_supplies(struct qcom_pcie *pcie)
> +{
> +       struct dw_pcie *pci = pcie->pci;
> +       int ret;
> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(pcie->slot_supplies),
> +                                   pcie->slot_supplies);
> +       if (ret < 0)
> +               dev_err(pci->dev, "Failed to enable slot regulators\n");
> +
> +       return ret;
> +}
> +
> +static void qcom_pcie_disable_slot_supplies(struct qcom_pcie *pcie)
> +{
> +       regulator_bulk_disable(ARRAY_SIZE(pcie->slot_supplies),
> +                              pcie->slot_supplies);
> +}
> +
> +static int qcom_pcie_get_slot_supplies(struct qcom_pcie *pcie)
> +{
> +       struct dw_pcie *pci = pcie->pci;
> +       int ret;
> +
> +       pcie->slot_supplies[0].supply = "vpcie12v";
> +       pcie->slot_supplies[1].supply = "vpcie3v3";
> +       pcie->slot_supplies[2].supply = "vpcie3v3aux";
> +       ret = devm_regulator_bulk_get(pci->dev, ARRAY_SIZE(pcie->slot_supplies),
> +                                     pcie->slot_supplies);
> +       if (ret < 0)
> +               dev_err(pci->dev, "Failed to get slot regulators\n");
> +
> +       return ret;
> +}
> +
>  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -1182,10 +1220,14 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>
>         qcom_ep_reset_assert(pcie);
>
> -       ret = pcie->cfg->ops->init(pcie);
> +       ret = qcom_pcie_enable_slot_supplies(pcie);
>         if (ret)
>                 return ret;
>
> +       ret = pcie->cfg->ops->init(pcie);
> +       if (ret)
> +               goto err_disable_slot;
> +
>         ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
>         if (ret)
>                 goto err_deinit;
> @@ -1216,7 +1258,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>         phy_power_off(pcie->phy);
>  err_deinit:
>         pcie->cfg->ops->deinit(pcie);
> -
> +err_disable_slot:
> +       qcom_pcie_disable_slot_supplies(pcie);
>         return ret;
>  }
>
> @@ -1228,6 +1271,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
>         qcom_ep_reset_assert(pcie);
>         phy_power_off(pcie->phy);
>         pcie->cfg->ops->deinit(pcie);
> +       qcom_pcie_disable_slot_supplies(pcie);
>  }
>
>  static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
> @@ -1602,6 +1646,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>                         goto err_pm_runtime_put;
>         }
>
> +       ret = qcom_pcie_get_slot_supplies(pcie);
> +       if (ret)
> +               goto err_pm_runtime_put;
> +
>         ret = pcie->cfg->ops->get_resources(pcie);
>         if (ret)
>                 goto err_pm_runtime_put;
> --
> 2.34.1
>
Manivannan Sadhasivam Aug. 27, 2024, 4:58 p.m. UTC | #3
On Tue, Aug 27, 2024 at 02:44:09PM +0300, Dmitry Baryshkov wrote:
> On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
> >
> > On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
> > support to use 3.3v, 3.3v aux and 12v regulators.
> 
> First of all, I don't see corresponding bindings change.
> 
> Second, these supplies power up the slot, not the host controller
> itself. As such these supplies do not belong to the host controller
> entry. Please consider using the pwrseq framework instead.
> 

Indeed. For legacy reasons, slot power supplies were populated in the host
bridge node itself until recently Rob started objecting it [1]. And it makes
real sense to put these supplies in the root port node and handle them in the
relevant driver.

I'm still evaluating whether the handling should be done in the portdrv or
pwrctl driver, but haven't reached the conclusion. Pwrctl seems to be the ideal
choice, but I see a few issues related to handling the OF node for the root
port.

Hope I'll come to a conclusion in the next few days and will update this thread.

- Mani

[1] https://lore.kernel.org/lkml/20240604235806.GA1903493-robh@kernel.org/

> >
> > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 52 +++++++++++++++++++++++++-
> >  1 file changed, 50 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 6f953e32d990..59fb415dfeeb 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -248,6 +248,8 @@ struct qcom_pcie_cfg {
> >         bool no_l0s;
> >  };
> >
> > +#define QCOM_PCIE_SLOT_MAX_SUPPLIES                    3
> > +
> >  struct qcom_pcie {
> >         struct dw_pcie *pci;
> >         void __iomem *parf;                     /* DT parf */
> > @@ -260,6 +262,7 @@ struct qcom_pcie {
> >         struct icc_path *icc_cpu;
> >         const struct qcom_pcie_cfg *cfg;
> >         struct dentry *debugfs;
> > +       struct regulator_bulk_data slot_supplies[QCOM_PCIE_SLOT_MAX_SUPPLIES];
> >         bool suspended;
> >         bool use_pm_opp;
> >  };
> > @@ -1174,6 +1177,41 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
> >         return !!(val & PCI_EXP_LNKSTA_DLLLA);
> >  }
> >
> > +static int qcom_pcie_enable_slot_supplies(struct qcom_pcie *pcie)
> > +{
> > +       struct dw_pcie *pci = pcie->pci;
> > +       int ret;
> > +
> > +       ret = regulator_bulk_enable(ARRAY_SIZE(pcie->slot_supplies),
> > +                                   pcie->slot_supplies);
> > +       if (ret < 0)
> > +               dev_err(pci->dev, "Failed to enable slot regulators\n");
> > +
> > +       return ret;
> > +}
> > +
> > +static void qcom_pcie_disable_slot_supplies(struct qcom_pcie *pcie)
> > +{
> > +       regulator_bulk_disable(ARRAY_SIZE(pcie->slot_supplies),
> > +                              pcie->slot_supplies);
> > +}
> > +
> > +static int qcom_pcie_get_slot_supplies(struct qcom_pcie *pcie)
> > +{
> > +       struct dw_pcie *pci = pcie->pci;
> > +       int ret;
> > +
> > +       pcie->slot_supplies[0].supply = "vpcie12v";
> > +       pcie->slot_supplies[1].supply = "vpcie3v3";
> > +       pcie->slot_supplies[2].supply = "vpcie3v3aux";
> > +       ret = devm_regulator_bulk_get(pci->dev, ARRAY_SIZE(pcie->slot_supplies),
> > +                                     pcie->slot_supplies);
> > +       if (ret < 0)
> > +               dev_err(pci->dev, "Failed to get slot regulators\n");
> > +
> > +       return ret;
> > +}
> > +
> >  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> >  {
> >         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > @@ -1182,10 +1220,14 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> >
> >         qcom_ep_reset_assert(pcie);
> >
> > -       ret = pcie->cfg->ops->init(pcie);
> > +       ret = qcom_pcie_enable_slot_supplies(pcie);
> >         if (ret)
> >                 return ret;
> >
> > +       ret = pcie->cfg->ops->init(pcie);
> > +       if (ret)
> > +               goto err_disable_slot;
> > +
> >         ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> >         if (ret)
> >                 goto err_deinit;
> > @@ -1216,7 +1258,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> >         phy_power_off(pcie->phy);
> >  err_deinit:
> >         pcie->cfg->ops->deinit(pcie);
> > -
> > +err_disable_slot:
> > +       qcom_pcie_disable_slot_supplies(pcie);
> >         return ret;
> >  }
> >
> > @@ -1228,6 +1271,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
> >         qcom_ep_reset_assert(pcie);
> >         phy_power_off(pcie->phy);
> >         pcie->cfg->ops->deinit(pcie);
> > +       qcom_pcie_disable_slot_supplies(pcie);
> >  }
> >
> >  static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
> > @@ -1602,6 +1646,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >                         goto err_pm_runtime_put;
> >         }
> >
> > +       ret = qcom_pcie_get_slot_supplies(pcie);
> > +       if (ret)
> > +               goto err_pm_runtime_put;
> > +
> >         ret = pcie->cfg->ops->get_resources(pcie);
> >         if (ret)
> >                 goto err_pm_runtime_put;
> > --
> > 2.34.1
> >
> 
> 
> -- 
> With best wishes
> Dmitry
Qiang Yu Aug. 28, 2024, 1:44 p.m. UTC | #4
On 8/27/2024 7:44 PM, Dmitry Baryshkov wrote:
> On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
>> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
>> support to use 3.3v, 3.3v aux and 12v regulators.
> First of all, I don't see corresponding bindings change.
>
> Second, these supplies power up the slot, not the host controller
> itself. As such these supplies do not belong to the host controller
> entry. Please consider using the pwrseq framework instead.
As Mani commented, he is exploring to use pwrctl driver to control this
three power. Will update the patch after Mani share his conclusion. This
patch may even not required.

Thanks,
Qiang
>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 52 +++++++++++++++++++++++++-
>>   1 file changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 6f953e32d990..59fb415dfeeb 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -248,6 +248,8 @@ struct qcom_pcie_cfg {
>>          bool no_l0s;
>>   };
>>
>> +#define QCOM_PCIE_SLOT_MAX_SUPPLIES                    3
>> +
>>   struct qcom_pcie {
>>          struct dw_pcie *pci;
>>          void __iomem *parf;                     /* DT parf */
>> @@ -260,6 +262,7 @@ struct qcom_pcie {
>>          struct icc_path *icc_cpu;
>>          const struct qcom_pcie_cfg *cfg;
>>          struct dentry *debugfs;
>> +       struct regulator_bulk_data slot_supplies[QCOM_PCIE_SLOT_MAX_SUPPLIES];
>>          bool suspended;
>>          bool use_pm_opp;
>>   };
>> @@ -1174,6 +1177,41 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
>>          return !!(val & PCI_EXP_LNKSTA_DLLLA);
>>   }
>>
>> +static int qcom_pcie_enable_slot_supplies(struct qcom_pcie *pcie)
>> +{
>> +       struct dw_pcie *pci = pcie->pci;
>> +       int ret;
>> +
>> +       ret = regulator_bulk_enable(ARRAY_SIZE(pcie->slot_supplies),
>> +                                   pcie->slot_supplies);
>> +       if (ret < 0)
>> +               dev_err(pci->dev, "Failed to enable slot regulators\n");
>> +
>> +       return ret;
>> +}
>> +
>> +static void qcom_pcie_disable_slot_supplies(struct qcom_pcie *pcie)
>> +{
>> +       regulator_bulk_disable(ARRAY_SIZE(pcie->slot_supplies),
>> +                              pcie->slot_supplies);
>> +}
>> +
>> +static int qcom_pcie_get_slot_supplies(struct qcom_pcie *pcie)
>> +{
>> +       struct dw_pcie *pci = pcie->pci;
>> +       int ret;
>> +
>> +       pcie->slot_supplies[0].supply = "vpcie12v";
>> +       pcie->slot_supplies[1].supply = "vpcie3v3";
>> +       pcie->slot_supplies[2].supply = "vpcie3v3aux";
>> +       ret = devm_regulator_bulk_get(pci->dev, ARRAY_SIZE(pcie->slot_supplies),
>> +                                     pcie->slot_supplies);
>> +       if (ret < 0)
>> +               dev_err(pci->dev, "Failed to get slot regulators\n");
>> +
>> +       return ret;
>> +}
>> +
>>   static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>   {
>>          struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> @@ -1182,10 +1220,14 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>
>>          qcom_ep_reset_assert(pcie);
>>
>> -       ret = pcie->cfg->ops->init(pcie);
>> +       ret = qcom_pcie_enable_slot_supplies(pcie);
>>          if (ret)
>>                  return ret;
>>
>> +       ret = pcie->cfg->ops->init(pcie);
>> +       if (ret)
>> +               goto err_disable_slot;
>> +
>>          ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
>>          if (ret)
>>                  goto err_deinit;
>> @@ -1216,7 +1258,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>          phy_power_off(pcie->phy);
>>   err_deinit:
>>          pcie->cfg->ops->deinit(pcie);
>> -
>> +err_disable_slot:
>> +       qcom_pcie_disable_slot_supplies(pcie);
>>          return ret;
>>   }
>>
>> @@ -1228,6 +1271,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
>>          qcom_ep_reset_assert(pcie);
>>          phy_power_off(pcie->phy);
>>          pcie->cfg->ops->deinit(pcie);
>> +       qcom_pcie_disable_slot_supplies(pcie);
>>   }
>>
>>   static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
>> @@ -1602,6 +1646,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>                          goto err_pm_runtime_put;
>>          }
>>
>> +       ret = qcom_pcie_get_slot_supplies(pcie);
>> +       if (ret)
>> +               goto err_pm_runtime_put;
>> +
>>          ret = pcie->cfg->ops->get_resources(pcie);
>>          if (ret)
>>                  goto err_pm_runtime_put;
>> --
>> 2.34.1
>>
>
Qiang Yu Sept. 11, 2024, 8:17 a.m. UTC | #5
On 8/28/2024 12:58 AM, Manivannan Sadhasivam wrote:
> On Tue, Aug 27, 2024 at 02:44:09PM +0300, Dmitry Baryshkov wrote:
>> On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
>>> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
>>> support to use 3.3v, 3.3v aux and 12v regulators.
>> First of all, I don't see corresponding bindings change.
>>
>> Second, these supplies power up the slot, not the host controller
>> itself. As such these supplies do not belong to the host controller
>> entry. Please consider using the pwrseq framework instead.
>>
> Indeed. For legacy reasons, slot power supplies were populated in the host
> bridge node itself until recently Rob started objecting it [1]. And it makes
> real sense to put these supplies in the root port node and handle them in the
> relevant driver.
>
> I'm still evaluating whether the handling should be done in the portdrv or
> pwrctl driver, but haven't reached the conclusion. Pwrctl seems to be the ideal
> choice, but I see a few issues related to handling the OF node for the root
> port.
>
> Hope I'll come to a conclusion in the next few days and will update this thread.
>
> - Mani
>
> [1] https://lore.kernel.org/lkml/20240604235806.GA1903493-robh@kernel.org/
Hi Mani, do you have any updates?

Thanks,
Qiang
>
>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>> ---
>>>   drivers/pci/controller/dwc/pcie-qcom.c | 52 +++++++++++++++++++++++++-
>>>   1 file changed, 50 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index 6f953e32d990..59fb415dfeeb 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -248,6 +248,8 @@ struct qcom_pcie_cfg {
>>>          bool no_l0s;
>>>   };
>>>
>>> +#define QCOM_PCIE_SLOT_MAX_SUPPLIES                    3
>>> +
>>>   struct qcom_pcie {
>>>          struct dw_pcie *pci;
>>>          void __iomem *parf;                     /* DT parf */
>>> @@ -260,6 +262,7 @@ struct qcom_pcie {
>>>          struct icc_path *icc_cpu;
>>>          const struct qcom_pcie_cfg *cfg;
>>>          struct dentry *debugfs;
>>> +       struct regulator_bulk_data slot_supplies[QCOM_PCIE_SLOT_MAX_SUPPLIES];
>>>          bool suspended;
>>>          bool use_pm_opp;
>>>   };
>>> @@ -1174,6 +1177,41 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
>>>          return !!(val & PCI_EXP_LNKSTA_DLLLA);
>>>   }
>>>
>>> +static int qcom_pcie_enable_slot_supplies(struct qcom_pcie *pcie)
>>> +{
>>> +       struct dw_pcie *pci = pcie->pci;
>>> +       int ret;
>>> +
>>> +       ret = regulator_bulk_enable(ARRAY_SIZE(pcie->slot_supplies),
>>> +                                   pcie->slot_supplies);
>>> +       if (ret < 0)
>>> +               dev_err(pci->dev, "Failed to enable slot regulators\n");
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +static void qcom_pcie_disable_slot_supplies(struct qcom_pcie *pcie)
>>> +{
>>> +       regulator_bulk_disable(ARRAY_SIZE(pcie->slot_supplies),
>>> +                              pcie->slot_supplies);
>>> +}
>>> +
>>> +static int qcom_pcie_get_slot_supplies(struct qcom_pcie *pcie)
>>> +{
>>> +       struct dw_pcie *pci = pcie->pci;
>>> +       int ret;
>>> +
>>> +       pcie->slot_supplies[0].supply = "vpcie12v";
>>> +       pcie->slot_supplies[1].supply = "vpcie3v3";
>>> +       pcie->slot_supplies[2].supply = "vpcie3v3aux";
>>> +       ret = devm_regulator_bulk_get(pci->dev, ARRAY_SIZE(pcie->slot_supplies),
>>> +                                     pcie->slot_supplies);
>>> +       if (ret < 0)
>>> +               dev_err(pci->dev, "Failed to get slot regulators\n");
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>>   {
>>>          struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> @@ -1182,10 +1220,14 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>>
>>>          qcom_ep_reset_assert(pcie);
>>>
>>> -       ret = pcie->cfg->ops->init(pcie);
>>> +       ret = qcom_pcie_enable_slot_supplies(pcie);
>>>          if (ret)
>>>                  return ret;
>>>
>>> +       ret = pcie->cfg->ops->init(pcie);
>>> +       if (ret)
>>> +               goto err_disable_slot;
>>> +
>>>          ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
>>>          if (ret)
>>>                  goto err_deinit;
>>> @@ -1216,7 +1258,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>>          phy_power_off(pcie->phy);
>>>   err_deinit:
>>>          pcie->cfg->ops->deinit(pcie);
>>> -
>>> +err_disable_slot:
>>> +       qcom_pcie_disable_slot_supplies(pcie);
>>>          return ret;
>>>   }
>>>
>>> @@ -1228,6 +1271,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
>>>          qcom_ep_reset_assert(pcie);
>>>          phy_power_off(pcie->phy);
>>>          pcie->cfg->ops->deinit(pcie);
>>> +       qcom_pcie_disable_slot_supplies(pcie);
>>>   }
>>>
>>>   static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
>>> @@ -1602,6 +1646,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>                          goto err_pm_runtime_put;
>>>          }
>>>
>>> +       ret = qcom_pcie_get_slot_supplies(pcie);
>>> +       if (ret)
>>> +               goto err_pm_runtime_put;
>>> +
>>>          ret = pcie->cfg->ops->get_resources(pcie);
>>>          if (ret)
>>>                  goto err_pm_runtime_put;
>>> --
>>> 2.34.1
>>>
>>
>> -- 
>> With best wishes
>> Dmitry
Manivannan Sadhasivam Sept. 11, 2024, 3:32 p.m. UTC | #6
On Wed, Sep 11, 2024 at 04:17:41PM +0800, Qiang Yu wrote:
> 
> On 8/28/2024 12:58 AM, Manivannan Sadhasivam wrote:
> > On Tue, Aug 27, 2024 at 02:44:09PM +0300, Dmitry Baryshkov wrote:
> > > On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
> > > > On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
> > > > support to use 3.3v, 3.3v aux and 12v regulators.
> > > First of all, I don't see corresponding bindings change.
> > > 
> > > Second, these supplies power up the slot, not the host controller
> > > itself. As such these supplies do not belong to the host controller
> > > entry. Please consider using the pwrseq framework instead.
> > > 
> > Indeed. For legacy reasons, slot power supplies were populated in the host
> > bridge node itself until recently Rob started objecting it [1]. And it makes
> > real sense to put these supplies in the root port node and handle them in the
> > relevant driver.
> > 
> > I'm still evaluating whether the handling should be done in the portdrv or
> > pwrctl driver, but haven't reached the conclusion. Pwrctl seems to be the ideal
> > choice, but I see a few issues related to handling the OF node for the root
> > port.
> > 
> > Hope I'll come to a conclusion in the next few days and will update this thread.
> > 
> > - Mani
> > 
> > [1] https://lore.kernel.org/lkml/20240604235806.GA1903493-robh@kernel.org/
> Hi Mani, do you have any updates?
> 

I'm working with Bartosz to add a new pwrctl driver for rootports. And we are
debugging an issue currently. Unfortunately, the progress is very slow as I'm on
vacation still.

Will post the patches once it got resolved.

- Mani

> Thanks,
> Qiang
> > 
> > > > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 52 +++++++++++++++++++++++++-
> > > >   1 file changed, 50 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index 6f953e32d990..59fb415dfeeb 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -248,6 +248,8 @@ struct qcom_pcie_cfg {
> > > >          bool no_l0s;
> > > >   };
> > > > 
> > > > +#define QCOM_PCIE_SLOT_MAX_SUPPLIES                    3
> > > > +
> > > >   struct qcom_pcie {
> > > >          struct dw_pcie *pci;
> > > >          void __iomem *parf;                     /* DT parf */
> > > > @@ -260,6 +262,7 @@ struct qcom_pcie {
> > > >          struct icc_path *icc_cpu;
> > > >          const struct qcom_pcie_cfg *cfg;
> > > >          struct dentry *debugfs;
> > > > +       struct regulator_bulk_data slot_supplies[QCOM_PCIE_SLOT_MAX_SUPPLIES];
> > > >          bool suspended;
> > > >          bool use_pm_opp;
> > > >   };
> > > > @@ -1174,6 +1177,41 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
> > > >          return !!(val & PCI_EXP_LNKSTA_DLLLA);
> > > >   }
> > > > 
> > > > +static int qcom_pcie_enable_slot_supplies(struct qcom_pcie *pcie)
> > > > +{
> > > > +       struct dw_pcie *pci = pcie->pci;
> > > > +       int ret;
> > > > +
> > > > +       ret = regulator_bulk_enable(ARRAY_SIZE(pcie->slot_supplies),
> > > > +                                   pcie->slot_supplies);
> > > > +       if (ret < 0)
> > > > +               dev_err(pci->dev, "Failed to enable slot regulators\n");
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static void qcom_pcie_disable_slot_supplies(struct qcom_pcie *pcie)
> > > > +{
> > > > +       regulator_bulk_disable(ARRAY_SIZE(pcie->slot_supplies),
> > > > +                              pcie->slot_supplies);
> > > > +}
> > > > +
> > > > +static int qcom_pcie_get_slot_supplies(struct qcom_pcie *pcie)
> > > > +{
> > > > +       struct dw_pcie *pci = pcie->pci;
> > > > +       int ret;
> > > > +
> > > > +       pcie->slot_supplies[0].supply = "vpcie12v";
> > > > +       pcie->slot_supplies[1].supply = "vpcie3v3";
> > > > +       pcie->slot_supplies[2].supply = "vpcie3v3aux";
> > > > +       ret = devm_regulator_bulk_get(pci->dev, ARRAY_SIZE(pcie->slot_supplies),
> > > > +                                     pcie->slot_supplies);
> > > > +       if (ret < 0)
> > > > +               dev_err(pci->dev, "Failed to get slot regulators\n");
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > >   static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> > > >   {
> > > >          struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > @@ -1182,10 +1220,14 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> > > > 
> > > >          qcom_ep_reset_assert(pcie);
> > > > 
> > > > -       ret = pcie->cfg->ops->init(pcie);
> > > > +       ret = qcom_pcie_enable_slot_supplies(pcie);
> > > >          if (ret)
> > > >                  return ret;
> > > > 
> > > > +       ret = pcie->cfg->ops->init(pcie);
> > > > +       if (ret)
> > > > +               goto err_disable_slot;
> > > > +
> > > >          ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> > > >          if (ret)
> > > >                  goto err_deinit;
> > > > @@ -1216,7 +1258,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> > > >          phy_power_off(pcie->phy);
> > > >   err_deinit:
> > > >          pcie->cfg->ops->deinit(pcie);
> > > > -
> > > > +err_disable_slot:
> > > > +       qcom_pcie_disable_slot_supplies(pcie);
> > > >          return ret;
> > > >   }
> > > > 
> > > > @@ -1228,6 +1271,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
> > > >          qcom_ep_reset_assert(pcie);
> > > >          phy_power_off(pcie->phy);
> > > >          pcie->cfg->ops->deinit(pcie);
> > > > +       qcom_pcie_disable_slot_supplies(pcie);
> > > >   }
> > > > 
> > > >   static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
> > > > @@ -1602,6 +1646,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >                          goto err_pm_runtime_put;
> > > >          }
> > > > 
> > > > +       ret = qcom_pcie_get_slot_supplies(pcie);
> > > > +       if (ret)
> > > > +               goto err_pm_runtime_put;
> > > > +
> > > >          ret = pcie->cfg->ops->get_resources(pcie);
> > > >          if (ret)
> > > >                  goto err_pm_runtime_put;
> > > > --
> > > > 2.34.1
> > > > 
> > > 
> > > -- 
> > > With best wishes
> > > Dmitry
Qiang Yu Sept. 12, 2024, 1:39 p.m. UTC | #7
On 9/11/2024 11:32 PM, Manivannan Sadhasivam wrote:
> On Wed, Sep 11, 2024 at 04:17:41PM +0800, Qiang Yu wrote:
>> On 8/28/2024 12:58 AM, Manivannan Sadhasivam wrote:
>>> On Tue, Aug 27, 2024 at 02:44:09PM +0300, Dmitry Baryshkov wrote:
>>>> On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
>>>>> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
>>>>> support to use 3.3v, 3.3v aux and 12v regulators.
>>>> First of all, I don't see corresponding bindings change.
>>>>
>>>> Second, these supplies power up the slot, not the host controller
>>>> itself. As such these supplies do not belong to the host controller
>>>> entry. Please consider using the pwrseq framework instead.
>>>>
>>> Indeed. For legacy reasons, slot power supplies were populated in the host
>>> bridge node itself until recently Rob started objecting it [1]. And it makes
>>> real sense to put these supplies in the root port node and handle them in the
>>> relevant driver.
>>>
>>> I'm still evaluating whether the handling should be done in the portdrv or
>>> pwrctl driver, but haven't reached the conclusion. Pwrctl seems to be the ideal
>>> choice, but I see a few issues related to handling the OF node for the root
>>> port.
>>>
>>> Hope I'll come to a conclusion in the next few days and will update this thread.
>>>
>>> - Mani
>>>
>>> [1] https://lore.kernel.org/lkml/20240604235806.GA1903493-robh@kernel.org/
>> Hi Mani, do you have any updates?
>>
> I'm working with Bartosz to add a new pwrctl driver for rootports. And we are
> debugging an issue currently. Unfortunately, the progress is very slow as I'm on
> vacation still.
>
> Will post the patches once it got resolved.
>
> - Mani
OK, thanks for your update.

Thanks,
Qiang
>> Thanks,
>> Qiang
>>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>>> ---
>>>>>    drivers/pci/controller/dwc/pcie-qcom.c | 52 +++++++++++++++++++++++++-
>>>>>    1 file changed, 50 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> index 6f953e32d990..59fb415dfeeb 100644
>>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> @@ -248,6 +248,8 @@ struct qcom_pcie_cfg {
>>>>>           bool no_l0s;
>>>>>    };
>>>>>
>>>>> +#define QCOM_PCIE_SLOT_MAX_SUPPLIES                    3
>>>>> +
>>>>>    struct qcom_pcie {
>>>>>           struct dw_pcie *pci;
>>>>>           void __iomem *parf;                     /* DT parf */
>>>>> @@ -260,6 +262,7 @@ struct qcom_pcie {
>>>>>           struct icc_path *icc_cpu;
>>>>>           const struct qcom_pcie_cfg *cfg;
>>>>>           struct dentry *debugfs;
>>>>> +       struct regulator_bulk_data slot_supplies[QCOM_PCIE_SLOT_MAX_SUPPLIES];
>>>>>           bool suspended;
>>>>>           bool use_pm_opp;
>>>>>    };
>>>>> @@ -1174,6 +1177,41 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
>>>>>           return !!(val & PCI_EXP_LNKSTA_DLLLA);
>>>>>    }
>>>>>
>>>>> +static int qcom_pcie_enable_slot_supplies(struct qcom_pcie *pcie)
>>>>> +{
>>>>> +       struct dw_pcie *pci = pcie->pci;
>>>>> +       int ret;
>>>>> +
>>>>> +       ret = regulator_bulk_enable(ARRAY_SIZE(pcie->slot_supplies),
>>>>> +                                   pcie->slot_supplies);
>>>>> +       if (ret < 0)
>>>>> +               dev_err(pci->dev, "Failed to enable slot regulators\n");
>>>>> +
>>>>> +       return ret;
>>>>> +}
>>>>> +
>>>>> +static void qcom_pcie_disable_slot_supplies(struct qcom_pcie *pcie)
>>>>> +{
>>>>> +       regulator_bulk_disable(ARRAY_SIZE(pcie->slot_supplies),
>>>>> +                              pcie->slot_supplies);
>>>>> +}
>>>>> +
>>>>> +static int qcom_pcie_get_slot_supplies(struct qcom_pcie *pcie)
>>>>> +{
>>>>> +       struct dw_pcie *pci = pcie->pci;
>>>>> +       int ret;
>>>>> +
>>>>> +       pcie->slot_supplies[0].supply = "vpcie12v";
>>>>> +       pcie->slot_supplies[1].supply = "vpcie3v3";
>>>>> +       pcie->slot_supplies[2].supply = "vpcie3v3aux";
>>>>> +       ret = devm_regulator_bulk_get(pci->dev, ARRAY_SIZE(pcie->slot_supplies),
>>>>> +                                     pcie->slot_supplies);
>>>>> +       if (ret < 0)
>>>>> +               dev_err(pci->dev, "Failed to get slot regulators\n");
>>>>> +
>>>>> +       return ret;
>>>>> +}
>>>>> +
>>>>>    static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>>>>    {
>>>>>           struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>> @@ -1182,10 +1220,14 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>>>>
>>>>>           qcom_ep_reset_assert(pcie);
>>>>>
>>>>> -       ret = pcie->cfg->ops->init(pcie);
>>>>> +       ret = qcom_pcie_enable_slot_supplies(pcie);
>>>>>           if (ret)
>>>>>                   return ret;
>>>>>
>>>>> +       ret = pcie->cfg->ops->init(pcie);
>>>>> +       if (ret)
>>>>> +               goto err_disable_slot;
>>>>> +
>>>>>           ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
>>>>>           if (ret)
>>>>>                   goto err_deinit;
>>>>> @@ -1216,7 +1258,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>>>>>           phy_power_off(pcie->phy);
>>>>>    err_deinit:
>>>>>           pcie->cfg->ops->deinit(pcie);
>>>>> -
>>>>> +err_disable_slot:
>>>>> +       qcom_pcie_disable_slot_supplies(pcie);
>>>>>           return ret;
>>>>>    }
>>>>>
>>>>> @@ -1228,6 +1271,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
>>>>>           qcom_ep_reset_assert(pcie);
>>>>>           phy_power_off(pcie->phy);
>>>>>           pcie->cfg->ops->deinit(pcie);
>>>>> +       qcom_pcie_disable_slot_supplies(pcie);
>>>>>    }
>>>>>
>>>>>    static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
>>>>> @@ -1602,6 +1646,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>>>                           goto err_pm_runtime_put;
>>>>>           }
>>>>>
>>>>> +       ret = qcom_pcie_get_slot_supplies(pcie);
>>>>> +       if (ret)
>>>>> +               goto err_pm_runtime_put;
>>>>> +
>>>>>           ret = pcie->cfg->ops->get_resources(pcie);
>>>>>           if (ret)
>>>>>                   goto err_pm_runtime_put;
>>>>> --
>>>>> 2.34.1
>>>>>
>>>> -- 
>>>> With best wishes
>>>> Dmitry
Konrad Dybcio Sept. 12, 2024, 2:15 p.m. UTC | #8
On 12.09.2024 3:39 PM, Qiang Yu wrote:
> 
> On 9/11/2024 11:32 PM, Manivannan Sadhasivam wrote:
>> On Wed, Sep 11, 2024 at 04:17:41PM +0800, Qiang Yu wrote:
>>> On 8/28/2024 12:58 AM, Manivannan Sadhasivam wrote:
>>>> On Tue, Aug 27, 2024 at 02:44:09PM +0300, Dmitry Baryshkov wrote:
>>>>> On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
>>>>>> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
>>>>>> support to use 3.3v, 3.3v aux and 12v regulators.
>>>>> First of all, I don't see corresponding bindings change.
>>>>>
>>>>> Second, these supplies power up the slot, not the host controller
>>>>> itself. As such these supplies do not belong to the host controller
>>>>> entry. Please consider using the pwrseq framework instead.
>>>>>
>>>> Indeed. For legacy reasons, slot power supplies were populated in the host
>>>> bridge node itself until recently Rob started objecting it [1]. And it makes
>>>> real sense to put these supplies in the root port node and handle them in the
>>>> relevant driver.
>>>>
>>>> I'm still evaluating whether the handling should be done in the portdrv or
>>>> pwrctl driver, but haven't reached the conclusion. Pwrctl seems to be the ideal
>>>> choice, but I see a few issues related to handling the OF node for the root
>>>> port.
>>>>
>>>> Hope I'll come to a conclusion in the next few days and will update this thread.
>>>>
>>>> - Mani
>>>>
>>>> [1] https://lore.kernel.org/lkml/20240604235806.GA1903493-robh@kernel.org/
>>> Hi Mani, do you have any updates?
>>>
>> I'm working with Bartosz to add a new pwrctl driver for rootports. And we are
>> debugging an issue currently. Unfortunately, the progress is very slow as I'm on
>> vacation still.
>>
>> Will post the patches once it got resolved.
>>
>> - Mani
> OK, thanks for your update.

Qiang, you can still resubmit the rest of the patches without having
to wait on that to be resolved

Konrad
Manivannan Sadhasivam Sept. 12, 2024, 2:44 p.m. UTC | #9
On Thu, Sep 12, 2024 at 04:15:56PM +0200, Konrad Dybcio wrote:
> On 12.09.2024 3:39 PM, Qiang Yu wrote:
> > 
> > On 9/11/2024 11:32 PM, Manivannan Sadhasivam wrote:
> >> On Wed, Sep 11, 2024 at 04:17:41PM +0800, Qiang Yu wrote:
> >>> On 8/28/2024 12:58 AM, Manivannan Sadhasivam wrote:
> >>>> On Tue, Aug 27, 2024 at 02:44:09PM +0300, Dmitry Baryshkov wrote:
> >>>>> On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
> >>>>>> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
> >>>>>> support to use 3.3v, 3.3v aux and 12v regulators.
> >>>>> First of all, I don't see corresponding bindings change.
> >>>>>
> >>>>> Second, these supplies power up the slot, not the host controller
> >>>>> itself. As such these supplies do not belong to the host controller
> >>>>> entry. Please consider using the pwrseq framework instead.
> >>>>>
> >>>> Indeed. For legacy reasons, slot power supplies were populated in the host
> >>>> bridge node itself until recently Rob started objecting it [1]. And it makes
> >>>> real sense to put these supplies in the root port node and handle them in the
> >>>> relevant driver.
> >>>>
> >>>> I'm still evaluating whether the handling should be done in the portdrv or
> >>>> pwrctl driver, but haven't reached the conclusion. Pwrctl seems to be the ideal
> >>>> choice, but I see a few issues related to handling the OF node for the root
> >>>> port.
> >>>>
> >>>> Hope I'll come to a conclusion in the next few days and will update this thread.
> >>>>
> >>>> - Mani
> >>>>
> >>>> [1] https://lore.kernel.org/lkml/20240604235806.GA1903493-robh@kernel.org/
> >>> Hi Mani, do you have any updates?
> >>>
> >> I'm working with Bartosz to add a new pwrctl driver for rootports. And we are
> >> debugging an issue currently. Unfortunately, the progress is very slow as I'm on
> >> vacation still.
> >>
> >> Will post the patches once it got resolved.
> >>
> >> - Mani
> > OK, thanks for your update.
> 
> Qiang, you can still resubmit the rest of the patches without having
> to wait on that to be resolved
> 

In that case, the slot supplies should be described in the PCIe bridge.

- Mani

> Konrad
Dmitry Baryshkov Sept. 12, 2024, 2:49 p.m. UTC | #10
On Thu, 12 Sept 2024 at 17:45, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Thu, Sep 12, 2024 at 04:15:56PM +0200, Konrad Dybcio wrote:
> > On 12.09.2024 3:39 PM, Qiang Yu wrote:
> > >
> > > On 9/11/2024 11:32 PM, Manivannan Sadhasivam wrote:
> > >> On Wed, Sep 11, 2024 at 04:17:41PM +0800, Qiang Yu wrote:
> > >>> On 8/28/2024 12:58 AM, Manivannan Sadhasivam wrote:
> > >>>> On Tue, Aug 27, 2024 at 02:44:09PM +0300, Dmitry Baryshkov wrote:
> > >>>>> On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
> > >>>>>> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
> > >>>>>> support to use 3.3v, 3.3v aux and 12v regulators.
> > >>>>> First of all, I don't see corresponding bindings change.
> > >>>>>
> > >>>>> Second, these supplies power up the slot, not the host controller
> > >>>>> itself. As such these supplies do not belong to the host controller
> > >>>>> entry. Please consider using the pwrseq framework instead.
> > >>>>>
> > >>>> Indeed. For legacy reasons, slot power supplies were populated in the host
> > >>>> bridge node itself until recently Rob started objecting it [1]. And it makes
> > >>>> real sense to put these supplies in the root port node and handle them in the
> > >>>> relevant driver.
> > >>>>
> > >>>> I'm still evaluating whether the handling should be done in the portdrv or
> > >>>> pwrctl driver, but haven't reached the conclusion. Pwrctl seems to be the ideal
> > >>>> choice, but I see a few issues related to handling the OF node for the root
> > >>>> port.
> > >>>>
> > >>>> Hope I'll come to a conclusion in the next few days and will update this thread.
> > >>>>
> > >>>> - Mani
> > >>>>
> > >>>> [1] https://lore.kernel.org/lkml/20240604235806.GA1903493-robh@kernel.org/
> > >>> Hi Mani, do you have any updates?
> > >>>
> > >> I'm working with Bartosz to add a new pwrctl driver for rootports. And we are
> > >> debugging an issue currently. Unfortunately, the progress is very slow as I'm on
> > >> vacation still.
> > >>
> > >> Will post the patches once it got resolved.
> > >>
> > >> - Mani
> > > OK, thanks for your update.
> >
> > Qiang, you can still resubmit the rest of the patches without having
> > to wait on that to be resolved
> >
>
> In that case, the slot supplies should be described in the PCIe bridge.

Patches 1-6 don't seem to depend on slot supplies, so they can be
submitted separately.

>
> - Mani
>
> > Konrad
>
> --
> மணிவண்ணன் சதாசிவம்
Qiang Yu Sept. 13, 2024, 8:41 a.m. UTC | #11
On 9/12/2024 10:49 PM, Dmitry Baryshkov wrote:
> On Thu, 12 Sept 2024 at 17:45, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
>> On Thu, Sep 12, 2024 at 04:15:56PM +0200, Konrad Dybcio wrote:
>>> On 12.09.2024 3:39 PM, Qiang Yu wrote:
>>>> On 9/11/2024 11:32 PM, Manivannan Sadhasivam wrote:
>>>>> On Wed, Sep 11, 2024 at 04:17:41PM +0800, Qiang Yu wrote:
>>>>>> On 8/28/2024 12:58 AM, Manivannan Sadhasivam wrote:
>>>>>>> On Tue, Aug 27, 2024 at 02:44:09PM +0300, Dmitry Baryshkov wrote:
>>>>>>>> On Tue, 27 Aug 2024 at 09:36, Qiang Yu <quic_qianyu@quicinc.com> wrote:
>>>>>>>>> On platform x1e80100 QCP, PCIe3 is a standard x8 form factor. Hence, add
>>>>>>>>> support to use 3.3v, 3.3v aux and 12v regulators.
>>>>>>>> First of all, I don't see corresponding bindings change.
>>>>>>>>
>>>>>>>> Second, these supplies power up the slot, not the host controller
>>>>>>>> itself. As such these supplies do not belong to the host controller
>>>>>>>> entry. Please consider using the pwrseq framework instead.
>>>>>>>>
>>>>>>> Indeed. For legacy reasons, slot power supplies were populated in the host
>>>>>>> bridge node itself until recently Rob started objecting it [1]. And it makes
>>>>>>> real sense to put these supplies in the root port node and handle them in the
>>>>>>> relevant driver.
>>>>>>>
>>>>>>> I'm still evaluating whether the handling should be done in the portdrv or
>>>>>>> pwrctl driver, but haven't reached the conclusion. Pwrctl seems to be the ideal
>>>>>>> choice, but I see a few issues related to handling the OF node for the root
>>>>>>> port.
>>>>>>>
>>>>>>> Hope I'll come to a conclusion in the next few days and will update this thread.
>>>>>>>
>>>>>>> - Mani
>>>>>>>
>>>>>>> [1] https://lore.kernel.org/lkml/20240604235806.GA1903493-robh@kernel.org/
>>>>>> Hi Mani, do you have any updates?
>>>>>>
>>>>> I'm working with Bartosz to add a new pwrctl driver for rootports. And we are
>>>>> debugging an issue currently. Unfortunately, the progress is very slow as I'm on
>>>>> vacation still.
>>>>>
>>>>> Will post the patches once it got resolved.
>>>>>
>>>>> - Mani
>>>> OK, thanks for your update.
>>> Qiang, you can still resubmit the rest of the patches without having
>>> to wait on that to be resolved
>>>
>> In that case, the slot supplies should be described in the PCIe bridge.
> Patches 1-6 don't seem to depend on slot supplies, so they can be
> submitted separately.
OK, let me send v2 patch. Hi Mani, if you need any supports, please let 
me know.

Thanks,
Qiang
>
>> - Mani
>>
>>> Konrad
>> --
>> மணிவண்ணன் சதாசிவம்
>
>
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6f953e32d990..59fb415dfeeb 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -248,6 +248,8 @@  struct qcom_pcie_cfg {
 	bool no_l0s;
 };
 
+#define QCOM_PCIE_SLOT_MAX_SUPPLIES			3
+
 struct qcom_pcie {
 	struct dw_pcie *pci;
 	void __iomem *parf;			/* DT parf */
@@ -260,6 +262,7 @@  struct qcom_pcie {
 	struct icc_path *icc_cpu;
 	const struct qcom_pcie_cfg *cfg;
 	struct dentry *debugfs;
+	struct regulator_bulk_data slot_supplies[QCOM_PCIE_SLOT_MAX_SUPPLIES];
 	bool suspended;
 	bool use_pm_opp;
 };
@@ -1174,6 +1177,41 @@  static int qcom_pcie_link_up(struct dw_pcie *pci)
 	return !!(val & PCI_EXP_LNKSTA_DLLLA);
 }
 
+static int qcom_pcie_enable_slot_supplies(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(pcie->slot_supplies),
+				    pcie->slot_supplies);
+	if (ret < 0)
+		dev_err(pci->dev, "Failed to enable slot regulators\n");
+
+	return ret;
+}
+
+static void qcom_pcie_disable_slot_supplies(struct qcom_pcie *pcie)
+{
+	regulator_bulk_disable(ARRAY_SIZE(pcie->slot_supplies),
+			       pcie->slot_supplies);
+}
+
+static int qcom_pcie_get_slot_supplies(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	int ret;
+
+	pcie->slot_supplies[0].supply = "vpcie12v";
+	pcie->slot_supplies[1].supply = "vpcie3v3";
+	pcie->slot_supplies[2].supply = "vpcie3v3aux";
+	ret = devm_regulator_bulk_get(pci->dev, ARRAY_SIZE(pcie->slot_supplies),
+				      pcie->slot_supplies);
+	if (ret < 0)
+		dev_err(pci->dev, "Failed to get slot regulators\n");
+
+	return ret;
+}
+
 static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -1182,10 +1220,14 @@  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 
 	qcom_ep_reset_assert(pcie);
 
-	ret = pcie->cfg->ops->init(pcie);
+	ret = qcom_pcie_enable_slot_supplies(pcie);
 	if (ret)
 		return ret;
 
+	ret = pcie->cfg->ops->init(pcie);
+	if (ret)
+		goto err_disable_slot;
+
 	ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
 	if (ret)
 		goto err_deinit;
@@ -1216,7 +1258,8 @@  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 	phy_power_off(pcie->phy);
 err_deinit:
 	pcie->cfg->ops->deinit(pcie);
-
+err_disable_slot:
+	qcom_pcie_disable_slot_supplies(pcie);
 	return ret;
 }
 
@@ -1228,6 +1271,7 @@  static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
 	qcom_ep_reset_assert(pcie);
 	phy_power_off(pcie->phy);
 	pcie->cfg->ops->deinit(pcie);
+	qcom_pcie_disable_slot_supplies(pcie);
 }
 
 static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
@@ -1602,6 +1646,10 @@  static int qcom_pcie_probe(struct platform_device *pdev)
 			goto err_pm_runtime_put;
 	}
 
+	ret = qcom_pcie_get_slot_supplies(pcie);
+	if (ret)
+		goto err_pm_runtime_put;
+
 	ret = pcie->cfg->ops->get_resources(pcie);
 	if (ret)
 		goto err_pm_runtime_put;