[PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Jisheng Zhang]

Implement an ARM delay timer to be used for udelay(). This allows us to
skip the delay loop calibration at boot on Marvell BG2, BG2Q, BG2CD
platforms. And after this patch, udelay() will be unaffected by CPU
frequency changes.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/clocksource/Kconfig           | 10 ++++++++++
 drivers/clocksource/dw_apb_timer_of.c | 16 ++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index a7726db..7b081805 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
 	select DW_APB_TIMER
 	select CLKSRC_OF
 
+config DW_APB_TIMER_BASED_DELAY
+	bool "DW APB timer based delay"
+	depends on ARM && DW_APB_TIMER_OF
+	default n
+	help
+	  This option enables support for using the DW APB timer to
+	  implement timer-based delay. It is useful for skiping the
+	  delay loop calibration at boot on some platforms. And the
+	  udelay() will be unaffected by CPU frequency changes.
+
 config ROCKCHIP_TIMER
 	bool
 	select CLKSRC_OF
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index a19a3f6..4bab048 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -16,6 +16,7 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
+#include <linux/delay.h>
 #include <linux/dw_apb_timer.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -130,6 +131,17 @@ static void __init init_sched_clock(void)
 	sched_clock_register(read_sched_clock, 32, sched_rate);
 }
 
+#ifdef CONFIG_DW_APB_TIMER_BASED_DELAY
+static unsigned long dw_apb_delay_timer_read(void)
+{
+	return ~readl_relaxed(sched_io_base);
+}
+
+static struct delay_timer dw_apb_delay_timer = {
+	.read_current_timer = dw_apb_delay_timer_read,
+};
+#endif
+
 static int num_called;
 static void __init dw_apb_timer_init(struct device_node *timer)
 {
@@ -142,6 +154,10 @@ static void __init dw_apb_timer_init(struct device_node *timer)
 		pr_debug("%s: found clocksource timer\n", __func__);
 		add_clocksource(timer);
 		init_sched_clock();
+#ifdef CONFIG_DW_APB_TIMER_BASED_DELAY
+		dw_apb_delay_timer.freq = sched_rate;
+		register_current_timer_delay(&dw_apb_delay_timer);
+#endif
 		break;
 	default:
 		break;
-- 
2.6.2

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Jisheng Zhang]

Dear Daniel,

On Fri, 30 Oct 2015 16:27:39 +0800
Jisheng Zhang <jszhang@marvell.com> wrote:

> Implement an ARM delay timer to be used for udelay(). This allows us to
> skip the delay loop calibration at boot on Marvell BG2, BG2Q, BG2CD
> platforms. And after this patch, udelay() will be unaffected by CPU
> frequency changes.

The commit msg doesn't follow "clocksource/drivers/...: Dnnn.."
But I guess I'll need to post v2, v3 ..., I'll change the msg style in v2.

Thanks,
Jisheng

> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/clocksource/Kconfig           | 10 ++++++++++
>  drivers/clocksource/dw_apb_timer_of.c | 16 ++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index a7726db..7b081805 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
>  	select DW_APB_TIMER
>  	select CLKSRC_OF
>  
> +config DW_APB_TIMER_BASED_DELAY
> +	bool "DW APB timer based delay"
> +	depends on ARM && DW_APB_TIMER_OF
> +	default n
> +	help
> +	  This option enables support for using the DW APB timer to
> +	  implement timer-based delay. It is useful for skiping the
> +	  delay loop calibration at boot on some platforms. And the
> +	  udelay() will be unaffected by CPU frequency changes.
> +
>  config ROCKCHIP_TIMER
>  	bool
>  	select CLKSRC_OF
> diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
> index a19a3f6..4bab048 100644
> --- a/drivers/clocksource/dw_apb_timer_of.c
> +++ b/drivers/clocksource/dw_apb_timer_of.c
> @@ -16,6 +16,7 @@
>   * You should have received a copy of the GNU General Public License
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
> +#include <linux/delay.h>
>  #include <linux/dw_apb_timer.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> @@ -130,6 +131,17 @@ static void __init init_sched_clock(void)
>  	sched_clock_register(read_sched_clock, 32, sched_rate);
>  }
>  
> +#ifdef CONFIG_DW_APB_TIMER_BASED_DELAY
> +static unsigned long dw_apb_delay_timer_read(void)
> +{
> +	return ~readl_relaxed(sched_io_base);
> +}
> +
> +static struct delay_timer dw_apb_delay_timer = {
> +	.read_current_timer = dw_apb_delay_timer_read,
> +};
> +#endif
> +
>  static int num_called;
>  static void __init dw_apb_timer_init(struct device_node *timer)
>  {
> @@ -142,6 +154,10 @@ static void __init dw_apb_timer_init(struct device_node *timer)
>  		pr_debug("%s: found clocksource timer\n", __func__);
>  		add_clocksource(timer);
>  		init_sched_clock();
> +#ifdef CONFIG_DW_APB_TIMER_BASED_DELAY
> +		dw_apb_delay_timer.freq = sched_rate;
> +		register_current_timer_delay(&dw_apb_delay_timer);
> +#endif
>  		break;
>  	default:
>  		break;

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Daniel Lezcano]

On 10/30/2015 09:27 AM, Jisheng Zhang wrote:
> Implement an ARM delay timer to be used for udelay(). This allows us to
> skip the delay loop calibration at boot on Marvell BG2, BG2Q, BG2CD
> platforms. And after this patch, udelay() will be unaffected by CPU
> frequency changes.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/clocksource/Kconfig           | 10 ++++++++++
>   drivers/clocksource/dw_apb_timer_of.c | 16 ++++++++++++++++
>   2 files changed, 26 insertions(+)
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index a7726db..7b081805 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
>   	select DW_APB_TIMER
>   	select CLKSRC_OF
>
> +config DW_APB_TIMER_BASED_DELAY
> +	bool "DW APB timer based delay"
> +	depends on ARM && DW_APB_TIMER_OF
> +	default n
> +	help
> +	  This option enables support for using the DW APB timer to
> +	  implement timer-based delay. It is useful for skiping the
> +	  delay loop calibration at boot on some platforms. And the
> +	  udelay() will be unaffected by CPU frequency changes.
> +

Why do you want it to be optional ?

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Jisheng Zhang]

Dear Daniel,

On Fri, 30 Oct 2015 11:44:46 +0100
Daniel Lezcano <daniel.lezcano@....> wrote:

> On 10/30/2015 09:27 AM, Jisheng Zhang wrote:
> > Implement an ARM delay timer to be used for udelay(). This allows us to
> > skip the delay loop calibration at boot on Marvell BG2, BG2Q, BG2CD
> > platforms. And after this patch, udelay() will be unaffected by CPU
> > frequency changes.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   drivers/clocksource/Kconfig           | 10 ++++++++++
> >   drivers/clocksource/dw_apb_timer_of.c | 16 ++++++++++++++++
> >   2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index a7726db..7b081805 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
> >   	select DW_APB_TIMER
> >   	select CLKSRC_OF
> >
> > +config DW_APB_TIMER_BASED_DELAY
> > +	bool "DW APB timer based delay"
> > +	depends on ARM && DW_APB_TIMER_OF
> > +	default n
> > +	help
> > +	  This option enables support for using the DW APB timer to
> > +	  implement timer-based delay. It is useful for skiping the
> > +	  delay loop calibration at boot on some platforms. And the
> > +	  udelay() will be unaffected by CPU frequency changes.
> > +  
> 
> Why do you want it to be optional ?
> 

Because in some platforms which has arm arch timer, this dw apb timer
delay isn't needed, the arch timer is better. So we want it be optional
so that the platforms which need this feature select it manually when config
the kernel.

Thanks,
Jisheng

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Daniel Lezcano]

On 10/30/2015 12:09 PM, Jisheng Zhang wrote:
> Dear Daniel,
>
> On Fri, 30 Oct 2015 11:44:46 +0100
> Daniel Lezcano <daniel.lezcano@....> wrote:
>
>> On 10/30/2015 09:27 AM, Jisheng Zhang wrote:
>>> Implement an ARM delay timer to be used for udelay(). This allows us to
>>> skip the delay loop calibration at boot on Marvell BG2, BG2Q, BG2CD
>>> platforms. And after this patch, udelay() will be unaffected by CPU
>>> frequency changes.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>> ---
>>>    drivers/clocksource/Kconfig           | 10 ++++++++++
>>>    drivers/clocksource/dw_apb_timer_of.c | 16 ++++++++++++++++
>>>    2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index a7726db..7b081805 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
>>>    	select DW_APB_TIMER
>>>    	select CLKSRC_OF
>>>
>>> +config DW_APB_TIMER_BASED_DELAY
>>> +	bool "DW APB timer based delay"
>>> +	depends on ARM && DW_APB_TIMER_OF
>>> +	default n
>>> +	help
>>> +	  This option enables support for using the DW APB timer to
>>> +	  implement timer-based delay. It is useful for skiping the
>>> +	  delay loop calibration at boot on some platforms. And the
>>> +	  udelay() will be unaffected by CPU frequency changes.
>>> +
>>
>> Why do you want it to be optional ?
>>
>
> Because in some platforms which has arm arch timer, this dw apb timer
> delay isn't needed, the arch timer is better. So we want it be optional
> so that the platforms which need this feature select it manually when config
> the kernel.

Correct me if I am wrong. If you have the arch timer, you don't need the 
dw apb timer at all, no ? So the selection would be arch arm timer *or* 
dw_apb_timer ? not arch_arm_timer for delay and dw_apb_timer for 
clockevents, right ?


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Jisheng Zhang]

Dear Daniel,

On Fri, 30 Oct 2015 13:37:01 +0100
Daniel Lezcano wrote:

> On 10/30/2015 12:09 PM, Jisheng Zhang wrote:
> > Dear Daniel,
> >
> > On Fri, 30 Oct 2015 11:44:46 +0100
> > Daniel Lezcano <daniel.lezcano@....> wrote:
> >  
> >> On 10/30/2015 09:27 AM, Jisheng Zhang wrote:  
> >>> Implement an ARM delay timer to be used for udelay(). This allows us to
> >>> skip the delay loop calibration at boot on Marvell BG2, BG2Q, BG2CD
> >>> platforms. And after this patch, udelay() will be unaffected by CPU
> >>> frequency changes.
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> ---
> >>>    drivers/clocksource/Kconfig           | 10 ++++++++++
> >>>    drivers/clocksource/dw_apb_timer_of.c | 16 ++++++++++++++++
> >>>    2 files changed, 26 insertions(+)
> >>>
> >>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> >>> index a7726db..7b081805 100644
> >>> --- a/drivers/clocksource/Kconfig
> >>> +++ b/drivers/clocksource/Kconfig
> >>> @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
> >>>    	select DW_APB_TIMER
> >>>    	select CLKSRC_OF
> >>>
> >>> +config DW_APB_TIMER_BASED_DELAY
> >>> +	bool "DW APB timer based delay"
> >>> +	depends on ARM && DW_APB_TIMER_OF
> >>> +	default n
> >>> +	help
> >>> +	  This option enables support for using the DW APB timer to
> >>> +	  implement timer-based delay. It is useful for skiping the
> >>> +	  delay loop calibration at boot on some platforms. And the
> >>> +	  udelay() will be unaffected by CPU frequency changes.
> >>> +  
> >>
> >> Why do you want it to be optional ?
> >>  
> >
> > Because in some platforms which has arm arch timer, this dw apb timer
> > delay isn't needed, the arch timer is better. So we want it be optional
> > so that the platforms which need this feature select it manually when config
> > the kernel.  
> 
> Correct me if I am wrong. If you have the arch timer, you don't need the 

