diff mbox

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

Message ID 1454327094-7848-1-git-send-email-majun258@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

majun (F) Feb. 1, 2016, 11:44 a.m. UTC
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(-)

Comments

Mark Rutland Feb. 1, 2016, 11:57 a.m. UTC | #1
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
> 
>
majun (F) Feb. 1, 2016, 1:06 p.m. UTC | #2
? 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
>>
>>
> 
> .
>
Mark Rutland Feb. 1, 2016, 1:25 p.m. UTC | #3
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.
majun (F) Feb. 2, 2016, 10:25 a.m. UTC | #4
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.
> 
> .
>
Mark Rutland Feb. 2, 2016, 11:43 a.m. UTC | #5
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.
majun (F) Feb. 3, 2016, 2:31 a.m. UTC | #6
? 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.
> 
> .
>
Mark Rutland Feb. 3, 2016, 11:16 a.m. UTC | #7
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@0 {
		reg = <0>;
		msi-parent = <&its 0>;
	};

	module@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.
majun (F) Feb. 16, 2016, 7:54 a.m. UTC | #8
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@0 {
> 		reg = <0>;
> 		msi-parent = <&its 0>;
> 	};
> 
> 	module@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.
> 
> .
>
diff mbox

Patch

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);