[PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The rating is parameterized depending on SoC generation to make sure it
takes precedence on implementations where the architected timer can't be
used. This rating is already used for the clock event device. Use the
same rating for the clock source to be consistent.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/clocksource/timer-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index f6a8eb0d7322..e6608141cccb 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
 
 	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
-				    "timer_us", TIMER_1MHz, 300, 32,
+				    "timer_us", TIMER_1MHz, rating, 32,
 				    clocksource_mmio_readl_up);
 	if (ret)
 		pr_err("failed to register clocksource: %d\n", ret);
-- 
2.21.0

[PATCH 2/2] rtc: tegra: Implement suspend clock source

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The suspend clock source for Tegra210 and earlier is currently
implemented in the Tegra timer driver. However, the suspend clock source
code accesses registers that are part of the RTC hardware block, so both
can step on each others' toes. In practice this isn't an issue, but
there is no reason why the RTC driver can't implement the clock source,
so move the code over to the tegra-rtc driver.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/clocksource/timer-tegra.c | 44 -------------------------------
 drivers/rtc/rtc-tegra.c           | 42 +++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index e6608141cccb..87eac618924d 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -21,10 +21,6 @@
 
 #include "timer-of.h"
 
-#define RTC_SECONDS		0x08
-#define RTC_SHADOW_SECONDS	0x0c
-#define RTC_MILLISECONDS	0x10
-
 #define TIMERUS_CNTR_1US	0x10
 #define TIMERUS_USEC_CFG	0x14
 #define TIMERUS_CNTR_FREEZE	0x4c
@@ -164,34 +160,6 @@ static struct delay_timer tegra_delay_timer = {
 };
 #endif
 
-static struct timer_of suspend_rtc_to = {
-	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
-};
-
-/*
- * tegra_rtc_read - Reads the Tegra RTC registers
- * Care must be taken that this function is not called while the
- * tegra_rtc driver could be executing to avoid race conditions
- * on the RTC shadow register
- */
-static u64 tegra_rtc_read_ms(struct clocksource *cs)
-{
-	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
-
-	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
-	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
-
-	return (u64)s * MSEC_PER_SEC + ms;
-}
-
-static struct clocksource suspend_rtc_clocksource = {
-	.name	= "tegra_suspend_timer",
-	.rating	= 200,
-	.read	= tegra_rtc_read_ms,
-	.mask	= CLOCKSOURCE_MASK(32),
-	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
-};
-
 static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
 {
 	if (tegra20) {
@@ -385,15 +353,3 @@ static int __init tegra20_init_timer(struct device_node *np)
 	return tegra_init_timer(np, true, rating);
 }
 TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
-
-static int __init tegra20_init_rtc(struct device_node *np)
-{
-	int ret;
-
-	ret = timer_of_init(np, &suspend_rtc_to);
-	if (ret)
-		return ret;
-
-	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
-}
-TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
index 8fa1b3febf69..6da54264a27a 100644
--- a/drivers/rtc/rtc-tegra.c
+++ b/drivers/rtc/rtc-tegra.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clocksource.h>
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -52,8 +53,15 @@ struct tegra_rtc_info {
 	struct clk *clk;
 	int irq; /* alarm and periodic IRQ */
 	spinlock_t lock;
+
+	struct clocksource clksrc;
 };
 
+static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
+{
+	return container_of(clksrc, struct tegra_rtc_info, clksrc);
+}
+
 /*
  * RTC hardware is busy when it is updating its values over AHB once every
  * eight 32 kHz clocks (~250 us). Outside of these updates the CPU is free to
@@ -268,6 +276,17 @@ static const struct rtc_class_ops tegra_rtc_ops = {
 	.alarm_irq_enable = tegra_rtc_alarm_irq_enable,
 };
 
+static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
+{
+	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
+	u32 ms, s;
+
+	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
+	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
+
+	return (u64)s * MSEC_PER_SEC + ms;
+}
+
 static const struct of_device_id tegra_rtc_dt_match[] = {
 	{ .compatible = "nvidia,tegra20-rtc", },
 	{}
@@ -339,6 +358,28 @@ static int tegra_rtc_probe(struct platform_device *pdev)
 		goto disable_clk;
 	}
 
+	/*
+	 * The Tegra RTC is the only reliable clock source that persists
+	 * across an SC7 transition (VDD_CPU and VDD_CORE off) on Tegra210
+	 * and earlier. Starting with Tegra186, the ARM v8 architected timer
+	 * is in an always on power partition and its reference clock keeps
+	 * running during SC7. Therefore, we technically don't need to have
+	 * the RTC register as a clock source on Tegra186 and later, but it
+	 * doesn't hurt either, so we just register it unconditionally here.
+	 */
+	info->clksrc.name = "tegra_rtc";
+	info->clksrc.rating = 200;
+	info->clksrc.read = tegra_rtc_read_ms;
+	info->clksrc.mask = CLOCKSOURCE_MASK(32);
+	info->clksrc.flags = CLOCK_SOURCE_SUSPEND_NONSTOP |
+			     CLOCK_SOURCE_IS_CONTINUOUS;
+
+	ret = clocksource_register_hz(&info->clksrc, 1000);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register clock source: %d\n", ret);
+		goto disable_clk;
+	}
+
 	dev_notice(&pdev->dev, "Tegra internal Real Time Clock\n");
 
 	return 0;
@@ -352,6 +393,7 @@ static int tegra_rtc_remove(struct platform_device *pdev)
 {
 	struct tegra_rtc_info *info = platform_get_drvdata(pdev);
 
+	clocksource_unregister(&info->clksrc);
 	clk_disable_unprepare(info->clk);
 
 	return 0;
-- 
2.21.0

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Dmitry Osipenko]

14.06.2019 13:47, Thierry Reding пишет:
> From: Thierry Reding <treding@nvidia.com>
> 
> The suspend clock source for Tegra210 and earlier is currently
> implemented in the Tegra timer driver. However, the suspend clock source
> code accesses registers that are part of the RTC hardware block, so both
> can step on each others' toes. In practice this isn't an issue, but
> there is no reason why the RTC driver can't implement the clock source,
> so move the code over to the tegra-rtc driver.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/clocksource/timer-tegra.c | 44 -------------------------------
>  drivers/rtc/rtc-tegra.c           | 42 +++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index e6608141cccb..87eac618924d 100644
> --- a/drivers/clocksource/timer-tegra.c
> +++ b/drivers/clocksource/timer-tegra.c
> @@ -21,10 +21,6 @@
>  
>  #include "timer-of.h"
>  
> -#define RTC_SECONDS		0x08
> -#define RTC_SHADOW_SECONDS	0x0c
> -#define RTC_MILLISECONDS	0x10
> -
>  #define TIMERUS_CNTR_1US	0x10
>  #define TIMERUS_USEC_CFG	0x14
>  #define TIMERUS_CNTR_FREEZE	0x4c
> @@ -164,34 +160,6 @@ static struct delay_timer tegra_delay_timer = {
>  };
>  #endif
>  
> -static struct timer_of suspend_rtc_to = {
> -	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
> -};
> -
> -/*
> - * tegra_rtc_read - Reads the Tegra RTC registers
> - * Care must be taken that this function is not called while the
> - * tegra_rtc driver could be executing to avoid race conditions
> - * on the RTC shadow register
> - */
> -static u64 tegra_rtc_read_ms(struct clocksource *cs)
> -{
> -	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
> -
> -	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
> -	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
> -
> -	return (u64)s * MSEC_PER_SEC + ms;
> -}
> -
> -static struct clocksource suspend_rtc_clocksource = {
> -	.name	= "tegra_suspend_timer",
> -	.rating	= 200,
> -	.read	= tegra_rtc_read_ms,
> -	.mask	= CLOCKSOURCE_MASK(32),
> -	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
> -};
> -
>  static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
>  {
>  	if (tegra20) {
> @@ -385,15 +353,3 @@ static int __init tegra20_init_timer(struct device_node *np)
>  	return tegra_init_timer(np, true, rating);
>  }
>  TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
> -
> -static int __init tegra20_init_rtc(struct device_node *np)
> -{
> -	int ret;
> -
> -	ret = timer_of_init(np, &suspend_rtc_to);
> -	if (ret)
> -		return ret;
> -
> -	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
> -}
> -TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
> diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
> index 8fa1b3febf69..6da54264a27a 100644
> --- a/drivers/rtc/rtc-tegra.c
> +++ b/drivers/rtc/rtc-tegra.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/clocksource.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> @@ -52,8 +53,15 @@ struct tegra_rtc_info {
>  	struct clk *clk;
>  	int irq; /* alarm and periodic IRQ */
>  	spinlock_t lock;
> +
> +	struct clocksource clksrc;
>  };
>  
> +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
> +{
> +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
> +}
> +
>  /*
>   * RTC hardware is busy when it is updating its values over AHB once every
>   * eight 32 kHz clocks (~250 us). Outside of these updates the CPU is free to
> @@ -268,6 +276,17 @@ static const struct rtc_class_ops tegra_rtc_ops = {
>  	.alarm_irq_enable = tegra_rtc_alarm_irq_enable,
>  };
>  
> +static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
> +{
> +	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
> +	u32 ms, s;
> +
> +	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
> +	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
> +
> +	return (u64)s * MSEC_PER_SEC + ms;
> +}
> +
>  static const struct of_device_id tegra_rtc_dt_match[] = {
>  	{ .compatible = "nvidia,tegra20-rtc", },
>  	{}
> @@ -339,6 +358,28 @@ static int tegra_rtc_probe(struct platform_device *pdev)
>  		goto disable_clk;
>  	}
>  
> +	/*
> +	 * The Tegra RTC is the only reliable clock source that persists
> +	 * across an SC7 transition (VDD_CPU and VDD_CORE off) on Tegra210
> +	 * and earlier. Starting with Tegra186, the ARM v8 architected timer
> +	 * is in an always on power partition and its reference clock keeps
> +	 * running during SC7. Therefore, we technically don't need to have
> +	 * the RTC register as a clock source on Tegra186 and later, but it
> +	 * doesn't hurt either, so we just register it unconditionally here.
> +	 */
> +	info->clksrc.name = "tegra_rtc";
> +	info->clksrc.rating = 200;
> +	info->clksrc.read = tegra_rtc_read_ms;
> +	info->clksrc.mask = CLOCKSOURCE_MASK(32);

Hm.. shouldn't this be CLOCKSOURCE_MASK(52)? Given that there are 32 bits for seconds and
10bits for milliseconds.

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Dmitry Osipenko]

14.06.2019 13:47, Thierry Reding пишет:
> From: Thierry Reding <treding@nvidia.com>
> 
> The rating is parameterized depending on SoC generation to make sure it
> takes precedence on implementations where the architected timer can't be
> used. This rating is already used for the clock event device. Use the
> same rating for the clock source to be consistent.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/clocksource/timer-tegra.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index f6a8eb0d7322..e6608141cccb 100644
> --- a/drivers/clocksource/timer-tegra.c
> +++ b/drivers/clocksource/timer-tegra.c
> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
>  
>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
> -				    "timer_us", TIMER_1MHz, 300, 32,
> +				    "timer_us", TIMER_1MHz, rating, 32,
>  				    clocksource_mmio_readl_up);
>  	if (ret)
>  		pr_err("failed to register clocksource: %d\n", ret);
> 

Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
of that already.

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Dmitry Osipenko]

14.06.2019 13:47, Thierry Reding пишет:
> From: Thierry Reding <treding@nvidia.com>
> 
> The suspend clock source for Tegra210 and earlier is currently
> implemented in the Tegra timer driver. However, the suspend clock source
> code accesses registers that are part of the RTC hardware block, so both
> can step on each others' toes. In practice this isn't an issue, but
> there is no reason why the RTC driver can't implement the clock source,
> so move the code over to the tegra-rtc driver.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---


[snip]

> +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
> +{
> +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
> +}

Shouldn't hurt to inline this function explicitly because I assume that it won't get
inlined with a certain kernel configurations, like with enabled ftracing for example.

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]

On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 13:47, Thierry Reding пишет:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The rating is parameterized depending on SoC generation to make sure it
> > takes precedence on implementations where the architected timer can't be
> > used. This rating is already used for the clock event device. Use the
> > same rating for the clock source to be consistent.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/clocksource/timer-tegra.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> > index f6a8eb0d7322..e6608141cccb 100644
> > --- a/drivers/clocksource/timer-tegra.c
> > +++ b/drivers/clocksource/timer-tegra.c
> > @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
> >  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
> >  
> >  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
> > -				    "timer_us", TIMER_1MHz, 300, 32,
> > +				    "timer_us", TIMER_1MHz, rating, 32,
> >  				    clocksource_mmio_readl_up);
> >  	if (ret)
> >  		pr_err("failed to register clocksource: %d\n", ret);
> > 
> 
> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
> of that already.

Yes, the architected timer doesn't work across an SC7 (which is what the
deepest idle state is called on Tegra210) transition, hence why we can't
use it as a suspend clocksource. I actually sent out a patch to do that,
earlier.

And yes, it's entirely possible that v5.1 doesn't work in this regard,
but we're not noticing that because we don't have suspend/resume support
for Tegra210 anyway. There are a couple of missing pieces that we need
in order to make it work.

This change in particular is only going to affect the CPU idle state
(CC7). Since the architected timer doesn't survive that either, we need
the Tegra timer to be preferred over the architected timer for normal
operation.

All of these issues go away on Tegra186 and later, where the architected
timer is in an always-on partition and has a PLL that remains on during
SC7 (and CC7).

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Dmitry Osipenko]

14.06.2019 16:22, Thierry Reding пишет:
> On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
>> 14.06.2019 13:47, Thierry Reding пишет:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> The rating is parameterized depending on SoC generation to make sure it
>>> takes precedence on implementations where the architected timer can't be
>>> used. This rating is already used for the clock event device. Use the
>>> same rating for the clock source to be consistent.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  drivers/clocksource/timer-tegra.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>>> index f6a8eb0d7322..e6608141cccb 100644
>>> --- a/drivers/clocksource/timer-tegra.c
>>> +++ b/drivers/clocksource/timer-tegra.c
>>> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>>>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
>>>  
>>>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
>>> -				    "timer_us", TIMER_1MHz, 300, 32,
>>> +				    "timer_us", TIMER_1MHz, rating, 32,
>>>  				    clocksource_mmio_readl_up);
>>>  	if (ret)
>>>  		pr_err("failed to register clocksource: %d\n", ret);
>>>
>>
>> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
>> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
>> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
>> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
>> of that already.
> 
> Yes, the architected timer doesn't work across an SC7 (which is what the
> deepest idle state is called on Tegra210) transition, hence why we can't
> use it as a suspend clocksource. I actually sent out a patch to do that,
> earlier.
> 
> And yes, it's entirely possible that v5.1 doesn't work in this regard,
> but we're not noticing that because we don't have suspend/resume support
> for Tegra210 anyway. There are a couple of missing pieces that we need
> in order to make it work.
> 
> This change in particular is only going to affect the CPU idle state
> (CC7). Since the architected timer doesn't survive that either, we need
> the Tegra timer to be preferred over the architected timer for normal
> operation.
> 
> All of these issues go away on Tegra186 and later, where the architected
> timer is in an always-on partition and has a PLL that remains on during
> SC7 (and CC7).

Thank you very much for the clarification. But then what about the
sched_clock? I suppose sched_clock will suffer on T210 as well and it's
a bit trickier case because apparently arch-timer always wins since it
has a higher precision. I guess the best solution will be to just bail
out from arch-timer's driver probe in a case of T210.

if (of_machine_is_compatible("nvidia,tegra210"))
	return 0.

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5746 bytes --]

On Fri, Jun 14, 2019 at 03:01:13PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 13:47, Thierry Reding пишет:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The suspend clock source for Tegra210 and earlier is currently
> > implemented in the Tegra timer driver. However, the suspend clock source
> > code accesses registers that are part of the RTC hardware block, so both
> > can step on each others' toes. In practice this isn't an issue, but
> > there is no reason why the RTC driver can't implement the clock source,
> > so move the code over to the tegra-rtc driver.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/clocksource/timer-tegra.c | 44 -------------------------------
> >  drivers/rtc/rtc-tegra.c           | 42 +++++++++++++++++++++++++++++
> >  2 files changed, 42 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> > index e6608141cccb..87eac618924d 100644
> > --- a/drivers/clocksource/timer-tegra.c
> > +++ b/drivers/clocksource/timer-tegra.c
> > @@ -21,10 +21,6 @@
> >  
> >  #include "timer-of.h"
> >  
> > -#define RTC_SECONDS		0x08
> > -#define RTC_SHADOW_SECONDS	0x0c
> > -#define RTC_MILLISECONDS	0x10
> > -
> >  #define TIMERUS_CNTR_1US	0x10
> >  #define TIMERUS_USEC_CFG	0x14
> >  #define TIMERUS_CNTR_FREEZE	0x4c
> > @@ -164,34 +160,6 @@ static struct delay_timer tegra_delay_timer = {
> >  };
> >  #endif
> >  
> > -static struct timer_of suspend_rtc_to = {
> > -	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
> > -};
> > -
> > -/*
> > - * tegra_rtc_read - Reads the Tegra RTC registers
> > - * Care must be taken that this function is not called while the
> > - * tegra_rtc driver could be executing to avoid race conditions
> > - * on the RTC shadow register
> > - */
> > -static u64 tegra_rtc_read_ms(struct clocksource *cs)
> > -{
> > -	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
> > -
> > -	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
> > -	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
> > -
> > -	return (u64)s * MSEC_PER_SEC + ms;
> > -}
> > -
> > -static struct clocksource suspend_rtc_clocksource = {
> > -	.name	= "tegra_suspend_timer",
> > -	.rating	= 200,
> > -	.read	= tegra_rtc_read_ms,
> > -	.mask	= CLOCKSOURCE_MASK(32),
> > -	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
> > -};
> > -
> >  static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
> >  {
> >  	if (tegra20) {
> > @@ -385,15 +353,3 @@ static int __init tegra20_init_timer(struct device_node *np)
> >  	return tegra_init_timer(np, true, rating);
> >  }
> >  TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
> > -
> > -static int __init tegra20_init_rtc(struct device_node *np)
> > -{
> > -	int ret;
> > -
> > -	ret = timer_of_init(np, &suspend_rtc_to);
> > -	if (ret)
> > -		return ret;
> > -
> > -	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
> > -}
> > -TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
> > diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
> > index 8fa1b3febf69..6da54264a27a 100644
> > --- a/drivers/rtc/rtc-tegra.c
> > +++ b/drivers/rtc/rtc-tegra.c
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <linux/clk.h>
> > +#include <linux/clocksource.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> >  #include <linux/io.h>
> > @@ -52,8 +53,15 @@ struct tegra_rtc_info {
> >  	struct clk *clk;
> >  	int irq; /* alarm and periodic IRQ */
> >  	spinlock_t lock;
> > +
> > +	struct clocksource clksrc;
> >  };
> >  
> > +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
> > +{
> > +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
> > +}
> > +
> >  /*
> >   * RTC hardware is busy when it is updating its values over AHB once every
> >   * eight 32 kHz clocks (~250 us). Outside of these updates the CPU is free to
> > @@ -268,6 +276,17 @@ static const struct rtc_class_ops tegra_rtc_ops = {
> >  	.alarm_irq_enable = tegra_rtc_alarm_irq_enable,
> >  };
> >  
> > +static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
> > +{
> > +	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
> > +	u32 ms, s;
> > +
> > +	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
> > +	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
> > +
> > +	return (u64)s * MSEC_PER_SEC + ms;
> > +}
> > +
> >  static const struct of_device_id tegra_rtc_dt_match[] = {
> >  	{ .compatible = "nvidia,tegra20-rtc", },
> >  	{}
> > @@ -339,6 +358,28 @@ static int tegra_rtc_probe(struct platform_device *pdev)
> >  		goto disable_clk;
> >  	}
> >  
> > +	/*
> > +	 * The Tegra RTC is the only reliable clock source that persists
> > +	 * across an SC7 transition (VDD_CPU and VDD_CORE off) on Tegra210
> > +	 * and earlier. Starting with Tegra186, the ARM v8 architected timer
> > +	 * is in an always on power partition and its reference clock keeps
> > +	 * running during SC7. Therefore, we technically don't need to have
> > +	 * the RTC register as a clock source on Tegra186 and later, but it
> > +	 * doesn't hurt either, so we just register it unconditionally here.
> > +	 */
> > +	info->clksrc.name = "tegra_rtc";
> > +	info->clksrc.rating = 200;
> > +	info->clksrc.read = tegra_rtc_read_ms;
> > +	info->clksrc.mask = CLOCKSOURCE_MASK(32);
> 
> Hm.. shouldn't this be CLOCKSOURCE_MASK(52)? Given that there are 32 bits for seconds and
> 10bits for milliseconds.

Did you mean to say CLOCKSOURCE_MASK(42)? Yeah, that's probably better
here.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

On Fri, Jun 14, 2019 at 03:35:14PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 13:47, Thierry Reding пишет:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The suspend clock source for Tegra210 and earlier is currently
> > implemented in the Tegra timer driver. However, the suspend clock source
> > code accesses registers that are part of the RTC hardware block, so both
> > can step on each others' toes. In practice this isn't an issue, but
> > there is no reason why the RTC driver can't implement the clock source,
> > so move the code over to the tegra-rtc driver.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> 
> 
> [snip]
> 
> > +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
> > +{
> > +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
> > +}
> 
> Shouldn't hurt to inline this function explicitly because I assume that it won't get
> inlined with a certain kernel configurations, like with enabled ftracing for example.

Yeah, makes sense.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Dmitry Osipenko]

14.06.2019 16:41, Thierry Reding пишет:
> On Fri, Jun 14, 2019 at 03:01:13PM +0300, Dmitry Osipenko wrote:
>> 14.06.2019 13:47, Thierry Reding пишет:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> The suspend clock source for Tegra210 and earlier is currently
>>> implemented in the Tegra timer driver. However, the suspend clock source
>>> code accesses registers that are part of the RTC hardware block, so both
>>> can step on each others' toes. In practice this isn't an issue, but
>>> there is no reason why the RTC driver can't implement the clock source,
>>> so move the code over to the tegra-rtc driver.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  drivers/clocksource/timer-tegra.c | 44 -------------------------------
>>>  drivers/rtc/rtc-tegra.c           | 42 +++++++++++++++++++++++++++++
>>>  2 files changed, 42 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>>> index e6608141cccb..87eac618924d 100644
>>> --- a/drivers/clocksource/timer-tegra.c
>>> +++ b/drivers/clocksource/timer-tegra.c
>>> @@ -21,10 +21,6 @@
>>>  
>>>  #include "timer-of.h"
>>>  
>>> -#define RTC_SECONDS		0x08
>>> -#define RTC_SHADOW_SECONDS	0x0c
>>> -#define RTC_MILLISECONDS	0x10
>>> -
>>>  #define TIMERUS_CNTR_1US	0x10
>>>  #define TIMERUS_USEC_CFG	0x14
>>>  #define TIMERUS_CNTR_FREEZE	0x4c
>>> @@ -164,34 +160,6 @@ static struct delay_timer tegra_delay_timer = {
>>>  };
>>>  #endif
>>>  
>>> -static struct timer_of suspend_rtc_to = {
>>> -	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
>>> -};
>>> -
>>> -/*
>>> - * tegra_rtc_read - Reads the Tegra RTC registers
>>> - * Care must be taken that this function is not called while the
>>> - * tegra_rtc driver could be executing to avoid race conditions
>>> - * on the RTC shadow register
>>> - */
>>> -static u64 tegra_rtc_read_ms(struct clocksource *cs)
>>> -{
>>> -	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
>>> -
>>> -	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
>>> -	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
>>> -
>>> -	return (u64)s * MSEC_PER_SEC + ms;
>>> -}
>>> -
>>> -static struct clocksource suspend_rtc_clocksource = {
>>> -	.name	= "tegra_suspend_timer",
>>> -	.rating	= 200,
>>> -	.read	= tegra_rtc_read_ms,
>>> -	.mask	= CLOCKSOURCE_MASK(32),
>>> -	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
>>> -};
>>> -
>>>  static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
>>>  {
>>>  	if (tegra20) {
>>> @@ -385,15 +353,3 @@ static int __init tegra20_init_timer(struct device_node *np)
>>>  	return tegra_init_timer(np, true, rating);
>>>  }
>>>  TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
>>> -
>>> -static int __init tegra20_init_rtc(struct device_node *np)
>>> -{
>>> -	int ret;
>>> -
>>> -	ret = timer_of_init(np, &suspend_rtc_to);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>> -	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
>>> -}
>>> -TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
>>> diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
>>> index 8fa1b3febf69..6da54264a27a 100644
>>> --- a/drivers/rtc/rtc-tegra.c
>>> +++ b/drivers/rtc/rtc-tegra.c
>>> @@ -6,6 +6,7 @@
>>>   */
>>>  
>>>  #include <linux/clk.h>
>>> +#include <linux/clocksource.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/init.h>
>>>  #include <linux/io.h>
>>> @@ -52,8 +53,15 @@ struct tegra_rtc_info {
>>>  	struct clk *clk;
>>>  	int irq; /* alarm and periodic IRQ */
>>>  	spinlock_t lock;
>>> +
>>> +	struct clocksource clksrc;
>>>  };
>>>  
>>> +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
>>> +{
>>> +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
>>> +}
>>> +
>>>  /*
>>>   * RTC hardware is busy when it is updating its values over AHB once every
>>>   * eight 32 kHz clocks (~250 us). Outside of these updates the CPU is free to
>>> @@ -268,6 +276,17 @@ static const struct rtc_class_ops tegra_rtc_ops = {
>>>  	.alarm_irq_enable = tegra_rtc_alarm_irq_enable,
>>>  };
>>>  
>>> +static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
>>> +{
>>> +	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
>>> +	u32 ms, s;
>>> +
>>> +	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
>>> +	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
>>> +
>>> +	return (u64)s * MSEC_PER_SEC + ms;
>>> +}
>>> +
>>>  static const struct of_device_id tegra_rtc_dt_match[] = {
>>>  	{ .compatible = "nvidia,tegra20-rtc", },
>>>  	{}
>>> @@ -339,6 +358,28 @@ static int tegra_rtc_probe(struct platform_device *pdev)
>>>  		goto disable_clk;
>>>  	}
>>>  
>>> +	/*
>>> +	 * The Tegra RTC is the only reliable clock source that persists
>>> +	 * across an SC7 transition (VDD_CPU and VDD_CORE off) on Tegra210
>>> +	 * and earlier. Starting with Tegra186, the ARM v8 architected timer
>>> +	 * is in an always on power partition and its reference clock keeps
>>> +	 * running during SC7. Therefore, we technically don't need to have
>>> +	 * the RTC register as a clock source on Tegra186 and later, but it
>>> +	 * doesn't hurt either, so we just register it unconditionally here.
>>> +	 */
>>> +	info->clksrc.name = "tegra_rtc";
>>> +	info->clksrc.rating = 200;
>>> +	info->clksrc.read = tegra_rtc_read_ms;
>>> +	info->clksrc.mask = CLOCKSOURCE_MASK(32);
>>
>> Hm.. shouldn't this be CLOCKSOURCE_MASK(52)? Given that there are 32 bits for seconds and
>> 10bits for milliseconds.
> 
> Did you mean to say CLOCKSOURCE_MASK(42)? Yeah, that's probably better
> here.

Yes, 42 :)

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3622 bytes --]

On Fri, Jun 14, 2019 at 04:29:17PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 16:22, Thierry Reding пишет:
> > On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
> >> 14.06.2019 13:47, Thierry Reding пишет:
> >>> From: Thierry Reding <treding@nvidia.com>
> >>>
> >>> The rating is parameterized depending on SoC generation to make sure it
> >>> takes precedence on implementations where the architected timer can't be
> >>> used. This rating is already used for the clock event device. Use the
> >>> same rating for the clock source to be consistent.
> >>>
> >>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>> ---
> >>>  drivers/clocksource/timer-tegra.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> >>> index f6a8eb0d7322..e6608141cccb 100644
> >>> --- a/drivers/clocksource/timer-tegra.c
> >>> +++ b/drivers/clocksource/timer-tegra.c
> >>> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
> >>>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
> >>>  
> >>>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
> >>> -				    "timer_us", TIMER_1MHz, 300, 32,
> >>> +				    "timer_us", TIMER_1MHz, rating, 32,
> >>>  				    clocksource_mmio_readl_up);
> >>>  	if (ret)
> >>>  		pr_err("failed to register clocksource: %d\n", ret);
> >>>
> >>
> >> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
> >> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
> >> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
> >> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
> >> of that already.
> > 
> > Yes, the architected timer doesn't work across an SC7 (which is what the
> > deepest idle state is called on Tegra210) transition, hence why we can't
> > use it as a suspend clocksource. I actually sent out a patch to do that,
> > earlier.
> > 
> > And yes, it's entirely possible that v5.1 doesn't work in this regard,
> > but we're not noticing that because we don't have suspend/resume support
> > for Tegra210 anyway. There are a couple of missing pieces that we need
> > in order to make it work.
> > 
> > This change in particular is only going to affect the CPU idle state
> > (CC7). Since the architected timer doesn't survive that either, we need
> > the Tegra timer to be preferred over the architected timer for normal
> > operation.
> > 
> > All of these issues go away on Tegra186 and later, where the architected
> > timer is in an always-on partition and has a PLL that remains on during
> > SC7 (and CC7).
> 
> Thank you very much for the clarification. But then what about the
> sched_clock? I suppose sched_clock will suffer on T210 as well and it's
> a bit trickier case because apparently arch-timer always wins since it
> has a higher precision. I guess the best solution will be to just bail
> out from arch-timer's driver probe in a case of T210.
> 
> if (of_machine_is_compatible("nvidia,tegra210"))
> 	return 0.

I don't think there's any issue with the scheduler clock on Tegra210.
Before the CPU can be turned off, all tasks scheduled on that CPU have
to be migrated to another CPU, right? Conversely, before any tasks can
be scheduled on a CPU that CPU needs to be brought online, at which
point the architected timer should work fine again.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Dmitry Osipenko]

14.06.2019 16:53, Thierry Reding пишет:
> On Fri, Jun 14, 2019 at 04:29:17PM +0300, Dmitry Osipenko wrote:
>> 14.06.2019 16:22, Thierry Reding пишет:
>>> On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
>>>> 14.06.2019 13:47, Thierry Reding пишет:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> The rating is parameterized depending on SoC generation to make sure it
>>>>> takes precedence on implementations where the architected timer can't be
>>>>> used. This rating is already used for the clock event device. Use the
>>>>> same rating for the clock source to be consistent.
>>>>>
>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>> ---
>>>>>  drivers/clocksource/timer-tegra.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>>>>> index f6a8eb0d7322..e6608141cccb 100644
>>>>> --- a/drivers/clocksource/timer-tegra.c
>>>>> +++ b/drivers/clocksource/timer-tegra.c
>>>>> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>>>>>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
>>>>>  
>>>>>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
>>>>> -				    "timer_us", TIMER_1MHz, 300, 32,
>>>>> +				    "timer_us", TIMER_1MHz, rating, 32,
>>>>>  				    clocksource_mmio_readl_up);
>>>>>  	if (ret)
>>>>>  		pr_err("failed to register clocksource: %d\n", ret);
>>>>>
>>>>
>>>> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
>>>> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
>>>> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
>>>> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
>>>> of that already.
>>>
>>> Yes, the architected timer doesn't work across an SC7 (which is what the
>>> deepest idle state is called on Tegra210) transition, hence why we can't
>>> use it as a suspend clocksource. I actually sent out a patch to do that,
>>> earlier.
>>>
>>> And yes, it's entirely possible that v5.1 doesn't work in this regard,
>>> but we're not noticing that because we don't have suspend/resume support
>>> for Tegra210 anyway. There are a couple of missing pieces that we need
>>> in order to make it work.
>>>
>>> This change in particular is only going to affect the CPU idle state
>>> (CC7). Since the architected timer doesn't survive that either, we need
>>> the Tegra timer to be preferred over the architected timer for normal
>>> operation.
>>>
>>> All of these issues go away on Tegra186 and later, where the architected
>>> timer is in an always-on partition and has a PLL that remains on during
>>> SC7 (and CC7).
>>
>> Thank you very much for the clarification. But then what about the
>> sched_clock? I suppose sched_clock will suffer on T210 as well and it's
>> a bit trickier case because apparently arch-timer always wins since it
>> has a higher precision. I guess the best solution will be to just bail
>> out from arch-timer's driver probe in a case of T210.
>>
>> if (of_machine_is_compatible("nvidia,tegra210"))
>> 	return 0.
> 
> I don't think there's any issue with the scheduler clock on Tegra210.
> Before the CPU can be turned off, all tasks scheduled on that CPU have
> to be migrated to another CPU, right? Conversely, before any tasks can
> be scheduled on a CPU that CPU needs to be brought online, at which
> point the architected timer should work fine again.

Is SC7 a CPU-idle state that cpuidle driver may enter or it's a
system-wide suspend state? It's still not clear to me.

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Dmitry Osipenko]

14.06.2019 17:02, Dmitry Osipenko пишет:
> 14.06.2019 16:53, Thierry Reding пишет:
>> On Fri, Jun 14, 2019 at 04:29:17PM +0300, Dmitry Osipenko wrote:
>>> 14.06.2019 16:22, Thierry Reding пишет:
>>>> On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
>>>>> 14.06.2019 13:47, Thierry Reding пишет:
>>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>>
>>>>>> The rating is parameterized depending on SoC generation to make sure it
>>>>>> takes precedence on implementations where the architected timer can't be
>>>>>> used. This rating is already used for the clock event device. Use the
>>>>>> same rating for the clock source to be consistent.
>>>>>>
>>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>>> ---
>>>>>>  drivers/clocksource/timer-tegra.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>>>>>> index f6a8eb0d7322..e6608141cccb 100644
>>>>>> --- a/drivers/clocksource/timer-tegra.c
>>>>>> +++ b/drivers/clocksource/timer-tegra.c
>>>>>> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>>>>>>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
>>>>>>  
>>>>>>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
>>>>>> -				    "timer_us", TIMER_1MHz, 300, 32,
>>>>>> +				    "timer_us", TIMER_1MHz, rating, 32,
>>>>>>  				    clocksource_mmio_readl_up);
>>>>>>  	if (ret)
>>>>>>  		pr_err("failed to register clocksource: %d\n", ret);
>>>>>>
>>>>>
>>>>> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
>>>>> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
>>>>> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
>>>>> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
>>>>> of that already.
>>>>
>>>> Yes, the architected timer doesn't work across an SC7 (which is what the
>>>> deepest idle state is called on Tegra210) transition, hence why we can't
>>>> use it as a suspend clocksource. I actually sent out a patch to do that,
>>>> earlier.
>>>>
>>>> And yes, it's entirely possible that v5.1 doesn't work in this regard,
>>>> but we're not noticing that because we don't have suspend/resume support
>>>> for Tegra210 anyway. There are a couple of missing pieces that we need
>>>> in order to make it work.
>>>>
>>>> This change in particular is only going to affect the CPU idle state
>>>> (CC7). Since the architected timer doesn't survive that either, we need
>>>> the Tegra timer to be preferred over the architected timer for normal
>>>> operation.
>>>>
>>>> All of these issues go away on Tegra186 and later, where the architected
>>>> timer is in an always-on partition and has a PLL that remains on during
>>>> SC7 (and CC7).
>>>
>>> Thank you very much for the clarification. But then what about the
>>> sched_clock? I suppose sched_clock will suffer on T210 as well and it's
>>> a bit trickier case because apparently arch-timer always wins since it
>>> has a higher precision. I guess the best solution will be to just bail
>>> out from arch-timer's driver probe in a case of T210.
>>>
>>> if (of_machine_is_compatible("nvidia,tegra210"))
>>> 	return 0.
>>
>> I don't think there's any issue with the scheduler clock on Tegra210.
>> Before the CPU can be turned off, all tasks scheduled on that CPU have
>> to be migrated to another CPU, right? Conversely, before any tasks can
>> be scheduled on a CPU that CPU needs to be brought online, at which
>> point the architected timer should work fine again.
> 
> Is SC7 a CPU-idle state that cpuidle driver may enter or it's a
> system-wide suspend state? It's still not clear to me.
> 

Ah, looks like I see now. So CC7 (CPU idle state) also affects the
arch-timer (like SC7) and hence scheduler clock will be stopped while it
shouldn't, which doesn't sound good.

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6781 bytes --]

On Fri, Jun 14, 2019 at 04:49:44PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 16:41, Thierry Reding пишет:
> > On Fri, Jun 14, 2019 at 03:01:13PM +0300, Dmitry Osipenko wrote:
> >> 14.06.2019 13:47, Thierry Reding пишет:
> >>> From: Thierry Reding <treding@nvidia.com>
> >>>
> >>> The suspend clock source for Tegra210 and earlier is currently
> >>> implemented in the Tegra timer driver. However, the suspend clock source
> >>> code accesses registers that are part of the RTC hardware block, so both
> >>> can step on each others' toes. In practice this isn't an issue, but
> >>> there is no reason why the RTC driver can't implement the clock source,
> >>> so move the code over to the tegra-rtc driver.
> >>>
> >>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>> ---
> >>>  drivers/clocksource/timer-tegra.c | 44 -------------------------------
> >>>  drivers/rtc/rtc-tegra.c           | 42 +++++++++++++++++++++++++++++
> >>>  2 files changed, 42 insertions(+), 44 deletions(-)
> >>>
> >>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> >>> index e6608141cccb..87eac618924d 100644
> >>> --- a/drivers/clocksource/timer-tegra.c
> >>> +++ b/drivers/clocksource/timer-tegra.c
> >>> @@ -21,10 +21,6 @@
> >>>  
> >>>  #include "timer-of.h"
> >>>  
> >>> -#define RTC_SECONDS		0x08
> >>> -#define RTC_SHADOW_SECONDS	0x0c
> >>> -#define RTC_MILLISECONDS	0x10
> >>> -
> >>>  #define TIMERUS_CNTR_1US	0x10
> >>>  #define TIMERUS_USEC_CFG	0x14
> >>>  #define TIMERUS_CNTR_FREEZE	0x4c
> >>> @@ -164,34 +160,6 @@ static struct delay_timer tegra_delay_timer = {
> >>>  };
> >>>  #endif
> >>>  
> >>> -static struct timer_of suspend_rtc_to = {
> >>> -	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
> >>> -};
> >>> -
> >>> -/*
> >>> - * tegra_rtc_read - Reads the Tegra RTC registers
> >>> - * Care must be taken that this function is not called while the
> >>> - * tegra_rtc driver could be executing to avoid race conditions
> >>> - * on the RTC shadow register
> >>> - */
> >>> -static u64 tegra_rtc_read_ms(struct clocksource *cs)
> >>> -{
> >>> -	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
> >>> -
> >>> -	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
> >>> -	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
> >>> -
> >>> -	return (u64)s * MSEC_PER_SEC + ms;
> >>> -}
> >>> -
> >>> -static struct clocksource suspend_rtc_clocksource = {
> >>> -	.name	= "tegra_suspend_timer",
> >>> -	.rating	= 200,
> >>> -	.read	= tegra_rtc_read_ms,
> >>> -	.mask	= CLOCKSOURCE_MASK(32),
> >>> -	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
> >>> -};
> >>> -
> >>>  static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
> >>>  {
> >>>  	if (tegra20) {
> >>> @@ -385,15 +353,3 @@ static int __init tegra20_init_timer(struct device_node *np)
> >>>  	return tegra_init_timer(np, true, rating);
> >>>  }
> >>>  TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
> >>> -
> >>> -static int __init tegra20_init_rtc(struct device_node *np)
> >>> -{
> >>> -	int ret;
> >>> -
> >>> -	ret = timer_of_init(np, &suspend_rtc_to);
> >>> -	if (ret)
> >>> -		return ret;
> >>> -
> >>> -	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
> >>> -}
> >>> -TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
> >>> diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
> >>> index 8fa1b3febf69..6da54264a27a 100644
> >>> --- a/drivers/rtc/rtc-tegra.c
> >>> +++ b/drivers/rtc/rtc-tegra.c
> >>> @@ -6,6 +6,7 @@
> >>>   */
> >>>  
> >>>  #include <linux/clk.h>
> >>> +#include <linux/clocksource.h>
> >>>  #include <linux/delay.h>
> >>>  #include <linux/init.h>
> >>>  #include <linux/io.h>
> >>> @@ -52,8 +53,15 @@ struct tegra_rtc_info {
> >>>  	struct clk *clk;
> >>>  	int irq; /* alarm and periodic IRQ */
> >>>  	spinlock_t lock;
> >>> +
> >>> +	struct clocksource clksrc;
> >>>  };
> >>>  
> >>> +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
> >>> +{
> >>> +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
> >>> +}
> >>> +
> >>>  /*
> >>>   * RTC hardware is busy when it is updating its values over AHB once every
> >>>   * eight 32 kHz clocks (~250 us). Outside of these updates the CPU is free to
> >>> @@ -268,6 +276,17 @@ static const struct rtc_class_ops tegra_rtc_ops = {
> >>>  	.alarm_irq_enable = tegra_rtc_alarm_irq_enable,
> >>>  };
> >>>  
> >>> +static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
> >>> +{
> >>> +	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
> >>> +	u32 ms, s;
> >>> +
> >>> +	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
> >>> +	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
> >>> +
> >>> +	return (u64)s * MSEC_PER_SEC + ms;
> >>> +}
> >>> +
> >>>  static const struct of_device_id tegra_rtc_dt_match[] = {
> >>>  	{ .compatible = "nvidia,tegra20-rtc", },
> >>>  	{}
> >>> @@ -339,6 +358,28 @@ static int tegra_rtc_probe(struct platform_device *pdev)
> >>>  		goto disable_clk;
> >>>  	}
> >>>  
> >>> +	/*
> >>> +	 * The Tegra RTC is the only reliable clock source that persists
> >>> +	 * across an SC7 transition (VDD_CPU and VDD_CORE off) on Tegra210
> >>> +	 * and earlier. Starting with Tegra186, the ARM v8 architected timer
> >>> +	 * is in an always on power partition and its reference clock keeps
> >>> +	 * running during SC7. Therefore, we technically don't need to have
> >>> +	 * the RTC register as a clock source on Tegra186 and later, but it
> >>> +	 * doesn't hurt either, so we just register it unconditionally here.
> >>> +	 */
> >>> +	info->clksrc.name = "tegra_rtc";
> >>> +	info->clksrc.rating = 200;
> >>> +	info->clksrc.read = tegra_rtc_read_ms;
> >>> +	info->clksrc.mask = CLOCKSOURCE_MASK(32);
> >>
> >> Hm.. shouldn't this be CLOCKSOURCE_MASK(52)? Given that there are 32 bits for seconds and
> >> 10bits for milliseconds.
> > 
> > Did you mean to say CLOCKSOURCE_MASK(42)? Yeah, that's probably better
> > here.
> 
> Yes, 42 :)

I'm wondering if that could perhaps be a little problematic because
we're not actually using all of the 10 bits for the milliseconds. So the
maximum value that we can return is:

	4294967296 * 1000 + 999 = 4294967296999

However, the maximum value for a 42 bit mask is:

	2^42 - 1 = 4398046511103

That mask is only used in order to wrap around in delta computations. So
I can imagine a situation where we'd end up with a wrong value in the
delta. I suppose this can only really happen if the two samples are very
far apart in time, so maybe this isn't worth worrying about.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4292 bytes --]

On Fri, Jun 14, 2019 at 05:02:45PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 16:53, Thierry Reding пишет:
> > On Fri, Jun 14, 2019 at 04:29:17PM +0300, Dmitry Osipenko wrote:
> >> 14.06.2019 16:22, Thierry Reding пишет:
> >>> On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
> >>>> 14.06.2019 13:47, Thierry Reding пишет:
> >>>>> From: Thierry Reding <treding@nvidia.com>
> >>>>>
> >>>>> The rating is parameterized depending on SoC generation to make sure it
> >>>>> takes precedence on implementations where the architected timer can't be
> >>>>> used. This rating is already used for the clock event device. Use the
> >>>>> same rating for the clock source to be consistent.
> >>>>>
> >>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>>>> ---
> >>>>>  drivers/clocksource/timer-tegra.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> >>>>> index f6a8eb0d7322..e6608141cccb 100644
> >>>>> --- a/drivers/clocksource/timer-tegra.c
> >>>>> +++ b/drivers/clocksource/timer-tegra.c
> >>>>> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
> >>>>>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
> >>>>>  
> >>>>>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
> >>>>> -				    "timer_us", TIMER_1MHz, 300, 32,
> >>>>> +				    "timer_us", TIMER_1MHz, rating, 32,
> >>>>>  				    clocksource_mmio_readl_up);
> >>>>>  	if (ret)
> >>>>>  		pr_err("failed to register clocksource: %d\n", ret);
> >>>>>
> >>>>
> >>>> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
> >>>> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
> >>>> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
> >>>> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
> >>>> of that already.
> >>>
> >>> Yes, the architected timer doesn't work across an SC7 (which is what the
> >>> deepest idle state is called on Tegra210) transition, hence why we can't
> >>> use it as a suspend clocksource. I actually sent out a patch to do that,
> >>> earlier.
> >>>
> >>> And yes, it's entirely possible that v5.1 doesn't work in this regard,
> >>> but we're not noticing that because we don't have suspend/resume support
> >>> for Tegra210 anyway. There are a couple of missing pieces that we need
> >>> in order to make it work.
> >>>
> >>> This change in particular is only going to affect the CPU idle state
> >>> (CC7). Since the architected timer doesn't survive that either, we need
> >>> the Tegra timer to be preferred over the architected timer for normal
> >>> operation.
> >>>
> >>> All of these issues go away on Tegra186 and later, where the architected
> >>> timer is in an always-on partition and has a PLL that remains on during
> >>> SC7 (and CC7).
> >>
> >> Thank you very much for the clarification. But then what about the
> >> sched_clock? I suppose sched_clock will suffer on T210 as well and it's
> >> a bit trickier case because apparently arch-timer always wins since it
> >> has a higher precision. I guess the best solution will be to just bail
> >> out from arch-timer's driver probe in a case of T210.
> >>
> >> if (of_machine_is_compatible("nvidia,tegra210"))
> >> 	return 0.
> > 
> > I don't think there's any issue with the scheduler clock on Tegra210.
> > Before the CPU can be turned off, all tasks scheduled on that CPU have
> > to be migrated to another CPU, right? Conversely, before any tasks can
> > be scheduled on a CPU that CPU needs to be brought online, at which
> > point the architected timer should work fine again.
> 
> Is SC7 a CPU-idle state that cpuidle driver may enter or it's a
> system-wide suspend state? It's still not clear to me.

SC7 is the system-wide suspend state. When entered pretty much
everything in the SoC is turned off. The CPU idle state is called CC7,
but I think we only expose that via PSCI. However, I'm not sure we have
any extensive testing for that. I'll have to check that.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5067 bytes --]

On Fri, Jun 14, 2019 at 05:06:48PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 17:02, Dmitry Osipenko пишет:
> > 14.06.2019 16:53, Thierry Reding пишет:
> >> On Fri, Jun 14, 2019 at 04:29:17PM +0300, Dmitry Osipenko wrote:
> >>> 14.06.2019 16:22, Thierry Reding пишет:
> >>>> On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
> >>>>> 14.06.2019 13:47, Thierry Reding пишет:
> >>>>>> From: Thierry Reding <treding@nvidia.com>
> >>>>>>
> >>>>>> The rating is parameterized depending on SoC generation to make sure it
> >>>>>> takes precedence on implementations where the architected timer can't be
> >>>>>> used. This rating is already used for the clock event device. Use the
> >>>>>> same rating for the clock source to be consistent.
> >>>>>>
> >>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>>>>> ---
> >>>>>>  drivers/clocksource/timer-tegra.c | 2 +-
> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> >>>>>> index f6a8eb0d7322..e6608141cccb 100644
> >>>>>> --- a/drivers/clocksource/timer-tegra.c
> >>>>>> +++ b/drivers/clocksource/timer-tegra.c
> >>>>>> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
> >>>>>>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
> >>>>>>  
> >>>>>>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
> >>>>>> -				    "timer_us", TIMER_1MHz, 300, 32,
> >>>>>> +				    "timer_us", TIMER_1MHz, rating, 32,
> >>>>>>  				    clocksource_mmio_readl_up);
> >>>>>>  	if (ret)
> >>>>>>  		pr_err("failed to register clocksource: %d\n", ret);
> >>>>>>
> >>>>>
> >>>>> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
> >>>>> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
> >>>>> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
> >>>>> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
> >>>>> of that already.
> >>>>
> >>>> Yes, the architected timer doesn't work across an SC7 (which is what the
> >>>> deepest idle state is called on Tegra210) transition, hence why we can't
> >>>> use it as a suspend clocksource. I actually sent out a patch to do that,
> >>>> earlier.
> >>>>
> >>>> And yes, it's entirely possible that v5.1 doesn't work in this regard,
> >>>> but we're not noticing that because we don't have suspend/resume support
> >>>> for Tegra210 anyway. There are a couple of missing pieces that we need
> >>>> in order to make it work.
> >>>>
> >>>> This change in particular is only going to affect the CPU idle state
> >>>> (CC7). Since the architected timer doesn't survive that either, we need
> >>>> the Tegra timer to be preferred over the architected timer for normal
> >>>> operation.
> >>>>
> >>>> All of these issues go away on Tegra186 and later, where the architected
> >>>> timer is in an always-on partition and has a PLL that remains on during
> >>>> SC7 (and CC7).
> >>>
> >>> Thank you very much for the clarification. But then what about the
> >>> sched_clock? I suppose sched_clock will suffer on T210 as well and it's
> >>> a bit trickier case because apparently arch-timer always wins since it
> >>> has a higher precision. I guess the best solution will be to just bail
> >>> out from arch-timer's driver probe in a case of T210.
> >>>
> >>> if (of_machine_is_compatible("nvidia,tegra210"))
> >>> 	return 0.
> >>
> >> I don't think there's any issue with the scheduler clock on Tegra210.
> >> Before the CPU can be turned off, all tasks scheduled on that CPU have
> >> to be migrated to another CPU, right? Conversely, before any tasks can
> >> be scheduled on a CPU that CPU needs to be brought online, at which
> >> point the architected timer should work fine again.
> > 
> > Is SC7 a CPU-idle state that cpuidle driver may enter or it's a
> > system-wide suspend state? It's still not clear to me.
> > 
> 
> Ah, looks like I see now. So CC7 (CPU idle state) also affects the
> arch-timer (like SC7) and hence scheduler clock will be stopped while it
> shouldn't, which doesn't sound good.

We enable CC7 on Jetson TX1 and I've just verified on Jetson Nano that
there are no issues if CC7 is enabled. From the boot log I can see that
the architected timer is still used as scheduler clock.

So that either means that the scheduler doesn't mind if the clock is
disabled when a CPU is asleep or it means that CC7 does not impact the
architected timer. I thought we had already confirmed that the latter
isn't true, i.e. that the architected timer is disabled during CC7, so
that would mean that indeed the scheduler continues to work fine if the
clock is off during sleep. I also don't understand why it would break,
given that it's only put to sleep when there are no longer any tasks
running on it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Dmitry Osipenko]

14.06.2019 17:14, Thierry Reding пишет:
> On Fri, Jun 14, 2019 at 04:49:44PM +0300, Dmitry Osipenko wrote:
>> 14.06.2019 16:41, Thierry Reding пишет:
>>> On Fri, Jun 14, 2019 at 03:01:13PM +0300, Dmitry Osipenko wrote:
>>>> 14.06.2019 13:47, Thierry Reding пишет:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> The suspend clock source for Tegra210 and earlier is currently
>>>>> implemented in the Tegra timer driver. However, the suspend clock source
>>>>> code accesses registers that are part of the RTC hardware block, so both
>>>>> can step on each others' toes. In practice this isn't an issue, but
>>>>> there is no reason why the RTC driver can't implement the clock source,
>>>>> so move the code over to the tegra-rtc driver.
>>>>>
>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>> ---
>>>>>  drivers/clocksource/timer-tegra.c | 44 -------------------------------
>>>>>  drivers/rtc/rtc-tegra.c           | 42 +++++++++++++++++++++++++++++
>>>>>  2 files changed, 42 insertions(+), 44 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>>>>> index e6608141cccb..87eac618924d 100644
>>>>> --- a/drivers/clocksource/timer-tegra.c
>>>>> +++ b/drivers/clocksource/timer-tegra.c
>>>>> @@ -21,10 +21,6 @@
>>>>>  
>>>>>  #include "timer-of.h"
>>>>>  
>>>>> -#define RTC_SECONDS		0x08
>>>>> -#define RTC_SHADOW_SECONDS	0x0c
>>>>> -#define RTC_MILLISECONDS	0x10
>>>>> -
>>>>>  #define TIMERUS_CNTR_1US	0x10
>>>>>  #define TIMERUS_USEC_CFG	0x14
>>>>>  #define TIMERUS_CNTR_FREEZE	0x4c
>>>>> @@ -164,34 +160,6 @@ static struct delay_timer tegra_delay_timer = {
>>>>>  };
>>>>>  #endif
>>>>>  
>>>>> -static struct timer_of suspend_rtc_to = {
>>>>> -	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
>>>>> -};
>>>>> -
>>>>> -/*
>>>>> - * tegra_rtc_read - Reads the Tegra RTC registers
>>>>> - * Care must be taken that this function is not called while the
>>>>> - * tegra_rtc driver could be executing to avoid race conditions
>>>>> - * on the RTC shadow register
>>>>> - */
>>>>> -static u64 tegra_rtc_read_ms(struct clocksource *cs)
>>>>> -{
>>>>> -	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
>>>>> -
>>>>> -	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
>>>>> -	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
>>>>> -
>>>>> -	return (u64)s * MSEC_PER_SEC + ms;
>>>>> -}
>>>>> -
>>>>> -static struct clocksource suspend_rtc_clocksource = {
>>>>> -	.name	= "tegra_suspend_timer",
>>>>> -	.rating	= 200,
>>>>> -	.read	= tegra_rtc_read_ms,
>>>>> -	.mask	= CLOCKSOURCE_MASK(32),
>>>>> -	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
>>>>> -};
>>>>> -
>>>>>  static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
>>>>>  {
>>>>>  	if (tegra20) {
>>>>> @@ -385,15 +353,3 @@ static int __init tegra20_init_timer(struct device_node *np)
>>>>>  	return tegra_init_timer(np, true, rating);
>>>>>  }
>>>>>  TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
>>>>> -
>>>>> -static int __init tegra20_init_rtc(struct device_node *np)
>>>>> -{
>>>>> -	int ret;
>>>>> -
>>>>> -	ret = timer_of_init(np, &suspend_rtc_to);
>>>>> -	if (ret)
>>>>> -		return ret;
>>>>> -
>>>>> -	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
>>>>> -}
>>>>> -TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
>>>>> diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
>>>>> index 8fa1b3febf69..6da54264a27a 100644
>>>>> --- a/drivers/rtc/rtc-tegra.c
>>>>> +++ b/drivers/rtc/rtc-tegra.c
>>>>> @@ -6,6 +6,7 @@
>>>>>   */
>>>>>  
>>>>>  #include <linux/clk.h>
>>>>> +#include <linux/clocksource.h>
>>>>>  #include <linux/delay.h>
>>>>>  #include <linux/init.h>
>>>>>  #include <linux/io.h>
>>>>> @@ -52,8 +53,15 @@ struct tegra_rtc_info {
>>>>>  	struct clk *clk;
>>>>>  	int irq; /* alarm and periodic IRQ */
>>>>>  	spinlock_t lock;
>>>>> +
>>>>> +	struct clocksource clksrc;
>>>>>  };
>>>>>  
>>>>> +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
>>>>> +{
>>>>> +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * RTC hardware is busy when it is updating its values over AHB once every
>>>>>   * eight 32 kHz clocks (~250 us). Outside of these updates the CPU is free to
>>>>> @@ -268,6 +276,17 @@ static const struct rtc_class_ops tegra_rtc_ops = {
>>>>>  	.alarm_irq_enable = tegra_rtc_alarm_irq_enable,
>>>>>  };
>>>>>  
>>>>> +static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
>>>>> +{
>>>>> +	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
>>>>> +	u32 ms, s;
>>>>> +
>>>>> +	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
>>>>> +	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
>>>>> +
>>>>> +	return (u64)s * MSEC_PER_SEC + ms;
>>>>> +}
>>>>> +
>>>>>  static const struct of_device_id tegra_rtc_dt_match[] = {
>>>>>  	{ .compatible = "nvidia,tegra20-rtc", },
>>>>>  	{}
>>>>> @@ -339,6 +358,28 @@ static int tegra_rtc_probe(struct platform_device *pdev)
>>>>>  		goto disable_clk;
>>>>>  	}
>>>>>  
>>>>> +	/*
>>>>> +	 * The Tegra RTC is the only reliable clock source that persists
>>>>> +	 * across an SC7 transition (VDD_CPU and VDD_CORE off) on Tegra210
>>>>> +	 * and earlier. Starting with Tegra186, the ARM v8 architected timer
>>>>> +	 * is in an always on power partition and its reference clock keeps
>>>>> +	 * running during SC7. Therefore, we technically don't need to have
>>>>> +	 * the RTC register as a clock source on Tegra186 and later, but it
>>>>> +	 * doesn't hurt either, so we just register it unconditionally here.
>>>>> +	 */
>>>>> +	info->clksrc.name = "tegra_rtc";
>>>>> +	info->clksrc.rating = 200;
>>>>> +	info->clksrc.read = tegra_rtc_read_ms;
>>>>> +	info->clksrc.mask = CLOCKSOURCE_MASK(32);
>>>>
>>>> Hm.. shouldn't this be CLOCKSOURCE_MASK(52)? Given that there are 32 bits for seconds and
>>>> 10bits for milliseconds.
>>>
>>> Did you mean to say CLOCKSOURCE_MASK(42)? Yeah, that's probably better
>>> here.
>>
>> Yes, 42 :)
> 
> I'm wondering if that could perhaps be a little problematic because
> we're not actually using all of the 10 bits for the milliseconds. So the
> maximum value that we can return is:
> 
> 	4294967296 * 1000 + 999 = 4294967296999
> 
> However, the maximum value for a 42 bit mask is:
> 
> 	2^42 - 1 = 4398046511103
> 
> That mask is only used in order to wrap around in delta computations. So
> I can imagine a situation where we'd end up with a wrong value in the
> delta. I suppose this can only really happen if the two samples are very
> far apart in time, so maybe this isn't worth worrying about.

I'm a bit puzzled now. Looks problematic that wraparound will happen unexpectedly for
the timekeeping. Although please bare in mind that I'm not expert in the area of
timekeeping, actually I know very little about it.

Maybe tegra_rtc_read_ms() should track wraparound case itself and return a
monotonically incrementing value?

static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
{
	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
	u64 ms, s, now;

	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);

	now = s * MSEC_PER_SEC + ms;

	if (now < info->last)
		info->ms_cnt += 0x3e8000003e8ull - info->last + now;
	else
		info->ms_cnt += now - info->last;

	info->last = now;

	return info->ms_cnt;
}

