| Message ID | 1421042407-13921-1-git-send-email-igal.liberman@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Scott Wood |
| Headers | show |
On Mon, 2015-01-12 at 08:00 +0200, Igal.Liberman wrote: > From: Igal Liberman <Igal.Liberman@freescale.com> > > Signed-off-by: Igal Liberman <Igal.Liberman@freescale.com> > Change-Id: I92d020651237041d3767aa35e9345439714f9831 > --- > arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) Please explain this more. Was it just wrong before? Is this for a new chip? If the latter, what effect does this have on existing chips? > diff --git a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > index 48e0b6e..7e1f074 100644 > --- a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > +++ b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > @@ -49,14 +49,16 @@ global-utilities@e1000 { > reg = <0x800 0x4>; > compatible = "fsl,qoriq-core-pll-2.0"; > clocks = <&sysclk>; > - clock-output-names = "pll0", "pll0-div2", "pll0-div4"; > + clock-output-names = "pll0", "pll0-div2", "pll0-div3", > + "pll0-div4"; You're changing the meaning of existing clock index 2. -Scott
Regaeds, Igal Liberman. > -----Original Message----- > From: Wood Scott-B07421 > Sent: Tuesday, January 20, 2015 9:44 AM > To: Liberman Igal-B31950 > Cc: linuxppc-dev@lists.ozlabs.org; Medve Emilian-EMMEDVE1 > Subject: Re: [PATCH] powerpc/dts: Update platform PLL node > > On Mon, 2015-01-12 at 08:00 +0200, Igal.Liberman wrote: > > From: Igal Liberman <Igal.Liberman@freescale.com> > > > > Signed-off-by: Igal Liberman <Igal.Liberman@freescale.com> > > Change-Id: I92d020651237041d3767aa35e9345439714f9831 > > --- > > arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > Please explain this more. Was it just wrong before? Is this for a new chip? If > the latter, what effect does this have on existing chips? > It wasn't wrong, however it was missing some clocking options which might be used by some hardware accelerators available in T/B devices. I need this options for FMan, however it might be used for other accelerators too. > > diff --git a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > > b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > > index 48e0b6e..7e1f074 100644 > > --- a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > > +++ b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > > @@ -49,14 +49,16 @@ global-utilities@e1000 { > > reg = <0x800 0x4>; > > compatible = "fsl,qoriq-core-pll-2.0"; > > clocks = <&sysclk>; > > - clock-output-names = "pll0", "pll0-div2", "pll0-div4"; > > + clock-output-names = "pll0", "pll0-div2", "pll0-div3", > > + "pll0-div4"; > > You're changing the meaning of existing clock index 2. > Yes, however this platform PLL is a new work which is not yet used, so we aren't breaking any functionality. > -Scott >
On Tue, 2015-01-20 at 02:51 -0600, Liberman Igal-B31950 wrote: > > > Regaeds, > Igal Liberman. > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Tuesday, January 20, 2015 9:44 AM > > To: Liberman Igal-B31950 > > Cc: linuxppc-dev@lists.ozlabs.org; Medve Emilian-EMMEDVE1 > > Subject: Re: [PATCH] powerpc/dts: Update platform PLL node > > > > On Mon, 2015-01-12 at 08:00 +0200, Igal.Liberman wrote: > > > From: Igal Liberman <Igal.Liberman@freescale.com> > > > > > > Signed-off-by: Igal Liberman <Igal.Liberman@freescale.com> > > > Change-Id: I92d020651237041d3767aa35e9345439714f9831 > > > --- > > > arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > Please explain this more. Was it just wrong before? Is this for a new chip? If > > the latter, what effect does this have on existing chips? > > > > It wasn't wrong, however it was missing some clocking options which > might be used by some hardware accelerators available in T/B devices. > I need this options for FMan, however it might be used for other > accelerators too. If the PLL had a div3 option and it wasn't described by the PLL node, the node was wrong. Do all chips that use this file have a div3? > > > diff --git a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > > > b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > > > index 48e0b6e..7e1f074 100644 > > > --- a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > > > +++ b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi > > > @@ -49,14 +49,16 @@ global-utilities@e1000 { > > > reg = <0x800 0x4>; > > > compatible = "fsl,qoriq-core-pll-2.0"; > > > clocks = <&sysclk>; > > > - clock-output-names = "pll0", "pll0-div2", "pll0-div4"; > > > + clock-output-names = "pll0", "pll0-div2", "pll0-div3", > > > + "pll0-div4"; > > > > You're changing the meaning of existing clock index 2. > > > > Yes, however this platform PLL is a new work which is not yet used, so we aren't breaking any functionality. No, it's the core PLL node which is already in use. However, it looks like the driver already interprets clock index 2 differently based on whether clock index 3 exists. None of this is mentioned in the binding document... -Scott
Hello Scott, On 01/29/2015 10:12 PM, Scott Wood wrote: >>>> From: Igal Liberman <Igal.Liberman@freescale.com> >>>> >>>> Signed-off-by: Igal Liberman <Igal.Liberman@freescale.com> >>>> Change-Id: I92d020651237041d3767aa35e9345439714f9831 >>>> --- >>>> arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> Please explain this more. Was it just wrong before? Is this for a new chip? If >>> the latter, what effect does this have on existing chips? >> >> It wasn't wrong, however it was missing some clocking options which >> might be used by some hardware accelerators available in T/B devices. >> I need this options for FMan, however it might be used for other >> accelerators too. > > If the PLL had a div3 option and it wasn't described by the PLL node, > the node was wrong. Somewhere between early versions of the RM and the cores not using this PLL output, yes the node is incomplete/wrong > Do all chips that use this file have a div3? Yup. T1/2/4, B4, etc. It's used as an input clock to the various IP blocks on said SoC(es), FMan, eSDHC, PME, etc. >>>> diff --git a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi >>>> b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi >>>> index 48e0b6e..7e1f074 100644 >>>> --- a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi >>>> +++ b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi >>>> @@ -49,14 +49,16 @@ global-utilities@e1000 { >>>> reg = <0x800 0x4>; >>>> compatible = "fsl,qoriq-core-pll-2.0"; >>>> clocks = <&sysclk>; >>>> - clock-output-names = "pll0", "pll0-div2", "pll0-div4"; >>>> + clock-output-names = "pll0", "pll0-div2", "pll0-div3", >>>> + "pll0-div4"; >>> >>> You're changing the meaning of existing clock index 2. >> >> Yes, however this platform PLL is a new work which is not yet >> used, so we aren't breaking any functionality. > > No, it's the core PLL node which is already in use. However, it looks > like the driver already interprets clock index 2 differently based on > whether clock index 3 exists. None of this is mentioned in the binding > document... Yes, it's a bit fishy and that clock registration (code) ends up working. However, I think we ought to key that code on the chassis version and not on the number of clock outputs in some (in/complete) node instance That being said, there are two outstanding issues: 1. The core mux nodes need to be updated to get pllX-div4 from index 3 not 2. Currently things work because our RCW(s) don't use pllX-div4 so the inconsistency goes unnoticed 2. Chassis v2 supports frequency scaling for the HW accelerators (FMan in this particular case) much like it's supported for the cores (both chassis v1/2). This patch doesn't cover that Cheers,
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi index 48e0b6e..7e1f074 100644 --- a/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi +++ b/arch/powerpc/boot/dts/fsl/qoriq-clockgen2.dtsi @@ -49,14 +49,16 @@ global-utilities@e1000 { reg = <0x800 0x4>; compatible = "fsl,qoriq-core-pll-2.0"; clocks = <&sysclk>; - clock-output-names = "pll0", "pll0-div2", "pll0-div4"; + clock-output-names = "pll0", "pll0-div2", "pll0-div3", + "pll0-div4"; }; pll1: pll1@820 { #clock-cells = <1>; reg = <0x820 0x4>; compatible = "fsl,qoriq-core-pll-2.0"; clocks = <&sysclk>; - clock-output-names = "pll1", "pll1-div2", "pll1-div4"; + clock-output-names = "pll1", "pll1-div2", "pll1-div3", + "pll1-div4"; }; platform_pll: platform-pll@c00 { #clock-cells = <1>;