[v3] dt/bindings: qoriq-clock: Add binding for FMan clock mux

Igal.Liberman at freescale.com Igal.Liberman at freescale.com
Fri May 1 00:28:27 AEST 2015



Regards,
Igal Liberman.

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, April 30, 2015 3:31 AM
> To: Liberman Igal-B31950
> Cc: devicetree at vger.kernel.org; linuxppc-dev at lists.ozlabs.org; Tang
> Yuantian-B29983
> Subject: Re: [v3] dt/bindings: qoriq-clock: Add binding for FMan clock mux
> 
> On Wed, 2015-04-22 at 05:47 -0500, Liberman Igal-B31950 wrote:
> >
> >
> > Regards,
> > Igal Liberman.
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, April 21, 2015 3:52 AM
> > > To: Liberman Igal-B31950
> > > Cc: devicetree at vger.kernel.org; linuxppc-dev at lists.ozlabs.org; Tang
> > > Yuantian-B29983
> > > Subject: Re: [v3] dt/bindings: qoriq-clock: Add binding for FMan
> > > clock mux
> > >
> > > On Mon, 2015-04-20 at 06:40 -0500, Liberman Igal-B31950 wrote:
> > > >
> > > >
> > > > Regards,
> > > > Igal Liberman.
> > > >
> > > > > -----Original Message-----
> > > > > From: Liberman Igal-B31950
> > > > > Sent: Monday, April 20, 2015 2:07 PM
> > > > > To: Wood Scott-B07421
> > > > > Cc: devicetree at vger.kernel.org; linuxppc-dev at lists.ozlabs.org
> > > > > Subject: RE: [v3] dt/bindings: qoriq-clock: Add binding for FMan
> > > > > clock mux
> > > > >
> > > > >
> > > > >
> > > > > Regards,
> > > > > Igal Liberman.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Friday, April 17, 2015 8:41 AM
> > > > > > To: Liberman Igal-B31950
> > > > > > Cc: devicetree at vger.kernel.org; linuxppc-dev at lists.ozlabs.org
> > > > > > Subject: Re: [v3] dt/bindings: qoriq-clock: Add binding for
> > > > > > FMan clock mux
> > > > > >
> > > > > > On Thu, 2015-04-16 at 01:11 -0500, Liberman Igal-B31950 wrote:
> > > > > > >
> > > > > > >
> > > > > > > Regards,
> > > > > > > Igal Liberman.
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Wood Scott-B07421
> > > > > > > > Sent: Wednesday, April 15, 2015 8:36 PM
> > > > > > > > To: Liberman Igal-B31950
> > > > > > > > Cc: devicetree at vger.kernel.org;
> > > > > > > > linuxppc-dev at lists.ozlabs.org
> > > > > > > > Subject: Re: [v3] dt/bindings: qoriq-clock: Add binding
> > > > > > > > for FMan clock mux
> > > > > > > >
> > > > > > > > On Tue, 2015-04-14 at 13:56 +0300, Igal.Liberman wrote:
> > > > > > > > > From: Igal Liberman <Igal.Liberman at freescale.com>
> > > > > > > > >
> > > > > > > > > v3: Addressed feedback from Scott:
> > > > > > > > > 	- Removed clock specifier description.
> > > > > > > > >
> > > > > > > > > v2: Addressed feedback from Scott:
> > > > > > > > > 	- Moved the "fman-clk-mux" clock provider details
> > > > > > > > > 	  under "clocks" property.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Igal Liberman
> > > > > > > > > <Igal.Liberman at freescale.com>
> > > > > > > > > ---
> > > > > > > > >  .../devicetree/bindings/clock/qoriq-clock.txt      |   17
> > > > > > +++++++++++++++--
> > > > > > > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git
> > > > > > > > > a/Documentation/devicetree/bindings/clock/qoriq-clock.tx
> > > > > > > > > t
> > > > > > > > > b/Documentation/devicetree/bindings/clock/qoriq-clock.tx
> > > > > > > > > t index b0d7b73..2bb3b38 100644
> > > > > > > > > ---
> > > > > > > > > a/Documentation/devicetree/bindings/clock/qoriq-clock.tx
> > > > > > > > > t
> > > > > > > > > +++ b/Documentation/devicetree/bindings/clock/qoriq-cloc
> > > > > > > > > +++ k.tx
> > > > > > > > > +++ t
> > > > > > > > > @@ -65,9 +65,10 @@ Required properties:
> > > > > > > > >  		It takes parent's clock-frequency as its clock.
> > > > > > > > >  	* "fsl,qoriq-platform-pll-1.0" for the platform PLL clock (v1.0)
> > > > > > > > >  	* "fsl,qoriq-platform-pll-2.0" for the platform PLL
> > > > > > > > > clock
> > > > > > > > > (v2.0)
> > > > > > > > > +	* "fsl,fman-clk-mux" for the Frame Manager clock.
> > > > > > > > >  - #clock-cells: From common clock binding. The number
> > > > > > > > > of cells in
> > > a
> > > > > > > > > -	clock-specifier. Should be <0> for "fsl,qoriq-sysclk-[1,2].0"
> > > > > > > > > -	clocks, or <1> for "fsl,qoriq-core-pll-[1,2].0" clocks.
> > > > > > > > > +	clock-specifier. Should be <0> for "fsl,qoriq-sysclk-
> [1,2].0"
> > > > > and
> > > > > > > > > +	"fsl,fman-clk-mux" clocks or <1> for
> > > > > > > > > +"fsl,qoriq-core-pll-
> > > > > [1,2].0".
> > > > > > > > >  	For "fsl,qoriq-core-pll-1.0" clocks, the single
> > > > > > > > >  	clock-specifier cell may take the following values:
> > > > > > > > >  	* 0 - equal to the PLL frequency @@ -145,6 +146,18 @@
> > > > > Example
> > > > > > > > > for clock block and clock provider:
> > > > > > > > >  			clocks = <&sysclk>;
> > > > > > > > >  			clock-output-names = "platform-pll",
> > > > > "platform-pll-
> > > > > > > > div2";
> > > > > > > > >  		};
> > > > > > > > > +
> > > > > > > > > +		fm0clk: fm0-clk-mux {
> > > > > > > > > +			#clock-cells = <0>;
> > > > > > > > > +			reg = <0x10 4>
> > > > > > > > > +			compatible = "fsl,fman-clk-mux";
> > > > > > > > > +			clocks = <&pll0 0>, <&pll0 1>, <&pll0
> 2>,
> > > > > <&pll0 3>,
> > > > > > > > > +				 <&platform_pll 0>, <&pll1 1>,
> <&pll1
> > > > > 2>;
> > > > > > > > > +			clock-names = "pll0", "pll0-div2",
> "pll0-div3",
> > > > > > > > > +				      "pll0-div4", "platform-pll",
> "pll1-
> > > > > div2",
> > > > > > > > > +				      "pll1-div3";
> > > > > > > > > +			clock-output-names = "fm0-clk";
> > > > > > > > > +		};
> > > > > > > > >  	};
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > >
> > > > > > > > I don't see this register in the manuals for older DPAA
> > > > > > > > chips, such as
> > > > > > > > p4080 or p3041.  Is it present but undocumented?  Should I
> > > > > > > > be looking somewhere other than "Clocking Memory Map"?
> > > > > > > >
> > > > > > >
> > > > > > > It's available only in part of the new chips (T4, T2, B4).
> > > > > > > In T1024/T1040 there's only one option for FMan clock so
> > > > > > > this register is not
> > > > > > available.
> > > > > >
> > > > > > So it's part of the 2.0 chassis?  I'd stick a 2.0 in there, then.
> > > > > > Who knows what we may see in the future.
> > > > > >
> > > > >
> > > > > OK,
> > > > > We can go with "fsl,fman-clk-mux-1/2-0.".
> > > > > In that case, we need to update FMan nodes and the clock driver:
> > > > > https://patchwork.ozlabs.org/patch/443973/
> > > > > https://patchwork.ozlabs.org/patch/461813/
> > > > > I will update those patches separately.
> > > > >
> > > >
> > > > Scott,
> > > > There are 2 options:
> > > > Use "fsl,fman-clk-mux-1.0" for SoC without CLKCGnHWACSR register.
> > > > Use "fsl,fman-clk-mux-2.0" for SoC with CLKCGnHWACSR register.
> > > > Or
> > > > Use "fsl,fman-clk-mux-1.0" for SoC which support FMan V2 (Pxxxx)
> > > > Use "fsl,fman-clk-mux-2.0" for SoC which support FMan V3 (B/T)
> > >
> > > 1.0/2.0 in the clockgen node refers to chassis version.  It has
> > > nothing to do with FMan version.
> > >
> >
> > I know. However there's a match: 1.0 chassis used in SoC with FMan v2 and
> 2.0 chassis in SoC with FMan v3.
> >
> > > In fact, fman should not be in the compatible because, now that I
> > > found the documentation for this, I see that it's more generic than that.
> > > "fman-clk-mux" should be replaced with "qoriq-hwacsr".
> > >
> > > > Using the 1st option might be confusing because core pll/mux 2.0
> > > represents B/T devices and 1.0 represent Pxxxx.
> > > > In this case, T1040 uses "fsl,qoriq-core-pll/mux-2.0" and
> > > > "fsl,fman-clk-mux-
> > > 1.0".
> > > > On the other hand, the second option doesn't distinguishes between
> > > > T4
> > > and T1 (for example), as T1 doesn't have reg property while T4 has.
> > >
> > > How would t1040 have a so-called "fman-clk-mux" node at all if it
> > > doesn't have this register?
> > >
> > > I think we've made a mess of the clock bindings and this is only
> > > going to make it worse.  We need to stay compatible with the mess
> > > we've made, but I'm inclined to say that we shouldn't add more nodes to
> it.
> > >
> > > Instead, have the toplevel clockgen node have a chip-based
> > > compatible in addition to version (e.g. compatible =
> > > "fsl,t4240-clockgen",
> > > "fsl,qoriq-clockgen-2.0") and have the toplevel node export whatever
> > > clocks are needed.  Put the logic to deal with all the different
> > > dividers, register values, and such in code, not the device tree.
> > > The binding should be focused on how to encode the specifier of the
> exported clocks.
> > >
> >
> > We have 2 cases:
> > 	- Devices (T2/T4/B4) with CLKCG1HWACSR register.
> > 	- Devices (Pxxxx, T1) without CLKCG1HWACSR register (Pxxxx
> devices
> > have many options, T1 has only one option)
> >
> > For the first group, we can have " qoriq-hwacsr" property in the clock node.
> 
> No, we're not going to describe every register with its own property.
> Describe the chip and let the driver be the place with the knowledge of what
> each chip is like.
> 

