Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v12 1/2] drivers/coresight: Add UltraSoc System Memory
 Buffer driver
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>
References: <20221109135008.9485-1-hejunhao3@huawei.com>
 <20221109135008.9485-2-hejunhao3@huawei.com>
 <20221109165615.00006060@Huawei.com>
CC:     <mathieu.poirier@linaro.org>, <suzuki.poulose@arm.com>,
        <mike.leach@linaro.org>, <leo.yan@linaro.org>,
        <john.garry@huawei.com>, <coresight@lists.linaro.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <lpieralisi@kernel.org>,
        <linuxarm@huawei.com>, <liuqi115@huawei.com>,
        <f.fangjian@huawei.com>, <prime.zeng@hisilicon.com>
From:   hejunhao <hejunhao3@huawei.com>
Message-ID: <025f4daf-b5c6-e70e-977d-f475a6aae8ef@huawei.com>
Date:   Thu, 10 Nov 2022 19:13:44 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.1
MIME-Version: 1.0
In-Reply-To: <20221109165615.00006060@Huawei.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.