All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
@ 2016-08-08  3:37 Leo Yan
  2016-08-08  5:53 ` Amit Kucheria
  0 siblings, 1 reply; 9+ messages in thread
From: Leo Yan @ 2016-08-08  3:37 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Jiancheng Xue, Philipp Zabel,
	Rob Herring, linux-clk, linux-kernel, Dietmar Eggemann,
	Guodong Xu
  Cc: Leo Yan

In current kernel config 'CONFIG_STUB_CLK_HI6220' is disabled by
default, as result stub clock driver has not been registered and
CPUFreq driver cannot work.

This patch is to enable stub clock driver in config for ARCH_HISI.

Reported-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/clk/hisilicon/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
index 3f537a0..9e0a95e 100644
--- a/drivers/clk/hisilicon/Kconfig
+++ b/drivers/clk/hisilicon/Kconfig
@@ -23,5 +23,6 @@ config RESET_HISI
 config STUB_CLK_HI6220
 	bool "Hi6220 Stub Clock Driver"
 	depends on COMMON_CLK_HI6220 && MAILBOX
+	default ARCH_HISI
 	help
 	  Build the Hisilicon Hi6220 stub clock driver.
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
  2016-08-08  3:37 [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI Leo Yan
@ 2016-08-08  5:53 ` Amit Kucheria
  2016-08-08  6:42   ` Leo Yan
  0 siblings, 1 reply; 9+ messages in thread
From: Amit Kucheria @ 2016-08-08  5:53 UTC (permalink / raw)
  To: Leo Yan
  Cc: Michael Turquette, Stephen Boyd, Jiancheng Xue, Philipp Zabel,
	Rob Herring, linux-clk, LKML, Dietmar Eggemann, Guodong Xu

On Mon, Aug 8, 2016 at 9:07 AM, Leo Yan <leo.yan@linaro.org> wrote:
> In current kernel config 'CONFIG_STUB_CLK_HI6220' is disabled by
> default, as result stub clock driver has not been registered and
> CPUFreq driver cannot work.

I have a related patch that has been pending for a while:
https://lkml.org/lkml/2016/6/20/375 but it was tied only to the
thermal driver.

Is the stub mandatory for the architecture? Will other SoCs in the
family will use the same stub?


> This patch is to enable stub clock driver in config for ARCH_HISI.
>
> Reported-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/clk/hisilicon/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
> index 3f537a0..9e0a95e 100644
> --- a/drivers/clk/hisilicon/Kconfig
> +++ b/drivers/clk/hisilicon/Kconfig
> @@ -23,5 +23,6 @@ config RESET_HISI
>  config STUB_CLK_HI6220
>         bool "Hi6220 Stub Clock Driver"
>         depends on COMMON_CLK_HI6220 && MAILBOX
> +       default ARCH_HISI
>         help
>           Build the Hisilicon Hi6220 stub clock driver.
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
  2016-08-08  5:53 ` Amit Kucheria
@ 2016-08-08  6:42   ` Leo Yan
  2016-08-08 14:42     ` Amit Kucheria
  0 siblings, 1 reply; 9+ messages in thread
From: Leo Yan @ 2016-08-08  6:42 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Michael Turquette, Stephen Boyd, Jiancheng Xue, Philipp Zabel,
	Rob Herring, linux-clk, LKML, Dietmar Eggemann, Guodong Xu

Hi Amit,

On Mon, Aug 08, 2016 at 11:23:31AM +0530, Amit Kucheria wrote:
> On Mon, Aug 8, 2016 at 9:07 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > In current kernel config 'CONFIG_STUB_CLK_HI6220' is disabled by
> > default, as result stub clock driver has not been registered and
> > CPUFreq driver cannot work.
> 
> I have a related patch that has been pending for a while:
> https://lkml.org/lkml/2016/6/20/375 but it was tied only to the
> thermal driver.

I also have concern this patch may duplicate with yours.

> Is the stub mandatory for the architecture? Will other SoCs in the
> family will use the same stub?

I don't think stub driver is mandartory for archtitecture and it's
only used by Hi6220 on Hikey. AFAIK, currently stub driver is only used
by CPU frequency change.

The logic is:
  Thermal cooling device driver
      `-> CPUFreq DT driver
              `-> Stub clock driver

ARM is working on Hikey for EAS profiling, so usually the use case is
to enable CPUFreq driver and stub clock driver. Sometimes it only
need enable this driver without thermal driver; so this is why we
cannot rely on thermal driver to enable stub clock driver.

