Message-ID: <CACRpkdZapQ5UeOQfO4g+Vz6r2V2R5OJUrkm9eKbm6uBJZObb9w@mail.gmail.com>
Date:	Wed, 3 Sep 2014 10:07:07 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Will Deacon <will.deacon@....com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Greg KH <gregkh@...uxfoundation.org>,
	Arnd Bergmann <arnd@...aro.org>,
	John Stultz <john.stultz@...aro.org>,
	Pratik Patel <pratikp@...eaurora.org>,
	Vikas Varshney <varshney@...com>, Al Grant <Al.Grant@....com>,
	Jonas Svennebring <jonas.svennebring@...gotech.com>,
	James King <james.king@...aro.org>,
	Panchaxari Prasannamurthy Tumkur 
	<panchaxari.prasannamurthy@...aro.org>,
	Kaixu Xia <kaixu.xia@...aro.org>,
	Marcin Jabrzyk <marcin.jabrzyk@...il.com>,
	r.sengupta@...sung.com, Robert Marklund <robbelibobban@...il.com>,
	Tony.Armitstead@....com, Patch Tracking <patches@...aro.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 01/11 v5] coresight: add CoreSight core layer framework

On Wed, Aug 27, 2014 at 7:17 PM,  <mathieu.poirier@...aro.org> wrote:

> From: Pratik Patel <pratikp@...eaurora.org>
>
> CoreSight components are compliant with the ARM CoreSight
> architecture specification and can be connected in various
> topologies to suite a particular SoCs tracing needs. These trace
> components can generally be classified as sources, links and
> sinks. Trace data produced by one or more sources flows through
> the intermediate links connecting the source to the currently
> selected sink.
>
> CoreSight framework provides an interface for the CoreSight trace
> drivers to register themselves with. It's intended to build up a
> topological view of the CoreSight components and configure the
> right series of components on user input via debugfs.
>
> For eg., when enabling a source, framework builds up a path
> consisting of all the components connecting the source to the
> currently selected sink(s) and enables all of them.
>
> The framework also supports switching between available sinks
> and also provides status information to user space applications
> through the debugfs interface.
>
> Signed-off-by: Pratik Patel <pratikp@...eaurora.org>
> Signed-off-by: Panchaxari Prasannamurthy <panchaxari.prasannamurthy@...aro.org>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.org>
(...)

> +       pr_err("coresight: couldn't find inport, parent: %d, child: %d\n",
> +              parent->id, csdev->id);

Instead of repitetively prefixing all debug prints with "coresight:" like this,
at the top of the file do this:

#define pr_fmt(fmt) "coresight: " fmt

Then just pr_err(couldn't find...\n");

Try it, it's cool!

Same thing for the other files using such prints. NB: has to be
in each .c file, a shared .h may affect unwanted stuff.

> +static int coresight_enable_sink(struct coresight_device *csdev)
> +{
> +       int ret;
> +
> +       if (csdev->refcnt.sink_refcnt == 0) {
> +               if (sink_ops(csdev)->enable) {
> +                       ret = sink_ops(csdev)->enable(csdev);
> +                       if (ret)
> +                               goto err;

Convoluted, just

  return err;

here.

> +                       csdev->enable = true;
> +               }
> +       }
> +       csdev->refcnt.sink_refcnt++;

There is quite a lot of refcounting code in this file.

Why can't <linux/kref.h> be used instead of inventing a custom
reference counter?

I think it could cut down the code a bit and make it even cleaner.

> +
> +       return 0;
> +err:
> +       return ret;
> +}

And skip that exit goto label.

> +static void coresight_disable_sink(struct coresight_device *csdev)
> +{
> +       if (csdev->refcnt.sink_refcnt == 1) {
> +               if (sink_ops(csdev)->disable) {
> +                       sink_ops(csdev)->disable(csdev);
> +                       csdev->enable = false;
> +               }
> +       }
> +       csdev->refcnt.sink_refcnt--;
> +}

So with kref you would just kref_put(kref, release) it and this
cleanup function would get called from the release function
when the ref goes to zero. Which is what you want.

> +static int coresight_enable_link(struct coresight_device *csdev)
> +{
> +       int ret;
> +       int link_subtype;
> +       int refport, inport, outport;
> +
> +       inport = coresight_find_link_inport(csdev);
> +       outport = coresight_find_link_outport(csdev);
> +
> +       link_subtype = csdev->subtype.link_subtype;
> +       if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
> +               refport = inport;
> +       else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT)
> +               refport = outport;
> +       else
> +               refport = 0;
> +
> +       if (csdev->refcnt.link_refcnts[refport] == 0) {
> +               if (link_ops(csdev)->enable) {
> +                       ret = link_ops(csdev)->enable(csdev, inport, outport);
> +                       if (ret)
> +                               goto err;

Just return err;

> +                       csdev->enable = true;
> +               }
> +       }

More refcounting, see.

> +       csdev->refcnt.link_refcnts[refport]++;
> +
> +       return 0;
> +err:
> +       return ret;
> +}

