| Message ID | 20210423160658.1542090-1-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | regulator: Fix current limit for QCOM PMIC VBUS | expand |
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)?
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
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
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.
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
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.
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
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; }
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(+) -- 2.30.1