| Message ID | 1457418835-31417-2-git-send-email-zhang.chunyan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chunyan Zhang <zhang.chunyan@linaro.org> writes: > From: Mathieu Poirier <mathieu.poirier@linaro.org> > > Some architecture like ARM assign masterIDs at the HW design > phase. Those are therefore unreachable to users, making masterID > management in the generic STM core irrelevant. > > In this kind of configuration channels are shared between masters > rather than being allocated on a per master basis. > > This patch adds a new 'mshared' flag to struct stm_data that tells the > core that this specific STM device doesn't need explicit masterID > management. There are two kinds of 'masterIDs' that we're talking about here: the ones that turn up in the STP stream and the ones that are accessible to trace-side software (sw_start/sw_end). So in this case we want to reflect the fact that there is no correlation between the two, because hardware assigns STP channels dynamically based on the states of the trace/execution environment. And although the trace side software can do very little with this information, it does make sense to provide it. The sw_start==sw_end situation, on the other hand, is a side effect of the above and, as I said in one of the previous threads, may not even be the case, or at least I don't see why it has to. And when it is the case, I don't see the point in handling it differently from sw_start<sw_end situation. > In the core sw_start/end of masterID are set to '1', > i.e there is only one masterID to deal with. Why does this need to be done in the core and why '1'? IOW, sw_{start,end} come straight from the driver, this new 'mshared' comes straight from the driver, why do we need the core to modify the former based on the latter? > Also this patch depends on [1], so that the number of masterID > is '1' too. It's 7b3bb0e753 in Linus' tree, but I don't see the logical connection in the statement above. Regards, -- Alex
On 21 March 2016 at 01:47, Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > Chunyan Zhang <zhang.chunyan@linaro.org> writes: > >> From: Mathieu Poirier <mathieu.poirier@linaro.org> >> >> Some architecture like ARM assign masterIDs at the HW design >> phase. Those are therefore unreachable to users, making masterID >> management in the generic STM core irrelevant. >> >> In this kind of configuration channels are shared between masters >> rather than being allocated on a per master basis. >> >> This patch adds a new 'mshared' flag to struct stm_data that tells the >> core that this specific STM device doesn't need explicit masterID >> management. > > There are two kinds of 'masterIDs' that we're talking about here: the > ones that turn up in the STP stream and the ones that are accessible to > trace-side software (sw_start/sw_end). So in this case we want to > reflect the fact that there is no correlation between the two, because > hardware assigns STP channels dynamically based on the states of the > trace/execution environment. And although the trace side software can do > very little with this information, it does make sense to provide it. > > The sw_start==sw_end situation, on the other hand, is a side effect of > the above and, as I said in one of the previous threads, may not even be > the case, or at least I don't see why it has to. And when it is the > case, I don't see the point in handling it differently from > sw_start<sw_end situation. > >> In the core sw_start/end of masterID are set to '1', >> i.e there is only one masterID to deal with. > > Why does this need to be done in the core and why '1'? IOW, > sw_{start,end} come straight from the driver, this new 'mshared' comes > straight from the driver, why do we need the core to modify the former > based on the latter? The logic here was to account for drivers that only set the 'mshared' flag. One could (wrongly) assume that if mshared is set in the driver, sw_{start,end} are don't need to be set. That way errors are caught quicker. I'm fine with removing that part. Other than the above, is there anything you'd like to see modified in this patch, i.e how the mshared flag is used to modify the output in sysFS/configFS? Thanks, Mathieu > >> Also this patch depends on [1], so that the number of masterID >> is '1' too. > > It's 7b3bb0e753 in Linus' tree, but I don't see the logical connection > in the statement above. That statement should have appeared in the cover letter rather than this patch's blurb - that way it applies cleanly. > > Regards, > -- > Alex
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c index 66cec56..0d7f1c5 100644 --- a/drivers/hwtracing/stm/core.c +++ b/drivers/hwtracing/stm/core.c @@ -44,9 +44,15 @@ static ssize_t masters_show(struct device *dev, char *buf) { struct stm_device *stm = to_stm_device(dev); - int ret; + int ret, sw_start, sw_end; + + sw_start = stm->data->sw_start; + sw_end = stm->data->sw_end; + + if (stm->data->mshared) + sw_start = sw_end = STM_SHARED_MASTERID; - ret = sprintf(buf, "%u %u\n", stm->data->sw_start, stm->data->sw_end); + ret = sprintf(buf, "%d %d\n", sw_start, sw_end); return ret; } @@ -618,6 +624,13 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data, if (!stm_data->packet || !stm_data->sw_nchannels) return -EINVAL; + /* + * MasterIDs are assigned at HW design phase. As such the core is + * using a single master for interaction with this device. + */ + if (stm_data->mshared) + stm_data->sw_start = stm_data->sw_end = 1; + nmasters = stm_data->sw_end - stm_data->sw_start + 1; stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL); if (!stm) diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c index 17a1416..19455db 100644 --- a/drivers/hwtracing/stm/policy.c +++ b/drivers/hwtracing/stm/policy.c @@ -80,10 +80,17 @@ static ssize_t stp_policy_node_masters_show(struct config_item *item, char *page) { struct stp_policy_node *policy_node = to_stp_policy_node(item); + struct stm_device *stm = policy_node->policy->stm; + int first_master, last_master; ssize_t count; - count = sprintf(page, "%u %u\n", policy_node->first_master, - policy_node->last_master); + first_master = policy_node->first_master; + last_master = policy_node->last_master; + + if (stm && stm->data->mshared) + first_master = last_master = STM_SHARED_MASTERID; + + count = sprintf(page, "%d %d\n", first_master, last_master); return count; } @@ -106,6 +113,13 @@ stp_policy_node_masters_store(struct config_item *item, const char *page, if (!stm) goto unlock; + /* + * masterIDs are allocated in HW, no point in trying to + * change their values. + */ + if (stm->data->mshared) + goto unlock; + /* must be within [sw_start..sw_end], which is an inclusive range */ if (first > INT_MAX || last > INT_MAX || first > last || first < stm->data->sw_start || diff --git a/include/linux/stm.h b/include/linux/stm.h index 9d0083d..6fac8b1 100644 --- a/include/linux/stm.h +++ b/include/linux/stm.h @@ -18,6 +18,11 @@ #include <linux/device.h> /** + * The masterIDs are set in hardware and can't be queried + */ +#define STM_SHARED_MASTERID -1 + +/** * enum stp_packet_type - STP packets that an STM driver sends */ enum stp_packet_type { @@ -46,6 +51,8 @@ struct stm_device; * struct stm_data - STM device description and callbacks * @name: device name * @stm: internal structure, only used by stm class code + * @mshared: true if masterIDs are assigned in HW. If so @sw_start + * and @sw_end are set to '1' by the core. * @sw_start: first STP master available to software * @sw_end: last STP master available to software * @sw_nchannels: number of STP channels per master @@ -71,6 +78,7 @@ struct stm_device; struct stm_data { const char *name; struct stm_device *stm; + bool mshared; unsigned int sw_start; unsigned int sw_end; unsigned int sw_nchannels;