[PATCH 0/3] Fixes for timer-ti-dm systimer posted mode

[PATCH 0/3] Fixes for timer-ti-dm systimer posted mode

[Tony Lindgren]

Hi all,

Here are few timer-ti-dm fixes. The first fix corrects the status bit
check order for posted mode. The other two are minor fixes noticed while
reviewing and testing the code.

Regards,

Tony


Tony Lindgren (3):
  clocksource/drivers/timer-ti-dm: Fix posted mode status check order
  clocksource/drivers/timer-ti-dm: Remove extra of_node_put()
  clocksource/drivers/timer-ti-dm: Add missing set_state_oneshot_stopped

 drivers/clocksource/timer-ti-dm-systimer.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

-- 
2.30.1

[PATCH 0/3] Fixes for timer-ti-dm systimer posted mode

[Tony Lindgren]

Hi all,

Here are few timer-ti-dm fixes. The first fix corrects the status bit
check order for posted mode. The other two are minor fixes noticed while
reviewing and testing the code.

Regards,

Tony


Tony Lindgren (3):
  clocksource/drivers/timer-ti-dm: Fix posted mode status check order
  clocksource/drivers/timer-ti-dm: Remove extra of_node_put()
  clocksource/drivers/timer-ti-dm: Add missing set_state_oneshot_stopped

 drivers/clocksource/timer-ti-dm-systimer.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

-- 
2.30.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[Tony Lindgren]

When the timer is configured in posted mode, we need to check the write-
posted status register (TWPS) before writing to the register.

We now check TWPS after the write starting with commit 52762fbd1c47
("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource
support").

For example, in the TRM for am571x the following is documented in chapter
"22.2.4.13.1.1 Write Posting Synchronization Mode":

"For each register, a status bit is provided in the timer write-posted
 status (TWPS) register. In this mode, it is mandatory that software check
 this status bit before any write access. If a write is attempted to a
 register with a previous access pending, the previous access is discarded
 without notice."

The regression happened when I updated the code to use standard read/write
accessors for the driver instead of using __omap_dm_timer_load_start().
We have__omap_dm_timer_load_start() check the TWPS status correctly using
__omap_dm_timer_write().

Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/clocksource/timer-ti-dm-systimer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
--- a/drivers/clocksource/timer-ti-dm-systimer.c
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
 	struct dmtimer_systimer *t = &clkevt->t;
 	void __iomem *pend = t->base + t->pend;
 
-	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
 	while (readl_relaxed(pend) & WP_TCRR)
 		cpu_relax();
+	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
 
-	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
 	while (readl_relaxed(pend) & WP_TCLR)
 		cpu_relax();
+	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
 
 	return 0;
 }
@@ -490,18 +490,18 @@ static int dmtimer_set_periodic(struct clock_event_device *evt)
 	dmtimer_clockevent_shutdown(evt);
 
 	/* Looks like we need to first set the load value separately */
-	writel_relaxed(clkevt->period, t->base + t->load);
 	while (readl_relaxed(pend) & WP_TLDR)
 		cpu_relax();
+	writel_relaxed(clkevt->period, t->base + t->load);
 
-	writel_relaxed(clkevt->period, t->base + t->counter);
 	while (readl_relaxed(pend) & WP_TCRR)
 		cpu_relax();
+	writel_relaxed(clkevt->period, t->base + t->counter);
 
-	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
-		       t->base + t->ctrl);
 	while (readl_relaxed(pend) & WP_TCLR)
 		cpu_relax();
+	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
+		       t->base + t->ctrl);
 
 	return 0;
 }
-- 
2.30.1

[PATCH 1/3] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[Tony Lindgren]

When the timer is configured in posted mode, we need to check the write-
posted status register (TWPS) before writing to the register.

We now check TWPS after the write starting with commit 52762fbd1c47
("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource
support").

For example, in the TRM for am571x the following is documented in chapter
"22.2.4.13.1.1 Write Posting Synchronization Mode":

"For each register, a status bit is provided in the timer write-posted
 status (TWPS) register. In this mode, it is mandatory that software check
 this status bit before any write access. If a write is attempted to a
 register with a previous access pending, the previous access is discarded
 without notice."

The regression happened when I updated the code to use standard read/write
accessors for the driver instead of using __omap_dm_timer_load_start().
We have__omap_dm_timer_load_start() check the TWPS status correctly using
__omap_dm_timer_write().

Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/clocksource/timer-ti-dm-systimer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
--- a/drivers/clocksource/timer-ti-dm-systimer.c
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
 	struct dmtimer_systimer *t = &clkevt->t;
 	void __iomem *pend = t->base + t->pend;
 
-	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
 	while (readl_relaxed(pend) & WP_TCRR)
 		cpu_relax();
+	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
 
-	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
 	while (readl_relaxed(pend) & WP_TCLR)
 		cpu_relax();
+	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
 
 	return 0;
 }
@@ -490,18 +490,18 @@ static int dmtimer_set_periodic(struct clock_event_device *evt)
 	dmtimer_clockevent_shutdown(evt);
 
 	/* Looks like we need to first set the load value separately */
-	writel_relaxed(clkevt->period, t->base + t->load);
 	while (readl_relaxed(pend) & WP_TLDR)
 		cpu_relax();
+	writel_relaxed(clkevt->period, t->base + t->load);
 
-	writel_relaxed(clkevt->period, t->base + t->counter);
 	while (readl_relaxed(pend) & WP_TCRR)
 		cpu_relax();
+	writel_relaxed(clkevt->period, t->base + t->counter);
 
-	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
-		       t->base + t->ctrl);
 	while (readl_relaxed(pend) & WP_TCLR)
 		cpu_relax();
+	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
+		       t->base + t->ctrl);
 
 	return 0;
 }
-- 
2.30.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Tony Lindgren]

We have of_translate_address() already do of_node_put() as needed.
I probably looked at __of_translate_address() earlier by accident
that of_translate_address() uses.

Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/clocksource/timer-ti-dm-systimer.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
--- a/drivers/clocksource/timer-ti-dm-systimer.c
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -265,7 +265,6 @@ static void __init dmtimer_systimer_assign_alwon(void)
 				    pa == 0x48318000)
 					continue;
 
-				of_node_put(np);
 				break;
 			}
 		}
@@ -300,7 +299,6 @@ static u32 __init dmtimer_systimer_find_first_available(void)
 				continue;
 			}
 
-			of_node_put(np);
 			break;
 		}
 	}
-- 
2.30.1

[PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Tony Lindgren]

We have of_translate_address() already do of_node_put() as needed.
I probably looked at __of_translate_address() earlier by accident
that of_translate_address() uses.

Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/clocksource/timer-ti-dm-systimer.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
--- a/drivers/clocksource/timer-ti-dm-systimer.c
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -265,7 +265,6 @@ static void __init dmtimer_systimer_assign_alwon(void)
 				    pa == 0x48318000)
 					continue;
 
-				of_node_put(np);
 				break;
 			}
 		}
@@ -300,7 +299,6 @@ static u32 __init dmtimer_systimer_find_first_available(void)
 				continue;
 			}
 
-			of_node_put(np);
 			break;
 		}
 	}
-- 
2.30.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] clocksource/drivers/timer-ti-dm: Add missing set_state_oneshot_stopped

[Tony Lindgren]

To avoid spurious timer interrupts when KTIME_MAX is used, we need to
configure set_state_oneshot_stopped(). Although implementing this is
optional, it still affects things like power management for the extra
timer interrupt.

For more information, please see commit 8fff52fd5093 ("clockevents:
Introduce CLOCK_EVT_STATE_ONESHOT_STOPPED state") and commit cf8c5009ee37
("clockevents/drivers/arm_arch_timer: Implement
->set_state_oneshot_stopped()").

Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/clocksource/timer-ti-dm-systimer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
--- a/drivers/clocksource/timer-ti-dm-systimer.c
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -552,6 +552,7 @@ static int __init dmtimer_clockevent_init(struct device_node *np)
 	dev->set_state_shutdown = dmtimer_clockevent_shutdown;
 	dev->set_state_periodic = dmtimer_set_periodic;
 	dev->set_state_oneshot = dmtimer_clockevent_shutdown;
+	dev->set_state_oneshot_stopped = dmtimer_clockevent_shutdown;
 	dev->tick_resume = dmtimer_clockevent_shutdown;
 	dev->cpumask = cpu_possible_mask;
 
-- 
2.30.1

[PATCH 3/3] clocksource/drivers/timer-ti-dm: Add missing set_state_oneshot_stopped

[Tony Lindgren]

To avoid spurious timer interrupts when KTIME_MAX is used, we need to
configure set_state_oneshot_stopped(). Although implementing this is
optional, it still affects things like power management for the extra
timer interrupt.

For more information, please see commit 8fff52fd5093 ("clockevents:
Introduce CLOCK_EVT_STATE_ONESHOT_STOPPED state") and commit cf8c5009ee37
("clockevents/drivers/arm_arch_timer: Implement
->set_state_oneshot_stopped()").

Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/clocksource/timer-ti-dm-systimer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
--- a/drivers/clocksource/timer-ti-dm-systimer.c
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -552,6 +552,7 @@ static int __init dmtimer_clockevent_init(struct device_node *np)
 	dev->set_state_shutdown = dmtimer_clockevent_shutdown;
 	dev->set_state_periodic = dmtimer_set_periodic;
 	dev->set_state_oneshot = dmtimer_clockevent_shutdown;
+	dev->set_state_oneshot_stopped = dmtimer_clockevent_shutdown;
 	dev->tick_resume = dmtimer_clockevent_shutdown;
 	dev->cpumask = cpu_possible_mask;
 
-- 
2.30.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Grygorii Strashko]

On 04/03/2021 09:21, Tony Lindgren wrote:
> We have of_translate_address() already do of_node_put() as needed.
> I probably looked at __of_translate_address() earlier by accident
> that of_translate_address() uses.

I do not see of_node_put() in of_translate_address() and
  __of_translate_address() is doing of_node_get(dev);
?

> 
> Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/clocksource/timer-ti-dm-systimer.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
> --- a/drivers/clocksource/timer-ti-dm-systimer.c
> +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> @@ -265,7 +265,6 @@ static void __init dmtimer_systimer_assign_alwon(void)
>   				    pa == 0x48318000)
>   					continue;
>   
> -				of_node_put(np);
>   				break;
>   			}
>   		}
> @@ -300,7 +299,6 @@ static u32 __init dmtimer_systimer_find_first_available(void)
>   				continue;
>   			}
>   
> -			of_node_put(np);
>   			break;
>   		}
>   	}
> 

-- 
Best regards,
grygorii

Re: [PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Grygorii Strashko]

On 04/03/2021 09:21, Tony Lindgren wrote:
> We have of_translate_address() already do of_node_put() as needed.
> I probably looked at __of_translate_address() earlier by accident
> that of_translate_address() uses.

I do not see of_node_put() in of_translate_address() and
  __of_translate_address() is doing of_node_get(dev);
?

> 
> Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/clocksource/timer-ti-dm-systimer.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
> --- a/drivers/clocksource/timer-ti-dm-systimer.c
> +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> @@ -265,7 +265,6 @@ static void __init dmtimer_systimer_assign_alwon(void)
>   				    pa == 0x48318000)
>   					continue;
>   
> -				of_node_put(np);
>   				break;
>   			}
>   		}
> @@ -300,7 +299,6 @@ static u32 __init dmtimer_systimer_find_first_available(void)
>   				continue;
>   			}
>   
> -			of_node_put(np);
>   			break;
>   		}
>   	}
> 

