From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lorenzo Pieralisi Subject: Re: [PATCH v9 5/8] drivers: cpuidle: implement DT based idle states infrastructure Date: Tue, 9 Sep 2014 16:51:50 +0100 Message-ID: <20140909155150.GB4948@e102568-lin.cambridge.arm.com> References: <1409938498-17984-1-git-send-email-lorenzo.pieralisi@arm.com> <1409938498-17984-6-git-send-email-lorenzo.pieralisi@arm.com> <7hlhpxygzv.fsf@paris.lan> <20140905212908.GA28626@e102568-lin.cambridge.arm.com> <7hoauqw1c2.fsf@paris.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <7hoauqw1c2.fsf@paris.lan> Content-Disposition: inline Sender: linux-pm-owner@vger.kernel.org To: Kevin Hilman Cc: "linux-arm-kernel@lists.infradead.org" , "linux-pm@vger.kernel.org" , "devicetree@vger.kernel.org" , Mark Rutland , Sudeep Holla , Catalin Marinas , Will Deacon , Charles Garcia-Tobin , Nicolas Pitre , Rob Herring , "grant.likely@linaro.org" , Peter De Schrijver , Santosh Shilimkar , Daniel Lezcano , Amit Kucheria , Vincent Guittot , Antti Miettinen , Stephen Boyd , Sebastian Capella , Mark Brown List-Id: devicetree@vger.kernel.org On Mon, Sep 08, 2014 at 04:58:53PM +0100, Kevin Hilman wrote: > Lorenzo Pieralisi writes: > > > On Fri, Sep 05, 2014 at 09:00:52PM +0100, Kevin Hilman wrote: > >> Lorenzo Pieralisi writes: > >> > >> > On most common ARM systems, the low-power states a CPU can be put into are > >> > not discoverable in HW and require device tree bindings to describe > >> > power down suspend operations and idle states parameters. > >> > > >> > In order to enable DT based idle states and configure idle drivers, this > >> > patch implements the bulk infrastructure required to parse the device tree > >> > idle states bindings and initialize the corresponding CPUidle driver states > >> > data. > >> > > >> > The parsing API accepts a start index that defines the first idle state > >> > that should be initialized by the parsing code in order to give new and > >> > legacy driver flexibility over which states should be parsed using the > >> > new DT mechanism. > >> > > >> > The idle states node(s) is obtained from the phandle list of the first cpu > >> > in the driver cpumask; the kernel checks that the idle state node phandle > >> > is the same for all CPUs in the driver cpumask before declaring the idle state > >> > as valid and start parsing its content. > >> > > >> > The idle state enter function pointer is initialized through DT match > >> > structures passed in by the CPUidle driver, so that ARM legacy code can > >> > cope with platform specific idle entry method based on compatible > >> > string matching and the code used to initialize the enter function pointer > >> > can be moved to the DT generic layer. > >> > > >> > Acked-by: Catalin Marinas > >> > Acked-by: Daniel Lezcano > >> > Signed-off-by: Lorenzo Pieralisi > >> > >> [...] > >> > >> > + idle_state->flags = CPUIDLE_FLAG_TIME_VALID; > >> > + if (of_property_read_bool(state_node, "local-timer-stop")) > >> > + idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP; > >> > + /* > >> > + * TODO: > >> > + * replace with kstrdup and pointer assignment when name > >> > + * and desc become string pointers > >> > + */ > >> > + strncpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN - 1); > >> > + strncpy(idle_state->desc, state_node->name, CPUIDLE_DESC_LEN - 1); > >> > >> This is a very minor concern, and shouldn't hold back this series, > >> but... > >> > >> I was playing with this series in order to test out the qcom cpuidle > >> driver from Lina, and noticed that the state name and descriptions were > >> not terribly helpful: > >> > >> /sys/devices/system/cpu/cpu0/cpuidle # cat state?/name > >> cpu-idle-state- > >> cpu-idle-state- > >> /sys/devices/system/cpu/cpu0/cpuidle # cat state?/desc > >> cpu-idle-state-0 > >> cpu-idle-state-1 > >> > >> Turns out these strings come from the node name itself, and truncated in > >> the case of state->name, but this can be fixed in the DTS itself (c.f. > >> reply to Lina's driver.) > >> > >> However, seeing that the node name is used to populate both the > >> state->name and ->, made me wonder if there should better way to set the > >> state->desc field to a more useful string. Tools like powertop actually > >> use that field and it can be quite useful. > > > > Well, the truncation problem will be solved when those strings will be > > kdup'ed, so for the name I think there is not a problem, copying the > > state node is fine waiting for those strings to become pointers. > > > > For desc, there are four options: > > > > (1) enumerating idle states (but that's worse than copying the name into > > desc since on ARM idle-state{1,2,3...} means nothing) > > (2) copying the idle state node compatible string into desc > > (3) Add an optional property to the DT bindings to describe the state > > (4) Leave code as it is > > > > (3) I am not extremely keen at this stage to re-patch the DT bindings, > > it has been an awful lot of work to make everyone agree so I would avoid > > any changes, I hope you understand (and I am not even sure DT maintainers > > would accept that, so even less keen on changing the DT bindings at this > > stage). > > > > (2) I am not sure it will clarify the description much. > > > > (1) I would rule it out. So either we accept that the name can be > > extended in length (that's going to be the case since we will > > dynamically allocate the string so there will be no truncation, to > > a reasonable extent) so (4) is fine, or we merge this code and I > > will take care of pushing for (3) in a separate patch and copy the resulting > > description into desc (if that change does not get NACK'ed). > > > > I would really want to see this code in the mailine asap since it is > > groundwork for all future CPUidle generalisation, I hope that what I am > > saying above is acceptable, please let me know what you think. > > Agreed, as I stated when I rasied this issue, it's a very minor concern > and I don't think it should hold back this series. > > After this series is merged, I think approach (3) is probably the best > and should be done as a follow-up patch/series. I agree and that's what I will do, thank you Kevin (and Mark). Lorenzo From mboxrd@z Thu Jan 1 00:00:00 1970 From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) Date: Tue, 9 Sep 2014 16:51:50 +0100 Subject: [PATCH v9 5/8] drivers: cpuidle: implement DT based idle states infrastructure In-Reply-To: <7hoauqw1c2.fsf@paris.lan> References: <1409938498-17984-1-git-send-email-lorenzo.pieralisi@arm.com> <1409938498-17984-6-git-send-email-lorenzo.pieralisi@arm.com> <7hlhpxygzv.fsf@paris.lan> <20140905212908.GA28626@e102568-lin.cambridge.arm.com> <7hoauqw1c2.fsf@paris.lan> Message-ID: <20140909155150.GB4948@e102568-lin.cambridge.arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Sep 08, 2014 at 04:58:53PM +0100, Kevin Hilman wrote: > Lorenzo Pieralisi writes: > > > On Fri, Sep 05, 2014 at 09:00:52PM +0100, Kevin Hilman wrote: > >> Lorenzo Pieralisi writes: > >> > >> > On most common ARM systems, the low-power states a CPU can be put into are > >> > not discoverable in HW and require device tree bindings to describe > >> > power down suspend operations and idle states parameters. > >> > > >> > In order to enable DT based idle states and configure idle drivers, this > >> > patch implements the bulk infrastructure required to parse the device tree > >> > idle states bindings and initialize the corresponding CPUidle driver states > >> > data. > >> > > >> > The parsing API accepts a start index that defines the first idle state > >> > that should be initialized by the parsing code in order to give new and > >> > legacy driver flexibility over which states should be parsed using the > >> > new DT mechanism. > >> > > >> > The idle states node(s) is obtained from the phandle list of the first cpu > >> > in the driver cpumask; the kernel checks that the idle state node phandle > >> > is the same for all CPUs in the driver cpumask before declaring the idle state > >> > as valid and start parsing its content. > >> > > >> > The idle state enter function pointer is initialized through DT match > >> > structures passed in by the CPUidle driver, so that ARM legacy code can > >> > cope with platform specific idle entry method based on compatible > >> > string matching and the code used to initialize the enter function pointer > >> > can be moved to the DT generic layer. > >> > > >> > Acked-by: Catalin Marinas > >> > Acked-by: Daniel Lezcano > >> > Signed-off-by: Lorenzo Pieralisi > >> > >> [...] > >> > >> > + idle_state->flags = CPUIDLE_FLAG_TIME_VALID; > >> > + if (of_property_read_bool(state_node, "local-timer-stop")) > >> > + idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP; > >> > + /* > >> > + * TODO: > >> > + * replace with kstrdup and pointer assignment when name > >> > + * and desc become string pointers > >> > + */ > >> > + strncpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN - 1); > >> > + strncpy(idle_state->desc, state_node->name, CPUIDLE_DESC_LEN - 1); > >> > >> This is a very minor concern, and shouldn't hold back this series, > >> but... > >> > >> I was playing with this series in order to test out the qcom cpuidle > >> driver from Lina, and noticed that the state name and descriptions were > >> not terribly helpful: > >> > >> /sys/devices/system/cpu/cpu0/cpuidle # cat state?/name > >> cpu-idle-state- > >> cpu-idle-state- > >> /sys/devices/system/cpu/cpu0/cpuidle # cat state?/desc > >> cpu-idle-state-0 > >> cpu-idle-state-1 > >> > >> Turns out these strings come from the node name itself, and truncated in > >> the case of state->name, but this can be fixed in the DTS itself (c.f. > >> reply to Lina's driver.) > >> > >> However, seeing that the node name is used to populate both the > >> state->name and ->, made me wonder if there should better way to set the > >> state->desc field to a more useful string. Tools like powertop actually > >> use that field and it can be quite useful. > > > > Well, the truncation problem will be solved when those strings will be > > kdup'ed, so for the name I think there is not a problem, copying the > > state node is fine waiting for those strings to become pointers. > > > > For desc, there are four options: > > > > (1) enumerating idle states (but that's worse than copying the name into > > desc since on ARM idle-state{1,2,3...} means nothing) > > (2) copying the idle state node compatible string into desc > > (3) Add an optional property to the DT bindings to describe the state > > (4) Leave code as it is > > > > (3) I am not extremely keen at this stage to re-patch the DT bindings, > > it has been an awful lot of work to make everyone agree so I would avoid > > any changes, I hope you understand (and I am not even sure DT maintainers > > would accept that, so even less keen on changing the DT bindings at this > > stage). > > > > (2) I am not sure it will clarify the description much. > > > > (1) I would rule it out. So either we accept that the name can be > > extended in length (that's going to be the case since we will > > dynamically allocate the string so there will be no truncation, to > > a reasonable extent) so (4) is fine, or we merge this code and I > > will take care of pushing for (3) in a separate patch and copy the resulting > > description into desc (if that change does not get NACK'ed). > > > > I would really want to see this code in the mailine asap since it is > > groundwork for all future CPUidle generalisation, I hope that what I am > > saying above is acceptable, please let me know what you think. > > Agreed, as I stated when I rasied this issue, it's a very minor concern > and I don't think it should hold back this series. > > After this series is merged, I think approach (3) is probably the best > and should be done as a follow-up patch/series. I agree and that's what I will do, thank you Kevin (and Mark). Lorenzo