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  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]
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/

Powered by blists - more mailing lists