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;
Message-ID: <9f5f66fa-0388-6a76-25c9-cacef0e7a4e2@arm.com>
Date:   Wed, 23 Nov 2022 14:03:14 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.5.0
Subject: Re: [PATCH v14 1/2] drivers/coresight: Add UltraSoc System Memory
 Buffer driver
To:     Junhao He <hejunhao3@huawei.com>, mathieu.poirier@linaro.org,
        mike.leach@linaro.org, leo.yan@linaro.org,
        jonathan.cameron@huawei.com, john.garry@huawei.com
Cc:     coresight@lists.linaro.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
        lpieralisi@kernel.org, linuxarm@huawei.com, yangyicong@huawei.com,
        liuqi6124@gmail.com, f.fangjian@huawei.com,
        prime.zeng@hisilicon.com
References: <20221123123823.27973-1-hejunhao3@huawei.com>
 <20221123123823.27973-2-hejunhao3@huawei.com>
From:   Suzuki K Poulose <suzuki.poulose@arm.com>
In-Reply-To: <20221123123823.27973-2-hejunhao3@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 23/11/2022 12:38, Junhao He wrote:
> From: Qi Liu <liuqi115@huawei.com>
> 
> Add 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.
> The SMB device is identifier as ACPI HID "HISI03A1". Device
> system memory address resources are allocated using the _CRS
> method and buffer modes is the circular buffer mode.
> 
> 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>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   drivers/hwtracing/coresight/Kconfig        |  12 +
>   drivers/hwtracing/coresight/Makefile       |   1 +
>   drivers/hwtracing/coresight/ultrasoc-smb.c | 658 +++++++++++++++++++++
>   drivers/hwtracing/coresight/ultrasoc-smb.h | 129 ++++
>   4 files changed, 800 insertions(+)
>   create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.c
>   create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.h
> 

> +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;
> +	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;


--8>-- cut here --<8--

> +	writel(sdb->start_addr + sdb->rd_offset,
> +		drvdata->base + SMB_LB_RD_ADDR_REG);
> +
> +	/*
> +	 * 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_REG),
> +		       drvdata->base + SMB_LB_RD_ADDR_REG);
> +	}

--<8-- end here --8>--

As pointed out in the last review, we must do this step
everytime for perf mode irrespective of whether the buffer
was "FULL" or not.

i.e, the above block should simply be:

	if (sdb->full)
		smb_purge_data(drvdata);

	/*
	 * The uncollected Data must be discarded for perf,
	 * as it cannot be clubbed with next schedule. We
	 * any way TRUNCATE the buffer in this case.
	 */
	writel(readl(drvdata->base + SMB_LB_WR_ADDR_REG),
		drvdata->base + SMB_LB_RD_ADDR_REG);

Suzuki