[1/2] clocksource: tegra: Use rating when registering clock source
diff mbox series

Message ID 20190614104747.19712-1-thierry.reding@gmail.com
State New
Headers show
Series
  • [1/2] clocksource: tegra: Use rating when registering clock source
Related show

Commit Message

Thierry Reding June 14, 2019, 10:47 a.m. UTC
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(-)

Comments

Dmitry Osipenko June 14, 2019, 12:24 p.m. UTC | #1
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.
Thierry Reding June 14, 2019, 1:22 p.m. UTC | #2
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
Dmitry Osipenko June 14, 2019, 1:29 p.m. UTC | #3
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.
Thierry Reding June 14, 2019, 1:53 p.m. UTC | #4
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
Dmitry Osipenko June 14, 2019, 2:02 p.m. UTC | #5
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.
Dmitry Osipenko June 14, 2019, 2:06 p.m. UTC | #6
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.
Thierry Reding June 14, 2019, 2:55 p.m. UTC | #7
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
Thierry Reding June 14, 2019, 3:37 p.m. UTC | #8
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
Dmitry Osipenko June 14, 2019, 5 p.m. UTC | #9
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?
Thierry Reding June 17, 2019, 10:52 a.m. UTC | #10
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

Patch
diff mbox series

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