Re: [PATCH v3 01/11] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree

From: Daniel Baluta <daniel.baluta@nxp.com>
To: "dongas86@gmail.com" <dongas86@gmail.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>
Cc: Aisheng Dong <aisheng.dong@nxp.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"sboyd@kernel.org" <sboyd@kernel.org>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	Fabio Estevam <fabio.estevam@nxp.com>,
	Daniel Baluta <daniel.baluta@nxp.com>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 01/11] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree
Date: Mon, 19 Aug 2019 12:36:36 +0000	[thread overview]
Message-ID: <caab750569dc246c99e88bbe6d9ca137ae7f198f.camel@nxp.com> (raw)
In-Reply-To: <CAA+hA=Sm0MAHNwH1sZQfK8cO+3jLkue97u=ceFiUv34+qGos1Q@mail.gmail.com>

On Mon, 2019-08-05 at 11:48 +0800, Dong Aisheng wrote:
> On Sun, Aug 4, 2019 at 11:49 AM Shawn Guo <shawnguo@kernel.org>
> wrote:
> > 
> > On Tue, Jul 16, 2019 at 11:00:55PM +0800, Dong Aisheng wrote:
> > > There's a few limitations on the original one cell clock binding
> > > (#clock-cells = <1>) that we have to define some SW 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 want to eliminate the using of SW Clock
> > > IDs and
> > > change to use a more close to HW one instead.
> > > For SCU clocks usage, only two params required: Resource id +
> > > Clock Type.
> > 
> > If this is how SCU firmware addresses the clock, I agree that it's
> > worth
> > witching to this new bindings, which describes the hardware (SCU
> > firmware in this case) better, IMO.
> > 
> > > Both parameters are platform independent. So we could use two
> > > cells binding
> > > to pass those parameters,
> > > 
> > > 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:
> > > v2->v3:
> > >  * Changed to two cells binding and register all clocks in driver
> > >    instead of parse from device tree.
> > > 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       | 12
> > > +++++++-----
> > >  include/dt-bindings/firmware/imx/rsrc.h                 | 17
> > > +++++++++++++++++
> > >  2 files changed, 24 insertions(+), 5 deletions(-)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 5d7dbab..351d335 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -89,7 +89,10 @@ Required properties:
> > >                         "fsl,imx8qm-clock"
> > >                         "fsl,imx8qxp-clock"
> > >                       followed by "fsl,scu-clk"
> > > -- #clock-cells:              Should be 1. Contains the Clock ID
> > > value.
> > > +- #clock-cells:              Should be either
> > > +                     2: Contains the Resource and Clock ID
> > > value.
> > > +                     or
> > > +                     1: Contains the Clock ID value.
> > > (DEPRECATED)
> > >  - clocks:            List of clock specifiers, must contain an
> > > entry for
> > >                       each required entry in clock-names
> > >  - clock-names:               Should include entries
> > > "xtal_32KHz", "xtal_24MHz"
> > > @@ -162,7 +165,7 @@ firmware {
> > > 
> > >               clk: clk {
> > >                       compatible = "fsl,imx8qxp-clk", "fsl,scu-
> > > clk";
> > > -                     #clock-cells = <1>;
> > > +                     #clock-cells = <2>;
> > >               };
> > > 
> > >               iomuxc {
> > > @@ -192,8 +195,7 @@ 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_R_UART_0 IMX_SC_PM_CLK_PER>;
> > > +     clock-names = "ipg";
> > >       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 */
> 
> This is for typical device resource.
> 
> > > +#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 */
> 
> This is for some special clock types which do not belong to above
> normal clock types.
> Used very rare in SCU firmware.
> e.g.
> enet0_mac0_rxclk SC_R_ENE T_0 / SC_PM_CL K_MISC0
> 
> > > +#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 */
> 
> They're for specific clock types for CPU/PLL/BYPASS only.

Hi Aisheng,

Yes, please separate this types of clocks in their own sections with
proper description.

> 
> > 
> > It seems that there are several sets of clock type which apply to
> > different resources/devices?  If so, can you separate them a bit
> > with
> > some comments to make the list easier for readers?
> > 
> 
> 
So, please send v4 with all comments fixed. I can help with testing.
I am also intrested in seeing this get in!

thanks,
Daniel.