lkml.org 
[lkml]   [2018]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk
    On 16 February 2018 at 19:35, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
    > 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.
    >>
    >> 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@lists.infradead.org
    > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

    \
     
     \ /
      Last update: 2018-02-21 11:31    [W:4.762 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site