[PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[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")

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 <mcgrof@kernel.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
- 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(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 5b21cb7d84d6..bf4cb88b9186 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -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", };
@@ -197,6 +214,10 @@ static __init int add_rtc_cmos(void)
 	if (!x86_platform.legacy.rtc)
 		return -ENODEV;
 
+	ret = allocate_rtc_cmos_irq();
+	if (ret < 0)
+		return ret;
+
 	platform_device_register(&rtc_device);
 	dev_info(&rtc_device.dev,
 		 "registered platform RTC device (no PNP device found)\n");
diff --git a/arch/x86/platform/intel-mid/sfi.c b/arch/x86/platform/intel-mid/sfi.c
index e4d4cabbb370..19b43e3a9f0f 100644
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -41,7 +41,6 @@
 #include <asm/intel_scu_ipc.h>
 #include <asm/apb_timer.h>
 #include <asm/reboot.h>
-#include <asm/time.h>
 
 #define	SFI_SIG_OEM0	"OEM0"
 #define MAX_IPCDEVS	24
@@ -540,21 +539,8 @@ static int __init sfi_parse_devs(struct sfi_table_header *table)
 	return 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);
-	return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
-}
-
 static int __init intel_mid_platform_init(void)
 {
-	intel_mid_legacy_rtc_init();
-
 	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
 	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
 	return 0;
-- 
2.11.0

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Luis R. Rodriguez]

On Mon, Jan 16, 2017 at 07:23:45PM +0200, Andy Shevchenko wrote:
> 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.

> does it in a wrong place, 

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

diff --git a/arch/x86/platform/intel-mid/mrfld.c b/arch/x86/platform/intel-mid/mrfld.c
index e0607c77a1bd..ae7bdeb0e507 100644
--- a/arch/x86/platform/intel-mid/mrfld.c
+++ b/arch/x86/platform/intel-mid/mrfld.c
@@ -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().

> 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.

> 
> 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 <mcgrof@kernel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> - 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(-)
> 
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 5b21cb7d84d6..bf4cb88b9186 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -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", };
> @@ -197,6 +214,10 @@ static __init int add_rtc_cmos(void)
>  	if (!x86_platform.legacy.rtc)
>  		return -ENODEV;
>  
> +	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?

   Luis

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Andy Shevchenko]

On Mon, 2017-01-16 at 20:04 +0100, Luis R. Rodriguez wrote:
> On Mon, Jan 16, 2017 at 07:23:45PM +0200, Andy Shevchenko wrote:
> > 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")
> 

Thanks for review, my comments/answers below.

> 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).

> > does it in a wrong place, 
> 
> There are other things wrong with it.. It had:
> 
> diff --git a/arch/x86/platform/intel-mid/mrfld.c
> b/arch/x86/platform/intel-mid/mrfld.c
> index e0607c77a1bd..ae7bdeb0e507 100644
> --- a/arch/x86/platform/intel-mid/mrfld.c
> +++ b/arch/x86/platform/intel-mid/mrfld.c
> @@ -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().

Why? What's wrong with arch setup code?
>From the description of that hook I didn't find it suitable in this
case.

> 
> > 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.

...also I have no idea when exactly it will happen during
initialization. The RTC setup is kinda sensitive to what platform might
or might not have.

> > --- a/arch/x86/kernel/rtc.c
> > +++ b/arch/x86/kernel/rtc.c
> > @@ -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", };
> > @@ -197,6 +214,10 @@ static __init int add_rtc_cmos(void)
> >  	if (!x86_platform.legacy.rtc)
> >  		return -ENODEV;
> >  
> > +	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).

So, I'm open to suggestions. Can you write down a skeleton which module
should contain what and their point of initialization in time with
regarding to RTC?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 4745 bytes --]

On Mon, 16 Jan 2017, Andy Shevchenko wrote:
> On Mon, 2017-01-16 at 20:04 +0100, Luis R. Rodriguez wrote:
> > 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).

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

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

> > 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().

