Re: [PATCH 01/14] DEVICETREE: Add bindings for PIC32 interrupt controller

[Joshua Henderson]

On 11/21/2015 1:47 PM, Arnd Bergmann wrote:
> On Friday 20 November 2015 17:17:13 Joshua Henderson wrote:
> 
>> +Example
>> +-------
>> +
>> +evic: interrupt-controller@1f810000 {
>> +        compatible = "microchip,evic-v2";
>> +        interrupt-controller;
>> +        #interrupt-cells = <3>;
>> +        reg = <0x1f810000 0x1000>;
>> +        device_type="evic-v2";
>> +};
> 
> This is not a correct use of device_type. Just drop that property.

Ack.

> 
>> diff --git a/include/dt-bindings/interrupt-controller/microchip,pic32mz-evic.h b/include/dt-bindings/interrupt-controller/microchip,pic32mz-evic.h
>> new file mode 100644
>> index 0000000..2c466b8
>> --- /dev/null
>> +++ b/include/dt-bindings/interrupt-controller/microchip,pic32mz-evic.h
>> @@ -0,0 +1,238 @@
>> +/*
>> + * This header provides constants for the MICROCHIP PIC32 EVIC.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MICROCHIP_EVIC_H
>> +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MICROCHIP_EVIC_H
>> +
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +/* Hardware interrupt number */
>> +#define CORE_TIMER_INTERRUPT 0
>> +#define CORE_SOFTWARE_INTERRUPT_0 1
>> +#define CORE_SOFTWARE_INTERRUPT_1 2
>> +#define EXTERNAL_INTERRUPT_0 3
>> +#define TIMER1 4
> 
> A header file like this is just going to make everyone's life
> miserable. Try to remove as much as possible here: normally
> you can just use the numbers from the data sheet that match
> the actual hardware registers, and put them into the dts file.
> 

Agreed.  Removing these defines along with removing the priorities from the bindings as suggested makes sense.  With doing that, this header file becomes pointless and it will be dropped.

>> +/* Interrupt priority bits */
>> +#define PRI_0	0	/* Note:This priority disables the interrupt! */
>> +#define PRI_1	1
>> +#define PRI_2	2
>> +#define PRI_3	3
>> +#define PRI_4	4
>> +#define PRI_5	5
>> +#define PRI_6	6
>> +#define PRI_7	7
> 
>> +/* Interrupt subpriority bits */
>> +#define SUB_PRI_0	0
>> +#define SUB_PRI_1	1
>> +#define SUB_PRI_2	2
>> +#define SUB_PRI_3	3
> 
> These are obviously silly and should be removed/
> 

Ack.

>> +#define PRI_MASK	0x7	/* 3 bit priority mask */
>> +#define SUBPRI_MASK	0x3	/* 2 bit subpriority mask */
>> +#define INT_MASK	0x1F	/* 5 bit pri and subpri mask */
>> +#define NR_EXT_IRQS	5	/* 5 external interrupts sources */
>> +
>> +#define MICROCHIP_EVIC_MIN_PRIORITY 0
>> +#define MICROCHIP_EVIC_MAX_PRIORITY INT_MASK
>> +
>> +#define INT_PRI(pri, subpri)	\
>> +	(((pri & PRI_MASK) << 2) | (subpri & SUBPRI_MASK))
>> +
>> +#define DEFINE_INT(irq, pri) { irq, pri }
>> +
>> +#define DEFAULT_INT_PRI INT_PRI(2, 0)
> 
> Is it required to have a specific priority configured for each line?
> If these are software selectable, it's probably better to not put
> them into DT in the first place.
> 
> If you absolutely need them, I would suggest using two separate cells
> for pri and subpri so you can avoid the macro.
> 

These priorities are hardware priorities that arbitrate pending interrupts to the CPU.  These are indeed software configurable and we can agree that DT is probably not the best place to put this configuration in light of this.  We'll default to something sane instead.  They will be removed from the binding.

Josh

--
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/