| Message ID | 20230215071649.9078-1-quic_mkshah@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use PSCI OS initiated mode for sc7280 | expand |
On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote: > This change adds power-domains for cpuidle states to use PSCI OS > initiated mode for sc7280. > > This change depends on external project changes [1] & [2] which are under > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware. > > I can update here once the dependency are in and change is ready to merge. > Please do, I will drop this from the queue for now. Thanks, Bjorn > [1] https://review.trustedfirmware.org/q/topic:psci-osi > [2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/19487 > > Maulik Shah (1): > arm64: dts: qcom: sc7280: Add power-domains for cpuidle states > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 96 +++++++++++++++++++++------- > 1 file changed, 72 insertions(+), 24 deletions(-) > > -- > 2.17.1 >
Hi, On Mon, Feb 27, 2023 at 7:35 AM Bjorn Andersson <andersson@kernel.org> wrote: > > On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote: > > This change adds power-domains for cpuidle states to use PSCI OS > > initiated mode for sc7280. > > > > This change depends on external project changes [1] & [2] which are under > > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware. > > > > I can update here once the dependency are in and change is ready to merge. > > > > Please do, I will drop this from the queue for now. I'm a bit confused about why we're doing this. There's always been a question about exactly why we need OSI mode. As far as I can tell it can't be for "correctness" reasons because we managed to ship sc7180 without OSI mode. ...so I guess somehow the argument is that OSI mode is more performant in some cases? Are there actual numbers backing this up, or is it all theoretical? Before making such a big change, it would be good to actually understand what the motivation is and see real data. This should be easy to collect since we currently have things working without OSI and (presumably) you have OSI working. It would also be good to document this motivation in the commit message and/or cover letter. -Doug
On Mon, 27 Feb 2023 at 17:10, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Feb 27, 2023 at 7:35 AM Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote: > > > This change adds power-domains for cpuidle states to use PSCI OS > > > initiated mode for sc7280. > > > > > > This change depends on external project changes [1] & [2] which are under > > > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware. > > > > > > I can update here once the dependency are in and change is ready to merge. > > > > > > > Please do, I will drop this from the queue for now. > > I'm a bit confused about why we're doing this. There's always been a > question about exactly why we need OSI mode. As far as I can tell it > can't be for "correctness" reasons because we managed to ship sc7180 > without OSI mode. ...so I guess somehow the argument is that OSI mode > is more performant in some cases? Are there actual numbers backing > this up, or is it all theoretical? Before making such a big change, it > would be good to actually understand what the motivation is and see > real data. This should be easy to collect since we currently have > things working without OSI and (presumably) you have OSI working. It > would also be good to document this motivation in the commit message > and/or cover letter. I certainly don't object to what you say here. Although, let me also share some more background to these suggested changes. As you know, for mobile platforms, Qcom have been using OS-initiated mode for years, but on Chromium platforms that has been limited to the default platform-coordinated mode. Whether that is a deliberate decision for the Chromium platforms or rather because the PSCI implementation in TF-A has been lacking OSI support, I don't know. Maybe you have some more insight to share around this? Note that, Wing has been working on adding support for PSCI OSI mode to TF-A [1], which hopefully should land soon. In this regard, it seems like we are getting closer to finally being able to run some more in-depth tests, that should allow us to better compare the behaviour of the PSCI CPU-suspend modes - at least on some platforms. In fact, Maulik/Wing also presented their work around this topic, including some results around performance/energy tests at the last TF-A call [2]. I think some of that data could be shared in the commit message too. Kind regards Uffe [1] https://review.trustedfirmware.org/q/topic:psci-osi [2] https://www.trustedfirmware.org/meetings/tf-a-technical-forum
Hi, On Tue, Feb 28, 2023 at 4:17 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Mon, 27 Feb 2023 at 17:10, Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Mon, Feb 27, 2023 at 7:35 AM Bjorn Andersson <andersson@kernel.org> wrote: > > > > > > On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote: > > > > This change adds power-domains for cpuidle states to use PSCI OS > > > > initiated mode for sc7280. > > > > > > > > This change depends on external project changes [1] & [2] which are under > > > > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware. > > > > > > > > I can update here once the dependency are in and change is ready to merge. > > > > > > > > > > Please do, I will drop this from the queue for now. > > > > I'm a bit confused about why we're doing this. There's always been a > > question about exactly why we need OSI mode. As far as I can tell it > > can't be for "correctness" reasons because we managed to ship sc7180 > > without OSI mode. ...so I guess somehow the argument is that OSI mode > > is more performant in some cases? Are there actual numbers backing > > this up, or is it all theoretical? Before making such a big change, it > > would be good to actually understand what the motivation is and see > > real data. This should be easy to collect since we currently have > > things working without OSI and (presumably) you have OSI working. It > > would also be good to document this motivation in the commit message > > and/or cover letter. > > I certainly don't object to what you say here. Although, let me also > share some more background to these suggested changes. > > As you know, for mobile platforms, Qcom have been using OS-initiated > mode for years, but on Chromium platforms that has been limited to the > default platform-coordinated mode. Whether that is a deliberate > decision for the Chromium platforms or rather because the PSCI > implementation in TF-A has been lacking OSI support, I don't know. > Maybe you have some more insight to share around this? You hit the reason exactly. Nobody on the ChromeOS team objected to OSI, per say, but it was never supported in ARM Trusted Firmware. I still don't have anything against OSI mode, but I just want to make sure that the data is there and that we're not just arbitrarily churning things. ;-) I think sc7180's ship has sailed at this point. While we could update the firmware for testing, I don't think we'd switch production sc7180 devices over to OSI. That means that we'll always need to support PC mode for sc7180. Switching sc7280 over to OSI needs to be justified given that we'll have to continue to support sc7180 with PC mode anyway. > Note that, Wing has been working on adding support for PSCI OSI mode > to TF-A [1], which hopefully should land soon. In this regard, it > seems like we are getting closer to finally being able to run some > more in-depth tests, that should allow us to better compare the > behaviour of the PSCI CPU-suspend modes - at least on some platforms. > In fact, Maulik/Wing also presented their work around this topic, > including some results around performance/energy tests at the last > TF-A call [2]. I think some of that data could be shared in the commit > message too. Yup, I was mostly just asking for data like you provided to be in the commit message. > Kind regards > Uffe > > [1] > https://review.trustedfirmware.org/q/topic:psci-osi > > [2] > https://www.trustedfirmware.org/meetings/tf-a-technical-forum
On Tue, 28 Feb 2023 at 16:25, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Feb 28, 2023 at 4:17 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Mon, 27 Feb 2023 at 17:10, Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Mon, Feb 27, 2023 at 7:35 AM Bjorn Andersson <andersson@kernel.org> wrote: > > > > > > > > On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote: > > > > > This change adds power-domains for cpuidle states to use PSCI OS > > > > > initiated mode for sc7280. > > > > > > > > > > This change depends on external project changes [1] & [2] which are under > > > > > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware. > > > > > > > > > > I can update here once the dependency are in and change is ready to merge. > > > > > > > > > > > > > Please do, I will drop this from the queue for now. > > > > > > I'm a bit confused about why we're doing this. There's always been a > > > question about exactly why we need OSI mode. As far as I can tell it > > > can't be for "correctness" reasons because we managed to ship sc7180 > > > without OSI mode. ...so I guess somehow the argument is that OSI mode > > > is more performant in some cases? Are there actual numbers backing > > > this up, or is it all theoretical? Before making such a big change, it > > > would be good to actually understand what the motivation is and see > > > real data. This should be easy to collect since we currently have > > > things working without OSI and (presumably) you have OSI working. It > > > would also be good to document this motivation in the commit message > > > and/or cover letter. > > > > I certainly don't object to what you say here. Although, let me also > > share some more background to these suggested changes. > > > > As you know, for mobile platforms, Qcom have been using OS-initiated > > mode for years, but on Chromium platforms that has been limited to the > > default platform-coordinated mode. Whether that is a deliberate > > decision for the Chromium platforms or rather because the PSCI > > implementation in TF-A has been lacking OSI support, I don't know. > > Maybe you have some more insight to share around this? > > You hit the reason exactly. Nobody on the ChromeOS team objected to > OSI, per say, but it was never supported in ARM Trusted Firmware. I > still don't have anything against OSI mode, but I just want to make > sure that the data is there and that we're not just arbitrarily > churning things. ;-) Thanks for sharing this information! It certainly helps to better understand the background for all of us. > > I think sc7180's ship has sailed at this point. While we could update > the firmware for testing, I don't think we'd switch production sc7180 > devices over to OSI. That means that we'll always need to support PC > mode for sc7180. Switching sc7280 over to OSI needs to be justified > given that we'll have to continue to support sc7180 with PC mode > anyway. Right. Supporting both OSI and PC mode, dynamically based upon what the PSCI firmware (TF-A) supports should work fine from Linux point of view. I may be overlooking something for this particular case - and in that case, I am happy to help to fix it! > > > > Note that, Wing has been working on adding support for PSCI OSI mode > > to TF-A [1], which hopefully should land soon. In this regard, it > > seems like we are getting closer to finally being able to run some > > more in-depth tests, that should allow us to better compare the > > behaviour of the PSCI CPU-suspend modes - at least on some platforms. > > In fact, Maulik/Wing also presented their work around this topic, > > including some results around performance/energy tests at the last > > TF-A call [2]. I think some of that data could be shared in the commit > > message too. > > Yup, I was mostly just asking for data like you provided to be in the > commit message. Great, thanks for confirming! > > > > Kind regards > > Uffe > > > > [1] > > https://review.trustedfirmware.org/q/topic:psci-osi > > > > [2] > > https://www.trustedfirmware.org/meetings/tf-a-technical-forum Kind regards Uffe