Re: [PATCH 4/5] coresight-stm: adding driver for CoreSight STM component

[Mathieu Poirier]

+ Al Grant

On 1 April 2015 at 08:27, Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> wrote:
> Adding Al Grant to the conversation - his knowledge on HW tracing for
> the ARM architecture is definitely an asset for this kind of planning.
> Please add him to future patchset as well.
>
> On 31 March 2015 at 09:04, Alexander Shishkin
> <alexander.shishkin@xxxxxxxxxxxxxxx> wrote:
>> Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> writes:
>>
>>> On 30 March 2015 at 08:04, Alexander Shishkin
>>> <alexander.shishkin@xxxxxxxxxxxxxxx> wrote:
>>>> As it looks from the above snippet, you're using a stream of DATA
>>>> packets for user's payload. I also noticed that you use an ioctl to
>>>> trigger timestamps.
>>>
>>> Right, the ioctl() conveys user space intentions on that channel.
>>> Options are kept and applied on every packet for as long as the
>>> channel is open.
>>
>> So this means that, for example, if you enable timestamps on a channel,
>> then every single data packet on that channel will be timestamped, which
>> is a lot of timestamps.
>
> That is how the original coresight-stm driver was implemented.  My
> initial goal was to upstream something that could be used as a
> conversation starter or a foundation to start building on.  I had
> planned to look into the protocol specification itself in later steps.
> But addressing the issue now is just as worthy.
>
>>Normally, you would only be interested in the
>> timestamp on the first data packet in the message (or frame or however
>> we decide to call it). This is one of the reasons why I'm suggesting a
>> common framing scheme or a "protocol".
>>
>>>> Now, in the STP protocol there are, for example, marked data packets
>>>> that can be used to mark beginning of a higher-level message,
>>>> timestamped data packets that can be used to mean the same thing and
>>>> FLAG packets to mark message boundaries.
>>>
>>> Same on my side, I simply haven't included them yet.  I'll do so in my
>>> next iteration.
>>>
>>>>
>>>> In my Intel TH code, I'm using D*TS packet for the beginning of a
>>>> message (or "frame") and FLAG packet for the the end of a message.
>
> By the way, are you following the OST specification of this is a
> scheme you came up with?
>
>>>>
>>>> So my question is, is there any specific STP framing pattern that you
>>>> use with Coresight STM or should we perhaps figure out a generic framing
>>>> pattern and make it part of the stm class as well?
>>>
>>> Now specific pattern... Sending a packet consists of MARK, DATA, FLAG.
>>
>> Is this pattern mandated by a decoder that you use or is there any other
>> reason why it's exactly that?
>
> The driver was following the OST specification, or something close to
> that.  I don't have access to the standard itself and as such not in a
> position to assert how accurate the implementation.  That is one of
> the reason I left it out of my patchset.
>
>>
>>>>
>>>> For example, we can replace stm's .write callback with something like
>>>>
>>>>     int (*packet)(struct stm_data *data,
>>>>                   unsigned int type,    /* data, flag, trig etc */
>>>>                   unsigned int options, /* timestamped, marked */
>>>>                   u64 payload);
>>>>
>>>> and let the stm core do the "framing", which, then, will be common and
>>>> consistent across different architectures/stm implementations.
>>>
>>> I think the framing should be left to individual drivers.  It's only a
>>> matter of time before we get a weird device that doesn't play well
>>> with others, forcing to carry the ugliness in the STM core rather than
>>> the driver.
>>
>> Not necessarily. If a device doesn't support one type of packet or the
>> other, it will be up to them to work around that in the above .packet
>> callback.
>>
>> As for the devices that don't play well, there's a question of how much
>> one can violate the spec and still call oneself compliant.
>
> I understand your point of view.  On my side I'm trying to avoid
> painting ourselves in the corner.
>
>>
>>> And isn't carrying "options" redundant?  Using "container_of" on the
>>> "data" field one can get back to the driver specific structure, which
>>> is definitely a better place to keep that information.  I think the
>>> general structure looks good right now, we simply need to find a way
>>> to get rid of the ioctls.
>>
>> No, what I mean by options here is a property of each individual packet,
>> not the whole channel. For example, if I want the underlying driver to
>> send a marked data packet, I do
>>
>>      stm_data->packet(stm_data, STP_PACKET_D8, STP_OPTION_MARKED, payload);
>>
>> or if I want to send a timestamped flag, I do
>>
>>      stm_data->packet(stm_data, STP_PACKET_FLAG, STP_OPTION_TS, 0);
>
> Ah!  It's getting clearer now.
>
>>
>> Like I said above, there seems little to be gained from enabling
>> timestamps for all packets in one channel.
>>
>>> Regarding the same "options", how did you plan on getting those from user space?
>>
>> Ideally, if we have a framing convension, we don't need to get it from
>> userspace at all, all userspace should care about is writing data to the
>> character device and we wrap it up and feed it to the underlying driver.
>
> What do you have in mind for "framing convention"?  As mentioned above
> codeAurora was using the OST specification but Al Grant tell me it
> isn't supported anymore.
>
> Thanks for the open dialogue,
> Mathieu
>
>>
>> Regards,
>> --
>> Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/