2021-06-17 21:29:29

by Anand Moon

[permalink] [raw]
Subject: [RFCv1 7/8] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode.

Power off the PHY by putting it into reset mode.
Drop the phy power reset since we are doing reset of phy
after we configure the phy. No functional change.

Cc: Martin Blumenstingl <[email protected]>
Signed-off-by: Anand Moon <[email protected]>
---
drivers/phy/amlogic/phy-meson8b-usb2.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c
index c1ed2e5c80d8..d5edd31686bb 100644
--- a/drivers/phy/amlogic/phy-meson8b-usb2.c
+++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
@@ -226,6 +226,11 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
regmap_update_bits(priv->regmap, REG_DBG_UART,
REG_DBG_UART_SET_IDDQ,
REG_DBG_UART_SET_IDDQ);
+
+ /* power off the PHY by putting it into reset mode */
+ regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
+ REG_CTRL_POWER_ON_RESET);
+
return 0;
}

@@ -245,8 +250,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
0x5 << REG_CTRL_FSEL_SHIFT);
/* reset the PHY */
- regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
- REG_CTRL_POWER_ON_RESET);
udelay(RESET_COMPLETE_TIME);
regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET, 0);
udelay(RESET_COMPLETE_TIME);
--
2.31.1


2021-06-18 02:47:59

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [RFCv1 7/8] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode.

Hi Anand,

On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <[email protected]> wrote:
[...]
> @@ -245,8 +250,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
> regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
> 0x5 << REG_CTRL_FSEL_SHIFT);
> /* reset the PHY */
> - regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
> - REG_CTRL_POWER_ON_RESET);
The vendor driver uses the following sequence for the power on reset:
- set the power on reset bit
- wait 500us
- clear the power on reset bit
- wait 500us

With your change we now:
- wait 500us
- clear the power on reset bit
- wait 500us

I don't know if this is sufficient to bring the PHY into a well-defined state.
Maybe it works, maybe it doesn't reset at all in this case - I don't
know how to verify this though.


Best regards,
Martin

2021-06-21 07:15:58

by Anand Moon

[permalink] [raw]
Subject: Re: [RFCv1 7/8] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode.

Hi Martin,

On Fri, 18 Jun 2021 at 04:07, Martin Blumenstingl
<[email protected]> wrote:
>
> Hi Anand,
>
> On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <[email protected]> wrote:
> [...]
> > @@ -245,8 +250,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
> > regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
> > 0x5 << REG_CTRL_FSEL_SHIFT);
> > /* reset the PHY */
> > - regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
> > - REG_CTRL_POWER_ON_RESET);
> The vendor driver uses the following sequence for the power on reset:
> - set the power on reset bit
> - wait 500us
> - clear the power on reset bit
> - wait 500us
>
> With your change we now:
> - wait 500us
> - clear the power on reset bit
> - wait 500us
>
> I don't know if this is sufficient to bring the PHY into a well-defined state.
> Maybe it works, maybe it doesn't reset at all in this case - I don't
> know how to verify this though.
>
Initially, I tried to some bit mask code to resolve this but it failed,
So no harm in keeping the original changes.

There is another parameter REG_CTRL_PORT_RESET to be considered.
>
> Best regards,
> Martin

Thanks



-Anand

2021-06-22 20:03:09

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [RFCv1 7/8] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode.

Hi Anand,

On Mon, Jun 21, 2021 at 9:15 AM Anand Moon <[email protected]> wrote:
>
> Hi Martin,
>
> On Fri, 18 Jun 2021 at 04:07, Martin Blumenstingl
> <[email protected]> wrote:
> >
> > Hi Anand,
> >
> > On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <[email protected]> wrote:
> > [...]
> > > @@ -245,8 +250,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
> > > regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK,
> > > 0x5 << REG_CTRL_FSEL_SHIFT);
> > > /* reset the PHY */
> > > - regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
> > > - REG_CTRL_POWER_ON_RESET);
> > The vendor driver uses the following sequence for the power on reset:
> > - set the power on reset bit
> > - wait 500us
> > - clear the power on reset bit
> > - wait 500us
> >
> > With your change we now:
> > - wait 500us
> > - clear the power on reset bit
> > - wait 500us
> >
> > I don't know if this is sufficient to bring the PHY into a well-defined state.
> > Maybe it works, maybe it doesn't reset at all in this case - I don't
> > know how to verify this though.
> >
> Initially, I tried to some bit mask code to resolve this but it failed,
> So no harm in keeping the original changes.
yes, I feel more comfortable with that

> There is another parameter REG_CTRL_PORT_RESET to be considered.
none of the vendor kernels that I have sets or clears this bit explicitly
I agree that the name seems related, but due to lack of an example or
documentation how/when to use this bit I suggest we don't touch it for
now


Best rergards,
Martin