[PATCH] clk: imx: introduce imx_clk_hw_critical

[PATCH] clk: imx: introduce imx_clk_hw_critical

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

To i.MX8M SoC, there is an case is when running dual OSes
with hypervisor, the clk of the hardware that passthrough
to the 2nd OS needs to be setup by 1st Linux OS.
So detect clock-critical from ccm node and enable the clocks to let
the 2nd OS could use the hardware without touch CCM module.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/clk.c | 19 +++++++++++++++++++
 drivers/clk/imx/clk.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index 87ab8db3d282..ec7d422540c1 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -177,3 +177,22 @@ static int __init imx_clk_disable_uart(void)
 	return 0;
 }
 late_initcall_sync(imx_clk_disable_uart);
+
+int imx_clk_hw_critical(struct device_node *np, struct clk_hw * const hws[])
+{
+	struct property *prop;
+	const __be32 *cur;
+	u32 idx;
+	int ret;
+
+	if (!np || !hws)
+		return -EINVAL;
+
+	of_property_for_each_u32(np, "clock-critical", prop, cur, idx) {
+		ret = clk_prepare_enable(hws[idx]->clk);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index d4ea1609bcb7..701d7440f98c 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -9,6 +9,7 @@ extern spinlock_t imx_ccm_lock;
 
 void imx_check_clocks(struct clk *clks[], unsigned int count);
 void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
+int imx_clk_hw_critical(struct device_node *np, struct clk_hw * const hws[]);
 void imx_register_uart_clocks(struct clk ** const clks[]);
 void imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned int chn);
 void imx_unregister_clocks(struct clk *clks[], unsigned int count);
-- 
2.16.4

RE: [PATCH] clk: imx: introduce imx_clk_hw_critical

[Aisheng Dong]

> From: Peng Fan <peng.fan@nxp.com>
> Sent: Thursday, April 23, 2020 2:52 PM
> 
> To i.MX8M SoC, there is an case is when running dual OSes with hypervisor, the
> clk of the hardware that passthrough to the 2nd OS needs to be setup by 1st
> Linux OS.
> So detect clock-critical from ccm node and enable the clocks to let the 2nd OS
> could use the hardware without touch CCM module.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/imx/clk.c | 19 +++++++++++++++++++  drivers/clk/imx/clk.h |  1
> +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c index
> 87ab8db3d282..ec7d422540c1 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -177,3 +177,22 @@ static int __init imx_clk_disable_uart(void)
>  	return 0;
>  }
>  late_initcall_sync(imx_clk_disable_uart);
> +
> +int imx_clk_hw_critical(struct device_node *np, struct clk_hw * const
> +hws[]) {
> +	struct property *prop;
> +	const __be32 *cur;
> +	u32 idx;
> +	int ret;
> +
> +	if (!np || !hws)
> +		return -EINVAL;
> +
> +	of_property_for_each_u32(np, "clock-critical", prop, cur, idx) {

Is there a binding for it already?

Regards
Aisheng

> +		ret = clk_prepare_enable(hws[idx]->clk);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index
> d4ea1609bcb7..701d7440f98c 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -9,6 +9,7 @@ extern spinlock_t imx_ccm_lock;
> 
>  void imx_check_clocks(struct clk *clks[], unsigned int count);  void
> imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
> +int imx_clk_hw_critical(struct device_node *np, struct clk_hw * const
> +hws[]);
>  void imx_register_uart_clocks(struct clk ** const clks[]);  void
> imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned int chn);
> void imx_unregister_clocks(struct clk *clks[], unsigned int count);
> --
> 2.16.4

RE: [PATCH] clk: imx: introduce imx_clk_hw_critical

[Peng Fan]

> Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> 
> > From: Peng Fan <peng.fan@nxp.com>
> > Sent: Thursday, April 23, 2020 2:52 PM
> >
> > To i.MX8M SoC, there is an case is when running dual OSes with
> > hypervisor, the clk of the hardware that passthrough to the 2nd OS
> > needs to be setup by 1st Linux OS.
> > So detect clock-critical from ccm node and enable the clocks to let
> > the 2nd OS could use the hardware without touch CCM module.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/clk/imx/clk.c | 19 +++++++++++++++++++  drivers/clk/imx/clk.h
> > |  1
> > +
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c index
> > 87ab8db3d282..ec7d422540c1 100644
> > --- a/drivers/clk/imx/clk.c
> > +++ b/drivers/clk/imx/clk.c
> > @@ -177,3 +177,22 @@ static int __init imx_clk_disable_uart(void)
> >  	return 0;
> >  }
> >  late_initcall_sync(imx_clk_disable_uart);
> > +
> > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw * const
> > +hws[]) {
> > +	struct property *prop;
> > +	const __be32 *cur;
> > +	u32 idx;
> > +	int ret;
> > +
> > +	if (!np || !hws)
> > +		return -EINVAL;
> > +
> > +	of_property_for_each_u32(np, "clock-critical", prop, cur, idx) {
> 
> Is there a binding for it already?

I think clock-critical is a common bindings? See of_clk_detect_critical.
Please review whether this approach is acceptable, if do
need bindings, I could add that in v2.

Thanks,
Peng.

> 
> Regards
> Aisheng
> 
> > +		ret = clk_prepare_enable(hws[idx]->clk);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index
> > d4ea1609bcb7..701d7440f98c 100644
> > --- a/drivers/clk/imx/clk.h
> > +++ b/drivers/clk/imx/clk.h
> > @@ -9,6 +9,7 @@ extern spinlock_t imx_ccm_lock;
> >
> >  void imx_check_clocks(struct clk *clks[], unsigned int count);  void
> > imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
> > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw * const
> > +hws[]);
> >  void imx_register_uart_clocks(struct clk ** const clks[]);  void
> > imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned int chn);
> > void imx_unregister_clocks(struct clk *clks[], unsigned int count);
> > --
> > 2.16.4

