[V2,1/2] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

There's a few limitations on the original 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. This can make us write a full generic clock driver
for SCU based SoCs. No more frequent changes needed in clock driver
any more.

In the meanwhile, we can also 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. The first one is resource id
which is encoded in reg property and the second is clock type index
which is encoded in generic clock-indices property they're not continuously.

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

Cc: Rob Herring <robh+dt@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>
---
ChangeLog:
v1->v2:
 * changed to one cell binding inspired by arm,scpi.txt
   Documentation/devicetree/bindings/arm/arm,scpi.txt
   Resource ID is encoded in 'reg' property.
   Clock type is encoded in generic clock-indices property.
   Then we don't have to search all the DT nodes to fetch
   those two value to construct clocks which is relatively
   low efficiency.
 * Add required power-domain property as well.
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 45 ++++++++++++++++++----
 include/dt-bindings/firmware/imx/rsrc.h            | 17 ++++++++
 2 files changed, 54 insertions(+), 8 deletions(-)

On Thu, May 2, 2019 at 8:34 PM Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> There's a few limitations on the original 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. This can make us write a full generic clock driver
> for SCU based SoCs. No more frequent changes needed in clock driver
> any more.
>
> In the meanwhile, we can also 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. The first one is resource id
> which is encoded in reg property and the second is clock type index
> which is encoded in generic clock-indices property they're not continuously.
>
> And as we also want to support clock set parent function, 'clocks' property
> is also used to pass all the possible input parents.
>
> Cc: Rob Herring <robh+dt@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>
> ---
> ChangeLog:
> v1->v2:
>  * changed to one cell binding inspired by arm,scpi.txt
>    Documentation/devicetree/bindings/arm/arm,scpi.txt
>    Resource ID is encoded in 'reg' property.
>    Clock type is encoded in generic clock-indices property.
>    Then we don't have to search all the DT nodes to fetch
>    those two value to construct clocks which is relatively
>    low efficiency.
>  * Add required power-domain property as well.
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 45 ++++++++++++++++++----
>  include/dt-bindings/firmware/imx/rsrc.h            | 17 ++++++++
>  2 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> index 5d7dbab..2f46e89 100644
> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -89,6 +89,27 @@ Required properties:
>                           "fsl,imx8qm-clock"
>                           "fsl,imx8qxp-clock"
>                         followed by "fsl,scu-clk"
> +- #address-cells:      Should be 1.
> +- #size-cells:         Should be 0.
> +
> +Sub nodes are required to represent all available SCU clocks within this
> +hardware subsystem and the following properties are needed:
> +
> +- reg:                 Should contain the Resource ID of this SCU clock.
> +- #clock-cells:                Should be 1.
> +- clock-indices:       Index of all clock types supported by this SCU clock.
> +                       The order should match the clock-output-names array.
> +                       Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> +                       available clock types supported by SCU.

I would have expected the clock cell to contain the Resource ID.

Also, this still has one clock per node which you should avoid unless
there's only a small number of clocks (say ~20). Move this all to a
single node with the list of clock IDs in clock-indices and other
properties like power-domains can match up with clock-indices. IOW,
both should have the same length (in elements).

For the clock type, perhaps combine that in the clock cell with the
resource ID such as using the upper 8-bits.

