13:40 <chewitt> sagner the OF messages appear to be harmless, I've not observed issues on a wide variety of devices that could be attributed to them

13:41 <chewitt> the reboot issue is odd; I see the exact opposite

13:41 <chewitt> with the meson_drv change I now have working reboot on 20+ different Amlogic devices

13:41 <chewitt> without the change they all hang in reboot

13:42 <chewitt> devices have (or had, when I was testing that change) a mix of mainline u-boot releases between 2010.04 and latest, and including devices with vendor u-boot

14:18 <sagner> chewitt: hm, interesting. Its reproducible here on multiple ODROID-N2+. Did you test on a ODROID-N2(+)?

14:19 <chewitt> yup, n2 and n2+ both reboot fine here

14:19 <sagner> What I also noticed is rmmod'ing meson_dw_hdmi crashes the kernel hard

14:19 <sagner> memory corruption of some sort

14:20 <sagner> chewitt: hm, maybe kernel config related?

14:20 <chewitt> I'm using an emmc module on the n2+ but random sandisk card on the n2

14:21 <chewitt> https://github.com/chewitt/LibreELEC.tv/blob/amlogic/projects/Amlogic/linux/linux.aarch64.conf

14:21 <chewitt> https://github.com/chewitt/linux/commits/amlogic-5.12.y

14:21 <sagner> This is my config: https://drive.google.com/file/d/1KOy-p8mBECrloQf0X6tmlageOkAxjY0v/view?usp=sharing

14:22 <chewitt> currently running on u-boot 2021.04 + patches https://github.com/chewitt/u-boot/commits/amlogic-2021.04

14:22 <sagner> On my side using the 128GB eMMC module sold by Hardkernel on N2+

14:22 <chewitt> I'm running whatever they shipped with samples .. think it's an 'orange' 16GB module

14:23 <sagner> I am running 2021.04 as well, with two patches ontop: https://github.com/home-assistant/operating-system/tree/dev/buildroot-external/board/hardkernel/patches/uboot

14:24 <chewitt> so my kernel is 5.12 .. and IIRC I was already on 5.11.y around the time Art send the meson_drv change in

14:26 <sagner> chewitt: hm, maybe a dependent patch missing in 5.10?

14:26 <chewitt> I have different CONFIG_POWER options to you

14:27 <chewitt> probably because I have no great clue what they all do, but might be worth a poke

14:27 <chewitt> I also run a rather minimal config .. for LibreELEC so anything not needed/supported for Kodi is out

14:28 <sagner> Hm, I see, CONFIG_MESON_GX_VPU_POWER_DOMAIN sounds like something which might make sense on that platform

14:29 <chewitt> that's in u-boot though?

14:29 <chewitt> G12 is a different one

14:31 <chewitt> CONFIG_MESON_EE_POWER_DOMAIN ?

14:31 <chewitt> @narmstrong ^

14:31 <chewitt> ?

14:36 <sagner> chewitt: oh yeah, sorry, looked at the wrong .config

14:36 <chewitt> :)

14:37 <chewitt> CONFIG_MESON_EE_PM_DOMAINS (plural) in the kernel

14:37 <sagner> Yes, that one is enabled in Linux as well.

15:25 <sagner> chewitt: it seems that the issue disappears when I build CONFIG_DRM_MESON_DW_HDMI=y (as opposed to CONFIG_DRM_MESON_DW_HDMI=m)

15:26 <chewitt> ahh, I build most meson stuff =y due to misc. historic issues with =m

15:26 <sagner> are kernel modules getting unloaded before shutdown/reboot?

15:38 <sagner> My theory is that when using =m meson_drv_unbind gets called after meson_drv_shutdown, and in that combination meson_drv_shutdown does make problem.

15:55 <chewitt> sounds plausible

16:31 <narmstrong> yes it's a side effect, the shutdown of the module is called in a different sequence than when it's built-in

17:14 <sagner> narmstrong: just added some printk statements to the meson_drv_shutdown and ..unbind function. It seems that unbind is not called, but its definitly shutdown which makes the system freeze in the module case.

17:57 <xdarklight> sagner: what about the .shutdown function in drivers/soc/amlogic/meson-ee-pwrc.c - is that one called?

18:01 <sagner> xdarklight: yes, it gets called, before meson_drv_shutdown

18:02 <sagner> Btw, its CONFIG_DRM_MESON which must be m to reproduce the issue.

18:03 <sagner> xdarklight: the crash happens somewhere in meson_drv_shutdown -> drm_atomic_helper_shutdown

18:09 <xdarklight> sagner: is it a crash or the board just hanging?

