Messages in this thread Patch in this message | | | Subject | Re: [PATCH v12 1/2] drivers/coresight: Add UltraSoc System Memory Buffer driver | From | hejunhao <> | Date | Thu, 10 Nov 2022 19:13:44 +0800 |
| |
On 2022/11/10 0:56, Jonathan Cameron wrote: > On Wed, 9 Nov 2022 21:50:07 +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, > > One trivial side effect of dropping the ACPI dependency. > > Also, I think (at the cost of a slightly lengthening of lines) > you can rename the register fields to avoid any potential > confusion between GLB and LB registers. > > With those fixed feel free to add > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Hi Jonathan,
Thanks for you comments! I will fix these in the next version.
Thanks. > > ... > >> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c >> new file mode 100644 >> index 000000000000..ea2552a98d28 >> --- /dev/null >> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c > ... > >> +static const struct acpi_device_id ultrasoc_smb_acpi_match[] = { >> + {"HISI03A1", 0}, >> + {} >> +}; >> +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), > Now the driver build isn't dependent on CONFIG_ACPI > if !CONFIG_ACPI ACPI_PTR() doesn't reference the parameter. > As such you'll get unused warnings. > > 1 options to fix this > a) Drop ACPI_PTR() and just have .acpi_match_data = ultrasoc_smb_acpi_match > b) ifdef magic around the acpi_match table. > > In theory the first option results in bloat, but in this case I doubt we care. Ok, will fix it, as follows
``` #ifdef CONFIG_ACPI static const struct acpi_device_id ultrasoc_smb_acpi_match[] = { {"HISI03A1", 0}, {} }; MODULE_DEVICE_TABLE(acpi, ultrasoc_smb_acpi_match); #endif ```
>> + .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..2e2f9f8fe54b >> --- /dev/null >> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h >> @@ -0,0 +1,116 @@ >> +/* 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/miscdevice.h> >> +#include <linux/mutex.h> >> + >> +/* Offset of SMB global registers */ >> +#define SMB_GLB_CFG_REG 0x00 >> +#define SMB_GLB_EN_REG 0x04 >> +#define SMB_GLB_INT_REG 0x08 >> + >> +/* Offset of SMB logical buffer registers */ >> +#define SMB_LB_CFG_LO_REG 0x40 >> +#define SMB_LB_CFG_HI_REG 0x44 >> +#define SMB_LB_INT_CTRL_REG 0x48 >> +#define SMB_LB_INT_STS_REG 0x4c >> +#define SMB_LB_RD_ADDR_REG 0x5c >> +#define SMB_LB_WR_ADDR_REG 0x60 >> +#define SMB_LB_PURGE_REG 0x64 >> + >> +/* Set global config register */ >> +#define SMB_CFG_BURST_LEN_MSK GENMASK(11, 4) > Given there are several CFG registers, possibly worth > prefix of SMB_GLB_CFG_ ... Sure, I will do that. >> +#define SMB_CFG_IDLE_PRD_MSK GENMASK(15, 12) >> +#define SMB_CFG_MEM_WR_MSK GENMASK(21, 16) >> +#define SMB_CFG_MEM_RD_MSK GENMASK(27, 22) >> +#define SMB_GLB_CFG_DEFAULT (FIELD_PREP(SMB_CFG_BURST_LEN_MSK, 0xf) | \ >> + FIELD_PREP(SMB_CFG_IDLE_PRD_MSK, 0xf) | \ >> + FIELD_PREP(SMB_CFG_MEM_WR_MSK, 0x3) | \ >> + FIELD_PREP(SMB_CFG_MEM_RD_MSK, 0x1b)) >> + >> +/* Set global interrupt control register */ >> +#define SMB_INT_EN BIT(0) > Again, multiple INT registers, so SMB_INT_GLB_* perhaps? Ok, will fix it.
>> +#define SMB_INT_PULSE BIT(1) /* Interrupt type: 1 - Pulse */ >> +#define SMB_INT_ACT_H BIT(2) /* Interrupt polarity: 1 - Active high */ >> +#define SMB_GLB_INT_CFG (SMB_INT_EN | SMB_INT_PULSE | SMB_INT_ACT_H) >> + >> +/* Set logical buffer config register lower 32 bits */ >> +#define SMB_CFG_LO_EN BIT(0) > SMB_LB_CFG_... > > etc for other cases. Sure, I will do that.
>> +#define SMB_CFG_LO_SINGLE_END BIT(1) >> +#define SMB_CFG_LO_INIT BIT(8) >> +#define SMB_CFG_LO_CONT BIT(11) >> +#define SMB_CFG_LO_FLOW_MSK GENMASK(19, 16) >> +#define SMB_LB_CFG_LO_DEFAULT (SMB_CFG_LO_EN | SMB_CFG_LO_SINGLE_END | \ >> + SMB_CFG_LO_INIT | SMB_CFG_LO_CONT | \ >> + FIELD_PREP(SMB_CFG_LO_FLOW_MSK, 0xf)) >> + >> +/* Set logical buffer config register upper 32 bits */ >> +#define SMB_CFG_HI_RANGE_UP_MSK GENMASK(15, 8) >> +#define SMB_LB_CFG_HI_DEFAULT FIELD_PREP(SMB_CFG_HI_RANGE_UP_MSK, 0xff) >> + >> +/* Set logical buffer interrupt control register */ >> +#define SMB_INT_CTRL_EN BIT(0) >> +#define SMB_INT_CTRL_BUF_NOTE_MSK GENMASK(11, 8) >> +#define SMB_LB_INT_CTRL_CFG (SMB_INT_CTRL_EN | \ >> + FIELD_PREP(SMB_INT_CTRL_BUF_NOTE_MSK, 0xf)) >> + >> +#define SMB_LB_INT_STS_NOT_EMPTY_MSK BIT(0) >> +#define SMB_LB_STS_RESET_MSK GENMASK(3, 0) >> +#define SMB_LB_INT_BUF_STS_RESET FIELD_PREP(SMB_LB_STS_RESET_MSK, 0xf) >> +#define SMB_LB_PURGE_PURGED BIT(0) >> +#define SMB_GLB_EN_HW_ENABLE BIT(0) >> + >> +#define SMB_REG_ADDR_RES 0 >> +#define SMB_BUF_ADDR_RES 1 >> +#define SMB_BUF_ADDR_LO_MSK GENMASK(31, 0) > ... > >> + >> +/** >> + * 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. > Trivial, but for consistency should be: How this... Yes, I will do that.
>> + */ >> +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.
| |