> +- clock-output-names:  Shall be the corresponding names of the outputs.
> +- power-domains:       Should contain the power domain used by this SCU clock.
> +
> +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 (No sub-nodes which is 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
> @@ -144,6 +165,21 @@ lsio_mu1: mailbox@5d1c0000 {
>         #mbox-cells = <2>;
>  };
>
> +conn-scu-clock-controller {
> +       compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +
> +       uart0_clk: clock-scu@57 {
> +               reg = <57>;
> +               #clock-cells = <1>;
> +               clock-indices = <IMX_SC_PM_CLK_PER>;
> +               clock-output-names = "uart0_clk";
> +               power-domains = <&pd IMX_SC_R_UART_0>;
> +       };
> +       ...
> +}
> +
>  firmware {
>         scu {
>                 compatible = "fsl,imx-scu";
> @@ -160,11 +196,6 @@ firmware {
>                           &lsio_mu1 1 3
>                           &lsio_mu1 3 3>;
>
> -               clk: clk {
> -                       compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
> -                       #clock-cells = <1>;
> -               };
> -
>                 iomuxc {
>                         compatible = "fsl,imx8qxp-iomuxc";
>
> @@ -192,8 +223,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 IMX_SC_PM_CLK_PER>;
>         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 4e61f64..fbeaca7 100644
> --- a/include/dt-bindings/firmware/imx/rsrc.h
> +++ b/include/dt-bindings/firmware/imx/rsrc.h
> @@ -547,4 +547,21 @@
>  #define IMX_SC_R_ATTESTATION           545
>  #define IMX_SC_R_LAST                  546
>
> +/*
> + * 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 */

I don't understand how these work with overlapping numbers.

Rob

> From: Rob Herring [mailto:robh+dt@kernel.org]
> Sent: Friday, May 3, 2019 10:53 PM
> Subject: Re: [PATCH V2 1/2] dt-bindings: firmware: imx-scu: new binding to
> parse clocks from device tree
> 
> On Thu, May 2, 2019 at 8:34 PM Aisheng Dong <aisheng.dong@nxp.com>
> wrote:
>
> > There's a few limitations on the original 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. This can make us write a full generic
> > clock driver for SCU based SoCs. No more frequent changes needed in
> > clock driver any more.
> >
> > In the meanwhile, we can also 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. The first one is resource id
> > which is encoded in reg property and the second is clock type index
> > which is encoded in generic clock-indices property they're not continuously.
> >
> > And as we also want to support clock set parent function, 'clocks'
> > property is also used to pass all the possible input parents.
> >
> > Cc: Rob Herring <robh+dt@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>
> > ---
> > ChangeLog:
> > v1->v2:
> >  * changed to one cell binding inspired by arm,scpi.txt
> >    Documentation/devicetree/bindings/arm/arm,scpi.txt
> >    Resource ID is encoded in 'reg' property.
> >    Clock type is encoded in generic clock-indices property.
> >    Then we don't have to search all the DT nodes to fetch
> >    those two value to construct clocks which is relatively
> >    low efficiency.
> >  * Add required power-domain property as well.
> > ---
> >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 45
> ++++++++++++++++++----
> >  include/dt-bindings/firmware/imx/rsrc.h            | 17 ++++++++
> >  2 files changed, 54 insertions(+), 8 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > index 5d7dbab..2f46e89 100644
> > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -89,6 +89,27 @@ Required properties:
> >                           "fsl,imx8qm-clock"
> >                           "fsl,imx8qxp-clock"
> >                         followed by "fsl,scu-clk"
> > +- #address-cells:      Should be 1.
> > +- #size-cells:         Should be 0.
> > +
> > +Sub nodes are required to represent all available SCU clocks within
> > +this hardware subsystem and the following properties are needed:
> > +
> > +- reg:                 Should contain the Resource ID of this SCU clock.
> > +- #clock-cells:                Should be 1.
> > +- clock-indices:       Index of all clock types supported by this SCU clock.
> > +                       The order should match the clock-output-names
> array.
> > +                       Refer to
> <include/dt-bindings/firmware/imx/rsrc.h> for
> > +                       available clock types supported by SCU.
> 
> I would have expected the clock cell to contain the Resource ID.
> 
> Also, this still has one clock per node which you should avoid unless there's
> only a small number of clocks (say ~20). Move this all to a single node with the
> list of clock IDs in clock-indices and other properties like power-domains can
> match up with clock-indices. IOW, both should have the same length (in
> elements).
> 

Do you mean something like this?

#define IMX_SCU_CLK_ID(rsrc, type)      (type << 16 | rsrc)
scu_clk: scu-clock-controller {
        compatible = "fsl,imx8qxp-scu-clk", "fsl,scu-clk";
        #clock-cells = <1>;
        clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_BYPASS)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_MISC0)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>,
                        ...

        clock-output-names = "enet0_clk",
                             "enet0_bypass_clk",
                             "enet0_rgmii_clk",
                             "uart0_clk",
                             ...

        power-domains = <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_UART_0>,
                        ...
};

serial@5a060000 {
        ...
        clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>;
        power-domains = <&pd IMX_SC_R_UART_0>;
};

I wonder moving all clock resources into a single clock controller node may result in losing
the configuration granularity of individual clocks from device tree.

For SCU based platforms, the resource availability (e.g. device/clocks/power) are
configurable by SCU firmware according to the different SW execution partition configuration.
e.g. According to customer's requirements, we may allocate some resources to M4 partition
like some I2C, CAN, audio resources which can't be accessed by A core. 
And we may allocate even more for virtual machines running at another CPU core.
Thus, defining all the clock sources (fixed) in device tree for A core seems to be a little bit
meaningless and it also causes us hard to extend for a new SoC.
E.g. MX8QM has more clocks than QXP in different SS.
That's why we want the per clock source node definition in DT.
Then we can configure the clock sources conveniently according to different partition
setting and new SoC property.

Furthermore, per clock resource node also makes us more easily to handle power domain
In a more standard way and do state save&restore during system suspend/resume due to
the clock state will be lost when the power is off.

Another important thing is that MX8 is consisted of a number of HW subsystem while each
Subsystem has separate clock controllers (both SCU clock controllers and LPCG clock controllers).
I believe this is different from other vendor like TI and ARM Juno which might make us feel we should
use the same way as theirs at the first glance. But we're different.

That's why I use per clock resource node as it seems to be better for i.MX special characteristic.

Considering all above requirements, how would you suggest us to do?

> For the clock type, perhaps combine that in the clock cell with the resource ID
> such as using the upper 8-bits.
> 

It seems we must combine them because current clock-indices binding does not
support two cells index which seems a drawback from user point of view.
e.g.
clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>;

> > +- clock-output-names:  Shall be the corresponding names of the outputs.
> > +- power-domains:       Should contain the power domain used by this
> SCU clock.
> > +
> > +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 (No sub-nodes which is 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 @@ -144,6
> > +165,21 @@ lsio_mu1: mailbox@5d1c0000 {
> >         #mbox-cells = <2>;
> >  };
> >
> > +conn-scu-clock-controller {
> > +       compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
> > +       #address-cells = <1>;
> > +       #size-cells = <0>;
> > +
> > +       uart0_clk: clock-scu@57 {
> > +               reg = <57>;
> > +               #clock-cells = <1>;
> > +               clock-indices = <IMX_SC_PM_CLK_PER>;
> > +               clock-output-names = "uart0_clk";
> > +               power-domains = <&pd IMX_SC_R_UART_0>;
> > +       };
> > +       ...
> > +}
> > +
> >  firmware {
> >         scu {
> >                 compatible = "fsl,imx-scu"; @@ -160,11 +196,6 @@
> > firmware {
> >                           &lsio_mu1 1 3
> >                           &lsio_mu1 3 3>;
> >
> > -               clk: clk {
> > -                       compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
> > -                       #clock-cells = <1>;
> > -               };
> > -
> >                 iomuxc {
> >                         compatible = "fsl,imx8qxp-iomuxc";
> >
> > @@ -192,8 +223,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 IMX_SC_PM_CLK_PER>;
> >         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 4e61f64..fbeaca7 100644
> > --- a/include/dt-bindings/firmware/imx/rsrc.h
> > +++ b/include/dt-bindings/firmware/imx/rsrc.h
> > @@ -547,4 +547,21 @@
> >  #define IMX_SC_R_ATTESTATION           545
> >  #define IMX_SC_R_LAST                  546
> >
> > +/*
> > + * 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 */
> 
> I don't understand how these work with overlapping numbers.

They're defined by SCU firmware protocol for different device resources.
e.g. For PLL resources, it only uses IMX_SC_PM_CLK_PLL.
But for USB, it may use MISC besides PER/MST_BUS/SLV_BUS.

Regards
Dong Aisheng

> 
> Rob

On Sat, May 4, 2019 at 7:19 AM Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> > From: Rob Herring [mailto:robh+dt@kernel.org]
> > Sent: Friday, May 3, 2019 10:53 PM
> > Subject: Re: [PATCH V2 1/2] dt-bindings: firmware: imx-scu: new binding to
> > parse clocks from device tree
> >
> > On Thu, May 2, 2019 at 8:34 PM Aisheng Dong <aisheng.dong@nxp.com>
> > wrote:
> >
> > > There's a few limitations on the original 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. This can make us write a full generic
> > > clock driver for SCU based SoCs. No more frequent changes needed in
> > > clock driver any more.
> > >
> > > In the meanwhile, we can also 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. The first one is resource id
> > > which is encoded in reg property and the second is clock type index
> > > which is encoded in generic clock-indices property they're not continuously.
> > >
> > > And as we also want to support clock set parent function, 'clocks'
> > > property is also used to pass all the possible input parents.
> > >
> > > Cc: Rob Herring <robh+dt@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>
> > > ---
> > > ChangeLog:
> > > v1->v2:
> > >  * changed to one cell binding inspired by arm,scpi.txt
> > >    Documentation/devicetree/bindings/arm/arm,scpi.txt
> > >    Resource ID is encoded in 'reg' property.
> > >    Clock type is encoded in generic clock-indices property.
> > >    Then we don't have to search all the DT nodes to fetch
> > >    those two value to construct clocks which is relatively
> > >    low efficiency.
> > >  * Add required power-domain property as well.
> > > ---
> > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 45
> > ++++++++++++++++++----
> > >  include/dt-bindings/firmware/imx/rsrc.h            | 17 ++++++++
> > >  2 files changed, 54 insertions(+), 8 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 5d7dbab..2f46e89 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -89,6 +89,27 @@ Required properties:
> > >                           "fsl,imx8qm-clock"
> > >                           "fsl,imx8qxp-clock"
> > >                         followed by "fsl,scu-clk"
> > > +- #address-cells:      Should be 1.
> > > +- #size-cells:         Should be 0.
> > > +
> > > +Sub nodes are required to represent all available SCU clocks within
> > > +this hardware subsystem and the following properties are needed:
> > > +
> > > +- reg:                 Should contain the Resource ID of this SCU clock.
> > > +- #clock-cells:                Should be 1.
> > > +- clock-indices:       Index of all clock types supported by this SCU clock.
> > > +                       The order should match the clock-output-names
> > array.
> > > +                       Refer to
> > <include/dt-bindings/firmware/imx/rsrc.h> for
> > > +                       available clock types supported by SCU.
> >
> > I would have expected the clock cell to contain the Resource ID.
> >
> > Also, this still has one clock per node which you should avoid unless there's
> > only a small number of clocks (say ~20). Move this all to a single node with the
> > list of clock IDs in clock-indices and other properties like power-domains can
> > match up with clock-indices. IOW, both should have the same length (in
> > elements).
> >
>
> Do you mean something like this?
>
> #define IMX_SCU_CLK_ID(rsrc, type)      (type << 16 | rsrc)
> scu_clk: scu-clock-controller {
>         compatible = "fsl,imx8qxp-scu-clk", "fsl,scu-clk";
>         #clock-cells = <1>;
>         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_BYPASS)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_MISC0)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>,
>                         ...
>
>         clock-output-names = "enet0_clk",
>                              "enet0_bypass_clk",
>                              "enet0_rgmii_clk",
>                              "uart0_clk",
>                              ...
>
>         power-domains = <&pd IMX_SC_R_ENET_0>,
>                         <&pd IMX_SC_R_ENET_0>,
>                         <&pd IMX_SC_R_ENET_0>,
>                         <&pd IMX_SC_R_UART_0>,
>                         ...
> };

Yes, but...

> serial@5a060000 {
>         ...
>         clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>;

I thought devices got clocks from the LPCG?

>         power-domains = <&pd IMX_SC_R_UART_0>;
> };
>
> I wonder moving all clock resources into a single clock controller node may result in losing
> the configuration granularity of individual clocks from device tree.
>
> For SCU based platforms, the resource availability (e.g. device/clocks/power) are
> configurable by SCU firmware according to the different SW execution partition configuration.
> e.g. According to customer's requirements, we may allocate some resources to M4 partition
> like some I2C, CAN, audio resources which can't be accessed by A core.
> And we may allocate even more for virtual machines running at another CPU core.
> Thus, defining all the clock sources (fixed) in device tree for A core seems to be a little bit
> meaningless and it also causes us hard to extend for a new SoC.

I'm not suggesting that. It's really just re-arranging all the same
data from a bunch of child nodes to a single node. Granted, it may be
easier to add/delete nodes than add/delete elements from an array of
property values, but really that's just a tooling problem

> E.g. MX8QM has more clocks than QXP in different SS.
> That's why we want the per clock source node definition in DT.
> Then we can configure the clock sources conveniently according to different partition
> setting and new SoC property.
>
> Furthermore, per clock resource node also makes us more easily to handle power domain
> In a more standard way and do state save&restore during system suspend/resume due to
> the clock state will be lost when the power is off.
>
> Another important thing is that MX8 is consisted of a number of HW subsystem while each
> Subsystem has separate clock controllers (both SCU clock controllers and LPCG clock controllers).
> I believe this is different from other vendor like TI and ARM Juno which might make us feel we should
> use the same way as theirs at the first glance. But we're different.
>
> That's why I use per clock resource node as it seems to be better for i.MX special characteristic.
>
> Considering all above requirements, how would you suggest us to do?
>
> > For the clock type, perhaps combine that in the clock cell with the resource ID
> > such as using the upper 8-bits.
> >
>
> It seems we must combine them because current clock-indices binding does not
> support two cells index which seems a drawback from user point of view.
> e.g.
> clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>;

That was my original thought, but we could just say the clock-indices
element size is equal to the #clock-cells size and use 2 cells.

Rob

> From: Rob Herring [mailto:robh+dt@kernel.org]
> Sent: Wednesday, May 8, 2019 2:03 AM
> 
> On Sat, May 4, 2019 at 7:19 AM Aisheng Dong <aisheng.dong@nxp.com>
> wrote:
> >
> > > From: Rob Herring [mailto:robh+dt@kernel.org]
> > > Sent: Friday, May 3, 2019 10:53 PM
> > > Subject: Re: [PATCH V2 1/2] dt-bindings: firmware: imx-scu: new
> > > binding to parse clocks from device tree
> > >
> > > On Thu, May 2, 2019 at 8:34 PM Aisheng Dong <aisheng.dong@nxp.com>
> > > wrote:
> > >
> > > > There's a few limitations on the original 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. This can make us write a full
> > > > generic clock driver for SCU based SoCs. No more frequent changes
> > > > needed in clock driver any more.
> > > >
> > > > In the meanwhile, we can also 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. The first one is
> > > > resource id which is encoded in reg property and the second is
> > > > clock type index which is encoded in generic clock-indices property
> they're not continuously.
> > > >
> > > > And as we also want to support clock set parent function, 'clocks'
> > > > property is also used to pass all the possible input parents.
> > > >
> > > > Cc: Rob Herring <robh+dt@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>
> > > > ---
> > > > ChangeLog:
> > > > v1->v2:
> > > >  * changed to one cell binding inspired by arm,scpi.txt
> > > >    Documentation/devicetree/bindings/arm/arm,scpi.txt
> > > >    Resource ID is encoded in 'reg' property.
> > > >    Clock type is encoded in generic clock-indices property.
> > > >    Then we don't have to search all the DT nodes to fetch
> > > >    those two value to construct clocks which is relatively
> > > >    low efficiency.
> > > >  * Add required power-domain property as well.
> > > > ---
> > > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 45
> > > ++++++++++++++++++----
> > > >  include/dt-bindings/firmware/imx/rsrc.h            | 17 ++++++++
> > > >  2 files changed, 54 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > index 5d7dbab..2f46e89 100644
> > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > @@ -89,6 +89,27 @@ Required properties:
> > > >                           "fsl,imx8qm-clock"
> > > >                           "fsl,imx8qxp-clock"
> > > >                         followed by "fsl,scu-clk"
> > > > +- #address-cells:      Should be 1.
> > > > +- #size-cells:         Should be 0.
> > > > +
> > > > +Sub nodes are required to represent all available SCU clocks
> > > > +within this hardware subsystem and the following properties are
> needed:
> > > > +
> > > > +- reg:                 Should contain the Resource ID of this SCU
> clock.
> > > > +- #clock-cells:                Should be 1.
> > > > +- clock-indices:       Index of all clock types supported by this SCU
> clock.
> > > > +                       The order should match the
> > > > +clock-output-names
> > > array.
> > > > +                       Refer to
> > > <include/dt-bindings/firmware/imx/rsrc.h> for
> > > > +                       available clock types supported by SCU.
> > >
> > > I would have expected the clock cell to contain the Resource ID.
> > >
> > > Also, this still has one clock per node which you should avoid
> > > unless there's only a small number of clocks (say ~20). Move this
> > > all to a single node with the list of clock IDs in clock-indices and
> > > other properties like power-domains can match up with clock-indices.
> > > IOW, both should have the same length (in elements).
> > >
> >
> > Do you mean something like this?
> >
> > #define IMX_SCU_CLK_ID(rsrc, type)      (type << 16 | rsrc)
> > scu_clk: scu-clock-controller {
> >         compatible = "fsl,imx8qxp-scu-clk", "fsl,scu-clk";
> >         #clock-cells = <1>;
> >         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> IMX_SC_PM_CLK_BYPASS)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> IMX_SC_PM_CLK_MISC0)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> IMX_SC_PM_CLK_PER)>,
> >                         ...
> >
> >         clock-output-names = "enet0_clk",
> >                              "enet0_bypass_clk",
> >                              "enet0_rgmii_clk",
> >                              "uart0_clk",
> >                              ...
> >
> >         power-domains = <&pd IMX_SC_R_ENET_0>,
> >                         <&pd IMX_SC_R_ENET_0>,
> >                         <&pd IMX_SC_R_ENET_0>,
> >                         <&pd IMX_SC_R_UART_0>,
> >                         ...
> > };
> 
> Yes, but...
> 
> > serial@5a060000 {
> >         ...
> >         clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> > IMX_SC_PM_CLK_PER)>;
> 
> I thought devices got clocks from the LPCG?
> 

Yes. Here is just an example of using SCU clocks.
And for some devices without LPCG, it could also get clocks directly from SCU clock.
 
> >         power-domains = <&pd IMX_SC_R_UART_0>; };
> >
> > I wonder moving all clock resources into a single clock controller
> > node may result in losing the configuration granularity of individual clocks
> from device tree.
> >
> > For SCU based platforms, the resource availability (e.g.
> > device/clocks/power) are configurable by SCU firmware according to the
> different SW execution partition configuration.
> > e.g. According to customer's requirements, we may allocate some
> > resources to M4 partition like some I2C, CAN, audio resources which can't be
> accessed by A core.
> > And we may allocate even more for virtual machines running at another CPU
> core.
> > Thus, defining all the clock sources (fixed) in device tree for A core
> > seems to be a little bit meaningless and it also causes us hard to extend for a
> new SoC.
> 
> I'm not suggesting that. It's really just re-arranging all the same data from a
> bunch of child nodes to a single node. Granted, it may be easier to add/delete
> nodes than add/delete elements from an array of property values, but really
> that's just a tooling problem
> 

Okay, understood.
So it seems we could still have a separate clock controller node for each SS but merge
all the same data of child nodes data into it.

However, we still have one concern.
Taking MX8QXP DMA SS as example, with one node description, it may be something
like below:
dma_scu_clk: dma-scu-clock-controller {
        compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
        #clock-cells = <1>;
        clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ADC_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_CAN_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_FTM_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_FTM_1, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_I2C_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_I2C_1, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_I2C_2, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_I2C_3, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0_PWM_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_SPI_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_SPI_1, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_SPI_2, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_SPI_3, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_UART_1, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_UART_2, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_UART_3, IMX_SC_PM_CLK_PER)>;
        clock-output-names = "adc0_clk",
                             "can0_clk",
                             "ftm0_clk",
                             "ftm1_clk",
                             "i2c0_clk",
                             "i2c1_clk",
                             "i2c2_clk",
                             "i2c3_clk",
                             "lcd0_clk",
                             "lcd0_pwm0_clk",
                             "spi0_clk",
                             "spi1_clk",
                             "spi2_clk",
                             "spi3_clk",
                             "uart0_clk",
                             "uart1_clk",
                             "uart2_clk",
                             "uart3_clk";
        power-domains = <&pd IMX_SC_R_ADC_0>,
                        <&pd IMX_SC_R_CAN_0>,
                        <&pd IMX_SC_R_FTM_0>,
                        <&pd IMX_SC_R_FTM_1>,
                        <&pd IMX_SC_R_I2C_0>,
                        <&pd IMX_SC_R_I2C_1>,
                        <&pd IMX_SC_R_I2C_2>,
                        <&pd IMX_SC_R_I2C_3>,
                        <&pd IMX_SC_R_LCD_0>,
                        <&pd IMX_SC_R_LCD_0_PWM_0>,
                        <&pd IMX_SC_R_SPI_0>,
                        <&pd IMX_SC_R_SPI_1>,
                        <&pd IMX_SC_R_SPI_2>,
                        <&pd IMX_SC_R_SPI_3>,
                        <&pd IMX_SC_R_UART_0>,
                        <&pd IMX_SC_R_UART_1>,
                        <&pd IMX_SC_R_UART_2>,
                        <&pd IMX_SC_R_UART_3>;
};

For MX8QM, even if we have one more UART_4, then we still have to write
all the same things again with an extra UART_4. It seems it's a bit violate our design
that using a shared one and do incremental changes for new SoCs.
Do you think if this is acceptable to you?
But if describe them per nodes, we do not have such issue.

Anyway, please tell me your choice, then I will follow.

BTW, I don't know how a tool can address this issue.
Please let me know if you have better idea.

> > E.g. MX8QM has more clocks than QXP in different SS.
> > That's why we want the per clock source node definition in DT.
> > Then we can configure the clock sources conveniently according to
> > different partition setting and new SoC property.
> >
> > Furthermore, per clock resource node also makes us more easily to
> > handle power domain In a more standard way and do state save&restore
> > during system suspend/resume due to the clock state will be lost when the
> power is off.
> >
> > Another important thing is that MX8 is consisted of a number of HW
> > subsystem while each Subsystem has separate clock controllers (both SCU
> clock controllers and LPCG clock controllers).
> > I believe this is different from other vendor like TI and ARM Juno
> > which might make us feel we should use the same way as theirs at the first
> glance. But we're different.
> >
> > That's why I use per clock resource node as it seems to be better for i.MX
> special characteristic.
> >
> > Considering all above requirements, how would you suggest us to do?
> >
> > > For the clock type, perhaps combine that in the clock cell with the
> > > resource ID such as using the upper 8-bits.
> > >
> >
> > It seems we must combine them because current clock-indices binding
> > does not support two cells index which seems a drawback from user point of
> view.
> > e.g.
> > clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> > IMX_SC_PM_CLK_PER)>;
> 
> That was my original thought, but we could just say the clock-indices element
> size is equal to the #clock-cells size and use 2 cells.
> 

