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

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

[MaJun]

From: Ma Jun <majun258@huawei.com>

For mbigen module, there is a special case that more than one mbigen 
device nodes use the same reg definition in DTS when these devices
exist in the same mbigen hardware module. 

mbigen_dev1:intc_dev1 {
	...
	reg = <0x0 0xc0080000 0x0 0x10000>;
	...
};

mbigen_dev2:intc_dev2 {
	...
	reg = <0x0 0xc0080000 0x0 0x10000>;
	...
};

On this case, devm_ioremap_resource() returns fail with info
"can't request region for resource" because of memory region check.

Because we only need to program 1 into corresponding bit into status 
register of mbigen to clear the interrupt status during runtime,
I think we can replace devm_ioremap_resource() by devm_ioremap().

Signed-off-by: Ma Jun <majun258@huawei.com>
---
 drivers/irqchip/irq-mbigen.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 4dd3eb8..b19e528 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -250,7 +250,7 @@ static int mbigen_device_probe(struct platform_device *pdev)
 	mgn_chip->pdev = pdev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
+	mgn_chip->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
 	if (IS_ERR(mgn_chip->base))
 		return PTR_ERR(mgn_chip->base);
 
-- 
1.7.1

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

[Mark Rutland]

On Mon, Feb 01, 2016 at 07:44:54PM +0800, MaJun wrote:
> From: Ma Jun <majun258@huawei.com>
> 
> For mbigen module, there is a special case that more than one mbigen 
> device nodes use the same reg definition in DTS when these devices
> exist in the same mbigen hardware module. 
> 
> mbigen_dev1:intc_dev1 {
> 	...
> 	reg = <0x0 0xc0080000 0x0 0x10000>;
> 	...
> };
> 
> mbigen_dev2:intc_dev2 {
> 	...
> 	reg = <0x0 0xc0080000 0x0 0x10000>;
> 	...
> };

This doesn't sound right. If they exist in the same place, and have the
same reg, they _are_ the same device.

You'll need to explain this better.

> On this case, devm_ioremap_resource() returns fail with info
> "can't request region for resource" because of memory region check.
> 
> Because we only need to program 1 into corresponding bit into status 
> register of mbigen to clear the interrupt status during runtime,
> I think we can replace devm_ioremap_resource() by devm_ioremap().
> 
> Signed-off-by: Ma Jun <majun258@huawei.com>
> ---
>  drivers/irqchip/irq-mbigen.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 4dd3eb8..b19e528 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -250,7 +250,7 @@ static int mbigen_device_probe(struct platform_device *pdev)
>  	mgn_chip->pdev = pdev;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
> +	mgn_chip->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));

These two behaving differently doesn't seem correct either...

Mark.

>  	if (IS_ERR(mgn_chip->base))
>  		return PTR_ERR(mgn_chip->base);
>  
> -- 
> 1.7.1
> 
> 

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

[majun (F)]

? 2016/2/1 19:57, Mark Rutland ??:
> On Mon, Feb 01, 2016 at 07:44:54PM +0800, MaJun wrote:
>> From: Ma Jun <majun258@huawei.com>
>>
>> For mbigen module, there is a special case that more than one mbigen 
>> device nodes use the same reg definition in DTS when these devices
>> exist in the same mbigen hardware module. 
>>
>> mbigen_dev1:intc_dev1 {
>> 	...
>> 	reg = <0x0 0xc0080000 0x0 0x10000>;
>> 	...
>> };
>>
>> mbigen_dev2:intc_dev2 {
>> 	...
>> 	reg = <0x0 0xc0080000 0x0 0x10000>;
>> 	...
>> };
> 
> This doesn't sound right. If they exist in the same place, and have the
> same reg, they _are_ the same device.
> 
> You'll need to explain this better.
> 

For a mbigen hardware module which connecte with several devices,
because these devices has different device ID,
I need to define a device node for each device in DTS file.

	mbigen
	|
------------------
| 	| 	|
dev1 	dev2 	dev3

Because of register layout,the registers related with these devices
are put together, I can't split them clearly.
So, I only make these devices which connect with
same mbigen hardware module usethe same address.

>> On this case, devm_ioremap_resource() returns fail with info
>> "can't request region for resource" because of memory region check.
>>
>> Because we only need to program 1 into corresponding bit into status 
>> register of mbigen to clear the interrupt status during runtime,
>> I think we can replace devm_ioremap_resource() by devm_ioremap().
>>
>> Signed-off-by: Ma Jun <majun258@huawei.com>
>> ---
>>  drivers/irqchip/irq-mbigen.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
>> index 4dd3eb8..b19e528 100644
>> --- a/drivers/irqchip/irq-mbigen.c
>> +++ b/drivers/irqchip/irq-mbigen.c
>> @@ -250,7 +250,7 @@ static int mbigen_device_probe(struct platform_device *pdev)
>>  	mgn_chip->pdev = pdev;
>>  
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -	mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
>> +	mgn_chip->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> 
> These two behaving differently doesn't seem correct either...

The problem is caused by memory region check fail because of duplicated address in devm_ioremap_resource().
For mbigen special case, there is no necessary to do this check.

