Hi,
On Mon, Mar 19, 2018 at 03:51:40AM +0800, Guo Ren wrote:
> +#define NC_VA_COUNTER_1_STATUS (void *)(timer_reg + 0x00)
> +#define NC_VA_COUNTER_1_VALUE (void *)(timer_reg + 0x04)
> +#define NC_VA_COUNTER_1_CONTROL (void *)(timer_reg + 0x10)
> +#define NC_VA_COUNTER_1_CONFIG (void *)(timer_reg + 0x20)
> +#define NC_VA_COUNTER_1_PRE (void *)(timer_reg + 0x24)
> +#define NC_VA_COUNTER_1_INI (void *)(timer_reg + 0x28)
> +#define NC_VA_COUNTER_2_STATUS (void *)(timer_reg + 0x40)
> +#define NC_VA_COUNTER_2_VALUE (void *)(timer_reg + 0x44)
> +#define NC_VA_COUNTER_2_CONTROL (void *)(timer_reg + 0x50)
> +#define NC_VA_COUNTER_2_CONFIG (void *)(timer_reg + 0x60)
> +#define NC_VA_COUNTER_2_PRE (void *)(timer_reg + 0x64)
> +#define NC_VA_COUNTER_2_INI (void *)(timer_reg + 0x68)
> +#define NC_VA_COUNTER_3_STATUS (void *)(timer_reg + 0x80)
> +#define NC_VA_COUNTER_3_VALUE (void *)(timer_reg + 0x84)
> +#define NC_VA_COUNTER_3_CONTROL (void *)(timer_reg + 0x90)
> +#define NC_VA_COUNTER_3_CONFIG (void *)(timer_reg + 0xa0)
> +#define NC_VA_COUNTER_3_PRE (void *)(timer_reg + 0xa4)
> +#define NC_VA_COUNTER_3_INI (void *)(timer_reg + 0xa8)
Please define the offsets alone, e.g.
#define NC_VA_COUNTER_1_STATUS 0x00
#define NC_VA_COUNTER_1_VALUE 0x04
... the base address should be added when calling the io accessor. Please see
below.
> +static unsigned int timer_reg;
This should be a void __iomem *, e.g.
static void __iomem *timer_base;
... though it would be better for this to be associated with the instance of
the clock_event_device, so that there can be multiple instances in a system.
> +
> +static inline void timer_reset(void)
> +{
> + __raw_writel(0x1, NC_VA_COUNTER_1_CONTROL);
> + __raw_writel(0x0, NC_VA_COUNTER_1_CONTROL);
> + __raw_writel(0x3, NC_VA_COUNTER_1_CONFIG);
> + __raw_writel(26, NC_VA_COUNTER_1_PRE);
Should this be 26 or 0x26?
It would be nice to have mnemonics for these magic numbers.
Please use writel_relaxed() rather than __raw_writel(). e.g.
writel_relaxed(0x1, timer_base + NC_VA_COUNTER_1_CONTROL);
writel_relaxed(0x0, timer_base + NC_VA_COUNTER_1_CONTROL);
writel_relaxed(0x3, timer_base + NC_VA_COUNTER_1_CONTROL);
writel_relaxed(26, timer_base + NC_VA_COUNTER_1_PRE);
[...]
> +static int __init nc_timer_init(struct device_node *np)
> +{
> + unsigned int irq;
> + unsigned int freq;
> +
> + /* parse from devicetree */
> + timer_reg = (unsigned int) of_iomap(np, 0);
> + if (!timer_reg)
> + panic("%s, of_iomap err.\n", __func__);
> +
> + irq = irq_of_parse_and_map(np, 0);
> + if (!irq)
> + panic("%s, irq_parse err.\n", __func__);
> +
> + if (of_property_read_u32(np, "clock-frequency", &freq))
> + panic("%s, clock-frequency error.\n", __func__);
> +
> + pr_info("Nationalchip Timer Init, reg: %x, irq: %d, freq: %d.\n",
> + timer_reg, irq, freq);
> +
> + /* setup irq */
> + if (request_irq(irq, timer_interrupt, IRQF_TIMER, np->name, &nc_ced))
> + panic("%s timer_interrupt error.\n", __func__);
> +
> + /* register */
> + clockevents_config_and_register(&nc_ced, freq, 1, ULONG_MAX);
> +
> + nc_csd_enable();
> + clocksource_mmio_init(NC_VA_COUNTER_2_VALUE, "nationalchip-clksource", freq, 200, 32, clocksource_mmio_readl_up);
> +
> + sched_clock_register(nc_sched_clock_read, 32, freq);
> +
> + return 0;
> +}
> +CLOCKSOURCE_OF_DECLARE(nc_timer, "nationalchip,timer-v1", nc_timer_init);
This needs a devicetree binding document. Please see Documentation/devicetree/bindings/submitting-patches.txt.
Thanks,
Mark
