Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-10 Thread Eddie Huang
On Fri, 2015-07-10 at 16:11 +0800, Daniel Kurtz wrote:
> On Fri, Jul 10, 2015 at 3:27 PM, Eddie Huang  wrote:
> > Hi all,
> >
> > On Wed, 2015-07-08 at 13:44 +0800, Sascha Hauer wrote:
> >> On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote:
> >> > On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
> >> > > On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer  
> >> > > wrote:
> >> > > > On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
> >> > > >> On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer 
> >> > > >>  wrote:
> >> > > >> > On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
> >> > > >> >> Add clk_null, which represents clocks that can not / need not
> >> > > >> >> controlled by software.
> >> > > >> >> There are many clocks' parent set to clk_null.
> >> > > >> >>
> >> > > >> >> Signed-off-by: James Liao 
> >> > > >> >> Signed-off-by: Eddie Huang 
> >> > > >> >> ---
> >> > > >> >> Base on 4.1-rc1
> >> > > >> >>
> >> > > >> >> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
> >> > > >> >> ---
> >> > > >> >>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
> >> > > >> >>  1 file changed, 6 insertions(+)
> >> > > >> >>
> >> > > >> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
> >> > > >> >> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> > > >> >> index 924fdb6..4798f44 100644
> >> > > >> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> > > >> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> > > >> >> @@ -81,6 +81,12 @@
> >> > > >> >>   cpu_on= <0x8403>;
> >> > > >> >>   };
> >> > > >> >>
> >> > > >> >> + clk_null: clk_null {
> >> > > >> >> + compatible = "fixed-clock";
> >> > > >> >> + clock-frequency = <0>;
> >> > > >> >> + #clock-cells = <0>;
> >> > > >> >> + };
> >> > > >> >
> >> > > >> > The discussion around this patch shows that we don't want to have 
> >> > > >> > this
> >> > > >> > clock in the device tree as it is not a hardware description.
> >> > > >> >
> >> > > >> > Ok, fine. Eddie, you told us that the rate of the current 
> >> > > >> > clk_null children
> >> > > >> > is not interesting. What's the motivation to send this patch 
> >> > > >> > anyway
> >> > > >> > then? Why can't you keep its children on the orphan list where 
> >> > > >> > they are
> >> > > >> > already now?
> >> > > >> >
> >> > > >> > Another possibility would be to instantiate the clk_null clock 
> >> > > >> > from C
> >> > > >> > code rather than from the device tree. This way we wouldn't put 
> >> > > >> > any
> >> > > >> > wrong descriptions into the device tree and still can implement 
> >> > > >> > the
> >> > > >> > support for the real parent clocks when we actually need them.
> >> > > >>
> >> > > >> Some device nodes, like mmc, use a clk_null phandle as one of their 
> >> > > >> clocks:
> >> > > >>
> >> > > >> mmc1: mmc@1124 {
> >> > > >> compatible = "mediatek,mt8173-mmc",
> >> > > >>  "mediatek,mt8135-mmc";
> >> > > >> reg = <0 0x1124 0 0x1000>;
> >> > > >> interrupts = ;
> >> > > >> clocks = < CLK_PERI_MSDC30_1>,
> >> > > >>  <_null>;
> >> > > >> clock-names = "source", "hclk";
> >> > > >> status = "disabled";
> >> > > >> };
> >> > > >
> >> > > > This is another case than the one we discussed about. In the case 
> >> > > > above
> >> > > > I motivated using a dummy clock since the clock exists in the system,
> >> > > > but is not software controllable. To abstract this from the driver
> >> > > > (which needs this clock since it exists) we here have the dummy 
> >> > > > clock.
> >> > > > However, of course I can't prove the clock is indeed not software
> >> > > > controllable; that's only the information I have.
> >> > >
> >> > > I was trying to answer your question "What's the motivation to send
> >> > > this patch anyway?".
> >> > > The motivation is to send follow on patches that use the clk_null
> >> > > phandle.  We need to provide some clock as the mmc1's hclk.  I do not
> >> > > understand why this has to be "clk_null", though.  It seems like this
> >> > > should be a real clock coming from one of the real clock_controller
> >> > > nodes.  After all, the mmc driver is going to be enabling/disabling
> >> > > this clock for power savings at runtime.  What does that even mean for
> >> > > clk_null ?
> >> >
> >> > The original purpose of this patch is to provide a common dummy clock
> >> > for both software don't care clock and clock that is not software
> >> > controllable.I got comments that device tree should describe hardware
> >> > and should put exact clock in device tree. I think this is true. So we
> >> > will remove this clock_null patch, and:
> >> >
> >> > 1. For Mediatek SoC CCF driver, James will clarify clock usage further.
> >> > Actually, we still think it's not necessary to describe whole tree that
> >> > software don't care, James will deal this in clock driver.
> >>
> >> I 

Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-10 Thread Daniel Kurtz
On Fri, Jul 10, 2015 at 3:27 PM, Eddie Huang  wrote:
> Hi all,
>
> On Wed, 2015-07-08 at 13:44 +0800, Sascha Hauer wrote:
>> On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote:
>> > On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
>> > > On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer  
>> > > wrote:
>> > > > On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
>> > > >> On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer  
>> > > >> wrote:
>> > > >> > On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
>> > > >> >> Add clk_null, which represents clocks that can not / need not
>> > > >> >> controlled by software.
>> > > >> >> There are many clocks' parent set to clk_null.
>> > > >> >>
>> > > >> >> Signed-off-by: James Liao 
>> > > >> >> Signed-off-by: Eddie Huang 
>> > > >> >> ---
>> > > >> >> Base on 4.1-rc1
>> > > >> >>
>> > > >> >> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
>> > > >> >> ---
>> > > >> >>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
>> > > >> >>  1 file changed, 6 insertions(+)
>> > > >> >>
>> > > >> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
>> > > >> >> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> > > >> >> index 924fdb6..4798f44 100644
>> > > >> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> > > >> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> > > >> >> @@ -81,6 +81,12 @@
>> > > >> >>   cpu_on= <0x8403>;
>> > > >> >>   };
>> > > >> >>
>> > > >> >> + clk_null: clk_null {
>> > > >> >> + compatible = "fixed-clock";
>> > > >> >> + clock-frequency = <0>;
>> > > >> >> + #clock-cells = <0>;
>> > > >> >> + };
>> > > >> >
>> > > >> > The discussion around this patch shows that we don't want to have 
>> > > >> > this
>> > > >> > clock in the device tree as it is not a hardware description.
>> > > >> >
>> > > >> > Ok, fine. Eddie, you told us that the rate of the current clk_null 
>> > > >> > children
>> > > >> > is not interesting. What's the motivation to send this patch anyway
>> > > >> > then? Why can't you keep its children on the orphan list where they 
>> > > >> > are
>> > > >> > already now?
>> > > >> >
>> > > >> > Another possibility would be to instantiate the clk_null clock from 
>> > > >> > C
>> > > >> > code rather than from the device tree. This way we wouldn't put any
>> > > >> > wrong descriptions into the device tree and still can implement the
>> > > >> > support for the real parent clocks when we actually need them.
>> > > >>
>> > > >> Some device nodes, like mmc, use a clk_null phandle as one of their 
>> > > >> clocks:
>> > > >>
>> > > >> mmc1: mmc@1124 {
>> > > >> compatible = "mediatek,mt8173-mmc",
>> > > >>  "mediatek,mt8135-mmc";
>> > > >> reg = <0 0x1124 0 0x1000>;
>> > > >> interrupts = ;
>> > > >> clocks = < CLK_PERI_MSDC30_1>,
>> > > >>  <_null>;
>> > > >> clock-names = "source", "hclk";
>> > > >> status = "disabled";
>> > > >> };
>> > > >
>> > > > This is another case than the one we discussed about. In the case above
>> > > > I motivated using a dummy clock since the clock exists in the system,
>> > > > but is not software controllable. To abstract this from the driver
>> > > > (which needs this clock since it exists) we here have the dummy clock.
>> > > > However, of course I can't prove the clock is indeed not software
>> > > > controllable; that's only the information I have.
>> > >
>> > > I was trying to answer your question "What's the motivation to send
>> > > this patch anyway?".
>> > > The motivation is to send follow on patches that use the clk_null
>> > > phandle.  We need to provide some clock as the mmc1's hclk.  I do not
>> > > understand why this has to be "clk_null", though.  It seems like this
>> > > should be a real clock coming from one of the real clock_controller
>> > > nodes.  After all, the mmc driver is going to be enabling/disabling
>> > > this clock for power savings at runtime.  What does that even mean for
>> > > clk_null ?
>> >
>> > The original purpose of this patch is to provide a common dummy clock
>> > for both software don't care clock and clock that is not software
>> > controllable.I got comments that device tree should describe hardware
>> > and should put exact clock in device tree. I think this is true. So we
>> > will remove this clock_null patch, and:
>> >
>> > 1. For Mediatek SoC CCF driver, James will clarify clock usage further.
>> > Actually, we still think it's not necessary to describe whole tree that
>> > software don't care, James will deal this in clock driver.
>>
>> I think that aswell since the device tree is not affected in this case.
>> Should we realize later that we indeed need the missing clocks we can
>> still implement them without modifying the device tree.
>>
>> >
>> > 2. For other module that use SW not controllable clock (mmc case
>> > mentioned by Dan), because this is a 

Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-10 Thread Eddie Huang
Hi all,

On Wed, 2015-07-08 at 13:44 +0800, Sascha Hauer wrote:
> On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote:
> > On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
> > > On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer  
> > > wrote:
> > > > On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
> > > >> On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer  
> > > >> wrote:
> > > >> > On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
> > > >> >> Add clk_null, which represents clocks that can not / need not
> > > >> >> controlled by software.
> > > >> >> There are many clocks' parent set to clk_null.
> > > >> >>
> > > >> >> Signed-off-by: James Liao 
> > > >> >> Signed-off-by: Eddie Huang 
> > > >> >> ---
> > > >> >> Base on 4.1-rc1
> > > >> >>
> > > >> >> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
> > > >> >> ---
> > > >> >>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
> > > >> >>  1 file changed, 6 insertions(+)
> > > >> >>
> > > >> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
> > > >> >> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > > >> >> index 924fdb6..4798f44 100644
> > > >> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > > >> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > > >> >> @@ -81,6 +81,12 @@
> > > >> >>   cpu_on= <0x8403>;
> > > >> >>   };
> > > >> >>
> > > >> >> + clk_null: clk_null {
> > > >> >> + compatible = "fixed-clock";
> > > >> >> + clock-frequency = <0>;
> > > >> >> + #clock-cells = <0>;
> > > >> >> + };
> > > >> >
> > > >> > The discussion around this patch shows that we don't want to have 
> > > >> > this
> > > >> > clock in the device tree as it is not a hardware description.
> > > >> >
> > > >> > Ok, fine. Eddie, you told us that the rate of the current clk_null 
> > > >> > children
> > > >> > is not interesting. What's the motivation to send this patch anyway
> > > >> > then? Why can't you keep its children on the orphan list where they 
> > > >> > are
> > > >> > already now?
> > > >> >
> > > >> > Another possibility would be to instantiate the clk_null clock from C
> > > >> > code rather than from the device tree. This way we wouldn't put any
> > > >> > wrong descriptions into the device tree and still can implement the
> > > >> > support for the real parent clocks when we actually need them.
> > > >>
> > > >> Some device nodes, like mmc, use a clk_null phandle as one of their 
> > > >> clocks:
> > > >>
> > > >> mmc1: mmc@1124 {
> > > >> compatible = "mediatek,mt8173-mmc",
> > > >>  "mediatek,mt8135-mmc";
> > > >> reg = <0 0x1124 0 0x1000>;
> > > >> interrupts = ;
> > > >> clocks = < CLK_PERI_MSDC30_1>,
> > > >>  <_null>;
> > > >> clock-names = "source", "hclk";
> > > >> status = "disabled";
> > > >> };
> > > >
> > > > This is another case than the one we discussed about. In the case above
> > > > I motivated using a dummy clock since the clock exists in the system,
> > > > but is not software controllable. To abstract this from the driver
> > > > (which needs this clock since it exists) we here have the dummy clock.
> > > > However, of course I can't prove the clock is indeed not software
> > > > controllable; that's only the information I have.
> > > 
> > > I was trying to answer your question "What's the motivation to send
> > > this patch anyway?".
> > > The motivation is to send follow on patches that use the clk_null
> > > phandle.  We need to provide some clock as the mmc1's hclk.  I do not
> > > understand why this has to be "clk_null", though.  It seems like this
> > > should be a real clock coming from one of the real clock_controller
> > > nodes.  After all, the mmc driver is going to be enabling/disabling
> > > this clock for power savings at runtime.  What does that even mean for
> > > clk_null ?
> > 
> > The original purpose of this patch is to provide a common dummy clock
> > for both software don't care clock and clock that is not software
> > controllable.I got comments that device tree should describe hardware
> > and should put exact clock in device tree. I think this is true. So we
> > will remove this clock_null patch, and:
> > 
> > 1. For Mediatek SoC CCF driver, James will clarify clock usage further.
> > Actually, we still think it's not necessary to describe whole tree that
> > software don't care, James will deal this in clock driver.
> 
> I think that aswell since the device tree is not affected in this case.
> Should we realize later that we indeed need the missing clocks we can
> still implement them without modifying the device tree.
> 
> > 
> > 2. For other module that use SW not controllable clock (mmc case
> > mentioned by Dan), because this is a real clock, we will put a dummy
> > clock in device tree, like
> > 
> > clk_mmchclk: dummyhclk {
> > compatible = "fixed-clock";
> > clock-frequency = <0>;
> > 

Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-10 Thread Daniel Kurtz
On Fri, Jul 10, 2015 at 3:27 PM, Eddie Huang eddie.hu...@mediatek.com wrote:
 Hi all,

 On Wed, 2015-07-08 at 13:44 +0800, Sascha Hauer wrote:
 On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote:
  On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
   On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer s.ha...@pengutronix.de 
   wrote:
On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer s.ha...@pengutronix.de 
wrote:
 On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
 Add clk_null, which represents clocks that can not / need not
 controlled by software.
 There are many clocks' parent set to clk_null.

 Signed-off-by: James Liao jamesjj.l...@mediatek.com
 Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
 ---
 Base on 4.1-rc1

 Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
 ---
  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
  1 file changed, 6 insertions(+)

 diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
 b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 index 924fdb6..4798f44 100644
 --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 @@ -81,6 +81,12 @@
   cpu_on= 0x8403;
   };

 + clk_null: clk_null {
 + compatible = fixed-clock;
 + clock-frequency = 0;
 + #clock-cells = 0;
 + };

 The discussion around this patch shows that we don't want to have 
 this
 clock in the device tree as it is not a hardware description.

 Ok, fine. Eddie, you told us that the rate of the current clk_null 
 children
 is not interesting. What's the motivation to send this patch anyway
 then? Why can't you keep its children on the orphan list where they 
 are
 already now?

 Another possibility would be to instantiate the clk_null clock from 
 C
 code rather than from the device tree. This way we wouldn't put any
 wrong descriptions into the device tree and still can implement the
 support for the real parent clocks when we actually need them.
   
