Messages in this thread Patch in this message | | | Date | Mon, 16 Jan 2017 22:00:58 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device |
| |
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; | |