Skip to contentSkip to conversation

RTC fixes

PATCH In review Bjorn Helgaas 2 participants 4 messages Oct 23, 2007 – Oct 24, 2007

Subsystems (2)
MIPS Char and misc drivers
lore
Share & tools
lore

Bjorn Helgaas

author
#0 lore raw

cover RTC fixes

Here are two fixes:

- Minor bugfix on error path for mips.
- If resource request fails, fall back to requesting only the ioports we actually use. This is not a problem yet, but it will be if the PNP core claims resources before drivers attach.

--

author
#1 lore raw

PATCH 1/2 rtc: release correct region in error path

The misc_register() error path always released an I/O port region, even if the region was memory-mapped (only mips uses memory-mapped RTC, as far as I can see).

Signed-off-by Bjorn Helgaas <[email protected]>
drivers/char/rtc.c +11 -9
Index: w/drivers/char/rtc.c
===================================================================
@@ -918,6 +918,14 @@
 };
 #endif
 
+static void rtc_release_region(void)
+{
+	if (RTC_IOMAPPED)
+		release_region(RTC_PORT(0), RTC_IO_EXTENT);
+	else
+		release_mem_region(RTC_PORT(0), RTC_IO_EXTENT);
+}
+
 static int __init rtc_init(void)
 {
 #ifdef CONFIG_PROC_FS

@@ -992,10 +1000,7 @@
 		/* Yeah right, seeing as irq 8 doesn't even hit the bus. */
 		rtc_has_irq = 0;
 		printk(KERN_ERR "rtc: IRQ %d is not free.\n", RTC_IRQ);
-		if (RTC_IOMAPPED)
-			release_region(RTC_PORT(0), RTC_IO_EXTENT);
-		else
-			release_mem_region(RTC_PORT(0), RTC_IO_EXTENT);
+		rtc_release_region();
 		return -EIO;
 	}
 	hpet_rtc_timer_init();

@@ -1009,7 +1014,7 @@
 		free_irq(RTC_IRQ, NULL);
 		rtc_has_irq = 0;
 #endif
-		release_region(RTC_PORT(0), RTC_IO_EXTENT);
+		rtc_release_region();
 		return -ENODEV;
 	}
 

@@ -1091,10 +1096,7 @@
 	if (rtc_has_irq)
 		free_irq (rtc_irq, &rtc_port);
 #else
-	if (RTC_IOMAPPED)
-		release_region(RTC_PORT(0), RTC_IO_EXTENT);
-	else
-		release_mem_region(RTC_PORT(0), RTC_IO_EXTENT);
+	rtc_release_region();
 #ifdef RTC_IRQ
 	if (rtc_has_irq)
 		free_irq (RTC_IRQ, NULL);

--

Acked-by on v1 1/2
#2 lore raw

Re: [patch 1/2] rtc: release correct region in error path

Quoted 5 lines · Bjorn Helgaas

Subject: [patch 1/2] rtc: release correct region in error path

The misc_register() error path always released an I/O port region, even if the region was memory-mapped (only mips uses memory-mapped RTC, as far as I can see).

Well, MIPS just like other sane architectures has no concept of ioports. So if there ever is something that's called an ioport on MIPS then it's really a memory location in a memory address range where other PCish devices are living, too. So typically those memory addresses are translated to an actual ioport access by a PCI bridge. There are a few systems where this concept just apply easily such as DECstations, so there we simply claim the device such as the RTC in this case is memory mapped.

Anway:

Acked-by Ralf Baechle <[email protected]>

Ralf

author
#3 lore raw

PATCH 2/2 rtc: fallback to requesting only the ports we actually use

In reply to Bjorn Helgaas

Firmware like PNPBIOS or ACPI can report the address space consumed by the RTC. The actual space consumed may be less than the size (RTC_IO_EXTENT) assumed by the RTC driver.

The PNP core doesn't request resources yet, but I'd like to make it do so. If/when it does, the RTC_IO_EXTENT request may fail, which prevents the RTC driver from loading.

Since we only use the RTC index and data registers at RTC_PORT(0) and RTC_PORT(1), we can fall back to requesting just enough space for those.

If the PNP core requests resources, this results in typical I/O port usage like this:

    0070-0073 : 00:06		<-- PNP device 00:06 responds to 70-73
      0070-0071 : rtc		<-- RTC driver uses only 70-71

instead of the current:

    0070-0077 : rtc		<-- RTC_IO_EXTENT == 8
Signed-off-by Bjorn Helgaas <[email protected]>
drivers/char/rtc.c +30 -6
Index: w/drivers/char/rtc.c
===================================================================
@@ -918,12 +918,29 @@
 };
 #endif
 
+static resource_size_t rtc_size;
+
+static struct resource * __init rtc_request_region(resource_size_t size)
+{
+	struct resource *r;
+
+	if (RTC_IOMAPPED)
+		r = request_region(RTC_PORT(0), size, "rtc");
+	else
+		r = request_mem_region(RTC_PORT(0), size, "rtc");
+
+	if (r)
+		rtc_size = size;
+
+	return r;
+}
+
 static void rtc_release_region(void)
 {
 	if (RTC_IOMAPPED)
-		release_region(RTC_PORT(0), RTC_IO_EXTENT);
+		release_region(RTC_PORT(0), rtc_size);
 	else
-		release_mem_region(RTC_PORT(0), RTC_IO_EXTENT);
+		release_mem_region(RTC_PORT(0), rtc_size);
 }
 
 static int __init rtc_init(void)

@@ -976,10 +993,17 @@
 	}
 no_irq:
 #else
-	if (RTC_IOMAPPED)
-		r = request_region(RTC_PORT(0), RTC_IO_EXTENT, "rtc");
-	else
-		r = request_mem_region(RTC_PORT(0), RTC_IO_EXTENT, "rtc");
+	r = rtc_request_region(RTC_IO_EXTENT);
+
+	/*
+	 * If we've already requested a smaller range (for example, because
+	 * PNPBIOS or ACPI told us how the device is configured), the request
+	 * above might fail because it's too big.
+	 *
+	 * If so, request just the range we actually use.
+	 */
+	if (!r)
+		r = rtc_request_region(RTC_IO_EXTENT_USED);
 	if (!r) {
 #ifdef RTC_IRQ
 		rtc_has_irq = 0;
include/linux/mc146818rtc.h +3 -0
Index: w/include/linux/mc146818rtc.h
===================================================================
@@ -109,8 +109,11 @@
 #ifndef ARCH_RTC_LOCATION	/* Override by <asm/mc146818rtc.h>? */
 
 #define RTC_IO_EXTENT	0x8
+#define RTC_IO_EXTENT_USED	0x2
 #define RTC_IOMAPPED	1	/* Default to I/O mapping. */
 
+#else
+#define RTC_IO_EXTENT_USED      RTC_IO_EXTENT
 #endif /* ARCH_RTC_LOCATION */
 
 #endif /* _MC146818RTC_H */

--