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

Fabrice Gasnier fabrice.gasnier at foss.st.com
Mon Mar 4 09:50:27 PST 2024


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.

> 
>>
>>> 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) ?

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.
(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).

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 ?

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 ?

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 ?

> 
>>
>>> 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.

> 
>>
>>>
>>> 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.

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).


> 
>>
>>> 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() ?

 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.

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

> 
>>
>> 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
> 
> 
> 



More information about the linux-arm-kernel mailing list