Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> writes:
> On Fri, Sep 05, 2014 at 09:00:52PM +0100, Kevin Hilman wrote:
>> Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> 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 <catalin.marinas@xxxxxxx>
>> > Acked-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>>
>> [...]
>>
>> > + 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.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