Thanks
Ma Jun
> 
> Mark.
> 
>>  	if (IS_ERR(mgn_chip->base))
>>  		return PTR_ERR(mgn_chip->base);
>>  
>> -- 
>> 1.7.1
>>
>>
> 
> .
> 

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

[Mark Rutland]

On Mon, Feb 01, 2016 at 09:06:38PM +0800, majun (F) wrote:
> 
> 
> ? 2016/2/1 19:57, Mark Rutland ??:
> > On Mon, Feb 01, 2016 at 07:44:54PM +0800, MaJun wrote:
> >> From: Ma Jun <majun258@huawei.com>
> >>
> >> For mbigen module, there is a special case that more than one mbigen 
> >> device nodes use the same reg definition in DTS when these devices
> >> exist in the same mbigen hardware module. 
> >>
> >> mbigen_dev1:intc_dev1 {
> >> 	...
> >> 	reg = <0x0 0xc0080000 0x0 0x10000>;
> >> 	...
> >> };
> >>
> >> mbigen_dev2:intc_dev2 {
> >> 	...
> >> 	reg = <0x0 0xc0080000 0x0 0x10000>;
> >> 	...
> >> };
> > 
> > This doesn't sound right. If they exist in the same place, and have the
> > same reg, they _are_ the same device.
> > 
> > You'll need to explain this better.
> > 
> 
> For a mbigen hardware module which connecte with several devices,
> because these devices has different device ID,
> I need to define a device node for each device in DTS file.
> 
> 	mbigen
> 	|
> ------------------
> | 	| 	|
> dev1 	dev2 	dev3
> 
> Because of register layout,the registers related with these devices
> are put together, I can't split them clearly.
> So, I only make these devices which connect with
> same mbigen hardware module usethe same address.

This sounds like we've either mis-described the mbigen in the binding,
or we're mis-describing the relationship with the devices.

It should not be necessary to describe the same set of registers
repeatedly.

> >>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> -	mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
> >> +	mgn_chip->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > 
> > These two behaving differently doesn't seem correct either...
> 
> The problem is caused by memory region check fail because of
> duplicated address in devm_ioremap_resource().

The check is sensible. Using devm_ioremap rather than
devm_ioremap_resource to avoid the check is not the correct solution.

> For mbigen special case, there is no necessary to do this check.

I'm not sure I fully agree.

Regardless, this is not the right solution; it is fragile at best.

Thanks,
Mark.

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

[majun (F)]

Hi Mark:

? 2016/2/1 21:25, Mark Rutland ??:
> On Mon, Feb 01, 2016 at 09:06:38PM +0800, majun (F) wrote:
>>
>>
>> ? 2016/2/1 19:57, Mark Rutland ??:
>>> On Mon, Feb 01, 2016 at 07:44:54PM +0800, MaJun wrote:
>>>> From: Ma Jun <majun258@huawei.com>
>>>>
>>>> For mbigen module, there is a special case that more than one mbigen 
>>>> device nodes use the same reg definition in DTS when these devices
>>>> exist in the same mbigen hardware module. 
>>>>
>>>> mbigen_dev1:intc_dev1 {
>>>> 	...
>>>> 	reg = <0x0 0xc0080000 0x0 0x10000>;
>>>> 	...
>>>> };
>>>>
>>>> mbigen_dev2:intc_dev2 {
>>>> 	...
>>>> 	reg = <0x0 0xc0080000 0x0 0x10000>;
>>>> 	...
>>>> };
>>>
>>> This doesn't sound right. If they exist in the same place, and have the
>>> same reg, they _are_ the same device.
>>>
>>> You'll need to explain this better.
>>>
>>
>> For a mbigen hardware module which connecte with several devices,
>> because these devices has different device ID,
>> I need to define a device node for each device in DTS file.
>>
>> 	mbigen
>> 	|
>> ------------------
>> | 	| 	|
>> dev1 	dev2 	dev3
>>
>> Because of register layout,the registers related with these devices
>> are put together, I can't split them clearly.
>> So, I only make these devices which connect with
>> same mbigen hardware module usethe same address.
> 
> This sounds like we've either mis-described the mbigen in the binding,
> or we're mis-describing the relationship with the devices.
> 

Sorry, I didn't express myself clearly.

For example, a mbigen hardware module can support several peripheral devices
with different device ID.

|-----------------------|-
|	mbigen		|
|-----------------------|-
 |       |        |
dev1    dev2     dev3

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.

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?

Thanks!

> It should not be necessary to describe the same set of registers
> repeatedly.
> 
>>>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> -	mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
>>>> +	mgn_chip->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>
>>> These two behaving differently doesn't seem correct either...
>>
>> The problem is caused by memory region check fail because of
>> duplicated address in devm_ioremap_resource().
> 
> The check is sensible. Using devm_ioremap rather than
> devm_ioremap_resource to avoid the check is not the correct solution.
> 
>> For mbigen special case, there is no necessary to do this check.
> 
> I'm not sure I fully agree.
> 
> Regardless, this is not the right solution; it is fragile at best.
> 
> Thanks,
> Mark.
> 
> .
> 

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

