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

RFC PATCH New MaJun 5 participants 20 messages Feb 1, 2016 – May 6, 2016

Subsystems (2)

Device tree bindings irqchip drivers

lore

Share & tools

lore

MaJun

author

2016-02-01 11:44 UTC #0 lore raw

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

From: Ma Jun <[email protected]>

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 <[email protected]>

---
 drivers/irqchip/irq-mbigen.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/irqchip/irq-mbigen.c +1 -1

index 4dd3eb8..b19e528 100644
@@ -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);

Signature · 1 line

1.7.1

Mark Rutland

2016-02-01 11:57 UTC #1 lore raw

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

Quoted 12 lines · MaJun

From: Ma Jun <[email protected]>
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>;
	...
};

Quoted 5 lines

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.

Quoted 17 lines

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 <[email protected]>
---
 drivers/irqchip/irq-mbigen.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/irqchip/irq-mbigen.c +0 -0
index 4dd3eb8..b19e528 100644

drivers/irqchip/irq-mbigen.c static int mbigen_device_probe(struct platform_device *pdev) · line 253

Quoted 5 lines

drivers/irqchip/irq-mbigen.c +1 -1

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

Quoted 7 lines

drivers/irqchip/irq-mbigen.c +0 -0
 	if (IS_ERR(mgn_chip->base))
 		return PTR_ERR(mgn_chip->base);
 
Signature · 1 line
1.7.1

majun (F)

author

2016-02-01 13:06 UTC #2 lore raw

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

? 2016/2/1 19:57, Mark Rutland ??:

Quoted 19 lines · MaJun

Quoted 12 lines
From: Ma Jun <[email protected]>
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>;
	...
};
Quoted 5 lines
mbigen_dev2:intc_dev2 {
	...
	reg = <0x0 0xc0080000 0x0 0x10000>;
	...
};

Quoted 5 lines

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.

Quoted 19 lines

Quoted 19 lines
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 <[email protected]>
---
 drivers/irqchip/irq-mbigen.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/irqchip/irq-mbigen.c +0 -0
index 4dd3eb8..b19e528 100644
@@ -250,7 +250,7 @@ static int mbigen_device_probe(struct platform_device *pdev)
 	mgn_chip->pdev = pdev;
 

Quoted 5 lines

drivers/irqchip/irq-mbigen.c static int mbigen_device_probe(struct platform_device *pdev) · line 253
Quoted 3 lines
drivers/irqchip/irq-mbigen.c +1 -1
 	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

Quoted 13 lines

Mark.
Quoted 7 lines
drivers/irqchip/irq-mbigen.c +0 -0
 	if (IS_ERR(mgn_chip->base))
 		return PTR_ERR(mgn_chip->base);
 
Signature · 1 line
1.7.1
.

Mark Rutland

2016-02-01 13:25 UTC #3 lore raw

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

Quoted 34 lines · majun (F)

? 2016/2/1 19:57, Mark Rutland ??:
Quoted 19 lines · MaJun
Quoted 12 lines
From: Ma Jun <[email protected]>
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>;
	...
};
Quoted 5 lines
mbigen_dev2:intc_dev2 {
	...
	reg = <0x0 0xc0080000 0x0 0x10000>;
	...
};
Quoted 5 lines
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
	|

Quoted 8 lines

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

Quoted 8 lines

Quoted 5 lines
drivers/irqchip/irq-mbigen.c static int mbigen_device_probe(struct platform_device *pdev) · line 253
Quoted 3 lines
drivers/irqchip/irq-mbigen.c +1 -1
 	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.

Quoted 1 line

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)

author

2016-02-02 10:25 UTC #4 lore raw

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

Hi Mark:

? 2016/2/1 21:25, Mark Rutland ??:

Quoted 39 lines · majun (F)

Quoted 38 lines
? 2016/2/1 19:57, Mark Rutland ??:
Quoted 19 lines · MaJun
Quoted 12 lines
From: Ma Jun <[email protected]>
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>;
	...
};
Quoted 5 lines
mbigen_dev2:intc_dev2 {
	...
	reg = <0x0 0xc0080000 0x0 0x10000>;
	...
};
Quoted 5 lines
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

Quoted 8 lines

Quoted 4 lines
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!

Quoted 26 lines

It should not be necessary to describe the same set of registers repeatedly.
Quoted 8 lines
Quoted 5 lines
drivers/irqchip/irq-mbigen.c static int mbigen_device_probe(struct platform_device *pdev) · line 253
Quoted 3 lines
drivers/irqchip/irq-mbigen.c +1 -1
 	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.
Quoted 1 line
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

2016-02-02 11:43 UTC #5 lore raw

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

Quoted 62 lines · majun (F)

Hi Mark:
? 2016/2/1 21:25, Mark Rutland ??:
Quoted 39 lines · majun (F)
Quoted 38 lines
? 2016/2/1 19:57, Mark Rutland ??:
Quoted 19 lines · MaJun
Quoted 12 lines
From: Ma Jun <[email protected]>
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>;
	...
};
Quoted 5 lines
mbigen_dev2:intc_dev2 {
	...
	reg = <0x0 0xc0080000 0x0 0x10000>;
	...
};
Quoted 5 lines
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
Quoted 8 lines
Quoted 4 lines
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

Quoted 7 lines

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.

Quoted 8 lines

Now, a mbigen hardware module contains several logical mbigen device.
-------------------------------
|mgn_dev1  mgn_dev2  mgn_dev3 |
|-----------------------------|
   |          |        |
dev1	    dev2      dev3

Quoted 5 lines

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)

author

2016-02-03 02:31 UTC #6 lore raw

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

? 2016/2/2 19:43, Mark Rutland ??:

Quoted 79 lines · majun (F)

Quoted 62 lines
Hi Mark:
? 2016/2/1 21:25, Mark Rutland ??:
Quoted 39 lines · majun (F)
Quoted 38 lines
? 2016/2/1 19:57, Mark Rutland ??:
Quoted 19 lines · MaJun
Quoted 12 lines
From: Ma Jun <[email protected]>
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>;
	...
};
Quoted 5 lines
mbigen_dev2:intc_dev2 {
	...
	reg = <0x0 0xc0080000 0x0 0x10000>;
	...
};
Quoted 5 lines
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
Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
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?

Quoted 5 lines

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 ?

Quoted 14 lines

Quoted 8 lines
Now, a mbigen hardware module contains several logical mbigen device.
-------------------------------
|mgn_dev1  mgn_dev2  mgn_dev3 |
|-----------------------------|
   |          |        |
dev1	    dev2      dev3
Quoted 5 lines
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?

Quoted 7 lines

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

Quoted 5 lines

Mark.
.

Mark Rutland

2016-02-03 11:16 UTC #7 lore raw

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

Quoted 26 lines · majun (F)

? 2016/2/2 19:43, Mark Rutland ??:
Quoted 17 lines · majun (F)
Quoted 7 lines
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?
Quoted 5 lines
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.

Quoted 4 lines

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?

Quoted 22 lines

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 ?
Quoted 17 lines
Quoted 8 lines
Now, a mbigen hardware module contains several logical mbigen device.
-------------------------------
|mgn_dev1  mgn_dev2  mgn_dev3 |
|-----------------------------|
   |          |        |
dev1	    dev2      dev3
Quoted 5 lines
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.

Quoted 8 lines

Quoted 4 lines
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)

author

2016-02-16 07:54 UTC #8 lore raw

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

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

? 2016/2/3 19:16, Mark Rutland ??:

Quoted 92 lines · majun (F)

Quoted 26 lines
? 2016/2/2 19:43, Mark Rutland ??:
Quoted 17 lines · majun (F)
Quoted 7 lines
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?
Quoted 5 lines
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.
Quoted 4 lines
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?
Quoted 27 lines
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 ?
Quoted 21 lines
Quoted 8 lines
Now, a mbigen hardware module contains several logical mbigen device.
-------------------------------
|mgn_dev1  mgn_dev2  mgn_dev3 |
|-----------------------------|
   |          |        |
