[PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Aisheng Dong]

There's a few limitations on one cell clock binding (#clock-cells = <1>)
that we have to define all clock IDs for device tree to reference.
This may cause troubles if we want to use common clock IDs for multi
platforms support when the clock of those platforms are mostly the same.
e.g. Current clock IDs name are defined with SS prefix. However the device
may reside in different SS across CPUs, that means the SS prefix may
not valid anymore for a new SoC. Furthermore, the device availability of
those clocks may also vary a bit.

For such situation, We formerly planned to add all new IDs for each SS
and dynamically check availability for different SoC in driver. That can
be done but that may involve a lot effort and may result in more changes
and duplicated code in driver, also make device tree upstreaming hard which
depends on Clock IDs.

To relief this situation, we want to move the clock definition into
device tree which can fully decouple the dependency of Clock ID definition
from device tree. And no frequent changes required in clock driver any more.

Then we can use the existence of clock nodes in device tree to address the
device and clock availability differences across different SoCs.

For SCU clocks, only two params required, thus two new property created:
rsrc-id = <IMX_SC_R_UART_0>;
clk-type = <IMX_SC_PM_CLK_PER>;

And as we want to support clock set parent function, 'clocks' property
is also used to pass all the possible input parents.

Cc: Rob Herring <robh@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 29 ++++++++++++++++------
 include/dt-bindings/firmware/imx/rsrc.h            | 17 +++++++++++++
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 72d481c..2816789 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -78,6 +78,19 @@ Required properties:
 			  "fsl,imx8qm-clock"
 			  "fsl,imx8qxp-clock"
 			followed by "fsl,scu-clk"
+- #clock-cells:		Should be 0.
+- rsrc-id:		Resource ID associated with this clock
+- clk-type:		Type of this clock.
+			Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
+			available clock types supported by SCU.
+- clock-output-names:	Shall be the corresponding names of the outputs.
+
+Optional properties:
+- clocks:		Shall be the input parent clock(s) phandle for the clock.
+			For multiplexed clocks, the list order must match the hardware
+			programming order.
+
+Legacy Clock binding (DEPRECATED):
 - #clock-cells:		Should be 1. Contains the Clock ID value.
 - clocks:		List of clock specifiers, must contain an entry for
 			each required entry in clock-names
@@ -129,6 +142,13 @@ lsio_mu1: mailbox@5d1c0000 {
 	#mbox-cells = <2>;
 };
 
+uart0_clk: uart0-clock-controller {
+	compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
+	#clock-cells = <0>;
+	rsrc-id = <IMX_SC_R_UART_0>;
+	clk-type = <IMX_SC_PM_CLK_PER>;
+};
+
 firmware {
 	scu {
 		compatible = "fsl,imx-scu";
@@ -143,11 +163,6 @@ firmware {
 			  &lsio_mu1 1 2
 			  &lsio_mu1 1 3>;
 
-		clk: clk {
-			compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
-			#clock-cells = <1>;
-		};
-
 		iomuxc {
 			compatible = "fsl,imx8qxp-iomuxc";
 
@@ -175,8 +190,6 @@ serial@5a060000 {
 	...
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_lpuart0>;
-	clocks = <&clk IMX8QXP_UART0_CLK>,
-		 <&clk IMX8QXP_UART0_IPG_CLK>;
-	clock-names = "per", "ipg";
+	clocks = <&uart0_clk>;
 	power-domains = <&pd IMX_SC_R_UART_0>;
 };
diff --git a/include/dt-bindings/firmware/imx/rsrc.h b/include/dt-bindings/firmware/imx/rsrc.h
index 4481f2d..f650fc3 100644
--- a/include/dt-bindings/firmware/imx/rsrc.h
+++ b/include/dt-bindings/firmware/imx/rsrc.h
@@ -556,4 +556,21 @@
 #define IMX_SC_R_VPU			540
 #define IMX_SC_R_LAST			541
 
+/*
+ * Defines for SC PM CLK
+ */
+#define IMX_SC_PM_CLK_SLV_BUS		0	/* Slave bus clock */
+#define IMX_SC_PM_CLK_MST_BUS		1	/* Master bus clock */
+#define IMX_SC_PM_CLK_PER		2	/* Peripheral clock */
+#define IMX_SC_PM_CLK_PHY		3	/* Phy clock */
+#define IMX_SC_PM_CLK_MISC		4	/* Misc clock */
+#define IMX_SC_PM_CLK_MISC0		0	/* Misc 0 clock */
+#define IMX_SC_PM_CLK_MISC1		1	/* Misc 1 clock */
+#define IMX_SC_PM_CLK_MISC2		2	/* Misc 2 clock */
+#define IMX_SC_PM_CLK_MISC3		3	/* Misc 3 clock */
+#define IMX_SC_PM_CLK_MISC4		4	/* Misc 4 clock */
+#define IMX_SC_PM_CLK_CPU		2	/* CPU clock */
+#define IMX_SC_PM_CLK_PLL		4	/* PLL */
+#define IMX_SC_PM_CLK_BYPASS		4	/* Bypass clock */
+
 #endif /* __DT_BINDINGS_RSCRC_IMX_H */
-- 
2.7.4

Re: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Rob Herring]

On Thu, Feb 21, 2019 at 06:03:43PM +0000, Aisheng Dong wrote:
> There's a few limitations on one cell clock binding (#clock-cells = <1>)
> that we have to define all clock IDs for device tree to reference.
> This may cause troubles if we want to use common clock IDs for multi
> platforms support when the clock of those platforms are mostly the same.
> e.g. Current clock IDs name are defined with SS prefix. However the device
> may reside in different SS across CPUs, that means the SS prefix may
> not valid anymore for a new SoC. Furthermore, the device availability of
> those clocks may also vary a bit.
> 
> For such situation, We formerly planned to add all new IDs for each SS
> and dynamically check availability for different SoC in driver. That can
> be done but that may involve a lot effort and may result in more changes
> and duplicated code in driver, also make device tree upstreaming hard which
> depends on Clock IDs.
> 
> To relief this situation, we want to move the clock definition into
> device tree which can fully decouple the dependency of Clock ID definition
> from device tree. And no frequent changes required in clock driver any more.
> 
> Then we can use the existence of clock nodes in device tree to address the
> device and clock availability differences across different SoCs.
> 
> For SCU clocks, only two params required, thus two new property created:
> rsrc-id = <IMX_SC_R_UART_0>;
> clk-type = <IMX_SC_PM_CLK_PER>;
> 
> And as we want to support clock set parent function, 'clocks' property
> is also used to pass all the possible input parents.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 29 ++++++++++++++++------
>  include/dt-bindings/firmware/imx/rsrc.h            | 17 +++++++++++++
>  2 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> index 72d481c..2816789 100644
> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -78,6 +78,19 @@ Required properties:
>  			  "fsl,imx8qm-clock"
>  			  "fsl,imx8qxp-clock"
>  			followed by "fsl,scu-clk"
> +- #clock-cells:		Should be 0.
> +- rsrc-id:		Resource ID associated with this clock
> +- clk-type:		Type of this clock.
> +			Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> +			available clock types supported by SCU.

Can't you just make these 2 values clock cells? I'm all for getting rid 
of made up clock numbers.

> +- clock-output-names:	Shall be the corresponding names of the outputs.
> +
> +Optional properties:
> +- clocks:		Shall be the input parent clock(s) phandle for the clock.
> +			For multiplexed clocks, the list order must match the hardware
> +			programming order.
> +
> +Legacy Clock binding (DEPRECATED):
>  - #clock-cells:		Should be 1. Contains the Clock ID value.
>  - clocks:		List of clock specifiers, must contain an entry for
>  			each required entry in clock-names
> @@ -129,6 +142,13 @@ lsio_mu1: mailbox@5d1c0000 {
>  	#mbox-cells = <2>;
>  };
>  
> +uart0_clk: uart0-clock-controller {
> +	compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> +	#clock-cells = <0>;
> +	rsrc-id = <IMX_SC_R_UART_0>;
> +	clk-type = <IMX_SC_PM_CLK_PER>;
> +};
> +
>  firmware {
>  	scu {
>  		compatible = "fsl,imx-scu";
> @@ -143,11 +163,6 @@ firmware {
>  			  &lsio_mu1 1 2
>  			  &lsio_mu1 1 3>;
>  
> -		clk: clk {
> -			compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
> -			#clock-cells = <1>;
> -		};
> -
>  		iomuxc {
>  			compatible = "fsl,imx8qxp-iomuxc";
>  
> @@ -175,8 +190,6 @@ serial@5a060000 {
>  	...
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_lpuart0>;
> -	clocks = <&clk IMX8QXP_UART0_CLK>,
> -		 <&clk IMX8QXP_UART0_IPG_CLK>;
> -	clock-names = "per", "ipg";
> +	clocks = <&uart0_clk>;
>  	power-domains = <&pd IMX_SC_R_UART_0>;
>  };
> diff --git a/include/dt-bindings/firmware/imx/rsrc.h b/include/dt-bindings/firmware/imx/rsrc.h
> index 4481f2d..f650fc3 100644
> --- a/include/dt-bindings/firmware/imx/rsrc.h
> +++ b/include/dt-bindings/firmware/imx/rsrc.h
> @@ -556,4 +556,21 @@
>  #define IMX_SC_R_VPU			540
>  #define IMX_SC_R_LAST			541
>  
> +/*
> + * Defines for SC PM CLK
> + */
> +#define IMX_SC_PM_CLK_SLV_BUS		0	/* Slave bus clock */
> +#define IMX_SC_PM_CLK_MST_BUS		1	/* Master bus clock */
> +#define IMX_SC_PM_CLK_PER		2	/* Peripheral clock */
> +#define IMX_SC_PM_CLK_PHY		3	/* Phy clock */
> +#define IMX_SC_PM_CLK_MISC		4	/* Misc clock */
> +#define IMX_SC_PM_CLK_MISC0		0	/* Misc 0 clock */
> +#define IMX_SC_PM_CLK_MISC1		1	/* Misc 1 clock */
> +#define IMX_SC_PM_CLK_MISC2		2	/* Misc 2 clock */
> +#define IMX_SC_PM_CLK_MISC3		3	/* Misc 3 clock */
> +#define IMX_SC_PM_CLK_MISC4		4	/* Misc 4 clock */
> +#define IMX_SC_PM_CLK_CPU		2	/* CPU clock */
> +#define IMX_SC_PM_CLK_PLL		4	/* PLL */
> +#define IMX_SC_PM_CLK_BYPASS		4	/* Bypass clock */
> +
>  #endif /* __DT_BINDINGS_RSCRC_IMX_H */
> -- 
> 2.7.4
> 

RE: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Aisheng Dong]

> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Tuesday, March 26, 2019 9:47 PM
> On Thu, Feb 21, 2019 at 06:03:43PM +0000, Aisheng Dong wrote:
> > There's a few limitations on one cell clock binding (#clock-cells =
> > <1>) that we have to define all clock IDs for device tree to reference.
> > This may cause troubles if we want to use common clock IDs for multi
> > platforms support when the clock of those platforms are mostly the same.
> > e.g. Current clock IDs name are defined with SS prefix. However the
> > device may reside in different SS across CPUs, that means the SS
> > prefix may not valid anymore for a new SoC. Furthermore, the device
> > availability of those clocks may also vary a bit.
> >
> > For such situation, We formerly planned to add all new IDs for each SS
> > and dynamically check availability for different SoC in driver. That
> > can be done but that may involve a lot effort and may result in more
> > changes and duplicated code in driver, also make device tree
> > upstreaming hard which depends on Clock IDs.
> >
> > To relief this situation, we want to move the clock definition into
> > device tree which can fully decouple the dependency of Clock ID
> > definition from device tree. And no frequent changes required in clock driver
> any more.
> >
> > Then we can use the existence of clock nodes in device tree to address
> > the device and clock availability differences across different SoCs.
> >
> > For SCU clocks, only two params required, thus two new property created:
> > rsrc-id = <IMX_SC_R_UART_0>;
> > clk-type = <IMX_SC_PM_CLK_PER>;
> >
> > And as we want to support clock set parent function, 'clocks' property
> > is also used to pass all the possible input parents.
> >
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 29
> ++++++++++++++++------
> >  include/dt-bindings/firmware/imx/rsrc.h            | 17
> +++++++++++++
> >  2 files changed, 38 insertions(+), 8 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > index 72d481c..2816789 100644
> > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -78,6 +78,19 @@ Required properties:
> >  			  "fsl,imx8qm-clock"
> >  			  "fsl,imx8qxp-clock"
> >  			followed by "fsl,scu-clk"
> > +- #clock-cells:		Should be 0.
> > +- rsrc-id:		Resource ID associated with this clock
> > +- clk-type:		Type of this clock.
> > +			Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> > +			available clock types supported by SCU.
> 
> Can't you just make these 2 values clock cells? I'm all for getting rid of made
> up clock numbers.
> 

Thanks for the agreement to remove clock IDs.

The 2 values clock cell seems not the best approach for i.MX because it still needs
to define all clocks in the driver which is exactly we want to avoid now due to some
special HW characteristic:
1. clock resources may be allocated to different SW execution partition by firmware
and A core may not have access rights for those clocks not belong to its partition.
So we want to describe them in DT according to the partition configuration.

2. Each clock is associated with a different power domain which is better to be
described in device tree. And clock state will be lost and need restore after power cycle
of the domain.

Based on above requirements, do you think we can do as below?

//LSIO SS
lsio_scu_clk: lsio-scu-clock-controller {
        compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";

        fspi0_clk: clock-fspi0{
                #clock-cells = <0>;
                rsrc-id = <IMX_SC_R_FSPI_0>;
                clk-type = <IMX_SC_PM_CLK_PER>;
                power-domains = <&pd IMX_SC_R_FSPI_0>;
        };
		
		fspi1_clk: clock-fspi1{
			...
		};
        ...
};    

/* LPCG clocks */
lsio_lpcg_clk: lsio-lpcg-clock-controller {
        compatible = "fsl,imx8qxp-lpcg";

        pwm0_lpcg: clock-controller@5d400000 {
                reg = <0x5d400000 0x10000>;
                #clock-cells = <1>;
                clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
                         <&lsio_bus_clk>, <&pwm0_clk>;
                bit-offset = <0 4 16 20 24>;
                clock-output-names = "pwm0_lpcg_ipg_clk",
                                     "pwm0_lpcg_ipg_hf_clk",
                                     "pwm0_lpcg_ipg_s_clk",
                                     "pwm0_lpcg_ipg_slv_clk",
                                     "pwm0_lpcg_ipg_mstr_clk";
                power-domains = <&pd IMX_SC_R_PWM_0>;
                status = "disabled";
        };

		pwm1_lpcg: clock-controller@5d410000 {
				...
		}
        ...
};

And for users, it could simply be:
usdhc1: mmc@5b010000 {
        clocks = <&sdhc0_lpcg 1>,
                 <&sdhc0_lpcg 0>,
                 <&sdhc0_lpcg 2>;
        clock-names = "ipg", "per", "ahb";
        assigned-clocks = <&sdhc0_clk>;
        assigned-clock-rates = <200000000>;
		....
};

Regards
Dong Aisheng

> > +- clock-output-names:	Shall be the corresponding names of the outputs.
> > +
> > +Optional properties:
> > +- clocks:		Shall be the input parent clock(s) phandle for the clock.
> > +			For multiplexed clocks, the list order must match the hardware
> > +			programming order.
> > +
> > +Legacy Clock binding (DEPRECATED):
> >  - #clock-cells:		Should be 1. Contains the Clock ID value.
> >  - clocks:		List of clock specifiers, must contain an entry for
> >  			each required entry in clock-names @@ -129,6 +142,13 @@
> lsio_mu1:
> > mailbox@5d1c0000 {
> >  	#mbox-cells = <2>;
> >  };
> >
> > +uart0_clk: uart0-clock-controller {
> > +	compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> > +	#clock-cells = <0>;
> > +	rsrc-id = <IMX_SC_R_UART_0>;
> > +	clk-type = <IMX_SC_PM_CLK_PER>;
> > +};
> > +
> >  firmware {
> >  	scu {
> >  		compatible = "fsl,imx-scu";
> > @@ -143,11 +163,6 @@ firmware {
> >  			  &lsio_mu1 1 2
> >  			  &lsio_mu1 1 3>;
> >
> > -		clk: clk {
> > -			compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
> > -			#clock-cells = <1>;
> > -		};
> > -
> >  		iomuxc {
> >  			compatible = "fsl,imx8qxp-iomuxc";
> >
> > @@ -175,8 +190,6 @@ serial@5a060000 {
> >  	...
> >  	pinctrl-names = "default";
> >  	pinctrl-0 = <&pinctrl_lpuart0>;
> > -	clocks = <&clk IMX8QXP_UART0_CLK>,
> > -		 <&clk IMX8QXP_UART0_IPG_CLK>;
> > -	clock-names = "per", "ipg";
> > +	clocks = <&uart0_clk>;
> >  	power-domains = <&pd IMX_SC_R_UART_0>;  }; diff --git
> > a/include/dt-bindings/firmware/imx/rsrc.h
> > b/include/dt-bindings/firmware/imx/rsrc.h
> > index 4481f2d..f650fc3 100644
> > --- a/include/dt-bindings/firmware/imx/rsrc.h
> > +++ b/include/dt-bindings/firmware/imx/rsrc.h
> > @@ -556,4 +556,21 @@
> >  #define IMX_SC_R_VPU			540
> >  #define IMX_SC_R_LAST			541
> >
> > +/*
> > + * Defines for SC PM CLK
> > + */
> > +#define IMX_SC_PM_CLK_SLV_BUS		0	/* Slave bus clock */
> > +#define IMX_SC_PM_CLK_MST_BUS		1	/* Master bus clock */
> > +#define IMX_SC_PM_CLK_PER		2	/* Peripheral clock */
> > +#define IMX_SC_PM_CLK_PHY		3	/* Phy clock */
> > +#define IMX_SC_PM_CLK_MISC		4	/* Misc clock */
> > +#define IMX_SC_PM_CLK_MISC0		0	/* Misc 0 clock */
> > +#define IMX_SC_PM_CLK_MISC1		1	/* Misc 1 clock */
> > +#define IMX_SC_PM_CLK_MISC2		2	/* Misc 2 clock */
> > +#define IMX_SC_PM_CLK_MISC3		3	/* Misc 3 clock */
> > +#define IMX_SC_PM_CLK_MISC4		4	/* Misc 4 clock */
> > +#define IMX_SC_PM_CLK_CPU		2	/* CPU clock */
> > +#define IMX_SC_PM_CLK_PLL		4	/* PLL */
> > +#define IMX_SC_PM_CLK_BYPASS		4	/* Bypass clock */
> > +
> >  #endif /* __DT_BINDINGS_RSCRC_IMX_H */
> > --
> > 2.7.4
> >

RE: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Aisheng Dong]

Hi Rob,

> From: Aisheng Dong
> Sent: Wednesday, March 27, 2019 10:35 PM
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Tuesday, March 26, 2019 9:47 PM
> > On Thu, Feb 21, 2019 at 06:03:43PM +0000, Aisheng Dong wrote:
> > > There's a few limitations on one cell clock binding (#clock-cells =
> > > <1>) that we have to define all clock IDs for device tree to reference.
> > > This may cause troubles if we want to use common clock IDs for multi
> > > platforms support when the clock of those platforms are mostly the same.
> > > e.g. Current clock IDs name are defined with SS prefix. However the
> > > device may reside in different SS across CPUs, that means the SS
> > > prefix may not valid anymore for a new SoC. Furthermore, the device
> > > availability of those clocks may also vary a bit.
> > >
> > > For such situation, We formerly planned to add all new IDs for each
> > > SS and dynamically check availability for different SoC in driver.
> > > That can be done but that may involve a lot effort and may result in
> > > more changes and duplicated code in driver, also make device tree
> > > upstreaming hard which depends on Clock IDs.
> > >
> > > To relief this situation, we want to move the clock definition into
> > > device tree which can fully decouple the dependency of Clock ID
> > > definition from device tree. And no frequent changes required in
> > > clock driver
> > any more.
> > >
> > > Then we can use the existence of clock nodes in device tree to
> > > address the device and clock availability differences across different SoCs.
> > >
> > > For SCU clocks, only two params required, thus two new property created:
> > > rsrc-id = <IMX_SC_R_UART_0>;
> > > clk-type = <IMX_SC_PM_CLK_PER>;
> > >
> > > And as we want to support clock set parent function, 'clocks'
> > > property is also used to pass all the possible input parents.
> > >
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 29
> > ++++++++++++++++------
> > >  include/dt-bindings/firmware/imx/rsrc.h            | 17
> > +++++++++++++
> > >  2 files changed, 38 insertions(+), 8 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 72d481c..2816789 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -78,6 +78,19 @@ Required properties:
> > >  			  "fsl,imx8qm-clock"
> > >  			  "fsl,imx8qxp-clock"
> > >  			followed by "fsl,scu-clk"
> > > +- #clock-cells:		Should be 0.
> > > +- rsrc-id:		Resource ID associated with this clock
> > > +- clk-type:		Type of this clock.
> > > +			Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> > > +			available clock types supported by SCU.
> >
> > Can't you just make these 2 values clock cells? I'm all for getting
> > rid of made up clock numbers.
> >
> 
> Thanks for the agreement to remove clock IDs.
> 
> The 2 values clock cell seems not the best approach for i.MX because it still
> needs to define all clocks in the driver which is exactly we want to avoid now
> due to some special HW characteristic:
> 1. clock resources may be allocated to different SW execution partition by
> firmware and A core may not have access rights for those clocks not belong to
> its partition.
> So we want to describe them in DT according to the partition configuration.
> 
> 2. Each clock is associated with a different power domain which is better to be
> described in device tree. And clock state will be lost and need restore after
> power cycle of the domain.
> 
> Based on above requirements, do you think we can do as below?
> 

Would you help check if this is okay to you?

Regards
Dong Aisheng

> //LSIO SS
> lsio_scu_clk: lsio-scu-clock-controller {
>         compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
> 
>         fspi0_clk: clock-fspi0{
>                 #clock-cells = <0>;
>                 rsrc-id = <IMX_SC_R_FSPI_0>;
>                 clk-type = <IMX_SC_PM_CLK_PER>;
>                 power-domains = <&pd IMX_SC_R_FSPI_0>;
>         };
> 
> 		fspi1_clk: clock-fspi1{
> 			...
> 		};
>         ...
> };
> 
> /* LPCG clocks */
> lsio_lpcg_clk: lsio-lpcg-clock-controller {
>         compatible = "fsl,imx8qxp-lpcg";
> 
>         pwm0_lpcg: clock-controller@5d400000 {
>                 reg = <0x5d400000 0x10000>;
>                 #clock-cells = <1>;
>                 clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
>                          <&lsio_bus_clk>, <&pwm0_clk>;
>                 bit-offset = <0 4 16 20 24>;
>                 clock-output-names = "pwm0_lpcg_ipg_clk",
>                                      "pwm0_lpcg_ipg_hf_clk",
>                                      "pwm0_lpcg_ipg_s_clk",
>                                      "pwm0_lpcg_ipg_slv_clk",
>                                      "pwm0_lpcg_ipg_mstr_clk";
>                 power-domains = <&pd IMX_SC_R_PWM_0>;
>                 status = "disabled";
>         };
> 
> 		pwm1_lpcg: clock-controller@5d410000 {
> 				...
> 		}
>         ...
> };
> 
> And for users, it could simply be:
> usdhc1: mmc@5b010000 {
>         clocks = <&sdhc0_lpcg 1>,
>                  <&sdhc0_lpcg 0>,
>                  <&sdhc0_lpcg 2>;
>         clock-names = "ipg", "per", "ahb";
>         assigned-clocks = <&sdhc0_clk>;
>         assigned-clock-rates = <200000000>;
> 		....
> };
> 
> Regards
> Dong Aisheng
> 
> > > +- clock-output-names:	Shall be the corresponding names of the outputs.
> > > +
> > > +Optional properties:
> > > +- clocks:		Shall be the input parent clock(s) phandle for the clock.
> > > +			For multiplexed clocks, the list order must match the hardware
> > > +			programming order.
> > > +
> > > +Legacy Clock binding (DEPRECATED):
> > >  - #clock-cells:		Should be 1. Contains the Clock ID value.
> > >  - clocks:		List of clock specifiers, must contain an entry for
> > >  			each required entry in clock-names @@ -129,6 +142,13
> @@
> > lsio_mu1:
> > > mailbox@5d1c0000 {
> > >  	#mbox-cells = <2>;
> > >  };
> > >
> > > +uart0_clk: uart0-clock-controller {
> > > +	compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> > > +	#clock-cells = <0>;
> > > +	rsrc-id = <IMX_SC_R_UART_0>;
> > > +	clk-type = <IMX_SC_PM_CLK_PER>;
> > > +};
> > > +
> > >  firmware {
> > >  	scu {
> > >  		compatible = "fsl,imx-scu";
> > > @@ -143,11 +163,6 @@ firmware {
> > >  			  &lsio_mu1 1 2
> > >  			  &lsio_mu1 1 3>;
> > >
> > > -		clk: clk {
> > > -			compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
> > > -			#clock-cells = <1>;
> > > -		};
> > > -
> > >  		iomuxc {
> > >  			compatible = "fsl,imx8qxp-iomuxc";
> > >
> > > @@ -175,8 +190,6 @@ serial@5a060000 {
> > >  	...
> > >  	pinctrl-names = "default";
> > >  	pinctrl-0 = <&pinctrl_lpuart0>;
> > > -	clocks = <&clk IMX8QXP_UART0_CLK>,
> > > -		 <&clk IMX8QXP_UART0_IPG_CLK>;
> > > -	clock-names = "per", "ipg";
> > > +	clocks = <&uart0_clk>;
> > >  	power-domains = <&pd IMX_SC_R_UART_0>;  }; diff --git
> > > a/include/dt-bindings/firmware/imx/rsrc.h
> > > b/include/dt-bindings/firmware/imx/rsrc.h
> > > index 4481f2d..f650fc3 100644
> > > --- a/include/dt-bindings/firmware/imx/rsrc.h
> > > +++ b/include/dt-bindings/firmware/imx/rsrc.h
> > > @@ -556,4 +556,21 @@
> > >  #define IMX_SC_R_VPU			540
> > >  #define IMX_SC_R_LAST			541
> > >
> > > +/*
> > > + * Defines for SC PM CLK
> > > + */
> > > +#define IMX_SC_PM_CLK_SLV_BUS		0	/* Slave bus clock */
> > > +#define IMX_SC_PM_CLK_MST_BUS		1	/* Master bus clock */
> > > +#define IMX_SC_PM_CLK_PER		2	/* Peripheral clock */
> > > +#define IMX_SC_PM_CLK_PHY		3	/* Phy clock */
> > > +#define IMX_SC_PM_CLK_MISC		4	/* Misc clock */
> > > +#define IMX_SC_PM_CLK_MISC0		0	/* Misc 0 clock */
> > > +#define IMX_SC_PM_CLK_MISC1		1	/* Misc 1 clock */
> > > +#define IMX_SC_PM_CLK_MISC2		2	/* Misc 2 clock */
> > > +#define IMX_SC_PM_CLK_MISC3		3	/* Misc 3 clock */
> > > +#define IMX_SC_PM_CLK_MISC4		4	/* Misc 4 clock */
> > > +#define IMX_SC_PM_CLK_CPU		2	/* CPU clock */
> > > +#define IMX_SC_PM_CLK_PLL		4	/* PLL */
> > > +#define IMX_SC_PM_CLK_BYPASS		4	/* Bypass clock */
> > > +
> > >  #endif /* __DT_BINDINGS_RSCRC_IMX_H */
> > > --
> > > 2.7.4
> > >

RE: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Aisheng Dong]

Hi Rob,

Could you maybe shed a little light on this?

We're blocking here for the further work.

Regards
Dong Aisheng

> From: Aisheng Dong
> Sent: Tuesday, April 2, 2019 10:47 PM
> 
> Hi Rob,
> 
> > From: Aisheng Dong
> > Sent: Wednesday, March 27, 2019 10:35 PM
> > > From: Rob Herring [mailto:robh@kernel.org]
> > > Sent: Tuesday, March 26, 2019 9:47 PM On Thu, Feb 21, 2019 at
> > > 06:03:43PM +0000, Aisheng Dong wrote:
> > > > There's a few limitations on one cell clock binding (#clock-cells
> > > > =
> > > > <1>) that we have to define all clock IDs for device tree to reference.
> > > > This may cause troubles if we want to use common clock IDs for
> > > > multi platforms support when the clock of those platforms are mostly the
> same.
> > > > e.g. Current clock IDs name are defined with SS prefix. However
> > > > the device may reside in different SS across CPUs, that means the
> > > > SS prefix may not valid anymore for a new SoC. Furthermore, the
> > > > device availability of those clocks may also vary a bit.
> > > >
> > > > For such situation, We formerly planned to add all new IDs for
> > > > each SS and dynamically check availability for different SoC in driver.
> > > > That can be done but that may involve a lot effort and may result
> > > > in more changes and duplicated code in driver, also make device
> > > > tree upstreaming hard which depends on Clock IDs.
> > > >
> > > > To relief this situation, we want to move the clock definition
> > > > into device tree which can fully decouple the dependency of Clock
> > > > ID definition from device tree. And no frequent changes required
> > > > in clock driver
> > > any more.
> > > >
> > > > Then we can use the existence of clock nodes in device tree to
> > > > address the device and clock availability differences across different SoCs.
> > > >
> > > > For SCU clocks, only two params required, thus two new property
> created:
> > > > rsrc-id = <IMX_SC_R_UART_0>;
> > > > clk-type = <IMX_SC_PM_CLK_PER>;
> > > >
> > > > And as we want to support clock set parent function, 'clocks'
> > > > property is also used to pass all the possible input parents.
> > > >
> > > > Cc: Rob Herring <robh@kernel.org>
> > > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > ---
> > > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 29
> > > ++++++++++++++++------
> > > >  include/dt-bindings/firmware/imx/rsrc.h            | 17
> > > +++++++++++++
> > > >  2 files changed, 38 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > index 72d481c..2816789 100644
> > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > @@ -78,6 +78,19 @@ Required properties:
> > > >  			  "fsl,imx8qm-clock"
> > > >  			  "fsl,imx8qxp-clock"
> > > >  			followed by "fsl,scu-clk"
> > > > +- #clock-cells:		Should be 0.
> > > > +- rsrc-id:		Resource ID associated with this clock
> > > > +- clk-type:		Type of this clock.
> > > > +			Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> > > > +			available clock types supported by SCU.
> > >
> > > Can't you just make these 2 values clock cells? I'm all for getting
> > > rid of made up clock numbers.
> > >
> >
> > Thanks for the agreement to remove clock IDs.
> >
> > The 2 values clock cell seems not the best approach for i.MX because
> > it still needs to define all clocks in the driver which is exactly we
> > want to avoid now due to some special HW characteristic:
> > 1. clock resources may be allocated to different SW execution
> > partition by firmware and A core may not have access rights for those
> > clocks not belong to its partition.
> > So we want to describe them in DT according to the partition configuration.
> >
> > 2. Each clock is associated with a different power domain which is
> > better to be described in device tree. And clock state will be lost
> > and need restore after power cycle of the domain.
> >
> > Based on above requirements, do you think we can do as below?
> >
> 
> Would you help check if this is okay to you?
> 
> Regards
> Dong Aisheng
> 
> > //LSIO SS
> > lsio_scu_clk: lsio-scu-clock-controller {
> >         compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
> >
> >         fspi0_clk: clock-fspi0{
> >                 #clock-cells = <0>;
> >                 rsrc-id = <IMX_SC_R_FSPI_0>;
> >                 clk-type = <IMX_SC_PM_CLK_PER>;
> >                 power-domains = <&pd IMX_SC_R_FSPI_0>;
> >         };
> >
> > 		fspi1_clk: clock-fspi1{
> > 			...
> > 		};
> >         ...
> > };
> >
> > /* LPCG clocks */
> > lsio_lpcg_clk: lsio-lpcg-clock-controller {
> >         compatible = "fsl,imx8qxp-lpcg";
> >
> >         pwm0_lpcg: clock-controller@5d400000 {
> >                 reg = <0x5d400000 0x10000>;
> >                 #clock-cells = <1>;
> >                 clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
> >                          <&lsio_bus_clk>, <&pwm0_clk>;
> >                 bit-offset = <0 4 16 20 24>;
> >                 clock-output-names = "pwm0_lpcg_ipg_clk",
> >                                      "pwm0_lpcg_ipg_hf_clk",
> >                                      "pwm0_lpcg_ipg_s_clk",
> >                                      "pwm0_lpcg_ipg_slv_clk",
> >                                      "pwm0_lpcg_ipg_mstr_clk";
> >                 power-domains = <&pd IMX_SC_R_PWM_0>;
> >                 status = "disabled";
> >         };
> >
> > 		pwm1_lpcg: clock-controller@5d410000 {
> > 				...
> > 		}
> >         ...
> > };
> >
> > And for users, it could simply be:
> > usdhc1: mmc@5b010000 {
> >         clocks = <&sdhc0_lpcg 1>,
> >                  <&sdhc0_lpcg 0>,
> >                  <&sdhc0_lpcg 2>;
> >         clock-names = "ipg", "per", "ahb";
> >         assigned-clocks = <&sdhc0_clk>;
> >         assigned-clock-rates = <200000000>;
> > 		....
> > };
> >
> > Regards
> > Dong Aisheng
> >
> > > > +- clock-output-names:	Shall be the corresponding names of the outputs.
> > > > +
> > > > +Optional properties:
> > > > +- clocks:		Shall be the input parent clock(s) phandle for the clock.
> > > > +			For multiplexed clocks, the list order must match the
> hardware
> > > > +			programming order.
> > > > +
> > > > +Legacy Clock binding (DEPRECATED):
> > > >  - #clock-cells:		Should be 1. Contains the Clock ID value.
> > > >  - clocks:		List of clock specifiers, must contain an entry for
> > > >  			each required entry in clock-names @@ -129,6 +142,13
> > @@
> > > lsio_mu1:
> > > > mailbox@5d1c0000 {
> > > >  	#mbox-cells = <2>;
> > > >  };
> > > >
> > > > +uart0_clk: uart0-clock-controller {
> > > > +	compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> > > > +	#clock-cells = <0>;
> > > > +	rsrc-id = <IMX_SC_R_UART_0>;
> > > > +	clk-type = <IMX_SC_PM_CLK_PER>;
> > > > +};
> > > > +
> > > >  firmware {
> > > >  	scu {
> > > >  		compatible = "fsl,imx-scu";
> > > > @@ -143,11 +163,6 @@ firmware {
> > > >  			  &lsio_mu1 1 2
> > > >  			  &lsio_mu1 1 3>;
> > > >
> > > > -		clk: clk {
> > > > -			compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
> > > > -			#clock-cells = <1>;
> > > > -		};
> > > > -
> > > >  		iomuxc {
> > > >  			compatible = "fsl,imx8qxp-iomuxc";
> > > >
> > > > @@ -175,8 +190,6 @@ serial@5a060000 {
> > > >  	...
> > > >  	pinctrl-names = "default";
> > > >  	pinctrl-0 = <&pinctrl_lpuart0>;
> > > > -	clocks = <&clk IMX8QXP_UART0_CLK>,
> > > > -		 <&clk IMX8QXP_UART0_IPG_CLK>;
> > > > -	clock-names = "per", "ipg";
> > > > +	clocks = <&uart0_clk>;
> > > >  	power-domains = <&pd IMX_SC_R_UART_0>;  }; diff --git
> > > > a/include/dt-bindings/firmware/imx/rsrc.h
> > > > b/include/dt-bindings/firmware/imx/rsrc.h
> > > > index 4481f2d..f650fc3 100644
> > > > --- a/include/dt-bindings/firmware/imx/rsrc.h
> > > > +++ b/include/dt-bindings/firmware/imx/rsrc.h
> > > > @@ -556,4 +556,21 @@
> > > >  #define IMX_SC_R_VPU			540
> > > >  #define IMX_SC_R_LAST			541
> > > >
> > > > +/*
> > > > + * Defines for SC PM CLK
> > > > + */
> > > > +#define IMX_SC_PM_CLK_SLV_BUS		0	/* Slave bus clock */
> > > > +#define IMX_SC_PM_CLK_MST_BUS		1	/* Master bus clock */
> > > > +#define IMX_SC_PM_CLK_PER		2	/* Peripheral clock */
> > > > +#define IMX_SC_PM_CLK_PHY		3	/* Phy clock */
> > > > +#define IMX_SC_PM_CLK_MISC		4	/* Misc clock */
> > > > +#define IMX_SC_PM_CLK_MISC0		0	/* Misc 0 clock */
> > > > +#define IMX_SC_PM_CLK_MISC1		1	/* Misc 1 clock */
> > > > +#define IMX_SC_PM_CLK_MISC2		2	/* Misc 2 clock */
> > > > +#define IMX_SC_PM_CLK_MISC3		3	/* Misc 3 clock */
> > > > +#define IMX_SC_PM_CLK_MISC4		4	/* Misc 4 clock */
> > > > +#define IMX_SC_PM_CLK_CPU		2	/* CPU clock */
> > > > +#define IMX_SC_PM_CLK_PLL		4	/* PLL */
> > > > +#define IMX_SC_PM_CLK_BYPASS		4	/* Bypass clock */
> > > > +
> > > >  #endif /* __DT_BINDINGS_RSCRC_IMX_H */
> > > > --
> > > > 2.7.4
> > > >

Re: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Rob Herring]

On Wed, Mar 27, 2019 at 9:35 AM Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Tuesday, March 26, 2019 9:47 PM
> > On Thu, Feb 21, 2019 at 06:03:43PM +0000, Aisheng Dong wrote:
> > > There's a few limitations on one cell clock binding (#clock-cells =
> > > <1>) that we have to define all clock IDs for device tree to reference.
> > > This may cause troubles if we want to use common clock IDs for multi
> > > platforms support when the clock of those platforms are mostly the same.
> > > e.g. Current clock IDs name are defined with SS prefix. However the
> > > device may reside in different SS across CPUs, that means the SS
> > > prefix may not valid anymore for a new SoC. Furthermore, the device
> > > availability of those clocks may also vary a bit.
> > >
> > > For such situation, We formerly planned to add all new IDs for each SS
> > > and dynamically check availability for different SoC in driver. That
> > > can be done but that may involve a lot effort and may result in more
> > > changes and duplicated code in driver, also make device tree
> > > upstreaming hard which depends on Clock IDs.
> > >
> > > To relief this situation, we want to move the clock definition into
> > > device tree which can fully decouple the dependency of Clock ID
> > > definition from device tree. And no frequent changes required in clock driver
> > any more.
> > >
> > > Then we can use the existence of clock nodes in device tree to address
> > > the device and clock availability differences across different SoCs.
> > >
> > > For SCU clocks, only two params required, thus two new property created:
> > > rsrc-id = <IMX_SC_R_UART_0>;
> > > clk-type = <IMX_SC_PM_CLK_PER>;
> > >
> > > And as we want to support clock set parent function, 'clocks' property
> > > is also used to pass all the possible input parents.
> > >
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 29
> > ++++++++++++++++------
> > >  include/dt-bindings/firmware/imx/rsrc.h            | 17
> > +++++++++++++
> > >  2 files changed, 38 insertions(+), 8 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 72d481c..2816789 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -78,6 +78,19 @@ Required properties:
> > >                       "fsl,imx8qm-clock"
> > >                       "fsl,imx8qxp-clock"
> > >                     followed by "fsl,scu-clk"
> > > +- #clock-cells:            Should be 0.
> > > +- rsrc-id:         Resource ID associated with this clock
> > > +- clk-type:                Type of this clock.
> > > +                   Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> > > +                   available clock types supported by SCU.
> >
> > Can't you just make these 2 values clock cells? I'm all for getting rid of made
> > up clock numbers.
> >
>
> Thanks for the agreement to remove clock IDs.
>
> The 2 values clock cell seems not the best approach for i.MX because it still needs
> to define all clocks in the driver which is exactly we want to avoid now due to some
> special HW characteristic:

Why's that? You can walk the DT and extract the 2 cells for each clock
present. That's not any different than walking child nodes here and
getting the resource ids and type. That's not really fast, but if
speed is really an issue we can consider addressing that in ways that
extend rather than change the binding.

> 1. clock resources may be allocated to different SW execution partition by firmware
> and A core may not have access rights for those clocks not belong to its partition.
> So we want to describe them in DT according to the partition configuration.

Do you have an example? I'd assume you assign peripherals to different
partitions and resource assignment simply follows that. Can clocks not
be available when a peripheral still is?

> 2. Each clock is associated with a different power domain which is better to be
> described in device tree. And clock state will be lost and need restore after power cycle
> of the domain.
>
> Based on above requirements, do you think we can do as below?

Can you provide an example that shows the whole hierarchy for a
peripheral. Here you have FSPI, PWM, and MMC. Reviewing SCU clocks and
LPCG clocks separately is not helpful.

>
> //LSIO SS
> lsio_scu_clk: lsio-scu-clock-controller {
>         compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
>
>         fspi0_clk: clock-fspi0{
>                 #clock-cells = <0>;
>                 rsrc-id = <IMX_SC_R_FSPI_0>;
>                 clk-type = <IMX_SC_PM_CLK_PER>;
>                 power-domains = <&pd IMX_SC_R_FSPI_0>;

Are the power domain ID and rsrc-id always the same for a clock?

>         };
>
>                 fspi1_clk: clock-fspi1{
>                         ...
>                 };
>         ...
> };
>
> /* LPCG clocks */
> lsio_lpcg_clk: lsio-lpcg-clock-controller {
>         compatible = "fsl,imx8qxp-lpcg";

I think this wrapper node should be removed and the compatible moved
into the child nodes.

>         pwm0_lpcg: clock-controller@5d400000 {
>                 reg = <0x5d400000 0x10000>;
>                 #clock-cells = <1>;
>                 clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
>                          <&lsio_bus_clk>, <&pwm0_clk>;
>                 bit-offset = <0 4 16 20 24>;

Are all LPCG instances the same, but some clocks are missing if the
child peripheral doesn't use certain clocks? IOW, bit 0 is always
ipg_clk, bit 24 is always ipg_mstr_clk?

Assuming so, 'bit-offset' should be removed and you should either have
a fixed number of clock entries with 0 entries for non-connected
clocks or use clock-names to define which clocks are present (with the
same set of defined names for all LPCG instances).

>                 clock-output-names = "pwm0_lpcg_ipg_clk",
>                                      "pwm0_lpcg_ipg_hf_clk",
>                                      "pwm0_lpcg_ipg_s_clk",
>                                      "pwm0_lpcg_ipg_slv_clk",
>                                      "pwm0_lpcg_ipg_mstr_clk";

IMO, this is wrong as the names should be relative to the module. So
'ipg_clk', 'ipg_hf_clk', etc.

>                 power-domains = <&pd IMX_SC_R_PWM_0>;
>                 status = "disabled";
>         };
>
>                 pwm1_lpcg: clock-controller@5d410000 {
>                                 ...
>                 }
>         ...
> };
>
> And for users, it could simply be:
> usdhc1: mmc@5b010000 {
>         clocks = <&sdhc0_lpcg 1>,
>                  <&sdhc0_lpcg 0>,
>                  <&sdhc0_lpcg 2>;
>         clock-names = "ipg", "per", "ahb";
>         assigned-clocks = <&sdhc0_clk>;
>         assigned-clock-rates = <200000000>;
>                 ....
> };

RE: [EXT] Re: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Aisheng Dong]

> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Wednesday, April 10, 2019 11:32 PM
> On Wed, Mar 27, 2019 at 9:35 AM Aisheng Dong <aisheng.dong@nxp.com>
> wrote:
> >
> > > From: Rob Herring [mailto:robh@kernel.org]
> > > Sent: Tuesday, March 26, 2019 9:47 PM On Thu, Feb 21, 2019 at
> > > 06:03:43PM +0000, Aisheng Dong wrote:
> > > > There's a few limitations on one cell clock binding (#clock-cells
> > > > =
> > > > <1>) that we have to define all clock IDs for device tree to reference.
> > > > This may cause troubles if we want to use common clock IDs for
> > > > multi platforms support when the clock of those platforms are mostly the
> same.
> > > > e.g. Current clock IDs name are defined with SS prefix. However
> > > > the device may reside in different SS across CPUs, that means the
> > > > SS prefix may not valid anymore for a new SoC. Furthermore, the
> > > > device availability of those clocks may also vary a bit.
> > > >
> > > > For such situation, We formerly planned to add all new IDs for
> > > > each SS and dynamically check availability for different SoC in
> > > > driver. That can be done but that may involve a lot effort and may
> > > > result in more changes and duplicated code in driver, also make
> > > > device tree upstreaming hard which depends on Clock IDs.
> > > >
> > > > To relief this situation, we want to move the clock definition
> > > > into device tree which can fully decouple the dependency of Clock
> > > > ID definition from device tree. And no frequent changes required
> > > > in clock driver
> > > any more.
> > > >
> > > > Then we can use the existence of clock nodes in device tree to
> > > > address the device and clock availability differences across different SoCs.
> > > >
> > > > For SCU clocks, only two params required, thus two new property
> created:
> > > > rsrc-id = <IMX_SC_R_UART_0>;
> > > > clk-type = <IMX_SC_PM_CLK_PER>;
> > > >
> > > > And as we want to support clock set parent function, 'clocks'
> > > > property is also used to pass all the possible input parents.
> > > >
> > > > Cc: Rob Herring <robh@kernel.org>
> > > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > ---
> > > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 29
> > > ++++++++++++++++------
> > > >  include/dt-bindings/firmware/imx/rsrc.h            | 17
> > > +++++++++++++
> > > >  2 files changed, 38 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > index 72d481c..2816789 100644
> > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > @@ -78,6 +78,19 @@ Required properties:
> > > >                       "fsl,imx8qm-clock"
> > > >                       "fsl,imx8qxp-clock"
> > > >                     followed by "fsl,scu-clk"
> > > > +- #clock-cells:            Should be 0.
> > > > +- rsrc-id:         Resource ID associated with this clock
> > > > +- clk-type:                Type of this clock.
> > > > +                   Refer to <include/dt-bindings/firmware/imx/rsrc.h>
> for
> > > > +                   available clock types supported by SCU.
> > >
> > > Can't you just make these 2 values clock cells? I'm all for getting
> > > rid of made up clock numbers.
> > >
> >
> > Thanks for the agreement to remove clock IDs.
> >
> > The 2 values clock cell seems not the best approach for i.MX because
> > it still needs to define all clocks in the driver which is exactly we
> > want to avoid now due to some special HW characteristic:
> 
> Why's that? You can walk the DT and extract the 2 cells for each clock present.
> That's not any different than walking child nodes here and getting the resource
> ids and type. That's not really fast, but if speed is really an issue we can
> consider addressing that in ways that extend rather than change the binding.
> 

Yes, it will be much slower.
And there may be duplicated parsing for the same SCU clocks in device nodes.
e.g. LPCG inputs for each outputs may be the same SCU clock and possible other devices.
It's a bit kind of lower efficiency.

Furthermore, for unused clocks by device, it then can't be visible by driver,
also can't use clock framework auto disable unused function.
And it creates a lot complexities in driver implementation to dynamically parse
and register clocks to the same controller.

So IMHO we'd still like the current simple 0 cells binding.

Anyway, if you think 2 cells bindings should be the right direction we have to go,
Just please let me know.
Then we will go that way rather than stop at here.

> > 1. clock resources may be allocated to different SW execution
> > partition by firmware and A core may not have access rights for those clocks
> not belong to its partition.
> > So we want to describe them in DT according to the partition configuration.
> 
> Do you have an example? I'd assume you assign peripherals to different
> partitions and resource assignment simply follows that. Can clocks not be
> available when a peripheral still is?
> 

Yes, resources goes with peripherals assignment.
For the real example on MX8QXP MEK board, we assign flexcan to M4 partition,
then all flexcan related clocks and power domains can't be access anymore
by A core partition.

> > 2. Each clock is associated with a different power domain which is
> > better to be described in device tree. And clock state will be lost
> > and need restore after power cycle of the domain.
> >
> > Based on above requirements, do you think we can do as below?
> 
> Can you provide an example that shows the whole hierarchy for a peripheral.
> Here you have FSPI, PWM, and MMC. Reviewing SCU clocks and LPCG clocks
> separately is not helpful.
> 

Below is an example of sdhc0 in connectivity SS which can show
the full hierarchy.

/* CONN SS */
conn_subsys: bus@5b000000 {
        compatible = "simple-bus";
        #address-cells = <1>;
        #size-cells = <1>;
        ranges = <0x5b000000 0x0 0x5b000000 0x1000000>;

        /* SCU clocks */
        conn_scu_clk: conn-scu-clock-controller {
                compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";

                sdhc0_clk: clock-sdhc0 {
                        #clock-cells = <0>;
                        rsrc-id = <IMX_SC_R_SDHC_0>;
                        clk-type = <IMX_SC_PM_CLK_PER>;
                        clock-output-names = "sdhc0_clk";
                        power-domains = <&pd IMX_SC_R_SDHC_0>;
                };

                /* other SCU clocks in this SS */
				...
        }

        /* LPCG clocks */
        conn_lpcg_clk: conn-lpcg-clock-controller {
                compatible = "fsl,imx8qxp-lpcg";
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;

                sdhc0_lpcg: clock-controller@5b200000 {
                        reg = <0x5b200000 0x10000>;
                        #clock-cells = <1>;
                        clocks = <&sdhc0_clk>, <&conn_ipg_clk>, <&conn_axi_clk>;
                        bit-offset = <0 16 20>;
                        clock-output-names = "sdhc0_lpcg_per_clk",
                                             "sdhc0_lpcg_ipg_clk",
                                             "sdhc0_lpcg_ahb_clk";
                        power-domains = <&pd IMX_SC_R_SDHC_0>;
                };

                /* other LPCG clocks in this SS */
                ...
        };

        usdhc1: mmc@5b010000 {
                interrupt-parent = <&gic>;
                interrupts = <GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
                reg = <0x5b010000 0x10000>;
                clocks = <&sdhc0_lpcg 1>,
                         <&sdhc0_lpcg 0>,
                         <&sdhc0_lpcg 2>;
                clock-names = "ipg", "per", "ahb";
                assigned-clocks = <&sdhc0_clk>;
                assigned-clock-rates = <200000000>;
                power-domains = <&pd IMX_SC_R_SDHC_0>;
                status = "disabled";
        };
        ....
};

> >
> > //LSIO SS
> > lsio_scu_clk: lsio-scu-clock-controller {
> >         compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
> >
> >         fspi0_clk: clock-fspi0{
> >                 #clock-cells = <0>;
> >                 rsrc-id = <IMX_SC_R_FSPI_0>;
> >                 clk-type = <IMX_SC_PM_CLK_PER>;
> >                 power-domains = <&pd IMX_SC_R_FSPI_0>;
> 
> Are the power domain ID and rsrc-id always the same for a clock?

So far, yes.
I'm still not aware of any clocks use multi power domains.

> 
> >         };
> >
> >                 fspi1_clk: clock-fspi1{
> >                         ...
> >                 };
> >         ...
> > };
> >
> > /* LPCG clocks */
> > lsio_lpcg_clk: lsio-lpcg-clock-controller {
> >         compatible = "fsl,imx8qxp-lpcg";
> 
> I think this wrapper node should be removed and the compatible moved into
> the child nodes.
> 

Yes, I originally did exactly like that.
One benefit of the wrapper is it can save many same compatible strings of the
Same type of clocks in the Soc SS dtsi file which is used to handle SoC compatible
string difference.
e.g 
https://patchwork.kernel.org/patch/10824563/
imx8qxp-ss-adma.dtsi
&uart0_lpcg {
        compatible = "fsl,imx8qxp-lpcg";
};

&uart1_lpcg {
        compatible = "fsl,imx8qxp-lpcg";
};

&uart2_lpcg {
        compatible = "fsl,imx8qxp-lpcg";
};

&uart3_lpcg {
        compatible = "fsl,imx8qxp-lpcg";
};
...

Just let me know if you think having too many repeat compatible strings
Is not problem, then we can remove it.

> >         pwm0_lpcg: clock-controller@5d400000 {
> >                 reg = <0x5d400000 0x10000>;
> >                 #clock-cells = <1>;
> >                 clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
> >                          <&lsio_bus_clk>, <&pwm0_clk>;
> >                 bit-offset = <0 4 16 20 24>;
> 
> Are all LPCG instances the same, but some clocks are missing if the child
> peripheral doesn't use certain clocks? IOW, bit 0 is always ipg_clk, bit 24 is
> always ipg_mstr_clk?

No, It varies on different LPCG clocks.
You can refer to sdhc&enet for an example at here:
https://patchwork.kernel.org/patch/10824561/

> 
> Assuming so, 'bit-offset' should be removed and you should either have a fixed
> number of clock entries with 0 entries for non-connected clocks or use
> clock-names to define which clocks are present (with the same set of defined
> names for all LPCG instances).
> 
> >                 clock-output-names = "pwm0_lpcg_ipg_clk",
> >                                      "pwm0_lpcg_ipg_hf_clk",
> >                                      "pwm0_lpcg_ipg_s_clk",
> >                                      "pwm0_lpcg_ipg_slv_clk",
> >                                      "pwm0_lpcg_ipg_mstr_clk";
> 
> IMO, this is wrong as the names should be relative to the module. So 'ipg_clk',
> 'ipg_hf_clk', etc.

For one certain LPCG, the output clocks is strictly bound to device.
That is how it's named in the HW reference manual.
For example:
PWM0 pwm0_ipg_clk SC_R_PWM_0/SC_PM_CLK_PER SLSLICE5 LPCG_LSIO_IPS_SYNC_PWM0_REGS
So it looks to me better to name to align with the HW doc.

Please let me know if you still think it's not okay.

Regards
Dong Aisheng

> 
> >                 power-domains = <&pd IMX_SC_R_PWM_0>;
> >                 status = "disabled";
> >         };
> >
> >                 pwm1_lpcg: clock-controller@5d410000 {
> >                                 ...
> >                 }
> >         ...
> > };
> >
> > And for users, it could simply be:
> > usdhc1: mmc@5b010000 {
> >         clocks = <&sdhc0_lpcg 1>,
> >                  <&sdhc0_lpcg 0>,
> >                  <&sdhc0_lpcg 2>;
> >         clock-names = "ipg", "per", "ahb";
> >         assigned-clocks = <&sdhc0_clk>;
> >         assigned-clock-rates = <200000000>;
> >                 ....
> > };

Re: [EXT] Re: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Rob Herring]

On Wed, Apr 10, 2019 at 12:35 PM Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Wednesday, April 10, 2019 11:32 PM
> > On Wed, Mar 27, 2019 at 9:35 AM Aisheng Dong <aisheng.dong@nxp.com>
> > wrote:
> > >
> > > > From: Rob Herring [mailto:robh@kernel.org]
> > > > Sent: Tuesday, March 26, 2019 9:47 PM On Thu, Feb 21, 2019 at
> > > > 06:03:43PM +0000, Aisheng Dong wrote:
> > > > > There's a few limitations on one cell clock binding (#clock-cells
> > > > > =
> > > > > <1>) that we have to define all clock IDs for device tree to reference.
> > > > > This may cause troubles if we want to use common clock IDs for
> > > > > multi platforms support when the clock of those platforms are mostly the
> > same.
> > > > > e.g. Current clock IDs name are defined with SS prefix. However
> > > > > the device may reside in different SS across CPUs, that means the
> > > > > SS prefix may not valid anymore for a new SoC. Furthermore, the
> > > > > device availability of those clocks may also vary a bit.
> > > > >
> > > > > For such situation, We formerly planned to add all new IDs for
> > > > > each SS and dynamically check availability for different SoC in
> > > > > driver. That can be done but that may involve a lot effort and may
> > > > > result in more changes and duplicated code in driver, also make
> > > > > device tree upstreaming hard which depends on Clock IDs.
> > > > >
> > > > > To relief this situation, we want to move the clock definition
> > > > > into device tree which can fully decouple the dependency of Clock
> > > > > ID definition from device tree. And no frequent changes required
> > > > > in clock driver
> > > > any more.
> > > > >
> > > > > Then we can use the existence of clock nodes in device tree to
> > > > > address the device and clock availability differences across different SoCs.
> > > > >
> > > > > For SCU clocks, only two params required, thus two new property
> > created:
> > > > > rsrc-id = <IMX_SC_R_UART_0>;
> > > > > clk-type = <IMX_SC_PM_CLK_PER>;
> > > > >
> > > > > And as we want to support clock set parent function, 'clocks'
> > > > > property is also used to pass all the possible input parents.
> > > > >
> > > > > Cc: Rob Herring <robh@kernel.org>
> > > > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > > > Cc: devicetree@vger.kernel.org
> > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > > ---
> > > > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 29
> > > > ++++++++++++++++------
> > > > >  include/dt-bindings/firmware/imx/rsrc.h            | 17
> > > > +++++++++++++
> > > > >  2 files changed, 38 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > index 72d481c..2816789 100644
> > > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > @@ -78,6 +78,19 @@ Required properties:
> > > > >                       "fsl,imx8qm-clock"
> > > > >                       "fsl,imx8qxp-clock"
> > > > >                     followed by "fsl,scu-clk"
> > > > > +- #clock-cells:            Should be 0.
> > > > > +- rsrc-id:         Resource ID associated with this clock
> > > > > +- clk-type:                Type of this clock.
> > > > > +                   Refer to <include/dt-bindings/firmware/imx/rsrc.h>
> > for
> > > > > +                   available clock types supported by SCU.
> > > >
> > > > Can't you just make these 2 values clock cells? I'm all for getting
> > > > rid of made up clock numbers.
> > > >
> > >
> > > Thanks for the agreement to remove clock IDs.
> > >
> > > The 2 values clock cell seems not the best approach for i.MX because
> > > it still needs to define all clocks in the driver which is exactly we
> > > want to avoid now due to some special HW characteristic:
> >
> > Why's that? You can walk the DT and extract the 2 cells for each clock present.
> > That's not any different than walking child nodes here and getting the resource
> > ids and type. That's not really fast, but if speed is really an issue we can
> > consider addressing that in ways that extend rather than change the binding.
> >
>
> Yes, it will be much slower.
> And there may be duplicated parsing for the same SCU clocks in device nodes.
> e.g. LPCG inputs for each outputs may be the same SCU clock and possible other devices.
> It's a bit kind of lower efficiency.
>
> Furthermore, for unused clocks by device, it then can't be visible by driver,
> also can't use clock framework auto disable unused function.
> And it creates a lot complexities in driver implementation to dynamically parse
> and register clocks to the same controller.
>
> So IMHO we'd still like the current simple 0 cells binding.
>
> Anyway, if you think 2 cells bindings should be the right direction we have to go,
> Just please let me know.
> Then we will go that way rather than stop at here.
>
> > > 1. clock resources may be allocated to different SW execution
> > > partition by firmware and A core may not have access rights for those clocks
> > not belong to its partition.
> > > So we want to describe them in DT according to the partition configuration.
> >
> > Do you have an example? I'd assume you assign peripherals to different
> > partitions and resource assignment simply follows that. Can clocks not be
> > available when a peripheral still is?
> >
>
> Yes, resources goes with peripherals assignment.
> For the real example on MX8QXP MEK board, we assign flexcan to M4 partition,
> then all flexcan related clocks and power domains can't be access anymore
> by A core partition.
>
> > > 2. Each clock is associated with a different power domain which is
> > > better to be described in device tree. And clock state will be lost
> > > and need restore after power cycle of the domain.
> > >
> > > Based on above requirements, do you think we can do as below?
> >
> > Can you provide an example that shows the whole hierarchy for a peripheral.
> > Here you have FSPI, PWM, and MMC. Reviewing SCU clocks and LPCG clocks
> > separately is not helpful.
> >
>
> Below is an example of sdhc0 in connectivity SS which can show
> the full hierarchy.
>
> /* CONN SS */
> conn_subsys: bus@5b000000 {
>         compatible = "simple-bus";
>         #address-cells = <1>;
>         #size-cells = <1>;
>         ranges = <0x5b000000 0x0 0x5b000000 0x1000000>;
>
>         /* SCU clocks */
>         conn_scu_clk: conn-scu-clock-controller {
>                 compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
>
>                 sdhc0_clk: clock-sdhc0 {
>                         #clock-cells = <0>;
>                         rsrc-id = <IMX_SC_R_SDHC_0>;
>                         clk-type = <IMX_SC_PM_CLK_PER>;
>                         clock-output-names = "sdhc0_clk";
>                         power-domains = <&pd IMX_SC_R_SDHC_0>;
>                 };
>
>                 /* other SCU clocks in this SS */
>                                 ...
>         }
>
>         /* LPCG clocks */
>         conn_lpcg_clk: conn-lpcg-clock-controller {
>                 compatible = "fsl,imx8qxp-lpcg";
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 ranges;
>
>                 sdhc0_lpcg: clock-controller@5b200000 {
>                         reg = <0x5b200000 0x10000>;
>                         #clock-cells = <1>;
>                         clocks = <&sdhc0_clk>, <&conn_ipg_clk>, <&conn_axi_clk>;
>                         bit-offset = <0 16 20>;
>                         clock-output-names = "sdhc0_lpcg_per_clk",
>                                              "sdhc0_lpcg_ipg_clk",
>                                              "sdhc0_lpcg_ahb_clk";
>                         power-domains = <&pd IMX_SC_R_SDHC_0>;
>                 };
>
>                 /* other LPCG clocks in this SS */
>                 ...
>         };
>
>         usdhc1: mmc@5b010000 {
>                 interrupt-parent = <&gic>;
>                 interrupts = <GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
>                 reg = <0x5b010000 0x10000>;
>                 clocks = <&sdhc0_lpcg 1>,
>                          <&sdhc0_lpcg 0>,
>                          <&sdhc0_lpcg 2>;
>                 clock-names = "ipg", "per", "ahb";
>                 assigned-clocks = <&sdhc0_clk>;
>                 assigned-clock-rates = <200000000>;
>                 power-domains = <&pd IMX_SC_R_SDHC_0>;
>                 status = "disabled";
>         };
>         ....
> };
>
> > >
> > > //LSIO SS
> > > lsio_scu_clk: lsio-scu-clock-controller {
> > >         compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
> > >
> > >         fspi0_clk: clock-fspi0{
> > >                 #clock-cells = <0>;
> > >                 rsrc-id = <IMX_SC_R_FSPI_0>;
> > >                 clk-type = <IMX_SC_PM_CLK_PER>;
> > >                 power-domains = <&pd IMX_SC_R_FSPI_0>;
> >
> > Are the power domain ID and rsrc-id always the same for a clock?
>
> So far, yes.
> I'm still not aware of any clocks use multi power domains.
>
> >
> > >         };
> > >
> > >                 fspi1_clk: clock-fspi1{
> > >                         ...
> > >                 };
> > >         ...
> > > };
> > >
> > > /* LPCG clocks */
> > > lsio_lpcg_clk: lsio-lpcg-clock-controller {
> > >         compatible = "fsl,imx8qxp-lpcg";
> >
> > I think this wrapper node should be removed and the compatible moved into
> > the child nodes.
> >
>
> Yes, I originally did exactly like that.
> One benefit of the wrapper is it can save many same compatible strings of the
> Same type of clocks in the Soc SS dtsi file which is used to handle SoC compatible
> string difference.
> e.g
> https://patchwork.kernel.org/patch/10824563/
> imx8qxp-ss-adma.dtsi
> &uart0_lpcg {
>         compatible = "fsl,imx8qxp-lpcg";
> };
>
> &uart1_lpcg {
>         compatible = "fsl,imx8qxp-lpcg";
> };
>
> &uart2_lpcg {
>         compatible = "fsl,imx8qxp-lpcg";
> };
>
> &uart3_lpcg {
>         compatible = "fsl,imx8qxp-lpcg";
> };
> ...
>
> Just let me know if you think having too many repeat compatible strings
> Is not problem, then we can remove it.
>
> > >         pwm0_lpcg: clock-controller@5d400000 {
> > >                 reg = <0x5d400000 0x10000>;
> > >                 #clock-cells = <1>;
> > >                 clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
> > >                          <&lsio_bus_clk>, <&pwm0_clk>;
> > >                 bit-offset = <0 4 16 20 24>;
> >
> > Are all LPCG instances the same, but some clocks are missing if the child
> > peripheral doesn't use certain clocks? IOW, bit 0 is always ipg_clk, bit 24 is
> > always ipg_mstr_clk?
>
> No, It varies on different LPCG clocks.
> You can refer to sdhc&enet for an example at here:
> https://patchwork.kernel.org/patch/10824561/
>
> >
> > Assuming so, 'bit-offset' should be removed and you should either have a fixed
> > number of clock entries with 0 entries for non-connected clocks or use
> > clock-names to define which clocks are present (with the same set of defined
> > names for all LPCG instances).
> >
> > >                 clock-output-names = "pwm0_lpcg_ipg_clk",
> > >                                      "pwm0_lpcg_ipg_hf_clk",
> > >                                      "pwm0_lpcg_ipg_s_clk",
> > >                                      "pwm0_lpcg_ipg_slv_clk",
> > >                                      "pwm0_lpcg_ipg_mstr_clk";
> >
> > IMO, this is wrong as the names should be relative to the module. So 'ipg_clk',
> > 'ipg_hf_clk', etc.
>
> For one certain LPCG, the output clocks is strictly bound to device.
> That is how it's named in the HW reference manual.
> For example:
> PWM0 pwm0_ipg_clk SC_R_PWM_0/SC_PM_CLK_PER SLSLICE5 LPCG_LSIO_IPS_SYNC_PWM0_REGS
> So it looks to me better to name to align with the HW doc.
>
> Please let me know if you still think it's not okay.
>
> Regards
> Dong Aisheng
>
> >
> > >                 power-domains = <&pd IMX_SC_R_PWM_0>;
> > >                 status = "disabled";
> > >         };
> > >
> > >                 pwm1_lpcg: clock-controller@5d410000 {
> > >                                 ...
> > >                 }
> > >         ...
> > > };
> > >
> > > And for users, it could simply be:
> > > usdhc1: mmc@5b010000 {
> > >         clocks = <&sdhc0_lpcg 1>,
> > >                  <&sdhc0_lpcg 0>,
> > >                  <&sdhc0_lpcg 2>;
> > >         clock-names = "ipg", "per", "ahb";
> > >         assigned-clocks = <&sdhc0_clk>;
> > >         assigned-clock-rates = <200000000>;
> > >                 ....
> > > };

[PATCH 2/4] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree

[Aisheng Dong]

MX8QM and MX8QXP LPCG Clocks are mostly the same except they may reside
in different subsystems across CPUs and also vary a bit on the availability.

Same as SCU clock, we want to move the clock definition into device tree
which can fully decouple the dependency of Clock ID definition from device
tree. And no frequent changes required in clock driver any more to handle
the difference.

We can use the existence of clock nodes in device tree to address the
device and clock availability differences across different SoCs.

Cc: Rob Herring <robh@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
index 965cfa4..a317844 100644
--- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
+++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
@@ -11,6 +11,20 @@ enabled by these control bits, it might still not be running based
 on the base resource.
 
 Required properties:
+- compatible:		Should be one of:
+			  "fsl,imx8qxp-lpcg"
+			  "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg".
+- reg:			Address and length of the register set.
+- #clock-cells:		Should be 1. One LPCG supports multiple clocks.
+- clocks:		Input parent clocks phandle array for each clock.
+- bit-offset:		An integer array indicating the bit offset for each clock.
+- hw-autogate:		Boolean array indicating whether supports HW autogate for
+			each clock.
+- clock-output-names:	Shall be the corresponding names of the outputs.
+			NOTE this property must be specified in the same order
+			as the clock bit-offset and hw-autogate property.
+
+Legacy binding (DEPRECATED):
 - compatible:	Should be one of:
 		  "fsl,imx8qxp-lpcg-adma",
 		  "fsl,imx8qxp-lpcg-conn",
-- 
2.7.4

Re: [PATCH 2/4] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree

[Stephen Boyd]

Quoting Aisheng Dong (2019-02-21 10:03:47)
> MX8QM and MX8QXP LPCG Clocks are mostly the same except they may reside
> in different subsystems across CPUs and also vary a bit on the availability.
> 
> Same as SCU clock, we want to move the clock definition into device tree
> which can fully decouple the dependency of Clock ID definition from device
> tree. And no frequent changes required in clock driver any more to handle
> the difference.
> 
> We can use the existence of clock nodes in device tree to address the
> device and clock availability differences across different SoCs.

This sounds similar to what TI folks are doing with their new firmware.
It leads to problems where we don't know what in the clk tree needs to
be registered, debugfs is not super helpful in that case, and late init
only turns off clks that are found during probe (so nothing then?).

> 
> diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> index 965cfa4..a317844 100644
> --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> @@ -11,6 +11,20 @@ enabled by these control bits, it might still not be running based
>  on the base resource.
>  
>  Required properties:
> +- compatible:          Should be one of:
> +                         "fsl,imx8qxp-lpcg"
> +                         "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg".
> +- reg:                 Address and length of the register set.
> +- #clock-cells:                Should be 1. One LPCG supports multiple clocks.
> +- clocks:              Input parent clocks phandle array for each clock.
> +- bit-offset:          An integer array indicating the bit offset for each clock.
> +- hw-autogate:         Boolean array indicating whether supports HW autogate for
> +                       each clock.

This looks like one clk per node style of bindings which is a direction
we don't want DT bindings to go in. It leads to a bunch of time parsing
DT to generate clks and in general doesn't represent the clock
controller hardware that is there. Basically, anything with 'bit-offset'
in the binding is not going to be acceptable.

> +- clock-output-names:  Shall be the corresponding names of the outputs.
> +                       NOTE this property must be specified in the same order
> +                       as the clock bit-offset and hw-autogate property.
> +
> +Legacy binding (DEPRECATED):
>  - compatible:  Should be one of:
>                   "fsl,imx8qxp-lpcg-adma",

RE: [PATCH 2/4] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree

[Aisheng Dong]

Hi Stephen,

Sorry for the long email, I want to describe things more clear to help the review.

First, i want to share some backgrounds on how this patch series comes from,
it mainly consists of three reasons:

1. New architecture to save a lot duplicated codes
We want to write a more generic <soc>-ss-<subsys>.dtsi shared by imx8qxp, imx8qm,
imx8dx, imx8dm according to HW characteristic then we'd better decouple the dependency
of Clock ID definitions from device tree due to the SS and device availability difference among them.
[00/14] arm64: dts: imx8: architecture improvement and adding imx8qm support
https://patchwork.kernel.org/cover/10824537/

2. Power domain requirements
Both SCU and LPCG clock access requires it's associated power domain enabled, CCF
already supports it and device tree seems to be the best place to describe it.
e.g. arch/arm64/boot/dts/exynos/exynos5433.dtsi
cmu_aud: clock-controller@114c0000 {
        compatible = "samsung,exynos5433-cmu-aud";
        #clock-cells = <1>;
		....
        power-domains = <&pd_aud>;
};

Furthermore, if the power domain is off (e.g. during system suspend) the clock state
Within this domain will be lost and we have to restored it after power domain is re-enabled.
We'd like to use common driver suspend/resume to handle it.
(In the future, we might support Runtime state save&restore as well in order to shut
down the whole SS if not used, that also need power domain info from DT).

3. Partition support
IMX SCU firmware supports Resource Partition service which allows each device resource
to be partitioned into different ownership groupings that are associated with different
execution environments including multiple operating systems executing on different
cores, TrustZone, and hypervisor.

That means we can't register all the clocks in Linux as some of them may not belong
to us and can't access. (we can check all the clocks via SCU RPC call before register, 
but that also needs a branch of time. However, LPCG not easy to check as it does not provide
resource id). Putting clocks of device in DT allows the dynamically configuration of it according
to the real partition requirements.
E.g. Remove some clock/device nodes once not belong to Linux OS partition or not exist in
hardware on this SoC SS.

And please see below for my replies to other of your questions:

> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: Tuesday, February 26, 2019 3:46 AM
> Quoting Aisheng Dong (2019-02-21 10:03:47)
> > MX8QM and MX8QXP LPCG Clocks are mostly the same except they may
> > reside in different subsystems across CPUs and also vary a bit on the
> availability.
> >
> > Same as SCU clock, we want to move the clock definition into device
> > tree which can fully decouple the dependency of Clock ID definition
> > from device tree. And no frequent changes required in clock driver any
> > more to handle the difference.
> >
> > We can use the existence of clock nodes in device tree to address the
> > device and clock availability differences across different SoCs.
> 
> This sounds similar to what TI folks are doing with their new firmware.
> It leads to problems where we don't know what in the clk tree needs to be
> registered, 

AFAICT we know what clocks need to be registered according to the device availability
in each SS (Subsystem) in HW definition.
Am I missed something?

> debugfs is not super helpful in that case, 

Debugfs functions the same as defining clocks in driver.
Every cock defined in driver can be defined in device tree according to HW configuration
and displayed with debugfs. So I'm not quite get the point of the concern.
Can you help clarify a bit more?

> and late init only turns off
> clks that are found during probe (so nothing then?).

Same as above.

BTW, we did not support turn off clocks for LPCG during late init.
Probably won't support in the future as LPCG is the next level gates of SCU clocks.
Gating off LPCG clocks while SCU clocks disabled already looks not so meaningful, right?
And gate off such type LPCG is quite expensive as LPCG needs enable its power domain
to access and chip reset already ensures the LPCG clocks are off and the later LPCG
enable/disable also can sync the HW state.

For the parent SCU clocks, we also still don't have the plan to support late gate off because
SCU clock might be shared with M-Core OS or Secure SW (e.g. ATF, OPTee) and A core can't
Gate off those "unused" ones it believes. Currently what we're doing is ensure gate off
power&clocks after using in bootloader before hand over to kernel.

> 
> >
> > diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > index 965cfa4..a317844 100644
> > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > @@ -11,6 +11,20 @@ enabled by these control bits, it might still not
> > be running based  on the base resource.
> >
> >  Required properties:
> > +- compatible:          Should be one of:
> > +                         "fsl,imx8qxp-lpcg"
> > +                         "fsl,imx8qm-lpcg" followed by
> "fsl,imx8qxp-lpcg".
> > +- reg:                 Address and length of the register set.
> > +- #clock-cells:                Should be 1. One LPCG supports multiple
> clocks.
> > +- clocks:              Input parent clocks phandle array for each clock.
> > +- bit-offset:          An integer array indicating the bit offset for each
> clock.
> > +- hw-autogate:         Boolean array indicating whether supports HW
> autogate for
> > +                       each clock.
> 
> This looks like one clk per node style of bindings which is a direction we don't
> want DT bindings to go in. It leads to a bunch of time parsing DT to generate
> clks and in general doesn't represent the clock controller hardware that is
> there. Basically, anything with 'bit-offset'
> in the binding is not going to be acceptable.
> 

It's one LPCG per node which represents a couple of clock output gates controlled by
this LPCG for one specific module.
For MX8QM/QXP platforms, there're separate LPCGs for each device resource.
LPCGs are independent with each other with separate io space, behaving separate module
clock controllers and they are distributed in different SS (subsystems).
Describing it separately in device tree is more comply with real hardware although
it sacrifices a bit parsing time.

Regards
Dong Aisheng

> > +- clock-output-names:  Shall be the corresponding names of the outputs.
> > +                       NOTE this property must be specified in the
> same order
> > +                       as the clock bit-offset and hw-autogate property.
> > +
> > +Legacy binding (DEPRECATED):
> >  - compatible:  Should be one of:
> >                   "fsl,imx8qxp-lpcg-adma",

RE: [PATCH 2/4] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree

[Aisheng Dong]

[...]

> > diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > index 965cfa4..a317844 100644
> > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > @@ -11,6 +11,20 @@ enabled by these control bits, it might still not
> > be running based  on the base resource.
> >
> >  Required properties:
> > +- compatible:          Should be one of:
> > +                         "fsl,imx8qxp-lpcg"
> > +                         "fsl,imx8qm-lpcg" followed by
> "fsl,imx8qxp-lpcg".
> > +- reg:                 Address and length of the register set.
> > +- #clock-cells:                Should be 1. One LPCG supports multiple
> clocks.
> > +- clocks:              Input parent clocks phandle array for each clock.
> > +- bit-offset:          An integer array indicating the bit offset for each
> clock.
> > +- hw-autogate:         Boolean array indicating whether supports HW
> autogate for
> > +                       each clock.
> 
> This looks like one clk per node style of bindings which is a direction we don't
> want DT bindings to go in. It leads to a bunch of time parsing DT to generate
> clks and in general doesn't represent the clock controller hardware that is
> there. Basically, anything with 'bit-offset'
> in the binding is not going to be acceptable.
> 

This is not one clk per node but one clock controller per node which strictly
describes the HW.

On MX8, each LPCG is a separate clock controller which can control a couple of
clock output gates for one specific device to use. Each device has a corresponding
LPCG clock controller and those LPCGs are independent with each other with
separate IO space.

Those LPCGs are distributed in various SS (subsystems) along with device resources.
Describing them in device tree SS dtsi doesn't seem to be an issue as it is representing
the real hardware.

For SCU clocks, they're similar case that each SS having separate clock controllers
In HW which are managed by SCU firmware. So it seems also okay to put them
in device tree SS dtsi file, right?

If you're concerning each node having one compatible string, how about doing like
many power domain does as below?

Having only one compatible string in clock parent nodes.

//LSIO SS
lsio_scu_clk: lsio-scu-clock-controller {
        compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";

        fspi0_clk: clock-fspi0{
                #clock-cells = <0>;
                rsrc-id = <IMX_SC_R_FSPI_0>;
                clk-type = <IMX_SC_PM_CLK_PER>;
                power-domains = <&pd IMX_SC_R_FSPI_0>;
        };
		
		fspi1_clk: clock-fspi1{
			...
		};
        ...
};    

/* LPCG clocks */
lsio_lpcg_clk: lsio-lpcg-clock-controller {
        compatible = "fsl,imx8qxp-lpcg";

        pwm0_lpcg: clock-controller@5d400000 {
                reg = <0x5d400000 0x10000>;
                #clock-cells = <1>;
                clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
                         <&lsio_bus_clk>, <&pwm0_clk>;
                bit-offset = <0 4 16 20 24>;
                clock-output-names = "pwm0_lpcg_ipg_clk",
                                     "pwm0_lpcg_ipg_hf_clk",
                                     "pwm0_lpcg_ipg_s_clk",
                                     "pwm0_lpcg_ipg_slv_clk",
                                     "pwm0_lpcg_ipg_mstr_clk";
                power-domains = <&pd IMX_SC_R_PWM_0>;
                status = "disabled";
        };
        ...
};

I also have spent a lot time to investigate how TI and Samsung does. However, 
finally i.MX is still different and I still believe current way is better for i.MX,
mainly due to below reasons:

1) IMX having separate clock controllers in HW, not shared one like others
2) IMX SoC is comprised of various HW SS (Subsystem) while others don't have.
  Describing clocks in DT can help a better SW architecture to describe HW.
3) Each clock is associated with a power domain. DT is the best place to indicate it.
4) Clock availability (Both SCU and LPCG) are configurable according to
  different HW partition configuration by SCU firmware.
  Defining them all in driver will cause annoying and continued churn in driver
  all the time when adding new SoC support.
  e.g. 
  Handling availability for different SS in different SoC.
  Defining Clock IDs for diferent SS in different SoC for same clocks.

By putting clocks in DT, we can make the clock driver completely generic and
no more churn in the driver anymore in the future for adding new SoC support.
It can significantly save the driver maintain effort.

Last, this won't break compatibility. It's just introduce a new binding.

Regards
Dong Aisheng

> > +- clock-output-names:  Shall be the corresponding names of the outputs.
> > +                       NOTE this property must be specified in the
> same order
> > +                       as the clock bit-offset and hw-autogate property.
> > +
> > +Legacy binding (DEPRECATED):
> >  - compatible:  Should be one of:
> >                   "fsl,imx8qxp-lpcg-adma",

RE: [PATCH 2/4] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree

[Aisheng Dong]

Hi Stephen,

> > > a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > > b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > > index 965cfa4..a317844 100644
> > > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > > @@ -11,6 +11,20 @@ enabled by these control bits, it might still not
> > > be running based  on the base resource.
> > >
> > >  Required properties:
> > > +- compatible:          Should be one of:
> > > +                         "fsl,imx8qxp-lpcg"
> > > +                         "fsl,imx8qm-lpcg" followed by
> > "fsl,imx8qxp-lpcg".
> > > +- reg:                 Address and length of the register set.
> > > +- #clock-cells:                Should be 1. One LPCG supports multiple
> > clocks.
> > > +- clocks:              Input parent clocks phandle array for each clock.
> > > +- bit-offset:          An integer array indicating the bit offset for each
> > clock.
> > > +- hw-autogate:         Boolean array indicating whether supports HW
> > autogate for
> > > +                       each clock.
> >
> > This looks like one clk per node style of bindings which is a
> > direction we don't want DT bindings to go in. It leads to a bunch of
> > time parsing DT to generate clks and in general doesn't represent the
> > clock controller hardware that is there. Basically, anything with 'bit-offset'
> > in the binding is not going to be acceptable.
> >
> 
> This is not one clk per node but one clock controller per node which strictly
> describes the HW.
> 
> On MX8, each LPCG is a separate clock controller which can control a couple of
> clock output gates for one specific device to use. Each device has a
> corresponding LPCG clock controller and those LPCGs are independent with
> each other with separate IO space.
> 
> Those LPCGs are distributed in various SS (subsystems) along with device
> resources.
> Describing them in device tree SS dtsi doesn't seem to be an issue as it is
> representing the real hardware.
> 
> For SCU clocks, they're similar case that each SS having separate clock
> controllers In HW which are managed by SCU firmware. So it seems also okay
> to put them in device tree SS dtsi file, right?
> 
> If you're concerning each node having one compatible string, how about doing
> like many power domain does as below?
> 
> Having only one compatible string in clock parent nodes.
> 
> //LSIO SS
> lsio_scu_clk: lsio-scu-clock-controller {
>         compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
> 
>         fspi0_clk: clock-fspi0{
>                 #clock-cells = <0>;
>                 rsrc-id = <IMX_SC_R_FSPI_0>;
>                 clk-type = <IMX_SC_PM_CLK_PER>;
>                 power-domains = <&pd IMX_SC_R_FSPI_0>;
>         };
> 
> 		fspi1_clk: clock-fspi1{
> 			...
> 		};
>         ...
> };
> 
> /* LPCG clocks */
> lsio_lpcg_clk: lsio-lpcg-clock-controller {
>         compatible = "fsl,imx8qxp-lpcg";
> 
>         pwm0_lpcg: clock-controller@5d400000 {
>                 reg = <0x5d400000 0x10000>;
>                 #clock-cells = <1>;
>                 clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
>                          <&lsio_bus_clk>, <&pwm0_clk>;
>                 bit-offset = <0 4 16 20 24>;
>                 clock-output-names = "pwm0_lpcg_ipg_clk",
>                                      "pwm0_lpcg_ipg_hf_clk",
>                                      "pwm0_lpcg_ipg_s_clk",
>                                      "pwm0_lpcg_ipg_slv_clk",
>                                      "pwm0_lpcg_ipg_mstr_clk";
>                 power-domains = <&pd IMX_SC_R_PWM_0>;
>                 status = "disabled";
>         };
>         ...
> };
> 
> I also have spent a lot time to investigate how TI and Samsung does. However,
> finally i.MX is still different and I still believe current way is better for i.MX,
> mainly due to below reasons:
> 
> 1) IMX having separate clock controllers in HW, not shared one like others
> 2) IMX SoC is comprised of various HW SS (Subsystem) while others don't have.
>   Describing clocks in DT can help a better SW architecture to describe HW.
> 3) Each clock is associated with a power domain. DT is the best place to
> indicate it.
> 4) Clock availability (Both SCU and LPCG) are configurable according to
>   different HW partition configuration by SCU firmware.
>   Defining them all in driver will cause annoying and continued churn in driver
>   all the time when adding new SoC support.
>   e.g.
>   Handling availability for different SS in different SoC.
>   Defining Clock IDs for diferent SS in different SoC for same clocks.
> 
> By putting clocks in DT, we can make the clock driver completely generic and
> no more churn in the driver anymore in the future for adding new SoC support.
> It can significantly save the driver maintain effort.
> 

Shawn is okay with the whole point of MX8 Arch improvement[1]. 
(e.g. moving clocks into DT)
But we still need the your agreement on the clock part first.
Could you help review if this is okay to you?

[1] https://patchwork.kernel.org/cover/10824537/

Regards
Dong Aisheng

> Last, this won't break compatibility. It's just introduce a new binding.
> 
> Regards
> Dong Aisheng
> 
> > > +- clock-output-names:  Shall be the corresponding names of the outputs.
> > > +                       NOTE this property must be specified in the
> > same order
> > > +                       as the clock bit-offset and hw-autogate
> property.
> > > +
> > > +Legacy binding (DEPRECATED):
> > >  - compatible:  Should be one of:
> > >                   "fsl,imx8qxp-lpcg-adma",

Re: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Aisheng Dong]

Hi Rob,

> > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > index 72d481c..2816789 100644
> > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > @@ -78,6 +78,19 @@ Required properties:
> > > >                       "fsl,imx8qm-clock"
> > > >                       "fsl,imx8qxp-clock"
> > > >                     followed by "fsl,scu-clk"
> > > > +- #clock-cells:            Should be 0.
> > > > +- rsrc-id:         Resource ID associated with this clock
> > > > +- clk-type:                Type of this clock.
> > > > +                   Refer to <include/dt-bindings/firmware/imx/rsrc.h>
> for
> > > > +                   available clock types supported by SCU.
> > >
> > > Can't you just make these 2 values clock cells? I'm all for getting
> > > rid of made up clock numbers.
> > >
> >
> > Thanks for the agreement to remove clock IDs.
> >
> > The 2 values clock cell seems not the best approach for i.MX because
> > it still needs to define all clocks in the driver which is exactly we
> > want to avoid now due to some special HW characteristic:
> 
> Why's that? You can walk the DT and extract the 2 cells for each clock present.
> That's not any different than walking child nodes here and getting the resource
> ids and type. That's not really fast, but if speed is really an issue we can
> consider addressing that in ways that extend rather than change the binding.
> 

Due to searching the 'clocks' property of all device nodes indirectly to 
exact the 2 cell value causes much troubles in driver implementation and
looks a bit weird and is very low efficiency ( the performance will also potentially
be affected by adding new unrelevant nodes which is bad), we found the below
alternative way to do the same 2 cell binding, but having no performance issue.
It's much similar to the ARM SCPI clock binding.
Documentation/devicetree/bindings/arm/arm,scpi.txt

Do you think if it's okay to you?

If we have to use 2 cell binding, we probably would prefer to use this way
as it can relief us a lot from indirectly searching the 'clocks' property. 

If you're ok, please let me know, I will make it in V2 for the review.

enet0_clk: clock-enet0 {
        compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
        #clock-cells = <2>;
        clock-indice = <IMX_SC_PM_CLK_PER>,		// clock cell 2 value
                       <IMX_SC_PM_CLK_BYPASS>,
                       <IMX_SC_PM_CLK_MISC0>;
        clock-output-names = "enet0_clk",
                             "enet0_bypass_clk",
                             "enet0_rgmii_clk";
		// we can use the same resource id for clock cells 1 value
		// or probably encoded in node@reg?
        power-domains = <&pd IMX_SC_R_ENET_0>; 
};

enet0_lpcg: clock-controller@5b230000 {
        compatible = "fsl,imx8qxp-lpcg";
        reg = <0x5b230000 0x10000>;
        #clock-cells = <1>;
        clocks = <&enet0_clk IMX_SC_R_ENET_0 IMX_SC_PM_CLK_PER>,
                 <&enet0_clk IMX_SC_R_ENET_0 IMX_SC_PM_CLK_PER>,
                 <&conn_axi_clk>, <&conn_ipg_clk>, <&conn_ipg_clk>;
        bit-offset = <0 4 8 16 20>;
        clock-output-names = "enet0_ipg_root_clk",
                             "enet0_tx_clk",
                             "enet0_ahb_clk",
                             "enet0_ipg_clk",
                             "enet0_ipg_s_clk";
        power-domains = <&pd IMX_SC_R_ENET_0>;
};

fec1: ethernet@5b040000 {
        reg = <0x5b040000 0x10000>;
        interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&enet0_lpcg 3>,
                 <&enet0_lpcg 2>,
                 <&enet0_lpcg 1>,
                 <&enet0_lpcg 0>;
        clock-names = "ipg", "ahb", "enet_clk_ref", "ptp";
        fsl,num-tx-queues=<3>;
        fsl,num-rx-queues=<3>;
        power-domains = <&pd IMX_SC_R_ENET_0>;
        status = "disabled";
};

Regards
Dong Aisheng