Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Riku Voipio]

On 16 February 2018 at 19:35, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
> On 12/06/2017 23:12, Arnd Bergmann wrote:
>> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
>> <daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>>> <daniel.lezcano@xxxxxxxxxx> 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.
>
> What about the options:

First, as distros, automatic selection down from selecting ARCH_X is
preferred over
defconfigs. However, we also prefer to build everything possible as
modules, so "default Y"
is sometimes too strong.

> CONFIG_HI3660_MBOX
> CONFIG_HI6220_MBOX

These are tristate and platorms can boot without them.

> CONFIG_STUB_CLK_HI6220
> CONFIG_STUB_CLK_HI3660

These are bool, so default Y is ok.

> Would make sense to do something like:

>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index b9546ab..3a07dfe 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -517,7 +517,6 @@ CONFIG_COMMON_CLK_CS2000_CP=y
>  CONFIG_COMMON_CLK_S2MPS11=y
>  CONFIG_CLK_QORIQ=y
>  CONFIG_COMMON_CLK_PWM=y
> -CONFIG_STUB_CLK_HI3660=y
>  CONFIG_COMMON_CLK_QCOM=y
>  CONFIG_QCOM_CLK_SMD_RPM=y
>  CONFIG_IPQ_GCC_8074=y
> @@ -529,8 +528,6 @@ CONFIG_HWSPINLOCK_QCOM=y
>  CONFIG_ARM_MHU=y
>  CONFIG_PLATFORM_MHU=y
>  CONFIG_BCM2835_MBOX=y
> -CONFIG_HI3660_MBOX=y
> -CONFIG_HI6220_MBOX=y
>  CONFIG_ROCKCHIP_IOMMU=y
>  CONFIG_ARM_SMMU=y
>  CONFIG_ARM_SMMU_V3=y
> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
> index 1bd4355..becdb1d 100644
> --- a/drivers/clk/hisilicon/Kconfig
> +++ b/drivers/clk/hisilicon/Kconfig
> @@ -44,14 +44,17 @@ config RESET_HISI
>           Build reset controller driver for HiSilicon device chipsets.
>
>  config STUB_CLK_HI6220
> -       bool "Hi6220 Stub Clock Driver"
> -       depends on COMMON_CLK_HI6220 && MAILBOX
> -       default ARCH_HISI
> +       bool "Hi6220 Stub Clock Driver" if EXPERT
> +       depends on (COMMON_CLK_HI6220 || COMPILE_TEST)
> +       depends on MAILBOX
> +       default COMMON_CLK_HI6220
>         help
>           Build the Hisilicon Hi6220 stub clock driver.
>
>  config STUB_CLK_HI3660
> -       bool "Hi3660 Stub Clock Driver"
> -       depends on COMMON_CLK_HI3660 && MAILBOX
> +       bool "Hi3660 Stub Clock Driver" if EXPERT
> +       depends on (COMMON_CLK_HI3660 || COMPILE_TEST)
> +       depends on MAILBOX
> +       default COMMON_CLK_HI3660
>         help
>           Build the Hisilicon Hi3660 stub clock driver.
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index de8390d4..8d1726c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -109,16 +109,19 @@ config TI_MESSAGE_MANAGER
>           platform has support for the hardware block.
>
>  config HI3660_MBOX
> -       tristate "Hi3660 Mailbox"
> -       depends on ARCH_HISI && OF
> +       tristate "Hi3660 Mailbox" if EXPERT
> +       depends on (ARCH_HISI || COMPILE_TEST)
> +       depends on OF
> +       default ARCH_HISI
>         help
>           An implementation of the hi3660 mailbox. It is used to send message
>           between application processors and other processors/MCU/DSP. Select
>           Y here if you want to use Hi3660 mailbox controller.

Which kernel tree is this from? I don't see this driver in mainline.

>
>  config HI6220_MBOX
> -       tristate "Hi6220 Mailbox"
> -       depends on ARCH_HISI
> +       tristate "Hi6220 Mailbox" if EXPERT
> +       depends on (ARCH_HISI || COMPILE_TEST)
> +       default ARCH_HISI
>         help
>           An implementation of the hi6220 mailbox. It is used to send message
>           between application processors and MCU. Say Y here if you want to
>
>
>
>
> --
>  <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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel