linux-kernel - Re: [PATCH v2] clocksource: exynos-mct: Register the timer for stable udelay

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53A2B9C3.1050906@gmail.com>
Date:	Thu, 19 Jun 2014 12:21:55 +0200
From:	Tomasz Figa <tomasz.figa@...il.com>
To:	Amit Daniel Kachhap <amit.daniel@...sung.com>,
	linux-samsung-soc@...r.kernel.org,
	Kukjin Kim <kgene.kim@...sung.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>
CC:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	David Riley <davidriley@...omium.org>,
	Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH v2] clocksource: exynos-mct: Register the timer for stable
 udelay

Hi Amit,

Please see my comments inline.

On 19.06.2014 10:39, Amit Daniel Kachhap wrote:
> This patch register the exynos mct clocksource as the current timer
> as it has constant clock rate. This will generate correct udelay for the
> exynos platform and avoid using unnecessary calibrated jiffies. This change
> has been tested on exynos5420 based board and udelay is very close to
> expected.
> 
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@...sung.com>
> ---
> Changes in V2:
> * Added #defines for ARM and ARM64 as pointed by Doug Anderson.
> 
> Patches from David Riley confirmed that udelay is broken in exynos5420.
> Link to those patches are,
> 1) https://patchwork.kernel.org/patch/4344911/
> 2) https://patchwork.kernel.org/patch/4344881/
> 
>  drivers/clocksource/exynos_mct.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index f71d55f..02927e2 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -195,10 +195,25 @@ static u64 notrace exynos4_read_sched_clock(void)
>  	return exynos4_frc_read(&mct_frc);
>  }
>  
> +static struct delay_timer exynos4_delay_timer;
> +
> +static unsigned long exynos4_read_current_timer(void)
> +{
> +#ifdef ARM
> +	return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L);
> +#else /* ARM64, etc */
> +	return exynos4_frc_read(&mct_frc);
> +#endif
> +}
> +

No need for anything like this. Even if running on ARM64, the delay
timer code should be able to cope with different timer widths. For
delays, 32 bits are enough, so just always read the lower part.

Also use of raw accessors in drivers is discouraged - please use
readl_relaxed().

Btw. I don't even see support for this on ARM64 in mainline, where arch
timer is always used for delays and AFAIK this is a platform requirement.

>  static void __init exynos4_clocksource_init(void)
>  {
>  	exynos4_mct_frc_start();
>  
> +	exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;

nit: No need for & for function pointers.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/