From: Kefeng Wang <hidden> Date: 2021-08-16 07:43:23
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Kefeng Wang <hidden> Date: 2021-08-16 07:43:19
After commit 77a7300abad7 ("of/irq: Get rid of NO_IRQ usage"),
no irq case has been removed, irq_of_parse_and_map() will return
0 in all cases when get error from parse and map an interrupt into
linux virq space.
amba_device_register() is only used on no-DT initialization, see
s3c64xx_pl080_init() arch/arm/mach-s3c/pl080.c
ep93xx_init_devices() arch/arm/mach-ep93xx/core.c
They won't set -1 to irq[0], so no need the warn.
This reverts commit 2eac58d5026e4ec8b17ff8b62877fea9e1d2f1b3.
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 3 ---
1 file changed, 3 deletions(-)
From: Kefeng Wang <hidden> Date: 2021-08-16 07:43:25
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
From: Rob Herring <robh@kernel.org> Date: 2021-08-17 22:27:42
On Mon, Aug 16, 2021 at 03:46:16PM +0800, Kefeng Wang wrote:
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
From: Kefeng Wang <hidden> Date: 2021-08-23 02:19:36
On 2021/8/18 6:27, Rob Herring wrote:
On Mon, Aug 16, 2021 at 03:46:16PM +0800, Kefeng Wang wrote:
quoted
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
Thanks Rob.
Hi Russell, should I send the patches to the ARM patch system?
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-08-23 09:05:32
On Mon, Aug 23, 2021 at 10:19:23AM +0800, Kefeng Wang wrote:
On 2021/8/18 6:27, Rob Herring wrote:
quoted
On Mon, Aug 16, 2021 at 03:46:16PM +0800, Kefeng Wang wrote:
quoted
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
Thanks Rob.
Hi Russell, should I send the patches to the ARM patch system?
From: Kefeng Wang <hidden> Date: 2021-08-23 10:57:48
On 2021/8/23 17:05, Russell King (Oracle) wrote:
On Mon, Aug 23, 2021 at 10:19:23AM +0800, Kefeng Wang wrote:
quoted
On 2021/8/18 6:27, Rob Herring wrote:
quoted
On Mon, Aug 16, 2021 at 03:46:16PM +0800, Kefeng Wang wrote:
quoted
Patch 1 and 2 make some cleanup, and patch 3 use of_irq_get() instead of
irq_of_parse_and_map() to get irq number, return -EPROBE_DEFER if the irq
domain is not yet created, amba_device_add() will properly to handle the
no IRQ domain issue via deferred probe.
Kefeng Wang (3):
amba: Drop unused functions about APB/AHB devices add
Revert "ARM: amba: make use of -1 IRQs warn"
amba: Properly handle device probe without IRQ domain
drivers/amba/bus.c | 100 ++++++++++-----------------------------
drivers/of/platform.c | 6 +--
include/linux/amba/bus.h | 18 -------
3 files changed, 27 insertions(+), 97 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
Thanks Rob.
Hi Russell, should I send the patches to the ARM patch system?
Yes please - I'll try to squeeze it in for this cycle but it's getting
a tad late for that. Thanks.
Done, but the sequence of patches is reordered at ARM patch system,
(using git send-email
and deliver patch1/2/3 in order).
BTW, could you give me some direction the following patchset[1] too if
you have time, I have
addressed your comments and resend, but there's been no new feedback for
a long time.
If it is too late for this cycle, I could resend them after 5.15-rc1.
Many thanks.
[1]
https://lore.kernel.org/linux-arm-kernel/20210610123556.171328-1-wangkefeng.wang@huawei.com/
From: Rob Herring <robh+dt@kernel.org> Date: 2021-08-24 20:05:39
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted hunk
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
-Saravana
quoted
ret = request_resource(parent, &dev->res);
if (ret)
goto err_out;
From: Kefeng Wang <hidden> Date: 2021-08-25 04:05:20
On 2021/8/25 4:08, Saravana Kannan wrote:
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
-Saravana
[1] - https://lore.kernel.org/lkml/CAGETcx8b228nDUho3cX9AAQ-pXOfZTMv8cj2vhdx9yc_pk8q+A@mail.gmail.com/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Kefeng Wang <hidden> Date: 2021-08-25 08:40:01
On 2021/8/25 16:04, Saravana Kannan wrote:
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
quoted
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
Sure，I will update it and resend.
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
From: Rob Herring <robh+dt@kernel.org> Date: 2021-08-25 12:33:56
On Tue, Aug 24, 2021 at 11:05 PM Kefeng Wang [off-list ref] wrote:
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
From: Kefeng Wang <hidden> Date: 2021-08-25 14:41:32
On 2021/8/25 20:33, Rob Herring wrote:
On Tue, Aug 24, 2021 at 11:05 PM Kefeng Wang [off-list ref] wrote:
...
quoted
quoted
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
I wonder if we could just remove these. Why does userspace need them
in the first place? It's only an ABI if someone notices. Looking at
the history, AMBA bus was added in 2003 with just 'irq' and then
changed (ABI break) in 2004 to 'irq0' and 'irq1'.
Rob
@@ -20,8 +20,6 @@#include<linux/platform_device.h>#include<linux/reset.h>-#include<asm/irq.h>-#define to_amba_driver(d) container_of(d, struct amba_driver, drv)/* called on periphid match and class 0x9 coresight device. */
@@ -467,10 +463,6 @@ static int amba_device_try_add(struct amba_device
*dev, struct resource *parent)
if (ret)
goto err_release;
- if (dev->irq[0])
- ret = device_create_file(&dev->dev, &dev_attr_irq0);
- if (ret == 0 && dev->irq[1])
- ret = device_create_file(&dev->dev, &dev_attr_irq1);
and do some cleanup about error handling in the next version.
From: Kefeng Wang <hidden> Date: 2021-08-26 02:45:28
On 2021/8/25 16:04, Saravana Kannan wrote:
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
quoted
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
I will resend with put it into amba_probe.
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Got it, and I could resend all combine your patch(due to context conflict
when changing same function) if you no object.
On Wed, Aug 25, 2021 at 7:45 PM Kefeng Wang [off-list ref] wrote:
On 2021/8/25 16:04, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
drivers/amba/bus.c | 27 +++++++++++++++++++++++++++
drivers/of/platform.c | 6 +-----
2 files changed, 28 insertions(+), 5 deletions(-)
@@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev)kfree(d);}+staticintof_amba_device_decode_irq(structamba_device*dev)+{+structdevice_node*node=dev->dev.of_node;+inti,irq=0;++if(IS_ENABLED(CONFIG_OF_IRQ)&&node){+/* Decode the IRQs and address ranges */+for(i=0;i<AMBA_NR_IRQS;i++){+irq=of_irq_get(node,i);+if(irq<0){+if(irq==-EPROBE_DEFER)+returnirq;+irq=0;+}++dev->irq[i]=irq;+}+}++return0;+}+staticintamba_device_try_add(structamba_device*dev,structresource*parent){u32size;void__iomem*tmp;inti,ret;+ret=of_amba_device_decode_irq(dev);+if(ret)+gotoerr_out;+
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
quoted
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
I will resend with put it into amba_probe.
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Got it, and I could resend all combine your patch(due to context conflict
when changing same function) if you no object.
If you want to resolve the conflict with my patch and resend it while
keeping me as the author, I would definitely appreciate it.
-Saravana
From: Kefeng Wang <hidden> Date: 2021-08-26 06:22:30
On 2021/8/26 12:45, Saravana Kannan wrote:
On Wed, Aug 25, 2021 at 7:45 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 16:04, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 9:05 PM Kefeng Wang [off-list ref] wrote:
quoted
On 2021/8/25 4:08, Saravana Kannan wrote:
quoted
On Tue, Aug 24, 2021 at 1:05 PM Rob Herring [off-list ref] wrote:
quoted
+Saravana
Saravana mentioned to me there may be some issues with this one...
On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang [off-list ref] wrote:
quoted
of_amba_device_create() uses irq_of_parse_and_map() to translate
a DT interrupt specification into a Linux virtual interrupt number.
But it doesn't properly handle the case where the interrupt controller
is not yet available, eg, when pl011 interrupt is connected to MBIGEN
interrupt controller, because the mbigen initialization is too late,
which will lead to no IRQ due to no IRQ domain found, log is shown below,
"irq: no irq domain found for uart0 !"
use of_irq_get() to return -EPROBE_DEFER as above, and in the function
amba_device_try_add()/amba_device_add(), it will properly handle in such
case, also return 0 in other fail cases to be consistent as before.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Reported-by: Ruizhe Lin <redacted>
Signed-off-by: Kefeng Wang <redacted>
---
...
quoted
quoted
quoted
quoted
Similar to other resources the AMBA bus "gets" for the device, I think
this should be moved into amba_probe() and not here. There's no reason
to delay the addition of the device (and loading its module) because
the IRQ isn't ready yet.
The following code in the amba_device_try_add() will be called, it uses irq[0]
and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add().
470 if (dev->irq[0])
471 ret = device_create_file(&dev->dev, &dev_attr_irq0);
472 if (ret == 0 && dev->irq[1])
473 ret = device_create_file(&dev->dev, &dev_attr_irq1);
474 if (ret == 0)
475 return ret;
of_amba_device_decode_irq() in amba_device_try_add() won't lead to issue,
only delay the device add, right?
But delaying the device add is the issue. For example, adding a device
could trigger the loading of the corresponding module using uevents.
But now this change would delay that step. That can have other
unintended consequences -- slowing down boot, what if the driver was
working fine without the IRQ, etc.
quoted
If make it into amba_probe(), the above code should be moved too, could we
make a new patch to move both of them, or don't move them?
I'd say move them both. If Russell hasn't already picked this up, then
I'd say redo your Patch 3/3.
I will resend with put it into amba_probe.
quoted
Btw, I've been working on [1] cleaning up the one-off deferred probe
solution that we have for amba devices. That causes a bunch of other
headaches. Your patch 3/3 takes us further in the wrong direction by
adding more reasons for delaying the addition of the device.
Got it, and I could resend all combine your patch(due to context conflict
when changing same function) if you no object.
If you want to resolve the conflict with my patch and resend it while
keeping me as the author, I would definitely appreciate it.
Yes, I will keep it, and rebase my patch based on it.