From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1C452C6FA8E for ; Thu, 2 Mar 2023 18:07:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:Date:References :In-Reply-To:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=WtXLSqVLZ0d6SgbAhaQS1pOvAKrwzNQEr7WwOlBRLMM=; b=EvHhlGj0hToM72 IqaUNY9wRQo7wqFqnLRQGTWDY6GVow2087xyn5chnFRTHjuLucAuPGtHZhCrCAN0jLl21QHmL+BiM ird89KMwOYcSg6r5njJneMRzS1bC3DSJQr8h82UqFs6J5pnWeV0sVAhC8crkO4q3WESb9XOqKuPEM H6jFxLIXUTzC+pVJJBlBkvqw8BUj1VGlGa15I4TS3S0sf8tRC9URdIwwvmjMXj7lQITRcBUR6YPrj Qzq5dANrd0xuCBYh3qWgBMDTvtXs42QyOS6lw3uP7Rcs6Ey6nxCHQtxrF1Al/ExWt2bc2qiPOq8kK uyE92NqLSZcIF5OLXR3Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pXnJ4-0031wL-JA; Thu, 02 Mar 2023 18:05:30 +0000 Received: from mga04.intel.com ([192.55.52.120]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pXnJ0-0031uz-QI for linux-arm-kernel@lists.infradead.org; Thu, 02 Mar 2023 18:05:28 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1677780326; x=1709316326; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=Bf3iWqZn8ZHJC8UqH7R7YhZdFebd0SSSAZOsxRPZafQ=; b=FAk1QV9I5smq7qqltcvC32EiSTBcJVaNOgVnBzp2jYsGMKhX7VAK0dFX 3v/xqU6+IFx/aCulek4YvO8yg0uZeKo+F35/4QGb9EzI6VZqFydfv61Gu A2RMLcCS/KneKZGniGbxEjCihK6mn900hcBYukuvHmXmPa0SLtRCZXKo8 iCpZcPpButjYNhuzl6F1bm+3BLze+ZCFiAPLnuEUa4AUBQO/G/KMz91hg VoT7+QwSfu0a/pxe64HSFgOaBwAyqqdIIXAZiq3DPA6WHhicBMNDn3x79 Cqq6PbxEKZwnQWZJMAtewKXW1U6rXDpprBLFsN3N7EvtOW8McLrumSbaX g==; X-IronPort-AV: E=McAfee;i="6500,9779,10637"; a="333522919" X-IronPort-AV: E=Sophos;i="5.98,228,1673942400"; d="scan'208";a="333522919" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Mar 2023 10:05:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10637"; a="1004222764" X-IronPort-AV: E=Sophos;i="5.98,228,1673942400"; d="scan'208";a="1004222764" Received: from ubik.fi.intel.com (HELO localhost) ([10.237.72.184]) by fmsmga005.fm.intel.com with ESMTP; 02 Mar 2023 10:05:20 -0800 From: Alexander Shishkin To: Mao Jinlong , Maxime Coquelin , Alexandre Torgue Cc: Mao Jinlong , 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 , Yuanfang Zhang , Tao Zhang , Hao Zhang , alexander.shishkin@linux.intel.com Subject: Re: [PATCH] stm: class: Add MIPI OST protocol support In-Reply-To: <20230208124053.18533-1-quic_jinlmao@quicinc.com> References: <20230208124053.18533-1-quic_jinlmao@quicinc.com> Date: Thu, 02 Mar 2023 20:05:20 +0200 Message-ID: <87lekfni5b.fsf@ubik.fi.intel.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230302_100526_939237_1F5681DF X-CRM114-Status: GOOD ( 19.48 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Mao Jinlong 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. > 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. > 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. > +#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. > + > +#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. > + */ > + /* 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. > + 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. > + *(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current()); Is there a reason why trc_hdr is not a struct? Thanks, -- Alex _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel