11:17 <mmind00> mriesch: just for my understanding of the rk399 gmac thingy ... pm_runtime_* is refcounted, so both the rk-gmac and stmmac-core both call _get + _put isn't really a problem ... as long as the calls match

11:18 <mmind00> mriesch: so on first glance it looks like the patch in question just maskerades a mismatch in rk_gmac_powerup/_powerdown invocations?

11:18 <mmind00> mriesch: what am I missing?

11:20 <punit> It looks like the pm_runtime_*() calls in rk_gmac_powerup() were hiding an issue which gets exposed after the patch

11:21 <mriesch> mmind00: starting from the origin: i noticed warnings that indicated that pm_runtime_enable is called twice (dwmac-rk + stmmac_main). i doubt that this is critical, but it is not too nice either

11:23 <mriesch> then pm_runtime_get is called twice, hence the refcounter is increased/decreased by two. if this is symmetric, it shouldn't be a problem. but again, it is not too nice

11:24 <mriesch> now if we let the stmmac-main handle it, things break. so yes, there seems to be a mismatch

11:25 <mmind00> mriesch: pm_runtime_enable ... normally is called in _probe something ... and I guess having it called twice really is an issue

11:25 <mriesch> all in all, the dwmac-rk could use some clean up. i'll prepare a patch to use the stmmac helper functions where possible -- after that we should test again

11:25 <mmind00> mriesch: but pm_runtime_get is refcounted for exactly that reason, so that each user of a device can safely do the get/put on its own when it needs the device to be active

11:26 <mmind00> and the system takes care of enabling on first use and disabling the device after the last user left

11:27 <mmind00> withour having to rely on other unknown parts for doing that

11:31 <mriesch> mmind00: consider rk_gmac_probe (in the old version, e.g. 5.13). first, rk_gmac_powerup increases the ref count and then stmmac_dvr_probe increases it once and decreases it once. so after probe the counter is 1 but no one has opened the device. i guess suspend is not called, is it?

11:31 <mmind00> mriesch: correct, so it would still be active at this point

11:33 <mriesch> mmind00: but a probed device without any users could be safely (or even should be) suspended. or am i missing something?

11:36 <mmind00> mriesch: depends on if the stmmac has some sort of "->enable" + disable call for the glue stuff

11:37 <mmind00> or you could wrap the fix_speed methods in pm_runtime_get + _put calls

11:39 <mriesch> mmind00: well i took a look at the other glue drivers and they use callbacks for init, exit and fix_speed. i prepared a patch to align the dwmac-rk with the other glue drivers but haven't sent it yet

11:40 <mriesch> there is also a callback to turn on/off the clocks and i would like to use it as well --> patch series will contain both patches

11:41 <mriesch> no guarantee though that this changes anything, but it does make sense to use those helpers

12:12 <mmind00> mriesch: "patch series will contain both patches" ... sounds great, please cc me :-)

12:14 <mriesch> mmind00: might take a while, though. messing around with the rockchip sdk for another task with no sign of success :-/

13:05 <eballetbo> After some time we tried to run again a Chromebook Scarlet (rk3399) with mainline and found it that display is not working (sometimes). By chance is this a known issue?

13:05 <eballetbo> [ 21.990009] dw-mipi-dsi-rockchip ff968000.mipi: failed to write command FIFO

13:05 <eballetbo> [ 21.997915] panel-innolux-p079zca ff960000.mipi.0: failed to write command 0

13:05 <eballetbo> That's with 5.14

13:11 <wens> mriesch: IIRC the init and exit callbacks are only used in the suspend/resume paths, as you will see the probe function also calls the init function directly

13:40 <mmind00> eballetbo: http://lore.kernel.org/r/20210908135356.18689-1-macroalpha82@gmail.com

13:41 <mmind00> eballetbo: ok I take it back

13:42 <mmind00> eballetbo: the issue with the clock-rate is 5.15-rc1 based ... so 5.14 has another issue? ... a px30-board with (single-)dsi display was working ok with 5.14 last time I checked

14:11 <wens> it doesn't work in 5.10 cros kernel either

14:13 <CounterPillow> does atomic_inc_return do ++foo or foo++?

14:13 <CounterPillow> it's sadly not a documented function

14:14 <wens> CounterPillow: - operations which return the modified value: atomic_{}_return()

14:14 <CounterPillow> Thanks

14:14 <wens> see Documentation/atomic_t.txt

14:21 <robmur01> cf. atomic_fetch_{}()

15:49 <mriesch> mmind00: stumbled over one of your patches from 2015 (c48fa33c)... the stmmaceth clock is common to all stmmac incarnations, right? should the glue code really interfere here or can the stmmac core take care of that?

15:50 <mriesch> if this is still needed, i would access the clock via plat->stmmac_clk

15:50 <mmind00> mriesch: 2015!!! :-O ... I have trouble remembering last month ;-)

15:51 <mriesch> i know, nanghi :-) just promise me that you'll test it thoroughly and i'll withdraw the question!

15:52 <mmind00> but I'd think that will still be necessary

15:53 <mmind00> i.e. on 1000mbit rgmii we always have an external clock provider, but the 100mbit rmii uses clocks from the clock controller ... and that setting most of the time is not adequate on boot

15:54 <mmind00> mriesch: while assigned-clocks would also be possible - the kernel should work with older devicetrees, so this will need to stay there

15:55 <mriesch> speaking of dt, there is pclk_mac in dwmac-rk and pclk in stmmac_platform -> are those the same?

15:55 <mmind00> mriesch: probably ... the pclk is the one needed to access the registers

15:58 <mriesch> so the plan is to provide a clks_config handler to turn on/off the specific clocks, where the common clocks stmmacetch and pclk can be handled by the core

15:58 <mriesch> but there seems to be an incompatibility in the name. we can't fix this, can we?

16:01 <mmind00> mriesch: nope ... if it's part of the binding, it's part of the binding ;-) ... while you can propose updates to the bindings, backwards compatiblity with old devicetrees must be maintained

16:01 <mriesch> right

17:05 <mriesch> mmind00: Esmil: punit: i sent out an rfc with cleanup patches. if you could test them on different boards it would be much appreciated

17:29 <Esmil> mriesch: it doesn't work for me :(

17:37 <mriesch> Esmil: mmk... can you describe the behavior you encounter?

17:38 <mriesch> and000000 is that with or without 2d26f6e3?

17:41 <Esmil> that was your 3 patches 5.14.3, so including 2d26f6e39afb88d32b8f39e76a51b542c3c51674

17:42 <Esmil> ..and the behaviour is the same as described yesterday. it detects link and negotiates flow contol just fine, but no packages seem to be sent or received. ip neigh is completely empty

17:42 <Esmil> *your 3 patches on 5.14.3

17:47 <mriesch> Esmil: ok thanks for your prompt response. do you think you could try it without 2d26f6e3?

17:49 <Esmil> sure

18:10 <Esmil> mriesch: hmm. that didn't go too well. could you try merging both the revert and your series for me?

18:18 <mriesch> Esmil: i'll have a look at it tomorrow

18:21 <Esmil> cool

21:03 <macromorgan> anybody here good with clk issues? https://paste.debian.net/1211910/

21:04 <macromorgan> long story short I tried adding a regulator for my rockchip phy. The regulator isn't ready upon first probe and the driver gets an error code of -517 (EPROBE_DEFER); so far so good. The issue is then when unregistering the driver I get this ugly warning.

21:05 <macromorgan> the PHY is served by the phy-rockchip-inno-usb2.c driver, and after this ugliness it seems to work otherwise just fine.

21:20 <mmind00> macromorgan: looks like the clk_unregister (for the 480m clock from the usbphy) is happening before it got disabled by some consumer

21:21 <mmind00> macromorgan: according to clk.c the clock framework assigns empty clock ops to a clock when clk_unregister is called ... this is supposed to catch clk_disable calls that run somewhere after clk_unregister

22:05 <macromorgan> I wonder who's unregistering it...

22:05 <mmind00> macromorgan: the phy-driver itself

22:05 <macromorgan> twice?

22:05 <mmind00> only once

22:06 <mmind00> macromorgan: see the 480m_register function ... which registers a devm_action to unregister the clock on device removal

22:10 <mmind00> macromorgan: maybe it's not even coming from inside the phy ... the clock controller on rockchip socs uses the 480m phy as input for some of its clocks ... creating a cycle (xin24m -> cru -> phy -> cru)

22:11 <macromorgan> forgive me for being thick, but it's registering that function to unregister the clock on device removal, and it's calling a function to unregister the clock if it fails probe after clock probe successful. if by that point the devm succeeded (and the clk register returned a 0) then we should no longer call clk_disable_unprepare?

22:12 <macromorgan> so basically, remove the disable_clks part of the probe function?

22:12 <mmind00> definitly not

22:12 <macromorgan> okay

22:12 <mmind00> macromorgan: to explain: the phy-clock coming from the cru is the source of the 480m pll inside the phy

22:14 <mmind00> macromorgan: after the phy probes sucessfully, can you mount debugfs and do a "cat $debugsomething/clk/clk_summary" and check if something is already a child of the 480m clock?

22:14 <macromorgan> sure

22:15 <mmind00> macromorgan: i.e. some cru clocks may already be parented to the 480m clock on boot ... even though that clock is not present yet ... they're orphans until the 480m clock turns up

22:17 <mmind00> macromorgan: so one guess would be that once the 480m clock is integrated into the clock controller, unregistering it makes a child of it unhappy

22:17 <macromorgan> usb480m is the only child listed of usb480m_phy

22:20 <macromorgan> usb480m is referenced in the clock driver at drivers/clk/rockchip/clk-px30.c, but I don't see any other references of it outside of there that could be consuming it

22:21 <macromorgan> (all this because I wanted to be anal and attach the phy-supply)

22:21 <mmind00> macromorgan: but usb480m is a clock controlled in the clock driver (and on the soc in the clock controller) ... where usb480m_phy is the clock generated from the usbphy

22:21 <macromorgan> which reminds me, the inno-usb2 driver also hates EPROBE_DEFER

22:22 <mmind00> macromorgan: one thing you could try is to use assigned-clocks in the usbphy node to reparent usb480m to say xin24m and check if warning goes away then

22:22 <macromorgan> here's my clock tree: https://paste.debian.net/1211923/

22:27 <macromorgan> the issue I'm having is that 1) the USB PHY on my 3326 doesn't work on battery, I've got a devicetree fix to send you to fix that. The other issue is that the regulator that powers the USB host isn't on until after the USB has probed, and as a result USB devices act "weird" for a second or two at boot with only 3.3v

22:27 <macromorgan> I was hoping to set the phy-supply, which as expected generates a 517 error early in the boot process and forces the usb phy to unload to be reloaded again later

22:28 <macromorgan> but this unloading triggers the warning

22:28 <macromorgan> setting phy-supply does fix the USB errors... it just gives me a whole host of new errors

22:37 <mmind00> macromorgan: a whole "host"? ... I see one warning right now ;-)

22:37 <mmind00> but yeah that ideally shouldn't be there

22:51 <macromorgan> okay, yeah just one

22:51 <macromorgan> technically the 517s are errors/warnings too, but that's easily fixable