18:09 <xdarklight> sagner: the latter would indicate that for example the clocks for the VPU IP are turned off while the VPU is still doing some work

18:12 <sagner> The board just hangs

18:12 <xdarklight> just as an experiment: can you make meson_ee_pwrc_shutdown a no-op

18:13 <xdarklight> (even if that would "work" I am not sure how a proper fix would look like, but at least it would give us some more info)

18:13 <sagner> sure, I'll give that a try

18:19 <sagner> xdarklight: bingo, that fixes it

18:19 <sagner> So its probably an ordering issue? shutdowns getting called in different order?

18:21 <xdarklight> sagner: I am not sure about the ordering part. the question that's in my head about meson_ee_pwrc_shutdown is: "why do we even need it?". there's a connection between the power domain and the VPU which we describe in the .dts(i). once the VPU driver is unloading this should also turn off the power domain (for which we implement the correct callback in the genpd subsystem). so in theory the extra shutdown function is not needed

18:21 <xdarklight> but also there's the comment in meson_ee_pwrc_init_domain about special handling of the VPU power domain

18:22 <xdarklight> maybe narmstrong or Kevin (currently not on IRC?) have an idea on how this can be improved

18:33 <sagner> xdarklight: just checked, CONFIG_DRM_MESON=m -> CONFIG_DRM_MESON=y changes order of shutdown functions: built-in calls meson_drv_shutdown before meson_ee_pwrc_shutdown

18:34 <sagner> In both cases I tested with CONFIG_MESON_EE_PM_DOMAINS=y

18:47 <xdarklight> sagner: you could give this a try (I only compile-tested this and as I mentioned before I am not sure about the big TOFIX comment in the original code): https://pastebin.com/igWgQ2Fq

18:49 <xdarklight> sagner: the idea of that patch: maybe it's enough to keep the clocks by just bypassing clk_bulk_disable_unprepare (when turning off the power domain) if we didn't previously call clk_bulk_prepare_enable (done while turning on the power domain)

18:50 <xdarklight> sagner: the clocks already have CLK_IGNORE_UNUSED set so we're keeping the state from the bootloader even if the VPU driver is not loaded (so my hope is that this may work, but I am all but sure on this one ;))

18:51 <sagner> Hm, I see instead of doing the disable in shutdown

18:51 <sagner> I'll give it a try

19:08 <sagner> xdarklight: seems to work here with CONFIG_DRM_MESON=m

19:09 <xdarklight> sagner: can you also test with =y and =n (the latter case is what's describe in the TOFIX comment if I understand it correctly)

19:16 <sagner> xdarklight: CONFIG_DRM_MESON=n looks good, bootup and reboot seems to work

19:16 <sagner> xdarklight: with CONFIG_DRM_MESON=y I do get a stacktrace when I CONFIG_DRM_MESON_DW_HDMI=m and don't load that module

19:16 <sagner> But I think that was already the case befor

19:16 <sagner> And also, weired combination :)

19:17 <xdarklight> sagner: that sounds good to me, thanks for testing! I'll send this as RFC patch later and asking Neil and Kevin to give their review comments

19:17 <xdarklight> sagner: are you subscribed to the mailing list? if not, it would be great if I could get your email address so I can Cc you on the patch

19:18 <sagner> linux-amlogic? I am subscribed, but a CC would be nice still, then its in my inbox :)

19:20 <xdarklight> I'll take the email from https://github.com/torvalds/linux/commit/9e454e37dc7c0ee9e108d70b983e7a71332aedff then (assuming it's still valid because it's a fairly recent change)

19:20 <sagner> xdarklight: that works

19:21 <sagner> xdarklight: This is the trace I get on reboot when not loading meson_dw_hdmi while meson-drm is built-in: https://pastebin.com/874qKmp7

20:33 <xdarklight> sagner: hmm, that part I am not sure about. it was added with https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fa0c16caf3d73 recently

20:35 <xdarklight> the root cause of the fix in that commit is not clear to me

20:50 <sagner> xdarklight: you mean why that patch fixes it in some cases?

20:51 <sagner> It basically is like the opposite: DRM/DW HDMI driver still active while power domain gets shutdown...?

20:52 <xdarklight> sagner: yes, it's not clear to me why fa0c16caf3d73 also fixes some of the shutdown issues

20:55 <sagner> Souns somewhat logical to me: With no shutdown function in the Meson DRM driver etc the driver will sooner or later access one of the registers (vsync irq or something? would explain why its racy: If the system reboots before this happens, no harm done.. but if not, the driver accesses unpowered registers...)

21:35 <xdarklight> then at least I am not sure why it's causing that crash and I am running out of time for today