Okay, then we have extend the clock core API of_clk_get_parent_name
to support it as well.

Regards
Dong Aisheng

> Rob

On Wed, May 08, 2019 at 07:16:11AM +0000, Aisheng Dong wrote:
> > From: Rob Herring [mailto:robh+dt@kernel.org]
> > Sent: Wednesday, May 8, 2019 2:03 AM
> > 
> > On Sat, May 4, 2019 at 7:19 AM Aisheng Dong <aisheng.dong@nxp.com>
> > wrote:
> > >
> > > > From: Rob Herring [mailto:robh+dt@kernel.org]
> > > > Sent: Friday, May 3, 2019 10:53 PM
> > > > Subject: Re: [PATCH V2 1/2] dt-bindings: firmware: imx-scu: new
> > > > binding to parse clocks from device tree
> > > >
> > > > On Thu, May 2, 2019 at 8:34 PM Aisheng Dong <aisheng.dong@nxp.com>
> > > > wrote:
> > > >
> > > > > There's a few limitations on the original 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. This can make us write a full
> > > > > generic clock driver for SCU based SoCs. No more frequent changes
> > > > > needed in clock driver any more.
> > > > >
> > > > > In the meanwhile, we can also 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. The first one is
> > > > > resource id which is encoded in reg property and the second is
> > > > > clock type index which is encoded in generic clock-indices property
> > they're not continuously.
> > > > >
> > > > > And as we also want to support clock set parent function, 'clocks'
> > > > > property is also used to pass all the possible input parents.
> > > > >
> > > > > Cc: Rob Herring <robh+dt@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>
> > > > > ---
> > > > > ChangeLog:
> > > > > v1->v2:
> > > > >  * changed to one cell binding inspired by arm,scpi.txt
> > > > >    Documentation/devicetree/bindings/arm/arm,scpi.txt
> > > > >    Resource ID is encoded in 'reg' property.
> > > > >    Clock type is encoded in generic clock-indices property.
> > > > >    Then we don't have to search all the DT nodes to fetch
> > > > >    those two value to construct clocks which is relatively
> > > > >    low efficiency.
> > > > >  * Add required power-domain property as well.
> > > > > ---
> > > > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 45
> > > > ++++++++++++++++++----
> > > > >  include/dt-bindings/firmware/imx/rsrc.h            | 17 ++++++++
> > > > >  2 files changed, 54 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > index 5d7dbab..2f46e89 100644
> > > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > @@ -89,6 +89,27 @@ Required properties:
> > > > >                           "fsl,imx8qm-clock"
> > > > >                           "fsl,imx8qxp-clock"
> > > > >                         followed by "fsl,scu-clk"
> > > > > +- #address-cells:      Should be 1.
> > > > > +- #size-cells:         Should be 0.
> > > > > +
> > > > > +Sub nodes are required to represent all available SCU clocks
> > > > > +within this hardware subsystem and the following properties are
> > needed:
> > > > > +
> > > > > +- reg:                 Should contain the Resource ID of this SCU
> > clock.
> > > > > +- #clock-cells:                Should be 1.
> > > > > +- clock-indices:       Index of all clock types supported by this SCU
> > clock.
> > > > > +                       The order should match the
> > > > > +clock-output-names
> > > > array.
> > > > > +                       Refer to
> > > > <include/dt-bindings/firmware/imx/rsrc.h> for
> > > > > +                       available clock types supported by SCU.
> > > >
> > > > I would have expected the clock cell to contain the Resource ID.
> > > >
> > > > Also, this still has one clock per node which you should avoid
> > > > unless there's only a small number of clocks (say ~20). Move this
> > > > all to a single node with the list of clock IDs in clock-indices and
> > > > other properties like power-domains can match up with clock-indices.
> > > > IOW, both should have the same length (in elements).
> > > >
> > >
> > > Do you mean something like this?
> > >
> > > #define IMX_SCU_CLK_ID(rsrc, type)      (type << 16 | rsrc)
> > > scu_clk: scu-clock-controller {
> > >         compatible = "fsl,imx8qxp-scu-clk", "fsl,scu-clk";
> > >         #clock-cells = <1>;
> > >         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> > IMX_SC_PM_CLK_BYPASS)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> > IMX_SC_PM_CLK_MISC0)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         ...
> > >
> > >         clock-output-names = "enet0_clk",
> > >                              "enet0_bypass_clk",
> > >                              "enet0_rgmii_clk",
> > >                              "uart0_clk",
> > >                              ...
> > >
> > >         power-domains = <&pd IMX_SC_R_ENET_0>,
> > >                         <&pd IMX_SC_R_ENET_0>,
> > >                         <&pd IMX_SC_R_ENET_0>,
> > >                         <&pd IMX_SC_R_UART_0>,
> > >                         ...
> > > };
> > 
> > Yes, but...
> > 
> > > serial@5a060000 {
> > >         ...
> > >         clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> > > IMX_SC_PM_CLK_PER)>;
> > 
> > I thought devices got clocks from the LPCG?
> > 
> 
> Yes. Here is just an example of using SCU clocks.
> And for some devices without LPCG, it could also get clocks directly from SCU clock.
>  
> > >         power-domains = <&pd IMX_SC_R_UART_0>; };
> > >
> > > I wonder moving all clock resources into a single clock controller
> > > node may result in losing the configuration granularity of individual clocks
> > from device tree.
> > >
> > > For SCU based platforms, the resource availability (e.g.
> > > device/clocks/power) are configurable by SCU firmware according to the
> > different SW execution partition configuration.
> > > e.g. According to customer's requirements, we may allocate some
> > > resources to M4 partition like some I2C, CAN, audio resources which can't be
> > accessed by A core.
> > > And we may allocate even more for virtual machines running at another CPU
> > core.
> > > Thus, defining all the clock sources (fixed) in device tree for A core
> > > seems to be a little bit meaningless and it also causes us hard to extend for a
> > new SoC.
> > 
> > I'm not suggesting that. It's really just re-arranging all the same data from a
> > bunch of child nodes to a single node. Granted, it may be easier to add/delete
> > nodes than add/delete elements from an array of property values, but really
> > that's just a tooling problem
> > 
> 
> Okay, understood.
> So it seems we could still have a separate clock controller node for each SS but merge
> all the same data of child nodes data into it.
> 
> However, we still have one concern.
> Taking MX8QXP DMA SS as example, with one node description, it may be something
> like below:
> dma_scu_clk: dma-scu-clock-controller {
>         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
>         #clock-cells = <1>;
>         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ADC_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_CAN_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_1, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_1, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_2, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_3, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0_PWM_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_1, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_2, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_3, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_1, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_2, IMX_SC_PM_CLK_PER)>,
>                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_3, IMX_SC_PM_CLK_PER)>;
>         clock-output-names = "adc0_clk",
>                              "can0_clk",
>                              "ftm0_clk",
>                              "ftm1_clk",
>                              "i2c0_clk",
>                              "i2c1_clk",
>                              "i2c2_clk",
>                              "i2c3_clk",
>                              "lcd0_clk",
>                              "lcd0_pwm0_clk",
>                              "spi0_clk",
>                              "spi1_clk",
>                              "spi2_clk",
>                              "spi3_clk",
>                              "uart0_clk",
>                              "uart1_clk",
>                              "uart2_clk",
>                              "uart3_clk";
>         power-domains = <&pd IMX_SC_R_ADC_0>,
>                         <&pd IMX_SC_R_CAN_0>,
>                         <&pd IMX_SC_R_FTM_0>,
>                         <&pd IMX_SC_R_FTM_1>,
>                         <&pd IMX_SC_R_I2C_0>,
>                         <&pd IMX_SC_R_I2C_1>,
>                         <&pd IMX_SC_R_I2C_2>,
>                         <&pd IMX_SC_R_I2C_3>,
>                         <&pd IMX_SC_R_LCD_0>,
>                         <&pd IMX_SC_R_LCD_0_PWM_0>,
>                         <&pd IMX_SC_R_SPI_0>,
>                         <&pd IMX_SC_R_SPI_1>,
>                         <&pd IMX_SC_R_SPI_2>,
>                         <&pd IMX_SC_R_SPI_3>,
>                         <&pd IMX_SC_R_UART_0>,
>                         <&pd IMX_SC_R_UART_1>,
>                         <&pd IMX_SC_R_UART_2>,
>                         <&pd IMX_SC_R_UART_3>;
> };
> 
> For MX8QM, even if we have one more UART_4, then we still have to write
> all the same things again with an extra UART_4. It seems it's a bit violate our design
> that using a shared one and do incremental changes for new SoCs.
> Do you think if this is acceptable to you?

Yes, as it should be a one time thing to do per SoC.

> But if describe them per nodes, we do not have such issue.
> 
> Anyway, please tell me your choice, then I will follow.
> 
> BTW, I don't know how a tool can address this issue.

I meant you could write one that understands the binding. It's a bit 
more complicated having to parse and update properties compared to 
adding or removing nodes, but it can still be done.

Rob

> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Tuesday, May 14, 2019 2:00 AM
> 
> On Wed, May 08, 2019 at 07:16:11AM +0000, Aisheng Dong wrote:
> > > From: Rob Herring [mailto:robh+dt@kernel.org]
> > > Sent: Wednesday, May 8, 2019 2:03 AM
> > >
> > > On Sat, May 4, 2019 at 7:19 AM Aisheng Dong <aisheng.dong@nxp.com>
> > > wrote:
> > > >
> > > > > From: Rob Herring [mailto:robh+dt@kernel.org]
> > > > > Sent: Friday, May 3, 2019 10:53 PM
> > > > > Subject: Re: [PATCH V2 1/2] dt-bindings: firmware: imx-scu: new
> > > > > binding to parse clocks from device tree
> > > > >
> > > > > On Thu, May 2, 2019 at 8:34 PM Aisheng Dong
> > > > > <aisheng.dong@nxp.com>
> > > > > wrote:
> > > > >
> > > > > > There's a few limitations on the original 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. This can make us write a
> > > > > > full generic clock driver for SCU based SoCs. No more frequent
> > > > > > changes needed in clock driver any more.
> > > > > >
> > > > > > In the meanwhile, we can also 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. The first one is
> > > > > > resource id which is encoded in reg property and the second is
> > > > > > clock type index which is encoded in generic clock-indices
> > > > > > property
> > > they're not continuously.
> > > > > >
> > > > > > And as we also want to support clock set parent function, 'clocks'
> > > > > > property is also used to pass all the possible input parents.
> > > > > >
> > > > > > Cc: Rob Herring <robh+dt@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>
> > > > > > ---
> > > > > > ChangeLog:
> > > > > > v1->v2:
> > > > > >  * changed to one cell binding inspired by arm,scpi.txt
> > > > > >    Documentation/devicetree/bindings/arm/arm,scpi.txt
> > > > > >    Resource ID is encoded in 'reg' property.
> > > > > >    Clock type is encoded in generic clock-indices property.
> > > > > >    Then we don't have to search all the DT nodes to fetch
> > > > > >    those two value to construct clocks which is relatively
> > > > > >    low efficiency.
> > > > > >  * Add required power-domain property as well.
> > > > > > ---
> > > > > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 45
> > > > > ++++++++++++++++++----
> > > > > >  include/dt-bindings/firmware/imx/rsrc.h            | 17
> ++++++++
> > > > > >  2 files changed, 54 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > > index 5d7dbab..2f46e89 100644
> > > > > > ---
> > > > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.
> > > > > > +++ txt
> > > > > > @@ -89,6 +89,27 @@ Required properties:
> > > > > >                           "fsl,imx8qm-clock"
> > > > > >                           "fsl,imx8qxp-clock"
> > > > > >                         followed by "fsl,scu-clk"
> > > > > > +- #address-cells:      Should be 1.
> > > > > > +- #size-cells:         Should be 0.
> > > > > > +
> > > > > > +Sub nodes are required to represent all available SCU clocks
> > > > > > +within this hardware subsystem and the following properties
> > > > > > +are
> > > needed:
> > > > > > +
> > > > > > +- reg:                 Should contain the Resource ID of this SCU
> > > clock.
> > > > > > +- #clock-cells:                Should be 1.
> > > > > > +- clock-indices:       Index of all clock types supported by this SCU
> > > clock.
> > > > > > +                       The order should match the
> > > > > > +clock-output-names
> > > > > array.
> > > > > > +                       Refer to
> > > > > <include/dt-bindings/firmware/imx/rsrc.h> for
> > > > > > +                       available clock types supported by SCU.
> > > > >
> > > > > I would have expected the clock cell to contain the Resource ID.
> > > > >
> > > > > Also, this still has one clock per node which you should avoid
> > > > > unless there's only a small number of clocks (say ~20). Move
> > > > > this all to a single node with the list of clock IDs in
> > > > > clock-indices and other properties like power-domains can match up
> with clock-indices.
> > > > > IOW, both should have the same length (in elements).
> > > > >
> > > >
> > > > Do you mean something like this?
> > > >
> > > > #define IMX_SCU_CLK_ID(rsrc, type)      (type << 16 | rsrc)
> > > > scu_clk: scu-clock-controller {
> > > >         compatible = "fsl,imx8qxp-scu-clk", "fsl,scu-clk";
> > > >         #clock-cells = <1>;
> > > >         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> > > IMX_SC_PM_CLK_BYPASS)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0,
> > > IMX_SC_PM_CLK_MISC0)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         ...
> > > >
> > > >         clock-output-names = "enet0_clk",
> > > >                              "enet0_bypass_clk",
> > > >                              "enet0_rgmii_clk",
> > > >                              "uart0_clk",
> > > >                              ...
> > > >
> > > >         power-domains = <&pd IMX_SC_R_ENET_0>,
> > > >                         <&pd IMX_SC_R_ENET_0>,
> > > >                         <&pd IMX_SC_R_ENET_0>,
> > > >                         <&pd IMX_SC_R_UART_0>,
> > > >                         ...
> > > > };
> > >
> > > Yes, but...
> > >
> > > > serial@5a060000 {
> > > >         ...
> > > >         clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> > > > IMX_SC_PM_CLK_PER)>;
> > >
> > > I thought devices got clocks from the LPCG?
> > >
> >
> > Yes. Here is just an example of using SCU clocks.
> > And for some devices without LPCG, it could also get clocks directly from SCU
> clock.
> >
> > > >         power-domains = <&pd IMX_SC_R_UART_0>; };
> > > >
> > > > I wonder moving all clock resources into a single clock controller
> > > > node may result in losing the configuration granularity of
> > > > individual clocks
> > > from device tree.
> > > >
> > > > For SCU based platforms, the resource availability (e.g.
> > > > device/clocks/power) are configurable by SCU firmware according to
> > > > the
> > > different SW execution partition configuration.
> > > > e.g. According to customer's requirements, we may allocate some
> > > > resources to M4 partition like some I2C, CAN, audio resources
> > > > which can't be
> > > accessed by A core.
> > > > And we may allocate even more for virtual machines running at
> > > > another CPU
> > > core.
> > > > Thus, defining all the clock sources (fixed) in device tree for A
> > > > core seems to be a little bit meaningless and it also causes us
> > > > hard to extend for a
> > > new SoC.
> > >
> > > I'm not suggesting that. It's really just re-arranging all the same
> > > data from a bunch of child nodes to a single node. Granted, it may
> > > be easier to add/delete nodes than add/delete elements from an array
> > > of property values, but really that's just a tooling problem
> > >
> >
> > Okay, understood.
> > So it seems we could still have a separate clock controller node for
> > each SS but merge all the same data of child nodes data into it.
> >
> > However, we still have one concern.
> > Taking MX8QXP DMA SS as example, with one node description, it may be
> > something like below:
> > dma_scu_clk: dma-scu-clock-controller {
> >         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> >         #clock-cells = <1>;
> >         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ADC_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_CAN_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_1,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_1,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_2,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_3,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0_PWM_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_1,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_2,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_3,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_1,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_2,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_3,
> IMX_SC_PM_CLK_PER)>;
> >         clock-output-names = "adc0_clk",
> >                              "can0_clk",
> >                              "ftm0_clk",
> >                              "ftm1_clk",
> >                              "i2c0_clk",
> >                              "i2c1_clk",
> >                              "i2c2_clk",
> >                              "i2c3_clk",
> >                              "lcd0_clk",
> >                              "lcd0_pwm0_clk",
> >                              "spi0_clk",
> >                              "spi1_clk",
> >                              "spi2_clk",
> >                              "spi3_clk",
> >                              "uart0_clk",
> >                              "uart1_clk",
> >                              "uart2_clk",
> >                              "uart3_clk";
> >         power-domains = <&pd IMX_SC_R_ADC_0>,
> >                         <&pd IMX_SC_R_CAN_0>,
> >                         <&pd IMX_SC_R_FTM_0>,
> >                         <&pd IMX_SC_R_FTM_1>,
> >                         <&pd IMX_SC_R_I2C_0>,
> >                         <&pd IMX_SC_R_I2C_1>,
> >                         <&pd IMX_SC_R_I2C_2>,
> >                         <&pd IMX_SC_R_I2C_3>,
> >                         <&pd IMX_SC_R_LCD_0>,
> >                         <&pd IMX_SC_R_LCD_0_PWM_0>,
> >                         <&pd IMX_SC_R_SPI_0>,
> >                         <&pd IMX_SC_R_SPI_1>,
> >                         <&pd IMX_SC_R_SPI_2>,
> >                         <&pd IMX_SC_R_SPI_3>,
> >                         <&pd IMX_SC_R_UART_0>,
> >                         <&pd IMX_SC_R_UART_1>,
> >                         <&pd IMX_SC_R_UART_2>,
> >                         <&pd IMX_SC_R_UART_3>; };
> >
> > For MX8QM, even if we have one more UART_4, then we still have to
> > write all the same things again with an extra UART_4. It seems it's a
> > bit violate our design that using a shared one and do incremental changes for
> new SoCs.
> > Do you think if this is acceptable to you?
> 
> Yes, as it should be a one time thing to do per SoC.

