pevik_ has quit [Remote host closed the connection]
macc24 has joined #linux-msm
svarbanov has joined #linux-msm
<arnd>
bamse: I get this arm32 build failure on linux-next today: ERROR: modpost: "__cpu_logical_map" [drivers/firmware/qcom-scm.ko] undefined!
<arnd>
there is probably a higher-level interface that one should use instead of directly accessing cpu_logical_map, but I don't remember what that would be at the moment
<arnd>
or maybe just add an #ifdef CONFIG_SMP check
pevik_ has joined #linux-msm
<arnd>
patch sent, I can apply it directly to the soc tree if it looks good to you
<DavidHeidelberg[m]>
robclark: Hello Rob, when you have time, please ack/nack Adreno docs conversion to YAML :)
<minecrell>
arnd: Hmm, (__)cpu_logical_map isn't EXPORT_SYMBOL so I guess this is going to explode anyway? :(
<minecrell>
arnd: From looking at the usages AFAICT cpu_logical_map() is what everyone uses to get the MPIDR stuff though :/
<arnd>
minecrell: ah right. So far, my randconfig tests did not run into that, but I guess that is since we now 'select QCOM_SCM' from drivers that need it, and that means it's usually either =y or =n but not =m
Danct12 has joined #linux-msm
<minecrell>
arnd: hmm but QCOM_SCM is still a tristate so if all drivers using it are =m it would be =m too or not?
<arnd>
yes, exactly: you can easily force such a configuration by hand, it's just unlikely to be seen with randconfig builds unless you do a lot of them
<minecrell>
arnd: This code should never be called when QCOM_SCM is compiled as a module though, is there some dumb way to make it only compile when builtin or is that too hacky?
<arnd>
#ifndef MODULE
<arnd>
minecrell: but it looks like drivers/cpuidle/cpuidle-qcom-spm.c can call it, and that can be a module
<minecrell>
arnd: config ARM_QCOM_SPM_CPUIDLE is a bool AFAICT
<arnd>
ah right, it is
<arnd>
in fact, it looks like all cpuidle drivers are. I wonder why they haven't been targetted by the GKI folks yet ;-)
<minecrell>
I'm not crazy enough to compile QCOM_SCM as module anyway :P
<minecrell>
so #if (defined(CONFIG_ARM) || defined(CONFIG_ARM64)) && defined(CONFIG_SMP) && !defined(MODULE) I guess, yay
<arnd>
as far as I can tell, those functions are only ever called on 32-bit, which forces the driver to be built-in
<arnd>
could it be moved one level down into arch/arm/mach-qcom/platsmp.c ?
<arnd>
or maybe pass a different set of arguments that includes the physical cpu number
<arnd>
as both callers have access to cpu_logical_map(). That would let us remove the #ifdef completely and make it compile on non-arm
<minecrell>
arnd: I have to admit I'm also using it on arm64 with a minimal extra patch that enables compiling ARM_QCOM_SPM_CPUIDLE on arm64 (this is pretty much just removing the depends on !ARM64 in Kconfig). This is to workaround signed firmware without PSCI support :(
<minecrell>
so while this isn't officially supported I'd prefer to not make that more complicated than necessary
<arnd>
but that would still work then, right? With arm64, you still have the cpuidle driver as built-in, so it can use cpu_logical_map()
<minecrell>
ahh I understand what you mean now
<minecrell>
The main disadvantage is that if we pass cpu_logical_map() to qcom_scm_set_(cold/warm)_boot_addr() then platsmp.c can no longer pass multiple CPUs (cpu_present_mask) at once (which translates to a single SMC instead of one per CPU)
Danct12 has quit [Quit: Quitting]
<minecrell>
arnd: perhaps a if (!IS_BUILTIN(CONFIG_SMP) || IS_MODULE(CONFIG_QCOM_SCM)) return -EINVAL; in __qcom_scm_set_boot_addr_mc() would already be a bit cleaner than adding more ifdefs?
<arnd>
minecrell: that wouldn't remove the need for an #ifdef to prevent a compile failure on x86 allmodconfig
<arnd>
minecrell: is the overhead of doing multiple smc calls relevant? How often is that called?
<minecrell>
arnd: It's probably not but passing cpu_logical_map() to qcom_scm_set_(cold/warm)_boot_addr() doesn't solve the x86 problem either. :/ You still need MPIDR_AFFINITY_LEVEL() which is ARM-only
<minecrell>
we'd need to pass all 3 affinity levels directly to the function
<minecrell>
arnd: tbh strictly speaking the interface is totally over-engineered and we could simply not take a CPU parameter at all. At the end the same thing is always done for all CPUs....
<minecrell>
if you'd prefer to simplify this entirely I could try doing that for 5.17
<arnd>
minecrell: would the trivial method work doing "desc.args[1] = desc.args[2] = desc.args[3] = desc.args[4] = ~0ULL;" ?
<minecrell>
arnd: Exactly
<minecrell>
at least for the currently present usages in the kernel, and I can't think of any others
<minecrell>
arnd: It may need some refactoring of the usages though to make sure it's called only once and not for each CPU, so it's probably more 5.17 material. It's fine for me if you would prefer to revert the original patch for 5.16
<arnd>
I wouldn't mind taking that patch for 5.16 if everyone agrees, otherwise we can do the stricter #ifdef for now and improve it for 5.17 as you say
<arnd>
a revert would definitely be the easiest ;-)
<minecrell>
I'd only do revert or ifdef for 5.16, both is fine
<minecrell>
I need some time to test the simplification properly
<arnd>
another bug: drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:162:6: error: variable 'commit' is uninitialized when used here [-Werror,-Wuninitialized]
<arnd>
minecrell: let's see what bamse thinks. If you are both fine with the revert, just let me know and I'll commit that directly, no need to send a patch then
<arnd>
jessica_24: that dpu_crtc.c bug is yours, do you already have a fix for it?
<arnd>
minecrell: that would avoid the warning, sure. But is this correct?
<minecrell>
no idea :D
<arnd>
I've applied that patch locally in my test tree now, it's probably fine
ahalaney has joined #linux-msm
Danct12 has joined #linux-msm
<bamse>
arnd: replied to your mail as well, but please apply a revert for now
<robclark>
DavidHeidelberg[m]: the yaml conversion is in the pull req I sent yesterday.. thx
<DavidHeidelberg[m]>
robclark: thanks!
wwilly has joined #linux-msm
<Marijn[m]>
Sad, my cc hence r-b got lost somewhere in the process
<Marijn[m]>
In any case very nice work David Heidelberg for getting that done!
<DavidHeidelberg[m]>
Marijn: thanks. We need more people converting txt to yaml, validation while conversion of old txts discovered so many typos already on few files (doing it right now mostly for tegra2/3 tablets)
<DavidHeidelberg[m]>
since grate tree uses CI, which is able to fire when new DTS or it's change don't comply Docs :)
<Marijn[m]>
David Heidelberg: True. We try to do it for new/updated drivers but I already have a lot of trouble cleaning and sending local patches due to time contraints - not to mention enough time for doing some conversions out of nowhere.
<Marijn[m]>
That said I'm surprised there's hardly any CI running in this corner of the kernel ;)
<DavidHeidelberg[m]>
well I did put it there...
<DavidHeidelberg[m]>
also I have it deployed on my apq8064 tree, but there is still too many warnings to produce useful results
<DavidHeidelberg[m]>
currently I follow as Rob H. doing it. Counting numbers of lines. If I see decrease, it's good, increase bad. But it's not the best way to do it :D
go4godvin is now known as frytaped
<jessica_24>
arnd: sorry for the late reply. i see that the patch has been added to msm-next. does it still need my review?
<arnd>
jessica_24: yes, it would help if you can reply to the email, if only to document that this was the right solution