and then simply CLOCKSOURCE_MASK(64).

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Dmitry Osipenko]

14.06.2019 18:37, Thierry Reding пишет:
> On Fri, Jun 14, 2019 at 05:06:48PM +0300, Dmitry Osipenko wrote:
>> 14.06.2019 17:02, Dmitry Osipenko пишет:
>>> 14.06.2019 16:53, Thierry Reding пишет:
>>>> On Fri, Jun 14, 2019 at 04:29:17PM +0300, Dmitry Osipenko wrote:
>>>>> 14.06.2019 16:22, Thierry Reding пишет:
>>>>>> On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
>>>>>>> 14.06.2019 13:47, Thierry Reding пишет:
>>>>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>>>>
>>>>>>>> The rating is parameterized depending on SoC generation to make sure it
>>>>>>>> takes precedence on implementations where the architected timer can't be
>>>>>>>> used. This rating is already used for the clock event device. Use the
>>>>>>>> same rating for the clock source to be consistent.
>>>>>>>>
>>>>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>>>>> ---
>>>>>>>>  drivers/clocksource/timer-tegra.c | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>>>>>>>> index f6a8eb0d7322..e6608141cccb 100644
>>>>>>>> --- a/drivers/clocksource/timer-tegra.c
>>>>>>>> +++ b/drivers/clocksource/timer-tegra.c
>>>>>>>> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>>>>>>>>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
>>>>>>>>  
>>>>>>>>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
>>>>>>>> -				    "timer_us", TIMER_1MHz, 300, 32,
>>>>>>>> +				    "timer_us", TIMER_1MHz, rating, 32,
>>>>>>>>  				    clocksource_mmio_readl_up);
>>>>>>>>  	if (ret)
>>>>>>>>  		pr_err("failed to register clocksource: %d\n", ret);
>>>>>>>>
>>>>>>>
>>>>>>> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
>>>>>>> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
>>>>>>> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
>>>>>>> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
>>>>>>> of that already.
>>>>>>
>>>>>> Yes, the architected timer doesn't work across an SC7 (which is what the
>>>>>> deepest idle state is called on Tegra210) transition, hence why we can't
>>>>>> use it as a suspend clocksource. I actually sent out a patch to do that,
>>>>>> earlier.
>>>>>>
>>>>>> And yes, it's entirely possible that v5.1 doesn't work in this regard,
>>>>>> but we're not noticing that because we don't have suspend/resume support
>>>>>> for Tegra210 anyway. There are a couple of missing pieces that we need
>>>>>> in order to make it work.
>>>>>>
>>>>>> This change in particular is only going to affect the CPU idle state
>>>>>> (CC7). Since the architected timer doesn't survive that either, we need
>>>>>> the Tegra timer to be preferred over the architected timer for normal
>>>>>> operation.
>>>>>>
>>>>>> All of these issues go away on Tegra186 and later, where the architected
>>>>>> timer is in an always-on partition and has a PLL that remains on during
>>>>>> SC7 (and CC7).
>>>>>
>>>>> Thank you very much for the clarification. But then what about the
>>>>> sched_clock? I suppose sched_clock will suffer on T210 as well and it's
>>>>> a bit trickier case because apparently arch-timer always wins since it
>>>>> has a higher precision. I guess the best solution will be to just bail
>>>>> out from arch-timer's driver probe in a case of T210.
>>>>>
>>>>> if (of_machine_is_compatible("nvidia,tegra210"))
>>>>> 	return 0.
>>>>
>>>> I don't think there's any issue with the scheduler clock on Tegra210.
>>>> Before the CPU can be turned off, all tasks scheduled on that CPU have
>>>> to be migrated to another CPU, right? Conversely, before any tasks can
>>>> be scheduled on a CPU that CPU needs to be brought online, at which
>>>> point the architected timer should work fine again.
>>>
>>> Is SC7 a CPU-idle state that cpuidle driver may enter or it's a
>>> system-wide suspend state? It's still not clear to me.
>>>
>>
>> Ah, looks like I see now. So CC7 (CPU idle state) also affects the
>> arch-timer (like SC7) and hence scheduler clock will be stopped while it
>> shouldn't, which doesn't sound good.
> 
> We enable CC7 on Jetson TX1 and I've just verified on Jetson Nano that
> there are no issues if CC7 is enabled. From the boot log I can see that
> the architected timer is still used as scheduler clock.
> 
> So that either means that the scheduler doesn't mind if the clock is
> disabled when a CPU is asleep or it means that CC7 does not impact the
> architected timer. I thought we had already confirmed that the latter
> isn't true, i.e. that the architected timer is disabled during CC7, so
> that would mean that indeed the scheduler continues to work fine if the
> clock is off during sleep. I also don't understand why it would break,
> given that it's only put to sleep when there are no longer any tasks
> running on it.

CPU may enter idling state while task is sleeping, i.e. waiting for some event. To be
honest, I don't know much about how scheduling actually works in the kernel and what
are the actual purposes of scheduler clock. Maybe Daniel could clarify it all for us?

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 9349 bytes --]

On Fri, Jun 14, 2019 at 07:41:54PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 17:14, Thierry Reding пишет:
> > On Fri, Jun 14, 2019 at 04:49:44PM +0300, Dmitry Osipenko wrote:
> >> 14.06.2019 16:41, Thierry Reding пишет:
> >>> On Fri, Jun 14, 2019 at 03:01:13PM +0300, Dmitry Osipenko wrote:
> >>>> 14.06.2019 13:47, Thierry Reding пишет:
> >>>>> From: Thierry Reding <treding@nvidia.com>
> >>>>>
> >>>>> The suspend clock source for Tegra210 and earlier is currently
> >>>>> implemented in the Tegra timer driver. However, the suspend clock source
> >>>>> code accesses registers that are part of the RTC hardware block, so both
> >>>>> can step on each others' toes. In practice this isn't an issue, but
> >>>>> there is no reason why the RTC driver can't implement the clock source,
> >>>>> so move the code over to the tegra-rtc driver.
> >>>>>
> >>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>>>> ---
> >>>>>  drivers/clocksource/timer-tegra.c | 44 -------------------------------
> >>>>>  drivers/rtc/rtc-tegra.c           | 42 +++++++++++++++++++++++++++++
> >>>>>  2 files changed, 42 insertions(+), 44 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> >>>>> index e6608141cccb..87eac618924d 100644
> >>>>> --- a/drivers/clocksource/timer-tegra.c
> >>>>> +++ b/drivers/clocksource/timer-tegra.c
> >>>>> @@ -21,10 +21,6 @@
> >>>>>  
> >>>>>  #include "timer-of.h"
> >>>>>  
> >>>>> -#define RTC_SECONDS		0x08
> >>>>> -#define RTC_SHADOW_SECONDS	0x0c
> >>>>> -#define RTC_MILLISECONDS	0x10
> >>>>> -
> >>>>>  #define TIMERUS_CNTR_1US	0x10
> >>>>>  #define TIMERUS_USEC_CFG	0x14
> >>>>>  #define TIMERUS_CNTR_FREEZE	0x4c
> >>>>> @@ -164,34 +160,6 @@ static struct delay_timer tegra_delay_timer = {
> >>>>>  };
> >>>>>  #endif
> >>>>>  
> >>>>> -static struct timer_of suspend_rtc_to = {
> >>>>> -	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
> >>>>> -};
> >>>>> -
> >>>>> -/*
> >>>>> - * tegra_rtc_read - Reads the Tegra RTC registers
> >>>>> - * Care must be taken that this function is not called while the
> >>>>> - * tegra_rtc driver could be executing to avoid race conditions
> >>>>> - * on the RTC shadow register
> >>>>> - */
> >>>>> -static u64 tegra_rtc_read_ms(struct clocksource *cs)
> >>>>> -{
> >>>>> -	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
> >>>>> -
> >>>>> -	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
> >>>>> -	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
> >>>>> -
> >>>>> -	return (u64)s * MSEC_PER_SEC + ms;
> >>>>> -}
> >>>>> -
> >>>>> -static struct clocksource suspend_rtc_clocksource = {
> >>>>> -	.name	= "tegra_suspend_timer",
> >>>>> -	.rating	= 200,
> >>>>> -	.read	= tegra_rtc_read_ms,
> >>>>> -	.mask	= CLOCKSOURCE_MASK(32),
> >>>>> -	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
> >>>>> -};
> >>>>> -
> >>>>>  static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
> >>>>>  {
> >>>>>  	if (tegra20) {
> >>>>> @@ -385,15 +353,3 @@ static int __init tegra20_init_timer(struct device_node *np)
> >>>>>  	return tegra_init_timer(np, true, rating);
> >>>>>  }
> >>>>>  TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
> >>>>> -
> >>>>> -static int __init tegra20_init_rtc(struct device_node *np)
> >>>>> -{
> >>>>> -	int ret;
> >>>>> -
> >>>>> -	ret = timer_of_init(np, &suspend_rtc_to);
> >>>>> -	if (ret)
> >>>>> -		return ret;
> >>>>> -
> >>>>> -	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
> >>>>> -}
> >>>>> -TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
> >>>>> diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
> >>>>> index 8fa1b3febf69..6da54264a27a 100644
> >>>>> --- a/drivers/rtc/rtc-tegra.c
> >>>>> +++ b/drivers/rtc/rtc-tegra.c
> >>>>> @@ -6,6 +6,7 @@
> >>>>>   */
> >>>>>  
> >>>>>  #include <linux/clk.h>
> >>>>> +#include <linux/clocksource.h>
> >>>>>  #include <linux/delay.h>
> >>>>>  #include <linux/init.h>
> >>>>>  #include <linux/io.h>
> >>>>> @@ -52,8 +53,15 @@ struct tegra_rtc_info {
> >>>>>  	struct clk *clk;
> >>>>>  	int irq; /* alarm and periodic IRQ */
> >>>>>  	spinlock_t lock;
> >>>>> +
> >>>>> +	struct clocksource clksrc;
> >>>>>  };
> >>>>>  
> >>>>> +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
> >>>>> +{
> >>>>> +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
> >>>>> +}
> >>>>> +
> >>>>>  /*
> >>>>>   * RTC hardware is busy when it is updating its values over AHB once every
> >>>>>   * eight 32 kHz clocks (~250 us). Outside of these updates the CPU is free to
> >>>>> @@ -268,6 +276,17 @@ static const struct rtc_class_ops tegra_rtc_ops = {
> >>>>>  	.alarm_irq_enable = tegra_rtc_alarm_irq_enable,
> >>>>>  };
> >>>>>  
> >>>>> +static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
> >>>>> +{
> >>>>> +	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
> >>>>> +	u32 ms, s;
> >>>>> +
> >>>>> +	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
> >>>>> +	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
> >>>>> +
> >>>>> +	return (u64)s * MSEC_PER_SEC + ms;
> >>>>> +}
> >>>>> +
> >>>>>  static const struct of_device_id tegra_rtc_dt_match[] = {
> >>>>>  	{ .compatible = "nvidia,tegra20-rtc", },
> >>>>>  	{}
> >>>>> @@ -339,6 +358,28 @@ static int tegra_rtc_probe(struct platform_device *pdev)
> >>>>>  		goto disable_clk;
> >>>>>  	}
> >>>>>  
> >>>>> +	/*
> >>>>> +	 * The Tegra RTC is the only reliable clock source that persists
> >>>>> +	 * across an SC7 transition (VDD_CPU and VDD_CORE off) on Tegra210
> >>>>> +	 * and earlier. Starting with Tegra186, the ARM v8 architected timer
> >>>>> +	 * is in an always on power partition and its reference clock keeps
> >>>>> +	 * running during SC7. Therefore, we technically don't need to have
> >>>>> +	 * the RTC register as a clock source on Tegra186 and later, but it
> >>>>> +	 * doesn't hurt either, so we just register it unconditionally here.
> >>>>> +	 */
> >>>>> +	info->clksrc.name = "tegra_rtc";
> >>>>> +	info->clksrc.rating = 200;
> >>>>> +	info->clksrc.read = tegra_rtc_read_ms;
> >>>>> +	info->clksrc.mask = CLOCKSOURCE_MASK(32);
> >>>>
> >>>> Hm.. shouldn't this be CLOCKSOURCE_MASK(52)? Given that there are 32 bits for seconds and
> >>>> 10bits for milliseconds.
> >>>
> >>> Did you mean to say CLOCKSOURCE_MASK(42)? Yeah, that's probably better
> >>> here.
> >>
> >> Yes, 42 :)
> > 
> > I'm wondering if that could perhaps be a little problematic because
> > we're not actually using all of the 10 bits for the milliseconds. So the
> > maximum value that we can return is:
> > 
> > 	4294967296 * 1000 + 999 = 4294967296999
> > 
> > However, the maximum value for a 42 bit mask is:
> > 
> > 	2^42 - 1 = 4398046511103
> > 
> > That mask is only used in order to wrap around in delta computations. So
> > I can imagine a situation where we'd end up with a wrong value in the
> > delta. I suppose this can only really happen if the two samples are very
> > far apart in time, so maybe this isn't worth worrying about.
> 
> I'm a bit puzzled now. Looks problematic that wraparound will happen unexpectedly for
> the timekeeping. Although please bare in mind that I'm not expert in the area of
> timekeeping, actually I know very little about it.
> 
> Maybe tegra_rtc_read_ms() should track wraparound case itself and return a
> monotonically incrementing value?
> 
> static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
> {
> 	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
> 	u64 ms, s, now;
> 
> 	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
> 	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
> 
> 	now = s * MSEC_PER_SEC + ms;
> 
> 	if (now < info->last)
> 		info->ms_cnt += 0x3e8000003e8ull - info->last + now;
> 	else
> 		info->ms_cnt += now - info->last;
> 
> 	info->last = now;
> 
> 	return info->ms_cnt;
> }
> 
> and then simply CLOCKSOURCE_MASK(64).

This sounds like a bit of overkill to me. There's an obscure constant in
there that's completely non-obvious and we're duplicating code that's
effectively already in the core. I think the only thing that we need to
make sure is that we don't confuse the timekeeping system by having this
discrepancy of 24 values between the values that we report and the mask.

I think we could probably do this by setting a mask of 41 bits and then
also applying that mask to the values that we return:

	return (u64)(s * MSEC_PER_SEC + ms) & clksrc->mask;

That way we always wrap around before we reach the maximum numerical
value that we could. So we "loose" that 24 value range, but that's not
really a problem.

Actually, saying it that way I don't think we actually have to worry
about this at all. Those 24 values (0x3E8-0x3FF) are already lost. We
never return any values in that range because the MILLI_SECONDS register
wraps around to 0x000 after 0x3E7. So we will never end up with any of
those 24 values being used. They are effectively already lost. The fact
that we "hide" them by contracting the 10 bit field to 1000 values does
not change that.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6041 bytes --]

On Fri, Jun 14, 2019 at 08:00:09PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 18:37, Thierry Reding пишет:
> > On Fri, Jun 14, 2019 at 05:06:48PM +0300, Dmitry Osipenko wrote:
> >> 14.06.2019 17:02, Dmitry Osipenko пишет:
> >>> 14.06.2019 16:53, Thierry Reding пишет:
> >>>> On Fri, Jun 14, 2019 at 04:29:17PM +0300, Dmitry Osipenko wrote:
> >>>>> 14.06.2019 16:22, Thierry Reding пишет:
> >>>>>> On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
> >>>>>>> 14.06.2019 13:47, Thierry Reding пишет:
> >>>>>>>> From: Thierry Reding <treding@nvidia.com>
> >>>>>>>>
> >>>>>>>> The rating is parameterized depending on SoC generation to make sure it
> >>>>>>>> takes precedence on implementations where the architected timer can't be
> >>>>>>>> used. This rating is already used for the clock event device. Use the
> >>>>>>>> same rating for the clock source to be consistent.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/clocksource/timer-tegra.c | 2 +-
> >>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> >>>>>>>> index f6a8eb0d7322..e6608141cccb 100644
> >>>>>>>> --- a/drivers/clocksource/timer-tegra.c
> >>>>>>>> +++ b/drivers/clocksource/timer-tegra.c
> >>>>>>>> @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
> >>>>>>>>  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
> >>>>>>>>  
> >>>>>>>>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
> >>>>>>>> -				    "timer_us", TIMER_1MHz, 300, 32,
> >>>>>>>> +				    "timer_us", TIMER_1MHz, rating, 32,
> >>>>>>>>  				    clocksource_mmio_readl_up);
> >>>>>>>>  	if (ret)
> >>>>>>>>  		pr_err("failed to register clocksource: %d\n", ret);
> >>>>>>>>
> >>>>>>>
> >>>>>>> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
> >>>>>>> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
> >>>>>>> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
> >>>>>>> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
> >>>>>>> of that already.
> >>>>>>
> >>>>>> Yes, the architected timer doesn't work across an SC7 (which is what the
> >>>>>> deepest idle state is called on Tegra210) transition, hence why we can't
> >>>>>> use it as a suspend clocksource. I actually sent out a patch to do that,
> >>>>>> earlier.
> >>>>>>
> >>>>>> And yes, it's entirely possible that v5.1 doesn't work in this regard,
> >>>>>> but we're not noticing that because we don't have suspend/resume support
> >>>>>> for Tegra210 anyway. There are a couple of missing pieces that we need
> >>>>>> in order to make it work.
> >>>>>>
> >>>>>> This change in particular is only going to affect the CPU idle state
> >>>>>> (CC7). Since the architected timer doesn't survive that either, we need
> >>>>>> the Tegra timer to be preferred over the architected timer for normal
> >>>>>> operation.
> >>>>>>
> >>>>>> All of these issues go away on Tegra186 and later, where the architected
> >>>>>> timer is in an always-on partition and has a PLL that remains on during
> >>>>>> SC7 (and CC7).
> >>>>>
> >>>>> Thank you very much for the clarification. But then what about the
> >>>>> sched_clock? I suppose sched_clock will suffer on T210 as well and it's
> >>>>> a bit trickier case because apparently arch-timer always wins since it
> >>>>> has a higher precision. I guess the best solution will be to just bail
> >>>>> out from arch-timer's driver probe in a case of T210.
> >>>>>
> >>>>> if (of_machine_is_compatible("nvidia,tegra210"))
> >>>>> 	return 0.
> >>>>
> >>>> I don't think there's any issue with the scheduler clock on Tegra210.
> >>>> Before the CPU can be turned off, all tasks scheduled on that CPU have
> >>>> to be migrated to another CPU, right? Conversely, before any tasks can
> >>>> be scheduled on a CPU that CPU needs to be brought online, at which
> >>>> point the architected timer should work fine again.
> >>>
> >>> Is SC7 a CPU-idle state that cpuidle driver may enter or it's a
> >>> system-wide suspend state? It's still not clear to me.
> >>>
> >>
> >> Ah, looks like I see now. So CC7 (CPU idle state) also affects the
> >> arch-timer (like SC7) and hence scheduler clock will be stopped while it
> >> shouldn't, which doesn't sound good.
> > 
> > We enable CC7 on Jetson TX1 and I've just verified on Jetson Nano that
> > there are no issues if CC7 is enabled. From the boot log I can see that
> > the architected timer is still used as scheduler clock.
> > 
> > So that either means that the scheduler doesn't mind if the clock is
> > disabled when a CPU is asleep or it means that CC7 does not impact the
> > architected timer. I thought we had already confirmed that the latter
> > isn't true, i.e. that the architected timer is disabled during CC7, so
> > that would mean that indeed the scheduler continues to work fine if the
> > clock is off during sleep. I also don't understand why it would break,
> > given that it's only put to sleep when there are no longer any tasks
> > running on it.
> 
> CPU may enter idling state while task is sleeping, i.e. waiting for some event. To be
> honest, I don't know much about how scheduling actually works in the kernel and what
> are the actual purposes of scheduler clock. Maybe Daniel could clarify it all for us?

My understanding is that a task that goes to sleep can be migrated to
another CPU, so there's got to be code in place already to account for
that. Migrating from one CPU to another is not very different from
resuming a task on the same CPU.

But yeah, I'm not very firm on the fundamentals here either, so a bit of
clarification from Daniel or Thomas would help.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] rtc: tegra: Implement suspend clock source

[Dmitry Osipenko]

17.06.2019 13:20, Thierry Reding пишет:
> On Fri, Jun 14, 2019 at 07:41:54PM +0300, Dmitry Osipenko wrote:
>> 14.06.2019 17:14, Thierry Reding пишет:
>>> On Fri, Jun 14, 2019 at 04:49:44PM +0300, Dmitry Osipenko wrote:
>>>> 14.06.2019 16:41, Thierry Reding пишет:
>>>>> On Fri, Jun 14, 2019 at 03:01:13PM +0300, Dmitry Osipenko wrote:
>>>>>> 14.06.2019 13:47, Thierry Reding пишет:
>>>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>>>
>>>>>>> The suspend clock source for Tegra210 and earlier is currently
>>>>>>> implemented in the Tegra timer driver. However, the suspend clock source
>>>>>>> code accesses registers that are part of the RTC hardware block, so both
>>>>>>> can step on each others' toes. In practice this isn't an issue, but
>>>>>>> there is no reason why the RTC driver can't implement the clock source,
>>>>>>> so move the code over to the tegra-rtc driver.
>>>>>>>
>>>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>>>> ---
>>>>>>>  drivers/clocksource/timer-tegra.c | 44 -------------------------------
>>>>>>>  drivers/rtc/rtc-tegra.c           | 42 +++++++++++++++++++++++++++++
>>>>>>>  2 files changed, 42 insertions(+), 44 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>>>>>>> index e6608141cccb..87eac618924d 100644
>>>>>>> --- a/drivers/clocksource/timer-tegra.c
>>>>>>> +++ b/drivers/clocksource/timer-tegra.c
>>>>>>> @@ -21,10 +21,6 @@
>>>>>>>  
>>>>>>>  #include "timer-of.h"
>>>>>>>  
>>>>>>> -#define RTC_SECONDS		0x08
>>>>>>> -#define RTC_SHADOW_SECONDS	0x0c
>>>>>>> -#define RTC_MILLISECONDS	0x10
>>>>>>> -
>>>>>>>  #define TIMERUS_CNTR_1US	0x10
>>>>>>>  #define TIMERUS_USEC_CFG	0x14
>>>>>>>  #define TIMERUS_CNTR_FREEZE	0x4c
>>>>>>> @@ -164,34 +160,6 @@ static struct delay_timer tegra_delay_timer = {
>>>>>>>  };
>>>>>>>  #endif
>>>>>>>  
>>>>>>> -static struct timer_of suspend_rtc_to = {
>>>>>>> -	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
>>>>>>> -};
>>>>>>> -
>>>>>>> -/*
>>>>>>> - * tegra_rtc_read - Reads the Tegra RTC registers
>>>>>>> - * Care must be taken that this function is not called while the
>>>>>>> - * tegra_rtc driver could be executing to avoid race conditions
>>>>>>> - * on the RTC shadow register
>>>>>>> - */
>>>>>>> -static u64 tegra_rtc_read_ms(struct clocksource *cs)
>>>>>>> -{
>>>>>>> -	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
>>>>>>> -
>>>>>>> -	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
>>>>>>> -	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
>>>>>>> -
>>>>>>> -	return (u64)s * MSEC_PER_SEC + ms;
>>>>>>> -}
>>>>>>> -
>>>>>>> -static struct clocksource suspend_rtc_clocksource = {
>>>>>>> -	.name	= "tegra_suspend_timer",
>>>>>>> -	.rating	= 200,
>>>>>>> -	.read	= tegra_rtc_read_ms,
>>>>>>> -	.mask	= CLOCKSOURCE_MASK(32),
>>>>>>> -	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
>>>>>>> -};
>>>>>>> -
>>>>>>>  static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
>>>>>>>  {
>>>>>>>  	if (tegra20) {
>>>>>>> @@ -385,15 +353,3 @@ static int __init tegra20_init_timer(struct device_node *np)
>>>>>>>  	return tegra_init_timer(np, true, rating);
>>>>>>>  }
>>>>>>>  TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
>>>>>>> -
>>>>>>> -static int __init tegra20_init_rtc(struct device_node *np)
>>>>>>> -{
>>>>>>> -	int ret;
>>>>>>> -
>>>>>>> -	ret = timer_of_init(np, &suspend_rtc_to);
>>>>>>> -	if (ret)
>>>>>>> -		return ret;
>>>>>>> -
>>>>>>> -	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
>>>>>>> -}
>>>>>>> -TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
>>>>>>> diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
>>>>>>> index 8fa1b3febf69..6da54264a27a 100644
>>>>>>> --- a/drivers/rtc/rtc-tegra.c
>>>>>>> +++ b/drivers/rtc/rtc-tegra.c
>>>>>>> @@ -6,6 +6,7 @@
>>>>>>>   */
>>>>>>>  
>>>>>>>  #include <linux/clk.h>
>>>>>>> +#include <linux/clocksource.h>
>>>>>>>  #include <linux/delay.h>
>>>>>>>  #include <linux/init.h>
>>>>>>>  #include <linux/io.h>
>>>>>>> @@ -52,8 +53,15 @@ struct tegra_rtc_info {
>>>>>>>  	struct clk *clk;
>>>>>>>  	int irq; /* alarm and periodic IRQ */
>>>>>>>  	spinlock_t lock;
>>>>>>> +
>>>>>>> +	struct clocksource clksrc;
>>>>>>>  };
>>>>>>>  
>>>>>>> +static struct tegra_rtc_info *to_tegra_rtc(struct clocksource *clksrc)
>>>>>>> +{
>>>>>>> +	return container_of(clksrc, struct tegra_rtc_info, clksrc);
>>>>>>> +}
>>>>>>> +
>>>>>>>  /*
>>>>>>>   * RTC hardware is busy when it is updating its values over AHB once every
>>>>>>>   * eight 32 kHz clocks (~250 us). Outside of these updates the CPU is free to
>>>>>>> @@ -268,6 +276,17 @@ static const struct rtc_class_ops tegra_rtc_ops = {
>>>>>>>  	.alarm_irq_enable = tegra_rtc_alarm_irq_enable,
>>>>>>>  };
>>>>>>>  
>>>>>>> +static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
>>>>>>> +{
>>>>>>> +	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
>>>>>>> +	u32 ms, s;
>>>>>>> +
>>>>>>> +	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
>>>>>>> +	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
>>>>>>> +
>>>>>>> +	return (u64)s * MSEC_PER_SEC + ms;
>>>>>>> +}
>>>>>>> +
>>>>>>>  static const struct of_device_id tegra_rtc_dt_match[] = {
>>>>>>>  	{ .compatible = "nvidia,tegra20-rtc", },
>>>>>>>  	{}
>>>>>>> @@ -339,6 +358,28 @@ static int tegra_rtc_probe(struct platform_device *pdev)
>>>>>>>  		goto disable_clk;
>>>>>>>  	}
>>>>>>>  
>>>>>>> +	/*
>>>>>>> +	 * The Tegra RTC is the only reliable clock source that persists
>>>>>>> +	 * across an SC7 transition (VDD_CPU and VDD_CORE off) on Tegra210
>>>>>>> +	 * and earlier. Starting with Tegra186, the ARM v8 architected timer
>>>>>>> +	 * is in an always on power partition and its reference clock keeps
>>>>>>> +	 * running during SC7. Therefore, we technically don't need to have
>>>>>>> +	 * the RTC register as a clock source on Tegra186 and later, but it
>>>>>>> +	 * doesn't hurt either, so we just register it unconditionally here.
>>>>>>> +	 */
>>>>>>> +	info->clksrc.name = "tegra_rtc";
>>>>>>> +	info->clksrc.rating = 200;
>>>>>>> +	info->clksrc.read = tegra_rtc_read_ms;
>>>>>>> +	info->clksrc.mask = CLOCKSOURCE_MASK(32);
>>>>>>
>>>>>> Hm.. shouldn't this be CLOCKSOURCE_MASK(52)? Given that there are 32 bits for seconds and
>>>>>> 10bits for milliseconds.
>>>>>
>>>>> Did you mean to say CLOCKSOURCE_MASK(42)? Yeah, that's probably better
>>>>> here.
>>>>
>>>> Yes, 42 :)
>>>
>>> I'm wondering if that could perhaps be a little problematic because
>>> we're not actually using all of the 10 bits for the milliseconds. So the
>>> maximum value that we can return is:
>>>
>>> 	4294967296 * 1000 + 999 = 4294967296999
>>>
>>> However, the maximum value for a 42 bit mask is:
>>>
>>> 	2^42 - 1 = 4398046511103
>>>
>>> That mask is only used in order to wrap around in delta computations. So
>>> I can imagine a situation where we'd end up with a wrong value in the
>>> delta. I suppose this can only really happen if the two samples are very
>>> far apart in time, so maybe this isn't worth worrying about.
>>
>> I'm a bit puzzled now. Looks problematic that wraparound will happen unexpectedly for
>> the timekeeping. Although please bare in mind that I'm not expert in the area of
>> timekeeping, actually I know very little about it.
>>
>> Maybe tegra_rtc_read_ms() should track wraparound case itself and return a
>> monotonically incrementing value?
>>
>> static u64 tegra_rtc_read_ms(struct clocksource *clksrc)
>> {
>> 	struct tegra_rtc_info *info = to_tegra_rtc(clksrc);
>> 	u64 ms, s, now;
>>
>> 	ms = readl_relaxed(info->base + TEGRA_RTC_REG_MILLI_SECONDS);
>> 	s = readl_relaxed(info->base + TEGRA_RTC_REG_SHADOW_SECONDS);
>>
>> 	now = s * MSEC_PER_SEC + ms;
>>
>> 	if (now < info->last)
>> 		info->ms_cnt += 0x3e8000003e8ull - info->last + now;
>> 	else
>> 		info->ms_cnt += now - info->last;
>>
>> 	info->last = now;
>>
>> 	return info->ms_cnt;
>> }
>>
>> and then simply CLOCKSOURCE_MASK(64).
> 
> This sounds like a bit of overkill to me. There's an obscure constant in
> there that's completely non-obvious and we're duplicating code that's
> effectively already in the core. I think the only thing that we need to
> make sure is that we don't confuse the timekeeping system by having this
> discrepancy of 24 values between the values that we report and the mask.
> 
> I think we could probably do this by setting a mask of 41 bits and then
> also applying that mask to the values that we return:
> 
> 	return (u64)(s * MSEC_PER_SEC + ms) & clksrc->mask;
> 
> That way we always wrap around before we reach the maximum numerical
> value that we could. So we "loose" that 24 value range, but that's not
> really a problem.
> 
> Actually, saying it that way I don't think we actually have to worry
> about this at all. Those 24 values (0x3E8-0x3FF) are already lost. We
> never return any values in that range because the MILLI_SECONDS register
> wraps around to 0x000 after 0x3E7. So we will never end up with any of
> those 24 values being used. They are effectively already lost. The fact
> that we "hide" them by contracting the 10 bit field to 1000 values does
> not change that.

IIUC, clocksource code has assumption that the wraparound happens at a pow2 value,
hence the problem is that the time will go backwards unexpectedly because 0xffffffff *
1000 isn't a pow2 value and masking of 41 bits won't help with that. The mask is only
needed by clocksource to account the pow2 overflow correctly and again, we don't have
the pow2.

Indeed, it looks like a bit of overkill. But it's not a performance-critical code at
all and thus it's better to preserve correctness whenever possible. You could add a
clarifying comment to the obscure constant and to the code in general, explaining why
that all is needed.

Also, please note that the info->lock should be taken within tegra_rtc_read_ms() for
consistency. I'd also rename "m" and "s" variables to "msec" "sec" to be consistent
with the tegra_rtc_read_time().