Patchwork x86: enable rtc-efi

login
register
mail settings
Submitter Jan Beulich
Date July 5, 2011, 7:38 a.m.
Message ID <4E12DBAB020000780004C1B2@nat28.tlf.novell.com>
Download mbox | patch
Permalink /patch/103207/
State New
Headers show

Comments

Jan Beulich - July 5, 2011, 7:38 a.m.
Besides a Kconfig change this just requires creating a respective
platform device.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: dann frazier <dannf@hp.com>

---
 arch/x86/platform/efi/efi.c |   16 ++++++++++++++++
 drivers/rtc/Kconfig         |    2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)
Matthew Garrett - July 5, 2011, 6:02 p.m.
On Tue, Jul 05, 2011 at 08:38:51AM +0100, Jan Beulich wrote:
> Besides a Kconfig change this just requires creating a respective
> platform device.

"During runtime, if a PC-AT CMOS device is present in the platform the 
caller must synchronize access to the device before calling GetTime()."

This shouldn't be enabled on PC hardware without locking to avoid races 
with direct ahrdware access.
Matthew Garrett - July 5, 2011, 6:29 p.m.
On Tue, Jul 05, 2011 at 08:38:51AM +0100, Jan Beulich wrote:
> Besides a Kconfig change this just requires creating a respective
> platform device.

Couple of other issues:

> +static struct platform_device rtc_efi_dev = {
> +	.name = "rtc-efi",
> +	.id = -1,
> +};

You haven't removed this from the IA64 code. Isn't that going to result 
in registering the same device twice?

> +static int __init rtc_init(void)
> +{
> +	if (efi_enabled && platform_device_register(&rtc_efi_dev) < 0)
> +		printk(KERN_ERR "unable to register rtc device...\n");

Ought to -ENODEV if efi isn't present? The error message should also be 
more verbose.
Jan Beulich - July 6, 2011, 7:09 a.m.
>>> On 05.07.11 at 20:29, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jul 05, 2011 at 08:38:51AM +0100, Jan Beulich wrote:
>> Besides a Kconfig change this just requires creating a respective
>> platform device.
> 
> Couple of other issues:
> 
>> +static struct platform_device rtc_efi_dev = {
>> +	.name = "rtc-efi",
>> +	.id = -1,
>> +};
> 
> You haven't removed this from the IA64 code. Isn't that going to result 
> in registering the same device twice?

How would code under arch/ia64/ and code under arch/x86/ ever
manage to register the same device twice?

>> +static int __init rtc_init(void)
>> +{
>> +	if (efi_enabled && platform_device_register(&rtc_efi_dev) < 0)
>> +		printk(KERN_ERR "unable to register rtc device...\n");
> 
> Ought to -ENODEV if efi isn't present? The error message should also be 
> more verbose.

I would agree on the -ENODEV part, but the error message is what
the original ia64 code (which I simply copied over and modified
slightly) has, so if a change is needed it ought to be done there first
I would think. Additionally I can't really see what meaningful addition
to the error message you envision.

Jan
Matthew Garrett - July 6, 2011, 11:42 a.m.
On Wed, Jul 06, 2011 at 08:09:45AM +0100, Jan Beulich wrote:
> >>> On 05.07.11 at 20:29, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > You haven't removed this from the IA64 code. Isn't that going to result 
> > in registering the same device twice?
> 
> How would code under arch/ia64/ and code under arch/x86/ ever
> manage to register the same device twice?

Gah. Sorry, I misread and thought thsi was in the generic code too.

> >> +static int __init rtc_init(void)
> >> +{
> >> +	if (efi_enabled && platform_device_register(&rtc_efi_dev) < 0)
> >> +		printk(KERN_ERR "unable to register rtc device...\n");
> > 
> > Ought to -ENODEV if efi isn't present? The error message should also be 
> > more verbose.
> 
> I would agree on the -ENODEV part, but the error message is what
> the original ia64 code (which I simply copied over and modified
> slightly) has, so if a change is needed it ought to be done there first
> I would think. Additionally I can't really see what meaningful addition
> to the error message you envision.

EFI is the only way to access the rtc on IA64, whereas on x86 you'll 
almost certainly also have direct hardware access as well. Make it clear 
which rtc device you're unable to register.

Patch

--- 3.0-rc6/arch/x86/platform/efi/efi.c
+++ 3.0-rc6-x86-EFI-RTC/arch/x86/platform/efi/efi.c
@@ -31,6 +31,7 @@ 
 #include <linux/efi.h>
 #include <linux/bootmem.h>
 #include <linux/memblock.h>
+#include <linux/platform_device.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
 #include <linux/time.h>
@@ -679,6 +680,21 @@  void __init efi_enter_virtual_mode(void)
 	kfree(new_memmap);
 }
 
+static struct platform_device rtc_efi_dev = {
+	.name = "rtc-efi",
+	.id = -1,
+};
+
+static int __init rtc_init(void)
+{
+	if (efi_enabled && platform_device_register(&rtc_efi_dev) < 0)
+		printk(KERN_ERR "unable to register rtc device...\n");
+
+	/* not necessarily an error */
+	return 0;
+}
+arch_initcall(rtc_init);
+
 /*
  * Convenience functions to obtain memory types and attributes
  */
--- 3.0-rc6/drivers/rtc/Kconfig
+++ 3.0-rc6-x86-EFI-RTC/drivers/rtc/Kconfig
@@ -556,7 +556,7 @@  config RTC_DRV_DS1742
 
 config RTC_DRV_EFI
 	tristate "EFI RTC"
-	depends on IA64
+	depends on EFI
 	help
 	  If you say yes here you will get support for the EFI
 	  Real Time Clock.