[PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[Bryan O'Donoghue]

VBUS current at +5 V on the pm8150b can be.

500 mA
1000 mA
1500 mA
2000 mA
2500 mA
3000 mA

Only 500 mA, 1 A, 1.5 A or 3 A is valid with respect to the standard.
Right now the first stage bootloader sets the value to 2 A. 2 A is just
fine until you connect a chunky enough type-c accessory.

Debugging a separate USB problem I noticed that a larger type-c dongle I
had was ramping VBUS up and then collapsing, never getting to +5 V.
Different dongles would get to +5 V and importantly downstream would
happily power the bigger dongle.

The root cause is failure to set the higher current limit to 3 A instead of
the defaulting 2 A from the bootloader.

Fixes: 4fe66d5a62fb ("regulator: Add support for QCOM PMIC VBUS booster")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/regulator/qcom_usb_vbus-regulator.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/regulator/qcom_usb_vbus-regulator.c b/drivers/regulator/qcom_usb_vbus-regulator.c
index 457788b505720..2b19e0483fccd 100644
--- a/drivers/regulator/qcom_usb_vbus-regulator.c
+++ b/drivers/regulator/qcom_usb_vbus-regulator.c
@@ -16,6 +16,14 @@
 
 #define CMD_OTG				0x40
 #define OTG_EN				BIT(0)
+#define OTG_CURRENT_LIMIT_CFG		0x52
+#define OTG_CURRENT_LIMIT_500MA		0
+#define OTG_CURRENT_LIMIT_1000MA	BIT(0)
+#define OTG_CURRENT_LIMIT_1500MA	BIT(1)
+#define OTG_CURRENT_LIMIT_2000MA	(BIT(1) | BIT(0))
+#define OTG_CURRENT_LIMIT_2500MA	BIT(2)
+#define OTG_CURRENT_LIMIT_3000MA	(BIT(2) | BIT(0))
+#define OTG_CURRENT_LIMIT_MASK		(BIT(2) | BIT(0))
 #define OTG_CFG				0x53
 #define OTG_EN_SRC_CFG			BIT(1)
 
@@ -76,6 +84,10 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
 	/* Disable HW logic for VBUS enable */
 	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG, 0);
 
+	/* Set OTG current limit to 3000mA up from bootloader set 2000mA */
+	regmap_update_bits(regmap, base + OTG_CURRENT_LIMIT_CFG,
+			   OTG_CURRENT_LIMIT_MASK, OTG_CURRENT_LIMIT_3000MA);
+
 	return 0;
 }
 
-- 
2.30.1

Re: [PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

On Fri, Apr 23, 2021 at 05:06:58PM +0100, Bryan O'Donoghue wrote:

> +	/* Set OTG current limit to 3000mA up from bootloader set 2000mA */
> +	regmap_update_bits(regmap, base + OTG_CURRENT_LIMIT_CFG,
> +			   OTG_CURRENT_LIMIT_MASK, OTG_CURRENT_LIMIT_3000MA);

This seems like something that needs to be configured per system, the
system may not be physically capable of delivering an additional amp
(150% of the current defaults) safely.  It's going to be better to be
out of spec for high current USB devices than to exceed safe physical
limits, there's a good solid reason why the regulator API never touches
the hardware without explicit constraints allowing it to do so.

I also thought there was support in the USB specs for negotiating how
much power is drawn (not that everything will DTRT there but still)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[Bryan O'Donoghue]

On 23/04/2021 17:32, Mark Brown wrote:
> On Fri, Apr 23, 2021 at 05:06:58PM +0100, Bryan O'Donoghue wrote:
> 
>> +	/* Set OTG current limit to 3000mA up from bootloader set 2000mA */
>> +	regmap_update_bits(regmap, base + OTG_CURRENT_LIMIT_CFG,
>> +			   OTG_CURRENT_LIMIT_MASK, OTG_CURRENT_LIMIT_3000MA);
> 
> This seems like something that needs to be configured per system, the
> system may not be physically capable of delivering an additional amp
> (150% of the current defaults) safely.  It's going to be better to be
> out of spec for high current USB devices than to exceed safe physical
> limits, there's a good solid reason why the regulator API never touches
> the hardware without explicit constraints allowing it to do so.

You're right that its per system.

- smb5_pmi632_params.otg_cl.max_u == 1000000
- smb5_pm8150b_params.otg_cl.max_u == 3000000

https://github.com/BeastRoms-Devices/kernel_realme_RMX1927/blame/master/drivers/power/supply/qcom/qpnp-smb5.c

See page 3 here

https://www.st.com/content/ccc/resource/sales_and_marketing/presentation/product_presentation/group0/5a/b1/8e/6c/2b/0d/46/3c/Apec/files/APEC_2016_USB_Power.pdf/_jcr_content/translations/en.APEC_2016_USB_Power.pdf

and section 1.2.2 here

https://cdn.sparkfun.com/assets/e/b/4/f/7/USB-C_Datasheet.pdf

For type-c 3 Amps at 5 Volts is correct or 1.5 Amps at 5 Volts, either 
way the default coming out of the bootloader at 2 Amps is wrong.

> I also thought there was support in the USB specs for negotiating how
> much power is drawn (not that everything will DTRT there but still)?

Right the Power-delivery stuff up to 100 watts is associated with Vconn 
which is driven on CC1 or CC2 depending on the orientation of the cable.

https://www.allaboutcircuits.com/uploads/articles/Fig1m11292018.png

This patch deals with the higher spec VBUS current that the pm8150b can 
drive, not the power-delivery protocol juice over VCONN.

---
bod

Re: [PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[Bryan O'Donoghue]

On 23/04/2021 21:24, Bryan O'Donoghue wrote:
> On 23/04/2021 17:32, Mark Brown wrote:
>> On Fri, Apr 23, 2021 at 05:06:58PM +0100, Bryan O'Donoghue wrote:
>>
>>> +    /* Set OTG current limit to 3000mA up from bootloader set 2000mA */
>>> +    regmap_update_bits(regmap, base + OTG_CURRENT_LIMIT_CFG,
>>> +               OTG_CURRENT_LIMIT_MASK, OTG_CURRENT_LIMIT_3000MA);
>>
>> This seems like something that needs to be configured per system, the
>> system may not be physically capable of delivering an additional amp
>> (150% of the current defaults) safely.  It's going to be better to be
>> out of spec for high current USB devices than to exceed safe physical
>> limits, there's a good solid reason why the regulator API never touches
>> the hardware without explicit constraints allowing it to do so.
> 
> You're right that its per system.
> 
> - smb5_pmi632_params.otg_cl.max_u == 1000000
> - smb5_pm8150b_params.otg_cl.max_u == 3000000
> 
> https://github.com/BeastRoms-Devices/kernel_realme_RMX1927/blame/master/drivers/power/supply/qcom/qpnp-smb5.c 
> 
> 
> See page 3 here
> 
> https://www.st.com/content/ccc/resource/sales_and_marketing/presentation/product_presentation/group0/5a/b1/8e/6c/2b/0d/46/3c/Apec/files/APEC_2016_USB_Power.pdf/_jcr_content/translations/en.APEC_2016_USB_Power.pdf 
> 
> 
> and section 1.2.2 here
> 
> https://cdn.sparkfun.com/assets/e/b/4/f/7/USB-C_Datasheet.pdf
> 
> For type-c 3 Amps at 5 Volts is correct or 1.5 Amps at 5 Volts, either 
> way the default coming out of the bootloader at 2 Amps is wrong.
> 
>> I also thought there was support in the USB specs for negotiating how
>> much power is drawn (not that everything will DTRT there but still)?
> 
> Right the Power-delivery stuff up to 100 watts is associated with Vconn 
> which is driven on CC1 or CC2 depending on the orientation of the cable.
> 
> https://www.allaboutcircuits.com/uploads/articles/Fig1m11292018.png
> 
> This patch deals with the higher spec VBUS current that the pm8150b can 
> drive, not the power-delivery protocol juice over VCONN.
> 
> ---
> bod

Oh and I should mention the pm8150b is a type-c controller and battery 
charger block, so you wouldn't connect it to a USB 3.x type-a port.

The Qualcomm reference schematics drive VBUS for those ports from a 
current limiter - a SY6288F1ABC - which steps down 5v/4amp rail to 
5v/1.2amp.

So the pm8150b is exclusively for the type-c not type-a ports.

---
bod

Re: [PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Fri, Apr 23, 2021 at 09:24:53PM +0100, Bryan O'Donoghue wrote:

> For type-c 3 Amps at 5 Volts is correct or 1.5 Amps at 5 Volts, either way
> the default coming out of the bootloader at 2 Amps is wrong.

It may be out of spec for USB C (though I can see someone choosing a
limit of 2A to give headroom on a specified 1.5A limit, AIUI the limits
in USB are on the side drawing the current and there's no requirement
for the supplier to enforce the limit) but that doesn't mean that the
hardware has been designed in a way that makes it safe to just increase
the limit.

> This patch deals with the higher spec VBUS current that the pm8150b can
> drive, not the power-delivery protocol juice over VCONN.

What individual components can do here isn't important, what is
important is the constrainst that the system has.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[Bryan O'Donoghue]

On 26/04/2021 12:55, Mark Brown wrote:
> It may be out of spec for USB C (though I can see someone choosing a
> limit of 2A to give headroom on a specified 1.5A limit, AIUI the limits
> in USB are on the side drawing the current and there's no requirement
> for the supplier to enforce the limit) but that doesn't mean that the
> hardware has been designed in a way that makes it safe to just increase
> the limit.

I get your point, however, the downstream kernel i.e. the kernel that 
ships with the n-million qcom devices sets the limit to 3 amps.

To my knowledge all qcom downstream hardware/software implementations 
based on the pm8150b set the 3amp limit so, what we are doing here is 
closing the gap in upstream.

---
bod

Re: [PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

On Mon, Apr 26, 2021 at 01:03:49PM +0100, Bryan O'Donoghue wrote:

> I get your point, however, the downstream kernel i.e. the kernel that ships
> with the n-million qcom devices sets the limit to 3 amps.

We've not traditionally used downstream kernels as a quality guide, nor
can we assume that every system running upstream kernels is running
unmodified downstream kernels.

To repeat my original feedback: this needs to be something that is
configured per system, please make it configured per system.  Simply
repeating yourself over and over again is not going to accomplish
anything constructive.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: Fix current limit for QCOM PMIC VBUS

[Bryan O'Donoghue]

On 26/04/2021 13:12, Mark Brown wrote:
> To repeat my original feedback: this needs to be something that is
> configured per system, please make it configured per system.

Sure thing,

It doesn't look like a big problem to add a set_current_limit for this 
regulator.

---
bod