I think that FMan clock mux (as we defined it) is similar to other muxes available in our SoC.
If we take T4 as example, it has 3 muxes defined in the device tree (mux0, mux1 and mux2).
Each mux has its own reg property (and clock providers).
	mux2: mux2 at 40 {
		#clock-cells = <0>;
		reg = <0x40 0x4>;
		compatible = "fsl,qoriq-core-mux-2.0";
		clocks = <&pll3 0>, <&pll3 1>, <&pll3 2>,
			<&pll4 0>, <&pll4 1>, <&pll4 2>;
		clock-names = "pll3", "pll3-div2", "pll3-div4",
			"pll4", "pll4-div2", "pll4-div4";
		clock-output-names = "cmux2";
	};

I agree that "fm1-clk-mux" need to be moved from the "guts" node to "clockgen" node.
However, I'm not sure which changes you want to perform in the node (beside adding reg property for SoC which has it for FMan clock).

> > Currently T4 FMan clock mux node is the following:
> > 	fm1clk: fm1-clk-mux {
> > 		#clock-cells = <0>;
> > 		compatible = "fsl,fman-clk-mux";
> > 		clocks = <&pll1 1>, <&pll1 2>, <&pll1 3>,
> > 			 <&platform_pll 0>, <&pll0 1>, <&pll0 2>;
> > 		clock-names = "pll1-div2", "pll1-div3", "pll1-div4",
> > 			      "platform-pll", "pll0-div2", "pll0-div3";
> > 		clock-output-names = "fm1-clk";
> > 	};
> > As far as I understand we need to move the node to the top level clock
> node.
> > In addition we need to add reg property and change the name of the node
> and the compatible.
> > In that case, the driver can read this register instead of parsing the RCW.
> 
> I'm having a hard time following.  What I'm suggesting is eliminating the
> above and having the clockgen node itself (which already has a reg
> property) be a clock source with a clock specifier encoding that distinguishes
> the fman clock from other clocks.
> 
> > What about the devices of the second group?
> > In this case we don't have a register to determine the source clock.
> > So we need access to guts registers,
> 
> I never suggested taking away access to the guts registers.
> 

I didn't mention that you want to remove them.
Just tried to explain that for Pxxxx devices, we need to use the current method (explained later).

> > like we have currently.
> > The suggestion above doesn’t suit for those devices.
> 
> How is the clock determined on those chips?
> 

In those chips (and currently others), currently, in the device tree we have all optional FMan clock providers (different between each SoC).
The clock driver reads the RCW in order to determine the clock provider and saves the index, later get_fm_clk_parent returns the index.

> -Scott
> 

Igal



More information about the Linuxppc-dev mailing list