[PATCH v8 16/24] coresight: cti: add function to register cti associate ops
Mathieu Poirier
mathieu.poirier at linaro.org
Tue Aug 18 13:16:01 EDT 2020
Good day,
On Mon, Aug 17, 2020 at 07:49:16PM +0100, Mike Leach wrote:
> Hi Mathieu,
>
> On Mon, 17 Aug 2020 at 17:02, Mathieu Poirier
> <mathieu.poirier at linaro.org> wrote:
> >
> > On Fri, Aug 07, 2020 at 07:11:45PM +0800, Tingwei Zhang wrote:
> > > Add static cti_assoc_ops to coresight core driver. Let cti
> > > driver register the add_assoc and remove_assoc call back.
> > > Avoid coresight core driver to depend on cti driver.
> > >
> > > Signed-off-by: Tingwei Zhang <tingwei at codeaurora.org>
> > > Tested-by: Mike Leach <mike.leach at linaro.org>
> > > ---
> > > drivers/hwtracing/coresight/coresight-cti.c | 32 ++++++++++++++++++--
> > > drivers/hwtracing/coresight/coresight-priv.h | 14 ++++-----
> > > drivers/hwtracing/coresight/coresight.c | 21 +++++++++++--
> > > 3 files changed, 53 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
> > > index 3ccc703dc940..1f470c47ba70 100644
> > > --- a/drivers/hwtracing/coresight/coresight-cti.c
> > > +++ b/drivers/hwtracing/coresight/coresight-cti.c
> > > @@ -589,7 +589,6 @@ void cti_add_assoc_to_csdev(struct coresight_device *csdev)
> > > cti_add_done:
> > > mutex_unlock(&ect_mutex);
> > > }
> > > -EXPORT_SYMBOL_GPL(cti_add_assoc_to_csdev);
> > >
> > > /*
> > > * Removing the associated devices is easier.
> > > @@ -616,7 +615,15 @@ void cti_remove_assoc_from_csdev(struct coresight_device *csdev)
> > > }
> > > mutex_unlock(&ect_mutex);
> > > }
> > > -EXPORT_SYMBOL_GPL(cti_remove_assoc_from_csdev);
> >
> > Mike and Tingwei,
> >
> > It seems to me that functions cti_add_assoc_to_csdev() and
> > cti_remote_assoc_from_csdev() don't have to be exported. They could be called
> > in cti_probe() and cti_device_release() with the ect_mutex held, the same way
> > cti_update_conn_xrefs() and cti_remove_conn_xrefs() are.
> >
>
> The different functions handle two different initialisation & two
> different teardown ordering scenarios
> a) csdev - e.g. ETM is discovered & initialised before the associated
> CTI. Therefore the CTI has to establish interconnections on creation -
> calls cti_update_conn_xrefs() .
> b) CTI is discovered before csdev - therefore the csdev calls
> cti_add_assoc_to_csdev().
> c) csdev is torn down before CTI - therefore csdev calls
> cti_remove_assoc_from_csdev()
> d) CTI is torn down before csdev - therefore calls cti_remove_conn_xrefs()
>
> We can't call cti_add_assoc_to_csdev() in cti probe as the csdev does
> not exist yet. Bear in mind that one CTI can actually be associated
> with multiple different csdev devices - especially in the none CPU
> bound infrastructure such as funnels and sinks.
Yes, I see your point now. The problem needs to be viewed from other devices'
(than CTI) perspective. And that part is done in
coresight_register/unregister().
Reviewed-by: Mathieu Poirier <mathieu.poirier at linaro.org>
> This has always been the case since the first CTI set - but is even
> more important now the modules can be removed / added as required.
>
> Regards
>
> Mike
>
>
> > If my assessment is correct this patch would be simpler.
> >
> > Thanks,
> > Mathieu
> >
> >
> > > +
> > > +/*
> > > + * Operations to add and remove associated CTI.
> > > + * Register to coresight core driver as call back function.
> > > + */
> > > +static struct cti_assoc_op cti_assoc_ops = {
> > > + .add = cti_add_assoc_to_csdev,
> > > + .remove = cti_remove_assoc_from_csdev
> > > +};
> > >
> > > /*
> > > * Update the cross references where the associated device was found
> > > @@ -972,4 +979,23 @@ static struct amba_driver cti_driver = {
> > > .probe = cti_probe,
> > > .id_table = cti_ids,
> > > };
> > > -builtin_amba_driver(cti_driver);
> > > +
> > > +static int __init cti_init(void)
> > > +{
> > > + int ret;
> > > +
> > > + ret = amba_driver_register(&cti_driver);
> > > + if (ret)
> > > + pr_info("Error registering cti driver\n");
> > > + coresight_set_cti_ops(&cti_assoc_ops);
> > > + return ret;
> > > +}
> > > +
> > > +static void __exit cti_exit(void)
> > > +{
> > > + coresight_remove_cti_ops();
> > > + amba_driver_unregister(&cti_driver);
> > > +}
> > > +
> > > +module_init(cti_init);
> > > +module_exit(cti_exit);
> > > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > > index dcb8aeb6af62..6cde6cf42554 100644
> > > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > > @@ -173,15 +173,13 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
> > > static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
> > > #endif
> > >
> > > -#ifdef CONFIG_CORESIGHT_CTI
> > > -extern void cti_add_assoc_to_csdev(struct coresight_device *csdev);
> > > -extern void cti_remove_assoc_from_csdev(struct coresight_device *csdev);
> > > +struct cti_assoc_op {
> > > + void (*add)(struct coresight_device *csdev);
> > > + void (*remove)(struct coresight_device *csdev);
> > > +};
> > >
> > > -#else
> > > -static inline void cti_add_assoc_to_csdev(struct coresight_device *csdev) {}
> > > -static inline void
> > > -cti_remove_assoc_from_csdev(struct coresight_device *csdev) {}
> > > -#endif
> > > +extern void coresight_set_cti_ops(const struct cti_assoc_op *cti_op);
> > > +extern void coresight_remove_cti_ops(void);
> > >
> > > /*
> > > * Macros and inline functions to handle CoreSight UCI data and driver
> > > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > > index 0b0e31577b9b..31b9ec8d3b9c 100644
> > > --- a/drivers/hwtracing/coresight/coresight.c
> > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > @@ -57,6 +57,20 @@ const u32 coresight_barrier_pkt[4] = {
> > > 0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff};
> > > EXPORT_SYMBOL_GPL(coresight_barrier_pkt);
> > >
> > > +static const struct cti_assoc_op *cti_assoc_ops;
> > > +
> > > +void coresight_set_cti_ops(const struct cti_assoc_op *cti_op)
> > > +{
> > > + cti_assoc_ops = cti_op;
> > > +}
> > > +EXPORT_SYMBOL_GPL(coresight_set_cti_ops);
> > > +
> > > +void coresight_remove_cti_ops(void)
> > > +{
> > > + cti_assoc_ops = NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(coresight_remove_cti_ops);
> > > +
> > > static int coresight_id_match(struct device *dev, void *data)
> > > {
> > > int trace_id, i_trace_id;
> > > @@ -1242,7 +1256,8 @@ static void coresight_device_release(struct device *dev)
> > > {
> > > struct coresight_device *csdev = to_coresight_device(dev);
> > >
> > > - cti_remove_assoc_from_csdev(csdev);
> > > + if (cti_assoc_ops && cti_assoc_ops->remove)
> > > + cti_assoc_ops->remove(csdev);
> > > fwnode_handle_put(csdev->dev.fwnode);
> > > kfree(csdev->refcnt);
> > > kfree(csdev);
> > > @@ -1553,8 +1568,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
> > > ret = coresight_fixup_device_conns(csdev);
> > > if (!ret)
> > > ret = coresight_fixup_orphan_conns(csdev);
> > > - if (!ret)
> > > - cti_add_assoc_to_csdev(csdev);
> > > + if (!ret && cti_assoc_ops && cti_assoc_ops->add)
> > > + cti_assoc_ops->add(csdev);
> > >
> > > mutex_unlock(&coresight_mutex);
> > > if (ret) {
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > >
>
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
More information about the linux-arm-kernel
mailing list