Re: [PATCH v5 06/14] coresight: cti: Add device tree support for v8 arch CTI

[Mike Leach <mike.leach@linaro.org>]

Hi Suzuki,

On Tue, 3 Dec 2019 at 11:28, Suzuki Kuruppassery Poulose
<suzuki.poulose@arm.com> wrote:
>
> On 03/12/2019 10:59, Mike Leach wrote:
> > Hi Suzuki,
> >
> > On Fri, 29 Nov 2019 at 11:33, Suzuki Kuruppassery Poulose
> > <suzuki.poulose@arm.com> wrote:
> >>
> >> On 19/11/2019 23:19, Mike Leach wrote:
> >>> The v8 architecture defines the relationship between a PE, its optional ETM
> >>> and a CTI. Unlike non-architectural CTIs which are implementation defined,
> >>> this has a fixed set of connections which can therefore be represented as a
> >>> simple tag in the device tree.
> >>>
> >>> This patch defines the tags needed to create an entry for this PE/ETM/CTI
> >>> relationship, and provides functionality to implement the connection model
> >>> in the CTI driver.
> >>>
> >>> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> >>> ---
>
>
> >>> +#ifdef CONFIG_OF
> >>> +/*
> >>> + * CTI can be bound to a CPU, or a system device.
> >>> + * CPU can be declared at the device top level or in a connections node
> >>> + * so need to check relative to node not device.
> >>> + */
> >>> +static int of_cti_get_cpu_at_node(const struct device_node *node)
> >>> +{
> >>> +     int cpu;
> >>> +     struct device_node *dn;
> >>> +
> >>> +     if (node == NULL)
> >>> +             return -1;
> >>> +
> >>> +     dn = of_parse_phandle(node, "cpu", 0);
> >>> +     /* CTI affinity defaults to no cpu */
> >>> +     if (!dn)
> >>> +             return -1;
> >>> +     cpu = of_cpu_node_to_id(dn);
> >>> +     of_node_put(dn);
> >>> +
> >>> +     /* No Affinity  if no cpu nodes are found */
> >>> +     return (cpu < 0) ? -1 : cpu;
> >>> +}
> >>> +
> >>> +static const char *of_cti_get_node_name(const struct device_node *node)
> >>> +{
> >>> +     if (node)
> >>> +             return node->full_name;
> >>> +     return "unknown";
> >>> +}
> >>> +#else
> >>> +static int of_cti_get_cpu_at_node(const struct device_node *node)
> >>> +{
> >>> +     return -1;
> >>> +}
> >>> +
> >>> +static const char *of_cti_get_node_name(const struct device_node *node)
> >>> +{
> >>> +     return "unknown";
> >>> +}
> >>> +#endif
> >>> +
> >>> +static int cti_plat_get_cpu_at_node(struct fwnode_handle *fwnode)
> >>> +{
> >>
> >> You may simply reuse coresight_get_cpu() below, instead of adding this
> >> duplicate set of functions. See below.
> >>
> >>
> >
> > No we can't. coresight_get_cpu gets the 'cpu' entry relative to the
> > device node, this gets the 'cpu' relative to the supplied node.
> > This is very important for the case where a none v8 architected PE is
> > attached to a CTI. This will use the devicetree form:-
> >
> > cti@<addr> {
> >      [ some stuff  ]
> >     trig_conns@1 {
> >            cpu = <&CPU0>
> >            [trigger signal  connection info for this cpu]
> >     }
> > }
> >
> > trig_conns is a child node and we must look for 'cpu' relative to it.
>
> Ok. May be we could refactor the function to find the 'CPU' node
> relative to the given "fwnode" and let the coresight_get_cpu() use it ?
>
> int coresight_get_cpu(struct device *dev)
> {
>         return coresight_get_fwnode_cpu(dev_fwnode(dev));
> }
>
> That way it is clear what we are dealing with. i.e, fwnode of any level
> (device or an intermediate node).
>

At present the generic coresight_get_cpu() deals with both DT and ACPI
bindings.
To refactor this would require re-factoring both binding types - and
at present we have no definition for ACPI bindings for CTI and hence
no way of knowing how the embedded cpu node is going to be
represented.

I think we have to take just the DT binding as is for now as a CTI
specific element and consider if it is worth re-factoring once the
ACPI bindings are defined

Regards

Mike

> >>> +     csdev = cti_get_assoc_csdev_by_fwnode(cs_fwnode);
> >>> +     if (csdev)
> >>> +             assoc_name = dev_name(&csdev->dev);
> >>
> >> Does it make sense to defer the probing until the ETM device  turn up ?
> >> Its fine either way.
> >>
> >
> > Not really as the ETM is optional but the PE still has a CTI.
>
> Ah, you're right. Please ignore my comment.
>
> Kind regards
> Suzuki



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel