ARM64: Kconfig: Fix the missing hi655x common clk

Message ID 1496686434-13181-1-git-send-email-daniel.lezcano@linaro.org
State New
Headers show

Commit Message

Daniel Lezcano June 5, 2017, 6:13 p.m.
With the addition of the hi655x common clock, the config option is missing
for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
the hi655x clock driver misses when initializing the power sequence via DT.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

-- 
2.7.4

Comments

Ulf Hansson June 6, 2017, 2:17 p.m. | #1
On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> With the addition of the hi655x common clock, the config option is missing

> for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because

> the hi655x clock driver misses when initializing the power sequence via DT.

>

> Cc: John Stultz <john.stultz@linaro.org>

> Cc: Ulf Hansson <ulf.hansson@linaro.org>

> Cc: Wei Xu <xuwei5@hisilicon.com>

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>


Acked-by: Ulf Hansson <ulf.hansson@linaro.org>


Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
work for Hikey.

Kind regards
Uffe

> ---

>  arch/arm64/Kconfig.platforms | 1 +

>  1 file changed, 1 insertion(+)

>

> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms

> index 73272f4..6dfe72c 100644

> --- a/arch/arm64/Kconfig.platforms

> +++ b/arch/arm64/Kconfig.platforms

> @@ -79,6 +79,7 @@ config ARCH_LG1K

>  config ARCH_HISI

>         bool "Hisilicon SoC Family"

>         select ARM_TIMER_SP804

> +       select COMMON_CLK_HI655X

>         select HISILICON_IRQ_MBIGEN if PCI

>         select PINCTRL

>         help

> --

> 2.7.4

>
Daniel Lezcano June 9, 2017, 3:46 p.m. | #2
On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> > With the addition of the hi655x common clock, the config option is missing

> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because

> > the hi655x clock driver misses when initializing the power sequence via DT.

> >

> > Cc: John Stultz <john.stultz@linaro.org>

> > Cc: Ulf Hansson <ulf.hansson@linaro.org>

> > Cc: Wei Xu <xuwei5@hisilicon.com>

> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> 

> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> 

> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi

> work for Hikey.

> 


I'm wondering if I submitted this patch for the right path.

Shall it go through arm-soc ?

+Olof, +Arnd

> 

> > ---

> >  arch/arm64/Kconfig.platforms | 1 +

> >  1 file changed, 1 insertion(+)

> >

> > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms

> > index 73272f4..6dfe72c 100644

> > --- a/arch/arm64/Kconfig.platforms

> > +++ b/arch/arm64/Kconfig.platforms

> > @@ -79,6 +79,7 @@ config ARCH_LG1K

> >  config ARCH_HISI

> >         bool "Hisilicon SoC Family"

> >         select ARM_TIMER_SP804

> > +       select COMMON_CLK_HI655X

> >         select HISILICON_IRQ_MBIGEN if PCI

> >         select PINCTRL

> >         help

> > --

> > 2.7.4

> >


-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Arnd Bergmann June 9, 2017, 8:06 p.m. | #3
On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:

>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

>> > With the addition of the hi655x common clock, the config option is missing

>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because

>> > the hi655x clock driver misses when initializing the power sequence via DT.

>> >

>> > Cc: John Stultz <john.stultz@linaro.org>

>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>

>> > Cc: Wei Xu <xuwei5@hisilicon.com>

>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>>

>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

>>

>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi

>> work for Hikey.

>>

>

> I'm wondering if I submitted this patch for the right path.

>

> Shall it go through arm-soc ?


Yes, but I'm not sure this is the right patch either. We tend to not
use 'select' for user-visible drivers, and most hisilicon platforms
won't need this driver.

I think it would be more consistent to add this to the defconfig
and regard it as a user error when the driver is disabled on a
machine that needs it.

       Arnd
John Stultz June 9, 2017, 8:15 p.m. | #4
On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano

> <daniel.lezcano@linaro.org> wrote:

>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:

>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

>>> > With the addition of the hi655x common clock, the config option is missing

>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because

>>> > the hi655x clock driver misses when initializing the power sequence via DT.

>>> >

>>> > Cc: John Stultz <john.stultz@linaro.org>

>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>

>>> > Cc: Wei Xu <xuwei5@hisilicon.com>

>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>>>

>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

>>>

>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi

>>> work for Hikey.

>>>

>>

>> I'm wondering if I submitted this patch for the right path.

>>

>> Shall it go through arm-soc ?

>

> Yes, but I'm not sure this is the right patch either. We tend to not

> use 'select' for user-visible drivers, and most hisilicon platforms

> won't need this driver.

>

> I think it would be more consistent to add this to the defconfig

> and regard it as a user error when the driver is disabled on a

> machine that needs it.


Maybe the select is not exactly in the right place, but I don't really
feel like a pmic on an SoC is a "user-visible driver". I deal with the
board often and when the new dependency was made on the clk, I would
have never have found it on my own w/o Ulf and Daniel pointing out
what I needed to enable.

thanks
-john
Arnd Bergmann June 9, 2017, 8:48 p.m. | #5
On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:

>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano

>> <daniel.lezcano@linaro.org> wrote:

>>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:

>>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

>>>> > With the addition of the hi655x common clock, the config option is missing

>>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because

>>>> > the hi655x clock driver misses when initializing the power sequence via DT.

>>>> >

>>>> > Cc: John Stultz <john.stultz@linaro.org>

>>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>

>>>> > Cc: Wei Xu <xuwei5@hisilicon.com>

>>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>>>>

>>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

>>>>

>>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi

>>>> work for Hikey.

>>>>

>>>

>>> I'm wondering if I submitted this patch for the right path.

>>>

>>> Shall it go through arm-soc ?

>>

>> Yes, but I'm not sure this is the right patch either. We tend to not

>> use 'select' for user-visible drivers, and most hisilicon platforms

>> won't need this driver.

>>

>> I think it would be more consistent to add this to the defconfig

>> and regard it as a user error when the driver is disabled on a

>> machine that needs it.

>

> Maybe the select is not exactly in the right place, but I don't really

> feel like a pmic on an SoC is a "user-visible driver". I deal with the

> board often and when the new dependency was made on the clk, I would

> have never have found it on my own w/o Ulf and Daniel pointing out

> what I needed to enable.


What I meant is that the Kconfig option is user-visible. On a very high
level, this is a result of arch/arm64/Kconfig.platforms listing only
very broad categories of SoCs, in many cases only the manufacturers
of very different chip families, which then control the visibility of the
individual Kconfig items for things like pinctrl or clk.

I now see that MFD_HI655X_PMIC is the top-level driver that you
have to select before enabling COMMON_CLK_HI655X, so the
patch is actually broken unless it actually selects both.

How about simply adding a 'default MFD_HI655X_PMIC' to
COMMON_CLK_HI655X to enable it unless it is explicitly
turned off?

      Arnd
Daniel Lezcano June 12, 2017, 9:38 a.m. | #6
On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:

> > On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> >> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano

> >> <daniel.lezcano@linaro.org> wrote:

> >>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:

> >>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> >>>> > With the addition of the hi655x common clock, the config option is missing

> >>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because

> >>>> > the hi655x clock driver misses when initializing the power sequence via DT.

> >>>> >

> >>>> > Cc: John Stultz <john.stultz@linaro.org>

> >>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>

> >>>> > Cc: Wei Xu <xuwei5@hisilicon.com>

> >>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> >>>>

> >>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> >>>>

> >>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi

> >>>> work for Hikey.

> >>>>

> >>>

> >>> I'm wondering if I submitted this patch for the right path.

> >>>

> >>> Shall it go through arm-soc ?

> >>

> >> Yes, but I'm not sure this is the right patch either. We tend to not

> >> use 'select' for user-visible drivers, and most hisilicon platforms

> >> won't need this driver.

> >>

> >> I think it would be more consistent to add this to the defconfig

> >> and regard it as a user error when the driver is disabled on a

> >> machine that needs it.

> >

> > Maybe the select is not exactly in the right place, but I don't really

> > feel like a pmic on an SoC is a "user-visible driver". I deal with the

> > board often and when the new dependency was made on the clk, I would

> > have never have found it on my own w/o Ulf and Daniel pointing out

> > what I needed to enable.

> 

> What I meant is that the Kconfig option is user-visible. On a very high

> level, this is a result of arch/arm64/Kconfig.platforms listing only

> very broad categories of SoCs, in many cases only the manufacturers

> of very different chip families, which then control the visibility of the

> individual Kconfig items for things like pinctrl or clk.

> 

> I now see that MFD_HI655X_PMIC is the top-level driver that you

> have to select before enabling COMMON_CLK_HI655X, so the

> patch is actually broken unless it actually selects both.

> 

> How about simply adding a 'default MFD_HI655X_PMIC' to

> COMMON_CLK_HI655X to enable it unless it is explicitly

> turned off?


Actually, I share John's opinion.

Ideally when we choose a platform, all the relevants devices configuration
options should be selected automatically from a single topmost node of a tree
(platform selection) to all the nodes corresponding to the devices, leaving the
user to select one simple option without knowledge of the SoC hardware
internals.

If the user is expert in the platform and knows exactly what he does, then he
can select an _EXPERT_ like option and be able to disable some drivers.

It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
MFD_HI655X_PMIC is enabled?



-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Arnd Bergmann June 12, 2017, 9:12 p.m. | #7
On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:

>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:

>> > On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:

>> >> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano

>> >> <daniel.lezcano@linaro.org> wrote:

>> >>

>> >> Yes, but I'm not sure this is the right patch either. We tend to not

>> >> use 'select' for user-visible drivers, and most hisilicon platforms

>> >> won't need this driver.

>> >>

>> >> I think it would be more consistent to add this to the defconfig

>> >> and regard it as a user error when the driver is disabled on a

>> >> machine that needs it.

>> >

>> > Maybe the select is not exactly in the right place, but I don't really

>> > feel like a pmic on an SoC is a "user-visible driver". I deal with the

>> > board often and when the new dependency was made on the clk, I would

>> > have never have found it on my own w/o Ulf and Daniel pointing out

>> > what I needed to enable.

>>

>> What I meant is that the Kconfig option is user-visible. On a very high

>> level, this is a result of arch/arm64/Kconfig.platforms listing only

>> very broad categories of SoCs, in many cases only the manufacturers

>> of very different chip families, which then control the visibility of the

>> individual Kconfig items for things like pinctrl or clk.

>>

>> I now see that MFD_HI655X_PMIC is the top-level driver that you

>> have to select before enabling COMMON_CLK_HI655X, so the

>> patch is actually broken unless it actually selects both.

>>

>> How about simply adding a 'default MFD_HI655X_PMIC' to

>> COMMON_CLK_HI655X to enable it unless it is explicitly

>> turned off?

>

> Actually, I share John's opinion.

>

> Ideally when we choose a platform, all the relevants devices configuration

> options should be selected automatically from a single topmost node of a tree

> (platform selection) to all the nodes corresponding to the devices, leaving the

> user to select one simple option without knowledge of the SoC hardware

> internals.

>

> If the user is expert in the platform and knows exactly what he does, then he

> can select an _EXPERT_ like option and be able to disable some drivers.

>

> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'

> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when

> MFD_HI655X_PMIC is enabled?


I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
a dependency on COMMON_CLK and will again cause a warning on
machines that disable that during compile testing.

Using 'select' for user-selectable options generally leads to problems,
and you are better off avoiding it. If you want to make the symbol impossible
to turn off for non-EXPERT configurations, you can write it like

config COMMON_CLK_HI655X
        tristate "Clock driver for Hi655x" if EXPERT
        depends on (MFD_HI655X_PMIC || COMPILE_TEST)
        depends on REGMAP
        default MFD_HI655X_PMIC

That way the option is completely hidden for non-EXPERT,
but still has the right default otherwise, and the dependencies
are tracked right for compile-testing.

     Arnd
Daniel Lezcano June 13, 2017, 12:48 p.m. | #8
On 12/06/2017 23:12, Arnd Bergmann wrote:
> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano

> <daniel.lezcano@linaro.org> wrote:

>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:

>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:

>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:

>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano

>>>>> <daniel.lezcano@linaro.org> wrote:

>>>>>

>>>>> Yes, but I'm not sure this is the right patch either. We tend to not

>>>>> use 'select' for user-visible drivers, and most hisilicon platforms

>>>>> won't need this driver.

>>>>>

>>>>> I think it would be more consistent to add this to the defconfig

>>>>> and regard it as a user error when the driver is disabled on a

>>>>> machine that needs it.

>>>>

>>>> Maybe the select is not exactly in the right place, but I don't really

>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the

>>>> board often and when the new dependency was made on the clk, I would

>>>> have never have found it on my own w/o Ulf and Daniel pointing out

>>>> what I needed to enable.

>>>

>>> What I meant is that the Kconfig option is user-visible. On a very high

>>> level, this is a result of arch/arm64/Kconfig.platforms listing only

>>> very broad categories of SoCs, in many cases only the manufacturers

>>> of very different chip families, which then control the visibility of the

>>> individual Kconfig items for things like pinctrl or clk.

>>>

>>> I now see that MFD_HI655X_PMIC is the top-level driver that you

>>> have to select before enabling COMMON_CLK_HI655X, so the

>>> patch is actually broken unless it actually selects both.

>>>

>>> How about simply adding a 'default MFD_HI655X_PMIC' to

>>> COMMON_CLK_HI655X to enable it unless it is explicitly

>>> turned off?

>>

>> Actually, I share John's opinion.

>>

>> Ideally when we choose a platform, all the relevants devices configuration

>> options should be selected automatically from a single topmost node of a tree

>> (platform selection) to all the nodes corresponding to the devices, leaving the

>> user to select one simple option without knowledge of the SoC hardware

>> internals.

>>

>> If the user is expert in the platform and knows exactly what he does, then he

>> can select an _EXPERT_ like option and be able to disable some drivers.

>>

>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'

>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when

>> MFD_HI655X_PMIC is enabled?

> 

> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains

> a dependency on COMMON_CLK and will again cause a warning on

> machines that disable that during compile testing.


This issue is related to the missing stubs in the includes.

> Using 'select' for user-selectable options generally leads to problems,

> and you are better off avoiding it. If you want to make the symbol impossible

> to turn off for non-EXPERT configurations, you can write it like

> 

> config COMMON_CLK_HI655X

>         tristate "Clock driver for Hi655x" if EXPERT

>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)

>         depends on REGMAP

>         default MFD_HI655X_PMIC

> 

> That way the option is completely hidden for non-EXPERT,

> but still has the right default otherwise, and the dependencies

> are tracked right for compile-testing.


Ok.

Thanks!

  -- Daniel


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Patch hide | download patch | download mbox

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 73272f4..6dfe72c 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -79,6 +79,7 @@  config ARCH_LG1K
 config ARCH_HISI
 	bool "Hisilicon SoC Family"
 	select ARM_TIMER_SP804
+	select COMMON_CLK_HI655X
 	select HISILICON_IRQ_MBIGEN if PCI
 	select PINCTRL
 	help