[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