| Message ID | 20230526153508.6208-1-quic_jinlmao@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for a streaming interface for TMC ETR | expand |
Hi, I have a few general comments about this patch set. Firstly, I am assuming that this is a standard TMC, with the new byte monitor functionality implemented in the CSR block. Now that being the case, all the byte counter operations, the sysfs block_size, file ops to read the data and interrupt handling should be in the CSR driver, not the TMC driver. This counter is not part of a generic TMC device - but a specific hardware addition into your system. As such I would expect it to be in a separate driver. The specific enabling of the CSR counters from within the enable code of the TMC should be removed. If your device is set up correctly as a helper device with appropriate connections between TMC and CSR, then the enable operations can be handled automatically using the helper function mechnisms added in this patchset:- https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/2BBWZCBJWP3AOLBJSB64I5727JZFA6QZ/ I also see that you are assuming that you will be able to read the TMC memory faster than it fills - as there is no guard against overflow or detection when the TMC buffer wraps. Given the amount of trace that can be generated in a very short space of time, I cannot see how this can be guaranteed. Any undetected buffer wrap will result in significant decode failures. The normal sysfs read operations synchronise the DMA using a system call and read the RRP and RWP to ensure determine the start and end positions of the buffer. This cannot be done safely without stopping the TMC. Moreover, you are assuming that the buffer allocated is a contiguous flat mapped buffer, and not scatter gather. The change to the TMC core code - even if this operation could be guaranteed to be reliable, should be limited to extracting the data only - ensuring that the above constraints are observed. I'll comment inline in a couple of the other patches Thanks and Regards Mike On Fri, 26 May 2023 at 16:35, Mao Jinlong <quic_jinlmao@quicinc.com> wrote: > > This patch series is to add support for a streaming interface for > TMC ETR to allow for continuous log collection to secondary storage. > An interrupt based mechanism is used to stream out the data from the device. > > QDSS_CS_QDSSCSR_ETRIRQCTRL register is used to set the IRQ byte counter > value. The value of this registers defines the number of bytes that when moved by > the ETR AXI interface. It will casues an interrupt which can be used by an > userspace program to know how much data is present in memory requiring copy to some > other location. A zero setting disables the interrupt.A one setting > means 8 bytes, two 16 bytes, etc. In other words, the value in this > register is the interrupt threshold times 8 bytes. ETR must be enabled > when use this interrupt function. > > Sample: > echo 4096 > /sys/bus/coresight/devices/tmc_etr0/block_size > echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink > echo 1 > /sys/bus/coresight/devices/stm0/enabl_source > > cat /dev/byte-cntr > /data/qdss_etr.bin & > > The log collection will stop after disabling the ETR. > > Commit link: > https://git.codelinaro.org/clo/linux-kernel/coresight/-/commits/coresight-byte-cntr-v1 > > Mao Jinlong (3): > Coresight: Add driver to support for CSR > coresight-tmc: byte-cntr: Add support for streaming interface for ETR > dt-bindings: arm: Adds CoreSight CSR hardware definitions > > .../testing/sysfs-bus-coresight-devices-tmc | 7 + > .../bindings/arm/qcom,coresight-csr.yaml | 62 ++++ > drivers/hwtracing/coresight/Kconfig | 12 + > drivers/hwtracing/coresight/Makefile | 3 +- > .../hwtracing/coresight/coresight-byte-cntr.c | 304 ++++++++++++++++++ > .../hwtracing/coresight/coresight-byte-cntr.h | 49 +++ > drivers/hwtracing/coresight/coresight-csr.c | 168 ++++++++++ > drivers/hwtracing/coresight/coresight-csr.h | 59 ++++ > .../hwtracing/coresight/coresight-tmc-core.c | 66 ++++ > .../hwtracing/coresight/coresight-tmc-etr.c | 8 +- > drivers/hwtracing/coresight/coresight-tmc.h | 12 +- > 11 files changed, 745 insertions(+), 5 deletions(-) > create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-csr.yaml > create mode 100644 drivers/hwtracing/coresight/coresight-byte-cntr.c > create mode 100644 drivers/hwtracing/coresight/coresight-byte-cntr.h > create mode 100644 drivers/hwtracing/coresight/coresight-csr.c > create mode 100644 drivers/hwtracing/coresight/coresight-csr.h > > -- > 2.17.1 > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Hi Mike, Sorry for the late response. Thanks for the comments. On 5/30/2023 11:11 PM, Mike Leach wrote: > Hi, > > I have a few general comments about this patch set. > > Firstly, I am assuming that this is a standard TMC, with the new byte > monitor functionality implemented in the CSR block. > > Now that being the case, all the byte counter operations, the sysfs > block_size, file ops to read the data and interrupt handling should be > in the CSR driver, not the TMC driver. > This counter is not part of a generic TMC device - but a specific > hardware addition into your system. As such I would expect it to be in > a separate driver. > > The specific enabling of the CSR counters from within the enable code > of the TMC should be removed. If your device is set up correctly as a > helper device with appropriate connections between TMC and CSR, then > the enable operations can be handled automatically using the helper > function mechnisms added in this patchset:- > https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/2BBWZCBJWP3AOLBJSB64I5727JZFA6QZ/ I will consider to use helper function mechanism for CSR device. > > I also see that you are assuming that you will be able to read the TMC > memory faster than it fills - as there is no guard against overflow or > detection when the TMC buffer wraps. Given the amount of trace that > can be generated in a very short space of time, I cannot see how this > can be guaranteed. Any undetected buffer wrap will result in > significant decode failures. It can't be guarantee that read the TMC memory faster than it fills. There could be override issue happens. When issue happens, we usually suggest user to increase the buffer size of the ETR to make the gap large enough for reading the data and filling the data to ETR. > > The normal sysfs read operations synchronise the DMA using a system > call and read the RRP and RWP to ensure determine the start and end > positions of the buffer. This cannot be done safely without stopping > the TMC. Moreover, you are assuming that the buffer allocated is a > contiguous flat mapped buffer, and not scatter gather. When it is scatter gather mode, 4K data will be read each time. When it is flat mode, the required length will be read. > > The change to the TMC core code - even if this operation could be > guaranteed to be reliable, should be limited to extracting the data > only - ensuring that the above constraints are observed. > > I'll comment inline in a couple of the other patches > > Thanks and Regards > > Mike > > > On Fri, 26 May 2023 at 16:35, Mao Jinlong <quic_jinlmao@quicinc.com> wrote: >> This patch series is to add support for a streaming interface for >> TMC ETR to allow for continuous log collection to secondary storage. >> An interrupt based mechanism is used to stream out the data from the device. >> >> QDSS_CS_QDSSCSR_ETRIRQCTRL register is used to set the IRQ byte counter >> value. The value of this registers defines the number of bytes that when moved by >> the ETR AXI interface. It will casues an interrupt which can be used by an >> userspace program to know how much data is present in memory requiring copy to some >> other location. A zero setting disables the interrupt.A one setting >> means 8 bytes, two 16 bytes, etc. In other words, the value in this >> register is the interrupt threshold times 8 bytes. ETR must be enabled >> when use this interrupt function. >> >> Sample: >> echo 4096 > /sys/bus/coresight/devices/tmc_etr0/block_size >> echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink >> echo 1 > /sys/bus/coresight/devices/stm0/enabl_source >> >> cat /dev/byte-cntr > /data/qdss_etr.bin & >> >> The log collection will stop after disabling the ETR. >> >> Commit link: >> https://git.codelinaro.org/clo/linux-kernel/coresight/-/commits/coresight-byte-cntr-v1 >> >> Mao Jinlong (3): >> Coresight: Add driver to support for CSR >> coresight-tmc: byte-cntr: Add support for streaming interface for ETR >> dt-bindings: arm: Adds CoreSight CSR hardware definitions >> >> .../testing/sysfs-bus-coresight-devices-tmc | 7 + >> .../bindings/arm/qcom,coresight-csr.yaml | 62 ++++ >> drivers/hwtracing/coresight/Kconfig | 12 + >> drivers/hwtracing/coresight/Makefile | 3 +- >> .../hwtracing/coresight/coresight-byte-cntr.c | 304 ++++++++++++++++++ >> .../hwtracing/coresight/coresight-byte-cntr.h | 49 +++ >> drivers/hwtracing/coresight/coresight-csr.c | 168 ++++++++++ >> drivers/hwtracing/coresight/coresight-csr.h | 59 ++++ >> .../hwtracing/coresight/coresight-tmc-core.c | 66 ++++ >> .../hwtracing/coresight/coresight-tmc-etr.c | 8 +- >> drivers/hwtracing/coresight/coresight-tmc.h | 12 +- >> 11 files changed, 745 insertions(+), 5 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-csr.yaml >> create mode 100644 drivers/hwtracing/coresight/coresight-byte-cntr.c >> create mode 100644 drivers/hwtracing/coresight/coresight-byte-cntr.h >> create mode 100644 drivers/hwtracing/coresight/coresight-csr.c >> create mode 100644 drivers/hwtracing/coresight/coresight-csr.h >> >> -- >> 2.17.1 >> > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK > _______________________________________________ > CoreSight mailing list -- coresight@lists.linaro.org > To unsubscribe send an email to coresight-leave@lists.linaro.org