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@xxxxxxxxxxxxxxx>
Intel Finland Oy