On Mon, Aug 12, 2019 at 02:41:55PM +0000, Aisheng Dong wrote:
> > 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
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>
