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