Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Daniel Lezcano]

On 11/03/2015 01:59 AM, Krzysztof Kozlowski wrote:

On 03.11.2015 09:30, Krzysztof Kozlowski wrote:

On 02.11.2015 21:56, Daniel Lezcano wrote:

Let the platform's Kconfig to select the clock instead of having a reverse
dependency from the driver to the platform options.


Selecting user-visible symbols is rather discouraged so why not
something like this:

-   def_bool y if ARCH_EXYNOS
-   depends on !ARM64
+   bool "Exynos multi core timer driver"
+   depends on ARCH_EXYNOS || (COMPILE_TEST && ARM)


Nope, that was wrong as we loose auto-select on Exynos. Instead:
-   def_bool y if ARCH_EXYNOS
-   depends on !ARM64
+   bool "Exynos multi core timer driver" if ARM
+   depends on ARCH_EXYNOS || COMPILE_TEST
+   default y if ARCH_EXYNOS

This way we avoid select (which is a reverse dependency for the driver),
have it auto-selectable and compile tested on arm.


I think you misunderstood the patch I sent.

It does two things:

1. Follow the thumb of rule of the current Kconfig format

   - The timer driver is selected by the platform (exynos in this case)
   - User can't select the driver in the menuconfig
   - There is no dependency on the platform except for compilation test

2. Add the COMPILE_TEST

   - User can select the driver for compilation testing. This is for 
allyesconfig when doing compilation test coverage (exynos timer could be 
compiled on other platform). As the delay code is not portable, we have 
to restrict the compilation on the ARM platform, this is why there is 
the dependency on ARM.


I am currently looking at splitting the delay code in order to prevent 
this restriction on this driver and some others drivers.




Add the COMPILE_TEST option for the compilation test coverage. Due to the
non portable 'delay' code, this driver is only compilable on ARM.

Signed-off-by: Daniel Lezcano 
---
  arch/arm/mach-exynos/Kconfig | 1 +
  drivers/clocksource/Kconfig  | 4 ++--
  2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 3a10f1a..ff10539 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -27,6 +27,7 @@ menuconfig ARCH_EXYNOS
select SRAM
select THERMAL
select MFD_SYSCON
+   select CLKSRC_EXYNOS_MCT
help
  Support for SAMSUNG EXYNOS SoCs (EXYNOS4/5)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 916c36d..d829cbe 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -213,8 +213,8 @@ config CLKSRC_METAG_GENERIC
  This option enables support for the Meta per-thread timers.

  config CLKSRC_EXYNOS_MCT
-   def_bool y if ARCH_EXYNOS
-   depends on !ARM64
+   bool "Exynos multi core timer driver" if COMPILE_TEST
+   depends on ARM
help
  Support for Multi Core Timer controller on Exynos SoCs.










--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Arnd Bergmann]

On Tuesday 03 November 2015 09:40:02 Daniel Lezcano wrote:
> On 11/03/2015 01:59 AM, Krzysztof Kozlowski wrote:
> > On 03.11.2015 09:30, Krzysztof Kozlowski wrote:
> >> On 02.11.2015 21:56, Daniel Lezcano wrote:
> >>> Let the platform's Kconfig to select the clock instead of having a reverse
> >>> dependency from the driver to the platform options.
> >>
> >> Selecting user-visible symbols is rather discouraged so why not
> >> something like this:
> >>
> >> -   def_bool y if ARCH_EXYNOS
> >> -   depends on !ARM64
> >> +   bool "Exynos multi core timer driver"
> >> +   depends on ARCH_EXYNOS || (COMPILE_TEST && ARM)
> >
> > Nope, that was wrong as we loose auto-select on Exynos. Instead:
> > -   def_bool y if ARCH_EXYNOS
> > -   depends on !ARM64
> > +   bool "Exynos multi core timer driver" if ARM
> > +   depends on ARCH_EXYNOS || COMPILE_TEST
> > +   default y if ARCH_EXYNOS
> >
> > This way we avoid select (which is a reverse dependency for the driver),
> > have it auto-selectable and compile tested on arm.
> 
> I think you misunderstood the patch I sent.
> 
> It does two things:
> 
> 1. Follow the thumb of rule of the current Kconfig format
> 
> - The timer driver is selected by the platform (exynos in this case)
> - User can't select the driver in the menuconfig
> - There is no dependency on the platform except for compilation test
> 
> 2. Add the COMPILE_TEST
> 
> - User can select the driver for compilation testing. This is for 
> allyesconfig when doing compilation test coverage (exynos timer could be 
> compiled on other platform). As the delay code is not portable, we have 
> to restrict the compilation on the ARM platform, this is why there is 
> the dependency on ARM.
> 
> I am currently looking at splitting the delay code in order to prevent 
> this restriction on this driver and some others drivers.

I suspect this will come up again in the future. The problem is
really that drivers/clocksource has different rules from almost
everything else, by requiring the platform to 'select' the driver.

The second version that Krzysztof posted is how we handle this in
other driver subsystems, and I would generally prefer it to do this
consistently for everything, but John Stultz has in the past argued
strongly for using 'select' in all clocksource drivers. The reason
is that for each platform we know in advance which driver we want,
and there is never a need for the user to have to select the right
one.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Daniel Lezcano]

On 11/03/2015 11:02 AM, Arnd Bergmann wrote:

On Tuesday 03 November 2015 09:40:02 Daniel Lezcano wrote:

On 11/03/2015 01:59 AM, Krzysztof Kozlowski wrote:

On 03.11.2015 09:30, Krzysztof Kozlowski wrote:

On 02.11.2015 21:56, Daniel Lezcano wrote:

Let the platform's Kconfig to select the clock instead of having a reverse
dependency from the driver to the platform options.


Selecting user-visible symbols is rather discouraged so why not
something like this:

-   def_bool y if ARCH_EXYNOS
-   depends on !ARM64
+   bool "Exynos multi core timer driver"
+   depends on ARCH_EXYNOS || (COMPILE_TEST && ARM)


Nope, that was wrong as we loose auto-select on Exynos. Instead:
-   def_bool y if ARCH_EXYNOS
-   depends on !ARM64
+   bool "Exynos multi core timer driver" if ARM
+   depends on ARCH_EXYNOS || COMPILE_TEST
+   default y if ARCH_EXYNOS

This way we avoid select (which is a reverse dependency for the driver),
have it auto-selectable and compile tested on arm.


I think you misunderstood the patch I sent.

It does two things:

1. Follow the thumb of rule of the current Kconfig format

 - The timer driver is selected by the platform (exynos in this case)
 - User can't select the driver in the menuconfig
 - There is no dependency on the platform except for compilation test

2. Add the COMPILE_TEST

 - User can select the driver for compilation testing. This is for
allyesconfig when doing compilation test coverage (exynos timer could be
compiled on other platform). As the delay code is not portable, we have
to restrict the compilation on the ARM platform, this is why there is
the dependency on ARM.

I am currently looking at splitting the delay code in order to prevent
this restriction on this driver and some others drivers.


I suspect this will come up again in the future. The problem is
really that drivers/clocksource has different rules from almost
everything else, by requiring the platform to 'select' the driver.

The second version that Krzysztof posted is how we handle this in
other driver subsystems, and I would generally prefer it to do this
consistently for everything, but John Stultz has in the past argued
strongly for using 'select' in all clocksource drivers. The reason
is that for each platform we know in advance which driver we want,
and there is never a need for the user to have to select the right
one.


Yes, and I second John in this.



--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Krzysztof Kozlowski]

W dniu 03.11.2015 o 19:02, Arnd Bergmann pisze:
> On Tuesday 03 November 2015 09:40:02 Daniel Lezcano wrote:
>> On 11/03/2015 01:59 AM, Krzysztof Kozlowski wrote:
>>> On 03.11.2015 09:30, Krzysztof Kozlowski wrote:
 On 02.11.2015 21:56, Daniel Lezcano wrote:
> Let the platform's Kconfig to select the clock instead of having a reverse
> dependency from the driver to the platform options.

 Selecting user-visible symbols is rather discouraged so why not
 something like this:

 -   def_bool y if ARCH_EXYNOS
 -   depends on !ARM64
 +   bool "Exynos multi core timer driver"
 +   depends on ARCH_EXYNOS || (COMPILE_TEST && ARM)
>>>
>>> Nope, that was wrong as we loose auto-select on Exynos. Instead:
>>> -   def_bool y if ARCH_EXYNOS
>>> -   depends on !ARM64
>>> +   bool "Exynos multi core timer driver" if ARM
>>> +   depends on ARCH_EXYNOS || COMPILE_TEST
>>> +   default y if ARCH_EXYNOS
>>>
>>> This way we avoid select (which is a reverse dependency for the driver),
>>> have it auto-selectable and compile tested on arm.
>>
>> I think you misunderstood the patch I sent.
>>
>> It does two things:
>>
>> 1. Follow the thumb of rule of the current Kconfig format
>>
>> - The timer driver is selected by the platform (exynos in this case)
>> - User can't select the driver in the menuconfig
>> - There is no dependency on the platform except for compilation test
>>
>> 2. Add the COMPILE_TEST
>>
>> - User can select the driver for compilation testing. This is for 
>> allyesconfig when doing compilation test coverage (exynos timer could be 
>> compiled on other platform). As the delay code is not portable, we have 
>> to restrict the compilation on the ARM platform, this is why there is 
>> the dependency on ARM.
>>
>> I am currently looking at splitting the delay code in order to prevent 
>> this restriction on this driver and some others drivers.
> 
> I suspect this will come up again in the future. The problem is
> really that drivers/clocksource has different rules from almost
> everything else, by requiring the platform to 'select' the driver.
> 
> The second version that Krzysztof posted is how we handle this in
> other driver subsystems, and I would generally prefer it to do this
> consistently for everything, but John Stultz has in the past argued
> strongly for using 'select' in all clocksource drivers. The reason
> is that for each platform we know in advance which driver we want,
> and there is never a need for the user to have to select the right
> one.

Arnd, Daniel,

Sure, makes sense to me, thanks for explanation. Actually this makes me
thinking that drivers/soc/* should probably follow the same
convention... but not all of them do that.

Anyway the patch worked fine and with explanation I can only confirm:

Tested-by: Krzysztof Kozlowski 
Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof



--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Daniel Lezcano]

On 11/03/2015 01:01 PM, Krzysztof Kozlowski wrote:

Anyway the patch worked fine and with explanation I can only confirm:


Great ! Thanks Krzysztof for testing.

  -- Daniel

--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[John Stultz]

On Tue, Nov 3, 2015 at 2:02 AM, Arnd Bergmann  wrote:
> I suspect this will come up again in the future. The problem is
> really that drivers/clocksource has different rules from almost
> everything else, by requiring the platform to 'select' the driver.
>
> The second version that Krzysztof posted is how we handle this in
> other driver subsystems, and I would generally prefer it to do this
> consistently for everything, but John Stultz has in the past argued
> strongly for using 'select' in all clocksource drivers. The reason
> is that for each platform we know in advance which driver we want,
> and there is never a need for the user to have to select the right
> one.

And just to clarify, I don't necessarily think "select" is the right
method, as creating an option that defaults to Y if the right
architecture/platform support is present is fine too.

I just don't want users to have to search deeply through menuconfig to
find a clocksource driver checkbox when they have already selected a
platform and provided enough information for us to know which
clocksource driver is needed.  So my argument its really all about
avoiding user-prompts in kconfig for clocksources.  It is conceptually
easier to do this w/ select, but you can also do it via default y and
proper dependencies.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Daniel Lezcano]

Let the platform's Kconfig to select the clock instead of having a reverse
dependency from the driver to the platform options.

Add the COMPILE_TEST option for the compilation test coverage. Due to the
non portable 'delay' code, this driver is only compilable on ARM.

Signed-off-by: Daniel Lezcano 
---
 arch/arm/mach-exynos/Kconfig | 1 +
 drivers/clocksource/Kconfig  | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 3a10f1a..ff10539 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -27,6 +27,7 @@ menuconfig ARCH_EXYNOS
select SRAM
select THERMAL
select MFD_SYSCON
+   select CLKSRC_EXYNOS_MCT
help
  Support for SAMSUNG EXYNOS SoCs (EXYNOS4/5)
 
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 916c36d..d829cbe 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -213,8 +213,8 @@ config CLKSRC_METAG_GENERIC
  This option enables support for the Meta per-thread timers.
 
 config CLKSRC_EXYNOS_MCT
-   def_bool y if ARCH_EXYNOS
-   depends on !ARM64
+   bool "Exynos multi core timer driver" if COMPILE_TEST
+   depends on ARM
help
  Support for Multi Core Timer controller on Exynos SoCs.
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Chanwoo Choi]

Hi,

On 2015년 11월 02일 21:56, Daniel Lezcano wrote:
> Let the platform's Kconfig to select the clock instead of having a reverse
> dependency from the driver to the platform options.
> 
> Add the COMPILE_TEST option for the compilation test coverage. Due to the
> non portable 'delay' code, this driver is only compilable on ARM.
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  arch/arm/mach-exynos/Kconfig | 1 +
>  drivers/clocksource/Kconfig  | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index 3a10f1a..ff10539 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -27,6 +27,7 @@ menuconfig ARCH_EXYNOS
>   select SRAM
>   select THERMAL
>   select MFD_SYSCON
> + select CLKSRC_EXYNOS_MCT
>   help
> Support for SAMSUNG EXYNOS SoCs (EXYNOS4/5)
>  
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 916c36d..d829cbe 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -213,8 +213,8 @@ config CLKSRC_METAG_GENERIC
> This option enables support for the Meta per-thread timers.
>  
>  config CLKSRC_EXYNOS_MCT
> - def_bool y if ARCH_EXYNOS
> - depends on !ARM64
> + bool "Exynos multi core timer driver" if COMPILE_TEST
> + depends on ARM
>   help
> Support for Multi Core Timer controller on Exynos SoCs.
>  
> 

Looks good to me. As you commented, delay_timer is only used on ARM 32bit.
I'm glad to use the 'depends on ARM' instead of reverse dependency.

Reviewed-by: Chanwoo Choi 

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Krzysztof Kozlowski]

On 02.11.2015 21:56, Daniel Lezcano wrote:
> Let the platform's Kconfig to select the clock instead of having a reverse
> dependency from the driver to the platform options.

Selecting user-visible symbols is rather discouraged so why not
something like this:

-   def_bool y if ARCH_EXYNOS
-   depends on !ARM64
+   bool "Exynos multi core timer driver"
+   depends on ARCH_EXYNOS || (COMPILE_TEST && ARM)

Best regards,
Krzysztof

> 
> Add the COMPILE_TEST option for the compilation test coverage. Due to the
> non portable 'delay' code, this driver is only compilable on ARM.
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  arch/arm/mach-exynos/Kconfig | 1 +
>  drivers/clocksource/Kconfig  | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index 3a10f1a..ff10539 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -27,6 +27,7 @@ menuconfig ARCH_EXYNOS
>   select SRAM
>   select THERMAL
>   select MFD_SYSCON
> + select CLKSRC_EXYNOS_MCT
>   help
> Support for SAMSUNG EXYNOS SoCs (EXYNOS4/5)
>  
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 916c36d..d829cbe 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -213,8 +213,8 @@ config CLKSRC_METAG_GENERIC
> This option enables support for the Meta per-thread timers.
>  
>  config CLKSRC_EXYNOS_MCT
> - def_bool y if ARCH_EXYNOS
> - depends on !ARM64
> + bool "Exynos multi core timer driver" if COMPILE_TEST
> + depends on ARM
>   help
> Support for Multi Core Timer controller on Exynos SoCs.
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/22] clocksource/drivers/exynos_mct: Fix Kconfig and add COMPILE_TEST option

[Krzysztof Kozlowski]

On 03.11.2015 09:30, Krzysztof Kozlowski wrote:
> On 02.11.2015 21:56, Daniel Lezcano wrote:
>> Let the platform's Kconfig to select the clock instead of having a reverse
>> dependency from the driver to the platform options.
> 
> Selecting user-visible symbols is rather discouraged so why not
> something like this:
> 
> -   def_bool y if ARCH_EXYNOS
> -   depends on !ARM64
> +   bool "Exynos multi core timer driver"
> +   depends on ARCH_EXYNOS || (COMPILE_TEST && ARM)

Nope, that was wrong as we loose auto-select on Exynos. Instead:
-   def_bool y if ARCH_EXYNOS
-   depends on !ARM64
+   bool "Exynos multi core timer driver" if ARM
+   depends on ARCH_EXYNOS || COMPILE_TEST
+   default y if ARCH_EXYNOS

This way we avoid select (which is a reverse dependency for the driver),
have it auto-selectable and compile tested on arm.

Best regards,
Krzysztof

> 
>>
>> Add the COMPILE_TEST option for the compilation test coverage. Due to the
>> non portable 'delay' code, this driver is only compilable on ARM.
>>
>> Signed-off-by: Daniel Lezcano 
>> ---
>>  arch/arm/mach-exynos/Kconfig | 1 +
>>  drivers/clocksource/Kconfig  | 4 ++--
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
>> index 3a10f1a..ff10539 100644
>> --- a/arch/arm/mach-exynos/Kconfig
>> +++ b/arch/arm/mach-exynos/Kconfig
>> @@ -27,6 +27,7 @@ menuconfig ARCH_EXYNOS
>>  select SRAM
>>  select THERMAL
>>  select MFD_SYSCON
>> +select CLKSRC_EXYNOS_MCT
>>  help
>>Support for SAMSUNG EXYNOS SoCs (EXYNOS4/5)
>>  
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 916c36d..d829cbe 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -213,8 +213,8 @@ config CLKSRC_METAG_GENERIC
>>This option enables support for the Meta per-thread timers.
>>  
>>  config CLKSRC_EXYNOS_MCT
>> -def_bool y if ARCH_EXYNOS
>> -depends on !ARM64
>> +bool "Exynos multi core timer driver" if COMPILE_TEST
>> +depends on ARM
>>  help
>>Support for Multi Core Timer controller on Exynos SoCs.
>>  
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html