From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751455AbdAPVBG (ORCPT ); Mon, 16 Jan 2017 16:01:06 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:42085 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750855AbdAPVBE (ORCPT ); Mon, 16 Jan 2017 16:01:04 -0500 Date: Mon, 16 Jan 2017 22:00:58 +0100 (CET) From: Thomas Gleixner To: Andy Shevchenko cc: "Luis R. Rodriguez" , Ingo Molnar , "H . Peter Anvin" , linux-kernel@vger.kernel.org, x86@kernel.org Subject: Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device In-Reply-To: <1484594499.2133.159.camel@linux.intel.com> Message-ID: References: <20170116172345.3823-1-andriy.shevchenko@linux.intel.com> <20170116190437.GB13946@wotan.suse.de> <1484594499.2133.159.camel@linux.intel.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-322136800-1484600048=:3923" Content-ID: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-322136800-1484600048=:3923 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: 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; --8323329-322136800-1484600048=:3923--