[PATCH v2 1/3] clocksource/drivers/exynos_mct: Remove mct interrupt index enum

[PATCH v2 1/3] clocksource/drivers/exynos_mct: Remove mct interrupt index enum

[Alim Akhtar]

MCT driver define an enum which list global and local timer's
irq index. Most of them are not used but MCT_G0_IRQ and
MCT_L0_IRQ and these two are at a fixed offset/index.
Get rid of this enum and use a #define for the used irq index.

No functional changes expected.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 drivers/clocksource/exynos_mct.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

* Changes since v1:
- Seperate out bumping up of max mct irq and local timer number
  as commented by Krzysztof

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 6db3d5511b0f..b3f3d27a837b 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -60,27 +60,17 @@
 #define MCT_CLKEVENTS_RATING		350
 #endif
 
+/* There are four Global timers starting with 0 offset */
+#define MCT_G0_IRQ	0
+/* Local timers count starts after global timer count */
+#define MCT_L0_IRQ	4
+#define MCT_NR_IRQS	12
+
 enum {
 	MCT_INT_SPI,
 	MCT_INT_PPI
 };
 
-enum {
-	MCT_G0_IRQ,
-	MCT_G1_IRQ,
-	MCT_G2_IRQ,
-	MCT_G3_IRQ,
-	MCT_L0_IRQ,
-	MCT_L1_IRQ,
-	MCT_L2_IRQ,
-	MCT_L3_IRQ,
-	MCT_L4_IRQ,
-	MCT_L5_IRQ,
-	MCT_L6_IRQ,
-	MCT_L7_IRQ,
-	MCT_NR_IRQS,
-};
-
 static void __iomem *reg_base;
 static unsigned long clk_rate;
 static unsigned int mct_int_type;
-- 
2.25.1

[PATCH v2 2/3] clocksource/drivers/exynos_mct: bump up mct max irq number

[Alim Akhtar]

Bump-up maximum number of MCT IRQ to match the binding
documentation.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 drivers/clocksource/exynos_mct.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index b3f3d27a837b..0c7931f7f99a 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -64,7 +64,8 @@
 #define MCT_G0_IRQ	0
 /* Local timers count starts after global timer count */
 #define MCT_L0_IRQ	4
-#define MCT_NR_IRQS	12
+/* Max number of IRQ as per DT binding document */
+#define MCT_NR_IRQS	20
 
 enum {
 	MCT_INT_SPI,
-- 
2.25.1

Re: [PATCH v2 2/3] clocksource/drivers/exynos_mct: bump up mct max irq number

[Krzysztof Kozlowski]

On 20/02/2022 14:38, Alim Akhtar wrote:
> Bump-up maximum number of MCT IRQ to match the binding
> documentation.

Would be nice to also see here comment whether there is a real case
(DTS) using more than 12, but anyway:


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

> 
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  drivers/clocksource/exynos_mct.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Best regards,
Krzysztof

[PATCH v2 3/3] clocksource/drivers/exynos_mct: bump up number of local timer

[Alim Akhtar]

As per the dt binding, maximum number of local timer can be
up to 16. Increase the array size of the _name_ variable
which holds the number of local timer name per CPU to
reflect the binding. While at it, change the magic number to a
meaningful macro.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 drivers/clocksource/exynos_mct.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 0c7931f7f99a..8d63a9376701 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -66,6 +66,8 @@
 #define MCT_L0_IRQ	4
 /* Max number of IRQ as per DT binding document */
 #define MCT_NR_IRQS	20
+/* Max number of local timers */
+#define MCT_NR_LOCAL_TIMERS	16
 
 enum {
 	MCT_INT_SPI,
@@ -80,7 +82,7 @@ static int mct_irqs[MCT_NR_IRQS];
 struct mct_clock_event_device {
 	struct clock_event_device evt;
 	unsigned long base;
-	char name[10];
+	char name[MCT_NR_LOCAL_TIMERS];
 };
 
 static void exynos4_mct_write(unsigned int value, unsigned long offset)
-- 
2.25.1

Re: [PATCH v2 3/3] clocksource/drivers/exynos_mct: bump up number of local timer

[Krzysztof Kozlowski]

On 20/02/2022 14:38, Alim Akhtar wrote:
> As per the dt binding, maximum number of local timer can be
> up to 16. Increase the array size of the _name_ variable
> which holds the number of local timer name per CPU to
> reflect the binding. While at it, change the magic number to a
> meaningful macro.

This still does not make sense. Let's say you have 16 local timers, so
why the name of clock event device should be maximum 16? How are these
related?

> 
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  drivers/clocksource/exynos_mct.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 0c7931f7f99a..8d63a9376701 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -66,6 +66,8 @@
>  #define MCT_L0_IRQ	4
>  /* Max number of IRQ as per DT binding document */
>  #define MCT_NR_IRQS	20
> +/* Max number of local timers */
> +#define MCT_NR_LOCAL_TIMERS	16
>  
>  enum {
>  	MCT_INT_SPI,
> @@ -80,7 +82,7 @@ static int mct_irqs[MCT_NR_IRQS];
>  struct mct_clock_event_device {
>  	struct clock_event_device evt;
>  	unsigned long base;
> -	char name[10];
> +	char name[MCT_NR_LOCAL_TIMERS];
>  };
>  
>  static void exynos4_mct_write(unsigned int value, unsigned long offset)


Best regards,
Krzysztof

RE: [PATCH v2 3/3] clocksource/drivers/exynos_mct: bump up number of local timer

[Alim Akhtar]

>-----Original Message-----
>From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com]
>Sent: Sunday, February 20, 2022 9:03 PM
>To: Alim Akhtar <alim.akhtar@samsung.com>; linux-arm-
>kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Cc: linux-samsung-soc@vger.kernel.org; daniel.lezcano@linaro.org;
>tglx@linutronix.de; pankaj.dubey@samsung.com;
>m.szyprowski@samsung.com
>Subject: Re: [PATCH v2 3/3] clocksource/drivers/exynos_mct: bump up
>number of local timer
>
>On 20/02/2022 14:38, Alim Akhtar wrote:
>> As per the dt binding, maximum number of local timer can be up to 16.
>> Increase the array size of the _name_ variable which holds the number
>> of local timer name per CPU to reflect the binding. While at it,
>> change the magic number to a meaningful macro.
>
>This still does not make sense. Let's say you have 16 local timers, so why the
>name of clock event device should be maximum 16? How are these related?
>
As you rightly commented on v1 and it is mainly for "mct_tick%d" with number of local timers and
local timer is per cpu. With increase in cpu number, name[10] restrict the scalability.
So either we can change to 11, or use some reference (as local timer number in this patch). 
Let me know your thought on this.

>>
>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>> ---
>>  drivers/clocksource/exynos_mct.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clocksource/exynos_mct.c
>> b/drivers/clocksource/exynos_mct.c
>> index 0c7931f7f99a..8d63a9376701 100644
>> --- a/drivers/clocksource/exynos_mct.c
>> +++ b/drivers/clocksource/exynos_mct.c
>> @@ -66,6 +66,8 @@
>>  #define MCT_L0_IRQ	4
>>  /* Max number of IRQ as per DT binding document */
>>  #define MCT_NR_IRQS	20
>> +/* Max number of local timers */
>> +#define MCT_NR_LOCAL_TIMERS	16
>>
>>  enum {
>>  	MCT_INT_SPI,
>> @@ -80,7 +82,7 @@ static int mct_irqs[MCT_NR_IRQS];  struct
>> mct_clock_event_device {
>>  	struct clock_event_device evt;
>>  	unsigned long base;
>> -	char name[10];
>> +	char name[MCT_NR_LOCAL_TIMERS];
>>  };
>>
>>  static void exynos4_mct_write(unsigned int value, unsigned long
>> offset)
>
>
>Best regards,
>Krzysztof

Re: [PATCH v2 3/3] clocksource/drivers/exynos_mct: bump up number of local timer

[Krzysztof Kozlowski]

On 20/02/2022 17:59, Alim Akhtar wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com]
>> Sent: Sunday, February 20, 2022 9:03 PM
>> To: Alim Akhtar <alim.akhtar@samsung.com>; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Cc: linux-samsung-soc@vger.kernel.org; daniel.lezcano@linaro.org;
>> tglx@linutronix.de; pankaj.dubey@samsung.com;
>> m.szyprowski@samsung.com
>> Subject: Re: [PATCH v2 3/3] clocksource/drivers/exynos_mct: bump up
>> number of local timer
>>
>> On 20/02/2022 14:38, Alim Akhtar wrote:
>>> As per the dt binding, maximum number of local timer can be up to 16.
>>> Increase the array size of the _name_ variable which holds the number
>>> of local timer name per CPU to reflect the binding. While at it,
>>> change the magic number to a meaningful macro.
>>
>> This still does not make sense. Let's say you have 16 local timers, so why the
>> name of clock event device should be maximum 16? How are these related?
>>
> As you rightly commented on v1 and it is mainly for "mct_tick%d" with number of local timers and
> local timer is per cpu. With increase in cpu number, name[10] restrict the scalability.
> So either we can change to 11, or use some reference (as local timer number in this patch). 
> Let me know your thought on this.

There could be some reference, like dedicated define. However length is
not directly related to the number of local irqs, but only number of
digits in decimal representation. If MCT_NR_LOCAL_TIMERS is increased
from 12 to 20, or from 20 to 80, the length should stay the same.

I propose to keep it at 11 and add a comment that length of the name
must be adjusted if number of interrupts grow over two digits.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] clocksource/drivers/exynos_mct: Remove mct interrupt index enum

[Krzysztof Kozlowski]

On 20/02/2022 14:38, Alim Akhtar wrote:
> MCT driver define an enum which list global and local timer's
> irq index. Most of them are not used but MCT_G0_IRQ and
> MCT_L0_IRQ and these two are at a fixed offset/index.
> Get rid of this enum and use a #define for the used irq index.
> 
> No functional changes expected.
> 
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  drivers/clocksource/exynos_mct.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> * Changes since v1:
> - Seperate out bumping up of max mct irq and local timer number
>   as commented by Krzysztof
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof