| Message ID | 1484699402-28738-2-git-send-email-javier@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Javier, On 2017-01-18 01:30, Javier Martinez Canillas wrote: > Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when > CONFIG_PM is unset") removed the implicit dependency that the driver > had with CONFIG_PM, since it relied on the config option to be enabled. > > In order to work with !CONFIG_PM, the GSC reset logic that happens in > the runtime resume callback had to be executed on the probe function. > > The problem is that if CONFIG_PM is enabled, the power domain for the > GSC could be disabled and so an attempt to write to the GSC_SW_RESET > register leads to an unhandled fault / imprecise external abort error: Driver core ensures that driver's probe() is called with respective power domain turned on, so this is not the right reason for the proposed change. > [ 10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000 > [ 10.186982] pgd = ed728000 > [ 10.190847] [00000000] *pgd=00000000 > [ 10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM > [ 10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) > [ 10.237134] task: ed49e400 task.stack: ed724000 > [ 10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc] > [ 10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc] > [ 10.256139] pc : [<bf2429e0>] lr : [<bf240734>] psr: 60070013 > [ 10.256139] sp : ed725d30 ip : 00000000 fp : 00000001 > [ 10.271492] r10: eea74800 r9 : ecd6a2c0 r8 : ed7d8854 > [ 10.277912] r7 : ed7d8c08 r6 : ed7d8810 r5 : ffff8ecd r4 : c0c03900 > [ 10.285664] r3 : 00000000 r2 : 00000001 r1 : ed7d8b98 r0 : ed7d8810 > > So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the > runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl, > making the reset in probe unneeded. > > Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset") > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> Frankly, I don't get why this change is needed. > > --- > > I-ve only tested with CONFIG_PM enabled since my Exynos5422 Odroid > XU4 board fails to boot when the config option is disabled. > > Best regards, > Javier > > drivers/media/platform/exynos-gsc/gsc-core.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c > index 83272f10722d..42e1e09ea915 100644 > --- a/drivers/media/platform/exynos-gsc/gsc-core.c > +++ b/drivers/media/platform/exynos-gsc/gsc-core.c > @@ -1083,8 +1083,10 @@ static int gsc_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, gsc); > > - gsc_hw_set_sw_reset(gsc); > - gsc_wait_reset(gsc); > + if (!IS_ENABLED(CONFIG_PM)) { > + gsc_hw_set_sw_reset(gsc); > + gsc_wait_reset(gsc); > + } > > vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32)); > Best regards
Hello Marek, Thanks a lot for your feedback. On 01/19/2017 11:17 AM, Marek Szyprowski wrote: > Hi Javier, > > On 2017-01-18 01:30, Javier Martinez Canillas wrote: >> Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when >> CONFIG_PM is unset") removed the implicit dependency that the driver >> had with CONFIG_PM, since it relied on the config option to be enabled. >> >> In order to work with !CONFIG_PM, the GSC reset logic that happens in >> the runtime resume callback had to be executed on the probe function. >> >> The problem is that if CONFIG_PM is enabled, the power domain for the >> GSC could be disabled and so an attempt to write to the GSC_SW_RESET >> register leads to an unhandled fault / imprecise external abort error: > > Driver core ensures that driver's probe() is called with respective power > domain turned on, so this is not the right reason for the proposed change. > Ok, I misunderstood the relationship between runtime PM and the power domains then. I thought the power domains were only powered on when the runtime PM framework resumed an associated device (i.e: pm_runtime_get_sync() is called). But even if this isn't the case, shouldn't the reset in probe only be needed if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message). >> [ 10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000 >> [ 10.186982] pgd = ed728000 >> [ 10.190847] [00000000] *pgd=00000000 >> [ 10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM >> [ 10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) >> [ 10.237134] task: ed49e400 task.stack: ed724000 >> [ 10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc] >> [ 10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc] >> [ 10.256139] pc : [<bf2429e0>] lr : [<bf240734>] psr: 60070013 >> [ 10.256139] sp : ed725d30 ip : 00000000 fp : 00000001 >> [ 10.271492] r10: eea74800 r9 : ecd6a2c0 r8 : ed7d8854 >> [ 10.277912] r7 : ed7d8c08 r6 : ed7d8810 r5 : ffff8ecd r4 : c0c03900 >> [ 10.285664] r3 : 00000000 r2 : 00000001 r1 : ed7d8b98 r0 : ed7d8810 >> >> So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the >> runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl, >> making the reset in probe unneeded. >> >> Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset") >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> > > Frankly, I don't get why this change is needed. > Yes, it seems $SUBJECT is just papering over the real issue. There's something really wrong with the Exynos power domains, I see that PDs can't be disabled by the genpd framework, exynos_pd_power_off() fail: # dmesg | grep power-domain [ 4.893318] Power domain power-domain@10044020 disable failed [ 4.893342] Power domain power-domain@10044120 disable failed [ 4.893711] Power domain power-domain@10044000 disable failed [ 12.690052] Power domain power-domain@10044000 disable failed [ 12.703963] Power domain power-domain@10044000 disable failed So PD are kept on even when unused / attached devices are suspended. Only the mfc_pd (power-domain@10044060) is correctly turned off. # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary domain status slaves /device runtime status ---------------------------------------------------------------------- power-domain@100440C0 on /devices/platform/soc/14450000.mixer active /devices/platform/soc/14530000.hdmi active power-domain@10044120 on power-domain@10044060 off-0 /devices/platform/soc/11000000.codec suspended power-domain@10044020 on power-domain@10044000 on /devices/platform/soc/13e00000.video-scaler suspended /devices/platform/soc/13e10000.video-scaler suspended Also when removing the exynos_gsc driver, I get the same error: # rmmod s5p_mfc [ 106.405972] s5p-mfc 11000000.codec: Removing 11000000.codec # rmmod exynos_gsc [ 227.008559] Unhandled fault: imprecise external abort (0x1c06) at 0x00048e14 [ 227.015116] pgd = ed5dc000 [ 227.017213] [00048e14] *pgd=b17c6835 [ 227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM ... [ 227.241585] [<bf2429bc>] (gsc_wait_reset [exynos_gsc]) from [<bf24009c>] (gsc_runtime_resume+0x9c/0xec [exynos_gsc]) [ 227.252331] [<bf24009c>] (gsc_runtime_resume [exynos_gsc]) from [<c042e488>] (genpd_runtime_resume+0x120/0x1d4) [ 227.262294] [<c042e488>] (genpd_runtime_resume) from [<c04241c0>] (__rpm_callback+0xc8/0x218) # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary domain status slaves /device runtime status ---------------------------------------------------------------------- power-domain@100440C0 on /devices/platform/soc/14450000.mixer active /devices/platform/soc/14530000.hdmi active power-domain@10044120 on power-domain@10044060 off-0 power-domain@10044020 on power-domain@10044000 on /devices/platform/soc/13e00000.video-scaler suspended /devices/platform/soc/13e10000.video-scaler resuming This seems to be caused by some needed clocks to access the power domains to be gated, since I don't get these erros when passing clk_ignore_unused as parameter in the kernel command line. Best regards,
Hi Javier, On 2017-01-19 15:56, Javier Martinez Canillas wrote: > Thanks a lot for your feedback. > > On 01/19/2017 11:17 AM, Marek Szyprowski wrote: >> On 2017-01-18 01:30, Javier Martinez Canillas wrote: >>> Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when >>> CONFIG_PM is unset") removed the implicit dependency that the driver >>> had with CONFIG_PM, since it relied on the config option to be enabled. >>> >>> In order to work with !CONFIG_PM, the GSC reset logic that happens in >>> the runtime resume callback had to be executed on the probe function. >>> >>> The problem is that if CONFIG_PM is enabled, the power domain for the >>> GSC could be disabled and so an attempt to write to the GSC_SW_RESET >>> register leads to an unhandled fault / imprecise external abort error: >> Driver core ensures that driver's probe() is called with respective power >> domain turned on, so this is not the right reason for the proposed change. > Ok, I misunderstood the relationship between runtime PM and the power domains > then. I thought the power domains were only powered on when the runtime PM > framework resumed an associated device (i.e: pm_runtime_get_sync() is called). Power domains are implemented transparently for the drivers. Even when driver doesn't support runtime pm, but its device is in the power domain, the core will ensure that the domain will be turned on all the time the driver is bound to the device. > But even if this isn't the case, shouldn't the reset in probe only be needed > if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message). This looks like an over-engineering. I don't like polluting driver code with conditional statements like IS_ENABLED(CONFIG_*). It should not hurt to reset the device in driver probe, especially just in case the device was left in some partially configured/working state by bootloader or previous kernel run (if started from kexec). Adding this conditional code to avoid some issues with power domain or clocks configuration also suggests that one should instead solve the problem elsewhere. Driver should be able to access device registers in its probe() in any case without the additional hacks. >>> [ 10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000 >>> [ 10.186982] pgd = ed728000 >>> [ 10.190847] [00000000] *pgd=00000000 >>> [ 10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM >>> [ 10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) >>> [ 10.237134] task: ed49e400 task.stack: ed724000 >>> [ 10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc] >>> [ 10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc] >>> [ 10.256139] pc : [<bf2429e0>] lr : [<bf240734>] psr: 60070013 >>> [ 10.256139] sp : ed725d30 ip : 00000000 fp : 00000001 >>> [ 10.271492] r10: eea74800 r9 : ecd6a2c0 r8 : ed7d8854 >>> [ 10.277912] r7 : ed7d8c08 r6 : ed7d8810 r5 : ffff8ecd r4 : c0c03900 >>> [ 10.285664] r3 : 00000000 r2 : 00000001 r1 : ed7d8b98 r0 : ed7d8810 >>> >>> So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the >>> runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl, >>> making the reset in probe unneeded. >>> >>> Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset") >>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> >> Frankly, I don't get why this change is needed. >> > Yes, it seems $SUBJECT is just papering over the real issue. There's > something really wrong with the Exynos power domains, I see that PDs > can't be disabled by the genpd framework, exynos_pd_power_off() fail: > > # dmesg | grep power-domain > [ 4.893318] Power domain power-domain@10044020 disable failed > [ 4.893342] Power domain power-domain@10044120 disable failed > [ 4.893711] Power domain power-domain@10044000 disable failed > [ 12.690052] Power domain power-domain@10044000 disable failed > [ 12.703963] Power domain power-domain@10044000 disable failed > > So PD are kept on even when unused / attached devices are suspended. > Only the mfc_pd (power-domain@10044060) is correctly turned off. > > # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > domain status slaves > /device runtime status > ---------------------------------------------------------------------- > power-domain@100440C0 on > /devices/platform/soc/14450000.mixer active > /devices/platform/soc/14530000.hdmi active > power-domain@10044120 on > power-domain@10044060 off-0 > /devices/platform/soc/11000000.codec suspended > power-domain@10044020 on > power-domain@10044000 on > /devices/platform/soc/13e00000.video-scaler suspended > /devices/platform/soc/13e10000.video-scaler suspended > > Also when removing the exynos_gsc driver, I get the same error: > > # rmmod s5p_mfc > [ 106.405972] s5p-mfc 11000000.codec: Removing 11000000.codec > # rmmod exynos_gsc > [ 227.008559] Unhandled fault: imprecise external abort (0x1c06) at 0x00048e14 > [ 227.015116] pgd = ed5dc000 > [ 227.017213] [00048e14] *pgd=b17c6835 > [ 227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM > ... > [ 227.241585] [<bf2429bc>] (gsc_wait_reset [exynos_gsc]) from [<bf24009c>] (gsc_runtime_resume+0x9c/0xec [exynos_gsc]) > [ 227.252331] [<bf24009c>] (gsc_runtime_resume [exynos_gsc]) from [<c042e488>] (genpd_runtime_resume+0x120/0x1d4) > [ 227.262294] [<c042e488>] (genpd_runtime_resume) from [<c04241c0>] (__rpm_callback+0xc8/0x218) > > # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > domain status slaves > /device runtime status > ---------------------------------------------------------------------- > power-domain@100440C0 on > /devices/platform/soc/14450000.mixer active > /devices/platform/soc/14530000.hdmi active > power-domain@10044120 on > power-domain@10044060 off-0 > power-domain@10044020 on > power-domain@10044000 on > /devices/platform/soc/13e00000.video-scaler suspended > /devices/platform/soc/13e10000.video-scaler resuming > > This seems to be caused by some needed clocks to access the power domains > to be gated, since I don't get these erros when passing clk_ignore_unused > as parameter in the kernel command line. I think that those issues were fixes by the following patch: https://patchwork.kernel.org/patch/9484607/ It still didn't reach mainline, but I hope it will go as a fix to v4.10. Please test if this solves your issue. Please not that adding more clocks to the power domain drivers will solve only the problem with turning domain on/off, but the are more cases where those clocks should be turned on (like IOMMU integration), so marking them as critical solves that problem too. Best regards
Hello Marek, On 01/20/2017 05:08 AM, Marek Szyprowski wrote: > Hi Javier, > [snip] >> Ok, I misunderstood the relationship between runtime PM and the power domains >> then. I thought the power domains were only powered on when the runtime PM >> framework resumed an associated device (i.e: pm_runtime_get_sync() is called). > > Power domains are implemented transparently for the drivers. Even when driver > doesn't support runtime pm, but its device is in the power domain, the core will > ensure that the domain will be turned on all the time the driver is bound to the > device. > I got it now, thank a lot for your explanation. >> But even if this isn't the case, shouldn't the reset in probe only be needed >> if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message). > > This looks like an over-engineering. I don't like polluting driver code with > conditional statements like IS_ENABLED(CONFIG_*). It should not hurt to reset > the device in driver probe, especially just in case the device was left in > some partially configured/working state by bootloader or previous kernel run > (if started from kexec). Adding this conditional code to avoid some issues > with power domain or clocks configuration also suggests that one should > instead solve the problem elsewhere. Driver should be able to access device > registers in its probe() in any case without the additional hacks. > Fair enough, I already posted the patch but I'll ask it to be dropped. [snip] >> >> This seems to be caused by some needed clocks to access the power domains >> to be gated, since I don't get these erros when passing clk_ignore_unused >> as parameter in the kernel command line. > > I think that those issues were fixes by the following patch: > https://patchwork.kernel.org/patch/9484607/ > It still didn't reach mainline, but I hope it will go as a fix to v4.10. > That won't be enough since the CLK_ACLK432_SCALER needs to be ungated for Exynos5422/5800 as mentioned on another mail in this thread. > Please test if this solves your issue. Please not that adding more clocks > to the power domain drivers will solve only the problem with turning domain > on/off, but the are more cases where those clocks should be turned on (like > IOMMU integration), so marking them as critical solves that problem too. > Ok, I understand your point. > Best regards Best regards,
Hello Marek, On 01/20/2017 05:08 AM, Marek Szyprowski wrote: [snip] >> >> This seems to be caused by some needed clocks to access the power domains >> to be gated, since I don't get these erros when passing clk_ignore_unused >> as parameter in the kernel command line. > > I think that those issues were fixes by the following patch: > https://patchwork.kernel.org/patch/9484607/ > It still didn't reach mainline, but I hope it will go as a fix to v4.10. > Argh, I missed on a first read that the patch you mentioned already marks CLK_ACLK432_SCALER as CLK_IS_CRITICAL. So please also ignore my clk patch. Sorry for the noise and the mess with these patches. Thought I also tested on linux-next to see if the issue was solved there, but it seems I didn't. Best regards,
diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c index 83272f10722d..42e1e09ea915 100644 --- a/drivers/media/platform/exynos-gsc/gsc-core.c +++ b/drivers/media/platform/exynos-gsc/gsc-core.c @@ -1083,8 +1083,10 @@ static int gsc_probe(struct platform_device *pdev) platform_set_drvdata(pdev, gsc); - gsc_hw_set_sw_reset(gsc); - gsc_wait_reset(gsc); + if (!IS_ENABLED(CONFIG_PM)) { + gsc_hw_set_sw_reset(gsc); + gsc_wait_reset(gsc); + } vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset") removed the implicit dependency that the driver had with CONFIG_PM, since it relied on the config option to be enabled. In order to work with !CONFIG_PM, the GSC reset logic that happens in the runtime resume callback had to be executed on the probe function. The problem is that if CONFIG_PM is enabled, the power domain for the GSC could be disabled and so an attempt to write to the GSC_SW_RESET register leads to an unhandled fault / imprecise external abort error: [ 10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000 [ 10.186982] pgd = ed728000 [ 10.190847] [00000000] *pgd=00000000 [ 10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM [ 10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [ 10.237134] task: ed49e400 task.stack: ed724000 [ 10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc] [ 10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc] [ 10.256139] pc : [<bf2429e0>] lr : [<bf240734>] psr: 60070013 [ 10.256139] sp : ed725d30 ip : 00000000 fp : 00000001 [ 10.271492] r10: eea74800 r9 : ecd6a2c0 r8 : ed7d8854 [ 10.277912] r7 : ed7d8c08 r6 : ed7d8810 r5 : ffff8ecd r4 : c0c03900 [ 10.285664] r3 : 00000000 r2 : 00000001 r1 : ed7d8b98 r0 : ed7d8810 So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl, making the reset in probe unneeded. Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset") Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> --- I-ve only tested with CONFIG_PM enabled since my Exynos5422 Odroid XU4 board fails to boot when the config option is disabled. Best regards, Javier drivers/media/platform/exynos-gsc/gsc-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)