Re: [PATCH] x86: Get irq for hpet timer

[Maciej W. Rozycki]

On Wed, 28 May 2008, Kevin Hao wrote:

> BTW, Maciej. I am not familiar with ACPI subsystem. But I think the 
> acpi_register_gsi should return -1 if mp_find_ioapic return error.
> Is that right? If you also think so, I will make a patch for that.

 Well, the interface is documented in the sources -- I do not feel like
reading through all the execution paths to see whether the implementation
matches though. ;)

 A few comments below -- I may not have time available to throw your piece 
of code at a piece of hardware though.

> +	/* we prefer level triggered mode */
> +	v = readl(&timer->hpet_config);
> +	if (!(v & Tn_INT_TYPE_CNF_MASK)) {
> +		v |= Tn_INT_TYPE_CNF_MASK;
> +		writel(v, &timer->hpet_config);
> +	}

 Please note that when routing through the 8259A you need to match the
trigger mode set for the respective input.  It is normally set in the ELCR
register in the hardware and should be recorded in the ACPI interrupt
source tables too -- the default for IRQ0-15 is edge-triggered, active
high, unless overridden.

> +	v = (readq(&timer->hpet_config) & Tn_INT_ROUTE_CAP_MASK)
> +				 >> Tn_INT_ROUTE_CAP_SHIFT;

 The operator should be at the end of the first line IMHO.

> +	/*
> +	 * In PIC mode, skip IRQ0-4, IRQ6-8, IRQ12-15 which is always used by
> +	 * legacy device. In IO APIC mode, we skip all the legacy IRQS.
> +	 */

 Quite reasonable assumption IMO, but again -- I'd expect the information 
about legacy devices present on these lines to be recorded in ACPI tables.  
Note that IRQ9 is used for the SCI -- I am not sure if that's a good 
choice for sharing.

> +		gsi = acpi_register_gsi(irq, ACPI_LEVEL_SENSITIVE,
> +					ACPI_ACTIVE_LOW);

 I am assuming you have verified ACPI_ACTIVE_LOW works here, contrary to 
the HPET spec.

> +	if  (irq < HPET_MAX_IRQ) {

 Single space here.

> @@ -37,6 +37,7 @@ struct hpet {
>  #define	hpet_compare	_u1._hpet_compare
>  
>  #define	HPET_MAX_TIMERS	(32)
> +#define HPET_MAX_IRQ	(32)

 Tab here for consistency with the others?

 Otherwise no obvious problems.

  Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/