Some device nodes, like mmc, use a clk_null phandle as one of their 
clocks:
   
mmc1: mmc@1124 {
compatible = mediatek,mt8173-mmc,
 mediatek,mt8135-mmc;
reg = 0 0x1124 0 0x1000;
interrupts = GIC_SPI 72 IRQ_TYPE_LEVEL_LOW;
clocks = pericfg CLK_PERI_MSDC30_1,
 clk_null;
clock-names = source, hclk;
status = disabled;
};
   
This is another case than the one we discussed about. In the case above
I motivated using a dummy clock since the clock exists in the system,
but is not software controllable. To abstract this from the driver
(which needs this clock since it exists) we here have the dummy clock.
However, of course I can't prove the clock is indeed not software
controllable; that's only the information I have.
  
   I was trying to answer your question What's the motivation to send
   this patch anyway?.
   The motivation is to send follow on patches that use the clk_null
   phandle.  We need to provide some clock as the mmc1's hclk.  I do not
   understand why this has to be clk_null, though.  It seems like this
   should be a real clock coming from one of the real clock_controller
   nodes.  After all, the mmc driver is going to be enabling/disabling
   this clock for power savings at runtime.  What does that even mean for
   clk_null ?
 
  The original purpose of this patch is to provide a common dummy clock
  for both software don't care clock and clock that is not software
  controllable.I got comments that device tree should describe hardware
  and should put exact clock in device tree. I think this is true. So we
  will remove this clock_null patch, and:
 
  1. For Mediatek SoC CCF driver, James will clarify clock usage further.
  Actually, we still think it's not necessary to describe whole tree that
  software don't care, James will deal this in clock driver.

 I think that aswell since the device tree is not affected in this case.
 Should we realize later that we indeed need the missing clocks we can
 still implement them without modifying the device tree.

 
  2. For other module that use SW not controllable clock (mmc case
  mentioned by Dan), because this is a real clock, we will put a dummy
  clock in device tree, like
 
  clk_mmchclk: dummyhclk {
  compatible = fixed-clock;
  clock-frequency = 0;
  #clock-cells = 0;
  };
 
  How about this modification ?

 I wouldn't name it 'dummy', this will again raise some eyebrows.


 I got mmc hclk from our designer. HCLK is from AXI Bus directly (sorry,
 I gave wrong information to Dan and Sascha yesterday). Because there is
 no any mux or gate register to 

Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-10 Thread Eddie Huang
Hi all,

On Wed, 2015-07-08 at 13:44 +0800, Sascha Hauer wrote:
 On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote:
  On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
   On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer s.ha...@pengutronix.de 
   wrote:
On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer s.ha...@pengutronix.de 
wrote:
 On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
 Add clk_null, which represents clocks that can not / need not
 controlled by software.
 There are many clocks' parent set to clk_null.

 Signed-off-by: James Liao jamesjj.l...@mediatek.com
 Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
 ---
 Base on 4.1-rc1

 Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
 ---
  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
  1 file changed, 6 insertions(+)

 diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
 b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 index 924fdb6..4798f44 100644
 --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 @@ -81,6 +81,12 @@
   cpu_on= 0x8403;
   };

 + clk_null: clk_null {
 + compatible = fixed-clock;
 + clock-frequency = 0;
 + #clock-cells = 0;
 + };

 The discussion around this patch shows that we don't want to have 
 this
 clock in the device tree as it is not a hardware description.

 Ok, fine. Eddie, you told us that the rate of the current clk_null 
 children
 is not interesting. What's the motivation to send this patch anyway
 then? Why can't you keep its children on the orphan list where they 
 are
 already now?

 Another possibility would be to instantiate the clk_null clock from C
 code rather than from the device tree. This way we wouldn't put any
 wrong descriptions into the device tree and still can implement the
 support for the real parent clocks when we actually need them.
   
Some device nodes, like mmc, use a clk_null phandle as one of their 
clocks:
   
mmc1: mmc@1124 {
compatible = mediatek,mt8173-mmc,
 mediatek,mt8135-mmc;
reg = 0 0x1124 0 0x1000;
interrupts = GIC_SPI 72 IRQ_TYPE_LEVEL_LOW;
clocks = pericfg CLK_PERI_MSDC30_1,
 clk_null;
clock-names = source, hclk;
status = disabled;
};
   
This is another case than the one we discussed about. In the case above
I motivated using a dummy clock since the clock exists in the system,
but is not software controllable. To abstract this from the driver
(which needs this clock since it exists) we here have the dummy clock.
However, of course I can't prove the clock is indeed not software
controllable; that's only the information I have.
   
   I was trying to answer your question What's the motivation to send
   this patch anyway?.
   The motivation is to send follow on patches that use the clk_null
   phandle.  We need to provide some clock as the mmc1's hclk.  I do not
   understand why this has to be clk_null, though.  It seems like this
   should be a real clock coming from one of the real clock_controller
   nodes.  After all, the mmc driver is going to be enabling/disabling
   this clock for power savings at runtime.  What does that even mean for
   clk_null ?
  
  The original purpose of this patch is to provide a common dummy clock
  for both software don't care clock and clock that is not software
  controllable.I got comments that device tree should describe hardware
  and should put exact clock in device tree. I think this is true. So we
  will remove this clock_null patch, and:
  
  1. For Mediatek SoC CCF driver, James will clarify clock usage further.
  Actually, we still think it's not necessary to describe whole tree that
  software don't care, James will deal this in clock driver.
 
 I think that aswell since the device tree is not affected in this case.
 Should we realize later that we indeed need the missing clocks we can
 still implement them without modifying the device tree.
 
  
  2. For other module that use SW not controllable clock (mmc case
  mentioned by Dan), because this is a real clock, we will put a dummy
  clock in device tree, like
  
  clk_mmchclk: dummyhclk {
  compatible = fixed-clock;
  clock-frequency = 0;
  #clock-cells = 0;
  };
  
  How about this modification ?
 
 I wouldn't name it 'dummy', this will again raise some eyebrows.
 

I got mmc hclk from our designer. HCLK is from AXI Bus directly (sorry,
I gave wrong information to Dan and Sascha yesterday). Because there is
no any mux or gate register to control this HCLK, so current
clk-mt8173.c didn't model it. Since I know where this 

Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-10 Thread Eddie Huang
On Fri, 2015-07-10 at 16:11 +0800, Daniel Kurtz wrote:
 On Fri, Jul 10, 2015 at 3:27 PM, Eddie Huang eddie.hu...@mediatek.com wrote:
  Hi all,
 
  On Wed, 2015-07-08 at 13:44 +0800, Sascha Hauer wrote:
  On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote:
   On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer s.ha...@pengutronix.de 
wrote:
 On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
 On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer 
 s.ha...@pengutronix.de wrote:
  On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
  Add clk_null, which represents clocks that can not / need not
  controlled by software.
  There are many clocks' parent set to clk_null.
 
  Signed-off-by: James Liao jamesjj.l...@mediatek.com
  Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
  ---
  Base on 4.1-rc1
 
  Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
  ---
   arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
   1 file changed, 6 insertions(+)
 
  diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
  b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  index 924fdb6..4798f44 100644
  --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  @@ -81,6 +81,12 @@
cpu_on= 0x8403;
};
 
  + clk_null: clk_null {
  + compatible = fixed-clock;
  + clock-frequency = 0;
  + #clock-cells = 0;
  + };
 
  The discussion around this patch shows that we don't want to have 
  this
  clock in the device tree as it is not a hardware description.
 
  Ok, fine. Eddie, you told us that the rate of the current 
  clk_null children
  is not interesting. What's the motivation to send this patch 
  anyway
  then? Why can't you keep its children on the orphan list where 
  they are
  already now?
 
  Another possibility would be to instantiate the clk_null clock 
  from C
  code rather than from the device tree. This way we wouldn't put 
  any
  wrong descriptions into the device tree and still can implement 
  the
  support for the real parent clocks when we actually need them.

 Some device nodes, like mmc, use a clk_null phandle as one of their 
 clocks:

 mmc1: mmc@1124 {
 compatible = mediatek,mt8173-mmc,
  mediatek,mt8135-mmc;
 reg = 0 0x1124 0 0x1000;
 interrupts = GIC_SPI 72 IRQ_TYPE_LEVEL_LOW;
 clocks = pericfg CLK_PERI_MSDC30_1,
  clk_null;
 clock-names = source, hclk;
 status = disabled;
 };

 This is another case than the one we discussed about. In the case 
 above
 I motivated using a dummy clock since the clock exists in the system,
 but is not software controllable. To abstract this from the driver
 (which needs this clock since it exists) we here have the dummy 
 clock.
 However, of course I can't prove the clock is indeed not software
 controllable; that's only the information I have.
   
I was trying to answer your question What's the motivation to send
this patch anyway?.
The motivation is to send follow on patches that use the clk_null
phandle.  We need to provide some clock as the mmc1's hclk.  I do not
understand why this has to be clk_null, though.  It seems like this
should be a real clock coming from one of the real clock_controller
nodes.  After all, the mmc driver is going to be enabling/disabling
this clock for power savings at runtime.  What does that even mean for
clk_null ?
  
   The original purpose of this patch is to provide a common dummy clock
   for both software don't care clock and clock that is not software
   controllable.I got comments that device tree should describe hardware
   and should put exact clock in device tree. I think this is true. So we
   will remove this clock_null patch, and:
  
   1. For Mediatek SoC CCF driver, James will clarify clock usage further.
   Actually, we still think it's not necessary to describe whole tree that
   software don't care, James will deal this in clock driver.
 
  I think that aswell since the device tree is not affected in this case.
  Should we realize later that we indeed need the missing clocks we can
  still implement them without modifying the device tree.
 
  
   2. For other module that use SW not controllable clock (mmc case
   mentioned by Dan), because this is a real clock, we will put a dummy
   clock in device tree, like
  
   clk_mmchclk: dummyhclk {
   compatible = fixed-clock;
   clock-frequency = 0;
   #clock-cells = 0;
   };
  
   How about this modification ?
 
  I wouldn't name it 'dummy', this will again 

Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Sascha Hauer
On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote:
> On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
> > On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer  
> > wrote:
> > > On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
> > >> On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer  
> > >> wrote:
> > >> > On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
> > >> >> Add clk_null, which represents clocks that can not / need not
> > >> >> controlled by software.
> > >> >> There are many clocks' parent set to clk_null.
> > >> >>
> > >> >> Signed-off-by: James Liao 
> > >> >> Signed-off-by: Eddie Huang 
> > >> >> ---
> > >> >> Base on 4.1-rc1
> > >> >>
> > >> >> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
> > >> >> ---
> > >> >>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
> > >> >>  1 file changed, 6 insertions(+)
> > >> >>
> > >> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
> > >> >> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > >> >> index 924fdb6..4798f44 100644
> > >> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > >> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > >> >> @@ -81,6 +81,12 @@
> > >> >>   cpu_on= <0x8403>;
> > >> >>   };
> > >> >>
> > >> >> + clk_null: clk_null {
> > >> >> + compatible = "fixed-clock";
> > >> >> + clock-frequency = <0>;
> > >> >> + #clock-cells = <0>;
> > >> >> + };
> > >> >
> > >> > The discussion around this patch shows that we don't want to have this
> > >> > clock in the device tree as it is not a hardware description.
> > >> >
> > >> > Ok, fine. Eddie, you told us that the rate of the current clk_null 
> > >> > children
> > >> > is not interesting. What's the motivation to send this patch anyway
> > >> > then? Why can't you keep its children on the orphan list where they are
> > >> > already now?
> > >> >
> > >> > Another possibility would be to instantiate the clk_null clock from C
> > >> > code rather than from the device tree. This way we wouldn't put any
> > >> > wrong descriptions into the device tree and still can implement the
> > >> > support for the real parent clocks when we actually need them.
> > >>
> > >> Some device nodes, like mmc, use a clk_null phandle as one of their 
> > >> clocks:
> > >>
> > >> mmc1: mmc@1124 {
> > >> compatible = "mediatek,mt8173-mmc",
> > >>  "mediatek,mt8135-mmc";
> > >> reg = <0 0x1124 0 0x1000>;
> > >> interrupts = ;
> > >> clocks = < CLK_PERI_MSDC30_1>,
> > >>  <_null>;
> > >> clock-names = "source", "hclk";
> > >> status = "disabled";
> > >> };
> > >
> > > This is another case than the one we discussed about. In the case above
> > > I motivated using a dummy clock since the clock exists in the system,
> > > but is not software controllable. To abstract this from the driver
> > > (which needs this clock since it exists) we here have the dummy clock.
> > > However, of course I can't prove the clock is indeed not software
> > > controllable; that's only the information I have.
> > 
> > I was trying to answer your question "What's the motivation to send
> > this patch anyway?".
> > The motivation is to send follow on patches that use the clk_null
> > phandle.  We need to provide some clock as the mmc1's hclk.  I do not
> > understand why this has to be "clk_null", though.  It seems like this
> > should be a real clock coming from one of the real clock_controller
> > nodes.  After all, the mmc driver is going to be enabling/disabling
> > this clock for power savings at runtime.  What does that even mean for
> > clk_null ?
> 
> The original purpose of this patch is to provide a common dummy clock
> for both software don't care clock and clock that is not software
> controllable.I got comments that device tree should describe hardware
> and should put exact clock in device tree. I think this is true. So we
> will remove this clock_null patch, and:
> 
> 1. For Mediatek SoC CCF driver, James will clarify clock usage further.
> Actually, we still think it's not necessary to describe whole tree that
> software don't care, James will deal this in clock driver.

I think that aswell since the device tree is not affected in this case.
Should we realize later that we indeed need the missing clocks we can
still implement them without modifying the device tree.

> 
> 2. For other module that use SW not controllable clock (mmc case
> mentioned by Dan), because this is a real clock, we will put a dummy
> clock in device tree, like
> 
> clk_mmchclk: dummyhclk {
>   compatible = "fixed-clock";
>   clock-frequency = <0>;
>   #clock-cells = <0>;
> };
> 
> How about this modification ?

