[PATCH 0/3] stm32mp: handle ck_usbo_48m clock provided by USBPHYC

[PATCH 0/3] stm32mp: handle ck_usbo_48m clock provided by USBPHYC

[Patrick Delaunay]

Update the RCC stm32mp1 clock driver to handle the USB_PHY_48
clock provided by USBPHYC and named "ck_usbo_48m".

With this series, the clock dependencies for USB PHYC, USBOTG (including
USB detection) and USBHOST are correctly managed.

See Linux kernel commit "ARM: dts: stm32: use usbphyc ck_usbo_48m as USBH
OHCI clock on stm32mp151" and "phy: stm32: register usbphyc as clock
provider of ck_usbo_48m clock"

Patrick


Patrick Delaunay (3):
  phy: stm32-usbphyc: add counter of PLL consumer
  phy: stm32-usbphyc: usbphyc is a clock provider of ck_usbo_48m clock
  clk: stm32mp: handle ck_usbo_48m clock provided by USBPHYC

 drivers/clk/clk_stm32mp1.c      |  35 ++++----
 drivers/phy/phy-stm32-usbphyc.c | 155 ++++++++++++++++++++++++++------
 2 files changed, 147 insertions(+), 43 deletions(-)

-- 
2.25.1

[PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Patrick Delaunay]

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);
-- 
2.25.1

[PATCH 2/3] phy: stm32-usbphyc: usbphyc is a clock provider of ck_usbo_48m clock

[Patrick Delaunay]

ck_usbo_48m is generated by usbphyc PLL and used by OTG controller
for Full-Speed use cases with dedicated Full-Speed transceiver.

ck_usbo_48m is available as soon as the PLL is enabled.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 drivers/phy/phy-stm32-usbphyc.c | 79 +++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c
index 16c8799eca..e0b8fcb8f2 100644
--- a/drivers/phy/phy-stm32-usbphyc.c
+++ b/drivers/phy/phy-stm32-usbphyc.c
@@ -7,6 +7,7 @@
 
 #include <common.h>
 #include <clk.h>
+#include <clk-uclass.h>
 #include <div64.h>
 #include <dm.h>
 #include <fdtdec.h>
@@ -17,6 +18,7 @@
 #include <usb.h>
 #include <asm/io.h>
 #include <dm/device_compat.h>
+#include <dm/lists.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <power/regulator.h>
@@ -49,6 +51,9 @@
 #define PLL_INFF_MIN_RATE	19200000 /* in Hz */
 #define PLL_INFF_MAX_RATE	38400000 /* in Hz */
 
+/* USBPHYC_CLK48 */
+#define USBPHYC_CLK48_FREQ	48000000 /* in Hz */
+
 struct pll_params {
 	u8 ndiv;
 	u16 frac;
@@ -355,6 +360,16 @@ static const struct phy_ops stm32_usbphyc_phy_ops = {
 	.of_xlate = stm32_usbphyc_of_xlate,
 };
 
+static int stm32_usbphyc_bind(struct udevice *dev)
+{
+	int ret;
+
+	ret = device_bind_driver_to_node(dev, "stm32-usbphyc-clk", "ck_usbo_48m",
+					 dev_ofnode(dev), NULL);
+
+	return log_ret(ret);
+}
+
 static int stm32_usbphyc_probe(struct udevice *dev)
 {
 	struct stm32_usbphyc *usbphyc = dev_get_priv(dev);
@@ -444,6 +459,70 @@ U_BOOT_DRIVER(stm32_usb_phyc) = {
 	.id = UCLASS_PHY,
 	.of_match = stm32_usbphyc_of_match,
 	.ops = &stm32_usbphyc_phy_ops,
+	.bind = stm32_usbphyc_bind,
 	.probe = stm32_usbphyc_probe,
 	.priv_auto	= sizeof(struct stm32_usbphyc),
 };
+
+struct stm32_usbphyc_clk {
+	bool enable;
+};
+
+static ulong stm32_usbphyc_clk48_get_rate(struct clk *clk)
+{
+	return USBPHYC_CLK48_FREQ;
+}
+
+static int stm32_usbphyc_clk48_enable(struct clk *clk)
+{
+	struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
+	struct stm32_usbphyc *usbphyc;
+	int ret;
+
+	if (usbphyc_clk->enable)
+		return 0;
+
+	usbphyc = dev_get_priv(clk->dev->parent);
+
+	/* ck_usbo_48m is generated by usbphyc PLL */
+	ret = stm32_usbphyc_pll_enable(usbphyc);
+	if (ret)
+		return ret;
+
+	usbphyc_clk->enable = true;
+
+	return 0;
+}
+
+static int stm32_usbphyc_clk48_disable(struct clk *clk)
+{
+	struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
+	struct stm32_usbphyc *usbphyc;
+	int ret;
+
+	if (!usbphyc_clk->enable)
+		return 0;
+
+	usbphyc = dev_get_priv(clk->dev->parent);
+
+	ret = stm32_usbphyc_pll_disable(usbphyc);
+	if (ret)
+		return ret;
+
+	usbphyc_clk->enable = false;
+
+	return 0;
+}
+
+const struct clk_ops usbphyc_clk48_ops = {
+	.get_rate = stm32_usbphyc_clk48_get_rate,
+	.enable = stm32_usbphyc_clk48_enable,
+	.disable = stm32_usbphyc_clk48_disable,
+};
+
+U_BOOT_DRIVER(stm32_usb_phyc_clk) = {
+	.name = "stm32-usbphyc-clk",
+	.id = UCLASS_CLK,
+	.ops = &usbphyc_clk48_ops,
+	.priv_auto = sizeof(struct stm32_usbphyc_clk),
+};
-- 
2.25.1

[PATCH 3/3] clk: stm32mp: handle ck_usbo_48m clock provided by USBPHYC

[Patrick Delaunay]

Handle the input clock of RCC USB_PHY_48, provided by USBPHYC
and named "ck_usbo_48m".

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 drivers/clk/clk_stm32mp1.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/clk_stm32mp1.c b/drivers/clk/clk_stm32mp1.c
index 83ab6b728e..a02921c43a 100644
--- a/drivers/clk/clk_stm32mp1.c
+++ b/drivers/clk/clk_stm32mp1.c
@@ -962,6 +962,24 @@ static ulong stm32mp1_read_pll_freq(struct stm32mp1_clk_priv *priv,
 	return dfout;
 }
 
+static ulong stm32mp1_clk_get_by_name(const char *name)
+{
+	struct clk clk;
+	struct udevice *dev = NULL;
+	ulong clock = 0;
+
+	if (!uclass_get_device_by_name(UCLASS_CLK, name, &dev)) {
+		if (clk_request(dev, &clk)) {
+			log_err("%s request", name);
+		} else {
+			clk.id = 0;
+			clock = clk_get_rate(&clk);
+		}
+	}
+
+	return clock;
+}
+
 static ulong stm32mp1_clk_get(struct stm32mp1_clk_priv *priv, int p)
 {
 	u32 reg;
@@ -1127,24 +1145,11 @@ static ulong stm32mp1_clk_get(struct stm32mp1_clk_priv *priv, int p)
 		break;
 	/* other */
 	case _USB_PHY_48:
-		clock = 48000000;
+		clock = stm32mp1_clk_get_by_name("ck_usbo_48m");
 		break;
 	case _DSI_PHY:
-	{
-		struct clk clk;
-		struct udevice *dev = NULL;
-
-		if (!uclass_get_device_by_name(UCLASS_CLK, "ck_dsi_phy",
-					       &dev)) {
-			if (clk_request(dev, &clk)) {
-				log_err("ck_dsi_phy request");
-			} else {
-				clk.id = 0;
-				clock = clk_get_rate(&clk);
-			}
-		}
+		clock = stm32mp1_clk_get_by_name("ck_dsi_phy");
 		break;
-	}
 	default:
 		break;
 	}
-- 
2.25.1

Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Patrice CHOTARD]

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

Re: [PATCH 2/3] phy: stm32-usbphyc: usbphyc is a clock provider of ck_usbo_48m clock

[Patrice CHOTARD]

Hi Patrick

On 4/26/22 14:37, Patrick Delaunay wrote:
> ck_usbo_48m is generated by usbphyc PLL and used by OTG controller
> for Full-Speed use cases with dedicated Full-Speed transceiver.
> 
> ck_usbo_48m is available as soon as the PLL is enabled.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  drivers/phy/phy-stm32-usbphyc.c | 79 +++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c
> index 16c8799eca..e0b8fcb8f2 100644
> --- a/drivers/phy/phy-stm32-usbphyc.c
> +++ b/drivers/phy/phy-stm32-usbphyc.c
> @@ -7,6 +7,7 @@
>  
>  #include <common.h>
>  #include <clk.h>
> +#include <clk-uclass.h>
>  #include <div64.h>
>  #include <dm.h>
>  #include <fdtdec.h>
> @@ -17,6 +18,7 @@
>  #include <usb.h>
>  #include <asm/io.h>
>  #include <dm/device_compat.h>
> +#include <dm/lists.h>
>  #include <linux/bitops.h>
>  #include <linux/delay.h>
>  #include <power/regulator.h>
> @@ -49,6 +51,9 @@
>  #define PLL_INFF_MIN_RATE	19200000 /* in Hz */
>  #define PLL_INFF_MAX_RATE	38400000 /* in Hz */
>  
> +/* USBPHYC_CLK48 */
> +#define USBPHYC_CLK48_FREQ	48000000 /* in Hz */
> +
>  struct pll_params {
>  	u8 ndiv;
>  	u16 frac;
> @@ -355,6 +360,16 @@ static const struct phy_ops stm32_usbphyc_phy_ops = {
>  	.of_xlate = stm32_usbphyc_of_xlate,
>  };
>  
> +static int stm32_usbphyc_bind(struct udevice *dev)
> +{
> +	int ret;
> +
> +	ret = device_bind_driver_to_node(dev, "stm32-usbphyc-clk", "ck_usbo_48m",
> +					 dev_ofnode(dev), NULL);
> +
> +	return log_ret(ret);
> +}
> +
>  static int stm32_usbphyc_probe(struct udevice *dev)
>  {
>  	struct stm32_usbphyc *usbphyc = dev_get_priv(dev);
> @@ -444,6 +459,70 @@ U_BOOT_DRIVER(stm32_usb_phyc) = {
>  	.id = UCLASS_PHY,
>  	.of_match = stm32_usbphyc_of_match,
>  	.ops = &stm32_usbphyc_phy_ops,
> +	.bind = stm32_usbphyc_bind,
>  	.probe = stm32_usbphyc_probe,
>  	.priv_auto	= sizeof(struct stm32_usbphyc),
>  };
> +
> +struct stm32_usbphyc_clk {
> +	bool enable;
> +};
> +
> +static ulong stm32_usbphyc_clk48_get_rate(struct clk *clk)
> +{
> +	return USBPHYC_CLK48_FREQ;
> +}
> +
> +static int stm32_usbphyc_clk48_enable(struct clk *clk)
> +{
> +	struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
> +	struct stm32_usbphyc *usbphyc;
> +	int ret;
> +
> +	if (usbphyc_clk->enable)
> +		return 0;
> +
> +	usbphyc = dev_get_priv(clk->dev->parent);
> +
> +	/* ck_usbo_48m is generated by usbphyc PLL */
> +	ret = stm32_usbphyc_pll_enable(usbphyc);
> +	if (ret)
> +		return ret;
> +
> +	usbphyc_clk->enable = true;
> +
> +	return 0;
> +}
> +
> +static int stm32_usbphyc_clk48_disable(struct clk *clk)
> +{
> +	struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
> +	struct stm32_usbphyc *usbphyc;
> +	int ret;
> +
> +	if (!usbphyc_clk->enable)
> +		return 0;
> +
> +	usbphyc = dev_get_priv(clk->dev->parent);
> +
> +	ret = stm32_usbphyc_pll_disable(usbphyc);
> +	if (ret)
> +		return ret;
> +
> +	usbphyc_clk->enable = false;
> +
> +	return 0;
> +}
> +
> +const struct clk_ops usbphyc_clk48_ops = {
> +	.get_rate = stm32_usbphyc_clk48_get_rate,
> +	.enable = stm32_usbphyc_clk48_enable,
> +	.disable = stm32_usbphyc_clk48_disable,
> +};
> +
> +U_BOOT_DRIVER(stm32_usb_phyc_clk) = {
> +	.name = "stm32-usbphyc-clk",
> +	.id = UCLASS_CLK,
> +	.ops = &usbphyc_clk48_ops,
> +	.priv_auto = sizeof(struct stm32_usbphyc_clk),
> +};


Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
PAtrice

Re: [PATCH 3/3] clk: stm32mp: handle ck_usbo_48m clock provided by USBPHYC

[Patrice CHOTARD]

Hi Patrick

On 4/26/22 14:37, Patrick Delaunay wrote:
> Handle the input clock of RCC USB_PHY_48, provided by USBPHYC
> and named "ck_usbo_48m".
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  drivers/clk/clk_stm32mp1.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/clk/clk_stm32mp1.c b/drivers/clk/clk_stm32mp1.c
> index 83ab6b728e..a02921c43a 100644
> --- a/drivers/clk/clk_stm32mp1.c
> +++ b/drivers/clk/clk_stm32mp1.c
> @@ -962,6 +962,24 @@ static ulong stm32mp1_read_pll_freq(struct stm32mp1_clk_priv *priv,
>  	return dfout;
>  }
>  
> +static ulong stm32mp1_clk_get_by_name(const char *name)
> +{
> +	struct clk clk;
> +	struct udevice *dev = NULL;
> +	ulong clock = 0;
> +
> +	if (!uclass_get_device_by_name(UCLASS_CLK, name, &dev)) {
> +		if (clk_request(dev, &clk)) {
> +			log_err("%s request", name);
> +		} else {
> +			clk.id = 0;
> +			clock = clk_get_rate(&clk);
> +		}
> +	}
> +
> +	return clock;
> +}
> +
>  static ulong stm32mp1_clk_get(struct stm32mp1_clk_priv *priv, int p)
>  {
>  	u32 reg;
> @@ -1127,24 +1145,11 @@ static ulong stm32mp1_clk_get(struct stm32mp1_clk_priv *priv, int p)
>  		break;
>  	/* other */
>  	case _USB_PHY_48:
> -		clock = 48000000;
> +		clock = stm32mp1_clk_get_by_name("ck_usbo_48m");
>  		break;
>  	case _DSI_PHY:
> -	{
> -		struct clk clk;
> -		struct udevice *dev = NULL;
> -
> -		if (!uclass_get_device_by_name(UCLASS_CLK, "ck_dsi_phy",
> -					       &dev)) {
> -			if (clk_request(dev, &clk)) {
> -				log_err("ck_dsi_phy request");
> -			} else {
> -				clk.id = 0;
> -				clock = clk_get_rate(&clk);
> -			}
> -		}
> +		clock = stm32mp1_clk_get_by_name("ck_dsi_phy");
>  		break;
> -	}
>  	default:
>  		break;
>  	}
Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Sean Anderson]

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);
> 

Re: [PATCH 2/3] phy: stm32-usbphyc: usbphyc is a clock provider of ck_usbo_48m clock

[Sean Anderson]

On 4/26/22 8:37 AM, Patrick Delaunay wrote:
> ck_usbo_48m is generated by usbphyc PLL and used by OTG controller
> for Full-Speed use cases with dedicated Full-Speed transceiver.
> 
> ck_usbo_48m is available as soon as the PLL is enabled.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>   drivers/phy/phy-stm32-usbphyc.c | 79 +++++++++++++++++++++++++++++++++
>   1 file changed, 79 insertions(+)
> 
> diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c
> index 16c8799eca..e0b8fcb8f2 100644
> --- a/drivers/phy/phy-stm32-usbphyc.c
> +++ b/drivers/phy/phy-stm32-usbphyc.c
> @@ -7,6 +7,7 @@
>   
>   #include <common.h>
>   #include <clk.h>
> +#include <clk-uclass.h>
>   #include <div64.h>
>   #include <dm.h>
>   #include <fdtdec.h>
> @@ -17,6 +18,7 @@
>   #include <usb.h>
>   #include <asm/io.h>
>   #include <dm/device_compat.h>
> +#include <dm/lists.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
>   #include <power/regulator.h>
> @@ -49,6 +51,9 @@
>   #define PLL_INFF_MIN_RATE	19200000 /* in Hz */
>   #define PLL_INFF_MAX_RATE	38400000 /* in Hz */
>   
> +/* USBPHYC_CLK48 */
> +#define USBPHYC_CLK48_FREQ	48000000 /* in Hz */
> +
>   struct pll_params {
>   	u8 ndiv;
>   	u16 frac;
> @@ -355,6 +360,16 @@ static const struct phy_ops stm32_usbphyc_phy_ops = {
>   	.of_xlate = stm32_usbphyc_of_xlate,
>   };
>   
> +static int stm32_usbphyc_bind(struct udevice *dev)
> +{
> +	int ret;
> +
> +	ret = device_bind_driver_to_node(dev, "stm32-usbphyc-clk", "ck_usbo_48m",
> +					 dev_ofnode(dev), NULL);
> +
> +	return log_ret(ret);
> +}
> +
>   static int stm32_usbphyc_probe(struct udevice *dev)
>   {
>   	struct stm32_usbphyc *usbphyc = dev_get_priv(dev);
> @@ -444,6 +459,70 @@ U_BOOT_DRIVER(stm32_usb_phyc) = {
>   	.id = UCLASS_PHY,
>   	.of_match = stm32_usbphyc_of_match,
>   	.ops = &stm32_usbphyc_phy_ops,
> +	.bind = stm32_usbphyc_bind,
>   	.probe = stm32_usbphyc_probe,
>   	.priv_auto	= sizeof(struct stm32_usbphyc),
>   };
> +
> +struct stm32_usbphyc_clk {
> +	bool enable;
> +};
> +
> +static ulong stm32_usbphyc_clk48_get_rate(struct clk *clk)
> +{
> +	return USBPHYC_CLK48_FREQ;

What is the relationship between this clock and the PLL?

> +}
> +
> +static int stm32_usbphyc_clk48_enable(struct clk *clk)
> +{
> +	struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
> +	struct stm32_usbphyc *usbphyc;
> +	int ret;
> +
> +	if (usbphyc_clk->enable)
> +		return 0;
> +
> +	usbphyc = dev_get_priv(clk->dev->parent);
> +
> +	/* ck_usbo_48m is generated by usbphyc PLL */
> +	ret = stm32_usbphyc_pll_enable(usbphyc);
> +	if (ret)
> +		return ret;
> +
> +	usbphyc_clk->enable = true;
> +
> +	return 0;
> +}
> +
> +static int stm32_usbphyc_clk48_disable(struct clk *clk)
> +{
> +	struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
> +	struct stm32_usbphyc *usbphyc;
> +	int ret;
> +
> +	if (!usbphyc_clk->enable)
> +		return 0;
> +
> +	usbphyc = dev_get_priv(clk->dev->parent);
> +
> +	ret = stm32_usbphyc_pll_disable(usbphyc);
> +	if (ret)
> +		return ret;
> +
> +	usbphyc_clk->enable = false;
> +
> +	return 0;
> +}
> +
> +const struct clk_ops usbphyc_clk48_ops = {
> +	.get_rate = stm32_usbphyc_clk48_get_rate,
> +	.enable = stm32_usbphyc_clk48_enable,
> +	.disable = stm32_usbphyc_clk48_disable,
> +};
> +
> +U_BOOT_DRIVER(stm32_usb_phyc_clk) = {
> +	.name = "stm32-usbphyc-clk",
> +	.id = UCLASS_CLK,
> +	.ops = &usbphyc_clk48_ops,
> +	.priv_auto = sizeof(struct stm32_usbphyc_clk),
> +};
> 

I see that in the next patch you call this device from drivers/clk/clk_stm32mp1.c

Why is this clock a separate driver from the clock driver?

--Sean

Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Patrick DELAUNAY]

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);
>>
>

Re: [PATCH 2/3] phy: stm32-usbphyc: usbphyc is a clock provider of ck_usbo_48m clock

[Patrick DELAUNAY]

Hi Sean

On 5/8/22 20:23, Sean Anderson wrote:
> On 4/26/22 8:37 AM, Patrick Delaunay wrote:
>> ck_usbo_48m is generated by usbphyc PLL and used by OTG controller
>> for Full-Speed use cases with dedicated Full-Speed transceiver.
>>
>> ck_usbo_48m is available as soon as the PLL is enabled.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>>
>>   drivers/phy/phy-stm32-usbphyc.c | 79 +++++++++++++++++++++++++++++++++
>>   1 file changed, 79 insertions(+)
>>
>> diff --git a/drivers/phy/phy-stm32-usbphyc.c 
>> b/drivers/phy/phy-stm32-usbphyc.c
>> index 16c8799eca..e0b8fcb8f2 100644
>> --- a/drivers/phy/phy-stm32-usbphyc.c
>> +++ b/drivers/phy/phy-stm32-usbphyc.c
>> @@ -7,6 +7,7 @@
>>     #include <common.h>
>>   #include <clk.h>
>> +#include <clk-uclass.h>
>>   #include <div64.h>
>>   #include <dm.h>
>>   #include <fdtdec.h>
>> @@ -17,6 +18,7 @@
>>   #include <usb.h>
>>   #include <asm/io.h>
>>   #include <dm/device_compat.h>
>> +#include <dm/lists.h>
>>   #include <linux/bitops.h>
>>   #include <linux/delay.h>
>>   #include <power/regulator.h>
>> @@ -49,6 +51,9 @@
>>   #define PLL_INFF_MIN_RATE    19200000 /* in Hz */
>>   #define PLL_INFF_MAX_RATE    38400000 /* in Hz */
>>   +/* USBPHYC_CLK48 */
>> +#define USBPHYC_CLK48_FREQ    48000000 /* in Hz */
>> +
>>   struct pll_params {
>>       u8 ndiv;
>>       u16 frac;
>> @@ -355,6 +360,16 @@ static const struct phy_ops 
>> stm32_usbphyc_phy_ops = {
>>       .of_xlate = stm32_usbphyc_of_xlate,
>>   };
>>   +static int stm32_usbphyc_bind(struct udevice *dev)
>> +{
>> +    int ret;
>> +
>> +    ret = device_bind_driver_to_node(dev, "stm32-usbphyc-clk", 
>> "ck_usbo_48m",
>> +                     dev_ofnode(dev), NULL);
>> +
>> +    return log_ret(ret);
>> +}
>> +
>>   static int stm32_usbphyc_probe(struct udevice *dev)
>>   {
>>       struct stm32_usbphyc *usbphyc = dev_get_priv(dev);
>> @@ -444,6 +459,70 @@ U_BOOT_DRIVER(stm32_usb_phyc) = {
>>       .id = UCLASS_PHY,
>>       .of_match = stm32_usbphyc_of_match,
>>       .ops = &stm32_usbphyc_phy_ops,
>> +    .bind = stm32_usbphyc_bind,
>>       .probe = stm32_usbphyc_probe,
>>       .priv_auto    = sizeof(struct stm32_usbphyc),
>>   };
>> +
>> +struct stm32_usbphyc_clk {
>> +    bool enable;
>> +};
>> +
>> +static ulong stm32_usbphyc_clk48_get_rate(struct clk *clk)
>> +{
>> +    return USBPHYC_CLK48_FREQ;
>
> What is the relationship between this clock and the PLL?


The output clock of USBPHYC, running a 48MHz is only available when the 
USBPHYC is initialized and the PLL is enabled.

=> ck_usbo_48m is available as soon as the PLL is enabled.

see the associated code in kernel:

https://lore.kernel.org/all/20210208114659.15269-1-amelie.delaunay@foss.st.com/T/


>
>> +}
>> +
>> +static int stm32_usbphyc_clk48_enable(struct clk *clk)
>> +{
>> +    struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
>> +    struct stm32_usbphyc *usbphyc;
>> +    int ret;
>> +
>> +    if (usbphyc_clk->enable)
>> +        return 0;
>> +
>> +    usbphyc = dev_get_priv(clk->dev->parent);
>> +
>> +    /* ck_usbo_48m is generated by usbphyc PLL */
>> +    ret = stm32_usbphyc_pll_enable(usbphyc);
>> +    if (ret)
>> +        return ret;
>> +
>> +    usbphyc_clk->enable = true;
>> +
>> +    return 0;
>> +}
>> +
>> +static int stm32_usbphyc_clk48_disable(struct clk *clk)
>> +{ usbphyc provides a unique clock called ck_usbo_48m.
>> STM32 USB OTG needs a 48Mhz clock (utmifs_clk48) for Full-Speed 
>> operation.
>> ck_usbo_48m is a possible parent clock for USB OTG 48Mhz clock.
>> +    struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
>> +    struct stm32_usbphyc *usbphyc;
>> +    int ret;
>> +
>> +    if (!usbphyc_clk->enable)
>> +        return 0;
>> +
>> +    usbphyc = dev_get_priv(clk->dev->parent);
>> +
>> +    ret = stm32_usbphyc_pll_disable(usbphyc);
>> +    if (ret)
>> +        return ret;
>> +
>> +    usbphyc_clk->enable = false;
>> +
>> +    return 0;
>> +}
>> +
>> +const struct clk_ops usbphyc_clk48_ops = {
>> +    .get_rate = stm32_usbphyc_clk48_get_rate,
>> +    .enable = stm32_usbphyc_clk48_enable,
>> +    .disable = stm32_usbphyc_clk48_disable,
>> +};
>> +
>> +U_BOOT_DRIVER(stm32_usb_phyc_clk) = {
>> +    .name = "stm32-usbphyc-clk",
>> +    .id = UCLASS_CLK,
>> +    .ops = &usbphyc_clk48_ops,
>> +    .priv_auto = sizeof(struct stm32_usbphyc_clk),
>> +};
>>
>
> I see that in the next patch you call this device from 
> drivers/clk/clk_stm32mp1.c
>
> Why is this clock a separate driver from the clock driver?


The clock driver STM32MP1 = drivers/clk/clk_stm32mp1.c manage the clock in

the hardware IP RCC include in STM32MPU

=> 4 PLL and several oscillators LSI / HSI / CSI / HSE / LSE

=> manage clock source for many peripheral clock in STM32 MPU


The USBPHYC drivers manages the USB PHYC hardware block, it manage a PHY for

USB operation (device or host).

This hardware block have a internal PLL for its operation which is managed

directly by USBPHYC registers:

   usbphyc provides a unique clock called ck_usbo_48m.
   STM32 USB OTG needs a 48Mhz clock (utmifs_clk48) for Full-Speed operation.
   ck_usbo_48m is a possible parent clock for USB OTG 48Mhz clock.


This PLL is required for internal operation of USBPHYC (this PLL need to 
be enable

to allow USB detection for example) but also as input clock for RCC 
peripheral clock

USBO is "rcc_ck_usbo_48m" (see RCC_USBCKSELR.USBOSRC : USB OTG kernel clock

source selection)


So in RCC driver, we need to get the rate (it is trivial = 48MHz)

and enable the PLL when the peripheral clock USBO is required.


For this last point I prefer export a generic clock provider = CLK 
UCLASS than

export a specific usbphy function or duplicate USBPHYC code in RCC CLK 
driver.

even if the enable request on "rcc_ck_usbo_48m" is not managed in STM32MP15x

RCC driver...(stm32mp1_clk_enable don't enable the parent clock)


This patch prepares the STM32MP13x RCC driver introduction (coming soon),

based on CCF (common clock framework, ported n U-Boot):

each clock on the system is a CLK UCLASS and the dependencies are correctly

handle (each parent clock is enable when a clock is enable) but without

a CLK UCLASS for each USBO parent, a error occur during STM32MP13x CLK

driver probe (it is linked to drivers/clk/clk.c::clk_register(), with 
unkwon parent).


I hope that it is more clear.


>
> --Sean


Patrick

Re: [Uboot-stm32] [PATCH 3/3] clk: stm32mp: handle ck_usbo_48m clock provided by USBPHYC

[Patrice CHOTARD]

On 5/6/22 16:26, Patrice CHOTARD wrote:
> Hi Patrick
> 
> On 4/26/22 14:37, Patrick Delaunay wrote:
>> Handle the input clock of RCC USB_PHY_48, provided by USBPHYC
>> and named "ck_usbo_48m".
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>>
>>  drivers/clk/clk_stm32mp1.c | 35 ++++++++++++++++++++---------------
>>  1 file changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/clk/clk_stm32mp1.c b/drivers/clk/clk_stm32mp1.c
>> index 83ab6b728e..a02921c43a 100644
>> --- a/drivers/clk/clk_stm32mp1.c
>> +++ b/drivers/clk/clk_stm32mp1.c
>> @@ -962,6 +962,24 @@ static ulong stm32mp1_read_pll_freq(struct stm32mp1_clk_priv *priv,
>>  	return dfout;
>>  }
>>  
>> +static ulong stm32mp1_clk_get_by_name(const char *name)
>> +{
>> +	struct clk clk;
>> +	struct udevice *dev = NULL;
>> +	ulong clock = 0;
>> +
>> +	if (!uclass_get_device_by_name(UCLASS_CLK, name, &dev)) {
>> +		if (clk_request(dev, &clk)) {
>> +			log_err("%s request", name);
>> +		} else {
>> +			clk.id = 0;
>> +			clock = clk_get_rate(&clk);
>> +		}
>> +	}
>> +
>> +	return clock;
>> +}
>> +
>>  static ulong stm32mp1_clk_get(struct stm32mp1_clk_priv *priv, int p)
>>  {
>>  	u32 reg;
>> @@ -1127,24 +1145,11 @@ static ulong stm32mp1_clk_get(struct stm32mp1_clk_priv *priv, int p)
>>  		break;
>>  	/* other */
>>  	case _USB_PHY_48:
>> -		clock = 48000000;
>> +		clock = stm32mp1_clk_get_by_name("ck_usbo_48m");
>>  		break;
>>  	case _DSI_PHY:
>> -	{
>> -		struct clk clk;
>> -		struct udevice *dev = NULL;
>> -
>> -		if (!uclass_get_device_by_name(UCLASS_CLK, "ck_dsi_phy",
>> -					       &dev)) {
>> -			if (clk_request(dev, &clk)) {
>> -				log_err("ck_dsi_phy request");
>> -			} else {
>> -				clk.id = 0;
>> -				clock = clk_get_rate(&clk);
>> -			}
>> -		}
>> +		clock = stm32mp1_clk_get_by_name("ck_dsi_phy");
>>  		break;
>> -	}
>>  	default:
>>  		break;
>>  	}
> 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

Re: [Uboot-stm32] [PATCH 2/3] phy: stm32-usbphyc: usbphyc is a clock provider of ck_usbo_48m clock

[Patrice CHOTARD]

On 5/6/22 16:24, Patrice CHOTARD wrote:
> Hi Patrick
> 
> On 4/26/22 14:37, Patrick Delaunay wrote:
>> ck_usbo_48m is generated by usbphyc PLL and used by OTG controller
>> for Full-Speed use cases with dedicated Full-Speed transceiver.
>>
>> ck_usbo_48m is available as soon as the PLL is enabled.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>>
>>  drivers/phy/phy-stm32-usbphyc.c | 79 +++++++++++++++++++++++++++++++++
>>  1 file changed, 79 insertions(+)
>>
>> diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c
>> index 16c8799eca..e0b8fcb8f2 100644
>> --- a/drivers/phy/phy-stm32-usbphyc.c
>> +++ b/drivers/phy/phy-stm32-usbphyc.c
>> @@ -7,6 +7,7 @@
>>  
>>  #include <common.h>
>>  #include <clk.h>
>> +#include <clk-uclass.h>
>>  #include <div64.h>
>>  #include <dm.h>
>>  #include <fdtdec.h>
>> @@ -17,6 +18,7 @@
>>  #include <usb.h>
>>  #include <asm/io.h>
>>  #include <dm/device_compat.h>
>> +#include <dm/lists.h>
>>  #include <linux/bitops.h>
>>  #include <linux/delay.h>
>>  #include <power/regulator.h>
>> @@ -49,6 +51,9 @@
>>  #define PLL_INFF_MIN_RATE	19200000 /* in Hz */
>>  #define PLL_INFF_MAX_RATE	38400000 /* in Hz */
>>  
>> +/* USBPHYC_CLK48 */
>> +#define USBPHYC_CLK48_FREQ	48000000 /* in Hz */
>> +
>>  struct pll_params {
>>  	u8 ndiv;
>>  	u16 frac;
>> @@ -355,6 +360,16 @@ static const struct phy_ops stm32_usbphyc_phy_ops = {
>>  	.of_xlate = stm32_usbphyc_of_xlate,
>>  };
>>  
>> +static int stm32_usbphyc_bind(struct udevice *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = device_bind_driver_to_node(dev, "stm32-usbphyc-clk", "ck_usbo_48m",
>> +					 dev_ofnode(dev), NULL);
>> +
>> +	return log_ret(ret);
>> +}
>> +
>>  static int stm32_usbphyc_probe(struct udevice *dev)
>>  {
>>  	struct stm32_usbphyc *usbphyc = dev_get_priv(dev);
>> @@ -444,6 +459,70 @@ U_BOOT_DRIVER(stm32_usb_phyc) = {
>>  	.id = UCLASS_PHY,
>>  	.of_match = stm32_usbphyc_of_match,
>>  	.ops = &stm32_usbphyc_phy_ops,
>> +	.bind = stm32_usbphyc_bind,
>>  	.probe = stm32_usbphyc_probe,
>>  	.priv_auto	= sizeof(struct stm32_usbphyc),
>>  };
>> +
>> +struct stm32_usbphyc_clk {
>> +	bool enable;
>> +};
>> +
>> +static ulong stm32_usbphyc_clk48_get_rate(struct clk *clk)
>> +{
>> +	return USBPHYC_CLK48_FREQ;
>> +}
>> +
>> +static int stm32_usbphyc_clk48_enable(struct clk *clk)
>> +{
>> +	struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
>> +	struct stm32_usbphyc *usbphyc;
>> +	int ret;
>> +
>> +	if (usbphyc_clk->enable)
>> +		return 0;
>> +
>> +	usbphyc = dev_get_priv(clk->dev->parent);
>> +
>> +	/* ck_usbo_48m is generated by usbphyc PLL */
>> +	ret = stm32_usbphyc_pll_enable(usbphyc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	usbphyc_clk->enable = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_usbphyc_clk48_disable(struct clk *clk)
>> +{
>> +	struct stm32_usbphyc_clk *usbphyc_clk = dev_get_priv(clk->dev);
>> +	struct stm32_usbphyc *usbphyc;
>> +	int ret;
>> +
>> +	if (!usbphyc_clk->enable)
>> +		return 0;
>> +
>> +	usbphyc = dev_get_priv(clk->dev->parent);
>> +
>> +	ret = stm32_usbphyc_pll_disable(usbphyc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	usbphyc_clk->enable = false;
>> +
>> +	return 0;
>> +}
>> +
>> +const struct clk_ops usbphyc_clk48_ops = {
>> +	.get_rate = stm32_usbphyc_clk48_get_rate,
>> +	.enable = stm32_usbphyc_clk48_enable,
>> +	.disable = stm32_usbphyc_clk48_disable,
>> +};
>> +
>> +U_BOOT_DRIVER(stm32_usb_phyc_clk) = {
>> +	.name = "stm32-usbphyc-clk",
>> +	.id = UCLASS_CLK,
>> +	.ops = &usbphyc_clk48_ops,
>> +	.priv_auto = sizeof(struct stm32_usbphyc_clk),
>> +};
> 
> 
> 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

Re: [Uboot-stm32] [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Patrice CHOTARD]

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

Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Amelie Delaunay]

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);
>>>
>>

Re: [Uboot-stm32] [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Patrice CHOTARD]

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

Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Sean Anderson]

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);
>>>>
>>>

Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Patrick DELAUNAY]

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

.

Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Sean Anderson]

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

Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Patrick DELAUNAY]

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

Re: [PATCH 2/3] phy: stm32-usbphyc: usbphyc is a clock provider of ck_usbo_48m clock

[Patrick DELAUNAY]

Hi,

On 4/26/22 14:37, Patrick Delaunay wrote:
> ck_usbo_48m is generated by usbphyc PLL and used by OTG controller
> for Full-Speed use cases with dedicated Full-Speed transceiver.
>
> ck_usbo_48m is available as soon as the PLL is enabled.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
>   drivers/phy/phy-stm32-usbphyc.c | 79 +++++++++++++++++++++++++++++++++
>   1 file changed, 79 insertions(+)
>

Applied to u-boot-stm/master, thanks!

Regards
Patrick

Re: [PATCH 3/3] clk: stm32mp: handle ck_usbo_48m clock provided by USBPHYC

[Patrick DELAUNAY]

Hi,

On 4/26/22 14:37, Patrick Delaunay wrote:
> Handle the input clock of RCC USB_PHY_48, provided by USBPHYC
> and named "ck_usbo_48m".
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
>   drivers/clk/clk_stm32mp1.c | 35 ++++++++++++++++++++---------------
>   1 file changed, 20 insertions(+), 15 deletions(-)
>

Applied to u-boot-stm/master, thanks!

Regards
Patrick