Get rid of err label.

(...)
> +static void coresight_disable_source(struct coresight_device *csdev)
> +{
> +       if (csdev->refcnt.source_refcnt == 1) {
> +               if (source_ops(csdev)->disable) {
> +                       source_ops(csdev)->disable(csdev);
> +                       csdev->enable = false;
> +               }
> +       }
> +       csdev->refcnt.source_refcnt--;
> +}

Again, kref is your friend.

> +static int coresight_build_paths(struct coresight_device *csdev,
> +                                struct list_head *path,
> +                                bool enable)
> +{
> +       int i, ret = -1;

-1 is not a proper error code.

> +       struct coresight_connection *conn;
> +
> +       list_add(&csdev->path_link, path);
> +
> +       if (csdev->type == CORESIGHT_DEV_TYPE_SINK && csdev->activated) {
> +               if (enable)
> +                       ret = coresight_enable_path(path);
> +               else
> +                       ret = coresight_disable_path(path);
> +       } else {
> +               for (i = 0; i < csdev->nr_conns; i++) {
> +                       conn = &csdev->conns[i];
> +                       if (coresight_build_paths(conn->child_dev,
> +                                                   path, enable) == 0)
> +                               ret = 0;
> +               }
> +       }
> +
> +       if (list_first_entry(path, struct coresight_device, path_link) != csdev)
> +               pr_err("coresight: wrong device in %s\n", __func__);
> +
> +       list_del(&csdev->path_link);
> +       return ret;
> +}
(...)

> +static ssize_t debugfs_active_get(void *data, u64 *val)
> +{
> +       struct coresight_device *csdev = data;
> +
> +       *val = csdev->activated;
> +       return 0;
> +}
> +
> +static ssize_t debugfs_active_set(void *data, u64 val)
> +{
> +       struct coresight_device *csdev = data;
> +
> +       val ? (csdev->activated = 1) : (csdev->activated = 0);

It looks vert much like csdev->activated is a bool and this should
assign true or false.

Using the ? operator like that is a bit stressful for me, why not
just use this funky boolean-clamp idiom:

csdev->activated = !!val;

(...)
> +static int debugfs_coresight_init(void)
> +{
> +       if (!cs_debugfs_parent) {
> +               cs_debugfs_parent = debugfs_create_dir("coresight", 0);
> +               if (IS_ERR(cs_debugfs_parent))
> +                       return -1;

return PTR_ERR(cs_debugfs_parent);

> +       }
> +
> +       return 0;
> +}

(...)
> +/*
> + * return 1 if the bit @position in @val is set to @value
> + */
> +int coresight_is_bit_set(u32 val, int position, int value)
> +{
> +       val &= BIT(position);
> +       val >>= position;
> +       return (val == value);
> +}

This is just too overengineered. The value can only be zero or one!
Can't you just inline and please replace *all* uses of this function from
like:

if (coresight_is_bit_set(foo, bar, baz)) {
   ...
}

to

if (foo & BIT(bar)) {
   ...
}

> +void coresight_timeout(void __iomem *addr, u32 offset, int position, int value)
> +{
> +       int i;
> +       u32 val;
> +
> +       for (i = TIMEOUT_US; i > 0; i--) {
> +               val = __raw_readl(addr + offset);
> +               if (coresight_is_bit_set(val, position, value))
> +                       return;
> +               udelay(1);

Why 1 us? Add a comment. Does this vary with silicon?

> +       }
> +
> +       WARN(1,
> +            "coresight: timeout observed when proving at offset %#x\n",
> +            offset);
> +}

(...)
> diff --git a/drivers/coresight/of_coresight.c b/drivers/coresight/of_coresight.c
(...)
> new file mode 100644
> index 0000000..f90a024
> --- /dev/null
> +++ b/drivers/coresight/of_coresight.c
> @@ -0,0 +1,202 @@
> +/* Copyright (c) 2012, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_graph.h>
> +#include <linux/coresight.h>
> +#include <asm/smp_plat.h>
> +
> +static int of_get_coresight_id(struct device_node *node, int *id)
> +{
> +       const __be32 *reg;
> +       u64 addr;
> +
> +       /* derive component id from its memory map */
> +       reg = of_get_property(node, "reg", NULL);
> +       if (reg) {
> +               addr = of_translate_address(node, reg);
> +               if (addr != OF_BAD_ADDR) {
> +                       *id = addr;
> +                       return 0;
> +               }
> +       }
> +
> +       /* no "reg", we have a non-configurable replicator */
> +       reg = of_get_property(node, "id", NULL);
> +       if (reg) {
> +               *id = of_read_ulong(reg, 1);
> +               return 0;
> +       }
> +
> +       return -1;

return -EINVAL?

(...)
> +static bool of_coresight_is_input_port(struct device_node *port)
> +{
> +       return of_find_property(port, "slave-mode", NULL);
> +}

Can't you just replace this at the place where it's used with:

if (of_coresight_is_input_port(np)) ...

Instead:

if (of_property_read_bool(np, "slave-mode")) ...

And skip this extra function.

If you absolutely want that helper function name make
the preprocessor inline it:

#define of_coresight_is_input_port(a) of_property_read_bool(a, "slave-mode")

> +static void of_coresight_get_ports(struct device_node *node,
> +                                  int *nr_inports, int *nr_outports)
> +{
> +       struct device_node *ep = NULL;
> +       int in = 0, out = 0;
> +
> +       do {
> +               ep = of_get_coresight_endpoint(node, ep);
> +               if (!ep)
> +                       break;
> +               of_coresight_is_input_port(ep) ? in++ : out++;

Aha there is one such construction again. Well I'd just

if (of_property_read_bool(ep, "slave-mode"))
   in++
else
  out++;

But I guess you maybe really like the ? operator so no
big deal.

(...)
> +struct coresight_platform_data *of_get_coresight_platform_data(
> +                               struct device *dev, struct device_node *node)
> +{
> +       int id, i = 0, ret = 0;
> +       struct device_node *cpu;
> +       struct coresight_platform_data *pdata;
> +       struct of_endpoint endpoint, rendpoint;
> +       struct device_node *ep = NULL;
> +       struct device_node *rparent = NULL;
> +       struct device_node *rport = NULL;
> +
> +       pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +       if (!pdata)
> +               return ERR_PTR(-ENOMEM);
> +
> +       /* use the base address as id */
> +       ret = of_get_coresight_id(node, &id);
> +       if (ret)
> +               return ERR_PTR(-EINVAL);

Just propagate the error code:

return ERR_PTR(ret);

> +       pdata->id = id;
> +
> +       /* use device name as debugfs handle */
> +       pdata->name = dev_name(dev);
> +
> +       /* get the number of input and output port for this component */
> +       of_coresight_get_ports(node, &pdata->nr_inports, &pdata->nr_outports);
> +
> +       if (pdata->nr_outports) {
> +               ret = of_coresight_alloc_memory(dev, pdata);
> +               if (ret)
> +                       return ERR_PTR(-ENOMEM);

return ERR_PTR(ret);

(...)
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> @@ -23,6 +23,7 @@
>
>  #define AMBA_NR_IRQS   9
>  #define AMBA_CID       0xb105f00d
> +#define CORESIGHT_CID  0xb105900d

These guys really have a sense of humour.

> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
(...)

> +struct coresight_platform_data {
> +       int id;
> +       int cpu;
> +       const char *name;
> +       int nr_inports;
> +       int *outports;
> +       int *child_ids;
> +       int *child_ports;
> +       int nr_outports;
> +       struct clk *clk;
> +};

This struct could maybe need some kerneldoc. Not all members
are self-evident.

(...)
> +struct coresight_refcnt {
> +       int sink_refcnt;
> +       int *link_refcnts;
> +       int source_refcnt;
> +};

So I suggest replacing this with kref.

> +struct coresight_device {
> +       int id;
> +       struct coresight_connection *conns;
> +       int nr_conns;
> +       enum coresight_dev_type type;
> +       struct coresight_dev_subtype subtype;
> +       const struct coresight_ops *ops;
> +       struct dentry *de;
> +       struct device dev;
> +       struct coresight_refcnt refcnt;
> +       struct list_head dev_link;
> +       struct list_head path_link;
> +       struct module *owner;
> +       bool enable;    /* true only if configured as part of a path */
> +       bool activated; /* only valid for sinks */
> +};

Hey it is bool ... yet the code assigns 0/1 instead of false/true
in most places.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists