Set the current load before enable regulator supplies at QUSB phy.

Encountered one issue where the board powered down instantly once the UVC
camera was attached to USB port while adding host mode on usb port and
testing a UVC camera with the driver on QCS615 platform. The extensible
boot loader mentioned that OCP(Over Current Protection) occurred at LDO12
from regulators-0 upon powered on board again. That indicates that the
current load set for QUSB phy, which use the regulator supply, is lower
than expected.

As per QUSB spec, set the maximum current load at 30mA to avoid overcurrent
load when attach a device to the USB port.

Fixes: 937e17f36a32 ("phy: qcom-qusb2: Power-on PHY before initialization")
Signed-off-by: Song Xue <quic_songxue@quicinc.com>
---
Changes in v2:
- Removed "---" above the Fixes. 
- Link to v1: https://lore.kernel.org/r/20241121-add_set_load_to_qusb_phy-v1-1-0f44f3a3290e@quicinc.com
---
 drivers/phy/qualcomm/phy-qcom-qusb2.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index c52655a383cef008552ed4533b9f31d1cbf34a13..80f0d17c42717e843937255a9a780bbae5998535 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -722,16 +722,27 @@ static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
 	return ret;
 }
 
+#define QUSB2PHY_HPM_LOAD 30000 /*uA*/
+
 static int qusb2_phy_init(struct phy *phy)
 {
 	struct qusb2_phy *qphy = phy_get_drvdata(phy);
 	const struct qusb2_phy_cfg *cfg = qphy->cfg;
 	unsigned int val = 0;
 	unsigned int clk_scheme;
-	int ret;
+	int ret, i;
 
 	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
 
+	/* set the current load */
+	for (i = 0; i < ARRAY_SIZE(qphy->vregs); i++) {
+		ret = regulator_set_load(qphy->vregs[i].consumer, QUSB2PHY_HPM_LOAD);
+		if (ret) {
+			dev_err(&phy->dev, "failed to set load at %s\n", qphy->vregs[i].supply);
+			return ret;
+		}
+	}
+
 	/* turn on regulator supplies */
 	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
 	if (ret)

---
base-commit: decc701f41d07481893fdea942c0ac6b226e84cd
change-id: 20241121-add_set_load_to_qusb_phy-d1327c797ffe

Best regards,
-- 
Song Xue <quic_songxue@quicinc.com>

On Thu, Nov 21, 2024 at 04:09:27PM +0800, Song Xue wrote:
> Set the current load before enable regulator supplies at QUSB phy.
> 
> Encountered one issue where the board powered down instantly once the UVC
> camera was attached to USB port while adding host mode on usb port and
> testing a UVC camera with the driver on QCS615 platform. The extensible
> boot loader mentioned that OCP(Over Current Protection) occurred at LDO12
> from regulators-0 upon powered on board again. That indicates that the
> current load set for QUSB phy, which use the regulator supply, is lower
> than expected.
> 
> As per QUSB spec, set the maximum current load at 30mA to avoid overcurrent
> load when attach a device to the USB port.
> 
> Fixes: 937e17f36a32 ("phy: qcom-qusb2: Power-on PHY before initialization")
> Signed-off-by: Song Xue <quic_songxue@quicinc.com>

The patch looks good. But if we describe the regulator(s) with
regulator-allow-set-load; and not all the consumers vote for load, the
sum of the load when USB phy is disabled goes to 0 and we will enter
LPM.

For this reason we're not doing any load requests today. Can you confirm
that this works fine with a dtb where only HPM is permitted (as well as
LPM and HPM)? If so I'd be in favor of us merging this change, but
keeping the dts HPM-only until someone confirms that all consumers of
these regulators specify load-votes.

Regards,
Bjorn

> ---
> Changes in v2:
> - Removed "---" above the Fixes. 
> - Link to v1: https://lore.kernel.org/r/20241121-add_set_load_to_qusb_phy-v1-1-0f44f3a3290e@quicinc.com
> ---
>  drivers/phy/qualcomm/phy-qcom-qusb2.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> index c52655a383cef008552ed4533b9f31d1cbf34a13..80f0d17c42717e843937255a9a780bbae5998535 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> @@ -722,16 +722,27 @@ static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
>  	return ret;
>  }
>  
> +#define QUSB2PHY_HPM_LOAD 30000 /*uA*/
> +
>  static int qusb2_phy_init(struct phy *phy)
>  {
>  	struct qusb2_phy *qphy = phy_get_drvdata(phy);
>  	const struct qusb2_phy_cfg *cfg = qphy->cfg;
>  	unsigned int val = 0;
>  	unsigned int clk_scheme;
> -	int ret;
> +	int ret, i;
>  
>  	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
>  
> +	/* set the current load */
> +	for (i = 0; i < ARRAY_SIZE(qphy->vregs); i++) {
> +		ret = regulator_set_load(qphy->vregs[i].consumer, QUSB2PHY_HPM_LOAD);
> +		if (ret) {
> +			dev_err(&phy->dev, "failed to set load at %s\n", qphy->vregs[i].supply);
> +			return ret;
> +		}
> +	}
> +
>  	/* turn on regulator supplies */
>  	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
>  	if (ret)
> 
> ---
> base-commit: decc701f41d07481893fdea942c0ac6b226e84cd
> change-id: 20241121-add_set_load_to_qusb_phy-d1327c797ffe
> 
> Best regards,
> -- 
> Song Xue <quic_songxue@quicinc.com>
> 
>

On 11/29/2024 12:43 AM, Bjorn Andersson wrote:
> On Thu, Nov 21, 2024 at 04:09:27PM +0800, Song Xue wrote:
>> Set the current load before enable regulator supplies at QUSB phy.
>>
>> Encountered one issue where the board powered down instantly once the UVC
>> camera was attached to USB port while adding host mode on usb port and
>> testing a UVC camera with the driver on QCS615 platform. The extensible
>> boot loader mentioned that OCP(Over Current Protection) occurred at LDO12
>> from regulators-0 upon powered on board again. That indicates that the
>> current load set for QUSB phy, which use the regulator supply, is lower
>> than expected.
>>
>> As per QUSB spec, set the maximum current load at 30mA to avoid overcurrent
>> load when attach a device to the USB port.
>>
>> Fixes: 937e17f36a32 ("phy: qcom-qusb2: Power-on PHY before initialization")
>> Signed-off-by: Song Xue <quic_songxue@quicinc.com>
> 
> The patch looks good. But if we describe the regulator(s) with
> regulator-allow-set-load; and not all the consumers vote for load, the
> sum of the load when USB phy is disabled goes to 0 and we will enter
> LPM.

That's exactly the issue we encountered on QCS615 ride. Qualcomm UFS 
driver sets load while USB phy doesn't set load. That's the reason we 
raised this patch.
> 
> For this reason we're not doing any load requests today. Can you confirm

When I grep regulator_set_load in Kernel, there are 27 hits in drivers. 
You are correct, it will trigger issue when some consumers set load 
while some don't.
However, how can we prevent other drivers outside of Qualcomm to use 
regulator_set_load? It will trigger the same issue.
Is there something we can do in regulator driver to prevent this issue? 
If consumer doesn't set load, regulator works in HPM even another 
consumer set load to 0?

> that this works fine with a dtb where only HPM is permitted (as well as
> LPM and HPM)? If so I'd be in favor of us merging this change, but

Do you mean test with HPM only regulator and regulator which allows to 
be set to HPM and LPM?

> keeping the dts HPM-only until someone confirms that all consumers of
> these regulators specify load-votes.
> 
> Regards,
> Bjorn
> 
>> ---
>> Changes in v2:
>> - Removed "---" above the Fixes.
>> - Link to v1: https://lore.kernel.org/r/20241121-add_set_load_to_qusb_phy-v1-1-0f44f3a3290e@quicinc.com
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qusb2.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> index c52655a383cef008552ed4533b9f31d1cbf34a13..80f0d17c42717e843937255a9a780bbae5998535 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> @@ -722,16 +722,27 @@ static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
>>   	return ret;
>>   }
>>   
>> +#define QUSB2PHY_HPM_LOAD 30000 /*uA*/
>> +
>>   static int qusb2_phy_init(struct phy *phy)
>>   {
>>   	struct qusb2_phy *qphy = phy_get_drvdata(phy);
>>   	const struct qusb2_phy_cfg *cfg = qphy->cfg;
>>   	unsigned int val = 0;
>>   	unsigned int clk_scheme;
>> -	int ret;
>> +	int ret, i;
>>   
>>   	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
>>   
>> +	/* set the current load */
>> +	for (i = 0; i < ARRAY_SIZE(qphy->vregs); i++) {
>> +		ret = regulator_set_load(qphy->vregs[i].consumer, QUSB2PHY_HPM_LOAD);
>> +		if (ret) {
>> +			dev_err(&phy->dev, "failed to set load at %s\n", qphy->vregs[i].supply);
>> +			return ret;
>> +		}
>> +	}
>> +
>>   	/* turn on regulator supplies */
>>   	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
>>   	if (ret)
>>
>> ---
>> base-commit: decc701f41d07481893fdea942c0ac6b226e84cd
>> change-id: 20241121-add_set_load_to_qusb_phy-d1327c797ffe
>>
>> Best regards,
>> -- 
>> Song Xue <quic_songxue@quicinc.com>
>>
>>


-- 
Thanks,
Tingwei

On Fri, Nov 29, 2024 at 06:26:30PM +0800, Tingwei Zhang wrote:
> On 11/29/2024 12:43 AM, Bjorn Andersson wrote:
> > On Thu, Nov 21, 2024 at 04:09:27PM +0800, Song Xue wrote:
> > > Set the current load before enable regulator supplies at QUSB phy.
> > > 
> > > Encountered one issue where the board powered down instantly once the UVC
> > > camera was attached to USB port while adding host mode on usb port and
> > > testing a UVC camera with the driver on QCS615 platform. The extensible
> > > boot loader mentioned that OCP(Over Current Protection) occurred at LDO12
> > > from regulators-0 upon powered on board again. That indicates that the
> > > current load set for QUSB phy, which use the regulator supply, is lower
> > > than expected.
> > > 
> > > As per QUSB spec, set the maximum current load at 30mA to avoid overcurrent
> > > load when attach a device to the USB port.
> > > 
> > > Fixes: 937e17f36a32 ("phy: qcom-qusb2: Power-on PHY before initialization")
> > > Signed-off-by: Song Xue <quic_songxue@quicinc.com>
> > 
> > The patch looks good. But if we describe the regulator(s) with
> > regulator-allow-set-load; and not all the consumers vote for load, the
> > sum of the load when USB phy is disabled goes to 0 and we will enter
> > LPM.
> 
> That's exactly the issue we encountered on QCS615 ride. Qualcomm UFS driver
> sets load while USB phy doesn't set load. That's the reason we raised this
> patch.
> > 
> > For this reason we're not doing any load requests today. Can you confirm
> 
> When I grep regulator_set_load in Kernel, there are 27 hits in drivers. You
> are correct, it will trigger issue when some consumers set load while some
> don't.
> However, how can we prevent other drivers outside of Qualcomm to use
> regulator_set_load?

We can avoid this by not specifying "regulator-allow-set-load" in
DeviceTree.

That said, this isn't the correct solution, we shouldn't use the
hardware description to deal with implementation issues. It is however
the currently chosen pragmatic solution, so feel free to follow this.

> It will trigger the same issue.
> Is there something we can do in regulator driver to prevent this issue? If
> consumer doesn't set load, regulator works in HPM even another consumer set
> load to 0?
> 
> > that this works fine with a dtb where only HPM is permitted (as well as
> > LPM and HPM)? If so I'd be in favor of us merging this change, but
> 
> Do you mean test with HPM only regulator and regulator which allows to be
> set to HPM and LPM?
> 

Yes, please double check that your patch works with and without
regulator-allow-set-load.

Regards,
Bjorn

> > keeping the dts HPM-only until someone confirms that all consumers of
> > these regulators specify load-votes.
> > 
> > Regards,
> > Bjorn
> > 
> > > ---
> > > Changes in v2:
> > > - Removed "---" above the Fixes.
> > > - Link to v1: https://lore.kernel.org/r/20241121-add_set_load_to_qusb_phy-v1-1-0f44f3a3290e@quicinc.com
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-qusb2.c | 13 ++++++++++++-
> > >   1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > index c52655a383cef008552ed4533b9f31d1cbf34a13..80f0d17c42717e843937255a9a780bbae5998535 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > @@ -722,16 +722,27 @@ static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
> > >   	return ret;
> > >   }
> > > +#define QUSB2PHY_HPM_LOAD 30000 /*uA*/
> > > +
> > >   static int qusb2_phy_init(struct phy *phy)
> > >   {
> > >   	struct qusb2_phy *qphy = phy_get_drvdata(phy);
> > >   	const struct qusb2_phy_cfg *cfg = qphy->cfg;
> > >   	unsigned int val = 0;
> > >   	unsigned int clk_scheme;
> > > -	int ret;
> > > +	int ret, i;
> > >   	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
> > > +	/* set the current load */
> > > +	for (i = 0; i < ARRAY_SIZE(qphy->vregs); i++) {
> > > +		ret = regulator_set_load(qphy->vregs[i].consumer, QUSB2PHY_HPM_LOAD);
> > > +		if (ret) {
> > > +			dev_err(&phy->dev, "failed to set load at %s\n", qphy->vregs[i].supply);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > >   	/* turn on regulator supplies */
> > >   	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
> > >   	if (ret)
> > > 
> > > ---
> > > base-commit: decc701f41d07481893fdea942c0ac6b226e84cd
> > > change-id: 20241121-add_set_load_to_qusb_phy-d1327c797ffe
> > > 
> > > Best regards,
> > > -- 
> > > Song Xue <quic_songxue@quicinc.com>
> > > 
> > > 
> 
> 
> -- 
> Thanks,
> Tingwei

On 11/30/2024 11:59 AM, Bjorn Andersson wrote:
> On Fri, Nov 29, 2024 at 06:26:30PM +0800, Tingwei Zhang wrote:
>> On 11/29/2024 12:43 AM, Bjorn Andersson wrote:
>>> On Thu, Nov 21, 2024 at 04:09:27PM +0800, Song Xue wrote:
>>>> Set the current load before enable regulator supplies at QUSB phy.
>>>>
>>>> Encountered one issue where the board powered down instantly once the UVC
>>>> camera was attached to USB port while adding host mode on usb port and
>>>> testing a UVC camera with the driver on QCS615 platform. The extensible
>>>> boot loader mentioned that OCP(Over Current Protection) occurred at LDO12
>>>> from regulators-0 upon powered on board again. That indicates that the
>>>> current load set for QUSB phy, which use the regulator supply, is lower
>>>> than expected.
>>>>
>>>> As per QUSB spec, set the maximum current load at 30mA to avoid overcurrent
>>>> load when attach a device to the USB port.
>>>>
>>>> Fixes: 937e17f36a32 ("phy: qcom-qusb2: Power-on PHY before initialization")
>>>> Signed-off-by: Song Xue <quic_songxue@quicinc.com>
>>>
>>> The patch looks good. But if we describe the regulator(s) with
>>> regulator-allow-set-load; and not all the consumers vote for load, the
>>> sum of the load when USB phy is disabled goes to 0 and we will enter
>>> LPM.
>>
>> That's exactly the issue we encountered on QCS615 ride. Qualcomm UFS driver
>> sets load while USB phy doesn't set load. That's the reason we raised this
>> patch.
>>>
>>> For this reason we're not doing any load requests today. Can you confirm
>>
>> When I grep regulator_set_load in Kernel, there are 27 hits in drivers. You
>> are correct, it will trigger issue when some consumers set load while some
>> don't.
>> However, how can we prevent other drivers outside of Qualcomm to use
>> regulator_set_load?
> 
> We can avoid this by not specifying "regulator-allow-set-load" in
> DeviceTree.
> 
> That said, this isn't the correct solution, we shouldn't use the
> hardware description to deal with implementation issues. It is however
> the currently chosen pragmatic solution, so feel free to follow this.
> 
In that case, shall we set all regulator to HPM-only and not specifying 
"regulator-allow-set-load" in device tree? This will make it safe 
whether driver sets load or not. With this, drivers can gradually add 
set load support and "regulator-allow-set-load" can be enabled in device 
tree once all consumer drivers have set load support.

>> It will trigger the same issue.
>> Is there something we can do in regulator driver to prevent this issue? If
>> consumer doesn't set load, regulator works in HPM even another consumer set
>> load to 0?
>>
>>> that this works fine with a dtb where only HPM is permitted (as well as
>>> LPM and HPM)? If so I'd be in favor of us merging this change, but
>>
>> Do you mean test with HPM only regulator and regulator which allows to be
>> set to HPM and LPM?
>>
> 
> Yes, please double check that your patch works with and without
> regulator-allow-set-load.
> 
Unfortunatly, it won't work for regulator with regulator-allow-set-load 
when there's another consumer driver doesn't set load. Once this USB phy 
driver disable regulator and clear it's load. Regualtor will go to LPM 
even other consumer still votes enable for this regulator.

> Regards,
> Bjorn
> 
>>> keeping the dts HPM-only until someone confirms that all consumers of
>>> these regulators specify load-votes.
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> ---
>>>> Changes in v2:
>>>> - Removed "---" above the Fixes.
>>>> - Link to v1: https://lore.kernel.org/r/20241121-add_set_load_to_qusb_phy-v1-1-0f44f3a3290e@quicinc.com
>>>> ---
>>>>    drivers/phy/qualcomm/phy-qcom-qusb2.c | 13 ++++++++++++-
>>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
>>>> index c52655a383cef008552ed4533b9f31d1cbf34a13..80f0d17c42717e843937255a9a780bbae5998535 100644
>>>> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
>>>> @@ -722,16 +722,27 @@ static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
>>>>    	return ret;
>>>>    }
>>>> +#define QUSB2PHY_HPM_LOAD 30000 /*uA*/
>>>> +
>>>>    static int qusb2_phy_init(struct phy *phy)
>>>>    {
>>>>    	struct qusb2_phy *qphy = phy_get_drvdata(phy);
>>>>    	const struct qusb2_phy_cfg *cfg = qphy->cfg;
>>>>    	unsigned int val = 0;
>>>>    	unsigned int clk_scheme;
>>>> -	int ret;
>>>> +	int ret, i;
>>>>    	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
>>>> +	/* set the current load */
>>>> +	for (i = 0; i < ARRAY_SIZE(qphy->vregs); i++) {
>>>> +		ret = regulator_set_load(qphy->vregs[i].consumer, QUSB2PHY_HPM_LOAD);
>>>> +		if (ret) {
>>>> +			dev_err(&phy->dev, "failed to set load at %s\n", qphy->vregs[i].supply);
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>>    	/* turn on regulator supplies */
>>>>    	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
>>>>    	if (ret)
>>>>
>>>> ---
>>>> base-commit: decc701f41d07481893fdea942c0ac6b226e84cd
>>>> change-id: 20241121-add_set_load_to_qusb_phy-d1327c797ffe
>>>>
>>>> Best regards,
>>>> -- 
>>>> Song Xue <quic_songxue@quicinc.com>
>>>>
>>>>
>>
>>
>> -- 
>> Thanks,
>> Tingwei


-- 
Thanks,
Tingwei

On Thu, Nov 21, 2024 at 04:09:27PM +0800, Song Xue wrote:
> Set the current load before enable regulator supplies at QUSB phy.
> 
> Encountered one issue where the board powered down instantly once the UVC
> camera was attached to USB port while adding host mode on usb port and
> testing a UVC camera with the driver on QCS615 platform. The extensible
> boot loader mentioned that OCP(Over Current Protection) occurred at LDO12
> from regulators-0 upon powered on board again. That indicates that the
> current load set for QUSB phy, which use the regulator supply, is lower
> than expected.
> 
> As per QUSB spec, set the maximum current load at 30mA to avoid overcurrent
> load when attach a device to the USB port.
> 
> Fixes: 937e17f36a32 ("phy: qcom-qusb2: Power-on PHY before initialization")
> Signed-off-by: Song Xue <quic_songxue@quicinc.com>
> ---
> Changes in v2:
> - Removed "---" above the Fixes. 
> - Link to v1: https://lore.kernel.org/r/20241121-add_set_load_to_qusb_phy-v1-1-0f44f3a3290e@quicinc.com
> ---
>  drivers/phy/qualcomm/phy-qcom-qusb2.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> index c52655a383cef008552ed4533b9f31d1cbf34a13..80f0d17c42717e843937255a9a780bbae5998535 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> @@ -722,16 +722,27 @@ static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
>  	return ret;
>  }
>  
> +#define QUSB2PHY_HPM_LOAD 30000 /*uA*/
> +
>  static int qusb2_phy_init(struct phy *phy)
>  {
>  	struct qusb2_phy *qphy = phy_get_drvdata(phy);
>  	const struct qusb2_phy_cfg *cfg = qphy->cfg;
>  	unsigned int val = 0;
>  	unsigned int clk_scheme;
> -	int ret;
> +	int ret, i;
>  
>  	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
>  
> +	/* set the current load */
> +	for (i = 0; i < ARRAY_SIZE(qphy->vregs); i++) {
> +		ret = regulator_set_load(qphy->vregs[i].consumer, QUSB2PHY_HPM_LOAD);

Please use regulator_set_mode() instead. Or just fix the mode in the
device tree, if the device can not operate if the regulator is in
non-HPM mode.

> +		if (ret) {
> +			dev_err(&phy->dev, "failed to set load at %s\n", qphy->vregs[i].supply);
> +			return ret;
> +		}
> +	}
> +
>  	/* turn on regulator supplies */
>  	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
>  	if (ret)
> 
> ---
> base-commit: decc701f41d07481893fdea942c0ac6b226e84cd
> change-id: 20241121-add_set_load_to_qusb_phy-d1327c797ffe
> 
> Best regards,
> -- 
> Song Xue <quic_songxue@quicinc.com>
> 

-- 
With best wishes
Dmitry

On 11/22/2024 6:24 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 21, 2024 at 04:09:27PM +0800, Song Xue wrote:
>> Set the current load before enable regulator supplies at QUSB phy.
>>
>> Encountered one issue where the board powered down instantly once the UVC
>> camera was attached to USB port while adding host mode on usb port and
>> testing a UVC camera with the driver on QCS615 platform. The extensible
>> boot loader mentioned that OCP(Over Current Protection) occurred at LDO12
>> from regulators-0 upon powered on board again. That indicates that the
>> current load set for QUSB phy, which use the regulator supply, is lower
>> than expected.
>>
>> As per QUSB spec, set the maximum current load at 30mA to avoid overcurrent
>> load when attach a device to the USB port.
>>
>> Fixes: 937e17f36a32 ("phy: qcom-qusb2: Power-on PHY before initialization")
>> Signed-off-by: Song Xue <quic_songxue@quicinc.com>
>> ---
>> Changes in v2:
>> - Removed "---" above the Fixes.
>> - Link to v1: https://lore.kernel.org/r/20241121-add_set_load_to_qusb_phy-v1-1-0f44f3a3290e@quicinc.com
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qusb2.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> index c52655a383cef008552ed4533b9f31d1cbf34a13..80f0d17c42717e843937255a9a780bbae5998535 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> @@ -722,16 +722,27 @@ static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
>>   	return ret;
>>   }
>>   
>> +#define QUSB2PHY_HPM_LOAD 30000 /*uA*/
>> +
>>   static int qusb2_phy_init(struct phy *phy)
>>   {
>>   	struct qusb2_phy *qphy = phy_get_drvdata(phy);
>>   	const struct qusb2_phy_cfg *cfg = qphy->cfg;
>>   	unsigned int val = 0;
>>   	unsigned int clk_scheme;
>> -	int ret;
>> +	int ret, i;
>>   
>>   	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
>>   
>> +	/* set the current load */
>> +	for (i = 0; i < ARRAY_SIZE(qphy->vregs); i++) {
>> +		ret = regulator_set_load(qphy->vregs[i].consumer, QUSB2PHY_HPM_LOAD);
> 
> Please use regulator_set_mode() instead. Or just fix the mode in the
> device tree, if the device can not operate if the regulator is in
> non-HPM mode.
> 
Thanks for comment.

 From my point, regulator_set_mode() will change the regulator's 
operating mode including current and voltage, which will also influence 
the other shared consumers. Meanwhile it is unacceptable to fix mode in 
the device tree because it is determined by regulator's device tree.

According to the required fix, regulator_set_load() simply aggregates 
the current load for the regulator and does not affect other shared 
consumers. Setting the current load is relevant to the issue.

Regards,
Song
>> +		if (ret) {
>> +			dev_err(&phy->dev, "failed to set load at %s\n", qphy->vregs[i].supply);
>> +			return ret;
>> +		}
>> +	}
>> +
>>   	/* turn on regulator supplies */
>>   	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
>>   	if (ret)
>>
>> ---
>> base-commit: decc701f41d07481893fdea942c0ac6b226e84cd
>> change-id: 20241121-add_set_load_to_qusb_phy-d1327c797ffe
>>
>> Best regards,
>> -- 
>> Song Xue <quic_songxue@quicinc.com>
>>
>

On Sat, Nov 23, 2024 at 01:07:06PM +0800, Song Xue wrote:
> 
> 
> On 11/22/2024 6:24 AM, Dmitry Baryshkov wrote:
> > On Thu, Nov 21, 2024 at 04:09:27PM +0800, Song Xue wrote:
> > > Set the current load before enable regulator supplies at QUSB phy.
> > > 
> > > Encountered one issue where the board powered down instantly once the UVC
> > > camera was attached to USB port while adding host mode on usb port and
> > > testing a UVC camera with the driver on QCS615 platform. The extensible
> > > boot loader mentioned that OCP(Over Current Protection) occurred at LDO12
> > > from regulators-0 upon powered on board again. That indicates that the
> > > current load set for QUSB phy, which use the regulator supply, is lower
> > > than expected.
> > > 
> > > As per QUSB spec, set the maximum current load at 30mA to avoid overcurrent
> > > load when attach a device to the USB port.
> > > 
> > > Fixes: 937e17f36a32 ("phy: qcom-qusb2: Power-on PHY before initialization")
> > > Signed-off-by: Song Xue <quic_songxue@quicinc.com>
> > > ---
> > > Changes in v2:
> > > - Removed "---" above the Fixes.
> > > - Link to v1: https://lore.kernel.org/r/20241121-add_set_load_to_qusb_phy-v1-1-0f44f3a3290e@quicinc.com
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-qusb2.c | 13 ++++++++++++-
> > >   1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > index c52655a383cef008552ed4533b9f31d1cbf34a13..80f0d17c42717e843937255a9a780bbae5998535 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > @@ -722,16 +722,27 @@ static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
> > >   	return ret;
> > >   }
> > > +#define QUSB2PHY_HPM_LOAD 30000 /*uA*/
> > > +
> > >   static int qusb2_phy_init(struct phy *phy)
> > >   {
> > >   	struct qusb2_phy *qphy = phy_get_drvdata(phy);
> > >   	const struct qusb2_phy_cfg *cfg = qphy->cfg;
> > >   	unsigned int val = 0;
> > >   	unsigned int clk_scheme;
> > > -	int ret;
> > > +	int ret, i;
> > >   	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
> > > +	/* set the current load */
> > > +	for (i = 0; i < ARRAY_SIZE(qphy->vregs); i++) {
> > > +		ret = regulator_set_load(qphy->vregs[i].consumer, QUSB2PHY_HPM_LOAD);
> > 
> > Please use regulator_set_mode() instead. Or just fix the mode in the
> > device tree, if the device can not operate if the regulator is in
> > non-HPM mode.
> > 
> Thanks for comment.
> 
> From my point, regulator_set_mode() will change the regulator's operating
> mode including current and voltage, which will also influence the other
> shared consumers. Meanwhile it is unacceptable to fix mode in the device
> tree because it is determined by regulator's device tree.
> 
> According to the required fix, regulator_set_load() simply aggregates the
> current load for the regulator and does not affect other shared consumers.
> Setting the current load is relevant to the issue.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> Regards,
> Song
> > > +		if (ret) {
> > > +			dev_err(&phy->dev, "failed to set load at %s\n", qphy->vregs[i].supply);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > >   	/* turn on regulator supplies */
> > >   	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
> > >   	if (ret)
> > > 
> > > ---
> > > base-commit: decc701f41d07481893fdea942c0ac6b226e84cd
> > > change-id: 20241121-add_set_load_to_qusb_phy-d1327c797ffe
> > > 
> > > Best regards,
> > > -- 
> > > Song Xue <quic_songxue@quicinc.com>
> > > 
> > 
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
With best wishes
Dmitry