[PATCH v2 0/3] let Marvell Berlin SoCs make use of the best delay timer

[PATCH v2 0/3] let Marvell Berlin SoCs make use of the best delay timer

[Jisheng Zhang]

In case there are several possible delay timers, we purely base the
selection on the frequency, which is suboptimal in some cases. Take
one Marvell Berlin platform for example: we have arch timer and dw-apb
timer. The arch timer freq is 25MHZ while the dw-apb timer freq is
100MHZ, current selection would choose the dw-apb timer. But the dw
apb timer is on the APB bus while arch timer sits in CPU, the cost
of accessing the apb timer is higher than the arch timer.
    
This series firstly modifies register_current_timer_delay() to choose
the highest rating delay timer: use the rating as a primary indication
and fall back to comparing the frequency if the rating is not set or
the same. Then we set the arch_delay_timer rating as 400, finally
Implement ARM delay timer for the dw_apb_timer and set its rating as 300.

Since v1:
 - add one patch to let register_current_timer_delay() to choose the the
   highest rating delay timer
 - add one patch to set arch_delay_timer rating as 400
 - remove CONFIG_DW_APB_TIMER_BASED_DELAY option, use CONFIG_ARM instead.
 - change the commit msg as "clocksource/drivers/abc: Foo...."


Jisheng Zhang (3):
  ARM: delay: choose the highest rating delay timer
  ARM: arch_timer: set the arch_delay_timer rating as 400
  clocksource/drivers/dw_apb_timer_of: Implement ARM delay timer

 arch/arm/include/asm/delay.h          |  1 +
 arch/arm/kernel/arch_timer.c          |  1 +
 arch/arm/lib/delay.c                  | 16 +++++++++++++++-
 drivers/clocksource/dw_apb_timer_of.c | 17 +++++++++++++++++
 4 files changed, 34 insertions(+), 1 deletion(-)

-- 
2.6.2

[PATCH v2 1/3] ARM: delay: choose the highest rating delay timer

[Jisheng Zhang]

In case there are several possible delay timers, we purely base the
selection on the frequency, which is suboptimal in some cases. Take
one Marvell Berlin platform for example: we have arch timer and dw-apb
timer. The arch timer freq is 25MHZ while the dw-apb timer freq is
100MHZ, current selection would choose the dw-apb timer. But the dw
apb timer is on the APB bus while arch timer sits in CPU, the cost
of accessing the apb timer is higher than the arch timer.

This patch introduces rating concept to the delay timer and use it
as a primary indication, we fall back to comparing the frequency if
the rating is not set or the same.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm/include/asm/delay.h |  1 +
 arch/arm/lib/delay.c         | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