That's not possible. At this point the particular subarch is not yet known
and the subarch initialization code which is called via
x86_init.oem.arch_setup() is a perfectly fine place.

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

It's a quirk only for the devices which need it.

> ...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....

> > > +#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", };
> > > @@ -197,6 +214,10 @@ static __init int add_rtc_cmos(void)
> > >  	if (!x86_platform.legacy.rtc)
> > >  		return -ENODEV;
> > >  
> > > +	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?

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.

> 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.

Thanks,

	tglx

8<--------------------
--- a/arch/x86/platform/intel-mid/intel-mid.c
+++ b/arch/x86/platform/intel-mid/intel-mid.c
@@ -117,6 +117,17 @@ static void __init intel_mid_time_init(v
 	x86_init.timers.setup_percpu_clockev = apbt_time_init;
 }
 
+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");
+		x86_platform.legacy.rtc = 0;
+	}
+}
+
 static void intel_mid_arch_setup(void)
 {
 	if (boot_cpu_data.x86 != 6) {
@@ -151,6 +162,10 @@ static void intel_mid_arch_setup(void)
 	if (intel_mid_ops->arch_setup)
 		intel_mid_ops->arch_setup();
 
+	/* If the platform has an RTC make sure the APIC entry is allocated */
+	if (x86_platform.legacy.rtc)
+		x86_init.timers.wallclock_init = intel_mid_legacy_rtc_init;
+
 	/*
 	 * Intel MID platforms are using explicitly defined regulators.
 	 *
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -540,21 +540,8 @@ static int __init sfi_parse_devs(struct
 	return 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);
-	return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
-}
-
 static int __init intel_mid_platform_init(void)
 {
-	intel_mid_legacy_rtc_init();
-
 	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
 	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
 	return 0;

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Andy Shevchenko]

On Mon, Jan 16, 2017 at 11:00 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 16 Jan 2017, Andy Shevchenko wrote:
>> On Mon, 2017-01-16 at 20:04 +0100, Luis R. Rodriguez wrote:
>> > 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).
>
> Which is obviously the case. tip x86/platform has:
>
> de1c2540aa4f: x86/platform/intel-mid: Enable RTC on Intel Merrifield

Oh, didn't realize that. Thanks for pointing out.

>> ...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....

Yes, that's correct. Just had no time to do that quickly.

>> 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.

I see your proposal below. Indeed it looks much cleaner.
Thanks!

> +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)) {

I will fix this (> 0 is okay, = 0 I have to check) and give it a try.

> +               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);
> -       return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
> -}
> -
>  static int __init intel_mid_platform_init(void)
>  {
> -       intel_mid_legacy_rtc_init();
> -
>         sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
>         sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
>         return 0;

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Thomas Gleixner]

On Tue, 17 Jan 2017, Andy Shevchenko wrote:
> By the way, would be better to revert or dismiss that commit at all?

We won't rebase the branch. Just send a delta fix please.

Thanks,

	tglx

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Andy Shevchenko]

On Mon, 2017-01-16 at 22:00 +0100, Thomas Gleixner wrote:

> 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

> +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");
> +		x86_platform.legacy.rtc = 0;
> +	}
> +}
> +
>  static void intel_mid_arch_setup(void)
>  {
>  	if (boot_cpu_data.x86 != 6) {
> @@ -151,6 +162,10 @@ static void intel_mid_arch_setup(void)
>  	if (intel_mid_ops->arch_setup)
>  		intel_mid_ops->arch_setup();
>  
> +	/* If the platform has an RTC make sure the APIC entry is
> allocated */
> +	if (x86_platform.legacy.rtc)
> +		x86_init.timers.wallclock_init =
> intel_mid_legacy_rtc_init;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

On Tue, 17 Jan 2017, Andy Shevchenko wrote:

> On Mon, 2017-01-16 at 22:00 +0100, Thomas Gleixner wrote:
> 
> > 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

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.

Thanks,

	tglx

--- a/arch/x86/platform/intel-mid/intel-mid.c
+++ b/arch/x86/platform/intel-mid/intel-mid.c
@@ -93,6 +93,17 @@ static void __init intel_mid_setup_bp_ti
 	setup_boot_APIC_clock();
 }
 
+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) < 0) {
+		pr_info("Failed to allocate RTC interrupt. Disabling RTC\n");
+		x86_platform.legacy.rtc = 0;
+	}
+}
+
 static void __init intel_mid_time_init(void)
 {
 	sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr);
@@ -115,6 +126,10 @@ static void __init intel_mid_time_init(v
 	}
 
 	x86_init.timers.setup_percpu_clockev = apbt_time_init;
+
+	/* If the platform has an RTC make sure the APIC entry is allocated */
+	if (x86_platform.legacy.rtc)
+		intel_mid_legacy_rtc_init();
 }
 
 static void intel_mid_arch_setup(void)
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -540,21 +540,8 @@ static int __init sfi_parse_devs(struct
 	return 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);
-	return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
-}
-
 static int __init intel_mid_platform_init(void)
 {
-	intel_mid_legacy_rtc_init();
-
 	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
 	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
 	return 0;

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Andy Shevchenko]

On Wed, 2017-01-18 at 11:24 +0100, Thomas Gleixner wrote:
> On Tue, 17 Jan 2017, Andy Shevchenko wrote:
> 
> > On Mon, 2017-01-16 at 22:00 +0100, Thomas Gleixner wrote:
> > 
> > > 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
> 
> 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.

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.

What do you think? Should I send a series for review?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Thomas Gleixner]

On Wed, 18 Jan 2017, Andy Shevchenko wrote:
> On Wed, 2017-01-18 at 11:24 +0100, Thomas Gleixner wrote:
> > 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

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

> - 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.

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.

Thanks,

	tglx

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Andy Shevchenko]

On Wed, 2017-01-18 at 12:02 +0100, Thomas Gleixner wrote:
> On Wed, 18 Jan 2017, Andy Shevchenko wrote:
> > On Wed, 2017-01-18 at 11:24 +0100, Thomas Gleixner wrote:
> > > 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
> 
> Oh no, please don't add yet another arch hook just because we can.

I see.

> > - 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.
> 
> 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.

Nothing specifically wrong. I would give a try to this and send a patch
if everything okay.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Andy Shevchenko]

On Wed, 2017-01-18 at 11:24 +0100, Thomas Gleixner wrote:
> On Tue, 17 Jan 2017, Andy Shevchenko wrote:
> 
> > On Mon, 2017-01-16 at 22:00 +0100, Thomas Gleixner wrote:
> > 
> > > 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
> 
> 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.

Still too early. There is kernel_init() thread which initializes IO-APIC 
IRQs AFAIU. Neither mine, nor your solution would work.

[    0.000000] NR_IRQS:4352 nr_irqs:512 0
[    0.000000] intel_mid: Failed to allocate RTC interrupt. Disabling
RTC
[    0.105359] ENABLING IO-APIC IRQs

I see this one, not sure if it's correct path

kernel_init() ->
 kernel_init_freeable() ->
  smp_prepare_cpus() / native_smp_prepare_cpus() ->
   apic_bsp_setup() ->
    setup_IO_APIC()

So, fixing in rtc.c seems now more reasonable (we may get rid of ugly
#ifdef:s by providing stubs in IOAPIC code).

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.

>  static void __init intel_mid_time_init(void)
>  {
>  	sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr);
> 

> +
> +	/* If the platform has an RTC make sure the APIC entry is
> allocated */
> +	if (x86_platform.legacy.rtc)
> +		intel_mid_legacy_rtc_init();

Just in case this should be first call in the function, otherwise it
never been called.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Andy Shevchenko]

On Wed, 2017-01-18 at 18:29 +0200, Andy Shevchenko wrote:
> 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.

I will go this way and just create
arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c
in a similar way how it's done for watchdog.

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

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy