Re: [PATCH V2 1/2] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

[Rob Herring <robh@kernel.org>]

On Wed, May 08, 2019 at 07:16:11AM +0000, Aisheng Dong wrote:
> > From: Rob Herring [mailto:robh+dt@kernel.org]
> > Sent: Wednesday, May 8, 2019 2:03 AM
> > 
> > On Sat, May 4, 2019 at 7:19 AM Aisheng Dong <aisheng.dong@nxp.com>
> > wrote:
> > >
> > > > From: Rob Herring [mailto:robh+dt@kernel.org]
> > > > 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@nxp.com>
> > > > 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@kernel.org>
> > > > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > > > Cc: devicetree@vger.kernel.org
> > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > > ---
> > > > > 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.

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

Rob