From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751113AbdAPTZo (ORCPT ); Mon, 16 Jan 2017 14:25:44 -0500 Received: from mga06.intel.com ([134.134.136.31]:31086 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750840AbdAPTZn (ORCPT ); Mon, 16 Jan 2017 14:25:43 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,240,1477983600"; d="scan'208";a="31268281" Message-ID: <1484594499.2133.159.camel@linux.intel.com> Subject: Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device From: Andy Shevchenko To: "Luis R. Rodriguez" Cc: Ingo Molnar , Thomas Gleixner , "H . Peter Anvin" , linux-kernel@vger.kernel.org, x86@kernel.org Date: Mon, 16 Jan 2017 21:21:39 +0200 In-Reply-To: <20170116190437.GB13946@wotan.suse.de> References: <20170116172345.3823-1-andriy.shevchenko@linux.intel.com> <20170116190437.GB13946@wotan.suse.de> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >  #include > >  #include > > +#include > >  #include > > +#include > >  #include > >   > >  #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 Intel Finland Oy