Message ID | 20240705090049.1656986-4-quic_jiegan@quicinc.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Coresight: Add Coresight Control Unit driver | expand |
On 05/07/2024 11:00, Jie Gan wrote: > The Coresight Control Unit hosts miscellaneous configuration registers > which control various features related to TMC ETR sink. > > Based on the trace ID, which is programmed in the related CCU ATID register > of a specific ETR, trace data with that trace ID gets into the ETR buffer, .... > +static int ccu_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct coresight_platform_data *pdata; > + struct ccu_drvdata *drvdata; > + struct coresight_desc desc = { 0 }; > + struct resource *res; > + > + desc.name = coresight_alloc_device_name(&ccu_devs, dev); > + if (!desc.name) > + return -ENOMEM; > + pdata = coresight_get_platform_data(dev); > + if (IS_ERR(pdata)) > + return PTR_ERR(pdata); > + pdev->dev.platform_data = pdata; > + > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; > + drvdata->dev = &pdev->dev; Use stored dev variable. > + drvdata->atid_offset = 0; > + platform_set_drvdata(pdev, drvdata); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ccu-base"); > + if (!res) > + return -ENODEV; > + drvdata->pbase = res->start; Drop. > + > + drvdata->base = devm_ioremap(dev, res->start, resource_size(res)); Use proper wrapper for this two. > + if (!drvdata->base) > + return -ENOMEM; > + > + desc.type = CORESIGHT_DEV_TYPE_HELPER; > + desc.pdata = pdev->dev.platform_data; > + desc.dev = &pdev->dev; > + desc.ops = &ccu_ops; > + > + drvdata->csdev = coresight_register(&desc); > + if (IS_ERR(drvdata->csdev)) > + return PTR_ERR(drvdata->csdev); > + > + dev_dbg(dev, "CCU initialized: %s\n", desc.name); Drop. > + return 0; > +} > + > +static void ccu_remove(struct platform_device *pdev) > +{ > + struct ccu_drvdata *drvdata = platform_get_drvdata(pdev); > + > + coresight_unregister(drvdata->csdev); > +} > + > +static const struct of_device_id ccu_match[] = { > + {.compatible = "qcom,coresight-ccu"}, > + {} > +}; > + > +static struct platform_driver ccu_driver = { > + .probe = ccu_probe, > + .remove = ccu_remove, > + .driver = { > + .name = "coresight-ccu", > + .of_match_table = ccu_match, > + .suppress_bind_attrs = true, Why? > + }, > +}; > + > +static int __init ccu_init(void) > +{ > + return platform_driver_register(&ccu_driver); > +} > +module_init(ccu_init); > + > +static void __exit ccu_exit(void) > +{ > + platform_driver_unregister(&ccu_driver); > +} > +module_exit(ccu_exit); Why this is not just module platform driver? Best regards, Krzysztof
On Fri, Jul 05, 2024 at 11:11:19AM +0200, Krzysztof Kozlowski wrote: > On 05/07/2024 11:00, Jie Gan wrote: > > The Coresight Control Unit hosts miscellaneous configuration registers > > which control various features related to TMC ETR sink. > > > > Based on the trace ID, which is programmed in the related CCU ATID register > > of a specific ETR, trace data with that trace ID gets into the ETR buffer, > > .... > > > +static int ccu_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct coresight_platform_data *pdata; > > + struct ccu_drvdata *drvdata; > > + struct coresight_desc desc = { 0 }; > > + struct resource *res; > > + > > + desc.name = coresight_alloc_device_name(&ccu_devs, dev); > > + if (!desc.name) > > + return -ENOMEM; > > + pdata = coresight_get_platform_data(dev); > > + if (IS_ERR(pdata)) > > + return PTR_ERR(pdata); > > + pdev->dev.platform_data = pdata; > > + > > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > > + if (!drvdata) > > + return -ENOMEM; > > + drvdata->dev = &pdev->dev; > > Use stored dev variable. pdev->dev replaced by dev variable. > > > + drvdata->atid_offset = 0; > > + platform_set_drvdata(pdev, drvdata); > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ccu-base"); > > + if (!res) > > + return -ENODEV; > > + drvdata->pbase = res->start; > > Drop. Dropped. > > > + > > + drvdata->base = devm_ioremap(dev, res->start, resource_size(res)); > > Use proper wrapper for this two. Replaced by: res = platform_get_resource(pdev, IORESOURCE_MEM, 0); drvdata->base = devm_ioremap_resource(dev, res); > > > + if (!drvdata->base) > > + return -ENOMEM; > > + > > + desc.type = CORESIGHT_DEV_TYPE_HELPER; > > + desc.pdata = pdev->dev.platform_data; > > + desc.dev = &pdev->dev; > > + desc.ops = &ccu_ops; > > + > > + drvdata->csdev = coresight_register(&desc); > > + if (IS_ERR(drvdata->csdev)) > > + return PTR_ERR(drvdata->csdev); > > + > > + dev_dbg(dev, "CCU initialized: %s\n", desc.name); > > Drop. Dropped. > > > + return 0; > > +} > > + > > +static void ccu_remove(struct platform_device *pdev) > > +{ > > + struct ccu_drvdata *drvdata = platform_get_drvdata(pdev); > > + > > + coresight_unregister(drvdata->csdev); > > +} > > + > > +static const struct of_device_id ccu_match[] = { > > + {.compatible = "qcom,coresight-ccu"}, > > + {} > > +}; > > + > > +static struct platform_driver ccu_driver = { > > + .probe = ccu_probe, > > + .remove = ccu_remove, > > + .driver = { > > + .name = "coresight-ccu", > > + .of_match_table = ccu_match, > > + .suppress_bind_attrs = true, > > Why? Sorry, I dont get the point here. We dont need automatic bind/unbind, so the suppress_bind_attrs sets to true. We need configure some settings before we register the device. > > > + }, > > +}; > > + > > +static int __init ccu_init(void) > > +{ > > + return platform_driver_register(&ccu_driver); > > +} > > +module_init(ccu_init); > > + > > +static void __exit ccu_exit(void) > > +{ > > + platform_driver_unregister(&ccu_driver); > > +} > > +module_exit(ccu_exit); > > Why this is not just module platform driver? Replaced by module_platform_driver(ccu_driver); > > Best regards, > Krzysztof > Thanks, Jie
On 08/07/2024 05:16, JieGan wrote: > >> >>> + >>> + drvdata->base = devm_ioremap(dev, res->start, resource_size(res)); >> >> Use proper wrapper for this two. > Replaced by: > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > drvdata->base = devm_ioremap_resource(dev, res); Why? Use the wrapper. ... >>> + >>> +static struct platform_driver ccu_driver = { >>> + .probe = ccu_probe, >>> + .remove = ccu_remove, >>> + .driver = { >>> + .name = "coresight-ccu", >>> + .of_match_table = ccu_match, >>> + .suppress_bind_attrs = true, >> >> Why? > Sorry, I dont get the point here. You do not get the point why I am asking "why?"? Why do you need it? > We dont need automatic bind/unbind, so the suppress_bind_attrs sets to true. But I need it... > We need configure some settings before we register the device. Hm, is this expected for coresight devices? Best regards, Krzysztof
… > Disabling source device resets the bit according to the source device's trace ID. How do you think about to improve such a change description with imperative wordings? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc7#n94 … > +++ b/drivers/hwtracing/coresight/coresight-ccu.c > @@ -0,0 +1,290 @@ … > +static int __ccu_set_etr_traceid(struct coresight_device *csdev, > + uint32_t traceid, bool enable) > +{ … > + spin_lock_irqsave(&drvdata->spin_lock, flags); > + CS_UNLOCK(drvdata->base); … > + ccu_writel(drvdata, val, reg_offset); > + > + CS_LOCK(drvdata->base); > + spin_unlock_irqrestore(&drvdata->spin_lock, flags); > + return 0; > +} … Under which circumstances would you become interested to apply a statement like “guard(spinlock_irqsave)(&drvdata->spin_lock);”? https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/spinlock.h#L574 Regards, Markus
On Mon, Jul 08, 2024 at 12:44:43PM +0200, Krzysztof Kozlowski wrote: > On 08/07/2024 05:16, JieGan wrote: > > > >> > >>> + > >>> + drvdata->base = devm_ioremap(dev, res->start, resource_size(res)); > >> > >> Use proper wrapper for this two. > > Replaced by: > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > drvdata->base = devm_ioremap_resource(dev, res); > > Why? > > Use the wrapper. Sorry, I misunderstood the "wrapper" before. Just wrapped drvdata->base by void __iomem *base; > > > ... > > >>> + > >>> +static struct platform_driver ccu_driver = { > >>> + .probe = ccu_probe, > >>> + .remove = ccu_remove, > >>> + .driver = { > >>> + .name = "coresight-ccu", > >>> + .of_match_table = ccu_match, > >>> + .suppress_bind_attrs = true, > >> > >> Why? > > Sorry, I dont get the point here. > > You do not get the point why I am asking "why?"? > > Why do you need it? > > > We dont need automatic bind/unbind, so the suppress_bind_attrs sets to true. > > But I need it... > > > We need configure some settings before we register the device. > > Hm, is this expected for coresight devices? The coresight device cannot be unbinded independently. As we known, The coresight devices collaborate to form a path, from source, link, to sink. If an unexpected unbinding occurs for a coresight device that is part of the path, it will disrupt the entire path. I think it's the reason why the coresight device driver does not need automatic bind/unbind. Here is the previous discussion for suppress_bind_attrs: https://lore.kernel.org/all/1453753248-1716-1-git-send-email-mathieu.poirier@linaro.org/#r > > Best regards, > Krzysztof > Thanks, Jie
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index 06f0a7594169..a36934daa505 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -133,6 +133,12 @@ config CORESIGHT_STM To compile this driver as a module, choose M here: the module will be called coresight-stm. +config CORESIGHT_CCU + tristate "CoreSight Control Unit driver" + help + This driver provides support for CoreSight Control Unit block + that hosts miscellaneous configuration registers. + config CORESIGHT_CPU_DEBUG tristate "CoreSight CPU Debug driver" depends on ARM || ARM64 diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 4ba478211b31..a668cdba926f 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -51,3 +51,4 @@ coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \ coresight-cti-sysfs.o obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o +obj-$(CONFIG_CORESIGHT_CCU) += coresight-ccu.o diff --git a/drivers/hwtracing/coresight/coresight-ccu.c b/drivers/hwtracing/coresight/coresight-ccu.c new file mode 100644 index 000000000000..30faf87b1a28 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-ccu.c @@ -0,0 +1,290 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include <linux/clk.h> +#include <linux/coresight.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/coresight-stm.h> + +#include "coresight-ccu.h" +#include "coresight-priv.h" +#include "coresight-tmc.h" +#include "coresight-trace-id.h" + +DEFINE_CORESIGHT_DEVLIST(ccu_devs, "ccu"); + +#define ccu_writel(drvdata, val, offset) __raw_writel((val), drvdata->base + offset) +#define ccu_readl(drvdata, offset) __raw_readl(drvdata->base + offset) + +/* The Coresight Control Unit uses four ATID registers to control the data filter function based + * on the trace ID for each TMC ETR sink. The length of each ATID register is 32 bits. Therefore, + * the ETR has a related field in CCU that is 128 bits long. Each trace ID is represented by one bit in that filed. + * e.g. ETR0ATID0 layout, set bit 5 for traceid 5 + * bit5 + * ------------------------------------------------------ + * | |28| |24| |20| |16| |12| |8| 1|4| |0| + * ------------------------------------------------------ + * + * e.g. ETR0: + * 127 0 from ATID_offset for ETR0ATID0 + * ------------------------- + * |ATID3|ATID2|ATID1|ATID0| + * + */ +#define CCU_ATID_REG_OFFSET(traceid, atid_offset) \ + ((traceid / 32) * 4 + atid_offset) + +#define CCU_ATID_REG_BIT(traceid) (traceid % 32) +#define CCU_ATID_REG_SIZE 0x10 + +/* + * __ccu_set_etr_traceid: Set bit in the ATID register based on trace ID when enable is true. + * Reset the bit of the ATID register based on trace ID when enable is false. + * + * @csdev: coresight_device struct related to the device + * @traceid: trace ID of the source tracer. + * @enable: True for set bit and false for reset bit. + * + * Returns 0 indicates success. Non-zero result means failure. + */ +static int __ccu_set_etr_traceid(struct coresight_device *csdev, + uint32_t traceid, bool enable) +{ + uint32_t atid_offset = 0; + struct ccu_drvdata *drvdata; + unsigned long flags; + uint32_t reg_offset; + int bit; + uint32_t val; + + drvdata = dev_get_drvdata(csdev->dev.parent); + if (IS_ERR_OR_NULL(drvdata)) + return -EINVAL; + + atid_offset = drvdata->atid_offset; + if (((traceid < 0) && (traceid >= CORESIGHT_TRACE_IDS_MAX)) || atid_offset <= 0) + return -EINVAL; + + spin_lock_irqsave(&drvdata->spin_lock, flags); + CS_UNLOCK(drvdata->base); + + reg_offset = CCU_ATID_REG_OFFSET(traceid, atid_offset); + bit = CCU_ATID_REG_BIT(traceid); + if (reg_offset - atid_offset >= CCU_ATID_REG_SIZE + || bit >= CORESIGHT_TRACE_IDS_MAX) { + CS_LOCK(drvdata); + spin_unlock_irqrestore(&drvdata->spin_lock, flags); + return -EINVAL; + } + + val = ccu_readl(drvdata, reg_offset); + if (enable) + val = val | BIT(bit); + else + val = val & ~BIT(bit); + ccu_writel(drvdata, val, reg_offset); + + CS_LOCK(drvdata->base); + spin_unlock_irqrestore(&drvdata->spin_lock, flags); + return 0; +} + +/* + * ccu_set_atid_offset: Retrieve the offset of the CCU ATID register and store it in the driver data of ETR. + * + * Returns 0 indicates success. If the result is less than zero, it means + * failure. + */ +static int __ccu_set_atid_offset(struct device_node *helper_node, struct coresight_device *helper, int port) +{ + int atid_offset = 0; + struct device_node *node = helper_node; + struct device_node *child_node = NULL; + struct fwnode_handle *child_fwnode = NULL; + struct ccu_drvdata *drvdata; + + if (!helper_node || !helper) + return -EINVAL; + + drvdata = dev_get_drvdata(helper->dev.parent); + if (IS_ERR_OR_NULL(drvdata)) + return -EINVAL; + + child_node = of_get_child_by_name(node, "in-ports"); + if (!child_node) + return -EINVAL; + + child_fwnode = fwnode_graph_get_endpoint_by_id(&child_node->fwnode, port, 0, 0); + if (!child_fwnode) + return -EINVAL; + + fwnode_property_read_u32(child_fwnode, "qcom,ccu-atid-offset", &atid_offset); + drvdata->atid_offset = atid_offset; + dev_dbg(&helper->dev, "atid_offset:0x%x\n", atid_offset); + + return 0; +} + +static int ccu_set_atid_offset(struct coresight_device *sink, struct coresight_device *helper) +{ + int port, i, ret = 0; + struct device_node *node; + + for (i = 0; i < sink->pdata->nr_outconns; ++i) { + if (sink->pdata->out_conns[i]->dest_dev) { + port = sink->pdata->out_conns[i]->dest_port; + node = sink->pdata->out_conns[i]->dest_fwnode->dev->of_node; + ret = __ccu_set_atid_offset(node, helper, port); + return ret; + } + } + + return -EINVAL; +} + +/* + * ccu_set_etr_traceid: Retrieve the ATID offset and trace ID. + * + * Returns 0 indicates success. None-zero result means failure. + */ +static int ccu_set_etr_traceid(struct coresight_device *csdev, struct cs_sink_data *sink_data, bool enable) +{ + if ((sink_data->traceid < 0) || (csdev == NULL) || (sink_data->sink == NULL)) { + dev_dbg(&csdev->dev, "Invalid parameters\n"); + return -EINVAL; + } + + if (ccu_set_atid_offset(sink_data->sink, csdev)) + return -EINVAL; + + dev_dbg(&csdev->dev, "traceid is %d\n", sink_data->traceid); + + return __ccu_set_etr_traceid(csdev, sink_data->traceid, enable); +} + +static int ccu_enable(struct coresight_device *csdev, enum cs_mode mode, + void *data) +{ + int ret = 0; + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; + + ret = ccu_set_etr_traceid(csdev, sink_data, true); + if (ret) + dev_dbg(&csdev->dev,"enable data filter failed\n"); + + return 0; +} + +static int ccu_disable(struct coresight_device *csdev, void *data) +{ + int ret = 0; + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; + + ret = ccu_set_etr_traceid(csdev, sink_data, false); + if (ret) + dev_dbg(&csdev->dev,"disable data filter failed\n"); + + return 0; +} + +static const struct coresight_ops_helper ccu_helper_ops = { + .enable = ccu_enable, + .disable = ccu_disable, +}; + +static const struct coresight_ops ccu_ops = { + .helper_ops = &ccu_helper_ops, +}; + +static int ccu_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct coresight_platform_data *pdata; + struct ccu_drvdata *drvdata; + struct coresight_desc desc = { 0 }; + struct resource *res; + + desc.name = coresight_alloc_device_name(&ccu_devs, dev); + if (!desc.name) + return -ENOMEM; + pdata = coresight_get_platform_data(dev); + if (IS_ERR(pdata)) + return PTR_ERR(pdata); + pdev->dev.platform_data = pdata; + + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); + if (!drvdata) + return -ENOMEM; + drvdata->dev = &pdev->dev; + drvdata->atid_offset = 0; + platform_set_drvdata(pdev, drvdata); + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ccu-base"); + if (!res) + return -ENODEV; + drvdata->pbase = res->start; + + drvdata->base = devm_ioremap(dev, res->start, resource_size(res)); + if (!drvdata->base) + return -ENOMEM; + + desc.type = CORESIGHT_DEV_TYPE_HELPER; + desc.pdata = pdev->dev.platform_data; + desc.dev = &pdev->dev; + desc.ops = &ccu_ops; + + drvdata->csdev = coresight_register(&desc); + if (IS_ERR(drvdata->csdev)) + return PTR_ERR(drvdata->csdev); + + dev_dbg(dev, "CCU initialized: %s\n", desc.name); + return 0; +} + +static void ccu_remove(struct platform_device *pdev) +{ + struct ccu_drvdata *drvdata = platform_get_drvdata(pdev); + + coresight_unregister(drvdata->csdev); +} + +static const struct of_device_id ccu_match[] = { + {.compatible = "qcom,coresight-ccu"}, + {} +}; + +static struct platform_driver ccu_driver = { + .probe = ccu_probe, + .remove = ccu_remove, + .driver = { + .name = "coresight-ccu", + .of_match_table = ccu_match, + .suppress_bind_attrs = true, + }, +}; + +static int __init ccu_init(void) +{ + return platform_driver_register(&ccu_driver); +} +module_init(ccu_init); + +static void __exit ccu_exit(void) +{ + platform_driver_unregister(&ccu_driver); +} +module_exit(ccu_exit); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("CoreSight Control Unit driver"); diff --git a/drivers/hwtracing/coresight/coresight-ccu.h b/drivers/hwtracing/coresight/coresight-ccu.h new file mode 100644 index 000000000000..d2895d288c88 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-ccu.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#ifndef _CORESIGHT_CCU_H +#define _CORESIGHT_CCU_H + +struct ccu_drvdata { + void __iomem *base; + phys_addr_t pbase; + struct device *dev; + struct coresight_device *csdev; + spinlock_t spin_lock; + uint32_t atid_offset; +}; + +#endif
The Coresight Control Unit hosts miscellaneous configuration registers which control various features related to TMC ETR sink. Based on the trace ID, which is programmed in the related CCU ATID register of a specific ETR, trace data with that trace ID gets into the ETR buffer, while other trace data gets dropped. Enabling source device sets one bit of the ATID register based on source device's trace ID. Disabling source device resets the bit according to the source device's trace ID. Signed-off-by: Jie Gan <quic_jiegan@quicinc.com> --- drivers/hwtracing/coresight/Kconfig | 6 + drivers/hwtracing/coresight/Makefile | 1 + drivers/hwtracing/coresight/coresight-ccu.c | 290 ++++++++++++++++++++ drivers/hwtracing/coresight/coresight-ccu.h | 18 ++ 4 files changed, 315 insertions(+) create mode 100644 drivers/hwtracing/coresight/coresight-ccu.c create mode 100644 drivers/hwtracing/coresight/coresight-ccu.h