| Message ID | 20201113000508.14702-4-aouledameur@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: meson: fix shared reset control use | expand |
Hi Amjad, On Fri, Nov 13, 2020 at 1:07 AM Amjad Ouled-Ameur <aouledameur@baylibre.com> wrote: [...] > ret = clk_prepare_enable(priv->clk_usb); > if (ret) { > dev_err(&phy->dev, "Failed to enable USB DDR clock\n"); > + reset_control_rearm(priv->reset); this should come after reset_control_rearm so we're cleaning up in reverse order of initializing things (in this case it probably makes no difference since reset_control_rearm is not touching any registers, but I'd still have it in the correct order to not confuse future developers) > clk_disable_unprepare(priv->clk_usb_general); > return ret; > } > @@ -197,6 +199,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) > regmap_read(priv->regmap, REG_ADP_BC, ®); > if (reg & REG_ADP_BC_ACA_PIN_FLOAT) { > dev_warn(&phy->dev, "USB ID detect failed!\n"); > + reset_control_rearm(priv->reset); same here, reset_control_rearm should be after clk_disable_unprepare > clk_disable_unprepare(priv->clk_usb); > clk_disable_unprepare(priv->clk_usb_general); > return -EINVAL; > @@ -216,6 +219,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy) > REG_DBG_UART_SET_IDDQ, > REG_DBG_UART_SET_IDDQ); > > + reset_control_rearm(priv->reset); same here, reset_control_rearm should be after clk_disable_unprepare > clk_disable_unprepare(priv->clk_usb); > clk_disable_unprepare(priv->clk_usb_general); Best regards, Martin
Hi Martin, Thank you for the review ! On 14/11/2020 20:11, Martin Blumenstingl wrote: > Hi Amjad, > > On Fri, Nov 13, 2020 at 1:07 AM Amjad Ouled-Ameur > <aouledameur@baylibre.com> wrote: > [...] >> ret = clk_prepare_enable(priv->clk_usb); >> if (ret) { >> dev_err(&phy->dev, "Failed to enable USB DDR clock\n"); >> + reset_control_rearm(priv->reset); > this should come after reset_control_rearm so we're cleaning up in > reverse order of initializing things > (in this case it probably makes no difference since > reset_control_rearm is not touching any registers, but I'd still have > it in the correct order to not confuse future developers) Agreed, it works in this current order since the two lines do not interfere with each other, but it is cleaner to do it in the reverse order of initialization. Will fix it in next change. >> clk_disable_unprepare(priv->clk_usb_general); >> return ret; >> } >> @@ -197,6 +199,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) >> regmap_read(priv->regmap, REG_ADP_BC, ®); >> if (reg & REG_ADP_BC_ACA_PIN_FLOAT) { >> dev_warn(&phy->dev, "USB ID detect failed!\n"); >> + reset_control_rearm(priv->reset); > same here, reset_control_rearm should be after clk_disable_unprepare Ditto, will fix it in next change. > >> clk_disable_unprepare(priv->clk_usb); >> clk_disable_unprepare(priv->clk_usb_general); >> return -EINVAL; >> @@ -216,6 +219,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy) >> REG_DBG_UART_SET_IDDQ, >> REG_DBG_UART_SET_IDDQ); >> >> + reset_control_rearm(priv->reset); > same here, reset_control_rearm should be after clk_disable_unprepare Ditto, will fix it in next change. > >> clk_disable_unprepare(priv->clk_usb); >> clk_disable_unprepare(priv->clk_usb_general); > > Best regards, > Martin > Sincerely, Amjad
diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c index 03c061dd5f0d..2a6c53f52423 100644 --- a/drivers/phy/amlogic/phy-meson8b-usb2.c +++ b/drivers/phy/amlogic/phy-meson8b-usb2.c @@ -154,12 +154,14 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) ret = clk_prepare_enable(priv->clk_usb_general); if (ret) { dev_err(&phy->dev, "Failed to enable USB general clock\n"); + reset_control_rearm(priv->reset); return ret; } ret = clk_prepare_enable(priv->clk_usb); if (ret) { dev_err(&phy->dev, "Failed to enable USB DDR clock\n"); + reset_control_rearm(priv->reset); clk_disable_unprepare(priv->clk_usb_general); return ret; } @@ -197,6 +199,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) regmap_read(priv->regmap, REG_ADP_BC, ®); if (reg & REG_ADP_BC_ACA_PIN_FLOAT) { dev_warn(&phy->dev, "USB ID detect failed!\n"); + reset_control_rearm(priv->reset); clk_disable_unprepare(priv->clk_usb); clk_disable_unprepare(priv->clk_usb_general); return -EINVAL; @@ -216,6 +219,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy) REG_DBG_UART_SET_IDDQ, REG_DBG_UART_SET_IDDQ); + reset_control_rearm(priv->reset); clk_disable_unprepare(priv->clk_usb); clk_disable_unprepare(priv->clk_usb_general);
Use reset_control_rearm() call if an error occurs in case phy_meson8b_usb2_power_on() fails after reset() has been called, or in case phy_meson8b_usb2_power_off() is called i.e the resource is no longer used and the reset line may be triggered again by other devices. reset_control_rearm() keeps use of triggered_count sane in the reset framework, use of reset_control_reset() on shared reset line should be balanced with reset_control_rearm(). Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com> Reported-by: Jerome Brunet <jbrunet@baylibre.com> --- Important: Please DO NOT merge before this patch [0] is merged, it adds reset_control_rearm() call to the reset framework API. drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++++ 1 file changed, 4 insertions(+)