Not really. We may need write a few other copies with slight differences according to
different partition configuration.
For example, on MX8QM MEK board, we allocate CAN resource as well as a few other I2C
to M4 partition which means M4 SW will be the one to handle CAN transfer.
Then we will have 3 copies of SCU clock configurations:
Imx8-ss-dma.dtsi => SS common part
Imx8qm-ss-dma.dtsi => Handle SoC SS difference
Imx8qm-mek.dts => Handle partition configuration or virtualization

Considering this is one SS, there're still other ~10 SS.
What I can imagine is that we might need re-write many big SS SCU clocks node 
as above in board.dts instead of doing granularly tuning by per clock resource nodes.

And actually LPCG clocks already did that way by adding/removing
Nodes for partition setting, as well as device nodes. Now SCU clocks can't
if combine them into one big node which are not aligned with others.

Do you think this is still all okay?
If you insist we can give a try.

> 
> > But if describe them per nodes, we do not have such issue.
> >
> > Anyway, please tell me your choice, then I will follow.
> >
> > BTW, I don't know how a tool can address this issue.
> 
> I meant you could write one that understands the binding. It's a bit more
> complicated having to parse and update properties compared to adding or
> removing nodes, but it can still be done.
> 

The written one(SCU clocks) might change according to different customer's
requirements on partition configuration. That's the trouble we have now.

