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:

quoted

quoted

quoted

+- compatible:                Should be one of:
+                       "fsl,imx8qxp-lpcg"
+                       "fsl,imx8qm-lpcg" followed by

"fsl,imx8qxp-lpcg".

quoted

quoted

quoted

+- reg:                       Address and length of the register set.
+- #clock-cells:              Should be 1. One LPCG supports multiple

clocks.

quoted

quoted

quoted

+- clocks:            Input parent clocks phandle array for each clock.
+- bit-offset:                An integer array indicating the bit offset

for each clock.

quoted

quoted

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>

quoted

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?

quoted

quoted

+- hw-autogate:               Boolean array indicating whether

supports HW autogate for

quoted

quoted

quoted

+                     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.

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel