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

[Thomas Gleixner <tglx@linutronix.de>]

[-- 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;