I wouldn't name it 'dummy', this will again raise some eyebrows.

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  

Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Eddie Huang
On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
> On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer  wrote:
> > On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
> >> On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer  
> >> wrote:
> >> > On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
> >> >> Add clk_null, which represents clocks that can not / need not
> >> >> controlled by software.
> >> >> There are many clocks' parent set to clk_null.
> >> >>
> >> >> Signed-off-by: James Liao 
> >> >> Signed-off-by: Eddie Huang 
> >> >> ---
> >> >> Base on 4.1-rc1
> >> >>
> >> >> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
> >> >> ---
> >> >>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
> >> >>  1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
> >> >> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> >> index 924fdb6..4798f44 100644
> >> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> >> @@ -81,6 +81,12 @@
> >> >>   cpu_on= <0x8403>;
> >> >>   };
> >> >>
> >> >> + clk_null: clk_null {
> >> >> + compatible = "fixed-clock";
> >> >> + clock-frequency = <0>;
> >> >> + #clock-cells = <0>;
> >> >> + };
> >> >
> >> > The discussion around this patch shows that we don't want to have this
> >> > clock in the device tree as it is not a hardware description.
> >> >
> >> > Ok, fine. Eddie, you told us that the rate of the current clk_null 
> >> > children
> >> > is not interesting. What's the motivation to send this patch anyway
> >> > then? Why can't you keep its children on the orphan list where they are
> >> > already now?
> >> >
> >> > Another possibility would be to instantiate the clk_null clock from C
> >> > code rather than from the device tree. This way we wouldn't put any
> >> > wrong descriptions into the device tree and still can implement the
> >> > support for the real parent clocks when we actually need them.
> >>
> >> Some device nodes, like mmc, use a clk_null phandle as one of their clocks:
> >>
> >> mmc1: mmc@1124 {
> >> compatible = "mediatek,mt8173-mmc",
> >>  "mediatek,mt8135-mmc";
> >> reg = <0 0x1124 0 0x1000>;
> >> interrupts = ;
> >> clocks = < CLK_PERI_MSDC30_1>,
> >>  <_null>;
> >> clock-names = "source", "hclk";
> >> status = "disabled";
> >> };
> >
> > This is another case than the one we discussed about. In the case above
> > I motivated using a dummy clock since the clock exists in the system,
> > but is not software controllable. To abstract this from the driver
> > (which needs this clock since it exists) we here have the dummy clock.
> > However, of course I can't prove the clock is indeed not software
> > controllable; that's only the information I have.
> 
> I was trying to answer your question "What's the motivation to send
> this patch anyway?".
> The motivation is to send follow on patches that use the clk_null
> phandle.  We need to provide some clock as the mmc1's hclk.  I do not
> understand why this has to be "clk_null", though.  It seems like this
> should be a real clock coming from one of the real clock_controller
> nodes.  After all, the mmc driver is going to be enabling/disabling
> this clock for power savings at runtime.  What does that even mean for
> clk_null ?

The original purpose of this patch is to provide a common dummy clock
for both software don't care clock and clock that is not software
controllable.I got comments that device tree should describe hardware
and should put exact clock in device tree. I think this is true. So we
will remove this clock_null patch, and:

1. For Mediatek SoC CCF driver, James will clarify clock usage further.
Actually, we still think it's not necessary to describe whole tree that
software don't care, James will deal this in clock driver.

2. For other module that use SW not controllable clock (mmc case
mentioned by Dan), because this is a real clock, we will put a dummy
clock in device tree, like

clk_mmchclk: dummyhclk {
compatible = "fixed-clock";
clock-frequency = <0>;
#clock-cells = <0>;
};

How about this modification ?

Thanks
Eddie


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Daniel Kurtz
On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer  wrote:
> On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
>> On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer  wrote:
>> > On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
>> >> Add clk_null, which represents clocks that can not / need not
>> >> controlled by software.
>> >> There are many clocks' parent set to clk_null.
>> >>
>> >> Signed-off-by: James Liao 
>> >> Signed-off-by: Eddie Huang 
>> >> ---
>> >> Base on 4.1-rc1
>> >>
>> >> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
>> >> ---
>> >>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
>> >>  1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
>> >> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> >> index 924fdb6..4798f44 100644
>> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> >> @@ -81,6 +81,12 @@
>> >>   cpu_on= <0x8403>;
>> >>   };
>> >>
>> >> + clk_null: clk_null {
>> >> + compatible = "fixed-clock";
>> >> + clock-frequency = <0>;
>> >> + #clock-cells = <0>;
>> >> + };
>> >
>> > The discussion around this patch shows that we don't want to have this
>> > clock in the device tree as it is not a hardware description.
>> >
>> > Ok, fine. Eddie, you told us that the rate of the current clk_null children
>> > is not interesting. What's the motivation to send this patch anyway
>> > then? Why can't you keep its children on the orphan list where they are
>> > already now?
>> >
>> > Another possibility would be to instantiate the clk_null clock from C
>> > code rather than from the device tree. This way we wouldn't put any
>> > wrong descriptions into the device tree and still can implement the
>> > support for the real parent clocks when we actually need them.
>>
>> Some device nodes, like mmc, use a clk_null phandle as one of their clocks:
>>
>> mmc1: mmc@1124 {
>> compatible = "mediatek,mt8173-mmc",
>>  "mediatek,mt8135-mmc";
>> reg = <0 0x1124 0 0x1000>;
>> interrupts = ;
>> clocks = < CLK_PERI_MSDC30_1>,
>>  <_null>;
>> clock-names = "source", "hclk";
>> status = "disabled";
>> };
>
> This is another case than the one we discussed about. In the case above
> I motivated using a dummy clock since the clock exists in the system,
> but is not software controllable. To abstract this from the driver
> (which needs this clock since it exists) we here have the dummy clock.
> However, of course I can't prove the clock is indeed not software
> controllable; that's only the information I have.

I was trying to answer your question "What's the motivation to send
this patch anyway?".
The motivation is to send follow on patches that use the clk_null
phandle.  We need to provide some clock as the mmc1's hclk.  I do not
understand why this has to be "clk_null", though.  It seems like this
should be a real clock coming from one of the real clock_controller
nodes.  After all, the mmc driver is going to be enabling/disabling
this clock for power savings at runtime.  What does that even mean for
clk_null ?

Sorry, I'm not exactly sure what you are saying in your last reply.

>
> Sascha
>
> --
> Pengutronix e.K.   | |
> Industrial Linux Solutions | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Sascha Hauer
On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
> On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer  wrote:
> > On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
> >> Add clk_null, which represents clocks that can not / need not
> >> controlled by software.
> >> There are many clocks' parent set to clk_null.
> >>
> >> Signed-off-by: James Liao 
> >> Signed-off-by: Eddie Huang 
> >> ---
> >> Base on 4.1-rc1
> >>
> >> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
> >> ---
> >>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
> >> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> index 924fdb6..4798f44 100644
> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> @@ -81,6 +81,12 @@
> >>   cpu_on= <0x8403>;
> >>   };
> >>
> >> + clk_null: clk_null {
> >> + compatible = "fixed-clock";
> >> + clock-frequency = <0>;
> >> + #clock-cells = <0>;
> >> + };
> >
> > The discussion around this patch shows that we don't want to have this
> > clock in the device tree as it is not a hardware description.
> >
> > Ok, fine. Eddie, you told us that the rate of the current clk_null children
> > is not interesting. What's the motivation to send this patch anyway
> > then? Why can't you keep its children on the orphan list where they are
> > already now?
> >
> > Another possibility would be to instantiate the clk_null clock from C
> > code rather than from the device tree. This way we wouldn't put any
> > wrong descriptions into the device tree and still can implement the
> > support for the real parent clocks when we actually need them.
> 
> Some device nodes, like mmc, use a clk_null phandle as one of their clocks:
> 
> mmc1: mmc@1124 {
> compatible = "mediatek,mt8173-mmc",
>  "mediatek,mt8135-mmc";
> reg = <0 0x1124 0 0x1000>;
> interrupts = ;
> clocks = < CLK_PERI_MSDC30_1>,
>  <_null>;
> clock-names = "source", "hclk";
> status = "disabled";
> };

This is another case than the one we discussed about. In the case above
I motivated using a dummy clock since the clock exists in the system,
but is not software controllable. To abstract this from the driver
(which needs this clock since it exists) we here have the dummy clock.
However, of course I can't prove the clock is indeed not software
controllable; that's only the information I have.

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Daniel Kurtz
On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer  wrote:
> On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
>> Add clk_null, which represents clocks that can not / need not
>> controlled by software.
>> There are many clocks' parent set to clk_null.
>>
>> Signed-off-by: James Liao 
>> Signed-off-by: Eddie Huang 
>> ---
>> Base on 4.1-rc1
>>
>> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
>> ---
>>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
>> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> index 924fdb6..4798f44 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> @@ -81,6 +81,12 @@
>>   cpu_on= <0x8403>;
>>   };
>>
>> + clk_null: clk_null {
>> + compatible = "fixed-clock";
>> + clock-frequency = <0>;
>> + #clock-cells = <0>;
>> + };
>
> The discussion around this patch shows that we don't want to have this
> clock in the device tree as it is not a hardware description.
>
> Ok, fine. Eddie, you told us that the rate of the current clk_null children
> is not interesting. What's the motivation to send this patch anyway
> then? Why can't you keep its children on the orphan list where they are
> already now?
>
> Another possibility would be to instantiate the clk_null clock from C
> code rather than from the device tree. This way we wouldn't put any
> wrong descriptions into the device tree and still can implement the
> support for the real parent clocks when we actually need them.

Some device nodes, like mmc, use a clk_null phandle as one of their clocks:

mmc1: mmc@1124 {
compatible = "mediatek,mt8173-mmc",
 "mediatek,mt8135-mmc";
reg = <0 0x1124 0 0x1000>;
interrupts = ;
clocks = < CLK_PERI_MSDC30_1>,
 <_null>;
clock-names = "source", "hclk";
status = "disabled";
};

-Dan


> Sascha
>
> --
> Pengutronix e.K.   | |
> Industrial Linux Solutions | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Sascha Hauer
On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
> Add clk_null, which represents clocks that can not / need not
> controlled by software.
> There are many clocks' parent set to clk_null.
> 
> Signed-off-by: James Liao 
> Signed-off-by: Eddie Huang 
> ---
> Base on 4.1-rc1
> 
> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
> ---
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index 924fdb6..4798f44 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -81,6 +81,12 @@
>   cpu_on= <0x8403>;
>   };
>  
> + clk_null: clk_null {
> + compatible = "fixed-clock";
> + clock-frequency = <0>;
> + #clock-cells = <0>;
> + };

The discussion around this patch shows that we don't want to have this
clock in the device tree as it is not a hardware description.

Ok, fine. Eddie, you told us that the rate of the current clk_null children
is not interesting. What's the motivation to send this patch anyway
then? Why can't you keep its children on the orphan list where they are
already now?

Another possibility would be to instantiate the clk_null clock from C
code rather than from the device tree. This way we wouldn't put any
wrong descriptions into the device tree and still can implement the
support for the real parent clocks when we actually need them.

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Daniel Kurtz
On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
 On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
 On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
  On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
  Add clk_null, which represents clocks that can not / need not
  controlled by software.
  There are many clocks' parent set to clk_null.
 
  Signed-off-by: James Liao jamesjj.l...@mediatek.com
  Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
  ---
  Base on 4.1-rc1
 
  Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
  ---
   arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
   1 file changed, 6 insertions(+)
 
  diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
  b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  index 924fdb6..4798f44 100644
  --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  @@ -81,6 +81,12 @@
cpu_on= 0x8403;
};
 
  + clk_null: clk_null {
  + compatible = fixed-clock;
  + clock-frequency = 0;
  + #clock-cells = 0;
  + };
 
  The discussion around this patch shows that we don't want to have this
  clock in the device tree as it is not a hardware description.
 
  Ok, fine. Eddie, you told us that the rate of the current clk_null children
  is not interesting. What's the motivation to send this patch anyway
  then? Why can't you keep its children on the orphan list where they are
  already now?
 
  Another possibility would be to instantiate the clk_null clock from C
  code rather than from the device tree. This way we wouldn't put any
  wrong descriptions into the device tree and still can implement the
  support for the real parent clocks when we actually need them.

 Some device nodes, like mmc, use a clk_null phandle as one of their clocks:

 mmc1: mmc@1124 {
 compatible = mediatek,mt8173-mmc,
  mediatek,mt8135-mmc;
 reg = 0 0x1124 0 0x1000;
 interrupts = GIC_SPI 72 IRQ_TYPE_LEVEL_LOW;
 clocks = pericfg CLK_PERI_MSDC30_1,
  clk_null;
 clock-names = source, hclk;
 status = disabled;
 };

 This is another case than the one we discussed about. In the case above
 I motivated using a dummy clock since the clock exists in the system,
 but is not software controllable. To abstract this from the driver
 (which needs this clock since it exists) we here have the dummy clock.
 However, of course I can't prove the clock is indeed not software
 controllable; that's only the information I have.

I was trying to answer your question What's the motivation to send
this patch anyway?.
The motivation is to send follow on patches that use the clk_null
phandle.  We need to provide some clock as the mmc1's hclk.  I do not
understand why this has to be clk_null, though.  It seems like this
should be a real clock coming from one of the real clock_controller
nodes.  After all, the mmc driver is going to be enabling/disabling
this clock for power savings at runtime.  What does that even mean for
clk_null ?

Sorry, I'm not exactly sure what you are saying in your last reply.


 Sascha

 --
 Pengutronix e.K.   | |
 Industrial Linux Solutions | http://www.pengutronix.de/  |
 Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
 Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Eddie Huang
On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
 On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
  On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
  On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer s.ha...@pengutronix.de 
  wrote:
   On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
   Add clk_null, which represents clocks that can not / need not
   controlled by software.
   There are many clocks' parent set to clk_null.
  
   Signed-off-by: James Liao jamesjj.l...@mediatek.com
   Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
   ---
   Base on 4.1-rc1
  
   Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
   ---
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
1 file changed, 6 insertions(+)
  
   diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
   b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
   index 924fdb6..4798f44 100644
   --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
   +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
   @@ -81,6 +81,12 @@
 cpu_on= 0x8403;
 };
  
   + clk_null: clk_null {
   + compatible = fixed-clock;
   + clock-frequency = 0;
   + #clock-cells = 0;
   + };
  
   The discussion around this patch shows that we don't want to have this
   clock in the device tree as it is not a hardware description.
  
   Ok, fine. Eddie, you told us that the rate of the current clk_null 
   children
   is not interesting. What's the motivation to send this patch anyway
   then? Why can't you keep its children on the orphan list where they are
   already now?
  
   Another possibility would be to instantiate the clk_null clock from C
   code rather than from the device tree. This way we wouldn't put any
   wrong descriptions into the device tree and still can implement the
   support for the real parent clocks when we actually need them.
 
  Some device nodes, like mmc, use a clk_null phandle as one of their clocks:
 
  mmc1: mmc@1124 {
  compatible = mediatek,mt8173-mmc,
   mediatek,mt8135-mmc;
  reg = 0 0x1124 0 0x1000;
  interrupts = GIC_SPI 72 IRQ_TYPE_LEVEL_LOW;
  clocks = pericfg CLK_PERI_MSDC30_1,
   clk_null;
  clock-names = source, hclk;
  status = disabled;
  };
 
  This is another case than the one we discussed about. In the case above
  I motivated using a dummy clock since the clock exists in the system,
  but is not software controllable. To abstract this from the driver
  (which needs this clock since it exists) we here have the dummy clock.
  However, of course I can't prove the clock is indeed not software
  controllable; that's only the information I have.
 
 I was trying to answer your question What's the motivation to send
 this patch anyway?.
 The motivation is to send follow on patches that use the clk_null
 phandle.  We need to provide some clock as the mmc1's hclk.  I do not
 understand why this has to be clk_null, though.  It seems like this
 should be a real clock coming from one of the real clock_controller
 nodes.  After all, the mmc driver is going to be enabling/disabling
 this clock for power savings at runtime.  What does that even mean for
 clk_null ?

The original purpose of this patch is to provide a common dummy clock
for both software don't care clock and clock that is not software
controllable.I got comments that device tree should describe hardware
and should put exact clock in device tree. I think this is true. So we
will remove this clock_null patch, and:

1. For Mediatek SoC CCF driver, James will clarify clock usage further.
Actually, we still think it's not necessary to describe whole tree that
software don't care, James will deal this in clock driver.

2. For other module that use SW not controllable clock (mmc case
mentioned by Dan), because this is a real clock, we will put a dummy
clock in device tree, like

clk_mmchclk: dummyhclk {
compatible = fixed-clock;
clock-frequency = 0;
#clock-cells = 0;
};

How about this modification ?

Thanks
Eddie


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Daniel Kurtz
On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
 On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
 Add clk_null, which represents clocks that can not / need not
 controlled by software.
 There are many clocks' parent set to clk_null.

 Signed-off-by: James Liao jamesjj.l...@mediatek.com
 Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
 ---
 Base on 4.1-rc1

 Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
 ---
  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
  1 file changed, 6 insertions(+)

 diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
 b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 index 924fdb6..4798f44 100644
 --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 @@ -81,6 +81,12 @@
   cpu_on= 0x8403;
   };

 + clk_null: clk_null {
 + compatible = fixed-clock;
 + clock-frequency = 0;
 + #clock-cells = 0;
 + };

 The discussion around this patch shows that we don't want to have this
 clock in the device tree as it is not a hardware description.

 Ok, fine. Eddie, you told us that the rate of the current clk_null children
 is not interesting. What's the motivation to send this patch anyway
 then? Why can't you keep its children on the orphan list where they are
 already now?

 Another possibility would be to instantiate the clk_null clock from C
 code rather than from the device tree. This way we wouldn't put any
 wrong descriptions into the device tree and still can implement the
 support for the real parent clocks when we actually need them.

Some device nodes, like mmc, use a clk_null phandle as one of their clocks:

mmc1: mmc@1124 {
compatible = mediatek,mt8173-mmc,
 mediatek,mt8135-mmc;
reg = 0 0x1124 0 0x1000;
interrupts = GIC_SPI 72 IRQ_TYPE_LEVEL_LOW;
clocks = pericfg CLK_PERI_MSDC30_1,
 clk_null;
clock-names = source, hclk;
status = disabled;
};

-Dan


 Sascha

 --
 Pengutronix e.K.   | |
 Industrial Linux Solutions | http://www.pengutronix.de/  |
 Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
 Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Sascha Hauer
On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
 Add clk_null, which represents clocks that can not / need not
 controlled by software.
 There are many clocks' parent set to clk_null.
 
 Signed-off-by: James Liao jamesjj.l...@mediatek.com
 Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
 ---
 Base on 4.1-rc1
 
 Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
 ---
  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
 b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 index 924fdb6..4798f44 100644
 --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 @@ -81,6 +81,12 @@
   cpu_on= 0x8403;
   };
  
 + clk_null: clk_null {
 + compatible = fixed-clock;
 + clock-frequency = 0;
 + #clock-cells = 0;
 + };

The discussion around this patch shows that we don't want to have this
clock in the device tree as it is not a hardware description.

Ok, fine. Eddie, you told us that the rate of the current clk_null children
is not interesting. What's the motivation to send this patch anyway
then? Why can't you keep its children on the orphan list where they are
already now?

Another possibility would be to instantiate the clk_null clock from C
code rather than from the device tree. This way we wouldn't put any
wrong descriptions into the device tree and still can implement the
support for the real parent clocks when we actually need them.

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Sascha Hauer
On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote:
 On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote:
  On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer s.ha...@pengutronix.de 
  wrote:
   On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
   On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer s.ha...@pengutronix.de 
   wrote:
On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
Add clk_null, which represents clocks that can not / need not
controlled by software.
There are many clocks' parent set to clk_null.
   
Signed-off-by: James Liao jamesjj.l...@mediatek.com
Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
---
Base on 4.1-rc1
   
Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
 1 file changed, 6 insertions(+)
   
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 924fdb6..4798f44 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -81,6 +81,12 @@
  cpu_on= 0x8403;
  };
   
+ clk_null: clk_null {
+ compatible = fixed-clock;
+ clock-frequency = 0;
+ #clock-cells = 0;
+ };
   
The discussion around this patch shows that we don't want to have this
clock in the device tree as it is not a hardware description.
   
Ok, fine. Eddie, you told us that the rate of the current clk_null 
children
is not interesting. What's the motivation to send this patch anyway
then? Why can't you keep its children on the orphan list where they are
already now?
   
Another possibility would be to instantiate the clk_null clock from C
code rather than from the device tree. This way we wouldn't put any
wrong descriptions into the device tree and still can implement the
support for the real parent clocks when we actually need them.
  
   Some device nodes, like mmc, use a clk_null phandle as one of their 
   clocks:
  
   mmc1: mmc@1124 {
   compatible = mediatek,mt8173-mmc,
mediatek,mt8135-mmc;
   reg = 0 0x1124 0 0x1000;
   interrupts = GIC_SPI 72 IRQ_TYPE_LEVEL_LOW;
   clocks = pericfg CLK_PERI_MSDC30_1,
clk_null;
   clock-names = source, hclk;
   status = disabled;
   };
  
   This is another case than the one we discussed about. In the case above
   I motivated using a dummy clock since the clock exists in the system,
   but is not software controllable. To abstract this from the driver
   (which needs this clock since it exists) we here have the dummy clock.
   However, of course I can't prove the clock is indeed not software
   controllable; that's only the information I have.
  
  I was trying to answer your question What's the motivation to send
  this patch anyway?.
  The motivation is to send follow on patches that use the clk_null
  phandle.  We need to provide some clock as the mmc1's hclk.  I do not
  understand why this has to be clk_null, though.  It seems like this
  should be a real clock coming from one of the real clock_controller
  nodes.  After all, the mmc driver is going to be enabling/disabling
  this clock for power savings at runtime.  What does that even mean for
  clk_null ?
 
 The original purpose of this patch is to provide a common dummy clock
 for both software don't care clock and clock that is not software
 controllable.I got comments that device tree should describe hardware
 and should put exact clock in device tree. I think this is true. So we
 will remove this clock_null patch, and:
 
 1. For Mediatek SoC CCF driver, James will clarify clock usage further.
 Actually, we still think it's not necessary to describe whole tree that
 software don't care, James will deal this in clock driver.

I think that aswell since the device tree is not affected in this case.
Should we realize later that we indeed need the missing clocks we can
still implement them without modifying the device tree.

 
 2. For other module that use SW not controllable clock (mmc case
 mentioned by Dan), because this is a real clock, we will put a dummy
 clock in device tree, like
 
 clk_mmchclk: dummyhclk {
   compatible = fixed-clock;
   clock-frequency = 0;
   #clock-cells = 0;
 };
 
 How about this modification ?

I wouldn't name it 'dummy', this will again raise some eyebrows.

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org

Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-07 Thread Sascha Hauer
On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote:
 On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
  On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote:
  Add clk_null, which represents clocks that can not / need not
  controlled by software.
  There are many clocks' parent set to clk_null.
 
  Signed-off-by: James Liao jamesjj.l...@mediatek.com
  Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
  ---
  Base on 4.1-rc1
 
  Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
  ---
   arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
   1 file changed, 6 insertions(+)
 
  diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi 
  b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  index 924fdb6..4798f44 100644
  --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
  @@ -81,6 +81,12 @@
cpu_on= 0x8403;
};
 
  + clk_null: clk_null {
  + compatible = fixed-clock;
  + clock-frequency = 0;
  + #clock-cells = 0;
  + };
 
  The discussion around this patch shows that we don't want to have this
  clock in the device tree as it is not a hardware description.
 
  Ok, fine. Eddie, you told us that the rate of the current clk_null children
  is not interesting. What's the motivation to send this patch anyway
  then? Why can't you keep its children on the orphan list where they are
  already now?
 
  Another possibility would be to instantiate the clk_null clock from C
  code rather than from the device tree. This way we wouldn't put any
  wrong descriptions into the device tree and still can implement the
  support for the real parent clocks when we actually need them.
 
 Some device nodes, like mmc, use a clk_null phandle as one of their clocks:
 
 mmc1: mmc@1124 {
 compatible = mediatek,mt8173-mmc,
  mediatek,mt8135-mmc;
 reg = 0 0x1124 0 0x1000;
 interrupts = GIC_SPI 72 IRQ_TYPE_LEVEL_LOW;
 clocks = pericfg CLK_PERI_MSDC30_1,
  clk_null;
 clock-names = source, hclk;
 status = disabled;
 };

This is another case than the one we discussed about. In the case above
I motivated using a dummy clock since the clock exists in the system,
but is not software controllable. To abstract this from the driver
(which needs this clock since it exists) we here have the dummy clock.
However, of course I can't prove the clock is indeed not software
controllable; that's only the information I have.

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-03 Thread Heiko Stübner
Am Freitag, 3. Juli 2015, 15:45:12 schrieb James Liao:
> On Thu, 2015-07-02 at 12:23 +0800, Daniel Kurtz wrote:
> > On Thu, Jul 2, 2015 at 11:06 AM, James Liao  
wrote:
> > > These clocks such as clkph_mck_o are configured by other modules before
> > > kernel init, and their rates may different among platforms.
> > 
> > What other modules?
> > Do you mean the rates are configured by firmware?
> > How are the rates set?
> > Are there registers that configure its rate?
> > If so, why can't the kernel read these registers and compute the current
> > rate?
> CLKPH_MCK_O for example, it's a DRAM related clock, and initialized in
> preloader (before kernel init). It's setting may be different because
> using different DRAM module. And the setting may be changed dynamically
> in runtime.

If it's set in the preloader (and maybe changed at runtime) this means, its 
setting can also be read back to determine its current clock rate, so this is 
a non-argument here ;-)


> We don't care CLKPH_MCK_O's rate in CCF because we don't need to change
> mem_sel's setting in kernel, and there is not driver need to know
> mem_sel's rate.

As Daniel said, some of these clocks may be parents of other clocks, and not 
knowing their rate might hurt in the future - especially when it's easy after 
all to get its actual value like in your CLKPH_MCK_O example above.


Heiko


> > For mt8173, we are essentially discussing the following clocks (whose
> > sole parent is clk_null):
> > 
> > FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
> > FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
> > FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
> > FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),
> > GATE_ICG(CLK_INFRA_CPUM, "infra_cpum", "clk_null", 15),
> > GATE_MM1(CLK_MM_DSI0_DIGITAL, "mm_dsi0_digital", "clk_null", 5),
> > GATE_MM1(CLK_MM_DSI1_DIGITAL, "mm_dsi1_digital", "clk_null", 7),
> > GATE_MM1(CLK_MM_DPI1_PIXEL, "mm_dpi1_pixel", "clk_null", 10),
> > GATE_MM1(CLK_MM_LVDS_PIXEL, "mm_lvds_pixel", "clk_null", 16),
> > GATE_MM1(CLK_MM_LVDS_CTS, "mm_lvds_cts", "clk_null", 17),
> > 
> > clkph_mck_o - This is the parent for dmpll_*, which are themselves
> > (potential) parent clocks for nearly every subsystem.
> > In fact, as shown above, the dmpll_* is the selected parent for
> > several other clocks, which all end up with an unknown rate.
> > So, I think it is worth investigating a little more how to properly
> > read or otherwise specify the rate for clkph_mck_o.
> 
> Please see above.
> 
> > dpi_ck, infra_cpum, mm_dsi0_digital, mm_dsi1_digital, mm_lvds_cts -
> > These are a dead-end (internal?) clock.
> > It is probably fine if their rates are unknown (0 Hz).
> > 
> > usb_syspll_125m - This sounds like a fixed 125 MHz clock.  It is also
> > a possible parent usb30 clock, so its value will propagate.
> > 
> > hdmitx_dig_cts - This is the root clock for the tree leading to
> > mm_hdmi_pllck, which includes hdmitxpll_d* and hdmi_sel.
> > However, I don't know how "mm_hdmi_pllck" is used.
> > 
> > mm_dpi1_pixel, mm_lvds_pixel - These two look very suspicious.  The
> > similar "mm_dpi0_pixel" and "mm_hdmi_pixel" have parent dpi0_sel.
> > It looks like maybe they should have "dpi1_sel" or "dpilvds_sel" as
> > parents, but the _sel are not hooked up.
> 
> Subsystem clocks with parent clk_null may have different reasons. I'll
> get back to you later.
> 
> 
> Best regards,
> 
> James

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-03 Thread James Liao
On Thu, 2015-07-02 at 12:23 +0800, Daniel Kurtz wrote:
> On Thu, Jul 2, 2015 at 11:06 AM, James Liao  wrote:
> > These clocks such as clkph_mck_o are configured by other modules before
> > kernel init, and their rates may different among platforms.
> 
> What other modules?
> Do you mean the rates are configured by firmware?
> How are the rates set?
> Are there registers that configure its rate?
> If so, why can't the kernel read these registers and compute the current rate?

CLKPH_MCK_O for example, it's a DRAM related clock, and initialized in
preloader (before kernel init). It's setting may be different because
using different DRAM module. And the setting may be changed dynamically
in runtime.

We don't care CLKPH_MCK_O's rate in CCF because we don't need to change
mem_sel's setting in kernel, and there is not driver need to know
mem_sel's rate.

> 
> For mt8173, we are essentially discussing the following clocks (whose
> sole parent is clk_null):
> 
> FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
> FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
> FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
> FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),
> GATE_ICG(CLK_INFRA_CPUM, "infra_cpum", "clk_null", 15),
> GATE_MM1(CLK_MM_DSI0_DIGITAL, "mm_dsi0_digital", "clk_null", 5),
> GATE_MM1(CLK_MM_DSI1_DIGITAL, "mm_dsi1_digital", "clk_null", 7),
> GATE_MM1(CLK_MM_DPI1_PIXEL, "mm_dpi1_pixel", "clk_null", 10),
> GATE_MM1(CLK_MM_LVDS_PIXEL, "mm_lvds_pixel", "clk_null", 16),
> GATE_MM1(CLK_MM_LVDS_CTS, "mm_lvds_cts", "clk_null", 17),
> 
> clkph_mck_o - This is the parent for dmpll_*, which are themselves
> (potential) parent clocks for nearly every subsystem.
> In fact, as shown above, the dmpll_* is the selected parent for
> several other clocks, which all end up with an unknown rate.
> So, I think it is worth investigating a little more how to properly
> read or otherwise specify the rate for clkph_mck_o.

