On Fri, 22 Nov 2019 at 19:26, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote: > > On Mon, Nov 18, 2019 at 02:37:41PM +0100, Ulf Hansson wrote: > > On Fri, 15 Nov 2019 at 18:30, Sudeep Holla <sudeep.holla@xxxxxxx> wrote: > > > > > > On Tue, Oct 29, 2019 at 05:44:35PM +0100, Ulf Hansson wrote: > > > > The per CPU variable psci_power_state, contains an array of fixed values, > > > > which reflects the corresponding arm,psci-suspend-param parsed from DT, for > > > > each of the available CPU idle states. > > > > > > > > This isn't sufficient when using the hierarchical CPU topology in DT, in > > > > combination with having PSCI OS initiated (OSI) mode enabled. More > > > > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what > > > > idle state the cluster (a group of CPUs) should enter, while in PSCI > > > > Platform Coordinated (PC) mode, each CPU independently votes for an idle > > > > state of the cluster. > > > > > > > > For this reason, introduce a per CPU variable called domain_state and > > > > implement two helper functions to read/write its value. Then let the > > > > domain_state take precedence over the regular selected state, when entering > > > > and idle state. > > > > > > > > Finally, let's also avoid sprinkling the existing non-OSI path with > > > > operations being specific for OSI. > > > > > > > > > > Mostly looks good. > > > > Thanks! > > > > > > > I am still wondering if we can keep all OSI related > > > info in the newly created structure and have psci_states outside it as > > > before. And I was think psci_enter_idle_state_pc and psci_enter_idle_state_osi > > > instead of single psci_enter_idle_state and assign/initialise state->enter > > > based on the mode chosen. I had to closer look now and looks like enter > > > is initialised in generic dt_idle_states. That said, what you have in this > > > patch also looks OK to me, was just trying to avoid access to the new > > > structure all together and keep the PC mode patch almost same as before > > > when suspending. I will see what Lorenzo thinks about this. > > > > I did explore that approach a bit, but found it easier to go with what > > I propose here. The most important point, in my view, is that in this > > suggested approach only one if-check, "if (!data->dev)", is added to > > the PC mode path compared to the original path. I think this should be > > fine, right!? > > I don't see why we should use data->dev at runtime when we can > have two separate idle enter functions and the detection can > be done at probe once for all instead of every idle entry. > > The overhead is close to nought but the point is that it is > really not needed. Alright, I will adopt our suggestion and override the assigned idle enter callback, when we succeed to attach the cpu-device to its PM domain. Do you have any other thoughts about the series? Kind regards Uffe