-- 
Best regards,
grygorii

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[Grygorii Strashko]

On 04/03/2021 09:21, Tony Lindgren wrote:
> When the timer is configured in posted mode, we need to check the write-
> posted status register (TWPS) before writing to the register.
> 
> We now check TWPS after the write starting with commit 52762fbd1c47
> ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource
> support").
> 
> For example, in the TRM for am571x the following is documented in chapter
> "22.2.4.13.1.1 Write Posting Synchronization Mode":
> 
> "For each register, a status bit is provided in the timer write-posted
>   status (TWPS) register. In this mode, it is mandatory that software check
>   this status bit before any write access. If a write is attempted to a
>   register with a previous access pending, the previous access is discarded
>   without notice."
> 
> The regression happened when I updated the code to use standard read/write
> accessors for the driver instead of using __omap_dm_timer_load_start().
> We have__omap_dm_timer_load_start() check the TWPS status correctly using
> __omap_dm_timer_write().
> 
> Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/clocksource/timer-ti-dm-systimer.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
> --- a/drivers/clocksource/timer-ti-dm-systimer.c
> +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> @@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
>   	struct dmtimer_systimer *t = &clkevt->t;
>   	void __iomem *pend = t->base + t->pend;
>   
> -	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
>   	while (readl_relaxed(pend) & WP_TCRR)
>   		cpu_relax();
> +	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
>   
> -	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
>   	while (readl_relaxed(pend) & WP_TCLR)
>   		cpu_relax();
> +	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);

It seems static [and inline] helper here could be better solution. no?

>   
>   	return 0;
>   }
> @@ -490,18 +490,18 @@ static int dmtimer_set_periodic(struct clock_event_device *evt)
>   	dmtimer_clockevent_shutdown(evt);
>   
>   	/* Looks like we need to first set the load value separately */
> -	writel_relaxed(clkevt->period, t->base + t->load);
>   	while (readl_relaxed(pend) & WP_TLDR)
>   		cpu_relax();
> +	writel_relaxed(clkevt->period, t->base + t->load);
>   
> -	writel_relaxed(clkevt->period, t->base + t->counter);
>   	while (readl_relaxed(pend) & WP_TCRR)
>   		cpu_relax();
> +	writel_relaxed(clkevt->period, t->base + t->counter);
>   
> -	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
> -		       t->base + t->ctrl);
>   	while (readl_relaxed(pend) & WP_TCLR)
>   		cpu_relax();
> +	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
> +		       t->base + t->ctrl);
>   
>   	return 0;
>   }
> 

-- 
Best regards,
grygorii

Re: [PATCH 1/3] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[Grygorii Strashko]

On 04/03/2021 09:21, Tony Lindgren wrote:
> When the timer is configured in posted mode, we need to check the write-
> posted status register (TWPS) before writing to the register.
> 
> We now check TWPS after the write starting with commit 52762fbd1c47
> ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource
> support").
> 
> For example, in the TRM for am571x the following is documented in chapter
> "22.2.4.13.1.1 Write Posting Synchronization Mode":
> 
> "For each register, a status bit is provided in the timer write-posted
>   status (TWPS) register. In this mode, it is mandatory that software check
>   this status bit before any write access. If a write is attempted to a
>   register with a previous access pending, the previous access is discarded
>   without notice."
> 
> The regression happened when I updated the code to use standard read/write
> accessors for the driver instead of using __omap_dm_timer_load_start().
> We have__omap_dm_timer_load_start() check the TWPS status correctly using
> __omap_dm_timer_write().
> 
> Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/clocksource/timer-ti-dm-systimer.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
> --- a/drivers/clocksource/timer-ti-dm-systimer.c
> +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> @@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
>   	struct dmtimer_systimer *t = &clkevt->t;
>   	void __iomem *pend = t->base + t->pend;
>   
> -	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
>   	while (readl_relaxed(pend) & WP_TCRR)
>   		cpu_relax();
> +	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
>   
> -	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
>   	while (readl_relaxed(pend) & WP_TCLR)
>   		cpu_relax();
> +	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);

