> From: Shawn Guo <shawnguo@xxxxxxxxxx>
> 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
