| Message ID | 1423005775-26457-3-git-send-email-robh@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tuesday 03 February 2015 17:22:53 Rob Herring wrote: > SA1100 and PXA differ only in register offsets which are currently > hardcoded in a machine specific header. Some arm64 platforms (PXA1928) > have this RTC block also. > > Convert the driver to use ioremap and set the register offsets dynamically. > Since we are touching all the register accesses, convert them all to > readl/writel. > > Signed-off-by: Rob Herring <robh@kernel.org> > Cc: Alessandro Zummo <a.zummo@towertech.it> > Cc: rtc-linux@googlegroups.com Hmm, I really should have sent out my version of the patch ages ago. :( Can you have a look at http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/commit/?h=multiplatform-3.19-next&id=4d9727883a363fb839e7d1633599166c0a08d644 to see compare the approaches and see if there is anything I did that you missed? Arnd
On Wed, Feb 4, 2015 at 7:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 03 February 2015 17:22:53 Rob Herring wrote: >> SA1100 and PXA differ only in register offsets which are currently >> hardcoded in a machine specific header. Some arm64 platforms (PXA1928) >> have this RTC block also. >> >> Convert the driver to use ioremap and set the register offsets dynamically. >> Since we are touching all the register accesses, convert them all to >> readl/writel. >> >> Signed-off-by: Rob Herring <robh@kernel.org> >> Cc: Alessandro Zummo <a.zummo@towertech.it> >> Cc: rtc-linux@googlegroups.com > > Hmm, I really should have sent out my version of the patch ages ago. :( Got any others? :) > > Can you have a look at > > http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/commit/?h=multiplatform-3.19-next&id=4d9727883a363fb839e7d1633599166c0a08d644 > > to see compare the approaches and see if there is anything I did that you > missed? You used the relaxed variants seems to be the main difference. Yours won't build on arm64 either. Rob
On Wed, Feb 4, 2015 at 7:44 AM, Rob Herring <robh@kernel.org> wrote: > On Wed, Feb 4, 2015 at 7:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Tuesday 03 February 2015 17:22:53 Rob Herring wrote: >>> SA1100 and PXA differ only in register offsets which are currently >>> hardcoded in a machine specific header. Some arm64 platforms (PXA1928) >>> have this RTC block also. >>> >>> Convert the driver to use ioremap and set the register offsets dynamically. >>> Since we are touching all the register accesses, convert them all to >>> readl/writel. >>> >>> Signed-off-by: Rob Herring <robh@kernel.org> >>> Cc: Alessandro Zummo <a.zummo@towertech.it> >>> Cc: rtc-linux@googlegroups.com >> >> Hmm, I really should have sent out my version of the patch ages ago. :( > > Got any others? :) > >> >> Can you have a look at >> >> http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/commit/?h=multiplatform-3.19-next&id=4d9727883a363fb839e7d1633599166c0a08d644 >> >> to see compare the approaches and see if there is anything I did that you >> missed? > > You used the relaxed variants seems to be the main difference. Yours > won't build on arm64 either. Also, I noticed the rtc-puv3 driver appears to be the same programming model. The quirks around interrupt clearing seem to be a bit different though. Rob
On Wednesday 04 February 2015 07:49:25 Rob Herring wrote: > On Wed, Feb 4, 2015 at 7:44 AM, Rob Herring <robh@kernel.org> wrote: > > On Wed, Feb 4, 2015 at 7:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >> On Tuesday 03 February 2015 17:22:53 Rob Herring wrote: > >>> SA1100 and PXA differ only in register offsets which are currently > >>> hardcoded in a machine specific header. Some arm64 platforms (PXA1928) > >>> have this RTC block also. > >>> > >>> Convert the driver to use ioremap and set the register offsets dynamically. > >>> Since we are touching all the register accesses, convert them all to > >>> readl/writel. > >>> > >>> Signed-off-by: Rob Herring <robh@kernel.org> > >>> Cc: Alessandro Zummo <a.zummo@towertech.it> > >>> Cc: rtc-linux@googlegroups.com > >> > >> Hmm, I really should have sent out my version of the patch ages ago. > > > > Got any others? Look at the whole branch, it converts a couple of platforms to CONFIG_MULTIPLATFORM: msm, mmp, realview, s3c64xx, versatile (your patches), ep93xx (partially), gemini. The patches are in different states and at least need proper review, in some cases rewrite. The mmp patches should be pretty much ready but I never got around to submit them. > >> Can you have a look at > >> > >> http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/commit/?h=multiplatform-3.19-next&id=4d9727883a363fb839e7d1633599166c0a08d644 > >> > >> to see compare the approaches and see if there is anything I did that you > >> missed? > > > > You used the relaxed variants seems to be the main difference. Yours > > won't build on arm64 either. > > Also, I noticed the rtc-puv3 driver appears to be the same programming > model. The quirks around interrupt clearing seem to be a bit different > though. Yes, I noticed before that not only is arch/unicore32 more or less a clone of arch/arm (aside from cleanups that I asked for when it was submitted), but the pkunity is also modeled around some IP blocks from sa1100/pxa. According to http://mprc.pku.edu.cn/eng/intro.html, the work on that SoC was done in a university lab with funding from Intel. The CPU instruction set apparently has some differences, and the same is likely true for the peripherals, it might be interesting to look at drivers/video/fbdev/fb-puv3.c, drivers/i2c/busses/i2c-puv3.c, drivers/pwm/pwm-puv3.c, arch/unicore32/kernel/gpio.c and arch/unicore32/kernel/irq.c for more similarities. I don't think anyone would bother changing those to use a common driver though, given the state of unicore32. Arnd
Rob Herring <robh@kernel.org> writes: Hi Rob, > SA1100 and PXA differ only in register offsets which are currently > hardcoded in a machine specific header. Some arm64 platforms (PXA1928) > have this RTC block also. > > Convert the driver to use ioremap and set the register offsets dynamically. > Since we are touching all the register accesses, convert them all to > readl/writel. A general comment, probably made by Arnd already, why not using the relaxed accessors ? ... zip ... > + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, iores); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + if (IS_ENABLED(CONFIG_ARCH_SA1100) || > + of_device_is_compatible(pdev->dev.of_node, "mrvl,sa1100-rtc")) { > + info->rcnr = base + 0x04; > + info->rtsr = base + 0x10; > + info->rtar = base + 0x00; > + info->rttr = base + 0x08; > + } else { > + info->rcnr = base + 0x0; > + info->rtsr = base + 0x8; > + info->rtar = base + 0x4; > + info->rttr = base + 0xc; > + } > + This is making me feel a bit uncomfortable. What if a single kernel is built for both SA1100 and PXA, and used in a PXA platform (as this drivers is also used in pxa2xx and pxa3xx) ? And just for your information, both rtc-sa1100 and rtc-pxa can be used at the same time in a pxa kernel. Apart from that worry it's quite nice. Cheers.
On Wed, Feb 4, 2015 at 11:30 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Rob Herring <robh@kernel.org> writes: > > Hi Rob, > >> SA1100 and PXA differ only in register offsets which are currently >> hardcoded in a machine specific header. Some arm64 platforms (PXA1928) >> have this RTC block also. >> >> Convert the driver to use ioremap and set the register offsets dynamically. >> Since we are touching all the register accesses, convert them all to >> readl/writel. > A general comment, probably made by Arnd already, why not using the relaxed > accessors ? I just default to writel/readl unless they are in a hot path as the the relaxed variants were not available on all architectures, but I think that is fixed now. I can switch them. > > ... zip ... > >> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + base = devm_ioremap_resource(&pdev->dev, iores); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + if (IS_ENABLED(CONFIG_ARCH_SA1100) || >> + of_device_is_compatible(pdev->dev.of_node, "mrvl,sa1100-rtc")) { >> + info->rcnr = base + 0x04; >> + info->rtsr = base + 0x10; >> + info->rtar = base + 0x00; >> + info->rttr = base + 0x08; >> + } else { >> + info->rcnr = base + 0x0; >> + info->rtsr = base + 0x8; >> + info->rtar = base + 0x4; >> + info->rttr = base + 0xc; >> + } >> + > This is making me feel a bit uncomfortable. What if a single kernel is built for > both SA1100 and PXA, and used in a PXA platform (as this drivers is also used in > pxa2xx and pxa3xx) ? If we ever support both simultaneously, then that implies we will be using DT. At that time we would need to remove "IS_ENABLED(CONFIG_ARCH_SA1100)" and the DT compatible strings determine which register layout. > And just for your information, both rtc-sa1100 and rtc-pxa can be used at the > same time in a pxa kernel. Yes, I know. I find that a bit odd. We'll have a bit of a problem supporting that with DT BTW. Rob > > Apart from that worry it's quite nice. > > Cheers. > > -- > Robert > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 05, 2015 at 07:34:50AM -0600, Rob Herring wrote: > > And just for your information, both rtc-sa1100 and rtc-pxa can be used at the > > same time in a pxa kernel. > > Yes, I know. I find that a bit odd. We'll have a bit of a problem > supporting that with DT BTW. I think you're close to that problem without DT anyway. You modify rtc-sa1100 to use devm_ioremap_resource(), which claims the memory resource exclusively, thus marking the memory region exclusive. Luckily, rtc-pxa uses devm_ioremap() without claiming the memory resource - which is the only saving grace for why it still works. If rtc-pxa were to be converted to use devm_ioremap_resource(), then they'll become mutually exclusive. Also note that by including the resource in rtc-sa1100's platform device resource list, you'll have stacked resources between the two platform devices appearing in /proc/iomem (you did look at that before posting the patches, right?)
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > On Thu, Feb 05, 2015 at 07:34:50AM -0600, Rob Herring wrote: >> > And just for your information, both rtc-sa1100 and rtc-pxa can be used at the >> > same time in a pxa kernel. >> >> Yes, I know. I find that a bit odd. We'll have a bit of a problem >> supporting that with DT BTW. > > I think you're close to that problem without DT anyway. You modify > rtc-sa1100 to use devm_ioremap_resource(), which claims the memory > resource exclusively, thus marking the memory region exclusive. Ah yes, I have not caught that one. > Luckily, rtc-pxa uses devm_ioremap() without claiming the memory > resource - which is the only saving grace for why it still works. > If rtc-pxa were to be converted to use devm_ioremap_resource(), then > they'll become mutually exclusive. True. Since the pxa-rtc begin ([1] and [2]), the idea was that both rtc-pxa and rtc-sa1100 could work in the same kernel, each handling a part of RTC functionality. Retrospectively, I think I made an error there, when I agreed the pxa2xx had 2 distinct RTC IPs. The sharing of ioresource as well of the IRQ should have made me decide it was a _single_ IP. And it's only a matter of time until rtc-pxa claims the resources too I think, probably by some device-tree push. As a consequence, I'm pretty much in favor of : - making rtc-pxa and rtc-sa1100 exclusive - adding to rtc-pxa driver : - a new rtc device (ie. it will register 2 rtc devices, one of the sa1100 kind, one of the pxa specific kind) This has flaws : - a bit of code will be duplicated between rtc-pxa and rtc-sa1100 - the ordering in rtc-pxa between the 2 rtc devices will be important, as if I remember correctly /dev/rtc points to the first /dev/rtc0 registered But it opens up : - DT path to both drivers - isolation between sa1100 architecture changes and pxa architecture changes - Rob's current patches can remain almost the same > Also note that by including the resource in rtc-sa1100's platform > device resource list, you'll have stacked resources between the two > platform devices appearing in /proc/iomem (you did look at that > before posting the patches, right?) I must admit I don't know if there are nasty consequences, I think I need to follow up the code to have a clear idea. Cheers. -- Robert A bit of history for sa1100/pxa-rtc split : [1] http://archive.arm.linux.org.uk/lurker/message/20081001.191833.4d220566.en.html [2] http://archive.arm.linux.org.uk/lurker/message/20081010.072816.93bd675d.en.html
On Thu, Feb 5, 2015 at 1:18 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > >> On Thu, Feb 05, 2015 at 07:34:50AM -0600, Rob Herring wrote: >>> > And just for your information, both rtc-sa1100 and rtc-pxa can be used at the >>> > same time in a pxa kernel. >>> >>> Yes, I know. I find that a bit odd. We'll have a bit of a problem >>> supporting that with DT BTW. >> >> I think you're close to that problem without DT anyway. You modify >> rtc-sa1100 to use devm_ioremap_resource(), which claims the memory >> resource exclusively, thus marking the memory region exclusive. > Ah yes, I have not caught that one. Yes, I was aware of this. >> Luckily, rtc-pxa uses devm_ioremap() without claiming the memory >> resource - which is the only saving grace for why it still works. >> If rtc-pxa were to be converted to use devm_ioremap_resource(), then >> they'll become mutually exclusive. > True. > > Since the pxa-rtc begin ([1] and [2]), the idea was that both rtc-pxa and > rtc-sa1100 could work in the same kernel, each handling a part of RTC > functionality. > > Retrospectively, I think I made an error there, when I agreed the pxa2xx had 2 > distinct RTC IPs. The sharing of ioresource as well of the IRQ should have made > me decide it was a _single_ IP. It is also shared registers. > And it's only a matter of time until rtc-pxa claims the resources too I think, > probably by some device-tree push. Blame DT for enforcing proper kernel driver design... ;) > As a consequence, I'm pretty much in favor of : > - making rtc-pxa and rtc-sa1100 exclusive +1 > - adding to rtc-pxa driver : > - a new rtc device (ie. it will register 2 rtc devices, one of the sa1100 > kind, one of the pxa specific kind) I fail to see what the rtc-pxa gets us? Something to do with which counter has correct time at boot? It has a rollover longer than ~126 years is all I see. Otherwise, it looks like over-engineered h/w to me. > This has flaws : > - a bit of code will be duplicated between rtc-pxa and rtc-sa1100 That can probably be mitigated with a common file of shared functions. > - the ordering in rtc-pxa between the 2 rtc devices will be important, as if I > remember correctly /dev/rtc points to the first /dev/rtc0 registered > > But it opens up : > - DT path to both drivers > - isolation between sa1100 architecture changes and pxa architecture changes > - Rob's current patches can remain almost the same > >> Also note that by including the resource in rtc-sa1100's platform >> device resource list, you'll have stacked resources between the two >> platform devices appearing in /proc/iomem (you did look at that >> before posting the patches, right?) > I must admit I don't know if there are nasty consequences, I think I need to > follow up the code to have a clear idea. I don't follow either. The resources don't appear unless requested and we only have 1 driver requesting it. Rob > Cheers. > > -- > Robert > > A bit of history for sa1100/pxa-rtc split : > [1] > http://archive.arm.linux.org.uk/lurker/message/20081001.191833.4d220566.en.html > [2] > http://archive.arm.linux.org.uk/lurker/message/20081010.072816.93bd675d.en.html
On Thu, Feb 05, 2015 at 08:18:05PM +0100, Robert Jarzmik wrote: > As a consequence, I'm pretty much in favor of : > - making rtc-pxa and rtc-sa1100 exclusive > - adding to rtc-pxa driver : > - a new rtc device (ie. it will register 2 rtc devices, one of the sa1100 > kind, one of the pxa specific kind) > > This has flaws : > - a bit of code will be duplicated between rtc-pxa and rtc-sa1100 > - the ordering in rtc-pxa between the 2 rtc devices will be important, as if I > remember correctly /dev/rtc points to the first /dev/rtc0 registered > > But it opens up : > - DT path to both drivers > - isolation between sa1100 architecture changes and pxa architecture changes > - Rob's current patches can remain almost the same Or you make one export a set of "library" calls which are referenced by the other driver, and only declare one platform device. Not _particularly_ nice but would avoid the code duplication issue (but has the benefit of not eating up module space, especially when the strict rw/execute protections are in effect.)
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > On Thu, Feb 05, 2015 at 08:18:05PM +0100, Robert Jarzmik wrote: >> As a consequence, I'm pretty much in favor of : >> - making rtc-pxa and rtc-sa1100 exclusive >> - adding to rtc-pxa driver : >> - a new rtc device (ie. it will register 2 rtc devices, one of the sa1100 >> kind, one of the pxa specific kind) >> >> This has flaws : >> - a bit of code will be duplicated between rtc-pxa and rtc-sa1100 >> - the ordering in rtc-pxa between the 2 rtc devices will be important, as if I >> remember correctly /dev/rtc points to the first /dev/rtc0 registered >> >> But it opens up : >> - DT path to both drivers >> - isolation between sa1100 architecture changes and pxa architecture changes >> - Rob's current patches can remain almost the same > > Or you make one export a set of "library" calls which are referenced > by the other driver, and only declare one platform device. Not > _particularly_ nice but would avoid the code duplication issue (but > has the benefit of not eating up module space, especially when the > strict rw/execute protections are in effect.) Or even better only export sa1100_rtc_ops, which should be enough, with : - Kconfig will have a "select RTC_SRV_SA1100" in the "config RTC_DRV_PXA". - struct sa1100_rtc_ops won't be static anymore, it should be exported - amend rtc-pxa : - the probe/release - the interrupt handler should take into account one more bit That will imply that rtc-pxa is one platform device, which registers 2 rtc devices. rtc-sa1100 is another platform device. And both will be exclusive. Yet I don't see the "not eating up module space", unless all sa1100 functions are exported into a "rtc-sa1100-lib.c", which will always be built builtin into the kernel when CONFIG_RTC is selected. If there are still 2 modules, rtc-pxa and rtc-sa1100, they'll both eat module space, unless I'm missing something. Cheers.
Rob Herring <robh@kernel.org> writes: >> - adding to rtc-pxa driver : >> - a new rtc device (ie. it will register 2 rtc devices, one of the sa1100 >> kind, one of the pxa specific kind) > > I fail to see what the rtc-pxa gets us? Something to do with which > counter has correct time at boot? It has a rollover longer than ~126 > years is all I see. Otherwise, it looks like over-engineered h/w to > me. Compatibility with other OSes, for multi-OS boots, which is the main reason rtc-pxa was developped. Of course the other OS is using the other registers, *and* resets RCNR to 0 on boot. >> This has flaws : >> - a bit of code will be duplicated between rtc-pxa and rtc-sa1100 > > That can probably be mitigated with a common file of shared functions. Oh yeah, I like that. Would it mean that in the future you'd have time to help with that task ? >> - the ordering in rtc-pxa between the 2 rtc devices will be important, as if I >> remember correctly /dev/rtc points to the first /dev/rtc0 registered >> >> But it opens up : >> - DT path to both drivers >> - isolation between sa1100 architecture changes and pxa architecture changes >> - Rob's current patches can remain almost the same >> >>> Also note that by including the resource in rtc-sa1100's platform >>> device resource list, you'll have stacked resources between the two >>> platform devices appearing in /proc/iomem (you did look at that >>> before posting the patches, right?) >> I must admit I don't know if there are nasty consequences, I think I need to >> follow up the code to have a clear idea. > > I don't follow either. The resources don't appear unless requested and > we only have 1 driver requesting it. There's only one small test to do to understand, as soon as I'm finished with my zylonite's ethernet interrupts, I will make the test to understand what Russell said, I might learn something in the test :) Cheers.
Rob Herring <robh@kernel.org> writes: > SA1100 and PXA differ only in register offsets which are currently > hardcoded in a machine specific header. Some arm64 platforms (PXA1928) > have this RTC block also. > > Convert the driver to use ioremap and set the register offsets dynamically. > Since we are touching all the register accesses, convert them all to > readl/writel. All right Rob, to conclude this discussion, provided you add to the git comment what we discussed, ie. that this patch opens the path to exclusive runtime usage between rtc-sa1100 and rtc-pxa on pxa, I'll ack it. If somebody complains, I'll tell him to develop the glue between sa1100 and pxa as Russell suggested, or use only one of the drivers. Cheers. -- Robert
diff --git a/drivers/rtc/rtc-sa1100.c b/drivers/rtc/rtc-sa1100.c index b6e1ca0..fb9fa8e 100644 --- a/drivers/rtc/rtc-sa1100.c +++ b/drivers/rtc/rtc-sa1100.c @@ -35,12 +35,10 @@ #include <linux/bitops.h> #include <linux/io.h> -#include <mach/hardware.h> -#include <mach/irqs.h> - -#if defined(CONFIG_ARCH_PXA) || defined(CONFIG_ARCH_MMP) -#include <mach/regs-rtc.h> -#endif +#define RTSR_HZE (1 << 3) /* HZ interrupt enable */ +#define RTSR_ALE (1 << 2) /* RTC alarm interrupt enable */ +#define RTSR_HZ (1 << 1) /* HZ rising-edge detected */ +#define RTSR_AL (1 << 0) /* RTC alarm detected */ #define RTC_DEF_DIVIDER (32768 - 1) #define RTC_DEF_TRIM 0 @@ -48,6 +46,10 @@ struct sa1100_rtc { spinlock_t lock; + void __iomem *rcnr; + void __iomem *rtar; + void __iomem *rtsr; + void __iomem *rttr; int irq_1hz; int irq_alarm; struct rtc_device *rtc; @@ -63,16 +65,16 @@ static irqreturn_t sa1100_rtc_interrupt(int irq, void *dev_id) spin_lock(&info->lock); - rtsr = RTSR; + rtsr = readl(info->rtsr); /* clear interrupt sources */ - RTSR = 0; + writel(0, info->rtsr); /* Fix for a nasty initialization problem the in SA11xx RTSR register. * See also the comments in sa1100_rtc_probe(). */ if (rtsr & (RTSR_ALE | RTSR_HZE)) { /* This is the original code, before there was the if test * above. This code does not clear interrupts that were not * enabled. */ - RTSR = (RTSR_AL | RTSR_HZ) & (rtsr >> 2); + writel((RTSR_AL | RTSR_HZ) & (rtsr >> 2), info->rtsr); } else { /* For some reason, it is possible to enter this routine * without interruptions enabled, it has been tested with @@ -81,13 +83,13 @@ static irqreturn_t sa1100_rtc_interrupt(int irq, void *dev_id) * This situation leads to an infinite "loop" of interrupt * routine calling and as a result the processor seems to * lock on its first call to open(). */ - RTSR = RTSR_AL | RTSR_HZ; + writel(RTSR_AL | RTSR_HZ, info->rtsr); } /* clear alarm interrupt if it has occurred */ if (rtsr & RTSR_AL) rtsr &= ~RTSR_ALE; - RTSR = rtsr & (RTSR_ALE | RTSR_HZE); + writel(rtsr & (RTSR_ALE | RTSR_HZE), info->rtsr); /* update irq data & counter */ if (rtsr & RTSR_AL) @@ -135,7 +137,7 @@ static void sa1100_rtc_release(struct device *dev) struct sa1100_rtc *info = dev_get_drvdata(dev); spin_lock_irq(&info->lock); - RTSR = 0; + writel(0, info->rtsr); spin_unlock_irq(&info->lock); free_irq(info->irq_alarm, dev); @@ -144,39 +146,45 @@ static void sa1100_rtc_release(struct device *dev) static int sa1100_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) { + u32 rtsr; struct sa1100_rtc *info = dev_get_drvdata(dev); spin_lock_irq(&info->lock); + rtsr = readl(info->rtsr); if (enabled) - RTSR |= RTSR_ALE; + rtsr |= RTSR_ALE; else - RTSR &= ~RTSR_ALE; + rtsr &= ~RTSR_ALE; + writel(rtsr, info->rtsr); spin_unlock_irq(&info->lock); return 0; } static int sa1100_rtc_read_time(struct device *dev, struct rtc_time *tm) { - rtc_time_to_tm(RCNR, tm); + struct sa1100_rtc *info = dev_get_drvdata(dev); + rtc_time_to_tm(readl(info->rcnr), tm); return 0; } static int sa1100_rtc_set_time(struct device *dev, struct rtc_time *tm) { + struct sa1100_rtc *info = dev_get_drvdata(dev); unsigned long time; int ret; ret = rtc_tm_to_time(tm, &time); if (ret == 0) - RCNR = time; + writel(time, info->rcnr); return ret; } static int sa1100_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) { u32 rtsr; + struct sa1100_rtc *info = dev_get_drvdata(dev); - rtsr = RTSR; + rtsr = readl(info->rtsr); alrm->enabled = (rtsr & RTSR_ALE) ? 1 : 0; alrm->pending = (rtsr & RTSR_AL) ? 1 : 0; return 0; @@ -192,12 +200,12 @@ static int sa1100_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) ret = rtc_tm_to_time(&alrm->time, &time); if (ret != 0) goto out; - RTSR = RTSR & (RTSR_HZE|RTSR_ALE|RTSR_AL); - RTAR = time; + writel(readl(info->rtsr) & (RTSR_HZE | RTSR_ALE | RTSR_AL), info->rtsr); + writel(time, info->rtar); if (alrm->enabled) - RTSR |= RTSR_ALE; + writel(readl(info->rtsr) | RTSR_ALE, info->rtsr); else - RTSR &= ~RTSR_ALE; + writel(readl(info->rtsr) & ~RTSR_ALE, info->rtsr); out: spin_unlock_irq(&info->lock); @@ -206,8 +214,9 @@ out: static int sa1100_rtc_proc(struct device *dev, struct seq_file *seq) { - seq_printf(seq, "trim/divider\t\t: 0x%08x\n", (u32) RTTR); - seq_printf(seq, "RTSR\t\t\t: 0x%08x\n", (u32)RTSR); + struct sa1100_rtc *info = dev_get_drvdata(dev); + seq_printf(seq, "trim/divider\t\t: 0x%08x\n", readl(info->rttr)); + seq_printf(seq, "RTSR\t\t\t: 0x%08x\n", readl(info->rtsr)); return 0; } @@ -227,6 +236,8 @@ static int sa1100_rtc_probe(struct platform_device *pdev) { struct rtc_device *rtc; struct sa1100_rtc *info; + struct resource *iores; + void __iomem *base; int irq_1hz, irq_alarm, ret = 0; irq_1hz = platform_get_irq_byname(pdev, "rtc 1Hz"); @@ -244,6 +255,26 @@ static int sa1100_rtc_probe(struct platform_device *pdev) } info->irq_1hz = irq_1hz; info->irq_alarm = irq_alarm; + + + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); + base = devm_ioremap_resource(&pdev->dev, iores); + if (IS_ERR(base)) + return PTR_ERR(base); + + if (IS_ENABLED(CONFIG_ARCH_SA1100) || + of_device_is_compatible(pdev->dev.of_node, "mrvl,sa1100-rtc")) { + info->rcnr = base + 0x04; + info->rtsr = base + 0x10; + info->rtar = base + 0x00; + info->rttr = base + 0x08; + } else { + info->rcnr = base + 0x0; + info->rtsr = base + 0x8; + info->rtar = base + 0x4; + info->rttr = base + 0xc; + } + spin_lock_init(&info->lock); platform_set_drvdata(pdev, info); @@ -257,12 +288,12 @@ static int sa1100_rtc_probe(struct platform_device *pdev) * If the clock divider is uninitialized then reset it to the * default value to get the 1Hz clock. */ - if (RTTR == 0) { - RTTR = RTC_DEF_DIVIDER + (RTC_DEF_TRIM << 16); + if (readl(info->rttr) == 0) { + writel(RTC_DEF_DIVIDER + (RTC_DEF_TRIM << 16), info->rttr); dev_warn(&pdev->dev, "warning: " "initializing default clock divider/trim value\n"); /* The current RTC value probably doesn't make sense either */ - RCNR = 0; + writel(0, info->rcnr); } device_init_wakeup(&pdev->dev, 1); @@ -298,7 +329,7 @@ static int sa1100_rtc_probe(struct platform_device *pdev) * * Notice that clearing bit 1 and 0 is accomplished by writting ONES to * the corresponding bits in RTSR. */ - RTSR = RTSR_AL | RTSR_HZ; + writel(RTSR_AL | RTSR_HZ, info->rtsr); return 0; err_dev:
SA1100 and PXA differ only in register offsets which are currently hardcoded in a machine specific header. Some arm64 platforms (PXA1928) have this RTC block also. Convert the driver to use ioremap and set the register offsets dynamically. Since we are touching all the register accesses, convert them all to readl/writel. Signed-off-by: Rob Herring <robh@kernel.org> Cc: Alessandro Zummo <a.zummo@towertech.it> Cc: rtc-linux@googlegroups.com --- drivers/rtc/rtc-sa1100.c | 85 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 58 insertions(+), 27 deletions(-)