[PATCH 0/2] clk: imx: introduce fsl,protected-clocks

[PATCH 0/2] clk: imx: introduce fsl,protected-clocks

[Peng Fan (OSS)]

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

There are two cases that I wanna this property could serve:
Virtualization: root cell linux run in parallel with inmate cell
AMP: M7/4 runs in parallel with A53

The major case is:
Jailhouse hypervisor only support partition, so devices are partitioned.
But there is only CCM module that provides clock, the CCM is handled by
root cell linux, need make sure the root cell linux not shutdown the
clocks using by inmate cell.

I was thinking whether need to provide a rate entry to ask root cell
configure the clk rate for inmate cell. But NXP downstream not have it,
see https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/clk/imx/clk-imx8mp.c?h=lf-5.15.y#n690
So just leave the property as uint32-array.

This patchset could also benifit AMP case, check the two commits
commit 19565ea12d61 ("clk: imx: add mcore_booted module paratemter")
commit d097cc045b64 ("clk: imx8mp: remove SYS PLL 1/2 clock gates")
Although I not plan to drop the upper two patches, this patchset
exposes opportunity for better power consumption.

Peng Fan (2):
  dt-bindings: clock: imx8m: introduce fsl,protected-clocks property
  clk: imx: support fsl,protected-clocks

 .../bindings/clock/imx8m-clock.yaml           |  4 ++++
 drivers/clk/imx/clk-imx8mm.c                  |  2 ++
 drivers/clk/imx/clk-imx8mn.c                  |  2 ++
 drivers/clk/imx/clk-imx8mp.c                  |  2 ++
 drivers/clk/imx/clk-imx8mq.c                  |  2 ++
 drivers/clk/imx/clk.c                         | 21 +++++++++++++++++++
 drivers/clk/imx/clk.h                         |  2 ++
 7 files changed, 35 insertions(+)

-- 
2.37.1

[PATCH 1/2] dt-bindings: clock: imx8m: introduce fsl,protected-clocks property

[Peng Fan (OSS)]

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

i.MX8M Linux run on top of Jailhouse hypervisor, the root cell Linux
should not disable clocks used by inmate. This would also benifit
AMP to avoid Linux disable clocks used by Cortex-M4/M7.

So introduce fsl,protected-clocks for above case.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 Documentation/devicetree/bindings/clock/imx8m-clock.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
index 458c7645ee68..0ec490ff9a09 100644
--- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
+++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
@@ -39,6 +39,10 @@ properties:
       ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h
       for the full list of i.MX8M clock IDs.
 
+  fsl,protected-clocks:
+    description: List of the Protected clock.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
 required:
   - compatible
   - reg
-- 
2.37.1

[PATCH 2/2] clk: imx: support fsl,protected-clocks

[Peng Fan (OSS)]

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

For the clocks listed in fsl,protected-clocks, enable them to avoid
Linux disable them. This will benifit root Linux and inmate cell run
on top of Jailhouse hypervisor, and benifit AMP case.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/clk-imx8mm.c |  2 ++
 drivers/clk/imx/clk-imx8mn.c |  2 ++
 drivers/clk/imx/clk-imx8mp.c |  2 ++
 drivers/clk/imx/clk-imx8mq.c |  2 ++
 drivers/clk/imx/clk.c        | 21 +++++++++++++++++++++
 drivers/clk/imx/clk.h        |  2 ++
 6 files changed, 31 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index b6d275855b36..24ddb1620bce 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -611,6 +611,8 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 
 	imx_register_uart_clocks(4);
 
+	imx_clk_protect(dev, hws);
+
 	return 0;
 
 unregister_hws:
diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index d37c45b676ab..57c486317d28 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -604,6 +604,8 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
 
 	imx_register_uart_clocks(4);
 
+	imx_clk_protect(dev, hws);
+
 	return 0;
 
 unregister_hws:
diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 652ae58c2735..a95862cc04a4 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -713,6 +713,8 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 
 	imx_register_uart_clocks(4);
 
+	imx_clk_protect(dev, hws);
+
 	return 0;
 }
 
diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index 882dcad4817d..2868e2390667 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -603,6 +603,8 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
 
 	imx_register_uart_clocks(4);
 
+	imx_clk_protect(dev, hws);
+
 	return 0;
 
 unregister_hws:
diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index 5582f18dd632..307da8bd5243 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -2,6 +2,7 @@
 #include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -214,4 +215,24 @@ static int __init imx_clk_disable_uart(void)
 late_initcall_sync(imx_clk_disable_uart);
 #endif
 
+int imx_clk_protect(struct device *dev, struct clk_hw * const clks[])
+{
+	struct device_node *np = dev->of_node;
+	struct property *prop;
+	const __be32 *p;
+	u32 i;
+	int ret;
+
+	of_property_for_each_u32(np, "fsl,protected-clocks", prop, p, i) {
+		ret = clk_prepare_enable(clks[i]->clk);
+		if (ret) {
+			dev_err(dev, "failed to enable %s\n", clk_hw_get_name(clks[i]));
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(imx_clk_protect);
+
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index dd49f90110e8..3f8099190b99 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -22,6 +22,8 @@ void imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned int chn);
 void imx_unregister_clocks(struct clk *clks[], unsigned int count);
 void imx_unregister_hw_clocks(struct clk_hw *hws[], unsigned int count);
 
+int imx_clk_protect(struct device *dev, struct clk_hw * const clks[]);
+
 extern void imx_cscmr1_fixup(u32 *val);
 
 enum imx_pllv1_type {
-- 
2.37.1

Re: [PATCH 2/2] clk: imx: support fsl,protected-clocks

[Abel Vesa]

On 22-08-15 11:36:32, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> For the clocks listed in fsl,protected-clocks, enable them to avoid
> Linux disable them. This will benifit root Linux and inmate cell run
> on top of Jailhouse hypervisor, and benifit AMP case.

Nitpick: s/benifit/benefit/

AMP?

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/imx/clk-imx8mm.c |  2 ++
>  drivers/clk/imx/clk-imx8mn.c |  2 ++
>  drivers/clk/imx/clk-imx8mp.c |  2 ++
>  drivers/clk/imx/clk-imx8mq.c |  2 ++
>  drivers/clk/imx/clk.c        | 21 +++++++++++++++++++++
>  drivers/clk/imx/clk.h        |  2 ++
>  6 files changed, 31 insertions(+)
> 
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index b6d275855b36..24ddb1620bce 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -611,6 +611,8 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>  
>  	imx_register_uart_clocks(4);
>  
> +	imx_clk_protect(dev, hws);
> +
>  	return 0;
>  
>  unregister_hws:
> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> index d37c45b676ab..57c486317d28 100644
> --- a/drivers/clk/imx/clk-imx8mn.c
> +++ b/drivers/clk/imx/clk-imx8mn.c
> @@ -604,6 +604,8 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
>  
>  	imx_register_uart_clocks(4);
>  
> +	imx_clk_protect(dev, hws);
> +
>  	return 0;
>  
>  unregister_hws:
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index 652ae58c2735..a95862cc04a4 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -713,6 +713,8 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>  
>  	imx_register_uart_clocks(4);
>  
> +	imx_clk_protect(dev, hws);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index 882dcad4817d..2868e2390667 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -603,6 +603,8 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>  
>  	imx_register_uart_clocks(4);
>  
> +	imx_clk_protect(dev, hws);
> +
>  	return 0;
>  
>  unregister_hws:
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index 5582f18dd632..307da8bd5243 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -2,6 +2,7 @@
>  #include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
> +#include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -214,4 +215,24 @@ static int __init imx_clk_disable_uart(void)
>  late_initcall_sync(imx_clk_disable_uart);
>  #endif
>  
> +int imx_clk_protect(struct device *dev, struct clk_hw * const clks[])
> +{
> +	struct device_node *np = dev->of_node;
> +	struct property *prop;
> +	const __be32 *p;
> +	u32 i;
> +	int ret;
> +
> +	of_property_for_each_u32(np, "fsl,protected-clocks", prop, p, i) {
> +		ret = clk_prepare_enable(clks[i]->clk);

I might be wrong here, but wouldn't CLK_IGNORE_UNUSED have the same effect?

I don't think we should circumvent that by adding vendor specific dts properties.

> +		if (ret) {
> +			dev_err(dev, "failed to enable %s\n", clk_hw_get_name(clks[i]));
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(imx_clk_protect);
> +
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index dd49f90110e8..3f8099190b99 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -22,6 +22,8 @@ void imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned int chn);
>  void imx_unregister_clocks(struct clk *clks[], unsigned int count);
>  void imx_unregister_hw_clocks(struct clk_hw *hws[], unsigned int count);
>  
> +int imx_clk_protect(struct device *dev, struct clk_hw * const clks[]);
> +
>  extern void imx_cscmr1_fixup(u32 *val);
>  
>  enum imx_pllv1_type {
> -- 
> 2.37.1
> 

RE: [PATCH 2/2] clk: imx: support fsl,protected-clocks

[Peng Fan]

> Subject: Re: [PATCH 2/2] clk: imx: support fsl,protected-clocks
> 
> On 22-08-15 11:36:32, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > For the clocks listed in fsl,protected-clocks, enable them to avoid
> > Linux disable them. This will benifit root Linux and inmate cell run
> > on top of Jailhouse hypervisor, and benifit AMP case.
> 
> Nitpick: s/benifit/benefit/
> 
> AMP?

Asymmetric Multi-Processing, I mean Cortex-M run together with A53.

Thanks,
Peng.

> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/clk/imx/clk-imx8mm.c |  2 ++
> >  drivers/clk/imx/clk-imx8mn.c |  2 ++
> >  drivers/clk/imx/clk-imx8mp.c |  2 ++
> >  drivers/clk/imx/clk-imx8mq.c |  2 ++
> >  drivers/clk/imx/clk.c        | 21 +++++++++++++++++++++
> >  drivers/clk/imx/clk.h        |  2 ++
> >  6 files changed, 31 insertions(+)
> >
> > diff --git a/drivers/clk/imx/clk-imx8mm.c
> > b/drivers/clk/imx/clk-imx8mm.c index b6d275855b36..24ddb1620bce
> 100644
> > --- a/drivers/clk/imx/clk-imx8mm.c
> > +++ b/drivers/clk/imx/clk-imx8mm.c
> > @@ -611,6 +611,8 @@ static int imx8mm_clocks_probe(struct
> > platform_device *pdev)
> >
> >  	imx_register_uart_clocks(4);
> >
> > +	imx_clk_protect(dev, hws);
> > +
> >  	return 0;
> >
> >  unregister_hws:
> > diff --git a/drivers/clk/imx/clk-imx8mn.c
> > b/drivers/clk/imx/clk-imx8mn.c index d37c45b676ab..57c486317d28
> 100644
> > --- a/drivers/clk/imx/clk-imx8mn.c
> > +++ b/drivers/clk/imx/clk-imx8mn.c
> > @@ -604,6 +604,8 @@ static int imx8mn_clocks_probe(struct
> > platform_device *pdev)
> >
> >  	imx_register_uart_clocks(4);
> >
> > +	imx_clk_protect(dev, hws);
> > +
> >  	return 0;
> >
> >  unregister_hws:
> > diff --git a/drivers/clk/imx/clk-imx8mp.c
> > b/drivers/clk/imx/clk-imx8mp.c index 652ae58c2735..a95862cc04a4
> 100644
> > --- a/drivers/clk/imx/clk-imx8mp.c
> > +++ b/drivers/clk/imx/clk-imx8mp.c
> > @@ -713,6 +713,8 @@ static int imx8mp_clocks_probe(struct
> > platform_device *pdev)
> >
> >  	imx_register_uart_clocks(4);
> >
> > +	imx_clk_protect(dev, hws);
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/clk/imx/clk-imx8mq.c
> > b/drivers/clk/imx/clk-imx8mq.c index 882dcad4817d..2868e2390667
> 100644
> > --- a/drivers/clk/imx/clk-imx8mq.c
> > +++ b/drivers/clk/imx/clk-imx8mq.c
> > @@ -603,6 +603,8 @@ static int imx8mq_clocks_probe(struct
> > platform_device *pdev)
> >
> >  	imx_register_uart_clocks(4);
> >
> > +	imx_clk_protect(dev, hws);
> > +
> >  	return 0;
> >
> >  unregister_hws:
> > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c index
> > 5582f18dd632..307da8bd5243 100644
> > --- a/drivers/clk/imx/clk.c
> > +++ b/drivers/clk/imx/clk.c
> > @@ -2,6 +2,7 @@
> >  #include <linux/bits.h>
> >  #include <linux/clk.h>
> >  #include <linux/clk-provider.h>
> > +#include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > @@ -214,4 +215,24 @@ static int __init imx_clk_disable_uart(void)
> > late_initcall_sync(imx_clk_disable_uart);
> >  #endif
> >
> > +int imx_clk_protect(struct device *dev, struct clk_hw * const clks[])
> > +{
> > +	struct device_node *np = dev->of_node;
> > +	struct property *prop;
> > +	const __be32 *p;
> > +	u32 i;
> > +	int ret;
> > +
> > +	of_property_for_each_u32(np, "fsl,protected-clocks", prop, p, i) {
> > +		ret = clk_prepare_enable(clks[i]->clk);
> 
> I might be wrong here, but wouldn't CLK_IGNORE_UNUSED have the same
> effect?
> 
> I don't think we should circumvent that by adding vendor specific dts
> properties.
> 
> > +		if (ret) {
> > +			dev_err(dev, "failed to enable %s\n",
> clk_hw_get_name(clks[i]));
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(imx_clk_protect);
> > +
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index
> > dd49f90110e8..3f8099190b99 100644
> > --- a/drivers/clk/imx/clk.h
> > +++ b/drivers/clk/imx/clk.h
> > @@ -22,6 +22,8 @@ void imx_mmdc_mask_handshake(void __iomem
> *ccm_base,
> > unsigned int chn);  void imx_unregister_clocks(struct clk *clks[],
> > unsigned int count);  void imx_unregister_hw_clocks(struct clk_hw
> > *hws[], unsigned int count);
> >
> > +int imx_clk_protect(struct device *dev, struct clk_hw * const
> > +clks[]);
> > +
> >  extern void imx_cscmr1_fixup(u32 *val);
> >
> >  enum imx_pllv1_type {
> > --
> > 2.37.1
> >

Re: [PATCH 1/2] dt-bindings: clock: imx8m: introduce fsl,protected-clocks property

[Sascha Hauer]

Hi Peng,

On Mon, Aug 15, 2022 at 11:36:31AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX8M Linux run on top of Jailhouse hypervisor, the root cell Linux
> should not disable clocks used by inmate. This would also benifit
> AMP to avoid Linux disable clocks used by Cortex-M4/M7.
> 
> So introduce fsl,protected-clocks for above case.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  Documentation/devicetree/bindings/clock/imx8m-clock.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> index 458c7645ee68..0ec490ff9a09 100644
> --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> @@ -39,6 +39,10 @@ properties:
>        ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h
>        for the full list of i.MX8M clock IDs.
>  
> +  fsl,protected-clocks:
> +    description: List of the Protected clock.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array

There already is a generic protected-clocks property described in
https://github.com/devicetree-org/dt-schema/blob/0d1b78cd0c3d9a3d523ced17d7da64b03f6c18ea/dtschema/schemas/clock/clock.yaml#L131
We probably shouldn't add a property with the same name but different
meaning.

I am not sure if we want to go the route of a fsl specific property, it
looks like other SoCs could have similar problems and it might be worth
solving this problem with a broader view.

Anyway, please add a description to the binding what this property
actually does.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH 1/2] dt-bindings: clock: imx8m: introduce fsl,protected-clocks property

[Peng Fan]

Hi Sascha,

> Subject: Re: [PATCH 1/2] dt-bindings: clock: imx8m: introduce fsl,protected-
> clocks property
> 
> Hi Peng,
> 
> On Mon, Aug 15, 2022 at 11:36:31AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > i.MX8M Linux run on top of Jailhouse hypervisor, the root cell Linux
> > should not disable clocks used by inmate. This would also benifit AMP
> > to avoid Linux disable clocks used by Cortex-M4/M7.
> >
> > So introduce fsl,protected-clocks for above case.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/clock/imx8m-clock.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> > b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> > index 458c7645ee68..0ec490ff9a09 100644
> > --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> > +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> > @@ -39,6 +39,10 @@ properties:
> >        ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-
> clock.h
> >        for the full list of i.MX8M clock IDs.
> >
> > +  fsl,protected-clocks:
> > +    description: List of the Protected clock.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> 
> There already is a generic protected-clocks property described in
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> b.com%2Fdevicetree-org%2Fdt-
> schema%2Fblob%2F0d1b78cd0c3d9a3d523ced17d7da64b03f6c18ea%2Fdtsc
> hema%2Fschemas%2Fclock%2Fclock.yaml%23L131&amp;data=05%7C01%7
> Cpeng.fan%40nxp.com%7C5dbc72639c9147765af208da7ec63315%7C686ea
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637961686968811809%7CUn
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=D%2BfJA5h
> wblaX8VH%2BdQoN0pEFmCipfZHHf0ZVo07B4kg%3D&amp;reserved=0
> We probably shouldn't add a property with the same name but different
> meaning.
> 

Thanks for sharing the info. I should check the common bindings before
cook this patchset.

> I am not sure if we want to go the route of a fsl specific property, it looks
> like other SoCs could have similar problems and it might be worth solving
> this problem with a broader view.
> 

I see qcom just drop the clock entries before registering the clocks. But to
i.MX8M, it is not feasible to drop those clocks, unless check the
protected-clocks property before registering every clock. This is odd.

So here I just wanna let i.MX8M clk driver prepare enable the clocks listed
in protected-clocks property to avoid linux disable those clocks.
> Anyway, please add a description to the binding what this property actually
> does.
I will switch to use the common bindings.

Thanks,
Peng.
> 
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7C5dbc
> 72639c9147765af208da7ec63315%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C637961686968811809%7CUnknown%7CTWFpbGZsb3d8eyJW
> IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> C3000%7C%7C%7C&amp;sdata=7UqyWwDDhaze2qZJ%2FknZ6Am7Y%2Bs%
> 2Fbpyv9SITJHgsZPw%3D&amp;reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] dt-bindings: clock: imx8m: introduce fsl,protected-clocks property

[Sascha Hauer]

On Tue, Aug 16, 2022 at 07:13:27AM +0000, Peng Fan wrote:
> Hi Sascha,
> 
> > Subject: Re: [PATCH 1/2] dt-bindings: clock: imx8m: introduce fsl,protected-
> > clocks property
> > 
> > Hi Peng,
> > 
> > On Mon, Aug 15, 2022 at 11:36:31AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > i.MX8M Linux run on top of Jailhouse hypervisor, the root cell Linux
> > > should not disable clocks used by inmate. This would also benifit AMP
> > > to avoid Linux disable clocks used by Cortex-M4/M7.
> > >
> > > So introduce fsl,protected-clocks for above case.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  Documentation/devicetree/bindings/clock/imx8m-clock.yaml | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> > > b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> > > index 458c7645ee68..0ec490ff9a09 100644
> > > --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> > > +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> > > @@ -39,6 +39,10 @@ properties:
> > >        ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-
> > clock.h
> > >        for the full list of i.MX8M clock IDs.
> > >
> > > +  fsl,protected-clocks:
> > > +    description: List of the Protected clock.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > 
> > There already is a generic protected-clocks property described in
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> > b.com%2Fdevicetree-org%2Fdt-
> > schema%2Fblob%2F0d1b78cd0c3d9a3d523ced17d7da64b03f6c18ea%2Fdtsc
> > hema%2Fschemas%2Fclock%2Fclock.yaml%23L131&amp;data=05%7C01%7
> > Cpeng.fan%40nxp.com%7C5dbc72639c9147765af208da7ec63315%7C686ea
> > 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637961686968811809%7CUn
> > known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> > 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=D%2BfJA5h
> > wblaX8VH%2BdQoN0pEFmCipfZHHf0ZVo07B4kg%3D&amp;reserved=0
> > We probably shouldn't add a property with the same name but different
> > meaning.
> > 
> 
> Thanks for sharing the info. I should check the common bindings before
> cook this patchset.
> 
> > I am not sure if we want to go the route of a fsl specific property, it looks
> > like other SoCs could have similar problems and it might be worth solving
> > this problem with a broader view.
> > 
> 
> I see qcom just drop the clock entries before registering the clocks. But to
> i.MX8M, it is not feasible to drop those clocks, unless check the
> protected-clocks property before registering every clock. This is odd.
> 
> So here I just wanna let i.MX8M clk driver prepare enable the clocks listed
> in protected-clocks property to avoid linux disable those clocks.
> > Anyway, please add a description to the binding what this property actually
> > does.
> I will switch to use the common bindings.

There were cases when a property first started with a "soc," prefix and
later when people realized that it could be useful for other
drivers/SoCs as well, the prefix was removed. With that in mind I
would expect that a "fsl,protected-clocks" property behaves the same
as a "protected-clocks" property without the prefix.

If it doesn't please pick a different name. I didn't want to suggest
to just drop the "fsl," prefix and to use the generic property name
when the properties have a different meaning.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |