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

[Aisheng Dong <aisheng.dong@nxp.com>]

Hi Rob,

> From: Aisheng Dong
> Sent: Wednesday, March 27, 2019 10:35 PM
> > From: Rob Herring [mailto:robh@kernel.org]
> > 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@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>
> > > ---
> > >  .../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
> > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel