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

[hejunhao]

On 2022/11/23 22:03, Suzuki K Poulose wrote:
> On 23/11/2022 12:38, Junhao He wrote:
> > From: Qi Liu <liuqi115@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Junhao He <hejunhao3@xxxxxxxxxx>
> > Tested-by: JunHao He <hejunhao3@xxxxxxxxxx>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxx
> To unsubscribe send an email to coresight-leave@xxxxxxxxxxxxxxxx
>
> .