Hi Rob, Could you maybe shed a little light on this? We're blocking here for the further work. Regards Dong Aisheng > From: Aisheng Dong > Sent: Tuesday, April 2, 2019 10:47 PM > > Hi Rob, > > > From: Aisheng Dong > > Sent: Wednesday, March 27, 2019 10:35 PM > > > From: Rob Herring [mailto:robh@xxxxxxxxxx] > > > Sent: Tuesday, March 26, 2019 9:47 PM On Thu, Feb 21, 2019 at > > > 06:03:43PM +0000, Aisheng Dong wrote: > > > > There's a few limitations on one cell clock binding (#clock-cells > > > > = > > > > <1>) that we have to define all clock IDs for device tree to reference. > > > > This may cause troubles if we want to use common clock IDs for > > > > multi platforms support when the clock of those platforms are mostly the > same. > > > > e.g. Current clock IDs name are defined with SS prefix. However > > > > the device may reside in different SS across CPUs, that means the > > > > SS prefix may not valid anymore for a new SoC. Furthermore, the > > > > device availability of those clocks may also vary a bit. > > > > > > > > For such situation, We formerly planned to add all new IDs for > > > > each SS and dynamically check availability for different SoC in driver. > > > > That can be done but that may involve a lot effort and may result > > > > in more changes and duplicated code in driver, also make device > > > > tree upstreaming hard which depends on Clock IDs. > > > > > > > > To relief this situation, we want to move the clock definition > > > > into device tree which can fully decouple the dependency of Clock > > > > ID definition from device tree. And no frequent changes required > > > > in clock driver > > > any more. > > > > > > > > Then we can use the existence of clock nodes in device tree to > > > > address the device and clock availability differences across different SoCs. > > > > > > > > For SCU clocks, only two params required, thus two new property > created: > > > > rsrc-id = <IMX_SC_R_UART_0>; > > > > clk-type = <IMX_SC_PM_CLK_PER>; > > > > > > > > And as we want to support clock set parent function, 'clocks' > > > > property is also used to pass all the possible input parents. > > > > > > > > Cc: Rob Herring <robh@xxxxxxxxxx> > > > > Cc: Stephen Boyd <sboyd@xxxxxxxxxx> > > > > Cc: Shawn Guo <shawnguo@xxxxxxxxxx> > > > > Cc: Sascha Hauer <kernel@xxxxxxxxxxxxxx> > > > > Cc: Michael Turquette <mturquette@xxxxxxxxxxxx> > > > > Cc: devicetree@xxxxxxxxxxxxxxx > > > > Signed-off-by: Dong Aisheng <aisheng.dong@xxxxxxx> > > > > --- > > > > .../devicetree/bindings/arm/freescale/fsl,scu.txt | 29 > > > ++++++++++++++++------ > > > > include/dt-bindings/firmware/imx/rsrc.h | 17 > > > +++++++++++++ > > > > 2 files changed, 38 insertions(+), 8 deletions(-) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > > > > index 72d481c..2816789 100644 > > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > > > > @@ -78,6 +78,19 @@ Required properties: > > > > "fsl,imx8qm-clock" > > > > "fsl,imx8qxp-clock" > > > > followed by "fsl,scu-clk" > > > > +- #clock-cells: Should be 0. > > > > +- rsrc-id: Resource ID associated with this clock > > > > +- clk-type: Type of this clock. > > > > + Refer to <include/dt-bindings/firmware/imx/rsrc.h> for > > > > + available clock types supported by SCU. > > > > > > Can't you just make these 2 values clock cells? I'm all for getting > > > rid of made up clock numbers. > > > > > > > Thanks for the agreement to remove clock IDs. > > > > The 2 values clock cell seems not the best approach for i.MX because > > it still needs to define all clocks in the driver which is exactly we > > want to avoid now due to some special HW characteristic: > > 1. clock resources may be allocated to different SW execution > > partition by firmware and A core may not have access rights for those > > clocks not belong to its partition. > > So we want to describe them in DT according to the partition configuration. > > > > 2. Each clock is associated with a different power domain which is > > better to be described in device tree. And clock state will be lost > > and need restore after power cycle of the domain. > > > > Based on above requirements, do you think we can do as below? > > > > Would you help check if this is okay to you? > > Regards > Dong Aisheng > > > //LSIO SS > > lsio_scu_clk: lsio-scu-clock-controller { > > compatible = "fsl,imx8qxp-clock", "fsl,scu-clk"; > > > > fspi0_clk: clock-fspi0{ > > #clock-cells = <0>; > > rsrc-id = <IMX_SC_R_FSPI_0>; > > clk-type = <IMX_SC_PM_CLK_PER>; > > power-domains = <&pd IMX_SC_R_FSPI_0>; > > }; > > > > fspi1_clk: clock-fspi1{ > > ... > > }; > > ... > > }; > > > > /* LPCG clocks */ > > lsio_lpcg_clk: lsio-lpcg-clock-controller { > > compatible = "fsl,imx8qxp-lpcg"; > > > > pwm0_lpcg: clock-controller@5d400000 { > > reg = <0x5d400000 0x10000>; > > #clock-cells = <1>; > > clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>, > > <&lsio_bus_clk>, <&pwm0_clk>; > > bit-offset = <0 4 16 20 24>; > > clock-output-names = "pwm0_lpcg_ipg_clk", > > "pwm0_lpcg_ipg_hf_clk", > > "pwm0_lpcg_ipg_s_clk", > > "pwm0_lpcg_ipg_slv_clk", > > "pwm0_lpcg_ipg_mstr_clk"; > > power-domains = <&pd IMX_SC_R_PWM_0>; > > status = "disabled"; > > }; > > > > pwm1_lpcg: clock-controller@5d410000 { > > ... > > } > > ... > > }; > > > > And for users, it could simply be: > > usdhc1: mmc@5b010000 { > > clocks = <&sdhc0_lpcg 1>, > > <&sdhc0_lpcg 0>, > > <&sdhc0_lpcg 2>; > > clock-names = "ipg", "per", "ahb"; > > assigned-clocks = <&sdhc0_clk>; > > assigned-clock-rates = <200000000>; > > .... > > }; > > > > Regards > > Dong Aisheng > > > > > > +- clock-output-names: Shall be the corresponding names of the outputs. > > > > + > > > > +Optional properties: > > > > +- clocks: Shall be the input parent clock(s) phandle for the clock. > > > > + For multiplexed clocks, the list order must match the > hardware > > > > + programming order. > > > > + > > > > +Legacy Clock binding (DEPRECATED): > > > > - #clock-cells: Should be 1. Contains the Clock ID value. > > > > - clocks: List of clock specifiers, must contain an entry for > > > > each required entry in clock-names @@ -129,6 +142,13 > > @@ > > > lsio_mu1: > > > > mailbox@5d1c0000 { > > > > #mbox-cells = <2>; > > > > }; > > > > > > > > +uart0_clk: uart0-clock-controller { > > > > + compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk"; > > > > + #clock-cells = <0>; > > > > + rsrc-id = <IMX_SC_R_UART_0>; > > > > + clk-type = <IMX_SC_PM_CLK_PER>; > > > > +}; > > > > + > > > > firmware { > > > > scu { > > > > compatible = "fsl,imx-scu"; > > > > @@ -143,11 +163,6 @@ firmware { > > > > &lsio_mu1 1 2 > > > > &lsio_mu1 1 3>; > > > > > > > > - clk: clk { > > > > - compatible = "fsl,imx8qxp-clk", "fsl,scu-clk"; > > > > - #clock-cells = <1>; > > > > - }; > > > > - > > > > iomuxc { > > > > compatible = "fsl,imx8qxp-iomuxc"; > > > > > > > > @@ -175,8 +190,6 @@ serial@5a060000 { > > > > ... > > > > pinctrl-names = "default"; > > > > pinctrl-0 = <&pinctrl_lpuart0>; > > > > - clocks = <&clk IMX8QXP_UART0_CLK>, > > > > - <&clk IMX8QXP_UART0_IPG_CLK>; > > > > - clock-names = "per", "ipg"; > > > > + clocks = <&uart0_clk>; > > > > power-domains = <&pd IMX_SC_R_UART_0>; }; diff --git > > > > a/include/dt-bindings/firmware/imx/rsrc.h > > > > b/include/dt-bindings/firmware/imx/rsrc.h > > > > index 4481f2d..f650fc3 100644 > > > > --- a/include/dt-bindings/firmware/imx/rsrc.h > > > > +++ b/include/dt-bindings/firmware/imx/rsrc.h > > > > @@ -556,4 +556,21 @@ > > > > #define IMX_SC_R_VPU 540 > > > > #define IMX_SC_R_LAST 541 > > > > > > > > +/* > > > > + * Defines for SC PM CLK > > > > + */ > > > > +#define IMX_SC_PM_CLK_SLV_BUS 0 /* Slave bus clock */ > > > > +#define IMX_SC_PM_CLK_MST_BUS 1 /* Master bus clock */ > > > > +#define IMX_SC_PM_CLK_PER 2 /* Peripheral clock */ > > > > +#define IMX_SC_PM_CLK_PHY 3 /* Phy clock */ > > > > +#define IMX_SC_PM_CLK_MISC 4 /* Misc clock */ > > > > +#define IMX_SC_PM_CLK_MISC0 0 /* Misc 0 clock */ > > > > +#define IMX_SC_PM_CLK_MISC1 1 /* Misc 1 clock */ > > > > +#define IMX_SC_PM_CLK_MISC2 2 /* Misc 2 clock */ > > > > +#define IMX_SC_PM_CLK_MISC3 3 /* Misc 3 clock */ > > > > +#define IMX_SC_PM_CLK_MISC4 4 /* Misc 4 clock */ > > > > +#define IMX_SC_PM_CLK_CPU 2 /* CPU clock */ > > > > +#define IMX_SC_PM_CLK_PLL 4 /* PLL */ > > > > +#define IMX_SC_PM_CLK_BYPASS 4 /* Bypass clock */ > > > > + > > > > #endif /* __DT_BINDINGS_RSCRC_IMX_H */ > > > > -- > > > > 2.7.4 > > > >
