| 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
| ||
|
Message-ID: <4BF5E8683E87FC4DA89822A5A3EB60CB6F25D2@hhmail02.hh.imgtec.org>
Date: Fri, 7 Aug 2015 15:14:38 +0000
From: Govindraj Raja <Govindraj.Raja@...tec.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mips@...ux-mips.org" <linux-mips@...ux-mips.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC: Thomas Gleixner <tglx@...utronix.de>,
Andrew Bresticker <abrestic@...omium.org>,
James Hartley <James.Hartley@...tec.com>,
"Damien Horsley" <Damien.Horsley@...tec.com>,
James Hogan <James.Hogan@...tec.com>,
Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
Ezequiel Garcia <Ezequiel.Garcia@...tec.com>
Subject: RE: [PATCH v5 6/7] clocksource: Add Pistachio clocksource-only
driver
Hi Daniel,
> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@...aro.org]
> Sent: 07 August 2015 11:28 AM
> To: Govindraj Raja; linux-kernel@...r.kernel.org; linux-mips@...ux-mips.org;
> devicetree@...r.kernel.org
> Cc: Thomas Gleixner; Andrew Bresticker; James Hartley; Damien Horsley; James
> Hogan; Ezequiel Garcia; Ezequiel Garcia
> Subject: Re: [PATCH v5 6/7] clocksource: Add Pistachio clocksource-only driver
>
> On 08/06/2015 01:22 PM, Govindraj Raja wrote:
> > From: Ezequiel Garcia <ezequiel.garcia@...tec.com>
> >
> > The Pistachio SoC provides four general purpose timers, and allow to
> > implement a clocksource driver.
> >
> > This driver can be used as a replacement for the MIPS GIC and MIPS R4K
> > clocksources and sched clocks, which are clocked from the CPU clock.
> >
> > Given the general purpose timers are clocked from an independent
> > clock, this new clocksource driver will be useful to introduce CPUFreq
> > support for Pistachio machines.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@...tec.com>
> > Signed-off-by: Govindraj Raja <govindraj.raja@...tec.com>
> > ---
> >
> > changes from v4
> > ----------------
> > Fixes comments from Daniel as dicussed in below thread:
> > http://patchwork.linux-mips.org/patch/10784/
> >
> >
> > drivers/clocksource/Kconfig | 5 +
> > drivers/clocksource/Makefile | 1 +
> > drivers/clocksource/time-pistachio.c | 216
> +++++++++++++++++++++++++++++++++++
> > 3 files changed, 222 insertions(+)
> > create mode 100644 drivers/clocksource/time-pistachio.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>
> [ ... ]
>
> > index 4e57730..e642825 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -111,6 +111,11 @@ config CLKSRC_LPC32XX
> > select CLKSRC_MMIO
> > select CLKSRC_OF
> >
> > +config CLKSRC_PISTACHIO
> > + bool
> > + default y if MACH_PISTACHIO
>
> You don't need to add this condition.
Ok fine. Will remove it.
>
> > + select CLKSRC_OF
>
> [ ... ]
>
> > +
> > +struct pistachio_clocksource pcs_gpt;
>
> static.
>
Ok.
> > +#define to_pistachio_clocksource(cs) \
> > + container_of(cs, struct pistachio_clocksource, cs)
> > +
> > +static inline u32 gpt_readl(void __iomem *base, u32 offset, u32
> > +gpt_id) {
> > + return readl(base + 0x20 * gpt_id + offset);
>
> Are you sure of the address computation ? I guess it is correct but just wanted
> you to double check.
>
Yes, tested it on Pistachio bring up board.
> > +}
> > +
> > +static inline void gpt_writel(void __iomem *base, u32 value, u32 offset,
> > + u32 gpt_id)
> > +{
> > + writel(value, base + 0x20 * gpt_id + offset); }
> > +
> > +static cycle_t pistachio_clocksource_read_cycles(struct clocksource
> > +*cs) {
> > + struct pistachio_clocksource *pcs = to_pistachio_clocksource(cs);
> > + u32 counter, overflw;
> > + unsigned long flags;
> > +
> > + /* The counter value is only refreshed after the overflow value is read.
> > + * And they must be read in strict order, hence raw spin lock added.
> > + */
>
> nit: extra carriage return and comment format:
>
> /*
> * blabla
> */
>
Ok.
> > + raw_spin_lock_irqsave(&pcs->lock, flags);
> > + overflw = gpt_readl(pcs->base, TIMER_CURRENT_OVERFLOW_VALUE,
> 0);
> > + counter = gpt_readl(pcs->base, TIMER_CURRENT_VALUE, 0);
> > + raw_spin_unlock_irqrestore(&pcs->lock, flags);
> > +
> > + return ~(cycle_t)counter;
> > +}
> > +
> > +static u64 notrace pistachio_read_sched_clock(void) {
> > + return pistachio_clocksource_read_cycles(&pcs_gpt.cs);
>
> Can you double check the u32 cast to cycle_t returning a u64 from this function ?
>
Sorry I didn't get you over this comment.
AFAIU,
from include/linux/types.h
[..]
/* clocksource cycle base type */
typedef u64 cycle_t;
[..]
Did you mean to check this?
> > +}
> > +
> > +static void pistachio_clksrc_set_mode(struct clocksource *cs, int timeridx,
> > + int enable)
> > +{
> > + struct pistachio_clocksource *pcs = to_pistachio_clocksource(cs);
> > + u32 val;
> > +
> > + val = gpt_readl(pcs->base, TIMER_CFG, timeridx);
> > + if (enable)
> > + val |= TIMER_ME_LOCAL;
> > + else
> > + val &= ~TIMER_ME_LOCAL;
> > +
> > + gpt_writel(pcs->base, val, TIMER_CFG, timeridx); }
> > +
> > +static void pistachio_clksrc_enable(struct clocksource *cs, int
> > +timeridx) {
> > + struct pistachio_clocksource *pcs = to_pistachio_clocksource(cs);
> > +
> > + /* Disable GPT local before loading reload value */
> > + pistachio_clksrc_set_mode(cs, timeridx, false);
> > + gpt_writel(pcs->base, RELOAD_VALUE, TIMER_RELOAD_VALUE,
> timeridx);
> > + pistachio_clksrc_set_mode(cs, timeridx, true); }
> > +
> > +static void pistachio_clksrc_disable(struct clocksource *cs, int
> > +timeridx) {
> > + /* Disable GPT local */
> > + pistachio_clksrc_set_mode(cs, timeridx, false); }
> > +
> > +static int pistachio_clocksource_enable(struct clocksource *cs) {
> > + pistachio_clksrc_enable(cs, 0);
> > + return 0;
> > +}
> > +
> > +static void pistachio_clocksource_disable(struct clocksource *cs) {
> > + pistachio_clksrc_disable(cs, 0);
> > +}
> > +
> > +/* Desirable clock source for pistachio platform */ struct
> > +pistachio_clocksource pcs_gpt = {
>
> static.
Ok.
Posting v6 addressing these comments.
--
Thanks.
Govindraj.R
Powered by blists - more mailing lists