[RFC PATCH 1/2] ARM: CSR: add rtc i/o bridge interface for SiRFprimaII

[Barry Song]

Hi Jamie,
Thanks.

2011/8/5 Jamie Iles <jamie at jamieiles.com>:
> Hi Barry, Zhiwu,
>
> A couple of minor comments inline.
>
> Jamie
>
> On Thu, Aug 04, 2011 at 07:54:47PM -0700, Barry Song wrote:
>> From: Zhiwu Song <zhiwu.song at csr.com>
>>
>> The module is a bridge between the RTC clock domain and the CPU interface
>> clock domain. ARM access the register of SYSRTC, GPSRTC and PWRC through
>> this module.
>>
>> Signed-off-by: Zhiwu Song <zhiwu.song at csr.com>
>> Signed-off-by: Barry Song <baohua.song at csr.com>
>> ---
>>  arch/arm/mach-prima2/Makefile        |    1 +
>>  arch/arm/mach-prima2/rtciobrg.c      |  118 ++++++++++++++++++++++++++++++++++
>>  include/linux/rtc/sirfsoc_rtciobrg.h |   18 +++++
>>  3 files changed, 137 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/mach-prima2/rtciobrg.c
>>  create mode 100644 include/linux/rtc/sirfsoc_rtciobrg.h
>>
>> diff --git a/arch/arm/mach-prima2/Makefile b/arch/arm/mach-prima2/Makefile
>> index 7af7fc0..f49d70b 100644
>> --- a/arch/arm/mach-prima2/Makefile
>> +++ b/arch/arm/mach-prima2/Makefile
>> @@ -3,5 +3,6 @@ obj-y += irq.o
>>  obj-y += clock.o
>>  obj-y += rstc.o
>>  obj-y += prima2.o
>> +obj-y += rtciobrg.o
>>  obj-$(CONFIG_DEBUG_LL) += lluart.o
>>  obj-$(CONFIG_CACHE_L2X0) += l2x0.o
>> diff --git a/arch/arm/mach-prima2/rtciobrg.c b/arch/arm/mach-prima2/rtciobrg.c
>> new file mode 100644
>> index 0000000..0dc29f8
>> --- /dev/null
>> +++ b/arch/arm/mach-prima2/rtciobrg.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + * RTC I/O Bridge interfaces for CSR SiRFprimaII
>> + * ARM access the registers of SYSRTC, GPSRTC and PWRC through this module
>> + *
>> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
>> + *
>> + * Licensed under GPLv2 or later.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +
>> +#define SIRFSOC_CPUIOBRG_CTRL           0x00
>> +#define SIRFSOC_CPUIOBRG_WRBE           0x04
>> +#define SIRFSOC_CPUIOBRG_ADDR           0x08
>> +#define SIRFSOC_CPUIOBRG_DATA           0x0c
>> +
>> +void __iomem *sirfsoc_rtciobrg_base;
>
> static void __iomem?

this var will be accessed by sleep.S in patch 2/2.

>
>> +static DEFINE_SPINLOCK(rtciobrg_lock);
>> +
>> +void sirfsoc_rtc_iobrg_wait_sync(void)
>> +{
>> +     while (readl(sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_CTRL))
>> +             cpu_relax();
>> +}
>
> I think this can be static too.

this function will be accessed by sleep.S too.

>
>> +
>> +void sirfsoc_rtc_iobrg_besyncing(void)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&rtciobrg_lock, flags);
>> +
>> +     sirfsoc_rtc_iobrg_wait_sync();
>> +
>> +     spin_unlock_irqrestore(&rtciobrg_lock, flags);
>> +}
>> +EXPORT_SYMBOL(sirfsoc_rtc_iobrg_besyncing);
>> +
>> +u32 __sirfsoc_rtc_iobrg_readl(u32 addr)
>> +{
>
> Maybe for clarity have offset rather than address for these accessors to
> indicate the nature of their use?

it should be an address not an offset. the address is local address
againest rtciobrg. it is not an address in memory bus againest CPU
core.
if it is an offset, than its memory address is:
MEMORY_BASE + addr.

but here its address is addr in local bus.

>
>> +     unsigned long val;
>> +
>> +     sirfsoc_rtc_iobrg_wait_sync();
>> +
>> +     writel(0x00, sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_WRBE);
>> +     writel(addr, sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_ADDR);
>> +     writel(0x01, sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_CTRL);
>> +
>> +     sirfsoc_rtc_iobrg_wait_sync();
>> +
>> +     val = readl(sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_DATA);
>> +
>> +     return val;
>> +}
>
> Static again?  Also you could just do "return readl(..)" to eliminate
> the local variable.

