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

On 12/06/2017 23:12, Arnd Bergmann wrote:

On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
[off-list ref] wrote:

quoted

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

quoted

On Fri, Jun 9, 2017 at 10:15 PM, John Stultz [off-list ref] wrote:

quoted

On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann [off-list ref] wrote:

quoted

On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
[off-list ref] 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:

CONFIG_HI3660_MBOX
CONFIG_HI6220_MBOX

CONFIG_STUB_CLK_HI6220
CONFIG_STUB_CLK_HI3660

?

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.

 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