Re: [PATCH v14 1/2] drivers/coresight: Add UltraSoc System Memory Buffer driver

hejunhao
2022-11-24T13:33:07Z

On 2022/11/23 22:03, Suzuki K Poulose wrote:
> 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

Hi Suzuki,

We need to update SMB_LB_RD_ADDR_REG register first, then
check the "full" flag, whether the register needs to be
updated again.

If we don`t update the value of SMB_LB_RD_ADDR_REG register
or reset buffer state, the buffer state will still be "full".
The buffer has not free area,so the data will still remain
in link.

Thanks.
HeJunhao.

> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org
>
> .
>