> From: Rob Herring [mailto:robh@xxxxxxxxxx] > Sent: Tuesday, May 14, 2019 2:00 AM > > On Wed, May 08, 2019 at 07:16:11AM +0000, Aisheng Dong wrote: > > > From: Rob Herring [mailto:robh+dt@xxxxxxxxxx] > > > Sent: Wednesday, May 8, 2019 2:03 AM > > > > > > On Sat, May 4, 2019 at 7:19 AM Aisheng Dong <aisheng.dong@xxxxxxx> > > > wrote: > > > > > > > > > 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>, > > > > ... > > > > }; > > > > > > Yes, but... > > > > > > > serial@5a060000 { > > > > ... > > > > clocks = <&scu_clk IMX_SCU_CLK_ID(IMX_SC_R_UART_0, > > > > IMX_SC_PM_CLK_PER)>; > > > > > > I thought devices got clocks from the LPCG? > > > > > > > Yes. Here is just an example of using SCU clocks. > > And for some devices without LPCG, it could also get clocks directly from SCU > clock. > > > > > > 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. > > > > > > I'm not suggesting that. It's really just re-arranging all the same > > > data from a bunch of child nodes to a single node. Granted, it may > > > be easier to add/delete nodes than add/delete elements from an array > > > of property values, but really that's just a tooling problem > > > > > > > Okay, understood. > > So it seems we could still have a separate clock controller node for > > each SS but merge all the same data of child nodes data into it. > > > > However, we still have one concern. > > Taking MX8QXP DMA SS as example, with one node description, it may be > > something like below: > > dma_scu_clk: dma-scu-clock-controller { > > compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk"; > > #clock-cells = <1>; > > clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ADC_0, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_CAN_0, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_FTM_0, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_FTM_1, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_I2C_0, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_I2C_1, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_I2C_2, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_I2C_3, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_LCD_0_PWM_0, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_SPI_0, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_SPI_1, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_SPI_2, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_SPI_3, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_UART_0, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_UART_1, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_UART_2, > IMX_SC_PM_CLK_PER)>, > > <IMX_SCU_CLK_ID(IMX_SC_R_UART_3, > IMX_SC_PM_CLK_PER)>; > > clock-output-names = "adc0_clk", > > "can0_clk", > > "ftm0_clk", > > "ftm1_clk", > > "i2c0_clk", > > "i2c1_clk", > > "i2c2_clk", > > "i2c3_clk", > > "lcd0_clk", > > "lcd0_pwm0_clk", > > "spi0_clk", > > "spi1_clk", > > "spi2_clk", > > "spi3_clk", > > "uart0_clk", > > "uart1_clk", > > "uart2_clk", > > "uart3_clk"; > > power-domains = <&pd IMX_SC_R_ADC_0>, > > <&pd IMX_SC_R_CAN_0>, > > <&pd IMX_SC_R_FTM_0>, > > <&pd IMX_SC_R_FTM_1>, > > <&pd IMX_SC_R_I2C_0>, > > <&pd IMX_SC_R_I2C_1>, > > <&pd IMX_SC_R_I2C_2>, > > <&pd IMX_SC_R_I2C_3>, > > <&pd IMX_SC_R_LCD_0>, > > <&pd IMX_SC_R_LCD_0_PWM_0>, > > <&pd IMX_SC_R_SPI_0>, > > <&pd IMX_SC_R_SPI_1>, > > <&pd IMX_SC_R_SPI_2>, > > <&pd IMX_SC_R_SPI_3>, > > <&pd IMX_SC_R_UART_0>, > > <&pd IMX_SC_R_UART_1>, > > <&pd IMX_SC_R_UART_2>, > > <&pd IMX_SC_R_UART_3>; }; > > > > For MX8QM, even if we have one more UART_4, then we still have to > > write all the same things again with an extra UART_4. It seems it's a > > bit violate our design that using a shared one and do incremental changes for > new SoCs. > > Do you think if this is acceptable to you? > > Yes, as it should be a one time thing to do per SoC. Not really. We may need write a few other copies with slight differences according to different partition configuration. For example, on MX8QM MEK board, we allocate CAN resource as well as a few other I2C to M4 partition which means M4 SW will be the one to handle CAN transfer. Then we will have 3 copies of SCU clock configurations: Imx8-ss-dma.dtsi => SS common part Imx8qm-ss-dma.dtsi => Handle SoC SS difference Imx8qm-mek.dts => Handle partition configuration or virtualization Considering this is one SS, there're still other ~10 SS. What I can imagine is that we might need re-write many big SS SCU clocks node as above in board.dts instead of doing granularly tuning by per clock resource nodes. And actually LPCG clocks already did that way by adding/removing Nodes for partition setting, as well as device nodes. Now SCU clocks can't if combine them into one big node which are not aligned with others. Do you think this is still all okay? If you insist we can give a try. > > > But if describe them per nodes, we do not have such issue. > > > > Anyway, please tell me your choice, then I will follow. > > > > BTW, I don't know how a tool can address this issue. > > I meant you could write one that understands the binding. It's a bit more > complicated having to parse and update properties compared to adding or > removing nodes, but it can still be done. > The written one(SCU clocks) might change according to different customer's requirements on partition configuration. That's the trouble we have now. Regards Dong Aisheng > Rob
