Re: [PATCH v5 6/9] coresight: add support for CPU debug module
led.",);
>
> [...]
>
>> >+/*
>> >+ * Unfortunately the CPU cannot be powered up, so return
>> >+ * back and later has no permission to access other
>> >+ * registers. For this case, should set 'idle_constraint'
>> >+ * to ensure CPU power domain is enabled!
>> >+ */
>> >+if (!(drvdata->edprsr & EDPRSR_PU)) {
>> >+pr_err("%s: power up request for CPU%d failed\n",
>> >+__func__, drvdata->cpu);
>> >+goto out;
>> >+}
>> >+
>> >+out_powered_up:
>> >+debug_os_unlock(drvdata);
>>
>> Question: Do we need a matching debug_os_lock() once we are done ?
>
> I have checked ARM ARMv8, but there have no detailed description for
> this. I refered coresight-etmv4 code and Mike's pseudo code, ther have
> no debug_os_lock() related operations.
>
> Mike, Mathieu, could you also help confirm this?
>
Debug OS lock / unlock allows the power management code running on the
core to lock out the external debugger while the debug registers are
saved/restored during a core power event.
e.g. A sequence such as this might occur in a correctly programmed system
debug_os_lock()
save_debug_regs() // visible from core power domain - incl breakpoints etc
save_etm_regs()
... // other stuff prior to core power down,
Followed by...
restore_etm_regs()
restore_debug_regs() // visible from core power domain - incl breakpoints etc
debug_os_unlock()
The value is 1 (locked) if cold resetting into AArch64 - it is
expected that some system software will set this to 0 as part of the
boot process.
The lock prevents write access to the external debug registers so we
need to clear it to set up the external debug registers we are using.
This suggests that it should be restored as we found it when done.
Mike
> [...]
>
>> >+static void debug_init_arch_data(void *info)
>> >+{
>> >+struct debug_drvdata *drvdata = info;
>> >+u32 mode, pcsr_offset;
>> >+
>> >+CS_UNLOCK(drvdata->base);
>> >+
>> >+debug_os_unlock(drvdata);
>> >+
>> >+/* Read device info */
>> >+drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID);
>> >+drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
>>
>> As mentioned above, both of these registers are only need at init time to
>> figure out the flags we set here. So we could remove them.
>>
>> >+
>> >+CS_LOCK(drvdata->base);
>> >+
>> >+/* Parse implementation feature */
>> >+mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE;
>> >+pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
>>
>>
>> >+
>> >+if (mode == EDDEVID_IMPL_NONE) {
>> >+drvdata->edpcsr_present = false;
>> >+drvdata->edcidsr_present = false;
>> >+drvdata->edvidsr_present = false;
>> >+} else if (mode == EDDEVID_IMPL_EDPCSR) {
>> >+drvdata->edpcsr_present = true;
>> >+drvdata->edcidsr_present = false;
>> >+drvdata->edvidsr_present = false;
>> >+} else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
>> >+if (!IS_ENABLED(CONFIG_64BIT) &&
>> >+(pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
>> >+drvdata->edpcsr_present = false;
>> >+else
>> >+drvdata->edpcsr_present = true;
>>
>> Sorry, I forgot why we do this check only in this mode. Shouldn't this be
>> common to all modes (of course which implies PCSR is present) ?
>
> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
> simplize PCSROffset value :
> - Sample offset applies based on the instruction state (indicated by
> PCSR[0])
> 0001 - No offset applies.
> 0010 - No offset applies, but do not use in AArch32 mode!
>
> So we need handle the corner case is when CPU runs AArch32 mode and
> PCSRoffset = 'b0010. Other cases the pcsr should be present.
>
> [...]
>
> Other suggestions are good for me, will take them in next version.
>
> Thanks,
> Leo Yan
--
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 6/9] coresight: add support for CPU debug module
idle_constraint_fops);
>> > + if (!file) {
>> > + pr_err("%s: unable to create idle constraint file\n",
>> > __func__);
>> > + ret = -ENOMEM;
>>
>> Same as above.
>>
>> > + goto err;
>> > + }
>> > +
>> > + /* Use sysfs node to enable functionality */
>> > + if (!debug_enable)
>> > + return 0;
>> > +
>> > + /* Enable debug module at boot time */
>> > + ret = debug_enable_func();
>> > + if (ret) {
>> > + pr_err("%s: unable to disable debug function: %d\n",
>> > + __func__, ret);
>> > + goto err;
>> > + }
>>
>> Use the debug_lock to avoid race conditions.
>
> I'm struggling to understand what's race condition at here? The
> function pairs debug_func_init()/debug_func_exit() are used for
> module's probing and removing, so naturally module's probing and
> removing are sequential, right?
>
>> > +
>> > + return 0;
>> > +
>> > +err:
>> > + debugfs_remove_recursive(debug_debugfs_dir);
>> > + return ret;
>> > +}
>> > +
>> > +static void debug_func_exit(void)
>> > +{
>> > + debugfs_remove_recursive(debug_debugfs_dir);
>> > +
>> > + /* Disable functionality if has enabled */
>> > + if (debug_enable)
>> > + debug_disable_func();
>> > +}
>> > +
>> > +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
>> > +{
>> > + void __iomem *base;
>> > + struct device *dev = >dev;
>> > + struct debug_drvdata *drvdata;
>> > + struct resource *res = >res;
>> > + struct device_node *np = adev->dev.of_node;
>> > + int ret;
>> > +
>> > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> > + if (!drvdata)
>> > + return -ENOMEM;
>> > +
>> > + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
>> > + if (per_cpu(debug_drvdata, drvdata->cpu)) {
>> > + dev_err(dev, "CPU's drvdata has been initialized\n");
>>
>> Might be worth adding the CPU number in the error message.
>
> Yeah, will add it.
>
> [...]
>
>> This driver doesn't call the pm_runtime_put/get() operations needed to
>> handle the
>> debug power domain. See the other CoreSight drivers for details.
>
> Sure, will do it.
>
>> Also, from the conversation that followed the previous post we agreed that
>> we wouldn't
>> deal with CPUidle issues in this driver. We deal with the CPU power domain
>> using the EDPRCR register (like you did) and that's it. System that don't
>> honor that register
>> can use other (external) means to solve this. As such please remove the
>> pm_qos_xyz() functions.
>
> From previous discussion, Mike reminds the CPU power domain design is
> quite SoC specific and usually the SoC has many different low power
> states, e.g. except CPU level and cluster level low power states, they
> also can define SoC level low power states. Any SoC with any power
> state is possible finally impact CPU power domain, so this is why I add
> this interface to let user can have the final decision based on their
> working platform.
>
> We can rely on "nohlt" and "cpuidle.off=1" in kernel command line to
> disable whole SoC low power states at boot time; or we can use sysfs
> node "echo 1 > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disble" to
> disable CPU low power states at runtime. But that means we need use
> different interfaces to control CPU power domain for booting and
> runtime, it's not nice for usage.
>
> So this is why add "idle_constraint" as a central place to control
> power domain for CPU debug purpose and I also think this is more
> friendly for hardware design, e.g. some platforms can enable partial
> low power states to save power and avoid overheat after using this
> driver.
>
> How about you think for this?
>
> Thanks,
> Leo Yan
--
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
Hi, I think a quick clarification of the ARM hardware STM architecture may be of value here. The ARM hardware STM, when implemented as recommend in a hardware design, the master IDs are not under driver control, but have a hardwire association with source devices in the system. The master IDs are hardwired to individual cores and core security states, and there could be other IDs associated with hardware trace sources requiring output via the hardware STM. (an example of this is the ARM Juno development board). Therefore in multi-core systems many masters may be associated with a single software STM stream. A user-space application running on Core 1, may have a master ID of 0x11 in the STPv2 trace stream, but if this application is context switched and later continues running on Core 2, then master ID could change to 0x21. Masters identify a hardware source in this scheme, the precise values used will be implementation dependent. So the number of masters "available to the software" depends on your interpretation of the phrase. If you mean "master IDs on which software trace will appear" then that will be many - it depends on which core is running the application. However as described above, this is not predictable by any decoder, and the master set used may not be contiguous. If you mean "master IDs selectable/allocated by the driver" then that will be 0 - the driver has no control, and possibly no knowledge of which master is associated with the current execution context. (this is of course system dependent - it may well be possible to have some configuration database associating cores with hardware IDs, but this will be implementation and board support dependant). Therefore, there is a requirement to tell both the user-space STM client and decoder that not only is master ID management not needed - it is in fact not possible and the key to identify the software source for a given STM packet is the channel alone. In addition we have a nominal single Master ID "available to the software", to satisfy the requirements of the generic ST module API. So on Juno for example, while we do have 128 masters, each with 65536 channels, the master allocation is not visible to system software - each core sees a single master with 65536 channels. Therefore differentiation between software sources in the STM trace is by channel number allocations only. Best Regards Mike. ---- Mike Leach +44 (0)1254 893911 (Direct) Principal Engineer +44 (0)1254 893900 (Main) Arm Blackburn Design Centre +44 (0)1254 893901 (Fax) Belthorn House Walker Rdmailto:mike.le...@arm.com Guide Blackburn BB1 2QE > -Original Message- > From: Alexander Shishkin [mailto:alexander.shish...@linux.intel.com] > Sent: 05 February 2016 12:52 > To: Chunyan Zhang; mathieu.poir...@linaro.org > Cc: r...@kernel.org; broo...@kernel.org; prat...@codeaurora.org; > nicolas.gu...@st.com; cor...@lwn.net; Mark Rutland; Mike Leach; > t...@ti.com; Al Grant; zhang.l...@gmail.com; linux-ker...@vger.kernel.org; > linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; linux- > d...@vger.kernel.org > Subject: Re: [PATCH V2 3/6] stm class: provision for statically assigned > masterIDs > > Chunyan Zhang <zhang.chun...@linaro.org> writes: > > > From: Mathieu Poirier <mathieu.poir...@linaro.org> > > > > Some architecture like ARM assign masterIDs statically at the HW design > > phase, making masterID manipulation in the generic STM core irrelevant. > > > > This patch adds a new 'mstatic' flag to struct stm_data that tells the > > core that this specific STM device doesn't need explicit masterID > > management. > > So why do we need this patch? If your STM only has master 42 allocated > for software sources, simply set sw_start = 42, sw_end = 42 and you're > good to go, software will have exactly one channel to choose from. See > also the comment from : > > * @sw_start: first STP master available to software > * @sw_end: last STP master available to software > > particularly the "available to software" part. Any other kinds of > masters the STM class code doesn't care about (yet). > > > In the core sw_start/end of masterID are set to '1', > > i.e there is only one masterID to deal with. > > This is also a completely arbitrary and unnecessary requirement. Again, > you can set both to 42 and it will still work. > > > Also this patch depends on [1], so that the number of masterID > > is '1' too. > > > > Finally the lower and upper bound for masterIDs as presented &
