| Message ID | a5050f68-3adb-6973-9164-1bb43272cbd0@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On 21/02/2018 11:30, Riku Voipio wrote: > 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. Yes, that's right. The HI6220 part is ok but the HI3660 is still not mainline yet.
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. 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