mbox series

[v4,0/3] Use PSCI OS initiated mode for sc7280

Message ID 20230424110933.3908-1-quic_mkshah@quicinc.com
Headers show
Series Use PSCI OS initiated mode for sc7280 | expand

Message

Maulik Shah (mkshah) April 24, 2023, 11:09 a.m. UTC
Changes in v4:
- Add missing s-o-b line and reviewed by in patch 1
- Address ulf's comments for error handling in patch 2

Changes in v3:
- Add new change to provide helper function dt_idle_pd_remove_topology()
- Address ulf's comments for error handling
- Add reviewed by ulf for devicetree change

Changes in v2:
- Add new change to Move enabling OSI mode after power domains creation
- Fix compatible string to domains-idle-states for cluster idle state.
- Update cover letter with some more details on OSI and PC mode
  comparision

The dependency [2] is now merged in trustedfirmware project.

Stats comparision between OSI and PC mode are captured at [3] with
usecase
details, where during multiple CPUs online the residency in cluster idle
state is better with OSI and also inline with single CPU mode. In PC
mode
with multiple CPUs cluster idle state residency is dropping compare to
single CPU mode.

Recording of this meeting is also available at [4].

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.

[1] https://review.trustedfirmware.org/q/topic:psci-osi
[2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/19487
[3] https://www.trustedfirmware.org/docs/PSCI-OS-initiated.pdf
[4] https://www.trustedfirmware.org/meetings/tf-a-technical-forum

Maulik Shah (3):
  cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
  cpuidle: psci: Move enabling OSI mode after power domains creation
  arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

 arch/arm64/boot/dts/qcom/sc7280.dtsi  | 98 ++++++++++++++++++++-------
 drivers/cpuidle/cpuidle-psci-domain.c | 39 ++++-------
 drivers/cpuidle/dt_idle_genpd.c       | 24 +++++++
 drivers/cpuidle/dt_idle_genpd.h       |  7 ++
 4 files changed, 117 insertions(+), 51 deletions(-)

Comments

Ulf Hansson May 24, 2023, 9:56 a.m. UTC | #1
On Mon, 24 Apr 2023 at 13:09, Maulik Shah <quic_mkshah@quicinc.com> wrote:
>
> Changes in v4:
> - Add missing s-o-b line and reviewed by in patch 1
> - Address ulf's comments for error handling in patch 2
>
> Changes in v3:
> - Add new change to provide helper function dt_idle_pd_remove_topology()
> - Address ulf's comments for error handling
> - Add reviewed by ulf for devicetree change
>
> Changes in v2:
> - Add new change to Move enabling OSI mode after power domains creation
> - Fix compatible string to domains-idle-states for cluster idle state.
> - Update cover letter with some more details on OSI and PC mode
>   comparision
>
> The dependency [2] is now merged in trustedfirmware project.
>
> Stats comparision between OSI and PC mode are captured at [3] with
> usecase
> details, where during multiple CPUs online the residency in cluster idle
> state is better with OSI and also inline with single CPU mode. In PC
> mode
> with multiple CPUs cluster idle state residency is dropping compare to
> single CPU mode.
>
> Recording of this meeting is also available at [4].
>
> 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.
>
> [1] https://review.trustedfirmware.org/q/topic:psci-osi
> [2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/19487
> [3] https://www.trustedfirmware.org/docs/PSCI-OS-initiated.pdf
> [4] https://www.trustedfirmware.org/meetings/tf-a-technical-forum
>
> Maulik Shah (3):
>   cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
>   cpuidle: psci: Move enabling OSI mode after power domains creation
>   arm64: dts: qcom: sc7280: Add power-domains for cpuidle states
>
>  arch/arm64/boot/dts/qcom/sc7280.dtsi  | 98 ++++++++++++++++++++-------
>  drivers/cpuidle/cpuidle-psci-domain.c | 39 ++++-------
>  drivers/cpuidle/dt_idle_genpd.c       | 24 +++++++
>  drivers/cpuidle/dt_idle_genpd.h       |  7 ++
>  4 files changed, 117 insertions(+), 51 deletions(-)
>

Looks like this series has not been queued up yet. Note that patch1
and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
DTS change) is dependent on patch 1 and patch2.

Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
Bjorn, is that okay for you?

Kind regards
Uffe
Bjorn Andersson May 25, 2023, 2:45 a.m. UTC | #2
On Wed, May 24, 2023 at 11:56:28AM +0200, Ulf Hansson wrote:
> On Mon, 24 Apr 2023 at 13:09, Maulik Shah <quic_mkshah@quicinc.com> wrote:
> >
> > Changes in v4:
> > - Add missing s-o-b line and reviewed by in patch 1
> > - Address ulf's comments for error handling in patch 2
> >
> > Changes in v3:
> > - Add new change to provide helper function dt_idle_pd_remove_topology()
> > - Address ulf's comments for error handling
> > - Add reviewed by ulf for devicetree change
> >
> > Changes in v2:
> > - Add new change to Move enabling OSI mode after power domains creation
> > - Fix compatible string to domains-idle-states for cluster idle state.
> > - Update cover letter with some more details on OSI and PC mode
> >   comparision
> >
> > The dependency [2] is now merged in trustedfirmware project.
> >
> > Stats comparision between OSI and PC mode are captured at [3] with
> > usecase
> > details, where during multiple CPUs online the residency in cluster idle
> > state is better with OSI and also inline with single CPU mode. In PC
> > mode
> > with multiple CPUs cluster idle state residency is dropping compare to
> > single CPU mode.
> >
> > Recording of this meeting is also available at [4].
> >
> > 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.
> >
> > [1] https://review.trustedfirmware.org/q/topic:psci-osi
> > [2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/19487
> > [3] https://www.trustedfirmware.org/docs/PSCI-OS-initiated.pdf
> > [4] https://www.trustedfirmware.org/meetings/tf-a-technical-forum
> >
> > Maulik Shah (3):
> >   cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
> >   cpuidle: psci: Move enabling OSI mode after power domains creation
> >   arm64: dts: qcom: sc7280: Add power-domains for cpuidle states
> >
> >  arch/arm64/boot/dts/qcom/sc7280.dtsi  | 98 ++++++++++++++++++++-------
> >  drivers/cpuidle/cpuidle-psci-domain.c | 39 ++++-------
> >  drivers/cpuidle/dt_idle_genpd.c       | 24 +++++++
> >  drivers/cpuidle/dt_idle_genpd.h       |  7 ++
> >  4 files changed, 117 insertions(+), 51 deletions(-)
> >
> 
> Looks like this series has not been queued up yet. Note that patch1
> and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
> DTS change) is dependent on patch 1 and patch2.
> 
> Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
> Bjorn, is that okay for you?
> 

Sorry, this fell between the chairs after you pointed me to it...

I can certainly pick the 3 patches through my tree, but are they fixing
any current regressions, or is it just that we need the first two
patches to land before the 3rd patch?

I also presume the 3rd patch is only needed when paired with the new
ATF?

Regards,
Bjorn
Ulf Hansson May 30, 2023, 10:11 p.m. UTC | #3
+ Rafael

On Mon, 29 May 2023 at 18:05, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Mon, May 29, 2023 at 10:58:23AM +0200, Ulf Hansson wrote:
> > On Thu, 25 May 2023 at 04:41, Bjorn Andersson <andersson@kernel.org> wrote:
> > >
> > > On Wed, May 24, 2023 at 11:56:28AM +0200, Ulf Hansson wrote:
> > > > On Mon, 24 Apr 2023 at 13:09, Maulik Shah <quic_mkshah@quicinc.com> wrote:
> > > > >
> > > > > Changes in v4:
> > > > > - Add missing s-o-b line and reviewed by in patch 1
> > > > > - Address ulf's comments for error handling in patch 2
> > > > >
> > > > > Changes in v3:
> > > > > - Add new change to provide helper function dt_idle_pd_remove_topology()
> > > > > - Address ulf's comments for error handling
> > > > > - Add reviewed by ulf for devicetree change
> > > > >
> > > > > Changes in v2:
> > > > > - Add new change to Move enabling OSI mode after power domains creation
> > > > > - Fix compatible string to domains-idle-states for cluster idle state.
> > > > > - Update cover letter with some more details on OSI and PC mode
> > > > >   comparision
> > > > >
> > > > > The dependency [2] is now merged in trustedfirmware project.
> > > > >
> > > > > Stats comparision between OSI and PC mode are captured at [3] with
> > > > > usecase
> > > > > details, where during multiple CPUs online the residency in cluster idle
> > > > > state is better with OSI and also inline with single CPU mode. In PC
> > > > > mode
> > > > > with multiple CPUs cluster idle state residency is dropping compare to
> > > > > single CPU mode.
> > > > >
> > > > > Recording of this meeting is also available at [4].
> > > > >
> > > > > 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.
> > > > >
> > > > > [1] https://review.trustedfirmware.org/q/topic:psci-osi
> > > > > [2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/19487
> > > > > [3] https://www.trustedfirmware.org/docs/PSCI-OS-initiated.pdf
> > > > > [4] https://www.trustedfirmware.org/meetings/tf-a-technical-forum
> > > > >
> > > > > Maulik Shah (3):
> > > > >   cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
> > > > >   cpuidle: psci: Move enabling OSI mode after power domains creation
> > > > >   arm64: dts: qcom: sc7280: Add power-domains for cpuidle states
> > > > >
> > > > >  arch/arm64/boot/dts/qcom/sc7280.dtsi  | 98 ++++++++++++++++++++-------
> > > > >  drivers/cpuidle/cpuidle-psci-domain.c | 39 ++++-------
> > > > >  drivers/cpuidle/dt_idle_genpd.c       | 24 +++++++
> > > > >  drivers/cpuidle/dt_idle_genpd.h       |  7 ++
> > > > >  4 files changed, 117 insertions(+), 51 deletions(-)
> > > > >
> > > >
> > > > Looks like this series has not been queued up yet. Note that patch1
> > > > and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
> > > > DTS change) is dependent on patch 1 and patch2.
> > > >
> > > > Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
> > > > Bjorn, is that okay for you?
> > > >
> > >
> > > Sorry, this fell between the chairs after you pointed me to it...
> > >
> > > I can certainly pick the 3 patches through my tree, but are they fixing
> > > any current regressions, or is it just that we need the first two
> > > patches to land before the 3rd patch?
> >
> > I am not aware of any current regressions.
> >
>
> Okay, that confirms my understanding. So not -rc material.
>
> > >
> > > I also presume the 3rd patch is only needed when paired with the new
> > > ATF?
> >
> > Patch3 is beneficial to use with a new TF-A, but works with an old
> > TF-A too. Anyway, forget what I said about patch3 earlier, as that was
> > just not the complete information.
> >
> > The problem is that we can't be using a new TF-A (supporting both PSCI
> > OSI and PC mode) without patch1 and patch2, unless we are using
> > patch3.
> >
> > Thus, I strongly suggest we tag patch1 and patch2 for stable kernels,
> > to avoid any potential conflicts of TF-A versions that may be used.
> >
>
> So you're suggesting that I pick them for v6.5 and add a Cc: stable?
>
> An alternative would be that you take the cpuidle patches for v6.4-rc
> and I pick the dt for v6.5 - given that the cpuidle patches actually
> resolves a problem, while the dts just introduces "new functionality".

Right, that's probably the best option. Although I don't have a tree
to take these patches through, let's ask Rafael if he can help with
this.

Rafael, can you pick patch 1 and patch 2 from $subject series for
v6.4-rc and tag them for stable? Then Bjorn can pick patch3 for v6.5.

Kind regards
Uffe
Ulf Hansson June 12, 2023, 10:46 a.m. UTC | #4
[...]

> > > > >
> > > > > Looks like this series has not been queued up yet. Note that patch1
> > > > > and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
> > > > > DTS change) is dependent on patch 1 and patch2.
> > > > >
> > > > > Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
> > > > > Bjorn, is that okay for you?
> > > > >
> > > >
> > > > Sorry, this fell between the chairs after you pointed me to it...
> > > >
> > > > I can certainly pick the 3 patches through my tree, but are they fixing
> > > > any current regressions, or is it just that we need the first two
> > > > patches to land before the 3rd patch?
> > >
> > > I am not aware of any current regressions.
> > >
> >
> > Okay, that confirms my understanding. So not -rc material.
> >
> > > >
> > > > I also presume the 3rd patch is only needed when paired with the new
> > > > ATF?
> > >
> > > Patch3 is beneficial to use with a new TF-A, but works with an old
> > > TF-A too. Anyway, forget what I said about patch3 earlier, as that was
> > > just not the complete information.
> > >
> > > The problem is that we can't be using a new TF-A (supporting both PSCI
> > > OSI and PC mode) without patch1 and patch2, unless we are using
> > > patch3.
> > >
> > > Thus, I strongly suggest we tag patch1 and patch2 for stable kernels,
> > > to avoid any potential conflicts of TF-A versions that may be used.
> > >
> >
> > So you're suggesting that I pick them for v6.5 and add a Cc: stable?
> >
> > An alternative would be that you take the cpuidle patches for v6.4-rc
> > and I pick the dt for v6.5 - given that the cpuidle patches actually
> > resolves a problem, while the dts just introduces "new functionality".
>
> Right, that's probably the best option. Although I don't have a tree
> to take these patches through, let's ask Rafael if he can help with
> this.
>
> Rafael, can you pick patch 1 and patch 2 from $subject series for
> v6.4-rc and tag them for stable? Then Bjorn can pick patch3 for v6.5.

Rafael, Bjorn, sorry for nagging about this series. Can you please
help to pick it up?

Kind regards
Uffe