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

On Mon, Aug 05, 2019 at 11:27:20AM +0800, Dong Aisheng wrote:

quoted

quoted

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

quoted

quoted

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