stm32mp1xx: use of reg11, reg18 and vdd_usb rails

stm32mp1xx: use of reg11, reg18 and vdd_usb rails

[Sean Nyekjaer]

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
without vdd_usb being turned off before reg18 as required in the data sheet[2], section 3.8.1:
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.

I can fix it by removing the vdd_usb from the usb33 supply[3]:
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?
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>;
 };

 &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;
+               }
+       }
+
        ret = regulator_disable(usbphyc->vdda1v8);
        if (ret)
                return ret;



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

stm32mp1xx: use of reg11, reg18 and vdd_usb rails

[Sean Nyekjaer]

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
without vdd_usb being turned off before reg18 as required in the data sheet[2], section 3.8.1:
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.

I can fix it by removing the vdd_usb from the usb33 supply[3]:
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?
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>;
 };

 &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;
+               }
+       }
+
        ret = regulator_disable(usbphyc->vdda1v8);
        if (ret)
                return ret;



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: stm32mp1xx: use of reg11, reg18 and vdd_usb rails

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1732 bytes --]

On Fri, Mar 01, 2024 at 03:41:08PM +0100, Sean Nyekjaer wrote:

> With this device tree[1]. We have noticed during boot the reg18 is toggled on and off
> without vdd_usb being turned off before reg18 as required in the data sheet[2], section 3.8.1:
> 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.

The most expedient way of dealing with this might be to mark the 1.8V
supply as always on.  That's not ideal if you've got low power use cases
though.

We don't actually have a facility for forcing one supply to be on
whenever another is on - it's something I was expecting someone to have
a need for but it never came up as a runtime issue before, there is
support for keeping the voltage different between two supplies within a
limit which I'd expect would look similar.

> I can fix it by removing the vdd_usb from the usb33 supply[3]:
> 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?

If this is not a regulator then it quite simply should not be exposed
via the regulator API.  People keep abusing it rather than implement
reference counting....

> I would like introduce a error in the stm32-pwr.c if something is trying to power off reg18 with usb33 present?
> Would it be okay to return -EBUSY? Or even -ESMOKE? :)

You'd also need to consider what's going to happen with the error but
yes, if the driver knows it's unsafe in all circumstances it's probably
a good idea to prevent the operation.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: stm32mp1xx: use of reg11, reg18 and vdd_usb rails

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1732 bytes --]

On Fri, Mar 01, 2024 at 03:41:08PM +0100, Sean Nyekjaer wrote:

> With this device tree[1]. We have noticed during boot the reg18 is toggled on and off
> without vdd_usb being turned off before reg18 as required in the data sheet[2], section 3.8.1:
> 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.

The most expedient way of dealing with this might be to mark the 1.8V
supply as always on.  That's not ideal if you've got low power use cases
though.

We don't actually have a facility for forcing one supply to be on
whenever another is on - it's something I was expecting someone to have
a need for but it never came up as a runtime issue before, there is
support for keeping the voltage different between two supplies within a
limit which I'd expect would look similar.

> I can fix it by removing the vdd_usb from the usb33 supply[3]:
> 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?

If this is not a regulator then it quite simply should not be exposed
via the regulator API.  People keep abusing it rather than implement
reference counting....

> I would like introduce a error in the stm32-pwr.c if something is trying to power off reg18 with usb33 present?
> Would it be okay to return -EBUSY? Or even -ESMOKE? :)

You'd also need to consider what's going to happen with the error but
yes, if the driver knows it's unsafe in all circumstances it's probably
a good idea to prevent the operation.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Fabrice Gasnier]

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,

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.

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

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

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

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

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.

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

>  };
> 
>  &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.

Hope this helps to clarify,
BR,
Fabrice

>         ret = regulator_disable(usbphyc->vdda1v8);
>         if (ret)
>                 return ret;
> 
> 
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Fabrice Gasnier]

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,

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.

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

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

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

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

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.

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

>  };
> 
>  &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.

Hope this helps to clarify,
BR,
Fabrice

>         ret = regulator_disable(usbphyc->vdda1v8);
>         if (ret)
>                 return ret;
> 
> 
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sean Nyekjaer]

> On 1 Mar 2024, at 18.58, Fabrice Gasnier <fabrice.gasnier@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@st-md-mailman.stormreply.com
>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32




-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Sean Nyekjaer]

> On 1 Mar 2024, at 18.58, Fabrice Gasnier <fabrice.gasnier@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@st-md-mailman.stormreply.com
>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Fabrice Gasnier]

On 3/4/24 09:30, Sean Nyekjaer wrote:
> 
> 
>> On 1 Mar 2024, at 18.58, Fabrice Gasnier <fabrice.gasnier@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@st-md-mailman.stormreply.com
>>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
> 
> 
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Fabrice Gasnier]

On 3/4/24 09:30, Sean Nyekjaer wrote:
> 
> 
>> On 1 Mar 2024, at 18.58, Fabrice Gasnier <fabrice.gasnier@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@st-md-mailman.stormreply.com
>>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@foss.st.com> wrote:
> 
> On 3/4/24 09:30, Sean Nyekjaer wrote:
>> 
>> 
>>> On 1 Mar 2024, at 18.58, Fabrice Gasnier <fabrice.gasnier@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@hostname:~# cat /sys/class/regulator/regulator.8/name
vdd_usb
root@hostname:~# cat /sys/class/regulator/regulator.8/num_users
4
root@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

[…]




-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [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@foss.st.com> wrote:
> 
> On 3/4/24 09:30, Sean Nyekjaer wrote:
>> 
>> 
>>> On 1 Mar 2024, at 18.58, Fabrice Gasnier <fabrice.gasnier@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@hostname:~# cat /sys/class/regulator/regulator.8/name
vdd_usb
root@hostname:~# cat /sys/class/regulator/regulator.8/num_users
4
root@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

[…]




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sean Nyekjaer]

Hi Fabrice,

[…]

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

Should I add a lock in stm32_pwr.c to protect the stm32 hardware?

[…]

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


/Sean

[…]




-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Sean Nyekjaer]

Hi Fabrice,

[…]

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

Should I add a lock in stm32_pwr.c to protect the stm32 hardware?

[…]

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


/Sean

[…]




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel