Re: [PATCH v6 2/2] Documentation: Add document for UltraSoc SMB drivers

[liuqi (BA)]

Hi Suzuki,

Thanks for your review.
On 2022/6/6 23:16, Suzuki K Poulose wrote:
> Hi Qi
>
> Please find my comments below.
>
> On 06/06/2022 14:02, Qi Liu wrote:
> > This patch bring in a documentation for UltraSoc SMB drivers.
> > It simple descripts the device, sysfs interface and the
> > firmware bindings.
> >
> > Signed-off-by: Qi Liu <liuqi115@xxxxxxxxxx>
> > ---
> >   .../trace/coresight/ultrasoc-smb.rst          | 80 +++++++++++++++++++
> >   1 file changed, 80 insertions(+)
> >   create mode 100644 Documentation/trace/coresight/ultrasoc-smb.rst
> >
> > diff --git a/Documentation/trace/coresight/ultrasoc-smb.rst 
> > b/Documentation/trace/coresight/ultrasoc-smb.rst
> > new file mode 100644
> > index 000000000000..b4cfea82deb8
> > --- /dev/null
> > +++ b/Documentation/trace/coresight/ultrasoc-smb.rst
> > @@ -0,0 +1,80 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +======================================
> > +UltraSoc - HW Assisted Tracing on SoC
> > +======================================
> > +   :Author:   Qi Liu <liuqi115@xxxxxxxxxx>
> > +   :Date:     March 2022
> > +
> > +Introduction
> > +------------
> > +
> > +UltraSoc SMB is a per SCCL(Super CPU Cluster) hardware, and it 
> > provides a way to buffer and store
>
> minor nit: alignment.
thanks, will pay attention to this.

> > +CPU trace messages in a region of shared system memory. SMB is 
> > plugged as
> > +a coresight sink device and the corresponding trace generators (ETM) are
> > +plugged in as source devices.
> > +
>
> --8>---
>
> > +Sysfs files and directories
> > +---------------------------
> > +
> > +The SMB devices appear on the existing coresight bus alongside the other
> > +coresight devices::
> > +
> > +    $# ls /sys/bus/coresight/devices/
> > +    ultra_smb0   ultra_smb1   ultra_smb2   ultra_smb3
> > +
> > +The ``ultra_smb<N>`` named SMB associated with SCCL.::
> > +
> > +    $# ls /sys/bus/coresight/devices/ultra_smb0
> > +    enable_sink   mgmt
> > +    $# ls /sys/bus/coresight/devices/ultra_smb0/mgmt
> > +    buf_size  buf_status  read_pos  write_pos
> > +
>
> These should also be placed in, with the appropriate format.
>
> Documentation/ABI/testing/sysfs-bus-coresight-devices-ultra_smb

got it, will add this document, thanks.
> > +*Key file items are:-*
> > +   * ``read_pos``: Shows the value held by UltraSoc SMB Read Pointer 
> > register.
> > +   * ``write_pos``: Shows the value held by UltraSoc SMB Write 
> > Pointer register.
> > +   * ``buf_status``: Shows the value held by UltraSoc SMB status 
> > register.
> > +             BIT(0) is zero means buffer is empty.
> > +   * ``buf_size``: Shows the buffer size of each UltraSoc SMB device.
> > +
> > +Firmware Bindings
> > +---------------------------
> > +
> > +Firmware binding of SMB device describes SMB device indentifier, 
> > resource
> > +information and graph structure.
>
> Please could you mention that this is only supported with ACPI here ?
sure, will add this next time.
> > +
> > +SMB is platform device and device id is "HISI03A1", resource of 
> > device is
> > +declared using the _CRS method. Each SMB must present two base address,
>
> Simply say, the device is identified by ACPI HID "HISI03A1".
ok, will modify this.
> > +the first one is the configuration base address of SMB device, the 
> > second
> > +one is the base address of shared system memory.
> > +
> > +examples::
> > +
> > +    Device(USMB) {                                               \
> > +      Name(_HID, "HISI03A1")                                     \
> > +      Name(_CRS, ResourceTemplate() {                            \
> > +          MEM_RESRC(0x95100000, 0x951FFFFF, 0x100000)            \
> > +          MEM_RESRC(0x50000000, 0x53FFFFFF, 0x4000000)           \
> > +      })                                                         \
> > +      Name(_DSD, Package() {                                     \
> > +        ToUUID("ab02a46b-74c7-45a2-bd68-f7d344ef2153"),          \
> > +    /* Use CoreSight Graph ACPI bindings to describe connections 
> > topology */
> > +        Package() {                                              \
> > +          0,                                                     \
> > +          1,                                                     \
> > +          Package() {                                            \
> > +            1,                                                   \
> > +            ToUUID("3ecbc8b6-1d0e-4fb3-8107-e627f805c6cd"),      \
> > +            8,                                                   \
> > +            Package() {0x8, 0, \_SB.S00.SL11.CL28.F008, 0},       \
> > +            Package() {0x9, 0, \_SB.S00.SL11.CL29.F009, 0},       \
> > +            Package() {0xa, 0, \_SB.S00.SL11.CL2A.F010, 0},       \
> > +            Package() {0xb, 0, \_SB.S00.SL11.CL2B.F011, 0},       \
> > +            Package() {0xc, 0, \_SB.S00.SL11.CL2C.F012, 0},       \
> > +            Package() {0xd, 0, \_SB.S00.SL11.CL2D.F013, 0},       \
> > +            Package() {0xe, 0, \_SB.S00.SL11.CL2E.F014, 0},       \
> > +            Package() {0xf, 0, \_SB.S00.SL11.CL2F.F015, 0},       \
> > +          }                                                      \
> > +        }                                                        \
> > +      })                                                         \
> > +    }
>
> Rest looks fine to me
>
> Suzuki

Thanks,
Qi
> .