Message-ID: <d04262c0-af6c-0f56-eb50-4a1cf5c2146b@quicinc.com>
Date:   Tue, 7 Mar 2023 21:28:47 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH] stm: class: Add MIPI OST protocol support
Content-Language: en-US
From:   Jinlong Mao <quic_jinlmao@quicinc.com>
To:     Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>
CC:     <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>,
        Tingwei Zhang <quic_tingweiz@quicinc.com>,
        Yuanfang Zhang <quic_yuanfang@quicinc.com>,
        "Tao Zhang" <quic_taozha@quicinc.com>,
        Hao Zhang <quic_hazha@quicinc.com>
References: <20230208124053.18533-1-quic_jinlmao@quicinc.com>
 <87lekfni5b.fsf@ubik.fi.intel.com>
 <37ec6af5-f71b-7a92-9c59-1c89595382bc@quicinc.com>
In-Reply-To: <37ec6af5-f71b-7a92-9c59-1c89595382bc@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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