[v3,02/11] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree
diff mbox series

Message ID 1563289265-10977-3-git-send-email-aisheng.dong@nxp.com
State Changes Requested
Headers show
Series
  • [v3,01/11] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree
Related show

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 1 warnings, 52 lines checked"

Commit Message

Dong Aisheng July 16, 2019, 3 p.m. UTC
MX8QM and MX8QXP LPCG Clocks are mostly the same except they may reside
in different subsystems across CPUs and also vary a bit on the availability.

Same as SCU clock, we want to move the clock definition into device tree
which can fully decouple the dependency of Clock ID definition from device
tree and make us be able to write a fully generic lpcg clock driver.

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

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:
v2->v3:
 * no changes
v1->v2:
 * Update example
 * Add power domain property
---
 .../devicetree/bindings/clock/imx8qxp-lpcg.txt     | 34 ++++++++++++++++++----
 1 file changed, 28 insertions(+), 6 deletions(-)

Comments

Shawn Guo Aug. 3, 2019, 1:50 p.m. UTC | #1
On Tue, Jul 16, 2019 at 11:00:56PM +0800, Dong Aisheng wrote:
> MX8QM and MX8QXP LPCG Clocks are mostly the same except they may reside
> in different subsystems across CPUs and also vary a bit on the availability.
> 
> Same as SCU clock, we want to move the clock definition into device tree
> which can fully decouple the dependency of Clock ID definition from device
> tree and make us be able to write a fully generic lpcg clock driver.
> 
> And we can also use the existence of clock nodes in device tree to address
> the device and clock availability differences across different SoCs.
> 
> 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:
> v2->v3:
>  * no changes
> v1->v2:
>  * Update example
>  * Add power domain property
> ---
>  .../devicetree/bindings/clock/imx8qxp-lpcg.txt     | 34 ++++++++++++++++++----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> index 965cfa4..6fc2fd8 100644
> --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> @@ -11,6 +11,21 @@ enabled by these control bits, it might still not be running based
>  on the base resource.
>  
>  Required properties:
> +- compatible:		Should be one of:
> +			  "fsl,imx8qxp-lpcg"
> +			  "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg".
> +- reg:			Address and length of the register set.
> +- #clock-cells:		Should be 1. One LPCG supports multiple clocks.
> +- clocks:		Input parent clocks phandle array for each clock.
> +- bit-offset:		An integer array indicating the bit offset for each clock.

I guess that the driver should be able to figure bit offset from
'clock-indices' property.

> +- hw-autogate:		Boolean array indicating whether supports HW autogate for
> +			each clock.

Not sure why it needs to be a property in DT.  Or asking it different
way, when it should be true and when false?

Shawn

> +- clock-output-names:	Shall be the corresponding names of the outputs.
> +			NOTE this property must be specified in the same order
> +			as the clock bit-offset and hw-autogate property.
> +- power-domains:	Should contain the power domain used by this clock.
> +
> +Legacy binding (DEPRECATED):
>  - compatible:	Should be one of:
>  		  "fsl,imx8qxp-lpcg-adma",
>  		  "fsl,imx8qxp-lpcg-conn",
> @@ -33,10 +48,17 @@ Examples:
>  
>  #include <dt-bindings/clock/imx8qxp-clock.h>
>  
> -conn_lpcg: clock-controller@5b200000 {
> -	compatible = "fsl,imx8qxp-lpcg-conn";
> -	reg = <0x5b200000 0xb0000>;
> +sdhc0_lpcg: clock-controller@5b200000 {
> +	compatible = "fsl,imx8qxp-lpcg";
> +	reg = <0x5b200000 0x10000>;
>  	#clock-cells = <1>;
> +	clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
> +		 <&conn_ipg_clk>, <&conn_axi_clk>;
> +	bit-offset = <0 16 20>;
> +	clock-output-names = "sdhc0_lpcg_per_clk",
> +			     "sdhc0_lpcg_ipg_clk",
> +			     "sdhc0_lpcg_ahb_clk";
> +	power-domains = <&pd IMX_SC_R_SDHC_0>;
>  };
>  
>  usdhc1: mmc@5b010000 {
> @@ -44,8 +66,8 @@ usdhc1: mmc@5b010000 {
>  	interrupt-parent = <&gic>;
>  	interrupts = <GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
>  	reg = <0x5b010000 0x10000>;
> -	clocks = <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_IPG_CLK>,
> -		 <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_PER_CLK>,
> -		 <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_HCLK>;
> +	clocks = <&sdhc0_lpcg 1>,
> +		 <&sdhc0_lpcg 0>,
> +		 <&sdhc0_lpcg 2>;
>  	clock-names = "ipg", "per", "ahb";
>  };
> -- 
> 2.7.4
>
Dong Aisheng Aug. 5, 2019, 3:27 a.m. UTC | #2
On Sun, Aug 4, 2019 at 11:45 AM Shawn Guo <shawnguo@kernel.org> wrote:
>
> On Tue, Jul 16, 2019 at 11:00:56PM +0800, Dong Aisheng wrote:
> > MX8QM and MX8QXP LPCG Clocks are mostly the same except they may reside
> > in different subsystems across CPUs and also vary a bit on the availability.
> >
> > Same as SCU clock, we want to move the clock definition into device tree
> > which can fully decouple the dependency of Clock ID definition from device
> > tree and make us be able to write a fully generic lpcg clock driver.
> >
> > And we can also use the existence of clock nodes in device tree to address
> > the device and clock availability differences across different SoCs.
> >
> > 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:
> > v2->v3:
> >  * no changes
> > v1->v2:
> >  * Update example
> >  * Add power domain property
> > ---
> >  .../devicetree/bindings/clock/imx8qxp-lpcg.txt     | 34 ++++++++++++++++++----
> >  1 file changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > index 965cfa4..6fc2fd8 100644
> > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > @@ -11,6 +11,21 @@ enabled by these control bits, it might still not be running based
> >  on the base resource.
> >
> >  Required properties:
> > +- compatible:                Should be one of:
> > +                       "fsl,imx8qxp-lpcg"
> > +                       "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg".
> > +- reg:                       Address and length of the register set.
> > +- #clock-cells:              Should be 1. One LPCG supports multiple clocks.
> > +- clocks:            Input parent clocks phandle array for each clock.
> > +- bit-offset:                An integer array indicating the bit offset for each clock.
>
> I guess that the driver should be able to figure bit offset from
> 'clock-indices' property.
>

Yes, it can be done in theory.
Then the binding may look like:
sdhc0_lpcg: clock-controller@5b200000 {
        ...
        #clock-cells = <1>;
        clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
                 <&conn_ipg_clk>, <&conn_axi_clk>;
        clock-indices = <0>, <16>, <20>;
        clock-output-names = "sdhc0_lpcg_per_clk",
                             "sdhc0_lpcg_ipg_clk",
                             "sdhc0_lpcg_ahb_clk";
        power-domains = <&pd IMX_SC_R_SDHC_0>;
};

usdhc1: mmc@5b010000 {
        ...
        clocks = <&sdhc0_lpcg 16>,
                 <&sdhc0_lpcg 0>,
                 <&sdhc0_lpcg 20>;
        clock-names = "ipg", "per", "ahb";
};

However, after trying, i found  one limitation if using clock-indices
that users have to do a secondary search for the indices value from clock names
which is not very friendly.

Formerly from the clock output names, user can easily get the clock
index as they're
in fixed orders as output names, so very easily to use.
e.g.
clocks = <&sdhc0_lpcg 1>,
         <&sdhc0_lpcg 0>,
         <&sdhc0_lpcg 2>;

If using clock-indices, users have no way to know it's clock index
from clock output names order
unless they do a secondary search from the clock-indice array accordingly.
For example, for "sdhc0_lpcg_ahb_clk", user can easily know its
reference is <&sdhc0_lpcg 2>.
But if using clock-indice, we need search clock-indices array to find
its reference
becomes <&sdhc0_lpcg 20>. So this seems like a drawback if using clock-indices.

Therefore, personally i'm still a bit intend to the original way which
is more simple and
straightforward from user point of view, unless there's a strong
objections on define another
vendor private property.

Shawn,
How do you think?
Should we enforce the complexity to users?

> > +- hw-autogate:               Boolean array indicating whether supports HW autogate for
> > +                     each clock.
>
> Not sure why it needs to be a property in DT.  Or asking it different
> way, when it should be true and when false?
>

It is one LPCG feature.
For some specific device LPCGs, it may support clock auto gating. (depends on
IP's capability. e.g. uSDHC).
So we define this feature in DT as well in case if user may want to
use it in the future.

But AFAIK, there's still no one using it. Most drivers reply on runtime PM to do
clock management. Did not use LPCG auto gate off feature.
But the current LPCG driver API does support this parameter.

If you think it's unnecessary to define it in DT as there're still no
users, i can remove it
and disabling autogate in driver by default.

Regards
Aisheng

> Shawn
>
> > +- clock-output-names:        Shall be the corresponding names of the outputs.
> > +                     NOTE this property must be specified in the same order
> > +                     as the clock bit-offset and hw-autogate property.
> > +- power-domains:     Should contain the power domain used by this clock.
> > +
> > +Legacy binding (DEPRECATED):
> >  - compatible:        Should be one of:
> >                 "fsl,imx8qxp-lpcg-adma",
> >                 "fsl,imx8qxp-lpcg-conn",
> > @@ -33,10 +48,17 @@ Examples:
> >
> >  #include <dt-bindings/clock/imx8qxp-clock.h>
> >
> > -conn_lpcg: clock-controller@5b200000 {
> > -     compatible = "fsl,imx8qxp-lpcg-conn";
> > -     reg = <0x5b200000 0xb0000>;
> > +sdhc0_lpcg: clock-controller@5b200000 {
> > +     compatible = "fsl,imx8qxp-lpcg";
> > +     reg = <0x5b200000 0x10000>;
> >       #clock-cells = <1>;
> > +     clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
> > +              <&conn_ipg_clk>, <&conn_axi_clk>;
> > +     bit-offset = <0 16 20>;
> > +     clock-output-names = "sdhc0_lpcg_per_clk",
> > +                          "sdhc0_lpcg_ipg_clk",
> > +                          "sdhc0_lpcg_ahb_clk";
> > +     power-domains = <&pd IMX_SC_R_SDHC_0>;
> >  };
> >
> >  usdhc1: mmc@5b010000 {
> > @@ -44,8 +66,8 @@ usdhc1: mmc@5b010000 {
> >       interrupt-parent = <&gic>;
> >       interrupts = <GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
> >       reg = <0x5b010000 0x10000>;
> > -     clocks = <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_IPG_CLK>,
> > -              <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_PER_CLK>,
> > -              <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_HCLK>;
> > +     clocks = <&sdhc0_lpcg 1>,
> > +              <&sdhc0_lpcg 0>,
> > +              <&sdhc0_lpcg 2>;
> >       clock-names = "ipg", "per", "ahb";
> >  };
> > --
> > 2.7.4
> >
Shawn Guo Aug. 12, 2019, 1 p.m. UTC | #3
On Mon, Aug 05, 2019 at 11:27:20AM +0800, Dong Aisheng wrote:
> > > +- compatible:                Should be one of:
> > > +                       "fsl,imx8qxp-lpcg"
> > > +                       "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg".
> > > +- reg:                       Address and length of the register set.
> > > +- #clock-cells:              Should be 1. One LPCG supports multiple clocks.
> > > +- clocks:            Input parent clocks phandle array for each clock.
> > > +- bit-offset:                An integer array indicating the bit offset for each clock.
> >
> > I guess that the driver should be able to figure bit offset from
> > 'clock-indices' property.
> >
> 
> Yes, it can be done in theory.
> Then the binding may look like:
> sdhc0_lpcg: clock-controller@5b200000 {
>         ...
>         #clock-cells = <1>;
>         clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
>                  <&conn_ipg_clk>, <&conn_axi_clk>;
>         clock-indices = <0>, <16>, <20>;
>         clock-output-names = "sdhc0_lpcg_per_clk",
>                              "sdhc0_lpcg_ipg_clk",
>                              "sdhc0_lpcg_ahb_clk";
>         power-domains = <&pd IMX_SC_R_SDHC_0>;
> };
> 
> usdhc1: mmc@5b010000 {
>         ...
>         clocks = <&sdhc0_lpcg 16>,
>                  <&sdhc0_lpcg 0>,
>                  <&sdhc0_lpcg 20>;
>         clock-names = "ipg", "per", "ahb";
> };
> 
> However, after trying, i found  one limitation if using clock-indices
> that users have to do a secondary search for the indices value from clock names
> which is not very friendly.
> 
> Formerly from the clock output names, user can easily get the clock
> index as they're
> in fixed orders as output names, so very easily to use.
> e.g.
> clocks = <&sdhc0_lpcg 1>,
>          <&sdhc0_lpcg 0>,
>          <&sdhc0_lpcg 2>;
> 
> If using clock-indices, users have no way to know it's clock index
> from clock output names order
> unless they do a secondary search from the clock-indice array accordingly.
> For example, for "sdhc0_lpcg_ahb_clk", user can easily know its
> reference is <&sdhc0_lpcg 2>.
> But if using clock-indice, we need search clock-indices array to find
> its reference
> becomes <&sdhc0_lpcg 20>. So this seems like a drawback if using clock-indices.

Shouldn't we have constant macro defined for those numbers, so that both
'clock-indices' and 'clocks' of client device can use?

> 
> Therefore, personally i'm still a bit intend to the original way which
> is more simple and
> straightforward from user point of view, unless there's a strong
> objections on define another
> vendor private property.
> 
> Shawn,
> How do you think?
> Should we enforce the complexity to users?
> 
> > > +- hw-autogate:               Boolean array indicating whether supports HW autogate for
> > > +                     each clock.
> >
> > Not sure why it needs to be a property in DT.  Or asking it different
> > way, when it should be true and when false?
> >
> 
> It is one LPCG feature.
> For some specific device LPCGs, it may support clock auto gating. (depends on
> IP's capability. e.g. uSDHC).
> So we define this feature in DT as well in case if user may want to
> use it in the future.
> 
> But AFAIK, there's still no one using it. Most drivers reply on runtime PM to do
> clock management. Did not use LPCG auto gate off feature.
> But the current LPCG driver API does support this parameter.
> 
> If you think it's unnecessary to define it in DT as there're still no
> users, i can remove it
> and disabling autogate in driver by default.

I would suggest to drop it then.

Shawn
Dong Aisheng Aug. 12, 2019, 2:41 p.m. UTC | #4
> From: Shawn Guo <shawnguo@kernel.org>
> Sent: Monday, August 12, 2019 9:01 PM 
> On Mon, Aug 05, 2019 at 11:27:20AM +0800, Dong Aisheng wrote:
> > > > +- compatible:                Should be one of:
> > > > +                       "fsl,imx8qxp-lpcg"
> > > > +                       "fsl,imx8qm-lpcg" followed by
> "fsl,imx8qxp-lpcg".
> > > > +- reg:                       Address and length of the register set.
> > > > +- #clock-cells:              Should be 1. One LPCG supports multiple
> clocks.
> > > > +- clocks:            Input parent clocks phandle array for each clock.
> > > > +- bit-offset:                An integer array indicating the bit offset
> for each clock.
> > >
> > > I guess that the driver should be able to figure bit offset from
> > > 'clock-indices' property.
> > >
> >
> > Yes, it can be done in theory.
> > Then the binding may look like:
> > sdhc0_lpcg: clock-controller@5b200000 {
> >         ...
> >         #clock-cells = <1>;
> >         clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
> >                  <&conn_ipg_clk>, <&conn_axi_clk>;
> >         clock-indices = <0>, <16>, <20>;
> >         clock-output-names = "sdhc0_lpcg_per_clk",
> >                              "sdhc0_lpcg_ipg_clk",
> >                              "sdhc0_lpcg_ahb_clk";
> >         power-domains = <&pd IMX_SC_R_SDHC_0>; };
> >
> > usdhc1: mmc@5b010000 {
> >         ...
> >         clocks = <&sdhc0_lpcg 16>,
> >                  <&sdhc0_lpcg 0>,
> >                  <&sdhc0_lpcg 20>;
> >         clock-names = "ipg", "per", "ahb"; };
> >
> > However, after trying, i found  one limitation if using clock-indices
> > that users have to do a secondary search for the indices value from
> > clock names which is not very friendly.
> >
> > Formerly from the clock output names, user can easily get the clock
> > index as they're in fixed orders as output names, so very easily to
> > use.
> > e.g.
> > clocks = <&sdhc0_lpcg 1>,
> >          <&sdhc0_lpcg 0>,
> >          <&sdhc0_lpcg 2>;
> >
> > If using clock-indices, users have no way to know it's clock index
> > from clock output names order unless they do a secondary search from
> > the clock-indice array accordingly.
> > For example, for "sdhc0_lpcg_ahb_clk", user can easily know its
> > reference is <&sdhc0_lpcg 2>.
> > But if using clock-indice, we need search clock-indices array to find
> > its reference becomes <&sdhc0_lpcg 20>. So this seems like a drawback
> > if using clock-indices.
> 
> Shouldn't we have constant macro defined for those numbers, so that both
> 'clock-indices' and 'clocks' of client device can use?
> 

I think we can do it.
Does below one look ok to you?
#define IMX_LPCG_ CLK_0	0
#define IMX_LPCG_ CLK_1	4
#define IMX_LPCG_ CLK_2	8
#define IMX_LPCG_ CLK_3	12
#define IMX_LPCG_ CLK_4	16
#define IMX_LPCG_ CLK_5	20
#define IMX_LPCG_ CLK_6	24
#define IMX_LPCG_ CLK_7	28

The usage will look like:
<&sdhc0_lpcg IMX_LPCG_CLK_5>

> >
> > Therefore, personally i'm still a bit intend to the original way which
> > is more simple and straightforward from user point of view, unless
> > there's a strong objections on define another vendor private property.
> >
> > Shawn,
> > How do you think?
> > Should we enforce the complexity to users?
> >
> > > > +- hw-autogate:               Boolean array indicating whether
> supports HW autogate for
> > > > +                     each clock.
> > >
> > > Not sure why it needs to be a property in DT.  Or asking it
> > > different way, when it should be true and when false?
> > >
> >
> > It is one LPCG feature.
> > For some specific device LPCGs, it may support clock auto gating.
> > (depends on IP's capability. e.g. uSDHC).
> > So we define this feature in DT as well in case if user may want to
> > use it in the future.
> >
> > But AFAIK, there's still no one using it. Most drivers reply on
> > runtime PM to do clock management. Did not use LPCG auto gate off
> feature.
> > But the current LPCG driver API does support this parameter.
> >
> > If you think it's unnecessary to define it in DT as there're still no
> > users, i can remove it and disabling autogate in driver by default.
> 
> I would suggest to drop it then.
> 

Got it.

Regards
Aisheng

> Shawn
Shawn Guo Aug. 12, 2019, 3:54 p.m. UTC | #5
On Mon, Aug 12, 2019 at 02:41:55PM +0000, Aisheng Dong wrote:
> > From: Shawn Guo <shawnguo@kernel.org>
> > Sent: Monday, August 12, 2019 9:01 PM 
> > On Mon, Aug 05, 2019 at 11:27:20AM +0800, Dong Aisheng wrote:
> > > > > +- compatible:                Should be one of:
> > > > > +                       "fsl,imx8qxp-lpcg"
> > > > > +                       "fsl,imx8qm-lpcg" followed by
> > "fsl,imx8qxp-lpcg".
> > > > > +- reg:                       Address and length of the register set.
> > > > > +- #clock-cells:              Should be 1. One LPCG supports multiple
> > clocks.
> > > > > +- clocks:            Input parent clocks phandle array for each clock.
> > > > > +- bit-offset:                An integer array indicating the bit offset
> > for each clock.
> > > >
> > > > I guess that the driver should be able to figure bit offset from
> > > > 'clock-indices' property.
> > > >
> > >
> > > Yes, it can be done in theory.
> > > Then the binding may look like:
> > > sdhc0_lpcg: clock-controller@5b200000 {
> > >         ...
> > >         #clock-cells = <1>;
> > >         clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
> > >                  <&conn_ipg_clk>, <&conn_axi_clk>;
> > >         clock-indices = <0>, <16>, <20>;
> > >         clock-output-names = "sdhc0_lpcg_per_clk",
> > >                              "sdhc0_lpcg_ipg_clk",
> > >                              "sdhc0_lpcg_ahb_clk";
> > >         power-domains = <&pd IMX_SC_R_SDHC_0>; };
> > >
> > > usdhc1: mmc@5b010000 {
> > >         ...
> > >         clocks = <&sdhc0_lpcg 16>,
> > >                  <&sdhc0_lpcg 0>,
> > >                  <&sdhc0_lpcg 20>;
> > >         clock-names = "ipg", "per", "ahb"; };
> > >
> > > However, after trying, i found  one limitation if using clock-indices
> > > that users have to do a secondary search for the indices value from
> > > clock names which is not very friendly.
> > >
> > > Formerly from the clock output names, user can easily get the clock
> > > index as they're in fixed orders as output names, so very easily to
> > > use.
> > > e.g.
> > > clocks = <&sdhc0_lpcg 1>,
> > >          <&sdhc0_lpcg 0>,
> > >          <&sdhc0_lpcg 2>;
> > >
> > > If using clock-indices, users have no way to know it's clock index
> > > from clock output names order unless they do a secondary search from
> > > the clock-indice array accordingly.
> > > For example, for "sdhc0_lpcg_ahb_clk", user can easily know its
> > > reference is <&sdhc0_lpcg 2>.
> > > But if using clock-indice, we need search clock-indices array to find
> > > its reference becomes <&sdhc0_lpcg 20>. So this seems like a drawback
> > > if using clock-indices.
> > 
> > Shouldn't we have constant macro defined for those numbers, so that both
> > 'clock-indices' and 'clocks' of client device can use?
> > 
> 
> I think we can do it.
> Does below one look ok to you?
> #define IMX_LPCG_ CLK_0	0
> #define IMX_LPCG_ CLK_1	4
> #define IMX_LPCG_ CLK_2	8
> #define IMX_LPCG_ CLK_3	12
> #define IMX_LPCG_ CLK_4	16
> #define IMX_LPCG_ CLK_5	20
> #define IMX_LPCG_ CLK_6	24
> #define IMX_LPCG_ CLK_7	28

Looks fine to me, except the space in the middle of macro name, which
compiler will complain anyway :)

Shawn

> 
> The usage will look like:
> <&sdhc0_lpcg IMX_LPCG_CLK_5>
Daniel Baluta Aug. 19, 2019, 12:42 p.m. UTC | #6
On Tue, 2019-07-16 at 23:00 +0800, Dong Aisheng wrote:
> MX8QM and MX8QXP LPCG Clocks are mostly the same except they may
> reside
> in different subsystems across CPUs and also vary a bit on the
> availability.
> 
> Same as SCU clock, we want to move the clock definition into device
> tree
> which can fully decouple the dependency of Clock ID definition from
> device
> tree and make us be able to write a fully generic lpcg clock driver.
> 
> And we can also use the existence of clock nodes in device tree to
> address
> the device and clock availability differences across different SoCs.
> 
> 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:
> v2->v3:
>  * no changes
> v1->v2:
>  * Update example
>  * Add power domain property
> ---
>  .../devicetree/bindings/clock/imx8qxp-lpcg.txt     | 34
> ++++++++++++++++++----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt 
> b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> index 965cfa4..6fc2fd8 100644
> --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> @@ -11,6 +11,21 @@ enabled by these control bits, it might still not
> be running based
>  on the base resource.
>  
>  Required properties:
> +- compatible:		Should be one of:
> +			  "fsl,imx8qxp-lpcg"
> +			  "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-
> lpcg".
> +- reg:			Address and length of the register set.
> +- #clock-cells:		Should be 1. One LPCG supports multiple
> clocks.
> +- clocks:		Input parent clocks phandle array for each
> clock.
> +- bit-offset:		An integer array indicating the bit
> offset for each clock.
> +- hw-autogate:		Boolean array indicating whether
> supports HW autogate for
> +			each clock.
> +- clock-output-names:	Shall be the corresponding names of the
> outputs.
> +			NOTE this property must be specified in the
> same order
> +			as the clock bit-offset and hw-autogate
> property.
> +- power-domains:	Should contain the power domain used by this
> clock.
> +
> +Legacy binding (DEPRECATED):
>  - compatible:	Should be one of:
>  		  "fsl,imx8qxp-lpcg-adma",
>  		  "fsl,imx8qxp-lpcg-conn",
> @@ -33,10 +48,17 @@ Examples:
>  
>  #include <dt-bindings/clock/imx8qxp-clock.h>
>  
> -conn_lpcg: clock-controller@5b200000 {
> -	compatible = "fsl,imx8qxp-lpcg-conn";
> -	reg = <0x5b200000 0xb0000>;
> +sdhc0_lpcg: clock-controller@5b200000 {
> +	compatible = "fsl,imx8qxp-lpcg";
> +	reg = <0x5b200000 0x10000>;
>  	#clock-cells = <1>;
> +	clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
> +		 <&conn_ipg_clk>, <&conn_axi_clk>;
> +	bit-offset = <0 16 20>;
> +	clock-output-names = "sdhc0_lpcg_per_clk",
> +			     "sdhc0_lpcg_ipg_clk",
> +			     "sdhc0_lpcg_ahb_clk";
> +	power-domains = <&pd IMX_SC_R_SDHC_0>;
>  };
>  
>  usdhc1: mmc@5b010000 {
> @@ -44,8 +66,8 @@ usdhc1: mmc@5b010000 {
>  	interrupt-parent = <&gic>;
>  	interrupts = <GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
>  	reg = <0x5b010000 0x10000>;
> -	clocks = <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_IPG_CLK>,
> -		 <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_PER_CLK>,
> -		 <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_HCLK>;
> +	clocks = <&sdhc0_lpcg 1>,
> +		 <&sdhc0_lpcg 0>,
> +		 <&sdhc0_lpcg 2>;

Is it possible to replace magic constants 1, 0, 2 with some meaningful
constants?

Are they the same with: IMX_SC_PM_CLK_PER, etc?

>  	clock-names = "ipg", "per", "ahb";
>  };

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
index 965cfa4..6fc2fd8 100644
--- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
+++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
@@ -11,6 +11,21 @@  enabled by these control bits, it might still not be running based
 on the base resource.
 
 Required properties:
+- compatible:		Should be one of:
+			  "fsl,imx8qxp-lpcg"
+			  "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg".
+- reg:			Address and length of the register set.
+- #clock-cells:		Should be 1. One LPCG supports multiple clocks.
+- clocks:		Input parent clocks phandle array for each clock.
+- bit-offset:		An integer array indicating the bit offset for each clock.
+- hw-autogate:		Boolean array indicating whether supports HW autogate for
+			each clock.
+- clock-output-names:	Shall be the corresponding names of the outputs.
+			NOTE this property must be specified in the same order
+			as the clock bit-offset and hw-autogate property.
+- power-domains:	Should contain the power domain used by this clock.
+
+Legacy binding (DEPRECATED):
 - compatible:	Should be one of:
 		  "fsl,imx8qxp-lpcg-adma",
 		  "fsl,imx8qxp-lpcg-conn",
@@ -33,10 +48,17 @@  Examples:
 
 #include <dt-bindings/clock/imx8qxp-clock.h>
 
-conn_lpcg: clock-controller@5b200000 {
-	compatible = "fsl,imx8qxp-lpcg-conn";
-	reg = <0x5b200000 0xb0000>;
+sdhc0_lpcg: clock-controller@5b200000 {
+	compatible = "fsl,imx8qxp-lpcg";
+	reg = <0x5b200000 0x10000>;
 	#clock-cells = <1>;
+	clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
+		 <&conn_ipg_clk>, <&conn_axi_clk>;
+	bit-offset = <0 16 20>;
+	clock-output-names = "sdhc0_lpcg_per_clk",
+			     "sdhc0_lpcg_ipg_clk",
+			     "sdhc0_lpcg_ahb_clk";
+	power-domains = <&pd IMX_SC_R_SDHC_0>;
 };
 
 usdhc1: mmc@5b010000 {
@@ -44,8 +66,8 @@  usdhc1: mmc@5b010000 {
 	interrupt-parent = <&gic>;
 	interrupts = <GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
 	reg = <0x5b010000 0x10000>;
-	clocks = <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_IPG_CLK>,
-		 <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_PER_CLK>,
-		 <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_HCLK>;
+	clocks = <&sdhc0_lpcg 1>,
+		 <&sdhc0_lpcg 0>,
+		 <&sdhc0_lpcg 2>;
 	clock-names = "ipg", "per", "ahb";
 };