From: swarren@wwwdotorg•org (Stephen Warren)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] ARM: bcm281xx: Add timer driver
Date: Tue, 04 Dec 2012 19:54:55 -0700 [thread overview]
Message-ID: <50BEB77F.10303@wwwdotorg.org> (raw)
In-Reply-To: <1354593324-21300-1-git-send-email-csd@broadcom.com>
On 12/03/2012 08:55 PM, Christian Daudt wrote:
> This adds support for the Broadcom timer, used in the following SoCs:
> BCM11130, BCM11140, BCM11351, BCM28145, BCM28155
>
> This patch needs the arm-soc/soc/next branch
I assume this is being applied for 3.9, so it'll want to be rebased on
the struct sys_timer removal, which I hope to get into linux-next very
early after 3.8-rc1.
> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
> + timer at 35006000 {
> + compatible = "bcm,kona-timer";
Is there DT binding documentation for this?
I'm slightly worried about "kona". Is it a well-known name outside
Broadcom for this HW block? If it really is the name though, it's fine I
guess, since it's within the "bcm," name-space here.
> diff --git a/drivers/clocksource/bcm_timer.c b/drivers/clocksource/bcm_timer.c
Is this timer HW used in every Broadcom chip? I wonder if the file
shouldn't be named bcm_kona_timer.c to allow co-existence with any others.
> +struct bcm_timers {
> + int tmr_irq;
> + void __iomem *tmr_regs;
> +};
> +
> +struct bcm_timers timers;
Should that be static?
> +/* We use the peripheral timers for system tick, the cpu global timer for
> + * profile tick
> + */
I think it's usual to leave the /* line empty, so that would be:
> +/*
> + * We use the peripheral timers for system tick, the cpu global timer for
> + * profile tick
> + */
> +static void timer_disable_and_clear(void __iomem *base)
> + reg = 0;
> +
> + /* Clear compare (0) interrupt */
> + reg |= 1 << KONA_GPTIMER_STCS_TIMER_MATCH_SHIFT;
> + /* disable compare */
> + reg &= ~(1 << KONA_GPTIMER_STCS_COMPARE_ENABLE_SHIFT);
That bit isn't set anywhere; is there a need to clear it; should "reg =
0" above be a readl() of the register?
> +static void
> +timer_get_counter(void *timer_base, uint32_t *msw, uint32_t *lsw)
> + while (1) {
> + *msw = readl(base + KONA_GPTIMER_STCHI_OFFSET);
> + *lsw = readl(base + KONA_GPTIMER_STCLO_OFFSET);
> + if (*msw == readl(base + KONA_GPTIMER_STCHI_OFFSET))
> + break;
> + }
I guess it's very unlikely, but should there be a limit to the loop
count here, or a diagnostic if it runs too long?
> +static void __init timer_clockevents_init(void)
Can this function use clockevents_config_and_register() to avoid a bunch
of the manual calculations and assignments?
> diff --git a/drivers/clocksource/bcm_timer.h b/drivers/clocksource/bcm_timer.h
You may as well move these definitions into the .c file since they don't
need to be public?
next prev parent reply other threads:[~2012-12-05 2:54 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-04 3:55 [PATCH] ARM: bcm281xx: Add timer driver Christian Daudt
2012-12-04 14:12 ` Arnd Bergmann
2012-12-05 2:54 ` Stephen Warren [this message]
2012-12-05 9:02 ` Christian Daudt
2012-12-05 15:50 ` Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50BEB77F.10303@wwwdotorg.org \
--to=swarren@wwwdotorg$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox