| Message ID | 20220426143736.1.I15bd7c3c8c983d6a6cec3d2ee371d75fe72fcd41@changeid |
|---|---|
| State | Accepted |
| Commit | 3c2db629581fbde2140a5cc6ec4f5b1f2290bda6 |
| Delegated to: | Patrick Delaunay |
| Headers | show |
| Series | stm32mp: handle ck_usbo_48m clock provided by USBPHYC | expand |
Hi Patrick On 4/26/22 14:37, Patrick Delaunay wrote: > Add the counter of the PLL user n_pll_cons managed by the 2 functions > stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. > > This counter allow to remove the function stm32_usbphyc_is_init > and it is a preliminary step for ck_usbo_48m introduction. > > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > --- > > drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ > 1 file changed, 48 insertions(+), 28 deletions(-) > > diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c > index 9c1dcfae52..16c8799eca 100644 > --- a/drivers/phy/phy-stm32-usbphyc.c > +++ b/drivers/phy/phy-stm32-usbphyc.c > @@ -65,6 +65,7 @@ struct stm32_usbphyc { > bool init; > bool powered; > } phys[MAX_PHYS]; > + int n_pll_cons; > }; > > static void stm32_usbphyc_get_pll_params(u32 clk_rate, > @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct stm32_usbphyc *usbphyc) > return 0; > } > > -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc) > -{ > - int i; > - > - for (i = 0; i < MAX_PHYS; i++) { > - if (usbphyc->phys[i].init) > - return true; > - } > - > - return false; > -} > - > static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) > { > int i; > @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) > return false; > } > > -static int stm32_usbphyc_phy_init(struct phy *phy) > +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc) > { > - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; > bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ? > true : false; > int ret; > > - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); > - /* Check if one phy port has already configured the pll */ > - if (pllen && stm32_usbphyc_is_init(usbphyc)) > - goto initialized; > + /* Check if one consumer has already configured the pll */ > + if (pllen && usbphyc->n_pll_cons) { > + usbphyc->n_pll_cons++; > + return 0; > + } > > if (usbphyc->vdda1v1) { > ret = regulator_set_enable(usbphyc->vdda1v1, true); > @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy) > if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN)) > return -EIO; > > -initialized: > - usbphyc_phy->init = true; > + usbphyc->n_pll_cons++; > > return 0; > } > > -static int stm32_usbphyc_phy_exit(struct phy *phy) > +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc) > { > - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; > int ret; > > - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); > - usbphyc_phy->init = false; > + usbphyc->n_pll_cons--; > > - /* Check if other phy port requires pllen */ > - if (stm32_usbphyc_is_init(usbphyc)) > + /* Check if other consumer requires pllen */ > + if (usbphyc->n_pll_cons) > return 0; > > clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN); > @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy) > return 0; > } > > +static int stm32_usbphyc_phy_init(struct phy *phy) > +{ > + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; > + int ret; > + > + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); > + if (usbphyc_phy->init) > + return 0; > + > + ret = stm32_usbphyc_pll_enable(usbphyc); > + if (ret) > + return log_ret(ret); > + > + usbphyc_phy->init = true; > + > + return 0; > +} > + > +static int stm32_usbphyc_phy_exit(struct phy *phy) > +{ > + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; > + int ret; > + > + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); > + if (!usbphyc_phy->init) > + return 0; > + > + ret = stm32_usbphyc_pll_disable(usbphyc); > + > + usbphyc_phy->init = false; > + > + return log_ret(ret); > +} > + > static int stm32_usbphyc_phy_power_on(struct phy *phy) > { > struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> Thanks Patrice
On 4/26/22 8:37 AM, Patrick Delaunay wrote: > Add the counter of the PLL user n_pll_cons managed by the 2 functions > stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. > > This counter allow to remove the function stm32_usbphyc_is_init > and it is a preliminary step for ck_usbo_48m introduction. Is it necessary to disable this clock before booting to Linux? If it isn't, then perhaps it is simpler to just not disable the clock. --Sean > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > --- > > drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ > 1 file changed, 48 insertions(+), 28 deletions(-) > > diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c > index 9c1dcfae52..16c8799eca 100644 > --- a/drivers/phy/phy-stm32-usbphyc.c > +++ b/drivers/phy/phy-stm32-usbphyc.c > @@ -65,6 +65,7 @@ struct stm32_usbphyc { > bool init; > bool powered; > } phys[MAX_PHYS]; > + int n_pll_cons; > }; > > static void stm32_usbphyc_get_pll_params(u32 clk_rate, > @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct stm32_usbphyc *usbphyc) > return 0; > } > > -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc) > -{ > - int i; > - > - for (i = 0; i < MAX_PHYS; i++) { > - if (usbphyc->phys[i].init) > - return true; > - } > - > - return false; > -} > - > static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) > { > int i; > @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) > return false; > } > > -static int stm32_usbphyc_phy_init(struct phy *phy) > +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc) > { > - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; > bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ? > true : false; > int ret; > > - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); > - /* Check if one phy port has already configured the pll */ > - if (pllen && stm32_usbphyc_is_init(usbphyc)) > - goto initialized; > + /* Check if one consumer has already configured the pll */ > + if (pllen && usbphyc->n_pll_cons) { > + usbphyc->n_pll_cons++; > + return 0; > + } > > if (usbphyc->vdda1v1) { > ret = regulator_set_enable(usbphyc->vdda1v1, true); > @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy) > if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN)) > return -EIO; > > -initialized: > - usbphyc_phy->init = true; > + usbphyc->n_pll_cons++; > > return 0; > } > > -static int stm32_usbphyc_phy_exit(struct phy *phy) > +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc) > { > - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; > int ret; > > - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); > - usbphyc_phy->init = false; > + usbphyc->n_pll_cons--; > > - /* Check if other phy port requires pllen */ > - if (stm32_usbphyc_is_init(usbphyc)) > + /* Check if other consumer requires pllen */ > + if (usbphyc->n_pll_cons) > return 0; > > clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN); > @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy) > return 0; > } > > +static int stm32_usbphyc_phy_init(struct phy *phy) > +{ > + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; > + int ret; > + > + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); > + if (usbphyc_phy->init) > + return 0; > + > + ret = stm32_usbphyc_pll_enable(usbphyc); > + if (ret) > + return log_ret(ret); > + > + usbphyc_phy->init = true; > + > + return 0; > +} > + > +static int stm32_usbphyc_phy_exit(struct phy *phy) > +{ > + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; > + int ret; > + > + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); > + if (!usbphyc_phy->init) > + return 0; > + > + ret = stm32_usbphyc_pll_disable(usbphyc); > + > + usbphyc_phy->init = false; > + > + return log_ret(ret); > +} > + > static int stm32_usbphyc_phy_power_on(struct phy *phy) > { > struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >
Hi Sean, On 5/8/22 20:21, Sean Anderson wrote: > On 4/26/22 8:37 AM, Patrick Delaunay wrote: >> Add the counter of the PLL user n_pll_cons managed by the 2 functions >> stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. >> >> This counter allow to remove the function stm32_usbphyc_is_init >> and it is a preliminary step for ck_usbo_48m introduction. > > Is it necessary to disable this clock before booting to Linux? If it > isn't, > then perhaps it is simpler to just not disable the clock. > > --Sean No, it is not necessary, we only need to enable the clock for the first user. I copy the clock behavior from kernel, but I agree that can be simpler. Amelie any notice about this point ? Do you prefer that I kept the behavior - same as kernel driver - or I simplify the U-Boot driver ? Patrick > >> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> >> --- >> >> drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ >> 1 file changed, 48 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/phy/phy-stm32-usbphyc.c >> b/drivers/phy/phy-stm32-usbphyc.c >> index 9c1dcfae52..16c8799eca 100644 >> --- a/drivers/phy/phy-stm32-usbphyc.c >> +++ b/drivers/phy/phy-stm32-usbphyc.c >> @@ -65,6 +65,7 @@ struct stm32_usbphyc { >> bool init; >> bool powered; >> } phys[MAX_PHYS]; >> + int n_pll_cons; >> }; >> static void stm32_usbphyc_get_pll_params(u32 clk_rate, >> @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct >> stm32_usbphyc *usbphyc) >> return 0; >> } >> -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc) >> -{ >> - int i; >> - >> - for (i = 0; i < MAX_PHYS; i++) { >> - if (usbphyc->phys[i].init) >> - return true; >> - } >> - >> - return false; >> -} >> - >> static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) >> { >> int i; >> @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct >> stm32_usbphyc *usbphyc) >> return false; >> } >> -static int stm32_usbphyc_phy_init(struct phy *phy) >> +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc) >> { >> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >> bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ? >> true : false; >> int ret; >> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >> - /* Check if one phy port has already configured the pll */ >> - if (pllen && stm32_usbphyc_is_init(usbphyc)) >> - goto initialized; >> + /* Check if one consumer has already configured the pll */ >> + if (pllen && usbphyc->n_pll_cons) { >> + usbphyc->n_pll_cons++; >> + return 0; >> + } >> if (usbphyc->vdda1v1) { >> ret = regulator_set_enable(usbphyc->vdda1v1, true); >> @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy) >> if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN)) >> return -EIO; >> -initialized: >> - usbphyc_phy->init = true; >> + usbphyc->n_pll_cons++; >> return 0; >> } >> -static int stm32_usbphyc_phy_exit(struct phy *phy) >> +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc) >> { >> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >> int ret; >> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >> - usbphyc_phy->init = false; >> + usbphyc->n_pll_cons--; >> - /* Check if other phy port requires pllen */ >> - if (stm32_usbphyc_is_init(usbphyc)) >> + /* Check if other consumer requires pllen */ >> + if (usbphyc->n_pll_cons) >> return 0; >> clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN); >> @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy) >> return 0; >> } >> +static int stm32_usbphyc_phy_init(struct phy *phy) >> +{ >> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >> + int ret; >> + >> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >> + if (usbphyc_phy->init) >> + return 0; >> + >> + ret = stm32_usbphyc_pll_enable(usbphyc); >> + if (ret) >> + return log_ret(ret); >> + >> + usbphyc_phy->init = true; >> + >> + return 0; >> +} >> + >> +static int stm32_usbphyc_phy_exit(struct phy *phy) >> +{ >> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >> + int ret; >> + >> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >> + if (!usbphyc_phy->init) >> + return 0; >> + >> + ret = stm32_usbphyc_pll_disable(usbphyc); >> + >> + usbphyc_phy->init = false; >> + >> + return log_ret(ret); >> +} >> + >> static int stm32_usbphyc_phy_power_on(struct phy *phy) >> { >> struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> >
On 5/6/22 16:18, Patrice CHOTARD wrote: > Hi Patrick > > On 4/26/22 14:37, Patrick Delaunay wrote: >> Add the counter of the PLL user n_pll_cons managed by the 2 functions >> stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. >> >> This counter allow to remove the function stm32_usbphyc_is_init >> and it is a preliminary step for ck_usbo_48m introduction. >> >> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> >> --- >> >> drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ >> 1 file changed, 48 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c >> index 9c1dcfae52..16c8799eca 100644 >> --- a/drivers/phy/phy-stm32-usbphyc.c >> +++ b/drivers/phy/phy-stm32-usbphyc.c >> @@ -65,6 +65,7 @@ struct stm32_usbphyc { >> bool init; >> bool powered; >> } phys[MAX_PHYS]; >> + int n_pll_cons; >> }; >> >> static void stm32_usbphyc_get_pll_params(u32 clk_rate, >> @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct stm32_usbphyc *usbphyc) >> return 0; >> } >> >> -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc) >> -{ >> - int i; >> - >> - for (i = 0; i < MAX_PHYS; i++) { >> - if (usbphyc->phys[i].init) >> - return true; >> - } >> - >> - return false; >> -} >> - >> static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) >> { >> int i; >> @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) >> return false; >> } >> >> -static int stm32_usbphyc_phy_init(struct phy *phy) >> +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc) >> { >> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >> bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ? >> true : false; >> int ret; >> >> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >> - /* Check if one phy port has already configured the pll */ >> - if (pllen && stm32_usbphyc_is_init(usbphyc)) >> - goto initialized; >> + /* Check if one consumer has already configured the pll */ >> + if (pllen && usbphyc->n_pll_cons) { >> + usbphyc->n_pll_cons++; >> + return 0; >> + } >> >> if (usbphyc->vdda1v1) { >> ret = regulator_set_enable(usbphyc->vdda1v1, true); >> @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy) >> if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN)) >> return -EIO; >> >> -initialized: >> - usbphyc_phy->init = true; >> + usbphyc->n_pll_cons++; >> >> return 0; >> } >> >> -static int stm32_usbphyc_phy_exit(struct phy *phy) >> +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc) >> { >> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >> int ret; >> >> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >> - usbphyc_phy->init = false; >> + usbphyc->n_pll_cons--; >> >> - /* Check if other phy port requires pllen */ >> - if (stm32_usbphyc_is_init(usbphyc)) >> + /* Check if other consumer requires pllen */ >> + if (usbphyc->n_pll_cons) >> return 0; >> >> clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN); >> @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy) >> return 0; >> } >> >> +static int stm32_usbphyc_phy_init(struct phy *phy) >> +{ >> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >> + int ret; >> + >> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >> + if (usbphyc_phy->init) >> + return 0; >> + >> + ret = stm32_usbphyc_pll_enable(usbphyc); >> + if (ret) >> + return log_ret(ret); >> + >> + usbphyc_phy->init = true; >> + >> + return 0; >> +} >> + >> +static int stm32_usbphyc_phy_exit(struct phy *phy) >> +{ >> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >> + int ret; >> + >> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >> + if (!usbphyc_phy->init) >> + return 0; >> + >> + ret = stm32_usbphyc_pll_disable(usbphyc); >> + >> + usbphyc_phy->init = false; >> + >> + return log_ret(ret); >> +} >> + >> static int stm32_usbphyc_phy_power_on(struct phy *phy) >> { >> struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); > Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> > > Thanks > Patrice > _______________________________________________ > Uboot-stm32 mailing list > Uboot-stm32@st-md-mailman.stormreply.com > https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32 Applied to u-boot-stm32 Thanks Patrice
Hi Patrick, Hi Sean, On 5/9/22 16:37, Patrick DELAUNAY wrote: > Hi Sean, > > On 5/8/22 20:21, Sean Anderson wrote: >> On 4/26/22 8:37 AM, Patrick Delaunay wrote: >>> Add the counter of the PLL user n_pll_cons managed by the 2 functions >>> stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. >>> >>> This counter allow to remove the function stm32_usbphyc_is_init >>> and it is a preliminary step for ck_usbo_48m introduction. >> >> Is it necessary to disable this clock before booting to Linux? If it >> isn't, >> then perhaps it is simpler to just not disable the clock. >> >> --Sean > > > No, it is not necessary, we only need to enable the clock for the first > user. > > I copy the clock behavior from kernel, > > but I agree that can be simpler. > > > Amelie any notice about this point ? > > Do you prefer that I kept the behavior - same as kernel driver - or I > simplify the U-Boot driver ? In case the PLL has not been disabled before Kernel boot, usbphyc Kernel driver will wait for the PLL pwerdown. USB could also not being used in Kernel, so PLL would remain enabled, and would waste power. I am rather in favor of disabling the PLL. Regards, Amelie > > > Patrick > > >> >>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> >>> --- >>> >>> drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ >>> 1 file changed, 48 insertions(+), 28 deletions(-) >>> >>> diff --git a/drivers/phy/phy-stm32-usbphyc.c >>> b/drivers/phy/phy-stm32-usbphyc.c >>> index 9c1dcfae52..16c8799eca 100644 >>> --- a/drivers/phy/phy-stm32-usbphyc.c >>> +++ b/drivers/phy/phy-stm32-usbphyc.c >>> @@ -65,6 +65,7 @@ struct stm32_usbphyc { >>> bool init; >>> bool powered; >>> } phys[MAX_PHYS]; >>> + int n_pll_cons; >>> }; >>> static void stm32_usbphyc_get_pll_params(u32 clk_rate, >>> @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct >>> stm32_usbphyc *usbphyc) >>> return 0; >>> } >>> -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc) >>> -{ >>> - int i; >>> - >>> - for (i = 0; i < MAX_PHYS; i++) { >>> - if (usbphyc->phys[i].init) >>> - return true; >>> - } >>> - >>> - return false; >>> -} >>> - >>> static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) >>> { >>> int i; >>> @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct >>> stm32_usbphyc *usbphyc) >>> return false; >>> } >>> -static int stm32_usbphyc_phy_init(struct phy *phy) >>> +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc) >>> { >>> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>> bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ? >>> true : false; >>> int ret; >>> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>> - /* Check if one phy port has already configured the pll */ >>> - if (pllen && stm32_usbphyc_is_init(usbphyc)) >>> - goto initialized; >>> + /* Check if one consumer has already configured the pll */ >>> + if (pllen && usbphyc->n_pll_cons) { >>> + usbphyc->n_pll_cons++; >>> + return 0; >>> + } >>> if (usbphyc->vdda1v1) { >>> ret = regulator_set_enable(usbphyc->vdda1v1, true); >>> @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy) >>> if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN)) >>> return -EIO; >>> -initialized: >>> - usbphyc_phy->init = true; >>> + usbphyc->n_pll_cons++; >>> return 0; >>> } >>> -static int stm32_usbphyc_phy_exit(struct phy *phy) >>> +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc) >>> { >>> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>> int ret; >>> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>> - usbphyc_phy->init = false; >>> + usbphyc->n_pll_cons--; >>> - /* Check if other phy port requires pllen */ >>> - if (stm32_usbphyc_is_init(usbphyc)) >>> + /* Check if other consumer requires pllen */ >>> + if (usbphyc->n_pll_cons) >>> return 0; >>> clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN); >>> @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy) >>> return 0; >>> } >>> +static int stm32_usbphyc_phy_init(struct phy *phy) >>> +{ >>> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>> + int ret; >>> + >>> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>> + if (usbphyc_phy->init) >>> + return 0; >>> + >>> + ret = stm32_usbphyc_pll_enable(usbphyc); >>> + if (ret) >>> + return log_ret(ret); >>> + >>> + usbphyc_phy->init = true; >>> + >>> + return 0; >>> +} >>> + >>> +static int stm32_usbphyc_phy_exit(struct phy *phy) >>> +{ >>> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>> + int ret; >>> + >>> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>> + if (!usbphyc_phy->init) >>> + return 0; >>> + >>> + ret = stm32_usbphyc_pll_disable(usbphyc); >>> + >>> + usbphyc_phy->init = false; >>> + >>> + return log_ret(ret); >>> +} >>> + >>> static int stm32_usbphyc_phy_power_on(struct phy *phy) >>> { >>> struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> >>
On 5/10/22 09:45, Patrice CHOTARD wrote: > > > On 5/6/22 16:18, Patrice CHOTARD wrote: >> Hi Patrick >> >> On 4/26/22 14:37, Patrick Delaunay wrote: >>> Add the counter of the PLL user n_pll_cons managed by the 2 functions >>> stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. >>> >>> This counter allow to remove the function stm32_usbphyc_is_init >>> and it is a preliminary step for ck_usbo_48m introduction. >>> >>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> >>> --- >>> >>> drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ >>> 1 file changed, 48 insertions(+), 28 deletions(-) >>> >>> diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c >>> index 9c1dcfae52..16c8799eca 100644 >>> --- a/drivers/phy/phy-stm32-usbphyc.c >>> +++ b/drivers/phy/phy-stm32-usbphyc.c >>> @@ -65,6 +65,7 @@ struct stm32_usbphyc { >>> bool init; >>> bool powered; >>> } phys[MAX_PHYS]; >>> + int n_pll_cons; >>> }; >>> >>> static void stm32_usbphyc_get_pll_params(u32 clk_rate, >>> @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct stm32_usbphyc *usbphyc) >>> return 0; >>> } >>> >>> -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc) >>> -{ >>> - int i; >>> - >>> - for (i = 0; i < MAX_PHYS; i++) { >>> - if (usbphyc->phys[i].init) >>> - return true; >>> - } >>> - >>> - return false; >>> -} >>> - >>> static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) >>> { >>> int i; >>> @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) >>> return false; >>> } >>> >>> -static int stm32_usbphyc_phy_init(struct phy *phy) >>> +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc) >>> { >>> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>> bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ? >>> true : false; >>> int ret; >>> >>> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>> - /* Check if one phy port has already configured the pll */ >>> - if (pllen && stm32_usbphyc_is_init(usbphyc)) >>> - goto initialized; >>> + /* Check if one consumer has already configured the pll */ >>> + if (pllen && usbphyc->n_pll_cons) { >>> + usbphyc->n_pll_cons++; >>> + return 0; >>> + } >>> >>> if (usbphyc->vdda1v1) { >>> ret = regulator_set_enable(usbphyc->vdda1v1, true); >>> @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy) >>> if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN)) >>> return -EIO; >>> >>> -initialized: >>> - usbphyc_phy->init = true; >>> + usbphyc->n_pll_cons++; >>> >>> return 0; >>> } >>> >>> -static int stm32_usbphyc_phy_exit(struct phy *phy) >>> +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc) >>> { >>> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>> int ret; >>> >>> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>> - usbphyc_phy->init = false; >>> + usbphyc->n_pll_cons--; >>> >>> - /* Check if other phy port requires pllen */ >>> - if (stm32_usbphyc_is_init(usbphyc)) >>> + /* Check if other consumer requires pllen */ >>> + if (usbphyc->n_pll_cons) >>> return 0; >>> >>> clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN); >>> @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy) >>> return 0; >>> } >>> >>> +static int stm32_usbphyc_phy_init(struct phy *phy) >>> +{ >>> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>> + int ret; >>> + >>> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>> + if (usbphyc_phy->init) >>> + return 0; >>> + >>> + ret = stm32_usbphyc_pll_enable(usbphyc); >>> + if (ret) >>> + return log_ret(ret); >>> + >>> + usbphyc_phy->init = true; >>> + >>> + return 0; >>> +} >>> + >>> +static int stm32_usbphyc_phy_exit(struct phy *phy) >>> +{ >>> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>> + int ret; >>> + >>> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>> + if (!usbphyc_phy->init) >>> + return 0; >>> + >>> + ret = stm32_usbphyc_pll_disable(usbphyc); >>> + >>> + usbphyc_phy->init = false; >>> + >>> + return log_ret(ret); >>> +} >>> + >>> static int stm32_usbphyc_phy_power_on(struct phy *phy) >>> { >>> struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >> Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> >> >> Thanks >> Patrice After discussion with Patrick, the whole series will not be merged in stm32 git custodian master branch Patrice >> _______________________________________________ >> Uboot-stm32 mailing list >> Uboot-stm32@st-md-mailman.stormreply.com >> https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32 > Applied to u-boot-stm32 > > Thanks > Patrice > _______________________________________________ > Uboot-stm32 mailing list > Uboot-stm32@st-md-mailman.stormreply.com > https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32
On 5/10/22 5:51 AM, Amelie Delaunay wrote: > Hi Patrick, > Hi Sean, > > On 5/9/22 16:37, Patrick DELAUNAY wrote: >> Hi Sean, >> >> On 5/8/22 20:21, Sean Anderson wrote: >>> On 4/26/22 8:37 AM, Patrick Delaunay wrote: >>>> Add the counter of the PLL user n_pll_cons managed by the 2 functions >>>> stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. >>>> >>>> This counter allow to remove the function stm32_usbphyc_is_init >>>> and it is a preliminary step for ck_usbo_48m introduction. >>> >>> Is it necessary to disable this clock before booting to Linux? If it isn't, >>> then perhaps it is simpler to just not disable the clock. >>> >>> --Sean >> >> >> No, it is not necessary, we only need to enable the clock for the first user. >> >> I copy the clock behavior from kernel, >> >> but I agree that can be simpler. >> >> >> Amelie any notice about this point ? >> >> Do you prefer that I kept the behavior - same as kernel driver - or I simplify the U-Boot driver ? > > In case the PLL has not been disabled before Kernel boot, usbphyc Kernel driver will wait for the PLL pwerdown. > USB could also not being used in Kernel, so PLL would remain enabled, and would waste power. > I am rather in favor of disabling the PLL. It should be disabled if clk_ignore_unused is not in the kernel parameters, as long as Linux is also aware of the clock. Generally, I would like to avoid refcounting if possible. Many U-Boot drivers do not disable their clocks (because they don't do any cleanup), so you can end up with the clock staying on anyway. --Sean > Regards, > Amelie > >> >> >> Patrick >> >> >>> >>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> >>>> --- >>>> >>>> drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ >>>> 1 file changed, 48 insertions(+), 28 deletions(-) >>>> >>>> diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c >>>> index 9c1dcfae52..16c8799eca 100644 >>>> --- a/drivers/phy/phy-stm32-usbphyc.c >>>> +++ b/drivers/phy/phy-stm32-usbphyc.c >>>> @@ -65,6 +65,7 @@ struct stm32_usbphyc { >>>> bool init; >>>> bool powered; >>>> } phys[MAX_PHYS]; >>>> + int n_pll_cons; >>>> }; >>>> static void stm32_usbphyc_get_pll_params(u32 clk_rate, >>>> @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct stm32_usbphyc *usbphyc) >>>> return 0; >>>> } >>>> -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc) >>>> -{ >>>> - int i; >>>> - >>>> - for (i = 0; i < MAX_PHYS; i++) { >>>> - if (usbphyc->phys[i].init) >>>> - return true; >>>> - } >>>> - >>>> - return false; >>>> -} >>>> - >>>> static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) >>>> { >>>> int i; >>>> @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) >>>> return false; >>>> } >>>> -static int stm32_usbphyc_phy_init(struct phy *phy) >>>> +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc) >>>> { >>>> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>>> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>>> bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ? >>>> true : false; >>>> int ret; >>>> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>>> - /* Check if one phy port has already configured the pll */ >>>> - if (pllen && stm32_usbphyc_is_init(usbphyc)) >>>> - goto initialized; >>>> + /* Check if one consumer has already configured the pll */ >>>> + if (pllen && usbphyc->n_pll_cons) { >>>> + usbphyc->n_pll_cons++; >>>> + return 0; >>>> + } >>>> if (usbphyc->vdda1v1) { >>>> ret = regulator_set_enable(usbphyc->vdda1v1, true); >>>> @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy) >>>> if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN)) >>>> return -EIO; >>>> -initialized: >>>> - usbphyc_phy->init = true; >>>> + usbphyc->n_pll_cons++; >>>> return 0; >>>> } >>>> -static int stm32_usbphyc_phy_exit(struct phy *phy) >>>> +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc) >>>> { >>>> - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>>> - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>>> int ret; >>>> - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>>> - usbphyc_phy->init = false; >>>> + usbphyc->n_pll_cons--; >>>> - /* Check if other phy port requires pllen */ >>>> - if (stm32_usbphyc_is_init(usbphyc)) >>>> + /* Check if other consumer requires pllen */ >>>> + if (usbphyc->n_pll_cons) >>>> return 0; >>>> clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN); >>>> @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy) >>>> return 0; >>>> } >>>> +static int stm32_usbphyc_phy_init(struct phy *phy) >>>> +{ >>>> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>>> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>>> + int ret; >>>> + >>>> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>>> + if (usbphyc_phy->init) >>>> + return 0; >>>> + >>>> + ret = stm32_usbphyc_pll_enable(usbphyc); >>>> + if (ret) >>>> + return log_ret(ret); >>>> + >>>> + usbphyc_phy->init = true; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int stm32_usbphyc_phy_exit(struct phy *phy) >>>> +{ >>>> + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>>> + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; >>>> + int ret; >>>> + >>>> + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); >>>> + if (!usbphyc_phy->init) >>>> + return 0; >>>> + >>>> + ret = stm32_usbphyc_pll_disable(usbphyc); >>>> + >>>> + usbphyc_phy->init = false; >>>> + >>>> + return log_ret(ret); >>>> +} >>>> + >>>> static int stm32_usbphyc_phy_power_on(struct phy *phy) >>>> { >>>> struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); >>>> >>>
Hi Sean, On 5/11/22 18:48, Sean Anderson wrote: > On 5/10/22 5:51 AM, Amelie Delaunay wrote: >> Hi Patrick, >> Hi Sean, >> >> On 5/9/22 16:37, Patrick DELAUNAY wrote: >>> Hi Sean, >>> >>> On 5/8/22 20:21, Sean Anderson wrote: >>>> On 4/26/22 8:37 AM, Patrick Delaunay wrote: >>>>> Add the counter of the PLL user n_pll_cons managed by the 2 functions >>>>> stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. >>>>> >>>>> This counter allow to remove the function stm32_usbphyc_is_init >>>>> and it is a preliminary step for ck_usbo_48m introduction. >>>> >>>> Is it necessary to disable this clock before booting to Linux? If >>>> it isn't, >>>> then perhaps it is simpler to just not disable the clock. >>>> >>>> --Sean >>> >>> >>> No, it is not necessary, we only need to enable the clock for the >>> first user. >>> >>> I copy the clock behavior from kernel, >>> >>> but I agree that can be simpler. >>> >>> >>> Amelie any notice about this point ? >>> >>> Do you prefer that I kept the behavior - same as kernel driver - or >>> I simplify the U-Boot driver ? >> >> In case the PLL has not been disabled before Kernel boot, usbphyc >> Kernel driver will wait for the PLL pwerdown. >> USB could also not being used in Kernel, so PLL would remain enabled, >> and would waste power. >> I am rather in favor of disabling the PLL. > > It should be disabled if clk_ignore_unused is not in the kernel > parameters, > as long as Linux is also aware of the clock. > > Generally, I would like to avoid refcounting if possible. Many U-Boot > drivers do not disable their clocks (because they don't do any cleanup), > so you can end up with the clock staying on anyway. > > --Sean > >> Regards, >> Amelie >> >>> >>> >>> Patrick >>> >>> >>>> In general, I'm also in favor of not disabling the clock in U-Boot. But this PLL with vdda1v1 and vdda1v8 dependency is requested for - USBPHYC himself or - source clock of RCC, And we have have see many issue for this PLL initialization sequence / dependencies. So for clock support, I only reused the existing/working function called by the PHY ops init & exit = stm32_usbphyc_phy_init & stm32_usbphyc_phy_exit => the PLL and the associated VDD are already deactivated on USBPHYC exit. And to be coherent I for the same for the clock to avoid conflict between these 2 users (USBPHYC or RCC) as it is done also in Linux kernel. Avoid to deactivate PLL on clock disable is a major objection or just a advice? Regards Patrick .
On 5/17/22 3:42 AM, Patrick DELAUNAY wrote: > Hi Sean, > > On 5/11/22 18:48, Sean Anderson wrote: >> On 5/10/22 5:51 AM, Amelie Delaunay wrote: >>> Hi Patrick, >>> Hi Sean, >>> >>> On 5/9/22 16:37, Patrick DELAUNAY wrote: >>>> Hi Sean, >>>> >>>> On 5/8/22 20:21, Sean Anderson wrote: >>>>> On 4/26/22 8:37 AM, Patrick Delaunay wrote: >>>>>> Add the counter of the PLL user n_pll_cons managed by the 2 functions >>>>>> stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. >>>>>> >>>>>> This counter allow to remove the function stm32_usbphyc_is_init >>>>>> and it is a preliminary step for ck_usbo_48m introduction. >>>>> >>>>> Is it necessary to disable this clock before booting to Linux? If it isn't, >>>>> then perhaps it is simpler to just not disable the clock. >>>>> >>>>> --Sean >>>> >>>> >>>> No, it is not necessary, we only need to enable the clock for the first user. >>>> >>>> I copy the clock behavior from kernel, >>>> >>>> but I agree that can be simpler. >>>> >>>> >>>> Amelie any notice about this point ? >>>> >>>> Do you prefer that I kept the behavior - same as kernel driver - or I simplify the U-Boot driver ? >>> >>> In case the PLL has not been disabled before Kernel boot, usbphyc Kernel driver will wait for the PLL pwerdown. >>> USB could also not being used in Kernel, so PLL would remain enabled, and would waste power. >>> I am rather in favor of disabling the PLL. >> >> It should be disabled if clk_ignore_unused is not in the kernel parameters, >> as long as Linux is also aware of the clock. >> >> Generally, I would like to avoid refcounting if possible. Many U-Boot >> drivers do not disable their clocks (because they don't do any cleanup), >> so you can end up with the clock staying on anyway. >> >> --Sean >> >>> Regards, >>> Amelie >>> >>>> >>>> >>>> Patrick >>>> >>>> >>>>> > > In general, I'm also in favor of not disabling the clock in U-Boot. > > But this PLL with vdda1v1 and vdda1v8 dependency is requested for > > - USBPHYC himself or > > - source clock of RCC, > > > And we have have see many issue for this PLL initialization sequence / dependencies. > > > So for clock support, I only reused the existing/working function called by the PHY ops init & exit = > > stm32_usbphyc_phy_init & stm32_usbphyc_phy_exit > > => the PLL and the associated VDD are already deactivated on USBPHYC exit. > > > And to be coherent I for the same for the clock to avoid conflict between these 2 users > > (USBPHYC or RCC) as it is done also in Linux kernel. > > > Avoid to deactivate PLL on clock disable is a major objection or just a advice? Just advice. If all of the clock's users call disable, then it is fine. --Sean
Hi, On 4/26/22 14:37, Patrick Delaunay wrote: > Add the counter of the PLL user n_pll_cons managed by the 2 functions > stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. > > This counter allow to remove the function stm32_usbphyc_is_init > and it is a preliminary step for ck_usbo_48m introduction. > > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > --- > > drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ > 1 file changed, 48 insertions(+), 28 deletions(-) > Applied to u-boot-stm/master, thanks! Regards Patrick
diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c index 9c1dcfae52..16c8799eca 100644 --- a/drivers/phy/phy-stm32-usbphyc.c +++ b/drivers/phy/phy-stm32-usbphyc.c @@ -65,6 +65,7 @@ struct stm32_usbphyc { bool init; bool powered; } phys[MAX_PHYS]; + int n_pll_cons; }; static void stm32_usbphyc_get_pll_params(u32 clk_rate, @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct stm32_usbphyc *usbphyc) return 0; } -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc) -{ - int i; - - for (i = 0; i < MAX_PHYS; i++) { - if (usbphyc->phys[i].init) - return true; - } - - return false; -} - static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) { int i; @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc) return false; } -static int stm32_usbphyc_phy_init(struct phy *phy) +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc) { - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ? true : false; int ret; - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); - /* Check if one phy port has already configured the pll */ - if (pllen && stm32_usbphyc_is_init(usbphyc)) - goto initialized; + /* Check if one consumer has already configured the pll */ + if (pllen && usbphyc->n_pll_cons) { + usbphyc->n_pll_cons++; + return 0; + } if (usbphyc->vdda1v1) { ret = regulator_set_enable(usbphyc->vdda1v1, true); @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy) if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN)) return -EIO; -initialized: - usbphyc_phy->init = true; + usbphyc->n_pll_cons++; return 0; } -static int stm32_usbphyc_phy_exit(struct phy *phy) +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc) { - struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); - struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; int ret; - dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); - usbphyc_phy->init = false; + usbphyc->n_pll_cons--; - /* Check if other phy port requires pllen */ - if (stm32_usbphyc_is_init(usbphyc)) + /* Check if other consumer requires pllen */ + if (usbphyc->n_pll_cons) return 0; clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN); @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy) return 0; } +static int stm32_usbphyc_phy_init(struct phy *phy) +{ + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; + int ret; + + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); + if (usbphyc_phy->init) + return 0; + + ret = stm32_usbphyc_pll_enable(usbphyc); + if (ret) + return log_ret(ret); + + usbphyc_phy->init = true; + + return 0; +} + +static int stm32_usbphyc_phy_exit(struct phy *phy) +{ + struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev); + struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id; + int ret; + + dev_dbg(phy->dev, "phy ID = %lu\n", phy->id); + if (!usbphyc_phy->init) + return 0; + + ret = stm32_usbphyc_pll_disable(usbphyc); + + usbphyc_phy->init = false; + + return log_ret(ret); +} + static int stm32_usbphyc_phy_power_on(struct phy *phy) { struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev);
Add the counter of the PLL user n_pll_cons managed by the 2 functions stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable. This counter allow to remove the function stm32_usbphyc_is_init and it is a preliminary step for ck_usbo_48m introduction. Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> --- drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 28 deletions(-)