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

[Sean Nyekjaer]

Hi,

> On 4 Mar 2024, at 18.50, Fabrice Gasnier <fabrice.gasnier at foss.st.com> wrote:
> 
> On 3/4/24 09:30, Sean Nyekjaer wrote:
>> 
>> 
>>> 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.
> 
> Hi Sean,
> 
> That's needed for low power (e.g. suspend). You may find upon suspend,
> HCD driver calls phy_exit() but still needs to access registers, before
> clk_disable() gets called. The PHY clock on MP1 is still needed in
> between the two calls. Hence, the clock is exposed and used for this low
> power sequence.

Fortunately we are not using suspend :)

> 
>> 
>>> 
>>>> 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.
> 
> So these regulators are all 'off', out of u-boot, before starting the
> kernel (toggling reg18 should then have no effect) ?

Yes they are all off, then reg18 is powered on and then vdd_usb.
reg18 is then briefly toggled. Because the clk framework is deactivating it.

> 
> As you confirmed the above behavior (reg18 toggles on-off) during
> stm32_usbphyc_clk48_register(), how is the vdd_usb regulator "on" when
> booting the kernel ? I don't get it.

stm32-pwr.c is adding a consumer to the vdd_usb.
root at hostname:~# cat /sys/class/regulator/regulator.8/name
vdd_usb
root at hostname:~# cat /sys/class/regulator/regulator.8/num_users
4
root at hostname:~# cat /sys/class/regulator/regulator.8/regulator.17-SUPPLY/name
usb33

> (The clock reparent operation comes during USBPHYC driver probe, it
> doesn't involve vdd_usb, no consumer should have had an opportunity to
> enable vdd_usb before that).

See above.

> 
> Please clarify the boot chain you're using. Is it on u-boot ? (SPL?
> TF-A? Is OPTEE involved ? ))
> I can't find the board DT in u-boot, is there some specific code / dts
> for this board in u-boot ?

No TF-A, no OPTEE.
I have copied the device tree from Linux.

> 
> U-boot normally doesn't probe the USB during normal boot from sd-card
> for example. Hence, these regulators are normally untouched by u-boot.
> 
> With stm32mp157c-dk2.dts, I can see when breaking boot (SPL config):
> STM32MP> regulator status
> Name                 Enabled            uV         mA Mode       Status
> reg11                enabled       1100000          - -          0
> reg18                enabled       1800000          - -          0
> usb33                disabled      3300000          - -          0
> ...
> vdd_usb              enabled       3300000          - -          0
> 
> Please clarify the use case? I'd like to avoid any confusion.
> Or, USB is used in your case before booting ?

I’m USB booting from u-boot.
But it doesn’t make a difference with the toggling of reg18, if the initial state is on/off.

> 
> In such case, after "usb start", or any other device class command in
> u-boot (ums, dfu...), all these regulators would be disabled in the end.
> 
> Could you check and report "regulator status" command output in u-boot,
> before boot ?

Before booting the kernel u-boot will exit the usb stack and power off related regulators. 

See here:
https://first.geanix.com/boot-linux.jpg

The Yellow line, is reg18 and the purple is vdd_usb.
If the initial state of reg18 is LOW doesn’t make a difference, as the toggling happens later :)

With my proposed fix (removing the vdd_3v3_usbfs, thus breaking otg)

https://first.geanix.com/boot-linux-good.jpg

Hope that clarifies.

> 
>> 
>>> 
>>>> 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 don't understand the condition here: PWR regulators are in the middle
> of the power tree, in between PMIC and USBPHYC or dwc2 consumers. PWR
> doesn't / shouldn't enable vdd_usb on its own: it rather cascades
> enable/disable requests from consumers, to the parent regulator. This
> should happen after the toggling: e.g. after USBPHC driver probe (clk
> reparent), when consumers are initializing.
> 
> Please clarify.

See above.

> 
>> 
>>> 
>>>> 
>>>> 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.
> 
> I noticed Mark's reply after I actually answered on friday.
> 
> If low power is out of scope, that may be a workaround. As he states,
> it's not ideal.
> 

We are running with this to avoid further hardware damage.

> Better approach (likely more reliable) would be to have some way to deal
> with hardware constraints as he suggested, e.g. not to disable a
> regulator (reg18), unless another regulator is off (vdd_usb).

Agree.

> 
> 
>> 
>>> 
>>>> 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.
> 
> The main difference in u-boot seems that vdd_usb is en(dis)able from
> phy_power_on(off). I haven't inspected the sequence for the clock
> provider in u-boot: the same scheme as in kernel to enable the PLL is
> implemented. Perhaps u-boot simply doesn't disable the clock after all.
> 
> I don't know if this could be acceptable, in Linux, to manage the
> vdd_usb also in stm32_usbphyc_regulators_enable() /
> stm32_usbphyc_regulators_disable() ?

I don’t know either :)

> 
> static int stm32_usbphyc_regulators_enable(struct stm32_usbphyc *usbphyc)
> {
> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys[0];
> int ret;
> 
> ...
> ret = regulator_enable(usbphyc->vdda1v8);
> if (ret)
> goto vdda1v1_disable;
> 
> + ret = regulator_enable(usbphyc_phy->phy->pwr);
> + if (ret)
> + goto vdda1v8_disable;
> +
> return 0;
> 
> +vdda1v8_disable:
> + dev_info(usbphyc->dev, "vdda1v8 disable\n");
> + regulator_disable(usbphyc->vdda1v8);
> 
> And the opposite in stm32_usbphyc_regulators_disable().
> 
> Doing it, will initialize vdd_usb regulator ref counting, e.g. 1 then 0
> when registering the USBPHYC clock. So it will be managed in the correct
> order (turned off before reg18).
> 
> It will work, if and only if, vdd_usb isn't shared on the board, with
> another function, this could enforce from phy driver the correct
> enable/disable sequence.

One could share the vdd_usb on the board. In those cases we need to avoid powering reg18 off.
When entering stm32_usbphyc_regulators_disable() and the phy is still powered on we should block power off.
In my first mail I added a warning if the this happens...

> 
> If the vdd_usb regulator is share with some other function, then I see
> no other way, but to look into Mark's suggestion to better control
> hardware constraints.
> 
> Hope this helps,
> Please clarify above points,
> Best Regards,
> Fabrice
> 

Sorry for the pictures on our own server, what is folks using for sharing pictures on the mailing lists?

/Sean

[…]