I'm open-minded if you have better idea to enable it.

> > This patch is to enable stub clock driver in config for ARCH_HISI.
> >
> > Reported-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  drivers/clk/hisilicon/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
> > index 3f537a0..9e0a95e 100644
> > --- a/drivers/clk/hisilicon/Kconfig
> > +++ b/drivers/clk/hisilicon/Kconfig
> > @@ -23,5 +23,6 @@ config RESET_HISI
> >  config STUB_CLK_HI6220
> >         bool "Hi6220 Stub Clock Driver"
> >         depends on COMMON_CLK_HI6220 && MAILBOX
> > +       default ARCH_HISI
> >         help
> >           Build the Hisilicon Hi6220 stub clock driver.
> > --
> > 1.9.1
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
  2016-08-08  6:42   ` Leo Yan
@ 2016-08-08 14:42     ` Amit Kucheria
  2016-08-08 15:32       ` Leo Yan
  2016-08-08 15:53       ` Daniel Thompson
  0 siblings, 2 replies; 9+ messages in thread
From: Amit Kucheria @ 2016-08-08 14:42 UTC (permalink / raw)
  To: Leo Yan
  Cc: Michael Turquette, Stephen Boyd, Jiancheng Xue, Philipp Zabel,
	Rob Herring, linux-clk, LKML, Dietmar Eggemann, Guodong Xu

On Mon, Aug 8, 2016 at 12:12 PM, Leo Yan <leo.yan@linaro.org> wrote:
> Hi Amit,
>
> On Mon, Aug 08, 2016 at 11:23:31AM +0530, Amit Kucheria wrote:
>> On Mon, Aug 8, 2016 at 9:07 AM, Leo Yan <leo.yan@linaro.org> wrote:
>> > In current kernel config 'CONFIG_STUB_CLK_HI6220' is disabled by
>> > default, as result stub clock driver has not been registered and
>> > CPUFreq driver cannot work.
>>
>> I have a related patch that has been pending for a while:
>> https://lkml.org/lkml/2016/6/20/375 but it was tied only to the
>> thermal driver.
>
> I also have concern this patch may duplicate with yours.
>
>> Is the stub mandatory for the architecture? Will other SoCs in the
>> family will use the same stub?
>
> I don't think stub driver is mandartory for archtitecture and it's
> only used by Hi6220 on Hikey. AFAIK, currently stub driver is only used
> by CPU frequency change.
>
> The logic is:
>   Thermal cooling device driver
>       `-> CPUFreq DT driver
>               `-> Stub clock driver
>
> ARM is working on Hikey for EAS profiling, so usually the use case is
> to enable CPUFreq driver and stub clock driver. Sometimes it only
> need enable this driver without thermal driver; so this is why we
> cannot rely on thermal driver to enable stub clock driver.
>
> I'm open-minded if you have better idea to enable it.
>
>> > This patch is to enable stub clock driver in config for ARCH_HISI.
>> >
>> > Reported-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
>> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> > ---
>> >  drivers/clk/hisilicon/Kconfig | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> > index 3f537a0..9e0a95e 100644
>> > --- a/drivers/clk/hisilicon/Kconfig
>> > +++ b/drivers/clk/hisilicon/Kconfig
>> > @@ -23,5 +23,6 @@ config RESET_HISI
>> >  config STUB_CLK_HI6220
>> >         bool "Hi6220 Stub Clock Driver"
>> >         depends on COMMON_CLK_HI6220 && MAILBOX
>> > +       default ARCH_HISI

Instead of forcing this up on the entire arch, why not restrict it to
just the cpufreq driver?  ARM_HISI_ACPU_CPUFREQ?

>> >         help
>> >           Build the Hisilicon Hi6220 stub clock driver.
>> > --
>> > 1.9.1
>> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
  2016-08-08 14:42     ` Amit Kucheria
@ 2016-08-08 15:32       ` Leo Yan
  2016-08-08 15:53       ` Daniel Thompson
  1 sibling, 0 replies; 9+ messages in thread
From: Leo Yan @ 2016-08-08 15:32 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Michael Turquette, Stephen Boyd, Jiancheng Xue, Philipp Zabel,
	Rob Herring, linux-clk, LKML, Dietmar Eggemann, Guodong Xu

On Mon, Aug 08, 2016 at 08:12:21PM +0530, Amit Kucheria wrote:

[...]

> >> > diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
> >> > index 3f537a0..9e0a95e 100644
> >> > --- a/drivers/clk/hisilicon/Kconfig
> >> > +++ b/drivers/clk/hisilicon/Kconfig
> >> > @@ -23,5 +23,6 @@ config RESET_HISI
> >> >  config STUB_CLK_HI6220
> >> >         bool "Hi6220 Stub Clock Driver"
> >> >         depends on COMMON_CLK_HI6220 && MAILBOX
> >> > +       default ARCH_HISI
> 
> Instead of forcing this up on the entire arch, why not restrict it to
> just the cpufreq driver?  ARM_HISI_ACPU_CPUFREQ?

ARM_HISI_ACPU_CPUFREQ has been removed by 3920be471ce7f "cpufreq:
hisilicon: Use generic platdev driver". By default now platforms are
using drivers/cpufreq/cpufreq-dt-platdev.c.

> >> >         help
> >> >           Build the Hisilicon Hi6220 stub clock driver.
> >> > --
> >> > 1.9.1
> >> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
  2016-08-08 14:42     ` Amit Kucheria
  2016-08-08 15:32       ` Leo Yan
@ 2016-08-08 15:53       ` Daniel Thompson
  2016-08-08 18:02         ` Amit Kucheria
  1 sibling, 1 reply; 9+ messages in thread
From: Daniel Thompson @ 2016-08-08 15:53 UTC (permalink / raw)
  To: Amit Kucheria, Leo Yan
  Cc: Michael Turquette, Stephen Boyd, Jiancheng Xue, Philipp Zabel,
	Rob Herring, linux-clk, LKML, Dietmar Eggemann, Guodong Xu

On 08/08/16 15:42, Amit Kucheria wrote:
> On Mon, Aug 8, 2016 at 12:12 PM, Leo Yan <leo.yan@linaro.org> wrote:
>> Hi Amit,
>>
>> On Mon, Aug 08, 2016 at 11:23:31AM +0530, Amit Kucheria wrote:
>>> On Mon, Aug 8, 2016 at 9:07 AM, Leo Yan <leo.yan@linaro.org> wrote:
>>>> In current kernel config 'CONFIG_STUB_CLK_HI6220' is disabled by
>>>> default, as result stub clock driver has not been registered and
>>>> CPUFreq driver cannot work.
>>>
>>> I have a related patch that has been pending for a while:
>>> https://lkml.org/lkml/2016/6/20/375 but it was tied only to the
>>> thermal driver.
>>
>> I also have concern this patch may duplicate with yours.
>>
>>> Is the stub mandatory for the architecture? Will other SoCs in the
>>> family will use the same stub?
>>
>> I don't think stub driver is mandartory for archtitecture and it's
>> only used by Hi6220 on Hikey. AFAIK, currently stub driver is only used
>> by CPU frequency change.
>>
>> The logic is:
>>   Thermal cooling device driver
>>       `-> CPUFreq DT driver
>>               `-> Stub clock driver
>>
>> ARM is working on Hikey for EAS profiling, so usually the use case is
>> to enable CPUFreq driver and stub clock driver. Sometimes it only
>> need enable this driver without thermal driver; so this is why we
>> cannot rely on thermal driver to enable stub clock driver.
>>
>> I'm open-minded if you have better idea to enable it.
>>
>>>> This patch is to enable stub clock driver in config for ARCH_HISI.
>>>>
>>>> Reported-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
>>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>>> ---
>>>>  drivers/clk/hisilicon/Kconfig | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>>>> index 3f537a0..9e0a95e 100644
>>>> --- a/drivers/clk/hisilicon/Kconfig
>>>> +++ b/drivers/clk/hisilicon/Kconfig
>>>> @@ -23,5 +23,6 @@ config RESET_HISI
>>>>  config STUB_CLK_HI6220
>>>>         bool "Hi6220 Stub Clock Driver"
>>>>         depends on COMMON_CLK_HI6220 && MAILBOX
>>>> +       default ARCH_HISI
>
> Instead of forcing this up on the entire arch, why not restrict it to
> just the cpufreq driver?  ARM_HISI_ACPU_CPUFREQ?

I'm struggling to understand the concern here. default doesn't force 
this option on anything, it just sets the default.

Personally I might prefer 'default y' because it is simpler and involves 
less thinking. However the other drivers in this directory use 'default 
ARCH_HISI' (i.e. they do not default on for a COMPILE_TEST) so copying 
their approach is quite reasonable.


Daniel.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
  2016-08-08 15:53       ` Daniel Thompson
@ 2016-08-08 18:02         ` Amit Kucheria
  2016-08-08 20:36           ` Daniel Thompson
  0 siblings, 1 reply; 9+ messages in thread
From: Amit Kucheria @ 2016-08-08 18:02 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Leo Yan, Michael Turquette, Stephen Boyd, Jiancheng Xue,
	Philipp Zabel, Rob Herring, linux-clk, LKML, Dietmar Eggemann,
	Guodong Xu

On Mon, Aug 8, 2016 at 9:23 PM, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> On 08/08/16 15:42, Amit Kucheria wrote:
>>
>> On Mon, Aug 8, 2016 at 12:12 PM, Leo Yan <leo.yan@linaro.org> wrote:
>>>
>>> Hi Amit,
>>>
>>> On Mon, Aug 08, 2016 at 11:23:31AM +0530, Amit Kucheria wrote:
>>>>
>>>> On Mon, Aug 8, 2016 at 9:07 AM, Leo Yan <leo.yan@linaro.org> wrote:
>>>>>
>>>>> In current kernel config 'CONFIG_STUB_CLK_HI6220' is disabled by
>>>>> default, as result stub clock driver has not been registered and
>>>>> CPUFreq driver cannot work.
>>>>
>>>>
>>>> I have a related patch that has been pending for a while:
>>>> https://lkml.org/lkml/2016/6/20/375 but it was tied only to the
>>>> thermal driver.
>>>
>>>
>>> I also have concern this patch may duplicate with yours.
>>>
>>>> Is the stub mandatory for the architecture? Will other SoCs in the
>>>> family will use the same stub?
>>>
>>>
>>> I don't think stub driver is mandartory for archtitecture and it's
>>> only used by Hi6220 on Hikey. AFAIK, currently stub driver is only used
>>> by CPU frequency change.
>>>
>>> The logic is:
>>>   Thermal cooling device driver
>>>       `-> CPUFreq DT driver
>>>               `-> Stub clock driver
>>>
>>> ARM is working on Hikey for EAS profiling, so usually the use case is
>>> to enable CPUFreq driver and stub clock driver. Sometimes it only
>>> need enable this driver without thermal driver; so this is why we
>>> cannot rely on thermal driver to enable stub clock driver.
>>>
>>> I'm open-minded if you have better idea to enable it.
>>>
>>>>> This patch is to enable stub clock driver in config for ARCH_HISI.
>>>>>
>>>>> Reported-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
>>>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>>>> ---
>>>>>  drivers/clk/hisilicon/Kconfig | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/clk/hisilicon/Kconfig
>>>>> b/drivers/clk/hisilicon/Kconfig
>>>>> index 3f537a0..9e0a95e 100644
>>>>> --- a/drivers/clk/hisilicon/Kconfig
>>>>> +++ b/drivers/clk/hisilicon/Kconfig
>>>>> @@ -23,5 +23,6 @@ config RESET_HISI
>>>>>  config STUB_CLK_HI6220
>>>>>         bool "Hi6220 Stub Clock Driver"
>>>>>         depends on COMMON_CLK_HI6220 && MAILBOX
>>>>> +       default ARCH_HISI
>>
>>
>> Instead of forcing this up on the entire arch, why not restrict it to
>> just the cpufreq driver?  ARM_HISI_ACPU_CPUFREQ?
>
>
> I'm struggling to understand the concern here. default doesn't force this
> option on anything, it just sets the default.
>
> Personally I might prefer 'default y' because it is simpler and involves
> less thinking. However the other drivers in this directory use 'default
> ARCH_HISI' (i.e. they do not default on for a COMPILE_TEST) so copying their
> approach is quite reasonable.

No concern actually - on the contrary, this dependency needs to be
fixed urgently. Just trying to figure out if there is a way to manage
the dependency chain in one place and including the root config option
into defconfig.

(Thermal driver) ---- dep ---> (cpufreq driver)  ----- dep --->
(hi2660 clock stub driver) ---- dep -----> (common hi6220 clock
driver)

My earlier patch focused on enabling the stub driver in the case the
thermal driver was enabled (and subsequently turning on HISI_THERMAL
in defconfig). The EAS profiling usecase to prevent thermal-throttling
from kicking in is a bad default to have in the kernel, IMO - it can
be easily achieved by just changing the thermal thresholds.

Something like the following, with HISI_THERMAL added to defconfig
would give a "stable" kernel on Hikey.

diff --git i/drivers/thermal/Kconfig w/drivers/thermal/Kconfig
index 2d702ca..77597a5 100644
--- i/drivers/thermal/Kconfig
+++ w/drivers/thermal/Kconfig
@@ -177,8 +177,11 @@ config THERMAL_EMULATION

 config HISI_THERMAL
  tristate "Hisilicon thermal driver"
-   depends on (ARCH_HISI && CPU_THERMAL && OF) || COMPILE_TEST
+ depends on (ARCH_HISI && OF) || COMPILE_TEST
  depends on HAS_IOMEM
+ select CPU_THERMAL
+ select CPUFREQ_DT
+ select STUB_CLK_HI6220
  help
    Enable this to plug hisilicon's thermal sensor driver into the Linux
    thermal framework. cpufreq is used as the cooling device to throttle

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
  2016-08-08 18:02         ` Amit Kucheria
@ 2016-08-08 20:36           ` Daniel Thompson
  2016-08-09  1:23             ` Leo Yan
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Thompson @ 2016-08-08 20:36 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Leo Yan, Michael Turquette, Stephen Boyd, Jiancheng Xue,
	Philipp Zabel, Rob Herring, linux-clk, LKML, Dietmar Eggemann,
	Guodong Xu

On 08/08/16 19:02, Amit Kucheria wrote:
>>>>>> This patch is to enable stub clock driver in config for ARCH_HISI.
>>>>>>
>>>>>> Reported-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
>>>>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>>>>> ---
>>>>>>  drivers/clk/hisilicon/Kconfig | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/clk/hisilicon/Kconfig
>>>>>> b/drivers/clk/hisilicon/Kconfig
>>>>>> index 3f537a0..9e0a95e 100644
>>>>>> --- a/drivers/clk/hisilicon/Kconfig
>>>>>> +++ b/drivers/clk/hisilicon/Kconfig
>>>>>> @@ -23,5 +23,6 @@ config RESET_HISI
>>>>>>  config STUB_CLK_HI6220
>>>>>>         bool "Hi6220 Stub Clock Driver"
>>>>>>         depends on COMMON_CLK_HI6220 && MAILBOX
>>>>>> +       default ARCH_HISI
>>>
>>>
>>> Instead of forcing this up on the entire arch, why not restrict it to
>>> just the cpufreq driver?  ARM_HISI_ACPU_CPUFREQ?
>>
>>
>> I'm struggling to understand the concern here. default doesn't force this
>> option on anything, it just sets the default.
>>
>> Personally I might prefer 'default y' because it is simpler and involves
>> less thinking. However the other drivers in this directory use 'default
>> ARCH_HISI' (i.e. they do not default on for a COMPILE_TEST) so copying their
>> approach is quite reasonable.
>
> No concern actually - on the contrary, this dependency needs to be
> fixed urgently. Just trying to figure out if there is a way to manage
> the dependency chain in one place and including the root config option
> into defconfig.
>
> (Thermal driver) ---- dep ---> (cpufreq driver)  ----- dep --->
> (hi2660 clock stub driver) ---- dep -----> (common hi6220 clock
> driver)

This strikes me as solving a different problem.

The thermal driver is not the only client of cpufreq. We'd *like* a 
system where CPUFREQ_DT will work regardless of whether the thermal 
driver is enabled. That can only be achieved by setting a default in 
drivers/clk/hisilicon/Kconfig .

In other words I don't think your and Leo's patches are in conflict.


> My earlier patch focused on enabling the stub driver in the case the
> thermal driver was enabled (and subsequently turning on HISI_THERMAL
> in defconfig). The EAS profiling usecase to prevent thermal-throttling
> from kicking in is a bad default to have in the kernel, IMO - it can
> be easily achieved by just changing the thermal thresholds.
>
> Something like the following, with HISI_THERMAL added to defconfig
> would give a "stable" kernel on Hikey.
>
> diff --git i/drivers/thermal/Kconfig w/drivers/thermal/Kconfig
> index 2d702ca..77597a5 100644
> --- i/drivers/thermal/Kconfig
> +++ w/drivers/thermal/Kconfig
> @@ -177,8 +177,11 @@ config THERMAL_EMULATION
>
>  config HISI_THERMAL
>   tristate "Hisilicon thermal driver"
> -   depends on (ARCH_HISI && CPU_THERMAL && OF) || COMPILE_TEST
> + depends on (ARCH_HISI && OF) || COMPILE_TEST
>   depends on HAS_IOMEM
> + select CPU_THERMAL
> + select CPUFREQ_DT
> + select STUB_CLK_HI6220

I'm actually a little uncomfortable having a thermal sensor dictate what 
cooling devices are used to react to its temperature reading. The link 
between sensors and cooling devices comes from DT.

However I admit there are other platforms (IMX and DB8500) that accept 
the same build time diktat from their thermal sensors.


Daniel.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI
  2016-08-08 20:36           ` Daniel Thompson
@ 2016-08-09  1:23             ` Leo Yan
  0 siblings, 0 replies; 9+ messages in thread
From: Leo Yan @ 2016-08-09  1:23 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Amit Kucheria, Michael Turquette, Stephen Boyd, Jiancheng Xue,
	Philipp Zabel, Rob Herring, linux-clk, LKML, Dietmar Eggemann,
	Guodong Xu

On Mon, Aug 08, 2016 at 09:36:32PM +0100, Daniel Thompson wrote:

[...]

> >My earlier patch focused on enabling the stub driver in the case the
> >thermal driver was enabled (and subsequently turning on HISI_THERMAL
> >in defconfig). The EAS profiling usecase to prevent thermal-throttling
> >from kicking in is a bad default to have in the kernel, IMO - it can
> >be easily achieved by just changing the thermal thresholds.
> >
> >Something like the following, with HISI_THERMAL added to defconfig
> >would give a "stable" kernel on Hikey.
> >
> >diff --git i/drivers/thermal/Kconfig w/drivers/thermal/Kconfig
> >index 2d702ca..77597a5 100644
> >--- i/drivers/thermal/Kconfig
> >+++ w/drivers/thermal/Kconfig
> >@@ -177,8 +177,11 @@ config THERMAL_EMULATION
> >
> > config HISI_THERMAL
> >  tristate "Hisilicon thermal driver"
> >-   depends on (ARCH_HISI && CPU_THERMAL && OF) || COMPILE_TEST
> >+ depends on (ARCH_HISI && OF) || COMPILE_TEST
> >  depends on HAS_IOMEM
> >+ select CPU_THERMAL
> >+ select CPUFREQ_DT
> >+ select STUB_CLK_HI6220
> 
> I'm actually a little uncomfortable having a thermal sensor dictate
> what cooling devices are used to react to its temperature reading.
> The link between sensors and cooling devices comes from DT.
> 
> However I admit there are other platforms (IMX and DB8500) that
> accept the same build time diktat from their thermal sensors.

For thermal enabling on Hikey with CPU cooling device, how about below
change? I checked arch/arm/configs/multi_v7_defconfig, both
CONFIG_CPUFREQ_DT and CONFIG_CPU_THERMAL have been enabled in it.
These two drivers are quite common and used by many ARM platforms.

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 0555b7c..f65336f 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -78,6 +78,7 @@ CONFIG_COMPAT=y
 CONFIG_CPU_IDLE=y
 CONFIG_ARM_CPUIDLE=y
 CONFIG_CPU_FREQ=y
+CONFIG_CPUFREQ_DT=y
 CONFIG_ARM_BIG_LITTLE_CPUFREQ=y
 CONFIG_ARM_SCPI_CPUFREQ=y
 CONFIG_NET=y
@@ -217,6 +218,7 @@ CONFIG_SENSORS_INA2XX=m
 CONFIG_SENSORS_ARM_SCPI=y
 CONFIG_THERMAL=y
 CONFIG_THERMAL_EMULATION=y
+CONFIG_CPU_THERMAL=y
 CONFIG_EXYNOS_THERMAL=y
 CONFIG_WATCHDOG=y
 CONFIG_RENESAS_WDT=y
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 2d702ca..91ebab3 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -177,8 +177,10 @@ config THERMAL_EMULATION
 
 config HISI_THERMAL
 	tristate "Hisilicon thermal driver"
-	depends on (ARCH_HISI && CPU_THERMAL && OF) || COMPILE_TEST
+	depends on ARCH_HISI || COMPILE_TEST
 	depends on HAS_IOMEM
+	depends on OF
+	default y
 	help
 	  Enable this to plug hisilicon's thermal sensor driver into the Linux
 	  thermal framework. cpufreq is used as the cooling device to throttle

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-08-09  1:23 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-08  3:37 [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI Leo Yan
2016-08-08  5:53 ` Amit Kucheria
2016-08-08  6:42   ` Leo Yan
2016-08-08 14:42     ` Amit Kucheria
2016-08-08 15:32       ` Leo Yan
2016-08-08 15:53       ` Daniel Thompson
2016-08-08 18:02         ` Amit Kucheria
2016-08-08 20:36           ` Daniel Thompson
2016-08-09  1:23             ` Leo Yan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.