RE: [PATCH] clk: imx: introduce imx_clk_hw_critical

[Aisheng Dong]

> From: Peng Fan <peng.fan@nxp.com>
> Sent: Thursday, April 23, 2020 6:54 PM
> 
> > Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> >
> > > From: Peng Fan <peng.fan@nxp.com>
> > > Sent: Thursday, April 23, 2020 2:52 PM
> > >
> > > To i.MX8M SoC, there is an case is when running dual OSes with
> > > hypervisor, the clk of the hardware that passthrough to the 2nd OS
> > > needs to be setup by 1st Linux OS.
> > > So detect clock-critical from ccm node and enable the clocks to let
> > > the 2nd OS could use the hardware without touch CCM module.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/clk/imx/clk.c | 19 +++++++++++++++++++
> > > drivers/clk/imx/clk.h
> > > |  1
> > > +
> > >  2 files changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c index
> > > 87ab8db3d282..ec7d422540c1 100644
> > > --- a/drivers/clk/imx/clk.c
> > > +++ b/drivers/clk/imx/clk.c
> > > @@ -177,3 +177,22 @@ static int __init imx_clk_disable_uart(void)
> > >  	return 0;
> > >  }
> > >  late_initcall_sync(imx_clk_disable_uart);
> > > +
> > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw *
> > > +const
> > > +hws[]) {
> > > +	struct property *prop;
> > > +	const __be32 *cur;
> > > +	u32 idx;
> > > +	int ret;
> > > +
> > > +	if (!np || !hws)
> > > +		return -EINVAL;
> > > +
> > > +	of_property_for_each_u32(np, "clock-critical", prop, cur, idx) {
> >
> > Is there a binding for it already?
> 
> I think clock-critical is a common bindings? See of_clk_detect_critical.
> Please review whether this approach is acceptable, if do need bindings, I could
> add that in v2.
> 

I'm ok if it's a common binding already supported by current clk framework.
But it seems to be more like a common clk feature rather than platform feature.
Also a bit strange that why I did not find the binding doc in latest kernel.
Maybe Stephen could comment more.

BTW, if using this for dual OSes case, will those critical clocks affect the normal users that
not booting the second OS?

Regards
Aisheng

> Thanks,
> Peng.
> 
> >
> > Regards
> > Aisheng
> >
> > > +		ret = clk_prepare_enable(hws[idx]->clk);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index
> > > d4ea1609bcb7..701d7440f98c 100644
> > > --- a/drivers/clk/imx/clk.h
> > > +++ b/drivers/clk/imx/clk.h
> > > @@ -9,6 +9,7 @@ extern spinlock_t imx_ccm_lock;
> > >
> > >  void imx_check_clocks(struct clk *clks[], unsigned int count);
> > > void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
> > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw *
> > > +const hws[]);
> > >  void imx_register_uart_clocks(struct clk ** const clks[]);  void
> > > imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned int chn);
> > > void imx_unregister_clocks(struct clk *clks[], unsigned int count);
> > > --
> > > 2.16.4

RE: [PATCH] clk: imx: introduce imx_clk_hw_critical

[Peng Fan]

> Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> 
> > From: Peng Fan <peng.fan@nxp.com>
> > Sent: Thursday, April 23, 2020 6:54 PM
> >
> > > Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > > Sent: Thursday, April 23, 2020 2:52 PM
> > > >
> > > > To i.MX8M SoC, there is an case is when running dual OSes with
> > > > hypervisor, the clk of the hardware that passthrough to the 2nd OS
> > > > needs to be setup by 1st Linux OS.
> > > > So detect clock-critical from ccm node and enable the clocks to
> > > > let the 2nd OS could use the hardware without touch CCM module.
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  drivers/clk/imx/clk.c | 19 +++++++++++++++++++
> > > > drivers/clk/imx/clk.h
> > > > |  1
> > > > +
> > > >  2 files changed, 20 insertions(+)
> > > >
> > > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c index
> > > > 87ab8db3d282..ec7d422540c1 100644
> > > > --- a/drivers/clk/imx/clk.c
> > > > +++ b/drivers/clk/imx/clk.c
> > > > @@ -177,3 +177,22 @@ static int __init imx_clk_disable_uart(void)
> > > >  	return 0;
> > > >  }
> > > >  late_initcall_sync(imx_clk_disable_uart);
> > > > +
> > > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw *
> > > > +const
> > > > +hws[]) {
> > > > +	struct property *prop;
> > > > +	const __be32 *cur;
> > > > +	u32 idx;
> > > > +	int ret;
> > > > +
> > > > +	if (!np || !hws)
> > > > +		return -EINVAL;
> > > > +
> > > > +	of_property_for_each_u32(np, "clock-critical", prop, cur, idx) {
> > >
> > > Is there a binding for it already?
> >
> > I think clock-critical is a common bindings? See of_clk_detect_critical.
> > Please review whether this approach is acceptable, if do need
> > bindings, I could add that in v2.
> >
> 
> I'm ok if it's a common binding already supported by current clk framework.
> But it seems to be more like a common clk feature rather than platform
> feature.

Here is to use it for dual Linux case running on i.MX plaforms.
It is to replace the init-on-array property in downstream kernel.

The recommended critical clock is using CLK_IS_CRITICAL flag in clk driver,
not use clock-critical property.

But here, we could not use CLK_IS_CRITICAL, because when support
dual OSes, it is not flexible, and it will affect non hypervisor case.

> Also a bit strange that why I did not find the binding doc in latest kernel.
> Maybe Stephen could comment more.

It is used here.
https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/clk/clk.c#L4891

> 
> BTW, if using this for dual OSes case, will those critical clocks affect the normal
> users that not booting the second OS?

No. dual os using x-root.dts, not x.dts. critical-clock will be push in x-root.dts.

Thanks,
Peng.

> 
> Regards
> Aisheng
> 
> > Thanks,
> > Peng.
> >
> > >
> > > Regards
> > > Aisheng
> > >
> > > > +		ret = clk_prepare_enable(hws[idx]->clk);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index
> > > > d4ea1609bcb7..701d7440f98c 100644
> > > > --- a/drivers/clk/imx/clk.h
> > > > +++ b/drivers/clk/imx/clk.h
> > > > @@ -9,6 +9,7 @@ extern spinlock_t imx_ccm_lock;
> > > >
> > > >  void imx_check_clocks(struct clk *clks[], unsigned int count);
> > > > void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
> > > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw *
> > > > +const hws[]);
> > > >  void imx_register_uart_clocks(struct clk ** const clks[]);  void
> > > > imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned int
> chn);
> > > > void imx_unregister_clocks(struct clk *clks[], unsigned int
> > > > count);
> > > > --
> > > > 2.16.4

RE: [PATCH] clk: imx: introduce imx_clk_hw_critical

[Aisheng Dong]

> From: Peng Fan <peng.fan@nxp.com>
> Sent: Friday, April 24, 2020 9:08 AM
> 
> > Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> >
> > > From: Peng Fan <peng.fan@nxp.com>
> > > Sent: Thursday, April 23, 2020 6:54 PM
> > >
> > > > Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> > > >
> > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > > Sent: Thursday, April 23, 2020 2:52 PM
> > > > >
> > > > > To i.MX8M SoC, there is an case is when running dual OSes with
> > > > > hypervisor, the clk of the hardware that passthrough to the 2nd
> > > > > OS needs to be setup by 1st Linux OS.
> > > > > So detect clock-critical from ccm node and enable the clocks to
> > > > > let the 2nd OS could use the hardware without touch CCM module.
> > > > >
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---
> > > > >  drivers/clk/imx/clk.c | 19 +++++++++++++++++++
> > > > > drivers/clk/imx/clk.h
> > > > > |  1
> > > > > +
> > > > >  2 files changed, 20 insertions(+)
> > > > >
> > > > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c index
> > > > > 87ab8db3d282..ec7d422540c1 100644
> > > > > --- a/drivers/clk/imx/clk.c
> > > > > +++ b/drivers/clk/imx/clk.c
> > > > > @@ -177,3 +177,22 @@ static int __init imx_clk_disable_uart(void)
> > > > >  	return 0;
> > > > >  }
> > > > >  late_initcall_sync(imx_clk_disable_uart);
> > > > > +
> > > > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw *
> > > > > +const
> > > > > +hws[]) {
> > > > > +	struct property *prop;
> > > > > +	const __be32 *cur;
> > > > > +	u32 idx;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!np || !hws)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	of_property_for_each_u32(np, "clock-critical", prop, cur, idx)
> > > > > +{
> > > >
> > > > Is there a binding for it already?
> > >
> > > I think clock-critical is a common bindings? See of_clk_detect_critical.
> > > Please review whether this approach is acceptable, if do need
> > > bindings, I could add that in v2.
> > >
> >
> > I'm ok if it's a common binding already supported by current clk framework.
> > But it seems to be more like a common clk feature rather than platform
> > feature.
> 
> Here is to use it for dual Linux case running on i.MX plaforms.
> It is to replace the init-on-array property in downstream kernel.
> 
> The recommended critical clock is using CLK_IS_CRITICAL flag in clk driver, not
> use clock-critical property.
> 

Clock-critical property seems DT usage of CLK_IS_CRITICAL flag and pure platform independent.
So I wonder if this feature could be added into of_clk_add_hw_provider.

> But here, we could not use CLK_IS_CRITICAL, because when support dual OSes,
> it is not flexible, and it will affect non hypervisor case.
> 

A bit confuing because you said the critical-clock property will only be used by
Hypervisor dts, then how to affect non hypervisor cases?

> > Also a bit strange that why I did not find the binding doc in latest kernel.
> > Maybe Stephen could comment more.
> 
> It is used here.
> https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/clk/clk.c#L4891

Yes, I also saw it, but didn't find binding doc.

Regards
Aisheng

> 
> >
> > BTW, if using this for dual OSes case, will those critical clocks
> > affect the normal users that not booting the second OS?
> 
> No. dual os using x-root.dts, not x.dts. critical-clock will be push in x-root.dts.
> 
> Thanks,
> Peng.
> 
> >
> > Regards
> > Aisheng
> >
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > Regards
> > > > Aisheng
> > > >
> > > > > +		ret = clk_prepare_enable(hws[idx]->clk);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index
> > > > > d4ea1609bcb7..701d7440f98c 100644
> > > > > --- a/drivers/clk/imx/clk.h
> > > > > +++ b/drivers/clk/imx/clk.h
> > > > > @@ -9,6 +9,7 @@ extern spinlock_t imx_ccm_lock;
> > > > >
> > > > >  void imx_check_clocks(struct clk *clks[], unsigned int count);
> > > > > void imx_check_clk_hws(struct clk_hw *clks[], unsigned int
> > > > > count);
> > > > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw *
> > > > > +const hws[]);
> > > > >  void imx_register_uart_clocks(struct clk ** const clks[]);
> > > > > void imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned
> > > > > int
> > chn);
> > > > > void imx_unregister_clocks(struct clk *clks[], unsigned int
> > > > > count);
> > > > > --
> > > > > 2.16.4