Regards
Dong Aisheng

> Rob

Hi Rob,

[...]

> > > > For SCU based platforms, the resource availability (e.g.
> > > > device/clocks/power) are configurable by SCU firmware according to
> > > > the
> > > different SW execution partition configuration.
> > > > e.g. According to customer's requirements, we may allocate some
> > > > resources to M4 partition like some I2C, CAN, audio resources
> > > > which can't be
> > > accessed by A core.
> > > > And we may allocate even more for virtual machines running at
> > > > another CPU
> > > core.
> > > > Thus, defining all the clock sources (fixed) in device tree for A
> > > > core seems to be a little bit meaningless and it also causes us
> > > > hard to extend for a
> > > new SoC.
> > >
> > > I'm not suggesting that. It's really just re-arranging all the same
> > > data from a bunch of child nodes to a single node. Granted, it may
> > > be easier to add/delete nodes than add/delete elements from an array
> > > of property values, but really that's just a tooling problem
> > >
> >
> > Okay, understood.
> > So it seems we could still have a separate clock controller node for
> > each SS but merge all the same data of child nodes data into it.
> >
> > However, we still have one concern.
> > Taking MX8QXP DMA SS as example, with one node description, it may be
> > something like below:
> > dma_scu_clk: dma-scu-clock-controller {
> >         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> >         #clock-cells = <1>;
> >         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ADC_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_CAN_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_1,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_1,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_2,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_3,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0_PWM_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_1,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_2,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_3,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_1,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_2,
> IMX_SC_PM_CLK_PER)>,
> >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_3,
> IMX_SC_PM_CLK_PER)>;
> >         clock-output-names = "adc0_clk",
> >                              "can0_clk",
> >                              "ftm0_clk",
> >                              "ftm1_clk",
> >                              "i2c0_clk",
> >                              "i2c1_clk",
> >                              "i2c2_clk",
> >                              "i2c3_clk",
> >                              "lcd0_clk",
> >                              "lcd0_pwm0_clk",
> >                              "spi0_clk",
> >                              "spi1_clk",
> >                              "spi2_clk",
> >                              "spi3_clk",
> >                              "uart0_clk",
> >                              "uart1_clk",
> >                              "uart2_clk",
> >                              "uart3_clk";
> >         power-domains = <&pd IMX_SC_R_ADC_0>,
> >                         <&pd IMX_SC_R_CAN_0>,
> >                         <&pd IMX_SC_R_FTM_0>,
> >                         <&pd IMX_SC_R_FTM_1>,
> >                         <&pd IMX_SC_R_I2C_0>,
> >                         <&pd IMX_SC_R_I2C_1>,
> >                         <&pd IMX_SC_R_I2C_2>,
> >                         <&pd IMX_SC_R_I2C_3>,
> >                         <&pd IMX_SC_R_LCD_0>,
> >                         <&pd IMX_SC_R_LCD_0_PWM_0>,
> >                         <&pd IMX_SC_R_SPI_0>,
> >                         <&pd IMX_SC_R_SPI_1>,
> >                         <&pd IMX_SC_R_SPI_2>,
> >                         <&pd IMX_SC_R_SPI_3>,
> >                         <&pd IMX_SC_R_UART_0>,
> >                         <&pd IMX_SC_R_UART_1>,
> >                         <&pd IMX_SC_R_UART_2>,
> >                         <&pd IMX_SC_R_UART_3>; };
> >
> > For MX8QM, even if we have one more UART_4, then we still have to
> > write all the same things again with an extra UART_4. It seems it's a
> > bit violate our design that using a shared one and do incremental changes for
> new SoCs.
> > Do you think if this is acceptable to you?
> 
> Yes, as it should be a one time thing to do per SoC.
> 

I found we may still can't use this new way after giving a try.
One know issue is that it can't support clock parent setting well with this binding
If merged all sub clocks into a single node.
Hard to describe parent clocks for each clock within the same big array.

For example in MX8 ADMA SS, there're another LCD PLL which can be optional parent clocks
to others peripherals. 
If we list them all in the same array, we can't describe LCD baud/pixel clock parents.
dma_scu_clk: dma-scu-clock-controller {
        compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
        #clock-cells = <1>;
        clock-indices = <SC_R_ELCDIF_PLL IMX_SC_PM_CLK_PLL>,
                        <SC_R_LCD_0 IMX_SC_PM_CLK_PER>,         /* lcd baud */
                        <SC_R_LCD_0 IMX_SC_PM_CLK_SLV_BUS>,     /* Pixel Link */
                        ...
        clock-output-names = "lcdif_pll",
                             "lcdif_baud_clk",
                             "lcdif_pixel_clk",
                                ...
        power-domains = <&pd IMX_SC_R_LCD_0>,
                        <&pd IMX_SC_R_LCD_0>,
                        <&pd IMX_SC_R_LCD_0>,
                        ...
};

And other peripherals might have different parents within the same array.

The old way does not have this issue because it's capable of configuring parents
respectively for each sub clocks.
/* SCU clocks */
dma_scu_clk: clock-controller-scu-dma {
        #address-cells = <1>;
        #size-cells = <0>;

        lcd_pll: clock-scu@323 {
                reg = <323>;
                #clock-cells = <1>;
                clock-indices = <IMX_SC_PM_CLK_PLL>;
                clock-output-names = "lcd_pll";
                power-domains = <&pd IMX_SC_R_ELCDIF_PLL>;
        };

        lcd0_clk: clock-scu@187 {
                reg = <187>;
                #clock-cells = <1>;
                /* parent clocks should match HW programing order */
                clocks = <&dummy_clk &dummy_clk &dummy_clk &dummy_clk &lcd_pll>;
                clock-indices = <IMX_SC_PM_CLK_PER>;
                clock-output-names = "lcd0_clk";
                power-domains = <&pd IMX_SC_R_LCD_0>;
        };
        ...
};

