x86/rtc: Allocate interrupt for platform device

Legacy RTC requires interrupt line 8 to be dedicated for it. On Intel MID platforms the legacy PIC is absent and in order to make RTC work we need to allocate interrupt separately.

Current solution brought by the commit

82a51c38f199 ("x86/platform/intel-mid: Enable RTC on Intel Merrifield")

does it in a wrong place,

and since it's done unconditionally for all
x86 devices, some of them, e.g. PNP based, might get it wrong -- at the beginning x86_platform.legacy.rtc flag is set for all x86 devices.

Move interrupt allocation to arch/x86/kernel/rtc.c module and allocate it for Intel MID platform devices only.

Fixes 82a51c38f199 ("x86/platform/intel-mid: Enable RTC on Intel Merrifield")

Cc Luis R. Rodriguez <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
- add headers and #ifdef to make kbuild bot happy
- re-test on PNP platform with acpi=off, thus, update patch accordingly

 arch/x86/kernel/rtc.c             | 21 +++++++++++++++++++++
 arch/x86/platform/intel-mid/sfi.c | 14 --------------
 2 files changed, 21 insertions(+), 14 deletions(-)

arch/x86/kernel/rtc.c +17 -0

index 5b21cb7d84d6..bf4cb88b9186 100644
@@ -12,7 +12,9 @@
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>
 #include <asm/time.h>
+#include <asm/hw_irq.h>
 #include <asm/intel-mid.h>
+#include <asm/io_apic.h>
 #include <asm/setup.h>
 
 #ifdef CONFIG_X86_32

@@ -155,6 +157,20 @@ void read_persistent_clock(struct timespec *ts)
 	x86_platform.get_wallclock(ts);
 }
 
