Re: [PATCH v2 02/11] coresight: etm4x: Fix input validation for sysfs.
On 29/08/2019 22:33, Mike Leach wrote: A number of issues are fixed relating to sysfs input validation:- 1) bb_ctrl_store() - incorrect compare of bit select field to absolute value. Reworked per ETMv4 specification. 2) seq_event_store() - incorrect mask value - register has two event values. 3) cyc_threshold_store() - must mask with max before checking min otherwise wrapped values can set illegal value below min. 4) res_ctrl_store() - update to mask off all res0 bits. Reviewed-by: Leo Yan Reviewed-by: Mathieu Poirier Signed-off-by: Mike Leach Does this need to goto stable ? May be add a Fixes tag ? It fixes real issues with the values that could be programmed into these registers. Cheers Suzuki
Re: [PATCH v2 03/11] coresight: etm4x: Add missing API to set EL match on address filters
Hi Mike,
On 29/08/2019 22:33, Mike Leach wrote:
TRCACATRn registers have match bits for secure and non-secure exception
levels which are not accessible by the sysfs API.
This adds a new sysfs parameter to enable this - addr_exlevel_s_ns.
Looks good to me. Some minor nits below.
Signed-off-by: Mike Leach
---
.../coresight/coresight-etm4x-sysfs.c | 42 +++
1 file changed, 42 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index cc8156318018..b520f3c1521f 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -1233,6 +1233,47 @@ static ssize_t addr_context_store(struct device *dev,
}
static DEVICE_ATTR_RW(addr_context);
+static ssize_t addr_exlevel_s_ns_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ u8 idx;
+ unsigned long val;
+ struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
+ struct etmv4_config *config = &drvdata->config;
+
+ spin_lock(&drvdata->spinlock);
+ idx = config->addr_idx;
+ val = BMVAL(config->addr_acc[idx], 14, 8);
+ spin_unlock(&drvdata->spinlock);
+ return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t addr_exlevel_s_ns_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ u8 idx;
+ unsigned long val;
+ struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
+ struct etmv4_config *config = &drvdata->config;
+
+ if (kstrtoul(buf, 16, &val))
+ return -EINVAL;
Can this be 0 instead of 16 to accept any base ?
+
+ if (val & ~0x7F)
minor nit: Do we need to use (GENMASK(14, 8) >> 8) here instead of
hard coding the mask ?
+ return -EINVAL;
+
+ spin_lock(&drvdata->spinlock);
+ idx = config->addr_idx;
+ /* clear Exlevel_ns & Exlevel_s bits[14:12, 11:8] */
It may be worth adding a comment that bit[15] is RES0.
+ config->addr_acc[idx] &= ~(GENMASK(14, 8));
+ config->addr_acc[idx] |= (val << 8);
+ spin_unlock(&drvdata->spinlock);
+ return size;
+}
+static DEVICE_ATTR_RW(addr_exlevel_s_ns);
+
static ssize_t seq_idx_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -2038,6 +2079,7 @@ static struct attribute *coresight_etmv4_attrs[] = {
&dev_attr_addr_stop.attr,
&dev_attr_addr_ctxtype.attr,
&dev_attr_addr_context.attr,
+ &dev_attr_addr_exlevel_s_ns.attr,
&dev_attr_seq_idx.attr,
&dev_attr_seq_state.attr,
&dev_attr_seq_event.attr,
Either ways, irrespective of the above comments :
Reviewed-by: Suzuki K Poulose
Re: [PATCH v5 01/10] arm64: Provide a command line to disable spectre_v2 mitigation
On 28/02/2019 18:21, Catalin Marinas wrote:
On Thu, Feb 28, 2019 at 06:14:34PM +, Suzuki K Poulose wrote:
On 27/02/2019 01:05, Jeremy Linton wrote:
There are various reasons, including bencmarking, to disable spectrev2
mitigation on a machine. Provide a command-line to do so.
Signed-off-by: Jeremy Linton
Cc: Jonathan Corbet
Cc: linux-doc@vger.kernel.org
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 9950bb0cbd52..d2b2c69d31bb 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -220,6 +220,14 @@ static void qcom_link_stack_sanitization(void)
: "=&r" (tmp));
}
+static bool __nospectre_v2;
+static int __init parse_nospectre_v2(char *str)
+{
+ __nospectre_v2 = true;
+ return 0;
+}
+early_param("nospectre_v2", parse_nospectre_v2);
+
static void
enable_smccc_arch_workaround_1(const struct arm64_cpu_capabilities *entry)
{
@@ -231,6 +239,11 @@ enable_smccc_arch_workaround_1(const struct
arm64_cpu_capabilities *entry)
if (!entry->matches(entry, SCOPE_LOCAL_CPU))
return;
+ if (__nospectre_v2) {
+ pr_info_once("spectrev2 mitigation disabled by command line
option\n");
+ return;
+ }
+
Could we not disable the "cap" altogether instead, rather than disabling the
work around ? Or do we need that information ?
There are a few ideas here but I think we settled on always reporting in
sysfs even if the mitigation is disabled in .config. So I guess we need
the "cap" around for the reporting part.
Thanks Catalin.
Reviewed-by: Suzuki K Poulose
Re: [PATCH v5 01/10] arm64: Provide a command line to disable spectre_v2 mitigation
Hi Jeremy
On 27/02/2019 01:05, Jeremy Linton wrote:
There are various reasons, including bencmarking, to disable spectrev2
mitigation on a machine. Provide a command-line to do so.
Signed-off-by: Jeremy Linton
Cc: Jonathan Corbet
Cc: linux-doc@vger.kernel.org
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 9950bb0cbd52..d2b2c69d31bb 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -220,6 +220,14 @@ static void qcom_link_stack_sanitization(void)
: "=&r" (tmp));
}
+static bool __nospectre_v2;
+static int __init parse_nospectre_v2(char *str)
+{
+ __nospectre_v2 = true;
+ return 0;
+}
+early_param("nospectre_v2", parse_nospectre_v2);
+
static void
enable_smccc_arch_workaround_1(const struct arm64_cpu_capabilities *entry)
{
@@ -231,6 +239,11 @@ enable_smccc_arch_workaround_1(const struct
arm64_cpu_capabilities *entry)
if (!entry->matches(entry, SCOPE_LOCAL_CPU))
return;
+ if (__nospectre_v2) {
+ pr_info_once("spectrev2 mitigation disabled by command line
option\n");
+ return;
+ }
+
Could we not disable the "cap" altogether instead, rather than disabling the
work around ? Or do we need that information ?
Cheers
Suzuki
Re: [PATCH v9 2/2] ThunderX2, perf : Add Cavium ThunderX2 SoC UNCORE PMU driver
l;
+ struct hw_perf_event *hwc = &event->hw;
+ int idx = GET_COUNTERID(event);
+ int event_id = GET_EVENTID(event);
+
+ /* enable and start counters.
+* 8 bits for each counter, bits[05:01] of a counter to set event type.
+*/
+ val = reg_readl(hwc->config_base);
+ val &= ~DMC_EVENT_CFG(idx, 0x1f);
+ val |= DMC_EVENT_CFG(idx, event_id);
+ reg_writel(val, hwc->config_base);
+ local64_set(&hwc->prev_count, 0);
+ reg_writel(0, hwc->event_base);
+}
+
+static void uncore_stop_event_dmc(struct perf_event *event)
+{
+ u32 val;
+ struct hw_perf_event *hwc = &event->hw;
+ int idx = GET_COUNTERID(event);
+
+ /* clear event type(bits[05:01]) to stop counter */
+ val = reg_readl(hwc->config_base);
+ val &= ~DMC_EVENT_CFG(idx, 0x1f);
+ reg_writel(val, hwc->config_base);
+}
+
+static void tx2_uncore_event_update(struct perf_event *event)
+{
+ s64 prev, delta, new = 0;
+ struct hw_perf_event *hwc = &event->hw;
+ struct tx2_uncore_pmu *tx2_pmu;
+ enum tx2_uncore_type type;
+ u32prorate_factor;
+
+ tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+ type = tx2_pmu->type;
+ prorate_factor = tx2_pmu->prorate_factor;
+
+ new = reg_readl(hwc->event_base);
+ prev = local64_xchg(&hwc->prev_count, new);
+
+ /* handles rollover of 32 bit counter */
+ delta = (u32)(((1UL << 32) - prev) + new);
+
+ /* DMC event data_transfers granularity is 16 Bytes, convert it to 64 */
+ if (type == PMU_TYPE_DMC &&
+ GET_EVENTID(event) == DMC_EVENT_DATA_TRANSFERS)
+ delta = delta/4;
+
+ /* L3C and DMC has 16 and 8 interleave channels respectively.
+* The sampled value is for channel 0 and multiplied with
+* prorate_factor to get the count for a device.
+*/
+ local64_add(delta * prorate_factor, &event->count);
+}
+
+enum tx2_uncore_type get_tx2_pmu_type(struct acpi_device *adev)
+{
+ int i = 0;
+ struct acpi_tx2_pmu_device {
+ __u8 id[ACPI_ID_LEN];
+ enum tx2_uncore_type type;
+ } devices[] = {
+ {"CAV901D", PMU_TYPE_L3C},
+ {"CAV901F", PMU_TYPE_DMC},
+ {"", PMU_TYPE_INVALID}
+ };
+
+ while (devices[i].type != PMU_TYPE_INVALID) {
+ if (!strcmp(acpi_device_hid(adev), devices[i].id))
+ break;
+ i++;
+ }
+
+ return devices[i].type;
+}
+
+static bool tx2_uncore_validate_event(struct pmu *pmu,
+ struct perf_event *event, int *counters)
+{
+ if (is_software_event(event))
+ return true;
+ /* Reject groups spanning multiple HW PMUs. */
+ if (event->pmu != pmu)
+ return false;
+
+ *counters = *counters + 1;
+ return true;
nit: alignment.
Otherwise looks good to me.
FWIW, with the above nits fixed:
Reviewed-by: Suzuki K Poulose
Re: [PATCH v7 2/2] ThunderX2, perf : Add Cavium ThunderX2 SoC UNCORE PMU driver
Hi,
On 10/25/2018 06:59 AM, Kulkarni, Ganapatrao wrote:
This patch adds a perf driver for the PMU UNCORE devices DDR4 Memory
Controller(DMC) and Level 3 Cache(L3C). Each PMU supports up to 4
counters. All counters lack overflow interrupt and are
sampled periodically.
The patch looks OK overall. Some minor comments below.
Signed-off-by: Ganapatrao Kulkarni
---
drivers/perf/Kconfig | 9 +
drivers/perf/Makefile| 1 +
drivers/perf/thunderx2_pmu.c | 867 +++
include/linux/cpuhotplug.h | 1 +
4 files changed, 878 insertions(+)
create mode 100644 drivers/perf/thunderx2_pmu.c
diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 08ebaf7cca8b..c1956b1af2bb 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -87,6 +87,15 @@ config QCOM_L3_PMU
Adds the L3 cache PMU into the perf events subsystem for
monitoring L3 cache events.
+config THUNDERX2_PMU
+tristate "Cavium ThunderX2 SoC PMU UNCORE"
+ depends on ARCH_THUNDER2 && ARM64 && ACPI && NUMA
+default m
+ help
+ Provides support for ThunderX2 UNCORE events.
+ The SoC has PMU support in its L3 cache controller (L3C) and
+ in the DDR4 Memory Controller (DMC).
+
config XGENE_PMU
depends on ARCH_XGENE
bool "APM X-Gene SoC PMU"
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index b3902bd37d53..909f27fd9db3 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -7,5 +7,6 @@ obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
obj-$(CONFIG_HISI_PMU) += hisilicon/
obj-$(CONFIG_QCOM_L2_PMU) += qcom_l2_pmu.o
obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
+obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
new file mode 100644
index ..450d36c6a1e1
--- /dev/null
+++ b/drivers/perf/thunderx2_pmu.c
@@ -0,0 +1,867 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CAVIUM THUNDERX2 SoC PMU UNCORE
+ * Copyright (C) 2018 Cavium Inc.
+ * Author: Ganapatrao Kulkarni
+ */
+
+#include
+#include
+#include
+#include
+
+/* Each ThunderX2(TX2) Socket has a L3C and DMC UNCORE PMU device.
+ * Each UNCORE PMU device consists of 4 independent programmable counters.
+ * Counters are 32 bit and do not support overflow interrupt,
+ * they need to be sampled before overflow(i.e, at every 2 seconds).
+ */
+
+#define TX2_PMU_MAX_COUNTERS 4
+#define TX2_PMU_DMC_CHANNELS 8
+#define TX2_PMU_L3_TILES 16
+
+#define TX2_PMU_HRTIMER_INTERVAL (2 * NSEC_PER_SEC)
+#define GET_EVENTID(ev)((ev->hw.config) & 0x1ff)
+#define GET_COUNTERID(ev) ((ev->hw.idx) & 0xf)
If the number of counters are 4, should the mask be 0x3 instead ?
+#define DMC_EVENT_CFG(idx, val)((val) << (((idx) * 8) + 1))
If I understand this correctly, you are accommodating bits[5:1] of the
8bit counter by "+1". It is worth adding a comment about the bit
position here.
+
+#define L3C_COUNTER_CTL0xA8
+#define L3C_COUNTER_DATA 0xAC
+#define DMC_COUNTER_CTL0x234
+#define DMC_COUNTER_DATA 0x240
+
+/* L3C event IDs */
+#define L3_EVENT_READ_REQ 0xD
+#define L3_EVENT_WRITEBACK_REQ 0xE
+#define L3_EVENT_INV_N_WRITE_REQ 0xF
+#define L3_EVENT_INV_REQ 0x10
+#define L3_EVENT_EVICT_REQ 0x13
+#define L3_EVENT_INV_N_WRITE_HIT 0x14
+#define L3_EVENT_INV_HIT 0x15
+#define L3_EVENT_READ_HIT 0x17
+#define L3_EVENT_MAX 0x18
+
+/* DMC event IDs */
+#define DMC_EVENT_COUNT_CYCLES 0x1
+#define DMC_EVENT_WRITE_TXNS 0xB
+#define DMC_EVENT_DATA_TRANSFERS 0xD
+#define DMC_EVENT_READ_TXNS0xF
+#define DMC_EVENT_MAX 0x10
+
+enum tx2_uncore_type {
+ PMU_TYPE_L3C,
+ PMU_TYPE_DMC,
+ PMU_TYPE_INVALID,
+};
+
+/*
+ * pmu on each socket has 2 uncore devices(dmc and l3c),
+ * each device has 4 counters.
+ */
+struct tx2_uncore_pmu {
+ struct hlist_node hpnode;
+ struct list_head entry;
+ struct pmu pmu;
+ char *name;
+ int node;
+ int cpu;
+ u32max_counters;
+ u32prorate_factor;
+ u32max_events;
+ u64 hrtimer_interval;
+ void __iomem *base;
+ DECLARE_BITMAP(active_counters, TX2_PMU_MAX_COUNTERS);
+ struct perf_event *events[TX2_PMU_MAX_COUNTERS];
+ struct device *dev;
+ struct hrtimer hrtimer;
+ const struct attribute_group **attr_groups;
+ enum tx2_uncore_type type;
+ raw_spinlock_t lock;
+ void(*init_cntr_base)(struct perf_event *event,
+ struct tx2_uncore_pmu *tx2_pmu);
+ void(*stop_event)(struct
Re: [PATCH v6 2/2] ThunderX2: Add Cavium ThunderX2 SoC UNCORE PMU driver
Hi Ganpatrao,
On 11/10/18 17:06, Ganapatrao Kulkarni wrote:
On Thu, Oct 11, 2018 at 2:43 PM Suzuki K Poulose wrote:
Hi Ganapatrao,
On 11/10/18 07:39, Ganapatrao Kulkarni wrote:
+
+/*
+ * We must NOT create groups containing events from multiple hardware PMUs,
+ * although mixing different software and hardware PMUs is allowed.
+ */
+static bool thunderx2_uncore_validate_event_group(struct perf_event *event)
+{
+ struct pmu *pmu = event->pmu;
+ struct perf_event *leader = event->group_leader;
+ struct perf_event *sibling;
+ int counters = 0;
+
+ if (leader->pmu != event->pmu && !is_software_event(leader))
+ return false;
+
+ for_each_sibling_event(sibling, event->group_leader) {
+ if (is_software_event(sibling))
+ continue;
+ if (sibling->pmu != pmu)
+ return false;
+ counters++;
+ }
The above loop will not account for :
1) The leader event
2) The event that we are about to add.
So you need to allocate counters for both the above to make sure we can
accommodate the events.
Is leader also part of sibling list?
No.
not sure, what you are expecting here,
The purpose of the validate_group is to ensure if a group of events can
be scheduled together. That implies, if the new event is added to the
group, do we have enough counters to profile. So, you have :
1 (leader) + number-of-siblings-in-the-group + 1(this_new) events
But you only account for the number-of-siblings.
this function is inline with other perf driver code added in driver/perf
No, it is not. arm-cci, arm-dsu-pmu, arm-pmu, all do separate accounting
for leader, siblings and the "new event".
Are we guaranteed that the counter0 is always allocated ? What if the
event is deleted and there are other events active ? A better check
timer will be active/restarted, till last bit is cleared( till last
counter is released).
Ok, so you kick off the timer when you start the first active event,
which is assumed to be on the counter0 and the timeout handler would
restart the counters when at least one event is active, as you don't
have a PMU pmu_{enable/disable} call back. But I am still not convinced
that assuming the "first counter" is always 0. I would rather use:
bitmap_weight() == 1 check.
would be :
if (find_last_bit(...) != pmu_uncore->uncore_dev->max_counters)
IIUC, find_last_bit will return zero if none of the counters are
active and we want to start timer for first active counter.
Well, if that was the case, how do you know if the bit zero is the only
bit set ?
AFAICS, lib/find_bit.c has :
unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
{
if (size) {
unsigned long val = BITMAP_LAST_WORD_MASK(size);
unsigned long idx = (size-1) / BITS_PER_LONG;
do {
val &= addr[idx];
if (val)
return idx * BITS_PER_LONG + __fls(val);
val = ~0ul;
} while (idx--);
}
return size;
}
It returns "size" when it can't find a set bit.
before start, alloc_counter is called to set, it seems we never hit
case where at-least one bit is not set.
timer will be started for first bit set and will continue till all
bits are cleared.
+static enum hrtimer_restart thunderx2_uncore_hrtimer_callback(
+ struct hrtimer *hrt)
+{
+ struct thunderx2_pmu_uncore_channel *pmu_uncore;
+ unsigned long irqflags;
+ int idx;
+ bool restart_timer = false;
+
+ pmu_uncore = container_of(hrt, struct thunderx2_pmu_uncore_channel,
+ hrtimer);
+
+ raw_spin_lock_irqsave(&pmu_uncore->uncore_dev->lock, irqflags);
+ for_each_set_bit(idx, pmu_uncore->active_counters,
+ pmu_uncore->uncore_dev->max_counters) {
+ struct perf_event *event = pmu_uncore->events[idx];
Do we need to check if the "event" is not NULL ? Couldn't this race with
an event_del() operation ?
i dont think so, i have not come across any such race condition during
my testing.
Thinking about it further, we may not hit it, as the PMU is "disable"d,
before "add/del", which implies that the IRQ won't be triggered.
Taking another look, since you don't have pmu_disable/enable callbacks,
the events could still be running. However, you are protected by the
free_counter() which holds the uncore_dev->lock to prevent the race.
Btw, in general, not hitting a race condition doesn't prove there cannot
be a race ;)
want to avoid, unnecessary defensive programming.
Of course.
Suzuki
Re: [PATCH v6 2/2] ThunderX2: Add Cavium ThunderX2 SoC UNCORE PMU driver
Hi Ganapatrao,
On 11/10/18 07:39, Ganapatrao Kulkarni wrote:
+
+/*
+ * We must NOT create groups containing events from multiple hardware PMUs,
+ * although mixing different software and hardware PMUs is allowed.
+ */
+static bool thunderx2_uncore_validate_event_group(struct perf_event *event)
+{
+ struct pmu *pmu = event->pmu;
+ struct perf_event *leader = event->group_leader;
+ struct perf_event *sibling;
+ int counters = 0;
+
+ if (leader->pmu != event->pmu && !is_software_event(leader))
+ return false;
+
+ for_each_sibling_event(sibling, event->group_leader) {
+ if (is_software_event(sibling))
+ continue;
+ if (sibling->pmu != pmu)
+ return false;
+ counters++;
+ }
The above loop will not account for :
1) The leader event
2) The event that we are about to add.
So you need to allocate counters for both the above to make sure we can
accommodate the events.
Is leader also part of sibling list?
No.
+static void thunderx2_uncore_start(struct perf_event *event, int flags)
+{
+ struct hw_perf_event *hwc = &event->hw;
+ struct thunderx2_pmu_uncore_channel *pmu_uncore;
+ struct thunderx2_pmu_uncore_dev *uncore_dev;
+ unsigned long irqflags;
+
+ hwc->state = 0;
+ pmu_uncore = pmu_to_thunderx2_pmu_uncore(event->pmu);
+ uncore_dev = pmu_uncore->uncore_dev;
+
+ raw_spin_lock_irqsave(&uncore_dev->lock, irqflags);
+ uncore_dev->select_channel(event);
+ uncore_dev->start_event(event, flags);
+ raw_spin_unlock_irqrestore(&uncore_dev->lock, irqflags);
+
+ perf_event_update_userpage(event);
+
+ if (!find_last_bit(pmu_uncore->active_counters,
+ pmu_uncore->uncore_dev->max_counters)) {
Are we guaranteed that the counter0 is always allocated ? What if the
event is deleted and there are other events active ? A better check
would be :
if (find_last_bit(...) != pmu_uncore->uncore_dev->max_counters)
IIUC, find_last_bit will return zero if none of the counters are
active and we want to start timer for first active counter.
Well, if that was the case, how do you know if the bit zero is the only
bit set ?
AFAICS, lib/find_bit.c has :
unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
{
if (size) {
unsigned long val = BITMAP_LAST_WORD_MASK(size);
unsigned long idx = (size-1) / BITS_PER_LONG;
do {
val &= addr[idx];
if (val)
return idx * BITS_PER_LONG + __fls(val);
val = ~0ul;
} while (idx--);
}
return size;
}
It returns "size" when it can't find a set bit.
+ thunderx2_uncore_update(event);
+ hwc->state |= PERF_HES_UPTODATE;
+ }
+ raw_spin_unlock_irqrestore(&uncore_dev->lock, irqflags);
+}
+
+static int thunderx2_uncore_add(struct perf_event *event, int flags)
+{
+ struct hw_perf_event *hwc = &event->hw;
+ struct thunderx2_pmu_uncore_channel *pmu_uncore;
+ struct thunderx2_pmu_uncore_dev *uncore_dev;
+
+ pmu_uncore = pmu_to_thunderx2_pmu_uncore(event->pmu);
+ uncore_dev = pmu_uncore->uncore_dev;
+
+ /* Allocate a free counter */
+ hwc->idx = alloc_counter(pmu_uncore);
+ if (hwc->idx < 0)
+ return -EAGAIN;
+
+ pmu_uncore->events[hwc->idx] = event;
+ /* set counter control and data registers base address */
+ uncore_dev->init_cntr_base(event, uncore_dev);
+
+ hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
+ if (flags & PERF_EF_START)
+ thunderx2_uncore_start(event, flags);
+
+ return 0;
+}
+
+static void thunderx2_uncore_del(struct perf_event *event, int flags)
+{
+ struct thunderx2_pmu_uncore_channel *pmu_uncore =
+ pmu_to_thunderx2_pmu_uncore(event->pmu);
+ struct hw_perf_event *hwc = &event->hw;
+
+ thunderx2_uncore_stop(event, PERF_EF_UPDATE);
+
+ /* clear the assigned counter */
+ free_counter(pmu_uncore, GET_COUNTERID(event));
+
+ perf_event_update_userpage(event);
+ pmu_uncore->events[hwc->idx] = NULL;
+ hwc->idx = -1;
+}
+
+static void thunderx2_uncore_read(struct perf_event *event)
+{
+ unsigned long irqflags;
+ struct thunderx2_pmu_uncore_channel *pmu_uncore =
+ pmu_to_thunderx2_pmu_uncore(event->pmu);
+
+ raw_spin_lock_irqsave(&pmu_uncore->uncore_dev->lock, irqflags);
+ thunderx2_uncore_update(event);
+ raw_spin_unlock_irqrestore(&pmu_uncore->uncore_dev->lock, irqflags);
+}
+
+static enum hrtimer_restart thunderx2_uncore_hrtimer_callback(
+ struct hrtimer *hrt)
+{
+ struct thunderx2_pmu_uncore_channel *pmu_uncore;
+ unsigned long irqflags;
+ int idx;
+ bool restart_timer = false;
+
+ pmu_uncore = container_of(hrt, struct thunderx2_pmu_uncore_chann
Re: [PATCH v6 2/2] ThunderX2: Add Cavium ThunderX2 SoC UNCORE PMU driver
Hi Ganapatrao,
On 21/06/18 07:33, Ganapatrao Kulkarni wrote:
This patch adds a perf driver for the PMU UNCORE devices DDR4 Memory
Controller(DMC) and Level 3 Cache(L3C).
ThunderX2 has 8 independent DMC PMUs to capture performance events
corresponding to 8 channels of DDR4 Memory Controller and 16 independent
L3C PMUs to capture events corresponding to 16 tiles of L3 cache.
Each PMU supports up to 4 counters. All counters lack overflow interrupt
and are sampled periodically.
Signed-off-by: Ganapatrao Kulkarni
---
drivers/perf/Kconfig | 8 +
drivers/perf/Makefile| 1 +
drivers/perf/thunderx2_pmu.c | 949 +++
include/linux/cpuhotplug.h | 1 +
4 files changed, 959 insertions(+)
create mode 100644 drivers/perf/thunderx2_pmu.c
+
+/*
+ * DMC and L3 counter interface is muxed across all channels.
+ * hence we need to select the channel before accessing counter
+ * data/control registers.
+ *
+ * L3 Tile and DMC channel selection is through SMC call
+ * SMC call arguments,
+ * x0 = THUNDERX2_SMC_CALL_ID (Vendor SMC call Id)
+ * x1 = THUNDERX2_SMC_SET_CHANNEL (Id to set DMC/L3C channel)
+ * x2 = Node id
+ * x3 = DMC(1)/L3C(0)
+ * x4 = channel Id
+ */
+static void uncore_select_channel(struct perf_event *event)
+{
+ struct arm_smccc_res res;
+ struct thunderx2_pmu_uncore_channel *pmu_uncore =
+ pmu_to_thunderx2_pmu_uncore(event->pmu);
+ struct thunderx2_pmu_uncore_dev *uncore_dev =
+ pmu_uncore->uncore_dev;
+
+ arm_smccc_smc(THUNDERX2_SMC_CALL_ID, THUNDERX2_SMC_SET_CHANNEL,
+ uncore_dev->node, uncore_dev->type,
+ pmu_uncore->channel, 0, 0, 0, &res);
+ if (res.a0) {
+ dev_err(uncore_dev->dev,
+ "SMC to Select channel failed for PMU UNCORE[%s]\n",
+ pmu_uncore->uncore_dev->name);
+ }
+}
+
+static void uncore_start_event_l3c(struct perf_event *event, int flags)
+{
+ u32 val;
+ struct hw_perf_event *hwc = &event->hw;
+
+ /* event id encoded in bits [07:03] */
+ val = GET_EVENTID(event) << 3;
+ reg_writel(val, hwc->config_base);
+ local64_set(&hwc->prev_count, 0);
+ reg_writel(0, hwc->event_base);
+}
+
+static void uncore_stop_event_l3c(struct perf_event *event)
+{
+ reg_writel(0, event->hw.config_base);
+}
+
+static void uncore_start_event_dmc(struct perf_event *event, int flags)
+{
+ u32 val;
+ struct hw_perf_event *hwc = &event->hw;
+ int idx = GET_COUNTERID(event);
+ int event_type = GET_EVENTID(event);
+
+ /* enable and start counters.
+* 8 bits for each counter, bits[05:01] of a counter to set event type.
+*/
+ val = reg_readl(hwc->config_base);
+ val &= ~DMC_EVENT_CFG(idx, 0x1f);
+ val |= DMC_EVENT_CFG(idx, event_type);
+ reg_writel(val, hwc->config_base);
+ local64_set(&hwc->prev_count, 0);
+ reg_writel(0, hwc->event_base);
+}
+
+static void uncore_stop_event_dmc(struct perf_event *event)
+{
+ u32 val;
+ struct hw_perf_event *hwc = &event->hw;
+ int idx = GET_COUNTERID(event);
+
+ /* clear event type(bits[05:01]) to stop counter */
+ val = reg_readl(hwc->config_base);
+ val &= ~DMC_EVENT_CFG(idx, 0x1f);
+ reg_writel(val, hwc->config_base);
+}
+
+static void init_cntr_base_l3c(struct perf_event *event,
+ struct thunderx2_pmu_uncore_dev *uncore_dev)
+{
+ struct hw_perf_event *hwc = &event->hw;
+
+ /* counter ctrl/data reg offset at 8 */
+ hwc->config_base = (unsigned long)uncore_dev->base
+ + L3C_COUNTER_CTL + (8 * GET_COUNTERID(event));
+ hwc->event_base = (unsigned long)uncore_dev->base
+ + L3C_COUNTER_DATA + (8 * GET_COUNTERID(event));
+}
+
+static void init_cntr_base_dmc(struct perf_event *event,
+ struct thunderx2_pmu_uncore_dev *uncore_dev)
+{
+ struct hw_perf_event *hwc = &event->hw;
+
+ hwc->config_base = (unsigned long)uncore_dev->base
+ + DMC_COUNTER_CTL;
+ /* counter data reg offset at 0xc */
+ hwc->event_base = (unsigned long)uncore_dev->base
+ + DMC_COUNTER_DATA + (0xc * GET_COUNTERID(event));
+}
+
+static void thunderx2_uncore_update(struct perf_event *event)
+{
+ s64 prev, new = 0;
+ u64 delta;
+ struct hw_perf_event *hwc = &event->hw;
+ struct thunderx2_pmu_uncore_channel *pmu_uncore;
+ enum thunderx2_uncore_type type;
+
+ pmu_uncore = pmu_to_thunderx2_pmu_uncore(event->pmu);
+ type = pmu_uncore->uncore_dev->type;
+
+ pmu_uncore->uncore_dev->select_channel(event);
+
+ new = reg_readl(hwc->event_base);
+ prev = local64_xchg(&hwc->prev_count, new);
+
+ /* handles rollover of 32 bit counter */
+ delta = (u32)(((1UL << 32) - prev) + new);
+ local
Re: [PATCH v3] arm64: v8.4: Support for new floating point multiplication instructions
On 13/12/17 10:13, Dongjiu Geng wrote: ARM v8.4 extensions add new neon instructions for performing a multiplication of each FP16 element of one vector with the corresponding FP16 element of a second vector, and to add or subtract this without an intermediate rounding to the corresponding FP32 element in a third vector. This patch detects this feature and let the userspace know about it via a HWCAP bit and MRS emulation. Cc: Dave Martin Cc: Suzuki K Poulose Signed-off-by: Dongjiu Geng Reviewed-by: Dave Martin Looks good to me. Reviewed-by: Suzuki K Poulose -- 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: [RESEND PATCH] arm64: v8.4: Support for new floating point multiplication variant
Hi gengdongjiu Sorry for the late response. I have a similar patch to add the support for "FHM", which I was about to post it this week. On 11/12/17 13:29, Dave Martin wrote: On Mon, Dec 11, 2017 at 08:47:00PM +0800, gengdongjiu wrote: On 2017/12/11 19:59, Dave P Martin wrote: On Sat, Dec 09, 2017 at 03:28:42PM +, Dongjiu Geng wrote: ARM v8.4 extensions include support for new floating point multiplication variant instructions to the AArch64 SIMD Do we have any human-readable description of what the new instructions do? Since the v8.4 spec itself only describes these as "New Floating Point Multiplication Variant", I wonder what "FHM" actually stands for. Thanks for the point out. In fact, this feature only adds two instructions: FP16 * FP16 + FP32 FP16 * FP16 - FP32 The spec call this bit to ID_AA64ISAR0_EL1.FHM, I do not know why it will call "FHM", I think call it "FMLXL" may be better, which can stand for FMLAL/FMLSL instructions. Although "FHM" is cryptic, I think it makes sense to keep this as "FHM" to match the ISAR0 field name -- we've tended to follow this policy for other extension names unless there's a much better or more obvious name available. For "FMLXL", new instructions might be added in the future that match the same pattern, and then "FMLXL" could become ambiguous. So maybe this is not the best choice. I think the FHM stands for "FP Half precision Multiplication instructions". I vote for keeping the feature bit in sync with the register bit definition. i.e, FHM. However, my version of the patch names the HWCAP bit "asimdfml", following the compiler name for the feature option "fp16fml", which is not perfect either. I think FHM is the safe option here. Maybe something like "widening half-precision floating-point multiply accumulate" is acceptable wording consistent with the existing architecture, but I just made that up, so it's not official ;) how about something like "performing a multiplication of each FP16 element of one vector with the corresponding FP16 element of a second vector, and to add or subtract this without an intermediate rounding to the corresponding FP32 element in a third vector."? We could have that, I guess. I agree, and that matches the feature description. -- 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 v10 10/10] arm64: dts: juno: Add Coresight CPU debug nodes
On 19/05/17 18:44, Sudeep Holla wrote: Hi Suzuki, Leo, On 19/05/17 05:25, Leo Yan wrote: From: Suzuki K Poulose Add Coresight CPU debug nodes for Juno r0, r1 & r2. The CPU debug areas are mapped at the same address for all revisions, like the ETM, even though the CPUs have changed from r1 to r2. Cc: Sudeep Holla Cc: Leo Yan Cc: Mathieu Poirier Cc: Liviu Dudau Signed-off-by: Suzuki K Poulose ARM-SoC team expect DTS to be routed via platform tree. So I have queued this for v4.13[1] Sudeep Thanks for picking that up. Suzuki -- 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 v9 0/9] coresight: enable debug module
On Tue, May 09, 2017 at 10:49:53AM +0800, Leo Yan wrote:
> ARMv8 architecture reference manual (ARM DDI 0487A.k) Chapter H7 "The
> Sample-based Profiling Extension" has description for sampling
> registers, we can utilize these registers to check program counter
> value with combined CPU exception level, secure state, etc. So this is
> helpful for CPU lockup bugs, e.g. if one CPU has run into infinite loop
> with IRQ disabled; the 'hang' CPU cannot switch context and handle any
> interrupt, so it cannot handle SMP call for stack dump, etc.
>
> This patch series is to enable coresight debug module with sample-based
> registers and register call back notifier for PCSR register dumping
> when panic happens, so we can see below dumping info for panic; and
> this patch series has considered the conditions for access permission
> for debug registers self, so this can avoid access debug registers when
> CPU power domain is off; the driver also try to figure out the CPU is
> in secure or non-secure state.
>
> Patch 0001 is to document the dt binding; patch 0002 adds one detailed
> document to describe the Coresight debug module implementation, the
> clock and power domain impaction on the driver, some examples for usage.
>
> Patch 0003 is to document boot parameters used in kernel command line.
>
> Patch 0004 is to add file entries for MAINTAINERS.
>
> Patch 0005 is used to fix the func of_get_coresight_platform_data()
> doesn't properly drop the reference to the CPU node pointer; and
> patch 0006 is refactor to add new function of_coresight_get_cpu().
>
> Patch 0007 is the driver for CPU debug module.
>
> Patch 0008 in this series are to enable debug unit on 96boards Hikey,
> Patch 0009 is to enable debug on 96boards DB410c. Have verified on both
> two boards.
Leo,
Please could you include the following patch in your series, which adds
the DT nodes for CPU debug on Juno boards ?
8>
From: Suzuki K Poulose
Date: Tue, 28 Mar 2017 13:40:24 +0100
Subject: [PATCH] arm64: dts: juno: Add Coresight CPU debug nodes
Add Coresight CPU debug nodes for Juno r0, r1 & r2. The CPU
debug areas are mapped at the same address for all revisions,
like the ETM, even though the CPUs have changed from r1 to r2.
Cc: Sudeep Holla
Cc: Leo Yan
Cc: Mathieu Poirier
Cc: Liviu Dudau
Signed-off-by: Suzuki K Poulose
---
arch/arm64/boot/dts/arm/juno-base.dtsi | 54 ++
arch/arm64/boot/dts/arm/juno-r1.dts| 24 +++
arch/arm64/boot/dts/arm/juno-r2.dts| 24 +++
arch/arm64/boot/dts/arm/juno.dts | 24 +++
4 files changed, 126 insertions(+)
diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi
b/arch/arm64/boot/dts/arm/juno-base.dtsi
index df539e8..0613aed 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -216,6 +216,15 @@
};
};
+ cpu_debug0: cpu_debug@2201 {
+ compatible = "arm,coresight-cpu-debug", "arm,primecell";
+ reg = <0 0x2201 0 0x1000>;
+
+ clocks = <&soc_smc50mhz>;
+ clock-names = "apb_pclk";
+ power-domains = <&scpi_devpd 0>;
+ };
+
funnel@220c { /* cluster0 funnel */
compatible = "arm,coresight-funnel", "arm,primecell";
reg = <0 0x220c 0 0x1000>;
@@ -266,6 +275,15 @@
};
};
+ cpu_debug1: cpu_debug@2211 {
+ compatible = "arm,coresight-cpu-debug", "arm,primecell";
+ reg = <0 0x2211 0 0x1000>;
+
+ clocks = <&soc_smc50mhz>;
+ clock-names = "apb_pclk";
+ power-domains = <&scpi_devpd 0>;
+ };
+
etm2: etm@2304 {
compatible = "arm,coresight-etm4x", "arm,primecell";
reg = <0 0x2304 0 0x1000>;
@@ -280,6 +298,15 @@
};
};
+ cpu_debug2: cpu_debug@2301 {
+ compatible = "arm,coresight-cpu-debug", "arm,primecell";
+ reg = <0 0x2301 0 0x1000>;
+
+ clocks = <&soc_smc50mhz>;
+ clock-names = "apb_pclk";
+ power-domains = <&scpi_devpd 0>;
+ };
+
funnel@230c { /* cluster1 funnel */
compatible = "arm,coresight-funnel", "arm,primecell";
reg = <0 0x230c 0 0x1000>;
@@ -344,6 +371,15 @@
};
};
+ cpu_debug3: cpu_debug@2311 {
+ compatible = "arm,coresight-cpu-
Re: [PATCH v9 7/9] coresight: add support for CPU debug module
On 09/05/17 03:50, Leo Yan wrote: Coresight includes debug module and usually the module connects with CPU debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has description for related info in "Part H: External Debug". Chapter H7 "The Sample-based Profiling Extension" introduces several sampling registers, e.g. we can check program counter value with combined CPU exception level, secure state, etc. So this is helpful for analysis CPU lockup scenarios, e.g. if one CPU has run into infinite loop with IRQ disabled. In this case the CPU cannot switch context and handle any interrupt (including IPIs), as the result it cannot handle SMP call for stack dump. This patch is to enable coresight debug module, so firstly this driver is to bind apb clock for debug module and this is to ensure the debug module can be accessed from program or external debugger. And the driver uses sample-based registers for debug purpose, e.g. when system triggers panic, the driver will dump program counter and combined context registers (EDCIDSR, EDVIDSR); by parsing context registers so can quickly get to know CPU secure state, exception level, etc. Some of the debug module registers are located in CPU power domain, so this requires the CPU power domain stays on when access related debug registers, but the power management for CPU power domain is quite dependent on SoC integration for power management. For the platforms which with sane power controller implementations, this driver follows the method to set EDPRCR to try to pull the CPU out of low power state and then set 'no power down request' bit so the CPU has no chance to lose power. If the SoC has not followed up this design well for power management controller, the user should use the command line parameter or sysfs to constrain all or partial idle states to ensure the CPU power domain is enabled and access coresight CPU debug component safely. Signed-off-by: Leo Yan With comments from Mathieu addressed, Reviewed-by: Suzuki K Poulose -- 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 v6 6/8] coresight: add support for CPU debug module
On 19/04/17 15:28, Leo Yan wrote:
Hi Suzuki,
On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:
Hi Leo,
This version looks good to me. I have two minor comments below.
Thanks for reviewing. Will take the suggestions. Just check a bit for
last comment.
[...]
+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
+{
+ void __iomem *base;
+ struct device *dev = &adev->dev;
+ struct debug_drvdata *drvdata;
+ struct resource *res = &adev->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%d drvdata has been initialized\n",
+ drvdata->cpu);
May be we could warn about a possible issue in the DT ?
So can I understand the suggestion is to add warning in function
of_coresight_get_cpu() when cannot find CPU number, and here directly
bail out?
No. One could have single CPU DT, where he doesn't need to provide the CPU
number.
Hence, it doesn't make sense to WARN in of_coresight_get_cpu().
But when we hit the case above, we find that the some node was registered for
the given CPU (be it 0 or any other), which is definitely an error in DT. Due to
1) Hasn't specified the CPU number for more than one node
OR
2) CPU number duplicated in the more than one nodes.
Cheers
Suzuki
--
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 v6 6/8] coresight: add support for CPU debug module
On 06/04/17 14:30, Leo Yan wrote:
Coresight includes debug module and usually the module connects with CPU
debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
description for related info in "Part H: External Debug".
Chapter H7 "The Sample-based Profiling Extension" introduces several
sampling registers, e.g. we can check program counter value with
combined CPU exception level, secure state, etc. So this is helpful for
analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
loop with IRQ disabled. In this case the CPU cannot switch context and
handle any interrupt (including IPIs), as the result it cannot handle
SMP call for stack dump.
This patch is to enable coresight debug module, so firstly this driver
is to bind apb clock for debug module and this is to ensure the debug
module can be accessed from program or external debugger. And the driver
uses sample-based registers for debug purpose, e.g. when system triggers
panic, the driver will dump program counter and combined context
registers (EDCIDSR, EDVIDSR); by parsing context registers so can
quickly get to know CPU secure state, exception level, etc.
Some of the debug module registers are located in CPU power domain, so
this requires the CPU power domain stays on when access related debug
registers, but the power management for CPU power domain is quite
dependent on SoC integration for power management. For the platforms
which with sane power controller implementations, this driver follows
the method to set EDPRCR to try to pull the CPU out of low power state
and then set 'no power down request' bit so the CPU has no chance to
lose power.
If the SoC has not followed up this design well for power management
controller, the user should use the command line parameter or sysfs
to constrain all or partial idle states to ensure the CPU power
domain is enabled and access coresight CPU debug component safely.
Hi Leo,
This version looks good to me. I have two minor comments below.
+
+static struct notifier_block debug_notifier = {
+ .notifier_call = debug_notifier_call,
+};
+
+static int debug_enable_func(void)
+{
+ struct debug_drvdata *drvdata;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ drvdata = per_cpu(debug_drvdata, cpu);
+ if (!drvdata)
+ continue;
+
+ pm_runtime_get_sync(drvdata->dev);
+ }
+
+ return atomic_notifier_chain_register(&panic_notifier_list,
+ &debug_notifier);
+}
+
+static int debug_disable_func(void)
+{
+ struct debug_drvdata *drvdata;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ drvdata = per_cpu(debug_drvdata, cpu);
+ if (!drvdata)
+ continue;
+
+ pm_runtime_put(drvdata->dev);
+ }
+
+ return atomic_notifier_chain_unregister(&panic_notifier_list,
+ &debug_notifier);
+}
I believe you should, reverse the order of these operations in
debug_disable_func()
to prevent getting a panic notifier after we have released the power domain for
the
debug.
i.e, :
atomic_notifier_chain_unregister(...);
for_each_possible_cpu(cpu) {}
+
+static ssize_t debug_func_knob_write(struct file *f,
+ const char __user *buf, size_t count, loff_t *ppos)
+{
+ u8 val;
+ int ret;
+
+ ret = kstrtou8_from_user(buf, count, 2, &val);
+ if (ret)
+ return ret;
+
+ mutex_lock(&debug_lock);
+
+ if (val == debug_enable)
+ goto out;
+
+ if (val)
+ ret = debug_enable_func();
+ else
+ ret = debug_disable_func();
+
+ if (ret) {
+ pr_err("%s: unable to %s debug function: %d\n",
+ __func__, val ? "enable" : "disable", ret);
+ goto err;
+ }
+
+ debug_enable = val;
+out:
+ ret = count;
+err:
+ mutex_unlock(&debug_lock);
+ return ret;
+}
+
+static ssize_t debug_func_knob_read(struct file *f,
+ char __user *ubuf, size_t count, loff_t *ppos)
+{
+ ssize_t ret;
+ char buf[2];
+
+ mutex_lock(&debug_lock);
+
+ buf[0] = '0' + debug_enable;
+ buf[1] = '\n';
+ ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
+
+ mutex_unlock(&debug_lock);
+ return ret;
+}
+
+static const struct file_operations debug_func_knob_fops = {
+ .open = simple_open,
+ .read = debug_func_knob_read,
+ .write = debug_func_knob_write,
+};
+
+static int debug_func_init(void)
+{
+ struct dentry *file;
+ int ret;
+
+ /* Create debugfs node */
+ debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
+ if (!debug_debugfs_dir) {
+ pr_err("%s: unable to create debugfs directory\n", __func__);
+ return -ENO
Re: [PATCH v5 1/9] coresight: bindings for CPU debug module
On 25/03/17 18:23, Leo Yan wrote: According to ARMv8 architecture reference manual (ARM DDI 0487A.k) Chapter 'Part H: External debug', the CPU can integrate debug module and it can support self-hosted debug and external debug. Especially for supporting self-hosted debug, this means the program can access the debug module from mmio region; and usually the mmio region is integrated with coresight. So add document for binding debug component, includes binding to APB clock; and also need specify the CPU node which the debug module is dedicated to specific CPU. Suggested-by: Mike Leach Reviewed-by: Mathieu Poirier Signed-off-by: Leo Yan Reviewed-by: Suzuki K Poulose -- 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 4/9] coresight: refactor with function of_coresight_get_cpu
On 25/03/17 18:23, Leo Yan wrote:
This is refactor to add function of_coresight_get_cpu(), so it's used to
retrieve CPU id for coresight component. Finally can use it as a common
function for multiple places.
Suggested-by: Mathieu Poirier
Signed-off-by: Leo Yan
Reviewed-by: Suzuki K Poulose
- if (found)
- break;
- }
- of_node_put(dn);
-
- /* Affinity to CPU0 if no cpu nodes are found */
- pdata->cpu = found ? cpu : 0;
+ pdata->cpu = of_coresight_get_cpu(node);
return pdata;
}
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 2a5982c..bf96678 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -263,9 +263,11 @@ static inline int coresight_timeout(void __iomem *addr,
u32 offset,
#endif
#ifdef CONFIG_OF
+extern int of_coresight_get_cpu(struct device_node *node);
extern struct coresight_platform_data *of_get_coresight_platform_data(
struct device *dev, struct device_node *node);
#else
+static inline int of_coresight_get_cpu(struct device_node *node) { return 0; }
static inline struct coresight_platform_data *of_get_coresight_platform_data(
struct device *dev, struct device_node *node) { return NULL; }
#endif
--
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
On 30/03/17 02:03, Leo Yan wrote:
On Wed, Mar 29, 2017 at 03:56:23PM +0100, Mike Leach wrote:
[...]
+ /*
+* 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);
+
+ /*
+* At this point the CPU is powered up, so set the no powerdown
+* request bit so we don't lose power and emulate power down.
+*/
+ drvdata->edprsr = readl(drvdata->base + EDPRCR);
+ drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
If we are here the core is already up. Shouldn't we need to set
EDPRCR_CORENPDRQ only?
Yeah. Will fix.
No - EDPRCR_COREPURQ and EDPRCR_CORENPDRQ have different semantics and purposes
EDPRCR_COREPURQ is in the debug power domain an is tied to an external
debug request that should be an input to the external (to the PE)
system power controller.
The requirement is that the system power controller powers up the core
domain and does not power it down while it remains asserted.
EDPRCR_CORENPDRQ is in the core power domain and thus to the specific
core only. This ensures that any power control software running on
that core should emulate a power down if this is set to one.
I'm curious the exact meaning for "power control software".
Does it mean EDPRCR_CORENPDRQ should be checked by kernel or PSCI
liked code in ARM trusted firmware to avoid to run CPU power off flow?
Or will EDPRCR_CORENPDRQ assert CPU external signal to notify power
controller so power controller emulate a power down?
We cannot know the power control design of the system, so the safe
solution is to set both bits.
Thanks a lot for the suggestion. Will set both bits.
Leo,
Also, it would be good to restore the PRCR register back to the original state
after we read the registers (if we changed them). I am exploring ways to make
use of this feature on demand (e.g, tie it to sysrq-t or sysrq-l) and not just
at panic time. So it would be good to have the state restored to not affect the
normal functioning even after dumping the registers.
PS: I have a small hack to hook this into a sysrq-x at runtime, but on a second
thought
it is better not to consume the key and rather tie it to something which
already exist,
as mentioned above.
Thanks
Suzuki
Thanks,
Leo Yan
--
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
On 29/03/17 11:37, Leo Yan wrote:
On Wed, Mar 29, 2017 at 11:31:03AM +0100, Suzuki K Poulose wrote:
On 29/03/17 11:27, Leo Yan wrote:
On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:
[...]
+ 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.
I understand that reasoning. But my question is, why do we check for PCSROffset
only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode ==
EDDEVID_IMPL_EDPCSR or
any other mode where PCSR is present.
Sorry I misunderstood your question.
I made mistake when I analyzed the possbile combination for mode and
PCSROffset so I thought it's the only case should handle:
{ EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
Below three combinations are possible to exist; so you are right, I
should move this out for the checking:
{ EDDEVID_IMPL_NONE, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
That need not be covered, as IMPL_NONE says PCSR is not implemented hence you
don't worry about anything as the functionality is missing. This should rather
be:
EDDEVID_IMPL_EDPCSR, where only PCSR is implemented.
I think below combination doesn't really exist:
{ EDDEVID_IMPL_EDPCSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 };
EDDEVID_IMPL_EDPCSR is only defined in ARMv7 ARM, and
EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 is only defined in ARMv8 ARM.
It is not wrong to check the PCSROffset in all cases where PCSR is available,
as if
we hit PCSR on ARMv7 then PCSROffset shouldn't be DIS_AARCH32. And in fact that
would make the code a bit more cleaner. Anyways, I am not particular about this.
Suzuki
--
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
On 29/03/17 11:27, Leo Yan wrote:
On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:
[...]
+ 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.
I understand that reasoning. But my question is, why do we check for PCSROffset
only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode ==
EDDEVID_IMPL_EDPCSR or
any other mode where PCSR is present.
Sorry I misunderstood your question.
I made mistake when I analyzed the possbile combination for mode and
PCSROffset so I thought it's the only case should handle:
{ EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
Below three combinations are possible to exist; so you are right, I
should move this out for the checking:
{ EDDEVID_IMPL_NONE, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
That need not be covered, as IMPL_NONE says PCSR is not implemented hence you
don't worry about anything as the functionality is missing. This should rather
be:
EDDEVID_IMPL_EDPCSR, where only PCSR is implemented.
My switch...case suggestion makes it easier to do all this checking.
Thanks
Suzuki
--
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
On 29/03/17 04:07, Leo Yan wrote:
Hi Suzuki,
On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:
On 25/03/17 18:23, Leo Yan wrote:
[...]
Leo,
Thanks a lot for the quick rework. I don't fully understand (yet!) why we need
the
idle_constraint. I will leave it for Sudeep to comment on it, as he is the
expert
in that area. Some minor comments below.
Thanks a lot for quick reviewing :)
Signed-off-by: Leo Yan
---
drivers/hwtracing/coresight/Kconfig | 11 +
drivers/hwtracing/coresight/Makefile | 1 +
drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++
3 files changed, 716 insertions(+)
create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
diff --git a/drivers/hwtracing/coresight/Kconfig
b/drivers/hwtracing/coresight/Kconfig
index 130cb21..18d7931 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -89,4 +89,15 @@ config CORESIGHT_STM
logging useful software events or data coming from various entities
in the system, possibly running different OSs
+config CORESIGHT_CPU_DEBUG
+ tristate "CoreSight CPU Debug driver"
+ depends on ARM || ARM64
+ depends on DEBUG_FS
+ help
+ This driver provides support for coresight debugging module. This
+ is primarily used to dump sample-based profiling registers when
+ system triggers panic, the driver will parse context registers so
+ can quickly get to know program counter (PC), secure state,
+ exception level, etc.
May be we should mention/warn the user about the possible caveats of using
this feature to help him make a better decision ? And / Or we should add a
documentation
for it. We have collected some real good information over the discussions and
it is a good idea to capture it somewhere.
Sure, I will add a documentation for this.
[...]
+static struct pm_qos_request debug_qos_req;
+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
+module_param(idle_constraint, int, 0600);
+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU
"
+"idle states (default is -1, which means have no limiation "
+"to CPU idle states; 0 means disabling all idle states; user "
+"can choose other platform dependent values so can disable "
+"specific idle states for the platform)");
Correct me if I am wrong,
All we want to do is disable the CPUIdle explicitly if the user knows that this
could be a problem to use CPU debug on his platform. So, in effect, we should
only be using idle_constraint = 0 or -1.
In which case, we could make it easier for the user to tell us, either
0 - Don't do anything with CPUIdle (default)
1 - Disable CPUIdle for me as I know the platform has issues with CPU debug
and CPUidle.
The reason for not using bool flag is: usually SoC may have many idle
states, so if user wants to partially enable some states then can set
the latency to constraint.
But of course, we can change this to binary value as you suggested,
this means turn on of turn off all states. The only one reason to use
latency value is it is more friendly for hardware design, e.g. some
platforms can enable partial states to save power and avoid overheat
after using this driver.
If you guys think this is a bit over design, I will follow up your
suggestion. I also have some replying in Mathieu's reviewing, please
help review as well.
than explaining the miscrosecond latency etc and make the appropriate calls
underneath.
something like (not necessarily the same name) :
module_param(broken_with_cpuidle, bool, 0600);
MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues
with CPUIdle on"
" the platform. Non-zero value implies
CPUIdle has to be"
" explicitly disabled.",);
[...]
+ /*
+* 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?
[...]
+static void debug_init_arch_data(void *info)
+
Re: [PATCH v5 6/9] coresight: add support for CPU debug module
On 25/03/17 18:23, Leo Yan wrote: Coresight includes debug module and usually the module connects with CPU debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has description for related info in "Part H: External Debug". Chapter H7 "The Sample-based Profiling Extension" introduces several sampling registers, e.g. we can check program counter value with combined CPU exception level, secure state, etc. So this is helpful for analysis CPU lockup scenarios, e.g. if one CPU has run into infinite loop with IRQ disabled. In this case the CPU cannot switch context and handle any interrupt (including IPIs), as the result it cannot handle SMP call for stack dump. This patch is to enable coresight debug module, so firstly this driver is to bind apb clock for debug module and this is to ensure the debug module can be accessed from program or external debugger. And the driver uses sample-based registers for debug purpose, e.g. when system triggers panic, the driver will dump program counter and combined context registers (EDCIDSR, EDVIDSR); by parsing context registers so can quickly get to know CPU secure state, exception level, etc. Some of the debug module registers are located in CPU power domain, so this requires the CPU power domain stays on when access related debug registers, but the power management for CPU power domain is quite dependent on SoC integration for power management. For the platforms which with sane power controller implementations, this driver follows the method to set EDPRCR to try to pull the CPU out of low power state and then set 'no power down request' bit so the CPU has no chance to lose power. If the SoC has not followed up this design well for power management controller, the driver introduces module parameter "idle_constraint". Setting this parameter for latency requirement in microseconds, finally we can constrain all or partial idle states to ensure the CPU power domain is enabled, this is a backup method to access coresight CPU debug component safely. Leo, Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert in that area. Some minor comments below. Signed-off-by: Leo Yan --- drivers/hwtracing/coresight/Kconfig | 11 + drivers/hwtracing/coresight/Makefile | 1 + drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++ 3 files changed, 716 insertions(+) create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index 130cb21..18d7931 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -89,4 +89,15 @@ config CORESIGHT_STM logging useful software events or data coming from various entities in the system, possibly running different OSs +config CORESIGHT_CPU_DEBUG + tristate "CoreSight CPU Debug driver" + depends on ARM || ARM64 + depends on DEBUG_FS + help + This driver provides support for coresight debugging module. This + is primarily used to dump sample-based profiling registers when + system triggers panic, the driver will parse context registers so + can quickly get to know program counter (PC), secure state, + exception level, etc. May be we should mention/warn the user about the possible caveats of using this feature to help him make a better decision ? And / Or we should add a documentation for it. We have collected some real good information over the discussions and it is a good idea to capture it somewhere. + endif diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index af480d9..433d590 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \ coresight-etm4x-sysfs.o obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c new file mode 100644 index 000..fbec1d1 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c +#define EDPCSR 0x0A0 +#define EDCIDSR0x0A4 +#define EDVIDSR0x0A8 +#define EDPCSR_HI 0x0AC +#define EDOSLAR0x300 +#define EDPRCR 0x310 +#define EDPRSR 0x314 +#define EDDEVID1 0xFC4 +#define EDDEVID0xFC8 + +#define EDPCSR_PROHIBITED 0x + +/* b
Re: [PATCH] arm64: erratum: Workaround for Kryo reserved system register read
On 08/04/16 11:24, Marc Zyngier wrote: On 08/04/16 10:58, Suzuki K Poulose wrote: On 07/04/16 18:31, Marc Zyngier wrote: + All system register encodings above use the form + + Op0, Op1, CRn, CRm, Op2. + + Note that some of the encodings listed above include + the system register space reserved for the following + identification registers which may appear in future revisions + of the ARM architecture beyond ARMv8.0. + This space includes: + ID_AA64PFR[2-7]_EL1 + ID_AA64DFR[2-3]_EL1 + ID_AA64AFR[2-3]_EL1 + ID_AA64ISAR[2-7]_EL1 + ID_AA64MMFR[2-7]_EL1 AFAIK, the id space is unassigned. So the naming above could cause confusion if the register is named something else. It is reserved *at the moment*, but already has a defined behaviour. My Absolutely, they do need to be RAZ. My point was assigning names to the reserved space where the names are unassigned. worry is that when some new architecture revision comes around, we start using these registers without thinking much about it (because we should be able to). At this point, your SoC will catch fire and nobody will have a clue about the problem because it is not apparent in the code. I'd really like to see something a bit more forward looking that covers that space for good. I agree, the patch definitely needs to take care of handling the entire space. Cheers Suzuki -- 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] arm64: erratum: Workaround for Kryo reserved system register read
On 07/04/16 18:31, Marc Zyngier wrote: + All system register encodings above use the form + + Op0, Op1, CRn, CRm, Op2. + + Note that some of the encodings listed above include + the system register space reserved for the following + identification registers which may appear in future revisions + of the ARM architecture beyond ARMv8.0. + This space includes: + ID_AA64PFR[2-7]_EL1 + ID_AA64DFR[2-3]_EL1 + ID_AA64AFR[2-3]_EL1 + ID_AA64ISAR[2-7]_EL1 + ID_AA64MMFR[2-7]_EL1 AFAIK, the id space is unassigned. So the naming above could cause confusion if the register is named something else. + + check_local_cpu_errata(); + What is the impact of moving this around? Suzuki, was there any particular reason why this check was done later rather than earlier? All the existing errata look for MIDR to match, which is read separately using read_cpuid_id(). The moment we need to do something w.r.t an ID register, this will break. So at the moment moving this doesn't have much of an impact. Thanks Suzuki -- 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
