[Linux-stm32] stm32mp1xx: use of reg11, reg18 and vdd_usb rails

[Sean Nyekjaer]

> On 1 Mar 2024, at 18.58, Fabrice Gasnier <fabrice.gasnier at foss.st.com> wrote:
> 
> On 3/1/24 15:41, Sean Nyekjaer wrote:
>> Hi all,
>> 
>> We are using the osd32mp1 SIP module [0].
>> We are seeing some hardware get’s fried inside the SIP module.
>> It’s somewhat traced down to the usb controller/phy/regulator.
>> 
>> With this device tree[1]. We have noticed during boot the reg18 is toggled on and off
> 
> Dear Sean,

Hi Fabrice,

> 
> I've tried to check what you've pointed out.
> 
> The toggling happens when registering the PHY as a clock provider. The
> USB PHY has a PLL to provide clock for OTG and USBH. This clock gets
> registered to the clock framework, as they go through RCC.
> 
> stm32_usbphyc_clk48_register() -> clk_hw_register()
> 
> In order to properly be inserted into the clock tree, (e.g. RCC does
> some gating then) reparent operation requires the PLL (with its
> supplies) to be enabled. Once the reparent is completed, it requests to
> turn it OFF.
> 
> That's the reason for the toggling.

Also our conclusion. A rather complex setup.

> 
>> without vdd_usb being turned off before reg18 as required in the data sheet[2], section 3.8.1:
> 
> vdd_usb is problably (I haven't checked) a boot-on regulator, totally
> untouched when the toggling happens. It gets enabled in drivers later,
> during phy_power_on() or in dwc2 driver (stm id glue / usb33d cascaded
> then to pwr). So it isn't controlled before that.

reg18 is boot-on because the BYPASS_REG1V8 is pulled low, reg18 is fed by VDD, which is has a lower PMIC ranking than vdd_usb/ldo4.
U-boot is actually powering off reg11, reg18 and vdd_usb(in the correct order). Before starting the kernel.

> 
>> VDD3V3_USBHS must not be present unless VDDA1V8_REG is present, otherwise permanent 
>> STM32MP157C/F damage could occur. Must be ensured by PMIC ranking order or with
>> external component in case of discrete component power supply implementation.
>> 
>> It’s happens because the something is already uses the vdd_usb, it’s both the drivers/phy/st/phy-stm32-usbphyc.c
>> and drivers/regulator/stm32-pwr.c that consumes it.
> 
> No (see above).

Yes, the stm32-pwr.c consumes the vdd_usb, and blocks the dwc2 driver from powering it off.

> 
>> 
>> I can fix it by removing the vdd_usb from the usb33 supply[3]:
> 
> This will break all implementations that rely on ID/Vbus pins on MP15.

OK. So we will have to use Mark’s suggestion and force it on.

> 
>> The stm32-pwr.c is (to me) rather weird, as it exposes the usb33 as a regulator when in fact it’s a detection pin.
>> Is that done on purpose?
>> 
>> I would like introduce a error in the stm32-pwr.c if something is trying to power off reg18 with usb33 present?
> 
> Well, adding some error as you have drafted should protect the hardware.
> Doesn't this brings error, when registering into the clock framework ?
> Does this prevent registering USB then ?
> 
> There's probably better options. It needs additional fix. I can't think
> about right now...
> It is just a thought, but when the PHY driver registers the clock, a
> better control of all the regulator 1v1, 1v8 and vdd_usb could be to
> enforce vdd_usb first gets disabled in this process.

This is how u-boot is handling things. The driver hard controls all regulators.

> 
> Or temporarily flag this initialization step, from
> stm32_usbphyc_clk48_register(), until phy_init() occurs, so the 1v1 and
> 1v8 don't get disabled ? This will spare time (e.g. toggling) as
> phy_init will reenable all these just few time after it's been disabled.
> 

So I should try to check init_count in stm32_usbphyc_clk48_register?

>> Would it be okay to return -EBUSY? Or even -ESMOKE? :)
>> 
>> Or is it better to do it in phy-stm32-usbphyc.c[4]?
>> 
>> /Sean
>> 
>> [0]: https://octavosystems.com/octavo_products/osd32mp15x/
>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/st/stm32mp157c-osd32mp1-red.dts
>> [2]: https://www.st.com/resource/en/datasheet/stm32mp157c.pdf
>> [3]:
>> diff --git a/arch/arm/boot/dts/st/stm32mp157c-osd32mp1-red.dts b/arch/arm/boot/dts/st/stm32mp157c-osd32mp1-red.dts
>> index 527c33be66cc..0d67006806c4 100644
>> --- a/arch/arm/boot/dts/st/stm32mp157c-osd32mp1-red.dts
>> +++ b/arch/arm/boot/dts/st/stm32mp157c-osd32mp1-red.dts
>> @@ -149,7 +149,6 @@ &m_can1 {
>> 
>> &pwr_regulators {
>>        vdd-supply = <&vdd>;
>> -       vdd_3v3_usbfs-supply = <&vdd_usb>;
> 
> As said above, this will make the ID and Vbus detection logic on OTG
> port not working.]

Ok, we are not using it. 

> 
>> };
>> 
>> &rtc {
>> [4]:
>> diff --git a/drivers/phy/st/phy-stm32-usbphyc.c b/drivers/phy/st/phy-stm32-usbphyc.c
>> index d5e7e44000b5..58fcc3099803 100644
>> --- a/drivers/phy/st/phy-stm32-usbphyc.c
>> +++ b/drivers/phy/st/phy-stm32-usbphyc.c
>> @@ -188,8 +188,18 @@ static int stm32_usbphyc_regulators_enable(struct stm32_usbphyc *usbphyc)
>> 
>> static int stm32_usbphyc_regulators_disable(struct stm32_usbphyc *usbphyc)
>> {
>> +       struct stm32_usbphyc_phy *usbphyc_phy;
>>        int ret;
>> 
>> +       for (port = 0; port < usbphyc->nphys; port++) {
>> +               usbphyc_phy = usbphyc->phys[port];
>> +
>> +               if(regulator_is_enabled(usbphyc_phy->phy->pwr)) {
>> +                       pr_err("%s: phy is powered not allowed to switch off regulator\n", __func__);
>> +                       return -EBUSY;
>> +               }
>> +       }
>> +
> 
> As above, this could make sense to flag the error.
> But it needs some handling to properly avoid the toggling, or make it safe.

Yes, we also need to make sure we aren’t bricking anything in the clk and regulator framework by returning error.

/Sean

> 
> Hope this helps to clarify,
> BR,
> Fabrice
> 
>>        ret = regulator_disable(usbphyc->vdda1v8);
>>        if (ret)
>>                return ret;
>> 
>> 
>> _______________________________________________
>> Linux-stm32 mailing list
>> Linux-stm32 at st-md-mailman.stormreply.com
>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32