linux-kernel - Re: [PATCH 04/12] clocksource: Sched clock source for Versatile Express

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: <1397658125.3407.84.camel@hornet>
Date:	Wed, 16 Apr 2014 15:22:05 +0100
From:	Pawel Moll <pawel.moll@....com>
To:	Rob Herring <robherring2@...il.com>
Cc:	"arm@...nel.org" <arm@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH 04/12] clocksource: Sched clock source for Versatile
 Express

Hi Rob,

Thanks for reminding me about this stuff. I'll get around and re-spin
the series this week.

On Wed, 2014-04-16 at 14:56 +0100, Rob Herring wrote:
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index cd6950f..9799744 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -140,3 +140,12 @@ config VF_PIT_TIMER
> >         bool
> >         help
> >           Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
> > +
> > +config CLKSRC_VEXPRESS
> > +       bool
> > +       depends on MFD_VEXPRESS_SYSREG
> 
> But you don't really depend on this...

Hm. Strictly speaking it's true (no code level dependency) but if one
doesn't build sysreg driver, one doesn't care 

> > +       depends on GENERIC_SCHED_CLOCK
> 
> I think this should be a select, not a depends.

Don't think so, no. It's being selected by an arch. 

> > +       select CLKSRC_OF
> > +       default y
> > +       help
> > +         Simple provider of sched clock on ARM Versatile Express platform.
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index c7ca50a..1051a23 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -37,3 +37,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER)          += arm_arch_timer.o
> >  obj-$(CONFIG_ARM_GLOBAL_TIMER)         += arm_global_timer.o
> >  obj-$(CONFIG_CLKSRC_METAG_GENERIC)     += metag_generic.o
> >  obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)  += dummy_timer.o
> > +obj-$(CONFIG_CLKSRC_VEXPRESS)  += vexpress.o
> > diff --git a/drivers/clocksource/vexpress.c b/drivers/clocksource/vexpress.c
> > new file mode 100644
> > index 0000000..55b8ab4
> > --- /dev/null
> > +++ b/drivers/clocksource/vexpress.c
> > @@ -0,0 +1,40 @@
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * Copyright (C) 2013 ARM Limited
> 
> Did you write this last year?

Yes. Can bump it up anyway.

> > + */
> > +
> > +#include <linux/clocksource.h>
> > +#include <linux/io.h>
> > +#include <linux/of_address.h>
> > +#include <linux/sched_clock.h>
> > +
> > +#define SYS_24MHZ 0x05c
> > +
> > +static void __iomem *vexpress_sys_24mhz;
> > +
> > +static u32 notrace vexpress_sys_24mhz_read(void)
> > +{
> > +       return readl(vexpress_sys_24mhz);
> > +}
> > +
> > +static void __init vexpress_sched_clock_init(struct device_node *node)
> > +{
> > +       void __iomem *base = of_iomap(node, 0);
> > +
> > +       if (!base)
> > +               return;
> > +
> > +       vexpress_sys_24mhz = base + SYS_24MHZ;
> > +
> > +       setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
> 
> This frequency should come from a DT clock binding. You will have to
> fallback to 24MHz for backwards compatibility though.

I don't see why would it go to the binding. You may have noticed the
register is called "SYS_24MHZ", not "SYS_RANDOMCLOCK". The driver
*knows* what the frequency is.

> > +}
> 
> Wouldn't this code work for Versatile and Realview ARM reference
> boards? Even the register offset is the same.
> 
> > +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
> > +               vexpress_sched_clock_init);

I guess it would, yes. The sysregs are annoyingly similar and different
at the same time.

One could of course try to come up with a "generic mmio clock source"
binding, taking the frequency as a property, but don't count on me doing
this... ;-)

Pawel

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