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