Please see above.

> dpi_ck, infra_cpum, mm_dsi0_digital, mm_dsi1_digital, mm_lvds_cts -
> These are a dead-end (internal?) clock.
> It is probably fine if their rates are unknown (0 Hz).
> 
> usb_syspll_125m - This sounds like a fixed 125 MHz clock.  It is also
> a possible parent usb30 clock, so its value will propagate.
> 
> hdmitx_dig_cts - This is the root clock for the tree leading to
> mm_hdmi_pllck, which includes hdmitxpll_d* and hdmi_sel.
> However, I don't know how "mm_hdmi_pllck" is used.
> 
> mm_dpi1_pixel, mm_lvds_pixel - These two look very suspicious.  The
> similar "mm_dpi0_pixel" and "mm_hdmi_pixel" have parent dpi0_sel.
> It looks like maybe they should have "dpi1_sel" or "dpilvds_sel" as
> parents, but the _sel are not hooked up.

Subsystem clocks with parent clk_null may have different reasons. I'll
get back to you later.


Best regards,

James





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-03 Thread Heiko Stübner
Am Freitag, 3. Juli 2015, 15:45:12 schrieb James Liao:
 On Thu, 2015-07-02 at 12:23 +0800, Daniel Kurtz wrote:
  On Thu, Jul 2, 2015 at 11:06 AM, James Liao jamesjj.l...@mediatek.com 
wrote:
   These clocks such as clkph_mck_o are configured by other modules before
   kernel init, and their rates may different among platforms.
  
  What other modules?
  Do you mean the rates are configured by firmware?
  How are the rates set?
  Are there registers that configure its rate?
  If so, why can't the kernel read these registers and compute the current
  rate?
 CLKPH_MCK_O for example, it's a DRAM related clock, and initialized in
 preloader (before kernel init). It's setting may be different because
 using different DRAM module. And the setting may be changed dynamically
 in runtime.

If it's set in the preloader (and maybe changed at runtime) this means, its 
setting can also be read back to determine its current clock rate, so this is 
a non-argument here ;-)


 We don't care CLKPH_MCK_O's rate in CCF because we don't need to change
 mem_sel's setting in kernel, and there is not driver need to know
 mem_sel's rate.

As Daniel said, some of these clocks may be parents of other clocks, and not 
knowing their rate might hurt in the future - especially when it's easy after 
all to get its actual value like in your CLKPH_MCK_O example above.


Heiko


  For mt8173, we are essentially discussing the following clocks (whose
  sole parent is clk_null):
  
  FACTOR(CLK_TOP_CLKPH_MCK_O, clkph_mck_o, clk_null, 1, 1),
  FACTOR(CLK_TOP_DPI, dpi_ck, clk_null, 1, 1),
  FACTOR(CLK_TOP_USB_SYSPLL_125M, usb_syspll_125m, clk_null, 1, 1),
  FACTOR(CLK_TOP_HDMITX_DIG_CTS, hdmitx_dig_cts, clk_null, 1, 1),
  GATE_ICG(CLK_INFRA_CPUM, infra_cpum, clk_null, 15),
  GATE_MM1(CLK_MM_DSI0_DIGITAL, mm_dsi0_digital, clk_null, 5),
  GATE_MM1(CLK_MM_DSI1_DIGITAL, mm_dsi1_digital, clk_null, 7),
  GATE_MM1(CLK_MM_DPI1_PIXEL, mm_dpi1_pixel, clk_null, 10),
  GATE_MM1(CLK_MM_LVDS_PIXEL, mm_lvds_pixel, clk_null, 16),
  GATE_MM1(CLK_MM_LVDS_CTS, mm_lvds_cts, clk_null, 17),
  
  clkph_mck_o - This is the parent for dmpll_*, which are themselves
  (potential) parent clocks for nearly every subsystem.
  In fact, as shown above, the dmpll_* is the selected parent for
  several other clocks, which all end up with an unknown rate.
  So, I think it is worth investigating a little more how to properly
  read or otherwise specify the rate for clkph_mck_o.
 
 Please see above.
 
  dpi_ck, infra_cpum, mm_dsi0_digital, mm_dsi1_digital, mm_lvds_cts -
  These are a dead-end (internal?) clock.
  It is probably fine if their rates are unknown (0 Hz).
  
  usb_syspll_125m - This sounds like a fixed 125 MHz clock.  It is also
  a possible parent usb30 clock, so its value will propagate.
  
  hdmitx_dig_cts - This is the root clock for the tree leading to
  mm_hdmi_pllck, which includes hdmitxpll_d* and hdmi_sel.
  However, I don't know how mm_hdmi_pllck is used.
  
  mm_dpi1_pixel, mm_lvds_pixel - These two look very suspicious.  The
  similar mm_dpi0_pixel and mm_hdmi_pixel have parent dpi0_sel.
  It looks like maybe they should have dpi1_sel or dpilvds_sel as
  parents, but the _sel are not hooked up.
 
 Subsystem clocks with parent clk_null may have different reasons. I'll
 get back to you later.
 
 
 Best regards,
 
 James

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-03 Thread James Liao
On Thu, 2015-07-02 at 12:23 +0800, Daniel Kurtz wrote:
 On Thu, Jul 2, 2015 at 11:06 AM, James Liao jamesjj.l...@mediatek.com wrote:
  These clocks such as clkph_mck_o are configured by other modules before
  kernel init, and their rates may different among platforms.
 
 What other modules?
 Do you mean the rates are configured by firmware?
 How are the rates set?
 Are there registers that configure its rate?
 If so, why can't the kernel read these registers and compute the current rate?

CLKPH_MCK_O for example, it's a DRAM related clock, and initialized in
preloader (before kernel init). It's setting may be different because
using different DRAM module. And the setting may be changed dynamically
in runtime.

We don't care CLKPH_MCK_O's rate in CCF because we don't need to change
mem_sel's setting in kernel, and there is not driver need to know
mem_sel's rate.

 
 For mt8173, we are essentially discussing the following clocks (whose
 sole parent is clk_null):
 
 FACTOR(CLK_TOP_CLKPH_MCK_O, clkph_mck_o, clk_null, 1, 1),
 FACTOR(CLK_TOP_DPI, dpi_ck, clk_null, 1, 1),
 FACTOR(CLK_TOP_USB_SYSPLL_125M, usb_syspll_125m, clk_null, 1, 1),
 FACTOR(CLK_TOP_HDMITX_DIG_CTS, hdmitx_dig_cts, clk_null, 1, 1),
 GATE_ICG(CLK_INFRA_CPUM, infra_cpum, clk_null, 15),
 GATE_MM1(CLK_MM_DSI0_DIGITAL, mm_dsi0_digital, clk_null, 5),
 GATE_MM1(CLK_MM_DSI1_DIGITAL, mm_dsi1_digital, clk_null, 7),
 GATE_MM1(CLK_MM_DPI1_PIXEL, mm_dpi1_pixel, clk_null, 10),
 GATE_MM1(CLK_MM_LVDS_PIXEL, mm_lvds_pixel, clk_null, 16),
 GATE_MM1(CLK_MM_LVDS_CTS, mm_lvds_cts, clk_null, 17),
 
 clkph_mck_o - This is the parent for dmpll_*, which are themselves
 (potential) parent clocks for nearly every subsystem.
 In fact, as shown above, the dmpll_* is the selected parent for
 several other clocks, which all end up with an unknown rate.
 So, I think it is worth investigating a little more how to properly
 read or otherwise specify the rate for clkph_mck_o.

Please see above.

 dpi_ck, infra_cpum, mm_dsi0_digital, mm_dsi1_digital, mm_lvds_cts -
 These are a dead-end (internal?) clock.
 It is probably fine if their rates are unknown (0 Hz).
 
 usb_syspll_125m - This sounds like a fixed 125 MHz clock.  It is also
 a possible parent usb30 clock, so its value will propagate.
 
 hdmitx_dig_cts - This is the root clock for the tree leading to
 mm_hdmi_pllck, which includes hdmitxpll_d* and hdmi_sel.
 However, I don't know how mm_hdmi_pllck is used.
 
 mm_dpi1_pixel, mm_lvds_pixel - These two look very suspicious.  The
 similar mm_dpi0_pixel and mm_hdmi_pixel have parent dpi0_sel.
 It looks like maybe they should have dpi1_sel or dpilvds_sel as
 parents, but the _sel are not hooked up.

Subsystem clocks with parent clk_null may have different reasons. I'll
get back to you later.


Best regards,

James





--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread Daniel Kurtz
On Thu, Jul 2, 2015 at 11:06 AM, James Liao  wrote:
> Hi Daniel,
>
> On Wed, 2015-07-01 at 19:54 +0800, Daniel Kurtz wrote:
>> On Wed, Jul 1, 2015 at 2:49 PM, Sascha Hauer  wrote:
>> > The problem is not that you use fixed clocks for non software
>> > controllable clocks of unknwown rates, but that you try to use a single
>> > clock for all of them and name it 'dummy' or 'null'. Name it
>> >
>> > dpi_ck {
>> > compatible = "fixed-clock";
>> > rate = <0>; /* unknown, generated by some Analog block */
>> > };
>>
>> It would be nice, though, to use the real clock rates.
>> Otherwise, we end up with a bunch of unknown clock rates, like this:
>>
>>clock enable_cnt  prepare_cntrate
>> accuracy   phase
>> 
>>  clk_null 22   0
>>0 0
>> mm_lvds_cts   00   0
>>0 0
>> mm_lvds_pixel 00   0
>>0 0
>> mm_dpi1_pixel 00   0
>>0 0
>
>> Furthermore, at least some of these children clocks are possible
>> source clocks for other clocks for which we do want to know the
>> resulting frequency.  For example, the "dmpll_*" clocks are mux inputs
>> for many of the subsystem clocks.
>
> These clocks such as clkph_mck_o are configured by other modules before
> kernel init, and their rates may different among platforms.

What other modules?
Do you mean the rates are configured by firmware?
How are the rates set?
Are there registers that configure its rate?
If so, why can't the kernel read these registers and compute the current rate?


For mt8173, we are essentially discussing the following clocks (whose
sole parent is clk_null):

FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),
GATE_ICG(CLK_INFRA_CPUM, "infra_cpum", "clk_null", 15),
GATE_MM1(CLK_MM_DSI0_DIGITAL, "mm_dsi0_digital", "clk_null", 5),
GATE_MM1(CLK_MM_DSI1_DIGITAL, "mm_dsi1_digital", "clk_null", 7),
GATE_MM1(CLK_MM_DPI1_PIXEL, "mm_dpi1_pixel", "clk_null", 10),
GATE_MM1(CLK_MM_LVDS_PIXEL, "mm_lvds_pixel", "clk_null", 16),
GATE_MM1(CLK_MM_LVDS_CTS, "mm_lvds_cts", "clk_null", 17),

clkph_mck_o - This is the parent for dmpll_*, which are themselves
(potential) parent clocks for nearly every subsystem.
In fact, as shown above, the dmpll_* is the selected parent for
several other clocks, which all end up with an unknown rate.
So, I think it is worth investigating a little more how to properly
read or otherwise specify the rate for clkph_mck_o.

dpi_ck, infra_cpum, mm_dsi0_digital, mm_dsi1_digital, mm_lvds_cts -
These are a dead-end (internal?) clock.
It is probably fine if their rates are unknown (0 Hz).

usb_syspll_125m - This sounds like a fixed 125 MHz clock.  It is also
a possible parent usb30 clock, so its value will propagate.

hdmitx_dig_cts - This is the root clock for the tree leading to
mm_hdmi_pllck, which includes hdmitxpll_d* and hdmi_sel.
However, I don't know how "mm_hdmi_pllck" is used.

mm_dpi1_pixel, mm_lvds_pixel - These two look very suspicious.  The
similar "mm_dpi0_pixel" and "mm_hdmi_pixel" have parent dpi0_sel.
It looks like maybe they should have "dpi1_sel" or "dpilvds_sel" as
parents, but the _sel are not hooked up.

-Dan

> So we can't
> use a hard-coded rate for them. And we also don't care the actual rate
> of these clocks, so assign a dummy rate such as 0 to them should be a
> better way.
>
>
> Best regards,
>
> james
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread James Liao
Hi Daniel,

On Wed, 2015-07-01 at 19:54 +0800, Daniel Kurtz wrote:
> On Wed, Jul 1, 2015 at 2:49 PM, Sascha Hauer  wrote:
> > The problem is not that you use fixed clocks for non software
> > controllable clocks of unknwown rates, but that you try to use a single
> > clock for all of them and name it 'dummy' or 'null'. Name it
> >
> > dpi_ck {
> > compatible = "fixed-clock";
> > rate = <0>; /* unknown, generated by some Analog block */
> > };
> 
> It would be nice, though, to use the real clock rates.
> Otherwise, we end up with a bunch of unknown clock rates, like this:
> 
>clock enable_cnt  prepare_cntrate
> accuracy   phase
> 
>  clk_null 22   0
>0 0
> mm_lvds_cts   00   0
>0 0
> mm_lvds_pixel 00   0
>0 0
> mm_dpi1_pixel 00   0
>0 0

> Furthermore, at least some of these children clocks are possible
> source clocks for other clocks for which we do want to know the
> resulting frequency.  For example, the "dmpll_*" clocks are mux inputs
> for many of the subsystem clocks.

These clocks such as clkph_mck_o are configured by other modules before
kernel init, and their rates may different among platforms. So we can't
use a hard-coded rate for them. And we also don't care the actual rate
of these clocks, so assign a dummy rate such as 0 to them should be a
better way.


Best regards,

james


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread James Liao
Hi Sascha,

On Wed, 2015-07-01 at 08:49 +0200, Sascha Hauer wrote:
> On Tue, Jun 30, 2015 at 05:07:09PM +0800, James Liao wrote:
> > There are 4 clocks which are derived from clk_null directly in current
> > topckgen implementation:
> > 
> > clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts
> > 
> > Our designer mentioned 2 things about external clocks:
> > 
> > 1. These 4 clocks come from analog macro, not from PLL, nor from
> > external clocks directly.
> 
> Ok, this means there actually are clocks. We can't control these clock and
> they have some known or unknown rate. This makes them fixed clocks. Just
> specify them in the device tree and you are done. Give them reasonable
> names and the rate if you know it, 0 otherwise.
> 
> The problem is not that you use fixed clocks for non software
> controllable clocks of unknwown rates, but that you try to use a single
> clock for all of them and name it 'dummy' or 'null'. Name it
> 
> dpi_ck {
>   compatible = "fixed-clock";
>   rate = <0>; /* unknown, generated by some Analog block */
> };
> 
> Technically it's the same, but it sounds much more professional and like
> you know what you are doing ;)

These clocks are SoC internal clocks. Is it suitable to specify them in
the device tree?

According to your and Heiko's suggestion, it looks the best way should
be specifying these clocks in the driver code without a dummy parent.
i.e.:

Original code:

FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),

New code:

FIXED(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", UNKNOWN_RATE),
FIXED(CLK_TOP_DPI, "dpi_ck", UNKNOWN_RATE),
FIXED(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", UNKNOWN_RATE),
FIXED(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", UNKNOWN_RATE),

Then we don't need to specify a dummy clock such as clk_null in device
tree.


Best regards,

James

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread Daniel Kurtz
On Wed, Jul 1, 2015 at 2:49 PM, Sascha Hauer  wrote:
>
> On Tue, Jun 30, 2015 at 05:07:09PM +0800, James Liao wrote:
> > Hi Heiko,
> >
> > There are 4 clocks which are derived from clk_null directly in current
> > topckgen implementation:
> >
> >   clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts
> >
> > Our designer mentioned 2 things about external clocks:
> >
> > 1. These 4 clocks come from analog macro, not from PLL, nor from
> > external clocks directly.
>
> Ok, this means there actually are clocks. We can't control these clock and
> they have some known or unknown rate. This makes them fixed clocks. Just
> specify them in the device tree and you are done. Give them reasonable
> names and the rate if you know it, 0 otherwise.
>
> The problem is not that you use fixed clocks for non software
> controllable clocks of unknwown rates, but that you try to use a single
> clock for all of them and name it 'dummy' or 'null'. Name it
>
> dpi_ck {
> compatible = "fixed-clock";
> rate = <0>; /* unknown, generated by some Analog block */
> };

It would be nice, though, to use the real clock rates.
Otherwise, we end up with a bunch of unknown clock rates, like this:

   clock enable_cnt  prepare_cntrate
accuracy   phase

 clk_null 22   0
   0 0
mm_lvds_cts   00   0
   0 0
mm_lvds_pixel 00   0
   0 0
mm_dpi1_pixel 00   0
   0 0
mm_dsi1_digital   00   0
   0 0
mm_dsi0_digital   11   0
   0 0
infra_cpum00   0
   0 0
hdmitx_dig_cts00   0
   0 0
   hdmi_sel   00   0
   0 0
  mm_hdmi_pllck   00   0
   0 0
   hdmitxpll_d3   00   0
   0 0
   hdmitxpll_d2   00   0
   0 0
usb_syspll_125m   00   0
   0 0
dpi_ck00   0
   0 0
clkph_mck_o   11   0
   0 0
   dmpll_d16  00   0
   0 0
   dmpll_d8   00   0
   0 0
   dmpll_d4   00   0
   0 0
   dmpll_d2   00   0
   0 0
   dmpll_ck   11   0
   0 0
  mem_sel 22   0
   0 0
 infra_m4u11   0
   0 0

Furthermore, at least some of these children clocks are possible
source clocks for other clocks for which we do want to know the
resulting frequency.  For example, the "dmpll_*" clocks are mux inputs
for many of the subsystem clocks.

-Dan

>
> Technically it's the same, but it sounds much more professional and like
> you know what you are doing ;)
>
> Sascha
>
> --
> Pengutronix e.K.   | |
> Industrial Linux Solutions | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread Sascha Hauer
On Tue, Jun 30, 2015 at 05:07:09PM +0800, James Liao wrote:
> Hi Heiko,
> 
> There are 4 clocks which are derived from clk_null directly in current
> topckgen implementation:
> 
>   clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts
> 
> Our designer mentioned 2 things about external clocks:
> 
> 1. These 4 clocks come from analog macro, not from PLL, nor from
> external clocks directly.

Ok, this means there actually are clocks. We can't control these clock and
they have some known or unknown rate. This makes them fixed clocks. Just
specify them in the device tree and you are done. Give them reasonable
names and the rate if you know it, 0 otherwise.

The problem is not that you use fixed clocks for non software
controllable clocks of unknwown rates, but that you try to use a single
clock for all of them and name it 'dummy' or 'null'. Name it

dpi_ck {
compatible = "fixed-clock";
rate = <0>; /* unknown, generated by some Analog block */
};

Technically it's the same, but it sounds much more professional and like
you know what you are doing ;)

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread Daniel Kurtz
On Wed, Jul 1, 2015 at 2:49 PM, Sascha Hauer s.ha...@pengutronix.de wrote:

 On Tue, Jun 30, 2015 at 05:07:09PM +0800, James Liao wrote:
  Hi Heiko,
 
  There are 4 clocks which are derived from clk_null directly in current
  topckgen implementation:
 
clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts
 
  Our designer mentioned 2 things about external clocks:
 
  1. These 4 clocks come from analog macro, not from PLL, nor from
  external clocks directly.

 Ok, this means there actually are clocks. We can't control these clock and
 they have some known or unknown rate. This makes them fixed clocks. Just
 specify them in the device tree and you are done. Give them reasonable
 names and the rate if you know it, 0 otherwise.

 The problem is not that you use fixed clocks for non software
 controllable clocks of unknwown rates, but that you try to use a single
 clock for all of them and name it 'dummy' or 'null'. Name it

 dpi_ck {
 compatible = fixed-clock;
 rate = 0; /* unknown, generated by some Analog block */
 };

It would be nice, though, to use the real clock rates.
Otherwise, we end up with a bunch of unknown clock rates, like this:

   clock enable_cnt  prepare_cntrate
accuracy   phase

 clk_null 22   0
   0 0
mm_lvds_cts   00   0
   0 0
mm_lvds_pixel 00   0
   0 0
mm_dpi1_pixel 00   0
   0 0
mm_dsi1_digital   00   0
   0 0
mm_dsi0_digital   11   0
   0 0
infra_cpum00   0
   0 0
hdmitx_dig_cts00   0
   0 0
   hdmi_sel   00   0
   0 0
  mm_hdmi_pllck   00   0
   0 0
   hdmitxpll_d3   00   0
   0 0
   hdmitxpll_d2   00   0
   0 0
usb_syspll_125m   00   0
   0 0
dpi_ck00   0
   0 0
clkph_mck_o   11   0
   0 0
   dmpll_d16  00   0
   0 0
   dmpll_d8   00   0
   0 0
   dmpll_d4   00   0
   0 0
   dmpll_d2   00   0
   0 0
   dmpll_ck   11   0
   0 0
  mem_sel 22   0
   0 0
 infra_m4u11   0
   0 0

Furthermore, at least some of these children clocks are possible
source clocks for other clocks for which we do want to know the
resulting frequency.  For example, the dmpll_* clocks are mux inputs
for many of the subsystem clocks.

-Dan


 Technically it's the same, but it sounds much more professional and like
 you know what you are doing ;)

 Sascha

 --
 Pengutronix e.K.   | |
 Industrial Linux Solutions | http://www.pengutronix.de/  |
 Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
 Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread James Liao
Hi Daniel,

On Wed, 2015-07-01 at 19:54 +0800, Daniel Kurtz wrote:
 On Wed, Jul 1, 2015 at 2:49 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
  The problem is not that you use fixed clocks for non software
  controllable clocks of unknwown rates, but that you try to use a single
  clock for all of them and name it 'dummy' or 'null'. Name it
 
  dpi_ck {
  compatible = fixed-clock;
  rate = 0; /* unknown, generated by some Analog block */
  };
 
 It would be nice, though, to use the real clock rates.
 Otherwise, we end up with a bunch of unknown clock rates, like this:
 
clock enable_cnt  prepare_cntrate
 accuracy   phase
 
  clk_null 22   0
0 0
 mm_lvds_cts   00   0
0 0
 mm_lvds_pixel 00   0
0 0
 mm_dpi1_pixel 00   0
0 0

 Furthermore, at least some of these children clocks are possible
 source clocks for other clocks for which we do want to know the
 resulting frequency.  For example, the dmpll_* clocks are mux inputs
 for many of the subsystem clocks.

These clocks such as clkph_mck_o are configured by other modules before
kernel init, and their rates may different among platforms. So we can't
use a hard-coded rate for them. And we also don't care the actual rate
of these clocks, so assign a dummy rate such as 0 to them should be a
better way.


Best regards,

james


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread Daniel Kurtz
On Thu, Jul 2, 2015 at 11:06 AM, James Liao jamesjj.l...@mediatek.com wrote:
 Hi Daniel,

 On Wed, 2015-07-01 at 19:54 +0800, Daniel Kurtz wrote:
 On Wed, Jul 1, 2015 at 2:49 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
  The problem is not that you use fixed clocks for non software
  controllable clocks of unknwown rates, but that you try to use a single
  clock for all of them and name it 'dummy' or 'null'. Name it
 
  dpi_ck {
  compatible = fixed-clock;
  rate = 0; /* unknown, generated by some Analog block */
  };

 It would be nice, though, to use the real clock rates.
 Otherwise, we end up with a bunch of unknown clock rates, like this:

clock enable_cnt  prepare_cntrate
 accuracy   phase
 
  clk_null 22   0
0 0
 mm_lvds_cts   00   0
0 0
 mm_lvds_pixel 00   0
0 0
 mm_dpi1_pixel 00   0
0 0

 Furthermore, at least some of these children clocks are possible
 source clocks for other clocks for which we do want to know the
 resulting frequency.  For example, the dmpll_* clocks are mux inputs
 for many of the subsystem clocks.

 These clocks such as clkph_mck_o are configured by other modules before
 kernel init, and their rates may different among platforms.

What other modules?
Do you mean the rates are configured by firmware?
How are the rates set?
Are there registers that configure its rate?
If so, why can't the kernel read these registers and compute the current rate?


For mt8173, we are essentially discussing the following clocks (whose
sole parent is clk_null):

FACTOR(CLK_TOP_CLKPH_MCK_O, clkph_mck_o, clk_null, 1, 1),
FACTOR(CLK_TOP_DPI, dpi_ck, clk_null, 1, 1),
FACTOR(CLK_TOP_USB_SYSPLL_125M, usb_syspll_125m, clk_null, 1, 1),
FACTOR(CLK_TOP_HDMITX_DIG_CTS, hdmitx_dig_cts, clk_null, 1, 1),
GATE_ICG(CLK_INFRA_CPUM, infra_cpum, clk_null, 15),
GATE_MM1(CLK_MM_DSI0_DIGITAL, mm_dsi0_digital, clk_null, 5),
GATE_MM1(CLK_MM_DSI1_DIGITAL, mm_dsi1_digital, clk_null, 7),
GATE_MM1(CLK_MM_DPI1_PIXEL, mm_dpi1_pixel, clk_null, 10),
GATE_MM1(CLK_MM_LVDS_PIXEL, mm_lvds_pixel, clk_null, 16),
GATE_MM1(CLK_MM_LVDS_CTS, mm_lvds_cts, clk_null, 17),

clkph_mck_o - This is the parent for dmpll_*, which are themselves
(potential) parent clocks for nearly every subsystem.
In fact, as shown above, the dmpll_* is the selected parent for
several other clocks, which all end up with an unknown rate.
So, I think it is worth investigating a little more how to properly
read or otherwise specify the rate for clkph_mck_o.

dpi_ck, infra_cpum, mm_dsi0_digital, mm_dsi1_digital, mm_lvds_cts -
These are a dead-end (internal?) clock.
It is probably fine if their rates are unknown (0 Hz).

usb_syspll_125m - This sounds like a fixed 125 MHz clock.  It is also
a possible parent usb30 clock, so its value will propagate.

hdmitx_dig_cts - This is the root clock for the tree leading to
mm_hdmi_pllck, which includes hdmitxpll_d* and hdmi_sel.
However, I don't know how mm_hdmi_pllck is used.

mm_dpi1_pixel, mm_lvds_pixel - These two look very suspicious.  The
similar mm_dpi0_pixel and mm_hdmi_pixel have parent dpi0_sel.
It looks like maybe they should have dpi1_sel or dpilvds_sel as
parents, but the _sel are not hooked up.

-Dan

 So we can't
 use a hard-coded rate for them. And we also don't care the actual rate
 of these clocks, so assign a dummy rate such as 0 to them should be a
 better way.


 Best regards,

 james
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread James Liao
Hi Sascha,

On Wed, 2015-07-01 at 08:49 +0200, Sascha Hauer wrote:
 On Tue, Jun 30, 2015 at 05:07:09PM +0800, James Liao wrote:
  There are 4 clocks which are derived from clk_null directly in current
  topckgen implementation:
  
  clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts
  
  Our designer mentioned 2 things about external clocks:
  
  1. These 4 clocks come from analog macro, not from PLL, nor from
  external clocks directly.
 
 Ok, this means there actually are clocks. We can't control these clock and
 they have some known or unknown rate. This makes them fixed clocks. Just
 specify them in the device tree and you are done. Give them reasonable
 names and the rate if you know it, 0 otherwise.
 
 The problem is not that you use fixed clocks for non software
 controllable clocks of unknwown rates, but that you try to use a single
 clock for all of them and name it 'dummy' or 'null'. Name it
 
 dpi_ck {
   compatible = fixed-clock;
   rate = 0; /* unknown, generated by some Analog block */
 };
 
 Technically it's the same, but it sounds much more professional and like
 you know what you are doing ;)

These clocks are SoC internal clocks. Is it suitable to specify them in
the device tree?

According to your and Heiko's suggestion, it looks the best way should
be specifying these clocks in the driver code without a dummy parent.
i.e.:

Original code:

FACTOR(CLK_TOP_CLKPH_MCK_O, clkph_mck_o, clk_null, 1, 1),
FACTOR(CLK_TOP_DPI, dpi_ck, clk_null, 1, 1),
FACTOR(CLK_TOP_USB_SYSPLL_125M, usb_syspll_125m, clk_null, 1, 1),
FACTOR(CLK_TOP_HDMITX_DIG_CTS, hdmitx_dig_cts, clk_null, 1, 1),

New code:

FIXED(CLK_TOP_CLKPH_MCK_O, clkph_mck_o, UNKNOWN_RATE),
FIXED(CLK_TOP_DPI, dpi_ck, UNKNOWN_RATE),
FIXED(CLK_TOP_USB_SYSPLL_125M, usb_syspll_125m, UNKNOWN_RATE),
FIXED(CLK_TOP_HDMITX_DIG_CTS, hdmitx_dig_cts, UNKNOWN_RATE),

Then we don't need to specify a dummy clock such as clk_null in device
tree.


Best regards,

James

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-07-01 Thread Sascha Hauer
On Tue, Jun 30, 2015 at 05:07:09PM +0800, James Liao wrote:
 Hi Heiko,
 
 There are 4 clocks which are derived from clk_null directly in current
 topckgen implementation:
 
   clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts
 
 Our designer mentioned 2 things about external clocks:
 
 1. These 4 clocks come from analog macro, not from PLL, nor from
 external clocks directly.

Ok, this means there actually are clocks. We can't control these clock and
they have some known or unknown rate. This makes them fixed clocks. Just
specify them in the device tree and you are done. Give them reasonable
names and the rate if you know it, 0 otherwise.

The problem is not that you use fixed clocks for non software
controllable clocks of unknwown rates, but that you try to use a single
clock for all of them and name it 'dummy' or 'null'. Name it

dpi_ck {
compatible = fixed-clock;
rate = 0; /* unknown, generated by some Analog block */
};

Technically it's the same, but it sounds much more professional and like
you know what you are doing ;)

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-30 Thread James Liao
On Wed, 2015-06-24 at 12:24 +0200, Heiko Stübner wrote:
> Am Mittwoch, 24. Juni 2015, 15:54:15 schrieb James Liao:
> > On Mon, 2015-06-22 at 14:53 +0200, Heiko Stübner wrote:
> > > Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
> > > > On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
> > > > Some clocks such as clkph_mck_o, we don't really care where they come
> > > > from and what frequencies are. We model these clocks just because they
> > > > or their derived clocks can be the source of topckgen muxes. Is there a
> > > > better way to model "don't care" clocks?
> > > 
> > > There are two different concepts at work here. You might not care in your
> > > kernel driver implementation _at the moment_ where the clocks come from;
> > > but the devicetree is supposed to model how the hardware is structured
> > > and not contain implementation specific details.
> > 
> > If we model "don't care" clocks inside the driver (i.e. create clk_null
> > in clock driver), then we don't need to model the dummy clock in device.
> > Is it an acceptable way?
> > 
> > > So the clock tree should be modeled according to how the hardware is layed
> > > out not how you want to use the clocks at the moment :-) .
> > > 
> > > It would it any case be good, if you could describe where these clocks
> > > come
> > > from in the hardware, so we can find the best solution on how to model
> > > those.
> > In fact, I don't know where these clocks come from at all, especially
> > when they come from outside of SoC. Besides, some clocks don't need to
> > model in CCF, but they can be the source of clocks that controlled by
> > CCF.
> 
> If a clock is used inside the ccf driver, its tree should be modeled 
> according 
> to the hardware - including these external clocks. Somebody (at least some 
> chip designer or so) should know where these clocks actually come from ;-) .
> 
> 
> > I don't think ALL clocks on a SoC need to be handled in CCF, so there
> > should be some clocks don't have a "real" or "correct" parent. In
> > current implementation I use a dummy clock (clk_null) to be the unreal
> > parent.
> 
> You are right that not all clocks needs to be implemented in a ccf _driver_, 
> but as the devicetree binding describes the hardware and is supposed to be 
> stable and (nearly) unchangeable outside-connections of the clock block need 
> to be defined carefully.
> 
> 
> > Do you think we should model as more clocks as we can in CCF even we
> > don't need them? If no, how do we handle those clocks which are not
> > handled in CCF but can be parent of CCF clocks?
> 
> In general I think everything that has a connection to the outside (external 
> source clock and clocks used by soc ips) should be modeled precisely. What 
> then happens inside the clock controller is less important, as it normally 
> can 
> be fixed later on too.
> 
> 
> Citing my own code [which got inspired on how Samsung did this], Rockchip 
> clock controllers also have numerous possible external clock inputs with only 
> the 24MHz oscillator being required [0].
> 
> All the other clocks may or may not be present. For example xin32k normally 
> comes from an i2c-connected rtc or pmic chip which gets probed a lot later in 
> the boot process, so we rely on the orphan-handling of the ccf for these.
> 
> 
> So please really try to find out what these clocks are in the first place ... 
> somebody must know this ;-)
> 
> 
> Heiko
> 
> 
> [0] 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/rockchip,rk3288-cru.txt#n26

Hi Heiko,

There are 4 clocks which are derived from clk_null directly in current
topckgen implementation:

clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts

Our designer mentioned 2 things about external clocks:

1. These 4 clocks come from analog macro, not from PLL, nor from
external clocks directly.

2. MT8173 only has 2 external clocks: 26M and 32K.

Analog macro may refer to 26M or other clocks to generate clocks with
specified frequency. But we don't know and don't care how they generate
these clocks. So we want to use a dummy clock to be the root of these 4
clocks.


Hi Sascha,

Do you have any suggestion on clk_null?


Best regards,

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-30 Thread James Liao
On Wed, 2015-06-24 at 12:24 +0200, Heiko Stübner wrote:
 Am Mittwoch, 24. Juni 2015, 15:54:15 schrieb James Liao:
  On Mon, 2015-06-22 at 14:53 +0200, Heiko Stübner wrote:
   Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
Some clocks such as clkph_mck_o, we don't really care where they come
from and what frequencies are. We model these clocks just because they
or their derived clocks can be the source of topckgen muxes. Is there a
better way to model don't care clocks?
   
   There are two different concepts at work here. You might not care in your
   kernel driver implementation _at the moment_ where the clocks come from;
   but the devicetree is supposed to model how the hardware is structured
   and not contain implementation specific details.
  
  If we model don't care clocks inside the driver (i.e. create clk_null
  in clock driver), then we don't need to model the dummy clock in device.
  Is it an acceptable way?
  
   So the clock tree should be modeled according to how the hardware is layed
   out not how you want to use the clocks at the moment :-) .
   
   It would it any case be good, if you could describe where these clocks
   come
   from in the hardware, so we can find the best solution on how to model
   those.
  In fact, I don't know where these clocks come from at all, especially
  when they come from outside of SoC. Besides, some clocks don't need to
  model in CCF, but they can be the source of clocks that controlled by
  CCF.
 
 If a clock is used inside the ccf driver, its tree should be modeled 
 according 
 to the hardware - including these external clocks. Somebody (at least some 
 chip designer or so) should know where these clocks actually come from ;-) .
 
 
  I don't think ALL clocks on a SoC need to be handled in CCF, so there
  should be some clocks don't have a real or correct parent. In
  current implementation I use a dummy clock (clk_null) to be the unreal
  parent.
 
 You are right that not all clocks needs to be implemented in a ccf _driver_, 
 but as the devicetree binding describes the hardware and is supposed to be 
 stable and (nearly) unchangeable outside-connections of the clock block need 
 to be defined carefully.
 
 
  Do you think we should model as more clocks as we can in CCF even we
  don't need them? If no, how do we handle those clocks which are not
  handled in CCF but can be parent of CCF clocks?
 
 In general I think everything that has a connection to the outside (external 
 source clock and clocks used by soc ips) should be modeled precisely. What 
 then happens inside the clock controller is less important, as it normally 
 can 
 be fixed later on too.
 
 
 Citing my own code [which got inspired on how Samsung did this], Rockchip 
 clock controllers also have numerous possible external clock inputs with only 
 the 24MHz oscillator being required [0].
 
 All the other clocks may or may not be present. For example xin32k normally 
 comes from an i2c-connected rtc or pmic chip which gets probed a lot later in 
 the boot process, so we rely on the orphan-handling of the ccf for these.
 
 
 So please really try to find out what these clocks are in the first place ... 
 somebody must know this ;-)
 
 
 Heiko
 
 
 [0] 
 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/rockchip,rk3288-cru.txt#n26

Hi Heiko,

There are 4 clocks which are derived from clk_null directly in current
topckgen implementation:

clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts

Our designer mentioned 2 things about external clocks:

1. These 4 clocks come from analog macro, not from PLL, nor from
external clocks directly.

2. MT8173 only has 2 external clocks: 26M and 32K.

Analog macro may refer to 26M or other clocks to generate clocks with
specified frequency. But we don't know and don't care how they generate
these clocks. So we want to use a dummy clock to be the root of these 4
clocks.


Hi Sascha,

Do you have any suggestion on clk_null?


Best regards,

James


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-24 Thread Heiko Stübner
Hi James,

Am Mittwoch, 24. Juni 2015, 15:54:15 schrieb James Liao:
> On Mon, 2015-06-22 at 14:53 +0200, Heiko Stübner wrote:
> > Hi James,
> > 
> > Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
> > > On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
> > > 
> > > Some clocks such as clkph_mck_o, we don't really care where they come
> > > from and what frequencies are. We model these clocks just because they
> > > or their derived clocks can be the source of topckgen muxes. Is there a
> > > better way to model "don't care" clocks?
> > 
> > There are two different concepts at work here. You might not care in your
> > kernel driver implementation _at the moment_ where the clocks come from;
> > but the devicetree is supposed to model how the hardware is structured
> > and not contain implementation specific details.
> 
> If we model "don't care" clocks inside the driver (i.e. create clk_null
> in clock driver), then we don't need to model the dummy clock in device.
> Is it an acceptable way?
> 
> > So the clock tree should be modeled according to how the hardware is layed
> > out not how you want to use the clocks at the moment :-) .
> > 
> > It would it any case be good, if you could describe where these clocks
> > come
> > from in the hardware, so we can find the best solution on how to model
> > those.
> In fact, I don't know where these clocks come from at all, especially
> when they come from outside of SoC. Besides, some clocks don't need to
> model in CCF, but they can be the source of clocks that controlled by
> CCF.

If a clock is used inside the ccf driver, its tree should be modeled according 
to the hardware - including these external clocks. Somebody (at least some 
chip designer or so) should know where these clocks actually come from ;-) .


> I don't think ALL clocks on a SoC need to be handled in CCF, so there
> should be some clocks don't have a "real" or "correct" parent. In
> current implementation I use a dummy clock (clk_null) to be the unreal
> parent.

You are right that not all clocks needs to be implemented in a ccf _driver_, 
but as the devicetree binding describes the hardware and is supposed to be 
stable and (nearly) unchangeable outside-connections of the clock block need 
to be defined carefully.


> Do you think we should model as more clocks as we can in CCF even we
> don't need them? If no, how do we handle those clocks which are not
> handled in CCF but can be parent of CCF clocks?

In general I think everything that has a connection to the outside (external 
source clock and clocks used by soc ips) should be modeled precisely. What 
then happens inside the clock controller is less important, as it normally can 
be fixed later on too.


Citing my own code [which got inspired on how Samsung did this], Rockchip 
clock controllers also have numerous possible external clock inputs with only 
the 24MHz oscillator being required [0].

All the other clocks may or may not be present. For example xin32k normally 
comes from an i2c-connected rtc or pmic chip which gets probed a lot later in 
the boot process, so we rely on the orphan-handling of the ccf for these.


So please really try to find out what these clocks are in the first place ... 
somebody must know this ;-)


Heiko


[0] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/rockchip,rk3288-cru.txt#n26


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-24 Thread James Liao
Hi Heiko,

On Mon, 2015-06-22 at 14:53 +0200, Heiko Stübner wrote:
> Hi James,
> 
> Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
> > On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
> > 
> > Some clocks such as clkph_mck_o, we don't really care where they come
> > from and what frequencies are. We model these clocks just because they
> > or their derived clocks can be the source of topckgen muxes. Is there a
> > better way to model "don't care" clocks?
> 
> There are two different concepts at work here. You might not care in your 
> kernel driver implementation _at the moment_ where the clocks come from; but 
> the devicetree is supposed to model how the hardware is structured and not 
> contain implementation specific details.

If we model "don't care" clocks inside the driver (i.e. create clk_null
in clock driver), then we don't need to model the dummy clock in device.
Is it an acceptable way?

> So the clock tree should be modeled according to how the hardware is layed 
> out 
> not how you want to use the clocks at the moment :-) .
> 
> It would it any case be good, if you could describe where these clocks come 
> from in the hardware, so we can find the best solution on how to model those.

In fact, I don't know where these clocks come from at all, especially
when they come from outside of SoC. Besides, some clocks don't need to
model in CCF, but they can be the source of clocks that controlled by
CCF.

I don't think ALL clocks on a SoC need to be handled in CCF, so there
should be some clocks don't have a "real" or "correct" parent. In
current implementation I use a dummy clock (clk_null) to be the unreal
parent.

Do you think we should model as more clocks as we can in CCF even we
don't need them? If no, how do we handle those clocks which are not
handled in CCF but can be parent of CCF clocks?


Best regards,

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-24 Thread James Liao
Hi Heiko,

On Mon, 2015-06-22 at 14:53 +0200, Heiko Stübner wrote:
 Hi James,
 
 Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
  On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
  
  Some clocks such as clkph_mck_o, we don't really care where they come
  from and what frequencies are. We model these clocks just because they
  or their derived clocks can be the source of topckgen muxes. Is there a
  better way to model don't care clocks?
 
 There are two different concepts at work here. You might not care in your 
 kernel driver implementation _at the moment_ where the clocks come from; but 
 the devicetree is supposed to model how the hardware is structured and not 
 contain implementation specific details.

If we model don't care clocks inside the driver (i.e. create clk_null
in clock driver), then we don't need to model the dummy clock in device.
Is it an acceptable way?

 So the clock tree should be modeled according to how the hardware is layed 
 out 
 not how you want to use the clocks at the moment :-) .
 
 It would it any case be good, if you could describe where these clocks come 
 from in the hardware, so we can find the best solution on how to model those.

In fact, I don't know where these clocks come from at all, especially
when they come from outside of SoC. Besides, some clocks don't need to
model in CCF, but they can be the source of clocks that controlled by
CCF.

I don't think ALL clocks on a SoC need to be handled in CCF, so there
should be some clocks don't have a real or correct parent. In
current implementation I use a dummy clock (clk_null) to be the unreal
parent.

Do you think we should model as more clocks as we can in CCF even we
don't need them? If no, how do we handle those clocks which are not
handled in CCF but can be parent of CCF clocks?


Best regards,

James


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-24 Thread Heiko Stübner
Hi James,

Am Mittwoch, 24. Juni 2015, 15:54:15 schrieb James Liao:
 On Mon, 2015-06-22 at 14:53 +0200, Heiko Stübner wrote:
  Hi James,
  
  Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
   On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
   
   Some clocks such as clkph_mck_o, we don't really care where they come
   from and what frequencies are. We model these clocks just because they
   or their derived clocks can be the source of topckgen muxes. Is there a
   better way to model don't care clocks?
  
  There are two different concepts at work here. You might not care in your
  kernel driver implementation _at the moment_ where the clocks come from;
  but the devicetree is supposed to model how the hardware is structured
  and not contain implementation specific details.
 
 If we model don't care clocks inside the driver (i.e. create clk_null
 in clock driver), then we don't need to model the dummy clock in device.
 Is it an acceptable way?
 
  So the clock tree should be modeled according to how the hardware is layed
  out not how you want to use the clocks at the moment :-) .
  
  It would it any case be good, if you could describe where these clocks
  come
  from in the hardware, so we can find the best solution on how to model
  those.
 In fact, I don't know where these clocks come from at all, especially
 when they come from outside of SoC. Besides, some clocks don't need to
 model in CCF, but they can be the source of clocks that controlled by
 CCF.

If a clock is used inside the ccf driver, its tree should be modeled according 
to the hardware - including these external clocks. Somebody (at least some 
chip designer or so) should know where these clocks actually come from ;-) .


 I don't think ALL clocks on a SoC need to be handled in CCF, so there
 should be some clocks don't have a real or correct parent. In
 current implementation I use a dummy clock (clk_null) to be the unreal
 parent.

You are right that not all clocks needs to be implemented in a ccf _driver_, 
but as the devicetree binding describes the hardware and is supposed to be 
stable and (nearly) unchangeable outside-connections of the clock block need 
to be defined carefully.


 Do you think we should model as more clocks as we can in CCF even we
 don't need them? If no, how do we handle those clocks which are not
 handled in CCF but can be parent of CCF clocks?

In general I think everything that has a connection to the outside (external 
source clock and clocks used by soc ips) should be modeled precisely. What 
then happens inside the clock controller is less important, as it normally can 
be fixed later on too.


Citing my own code [which got inspired on how Samsung did this], Rockchip 
clock controllers also have numerous possible external clock inputs with only 
the 24MHz oscillator being required [0].

All the other clocks may or may not be present. For example xin32k normally 
comes from an i2c-connected rtc or pmic chip which gets probed a lot later in 
the boot process, so we rely on the orphan-handling of the ccf for these.


So please really try to find out what these clocks are in the first place ... 
somebody must know this ;-)


Heiko


[0] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/rockchip,rk3288-cru.txt#n26


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-22 Thread Heiko Stübner
Hi James,

Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
> On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
> > Am Donnerstag, 18. Juni 2015, 18:15:03 schrieb Heiko Stuebner:
> > > Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
> > > > Add clk_null, which represents clocks that can not / need not
> > > > controlled by software.
> > > > There are many clocks' parent set to clk_null.
> > > 
> > > Devicetree is supposed to describe hardware, and ideally not what
> > > software
> > > does with it. If the clock simply cannot be controlled by software, it
> > > will
> > > still have a rate and I think it should probably be modelled - similarly
> > > we
> > > sometimes have fixed regulators that also are not software controllable.
> > > 
> > > 
> > > While it might be ok to define dummy clocks as a temporary stopgap,
> > > these
> > > should definitly be marked as such. This clk_null at least sounds like
> > > there is no plan to replace this with a real solution at some point.
> > > 
> > > And of course a bit of context would be cool, to know which type of
> > > clocks
> > > this actually replaces.
> > 
> > After looking a bit more into this, I'm feel that the clk_null approach is
> > wrong.
> > 
> > For one, even if the clk_null stuff would be ok, the binding doc of the
> > clock controller does not describe the need of this specific clock at
> > all.
> > 
> > 
> > The other more important point, looking at clk-mt8173 I see at least these
> > clocks being set as children of "clk_null":
> > 
> > static const struct mtk_fixed_factor root_clk_alias[] __initconst = {
> > 
> > FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
> > FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
> > FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
> > FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),
> > 
> > };
> > 
> > 
> > These look more like they are fed from some external source to the clock
> > controller? I did ask Matthias but he also couldn't find anything
> > describing where these clocks actually come from.
> 
> Some clocks such as clkph_mck_o, we don't really care where they come
> from and what frequencies are. We model these clocks just because they
> or their derived clocks can be the source of topckgen muxes. Is there a
> better way to model "don't care" clocks?

There are two different concepts at work here. You might not care in your 
kernel driver implementation _at the moment_ where the clocks come from; but 
the devicetree is supposed to model how the hardware is structured and not 
contain implementation specific details.

So the clock tree should be modeled according to how the hardware is layed out 
not how you want to use the clocks at the moment :-) .

It would it any case be good, if you could describe where these clocks come 
from in the hardware, so we can find the best solution on how to model those.


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-22 Thread Heiko Stübner
Hi James,

Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
 On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
  Am Donnerstag, 18. Juni 2015, 18:15:03 schrieb Heiko Stuebner:
   Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
Add clk_null, which represents clocks that can not / need not
controlled by software.
There are many clocks' parent set to clk_null.
   
   Devicetree is supposed to describe hardware, and ideally not what
   software
   does with it. If the clock simply cannot be controlled by software, it
   will
   still have a rate and I think it should probably be modelled - similarly
   we
   sometimes have fixed regulators that also are not software controllable.
   
   
   While it might be ok to define dummy clocks as a temporary stopgap,
   these
   should definitly be marked as such. This clk_null at least sounds like
   there is no plan to replace this with a real solution at some point.
   
   And of course a bit of context would be cool, to know which type of
   clocks
   this actually replaces.
  
  After looking a bit more into this, I'm feel that the clk_null approach is
  wrong.
  
  For one, even if the clk_null stuff would be ok, the binding doc of the
  clock controller does not describe the need of this specific clock at
  all.
  
  
  The other more important point, looking at clk-mt8173 I see at least these
  clocks being set as children of clk_null:
  
  static const struct mtk_fixed_factor root_clk_alias[] __initconst = {
  
  FACTOR(CLK_TOP_CLKPH_MCK_O, clkph_mck_o, clk_null, 1, 1),
  FACTOR(CLK_TOP_DPI, dpi_ck, clk_null, 1, 1),
  FACTOR(CLK_TOP_USB_SYSPLL_125M, usb_syspll_125m, clk_null, 1, 1),
  FACTOR(CLK_TOP_HDMITX_DIG_CTS, hdmitx_dig_cts, clk_null, 1, 1),
  
  };
  
  
  These look more like they are fed from some external source to the clock
  controller? I did ask Matthias but he also couldn't find anything
  describing where these clocks actually come from.
 
 Some clocks such as clkph_mck_o, we don't really care where they come
 from and what frequencies are. We model these clocks just because they
 or their derived clocks can be the source of topckgen muxes. Is there a
 better way to model don't care clocks?

There are two different concepts at work here. You might not care in your 
kernel driver implementation _at the moment_ where the clocks come from; but 
the devicetree is supposed to model how the hardware is structured and not 
contain implementation specific details.

So the clock tree should be modeled according to how the hardware is layed out 
not how you want to use the clocks at the moment :-) .

It would it any case be good, if you could describe where these clocks come 
from in the hardware, so we can find the best solution on how to model those.


Heiko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-21 Thread James Liao
Hi Heiko,

On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
> Am Donnerstag, 18. Juni 2015, 18:15:03 schrieb Heiko Stuebner:
> > Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
> > > Add clk_null, which represents clocks that can not / need not
> > > controlled by software.
> > > There are many clocks' parent set to clk_null.
> > 
> > Devicetree is supposed to describe hardware, and ideally not what software
> > does with it. If the clock simply cannot be controlled by software, it will
> > still have a rate and I think it should probably be modelled - similarly we
> > sometimes have fixed regulators that also are not software controllable.
> > 
> > 
> > While it might be ok to define dummy clocks as a temporary stopgap, these
> > should definitly be marked as such. This clk_null at least sounds like there
> > is no plan to replace this with a real solution at some point.
> > 
> > And of course a bit of context would be cool, to know which type of clocks
> > this actually replaces.
> 
> After looking a bit more into this, I'm feel that the clk_null approach is 
> wrong.
> 
> For one, even if the clk_null stuff would be ok, the binding doc of the clock 
> controller does not describe the need of this specific clock at all.
> 
> 
> The other more important point, looking at clk-mt8173 I see at least these 
> clocks being set as children of "clk_null":
> 
> static const struct mtk_fixed_factor root_clk_alias[] __initconst = {
>   FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
>   FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
>   FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
>   FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),
> };
> 
> 
> These look more like they are fed from some external source to the clock 
> controller? I did ask Matthias but he also couldn't find anything describing 
> where these clocks actually come from.

Some clocks such as clkph_mck_o, we don't really care where they come
from and what frequencies are. We model these clocks just because they
or their derived clocks can be the source of topckgen muxes. Is there a
better way to model "don't care" clocks?


Best regards,

James



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-21 Thread James Liao
Hi Heiko,

On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
 Am Donnerstag, 18. Juni 2015, 18:15:03 schrieb Heiko Stuebner:
  Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
   Add clk_null, which represents clocks that can not / need not
   controlled by software.
   There are many clocks' parent set to clk_null.
  
  Devicetree is supposed to describe hardware, and ideally not what software
  does with it. If the clock simply cannot be controlled by software, it will
  still have a rate and I think it should probably be modelled - similarly we
  sometimes have fixed regulators that also are not software controllable.
  
  
  While it might be ok to define dummy clocks as a temporary stopgap, these
  should definitly be marked as such. This clk_null at least sounds like there
  is no plan to replace this with a real solution at some point.
  
  And of course a bit of context would be cool, to know which type of clocks
  this actually replaces.
 
 After looking a bit more into this, I'm feel that the clk_null approach is 
 wrong.
 
 For one, even if the clk_null stuff would be ok, the binding doc of the clock 
 controller does not describe the need of this specific clock at all.
 
 
 The other more important point, looking at clk-mt8173 I see at least these 
 clocks being set as children of clk_null:
 
 static const struct mtk_fixed_factor root_clk_alias[] __initconst = {
   FACTOR(CLK_TOP_CLKPH_MCK_O, clkph_mck_o, clk_null, 1, 1),
   FACTOR(CLK_TOP_DPI, dpi_ck, clk_null, 1, 1),
   FACTOR(CLK_TOP_USB_SYSPLL_125M, usb_syspll_125m, clk_null, 1, 1),
   FACTOR(CLK_TOP_HDMITX_DIG_CTS, hdmitx_dig_cts, clk_null, 1, 1),
 };
 
 
 These look more like they are fed from some external source to the clock 
 controller? I did ask Matthias but he also couldn't find anything describing 
 where these clocks actually come from.

Some clocks such as clkph_mck_o, we don't really care where they come
from and what frequencies are. We model these clocks just because they
or their derived clocks can be the source of topckgen muxes. Is there a
better way to model don't care clocks?


Best regards,

James



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-19 Thread Heiko Stuebner
Am Donnerstag, 18. Juni 2015, 18:15:03 schrieb Heiko Stuebner:
> Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
> > Add clk_null, which represents clocks that can not / need not
> > controlled by software.
> > There are many clocks' parent set to clk_null.
> 
> Devicetree is supposed to describe hardware, and ideally not what software
> does with it. If the clock simply cannot be controlled by software, it will
> still have a rate and I think it should probably be modelled - similarly we
> sometimes have fixed regulators that also are not software controllable.
> 
> 
> While it might be ok to define dummy clocks as a temporary stopgap, these
> should definitly be marked as such. This clk_null at least sounds like there
> is no plan to replace this with a real solution at some point.
> 
> And of course a bit of context would be cool, to know which type of clocks
> this actually replaces.

After looking a bit more into this, I'm feel that the clk_null approach is 
wrong.

For one, even if the clk_null stuff would be ok, the binding doc of the clock 
controller does not describe the need of this specific clock at all.


The other more important point, looking at clk-mt8173 I see at least these 
clocks being set as children of "clk_null":

static const struct mtk_fixed_factor root_clk_alias[] __initconst = {
FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),
};


These look more like they are fed from some external source to the clock 
controller? I did ask Matthias but he also couldn't find anything describing 
where these clocks actually come from.


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-19 Thread Heiko Stuebner
Am Donnerstag, 18. Juni 2015, 18:15:03 schrieb Heiko Stuebner:
 Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
  Add clk_null, which represents clocks that can not / need not
  controlled by software.
  There are many clocks' parent set to clk_null.
 
 Devicetree is supposed to describe hardware, and ideally not what software
 does with it. If the clock simply cannot be controlled by software, it will
 still have a rate and I think it should probably be modelled - similarly we
 sometimes have fixed regulators that also are not software controllable.
 
 
 While it might be ok to define dummy clocks as a temporary stopgap, these
 should definitly be marked as such. This clk_null at least sounds like there
 is no plan to replace this with a real solution at some point.
 
 And of course a bit of context would be cool, to know which type of clocks
 this actually replaces.

After looking a bit more into this, I'm feel that the clk_null approach is 
wrong.

For one, even if the clk_null stuff would be ok, the binding doc of the clock 
controller does not describe the need of this specific clock at all.


The other more important point, looking at clk-mt8173 I see at least these 
clocks being set as children of clk_null:

static const struct mtk_fixed_factor root_clk_alias[] __initconst = {
FACTOR(CLK_TOP_CLKPH_MCK_O, clkph_mck_o, clk_null, 1, 1),
FACTOR(CLK_TOP_DPI, dpi_ck, clk_null, 1, 1),
FACTOR(CLK_TOP_USB_SYSPLL_125M, usb_syspll_125m, clk_null, 1, 1),
FACTOR(CLK_TOP_HDMITX_DIG_CTS, hdmitx_dig_cts, clk_null, 1, 1),
};


These look more like they are fed from some external source to the clock 
controller? I did ask Matthias but he also couldn't find anything describing 
where these clocks actually come from.


Heiko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-18 Thread Heiko Stuebner
Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
> Add clk_null, which represents clocks that can not / need not
> controlled by software.
> There are many clocks' parent set to clk_null.

Devicetree is supposed to describe hardware, and ideally not what software 
does with it. If the clock simply cannot be controlled by software, it will 
still have a rate and I think it should probably be modelled - similarly we 
sometimes have fixed regulators that also are not software controllable.


While it might be ok to define dummy clocks as a temporary stopgap, these 
should definitly be marked as such. This clk_null at least sounds like there is 
no plan to replace this with a real solution at some point.

And of course a bit of context would be cool, to know which type of clocks 
this actually replaces.


Heiko

> Signed-off-by: James Liao 
> Signed-off-by: Eddie Huang 
> ---
> Base on 4.1-rc1
> 
> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
> ---
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi index 924fdb6..4798f44 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -81,6 +81,12 @@
>   cpu_on= <0x8403>;
>   };
> 
> + clk_null: clk_null {
> + compatible = "fixed-clock";
> + clock-frequency = <0>;
> + #clock-cells = <0>;
> + };
> +
>   uart_clk: dummy26m {
>   compatible = "fixed-clock";
>   clock-frequency = <2600>;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] arm64: dts: mt8173: add clock_null

2015-06-18 Thread Heiko Stuebner
Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
 Add clk_null, which represents clocks that can not / need not
 controlled by software.
 There are many clocks' parent set to clk_null.

Devicetree is supposed to describe hardware, and ideally not what software 
does with it. If the clock simply cannot be controlled by software, it will 
still have a rate and I think it should probably be modelled - similarly we 
sometimes have fixed regulators that also are not software controllable.


While it might be ok to define dummy clocks as a temporary stopgap, these 
should definitly be marked as such. This clk_null at least sounds like there is 
no plan to replace this with a real solution at some point.

And of course a bit of context would be cool, to know which type of clocks 
this actually replaces.


Heiko

 Signed-off-by: James Liao jamesjj.l...@mediatek.com
 Signed-off-by: Eddie Huang eddie.hu...@mediatek.com
 ---
 Base on 4.1-rc1
 
 Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
 ---
  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 b/arch/arm64/boot/dts/mediatek/mt8173.dtsi index 924fdb6..4798f44 100644
 --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
 @@ -81,6 +81,12 @@
   cpu_on= 0x8403;
   };
 
 + clk_null: clk_null {
 + compatible = fixed-clock;
 + clock-frequency = 0;
 + #clock-cells = 0;
 + };
 +
   uart_clk: dummy26m {
   compatible = fixed-clock;
   clock-frequency = 2600;

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/