sleep.S call it.

>
>> +u32 sirfsoc_rtc_iobrg_readl(u32 addr)
>> +{
>> +     unsigned long flags, val;
>> +
>> +     spin_lock_irqsave(&rtciobrg_lock, flags);
>> +
>> +     val = __sirfsoc_rtc_iobrg_readl(addr);
>> +
>> +     spin_unlock_irqrestore(&rtciobrg_lock, flags);
>> +
>> +     return val;
>> +}
>> +EXPORT_SYMBOL(sirfsoc_rtc_iobrg_readl);
>> +
>> +void sirfsoc_rtc_iobrg_pre_writel(u32 val, u32 addr)
>> +{
>> +     sirfsoc_rtc_iobrg_wait_sync();
>> +
>> +     writel(0xf1, sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_WRBE);
>> +     writel(addr, sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_ADDR);
>> +
>> +     writel(val, sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_DATA);
>> +}
>
> Static?
>
>> +
>> +void sirfsoc_rtc_iobrg_writel(u32 val, u32 addr)
>> +{
>> +     unsigned long flags = 0;
>
> Initializer not needed.

agree.

>
>> +     spin_lock_irqsave(&rtciobrg_lock, flags);
>> +
>> +     sirfsoc_rtc_iobrg_pre_writel(val, addr);
>> +
>> +     writel(0x01, sirfsoc_rtciobrg_base + SIRFSOC_CPUIOBRG_CTRL);
>> +
>> +     sirfsoc_rtc_iobrg_wait_sync();
>> +
>> +     spin_unlock_irqrestore(&rtciobrg_lock, flags);
>> +}
>> +EXPORT_SYMBOL(sirfsoc_rtc_iobrg_writel);
>> +
>> +static struct of_device_id rtciobrg_ids[] = {
>> +     { .compatible = "sirf,prima2-rtciobg" },
>> +};
>
> Missing an end of list {} sentinel?

agree.

>
>> +
>> +static int __init sirfsoc_of_rtciobrg_map(void)
>> +{
>> +     struct device_node *np;
>> +
>> +     np = of_find_matching_node(NULL, rtciobrg_ids);
>> +     if (!np)
>> +             panic("unable to find compatible rtc iobrg node in dtb\n");
>> +     sirfsoc_rtciobrg_base = of_iomap(np, 0);
>> +     if (!sirfsoc_rtciobrg_base)
>> +             panic("unable to map rtc iobrg registers\n");
>> +
>> +     of_node_put(np);
>> +
>> +     return 0;
>> +}
>> +early_initcall(sirfsoc_of_rtciobrg_map);
>> diff --git a/include/linux/rtc/sirfsoc_rtciobrg.h b/include/linux/rtc/sirfsoc_rtciobrg.h
>> new file mode 100644
>> index 0000000..2c92e1c
>> --- /dev/null
>> +++ b/include/linux/rtc/sirfsoc_rtciobrg.h
>> @@ -0,0 +1,18 @@
>> +/*
>> + * RTC I/O Bridge interfaces for CSR SiRFprimaII
>> + * ARM access the registers of SYSRTC, GPSRTC and PWRC through this module
>> + *
>> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
>> + *
>> + * Licensed under GPLv2 or later.
>> + */
>> +#ifndef _SIRFSOC_RTC_IOBRG_H_
>> +#define _SIRFSOC_RTC_IOBRG_H_
>> +
>> +extern void sirfsoc_rtc_iobrg_besyncing(void);
>> +
>> +extern u32 sirfsoc_rtc_iobrg_readl(u32 addr);
>> +
>> +extern void sirfsoc_rtc_iobrg_writel(u32 val, u32 addr);
>> +
>> +#endif
>> --
>> 1.7.1
>>
>>
-barry