diff mbox series

[RFCv1,2/8] phy: amlogic: meson8b-usb2: Use phy init callback function

Message ID 20210617194154.2397-3-linux.amoon@gmail.com
State RFC
Headers show
Series Meson-8b and Meson-gxbb USB phy code re-structure | expand

Commit Message

Anand Moon June 17, 2021, 7:41 p.m. UTC
Reorder the code for bulk clk_enable into .init callback function.

Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Martin Blumenstingl June 18, 2021, 12:26 p.m. UTC | #1
Hi Anand,

On Thu, Jun 17, 2021 at 9:43 PM Anand Moon <linux.amoon@gmail.com> wrote:
>
> Reorder the code for bulk clk_enable into .init callback function.
Depending on whether changes are made to the dwc2 driver this is
either fine or might cause some issues.
My understanding is that drivers which are using a PHY should use the
following code-flow:
- phy_init
- phy_power_on
- phy_power_off
- phy_exit

dwc2's platform.c [0] however currently uses the following order:
- phy_init
- phy_power_on
- (remaining ones omitted to keep this example short)

So with this patch and the way dwc2 is currently implemented the
phy_meson8b_usb2_power_on implementation might not work anymore.
That is because the clocks are turned off and in this case all
registers will read 0.
You might be lucky that u-boot left the clocks enabled for your case -
but in general I would not rely on this.

In case of the phy-meson-gxl-usb2 PHY driver the ordering is different
in the "consumer" driver (dwc3-meson-g12a.c).
There the order is:
- phy_init
- phy_power_on
- (remaining ones omitted to keep this example short)

I don't know if the order in the dwc2 driver can be changed easily
(without breaking other platforms).
Until that dwc2 driver issue is resolved I suggest not applying this
patch (even though in general I like the approach).
The same thing also applies for patch #3 from this series (for
implementing phy_ops.exit)


Best regards,
Martin


[0] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc2/platform.c#L157
Anand Moon June 18, 2021, 1:17 p.m. UTC | #2
Hi Martin,

On Fri, 18 Jun 2021 at 17:56, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Anand,
>
> On Thu, Jun 17, 2021 at 9:43 PM Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Reorder the code for bulk clk_enable into .init callback function.
> Depending on whether changes are made to the dwc2 driver this is
> either fine or might cause some issues.
> My understanding is that drivers which are using a PHY should use the
> following code-flow:
> - phy_init
> - phy_power_on
> - phy_power_off
> - phy_exit
>
> dwc2's platform.c [0] however currently uses the following order:
> - phy_init
> - phy_power_on
> - (remaining ones omitted to keep this example short)
>
> So with this patch and the way dwc2 is currently implemented the
> phy_meson8b_usb2_power_on implementation might not work anymore.
> That is because the clocks are turned off and in this case all
> registers will read 0.
> You might be lucky that u-boot left the clocks enabled for your case -
> but in general I would not rely on this.
>
> In case of the phy-meson-gxl-usb2 PHY driver the ordering is different
> in the "consumer" driver (dwc3-meson-g12a.c).
> There the order is:
> - phy_init
> - phy_power_on
> - (remaining ones omitted to keep this example short)
>
> I don't know if the order in the dwc2 driver can be changed easily
> (without breaking other platforms).
> Until that dwc2 driver issue is resolved I suggest not applying this
> patch (even though in general I like the approach).
> The same thing also applies for patch #3 from this series (for
> implementing phy_ops.exit)
>
>
> Best regards,
> Martin
>
>
> [0] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc2/platform.c#L157

Ok got your point of view.
Thanks for your review comments.

Thanks
-Anand
diff mbox series

Patch

diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c
index 771b73f3b44e..d48171b0b32e 100644
--- a/drivers/phy/amlogic/phy-meson8b-usb2.c
+++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
@@ -142,10 +142,9 @@  static const struct regmap_config phy_meson8b_usb2_regmap_conf = {
 	.max_register = REG_TUNE,
 };
 
-static int phy_meson8b_usb2_power_on(struct phy *phy)
+static int phy_meson8b_usb2_init(struct phy *phy)
 {
 	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
-	u32 reg;
 	int ret;
 
 	if (!IS_ERR_OR_NULL(priv->reset)) {
@@ -162,6 +161,14 @@  static int phy_meson8b_usb2_power_on(struct phy *phy)
 		return ret;
 	}
 
+	return 0;
+}
+
+static int phy_meson8b_usb2_power_on(struct phy *phy)
+{
+	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+	u32 reg;
+
 	regmap_update_bits(priv->regmap, REG_CONFIG, REG_CONFIG_CLK_32k_ALTSEL,
 			   REG_CONFIG_CLK_32k_ALTSEL);
 
@@ -219,6 +226,7 @@  static int phy_meson8b_usb2_power_off(struct phy *phy)
 }
 
 static const struct phy_ops phy_meson8b_usb2_ops = {
+	.init           = phy_meson8b_usb2_init,
 	.power_on	= phy_meson8b_usb2_power_on,
 	.power_off	= phy_meson8b_usb2_power_off,
 	.owner		= THIS_MODULE,