| Message ID | 20200819161907.1155110-1-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "irqchip/mtk-sysirq: Convert to a platform driver" | expand |
On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <wichtig@fw-web.de> 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 <wichtig@fw-web.de> > 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.
On Thu, Aug 20, 2020 at 12:56 AM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-08-19 19:51, Saravana Kannan wrote: > > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <wichtig@fw-web.de> > > 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 <maz@kernel.org> wrote: >> >> On 2020-08-19 19:51, Saravana Kannan wrote: >> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <wichtig@fw-web.de> >> > 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
On Thu, Aug 20, 2020 at 7:53 AM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-08-20 09:07, Saravana Kannan wrote: > > On Thu, Aug 20, 2020 at 12:56 AM Marc Zyngier <maz@kernel.org> wrote: > >> > >> On 2020-08-19 19:51, Saravana Kannan wrote: > >> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <wichtig@fw-web.de> > >> > 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 <maz@kernel.org> wrote: >>> >>> On 2020-08-19 19:51, Saravana Kannan wrote: >>> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <wichtig@fw-web.de> >>> > 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 <maz@kernel.org> wrote: >>>> >>>> On 2020-08-19 19:51, Saravana Kannan wrote: >>>> > On Wed, Aug 19, 2020 at 9:52 AM Frank Wunderlich <wichtig@fw-web.de> >>>> > 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.
Am 21. August 2020 12:17:36 MESZ schrieb Marc Zyngier <maz@kernel.org>: >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
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);
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 <saravanak@google.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Hanks Chen <hanks.chen@mediatek.com> Fixes: f97dbf48ca43 ("irqchip/mtk-sysirq: Convert to a platform driver") Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> --- drivers/irqchip/irq-mtk-sysirq.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)