[v2,05/12] ARM: davinci: da850: switch to using the clocksource driver
diff mbox series

Message ID 20190204171757.32073-6-brgl@bgdev.pl
State New
Headers show
Series
  • ARM: davinci: modernize the timer support
Related show

Commit Message

Bartosz Golaszewski Feb. 4, 2019, 5:17 p.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We now have a proper clocksource driver for davinci. Switch the platform
to using it.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da850.c | 56 ++++++++++++++---------------------
 1 file changed, 22 insertions(+), 34 deletions(-)

Comments

David Lechner Feb. 5, 2019, 2:35 a.m. UTC | #1
On 2/4/19 11:17 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We now have a proper clocksource driver for davinci. Switch the platform
> to using it.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   arch/arm/mach-davinci/da850.c | 56 ++++++++++++++---------------------
>   1 file changed, 22 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index beb34ee42e3a..66c2ffd4885b 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -34,7 +34,8 @@
>   #include <mach/da8xx.h>
>   #include <mach/irqs.h>
>   #include <mach/pm.h>
> -#include <mach/time.h>
> +
> +#include <clocksource/timer-davinci.h>
>   
>   #include "mux.h"
>   
> @@ -436,38 +437,24 @@ static struct davinci_id da850_ids[] = {
>   	},
>   };
>   
> -static struct davinci_timer_instance da850_timer_instance[4] = {
> -	{
> -		.base		= DA8XX_TIMER64P0_BASE,
> -		.bottom_irq	= IRQ_DA8XX_TINT12_0,
> -		.top_irq	= IRQ_DA8XX_TINT34_0,
> -	},
> -	{
> -		.base		= DA8XX_TIMER64P1_BASE,
> -		.bottom_irq	= IRQ_DA8XX_TINT12_1,
> -		.top_irq	= IRQ_DA8XX_TINT34_1,
> -	},
> -	{
> -		.base		= DA850_TIMER64P2_BASE,
> -		.bottom_irq	= IRQ_DA850_TINT12_2,
> -		.top_irq	= IRQ_DA850_TINT34_2,
> -	},
> -	{
> -		.base		= DA850_TIMER64P3_BASE,
> -		.bottom_irq	= IRQ_DA850_TINT12_3,
> -		.top_irq	= IRQ_DA850_TINT34_3,
> +static const struct davinci_timer_cfg da850_timer_cfg = {
> +	.reg = {
> +		.start		= DA8XX_TIMER64P0_BASE,
> +		.end		= DA8XX_TIMER64P0_BASE + SZ_4K,

Missing minus one?

+		.end		= DA8XX_TIMER64P0_BASE + SZ_4K - 1,


> +		.flags		= IORESOURCE_MEM,
>   	},
> -};
> -
> -/*
> - * T0_BOT: Timer 0, bottom		: Used for clock_event
> - * T0_TOP: Timer 0, top			: Used for clocksource
> - * T1_BOT, T1_TOP: Timer 1, bottom & top: Used for watchdog timer
> - */
> -static struct davinci_timer_info da850_timer_info = {
> -	.timers		= da850_timer_instance,
> -	.clockevent_id	= T0_BOT,
> -	.clocksource_id	= T0_TOP,
> +	.irq = {
> +		{

It would be nice to use the enum values from <clocksource/timer-davinci.h>
here so we know which IRQ is which.

		[DAVINCI_TIMER_CLOCKEVENT_IRQ] = {

> +			.start	= IRQ_DA8XX_TINT12_0,
> +			.end	= IRQ_DA8XX_TINT12_0,

Can .end be omitted since it is the same as .start?

> +			.flags	= IORESOURCE_IRQ,
> +		},
> +		{

		[DAVINCI_TIMER_CLOCKSOURCE_IRQ] = {

> +			.start	= IRQ_DA8XX_TINT34_0,
> +			.end	= IRQ_DA8XX_TINT34_0,
> +			.flags	= IORESOURCE_IRQ,
> +		}
> +	}
>   };
>   
>   #ifdef CONFIG_CPU_FREQ
> @@ -742,7 +729,6 @@ static const struct davinci_soc_info davinci_soc_info_da850 = {
>   	.intc_type		= DAVINCI_INTC_TYPE_CP_INTC,
>   	.intc_irq_prios		= da850_default_priorities,
>   	.intc_irq_num		= DA850_N_CP_INTC_IRQ,
> -	.timer_info		= &da850_timer_info,
>   	.emac_pdata		= &da8xx_emac_pdata,
>   	.sram_dma		= DA8XX_SHARED_RAM_BASE,
>   	.sram_len		= SZ_128K,
> @@ -765,6 +751,7 @@ void __init da850_init_time(void)
>   	void __iomem *pll0;
>   	struct regmap *cfgchip;
>   	struct clk *clk;
> +	int rv;

bikeshed: int ret; and int err; are used way more often, so rv looks
strange to me.

>   
>   	clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA850_REF_FREQ);
>   
> @@ -779,7 +766,8 @@ void __init da850_init_time(void)
>   		return;
>   	}
>   
> -	davinci_timer_init(clk);
> +	rv = davinci_timer_register(clk, &da850_timer_cfg);
> +	WARN(rv, "Unable to register the timer: %d\n", rv);
>   }
>   
>   static struct resource da850_pll1_resources[] = {
>
Sekhar Nori Feb. 8, 2019, 12:06 p.m. UTC | #2
On 04/02/19 10:47 PM, Bartosz Golaszewski wrote:
> +static const struct davinci_timer_cfg da850_timer_cfg = {
> +	.reg = {
> +		.start		= DA8XX_TIMER64P0_BASE,
> +		.end		= DA8XX_TIMER64P0_BASE + SZ_4K,

SZ_4K - 1

This should have prevented watchdog timer from getting registered.

Thanks,
Sekhar
Bartosz Golaszewski Feb. 8, 2019, 12:34 p.m. UTC | #3
pt., 8 lut 2019 o 13:06 Sekhar Nori <nsekhar@ti.com> napisał(a):
>
> On 04/02/19 10:47 PM, Bartosz Golaszewski wrote:
> > +static const struct davinci_timer_cfg da850_timer_cfg = {
> > +     .reg = {
> > +             .start          = DA8XX_TIMER64P0_BASE,
> > +             .end            = DA8XX_TIMER64P0_BASE + SZ_4K,
>
> SZ_4K - 1
>
> This should have prevented watchdog timer from getting registered.
>
> Thanks,
> Sekhar

My clocksource driver doesn't call request_region() so a subsequent
devm_ioremap_resource() in the watchdog driver would still succeed. I
now fixed both the missing call and the value here.

Bart
Sekhar Nori Feb. 8, 2019, 12:37 p.m. UTC | #4
On 08/02/19 6:04 PM, Bartosz Golaszewski wrote:
> pt., 8 lut 2019 o 13:06 Sekhar Nori <nsekhar@ti.com> napisał(a):
>>
>> On 04/02/19 10:47 PM, Bartosz Golaszewski wrote:
>>> +static const struct davinci_timer_cfg da850_timer_cfg = {
>>> +     .reg = {
>>> +             .start          = DA8XX_TIMER64P0_BASE,
>>> +             .end            = DA8XX_TIMER64P0_BASE + SZ_4K,
>>
>> SZ_4K - 1
>>
>> This should have prevented watchdog timer from getting registered.
>>
>> Thanks,
>> Sekhar
> 
> My clocksource driver doesn't call request_region() so a subsequent
> devm_ioremap_resource() in the watchdog driver would still succeed. I
> now fixed both the missing call and the value here.

Ah, got it. Perhaps a call to request_region() should be added to catch
issues?

Thanks,
Sekhar
Bartosz Golaszewski Feb. 8, 2019, 12:47 p.m. UTC | #5
pt., 8 lut 2019 o 13:37 Sekhar Nori <nsekhar@ti.com> napisał(a):
>
> On 08/02/19 6:04 PM, Bartosz Golaszewski wrote:
> > pt., 8 lut 2019 o 13:06 Sekhar Nori <nsekhar@ti.com> napisał(a):
> >>
> >> On 04/02/19 10:47 PM, Bartosz Golaszewski wrote:
> >>> +static const struct davinci_timer_cfg da850_timer_cfg = {
> >>> +     .reg = {
> >>> +             .start          = DA8XX_TIMER64P0_BASE,
> >>> +             .end            = DA8XX_TIMER64P0_BASE + SZ_4K,
> >>
> >> SZ_4K - 1
> >>
> >> This should have prevented watchdog timer from getting registered.
> >>
> >> Thanks,
> >> Sekhar
> >
> > My clocksource driver doesn't call request_region() so a subsequent
> > devm_ioremap_resource() in the watchdog driver would still succeed. I
> > now fixed both the missing call and the value here.
>
> Ah, got it. Perhaps a call to request_region() should be added to catch
> issues?
>

Yes, that's what I meant by "fixing the missing call". It will be
there in v3. Actually the kernel seems to be missing a non-managed
ioremap_resource() helper. Maybe I should add it as well.

Bart

Patch
diff mbox series

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index beb34ee42e3a..66c2ffd4885b 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -34,7 +34,8 @@ 
 #include <mach/da8xx.h>
 #include <mach/irqs.h>
 #include <mach/pm.h>
-#include <mach/time.h>
+
+#include <clocksource/timer-davinci.h>
 
 #include "mux.h"
 
@@ -436,38 +437,24 @@  static struct davinci_id da850_ids[] = {
 	},
 };
 
-static struct davinci_timer_instance da850_timer_instance[4] = {
-	{
-		.base		= DA8XX_TIMER64P0_BASE,
-		.bottom_irq	= IRQ_DA8XX_TINT12_0,
-		.top_irq	= IRQ_DA8XX_TINT34_0,
-	},
-	{
-		.base		= DA8XX_TIMER64P1_BASE,
-		.bottom_irq	= IRQ_DA8XX_TINT12_1,
-		.top_irq	= IRQ_DA8XX_TINT34_1,
-	},
-	{
-		.base		= DA850_TIMER64P2_BASE,
-		.bottom_irq	= IRQ_DA850_TINT12_2,
-		.top_irq	= IRQ_DA850_TINT34_2,
-	},
-	{
-		.base		= DA850_TIMER64P3_BASE,
-		.bottom_irq	= IRQ_DA850_TINT12_3,
-		.top_irq	= IRQ_DA850_TINT34_3,
+static const struct davinci_timer_cfg da850_timer_cfg = {
+	.reg = {
+		.start		= DA8XX_TIMER64P0_BASE,
+		.end		= DA8XX_TIMER64P0_BASE + SZ_4K,
+		.flags		= IORESOURCE_MEM,
 	},
-};
-
-/*
- * T0_BOT: Timer 0, bottom		: Used for clock_event
- * T0_TOP: Timer 0, top			: Used for clocksource
- * T1_BOT, T1_TOP: Timer 1, bottom & top: Used for watchdog timer
- */
-static struct davinci_timer_info da850_timer_info = {
-	.timers		= da850_timer_instance,
-	.clockevent_id	= T0_BOT,
-	.clocksource_id	= T0_TOP,
+	.irq = {
+		{
+			.start	= IRQ_DA8XX_TINT12_0,
+			.end	= IRQ_DA8XX_TINT12_0,
+			.flags	= IORESOURCE_IRQ,
+		},
+		{
+			.start	= IRQ_DA8XX_TINT34_0,
+			.end	= IRQ_DA8XX_TINT34_0,
+			.flags	= IORESOURCE_IRQ,
+		}
+	}
 };
 
 #ifdef CONFIG_CPU_FREQ
@@ -742,7 +729,6 @@  static const struct davinci_soc_info davinci_soc_info_da850 = {
 	.intc_type		= DAVINCI_INTC_TYPE_CP_INTC,
 	.intc_irq_prios		= da850_default_priorities,
 	.intc_irq_num		= DA850_N_CP_INTC_IRQ,
-	.timer_info		= &da850_timer_info,
 	.emac_pdata		= &da8xx_emac_pdata,
 	.sram_dma		= DA8XX_SHARED_RAM_BASE,
 	.sram_len		= SZ_128K,
@@ -765,6 +751,7 @@  void __init da850_init_time(void)
 	void __iomem *pll0;
 	struct regmap *cfgchip;
 	struct clk *clk;
+	int rv;
 
 	clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA850_REF_FREQ);
 
@@ -779,7 +766,8 @@  void __init da850_init_time(void)
 		return;
 	}
 
-	davinci_timer_init(clk);
+	rv = davinci_timer_register(clk, &da850_timer_cfg);
+	WARN(rv, "Unable to register the timer: %d\n", rv);
 }
 
 static struct resource da850_pll1_resources[] = {