I double checked other SS like Audio, DC, MIPI, PI which have the same issue. 
I really don't know if there will be a way out if using the one single node way.
And I'm also a bit worrying whether it may cause more issues due to its
losing of the flexibility and causes potential issues.

Do you think if we can still go back to the old way which is proposed
In this patch set?
As it can perfectly meet our requirements and also ease the driver
implementation.

Hope you can help shed some lights as we're pending on it for a long time.

Regards
Dong Aisheng

> > But if describe them per nodes, we do not have such issue.
> >
> > Anyway, please tell me your choice, then I will follow.
> >
> > BTW, I don't know how a tool can address this issue.
> 
> I meant you could write one that understands the binding. It's a bit more
> complicated having to parse and update properties compared to adding or
> removing nodes, but it can still be done.
> 
> Rob

Hi Rob,

Would you help check my reply and offer some suggestions?
We're a bit lost on what to do and being blocked here for a long time which affects
all the following MX8QM/QXP upstreaming works.

We really need your help to clarify how to move forward.
If any more information need me to provide, feel free to let me know.

Thanks a lot in advance!

Regards
Dong Aisheng

> From: Aisheng Dong
> Sent: Wednesday, May 22, 2019 1:57 AM
> Hi Rob,
> 
> [...]
> 
> > > > > For SCU based platforms, the resource availability (e.g.
> > > > > device/clocks/power) are configurable by SCU firmware according
> > > > > to the
> > > > different SW execution partition configuration.
> > > > > e.g. According to customer's requirements, we may allocate some
> > > > > resources to M4 partition like some I2C, CAN, audio resources
> > > > > which can't be
> > > > accessed by A core.
> > > > > And we may allocate even more for virtual machines running at
> > > > > another CPU
> > > > core.
> > > > > Thus, defining all the clock sources (fixed) in device tree for
> > > > > A core seems to be a little bit meaningless and it also causes
> > > > > us hard to extend for a
> > > > new SoC.
> > > >
> > > > I'm not suggesting that. It's really just re-arranging all the
> > > > same data from a bunch of child nodes to a single node. Granted,
> > > > it may be easier to add/delete nodes than add/delete elements from
> > > > an array of property values, but really that's just a tooling
> > > > problem
> > > >
> > >
> > > Okay, understood.
> > > So it seems we could still have a separate clock controller node for
> > > each SS but merge all the same data of child nodes data into it.
> > >
> > > However, we still have one concern.
> > > Taking MX8QXP DMA SS as example, with one node description, it may
> > > be something like below:
> > > dma_scu_clk: dma-scu-clock-controller {
> > >         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> > >         #clock-cells = <1>;
> > >         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ADC_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_CAN_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_1,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_1,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_2,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_3,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0_PWM_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_1,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_2,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_3,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_1,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_2,
> > IMX_SC_PM_CLK_PER)>,
> > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_3,
> > IMX_SC_PM_CLK_PER)>;
> > >         clock-output-names = "adc0_clk",
> > >                              "can0_clk",
> > >                              "ftm0_clk",
> > >                              "ftm1_clk",
> > >                              "i2c0_clk",
> > >                              "i2c1_clk",
> > >                              "i2c2_clk",
> > >                              "i2c3_clk",
> > >                              "lcd0_clk",
> > >                              "lcd0_pwm0_clk",
> > >                              "spi0_clk",
> > >                              "spi1_clk",
> > >                              "spi2_clk",
> > >                              "spi3_clk",
> > >                              "uart0_clk",
> > >                              "uart1_clk",
> > >                              "uart2_clk",
> > >                              "uart3_clk";
> > >         power-domains = <&pd IMX_SC_R_ADC_0>,
> > >                         <&pd IMX_SC_R_CAN_0>,
> > >                         <&pd IMX_SC_R_FTM_0>,
> > >                         <&pd IMX_SC_R_FTM_1>,
> > >                         <&pd IMX_SC_R_I2C_0>,
> > >                         <&pd IMX_SC_R_I2C_1>,
> > >                         <&pd IMX_SC_R_I2C_2>,
> > >                         <&pd IMX_SC_R_I2C_3>,
> > >                         <&pd IMX_SC_R_LCD_0>,
> > >                         <&pd IMX_SC_R_LCD_0_PWM_0>,
> > >                         <&pd IMX_SC_R_SPI_0>,
> > >                         <&pd IMX_SC_R_SPI_1>,
> > >                         <&pd IMX_SC_R_SPI_2>,
> > >                         <&pd IMX_SC_R_SPI_3>,
> > >                         <&pd IMX_SC_R_UART_0>,
> > >                         <&pd IMX_SC_R_UART_1>,
> > >                         <&pd IMX_SC_R_UART_2>,
> > >                         <&pd IMX_SC_R_UART_3>; };
> > >
> > > For MX8QM, even if we have one more UART_4, then we still have to
> > > write all the same things again with an extra UART_4. It seems it's
> > > a bit violate our design that using a shared one and do incremental
> > > changes for
> > new SoCs.
> > > Do you think if this is acceptable to you?
> >
> > Yes, as it should be a one time thing to do per SoC.
> >
> 
> I found we may still can't use this new way after giving a try.
> One know issue is that it can't support clock parent setting well with this
> binding If merged all sub clocks into a single node.
> Hard to describe parent clocks for each clock within the same big array.
> 
> For example in MX8 ADMA SS, there're another LCD PLL which can be optional
> parent clocks to others peripherals.
> If we list them all in the same array, we can't describe LCD baud/pixel clock
> parents.
> dma_scu_clk: dma-scu-clock-controller {
>         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
>         #clock-cells = <1>;
>         clock-indices = <SC_R_ELCDIF_PLL IMX_SC_PM_CLK_PLL>,
>                         <SC_R_LCD_0 IMX_SC_PM_CLK_PER>,         /*
> lcd baud */
>                         <SC_R_LCD_0 IMX_SC_PM_CLK_SLV_BUS>,     /*
> Pixel Link */
>                         ...
>         clock-output-names = "lcdif_pll",
>                              "lcdif_baud_clk",
>                              "lcdif_pixel_clk",
>                                 ...
>         power-domains = <&pd IMX_SC_R_LCD_0>,
>                         <&pd IMX_SC_R_LCD_0>,
>                         <&pd IMX_SC_R_LCD_0>,
>                         ...
> };
> 
> And other peripherals might have different parents within the same array.
> 
> The old way does not have this issue because it's capable of configuring
> parents respectively for each sub clocks.
> /* SCU clocks */
> dma_scu_clk: clock-controller-scu-dma {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         lcd_pll: clock-scu@323 {
>                 reg = <323>;
>                 #clock-cells = <1>;
>                 clock-indices = <IMX_SC_PM_CLK_PLL>;
>                 clock-output-names = "lcd_pll";
>                 power-domains = <&pd IMX_SC_R_ELCDIF_PLL>;
>         };
> 
>         lcd0_clk: clock-scu@187 {
>                 reg = <187>;
>                 #clock-cells = <1>;
>                 /* parent clocks should match HW programing order */
>                 clocks = <&dummy_clk &dummy_clk &dummy_clk
> &dummy_clk &lcd_pll>;
>                 clock-indices = <IMX_SC_PM_CLK_PER>;
>                 clock-output-names = "lcd0_clk";
>                 power-domains = <&pd IMX_SC_R_LCD_0>;
>         };
>         ...
> };
> 
> I double checked other SS like Audio, DC, MIPI, PI which have the same issue.
> I really don't know if there will be a way out if using the one single node way.
> And I'm also a bit worrying whether it may cause more issues due to its losing
> of the flexibility and causes potential issues.
> 
> Do you think if we can still go back to the old way which is proposed In this
> patch set?
> As it can perfectly meet our requirements and also ease the driver
> implementation.
> 
> Hope you can help shed some lights as we're pending on it for a long time.
> 
> Regards
> Dong Aisheng
> 
> > > But if describe them per nodes, we do not have such issue.
> > >
> > > Anyway, please tell me your choice, then I will follow.
> > >
> > > BTW, I don't know how a tool can address this issue.
> >
> > I meant you could write one that understands the binding. It's a bit
> > more complicated having to parse and update properties compared to
> > adding or removing nodes, but it can still be done.
> >
> > Rob

Hi Rob,

Would you help check the new patch series I just sent?
[v3,00/11] clk: imx8: add new clock binding for better pm support
https://patchwork.kernel.org/cover/11046287/

The former solution you suggested seems can't work as it lost the ability
to set parents for each individual clocks within the same hardware subsystems.

And seems both Stephen & You may not quite like the per node clock in DT for SCU
clocks, In order to not stuck at here, I totally removed the SCU clock nodes in DT,
Instead, we still define those SCU clocks in driver but changed to two cells binding
which is more close to hardware (SW Clock IDs are removed) and can handle
above issues.