[Mark Rutland]

On Tue, Feb 02, 2016 at 06:25:53PM +0800, majun (F) wrote:
> Hi Mark:
> 
> ? 2016/2/1 21:25, Mark Rutland ??:
> > On Mon, Feb 01, 2016 at 09:06:38PM +0800, majun (F) wrote:
> >>
> >>
> >> ? 2016/2/1 19:57, Mark Rutland ??:
> >>> On Mon, Feb 01, 2016 at 07:44:54PM +0800, MaJun wrote:
> >>>> From: Ma Jun <majun258@huawei.com>
> >>>>
> >>>> For mbigen module, there is a special case that more than one mbigen 
> >>>> device nodes use the same reg definition in DTS when these devices
> >>>> exist in the same mbigen hardware module. 
> >>>>
> >>>> mbigen_dev1:intc_dev1 {
> >>>> 	...
> >>>> 	reg = <0x0 0xc0080000 0x0 0x10000>;
> >>>> 	...
> >>>> };
> >>>>
> >>>> mbigen_dev2:intc_dev2 {
> >>>> 	...
> >>>> 	reg = <0x0 0xc0080000 0x0 0x10000>;
> >>>> 	...
> >>>> };
> >>>
> >>> This doesn't sound right. If they exist in the same place, and have the
> >>> same reg, they _are_ the same device.
> >>>
> >>> You'll need to explain this better.
> >>>
> >>
> >> For a mbigen hardware module which connecte with several devices,
> >> because these devices has different device ID,
> >> I need to define a device node for each device in DTS file.
> >>
> >> 	mbigen
> >> 	|
> >> ------------------
> >> | 	| 	|
> >> dev1 	dev2 	dev3
> >>
> >> Because of register layout,the registers related with these devices
> >> are put together, I can't split them clearly.
> >> So, I only make these devices which connect with
> >> same mbigen hardware module usethe same address.
> > 
> > This sounds like we've either mis-described the mbigen in the binding,
> > or we're mis-describing the relationship with the devices.
> > 
> 
> Sorry, I didn't express myself clearly.
> 
> For example, a mbigen hardware module can support several peripheral devices
> with different device ID.
> 
> |-----------------------|-
> |	mbigen		|
> |-----------------------|-
>  |       |        |
> dev1    dev2     dev3
> 
> 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.

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

Mark.

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

[majun (F)]

? 2016/2/2 19:43, Mark Rutland ??:
> On Tue, Feb 02, 2016 at 06:25:53PM +0800, majun (F) wrote:
>> Hi Mark:
>>
>> ? 2016/2/1 21:25, Mark Rutland ??:
>>> On Mon, Feb 01, 2016 at 09:06:38PM +0800, majun (F) wrote:
>>>>
>>>>
>>>> ? 2016/2/1 19:57, Mark Rutland ??:
>>>>> On Mon, Feb 01, 2016 at 07:44:54PM +0800, MaJun wrote:
>>>>>> From: Ma Jun <majun258@huawei.com>
>>>>>>
>>>>>> For mbigen module, there is a special case that more than one mbigen 
>>>>>> device nodes use the same reg definition in DTS when these devices
>>>>>> exist in the same mbigen hardware module. 
>>>>>>
>>>>>> mbigen_dev1:intc_dev1 {
>>>>>> 	...
>>>>>> 	reg = <0x0 0xc0080000 0x0 0x10000>;
>>>>>> 	...
>>>>>> };
>>>>>>
>>>>>> mbigen_dev2:intc_dev2 {
>>>>>> 	...
>>>>>> 	reg = <0x0 0xc0080000 0x0 0x10000>;
>>>>>> 	...
>>>>>> };
>>>>>
>>>>> This doesn't sound right. If they exist in the same place, and have the
>>>>> same reg, they _are_ the same device.
>>>>>
>>>>> You'll need to explain this better.
>>>>>
>>>>
>>>> For a mbigen hardware module which connecte with several devices,
>>>> because these devices has different device ID,
>>>> I need to define a device node for each device in DTS file.
>>>>
>>>> 	mbigen
>>>> 	|
>>>> ------------------
>>>> | 	| 	|
>>>> dev1 	dev2 	dev3
>>>>
>>>> Because of register layout,the registers related with these devices
>>>> are put together, I can't split them clearly.
>>>> So, I only make these devices which connect with
>>>> same mbigen hardware module usethe same address.
>>>
>>> This sounds like we've either mis-described the mbigen in the binding,
>>> or we're mis-describing the relationship with the devices.
>>>
>>
>> Sorry, I didn't express myself clearly.
>>
>> For example, a mbigen hardware module can support several peripheral devices
>> with different device ID.
>>
>> |-----------------------|-
>> |	mbigen		|
>> |-----------------------|-
>>  |       |        |
>> dev1    dev2     dev3
>>
>> 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 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.

Thanks!
Ma Jun


> 
> Mark.
> 
> .
> 

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

[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 think we do need to worry about this.

Mark.

[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.
> 
> .
>