Yes, I don't need the dw apb timer if we have arch timer,

> dw apb timer at all, no ? So the selection would be arch arm timer *or* 
> dw_apb_timer ? not arch_arm_timer for delay and dw_apb_timer for 
> clockevents, right ?

Yes, if we have arch timer, I prefer to use it for clockevent and delay.

Could you please provide suggestion how to handle this case?

Thanks,
Jisheng

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Daniel Lezcano]

On 11/02/2015 03:51 AM, Jisheng Zhang wrote:
> Dear Daniel,
>
> On Fri, 30 Oct 2015 13:37:01 +0100
> Daniel Lezcano wrote:
>
>> On 10/30/2015 12:09 PM, Jisheng Zhang wrote:
>>> Dear Daniel,
>>>
>>> On Fri, 30 Oct 2015 11:44:46 +0100
>>> Daniel Lezcano <daniel.lezcano@....> wrote:
>>>
>>>> On 10/30/2015 09:27 AM, Jisheng Zhang wrote:
>>>>> Implement an ARM delay timer to be used for udelay(). This allows us to
>>>>> skip the delay loop calibration at boot on Marvell BG2, BG2Q, BG2CD
>>>>> platforms. And after this patch, udelay() will be unaffected by CPU
>>>>> frequency changes.
>>>>>
>>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>>>> ---
>>>>>     drivers/clocksource/Kconfig           | 10 ++++++++++
>>>>>     drivers/clocksource/dw_apb_timer_of.c | 16 ++++++++++++++++
>>>>>     2 files changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>>>> index a7726db..7b081805 100644
>>>>> --- a/drivers/clocksource/Kconfig
>>>>> +++ b/drivers/clocksource/Kconfig
>>>>> @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
>>>>>     	select DW_APB_TIMER
>>>>>     	select CLKSRC_OF
>>>>>
>>>>> +config DW_APB_TIMER_BASED_DELAY
>>>>> +	bool "DW APB timer based delay"
>>>>> +	depends on ARM && DW_APB_TIMER_OF
>>>>> +	default n
>>>>> +	help
>>>>> +	  This option enables support for using the DW APB timer to
>>>>> +	  implement timer-based delay. It is useful for skiping the
>>>>> +	  delay loop calibration at boot on some platforms. And the
>>>>> +	  udelay() will be unaffected by CPU frequency changes.
>>>>> +
>>>>
>>>> Why do you want it to be optional ?
>>>>
>>>
>>> Because in some platforms which has arm arch timer, this dw apb timer
>>> delay isn't needed, the arch timer is better. So we want it be optional
>>> so that the platforms which need this feature select it manually when config
>>> the kernel.
>>
>> Correct me if I am wrong. If you have the arch timer, you don't need the
>
> Yes, I don't need the dw apb timer if we have arch timer,
>
>> dw apb timer at all, no ? So the selection would be arch arm timer *or*
>> dw_apb_timer ? not arch_arm_timer for delay and dw_apb_timer for
>> clockevents, right ?
>
> Yes, if we have arch timer, I prefer to use it for clockevent and delay.
>
> Could you please provide suggestion how to handle this case?

If I follow the logic of arch_arm_timer is better than dw_apb timer.

1. The arch_arm_timer is present

  => dw_apb timer is not used at all

  CONFIG_ARM_ARCH_TIMER=y
  # CONFIG_DW_APB_TIMER is not set

2. The arch_arm_timer is *not* present

  => dw_apb_timer is used with delay code

  # CONFIG_ARM_ARCH_TIMER is not set
  CONFIG_DW_APB_TIMER=y

In both cases, DW_APB_TIMER_BASED_DELAY is not needed.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Jisheng Zhang]

Dear Daniel,

On Mon, 2 Nov 2015 09:48:38 +0100
Daniel Lezcano  wrote:

