Messages in this thread | | | Subject | Re: [PATCH v11 1/2] drivers/coresight: Add UltraSoc System Memory Buffer driver | From | hejunhao <> | Date | Tue, 8 Nov 2022 17:44:41 +0800 |
| |
Hi Jonathan,
Thanks for your comments.
On 2022/11/7 22:30, Jonathan Cameron wrote: > On Mon, 7 Nov 2022 21:06:23 +0800 > Junhao He <hejunhao3@huawei.com> wrote: > >> From: Qi Liu <liuqi115@huawei.com> >> >> This patch adds driver for UltraSoc SMB(System Memory Buffer) >> device. SMB provides a way to buffer messages from ETM, and >> store these "CPU instructions trace" in system memory. >> >> SMB is developed by UltraSoc technology, which is acquired by >> Siemens, and we still use "UltraSoc" to name driver. >> >> Signed-off-by: Qi Liu <liuqi115@huawei.com> >> Signed-off-by: Junhao He <hejunhao3@huawei.com> >> Tested-by: JunHao He <hejunhao3@huawei.com> > Hi JunHao, > > It's been a while since I last looked at this driver, so I may have > forgotten or missed previous discussions. > > All the comments inline are fairly superficial and mostly concerned > with making the code easy to review / maintain rather than correctness. > > Jonathan Yes, I will do that. Junhao. >> --- >> drivers/hwtracing/coresight/Kconfig | 11 + >> drivers/hwtracing/coresight/Makefile | 1 + >> drivers/hwtracing/coresight/ultrasoc-smb.c | 631 +++++++++++++++++++++ >> drivers/hwtracing/coresight/ultrasoc-smb.h | 113 ++++ >> 4 files changed, 756 insertions(+) >> create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.c >> create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.h >> >> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig >> index 45c1eb5dfcb7..05d791cb05e3 100644 >> --- a/drivers/hwtracing/coresight/Kconfig >> +++ b/drivers/hwtracing/coresight/Kconfig >> @@ -201,4 +201,15 @@ config CORESIGHT_TRBE >> >> To compile this driver as a module, choose M here: the module will be >> called coresight-trbe. >> + >> +config ULTRASOC_SMB >> + tristate "Ultrasoc system memory buffer drivers" >> + depends on ACPI && ARM64 && CORESIGHT_LINKS_AND_SINKS > Can you relax this at all in the interests of getting better CI build coverage > from random configs etc. > > >From a quick look, I think you can safely drop the ACPI dependency on basis > relevant functions are stubbed out in acpi.h > > However, it looks like coresight more generally uses such depends, so perhaps > better to just leave them here for consistency. Ok, Will check it. Thanks. >> + help >> + This driver provides support for the Ultrasoc system memory buffer (SMB). >> + SMB is responsible for receiving the trace data from Coresight ETM devices >> + and storing them to a system buffer. >> + >> + To compile this driver as a module, choose M here: the module will be >> + called ultrasoc-smb. >> endif > >> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c >> new file mode 100644 >> index 000000000000..7fe8bf9623e8 >> --- /dev/null >> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c >> @@ -0,0 +1,631 @@ > ... > > >> + >> +static void smb_buffer_sync_status(struct smb_drv_data *drvdata) >> +{ >> + struct smb_data_buffer *sdb = &drvdata->sdb; >> + >> + sdb->wr_offset = readl(drvdata->base + SMB_LB_WR_ADDR) - sdb->start_addr; >> + sdb->rd_offset = readl(drvdata->base + SMB_LB_RD_ADDR) - sdb->start_addr; >> + if (sdb->wr_offset == sdb->rd_offset && !smb_buffer_is_empty(drvdata)) >> + sdb->full = true; >> + else >> + sdb->full = false; > Could do as > sdb->full = sdb->wr_offset == sdb->rd_offset && !smb_buffer_is_empty(drvdata); > up to you on which you think is more readable. Ok, I will drop this "if else" block. Thanks. > >> +} >> + > > >> +static struct attribute *smb_sink_attrs[] = { >> + coresight_simple_reg32(read_pos, SMB_LB_RD_ADDR), >> + coresight_simple_reg32(write_pos, SMB_LB_WR_ADDR), >> + coresight_simple_reg32(buf_status, SMB_LB_INT_STS), >> + &dev_attr_buf_size.attr, >> + NULL, > As below. Sure, Will fix in next version. Thanks. > >> +}; >> + >> +static const struct attribute_group smb_sink_group = { >> + .attrs = smb_sink_attrs, >> + .name = "mgmt", >> +}; >> + >> +static const struct attribute_group *smb_sink_groups[] = { >> + &smb_sink_group, >> + NULL, > Generally no comma after a NULL terminator. Having a comma > implies it may make sense to put something after it, which is never > the case for these. Sure, Will fix in next version. Thanks. >> +}; >> + > > ... > >> +static void smb_sync_perf_buffer(struct smb_drv_data *drvdata, >> + struct cs_buffers *buf, >> + unsigned long head, >> + unsigned long data_size) >> +{ >> + struct smb_data_buffer *sdb = &drvdata->sdb; >> + char **dst_pages = (char **)buf->data_pages; > Do you need the cast? It's void ** so implicit cast should work I think. > char **dst_pages = buf->data_pages; Ok, Will fix in next version. Thanks. >> + unsigned long to_copy; >> + long pg_idx, pg_offset; >> + >> + pg_idx = head >> PAGE_SHIFT; >> + pg_offset = head & (PAGE_SIZE - 1); >> + >> + while (data_size) { >> + unsigned long pg_space = PAGE_SIZE - pg_offset; >> + >> + /* Copy parts of trace data when read pointer wrap around */ >> + if (sdb->rd_offset + pg_space > sdb->buf_size) >> + to_copy = sdb->buf_size - sdb->rd_offset; >> + else >> + to_copy = min(data_size, pg_space); >> + >> + memcpy(dst_pages[pg_idx] + pg_offset, >> + sdb->buf_base + sdb->rd_offset, to_copy); >> + >> + pg_offset += to_copy; >> + if (pg_offset >= PAGE_SIZE) { >> + pg_offset = 0; >> + pg_idx++; >> + pg_idx %= buf->nr_pages; >> + } >> + data_size -= to_copy; >> + sdb->rd_offset += to_copy; >> + sdb->rd_offset %= sdb->buf_size; >> + } >> + >> + sdb->data_size = 0; >> + writel(sdb->start_addr + sdb->rd_offset, drvdata->base + SMB_LB_RD_ADDR); >> + >> + /* >> + * Data remained in link cannot be purged when SMB is full, so >> + * synchronize the read pointer to write pointer, to make sure >> + * these remained data won't influence next trace. >> + */ >> + if (sdb->full) { >> + smb_purge_data(drvdata); >> + writel(readl(drvdata->base + SMB_LB_WR_ADDR), >> + drvdata->base + SMB_LB_RD_ADDR); >> + } >> + smb_reset_buffer_status(drvdata); >> +} > > ... > >> + >> +static void smb_init_hw(struct smb_drv_data *drvdata) >> +{ >> + /* First disable smb and clear the status of SMB buffer */ > Check for consistency in capitalization of SMB in all comments. Sure, Will fix in next version. Thanks. > >> + smb_reset_buffer_status(drvdata); >> + smb_disable_hw(drvdata); >> + smb_purge_data(drvdata); >> + >> + writel(SMB_BUF_CFG_STREAMING, drvdata->base + SMB_LB_CFG_LO); >> + writel(SMB_MSG_FILTER, drvdata->base + SMB_LB_CFG_HI); >> + writel(SMB_GLOBAL_CFG, drvdata->base + SMB_CFG_REG); >> + writel(SMB_GLB_INT_CFG, drvdata->base + SMB_GLOBAL_INT); >> + writel(SMB_BUF_INT_CFG, drvdata->base + SMB_LB_INT_CTRL); >> +} >> + > > ... > >> +static int smb_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct smb_drv_data *drvdata; >> + int ret; >> + >> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); >> + if (!drvdata) >> + return -ENOMEM; >> + >> + drvdata->base = devm_platform_ioremap_resource(pdev, SMB_BASE_ADDR_RES); >> + if (IS_ERR(drvdata->base)) { >> + dev_err(dev, "Failed to ioremap resource\n"); >> + return PTR_ERR(drvdata->base); >> + } >> + >> + ret = smb_init_data_buffer(pdev, &drvdata->sdb); >> + if (ret) { >> + dev_err(dev, "Failed to init buffer, ret = %d\n", ret); >> + return ret; >> + } >> + >> + smb_init_hw(drvdata); >> + mutex_init(&drvdata->mutex); >> + drvdata->pid = -1; >> + >> + ret = smb_register_sink(pdev, drvdata); >> + if (ret) { >> + dev_err(dev, "Failed to register smb sink\n"); >> + return ret; >> + } >> + >> + ret = smb_config_inport(dev, true); >> + if (ret) { >> + smb_unregister_sink(drvdata); >> + return ret; >> + } >> + >> + platform_set_drvdata(pdev, drvdata); >> + return 0; >> +} >> + >> +static int smb_remove(struct platform_device *pdev) >> +{ >> + struct smb_drv_data *drvdata = platform_get_drvdata(pdev); >> + int ret; >> + >> + ret = smb_config_inport(&pdev->dev, false); >> + if (ret) >> + return ret; >> + >> + smb_unregister_sink(drvdata); > Trivial: I find a blank line before plane returns like this helps > a little with readability. Up to you though! Ok, Will fix it. Thanks. > >> + return 0; >> +} >> + >> +static const struct acpi_device_id ultrasoc_smb_acpi_match[] = { >> + {"HISI03A1", 0}, >> + {}, > Trivial, but little point in a trailing comma on a NULL terminator. Sure, Will fix in next version. Thanks. > > {} > }; > > is normally fine - note this is a bit subsystem specific so maintainer > may say otherwise. Ok, Thanks. > >> +}; >> +MODULE_DEVICE_TABLE(acpi, ultrasoc_smb_acpi_match); >> + >> +static struct platform_driver smb_driver = { >> + .driver = { >> + .name = "ultrasoc-smb", >> + .acpi_match_table = ACPI_PTR(ultrasoc_smb_acpi_match), >> + .suppress_bind_attrs = true, >> + }, >> + .probe = smb_probe, >> + .remove = smb_remove, >> +}; >> +module_platform_driver(smb_driver); >> + >> +MODULE_DESCRIPTION("UltraSoc SMB CoreSight driver"); >> +MODULE_LICENSE("Dual MIT/GPL"); >> +MODULE_AUTHOR("Jonathan Zhou <jonathan.zhouwen@huawei.com>"); >> +MODULE_AUTHOR("Qi Liu <liuqi115@huawei.com>"); >> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h >> new file mode 100644 >> index 000000000000..56170e1a883d >> --- /dev/null >> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h >> @@ -0,0 +1,113 @@ >> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ >> +/* >> + * Siemens System Memory Buffer driver. >> + * Copyright(c) 2022, HiSilicon Limited. >> + */ >> + >> +#ifndef _ULTRASOC_SMB_H >> +#define _ULTRASOC_SMB_H >> + >> +#include <linux/coresight.h> > I think you could move this down into the c files and provide > a forwards definition of struct coresight device > > Always good to keep scope of includes to minimum necessary. Sure, Will fix in next version. Thanks. > >> +#include <linux/miscdevice.h> >> +#include <linux/mutex.h> >> + >> +/* Offset of SMB logical buffer registers */ >> +#define SMB_CFG_REG 0x00 > To avoid any naming confusion I would postfix all the register > addresses with _REG > Then rethink the naming so the field names make it clear which > register they are in. OK, I'll check all REG names. Thanks. > >> +#define SMB_GLOBAL_EN 0x04 >> +#define SMB_GLOBAL_INT 0x08 >> +#define SMB_LB_CFG_LO 0x40 >> +#define SMB_LB_CFG_HI 0x44 >> +#define SMB_LB_INT_CTRL 0x48 >> +#define SMB_LB_INT_STS 0x4c >> +#define SMB_LB_LIMIT 0x58 >> +#define SMB_LB_RD_ADDR 0x5c >> +#define SMB_LB_WR_ADDR 0x60 >> +#define SMB_LB_PURGE 0x64 >> + >> +/* Set SMB_CFG_REG register */ >> +#define SMB_BURST_LEN GENMASK(7, 4) >> +#define SMB_IDLE_PRD GENMASK(15, 12) >> +#define SMB_MEM_WR GENMASK(17, 16) >> +#define SMB_MEM_RD (GENMASK(26, 25) | GENMASK(23, 22)) > Are these masks, or default values? Ideally express them as > a field mask then the value via FIELD_PREP > > e.g. > #define SMB_CFG_BURST_LEN_MSK GENMASK(7, 4) > #define SMB_GLOBAL_CFG_DEFAULT ... | FIELD_PREP(SMB_CFG_BURST_LEN_MSK, 0xf) | etc Sure, I will do that. Thanks. > > >> +#define SMB_GLOBAL_CFG (SMB_IDLE_PRD | SMB_MEM_WR | SMB_MEM_RD | \ >> + SMB_BURST_LEN) >> + >> +/* Set SMB_GLOBAL_INT register */ >> +#define SMB_INT_EN BIT(0) >> +#define SMB_INT_TYPE_PULSE BIT(1) >> +#define SMB_INT_POLARITY_HIGH BIT(2) >> +#define SMB_GLB_INT_CFG (SMB_INT_EN | SMB_INT_TYPE_PULSE | \ >> + SMB_INT_POLARITY_HIGH) >> + >> +/* Set SMB_LB_CFG_LO register */ >> +#define SMB_BUF_EN BIT(0) >> +#define SMB_BUF_SINGLE_END BIT(1) >> +#define SMB_BUF_INIT BIT(8) >> +#define SMB_BUF_CONTINUOUS BIT(11) >> +#define SMB_FILTER_FLOW GENMASK(19, 16) >> +#define SMB_BUF_CFG_STREAMING (SMB_BUF_INIT | SMB_BUF_CONTINUOUS | \ >> + SMB_FILTER_FLOW | SMB_BUF_SINGLE_END | \ >> + SMB_BUF_EN) >> + >> +#define SMB_BASE_LOW_MASK GENMASK(31, 0) >> + >> +/* Set SMB_LB_CFG_HI register */ >> +#define SMB_MSG_FILTER GENMASK(15, 8) >> + >> +/* Set SMB_LB_INT_CTRL */ >> +#define SMB_BUF_INT_EN BIT(0) >> +#define SMB_BUF_NOTE_MASK GENMASK(11, 8) >> +#define SMB_BUF_INT_CFG (SMB_BUF_INT_EN | SMB_BUF_NOTE_MASK) >> + >> +#define SMB_BUF_NOT_EMPTY BIT(0) >> +#define SMB_RESET_BUF_STS GENMASK(3, 0) >> +#define SMB_PURGED BIT(0) >> +#define SMB_HW_ENABLE BIT(0) > It is useful to give fields names that reflect which register they are in. > Perhaps > SMB_GLOBAL_EN_HW_ENABLE for this one. Sure, I will do that. Thanks. > >> + >> +#define SMB_BASE_ADDR_RES 0 >> +#define SMB_BUF_INFO_RES 1 >> + >> +/** >> + * struct smb_data_buffer - Details of the buffer used by SMB >> + * @buf_base: Memory mapped base address of SMB. >> + * @start_addr: SMB buffer start Physical address. >> + * @buf_size: Size of the buffer. >> + * @data_size: Size of Trace data copy to userspace. >> + * @rd_offset: Offset of the read pointer in the buffer. >> + * @wr_offset: Offset of the write pointer in the buffer. >> + * @status: Status of SMB buffer. > Naming wrong. Sure, Will fix in next version. Thanks. > >> + */ >> +struct smb_data_buffer { >> + void __iomem *buf_base; >> + u32 start_addr; >> + unsigned long buf_size; >> + unsigned long data_size; >> + unsigned long rd_offset; >> + unsigned long wr_offset; >> + bool full; >> +}; >> + >> +/** >> + * struct smb_drv_data - specifics associated to an SMB component >> + * @base: Memory mapped base address for SMB component. >> + * @csdev: Component vitals needed by the framework. >> + * @sdb: Data buffer for SMB. >> + * @miscdev: Specifics to handle "/dev/xyz.smb" entry. >> + * @mutex: Control data access to one at a time. >> + * @reading: Synchronise user space access to SMB buffer. >> + * @pid: Process ID of the process being monitored by the >> + * session that is using this component. >> + * @mode: how this SMB is being used, perf mode or sysfs mode. >> + */ >> +struct smb_drv_data { >> + void __iomem *base; >> + struct coresight_device *csdev; >> + struct smb_data_buffer sdb; >> + struct miscdevice miscdev; >> + struct mutex mutex; >> + local_t reading; >> + pid_t pid; >> + u32 mode; >> +}; >> + >> +#endif > . > Best regards, Junhao.
| |