| 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: <alpine.LNX.2.00.1506240703440.24366@localhost.localdomain>
Date: Wed, 24 Jun 2015 07:09:06 +0200 (CEST)
From: Paul Osmialowski <pawelo@...g.net.pl>
To: Stephen Boyd <sboyd@...eaurora.org>
cc: Paul Osmialowski <pawelo@...g.net.pl>,
Andrew Morton <akpm@...ux-foundation.org>,
Anson Huang <b20788@...escale.com>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Arnd Bergmann <arnd@...db.de>,
Bhupesh Sharma <bhupesh.sharma@...escale.com>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Frank Li <Frank.Li@...escale.com>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Guenter Roeck <linux@...ck-us.net>,
Haojian Zhuang <haojian.zhuang@...il.com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Jingchang Lu <jingchang.lu@...escale.com>,
Jiri Slaby <jslaby@...e.cz>, Kees Cook <keescook@...omium.org>,
Kumar Gala <galak@...eaurora.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Linus Walleij <linus.walleij@...aro.org>,
Magnus Damm <damm+renesas@...nsource.se>,
Michael Turquette <mturquette@...libre.com>,
Nathan Lynch <nathan_lynch@...tor.com>,
Nicolas Pitre <nico@...aro.org>,
Maxime Coquelin stm32 <mcoquelin.stm32@...il.com>,
Olof Johansson <olof@...om.net>,
Paul Bolle <pebolle@...cali.nl>,
Rob Herring <r.herring@...escale.com>,
Rob Herring <robh+dt@...nel.org>,
Russell King <linux@....linux.org.uk>,
Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
Shawn Guo <shawn.guo@...aro.org>,
Simon Horman <horms+renesas@...ge.net.au>,
Stefan Agner <stefan@...er.ch>,
Thomas Gleixner <tglx@...utronix.de>,
Uwe Kleine-Koenig <u.kleine-koenig@...gutronix.de>,
Catalin Marinas <catalin.marinas@....com>,
Dave Martin <Dave.Martin@....com>,
Mark Rutland <mark.rutland@....com>,
Pawel Moll <pawel.moll@....com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-serial@...r.kernel.org,
devicetree@...r.kernel.org, dmaengine@...r.kernel.org,
Yuri Tikhonov <yur@...raft.com>,
Sergei Poselenov <sposelenov@...raft.com>,
Dmitry Cherkassov <d_cherkasov@...raft.com>,
Alexander Potashev <aspotashev@...raft.com>
Subject: Re: [PATCH 6/9] arm: twr-k70f120m: clock source drivers for Kinetis
SoC
Hi Stephen,
Thanks for the valuable input - all of those points are now on my
checklist for the work on the second iteration of this patchset.
On Tue, 23 Jun 2015, Stephen Boyd wrote:
> On 06/23/2015 02:19 PM, Paul Osmialowski wrote:
>>
>> diff --git a/drivers/clk/clk-kinetis.c b/drivers/clk/clk-kinetis.c
>> new file mode 100644
>> index 0000000..dea1054
>> --- /dev/null
>> +++ b/drivers/clk/clk-kinetis.c
>> @@ -0,0 +1,226 @@
>> +/*
>> + * clk-kinetis.c - Clock driver for Kinetis K70 MCG
>> + *
>> + * Based on legacy pre-OF code by Alexander Potashev <aspotashev@...raft.com>
>> + *
>> + * Copyright (C) 2015 Paul Osmialowski <pawelo@...g.net.pl>
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/clk.h>
>
> Is this using the consumer API? Please remove this include.
>
>> +#include <linux/io.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/err.h>
>> +#include <mach/kinetis.h>
>> +#include <mach/power.h>
>
> It would be nice if we didn't need these mach includes so that this
> driver can be easily build tested.
>
>> +
>> +#include <dt-bindings/clock/kinetis-mcg.h>
> [..]
>> +}
>> +
>> +CLK_OF_DECLARE(kinetis_mcg, "fsl,kinetis-cmu", kinetis_mcg_init);
>
> A clocksource isn't the same as a clk provider. Please split this patch
> into two, one for the clk provider (drivers/clk) and one for the
> clocksource driver (drivers/clocksource).
>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 0f1c77e..1d2ecde 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -106,6 +106,11 @@ config CLKSRC_EFM32
>> Support to use the timers of EFM32 SoCs as clock source and clock
>> event device.
>>
>> +config CLKSRC_KINETIS
>> + bool "Clocksource for Kinetis SoCs"
>> + depends on OF && ARM && ARCH_KINETIS
>
> Doesn't ARCH_KINETIS imply ARM? Seems that we can drop the ARM
> dependency here.
>
>> + select CLKSRC_OF
>>
>>
>> diff --git a/drivers/clocksource/timer-kinetis.c b/drivers/clocksource/timer-kinetis.c
>> new file mode 100644
>> index 0000000..634f365
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-kinetis.c
> [..]
>> +
>> +/*
>> + * Clock event device set mode function
>> + */
>> +static void kinetis_clockevent_tmr_set_mode(
>> + enum clock_event_mode mode, struct clock_event_device *clk)
>
> s/clk/evt/ ?
>
>> +{
>> + struct kinetis_clock_event_ddata *pit =
>> + container_of(clk, struct kinetis_clock_event_ddata, evtdev);
>> +
>> + switch (mode) {
>> + case CLOCK_EVT_MODE_PERIODIC:
>> + kinetis_pit_enable(pit->base, 1);
>> + break;
>> + case CLOCK_EVT_MODE_ONESHOT:
>> + case CLOCK_EVT_MODE_UNUSED:
>> + case CLOCK_EVT_MODE_SHUTDOWN:
>> + default:
>> + kinetis_pit_enable(pit->base, 0);
>> + }
>> +}
>> +
>> +/*
>> + * Configure the timer to generate an interrupt in the specified amount of ticks
>> + */
>> +static int kinetis_clockevent_tmr_set_next_event(
>> + unsigned long delta, struct clock_event_device *c)
>> +{
>> + struct kinetis_clock_event_ddata *pit =
>> + container_of(c, struct kinetis_clock_event_ddata, evtdev);
>> + unsigned long flags;
>> +
>> + raw_local_irq_save(flags);
>
> What is this protecting against?
>
>> + kinetis_pit_init(pit->base, delta);
>> + kinetis_pit_enable(pit->base, 1);
>> + raw_local_irq_restore(flags);
>> +
>> + return 0;
>> +}
>> +
>> +static struct kinetis_clock_event_ddata
>> + kinetis_clockevent_tmrs[KINETIS_PIT_CHANNELS] = {
>> + {
>> + .evtdev = {
>> + .name = "fsl,kinetis-pit-timer0",
>> + .rating = 200,
>> + .features =
>> + CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
>> + .set_mode = kinetis_clockevent_tmr_set_mode,
>> + .set_next_event = kinetis_clockevent_tmr_set_next_event,
>> + },
>> + },
>> + {
>> + .evtdev = {
>> + .name = "fsl,kinetis-pit-timer1",
>> + },
>> + },
>> + {
>> + .evtdev = {
>> + .name = "fsl,kinetis-pit-timer2",
>> + },
>> + },
>> + {
>> + .evtdev = {
>> + .name = "fsl,kinetis-pit-timer3",
>> + },
>> + },
>> +};
>> +
>> +/*
>> + * Timer IRQ handler
>> + */
>> +static irqreturn_t kinetis_clockevent_tmr_irq_handler(int irq, void *dev_id)
>> +{
>> + struct kinetis_clock_event_ddata *tmr = dev_id;
>> +
>> + KINETIS_PIT_WR(tmr->base, tflg, KINETIS_PIT_TFLG_TIF_MSK);
>> +
>> + tmr->evtdev.event_handler(&(tmr->evtdev));
>
> Unnecessary parentheses, please remove them.
>
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/*
>> + * System timer IRQ action
>> + */
>> +static struct irqaction kinetis_clockevent_irqaction[KINETIS_PIT_CHANNELS] = {
>> + {
>> + .name = "Kinetis Kernel Time Tick (pit0)",
>> + .flags = IRQF_TIMER | IRQF_IRQPOLL,
>> + .dev_id = &kinetis_clockevent_tmrs[0],
>> + .handler = kinetis_clockevent_tmr_irq_handler,
>> + }, {
>> + .name = "Kinetis Kernel Time Tick (pit1)",
>> + .flags = IRQF_TIMER | IRQF_IRQPOLL,
>> + .dev_id = &kinetis_clockevent_tmrs[1],
>> + .handler = kinetis_clockevent_tmr_irq_handler,
>> + }, {
>> + .name = "Kinetis Kernel Time Tick (pit2)",
>> + .flags = IRQF_TIMER | IRQF_IRQPOLL,
>> + .dev_id = &kinetis_clockevent_tmrs[2],
>> + .handler = kinetis_clockevent_tmr_irq_handler,
>> + }, {
>> + .name = "Kinetis Kernel Time Tick (pit3)",
>> + .flags = IRQF_TIMER | IRQF_IRQPOLL,
>> + .dev_id = &kinetis_clockevent_tmrs[3],
>> + .handler = kinetis_clockevent_tmr_irq_handler,
>> + },
>> +};
>
> Any reason we can't just use request_irq() instead of having a set of
> static irq actions?
>
>> +
>> +static void __init kinetis_clockevent_init(struct device_node *np)
>> +{
> [..]
>> irq;
>> + }
>> +
>> + chan = of_alias_get_id(np, "pit");
>> + if ((chan < 0) || (chan >= KINETIS_PIT_CHANNELS)) {
>
> Unnecessary parentheses, please remove them.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
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