This reverts commit f97dbf48ca43009e8b8bcdf07f47fc9f06149b36.
Although converting to a platform driver sounds good, doing this for the
MediaTek devices has a side effect, the probe of the driver is delayed
and the interrupt controller is not available for other drivers when
they probe. For MT8173 that means that the `systimer` and `mtk_cmdq`
drivers will fail to probe with the following errors:
[ 0.000197] Failed to map interrupt for /soc/timer@10008000
[ 0.000209] Failed to initialize '/soc/timer@10008000': -22
[ 0.067386] mtk_cmdq 10212000.mailbox: failed to register ISR (-22)
For MT8183, apart from the above errors, you will get the `pinctrl`
driver without the EINT support, because the `pinctrl` is probed before
the interrupt controller, for what is worth, then it doesn't booting
properly.
[ 0.062309] mt8183-pinctrl 10005000.pinctrl: Failed to add EINT, but pinctrl still can work
While deferring these drivers until the irqchip is available could be an
option, at this point, seems reasonable to revert this commit for now until
all these drivers are properly handling/deferring when the irqchip is not
available.
Cc: Saravana Kannan <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Hanks Chen <[email protected]>
Fixes: f97dbf48ca43 ("irqchip/mtk-sysirq: Convert to a platform driver")
Signed-off-by: Enric Balletbo i Serra <[email protected]>
---
drivers/irqchip/irq-mtk-sysirq.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 7299c5ab4d10..6ff98b87e5c0 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -231,6 +231,4 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
kfree(chip_data);
return ret;
}
-IRQCHIP_PLATFORM_DRIVER_BEGIN(mtk_sysirq)
-IRQCHIP_MATCH("mediatek,mt6577-sysirq", mtk_sysirq_of_init)
-IRQCHIP_PLATFORM_DRIVER_END(mtk_sysirq)
+IRQCHIP_DECLARE(mtk_sysirq, "mediatek,mt6577-sysirq", mtk_sysirq_of_init);
--
2.28.0
On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <[email protected]> wrote:
>
> hi,
>
> does the fix you've linked to my revert [1] not work in your case?
>
> [1] https://patchwork.kernel.org/patch/11718481/
Thanks for pointing it out Frank. Also, might want to avoid top
posting in the future.
Enric, Can you please try that other fix and see if that solves your issue?
Thanks,
Saravana
On 2020-08-19 19:51, Saravana Kannan wrote:
> On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <[email protected]>
> wrote:
>>
>> hi,
>>
>> does the fix you've linked to my revert [1] not work in your case?
>>
>> [1] https://patchwork.kernel.org/patch/11718481/
>
> Thanks for pointing it out Frank. Also, might want to avoid top
> posting in the future.
>
> Enric, Can you please try that other fix and see if that solves your
> issue?
I think Enric was clear that the driver does probe correctly
(meaning that he has the fix in his tree). It is everything else
that breaks, because none of the drivers on the platform are
equipped to defer their own probing.
I think we need to change this works right now, meaning that we can't
blindly change the behaviour of *built-in* drivers. I'll see if I can
come up with something quickly, but I'll otherwise take Enric patch.
M.
--
Jazz is not dead. It just smells funny...
On Thu, Aug 20, 2020 at 12:56 AM Marc Zyngier <[email protected]> wrote:
>
> On 2020-08-19 19:51, Saravana Kannan wrote:
> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <[email protected]>
> > wrote:
> >>
> >> hi,
> >>
> >> does the fix you've linked to my revert [1] not work in your case?
> >>
> >> [1] https://patchwork.kernel.org/patch/11718481/
> >
> > Thanks for pointing it out Frank. Also, might want to avoid top
> > posting in the future.
> >
> > Enric, Can you please try that other fix and see if that solves your
> > issue?
>
> I think Enric was clear that the driver does probe correctly
> (meaning that he has the fix in his tree). It is everything else
> that breaks, because none of the drivers on the platform are
> equipped to defer their own probing.
>
> I think we need to change this works right now, meaning that we can't
> blindly change the behaviour of *built-in* drivers. I'll see if I can
> come up with something quickly, but I'll otherwise take Enric patch.
Sounds fair Marc.
Btw, Enric, out of curiosity, can you try adding "fw_devlink=on" to
your kernel command line to see if it helps? It basically ensures
proper probe ordering without depending on the drivers. There are some
corner cases where it still can't work properly (too much to explain
for a late night email), but if the platforms don't have those corner
cases it'll work perfectly.
I'm fine with the revert if Marc isn't able to find a quick fix to the
drivers, but this might also fix your problem right away.
-Saravana
On 2020-08-20 09:07, Saravana Kannan wrote:
> On Thu, Aug 20, 2020 at 12:56 AM Marc Zyngier <[email protected]> wrote:
>>
>> On 2020-08-19 19:51, Saravana Kannan wrote:
>> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <[email protected]>
>> > wrote:
>> >>
>> >> hi,
>> >>
>> >> does the fix you've linked to my revert [1] not work in your case?
>> >>
>> >> [1] https://patchwork.kernel.org/patch/11718481/
>> >
>> > Thanks for pointing it out Frank. Also, might want to avoid top
>> > posting in the future.
>> >
>> > Enric, Can you please try that other fix and see if that solves your
>> > issue?
>>
>> I think Enric was clear that the driver does probe correctly
>> (meaning that he has the fix in his tree). It is everything else
>> that breaks, because none of the drivers on the platform are
>> equipped to defer their own probing.
>>
>> I think we need to change this works right now, meaning that we can't
>> blindly change the behaviour of *built-in* drivers. I'll see if I can
>> come up with something quickly, but I'll otherwise take Enric patch.
>
> Sounds fair Marc.
>
> Btw, Enric, out of curiosity, can you try adding "fw_devlink=on" to
> your kernel command line to see if it helps? It basically ensures
> proper probe ordering without depending on the drivers. There are some
> corner cases where it still can't work properly (too much to explain
> for a late night email), but if the platforms don't have those corner
> cases it'll work perfectly.
>
> I'm fine with the revert if Marc isn't able to find a quick fix to the
> drivers, but this might also fix your problem right away.
I'm afraid there is no quick fix if we want to preserve the current
behavior with built-in drivers, and not having "fw_devlink=on" by
default makes it irrelevant for most people.
fw_devlink also prevents my test platforms from booting (my rk3399
doesn't find its PCI devices with it), while the same kernel boots
just fine without it. It could well be that the corner case is
likely to be more prevalent than you seem to expect.
I will probably end-up end-up queuing reverts for both mtk-sysirq,
mtk-cirq, and qcom-pdc (the first two can't be built as module with
mainline anyway, and I seem to remember that the latter caused some
controversy as well).
As an experiment, I have pushed out a branch[1] that implements
a "hybrid" probe, retaining the previous early probe mechanism when
the driver is built-in, and letting things rip when built as a
module (if you do that, you hopefully know what you are doing).
I'd welcome some testing on affected platforms (I don't have
anything I can run mainline on that'd be affected).
Thanks,
M.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/hybrid-probe
--
Jazz is not dead. It just smells funny...
On Thu, Aug 20, 2020 at 7:53 AM Marc Zyngier <[email protected]> wrote:
>
> On 2020-08-20 09:07, Saravana Kannan wrote:
> > On Thu, Aug 20, 2020 at 12:56 AM Marc Zyngier <[email protected]> wrote:
> >>
> >> On 2020-08-19 19:51, Saravana Kannan wrote:
> >> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <[email protected]>
> >> > wrote:
> >> >>
> >> >> hi,
> >> >>
> >> >> does the fix you've linked to my revert [1] not work in your case?
> >> >>
> >> >> [1] https://patchwork.kernel.org/patch/11718481/
> >> >
> >> > Thanks for pointing it out Frank. Also, might want to avoid top
> >> > posting in the future.
> >> >
> >> > Enric, Can you please try that other fix and see if that solves your
> >> > issue?
> >>
> >> I think Enric was clear that the driver does probe correctly
> >> (meaning that he has the fix in his tree). It is everything else
> >> that breaks, because none of the drivers on the platform are
> >> equipped to defer their own probing.
> >>
> >> I think we need to change this works right now, meaning that we can't
> >> blindly change the behaviour of *built-in* drivers. I'll see if I can
> >> come up with something quickly, but I'll otherwise take Enric patch.
> >
> > Sounds fair Marc.
> >
> > Btw, Enric, out of curiosity, can you try adding "fw_devlink=on" to
> > your kernel command line to see if it helps? It basically ensures
> > proper probe ordering without depending on the drivers. There are some
> > corner cases where it still can't work properly (too much to explain
> > for a late night email), but if the platforms don't have those corner
> > cases it'll work perfectly.
> >
> > I'm fine with the revert if Marc isn't able to find a quick fix to the
> > drivers, but this might also fix your problem right away.
>
> I'm afraid there is no quick fix if we want to preserve the current
> behavior with built-in drivers,
> and not having "fw_devlink=on" by
> default makes it irrelevant for most people.
Agreed.
> fw_devlink also prevents my test platforms from booting (my rk3399
> doesn't find its PCI devices with it), while the same kernel boots
> just fine without it. It could well be that the corner case is
> likely to be more prevalent than you seem to expect.
Yeah, I know it has a few corner cases I need to deal with.
> I will probably end-up end-up queuing reverts for both mtk-sysirq,
> mtk-cirq, and qcom-pdc (the first two can't be built as module with
> mainline anyway, and I seem to remember that the latter caused some
> controversy as well).
>
> As an experiment, I have pushed out a branch[1] that implements
> a "hybrid" probe, retaining the previous early probe mechanism when
> the driver is built-in, and letting things rip when built as a
> module (if you do that, you hopefully know what you are doing).
> I'd welcome some testing on affected platforms (I don't have
> anything I can run mainline on that'd be affected).
I like [1] and the code looks good. Hopefully, we can stick with that.
-Saravana
Hi Marc,
On 20/8/20 16:53, Marc Zyngier wrote:
> On 2020-08-20 09:07, Saravana Kannan wrote:
>> On Thu, Aug 20, 2020 at 12:56 AM Marc Zyngier <[email protected]> wrote:
>>>
>>> On 2020-08-19 19:51, Saravana Kannan wrote:
>>> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <[email protected]>
>>> > wrote:
>>> >>
>>> >> hi,
>>> >>
>>> >> does the fix you've linked to my revert [1] not work in your case?
>>> >>
>>> >> [1] https://patchwork.kernel.org/patch/11718481/
>>> >
>>> > Thanks for pointing it out Frank. Also, might want to avoid top
>>> > posting in the future.
>>> >
>>> > Enric, Can you please try that other fix and see if that solves your
>>> > issue?
>>>
>>> I think Enric was clear that the driver does probe correctly
>>> (meaning that he has the fix in his tree). It is everything else
>>> that breaks, because none of the drivers on the platform are
>>> equipped to defer their own probing.
>>>
>>> I think we need to change this works right now, meaning that we can't
>>> blindly change the behaviour of *built-in* drivers. I'll see if I can
>>> come up with something quickly, but I'll otherwise take Enric patch.
>>
>> Sounds fair Marc.
>>
>> Btw, Enric, out of curiosity, can you try adding "fw_devlink=on" to
>> your kernel command line to see if it helps? It basically ensures
>> proper probe ordering without depending on the drivers. There are some
>> corner cases where it still can't work properly (too much to explain
>> for a late night email), but if the platforms don't have those corner
>> cases it'll work perfectly.
>>
>> I'm fine with the revert if Marc isn't able to find a quick fix to the
>> drivers, but this might also fix your problem right away.
>
> I'm afraid there is no quick fix if we want to preserve the current
> behavior with built-in drivers, and not having "fw_devlink=on" by
> default makes it irrelevant for most people.
>
> fw_devlink also prevents my test platforms from booting (my rk3399
> doesn't find its PCI devices with it), while the same kernel boots
> just fine without it. It could well be that the corner case is
> likely to be more prevalent than you seem to expect.
>
> I will probably end-up end-up queuing reverts for both mtk-sysirq,
> mtk-cirq, and qcom-pdc (the first two can't be built as module with
> mainline anyway, and I seem to remember that the latter caused some
> controversy as well).
>
> As an experiment, I have pushed out a branch[1] that implements
> a "hybrid" probe, retaining the previous early probe mechanism when
> the driver is built-in, and letting things rip when built as a
> module (if you do that, you hopefully know what you are doing).
> I'd welcome some testing on affected platforms (I don't have
> anything I can run mainline on that'd be affected).
>
Unfortunately, my Kukui (MT8183) board doesn't boot at all with those patches. I
only did a quick test and I didn't dig further, please let me know if you want I
debug more the issue. IMHO, right now, the revert seems to be the better
solution for this cycle.
Thanks,
Enric
> Thanks,
>
> M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/hybrid-probe
>
Hi Enric,
On 2020-08-21 10:20, Enric Balletbo i Serra wrote:
> Hi Marc,
>
> On 20/8/20 16:53, Marc Zyngier wrote:
>> On 2020-08-20 09:07, Saravana Kannan wrote:
>>> On Thu, Aug 20, 2020 at 12:56 AM Marc Zyngier <[email protected]> wrote:
>>>>
>>>> On 2020-08-19 19:51, Saravana Kannan wrote:
>>>> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <[email protected]>
>>>> > wrote:
>>>> >>
>>>> >> hi,
>>>> >>
>>>> >> does the fix you've linked to my revert [1] not work in your case?
>>>> >>
>>>> >> [1] https://patchwork.kernel.org/patch/11718481/
>>>> >
>>>> > Thanks for pointing it out Frank. Also, might want to avoid top
>>>> > posting in the future.
>>>> >
>>>> > Enric, Can you please try that other fix and see if that solves your
>>>> > issue?
>>>>
>>>> I think Enric was clear that the driver does probe correctly
>>>> (meaning that he has the fix in his tree). It is everything else
>>>> that breaks, because none of the drivers on the platform are
>>>> equipped to defer their own probing.
>>>>
>>>> I think we need to change this works right now, meaning that we
>>>> can't
>>>> blindly change the behaviour of *built-in* drivers. I'll see if I
>>>> can
>>>> come up with something quickly, but I'll otherwise take Enric patch.
>>>
>>> Sounds fair Marc.
>>>
>>> Btw, Enric, out of curiosity, can you try adding "fw_devlink=on" to
>>> your kernel command line to see if it helps? It basically ensures
>>> proper probe ordering without depending on the drivers. There are
>>> some
>>> corner cases where it still can't work properly (too much to explain
>>> for a late night email), but if the platforms don't have those corner
>>> cases it'll work perfectly.
>>>
>>> I'm fine with the revert if Marc isn't able to find a quick fix to
>>> the
>>> drivers, but this might also fix your problem right away.
>>
>> I'm afraid there is no quick fix if we want to preserve the current
>> behavior with built-in drivers, and not having "fw_devlink=on" by
>> default makes it irrelevant for most people.
>>
>> fw_devlink also prevents my test platforms from booting (my rk3399
>> doesn't find its PCI devices with it), while the same kernel boots
>> just fine without it. It could well be that the corner case is
>> likely to be more prevalent than you seem to expect.
>>
>> I will probably end-up end-up queuing reverts for both mtk-sysirq,
>> mtk-cirq, and qcom-pdc (the first two can't be built as module with
>> mainline anyway, and I seem to remember that the latter caused some
>> controversy as well).
>>
>> As an experiment, I have pushed out a branch[1] that implements
>> a "hybrid" probe, retaining the previous early probe mechanism when
>> the driver is built-in, and letting things rip when built as a
>> module (if you do that, you hopefully know what you are doing).
>> I'd welcome some testing on affected platforms (I don't have
>> anything I can run mainline on that'd be affected).
>>
>
> Unfortunately, my Kukui (MT8183) board doesn't boot at all with those
> patches. I
> only did a quick test and I didn't dig further, please let me know if
> you want I
> debug more the issue. IMHO, right now, the revert seems to be the
> better
> solution for this cycle.
It'd be good if you could help with that, but I will definitely apply
the revert (below for the revert list). Any change is too invasive to
be added to this cycle.
920ecb8c35cb irqchip/mtk-cirq: Convert to a platform driver
f97dbf48ca43 irqchip/mtk-sysirq: Convert to a platform driver
5be57099d445 irqchip/qcom-pdc: Switch to using IRQCHIP_PLATFORM_DRIVER
helper macros
95bf9305d2e3 irqchip/qcom-pdc: Allow QCOM_PDC to be loadable as a
permanent module
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Am 21. August 2020 12:17:36 MESZ schrieb Marc Zyngier <[email protected]>:
>It'd be good if you could help with that, but I will definitely apply
>the revert (below for the revert list). Any change is too invasive to
>be added to this cycle.
>
>920ecb8c35cb irqchip/mtk-cirq: Convert to a platform driver
>f97dbf48ca43 irqchip/mtk-sysirq: Convert to a platform driver
>5be57099d445 irqchip/qcom-pdc: Switch to using IRQCHIP_PLATFORM_DRIVER
>helper macros
with Patch "irqchip: Fix probing deferal when using IRQCHIP_PLATFORM_DRIVER helper" i can boot my board, but i get these errors:
[ 0.014234] irq: no irq domain found for interrupt-controller@10200100 !
[ 0.020981] Failed to map interrupt for /timer@10008000
[ 0.026248] Failed to initialize '/timer@10008000': -22
[ 4.314126] hw perfevents: /pmu: failed to register PMU devices!
if i revert f97dbf48ca43009e8b8bcdf07f47fc9f06149b36 these are gone
So from my pov revert is best way at the moment
regards Frank
The following commit has been merged into the irq/urgent branch of tip:
Commit-ID: a150dac5a8fb711fdc378c23f44bee4546f04246
Gitweb: https://git.kernel.org/tip/a150dac5a8fb711fdc378c23f44bee4546f04246
Author: Marc Zyngier <[email protected]>
AuthorDate: Tue, 25 Aug 2020 10:38:39 +01:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Tue, 25 Aug 2020 10:48:54 +01:00
irqchip: Revert modular support for drivers using IRQCHIP_PLATFORM_DRIVER helperse
It has become obvious that switching a number of irqchip drivers
to being platform drivers without considering the platform was a
mistake. We have multiple reports of end-point drivers not
probing because the irqchip driver isn't there yet, breaking
the expectations of the users.
This patch reverts:
920ecb8c35cb ("irqchip/mtk-cirq: Convert to a platform driver")
f97dbf48ca43 ("irqchip/mtk-sysirq: Convert to a platform driver")
5be57099d445 ("irqchip/qcom-pdc: Switch to using IRQCHIP_PLATFORM_DRIVER helper macros")
95bf9305d2e3 ("irqchip/qcom-pdc: Allow QCOM_PDC to be loadable as a permanent module")
and leave QCOM PDC, MTK sysrq and cirq drivers as built-in, special purpose
drivers for the time being until we have worked out a better solution.
Reported-by: Enric Balletbo i Serra <[email protected]>
Reported-by: Frank Wunderlich <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/Kconfig | 2 +-
drivers/irqchip/irq-mtk-cirq.c | 4 +---
drivers/irqchip/irq-mtk-sysirq.c | 4 +---
drivers/irqchip/qcom-pdc.c | 8 +-------
4 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bb70b71..bfc9719 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -425,7 +425,7 @@ config GOLDFISH_PIC
for Goldfish based virtual platforms.
config QCOM_PDC
- tristate "QCOM PDC"
+ bool "QCOM PDC"
depends on ARCH_QCOM
select IRQ_DOMAIN_HIERARCHY
help
diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c
index 62a6127..69ba8ce 100644
--- a/drivers/irqchip/irq-mtk-cirq.c
+++ b/drivers/irqchip/irq-mtk-cirq.c
@@ -295,6 +295,4 @@ out_free:
return ret;
}
-IRQCHIP_PLATFORM_DRIVER_BEGIN(mtk_cirq)
-IRQCHIP_MATCH("mediatek,mtk-cirq", mtk_cirq_of_init)
-IRQCHIP_PLATFORM_DRIVER_END(mtk_cirq)
+IRQCHIP_DECLARE(mtk_cirq, "mediatek,mtk-cirq", mtk_cirq_of_init);
diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 7299c5a..6ff98b8 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -231,6 +231,4 @@ out_free_chip:
kfree(chip_data);
return ret;
}
-IRQCHIP_PLATFORM_DRIVER_BEGIN(mtk_sysirq)
-IRQCHIP_MATCH("mediatek,mt6577-sysirq", mtk_sysirq_of_init)
-IRQCHIP_PLATFORM_DRIVER_END(mtk_sysirq)
+IRQCHIP_DECLARE(mtk_sysirq, "mediatek,mt6577-sysirq", mtk_sysirq_of_init);
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index c1c5dfa..6ae9e1f 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -11,11 +11,9 @@
#include <linux/irqdomain.h>
#include <linux/io.h>
#include <linux/kernel.h>
-#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
-#include <linux/of_irq.h>
#include <linux/soc/qcom/irq.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
@@ -432,8 +430,4 @@ fail:
return ret;
}
-IRQCHIP_PLATFORM_DRIVER_BEGIN(qcom_pdc)
-IRQCHIP_MATCH("qcom,pdc", qcom_pdc_init)
-IRQCHIP_PLATFORM_DRIVER_END(qcom_pdc)
-MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Power Domain Controller");
-MODULE_LICENSE("GPL v2");
+IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init);