[RFC PATCH] irq/mbigen:Fix the problem of IO remap for duplicated physical address in mbigen driver

[majun (F)]

Hi Mark:
	sorry for late response because of chinese new year.

在 2016/2/3 19:16, Mark Rutland 写道:
> On Wed, Feb 03, 2016 at 10:31:52AM +0800, majun (F) wrote:
>>
>>
>> 在 2016/2/2 19:43, Mark Rutland 写道:
>>> On Tue, Feb 02, 2016 at 06:25:53PM +0800, majun (F) wrote:
>>>> To simplify the mbigen drvier,
>>>> I didn't use the whole mbigen module as a mbgien device, but use
>>>> the register collections(vector, trigger type,status etc.) corresponding
>>>> to a peripheral device as a mbigen device.
>>>> So, mbigen device is a logical conception or logical device.
>>>
>>> >From the above, it sounds like the DT representation is not an accurate
>>> representation of the hardware. We should describe the _whole_ mbigen,
>>> not portions thereof. Trying to describe it piecemeal leads to problems
>>> like this one.
>>>
>>> I don't understand the rationale for describing the mbigen as separate
>>> nodes. Above you mention that we "need to define a device node for each
>>> device", but I don't see why that's necessary. Why do you believe we
>>> need an mbigen node per client device?
>>>
>>> As far as I can tell, we should be able to map an arbitrary
>>> interrupt-specifier to a particular MSI (complete with device id) within
>>> the mbigen driver. We just need to define the full set of MSIs the
>>> mbigen owns.
>>>
>>
>> mbigen device is a interrupt controller, it is also a ITS device for ITS module.
>> So, we need to register the each mbigen device to ITS with a unique ID.
>> Based on the current MSI/ITS driver, I can't register whole mbigen module which
>> contains several mbigen devices at one time. Because they have different device ID.
> 
> I don't follow.
> 
> You can describe this by having a top-level mbigen node featuring a reg,
> with a sub-node for each mbigen module with an appropriate msi-parent,
> e.g.
> 
> mbigen {
> 	reg = < ... ... >;
> 
> 	#interrupt-cells = <2>;
> 
> 	#address-cells = <1>; /* module index */
> 
> 	module at 0 {
> 		reg = <0>;
> 		msi-parent = <&its 0>;
> 	};
> 
> 	module at 1 {
> 		reg = <1>;
> 		reg = <&its 1>;
> 	};
> };
> 
> 
> That clearly does not require the reg to be duplicated, and encodes the
> information you want. The infrastructure for handling that might not
> exist yet, but that is a Linux issue that we can fix.
> 
> Marc, thoughts?
> 
> I take it all interrupts within a module share the same device id?
> 
>> I don't know whether this explanation is clear or not.
>> I think Marc can explain this well.
>>
>> Marc, would you please help me explain this?  or, do you have any opinion about this ?
>>
>>>> Now, a mbigen hardware module contains several logical mbigen device.
>>>>
>>>> -------------------------------
>>>> |mgn_dev1  mgn_dev2  mgn_dev3 |
>>>> |-----------------------------|
>>>>    |          |        |
>>>> dev1	    dev2      dev3
>>>>
>>>> So,mgn_dev1, mgn_dev2 and mgn_dev3 exist in same mbigen hardware module,
>>>> and,they use the same reg address region,that is adress of mbigen hardware module.
>>>>
>>>> For this case, I think the question is can we map an reg address
>>>> region more than one time?
>>>
>>> As above, I think we've mis-described the hardware. Rather than making
>>> things simpler, this has resulted in problems like this one.
>>>
>>> We should not map a reg region more than once. Even if it's technically
>>> possible to do so, I do not believe that would be the right solution
>>> here. Implicitly sharing resources (e.g. portions of the mbigen control
>>> registers) is inevitably going to lead to more problems later on.
>>
>> Because we only need to write 1 into corresponding bit of status
>> register to clear interrupt status during runtime,I think we don't
>> need to worry about this.
> 
> That might be currently true, but I doubt that will remain true in
> future. Presumably there are other control registers in the mbigen which
> are shared between modules.
> 

I'm sure there is nothing to control except the status register even in future.

Thanks
MaJun

> I think we do need to worry about this.
> 
> Mark.
> 
> .
>