dev1	    dev2      dev3
Quoted 5 lines
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.

Quoted 8 lines

Quoted 3 lines
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

Quoted 6 lines

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

MaJun

author

2016-03-17 08:33 UTC #9 lore raw

v3 cover irqchip/mbigen: fix the IO remap problem in mbigen driver.

From: Ma Jun <[email protected]>

This patch set is used to fix the problem of remap a set of registers repeatedly in current mbigen driver.

Changes in v3:
--- Change the log to make more detail description about
the IO remap problem.

Changes in v2:
--- Change the mbigen device node definition as Mark suggested. --- Change the mbigen driver based on the new mbigen dts structure.

Ma Jun (2):
irqchip/mbigen:Change the mbigen node definition in dt binding file irqchip/mbigen:Change the mbigen driver based on the new mbigen node definition

 .../interrupt-controller/hisilicon,mbigen-v2.txt   |   22 +++++++++++---
 drivers/irqchip/irq-mbigen.c                       |   30 ++++++++++++++------
 2 files changed, 38 insertions(+), 14 deletions(-)

MaJun

author

2016-03-17 08:34 UTC #10 lore raw

PATCH v3 1/2 irqchip/mbigen:Change the mbigen node definition in dt binding file

From: Ma Jun <[email protected]>

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.

In current mbigen driver, these mbigen devices definition as below:

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.

To fix this problem, the mbigen node definition and structure are changed as Mark Rutland suggested in v1 patch[1].

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/403691.html

Signed-off-by Ma Jun <[email protected]>

---
 .../interrupt-controller/hisilicon,mbigen-v2.txt   |   22 +++++++++++++++----
 1 files changed, 17 insertions(+), 5 deletions(-)

Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt +17 -5

index 720f7c9..3b2f4c4 100644
@@ -21,6 +21,8 @@ Mbigen main node required properties:
 - reg: Specifies the base physical address and size of the Mbigen
   registers.
 
+Mbigen sub node required properties:
+------------------------------------------
 - interrupt controller: Identifies the node as an interrupt controller
 
 - msi-parent: Specifies the MSI controller this mbigen use.

@@ -45,13 +47,23 @@ Mbigen main node required properties:
 
 Examples:
 
