On Mon, Sep 08, 2014 at 04:58:53PM +0100, Kevin Hilman wrote:
> 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.
I agree and that's what I will do, thank you Kevin (and Mark).
Lorenzo
--
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
