| Message ID | 1470627463-20570-1-git-send-email-leo.yan@linaro.org |
|---|---|
| State | Accepted |
| Commit | 9a881bc55d7b2cb4093005391613a2c517f09516 |
| Headers | show |
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 >
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 > >
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 >> >
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 > >> >
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.
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.
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(+) -- 1.9.1