Messages in this thread | | | Date | Tue, 7 Mar 2023 21:26:19 +0800 | Subject | Re: [PATCH] stm: class: Add MIPI OST protocol support | From | Jinlong Mao <> |
| |
Hi Alexande,
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
| |