Re: [PATCH 08/20] coresight: dts: Cleanup device tree graph bindings

[Suzuki K Poulose]

On 11/06/18 17:52, Mathieu Poirier wrote:
> On 11 June 2018 at 03:22, Suzuki K Poulose <Suzuki.Poulose@xxxxxxx> wrote:
> > On 08/06/18 22:22, Mathieu Poirier wrote:
> > > On Tue, Jun 05, 2018 at 10:43:19PM +0100, Suzuki K Poulose wrote:
> > > > The coresight drivers relied on default bindings for graph
> > > > in DT, while reusing the "reg" field of the "ports" to indicate
> > > > the actual hardware port number for the connections. However,
> > > > with the rules getting stricter w.r.t to the address mismatch
> > > > with the label, it is no longer possible to use the port address
> > > > field for the hardware port number. Hence, we add an explicit
> > > > property to denote the hardware port number, "coresight,hwid"
> > > > which must be specified for each "endpoint".
> > > >
> > > > Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > > > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> > > > Cc: Rob Herring <robh@xxxxxxxxxx>
> > > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > > > ---
> > > >    .../devicetree/bindings/arm/coresight.txt          | 29 ++++++++++---
> > > >    drivers/hwtracing/coresight/of_coresight.c         | 49
> > > > +++++++++++++++++-----
> > > >    2 files changed, 62 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/arm/coresight.txt
> > > > b/Documentation/devicetree/bindings/arm/coresight.txt
> > > > index ed6b555..bf75ab3 100644
> > > > --- a/Documentation/devicetree/bindings/arm/coresight.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> > > > @@ -108,8 +108,13 @@ following properties to uniquely identify the
> > > > connection details.
> > > >          "slave-mode"
> >
> >
> >
> > > >          };
> > >
> > >
> > > For the binding part:
> > > Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

...

> > > > @@ -140,9 +166,6 @@ static int of_coresight_parse_endpoint(struct
> > > > device_node *ep,
> > > >                  rparent = of_graph_get_port_parent(rep);
> > > >                  if (!rparent)
> > > >                          break;
> > > > -               if (of_graph_parse_endpoint(rep, &rendpoint))
> > > > -                       break;
> > > > -
> > > >                  /* If the remote device is not available, defer probing
> > > > */
> > > >                  rdev = of_coresight_get_endpoint_device(rparent);
> > > >                  if (!rdev) {
> > > > @@ -150,9 +173,15 @@ static int of_coresight_parse_endpoint(struct
> > > > device_node *ep,
> > > >                          break;
> > > >                  }
> > > >    -             conn->outport = endpoint.port;
> > > > +               child_port = of_coresight_endpoint_get_port_id(rdev,
> > > > rep);
> > > > +               if (child_port < 0) {
> > > > +                       ret = 0;
> > >
> > >
> > > Why returning '0' on an error condition?  Same for 'local_port' above.
> >
> > If we are unable to parse a port, we can simply ignore the port and
> > continue, which
> > is what we have today with the existing code. I didn't change it and still
> > think
> > it is the best effort thing. We could spit a warning for such cases, if
> > really needed.
> > Also, the parsing code almost never fails at the moment. If it fails to find
> > "reg" field,
> > it is assumed to be '0'. Either way ignoring it seems harmless. That said I
> > am open
> > to suggestions.
>
> Looking at the original code I remember not mandating enpoints to be
> valid for debugging purposes.  That certainly helps when building up a
> device tree file but also has the side effect of silently overlooking
> specification problems.  Fortunately the revamping you did on that
> part of the code makes it very easy to change that, something I think
> we should take advantage of (it can only lead to positive scenarios
> where defective specifications get pointed out).
>
> That being said and because the original behaviour is just as
> permissive, you can leave as is.

Thanks. So can I assume the Reviewed-by applies for the code now ?

Suzuki