Hi Philipp,
On Thu, Oct 4, 2018 at 12:15 PM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
> Thank you for the patch. I'd still like to hear the device tree
> maintainers' (added to Cc:) opinion on parsing the whole DT for "resets"
> phandle properties to find shared resets like this.
>
> On Thu, 2018-09-27 at 20:00 +0200, Geert Uytterhoeven wrote:
> > In some SoCs multiple hardware blocks may share a reset control.
> > The reset control API for shared resets will only assert such a reset
> > when the drivers for all hardware blocks agree.
> > The exclusive reset control API still allows to assert such a reset, but
> > that impacts all other hardware blocks sharing the reset.
> >
> > While the kernel doc comments clearly state that the API for shared
> > resets applies to reset controls which are shared between hardware
> > blocks, the exact meaning of exclusive resets is not documented.
> > Fix the semantic ambiguity with respect to exclusive access vs.
> > exclusive reset lines by:
> > 1. Clarifying that exclusive resets really are intended for use with
> > reset controls which are dedicated to a single hardware block,
> > 2. Ensuring that obtaining an exclusive reset control will fail if the
> > reset is shared by multiple hardware blocks, for both DT-based and
> > lookup-based reset controls.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > ---
> > This is v2 of "[RFC] reset: Add support for dedicated reset controls":
> > - Fix wrong variable in __reset_is_dedicated() loop,
> > - Add missing of_node_put() in __of_reset_is_dedicated(),
> > - Document that exclusive reset controls imply they are dedicated to a
> > single hardware block,
> > - Drop new dedicated flag and new API reset_control_get_dedicated(),
> > as exclusive already implies dedicated,
> > - Rename {__of_,}reset_is_dedicated() to {__of_,}reset_is_exclusive(),
> > - Reword description.
> >
> > Note: Exclusive lookup-based reset controls were not tested.
> > ---
> > drivers/reset/core.c | 58 +++++++++++++++++++++++++++++++++++++++++++
> > include/linux/reset.h | 5 +++-
> > 2 files changed, 62 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 225e34c56b94a2e3..2f5b61226c7964eb 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -459,6 +459,38 @@ static void __reset_control_put_internal(struct reset_control *rstc)
> > kref_put(&rstc->refcnt, __reset_control_release);
> > }
> >
> > +static bool __of_reset_is_exclusive(const struct device_node *node,
> > + const struct of_phandle_args args)
Oops, this should take *args, not args.
> > +{
> > + struct of_phandle_args args2;
> > + struct device_node *node2;
> > + int index, ret;
> > + bool eq;
>
> I suppose it is very unlikely to get false positives where an arbitrary
> node contains a "resets" property that looks like a proper phandle to an
> actual reset-controller node.
> Are we allowed though to scan the whole tree for "resets" properties
> regardless of the nodes' bindings or compatible properties like this?
Given "resets" is a more-or-less standard property, I'd say yes.
Especially given of_parse_phandle_with_args() does verify that the target
node has #reset-cells, and that the number of parameters matches that.
> > + for_each_node_with_property(node2, "resets") {
> > + if (node == node2)
> > + continue;
> > +
> > + for (index = 0; ; index++) {
> > + ret = of_parse_phandle_with_args(node2, "resets",
> > + "#reset-cells", index,
> > + &args2);
> > + if (ret)
> > + break;
> > +
> > + eq = (args2.np == args.np &&
> > + args2.args_count == args.args_count &&
> > + !memcmp(args2.args, args.args,
> > + args.args_count * sizeof(args.args[0])));
As there's at least one other function in -next that compares of_phandle_args,
I will add a helper of_phandle_args_eq().
> > + of_node_put(args2.np);
> > + if (eq)
>
> Emitting a loud warning here could be very helpful if it contains
> both the reset controller node and the reset index, as well as the
> consumer nodes: node and node2.
Indeed, will do, also for lookup resets.
We already have of_print_phandle_args(), but that is a bit inflexible.
Adding support for "%pOFa" looks like the modern thing to do.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