It seems static [and inline] helper here could be better solution. no?

>   
>   	return 0;
>   }
> @@ -490,18 +490,18 @@ static int dmtimer_set_periodic(struct clock_event_device *evt)
>   	dmtimer_clockevent_shutdown(evt);
>   
>   	/* Looks like we need to first set the load value separately */
> -	writel_relaxed(clkevt->period, t->base + t->load);
>   	while (readl_relaxed(pend) & WP_TLDR)
>   		cpu_relax();
> +	writel_relaxed(clkevt->period, t->base + t->load);
>   
> -	writel_relaxed(clkevt->period, t->base + t->counter);
>   	while (readl_relaxed(pend) & WP_TCRR)
>   		cpu_relax();
> +	writel_relaxed(clkevt->period, t->base + t->counter);
>   
> -	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
> -		       t->base + t->ctrl);
>   	while (readl_relaxed(pend) & WP_TCLR)
>   		cpu_relax();
> +	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
> +		       t->base + t->ctrl);
>   
>   	return 0;
>   }
> 

-- 
Best regards,
grygorii

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:56]:
> 
> 
> On 04/03/2021 09:21, Tony Lindgren wrote:
> > We have of_translate_address() already do of_node_put() as needed.
> > I probably looked at __of_translate_address() earlier by accident
> > that of_translate_address() uses.
> 
> I do not see of_node_put() in of_translate_address() and
>  __of_translate_address() is doing of_node_get(dev);
> ?

Oh right.. this is confusing.. Yeah we can ignore this patch.
We should have the use count set for only the system timer(s)
we claim.

Regards,

Tony

Re: [PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:56]:
> 
> 
> On 04/03/2021 09:21, Tony Lindgren wrote:
> > We have of_translate_address() already do of_node_put() as needed.
> > I probably looked at __of_translate_address() earlier by accident
> > that of_translate_address() uses.
> 
> I do not see of_node_put() in of_translate_address() and
>  __of_translate_address() is doing of_node_get(dev);
> ?

Oh right.. this is confusing.. Yeah we can ignore this patch.
We should have the use count set for only the system timer(s)
we claim.

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:58]:
> On 04/03/2021 09:21, Tony Lindgren wrote:
> > When the timer is configured in posted mode, we need to check the write-
> > posted status register (TWPS) before writing to the register.
...

> > --- a/drivers/clocksource/timer-ti-dm-systimer.c
> > +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> > @@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
> >   	struct dmtimer_systimer *t = &clkevt->t;
> >   	void __iomem *pend = t->base + t->pend;
> > -	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
> >   	while (readl_relaxed(pend) & WP_TCRR)
> >   		cpu_relax();
> > +	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
> > -	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
> >   	while (readl_relaxed(pend) & WP_TCLR)
> >   		cpu_relax();
> > +	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
> 
> It seems static [and inline] helper here could be better solution. no?

Well we wanted to get rid of the confusing macros. And in this case I
suspect we can eventually do just one read of the pending register for
the registers used mask rather than check the status separately multiple
times. But that needs to be carefully tested and is not a fix :)

Regards,

Tony

Re: [PATCH 1/3] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:58]:
> On 04/03/2021 09:21, Tony Lindgren wrote:
> > When the timer is configured in posted mode, we need to check the write-
> > posted status register (TWPS) before writing to the register.
...

