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