> On 11/02/2015 03:51 AM, Jisheng Zhang wrote:
> > Dear Daniel,
> >
> > On Fri, 30 Oct 2015 13:37:01 +0100
> > Daniel Lezcano wrote:
> >  
> >> On 10/30/2015 12:09 PM, Jisheng Zhang wrote:  
> >>> Dear Daniel,
> >>>
> >>> On Fri, 30 Oct 2015 11:44:46 +0100
> >>> Daniel Lezcano <daniel.lezcano@....> wrote:
> >>>  
> >>>> On 10/30/2015 09:27 AM, Jisheng Zhang wrote:  
> >>>>> Implement an ARM delay timer to be used for udelay(). This allows us to
> >>>>> skip the delay loop calibration at boot on Marvell BG2, BG2Q, BG2CD
> >>>>> platforms. And after this patch, udelay() will be unaffected by CPU
> >>>>> frequency changes.
> >>>>>
> >>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>>>> ---
> >>>>>     drivers/clocksource/Kconfig           | 10 ++++++++++
> >>>>>     drivers/clocksource/dw_apb_timer_of.c | 16 ++++++++++++++++
> >>>>>     2 files changed, 26 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> >>>>> index a7726db..7b081805 100644
> >>>>> --- a/drivers/clocksource/Kconfig
> >>>>> +++ b/drivers/clocksource/Kconfig
> >>>>> @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
> >>>>>     	select DW_APB_TIMER
> >>>>>     	select CLKSRC_OF
> >>>>>
> >>>>> +config DW_APB_TIMER_BASED_DELAY
> >>>>> +	bool "DW APB timer based delay"
> >>>>> +	depends on ARM && DW_APB_TIMER_OF
> >>>>> +	default n
> >>>>> +	help
> >>>>> +	  This option enables support for using the DW APB timer to
> >>>>> +	  implement timer-based delay. It is useful for skiping the
> >>>>> +	  delay loop calibration at boot on some platforms. And the
> >>>>> +	  udelay() will be unaffected by CPU frequency changes.
> >>>>> +  
> >>>>
> >>>> Why do you want it to be optional ?
> >>>>  
> >>>
> >>> Because in some platforms which has arm arch timer, this dw apb timer
> >>> delay isn't needed, the arch timer is better. So we want it be optional
> >>> so that the platforms which need this feature select it manually when config
> >>> the kernel.  
> >>
> >> Correct me if I am wrong. If you have the arch timer, you don't need the  
> >
> > Yes, I don't need the dw apb timer if we have arch timer,
> >  
> >> dw apb timer at all, no ? So the selection would be arch arm timer *or*
> >> dw_apb_timer ? not arch_arm_timer for delay and dw_apb_timer for
> >> clockevents, right ?  
> >
> > Yes, if we have arch timer, I prefer to use it for clockevent and delay.
> >
> > Could you please provide suggestion how to handle this case?  
> 
> If I follow the logic of arch_arm_timer is better than dw_apb timer.
> 
> 1. The arch_arm_timer is present
> 
>   => dw_apb timer is not used at all  
> 
>   CONFIG_ARM_ARCH_TIMER=y
>   # CONFIG_DW_APB_TIMER is not set
> 
> 2. The arch_arm_timer is *not* present
> 
>   => dw_apb_timer is used with delay code  
> 
>   # CONFIG_ARM_ARCH_TIMER is not set
>   CONFIG_DW_APB_TIMER=y
> 
> In both cases, DW_APB_TIMER_BASED_DELAY is not needed.
> 

The problem is register_current_timer_delay() is only available in ARM but
DW APB timer isn't only used in ARM platforms, but also ARM64 etc. where the
register_current_timer_delay() isn't defined. so I used DW_APB_TIMER_BASED_DELAY
to 

1. distinguish whether we need the dw apb timer based delay

2. distinguish whether we have register_current_timer_delay()

I have a solution which doesn't need DW_APB_TIMER_BASED_DELAY symbol -- put
all timer based delay code under CONFIG_ARM, I'm not sure whether this is
better. Could you please give suggestion?

Thanks in advance,
Jisheng

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Arnd Bergmann]

On Monday 02 November 2015 09:48:38 Daniel Lezcano wrote:
> If I follow the logic of arch_arm_timer is better than dw_apb timer.
> 
> 1. The arch_arm_timer is present
> 
>   => dw_apb timer is not used at all
> 
>   CONFIG_ARM_ARCH_TIMER=y
>   # CONFIG_DW_APB_TIMER is not set
> 
> 2. The arch_arm_timer is *not* present
> 
>   => dw_apb_timer is used with delay code
> 
>   # CONFIG_ARM_ARCH_TIMER is not set
>   CONFIG_DW_APB_TIMER=y
> 
> In both cases, DW_APB_TIMER_BASED_DELAY is not needed.
> 

This still has the same problem that running a multi_v7_defconfig
kernel or a binary distro kernel will always enable both. We really
want to have a good run-time decision, not compile-time.

	Arnd

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Arnd Bergmann]

On Friday 30 October 2015 19:09:29 Jisheng Zhang wrote:
> > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > index a7726db..7b081805 100644
> > > --- a/drivers/clocksource/Kconfig
> > > +++ b/drivers/clocksource/Kconfig
> > > @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
> > >     select DW_APB_TIMER
> > >     select CLKSRC_OF
> > >
> > > +config DW_APB_TIMER_BASED_DELAY
> > > +   bool "DW APB timer based delay"
> > > +   depends on ARM && DW_APB_TIMER_OF
> > > +   default n
> > > +   help
> > > +     This option enables support for using the DW APB timer to
> > > +     implement timer-based delay. It is useful for skiping the
> > > +     delay loop calibration at boot on some platforms. And the
> > > +     udelay() will be unaffected by CPU frequency changes.
> > > +  
> > 
> > Why do you want it to be optional ?
> > 
> 
> Because in some platforms which has arm arch timer, this dw apb timer
> delay isn't needed, the arch timer is better. So we want it be optional
> so that the platforms which need this feature select it manually when config
> the kernel.
> 

This is not ideal from an overall maintenance perspective. We want to
be able to have a kernel with all drivers enabled that gives us the
best behavior on all platforms.

The current behavior appears to be that we override all previous
registrations as long as the new one is higher resolution. Is that
the case here? I.e. does the arch timer have a lower resultion than
the dw-apb timer but have some other advantages?

	Arnd

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Jisheng Zhang]

Dear Arnd and Daniel,

On Fri, 30 Oct 2015 13:42:01 +0100
Arnd Bergmann wrote:

> On Friday 30 October 2015 19:09:29 Jisheng Zhang wrote:
> > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > > index a7726db..7b081805 100644
> > > > --- a/drivers/clocksource/Kconfig
> > > > +++ b/drivers/clocksource/Kconfig
> > > > @@ -29,6 +29,16 @@ config DW_APB_TIMER_OF
> > > >     select DW_APB_TIMER
> > > >     select CLKSRC_OF
> > > >
> > > > +config DW_APB_TIMER_BASED_DELAY
> > > > +   bool "DW APB timer based delay"
> > > > +   depends on ARM && DW_APB_TIMER_OF
> > > > +   default n
> > > > +   help
> > > > +     This option enables support for using the DW APB timer to
> > > > +     implement timer-based delay. It is useful for skiping the
> > > > +     delay loop calibration at boot on some platforms. And the
> > > > +     udelay() will be unaffected by CPU frequency changes.
> > > > +    
> > > 
> > > Why do you want it to be optional ?
> > >   
> > 
> > Because in some platforms which has arm arch timer, this dw apb timer
> > delay isn't needed, the arch timer is better. So we want it be optional
> > so that the platforms which need this feature select it manually when config
> > the kernel.
> >   
> 
> This is not ideal from an overall maintenance perspective. We want to
> be able to have a kernel with all drivers enabled that gives us the
> best behavior on all platforms.
> 
> The current behavior appears to be that we override all previous
> registrations as long as the new one is higher resolution. Is that
> the case here? I.e. does the arch timer have a lower resultion than
> the dw-apb timer but have some other advantages?

