From: Alexander Shishkin <alexander.shishkin@linux.intel.com> To: Mathieu Poirier <mathieu.poirier@linaro.org>, gregkh@linuxfoundation.org Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, zhang.chunyan@linaro.org, kaixu.xia@linaro.org, norbert.schulz@intel.com, peter.lachner@intel.com Subject: Re: [PATCH 4/5] coresight-stm: adding driver for CoreSight STM component Date: Mon, 30 Mar 2015 17:04:40 +0300 [thread overview] Message-ID: <87bnjad1jb.fsf@ashishki-desk.ger.corp.intel.com> (raw) In-Reply-To: <1425078294-13059-5-git-send-email-mathieu.poirier@linaro.org> Mathieu Poirier <mathieu.poirier@linaro.org> writes: > +static int stm_send(void *addr, const void *data, u32 size) > +{ > + u32 len = size; > + > + if (((unsigned long)data & 0x1) && (size >= 1)) { > + writeb_relaxed(*(u8 *)data, addr); > + data++; > + size--; > + } > + if (((unsigned long)data & 0x2) && (size >= 2)) { > + writew_relaxed(*(u16 *)data, addr); > + data += 2; > + size -= 2; > + } > + > + /* now we are 32bit aligned */ > + while (size >= 4) { > + writel_relaxed(*(u32 *)data, addr); > + data += 4; > + size -= 4; > + } > + > + if (size >= 2) { > + writew_relaxed(*(u16 *)data, addr); > + data += 2; > + size -= 2; > + } > + if (size >= 1) { > + writeb_relaxed(*(u8 *)data, addr); > + data++; > + size--; > + } > + > + return len; > +} > + > +static int stm_trace_data(unsigned long ch_addr, u32 options, > + const void *data, u32 size) > +{ > + void *addr; > + > + options &= ~STM_OPTION_TIMESTAMPED; > + addr = (void *)(ch_addr | stm_channel_off(STM_PKT_TYPE_DATA, options)); > + > + return stm_send(addr, data, size); > +} > + > +static inline int stm_trace_hw(u32 options, u32 channel, u8 entity_id, > + const void *data, u32 size) > +{ > + int len = 0; > + unsigned long ch_addr; > + struct stm_drvdata *drvdata = stmdrvdata; > + > + > + /* get the channel address */ > + ch_addr = (unsigned long)stm_channel_addr(drvdata, channel); > + > + if (drvdata->write_64bit) > + len = stm_trace_data_64bit(ch_addr, options, data, size); > + else > + /* send the payload data */ > + len = stm_trace_data(ch_addr, options, data, size); > + > + return len; > +} 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. 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. 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. 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? 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. > +static long stm_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > +{ > + u32 options; > + struct stm_node *node = file->private_data; > + > + switch (cmd) { > + case STM_IOCTL_SET_OPTIONS: > + if (copy_from_user(&options, (void __user *)arg, sizeof(u32))) > + return -EFAULT; > + > + options &= (STM_OPTION_TIMESTAMPED | STM_OPTION_GUARANTEED); > + node->options = options; > + break; > + case STM_IOCTL_GET_OPTIONS: > + options = node->options; > + if (copy_to_user((void __user *)arg, &options, sizeof(options))) > + return -EFAULT; > + break; > + default: > + return -EINVAL; > + }; > + > + return 0; > +} That way, we also won't need private ioctl()s, or at least, not for this reason. How do you feel about this? Regards, -- Alex
WARNING: multiple messages have this Message-ID (diff)
From: alexander.shishkin@linux.intel.com (Alexander Shishkin) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 4/5] coresight-stm: adding driver for CoreSight STM component Date: Mon, 30 Mar 2015 17:04:40 +0300 [thread overview] Message-ID: <87bnjad1jb.fsf@ashishki-desk.ger.corp.intel.com> (raw) In-Reply-To: <1425078294-13059-5-git-send-email-mathieu.poirier@linaro.org> Mathieu Poirier <mathieu.poirier@linaro.org> writes: > +static int stm_send(void *addr, const void *data, u32 size) > +{ > + u32 len = size; > + > + if (((unsigned long)data & 0x1) && (size >= 1)) { > + writeb_relaxed(*(u8 *)data, addr); > + data++; > + size--; > + } > + if (((unsigned long)data & 0x2) && (size >= 2)) { > + writew_relaxed(*(u16 *)data, addr); > + data += 2; > + size -= 2; > + } > + > + /* now we are 32bit aligned */ > + while (size >= 4) { > + writel_relaxed(*(u32 *)data, addr); > + data += 4; > + size -= 4; > + } > + > + if (size >= 2) { > + writew_relaxed(*(u16 *)data, addr); > + data += 2; > + size -= 2; > + } > + if (size >= 1) { > + writeb_relaxed(*(u8 *)data, addr); > + data++; > + size--; > + } > + > + return len; > +} > + > +static int stm_trace_data(unsigned long ch_addr, u32 options, > + const void *data, u32 size) > +{ > + void *addr; > + > + options &= ~STM_OPTION_TIMESTAMPED; > + addr = (void *)(ch_addr | stm_channel_off(STM_PKT_TYPE_DATA, options)); > + > + return stm_send(addr, data, size); > +} > + > +static inline int stm_trace_hw(u32 options, u32 channel, u8 entity_id, > + const void *data, u32 size) > +{ > + int len = 0; > + unsigned long ch_addr; > + struct stm_drvdata *drvdata = stmdrvdata; > + > + > + /* get the channel address */ > + ch_addr = (unsigned long)stm_channel_addr(drvdata, channel); > + > + if (drvdata->write_64bit) > + len = stm_trace_data_64bit(ch_addr, options, data, size); > + else > + /* send the payload data */ > + len = stm_trace_data(ch_addr, options, data, size); > + > + return len; > +} 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. 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. 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. 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? 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. > +static long stm_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > +{ > + u32 options; > + struct stm_node *node = file->private_data; > + > + switch (cmd) { > + case STM_IOCTL_SET_OPTIONS: > + if (copy_from_user(&options, (void __user *)arg, sizeof(u32))) > + return -EFAULT; > + > + options &= (STM_OPTION_TIMESTAMPED | STM_OPTION_GUARANTEED); > + node->options = options; > + break; > + case STM_IOCTL_GET_OPTIONS: > + options = node->options; > + if (copy_to_user((void __user *)arg, &options, sizeof(options))) > + return -EFAULT; > + break; > + default: > + return -EINVAL; > + }; > + > + return 0; > +} That way, we also won't need private ioctl()s, or at least, not for this reason. How do you feel about this? Regards, -- Alex
next prev parent reply other threads:[~2015-03-30 14:06 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-02-27 23:04 [PATCH 0/5] coresight: next Mathieu Poirier 2015-02-27 23:04 ` Mathieu Poirier 2015-02-27 23:04 ` [PATCH 1/5] coresight: making cpu index lookup arm64 compliant Mathieu Poirier 2015-02-27 23:04 ` Mathieu Poirier 2015-02-27 23:04 ` [PATCH 2/5] coresight: fixing compilation warnings picked up by 64bit compiler Mathieu Poirier 2015-02-27 23:04 ` Mathieu Poirier 2015-02-27 23:04 ` [PATCH 3/5] coresight: Adding coresight support for arm64 architecture Mathieu Poirier 2015-02-27 23:04 ` Mathieu Poirier 2015-02-27 23:04 ` [PATCH 4/5] coresight-stm: adding driver for CoreSight STM component Mathieu Poirier 2015-02-27 23:04 ` Mathieu Poirier 2015-03-07 12:27 ` Alexander Shishkin 2015-03-07 12:27 ` Alexander Shishkin 2015-03-30 14:04 ` Alexander Shishkin [this message] 2015-03-30 14:04 ` Alexander Shishkin 2015-03-30 15:48 ` Mathieu Poirier 2015-03-30 15:48 ` Mathieu Poirier 2015-03-31 15:04 ` Alexander Shishkin 2015-03-31 15:04 ` Alexander Shishkin 2015-04-01 14:27 ` Mathieu Poirier 2015-04-01 14:27 ` Mathieu Poirier 2015-04-01 14:28 ` Mathieu Poirier 2015-04-01 14:28 ` Mathieu Poirier 2015-02-27 23:04 ` [PATCH 5/5] coresight-stm: Bindings for System Trace Macrocell Mathieu Poirier 2015-02-27 23:04 ` Mathieu Poirier 2015-03-19 22:09 ` [PATCH 0/5] coresight: next Mathieu Poirier 2015-03-19 22:09 ` Mathieu Poirier 2015-03-19 22:24 ` Greg KH 2015-03-19 22:24 ` Greg KH
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87bnjad1jb.fsf@ashishki-desk.ger.corp.intel.com \ --to=alexander.shishkin@linux.intel.com \ --cc=gregkh@linuxfoundation.org \ --cc=kaixu.xia@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mathieu.poirier@linaro.org \ --cc=norbert.schulz@intel.com \ --cc=peter.lachner@intel.com \ --cc=zhang.chunyan@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.