> From: Rob Herring [mailto:robh+dt@xxxxxxxxxx] > Sent: Friday, May 3, 2019 10:53 PM > Subject: Re: [PATCH V2 1/2] dt-bindings: firmware: imx-scu: new binding to > parse clocks from device tree > > On Thu, May 2, 2019 at 8:34 PM Aisheng Dong <aisheng.dong@xxxxxxx> > wrote: > > > There's a few limitations on the original 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. This can make us write a full generic > > clock driver for SCU based SoCs. No more frequent changes needed in > > clock driver any more. > > > > In the meanwhile, we can also 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. The first one is resource id > > which is encoded in reg property and the second is clock type index > > which is encoded in generic clock-indices property they're not continuously. > > > > And as we also want to support clock set parent function, 'clocks' > > property is also used to pass all the possible input parents. > > > > Cc: Rob Herring <robh+dt@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> > > --- > > ChangeLog: > > v1->v2: > > * changed to one cell binding inspired by arm,scpi.txt > > Documentation/devicetree/bindings/arm/arm,scpi.txt > > Resource ID is encoded in 'reg' property. > > Clock type is encoded in generic clock-indices property. > > Then we don't have to search all the DT nodes to fetch > > those two value to construct clocks which is relatively > > low efficiency. > > * Add required power-domain property as well. > > --- > > .../devicetree/bindings/arm/freescale/fsl,scu.txt | 45 > ++++++++++++++++++---- > > include/dt-bindings/firmware/imx/rsrc.h | 17 ++++++++ > > 2 files changed, 54 insertions(+), 8 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > > index 5d7dbab..2f46e89 100644 > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > > @@ -89,6 +89,27 @@ Required properties: > > "fsl,imx8qm-clock" > > "fsl,imx8qxp-clock" > > followed by "fsl,scu-clk" > > +- #address-cells: Should be 1. > > +- #size-cells: Should be 0. > > + > > +Sub nodes are required to represent all available SCU clocks within > > +this hardware subsystem and the following properties are needed: > > + > > +- reg: Should contain the Resource ID of this SCU clock. > > +- #clock-cells: Should be 1. > > +- clock-indices: Index of all clock types supported by this SCU clock. > > + The order should match the clock-output-names > array. > > + Refer to > <include/dt-bindings/firmware/imx/rsrc.h> for > > + available clock types supported by SCU. > > I would have expected the clock cell to contain the Resource ID. > > Also, this still has one clock per node which you should avoid unless there's > only a small number of clocks (say ~20). Move this all to a single node with the > list of clock IDs in clock-indices and other properties like power-domains can > match up with clock-indices. IOW, both should have the same length (in > elements). > Do you mean something like this? #define IMX_SCU_CLK_ID(rsrc, type) (type << 16 | rsrc) scu_clk: scu-clock-controller { compatible = "fsl,imx8qxp-scu-clk", "fsl,scu-clk"; #clock-cells = <1>; clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_PER)>, <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_BYPASS)>, <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_MISC0)>, <IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>, ... clock-output-names = "enet0_clk", "enet0_bypass_clk", "enet0_rgmii_clk", "uart0_clk", ... power-domains = <&pd IMX_SC_R_ENET_0>, <&pd IMX_SC_R_ENET_0>, <&pd IMX_SC_R_ENET_0>, <&pd IMX_SC_R_UART_0>, ... }; serial@5a060000 { ... clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>; power-domains = <&pd IMX_SC_R_UART_0>; }; I wonder moving all clock resources into a single clock controller node may result in losing the configuration granularity of individual clocks from device tree. For SCU based platforms, the resource availability (e.g. device/clocks/power) are configurable by SCU firmware according to the different SW execution partition configuration. e.g. According to customer's requirements, we may allocate some resources to M4 partition like some I2C, CAN, audio resources which can't be accessed by A core. And we may allocate even more for virtual machines running at another CPU core. Thus, defining all the clock sources (fixed) in device tree for A core seems to be a little bit meaningless and it also causes us hard to extend for a new SoC. E.g. MX8QM has more clocks than QXP in different SS. That's why we want the per clock source node definition in DT. Then we can configure the clock sources conveniently according to different partition setting and new SoC property. Furthermore, per clock resource node also makes us more easily to handle power domain In a more standard way and do state save&restore during system suspend/resume due to the clock state will be lost when the power is off. Another important thing is that MX8 is consisted of a number of HW subsystem while each Subsystem has separate clock controllers (both SCU clock controllers and LPCG clock controllers). I believe this is different from other vendor like TI and ARM Juno which might make us feel we should use the same way as theirs at the first glance. But we're different. That's why I use per clock resource node as it seems to be better for i.MX special characteristic. Considering all above requirements, how would you suggest us to do? > For the clock type, perhaps combine that in the clock cell with the resource ID > such as using the upper 8-bits. > It seems we must combine them because current clock-indices binding does not support two cells index which seems a drawback from user point of view. e.g. clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>; > > +- clock-output-names: Shall be the corresponding names of the outputs. > > +- power-domains: Should contain the power domain used by this > SCU clock. > > + > > +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 (No sub-nodes which is 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 @@ -144,6 > > +165,21 @@ lsio_mu1: mailbox@5d1c0000 { > > #mbox-cells = <2>; > > }; > > > > +conn-scu-clock-controller { > > + compatible = "fsl,imx8qxp-clk", "fsl,scu-clk"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + uart0_clk: clock-scu@57 { > > + reg = <57>; > > + #clock-cells = <1>; > > + clock-indices = <IMX_SC_PM_CLK_PER>; > > + clock-output-names = "uart0_clk"; > > + power-domains = <&pd IMX_SC_R_UART_0>; > > + }; > > + ... > > +} > > + > > firmware { > > scu { > > compatible = "fsl,imx-scu"; @@ -160,11 +196,6 @@ > > firmware { > > &lsio_mu1 1 3 > > &lsio_mu1 3 3>; > > > > - clk: clk { > > - compatible = "fsl,imx8qxp-clk", "fsl,scu-clk"; > > - #clock-cells = <1>; > > - }; > > - > > iomuxc { > > compatible = "fsl,imx8qxp-iomuxc"; > > > > @@ -192,8 +223,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 IMX_SC_PM_CLK_PER>; > > 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 4e61f64..fbeaca7 100644 > > --- a/include/dt-bindings/firmware/imx/rsrc.h > > +++ b/include/dt-bindings/firmware/imx/rsrc.h > > @@ -547,4 +547,21 @@ > > #define IMX_SC_R_ATTESTATION 545 > > #define IMX_SC_R_LAST 546 > > > > +/* > > + * 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 */ > > I don't understand how these work with overlapping numbers. They're defined by SCU firmware protocol for different device resources. e.g. For PLL resources, it only uses IMX_SC_PM_CLK_PLL. But for USB, it may use MISC besides PER/MST_BUS/SLV_BUS. Regards Dong Aisheng > > Rob