Take one Marvell Berlin platform for example, the arch timer freq is 25MHZ,
whose resolution is lower than the dw apb timer at 100MHZ. But dw apb timer
is on the APB bus while arch timer sits in CPU, so I guess the cost of
accessing the apb timer is higher than arch timer. 

I have a solution for this case: in platforms with arch timer, I can mark
the dw apb timer as "disabled" in the dts even though the timer sits there.
Then I could make DW_APB_TIMER_BASED_DELAY non-optional but selected by the
the ARCH_XYZ. Is this acceptable?

Thanks in advance,
Jisheng

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Arnd Bergmann]

On Monday 02 November 2015 11:03:34 Jisheng Zhang wrote:
> On Fri, 30 Oct 2015 13:42:01 +0100 Arnd Bergmann wrote:
> > 
> > This is not ideal from an overall maintenance perspective. We want to
> > be able to have a kernel with all drivers enabled that gives us the
> > best behavior on all platforms.
> > 
> > The current behavior appears to be that we override all previous
> > registrations as long as the new one is higher resolution. Is that
> > the case here? I.e. does the arch timer have a lower resultion than
> > the dw-apb timer but have some other advantages?
> 
> Take one Marvell Berlin platform for example, the arch timer freq is 25MHZ,
> whose resolution is lower than the dw apb timer at 100MHZ. But dw apb timer
> is on the APB bus while arch timer sits in CPU, so I guess the cost of
> accessing the apb timer is higher than arch timer. 

Ok, I see.

> I have a solution for this case: in platforms with arch timer, I can mark
> the dw apb timer as "disabled" in the dts even though the timer sits there.
> Then I could make DW_APB_TIMER_BASED_DELAY non-optional but selected by the
> the ARCH_XYZ. Is this acceptable?

That would do the right thing, but doesn't look ideal: The DW_APB timer
on those platforms is fully functional, and a future Linux version or
another OS might decide to use both timers for one reason or another.

I'd be happier with a solution that keeps the DT describing the hardware
and not the way we expect Linux to use it, and instead has some heuristic
in the selection of the delay timer. At the moment, we purely base this
on the frequency, which as you say is suboptimal.

One possible way to improve this would be to add an optional 'latency'
property to the DT nodes (or the driver), and use a combination of latency
and resolution to make the decision.
A simpler way would be to always prefer the arch timer on ARM if that
is present, even if another timer has a higher resolution. This should
be only a few additional lines in register_current_timer_delay(), or
possibly an additional function argument.

	Arnd

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Jisheng Zhang]

Dear Arnd,

On Mon, 2 Nov 2015 22:56:02 +0100
Arnd Bergmann wrote:

> On Monday 02 November 2015 11:03:34 Jisheng Zhang wrote:
> > On Fri, 30 Oct 2015 13:42:01 +0100 Arnd Bergmann wrote:  
> > > 
> > > This is not ideal from an overall maintenance perspective. We want to
> > > be able to have a kernel with all drivers enabled that gives us the
> > > best behavior on all platforms.
> > > 
> > > The current behavior appears to be that we override all previous
> > > registrations as long as the new one is higher resolution. Is that
> > > the case here? I.e. does the arch timer have a lower resultion than
> > > the dw-apb timer but have some other advantages?  
> > 
> > Take one Marvell Berlin platform for example, the arch timer freq is 25MHZ,
> > whose resolution is lower than the dw apb timer at 100MHZ. But dw apb timer
> > is on the APB bus while arch timer sits in CPU, so I guess the cost of
> > accessing the apb timer is higher than arch timer.   
> 
> Ok, I see.
> 
> > I have a solution for this case: in platforms with arch timer, I can mark
> > the dw apb timer as "disabled" in the dts even though the timer sits there.
> > Then I could make DW_APB_TIMER_BASED_DELAY non-optional but selected by the
> > the ARCH_XYZ. Is this acceptable?  
> 
> That would do the right thing, but doesn't look ideal: The DW_APB timer
> on those platforms is fully functional, and a future Linux version or
> another OS might decide to use both timers for one reason or another.
> 
> I'd be happier with a solution that keeps the DT describing the hardware
> and not the way we expect Linux to use it, and instead has some heuristic
> in the selection of the delay timer. At the moment, we purely base this
> on the frequency, which as you say is suboptimal.
> 
> One possible way to improve this would be to add an optional 'latency'
> property to the DT nodes (or the driver), and use a combination of latency
> and resolution to make the decision.

Got it. Thanks for the suggestions. The 'latency' here seems a 'rating'
similar as the one in clocksource. I will cook a series for review:

patch 1 to make register_current_timer_delay() aware of 'rating'

patch 2 to set rating of arch timer as 400

patch 3 to add timer based delay support to dw_apb_timer whose rating is 300

Thanks a lot,
Jisheng

> A simpler way would be to always prefer the arch timer on ARM if that
> is present, even if another timer has a higher resolution. This should
> be only a few additional lines in register_current_timer_delay(), or
> possibly an additional function argument.
> 

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Arnd Bergmann]

On Tuesday 03 November 2015 14:59:40 Jisheng Zhang wrote:
> > On Monday 02 November 2015 11:03:34 Jisheng Zhang wrote:
> > > On Fri, 30 Oct 2015 13:42:01 +0100 Arnd Bergmann wrote:  
> > I'd be happier with a solution that keeps the DT describing the hardware
> > and not the way we expect Linux to use it, and instead has some heuristic
> > in the selection of the delay timer. At the moment, we purely base this
> > on the frequency, which as you say is suboptimal.
> > 
> > One possible way to improve this would be to add an optional 'latency'
> > property to the DT nodes (or the driver), and use a combination of latency
> > and resolution to make the decision.
> 
> Got it. Thanks for the suggestions. The 'latency' here seems a 'rating'
> similar as the one in clocksource. I will cook a series for review:
> 
> patch 1 to make register_current_timer_delay() aware of 'rating'
> 
> patch 2 to set rating of arch timer as 400
> 
> patch 3 to add timer based delay support to dw_apb_timer whose rating is 300

Ok. Just to make sure I got this right: your plan is to use the existing
'rating' setting as a primary indication, and fall back to comparing the
frequency if the rating is the same?

	Arnd

Re: [PATCH] clocksource: dw_apb_timer_of: support timer-based delay

[Jisheng Zhang]

Dear Arnd

On Tue, 3 Nov 2015 09:49:32 +0100
Arnd Bergmann wrote:

> On Tuesday 03 November 2015 14:59:40 Jisheng Zhang wrote:
> > > On Monday 02 November 2015 11:03:34 Jisheng Zhang wrote:  
> > > > On Fri, 30 Oct 2015 13:42:01 +0100 Arnd Bergmann wrote:    
> > > I'd be happier with a solution that keeps the DT describing the hardware
> > > and not the way we expect Linux to use it, and instead has some heuristic
> > > in the selection of the delay timer. At the moment, we purely base this
> > > on the frequency, which as you say is suboptimal.
> > > 
> > > One possible way to improve this would be to add an optional 'latency'
> > > property to the DT nodes (or the driver), and use a combination of latency
> > > and resolution to make the decision.  
> > 
> > Got it. Thanks for the suggestions. The 'latency' here seems a 'rating'
> > similar as the one in clocksource. I will cook a series for review:
> > 
> > patch 1 to make register_current_timer_delay() aware of 'rating'
> > 
> > patch 2 to set rating of arch timer as 400
> > 
> > patch 3 to add timer based delay support to dw_apb_timer whose rating is 300  
> 
> Ok. Just to make sure I got this right: your plan is to use the existing
> 'rating' setting as a primary indication, and fall back to comparing the
> frequency if the rating is the same?

Yes, this is my plan.

Thanks,
Jisheng