[Fwd: [PATCH Resend 01/02] Add Linux ASMP support for MPC8641D]

[Kumar Gala]

On Mar 22, 2007, at 10:58 AM, Yoder Stuart-B08248 wrote:

>
>
>> -----Original Message-----
>> From: Kumar Gala [mailto:galak at kernel.crashing.org]
>> Sent: Thursday, March 22, 2007 10:44 AM
>> To: Olof Johansson
>> Cc: Yoder Stuart-B08248; linuxppc-dev at ozlabs.org
>> Subject: Re: [Fwd: [PATCH Resend 01/02] Add Linux ASMP
>> support for MPC8641D]
>>
>>
>> On Mar 22, 2007, at 10:51 AM, Olof Johansson wrote:
>>
>>> On Thu, Mar 22, 2007 at 08:32:33AM -0700, Yoder Stuart-B08248 wrote:
>>>>
>>>>> +		mpic: pic at 40000 {
>>>>> +			clock-frequency = <0>;
>>>>> +			interrupt-controller;
>>>>> +			#address-cells = <0>;
>>>>> +			#interrupt-cells = <2>;
>>>>> +			reg = <40000 40000>;
>>>>> +			built-in;
>>>>> +			compatible = "chrp,open-pic";
>>>>> +			device_type = "open-pic";
>>>>> +			big-endian;
>>>>> +			interrupts = <
>>>>> +			18 2 49 2 19 2
>>>>> +			2a 2 2b 2 4a 1
>>>>> +			1d 2 1e 2 22 2
>>>>> +			23 2 24 2 28 2
>>>>> +			>;
>>>>> +		};
>>>>> +	};
>>>>> +};
>>>>
>>>> I think using the 'interrupts' property in this way is bad.  It's
>>>> being used to tell the pic which interrupts belong to it.
>>>>
>>>> The problem is that the interrupts property is being overloaded
>>>> and used for a completely different purpose than that which
>>>> it was originally intended and documented.  Interrupt controllers
>>>> should not have an interrupts property.
>>>>
>>>> The only reason I can see to do this is as a convenience to avoid
>>>> walking the device tree.   The pic code should walk the device tree
>>>> to determine the irq numbers the running core.
>>>
>>> Not speaking for the case of the freescale boards here, but not all
>>> interrupts are in the device tree, nor do they have to.
>> Especially for
>>> PCI devices, etc.
>>>
>>> This is quite a special case. Having a brand new property
>> for it might
>>> be a good idea. It's a pretty big hack anyway, so adding another
>>> one to
>>> the device tree isn't that big a deal.
>>
>> I think the issue is you don't want the per interrupt registers
>> initialized twice.  Thus I suggest we just let set_irq_type handle  
>> it.
>>
>> There is still an issue for shared interrupts, but I don't think you
>> can solve that.  W/o moving the interrupt processing to some common
>> level.
>
> As I understand the patch, there are two DTS files once for each core
> and each having a partitioned set of devices for that core.  In the
> current patch the pic in each DTS has a unique 'interrupts' property.
>
> This shouldn't be necessary-- to initialize the pic should simply be
> a matter of each core walking it's device tree, which only has the
> devices
> assigned to it, and programming the pic appropriately.   No registers
> should be programmed twice.

If you look at what mpic_init() does you will see that the registers  
would be setup twice.

Also, you don't need to walk the device-tree at all.  As Olof pointed  
out there maybe interrupts that are not described in the device  
tree.  We can setup this information when you do a request_irq() ->  
set_irq_type().

If you aren't doing a request_irq() for the interrupt than it doesn't  
need to be setup.  Additionally, you should only doing a request_irq 
() for an interrupt that is "owned" by that core.

- k