* PSCI domains without OSI support @ 2022-07-27 9:09 Dmitry Baryshkov 2022-07-27 11:14 ` Sudeep Holla 0 siblings, 1 reply; 12+ messages in thread From: Dmitry Baryshkov @ 2022-07-27 9:09 UTC (permalink / raw) To: Ulf Hansson; +Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM Hi, Lately I have been working on improving the msm8996 platform support. Vendor kernel seems to support domain-like idle (see [1], [2]). However when I tried changing upstream msm8996.dtsi to use PSCI domains, I faced the firmware reporting NOT_SUPPORTED to an attempt to enable OSI (thus rendering PSCI domains useless, as they are now marked with ALWAYS_ON). I noticed that vendor kernel makes a call to cpu_suspend() with power_state following the original format (described in PSCI spec 5.4.2.1). What would be the best way to support this? - Allow DTS forcing the PSCI power domains even if OSI enablement fails? - Add a separate cpuidle driver? - Just forget about it and use plain PSCI as we currently do? Additional topic: for one of idle states the vendor kernel uses a proprietary call into the hypervisor ([3]). Up to now we have ignored this, as 8996 seems to be the only platform using it. I suppose that adding it to cpuidle-psci.c would be frowned upon. Is this assumption correct? Would it add another point for adding a separate cpuidle driver? [1] https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.6.6.c31-02700-89xx.0/arch/arm/boot/dts/qcom/msm8996-pm.dtsi#L32 [2] https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.6.6.c31-02700-89xx.0/drivers/cpuidle/lpm-levels.c#L927 [3] https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.6.6.c31-02700-89xx.0/drivers/cpuidle/lpm-levels.c#L944 -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-07-27 9:09 PSCI domains without OSI support Dmitry Baryshkov @ 2022-07-27 11:14 ` Sudeep Holla 2022-07-27 13:24 ` Dmitry Baryshkov 0 siblings, 1 reply; 12+ messages in thread From: Sudeep Holla @ 2022-07-27 11:14 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Ulf Hansson, linux-arm-msm, Bjorn Andersson, Sudeep Holla, Vinod Koul, Linux PM On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > Hi, > > Lately I have been working on improving the msm8996 platform support. > Vendor kernel seems to support domain-like idle (see [1], [2]). > However when I tried changing upstream msm8996.dtsi to use PSCI > domains, I faced the firmware reporting NOT_SUPPORTED to an attempt to > enable OSI (thus rendering PSCI domains useless, as they are now > marked with ALWAYS_ON). > That's not good to hear 🙁. > I noticed that vendor kernel makes a call to cpu_suspend() with > power_state following the original format (described in PSCI spec > 5.4.2.1). What would be the best way to support this? And why is this not possible with the existing code ? Not sure if I understood it right, I am assuming you are mentioning that it is not possible. > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? Meaning DTS flag for this ? If OSI enable fails, why would you want to still proceed. It is non-compliant and must be fixed if the firmware supports OSI and expects OSPM to use the same. > - Add a separate cpuidle driver? I would avoid that. > - Just forget about it and use plain PSCI as we currently do? > Worst case yes. My main worry is how many of the old SDM SoC has such a behaviour and how much they wary from each other. The OSI mode was pushed after lengthy discussions to support all these platforms and now we have platforms needing separate idle driver ? > Additional topic: for one of idle states the vendor kernel uses a > proprietary call into the hypervisor ([3]). Again I would say it is not spec compliant. > Up to now we have ignored this, as 8996 seems to be the only platform using > it. I suppose that adding it to cpuidle-psci.c would be frowned upon. Indeed. > Is this assumption correct? Would it add another point for adding a separate > cpuidle driver? > I am getting a sense that this would be cleaner approach but I would like to understand how much of these non-compliance is carried to the other relatively newer SoCs. I understand this is atleast 5-6+ years old. I don't want this to set example to deviate from standard driver by adding new drivers though they all are supposedly using PSCI(and are not fully compliant) -- Regards, Sudeep ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-07-27 11:14 ` Sudeep Holla @ 2022-07-27 13:24 ` Dmitry Baryshkov 2022-07-27 13:39 ` Dmitry Baryshkov 2022-07-28 8:40 ` Sudeep Holla 0 siblings, 2 replies; 12+ messages in thread From: Dmitry Baryshkov @ 2022-07-27 13:24 UTC (permalink / raw) To: Sudeep Holla Cc: Ulf Hansson, linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > > Hi, > > > > Lately I have been working on improving the msm8996 platform support. > > Vendor kernel seems to support domain-like idle (see [1], [2]). > > However when I tried changing upstream msm8996.dtsi to use PSCI > > domains, I faced the firmware reporting NOT_SUPPORTED to an attempt to > > enable OSI (thus rendering PSCI domains useless, as they are now > > marked with ALWAYS_ON). > > > > That's not good to hear 🙁. > > > I noticed that vendor kernel makes a call to cpu_suspend() with > > power_state following the original format (described in PSCI spec > > 5.4.2.1). What would be the best way to support this? > > And why is this not possible with the existing code ? Not sure if I > understood it right, I am assuming you are mentioning that it is not > possible. It's not possible with the cpuidle-psci-domains.c. The driver marks all genpds as ALWAYS_ON, thus making sure that they are never suspended. > > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > > Meaning DTS flag for this ? If OSI enable fails, why would you want to > still proceed. It is non-compliant and must be fixed if the firmware > supports OSI and expects OSPM to use the same. I'm not sure at this moment. PSCI firmware reports that OSI mode is supported, but then when psci_pd_try_set_osi_mode() tries to switch into OSI mode, it gets NOT_SUPPORTED. Just for the sake of completeness, I added a print to the psci.c to dump the result of the psci_set_osi_mode(false). It also returns NOT_SUPPORTED! My logical assumption would be that the firmware reports support for OS_INITIATED, but then just fails to properly support SET_SUSPEND_MODE. I should probably try ignoring the error psci-domain.c and continue with binding power domains. What would be the best way to check that the domains setup works as expected? > > > - Add a separate cpuidle driver? > > I would avoid that. > > > - Just forget about it and use plain PSCI as we currently do? > > > > Worst case yes. My main worry is how many of the old SDM SoC has such a > behaviour and how much they wary from each other. The OSI mode was pushed > after lengthy discussions to support all these platforms and now we have > platforms needing separate idle driver ? I'm not sure. 32-bit SoCs use non-PSCI idle driver. MSM8916, sdm845 and later SoCs are using proper domains support. I tested the sdm660, it looks like it can work with the power domains. So this leaves only MSM8992/4/8 in question (at least from the platforms that are supported by the mainline). > > > Additional topic: for one of idle states the vendor kernel uses a > > proprietary call into the hypervisor ([3]). > > Again I would say it is not spec compliant. Yes, of course. Otherwise there would not be such a question. Judging from the vendor kernels, this call is limited to 8996 only. > > > Up to now we have ignored this, as 8996 seems to be the only platform using > > it. I suppose that adding it to cpuidle-psci.c would be frowned upon. > > Indeed. > > > Is this assumption correct? Would it add another point for adding a separate > > cpuidle driver? > > > > I am getting a sense that this would be cleaner approach but I would like > to understand how much of these non-compliance is carried to the other > relatively newer SoCs. I understand this is atleast 5-6+ years old. I don't > want this to set example to deviate from standard driver by adding new > drivers though they all are supposedly using PSCI(and are not fully compliant) At this moment I fear that it might be limited to msm8996 only. Maybe including 8992/4/8. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-07-27 13:24 ` Dmitry Baryshkov @ 2022-07-27 13:39 ` Dmitry Baryshkov 2022-07-27 20:51 ` Dmitry Baryshkov 2022-07-28 8:40 ` Sudeep Holla 1 sibling, 1 reply; 12+ messages in thread From: Dmitry Baryshkov @ 2022-07-27 13:39 UTC (permalink / raw) To: Sudeep Holla Cc: Ulf Hansson, linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM On Wed, 27 Jul 2022 at 16:24, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > > > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > > > > Meaning DTS flag for this ? If OSI enable fails, why would you want to > > still proceed. It is non-compliant and must be fixed if the firmware > > supports OSI and expects OSPM to use the same. > > I'm not sure at this moment. PSCI firmware reports that OSI mode is > supported, but then when psci_pd_try_set_osi_mode() tries to switch > into OSI mode, it gets NOT_SUPPORTED. > Just for the sake of completeness, I added a print to the psci.c to > dump the result of the psci_set_osi_mode(false). It also returns > NOT_SUPPORTED! > > My logical assumption would be that the firmware reports support for > OS_INITIATED, but then just fails to properly support > SET_SUSPEND_MODE. Okay. From the msm-3.14 commit log: Add support to terminate all low power modes in PSCI. The lpm-levels will work with version 1.0 of PSCI specification using the OS initiated scheme. The lpm-levels driver would determine the last man standing and vote into TZ accordingly. Which means that the vendor kernel expected to work in the OSI mode without calling SET_SUSPEND (such call doesn't exist in 3.14) So, this looks like the "force-psci-domains" or "ignore-osi-error" flag would be logical. The question about testing still holds. > I should probably try ignoring the error psci-domain.c and continue > with binding power domains. What would be the best way to check that > the domains setup works as expected? -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-07-27 13:39 ` Dmitry Baryshkov @ 2022-07-27 20:51 ` Dmitry Baryshkov 2022-08-05 13:47 ` Ulf Hansson 0 siblings, 1 reply; 12+ messages in thread From: Dmitry Baryshkov @ 2022-07-27 20:51 UTC (permalink / raw) To: Sudeep Holla Cc: Ulf Hansson, linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM On Wed, 27 Jul 2022 at 16:39, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 27 Jul 2022 at 16:24, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > > > > > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > > > > > > Meaning DTS flag for this ? If OSI enable fails, why would you want to > > > still proceed. It is non-compliant and must be fixed if the firmware > > > supports OSI and expects OSPM to use the same. > > > > I'm not sure at this moment. PSCI firmware reports that OSI mode is > > supported, but then when psci_pd_try_set_osi_mode() tries to switch > > into OSI mode, it gets NOT_SUPPORTED. > > Just for the sake of completeness, I added a print to the psci.c to > > dump the result of the psci_set_osi_mode(false). It also returns > > NOT_SUPPORTED! > > > > My logical assumption would be that the firmware reports support for > > OS_INITIATED, but then just fails to properly support > > SET_SUSPEND_MODE. > > Okay. From the msm-3.14 commit log: > > Add support to terminate all low power modes in PSCI. The lpm-levels will > work with version 1.0 of PSCI specification using the OS initiated scheme. > The lpm-levels driver would determine the last man standing and vote into > TZ accordingly. > > Which means that the vendor kernel expected to work in the OSI mode > without calling SET_SUSPEND (such call doesn't exist in 3.14) After adding the debugfs file, it's clear that this is the case. Compare msm8996: PSCIv1.0 SMC Calling Convention v1.0 is assumed OSI is supported Extended StateID format is used vs sdm845: PSCIv1.1 SMC Calling Convention v1.2 OSI is supported Extended StateID format is used CPU_FREEZE is supported SET_SUSPEND_MODE is supported Judging by people reporting 'failure to enable OSI mode' on several other Qualcomm SoCs (msm8976, msm8953), this bug is present on several older Qualcomm platforms. > > So, this looks like the "force-psci-domains" or "ignore-osi-error" > flag would be logical. > The question about testing still holds. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-07-27 20:51 ` Dmitry Baryshkov @ 2022-08-05 13:47 ` Ulf Hansson 0 siblings, 0 replies; 12+ messages in thread From: Ulf Hansson @ 2022-08-05 13:47 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Sudeep Holla, linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM On Wed, 27 Jul 2022 at 22:51, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 27 Jul 2022 at 16:39, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Wed, 27 Jul 2022 at 16:24, Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > > > > > > > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > > > > > > > > Meaning DTS flag for this ? If OSI enable fails, why would you want to > > > > still proceed. It is non-compliant and must be fixed if the firmware > > > > supports OSI and expects OSPM to use the same. > > > > > > I'm not sure at this moment. PSCI firmware reports that OSI mode is > > > supported, but then when psci_pd_try_set_osi_mode() tries to switch > > > into OSI mode, it gets NOT_SUPPORTED. > > > Just for the sake of completeness, I added a print to the psci.c to > > > dump the result of the psci_set_osi_mode(false). It also returns > > > NOT_SUPPORTED! > > > > > > My logical assumption would be that the firmware reports support for > > > OS_INITIATED, but then just fails to properly support > > > SET_SUSPEND_MODE. > > > > Okay. From the msm-3.14 commit log: > > > > Add support to terminate all low power modes in PSCI. The lpm-levels will > > work with version 1.0 of PSCI specification using the OS initiated scheme. > > The lpm-levels driver would determine the last man standing and vote into > > TZ accordingly. > > > > Which means that the vendor kernel expected to work in the OSI mode > > without calling SET_SUSPEND (such call doesn't exist in 3.14) > > After adding the debugfs file, it's clear that this is the case. > > Compare msm8996: > PSCIv1.0 > SMC Calling Convention v1.0 is assumed > OSI is supported > Extended StateID format is used > > vs sdm845: > PSCIv1.1 > SMC Calling Convention v1.2 > OSI is supported > Extended StateID format is used > CPU_FREEZE is supported > SET_SUSPEND_MODE is supported > > Judging by people reporting 'failure to enable OSI mode' on several > other Qualcomm SoCs (msm8976, msm8953), this bug is present on several > older Qualcomm platforms. > > > > > So, this looks like the "force-psci-domains" or "ignore-osi-error" > > flag would be logical. > > The question about testing still holds. Alright, so this looks like a deviation from the spec. Nevertheless, it seems quite simple for us to fix by overriding the FW error, by adding a new DT flag, in the way you propose. In principle, I think the new DT flag should avoid us to call psci_set_osi_mode() in psci_pd_try_set_osi_mode(), but rather just return true when the flag is set. In this way, the GENPD_FLAG_ALWAYS_ON will not be set for the genpds that are created, so things should work as the PSCI FW has OSI mode enabled. Kind regards Uffe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-07-27 13:24 ` Dmitry Baryshkov 2022-07-27 13:39 ` Dmitry Baryshkov @ 2022-07-28 8:40 ` Sudeep Holla 2022-07-28 9:15 ` Dmitry Baryshkov 2022-08-05 14:12 ` Ulf Hansson 1 sibling, 2 replies; 12+ messages in thread From: Sudeep Holla @ 2022-07-28 8:40 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Ulf Hansson, linux-arm-msm, Bjorn Andersson, Sudeep Holla, Vinod Koul, Linux PM On Wed, Jul 27, 2022 at 04:24:22PM +0300, Dmitry Baryshkov wrote: > On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > > > Hi, > > > > > > Lately I have been working on improving the msm8996 platform support. > > > Vendor kernel seems to support domain-like idle (see [1], [2]). > > > However when I tried changing upstream msm8996.dtsi to use PSCI > > > domains, I faced the firmware reporting NOT_SUPPORTED to an attempt to > > > enable OSI (thus rendering PSCI domains useless, as they are now > > > marked with ALWAYS_ON). > > > > > > > That's not good to hear 🙁. > > > > > I noticed that vendor kernel makes a call to cpu_suspend() with > > > power_state following the original format (described in PSCI spec > > > 5.4.2.1). What would be the best way to support this? > > > > And why is this not possible with the existing code ? Not sure if I > > understood it right, I am assuming you are mentioning that it is not > > possible. > > It's not possible with the cpuidle-psci-domains.c. The driver marks > all genpds as ALWAYS_ON, thus making sure that they are never > suspended. > That doesn't sound correct. I am sure Ulf has tried this on one of SDM platform for sure when it was merged. > > > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > > > > Meaning DTS flag for this ? If OSI enable fails, why would you want to > > still proceed. It is non-compliant and must be fixed if the firmware > > supports OSI and expects OSPM to use the same. > > I'm not sure at this moment. PSCI firmware reports that OSI mode is > supported, but then when psci_pd_try_set_osi_mode() tries to switch > into OSI mode, it gets NOT_SUPPORTED. Yikes, fix the damn broken firmware. That is utter non-sense. I don't understand why would the firmware authors enable some feature before it is ready. > Just for the sake of completeness, I added a print to the psci.c to > dump the result of the psci_set_osi_mode(false). It also returns > NOT_SUPPORTED! > Well it is simply broken then. Not tested firmware, so please don't attempt to use OSI if it is so fundamentally broken. I find it hard to accept the argument that well it works just that the query API is failing. But what is the guarantee that it is tested well enough. We will end up adding more quirks after adding one to enable it. > My logical assumption would be that the firmware reports support for > OS_INITIATED, but then just fails to properly support > SET_SUSPEND_MODE. I knew this argument was coming as I wrote above, sorry I don't buy that. It is probably one of the early platforms supporting PSCI and not well tested for conformance. So I am inclined to just say we can't support it. > I should probably try ignoring the error psci-domain.c and continue > with binding power domains. What would be the best way to check that > the domains setup works as expected? > > > > > > - Add a separate cpuidle driver? > > > > I would avoid that. > > > > > - Just forget about it and use plain PSCI as we currently do? > > > > > > > Worst case yes. My main worry is how many of the old SDM SoC has such a > > behaviour and how much they wary from each other. The OSI mode was pushed > > after lengthy discussions to support all these platforms and now we have > > platforms needing separate idle driver ? > > I'm not sure. 32-bit SoCs use non-PSCI idle driver. MSM8916, sdm845 > and later SoCs are using proper domains support. Yes that is 32-bit, we just support PSCI on 64-bit. No more arguments there, we had enough in the early days for almost more than a year. We are not going back to that again. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-07-28 8:40 ` Sudeep Holla @ 2022-07-28 9:15 ` Dmitry Baryshkov 2022-08-05 14:12 ` Ulf Hansson 1 sibling, 0 replies; 12+ messages in thread From: Dmitry Baryshkov @ 2022-07-28 9:15 UTC (permalink / raw) To: Sudeep Holla Cc: Ulf Hansson, linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM On Thu, 28 Jul 2022 at 11:40, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jul 27, 2022 at 04:24:22PM +0300, Dmitry Baryshkov wrote: > > On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > > > > > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > > > > > > Meaning DTS flag for this ? If OSI enable fails, why would you want to > > > still proceed. It is non-compliant and must be fixed if the firmware > > > supports OSI and expects OSPM to use the same. > > > > I'm not sure at this moment. PSCI firmware reports that OSI mode is > > supported, but then when psci_pd_try_set_osi_mode() tries to switch > > into OSI mode, it gets NOT_SUPPORTED. > > Yikes, fix the damn broken firmware. That is utter non-sense. I don't > understand why would the firmware authors enable some feature before it > is ready. > > > Just for the sake of completeness, I added a print to the psci.c to > > dump the result of the psci_set_osi_mode(false). It also returns > > NOT_SUPPORTED! > > > > Well it is simply broken then. Not tested firmware, so please don't > attempt to use OSI if it is so fundamentally broken. I find it hard to > accept the argument that well it works just that the query API is failing. > But what is the guarantee that it is tested well enough. We will end up > adding more quirks after adding one to enable it. > > > My logical assumption would be that the firmware reports support for > > OS_INITIATED, but then just fails to properly support > > SET_SUSPEND_MODE. > > I knew this argument was coming as I wrote above, sorry I don't buy that. > It is probably one of the early platforms supporting PSCI and not well tested > for conformance. So I am inclined to just say we can't support it. I have quoted the commit message from the vendor kernel (and the debugfs entries) in other messages in this thread. It definitely seems like the platform breaks strict standard conformance by supporting CPU_SUSPEND, but not the SET_SUSPEND_MODE. Vendor kernel still used the CPU_SUSPEND with a domain-like approach, expecting to find the PSCI already being running in the OSI mode. So I'd ask for the ability to add a flag to DT telling the PSCI_DOMAINS code that the platform is already running in the OSI mode despite set_osi_mode() returning NOT_SUPPORTED. I would not say that this firmware was not tested to work with the domains approach. The whole bunch of phones/tables were shipped with this issue being present and the vendor kernel using a tree-wide approach to cpu/cluster/system states. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-07-28 8:40 ` Sudeep Holla 2022-07-28 9:15 ` Dmitry Baryshkov @ 2022-08-05 14:12 ` Ulf Hansson 2022-08-05 16:00 ` Sudeep Holla 1 sibling, 1 reply; 12+ messages in thread From: Ulf Hansson @ 2022-08-05 14:12 UTC (permalink / raw) To: Sudeep Holla Cc: Dmitry Baryshkov, linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM On Thu, 28 Jul 2022 at 10:40, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jul 27, 2022 at 04:24:22PM +0300, Dmitry Baryshkov wrote: > > On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > > > > Hi, > > > > > > > > Lately I have been working on improving the msm8996 platform support. > > > > Vendor kernel seems to support domain-like idle (see [1], [2]). > > > > However when I tried changing upstream msm8996.dtsi to use PSCI > > > > domains, I faced the firmware reporting NOT_SUPPORTED to an attempt to > > > > enable OSI (thus rendering PSCI domains useless, as they are now > > > > marked with ALWAYS_ON). > > > > > > > > > > That's not good to hear 🙁. > > > > > > > I noticed that vendor kernel makes a call to cpu_suspend() with > > > > power_state following the original format (described in PSCI spec > > > > 5.4.2.1). What would be the best way to support this? > > > > > > And why is this not possible with the existing code ? Not sure if I > > > understood it right, I am assuming you are mentioning that it is not > > > possible. > > > > It's not possible with the cpuidle-psci-domains.c. The driver marks > > all genpds as ALWAYS_ON, thus making sure that they are never > > suspended. > > > > That doesn't sound correct. I am sure Ulf has tried this on one of SDM > platform for sure when it was merged. It looks like there may be some misunderstanding here. I think the point Dmitry is trying to make, is that the we set the GENPD_FLAG_ALWAYS_ON for the created genpds, when the OSI mode could not be turned on, when probing the cpuidle-psci-domain driver. In this way, all of cluster idle-states become disabled. More details about why we have this behaviour can be found from the commit below. 70c179b49870 ("cpuidle: psci: Allow PM domain to be initialized even if no OSI mode") > > > > > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > > > > > > Meaning DTS flag for this ? If OSI enable fails, why would you want to > > > still proceed. It is non-compliant and must be fixed if the firmware > > > supports OSI and expects OSPM to use the same. > > > > I'm not sure at this moment. PSCI firmware reports that OSI mode is > > supported, but then when psci_pd_try_set_osi_mode() tries to switch > > into OSI mode, it gets NOT_SUPPORTED. > > Yikes, fix the damn broken firmware. That is utter non-sense. I don't > understand why would the firmware authors enable some feature before it > is ready. I certainly agree that the FW is broken and should really have been fixed, but that seems unlikely to happen when moving forward. On the other hand, it's quite common that we try to add workarounds at the Linux side to fix FW issues. Of course, it depends on what kind of hacks it means for us to carry. In this particular case, I am of the opinion that it looks like the "hack" may be worth it. Unless I have underestimated the problem, it seems like a new DT property/flag and a simple if-clause in psci_pd_try_set_osi_mode() should do the trick for us. I wouldn't mind maintaining such small parts, when going forward - and of course I think we should also reject any newer platforms from using it. > > > Just for the sake of completeness, I added a print to the psci.c to > > dump the result of the psci_set_osi_mode(false). It also returns > > NOT_SUPPORTED! > > > > Well it is simply broken then. Not tested firmware, so please don't > attempt to use OSI if it is so fundamentally broken. I find it hard to > accept the argument that well it works just that the query API is failing. > But what is the guarantee that it is tested well enough. We will end up > adding more quirks after adding one to enable it. > > > My logical assumption would be that the firmware reports support for > > OS_INITIATED, but then just fails to properly support > > SET_SUSPEND_MODE. > > I knew this argument was coming as I wrote above, sorry I don't buy that. > It is probably one of the early platforms supporting PSCI and not well tested > for conformance. So I am inclined to just say we can't support it. > As Dmitry also wrote in his other reply, the FW has certainly been well tested with the cluster idle states (acting like OSI mode has been enabled). To me, it seems like a pity, if we just decided to leave all those devices out there in the field, lacking support for deeper idle states. Don't you think? Kind regards Uffe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-08-05 14:12 ` Ulf Hansson @ 2022-08-05 16:00 ` Sudeep Holla 2022-08-05 21:39 ` Dmitry Baryshkov 0 siblings, 1 reply; 12+ messages in thread From: Sudeep Holla @ 2022-08-05 16:00 UTC (permalink / raw) To: Ulf Hansson Cc: Dmitry Baryshkov, linux-arm-msm, Bjorn Andersson, Sudeep Holla, Vinod Koul, Linux PM On Fri, Aug 05, 2022 at 04:12:42PM +0200, Ulf Hansson wrote: > On Thu, 28 Jul 2022 at 10:40, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Wed, Jul 27, 2022 at 04:24:22PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > > > > > Hi, > > > > > > > > > > Lately I have been working on improving the msm8996 platform support. > > > > > Vendor kernel seems to support domain-like idle (see [1], [2]). > > > > > However when I tried changing upstream msm8996.dtsi to use PSCI > > > > > domains, I faced the firmware reporting NOT_SUPPORTED to an attempt to > > > > > enable OSI (thus rendering PSCI domains useless, as they are now > > > > > marked with ALWAYS_ON). > > > > > > > > > > > > > That's not good to hear 🙁. > > > > > > > > > I noticed that vendor kernel makes a call to cpu_suspend() with > > > > > power_state following the original format (described in PSCI spec > > > > > 5.4.2.1). What would be the best way to support this? > > > > > > > > And why is this not possible with the existing code ? Not sure if I > > > > understood it right, I am assuming you are mentioning that it is not > > > > possible. > > > > > > It's not possible with the cpuidle-psci-domains.c. The driver marks > > > all genpds as ALWAYS_ON, thus making sure that they are never > > > suspended. > > > > > > > That doesn't sound correct. I am sure Ulf has tried this on one of SDM > > platform for sure when it was merged. > > It looks like there may be some misunderstanding here. > > I think the point Dmitry is trying to make, is that the we set the > GENPD_FLAG_ALWAYS_ON for the created genpds, when the OSI mode could > not be turned on, when probing the cpuidle-psci-domain driver. In this > way, all of cluster idle-states become disabled. > > More details about why we have this behaviour can be found from the > commit below. > > 70c179b49870 ("cpuidle: psci: Allow PM domain to be initialized even > if no OSI mode") > Ah OK, understood now. > > > > > > > - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > > > > > > > > Meaning DTS flag for this ? If OSI enable fails, why would you want to > > > > still proceed. It is non-compliant and must be fixed if the firmware > > > > supports OSI and expects OSPM to use the same. > > > > > > I'm not sure at this moment. PSCI firmware reports that OSI mode is > > > supported, but then when psci_pd_try_set_osi_mode() tries to switch > > > into OSI mode, it gets NOT_SUPPORTED. > > > > Yikes, fix the damn broken firmware. That is utter non-sense. I don't > > understand why would the firmware authors enable some feature before it > > is ready. > > I certainly agree that the FW is broken and should really have been > fixed, but that seems unlikely to happen when moving forward. > > On the other hand, it's quite common that we try to add workarounds at > the Linux side to fix FW issues. Of course, it depends on what kind of > hacks it means for us to carry. > > In this particular case, I am of the opinion that it looks like the > "hack" may be worth it. Unless I have underestimated the problem, it > seems like a new DT property/flag and a simple if-clause in > psci_pd_try_set_osi_mode() should do the trick for us. > I don't like the idea of new property/flag for this for simple reason. Once you have that it is impossible to control if downstream new platforms are using it and they will expect it to be maintained once they ship the product. > I wouldn't mind maintaining such small parts, when going forward - and > of course I think we should also reject any newer platforms from using > it. > The only way that we can achieve this IMO is to have quirks based on platform compatible which needs to be updated and can be rejected for newer platforms. New flags means new feature which is expected to be supported for long and hard to control newer platforms not using them. > > > > > Just for the sake of completeness, I added a print to the psci.c to > > > dump the result of the psci_set_osi_mode(false). It also returns > > > NOT_SUPPORTED! > > > > > > > Well it is simply broken then. Not tested firmware, so please don't > > attempt to use OSI if it is so fundamentally broken. I find it hard to > > accept the argument that well it works just that the query API is failing. > > But what is the guarantee that it is tested well enough. We will end up > > adding more quirks after adding one to enable it. > > > > > My logical assumption would be that the firmware reports support for > > > OS_INITIATED, but then just fails to properly support > > > SET_SUSPEND_MODE. > > > > I knew this argument was coming as I wrote above, sorry I don't buy that. > > It is probably one of the early platforms supporting PSCI and not well tested > > for conformance. So I am inclined to just say we can't support it. > > > > As Dmitry also wrote in his other reply, the FW has certainly been > well tested with the cluster idle states (acting like OSI mode has > been enabled). > OK, fair enough if you are aware that is the case here. > To me, it seems like a pity, if we just decided to leave all those > devices out there in the field, lacking support for deeper idle > states. Don't you think? > Sure, but I don't like new flags for handling this for reasons stated above. Unless DT maintainers expect to take "new flag/property" for some reasons that I am not aware of, I prefer the check on existing platform compatible to deal with this problem so that this problem doesn't trickle down to newer platforms as well. Thoughts ? And please add that we can't add any compatibles that are added later than certain date to that list when we are adding this support. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-08-05 16:00 ` Sudeep Holla @ 2022-08-05 21:39 ` Dmitry Baryshkov 2022-08-12 11:32 ` Ulf Hansson 0 siblings, 1 reply; 12+ messages in thread From: Dmitry Baryshkov @ 2022-08-05 21:39 UTC (permalink / raw) To: Sudeep Holla, Ulf Hansson Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM On 05/08/2022 19:00, Sudeep Holla wrote: > On Fri, Aug 05, 2022 at 04:12:42PM +0200, Ulf Hansson wrote: >> On Thu, 28 Jul 2022 at 10:40, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >>> On Wed, Jul 27, 2022 at 04:24:22PM +0300, Dmitry Baryshkov wrote: >>>> On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>>> >>>>> On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: >>>>>> - Allow DTS forcing the PSCI power domains even if OSI enablement fails? >>>>> >>>>> Meaning DTS flag for this ? If OSI enable fails, why would you want to >>>>> still proceed. It is non-compliant and must be fixed if the firmware >>>>> supports OSI and expects OSPM to use the same. >>>> >>>> I'm not sure at this moment. PSCI firmware reports that OSI mode is >>>> supported, but then when psci_pd_try_set_osi_mode() tries to switch >>>> into OSI mode, it gets NOT_SUPPORTED. >>> >>> Yikes, fix the damn broken firmware. That is utter non-sense. I don't >>> understand why would the firmware authors enable some feature before it >>> is ready. >> >> I certainly agree that the FW is broken and should really have been >> fixed, but that seems unlikely to happen when moving forward. >> >> On the other hand, it's quite common that we try to add workarounds at >> the Linux side to fix FW issues. Of course, it depends on what kind of >> hacks it means for us to carry. >> >> In this particular case, I am of the opinion that it looks like the >> "hack" may be worth it. Unless I have underestimated the problem, it >> seems like a new DT property/flag and a simple if-clause in >> psci_pd_try_set_osi_mode() should do the trick for us. >> > > I don't like the idea of new property/flag for this for simple reason. > Once you have that it is impossible to control if downstream new platforms > are using it and they will expect it to be maintained once they ship the > product. According to my quick research the requested issue was present on platforms revealed from 2015 (2013?) to 2017. Since sdm845 (December 2017) the issue is not present. So this is a part of history rather than current platforms being in need of this quirk. > >> I wouldn't mind maintaining such small parts, when going forward - and >> of course I think we should also reject any newer platforms from using >> it. >> > > The only way that we can achieve this IMO is to have quirks based on > platform compatible which needs to be updated and can be rejected for > newer platforms. New flags means new feature which is expected to be > supported for long and hard to control newer platforms not using them. I see your point. However from my point of view, the DT property allows more finer-grained control. Compare the semantics: - Proprety: assume that the platform is in OSI mode, do not call psci_osi_set_mode(). - Platform compat list: If the psci_osi_set_mode() has failed, ignore the error. I would not dare to blindly assume that e.g. all msm8996 devices are in OSI mode. [skipped] >> To me, it seems like a pity, if we just decided to leave all those >> devices out there in the field, lacking support for deeper idle >> states. Don't you think? >> > > Sure, but I don't like new flags for handling this for reasons stated > above. Unless DT maintainers expect to take "new flag/property" for > some reasons that I am not aware of, I prefer the check on existing > platform compatible to deal with this problem so that this problem > doesn't trickle down to newer platforms as well. Thoughts ? > > And please add that we can't add any compatibles that are added later > than certain date to that list when we are adding this support. Then we are ruling out existing platforms, which are not yet supported by Linux. Hopefully you meant that we can not extend this list with platforms developed/announced after certain date (I might be slightly wrong here, but I estimate that the end of 2018 as a safe boundary). Newer platforms (sdm845, 2018, qcs404, 2019, etc) report SET_SUSPEND as supported (and return DENIED in attempt to set the PC mode). -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PSCI domains without OSI support 2022-08-05 21:39 ` Dmitry Baryshkov @ 2022-08-12 11:32 ` Ulf Hansson 0 siblings, 0 replies; 12+ messages in thread From: Ulf Hansson @ 2022-08-12 11:32 UTC (permalink / raw) To: Dmitry Baryshkov, Sudeep Holla Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, Linux PM On Fri, 5 Aug 2022 at 23:39, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 05/08/2022 19:00, Sudeep Holla wrote: > > On Fri, Aug 05, 2022 at 04:12:42PM +0200, Ulf Hansson wrote: > >> On Thu, 28 Jul 2022 at 10:40, Sudeep Holla <sudeep.holla@arm.com> wrote: > >>> > >>> On Wed, Jul 27, 2022 at 04:24:22PM +0300, Dmitry Baryshkov wrote: > >>>> On Wed, 27 Jul 2022 at 14:14, Sudeep Holla <sudeep.holla@arm.com> wrote: > >>>>> > >>>>> On Wed, Jul 27, 2022 at 12:09:27PM +0300, Dmitry Baryshkov wrote: > >>>>>> - Allow DTS forcing the PSCI power domains even if OSI enablement fails? > >>>>> > >>>>> Meaning DTS flag for this ? If OSI enable fails, why would you want to > >>>>> still proceed. It is non-compliant and must be fixed if the firmware > >>>>> supports OSI and expects OSPM to use the same. > >>>> > >>>> I'm not sure at this moment. PSCI firmware reports that OSI mode is > >>>> supported, but then when psci_pd_try_set_osi_mode() tries to switch > >>>> into OSI mode, it gets NOT_SUPPORTED. > >>> > >>> Yikes, fix the damn broken firmware. That is utter non-sense. I don't > >>> understand why would the firmware authors enable some feature before it > >>> is ready. > >> > >> I certainly agree that the FW is broken and should really have been > >> fixed, but that seems unlikely to happen when moving forward. > >> > >> On the other hand, it's quite common that we try to add workarounds at > >> the Linux side to fix FW issues. Of course, it depends on what kind of > >> hacks it means for us to carry. > >> > >> In this particular case, I am of the opinion that it looks like the > >> "hack" may be worth it. Unless I have underestimated the problem, it > >> seems like a new DT property/flag and a simple if-clause in > >> psci_pd_try_set_osi_mode() should do the trick for us. > >> > > > > I don't like the idea of new property/flag for this for simple reason. > > Once you have that it is impossible to control if downstream new platforms > > are using it and they will expect it to be maintained once they ship the > > product. > > According to my quick research the requested issue was present on > platforms revealed from 2015 (2013?) to 2017. Since sdm845 (December > 2017) the issue is not present. So this is a part of history rather than > current platforms being in need of this quirk. > > > > >> I wouldn't mind maintaining such small parts, when going forward - and > >> of course I think we should also reject any newer platforms from using > >> it. > >> > > > > The only way that we can achieve this IMO is to have quirks based on > > platform compatible which needs to be updated and can be rejected for > > newer platforms. New flags means new feature which is expected to be > > supported for long and hard to control newer platforms not using them. > > I see your point. However from my point of view, the DT property allows > more finer-grained control. It does, but perhaps it's not really needed, as you indicate below? I share Sudeep's concerns about future possible abuse, so if we think we can get the "compat list approach" to work, that sounds better for me too. > > Compare the semantics: > - Proprety: assume that the platform is in OSI mode, do not call > psci_osi_set_mode(). > > - Platform compat list: If the psci_osi_set_mode() has failed, ignore > the error. I would not dare to blindly assume that e.g. all msm8996 > devices are in OSI mode. You have a point! So it looks safer to ignore the error that is returned from psci_osi_set_mode(), rather than skipping to call it (maybe we want to log a warning when it fails though?). [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-08-12 11:33 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-27 9:09 PSCI domains without OSI support Dmitry Baryshkov 2022-07-27 11:14 ` Sudeep Holla 2022-07-27 13:24 ` Dmitry Baryshkov 2022-07-27 13:39 ` Dmitry Baryshkov 2022-07-27 20:51 ` Dmitry Baryshkov 2022-08-05 13:47 ` Ulf Hansson 2022-07-28 8:40 ` Sudeep Holla 2022-07-28 9:15 ` Dmitry Baryshkov 2022-08-05 14:12 ` Ulf Hansson 2022-08-05 16:00 ` Sudeep Holla 2022-08-05 21:39 ` Dmitry Baryshkov 2022-08-12 11:32 ` Ulf Hansson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.