> > --- a/drivers/clocksource/timer-ti-dm-systimer.c
> > +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> > @@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
> >   	struct dmtimer_systimer *t = &clkevt->t;
> >   	void __iomem *pend = t->base + t->pend;
> > -	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
> >   	while (readl_relaxed(pend) & WP_TCRR)
> >   		cpu_relax();
> > +	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
> > -	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
> >   	while (readl_relaxed(pend) & WP_TCLR)
> >   		cpu_relax();
> > +	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
> 
> It seems static [and inline] helper here could be better solution. no?

Well we wanted to get rid of the confusing macros. And in this case I
suspect we can eventually do just one read of the pending register for
the registers used mask rather than check the status separately multiple
times. But that needs to be carefully tested and is not a fix :)

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[Grygorii Strashko]

On 05/03/2021 09:53, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:58]:
>> On 04/03/2021 09:21, Tony Lindgren wrote:
>>> When the timer is configured in posted mode, we need to check the write-
>>> posted status register (TWPS) before writing to the register.
> ...
> 
>>> --- a/drivers/clocksource/timer-ti-dm-systimer.c
>>> +++ b/drivers/clocksource/timer-ti-dm-systimer.c
>>> @@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
>>>    	struct dmtimer_systimer *t = &clkevt->t;
>>>    	void __iomem *pend = t->base + t->pend;
>>> -	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
>>>    	while (readl_relaxed(pend) & WP_TCRR)
>>>    		cpu_relax();
>>> +	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
>>> -	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
>>>    	while (readl_relaxed(pend) & WP_TCLR)
>>>    		cpu_relax();
>>> +	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
>>
>> It seems static [and inline] helper here could be better solution. no?
> 
> Well we wanted to get rid of the confusing macros. And in this case I
> suspect we can eventually do just one read of the pending register for
> the registers used mask rather than check the status separately multiple
> times. But that needs to be carefully tested and is not a fix :)

Might work.

-- 
Best regards,
grygorii

Re: [PATCH 1/3] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[Grygorii Strashko]

On 05/03/2021 09:53, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:58]:
>> On 04/03/2021 09:21, Tony Lindgren wrote:
>>> When the timer is configured in posted mode, we need to check the write-
>>> posted status register (TWPS) before writing to the register.
> ...
> 
>>> --- a/drivers/clocksource/timer-ti-dm-systimer.c
>>> +++ b/drivers/clocksource/timer-ti-dm-systimer.c
>>> @@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
>>>    	struct dmtimer_systimer *t = &clkevt->t;
>>>    	void __iomem *pend = t->base + t->pend;
>>> -	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
>>>    	while (readl_relaxed(pend) & WP_TCRR)
>>>    		cpu_relax();
>>> +	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
>>> -	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
>>>    	while (readl_relaxed(pend) & WP_TCLR)
>>>    		cpu_relax();
>>> +	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
>>
>> It seems static [and inline] helper here could be better solution. no?
> 
> Well we wanted to get rid of the confusing macros. And in this case I
> suspect we can eventually do just one read of the pending register for
> the registers used mask rather than check the status separately multiple
> times. But that needs to be carefully tested and is not a fix :)

Might work.

-- 
Best regards,
grygorii

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Tony Lindgren]

Hi,

* Tony Lindgren <tony@atomide.com> [210305 07:58]:
> * Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:56]:
> > 
> > 
> > On 04/03/2021 09:21, Tony Lindgren wrote:
> > > We have of_translate_address() already do of_node_put() as needed.
> > > I probably looked at __of_translate_address() earlier by accident
> > > that of_translate_address() uses.
> > 
> > I do not see of_node_put() in of_translate_address() and
> >  __of_translate_address() is doing of_node_get(dev);
> > ?
> 
> Oh right.. this is confusing.. Yeah we can ignore this patch.
> We should have the use count set for only the system timer(s)
> we claim.

Daniel, would you like me to repost this series with this patch dropped?

Regards,

Tony

Re: [PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Tony Lindgren]

Hi,

* Tony Lindgren <tony@atomide.com> [210305 07:58]:
> * Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:56]:
> > 
> > 
> > On 04/03/2021 09:21, Tony Lindgren wrote:
> > > We have of_translate_address() already do of_node_put() as needed.
> > > I probably looked at __of_translate_address() earlier by accident
> > > that of_translate_address() uses.
> > 
> > I do not see of_node_put() in of_translate_address() and
> >  __of_translate_address() is doing of_node_get(dev);
> > ?
> 
> Oh right.. this is confusing.. Yeah we can ignore this patch.
> We should have the use count set for only the system timer(s)
> we claim.

Daniel, would you like me to repost this series with this patch dropped?

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Daniel Lezcano]

On 08/03/2021 16:26, Tony Lindgren wrote:
> Hi,
> 
> * Tony Lindgren <tony@atomide.com> [210305 07:58]:
>> * Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:56]:
>>>
>>>
>>> On 04/03/2021 09:21, Tony Lindgren wrote:
>>>> We have of_translate_address() already do of_node_put() as needed.
>>>> I probably looked at __of_translate_address() earlier by accident
>>>> that of_translate_address() uses.
>>>
>>> I do not see of_node_put() in of_translate_address() and
>>>  __of_translate_address() is doing of_node_get(dev);
>>> ?
>>
>> Oh right.. this is confusing.. Yeah we can ignore this patch.
>> We should have the use count set for only the system timer(s)
>> we claim.
> 
> Daniel, would you like me to repost this series with this patch dropped?

No, it is ok. I will take care of not picking it.

Thanks
  -- Daniel



-- 
<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

Re: [PATCH 2/3] clocksource/drivers/timer-ti-dm: Remove extra of_node_put()

[Daniel Lezcano]

On 08/03/2021 16:26, Tony Lindgren wrote:
> Hi,
> 
> * Tony Lindgren <tony@atomide.com> [210305 07:58]:
>> * Grygorii Strashko <grygorii.strashko@ti.com> [210304 20:56]:
>>>
>>>
>>> On 04/03/2021 09:21, Tony Lindgren wrote:
>>>> We have of_translate_address() already do of_node_put() as needed.
>>>> I probably looked at __of_translate_address() earlier by accident
>>>> that of_translate_address() uses.
>>>
>>> I do not see of_node_put() in of_translate_address() and
>>>  __of_translate_address() is doing of_node_get(dev);
>>> ?
>>
>> Oh right.. this is confusing.. Yeah we can ignore this patch.
>> We should have the use count set for only the system timer(s)
>> we claim.
> 
> Daniel, would you like me to repost this series with this patch dropped?

No, it is ok. I will take care of not picking it.

Thanks
  -- Daniel



-- 
<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

[tip: timers/core] clocksource/drivers/timer-ti-dm: Add missing set_state_oneshot_stopped

[tip-bot2 for Tony Lindgren]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     ac4daf737674b4d29e19b7c300caff3bcf7160d8
Gitweb:        https://git.kernel.org/tip/ac4daf737674b4d29e19b7c300caff3bcf7160d8
Author:        Tony Lindgren <tony@atomide.com>
AuthorDate:    Thu, 04 Mar 2021 09:21:35 +02:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Thu, 08 Apr 2021 13:23:47 +02:00

clocksource/drivers/timer-ti-dm: Add missing set_state_oneshot_stopped

To avoid spurious timer interrupts when KTIME_MAX is used, we need to
configure set_state_oneshot_stopped(). Although implementing this is
optional, it still affects things like power management for the extra
timer interrupt.

For more information, please see commit 8fff52fd5093 ("clockevents:
Introduce CLOCK_EVT_STATE_ONESHOT_STOPPED state") and commit cf8c5009ee37
("clockevents/drivers/arm_arch_timer: Implement
->set_state_oneshot_stopped()").

Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
Signed-off-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210304072135.52712-4-tony@atomide.com
---
 drivers/clocksource/timer-ti-dm-systimer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
index 3a65434..186a599 100644
--- a/drivers/clocksource/timer-ti-dm-systimer.c
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -554,6 +554,7 @@ static int __init dmtimer_clockevent_init(struct device_node *np)
 	dev->set_state_shutdown = dmtimer_clockevent_shutdown;
 	dev->set_state_periodic = dmtimer_set_periodic;
 	dev->set_state_oneshot = dmtimer_clockevent_shutdown;
+	dev->set_state_oneshot_stopped = dmtimer_clockevent_shutdown;
 	dev->tick_resume = dmtimer_clockevent_shutdown;
 	dev->cpumask = cpu_possible_mask;
 

[tip: timers/core] clocksource/drivers/timer-ti-dm: Fix posted mode status check order

[tip-bot2 for Tony Lindgren]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     212709926c5493a566ca4086ad4f4b0d4e66b553
Gitweb:        https://git.kernel.org/tip/212709926c5493a566ca4086ad4f4b0d4e66b553
Author:        Tony Lindgren <tony@atomide.com>
AuthorDate:    Thu, 04 Mar 2021 09:21:33 +02:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Thu, 08 Apr 2021 13:23:41 +02:00

clocksource/drivers/timer-ti-dm: Fix posted mode status check order

When the timer is configured in posted mode, we need to check the write-
posted status register (TWPS) before writing to the register.

We now check TWPS after the write starting with commit 52762fbd1c47
("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource
support").

For example, in the TRM for am571x the following is documented in chapter
"22.2.4.13.1.1 Write Posting Synchronization Mode":

"For each register, a status bit is provided in the timer write-posted
 status (TWPS) register. In this mode, it is mandatory that software check
 this status bit before any write access. If a write is attempted to a
 register with a previous access pending, the previous access is discarded
 without notice."

The regression happened when I updated the code to use standard read/write
accessors for the driver instead of using __omap_dm_timer_load_start().
We have__omap_dm_timer_load_start() check the TWPS status correctly using
__omap_dm_timer_write().

Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
Signed-off-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210304072135.52712-2-tony@atomide.com
---
 drivers/clocksource/timer-ti-dm-systimer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
index 614c838..3a65434 100644
--- a/drivers/clocksource/timer-ti-dm-systimer.c
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -449,13 +449,13 @@ static int dmtimer_set_next_event(unsigned long cycles,
 	struct dmtimer_systimer *t = &clkevt->t;
 	void __iomem *pend = t->base + t->pend;
 
-	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
 	while (readl_relaxed(pend) & WP_TCRR)
 		cpu_relax();
+	writel_relaxed(0xffffffff - cycles, t->base + t->counter);
 
-	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
 	while (readl_relaxed(pend) & WP_TCLR)
 		cpu_relax();
+	writel_relaxed(OMAP_TIMER_CTRL_ST, t->base + t->ctrl);
 
 	return 0;
 }
@@ -490,18 +490,18 @@ static int dmtimer_set_periodic(struct clock_event_device *evt)
 	dmtimer_clockevent_shutdown(evt);
 
 	/* Looks like we need to first set the load value separately */
-	writel_relaxed(clkevt->period, t->base + t->load);
 	while (readl_relaxed(pend) & WP_TLDR)
 		cpu_relax();
+	writel_relaxed(clkevt->period, t->base + t->load);
 
-	writel_relaxed(clkevt->period, t->base + t->counter);
 	while (readl_relaxed(pend) & WP_TCRR)
 		cpu_relax();
+	writel_relaxed(clkevt->period, t->base + t->counter);
 
-	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
-		       t->base + t->ctrl);
 	while (readl_relaxed(pend) & WP_TCLR)
 		cpu_relax();
+	writel_relaxed(OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
+		       t->base + t->ctrl);
 
 	return 0;
 }