Re: [PATCH 1/9 v2] coresight: add CoreSight core layer framework

[Mathieu Poirier]

snip...

>> +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);
>
> Does for_each_child_of_node not work here?

Using for_each_child_of_node yields the first child of that node,
which may or may not be "ports" or "port".  By using
of_get_coresight_endpoint we let of_graph_get_next_endpoint to deal
with that hierarchy.  Moreover the latter will also deal with the
hierarchy of port under ports, something that would need to be
duplicated if for_each_child_of_node was used.

Get back to me if you disagree.

>
>> +               if (!ep)
>> +                       break;
>> +               of_coresight_is_input_port(ep) ? in++ : out++;
>> +
>> +       } while (ep);
>> +
>> +       *nr_inports = in;
>> +       *nr_outports = out;
>> +}
>> +
>> +static int of_coresight_alloc_memory(struct device *dev,
>> +                       struct coresight_platform_data *pdata)
>> +{
>> +       /* list of output port on this component */
>> +       pdata->outports = devm_kzalloc(dev, pdata->nr_outports *
>> +                                      sizeof(*pdata->outports),
>> +                                      GFP_KERNEL);
>> +       if (!pdata->outports)
>> +               return -ENOMEM;
>> +
>> +
>> +       /* children connected to this component via @outport */
>> +       pdata->child_ids = devm_kzalloc(dev, pdata->nr_outports *
>> +                                       sizeof(*pdata->child_ids),
>> +                                       GFP_KERNEL);
>> +       if (!pdata->child_ids)
>> +               return -ENOMEM;
>> +
>> +       /* port number on the child this component is connected to */
>> +       pdata->child_ports = devm_kzalloc(dev, pdata->nr_outports *
>> +                                         sizeof(*pdata->child_ports),
>> +                                         GFP_KERNEL);
>> +       if (!pdata->child_ports)
>> +               return -ENOMEM;
>> +
>> +       return 0;
>> +}
>> +
>> +struct coresight_platform_data *of_get_coresight_platform_data(
>> +                               struct device *dev, struct device_node *node)
>> +{
>> +       u32 id;
>> +       int 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 */
>> +       id = of_get_coresight_id(node);
>> +       if (id == 0)
>> +               return  ERR_PTR(-EINVAL);
>> +
>> +       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);
>> +
>> +               /* iterate through each port to discover topology */
>> +               do {
>> +                       /* get a handle on a port */
>> +                       ep = of_get_coresight_endpoint(node, ep);
>> +                       if (!ep)
>> +                               break;
>
> for_each_child_of_node

Same as above.

>
>> +
>> +                       /* no need to deal with input ports, processing for as
>> +                        * processing for output ports will deal with them.
>> +                        */
>> +                       if (of_coresight_is_input_port(ep))
>> +                               continue;
>> +
>> +                       /* get a handle on the local endpoint */
>> +                       ret = of_graph_parse_endpoint(ep, &endpoint);
>> +
>> +                       if (!ret) {
>
> You can save some indentation with:
>
> if (ret)
>  continue;
>
>> +                               /* the local out port number */
>> +                               *(u32 *)&pdata->outports[i] = endpoint.id;
>
> Can't you avoid this casting?
>
>> +
>> +                               /* get a handle the remote port and parent
>> +                                * attached to it.
>> +                                */
>> +                               rparent = of_graph_get_remote_port_parent(ep);
>> +                               rport = of_graph_get_remote_port(ep);
>> +
>> +                               if (!rparent || !rport)
>> +                                       continue;
>> +
>> +                               if (of_graph_parse_endpoint(rport,
>> +                                                            &rendpoint))
>> +                                       continue;
>> +
>> +                               *(u32 *)&pdata->child_ids[i] =
>> +                                       of_get_coresight_id(rparent);
>> +                               *(u32 *)&pdata->child_ports[i] = rendpoint.id;
>
> and these?
>
>> +
>> +                               i++;
>> +                       }
>> +
>> +               } while (ep);
>> +       }
>> +
>> +       pdata->default_sink = of_property_read_bool(node,
>> +                                                   "coresight-default-sink");
>> +
>> +       /* affinity defaults to CPU0 */
>> +       pdata->cpu = 0;
>> +       cpu = of_parse_phandle(node, "cpu", 0);
>> +       if (cpu) {
>> +               const u32 *mpidr;
>> +               int len, index;
>> +
>> +               mpidr = of_get_property(cpu, "reg", &len);
>> +               if (mpidr && len == 4) {
>> +                       index = get_logical_index(be32_to_cpup(mpidr));
>
> Don't we have some helper for translating a cpu phandle to logical index?

I haven't found one - the same trickery can be found in [1] [2]. Even
the very recent 64 bit implementation doesn't use a helper function
[3].  The opposite direction i.e, going from a logical CPU to a
device_node, has a helper function.  If you have something on the top
of your mind I'd like to know.

[1]. arch/arm/kernel/devtree.c, (arm_dt_init_cpu_maps)
[2]. arch/powerpc/kernel/smp.c, (cpu_to_corei_id)
[3]. arch/arm64/kernel/smp.c, (smp_init_cpus)

>
>> +                       if (index != -EINVAL)
>> +                               pdata->cpu = index;
>> +               }
>> +       }
>> +
>> +       return pdata;
>> +}
>> +EXPORT_SYMBOL_GPL(of_get_coresight_platform_data);
>> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
>> index fdd7e1b..cdddabe 100644
>> --- 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
>>
>>  struct clk;
>>
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> new file mode 100644
>> index 0000000..a19420e
>> --- /dev/null
>> +++ b/include/linux/coresight.h
>> @@ -0,0 +1,190 @@
>> +/* 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.
>> + */
>> +
>> +#ifndef _LINUX_CORESIGHT_H
>> +#define _LINUX_CORESIGHT_H
>> +
>> +#include <linux/device.h>
>> +
>> +/* Peripheral id registers (0xFD0-0xFEC) */
>> +#define CORESIGHT_PERIPHIDR4   (0xFD0)
>> +#define CORESIGHT_PERIPHIDR5   (0xFD4)
>> +#define CORESIGHT_PERIPHIDR6   (0xFD8)
>> +#define CORESIGHT_PERIPHIDR7   (0xFDC)
>> +#define CORESIGHT_PERIPHIDR0   (0xFE0)
>> +#define CORESIGHT_PERIPHIDR1   (0xFE4)
>> +#define CORESIGHT_PERIPHIDR2   (0xFE8)
>> +#define CORESIGHT_PERIPHIDR3   (0xFEC)
>> +/* Component id registers (0xFF0-0xFFC) */
>> +#define CORESIGHT_COMPIDR0     (0xFF0)
>> +#define CORESIGHT_COMPIDR1     (0xFF4)
>> +#define CORESIGHT_COMPIDR2     (0xFF8)
>> +#define CORESIGHT_COMPIDR3     (0xFFC)
>> +
>> +#define ETM_ARCH_V3_3          (0x23)
>> +#define ETM_ARCH_V3_5          (0x25)
>> +#define PFT_ARCH_V1_1          (0x31)
>> +
>> +#define CORESIGHT_UNLOCK       (0xC5ACCE55)
>
> Parentheses are not necessary.
>
>> +enum coresight_clk_rate {
>> +       CORESIGHT_CLK_RATE_OFF,
>> +       CORESIGHT_CLK_RATE_TRACE,
>> +       CORESIGHT_CLK_RATE_HSTRACE,
>> +};
>> +
>> +enum coresight_dev_type {
>> +       CORESIGHT_DEV_TYPE_NONE,
>> +       CORESIGHT_DEV_TYPE_SINK,
>> +       CORESIGHT_DEV_TYPE_LINK,
>> +       CORESIGHT_DEV_TYPE_LINKSINK,
>> +       CORESIGHT_DEV_TYPE_SOURCE,
>> +};
>> +
>> +enum coresight_dev_subtype_sink {
>> +       CORESIGHT_DEV_SUBTYPE_SINK_NONE,
>> +       CORESIGHT_DEV_SUBTYPE_SINK_PORT,
>> +       CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
>> +};
>> +
>> +enum coresight_dev_subtype_link {
>> +       CORESIGHT_DEV_SUBTYPE_LINK_NONE,
>> +       CORESIGHT_DEV_SUBTYPE_LINK_MERG,
>> +       CORESIGHT_DEV_SUBTYPE_LINK_SPLIT,
>> +       CORESIGHT_DEV_SUBTYPE_LINK_FIFO,
>> +};
>> +
>> +enum coresight_dev_subtype_source {
>> +       CORESIGHT_DEV_SUBTYPE_SOURCE_NONE,
>> +       CORESIGHT_DEV_SUBTYPE_SOURCE_PROC,
>> +       CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
>> +       CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
>> +};
>> +
>> +struct coresight_ops_entry {
>> +       const char *name;
>> +       umode_t mode;
>> +       const struct file_operations *ops;
>> +};
>> +
>> +struct coresight_dev_subtype {
>> +       enum coresight_dev_subtype_sink sink_subtype;
>> +       enum coresight_dev_subtype_link link_subtype;
>> +       enum coresight_dev_subtype_source source_subtype;
>> +};
>> +
>> +struct coresight_platform_data {
>> +       int id;
>> +       int cpu;
>> +       const char *name;
>> +       int nr_inports;
>> +       const int *outports;
>> +       const int *child_ids;
>> +       const int *child_ports;
>> +       int nr_outports;
>> +       bool default_sink;
>> +       struct clk *clk;
>> +};
>> +
>> +struct coresight_desc {
>> +       enum coresight_dev_type type;
>> +       struct coresight_dev_subtype subtype;
>> +       const struct coresight_ops *ops;
>> +       struct coresight_platform_data *pdata;
>> +       struct device *dev;
>> +       const struct coresight_ops_entry **debugfs_ops;
>> +       struct module *owner;
>> +};
>> +
>> +struct coresight_connection {
>> +       int outport;
>> +       int child_id;
>> +       int child_port;
>> +       struct coresight_device *child_dev;
>> +       struct list_head link;
>> +};
>> +
>> +struct coresight_refcnt {
>> +       int sink_refcnt;
>> +       int *link_refcnts;
>> +       int source_refcnt;
>> +};
>> +
>> +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;
>> +};
>> +
>> +#define to_coresight_device(d) container_of(d, struct coresight_device, dev)
>> +
>> +#define CORESIGHT_DEBUGFS_ENTRY(__name, __entry_name,                  \
>> +                                __mode, __get, __set, __fmt)           \
>> +DEFINE_SIMPLE_ATTRIBUTE(__name ## _ops, __get, __set, __fmt)           \
>> +static const struct coresight_ops_entry __name ## _entry = {           \
>> +       .name = __entry_name,                                           \
>> +       .mode = __mode,                                                 \
>> +       .ops  = &__name ## _ops                                         \
>> +}
>> +
>> +struct coresight_ops_sink {
>> +       int (*enable)(struct coresight_device *csdev);
>> +       void (*disable)(struct coresight_device *csdev);
>> +       void (*abort)(struct coresight_device *csdev);
>> +};
>> +
>> +struct coresight_ops_link {
>> +       int (*enable)(struct coresight_device *csdev, int iport, int oport);
>> +       void (*disable)(struct coresight_device *csdev, int iport, int oport);
>> +};
>> +
>> +struct coresight_ops_source {
>> +       int (*enable)(struct coresight_device *csdev);
>> +       void (*disable)(struct coresight_device *csdev);
>> +};
>> +
>> +struct coresight_ops {
>> +       const struct coresight_ops_sink *sink_ops;
>> +       const struct coresight_ops_link *link_ops;
>> +       const struct coresight_ops_source *sourcearch/arm64/kernel/smp.c_ops;
>> +};
>> +
>> +#ifdef CONFIG_CORESIGHT
>> +extern struct coresight_device *
>> +coresight_register(struct coresight_desc *desc);
>> +extern void coresight_unregister(struct coresight_device *csdev);
>> +extern int coresight_enable(struct coresight_device *csdev);
>> +extern void coresight_disable(struct coresight_device *csdev);
>> +extern void coresight_abort(void);
>> +extern struct clk *coresight_get_clk(void);
>> +#else
>> +static inline struct coresight_device *
>> +coresight_register(struct coresight_desc *desc) { return NULL; }
>> +static inline void coresight_unregister(struct coresight_device *csdev) {}
>> +static inline int
>> +coresight_enable(struct coresight_device *csdev) { return -ENOSYS; }
>> +static inline void coresight_disable(struct coresight_device *csdev) {}
>> +static inline void coresight_abort(void) {}
>> +extern struct clk *coresight_get_clk(void) {};
>> +#endif
>> +
>> +#endif
>> diff --git a/include/linux/of_coresight.h b/include/linux/of_coresight.h
>> new file mode 100644
>> index 0000000..6a5e4d4
>> --- /dev/null
>> +++ b/include/linux/of_coresight.h
>
> I would just put this into coresight.h.
>
>
>> @@ -0,0 +1,27 @@
>> +/* 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.
>> + */
>> +
>> +#ifndef __LINUX_OF_CORESIGHT_H
>> +#define __LINUX_OF_CORESIGHT_H
>> +
>> +#ifdef CONFIG_OF
>> +extern struct coresight_platform_data *of_get_coresight_platform_data(
>> +                               struct device *dev, struct device_node *node);
>> +#else
>> +static inline struct coresight_platform_data *of_get_coresight_platform_data(
>> +                               struct device *dev, struct device_node *node)
>> +{
>> +       return NULL;
>> +}
>> +#endif
>> +
>> +#endif
>> --
>> 1.9.1
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/