Hi Rob,
[...]
> > > > 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.
>
I found we may still can't use this new way after giving a try.
One know issue is that it can't support clock parent setting well with this binding
If merged all sub clocks into a single node.
Hard to describe parent clocks for each clock within the same big array.
For example in MX8 ADMA SS, there're another LCD PLL which can be optional parent clocks
to others peripherals.
If we list them all in the same array, we can't describe LCD baud/pixel clock parents.
dma_scu_clk: dma-scu-clock-controller {
compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-clk";
#clock-cells = <1>;
clock-indices = <SC_R_ELCDIF_PLL IMX_SC_PM_CLK_PLL>,
<SC_R_LCD_0 IMX_SC_PM_CLK_PER>, /* lcd baud */
<SC_R_LCD_0 IMX_SC_PM_CLK_SLV_BUS>, /* Pixel Link */
...
clock-output-names = "lcdif_pll",
"lcdif_baud_clk",
"lcdif_pixel_clk",
...
power-domains = <&pd IMX_SC_R_LCD_0>,
<&pd IMX_SC_R_LCD_0>,
<&pd IMX_SC_R_LCD_0>,
...
};
And other peripherals might have different parents within the same array.
The old way does not have this issue because it's capable of configuring parents
respectively for each sub clocks.
/* SCU clocks */
dma_scu_clk: clock-controller-scu-dma {
#address-cells = <1>;
#size-cells = <0>;
lcd_pll: clock-scu@323 {
reg = <323>;
#clock-cells = <1>;
clock-indices = <IMX_SC_PM_CLK_PLL>;
clock-output-names = "lcd_pll";
power-domains = <&pd IMX_SC_R_ELCDIF_PLL>;
};
lcd0_clk: clock-scu@187 {
reg = <187>;
#clock-cells = <1>;
/* parent clocks should match HW programing order */
clocks = <&dummy_clk &dummy_clk &dummy_clk &dummy_clk &lcd_pll>;
clock-indices = <IMX_SC_PM_CLK_PER>;
clock-output-names = "lcd0_clk";
power-domains = <&pd IMX_SC_R_LCD_0>;
};
...
};
I double checked other SS like Audio, DC, MIPI, PI which have the same issue.
I really don't know if there will be a way out if using the one single node way.
And I'm also a bit worrying whether it may cause more issues due to its
losing of the flexibility and causes potential issues.
Do you think if we can still go back to the old way which is proposed
In this patch set?
As it can perfectly meet our requirements and also ease the driver
implementation.
Hope you can help shed some lights as we're pending on it for a long time.
Regards
Dong Aisheng
> > 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