-	mbigen_device_gmac:intc {
+	mbigen_chip_dsa {
 			compatible = "hisilicon,mbigen-v2";
 			reg = <0x0 0xc0080000 0x0 0x10000>;
-			interrupt-controller;
-			msi-parent = <&its_dsa 0x40b1c>;
-			num-pins = <9>;
-			#interrupt-cells = <2>;
+
+			mbigen_gmac:intc_gmac {
+				interrupt-controller;
+				msi-parent = <&its_dsa 0x40b1c>;
+				num-pins = <9>;
+				#interrupt-cells = <2>;
+			};
+
+			mbigen_i2c:intc_i2c {
+				interrupt-controller;
+				msi-parent = <&its_dsa 0x40b0e>;
+				num-pins = <2>;
+				#interrupt-cells = <2>;
+			};
 	};
 
 Devices connect to mbigen required properties:

Signature · 1 line

1.7.1

tip-bot for MaJun

merge bot

2016-03-21 10:34 UTC #11 lore raw

[tip:irq/urgent] irqchip/mbigen: Adjust DT bindings to handle multiple devices in a module

irqchip/mbigen: Adjust DT bindings to handle multiple devices in a module

Commit-ID:  d0e286415dc1f4fea2971d6186b0775c7062575b
Gitweb:     http://git.kernel.org/tip/d0e286415dc1f4fea2971d6186b0775c7062575b
Author:     MaJun <[email protected]>
AuthorDate: Thu, 17 Mar 2016 16:34:00 +0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 21 Mar 2016 11:24:10 +0100

irqchip/mbigen: Adjust DT bindings to handle multiple devices in a module

A mbigen hardware module can contain more than one device node. These device nodes contain the same register definition.

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

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

In this case both devices try to request the same resource resulting in a resource conflict.

To address this problem the devices need to be subnodes of the mbigen hardware module, which then contains the unique register space.

[ tglx: Massaged changelog ]

Signed-off-by Ma Jun <[email protected]>

Cc [email protected]

Link http://lkml.kernel.org/r/20160203111602.GA1234@leverpostej open here

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Thomas Gleixner <[email protected]>

---
 .../interrupt-controller/hisilicon,mbigen-v2.txt   | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt +17 -5

index 720f7c9..3b2f4c4 100644
@@ -21,6 +21,8 @@ Mbigen main node required properties:
 - reg: Specifies the base physical address and size of the Mbigen
   registers.
 
+Mbigen sub node required properties:
+------------------------------------------
 - interrupt controller: Identifies the node as an interrupt controller
 
 - msi-parent: Specifies the MSI controller this mbigen use.

@@ -45,13 +47,23 @@ Mbigen main node required properties:
 
 Examples:
 
-	mbigen_device_gmac:intc {
+	mbigen_chip_dsa {
 			compatible = "hisilicon,mbigen-v2";
 			reg = <0x0 0xc0080000 0x0 0x10000>;
-			interrupt-controller;
-			msi-parent = <&its_dsa 0x40b1c>;
-			num-pins = <9>;
-			#interrupt-cells = <2>;
+
+			mbigen_gmac:intc_gmac {
+				interrupt-controller;
+				msi-parent = <&its_dsa 0x40b1c>;
+				num-pins = <9>;
+				#interrupt-cells = <2>;
+			};
+
+			mbigen_i2c:intc_i2c {
+				interrupt-controller;
+				msi-parent = <&its_dsa 0x40b0e>;
+				num-pins = <2>;
+				#interrupt-cells = <2>;
+			};
 	};
 
 Devices connect to mbigen required properties:

MaJun

author

2016-03-17 08:34 UTC #12 lore raw

PATCH v3 2/2 irqchip/mbigen:Change the mbigen driver based on the new mbigen node definition.

In reply to MaJun

From: Ma Jun <[email protected]>

In current mbigen driver, each mbigen device is initialized as a platform device. When these devices belong to same mbigen hardware module(chip), they use the same register definition in their device node and caused the problem of registers remapped repeatedly.

Now, I try to initialize the mbigen module(chip) as a platform device and remap the register once to fix this problem.

Signed-off-by Ma Jun <[email protected]>

---
 drivers/irqchip/irq-mbigen.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

drivers/irqchip/irq-mbigen.c +21 -9

index 4dd3eb8..4d413bc 100644
@@ -242,6 +242,8 @@ static int mbigen_device_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct irq_domain *domain;
 	u32 num_pins;
+	struct platform_device *child_pdev;
+	struct device_node *np;
 
 	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
 	if (!mgn_chip)

@@ -251,25 +253,35 @@ static int mbigen_device_probe(struct platform_device *pdev)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
+
 	if (IS_ERR(mgn_chip->base))
 		return PTR_ERR(mgn_chip->base);
 
-	if (of_property_read_u32(pdev->dev.of_node, "num-pins", &num_pins) < 0) {
-		dev_err(&pdev->dev, "No num-pins property\n");
-		return -EINVAL;
-	}
+	for_each_child_of_node(pdev->dev.of_node, np) {
+		if (!of_property_read_bool(np, "interrupt-controller"))
+			continue;
+
+		child_pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
+		if (IS_ERR(child_pdev))
+			return PTR_ERR(child_pdev);
+
+		if (of_property_read_u32(child_pdev->dev.of_node, "num-pins", &num_pins) < 0) {
+			dev_err(&pdev->dev, "No num-pins property\n");
+			return -EINVAL;
+		}
 
-	domain = platform_msi_create_device_domain(&pdev->dev, num_pins,
+		domain = platform_msi_create_device_domain(&child_pdev->dev, num_pins,
 							mbigen_write_msg,
 							&mbigen_domain_ops,
 							mgn_chip);
 
-	if (!domain)
-		return -ENOMEM;
+		if (!domain)
+			return -ENOMEM;
 
-	platform_set_drvdata(pdev, mgn_chip);
+		dev_info(&child_pdev->dev, "Allocated %d MSIs\n", num_pins);
+	}
 
-	dev_info(&pdev->dev, "Allocated %d MSIs\n", num_pins);
+	platform_set_drvdata(pdev, mgn_chip);
 
 	return 0;
 }

Signature · 1 line

1.7.1

tip-bot for MaJun

merge bot

2016-03-21 10:33 UTC #13 lore raw

[tip:irq/urgent] irqchip/mbigen: Handle multiple device nodes in a mbigen module

irqchip/mbigen: Handle multiple device nodes in a mbigen module

Commit-ID:  ed2a1002d25ccdb6606c8ccb608524118bd30614
Gitweb:     http://git.kernel.org/tip/ed2a1002d25ccdb6606c8ccb608524118bd30614
Author:     MaJun <[email protected]>
AuthorDate: Thu, 17 Mar 2016 16:34:01 +0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 21 Mar 2016 11:24:11 +0100

irqchip/mbigen: Handle multiple device nodes in a mbigen module

Each mbigen device is represented as a independent platform device. If the devices belong to the same mbigen hardware module, then the register space for these devices is the same. That leads to a resource conflict.

The solution for this is to represent the mbigen module as a platform device and make the mbigen devices subdevices of that. The register space is associated to the mbigen module and therefor the resource conflict is avoided.

[ tglx: Massaged changelog, cleaned up the code and removed the silly printk ]

Signed-off-by Ma Jun <[email protected]>

Cc [email protected]

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Thomas Gleixner <[email protected]>

---
 drivers/irqchip/irq-mbigen.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/irqchip/irq-mbigen.c +24 ~~-14~~

index 4dd3eb8..d67baa2 100644
@@ -239,8 +239,11 @@ static struct irq_domain_ops mbigen_domain_ops = {
 static int mbigen_device_probe(struct platform_device *pdev)
 {
 	struct mbigen_device *mgn_chip;
-	struct resource *res;
+	struct platform_device *child;
 	struct irq_domain *domain;
+	struct device_node *np;
+	struct device *parent;
+	struct resource *res;
 	u32 num_pins;
 
 	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);

@@ -254,23 +257,30 @@ static int mbigen_device_probe(struct platform_device *pdev)
 	if (IS_ERR(mgn_chip->base))
 		return PTR_ERR(mgn_chip->base);
 
-	if (of_property_read_u32(pdev->dev.of_node, "num-pins", &num_pins) < 0) {
-		dev_err(&pdev->dev, "No num-pins property\n");
-		return -EINVAL;
-	}
+	for_each_child_of_node(pdev->dev.of_node, np) {
+		if (!of_property_read_bool(np, "interrupt-controller"))
+			continue;
 
-	domain = platform_msi_create_device_domain(&pdev->dev, num_pins,
-							mbigen_write_msg,
-							&mbigen_domain_ops,
-							mgn_chip);
+		parent = platform_bus_type.dev_root;
+		child = of_platform_device_create(np, NULL, parent);
+		if (IS_ERR(child))
+			return PTR_ERR(child);
 
-	if (!domain)
-		return -ENOMEM;
+		if (of_property_read_u32(child->dev.of_node, "num-pins",
+					 &num_pins) < 0) {
+			dev_err(&pdev->dev, "No num-pins property\n");
+			return -EINVAL;
+		}
+
+		domain = platform_msi_create_device_domain(&child->dev, num_pins,
+							   mbigen_write_msg,
+							   &mbigen_domain_ops,
+							   mgn_chip);
+		if (!domain)
+			return -ENOMEM;
+	}
 
 	platform_set_drvdata(pdev, mgn_chip);
-
-	dev_info(&pdev->dev, "Allocated %d MSIs\n", num_pins);
-
 	return 0;
 }

Thomas Gleixner

2016-03-21 10:29 UTC #14 lore raw

Re: [PATCH v3 0/2] irqchip/mbigen: fix the IO remap problem in mbigen driver.

In reply to MaJun

Quoted 5 lines · MaJun

This patch set is used to fix the problem of remap a set of registers repeatedly in current mbigen driver.
irqchip/mbigen:Change the mbigen node definition in dt binding file irqchip/mbigen:Change the mbigen driver based on the new mbigen node definition

These subject lines are still horrible as they tell nothing about the nature of the change. I fixed them up along with the changelogs and applied them to irq/urgent. Can you spot the difference?

While at it. The config switch for this driver is horrible. It's just in the middle of the device driver configs. Why is this configurable by the user at all? It simply should be selected by arm64 or some subarch configuration there. Please clean that up.

Thanks,

tglx

majun (F)

author

2016-03-22 03:10 UTC #15 lore raw

Re: [PATCH v3 0/2] irqchip/mbigen: fix the IO remap problem in mbigen driver.

? 2016/3/21 18:29, Thomas Gleixner ??:

Quoted 10 lines · MaJun

Quoted 5 lines
This patch set is used to fix the problem of remap a set of registers repeatedly in current mbigen driver.
irqchip/mbigen:Change the mbigen node definition in dt binding file irqchip/mbigen:Change the mbigen driver based on the new mbigen node definition
These subject lines are still horrible as they tell nothing about the nature of the change. I fixed them up along with the changelogs and applied them to irq/urgent. Can you spot the difference?

Yes, after you changing, the subject and change log clearly show why we need to do this change.
I have learned a lot from you :)

Quoted 6 lines

While at it. The config switch for this driver is horrible. It's just in the middle of the device driver configs. Why is this configurable by the user at all? It simply should be selected by arm64 or some subarch configuration there. Please clean that up.

will do.

Thanks!
MaJun

Quoted 6 lines

Thanks,
tglx
.

Marc Zyngier

2016-05-05 14:49 UTC #16 lore raw

Re: [PATCH v3 0/2] irqchip/mbigen: fix the IO remap problem in mbigen driver.

Quoted 19 lines · majun (F)

? 2016/3/21 18:29, Thomas Gleixner ??:
Quoted 10 lines · MaJun
Quoted 5 lines
This patch set is used to fix the problem of remap a set of registers repeatedly in current mbigen driver.
irqchip/mbigen:Change the mbigen node definition in dt binding file irqchip/mbigen:Change the mbigen driver based on the new mbigen node definition
These subject lines are still horrible as they tell nothing about the nature of the change. I fixed them up along with the changelogs and applied them to irq/urgent. Can you spot the difference?
Yes, after you changing, the subject and change log clearly show why we need to do this change.
I have learned a lot from you :)
Quoted 1 line

Quoted 6 lines

Quoted 5 lines
While at it. The config switch for this driver is horrible. It's just in the middle of the device driver configs. Why is this configurable by the user at all? It simply should be selected by arm64 or some subarch configuration there. Please clean that up.
will do.

So what's the status of this? Do you still plan to respin it?

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

majun (F)

author

2016-05-06 01:12 UTC #17 lore raw

Re: [PATCH v3 0/2] irqchip/mbigen: fix the IO remap problem in mbigen driver.

Hi Marc:

? 2016/5/5 22:49, Marc Zyngier ??:

Quoted 20 lines · majun (F)

Quoted 19 lines
? 2016/3/21 18:29, Thomas Gleixner ??:
Quoted 10 lines · MaJun
Quoted 5 lines
This patch set is used to fix the problem of remap a set of registers repeatedly in current mbigen driver.
irqchip/mbigen:Change the mbigen node definition in dt binding file irqchip/mbigen:Change the mbigen driver based on the new mbigen node definition
These subject lines are still horrible as they tell nothing about the nature of the change. I fixed them up along with the changelogs and applied them to irq/urgent. Can you spot the difference?
Yes, after you changing, the subject and change log clearly show why we need to do this change.
I have learned a lot from you :)
Quoted 1 line

Quoted 9 lines

Quoted 6 lines
Quoted 5 lines
While at it. The config switch for this driver is horrible. It's just in the middle of the device driver configs. Why is this configurable by the user at all? It simply should be selected by arm64 or some subarch configuration there. Please clean that up.
will do.
So what's the status of this? Do you still plan to respin it?

This patch set had been ACK by Thomas.
If you mean the mbigen driver config switch problem, the patch for this problem also had been ACK by thomas.
please see the link:
https://lkml.org/lkml/2016/3/23/76

Thanks
Ma Jun

Quoted 4 lines

Thanks,
	M.

Marc Zyngier

2016-05-06 07:32 UTC #18 lore raw

Re: [PATCH v3 0/2] irqchip/mbigen: fix the IO remap problem in mbigen driver.

Quoted 28 lines · majun (F)

Hi Marc:
? 2016/5/5 22:49, Marc Zyngier ??:
Quoted 25 lines · majun (F)
Quoted 24 lines
? 2016/3/21 18:29, Thomas Gleixner ??:
Quoted 10 lines · MaJun
Quoted 5 lines
This patch set is used to fix the problem of remap a set of registers repeatedly in current mbigen driver.
irqchip/mbigen:Change the mbigen node definition in dt binding file irqchip/mbigen:Change the mbigen driver based on the new mbigen node definition
These subject lines are still horrible as they tell nothing about the nature of the change. I fixed them up along with the changelogs and applied them to irq/urgent. Can you spot the difference?
Yes, after you changing, the subject and change log clearly show why we need to do this change.
I have learned a lot from you :)
Quoted 6 lines
While at it. The config switch for this driver is horrible. It's just in the middle of the device driver configs. Why is this configurable by the user at all? It simply should be selected by arm64 or some subarch configuration there. Please clean that up.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
will do.
So what's the status of this? Do you still plan to respin it?
This patch set had been ACK by Thomas.

Interesting. Somehow, the ARM email server has decided not to deliver the tip-bot messages, so I didn't see it was already in.

Sorry for the noise.

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

majun (F)

author

2016-05-06 09:07 UTC #19 lore raw

Re: [PATCH v3 0/2] irqchip/mbigen: fix the IO remap problem in mbigen driver.

Hi Marc:

? 2016/5/6 15:32, Marc Zyngier ??:

Quoted 34 lines · majun (F)

Quoted 33 lines
Hi Marc:
? 2016/5/5 22:49, Marc Zyngier ??:
Quoted 29 lines · majun (F)
Quoted 19 lines
? 2016/3/21 18:29, Thomas Gleixner ??:
Quoted 10 lines · MaJun
Quoted 5 lines
This patch set is used to fix the problem of remap a set of registers repeatedly in current mbigen driver.
irqchip/mbigen:Change the mbigen node definition in dt binding file irqchip/mbigen:Change the mbigen driver based on the new mbigen node definition
These subject lines are still horrible as they tell nothing about the nature of the change. I fixed them up along with the changelogs and applied them to irq/urgent. Can you spot the difference?
Yes, after you changing, the subject and change log clearly show why we need to do this change.
I have learned a lot from you :)
Quoted 1 line
Quoted 6 lines
Quoted 5 lines
While at it. The config switch for this driver is horrible. It's just in the middle of the device driver configs. Why is this configurable by the user at all? It simply should be selected by arm64 or some subarch configuration there. Please clean that up.
will do.
So what's the status of this? Do you still plan to respin it?

Quoted 6 lines

Quoted 1 line
This patch set had been ACK by Thomas.
Interesting. Somehow, the ARM email server has decided not to deliver the tip-bot messages, so I didn't see it was already in.
Sorry for the noise.

Not at all. Thank you for your attention.
MaJun
Thanks!

Quoted 3 lines

M.