index dff714d..cb445d7 100644
--- a/arch/arm/include/asm/delay.h
+++ b/arch/arm/include/asm/delay.h
@@ -18,6 +18,7 @@
 struct delay_timer {
 	unsigned long (*read_current_timer)(void);
 	unsigned long freq;
+	int rating;
 };
 
 extern struct arm_delay_ops {
diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
index 8044591..91cee12 100644
--- a/arch/arm/lib/delay.c
+++ b/arch/arm/lib/delay.c
@@ -38,6 +38,7 @@ struct arm_delay_ops arm_delay_ops = {
 static const struct delay_timer *delay_timer;
 static bool delay_calibrated;
 static u64 delay_res;
+static int delay_rating;
 
 int read_current_timer(unsigned long *timer_val)
 {
@@ -78,6 +79,7 @@ void __init register_current_timer_delay(const struct delay_timer *timer)
 {
 	u32 new_mult, new_shift;
 	u64 res;
+	bool update_delay_ops = false;
 
 	clocks_calc_mult_shift(&new_mult, &new_shift, timer->freq,
 			       NSEC_PER_SEC, 3600);
@@ -89,11 +91,23 @@ void __init register_current_timer_delay(const struct delay_timer *timer)
 		return;
 	}
 
-	if (!delay_calibrated && (!delay_res || (res < delay_res))) {
+	if (!delay_calibrated) {
+		if (delay_rating && timer->rating &&
+				delay_rating != timer->rating) {
+			if (timer->rating > delay_rating)
+				update_delay_ops = true;
+		} else {
+			if (!delay_res || (res < delay_res))
+				update_delay_ops = true;
+		}
+	}
+
+	if (update_delay_ops) {
 		pr_info("Switching to timer-based delay loop, resolution %lluns\n", res);
 		delay_timer			= timer;
 		lpj_fine			= timer->freq / HZ;
 		delay_res			= res;
+		delay_rating			= timer->rating;
 
 		/* cpufreq may scale loops_per_jiffy, so keep a private copy */
 		arm_delay_ops.ticks_per_jiffy	= lpj_fine;
-- 
2.6.2

[PATCH v2 2/3] ARM: arch_timer: set the arch_delay_timer rating as 400

[Jisheng Zhang]

This patch sets the arch_delay_timer rating as the same value used in
arch_timer clocksource. This is to help register_current_timer_delay()
to choose the best delay timer during several possible delay timers.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm/kernel/arch_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index 1791f12..db5f556 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -28,6 +28,7 @@ static void __init arch_timer_delay_timer_register(void)
 	/* Use the architected timer for the delay loop. */
 	arch_delay_timer.read_current_timer = arch_timer_read_counter_long;
 	arch_delay_timer.freq = arch_timer_get_rate();
+	arch_delay_timer.rating = 400;
 	register_current_timer_delay(&arch_delay_timer);
 }
 
-- 
2.6.2

[PATCH v2 3/3] clocksource/drivers/dw_apb_timer_of: Implement ARM delay timer

[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/dw_apb_timer_of.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index a19a3f6..7a6c3bf 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,18 @@ static void __init init_sched_clock(void)
 	sched_clock_register(read_sched_clock, 32, sched_rate);
 }
 
+#ifdef CONFIG_ARM
+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,
+	.rating			= 300,
+};
+#endif
+
 static int num_called;
 static void __init dw_apb_timer_init(struct device_node *timer)
 {
@@ -142,6 +155,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_ARM
+		dw_apb_delay_timer.freq = sched_rate;
+		register_current_timer_delay(&dw_apb_delay_timer);
+#endif
 		break;
 	default:
 		break;
-- 
2.6.2

Re: [PATCH v2 0/3] let Marvell Berlin SoCs make use of the best delay timer

[Daniel Lezcano]

On 11/03/2015 03:28 PM, Jisheng Zhang wrote:
> In case there are several possible delay timers, we purely base the
> selection on the frequency, which is suboptimal in some cases. Take
> one Marvell Berlin platform for example: we have arch timer and dw-apb
> timer. The arch timer freq is 25MHZ while the dw-apb timer freq is
> 100MHZ, current selection would choose the dw-apb timer. But the dw
> apb timer is on the APB bus while arch timer sits in CPU, the cost
> of accessing the apb timer is higher than the arch timer.
>
> This series firstly modifies register_current_timer_delay() to choose
> the highest rating delay timer: use the rating as a primary indication
> and fall back to comparing the frequency if the rating is not set or
> the same. Then we set the arch_delay_timer rating as 400, finally
> Implement ARM delay timer for the dw_apb_timer and set its rating as 300.

Hi Jisheng, Arnd,

I don't feel comfortable with the rating / freq think. I am afraid this 
approach based on heuristic will bring a lot of complexity and 
workarounds in the code for a small benefit.

Why don't we define a DT entry for the delay timer ? So we delegate the 
choice to the platform DT definition.


-- 
  <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 v2 0/3] let Marvell Berlin SoCs make use of the best delay timer

[Arnd Bergmann]

On Wednesday 04 November 2015 10:46:49 Daniel Lezcano wrote:
> On 11/03/2015 03:28 PM, Jisheng Zhang wrote:
> > In case there are several possible delay timers, we purely base the
> > selection on the frequency, which is suboptimal in some cases. Take
> > one Marvell Berlin platform for example: we have arch timer and dw-apb
> > timer. The arch timer freq is 25MHZ while the dw-apb timer freq is
> > 100MHZ, current selection would choose the dw-apb timer. But the dw
> > apb timer is on the APB bus while arch timer sits in CPU, the cost
> > of accessing the apb timer is higher than the arch timer.
> >
> > This series firstly modifies register_current_timer_delay() to choose
> > the highest rating delay timer: use the rating as a primary indication
> > and fall back to comparing the frequency if the rating is not set or
> > the same. Then we set the arch_delay_timer rating as 400, finally
> > Implement ARM delay timer for the dw_apb_timer and set its rating as 300.
> 
> Hi Jisheng, Arnd,
> 
> I don't feel comfortable with the rating / freq think. I am afraid this 
> approach based on heuristic will bring a lot of complexity and 
> workarounds in the code for a small benefit.
> 
> Why don't we define a DT entry for the delay timer ? So we delegate the 
> choice to the platform DT definition.

That would be wrong, because the fact that Linux uses a timer to
optimize its udelay() function is not a feature of the hardware.

	Arnd

Re: [PATCH v2 0/3] let Marvell Berlin SoCs make use of the best delay timer

[Daniel Lezcano]

On 11/04/2015 11:30 AM, Arnd Bergmann wrote:
> On Wednesday 04 November 2015 10:46:49 Daniel Lezcano wrote:
>> On 11/03/2015 03:28 PM, Jisheng Zhang wrote:
>>> In case there are several possible delay timers, we purely base the
>>> selection on the frequency, which is suboptimal in some cases. Take
>>> one Marvell Berlin platform for example: we have arch timer and dw-apb
>>> timer. The arch timer freq is 25MHZ while the dw-apb timer freq is
>>> 100MHZ, current selection would choose the dw-apb timer. But the dw
>>> apb timer is on the APB bus while arch timer sits in CPU, the cost
>>> of accessing the apb timer is higher than the arch timer.
>>>
>>> This series firstly modifies register_current_timer_delay() to choose
>>> the highest rating delay timer: use the rating as a primary indication
>>> and fall back to comparing the frequency if the rating is not set or
>>> the same. Then we set the arch_delay_timer rating as 400, finally
>>> Implement ARM delay timer for the dw_apb_timer and set its rating as 300.
>>
>> Hi Jisheng, Arnd,
>>
>> I don't feel comfortable with the rating / freq think. I am afraid this
>> approach based on heuristic will bring a lot of complexity and
>> workarounds in the code for a small benefit.
>>
>> Why don't we define a DT entry for the delay timer ? So we delegate the
>> choice to the platform DT definition.
>
> That would be wrong, because the fact that Linux uses a timer to
> optimize its udelay() function is not a feature of the hardware.

True.

Any ideas / suggestions for an alternative ?



-- 
  <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 v2 0/3] let Marvell Berlin SoCs make use of the best delay timer

[Arnd Bergmann]

On Wednesday 04 November 2015 12:19:57 Daniel Lezcano wrote:
> On 11/04/2015 11:30 AM, Arnd Bergmann wrote:
> > On Wednesday 04 November 2015 10:46:49 Daniel Lezcano wrote:
> >> On 11/03/2015 03:28 PM, Jisheng Zhang wrote:
> >>> In case there are several possible delay timers, we purely base the
> >>> selection on the frequency, which is suboptimal in some cases. Take
> >>> one Marvell Berlin platform for example: we have arch timer and dw-apb
> >>> timer. The arch timer freq is 25MHZ while the dw-apb timer freq is
> >>> 100MHZ, current selection would choose the dw-apb timer. But the dw
> >>> apb timer is on the APB bus while arch timer sits in CPU, the cost
> >>> of accessing the apb timer is higher than the arch timer.
> >>>
> >>> This series firstly modifies register_current_timer_delay() to choose
> >>> the highest rating delay timer: use the rating as a primary indication
> >>> and fall back to comparing the frequency if the rating is not set or
> >>> the same. Then we set the arch_delay_timer rating as 400, finally
> >>> Implement ARM delay timer for the dw_apb_timer and set its rating as 300.
> >>
> >> Hi Jisheng, Arnd,
> >>
> >> I don't feel comfortable with the rating / freq think. I am afraid this
> >> approach based on heuristic will bring a lot of complexity and
> >> workarounds in the code for a small benefit.
> >>
> >> Why don't we define a DT entry for the delay timer ? So we delegate the
> >> choice to the platform DT definition.
> >
> > That would be wrong, because the fact that Linux uses a timer to
> > optimize its udelay() function is not a feature of the hardware.
> 
> True.
> 
> Any ideas / suggestions for an alternative ?

How about simply hardcoding the fact that we prefer the arch timer
over any other one for delay as I suggested earlier?

Another idea I just had is to do nothing: According to Jisheng's
description for this series, the reason for preferring the arch
timer is that it is faster to access. However, we could argue
that this actually doesn't matter at all, because the entire
point of the ndelay()/udelay()/mdelay() functions is to waste
CPU cycles doing not much at all, so we can just as well waste
them reading the timer register than spinning on the CPU reading
the arch timer more often.

	Arnd

Re: [PATCH v2 0/3] let Marvell Berlin SoCs make use of the best delay timer

[Jisheng Zhang]

Dear Arnd and Daniel,

On Wed, 4 Nov 2015 13:19:53 +0100
Arnd Bergmann wrote:

> On Wednesday 04 November 2015 12:19:57 Daniel Lezcano wrote:
> > On 11/04/2015 11:30 AM, Arnd Bergmann wrote:  
> > > On Wednesday 04 November 2015 10:46:49 Daniel Lezcano wrote:  
> > >> On 11/03/2015 03:28 PM, Jisheng Zhang wrote:  
> > >>> In case there are several possible delay timers, we purely base the
> > >>> selection on the frequency, which is suboptimal in some cases. Take
> > >>> one Marvell Berlin platform for example: we have arch timer and dw-apb
> > >>> timer. The arch timer freq is 25MHZ while the dw-apb timer freq is
> > >>> 100MHZ, current selection would choose the dw-apb timer. But the dw
> > >>> apb timer is on the APB bus while arch timer sits in CPU, the cost
> > >>> of accessing the apb timer is higher than the arch timer.
> > >>>
> > >>> This series firstly modifies register_current_timer_delay() to choose
> > >>> the highest rating delay timer: use the rating as a primary indication
> > >>> and fall back to comparing the frequency if the rating is not set or
> > >>> the same. Then we set the arch_delay_timer rating as 400, finally
> > >>> Implement ARM delay timer for the dw_apb_timer and set its rating as 300.  
> > >>
> > >> Hi Jisheng, Arnd,
> > >>
> > >> I don't feel comfortable with the rating / freq think. I am afraid this
> > >> approach based on heuristic will bring a lot of complexity and
> > >> workarounds in the code for a small benefit.
> > >>
> > >> Why don't we define a DT entry for the delay timer ? So we delegate the
> > >> choice to the platform DT definition.  
> > >
> > > That would be wrong, because the fact that Linux uses a timer to
> > > optimize its udelay() function is not a feature of the hardware.  
> > 
> > True.
> > 
> > Any ideas / suggestions for an alternative ?  
> 
> How about simply hardcoding the fact that we prefer the arch timer
> over any other one for delay as I suggested earlier?
> 
> Another idea I just had is to do nothing: According to Jisheng's
> description for this series, the reason for preferring the arch
> timer is that it is faster to access. However, we could argue
> that this actually doesn't matter at all, because the entire
> point of the ndelay()/udelay()/mdelay() functions is to waste
> CPU cycles doing not much at all, so we can just as well waste
> them reading the timer register than spinning on the CPU reading
> the arch timer more often.
> 

I like this "Another idea", indeed, the delay timer speed doesn't matter
at all. So I just cooked v3 to simply register dw apb based delay timer.

Thanks a lot for the inspiration,
Jisheng