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@xxxxxxxxxxxxxxx>
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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
