diff mbox

clk: Hi6220: enable stub clock driver for ARCH_HISI

Message ID CAHLCerPxpCVZONujNhjgQ-QKkiibKW1xYoFWOO9dGkLSO--hPQ@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Amit Kucheria Aug. 8, 2016, 6:02 p.m. UTC
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Daniel Thompson Aug. 8, 2016, 8:36 p.m. UTC | #1
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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