Messages in this thread | | | Date | Tue, 7 Mar 2023 21:28:47 +0800 | Subject | Re: [PATCH] stm: class: Add MIPI OST protocol support | From | Jinlong Mao <> |
| |
On 3/7/2023 9:26 PM, Jinlong Mao wrote:
> Hi Alexander, Sorry, correct the typo. > > On 3/3/2023 2:05 AM, Alexander Shishkin wrote: >> Mao Jinlong <quic_jinlmao@quicinc.com> writes: >> >>> Add MIPI OST protocol support for stm to format the traces. >> Missing an explanation of what OST is, what it's used for, how it is >> different from the SyS-T and others. > I will updae the explanation in next version. >> >>> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of >> You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver. > > The driver refers to code structure of p_sys-t driver. So, add this > comments after > internal review. > >> >>> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol >>> support"). >> Why is this significant? >> >>> diff --git a/drivers/hwtracing/stm/p_ost.c >>> b/drivers/hwtracing/stm/p_ost.c >>> new file mode 100644 >>> index 000000000000..2ca1a3fda57f >>> --- /dev/null >>> +++ b/drivers/hwtracing/stm/p_ost.c >>> @@ -0,0 +1,95 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/* >>> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit >>> d69d5e83110f >>> + * ("stm class: Add MIPI SyS-T protocol support"). >> Same as in the commit message. >> >> [...] >> >>> +#define OST_TOKEN_STARTSIMPLE (0x10) >>> +#define OST_VERSION_MIPI1 (0x10 << 8) >>> +#define OST_ENTITY_FTRACE (0x01 << 16) >>> +#define OST_CONTROL_PROTOCOL (0x0 << 24) >> These could use an explanation. > I will add the explanation. >>> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \ >>> + OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL) >> Does this mean that everything is ftrace? Because it's not. > Only ftrace is supported in p_ost now. Other header type will be added > later. >> >>> + >>> +#define STM_MAKE_VERSION(ma, mi) ((ma << 8) | mi) >>> +#define STM_HEADER_MAGIC (0x5953) >>> + >>> +static ssize_t notrace ost_write(struct stm_data *data, >>> + struct stm_output *output, unsigned int chan, >>> + const char *buf, size_t count) >>> +{ >>> + unsigned int c = output->channel + chan; >>> + unsigned int m = output->master; >>> + const unsigned char nil = 0; >>> + u32 header = DATA_HEADER; >>> + u8 trc_hdr[24]; >>> + ssize_t sz; >>> + >>> + /* >>> + * STP framing rules for OST frames: >>> + * * the first packet of the OST frame is marked; >>> + * * the last packet is a FLAG. >> Which in your case is also timestamped. > I will add the comments. >> >>> + */ >>> + /* Message layout: HEADER / DATA / TAIL */ >>> + /* HEADER */ >>> + >>> + sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED, >>> + 4, (u8 *)&header); >> The /* HEADER */ comment applies to the above line, so it should >> probably be directly before it. > Got it. >> >>> + if (sz <= 0) >>> + return sz; >>> + *(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3); >>> + *(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC; >>> + *(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id(); >>> + *(uint64_t *)(trc_hdr + 8) = sched_clock(); >> Why sched_clock()? It should, among other things, be called with >> interrupts disabled, which is not the case here. > I will check. If it is not necessary here, I will remove it. >> >>> + *(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current()); >> Is there a reason why trc_hdr is not a struct? > No particular reason here. >> >> Thanks, >> -- >> Alex
| |