On 12.02.2024 22:32, Bjorn Helgaas wrote: > "Properly" is a noise word that suggests "we're doing it right this > time" but doesn't hint at what actually makes this better. > > On Sat, Feb 10, 2024 at 06:10:07PM +0100, Konrad Dybcio wrote: >> Currently, we've only been minimizing the power draw while keeping the >> RC up at all times. This is suboptimal, as it draws a whole lot of power >> and prevents the SoC from power collapsing. > > Is "power collapse" a technical term specific to this device, or is > there some more common term that could be used? I assume the fact > that the RC remains powered precludes some lower power state of the > entire SoC? That's spot on, "power collapse" commonly refers to shutting down as many parts of the SoC as possible, in order to achieve miliwatt-order power draw. > >> Implement full shutdown and re-initialization to allow for powering off >> the controller. >> >> This is mainly indended for SC8280XP with a broken power rail setup, >> which requires a full RC shutdown/reinit in order to reach SoC-wide >> power collapse, but sleeping is generally better than not sleeping and >> less destructive suspend can be implemented later for platforms that >> support it. > > s/indended/intended/ > >> config PCIE_QCOM >> bool "Qualcomm PCIe controller (host mode)" >> depends on OF && (ARCH_QCOM || COMPILE_TEST) >> + depends on QCOM_COMMAND_DB || QCOM_COMMAND_DB=n > > Just out of curiosity since I'm not a Kconfig expert, what does > "depends on X || X=n" mean? "not a module" > > I guess it's different from > "depends on (QCOM_COMMAND_DB || !QCOM_COMMAND_DB)", which I also see > used for QCOM_RPMH? Yep > > Does this reduce compile testing? I see COMPILE_TEST mentioned in a > few other QCOM_COMMAND_DB dependencies. I can add "&& COMPILE_TEST", yeah > >> + ret_l23 = readl_poll_timeout(pcie->parf + PARF_PM_STTS, val, >> + val & PM_ENTER_L23, 10000, 100000); > > Are these timeout values rooted in some PCIe or Qcom spec? Would be > nice to have a spec citation or other reason for choosing these > values. > >> + reset_control_assert(res->rst); >> + usleep_range(2000, 2500); > > Ditto, some kind of citation would be nice. Both are magic values coming from Qualcomm BSP, that we suppose we can safely assume (and that's a two-level assumption at this point, I know..) is going to work fine, as it does so on millions of shipped devices. Maybe Mani or Bjorn A can find something interesting in the documentation. Konrad