+#ifdef CONFIG_X86_IO_APIC
+static __init int allocate_rtc_cmos_irq(void)
+{
+	struct irq_alloc_info info;
+
+	if (!intel_mid_identify_cpu())
+	       return 0;
+
+	ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
+	return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
+}
+#else
+static inline int allocate_rtc_cmos_irq(void) { return 0; }
+#endif
 
 static struct resource rtc_resources[] = {
 	[0] = {

@@ -178,6 +194,7 @@ static struct platform_device rtc_device = {
 
 static __init int add_rtc_cmos(void)
 {
+	int ret;
 #ifdef CONFIG_PNP
 	static const char * const ids[] __initconst =
 	    { "PNP0b00", "PNP0b01", "PNP0b02", };

Quoted 9 lines · Andy Shevchenko

Legacy RTC requires interrupt line 8 to be dedicated for it. On Intel MID platforms the legacy PIC is absent and in order to make RTC
work we need to allocate interrupt separately.

Current solution brought by the commit

  82a51c38f199 ("x86/platform/intel-mid: Enable RTC on Intel Merrifield")

Referring to commits on linux-next gitsums is not proper as they change
every day, so you might as well leave them out.

Quoted 1 line

does it in a wrong place,

There are other things wrong with it.. It had:

arch/x86/platform/intel-mid/mrfld.c +1 -0

index e0607c77a1bd..ae7bdeb0e507 100644
@@ -91,6 +91,7 @@ static unsigned long __init
 tangier_calibrate_tsc(void)
  static void __init tangier_arch_setup(void)
  {
         x86_platform.calibrate_tsc = tangier_calibrate_tsc;
+       x86_platform.legacy.rtc = 1;
  }

 /* tangier arch ops */

You want to set quirks by setting the x86_platform.set_legacy_features given
then on x86_early_init_platform_quirks() we have:

        if (x86_platform.set_legacy_features) x86_platform.set_legacy_features();

For example see xen_dom0_set_legacy_features().

From the description of that hook I didn't find it suitable in this

Quoted 5 lines

and since it's done unconditionally for all
x86 devices, some of them, e.g. PNP based, might get it wrong -- at the
beginning x86_platform.legacy.rtc flag is set for all x86 devices.

Doing it the above way would also make it a quirk only for the needed devices.

Quoted 44 lines

arch/x86/kernel/rtc.c +2 -0

@@ -12,7 +12,9 @@
  #include <asm/vsyscall.h>
  #include <asm/x86_init.h>
  #include <asm/time.h>
+#include <asm/hw_irq.h>
  #include <asm/intel-mid.h>
+#include <asm/io_apic.h>
  #include <asm/setup.h>

 #ifdef CONFIG_X86_32

arch/x86/kernel/rtc.c +0 -0

@@ -155,6 +157,20 @@ void read_persistent_clock(struct timespec *ts)
  	x86_platform.get_wallclock(ts);
  }

arch/x86/kernel/rtc.c +14 -0

+#ifdef CONFIG_X86_IO_APIC
+static __init int allocate_rtc_cmos_irq(void)
+{
+	struct irq_alloc_info info;
+
+	if (!intel_mid_identify_cpu())
+	       return 0;
+
+	ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
+	return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
+}
+#else
+static inline int allocate_rtc_cmos_irq(void) { return 0; }
+#endif

 static struct resource rtc_resources[] = {
  [0] = {

arch/x86/kernel/rtc.c +0 -0

@@ -178,6 +194,7 @@ static struct platform_device rtc_device = {

 static __init int add_rtc_cmos(void)
 {

arch/x86/kernel/rtc.c +1 -0

+	int ret;

 #ifdef CONFIG_PNP
  static const char * const ids[] __initconst = { "PNP0b00", "PNP0b01", "PNP0b02", };

arch/x86/kernel/rtc.c +0 -0

@@ -197,6 +214,10 @@ static __init int add_rtc_cmos(void)
  	if (!x86_platform.legacy.rtc)
  		return -ENODEV;

arch/x86/kernel/rtc.c

Quoted 5 lines

arch/x86/kernel/rtc.c +4 -0

+	ret = allocate_rtc_cmos_irq();
+	if (ret < 0)
+		return ret;
+

Ugh this is seriously ugly, can't we avoid this sort of thing with the callback and then let the internal MID code do what it needs?

Quoted 3 lines · Luis R. Rodriguez

Referring to commits on linux-next gitsums is not proper as they change
every day, so you might as well leave them out.

It's in maintainer's tree which has it the same. And most probably it would go the same (since commit includes timestamps). I never heard before of such issues (unless someone *rebased* tree for linux-next).

Quoted 9 lines

You want to set quirks by setting the x86_platform.set_legacy_features given
then on x86_early_init_platform_quirks() we have:

        if (x86_platform.set_legacy_features) x86_platform.set_legacy_features();

For example see xen_dom0_set_legacy_features().

Quoted 2 lines

Doing it the above way would also make it a quirk only for the needed devices.

...also I have no idea when exactly it will happen during initialization.

Quoted 28 lines

Quoted 28 lines

(quoted diff) +14 -0

+#ifdef CONFIG_X86_IO_APIC
+static __init int allocate_rtc_cmos_irq(void)
+{
+	struct irq_alloc_info info;
+
+	if (!intel_mid_identify_cpu())
+	       return 0;
+
+	ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
+	return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
+}
+#else
+static inline int allocate_rtc_cmos_irq(void) { return 0; }
+#endif

 static struct resource rtc_resources[] = {
  [0] = {

(quoted diff) +0 -0

@@ -178,6 +194,7 @@ static struct platform_device rtc_device = {

 static __init int add_rtc_cmos(void)
 {

(quoted diff) +1 -0

+	int ret;

 #ifdef CONFIG_PNP
  static const char * const ids[] __initconst = { "PNP0b00", "PNP0b01", "PNP0b02", };

(quoted diff) +0 -0

@@ -197,6 +214,10 @@ static __init int add_rtc_cmos(void)
  	if (!x86_platform.legacy.rtc)
  		return -ENODEV;

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+	ret = allocate_rtc_cmos_irq();
+	if (ret < 0)
+		return ret;
+

Ugh this is seriously ugly, can't we avoid this sort of thing with the callback and then let the internal MID code do what it needs?

I agree that's not nice looking piece of code, but this due to absence of some stubs. IOAPIC code is really old one and misses stuff (proper error codes, stubs for no IOAPIC case).

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Luis R. Rodriguez

Referring to commits on linux-next gitsums is not proper as they change
every day, so you might as well leave them out.

Quoted 3 lines

It's in maintainer's tree which has it the same. And most probably it would go the same (since commit includes timestamps). I never heard before of such issues (unless someone *rebased* tree for linux-next).

Which is obviously the case. tip x86/platform has:

de1c2540aa4f: x86/platform/intel-mid: Enable RTC on Intel Merrifield

Quoted 2 lines

...also I have no idea when exactly it will happen during initialization.

That's a non argument. You really can figure that out ....

Like you could have figured out that calling that function unconditially is not a brilliant idea....

Quoted 3 lines

I agree that's not nice looking piece of code, but this due to absence of some stubs. IOAPIC code is really old one and misses stuff (proper error codes, stubs for no IOAPIC case).

That's a fixable problem and in no way a justification for horrible hackery.

arch/x86/platform/intel-mid/sfi.c +4 -8

+               pr_info("Failed to allocate RTC interrupt. Disabling RTC\n");
+               x86_platform.legacy.rtc = 0;
+       }
+}
 
-static int __init intel_mid_legacy_rtc_init(void)
-{
-       struct irq_alloc_info info;
-
-       if (!x86_platform.legacy.rtc)
-               return -ENODEV;
-
-       ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);

By the way, would be better to revert or dismiss that commit at all?

The early callback does not work, but we have one which is invoked later
on: x86_init.wallclock_init(). That's invoked after the (IO/APIC) setup has
been completed. See patch below.

arch/x86/platform/intel-mid/intel-mid.c +7 -0

+static void __init intel_mid_legacy_rtc_init(void)
+{
+	struct irq_alloc_info info;
+
+	ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
+	if (mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info)) {
+		pr_info("Failed to allocate RTC interrupt. Disabling

RTC\n");

arch/x86/platform/intel-mid/intel-mid.c +4 -0

+		x86_platform.legacy.rtc = 0;
+	}
+}
+

 static void intel_mid_arch_setup(void)
 {
  if (boot_cpu_data.x86 != 6) {

arch/x86/platform/intel-mid/intel-mid.c +0 -0

@@ -151,6 +162,10 @@ static void intel_mid_arch_setup(void)
  	if (intel_mid_ops->arch_setup)
  		intel_mid_ops->arch_setup();

(quoted diff) +1 -0

+	/* If the platform has an RTC make sure the APIC entry is

allocated */

(quoted diff) +2 -0

+	if (x86_platform.legacy.rtc)
+		x86_init.timers.wallclock_init =

intel_mid_legacy_rtc_init;

Quoted 5 lines · Thomas Gleixner

The early callback does not work, but we have one which is invoked later
on: x86_init.wallclock_init(). That's invoked after the (IO/APIC) setup has
been completed. See patch below.

Unfortunately it is till too early. Looks like descriptors are not available yet and we still can't get an allocation:

[    0.000000] intel_mid: Failed to allocate RTC interrupt. Disabling RTC

...

[    0.000000] NR_IRQS:4352 nr_irqs:512 0

Quoted 11 lines · Andy Shevchenko

Quoted 5 lines · Thomas Gleixner

The early callback does not work, but we have one which is invoked later
on: x86_init.wallclock_init(). That's invoked after the (IO/APIC) setup has
been completed. See patch below.

Unfortunately it is till too early. Looks like descriptors are not available yet and we still can't get an allocation:

Quoted 7 lines

[    0.000000] intel_mid: Failed to allocate RTC interrupt. Disabling
RTC

...

[    0.000000] NR_IRQS:4352 nr_irqs:512 0

Indeed. Did not think about that we need the irq subsystem up not only the
primary IOAPIC init done.

Looking deeper it's actually simple. MID already overloads the timer_init()
setup function. So we can just do it there.

Quoted 3 lines · Thomas Gleixner

Looking deeper it's actually simple. MID already overloads the timer_init()
setup function. So we can just do it there.

Yes, it does. However I have another solution, just would like to discuss.

There is a timekeeping_init() call, which is a first user of the RTC. I have 3 changes:
- introduce arch_pre_timekeeping_init() and move wallclock_init() call there

- use almost your initial suggestion
- move wallclock_init() to x86_platform and rename to init_wallclock() to be consistent with the rest of wallclock API (this, though, has item to discuss, i.e. __init use for callbacks)

This would allow to clearly initialize virtual RTC or legacy one at the same know point.

Quoted 8 lines · Andy Shevchenko

Quoted 3 lines · Thomas Gleixner

Looking deeper it's actually simple. MID already overloads the timer_init()
setup function. So we can just do it there.

Yes, it does. However I have another solution, just would like to discuss.

Quoted 6 lines

There is a timekeeping_init() call, which is a first user of the RTC.
I have 3 changes:
- introduce arch_pre_timekeeping_init() and move wallclock_init() call
there

Oh no, please don't add yet another arch hook just because we can.

Quoted 7 lines

- use almost your initial suggestion
- move wallclock_init() to x86_platform and rename to init_wallclock()
to be consistent with the rest of wallclock API (this, though, has item
to discuss, i.e. __init use for callbacks)

Quoted 3 lines

This would allow to clearly initialize virtual RTC or legacy one at the
same know point.

What's wrong with setting up the rtc interrupt in that existing mid function? It's a platform quirk and we really do no need yet another hook
to make it look 'generic'. Nothing else than MID uses it.

Quoted 11 lines · Andy Shevchenko

Quoted 5 lines · Thomas Gleixner

The early callback does not work, but we have one which is invoked later
on: x86_init.wallclock_init(). That's invoked after the (IO/APIC) setup has
been completed. See patch below.

Unfortunately it is till too early. Looks like descriptors are not available yet and we still can't get an allocation:

Quoted 7 lines

[    0.000000] intel_mid: Failed to allocate RTC interrupt. Disabling
RTC

...

[    0.000000] NR_IRQS:4352 nr_irqs:512 0

Indeed. Did not think about that we need the irq subsystem up not only the
primary IOAPIC init done.

Looking deeper it's actually simple. MID already overloads the timer_init()
setup function. So we can just do it there.

On Wed, 2017-01-18 at 11:24 +0100, Thomas Gleixner wrote:

Another variant is to use just a separate callback on arch_initcall(). It would be closer to current solution.

If you have something better to try, please tell, I would test it.