RE: [PATCH] clk: imx: introduce imx_clk_hw_critical

[Peng Fan]

> Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> 
> > From: Peng Fan <peng.fan@nxp.com>
> > Sent: Friday, April 24, 2020 9:08 AM
> >
> > > Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > > Sent: Thursday, April 23, 2020 6:54 PM
> > > >
> > > > > Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> > > > >
> > > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > > > Sent: Thursday, April 23, 2020 2:52 PM
> > > > > >
> > > > > > To i.MX8M SoC, there is an case is when running dual OSes with
> > > > > > hypervisor, the clk of the hardware that passthrough to the
> > > > > > 2nd OS needs to be setup by 1st Linux OS.
> > > > > > So detect clock-critical from ccm node and enable the clocks
> > > > > > to let the 2nd OS could use the hardware without touch CCM
> module.
> > > > > >
> > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > > ---
> > > > > >  drivers/clk/imx/clk.c | 19 +++++++++++++++++++
> > > > > > drivers/clk/imx/clk.h
> > > > > > |  1
> > > > > > +
> > > > > >  2 files changed, 20 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > > > > > index
> > > > > > 87ab8db3d282..ec7d422540c1 100644
> > > > > > --- a/drivers/clk/imx/clk.c
> > > > > > +++ b/drivers/clk/imx/clk.c
> > > > > > @@ -177,3 +177,22 @@ static int __init imx_clk_disable_uart(void)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  late_initcall_sync(imx_clk_disable_uart);
> > > > > > +
> > > > > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw
> > > > > > +* const
> > > > > > +hws[]) {
> > > > > > +	struct property *prop;
> > > > > > +	const __be32 *cur;
> > > > > > +	u32 idx;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (!np || !hws)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	of_property_for_each_u32(np, "clock-critical", prop, cur,
> > > > > > +idx) {
> > > > >
> > > > > Is there a binding for it already?
> > > >
> > > > I think clock-critical is a common bindings? See of_clk_detect_critical.
> > > > Please review whether this approach is acceptable, if do need
> > > > bindings, I could add that in v2.
> > > >
> > >
> > > I'm ok if it's a common binding already supported by current clk
> framework.
> > > But it seems to be more like a common clk feature rather than
> > > platform feature.
> >
> > Here is to use it for dual Linux case running on i.MX plaforms.
> > It is to replace the init-on-array property in downstream kernel.
> >
> > The recommended critical clock is using CLK_IS_CRITICAL flag in clk
> > driver, not use clock-critical property.
> >
> 
> Clock-critical property seems DT usage of CLK_IS_CRITICAL flag and pure
> platform independent.
> So I wonder if this feature could be added into of_clk_add_hw_provider.
> 
> > But here, we could not use CLK_IS_CRITICAL, because when support dual
> > OSes, it is not flexible, and it will affect non hypervisor case.
> >
> 
> A bit confuing because you said the critical-clock property will only be used by
> Hypervisor dts, then how to affect non hypervisor cases?

If put the array into hypervisor dts, it will not affect non hypervisor case.

I mean if using CLK_IS_CRITICAL in driver code, it will affect non hypervisor case.

Thanks,
Peng.

> 
> > > Also a bit strange that why I did not find the binding doc in latest kernel.
> > > Maybe Stephen could comment more.
> >
> > It is used here.
> > https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/clk/clk.c#L48
> > 91
> 
> Yes, I also saw it, but didn't find binding doc.
> 
> Regards
> Aisheng
> 
> >
> > >
> > > BTW, if using this for dual OSes case, will those critical clocks
> > > affect the normal users that not booting the second OS?
> >
> > No. dual os using x-root.dts, not x.dts. critical-clock will be push in
> x-root.dts.
> >
> > Thanks,
> > Peng.
> >
> > >
> > > Regards
> > > Aisheng
> > >
> > > > Thanks,
> > > > Peng.
> > > >
> > > > >
> > > > > Regards
> > > > > Aisheng
> > > > >
> > > > > > +		ret = clk_prepare_enable(hws[idx]->clk);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> > > > > > index d4ea1609bcb7..701d7440f98c 100644
> > > > > > --- a/drivers/clk/imx/clk.h
> > > > > > +++ b/drivers/clk/imx/clk.h
> > > > > > @@ -9,6 +9,7 @@ extern spinlock_t imx_ccm_lock;
> > > > > >
> > > > > >  void imx_check_clocks(struct clk *clks[], unsigned int
> > > > > > count); void imx_check_clk_hws(struct clk_hw *clks[], unsigned
> > > > > > int count);
> > > > > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw
> > > > > > +* const hws[]);
> > > > > >  void imx_register_uart_clocks(struct clk ** const clks[]);
> > > > > > void imx_mmdc_mask_handshake(void __iomem *ccm_base,
> unsigned
> > > > > > int
> > > chn);
> > > > > > void imx_unregister_clocks(struct clk *clks[], unsigned int
> > > > > > count);
> > > > > > --
> > > > > > 2.16.4

RE: [PATCH] clk: imx: introduce imx_clk_hw_critical

[Peng Fan]

> Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> 
> > From: Peng Fan <peng.fan@nxp.com>
> > Sent: Friday, April 24, 2020 9:08 AM
> >
> > > Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > > Sent: Thursday, April 23, 2020 6:54 PM
> > > >
> > > > > Subject: RE: [PATCH] clk: imx: introduce imx_clk_hw_critical
> > > > >
> > > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > > > Sent: Thursday, April 23, 2020 2:52 PM
> > > > > >
> > > > > > To i.MX8M SoC, there is an case is when running dual OSes with
> > > > > > hypervisor, the clk of the hardware that passthrough to the
> > > > > > 2nd OS needs to be setup by 1st Linux OS.
> > > > > > So detect clock-critical from ccm node and enable the clocks
> > > > > > to let the 2nd OS could use the hardware without touch CCM
> module.
> > > > > >
> > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > > ---
> > > > > >  drivers/clk/imx/clk.c | 19 +++++++++++++++++++
> > > > > > drivers/clk/imx/clk.h
> > > > > > |  1
> > > > > > +
> > > > > >  2 files changed, 20 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > > > > > index
> > > > > > 87ab8db3d282..ec7d422540c1 100644
> > > > > > --- a/drivers/clk/imx/clk.c
> > > > > > +++ b/drivers/clk/imx/clk.c
> > > > > > @@ -177,3 +177,22 @@ static int __init imx_clk_disable_uart(void)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  late_initcall_sync(imx_clk_disable_uart);
> > > > > > +
> > > > > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw
> > > > > > +* const
> > > > > > +hws[]) {
> > > > > > +	struct property *prop;
> > > > > > +	const __be32 *cur;
> > > > > > +	u32 idx;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (!np || !hws)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	of_property_for_each_u32(np, "clock-critical", prop, cur,
> > > > > > +idx) {
> > > > >
> > > > > Is there a binding for it already?
> > > >
> > > > I think clock-critical is a common bindings? See of_clk_detect_critical.
> > > > Please review whether this approach is acceptable, if do need
> > > > bindings, I could add that in v2.
> > > >
> > >
> > > I'm ok if it's a common binding already supported by current clk
> framework.
> > > But it seems to be more like a common clk feature rather than
> > > platform feature.
> >
> > Here is to use it for dual Linux case running on i.MX plaforms.
> > It is to replace the init-on-array property in downstream kernel.
> >
> > The recommended critical clock is using CLK_IS_CRITICAL flag in clk
> > driver, not use clock-critical property.
> >
> 
> Clock-critical property seems DT usage of CLK_IS_CRITICAL flag and pure
> platform independent.
> So I wonder if this feature could be added into of_clk_add_hw_provider.

After check, of_clk_add_hw_provider takes node pointer and a function pointer
as parameter, however clock-critical needs clk index and the clk_hw array
pointer. It needs more changes to common code that I would not like
to do.

Thanks,
Peng.

> 
> > But here, we could not use CLK_IS_CRITICAL, because when support dual
> > OSes, it is not flexible, and it will affect non hypervisor case.
> >
> 
> A bit confuing because you said the critical-clock property will only be used by
> Hypervisor dts, then how to affect non hypervisor cases?
> 
> > > Also a bit strange that why I did not find the binding doc in latest kernel.
> > > Maybe Stephen could comment more.
> >
> > It is used here.
> > https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/clk/clk.c#L48
> > 91
> 
> Yes, I also saw it, but didn't find binding doc.
> 
> Regards
> Aisheng
> 
> >
> > >
> > > BTW, if using this for dual OSes case, will those critical clocks
> > > affect the normal users that not booting the second OS?
> >
> > No. dual os using x-root.dts, not x.dts. critical-clock will be push in
> x-root.dts.
> >
> > Thanks,
> > Peng.
> >
> > >
> > > Regards
> > > Aisheng
> > >
> > > > Thanks,
> > > > Peng.
> > > >
> > > > >
> > > > > Regards
> > > > > Aisheng
> > > > >
> > > > > > +		ret = clk_prepare_enable(hws[idx]->clk);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> > > > > > index d4ea1609bcb7..701d7440f98c 100644
> > > > > > --- a/drivers/clk/imx/clk.h
> > > > > > +++ b/drivers/clk/imx/clk.h
> > > > > > @@ -9,6 +9,7 @@ extern spinlock_t imx_ccm_lock;
> > > > > >
> > > > > >  void imx_check_clocks(struct clk *clks[], unsigned int
> > > > > > count); void imx_check_clk_hws(struct clk_hw *clks[], unsigned
> > > > > > int count);
> > > > > > +int imx_clk_hw_critical(struct device_node *np, struct clk_hw
> > > > > > +* const hws[]);
> > > > > >  void imx_register_uart_clocks(struct clk ** const clks[]);
> > > > > > void imx_mmdc_mask_handshake(void __iomem *ccm_base,
> unsigned
> > > > > > int
> > > chn);
> > > > > > void imx_unregister_clocks(struct clk *clks[], unsigned int
> > > > > > count);
> > > > > > --
> > > > > > 2.16.4