And we also need some tricks in driver to handle the required power domains
and Clock availability for different partition configuration, as well as 
clock state save& restore.

The patch series has been pending for quite a few months.
So it would be really appreciated if you can help review and provide some
advices.

Note: for DT patches which may help the understanding, please refer to:
[v2,00/15] arm64: dts: imx8: architecture improvement and adding imx8qm support
https://patchwork.kernel.org/cover/11046341/

Regards
Dong Aisheng

> From: Aisheng Dong
> Sent: Tuesday, June 4, 2019 12:50 AM
> Subject: RE: [PATCH V2 1/2] dt-bindings: firmware: imx-scu: new binding to
> parse clocks from device tree
> 
> Hi Rob,
> 
> Would you help check my reply and offer some suggestions?
> We're a bit lost on what to do and being blocked here for a long time which
> affects all the following MX8QM/QXP upstreaming works.
> 
> We really need your help to clarify how to move forward.
> If any more information need me to provide, feel free to let me know.
> 
> Thanks a lot in advance!
> 
> Regards
> Dong Aisheng
> 
> > From: Aisheng Dong
> > Sent: Wednesday, May 22, 2019 1:57 AM
> > Hi Rob,
> >
> > [...]
> >
> > > > > > For SCU based platforms, the resource availability (e.g.
> > > > > > device/clocks/power) are configurable by SCU firmware
> > > > > > according to the
> > > > > different SW execution partition configuration.
> > > > > > e.g. According to customer's requirements, we may allocate
> > > > > > some resources to M4 partition like some I2C, CAN, audio
> > > > > > resources which can't be
> > > > > accessed by A core.
> > > > > > And we may allocate even more for virtual machines running at
> > > > > > another CPU
> > > > > core.
> > > > > > Thus, defining all the clock sources (fixed) in device tree
> > > > > > for A core seems to be a little bit meaningless and it also
> > > > > > causes us hard to extend for a
> > > > > new SoC.
> > > > >
> > > > > I'm not suggesting that. It's really just re-arranging all the
> > > > > same data from a bunch of child nodes to a single node. Granted,
> > > > > it may be easier to add/delete nodes than add/delete elements
> > > > > from an array of property values, but really that's just a
> > > > > tooling problem
> > > > >
> > > >
> > > > Okay, understood.
> > > > So it seems we could still have a separate clock controller node
> > > > for each SS but merge all the same data of child nodes data into it.
> > > >
> > > > However, we still have one concern.
> > > > Taking MX8QXP DMA SS as example, with one node description, it may
> > > > be something like below:
> > > > dma_scu_clk: dma-scu-clock-controller {
> > > >         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> > > >         #clock-cells = <1>;
> > > >         clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ADC_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_CAN_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_FTM_1,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_1,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_2,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_I2C_3,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >
> <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0_PWM_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_1,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_2,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_SPI_3,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_0,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_1,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_2,
> > > IMX_SC_PM_CLK_PER)>,
> > > >                         <IMX_SCU_CLK_ID(IMX_SC_R_UART_3,
> > > IMX_SC_PM_CLK_PER)>;
> > > >         clock-output-names = "adc0_clk",
> > > >                              "can0_clk",
> > > >                              "ftm0_clk",
> > > >                              "ftm1_clk",
> > > >                              "i2c0_clk",
> > > >                              "i2c1_clk",
> > > >                              "i2c2_clk",
> > > >                              "i2c3_clk",
> > > >                              "lcd0_clk",
> > > >                              "lcd0_pwm0_clk",
> > > >                              "spi0_clk",
> > > >                              "spi1_clk",
> > > >                              "spi2_clk",
> > > >                              "spi3_clk",
> > > >                              "uart0_clk",
> > > >                              "uart1_clk",
> > > >                              "uart2_clk",
> > > >                              "uart3_clk";
> > > >         power-domains = <&pd IMX_SC_R_ADC_0>,
> > > >                         <&pd IMX_SC_R_CAN_0>,
> > > >                         <&pd IMX_SC_R_FTM_0>,
> > > >                         <&pd IMX_SC_R_FTM_1>,
> > > >                         <&pd IMX_SC_R_I2C_0>,
> > > >                         <&pd IMX_SC_R_I2C_1>,
> > > >                         <&pd IMX_SC_R_I2C_2>,
> > > >                         <&pd IMX_SC_R_I2C_3>,
> > > >                         <&pd IMX_SC_R_LCD_0>,
> > > >                         <&pd IMX_SC_R_LCD_0_PWM_0>,
> > > >                         <&pd IMX_SC_R_SPI_0>,
> > > >                         <&pd IMX_SC_R_SPI_1>,
> > > >                         <&pd IMX_SC_R_SPI_2>,
> > > >                         <&pd IMX_SC_R_SPI_3>,
> > > >                         <&pd IMX_SC_R_UART_0>,
> > > >                         <&pd IMX_SC_R_UART_1>,
> > > >                         <&pd IMX_SC_R_UART_2>,
> > > >                         <&pd IMX_SC_R_UART_3>; };
> > > >
> > > > For MX8QM, even if we have one more UART_4, then we still have to
> > > > write all the same things again with an extra UART_4. It seems
> > > > it's a bit violate our design that using a shared one and do
> > > > incremental changes for
> > > new SoCs.
> > > > Do you think if this is acceptable to you?
> > >
> > > Yes, as it should be a one time thing to do per SoC.
> > >
> >
> > I found we may still can't use this new way after giving a try.
> > One know issue is that it can't support clock parent setting well with
> > this binding If merged all sub clocks into a single node.
> > Hard to describe parent clocks for each clock within the same big array.
> >
> > For example in MX8 ADMA SS, there're another LCD PLL which can be
> > optional parent clocks to others peripherals.
> > If we list them all in the same array, we can't describe LCD
> > baud/pixel clock parents.
> > dma_scu_clk: dma-scu-clock-controller {
> >         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
> >         #clock-cells = <1>;
> >         clock-indices = <SC_R_ELCDIF_PLL IMX_SC_PM_CLK_PLL>,
> >                         <SC_R_LCD_0 IMX_SC_PM_CLK_PER>,
> /*
> > lcd baud */
> >                         <SC_R_LCD_0 IMX_SC_PM_CLK_SLV_BUS>,
> /*
> > Pixel Link */
> >                         ...
> >         clock-output-names = "lcdif_pll",
> >                              "lcdif_baud_clk",
> >                              "lcdif_pixel_clk",
> >                                 ...
> >         power-domains = <&pd IMX_SC_R_LCD_0>,
> >                         <&pd IMX_SC_R_LCD_0>,
> >                         <&pd IMX_SC_R_LCD_0>,
> >                         ...
> > };
> >
> > And other peripherals might have different parents within the same array.
> >
> > The old way does not have this issue because it's capable of
> > configuring parents respectively for each sub clocks.
> > /* SCU clocks */
> > dma_scu_clk: clock-controller-scu-dma {
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >
> >         lcd_pll: clock-scu@323 {
> >                 reg = <323>;
> >                 #clock-cells = <1>;
> >                 clock-indices = <IMX_SC_PM_CLK_PLL>;
> >                 clock-output-names = "lcd_pll";
> >                 power-domains = <&pd IMX_SC_R_ELCDIF_PLL>;
> >         };
> >
> >         lcd0_clk: clock-scu@187 {
> >                 reg = <187>;
> >                 #clock-cells = <1>;
> >                 /* parent clocks should match HW programing order */
> >                 clocks = <&dummy_clk &dummy_clk &dummy_clk
> &dummy_clk
> > &lcd_pll>;
> >                 clock-indices = <IMX_SC_PM_CLK_PER>;
> >                 clock-output-names = "lcd0_clk";
> >                 power-domains = <&pd IMX_SC_R_LCD_0>;
> >         };
> >         ...
> > };
> >
> > I double checked other SS like Audio, DC, MIPI, PI which have the same issue.
> > I really don't know if there will be a way out if using the one single node way.
> > And I'm also a bit worrying whether it may cause more issues due to
> > its losing of the flexibility and causes potential issues.
> >
> > Do you think if we can still go back to the old way which is proposed
> > In this patch set?
> > As it can perfectly meet our requirements and also ease the driver
> > implementation.
> >
> > Hope you can help shed some lights as we're pending on it for a long time.
> >
> > Regards
> > Dong Aisheng
> >
> > > > But if describe them per nodes, we do not have such issue.
> > > >
> > > > Anyway, please tell me your choice, then I will follow.
> > > >
> > > > BTW, I don't know how a tool can address this issue.
> > >
> > > I meant you could write one that understands the binding. It's a bit
> > > more complicated having to parse and update properties compared to
> > > adding or removing nodes, but it can still be done.
> > >
> > > Rob