* [PATCH v6 6/8] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-04-19 17:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1491485461-22800-7-git-send-email-leo.yan@linaro.org>
On 6 April 2017 at 07:30, Leo Yan <leo.yan@linaro.org> 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 <leo.yan@linaro.org>
> ---
> drivers/hwtracing/coresight/Kconfig | 14 +
> drivers/hwtracing/coresight/Makefile | 1 +
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
> 3 files changed, 682 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..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ 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. Before use debugging functionality, platform
> + needs to ensure the clock domain and power domain are enabled
> + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> + for detailed description and the example for usage.
> +
> 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 0000000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2017 Linaro Limited. All rights reserved.
> + *
> + * Author: Leo Yan <leo.yan@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +#include <linux/amba/bus.h>
> +#include <linux/coresight.h>
> +#include <linux/cpu.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pm_qos.h>
> +#include <linux/slab.h>
> +#include <linux/smp.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "coresight-priv.h"
> +
> +#define EDPCSR 0x0A0
> +#define EDCIDSR 0x0A4
> +#define EDVIDSR 0x0A8
> +#define EDPCSR_HI 0x0AC
> +#define EDOSLAR 0x300
> +#define EDPRCR 0x310
> +#define EDPRSR 0x314
> +#define EDDEVID1 0xFC4
> +#define EDDEVID 0xFC8
> +
> +#define EDPCSR_PROHIBITED 0xFFFFFFFF
> +
> +/* bits definition for EDPCSR */
> +#define EDPCSR_THUMB BIT(0)
> +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
> +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
> +
> +/* bits definition for EDPRCR */
> +#define EDPRCR_COREPURQ BIT(3)
> +#define EDPRCR_CORENPDRQ BIT(0)
> +
> +/* bits definition for EDPRSR */
> +#define EDPRSR_DLK BIT(6)
> +#define EDPRSR_PU BIT(0)
> +
> +/* bits definition for EDVIDSR */
> +#define EDVIDSR_NS BIT(31)
> +#define EDVIDSR_E2 BIT(30)
> +#define EDVIDSR_E3 BIT(29)
> +#define EDVIDSR_HV BIT(28)
> +#define EDVIDSR_VMID GENMASK(7, 0)
> +
> +/*
> + * bits definition for EDDEVID1:PSCROffset
> + *
> + * NOTE: armv8 and armv7 have different definition for the register,
> + * so consolidate the bits definition as below:
> + *
> + * 0b0000 - Sample offset applies based on the instruction state, we
> + * rely on EDDEVID to check if EDPCSR is implemented or not
> + * 0b0001 - No offset applies.
> + * 0b0010 - No offset applies, but do not use in AArch32 mode
> + *
> + */
> +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
> +
> +/* bits definition for EDDEVID */
> +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> +#define EDDEVID_IMPL_EDPCSR (0x1)
> +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> +#define EDDEVID_IMPL_FULL (0x3)
> +
> +#define DEBUG_WAIT_SLEEP 1000
> +#define DEBUG_WAIT_TIMEOUT 32000
> +
> +struct debug_drvdata {
> + void __iomem *base;
> + struct device *dev;
> + int cpu;
> +
> + bool edpcsr_present;
> + bool edcidsr_present;
> + bool edvidsr_present;
> + bool pc_has_offset;
> +
> + u32 edpcsr;
> + u32 edpcsr_hi;
> + u32 edprsr;
> + u32 edvidsr;
> + u32 edcidsr;
> +};
> +
> +static DEFINE_MUTEX(debug_lock);
> +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> +static int debug_count;
> +static struct dentry *debug_debugfs_dir;
> +
> +static bool debug_enable;
> +module_param_named(enable, debug_enable, bool, 0600);
> +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> + "(default is 0, which means is disabled by default)");
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> + /* Unlocks the debug registers */
> + writel_relaxed(0x0, drvdata->base + EDOSLAR);
> + wmb();
> +}
> +
> +/*
> + * According to ARM DDI 0487A.k, before access external debug
> + * registers should firstly check the access permission; if any
> + * below condition has been met then cannot access debug
> + * registers to avoid lockup issue:
> + *
> + * - CPU power domain is powered off;
> + * - The OS Double Lock is locked;
> + *
> + * By checking EDPRSR can get to know if meet these conditions.
> + */
> +static bool debug_access_permitted(struct debug_drvdata *drvdata)
> +{
> + /* CPU is powered off */
> + if (!(drvdata->edprsr & EDPRSR_PU))
> + return false;
> +
> + /* The OS Double Lock is locked */
> + if (drvdata->edprsr & EDPRSR_DLK)
> + return false;
> +
> + return true;
> +}
> +
> +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
> +{
> + bool retried = false;
> + u32 edprcr;
> +
> +try_again:
> +
> + /*
> + * Send request to power management controller and assert
> + * DBGPWRUPREQ signal; if power management controller has
> + * sane implementation, it should enable CPU power domain
> + * in case CPU is in low power state.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + /* Wait for CPU to be powered up (timeout~=32ms) */
> + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
> + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
> + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
> + /*
> + * Unfortunately the CPU cannot be powered up, so return
> + * back and later has no permission to access other
> + * registers. For this case, should disable CPU low power
> + * states to ensure CPU power domain is enabled!
> + */
> + pr_err("%s: power up request for CPU%d failed\n",
> + __func__, drvdata->cpu);
> + return;
> + }
> +
> + /*
> + * 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.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> +
> + /* Bail out if CPU is powered up */
> + if (likely(drvdata->edprsr & EDPRSR_PU))
> + return;
> +
> + /*
> + * Handle race condition if CPU has been waken up but it sleeps
> + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
> + * waken flow one more time.
> + */
> + if (!retried) {
> + retried = true;
> + goto try_again;
> + }
> +}
> +
> +static void debug_read_regs(struct debug_drvdata *drvdata)
> +{
> + u32 save_edprcr;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Unlock os lock */
> + debug_os_unlock(drvdata);
> +
> + /* Save EDPRCR register */
> + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
> +
> + /*
> + * Ensure CPU power domain is enabled to let registers
> + * are accessiable.
> + */
> + debug_force_cpu_powered_up(drvdata);
> +
> + if (!debug_access_permitted(drvdata))
> + goto out;
> +
> + drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR);
> +
> + /*
> + * As described in ARM DDI 0487A.k, if the processing
> + * element (PE) is in debug state, or sample-based
> + * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF;
> + * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become
> + * UNKNOWN state. So directly bail out for this case.
> + */
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED)
> + goto out;
> +
> + /*
> + * A read of the EDPCSR normally has the side-effect of
> + * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI;
> + * at this point it's safe to read value from them.
> + */
> + if (IS_ENABLED(CONFIG_64BIT))
> + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> + if (drvdata->edcidsr_present)
> + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> +
> + if (drvdata->edvidsr_present)
> + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> +
> +out:
> + /* Restore EDPRCR register */
> + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
> +
> + CS_LOCK(drvdata->base);
> +}
> +
> +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
> +{
> + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> + unsigned long pc;
> +
> + if (IS_ENABLED(CONFIG_64BIT))
> + return (unsigned long)drvdata->edpcsr_hi << 32 |
> + (unsigned long)drvdata->edpcsr;
> +
> + pc = (unsigned long)drvdata->edpcsr;
> +
> + if (drvdata->pc_has_offset) {
> + arm_inst_offset = 8;
> + thumb_inst_offset = 4;
> + }
> +
> + /* Handle thumb instruction */
> + if (pc & EDPCSR_THUMB) {
> + pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset;
> + return pc;
> + }
> +
> + /*
> + * Handle arm instruction offset, if the arm instruction
> + * is not 4 byte alignment then it's possible the case
> + * for implementation defined; keep original value for this
> + * case and print info for notice.
> + */
> + if (pc & BIT(1))
> + pr_emerg("Instruction offset is implementation defined\n");
> + else
> + pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset;
> +
> + return pc;
> +}
> +
> +static void debug_dump_regs(struct debug_drvdata *drvdata)
> +{
> + unsigned long pc;
> +
> + pr_emerg("\tEDPRSR: %08x (Power:%s DLK:%s)\n", drvdata->edprsr,
> + drvdata->edprsr & EDPRSR_PU ? "On" : "Off",
> + drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock");
> +
> + if (!debug_access_permitted(drvdata)) {
> + pr_emerg("No permission to access debug registers!\n");
> + return;
> + }
> +
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED) {
> + pr_emerg("CPU is in Debug state or profiling is prohibited!\n");
> + return;
> + }
> +
> + pc = debug_adjust_pc(drvdata);
> + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> +
> + if (drvdata->edcidsr_present)
> + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> +
> + if (drvdata->edvidsr_present)
> + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits VMID:%x)\n",
> + drvdata->edvidsr,
> + drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure",
> + drvdata->edvidsr & EDVIDSR_E3 ? "EL3" :
> + (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"),
> + drvdata->edvidsr & EDVIDSR_HV ? 64 : 32,
> + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> +}
> +
> +static void debug_init_arch_data(void *info)
> +{
> + struct debug_drvdata *drvdata = info;
> + u32 mode, pcsr_offset;
> + u32 eddevid, eddevid1;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Read device info */
> + eddevid = readl_relaxed(drvdata->base + EDDEVID);
> + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> +
> + CS_LOCK(drvdata->base);
> +
> + /* Parse implementation feature */
> + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
> + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> +
> + drvdata->edpcsr_present = false;
> + drvdata->edcidsr_present = false;
> + drvdata->edvidsr_present = false;
> + drvdata->pc_has_offset = false;
> +
> + switch (mode) {
> + case EDDEVID_IMPL_FULL:
> + drvdata->edvidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
> + drvdata->edcidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR:
> + /*
> + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
> + * define if has the offset for PC sampling value; if read
> + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
> + * module does not sample the instruction set state when
> + * armv8 CPU in AArch32 state.
> + */
> + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
> + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
Following my previous comment in patch 2/6 this condition needs to be
reviewed to deal with conditions where pcsr_offset == 0 on AArch64.
Probably
drvdata->edpcsr_present = ((IS_ENABLED(CONFIG_64BIT) &&
pcsr_offset != 0)) ||
(pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
> +
> + drvdata->pc_has_offset =
> + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * Dump out information on panic.
> + */
> +static int debug_notifier_call(struct notifier_block *self,
> + unsigned long v, void *p)
> +{
> + int cpu;
> + struct debug_drvdata *drvdata;
> +
> + pr_emerg("ARM external debug module:\n");
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pr_emerg("CPU[%d]:\n", drvdata->cpu);
> +
> + debug_read_regs(drvdata);
> + debug_dump_regs(drvdata);
> + }
> +
> + return 0;
> +}
> +
> +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);
> +}
> +
> +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 -ENOMEM;
> + }
> +
> + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> + debug_debugfs_dir, NULL, &debug_func_knob_fops);
> + if (!file) {
> + pr_err("%s: unable to create enable knob file\n", __func__);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Use sysfs node to enable functionality */
> + if (!debug_enable)
> + return 0;
> +
> + /* Register function to be called for panic */
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + debugfs_remove_recursive(debug_debugfs_dir);
> + return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> + debugfs_remove_recursive(debug_debugfs_dir);
> +
> + /* Unregister panic notifier callback */
> + if (debug_enable)
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +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);
> + return -EBUSY;
> + }
> +
> + drvdata->dev = &adev->dev;
> + amba_set_drvdata(adev, drvdata);
> +
> + /* Validity for the resource is already checked by the AMBA core */
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + drvdata->base = base;
> +
> + get_online_cpus();
> + per_cpu(debug_drvdata, drvdata->cpu) = drvdata;
> + ret = smp_call_function_single(drvdata->cpu,
> + debug_init_arch_data, drvdata, 1);
> + put_online_cpus();
> +
> + if (ret) {
> + dev_err(dev, "CPU%d debug arch init failed\n", drvdata->cpu);
> + goto err;
> + }
> +
> + if (!drvdata->edpcsr_present) {
> + dev_err(dev, "CPU%d sample-based profiling isn't implemented\n",
> + drvdata->cpu);
> + ret = -ENXIO;
> + goto err;
> + }
> +
> + if (!debug_count++) {
> + ret = debug_func_init();
> + if (ret)
> + goto err_func_init;
> + }
> +
> + if (!debug_enable)
> + pm_runtime_put(dev);
> +
> + dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
> + return 0;
> +
> +err_func_init:
> + debug_count--;
> +err:
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> + return ret;
> +}
> +
> +static int debug_remove(struct amba_device *adev)
> +{
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata = amba_get_drvdata(adev);
> +
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> +
> + if (debug_enable)
> + pm_runtime_put(dev);
> +
> + if (!--debug_count)
> + debug_func_exit();
> +
> + return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> + { /* Debug for Cortex-A53 */
> + .id = 0x000bbd03,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A57 */
> + .id = 0x000bbd07,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A72 */
> + .id = 0x000bbd08,
> + .mask = 0x000fffff,
> + },
> + { 0, 0 },
> +};
> +
> +static struct amba_driver debug_driver = {
> + .drv = {
> + .name = "coresight-cpu-debug",
> + .suppress_bind_attrs = true,
> + },
> + .probe = debug_probe,
> + .remove = debug_remove,
> + .id_table = debug_ids,
> +};
> +
> +module_amba_driver(debug_driver);
> +
> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
> +MODULE_DESCRIPTION("ARM Coresight CPU Debug Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH] memory: atmel-ebi: mark PM ops as __maybe_unused
From: Alexandre Belloni @ 2017-04-19 17:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170419174824.4134087-1-arnd@arndb.de>
On 19/04/2017 at 19:48:07 +0200, Arnd Bergmann wrote:
> We get a harmless warning without CONFIG_PM:
>
> drivers/memory/atmel-ebi.c:584:12: error: 'atmel_ebi_resume' defined but not used [-Werror=unused-function]
>
> Marking the function as __maybe_unused does the right thing here
> and drops it silently when unused.
>
> Fixes: a483fb10e5ea ("memory: atmel-ebi: Add PM ops")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Sure,
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
I think you can safely take it through arm-soc directly.
> ---
> drivers/memory/atmel-ebi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
> index 35910f945bfa..99e644cda4d1 100644
> --- a/drivers/memory/atmel-ebi.c
> +++ b/drivers/memory/atmel-ebi.c
> @@ -581,7 +581,7 @@ static int atmel_ebi_probe(struct platform_device *pdev)
> return of_platform_populate(np, NULL, NULL, dev);
> }
>
> -static int atmel_ebi_resume(struct device *dev)
> +static __maybe_unused int atmel_ebi_resume(struct device *dev)
> {
> struct atmel_ebi *ebi = dev_get_drvdata(dev);
> struct atmel_ebi_dev *ebid;
> --
> 2.9.0
>
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT
From: Florian Fainelli @ 2017-04-19 17:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cd324440-ad88-0981-1577-822d39ee289d@gmail.com>
On 04/19/2017 10:35 AM, Rafa? Mi?ecki wrote:
> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>> On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>
>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>> attached.
>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>> yet).
>>>>>
>>>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>> ---
>>>>> arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>> 1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> @@ -320,6 +320,13 @@
>>>>> };
>>>>> };
>>>>>
>>>>> + mdio at 18003000 {
>>>>> + compatible = "brcm,iproc-mdio";
>>>>> + reg = <0x18003000 0x8>;
>>>>> + #size-cells = <1>;
>>>>> + #address-cells = <0>;
>>>>> + };
>>>>
>>>> This looks fine, but usually the block should be enabled on a per-board
>>>> basis, such that there should be a status = "disabled" property here by
>>>> default.
>>>
>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>> it's
>>> for stuff that is always present on every SoC family board: rng, nand,
>>> spi to
>>> name few.
>>>
>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>> controller so it's enabled by default. Not every board has SPI flash, so
>>> it's
>>> disabled by default.
>>>
>>> It's there and it make sense to me. Is that OK or not?
>>
>> Even though there are devices that are always enabled on a given SoC,
>> because the board designs are always consistent does not necessarily
>> make them good candidates to be enabled at the .dtsi level. This is
>> particularly true when there are external connections to blocks (SPI,
>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>> default is safer as a starting point to begin with.
>
> In case of Northstar there is USB 3.0 PHY connected *internally* to this
> MDIO.
> I don't think any board manufacturer is able to rip SoC out of the MDIO
> or the
> USB 3.0 PHY.
OK, then can you still resubmit a proper patch that a) puts that
information in the commit message, and b) also adds a proper label to
the mdio node such that it can later on be referenced by label in
board-level DTS files? By that I mean:
mdio: mdio at 18003000 {
Thank you
>
>
>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>> MDIO bus
>>> just devices may differ and should not be enabled by default.
>>
>> In which case, the only difference, for you would be to do to, at the
>> board-level DTS:
>>
>> &mdio {
>> status = "okay";
>>
>> phy at 0 {
>> reg = <0>;
>> ...
>> };
>> };
>>
>> versus:
>>
>> &mdio {
>> phy at 0 {
>> reg = <0>;
>> ...
>> };
>> };
>>
>> I think we can afford putting the mdio node's status property in each
>> board-level DTS and make it clear that way that it is enabled because
>> there are child nodes enabled?
>
> This will be a pretty big effort because every Northstar device I know
> has USB
> 3.0 PHY in the SoC.
Adding a one liner is a "pretty big effort", for sure.
>
>
>> NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
>> devices because it relies on child nodes being declared, but you would
>> still get the driver to be probed and enabled, which is a waste of
>> resources at best.
>
> Right, but DT role is to describe device/board and not really care if
> operating
> system handles that efficiently.
--
Florian
^ permalink raw reply
* [patch V2 11/24] ARM/hw_breakpoint: Use cpuhp_setup_state_cpuslocked()
From: Mark Rutland @ 2017-04-19 17:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418170553.274229815@linutronix.de>
Hi,
On Tue, Apr 18, 2017 at 07:04:53PM +0200, Thomas Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> arch_hw_breakpoint_init() holds get_online_cpus() while registerring the
> hotplug callbacks.
>
> cpuhp_setup_state() invokes get_online_cpus() as well. This is correct, but
> prevents the conversion of the hotplug locking to a percpu rwsem.
>
> Use cpuhp_setup_state_cpuslocked() to avoid the nested call.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-arm-kernel at lists.infradead.org
>
> ---
> arch/arm/kernel/hw_breakpoint.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -1098,8 +1098,9 @@ static int __init arch_hw_breakpoint_ini
> * assume that a halting debugger will leave the world in a nice state
> * for us.
> */
> - ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
> - dbg_reset_online, NULL);
> + ret = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> + "arm/hw_breakpoint:online",
> + dbg_reset_online, NULL);
Given the callsite, this particular change looks ok to me. So FWIW:
Acked-by: Mark Rutland <mark.rutland@arm.com>
However, as a more general note, the changes make the API feel odd. per
their current names, {get,put}_online_cpus() sound/feel like refcounting
ops, which should be able to nest.
Is there any chance these could get a better name, e.g.
{lock,unlock}_online_cpus(), so as to align with _cpuslocked?
Thanks,
Mark.
^ permalink raw reply
* [PATCH] arm64: arch_timer: mark errata handlers as __maybe_unused
From: Marc Zyngier @ 2017-04-19 18:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170419173737.3846098-1-arnd@arndb.de>
On 19/04/17 18:37, Arnd Bergmann wrote:
> In some rare randconfig builds, we end up with two functions being entirely
> unused:
>
> drivers/clocksource/arm_arch_timer.c:342:12: error: 'erratum_set_next_event_tval_phys' defined but not used [-Werror=unused-function]
> static int erratum_set_next_event_tval_phys(unsigned long evt,
> drivers/clocksource/arm_arch_timer.c:335:12: error: 'erratum_set_next_event_tval_virt' defined but not used [-Werror=unused-function]
> static int erratum_set_next_event_tval_virt(unsigned long evt,
>
> We could add an #ifdef around them, but we would already have to check for
> several symbols there and there is a chance this would get more complicated
> over time, so marking them as __maybe_unused is the simplest way to avoid the
> harmless warnings.
>
> Fixes: 01d3e3ff2608 ("arm64: arch_timer: Rework the set_next_event workarounds")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Ah, well spotted. You need CONFIG_ARM64_ERRATUM_858921, but none of
CONFIG_HISILICON_ERRATUM_161010101 or CONFIG_FSL_ERRATUM_A008585.
Dreadful stuff.
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 3/3] perf tool, arm64, thunderx2: Add implementation defined events for ThunderX2
From: Ganapatrao Kulkarni @ 2017-04-19 18:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170406095533.GB29947@leverpostej>
Hi Mark,
On Thu, Apr 6, 2017 at 3:25 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Apr 06, 2017 at 09:50:33AM +0530, Ganapatrao Kulkarni wrote:
>> On Wed, Apr 5, 2017 at 3:35 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Wed, Apr 05, 2017 at 02:42:39PM +0530, Ganapatrao Kulkarni wrote:
>> >> On Tue, Apr 4, 2017 at 5:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> > On Tue, Apr 04, 2017 at 01:06:43PM +0530, Ganapatrao Kulkarni wrote:
>
>> >> >> + "CPU" :"armv8_pmuv3_0"
>> >> >
>> >> > Please let's not hard-code the name like this. Surely we can get rid of this?
>> >> >
>> >> > The kernel doesn't currently name PMUs as armv8_pmuv3_*, and as that can
>> >> > differ across DT/ACPI and in big.LITTLE, I don't think it makes sense to
>> >> > try to rely one particular string regardless.
>> >>
>> >> This string/name is fixed for a platform. having name here is essential to
>> >> know which devices among pmu (armv8_pmuv3_0, breakpoint, software)
>> >> devices, these jevents to be added.
>> >> also this json file is specific to a arch/soc/board, it is not a
>> >> generic file to be common.
>> >
>> > This file describe the events of a CPU PMU, and CPUs are not specific to
>> > a platform in general. There are many systems using Cortex-A57, for
>> > example.
>> >
>> > Across big.LITTLE SoCs with Cortex-A57, there's no guarantee as to
>> > whether the Cortex-A57 cores would be named armv8_pmuv3_0, or
>> > armv8_pmuv3_1, etc. This would depend on the boot CPU, probe order of
>> > secondaries, etc.
some of the applications(perf etc) use sysfs files of perf PMU CORE devices.
at present the names are created as per SOC/platform like
armv8_pmuv3, armv8_cavium_thunder, armv8_cortex_a57 etc.
cpu_pmu->name = "armv8_cavium_thunder";
can we please have common name similar to x86(cpu) and call them as
cpu_0 and cpu_1?
>>
>> OK, we may not have complete name however, common part can be used to recognize
>> the PMU CORE devices from /sys/bus/event_source/devices
>> i.e we can have CPU id as "armv8_pmuv3".
>
> For better or worse, that's not the case on DT systems.
>
> I'd much rather that we identified the CPU PMUs without requiring
> particular names (e.g by looking for a "cpus" attribute).
>
>> same is extended to UNCORE as well.
>
> Could you elaborate on that? I'm not sure I follow.
>
>> mapfile.csv file will have entry for both BIG and LITTLE processors event files.
>> the jevents creates table of pmu_events_map for all entries present in
>> mapfile.csv file
>> while lookup, which ever pmu matches the cpuid of pmu_events_map
>> then corresponding table created from json file is used to add the
>> jevents to that PMU.
>
> Sorry, but I don't follow how that's related to the above.
>
> Thanks,
> Mark.
thanks
Ganapat
^ permalink raw reply
* [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT
From: Rafał Miłecki @ 2017-04-19 18:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <221a0208-8a64-6bd8-f6b2-39e845520a83@gmail.com>
On 04/19/2017 07:52 PM, Florian Fainelli wrote:
> On 04/19/2017 10:35 AM, Rafa? Mi?ecki wrote:
>> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>>> On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
>>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>>
>>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>>> attached.
>>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>>> yet).
>>>>>>
>>>>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>> ---
>>>>>> arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>> 1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> @@ -320,6 +320,13 @@
>>>>>> };
>>>>>> };
>>>>>>
>>>>>> + mdio at 18003000 {
>>>>>> + compatible = "brcm,iproc-mdio";
>>>>>> + reg = <0x18003000 0x8>;
>>>>>> + #size-cells = <1>;
>>>>>> + #address-cells = <0>;
>>>>>> + };
>>>>>
>>>>> This looks fine, but usually the block should be enabled on a per-board
>>>>> basis, such that there should be a status = "disabled" property here by
>>>>> default.
>>>>
>>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>>> it's
>>>> for stuff that is always present on every SoC family board: rng, nand,
>>>> spi to
>>>> name few.
>>>>
>>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>>> controller so it's enabled by default. Not every board has SPI flash, so
>>>> it's
>>>> disabled by default.
>>>>
>>>> It's there and it make sense to me. Is that OK or not?
>>>
>>> Even though there are devices that are always enabled on a given SoC,
>>> because the board designs are always consistent does not necessarily
>>> make them good candidates to be enabled at the .dtsi level. This is
>>> particularly true when there are external connections to blocks (SPI,
>>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>>> default is safer as a starting point to begin with.
>>
>> In case of Northstar there is USB 3.0 PHY connected *internally* to this
>> MDIO.
>> I don't think any board manufacturer is able to rip SoC out of the MDIO
>> or the
>> USB 3.0 PHY.
>
> OK, then can you still resubmit a proper patch that a) puts that
> information in the commit message, and b) also adds a proper label to
> the mdio node such that it can later on be referenced by label in
> board-level DTS files? By that I mean:
>
> mdio: mdio at 18003000 {
>
> Thank you
>
>>
>>
>>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>>> MDIO bus
>>>> just devices may differ and should not be enabled by default.
>>>
>>> In which case, the only difference, for you would be to do to, at the
>>> board-level DTS:
>>>
>>> &mdio {
>>> status = "okay";
>>>
>>> phy at 0 {
>>> reg = <0>;
>>> ...
>>> };
>>> };
>>>
>>> versus:
>>>
>>> &mdio {
>>> phy at 0 {
>>> reg = <0>;
>>> ...
>>> };
>>> };
>>>
>>> I think we can afford putting the mdio node's status property in each
>>> board-level DTS and make it clear that way that it is enabled because
>>> there are child nodes enabled?
>>
>> This will be a pretty big effort because every Northstar device I know
>> has USB
>> 3.0 PHY in the SoC.
>
> Adding a one liner is a "pretty big effort", for sure.
Sorry, we got a misunderstanding here.
I thought you meant adding something like this for every device:
&mdio {
status = "okay";
usb3_phy: usb-phy at 10 {
compatible = "brcm,ns-ax-usb3-phy";
reg = <0x10>;
usb3-dmp-syscon = <&usb3_dmp>;
#phy-cells = <0>;
};
};
usb3_dmp: syscon at 18105000 {
reg = <0x18105000 0x1000>;
};
So I clearly missed something important. Did you want to have USB 3.0 PHY
defined in the dtsi file?
^ permalink raw reply
* [PATCH] arm64: arch_timer: mark errata handlers as __maybe_unused
From: Daniel Lezcano @ 2017-04-19 18:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170419173737.3846098-1-arnd@arndb.de>
On Wed, Apr 19, 2017 at 07:37:09PM +0200, Arnd Bergmann wrote:
> In some rare randconfig builds, we end up with two functions being entirely
> unused:
>
> drivers/clocksource/arm_arch_timer.c:342:12: error: 'erratum_set_next_event_tval_phys' defined but not used [-Werror=unused-function]
> static int erratum_set_next_event_tval_phys(unsigned long evt,
> drivers/clocksource/arm_arch_timer.c:335:12: error: 'erratum_set_next_event_tval_virt' defined but not used [-Werror=unused-function]
> static int erratum_set_next_event_tval_virt(unsigned long evt,
>
> We could add an #ifdef around them, but we would already have to check for
> several symbols there and there is a chance this would get more complicated
> over time, so marking them as __maybe_unused is the simplest way to avoid the
> harmless warnings.
>
> Fixes: 01d3e3ff2608 ("arm64: arch_timer: Rework the set_next_event workarounds")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
^ permalink raw reply
* [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT
From: Florian Fainelli @ 2017-04-19 18:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ab696d24-3544-a554-82d1-18839b29caa0@milecki.pl>
On 04/19/2017 11:10 AM, Rafa? Mi?ecki wrote:
> On 04/19/2017 07:52 PM, Florian Fainelli wrote:
>> On 04/19/2017 10:35 AM, Rafa? Mi?ecki wrote:
>>> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>>>> On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
>>>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>>>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>>>
>>>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>>>> attached.
>>>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>>>> yet).
>>>>>>>
>>>>>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>>> ---
>>>>>>> arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>>> 1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> @@ -320,6 +320,13 @@
>>>>>>> };
>>>>>>> };
>>>>>>>
>>>>>>> + mdio at 18003000 {
>>>>>>> + compatible = "brcm,iproc-mdio";
>>>>>>> + reg = <0x18003000 0x8>;
>>>>>>> + #size-cells = <1>;
>>>>>>> + #address-cells = <0>;
>>>>>>> + };
>>>>>>
>>>>>> This looks fine, but usually the block should be enabled on a
>>>>>> per-board
>>>>>> basis, such that there should be a status = "disabled" property
>>>>>> here by
>>>>>> default.
>>>>>
>>>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I
>>>>> guess
>>>>> it's
>>>>> for stuff that is always present on every SoC family board: rng, nand,
>>>>> spi to
>>>>> name few.
>>>>>
>>>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>>>> controller so it's enabled by default. Not every board has SPI
>>>>> flash, so
>>>>> it's
>>>>> disabled by default.
>>>>>
>>>>> It's there and it make sense to me. Is that OK or not?
>>>>
>>>> Even though there are devices that are always enabled on a given SoC,
>>>> because the board designs are always consistent does not necessarily
>>>> make them good candidates to be enabled at the .dtsi level. This is
>>>> particularly true when there are external connections to blocks (SPI,
>>>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>>>> default is safer as a starting point to begin with.
>>>
>>> In case of Northstar there is USB 3.0 PHY connected *internally* to this
>>> MDIO.
>>> I don't think any board manufacturer is able to rip SoC out of the MDIO
>>> or the
>>> USB 3.0 PHY.
>>
>> OK, then can you still resubmit a proper patch that a) puts that
>> information in the commit message, and b) also adds a proper label to
>> the mdio node such that it can later on be referenced by label in
>> board-level DTS files? By that I mean:
>>
>> mdio: mdio at 18003000 {
>>
>> Thank you
>>
>>>
>>>
>>>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>>>> MDIO bus
>>>>> just devices may differ and should not be enabled by default.
>>>>
>>>> In which case, the only difference, for you would be to do to, at the
>>>> board-level DTS:
>>>>
>>>> &mdio {
>>>> status = "okay";
>>>>
>>>> phy at 0 {
>>>> reg = <0>;
>>>> ...
>>>> };
>>>> };
>>>>
>>>> versus:
>>>>
>>>> &mdio {
>>>> phy at 0 {
>>>> reg = <0>;
>>>> ...
>>>> };
>>>> };
>>>>
>>>> I think we can afford putting the mdio node's status property in each
>>>> board-level DTS and make it clear that way that it is enabled because
>>>> there are child nodes enabled?
>>>
>>> This will be a pretty big effort because every Northstar device I know
>>> has USB
>>> 3.0 PHY in the SoC.
>>
>> Adding a one liner is a "pretty big effort", for sure.
>
> Sorry, we got a misunderstanding here.
>
> I thought you meant adding something like this for every device:
>
> &mdio {
> status = "okay";
>
> usb3_phy: usb-phy at 10 {
> compatible = "brcm,ns-ax-usb3-phy";
> reg = <0x10>;
> usb3-dmp-syscon = <&usb3_dmp>;
> #phy-cells = <0>;
> };
> };
Ah no, I would have just done the following in the per-board DTS:
&mdio {
status = "okay";
};
&usb3_phy {
status = "okay";
};
&xhci {
status = "okay";
};
Something like that.
>
> usb3_dmp: syscon at 18105000 {
> reg = <0x18105000 0x1000>;
> };
>
> So I clearly missed something important. Did you want to have USB 3.0 PHY
> defined in the dtsi file?
Yes, I think it does make sense to have it defined in the .dtsi file
because it's internal to the SoC, however it should probably be marked
disabled by default, unless a board enables its xHCI controller, does
that make sense?
--
Florian
^ permalink raw reply
* [PATCH 1/2] xen/arm, arm64: fix xen_dma_ops after 815dd18 "Consolidate get_dma_ops..."
From: Juergen Gross @ 2017-04-19 18:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <alpine.DEB.2.10.1704191023120.31486@sstabellini-ThinkPad-X260>
On 19/04/17 19:25, Stefano Stabellini wrote:
> Hello Russell,
>
> Can I have your acked-by on the following fix (original patch is
> 1492117462-19886-1-git-send-email-sstabellini at kernel.org)?
Stefano, through which tree should this go? ARM or Xen or other?
Juergen
>
> Thanks,
>
> Stefano
>
>
> On Tue, 18 Apr 2017, Catalin Marinas wrote:
>> On Thu, Apr 13, 2017 at 02:04:21PM -0700, Stefano Stabellini wrote:
>>> The following commit:
>>>
>>> commit 815dd18788fe0d41899f51b91d0560279cf16b0d
>>> Author: Bart Van Assche <bart.vanassche@sandisk.com>
>>> Date: Fri Jan 20 13:04:04 2017 -0800
>>>
>>> treewide: Consolidate get_dma_ops() implementations
>>>
>>> rearranges get_dma_ops in a way that xen_dma_ops are not returned when
>>> running on Xen anymore, dev->dma_ops is returned instead (see
>>> arch/arm/include/asm/dma-mapping.h:get_arch_dma_ops and
>>> include/linux/dma-mapping.h:get_dma_ops).
>>>
>>> Fix the problem by storing dev->dma_ops in dev_archdata, and setting
>>> dev->dma_ops to xen_dma_ops. This way, xen_dma_ops is returned naturally
>>> by get_dma_ops. The Xen code can retrieve the original dev->dma_ops from
>>> dev_archdata when needed. It also allows us to remove __generic_dma_ops
>>> from common headers.
>>>
>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>> Tested-by: Julien Grall <julien.grall@arm.com>
>>> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
>>> CC: linux at armlinux.org.uk
>>> CC: catalin.marinas at arm.com
>>> CC: will.deacon at arm.com
>>> CC: boris.ostrovsky at oracle.com
>>> CC: jgross at suse.com
>>> CC: Julien Grall <julien.grall@arm.com>
>>
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>>
>>> diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
>>> index 220ba20..36ec9c8 100644
>>> --- a/arch/arm/include/asm/device.h
>>> +++ b/arch/arm/include/asm/device.h
>>> @@ -16,6 +16,9 @@ struct dev_archdata {
>>> #ifdef CONFIG_ARM_DMA_USE_IOMMU
>>> struct dma_iommu_mapping *mapping;
>>> #endif
>>> +#ifdef CONFIG_XEN
>>> + const struct dma_map_ops *dev_dma_ops;
>>> +#endif
>>> bool dma_coherent;
>>> };
>>>
>>> diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
>>> index 7166569..680d3f3 100644
>>> --- a/arch/arm/include/asm/dma-mapping.h
>>> +++ b/arch/arm/include/asm/dma-mapping.h
>>> @@ -16,19 +16,9 @@
>>> extern const struct dma_map_ops arm_dma_ops;
>>> extern const struct dma_map_ops arm_coherent_dma_ops;
>>>
>>> -static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
>>> -{
>>> - if (dev && dev->dma_ops)
>>> - return dev->dma_ops;
>>> - return &arm_dma_ops;
>>> -}
>>> -
>>> static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
>>> {
>>> - if (xen_initial_domain())
>>> - return xen_dma_ops;
>>> - else
>>> - return __generic_dma_ops(NULL);
>>> + return &arm_dma_ops;
>>> }
>>>
>>> #define HAVE_ARCH_DMA_SUPPORTED 1
>>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>>> index 63eabb0..a614459 100644
>>> --- a/arch/arm/mm/dma-mapping.c
>>> +++ b/arch/arm/mm/dma-mapping.c
>>> @@ -2396,6 +2396,13 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>> dma_ops = arm_get_dma_map_ops(coherent);
>>>
>>> set_dma_ops(dev, dma_ops);
>>> +
>>> +#ifdef CONFIG_XEN
>>> + if (xen_initial_domain()) {
>>> + dev->archdata.dev_dma_ops = dev->dma_ops;
>>> + dev->dma_ops = xen_dma_ops;
>>> + }
>>> +#endif
>>> }
>>>
>>> void arch_teardown_dma_ops(struct device *dev)
>>> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
>>> index 73d5bab..5a5fa47 100644
>>> --- a/arch/arm64/include/asm/device.h
>>> +++ b/arch/arm64/include/asm/device.h
>>> @@ -20,6 +20,9 @@ struct dev_archdata {
>>> #ifdef CONFIG_IOMMU_API
>>> void *iommu; /* private IOMMU data */
>>> #endif
>>> +#ifdef CONFIG_XEN
>>> + const struct dma_map_ops *dev_dma_ops;
>>> +#endif
>>> bool dma_coherent;
>>> };
>>>
>>> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
>>> index 505756c..5392dbe 100644
>>> --- a/arch/arm64/include/asm/dma-mapping.h
>>> +++ b/arch/arm64/include/asm/dma-mapping.h
>>> @@ -27,11 +27,8 @@
>>> #define DMA_ERROR_CODE (~(dma_addr_t)0)
>>> extern const struct dma_map_ops dummy_dma_ops;
>>>
>>> -static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
>>> +static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
>>> {
>>> - if (dev && dev->dma_ops)
>>> - return dev->dma_ops;
>>> -
>>> /*
>>> * We expect no ISA devices, and all other DMA masters are expected to
>>> * have someone call arch_setup_dma_ops at device creation time.
>>> @@ -39,14 +36,6 @@ static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
>>> return &dummy_dma_ops;
>>> }
>>>
>>> -static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
>>> -{
>>> - if (xen_initial_domain())
>>> - return xen_dma_ops;
>>> - else
>>> - return __generic_dma_ops(NULL);
>>> -}
>>> -
>>> void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>> const struct iommu_ops *iommu, bool coherent);
>>> #define arch_setup_dma_ops arch_setup_dma_ops
>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>>> index 81cdb2e..7f8b37e 100644
>>> --- a/arch/arm64/mm/dma-mapping.c
>>> +++ b/arch/arm64/mm/dma-mapping.c
>>> @@ -977,4 +977,11 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>>
>>> dev->archdata.dma_coherent = coherent;
>>> __iommu_setup_dma_ops(dev, dma_base, size, iommu);
>>> +
>>> +#ifdef CONFIG_XEN
>>> + if (xen_initial_domain()) {
>>> + dev->archdata.dev_dma_ops = dev->dma_ops;
>>> + dev->dma_ops = xen_dma_ops;
>>> + }
>>> +#endif
>>> }
>>> diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
>>> index 95ce6ac..b0a2bfc 100644
>>> --- a/include/xen/arm/page-coherent.h
>>> +++ b/include/xen/arm/page-coherent.h
>>> @@ -2,8 +2,16 @@
>>> #define _ASM_ARM_XEN_PAGE_COHERENT_H
>>>
>>> #include <asm/page.h>
>>> +#include <asm/dma-mapping.h>
>>> #include <linux/dma-mapping.h>
>>>
>>> +static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
>>> +{
>>> + if (dev && dev->archdata.dev_dma_ops)
>>> + return dev->archdata.dev_dma_ops;
>>> + return get_arch_dma_ops(NULL);
>>> +}
>>> +
>>> void __xen_dma_map_page(struct device *hwdev, struct page *page,
>>> dma_addr_t dev_addr, unsigned long offset, size_t size,
>>> enum dma_data_direction dir, unsigned long attrs);
>
^ permalink raw reply
* [patch V2 11/24] ARM/hw_breakpoint: Use cpuhp_setup_state_cpuslocked()
From: Thomas Gleixner @ 2017-04-19 18:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170419175454.GM27829@leverpostej>
On Wed, 19 Apr 2017, Mark Rutland wrote:
> Hi,
>
> On Tue, Apr 18, 2017 at 07:04:53PM +0200, Thomas Gleixner wrote:
> > From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >
> > arch_hw_breakpoint_init() holds get_online_cpus() while registerring the
> > hotplug callbacks.
> >
> > cpuhp_setup_state() invokes get_online_cpus() as well. This is correct, but
> > prevents the conversion of the hotplug locking to a percpu rwsem.
> >
> > Use cpuhp_setup_state_cpuslocked() to avoid the nested call.
> >
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: linux-arm-kernel at lists.infradead.org
> >
> > ---
> > arch/arm/kernel/hw_breakpoint.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > --- a/arch/arm/kernel/hw_breakpoint.c
> > +++ b/arch/arm/kernel/hw_breakpoint.c
> > @@ -1098,8 +1098,9 @@ static int __init arch_hw_breakpoint_ini
> > * assume that a halting debugger will leave the world in a nice state
> > * for us.
> > */
> > - ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
> > - dbg_reset_online, NULL);
> > + ret = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> > + "arm/hw_breakpoint:online",
> > + dbg_reset_online, NULL);
>
> Given the callsite, this particular change looks ok to me. So FWIW:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> However, as a more general note, the changes make the API feel odd. per
> their current names, {get,put}_online_cpus() sound/feel like refcounting
> ops, which should be able to nest.
>
> Is there any chance these could get a better name, e.g.
> {lock,unlock}_online_cpus(), so as to align with _cpuslocked?
Yes, that's a follow up cleanup patch treewide once this hit Linus tree.
Thanks,
tglx
^ permalink raw reply
* [PATCH] [RFC] gpu: host1x: shut up warning about DMA API misuse
From: Arnd Bergmann @ 2017-04-19 18:24 UTC (permalink / raw)
To: linux-arm-kernel
When dma_addr_t and phys_addr_t are not the same size, we get a warning
from the dma_alloc_wc function:
drivers/gpu/host1x/cdma.c: In function 'host1x_pushbuffer_init':
drivers/gpu/host1x/cdma.c:94:48: error: passing argument 3 of 'dma_alloc_wc' from incompatible pointer type [-Werror=incompatible-pointer-types]
pb->mapped = dma_alloc_wc(host1x->dev, size, &pb->phys,
^
In file included from drivers/gpu/host1x/cdma.c:22:0:
include/linux/dma-mapping.h:761:37: note: expected 'dma_addr_t * {aka long long unsigned int *}' but argument is of type 'phys_addr_t * {aka unsigned int *}'
static noinline_if_stackbloat void *dma_alloc_wc(struct device *dev, size_t size,
^~~~~~~~~~~~
drivers/gpu/host1x/cdma.c:113:48: error: passing argument 3 of 'dma_alloc_wc' from incompatible pointer type [-Werror=incompatible-pointer-types]
pb->mapped = dma_alloc_wc(host1x->dev, size, &pb->phys,
^
In file included from drivers/gpu/host1x/cdma.c:22:0:
include/linux/dma-mapping.h:761:37: note: expected 'dma_addr_t * {aka long long unsigned int *}' but argument is of type 'phys_addr_t * {aka unsigned int *}'
static noinline_if_stackbloat void *dma_alloc_wc(struct device *dev, size_t size,
^~~~~~~~~~~~
The problem here is that dma_alloc_wc() returns a pointer to a dma_addr_t
that may already be translated by an IOMMU, but the driver passes this
into iommu_map() as a physical address. This works by accident only when
the IOMMU does not get registered with the DMA API and there is a 1:1
mapping between physical addresses as seen by the CPU and the device.
The fundamental problem here is the lack of a generic API to do what the
driver wants: allocating CPU-visible memory for use by a device through
user-defined IOMMU settings. Neither the dma-mapping API nor the IOMMU
API can do this by itself, and combining the two is not well-defined.
This patch addresses the type mismatch by adding a third pointer into the
push_buffer structure: in addition to the address as seen from the CPU
and the address inside of the local IOMMU domain, the pb->alloc pointer
is the token returned by dma_alloc_wc(), and this is what we later need
to pass into dma_free_wc().
The address we pass into iommu_map() however is the physical address
computed from virt_to_phys(), assuming that the pointer we have here
is part of the linear mapping (which is also questionable, e.g. when we
have a non-coherent device on ARM32 this may be false). Also, when
the DMA API uses the IOMMU to allocate the pointer for the default
domain, we end up with two IOMMU mappings for the same physical address.
Fixes: 404bfb78daf3 ("gpu: host1x: Add IOMMU support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/gpu/host1x/cdma.c | 26 ++++++++++----------------
drivers/gpu/host1x/cdma.h | 1 +
2 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
index 28541b280739..286edeca7ba1 100644
--- a/drivers/gpu/host1x/cdma.c
+++ b/drivers/gpu/host1x/cdma.c
@@ -59,7 +59,7 @@ static void host1x_pushbuffer_destroy(struct push_buffer *pb)
free_iova(&host1x->iova, iova_pfn(&host1x->iova, pb->dma));
}
- dma_free_wc(host1x->dev, pb->alloc_size, pb->mapped, pb->phys);
+ dma_free_wc(host1x->dev, pb->alloc_size, pb->mapped, pb->alloc);
pb->mapped = NULL;
pb->phys = 0;
@@ -81,20 +81,21 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
pb->size = HOST1X_PUSHBUFFER_SLOTS * 8;
size = pb->size + 4;
+ if (host1x->domain)
+ size = iova_align(&host1x->iova, size);
/* initialize buffer pointers */
pb->fence = pb->size - 8;
pb->pos = 0;
- if (host1x->domain) {
- unsigned long shift;
+ pb->mapped = dma_alloc_wc(host1x->dev, size, &pb->alloc, GFP_KERNEL);
+ if (!pb->mapped)
+ return -ENOMEM;
- size = iova_align(&host1x->iova, size);
+ pb->phys = virt_to_phys(pb->mapped);
- pb->mapped = dma_alloc_wc(host1x->dev, size, &pb->phys,
- GFP_KERNEL);
- if (!pb->mapped)
- return -ENOMEM;
+ if (host1x->domain) {
+ unsigned long shift;
shift = iova_shift(&host1x->iova);
alloc = alloc_iova(&host1x->iova, size >> shift,
@@ -109,13 +110,6 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
IOMMU_READ);
if (err)
goto iommu_free_iova;
- } else {
- pb->mapped = dma_alloc_wc(host1x->dev, size, &pb->phys,
- GFP_KERNEL);
- if (!pb->mapped)
- return -ENOMEM;
-
- pb->dma = pb->phys;
}
pb->alloc_size = size;
@@ -127,7 +121,7 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
iommu_free_iova:
__free_iova(&host1x->iova, alloc);
iommu_free_mem:
- dma_free_wc(host1x->dev, pb->alloc_size, pb->mapped, pb->phys);
+ dma_free_wc(host1x->dev, pb->alloc_size, pb->mapped, pb->alloc);
return err;
}
diff --git a/drivers/gpu/host1x/cdma.h b/drivers/gpu/host1x/cdma.h
index ec170a78f4e1..8479192d4265 100644
--- a/drivers/gpu/host1x/cdma.h
+++ b/drivers/gpu/host1x/cdma.h
@@ -43,6 +43,7 @@ struct host1x_job;
struct push_buffer {
void *mapped; /* mapped pushbuffer memory */
+ dma_addr_t alloc; /* device address in root domain */
dma_addr_t dma; /* device address of pushbuffer */
phys_addr_t phys; /* physical address of pushbuffer */
u32 fence; /* index we've written */
--
2.9.0
^ permalink raw reply related
* [PATCH 0/8] i.MX7 PCIe related device tree changes
From: Tyler Baker @ 2017-04-19 18:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170413133242.5068-1-andrew.smirnov@gmail.com>
On 13 April 2017 at 06:32, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Shawn, everyone:
>
> This series includes changes made to device-tree in order to support
> PCIe on i.MX7 platform. They include:
>
> - Bringing 'anatop-enable-bit' property of ANATOP regulators back
> and extending it to all of the HW it is applicable to
>
> - Adding GPCv2 node for i.MX7 (which was missing, despite the
> irqchip driver for it being in the tree for quite some time)
>
> - Adding a PCIe node for i.MX7
>
> - Adding GPIO expander used by PCIe and enabling PCIe node from
> above on i.MX7 based Sabre board
>
> As usual, feedback is welcome.
>
> Thanks,
> Andrey Smrinov
>
> Andrey Smirnov (8):
> Revert "ARM: dts: imx: Remove unexistant property"
> ARM: dts: imx6: Specify 'anatop-enable-bit' where appropriate
> ARM: dts: imx7s: Adjust anatop-enable-bit for 'reg_1p0d'
> ARM: dts: imx7s: Add node for GPC
> ARM: dts: imx7s: Mark 'gpr' compatible with i.MX6 variant
> ARM: dts: imx7d-sdb: Add GPIO expander node
> ARM: dts: imx7d: Add node for PCIe controller
> ARM: dts: imx7d-sdb: Enable PCIe peripheral
FWIW:
Tested-by: Tyler Baker <tyler.baker@linaro.org>
This whole series on top of v4.11-rc7, with the addition of the iMX7
GPCv2 and reset driver. Tested on a imx7d-cl-som with a CUK Killer
Doubleshot Wireless-AC 1535 wlan/bt radio using a mini pcie to m2
adapter. Confirmed that the radio was able to associate with an AP
using WPA2, and connected to multiple devices using 6lowpan over BLE.
Tyler
^ permalink raw reply
* [PATCH V15 01/11] acpi: apei: read ack upon ghes record consumption
From: Borislav Petkov @ 2017-04-19 18:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1492556723-9189-2-git-send-email-tbaicar@codeaurora.org>
On Tue, Apr 18, 2017 at 05:05:13PM -0600, Tyler Baicar wrote:
> A RAS (Reliability, Availability, Serviceability) controller
> may be a separate processor running in parallel with OS
> execution, and may generate error records for consumption by
> the OS. If the RAS controller produces multiple error records,
> then they may be overwritten before the OS has consumed them.
>
> The Generic Hardware Error Source (GHES) v2 structure
> introduces the capability for the OS to acknowledge the
> consumption of the error record generated by the RAS
> controller. A RAS controller supporting GHESv2 shall wait for
> the acknowledgment before writing a new error record, thus
> eliminating the race condition.
>
> Add support for parsing of GHESv2 sub-tables as well.
>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Reviewed-by: James Morse <james.morse@arm.com>
> ---
> drivers/acpi/apei/ghes.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---
> drivers/acpi/apei/hest.c | 7 ++++--
> include/acpi/ghes.h | 5 ++++-
> 3 files changed, 61 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 79b3c9c..6d87ab7 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -46,6 +46,7 @@
> #include <linux/nmi.h>
> #include <linux/sched/clock.h>
>
> +#include <acpi/actbl1.h>
> #include <acpi/ghes.h>
> #include <acpi/apei.h>
> #include <asm/tlbflush.h>
> @@ -80,6 +81,10 @@
> ((struct acpi_hest_generic_status *) \
> ((struct ghes_estatus_node *)(estatus_node) + 1))
>
> +#define IS_HEST_TYPE_GENERIC_V2(ghes) \
> + ((struct acpi_hest_header *)ghes->generic)->type == \
This is a nasty hack: casting the ghes->generic pointer to a pointer of its
first member which is a acpi_hest_header.
Why isn't this a nice inline function with proper dereferencing:
static inline bool is_hest_type_generic_v2(struct ghes *ghes)
{
return ghes->generic->header.type == ACPI_HEST_TYPE_GENERIC_ERROR_V2;
}
?
Also, please integrate scripts/checkpatch.pl in your patch creation
workflow. Some of the warnings/errors *actually* make sense.
> /*
> * This driver isn't really modular, however for the time being,
> * continuing to use module_param is the easiest way to remain
> @@ -240,6 +245,17 @@ static int ghes_estatus_pool_expand(unsigned long len)
> return 0;
> }
>
> +static int map_gen_v2(struct ghes *ghes)
> +{
> + return apei_map_generic_address(&ghes->generic_v2->read_ack_register);
> +}
> +
> +static void unmap_gen_v2(struct ghes *ghes)
> +{
> + apei_unmap_generic_address(&ghes->generic_v2->read_ack_register);
> + return;
> +}
Like this one, for example:
WARNING: void function return statements are not generally useful
#89: FILE: drivers/acpi/apei/ghes.c:257:
+ return;
+}
> +
> static struct ghes *ghes_new(struct acpi_hest_generic *generic)
> {
> struct ghes *ghes;
> @@ -249,10 +265,17 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
> ghes = kzalloc(sizeof(*ghes), GFP_KERNEL);
> if (!ghes)
> return ERR_PTR(-ENOMEM);
> +
> ghes->generic = generic;
> + if (IS_HEST_TYPE_GENERIC_V2(ghes)) {
> + rc = map_gen_v2(ghes);
> + if (rc)
> + goto err_free;
> + }
> +
> rc = apei_map_generic_address(&generic->error_status_address);
> if (rc)
> - goto err_free;
> + goto err_unmap_read_ack_addr;
> error_block_length = generic->error_block_length;
> if (error_block_length > GHES_ESTATUS_MAX_SIZE) {
> pr_warning(FW_WARN GHES_PFX
> @@ -264,13 +287,16 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
> ghes->estatus = kmalloc(error_block_length, GFP_KERNEL);
> if (!ghes->estatus) {
> rc = -ENOMEM;
> - goto err_unmap;
> + goto err_unmap_status_addr;
> }
>
> return ghes;
>
> -err_unmap:
> +err_unmap_status_addr:
> apei_unmap_generic_address(&generic->error_status_address);
> +err_unmap_read_ack_addr:
> + if (IS_HEST_TYPE_GENERIC_V2(ghes))
> + unmap_gen_v2(ghes);
> err_free:
> kfree(ghes);
> return ERR_PTR(rc);
> @@ -280,6 +306,8 @@ static void ghes_fini(struct ghes *ghes)
> {
> kfree(ghes->estatus);
> apei_unmap_generic_address(&ghes->generic->error_status_address);
> + if (IS_HEST_TYPE_GENERIC_V2(ghes))
> + unmap_gen_v2(ghes);
> }
>
> static inline int ghes_severity(int severity)
> @@ -649,6 +677,21 @@ static void ghes_estatus_cache_add(
> rcu_read_unlock();
> }
>
> +static int ghes_ack_error(struct acpi_hest_generic_v2 *generic_v2)
If you name this function parameter to something shorter, say gv2, for
example...
> +{
> + int rc;
> + u64 val = 0;
> +
> + rc = apei_read(&val, &generic_v2->read_ack_register);
> + if (rc)
> + return rc;
> +
> + val &= generic_v2->read_ack_preserve << generic_v2->read_ack_register.bit_offset;
> + val |= generic_v2->read_ack_write << generic_v2->read_ack_register.bit_offset;
... you can align those two nicely while remaining within the 80 cols width:
val &= gv2->read_ack_preserve << gv2->read_ack_register.bit_offset;
val |= gv2->read_ack_write << gv2->read_ack_register.bit_offset;
and make them readable at a quick glance.
> +
> + return apei_write(val, &generic_v2->read_ack_register);
> +}
> +
> static int ghes_proc(struct ghes *ghes)
> {
> int rc;
> @@ -661,6 +704,12 @@ static int ghes_proc(struct ghes *ghes)
> ghes_estatus_cache_add(ghes->generic, ghes->estatus);
> }
> ghes_do_proc(ghes, ghes->estatus);
This needs a comment why v2 needs to ACK the error. The commit message
is not necessarily something we'll find quickly in the future.
> +
> + if (IS_HEST_TYPE_GENERIC_V2(ghes)) {
> + rc = ghes_ack_error(ghes->generic_v2);
> + if (rc)
> + return rc;
> + }
> out:
> ghes_clear_estatus(ghes);
> return rc;
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* [PATCH 1/2] xen/arm, arm64: fix xen_dma_ops after 815dd18 "Consolidate get_dma_ops..."
From: Stefano Stabellini @ 2017-04-19 18:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a4e371e4-44ff-ee6d-22b6-3ff9d57ce421@suse.com>
On Wed, 19 Apr 2017, Juergen Gross wrote:
> On 19/04/17 19:25, Stefano Stabellini wrote:
> > Hello Russell,
> >
> > Can I have your acked-by on the following fix (original patch is
> > 1492117462-19886-1-git-send-email-sstabellini at kernel.org)?
>
> Stefano, through which tree should this go? ARM or Xen or other?
I think it could go via the Xen tree, but we still need an ack from the
ARM maintainer for changes to code under arch/arm.
> Juergen
>
> >
> > Thanks,
> >
> > Stefano
> >
> >
> > On Tue, 18 Apr 2017, Catalin Marinas wrote:
> >> On Thu, Apr 13, 2017 at 02:04:21PM -0700, Stefano Stabellini wrote:
> >>> The following commit:
> >>>
> >>> commit 815dd18788fe0d41899f51b91d0560279cf16b0d
> >>> Author: Bart Van Assche <bart.vanassche@sandisk.com>
> >>> Date: Fri Jan 20 13:04:04 2017 -0800
> >>>
> >>> treewide: Consolidate get_dma_ops() implementations
> >>>
> >>> rearranges get_dma_ops in a way that xen_dma_ops are not returned when
> >>> running on Xen anymore, dev->dma_ops is returned instead (see
> >>> arch/arm/include/asm/dma-mapping.h:get_arch_dma_ops and
> >>> include/linux/dma-mapping.h:get_dma_ops).
> >>>
> >>> Fix the problem by storing dev->dma_ops in dev_archdata, and setting
> >>> dev->dma_ops to xen_dma_ops. This way, xen_dma_ops is returned naturally
> >>> by get_dma_ops. The Xen code can retrieve the original dev->dma_ops from
> >>> dev_archdata when needed. It also allows us to remove __generic_dma_ops
> >>> from common headers.
> >>>
> >>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> >>> Tested-by: Julien Grall <julien.grall@arm.com>
> >>> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> >>> CC: linux at armlinux.org.uk
> >>> CC: catalin.marinas at arm.com
> >>> CC: will.deacon at arm.com
> >>> CC: boris.ostrovsky at oracle.com
> >>> CC: jgross at suse.com
> >>> CC: Julien Grall <julien.grall@arm.com>
> >>
> >> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> >>
> >>
> >>> diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
> >>> index 220ba20..36ec9c8 100644
> >>> --- a/arch/arm/include/asm/device.h
> >>> +++ b/arch/arm/include/asm/device.h
> >>> @@ -16,6 +16,9 @@ struct dev_archdata {
> >>> #ifdef CONFIG_ARM_DMA_USE_IOMMU
> >>> struct dma_iommu_mapping *mapping;
> >>> #endif
> >>> +#ifdef CONFIG_XEN
> >>> + const struct dma_map_ops *dev_dma_ops;
> >>> +#endif
> >>> bool dma_coherent;
> >>> };
> >>>
> >>> diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
> >>> index 7166569..680d3f3 100644
> >>> --- a/arch/arm/include/asm/dma-mapping.h
> >>> +++ b/arch/arm/include/asm/dma-mapping.h
> >>> @@ -16,19 +16,9 @@
> >>> extern const struct dma_map_ops arm_dma_ops;
> >>> extern const struct dma_map_ops arm_coherent_dma_ops;
> >>>
> >>> -static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
> >>> -{
> >>> - if (dev && dev->dma_ops)
> >>> - return dev->dma_ops;
> >>> - return &arm_dma_ops;
> >>> -}
> >>> -
> >>> static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
> >>> {
> >>> - if (xen_initial_domain())
> >>> - return xen_dma_ops;
> >>> - else
> >>> - return __generic_dma_ops(NULL);
> >>> + return &arm_dma_ops;
> >>> }
> >>>
> >>> #define HAVE_ARCH_DMA_SUPPORTED 1
> >>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> >>> index 63eabb0..a614459 100644
> >>> --- a/arch/arm/mm/dma-mapping.c
> >>> +++ b/arch/arm/mm/dma-mapping.c
> >>> @@ -2396,6 +2396,13 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> >>> dma_ops = arm_get_dma_map_ops(coherent);
> >>>
> >>> set_dma_ops(dev, dma_ops);
> >>> +
> >>> +#ifdef CONFIG_XEN
> >>> + if (xen_initial_domain()) {
> >>> + dev->archdata.dev_dma_ops = dev->dma_ops;
> >>> + dev->dma_ops = xen_dma_ops;
> >>> + }
> >>> +#endif
> >>> }
> >>>
> >>> void arch_teardown_dma_ops(struct device *dev)
> >>> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
> >>> index 73d5bab..5a5fa47 100644
> >>> --- a/arch/arm64/include/asm/device.h
> >>> +++ b/arch/arm64/include/asm/device.h
> >>> @@ -20,6 +20,9 @@ struct dev_archdata {
> >>> #ifdef CONFIG_IOMMU_API
> >>> void *iommu; /* private IOMMU data */
> >>> #endif
> >>> +#ifdef CONFIG_XEN
> >>> + const struct dma_map_ops *dev_dma_ops;
> >>> +#endif
> >>> bool dma_coherent;
> >>> };
> >>>
> >>> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> >>> index 505756c..5392dbe 100644
> >>> --- a/arch/arm64/include/asm/dma-mapping.h
> >>> +++ b/arch/arm64/include/asm/dma-mapping.h
> >>> @@ -27,11 +27,8 @@
> >>> #define DMA_ERROR_CODE (~(dma_addr_t)0)
> >>> extern const struct dma_map_ops dummy_dma_ops;
> >>>
> >>> -static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
> >>> +static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
> >>> {
> >>> - if (dev && dev->dma_ops)
> >>> - return dev->dma_ops;
> >>> -
> >>> /*
> >>> * We expect no ISA devices, and all other DMA masters are expected to
> >>> * have someone call arch_setup_dma_ops at device creation time.
> >>> @@ -39,14 +36,6 @@ static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
> >>> return &dummy_dma_ops;
> >>> }
> >>>
> >>> -static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
> >>> -{
> >>> - if (xen_initial_domain())
> >>> - return xen_dma_ops;
> >>> - else
> >>> - return __generic_dma_ops(NULL);
> >>> -}
> >>> -
> >>> void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> >>> const struct iommu_ops *iommu, bool coherent);
> >>> #define arch_setup_dma_ops arch_setup_dma_ops
> >>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> >>> index 81cdb2e..7f8b37e 100644
> >>> --- a/arch/arm64/mm/dma-mapping.c
> >>> +++ b/arch/arm64/mm/dma-mapping.c
> >>> @@ -977,4 +977,11 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> >>>
> >>> dev->archdata.dma_coherent = coherent;
> >>> __iommu_setup_dma_ops(dev, dma_base, size, iommu);
> >>> +
> >>> +#ifdef CONFIG_XEN
> >>> + if (xen_initial_domain()) {
> >>> + dev->archdata.dev_dma_ops = dev->dma_ops;
> >>> + dev->dma_ops = xen_dma_ops;
> >>> + }
> >>> +#endif
> >>> }
> >>> diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
> >>> index 95ce6ac..b0a2bfc 100644
> >>> --- a/include/xen/arm/page-coherent.h
> >>> +++ b/include/xen/arm/page-coherent.h
> >>> @@ -2,8 +2,16 @@
> >>> #define _ASM_ARM_XEN_PAGE_COHERENT_H
> >>>
> >>> #include <asm/page.h>
> >>> +#include <asm/dma-mapping.h>
> >>> #include <linux/dma-mapping.h>
> >>>
> >>> +static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
> >>> +{
> >>> + if (dev && dev->archdata.dev_dma_ops)
> >>> + return dev->archdata.dev_dma_ops;
> >>> + return get_arch_dma_ops(NULL);
> >>> +}
> >>> +
> >>> void __xen_dma_map_page(struct device *hwdev, struct page *page,
> >>> dma_addr_t dev_addr, unsigned long offset, size_t size,
> >>> enum dma_data_direction dir, unsigned long attrs);
> >
>
^ permalink raw reply
* [PATCH 2/4] mmc: pxamci: Enhance error checking
From: Robert Jarzmik @ 2017-04-19 19:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a8769ce8-2732-1f23-eaca-33591951ee78@tul.cz>
Petr Cvek <petr.cvek@tul.cz> writes:
> The pxamci_dma_irq() and pxamci_data_done() should print errors if they
> obtains invalid parameters. Make the pxamci_dma_irq() call with
> the MMC_DATA_READ flag a fault as the DMA callback is used only for write.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
> drivers/mmc/host/pxamci.c | 33 ++++++++++++++++++++++++---------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 80bc8065b50f..48c26d848e9f 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -354,13 +354,22 @@ static int pxamci_data_done(struct pxamci_host *host, unsigned int stat)
> struct mmc_data *data = host->data;
> struct dma_chan *chan;
>
> - if (!data)
> + if (!data) {
> + pr_err("%s: Missing data structure\n",
> + mmc_hostname(host->mmc));
I'd rather have :
dev_err(mmc_dev(mmc), ...)
> return 0;
> + }
>
> if (data->flags & MMC_DATA_READ)
> chan = host->dma_chan_rx;
> - else
> + else if (data->flags & MMC_DATA_WRITE)
> chan = host->dma_chan_tx;
> + else {
> + pr_err("%s: Unknown data direction, flags=%08x\n",
> + mmc_hostname(host->mmc), data->flags);
Ditto.
> + if (!host->data) {
> + pr_err("%s: Missing data structure\n",
> + mmc_hostname(host->mmc));
Ditto.
> goto out_unlock;
> + }
>
> - if (host->data->flags & MMC_DATA_READ)
> - chan = host->dma_chan_rx;
> - else
> - chan = host->dma_chan_tx;
> + if (!(host->data->flags & MMC_DATA_WRITE)) {
> + pr_err("%s: DMA callback is only for tx channel, flags=%x\n",
> + mmc_hostname(host->mmc), host->data->flags);
Ditto.
> + goto out_unlock;
> + }
> +
> + chan = host->dma_chan_tx;
>
> status = dmaengine_tx_status(chan, host->dma_cookie, &state);
>
> if (likely(status == DMA_COMPLETE)) {
> writel(BUF_PART_FULL, host->base + MMC_PRTBUF);
> } else {
> - pr_err("%s: DMA error on %s channel\n", mmc_hostname(host->mmc),
> - host->data->flags & MMC_DATA_READ ? "rx" : "tx");
> + pr_err("%s: Invalid DMA status %i\n", mmc_hostname(host->mmc),
> + status);
Ditto.
Cheers.
--
Robert
^ permalink raw reply
* [PATCH 3/4] mmc: pxamci: Disable DATA_TRAN_DONE interrupt sooner
From: Robert Jarzmik @ 2017-04-19 19:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <04139c1a-cee8-f64c-4b0e-2e3c472169a3@tul.cz>
Petr Cvek <petr.cvek@tul.cz> writes:
> Disable the DATA_TRAN_DONE interrupt as soon as possible in the handler.
Yeah, but why, please explain.
If that's only to "reduce" the race occurrence, then I'd rather have this patch
dropped. Otherwise if there is a compelling reason let's see ...
Cheers.
--
Robert
^ permalink raw reply
* [PATCH 0/2] clk: mediatek: add missing ethernet reset definition
From: John Crispin @ 2017-04-19 19:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170419171835.GO7065@codeaurora.org>
On 19/04/17 19:18, Stephen Boyd wrote:
> On 01/27, John Crispin wrote:
>>
>> On 27/01/2017 01:14, Stephen Boyd wrote:
>>> On 01/23, John Crispin wrote:
>>>> When the clk code for mt2701 was merged, we forgot to add the reset
>>>> controller of the ethernet subsystem. This series adds the missing code
>>>> and header files required to reference the bits from within a devicetree
>>>> file.
>>>>
>>>> John Crispin (2):
>>>> clk: mediatek: add mt2701 ethernet reset
>>>> reset: mediatek: Add MT2701 ethsys reset controller include file
>>>>
>>>> drivers/clk/mediatek/clk-mt2701-eth.c | 2 ++
>>>> include/dt-bindings/reset/mt2701-resets.h | 7 +++++++
>>> Any planned DT users in the next merge window? Should I put this
>>> into a topic branch? Do you want a clk maintainer ack? At the
>>> least we need someone like James to ack this first.
>>>
>> Hi Stephen,
>>
>> the addition of the dtsi snippets for the ethernet core is pending due
>> to this patch. getting it into v4.11 would be geat so that i can sent
>> dtsi changes during the next merge window.
>>
> Sorry, I never saw anything from anyone @mediatek so I never did
> anything here. I suppose I'll just go and apply this to clk-next
> now and then v4.13 can get the ethernet patches?
>
Hi Stephen,
sounds like a plan. i'll add the series to my local pending tree and
resend stuff during the next merge window if it was not merged by then.
John
^ permalink raw reply
* [PATCH 4/4] mmc: pxamci: Fix race condition between pxamci_dma_irq() and pxamci_irq()
From: Robert Jarzmik @ 2017-04-19 19:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <31e332fa-f152-1eff-39fb-91f332b84757@tul.cz>
Petr Cvek <petr.cvek@tul.cz> writes:
> The data write requests may require an FIFO flush when the DMA transaction
> ends. This is handled by a DMA callback pxamci_dma_irq(). After flushing
> the FIFO the MCI controller generates the DATA_TRAN_DONE interrupt.
>
> Problem is the DATA_TRAN_DONE interrupt will be generated when the write
> data length is divisible by the FIFO size (no flush is required). And in
> this case the DMA callback can be called long time after the
> DATA_TRAN_DONE interrupt (as the DMA callback is realised by a tasklet,
> it can even stack). When the DMA callback is finally called there can
> already be a different type of the transaction (another data read or write
> request).
>
> The dmaengine_tx_status() will be called for a wrong DMA transaction and
> in some case it returns DMA_IN_PROGRESS, which the code recognize as
> an error and ends a running DMA and halts the MCI controller.
>
> The problem presents itself under heavy (interrupt) load with a high MCI
> traffic with this message:
>
> mmc0: DMA error on tx channel
>
> The fix must obey these situations:
> - Any command will erase the FIFO
> - Data writes divisible by the FIFO size will (probably) automatically
> generate a DATA_TRAN_DONE interrupt
> - Data writes with a nonzero FIFO remainder must be flushed and then MCI
> generates a DATA_TRAN_DONE interrupt
> - Data reads do not require a flush but they will generate
> a DATA_TRAN_DONE interrupt
>
> The fix changes the DATA_TRAN_DONE interrupt enable from read/write
> requests to read requests. The DATA_TRAN_DONE interrupt for a write
> request is enabled in the DMA callback, this assures a DATA_TRAN_DONE
> interrupt will be always called after a callback (with or without an FIFO
> flush).
I'm a bit concerned with the way this patch works.
What bothers me is the re-enabling of the interrupt source in the DMA completion
path, ie. in pxamci_dma_irq().
For example, imagine :
- the tran_done bit is left set (for whatever reason)
- a new transation is queued
- the DMA finishes, but not the last request
- the pxamci_enable_irq() enables the interrupt, which fires right away even if
the tran_done for this interrupt wasn't yet set
I will need a bit more time to think this one through, as I'm not yet set about
all the consequences. That shouldn't prevent you from pushing for reviews of
these patches of course, as I think this serie (or an equivalent) is required to
fix the current race condition.
As this is the last patch, I wonder if this serie is bisectable, especially is
patch 1/4 self contained ?
Cheers.
--
Robert
^ permalink raw reply
* [PATCH] arm64: xen: Implement EFI reset_system callback
From: Daniel Kiper @ 2017-04-19 19:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418134650.GL24360@codeblueprint.co.uk>
On Tue, Apr 18, 2017 at 02:46:50PM +0100, Matt Fleming wrote:
> On Thu, 06 Apr, at 04:55:11PM, Mark Rutland wrote:
> >
> > Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
> > rather than spreading it further.
> >
> > IMO, given reset_system is a *mandatory* function, the Xen wrapper
> > should provide an implementation.
> >
> > I don't see why you can't implement a wrapper that calls the usual Xen
> > poweroff/reset functions.
>
> I realise I'm making a sweeping generalisation, but adding
> EFI_PARAVIRT is almost always the wrong thing to do.
Why?
> I'd much prefer to see Mark's idea implemented.
If you wish I do not object.
Daniel
^ permalink raw reply
* [PATCH] arm64: xen: Implement EFI reset_system callback
From: Matt Fleming @ 2017-04-19 19:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170419192906.GO16658@olila.local.net-space.pl>
On Wed, 19 Apr, at 09:29:06PM, Daniel Kiper wrote:
> On Tue, Apr 18, 2017 at 02:46:50PM +0100, Matt Fleming wrote:
> > On Thu, 06 Apr, at 04:55:11PM, Mark Rutland wrote:
> > >
> > > Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
> > > rather than spreading it further.
> > >
> > > IMO, given reset_system is a *mandatory* function, the Xen wrapper
> > > should provide an implementation.
> > >
> > > I don't see why you can't implement a wrapper that calls the usual Xen
> > > poweroff/reset functions.
> >
> > I realise I'm making a sweeping generalisation, but adding
> > EFI_PARAVIRT is almost always the wrong thing to do.
>
> Why?
Because it makes paravirt a special case, and there's usually very
little reason to make it special in the EFI code. Special-casing means
more branches, more code paths, a bigger testing matrix and more
complex code.
EFI_PARAVIRT does have its uses, like for those scenarios where we
don't have a table of function pointers that can be overidden for
paravirt.
But we do have such a table for ->reset_system().
^ permalink raw reply
* [v5,2/8] mfd: retu: Drop -mfd suffix from I2C device ID name
From: Wolfram Sang @ 2017-04-19 19:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170412172800.23035-3-javier@osg.samsung.com>
On Wed, Apr 12, 2017 at 02:27:53PM -0300, Javier Martinez Canillas wrote:
> It's not correct to encode the subsystem in the I2C device name, so
> drop the -mfd suffix. To maintain bisect-ability, change driver and
> platform code / DTS users in the same patch.
>
> Suggested-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Acked-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Acked-by: Tony Lindgren <tony@atomide.com>
Reviewed-by: Wolfram Sang <wsa@the-dreams.de>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170419/6616a017/attachment.sig>
^ permalink raw reply
* [v5,4/8] ARM: dts: n8x0: Add vendor prefix to retu node
From: Wolfram Sang @ 2017-04-19 19:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170412172800.23035-5-javier@osg.samsung.com>
On Wed, Apr 12, 2017 at 02:27:55PM -0300, Javier Martinez Canillas wrote:
> The retu device node doesn't have a vendor prefix
> in its compatible string, fix it by adding one.
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Acked-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Acked-by: Tony Lindgren <tony@atomide.com>
Reviewed-by: Wolfram Sang <wsa@the-dreams.de>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170419/8e84b1be/attachment-0001.sig>
^ permalink raw reply
* [PATCH] arm64: xen: Implement EFI reset_system callback
From: Daniel Kiper @ 2017-04-19 19:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170419193738.GM24360@codeblueprint.co.uk>
On Wed, Apr 19, 2017 at 08:37:38PM +0100, Matt Fleming wrote:
> On Wed, 19 Apr, at 09:29:06PM, Daniel Kiper wrote:
> > On Tue, Apr 18, 2017 at 02:46:50PM +0100, Matt Fleming wrote:
> > > On Thu, 06 Apr, at 04:55:11PM, Mark Rutland wrote:
> > > >
> > > > Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
> > > > rather than spreading it further.
> > > >
> > > > IMO, given reset_system is a *mandatory* function, the Xen wrapper
> > > > should provide an implementation.
> > > >
> > > > I don't see why you can't implement a wrapper that calls the usual Xen
> > > > poweroff/reset functions.
> > >
> > > I realise I'm making a sweeping generalisation, but adding
> > > EFI_PARAVIRT is almost always the wrong thing to do.
> >
> > Why?
>
> Because it makes paravirt a special case, and there's usually very
> little reason to make it special in the EFI code. Special-casing means
> more branches, more code paths, a bigger testing matrix and more
> complex code.
>
> EFI_PARAVIRT does have its uses, like for those scenarios where we
> don't have a table of function pointers that can be overidden for
> paravirt.
>
> But we do have such a table for ->reset_system().
This is more or less what I expected. Thanks a lot for explanation.
Daniel
^ permalink raw reply
* [PATCH 1/6] ARM/dma: pl08x: pass reasonable memcpy settings
From: Arnd Bergmann @ 2017-04-19 19:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170419135801.GH29219@localhost>
On Wed, Apr 19, 2017 at 3:58 PM, Olof Johansson <olof@lixom.net> wrote:
> On Sat, Apr 08, 2017 at 02:04:52PM +0200, Linus Walleij wrote:
>> We cannot use bits from configuration registers as API between
>> platforms and driver like this, abstract it out to two enums
>> and mimic the stuff passed as device tree data.
>>
>> This is done to make it possible for the driver to generate the
>> ccfg word on-the-fly so we can support more PL08x derivatives.
>>
>> Cc: arm at kernel.org
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> ARM SoC folks: I'd like your ACK on this eventually, it needs to
>> go through the DMA engine tree with the patches following this one.
>
> Acked-by: Olof Johansson <olof@lixom.net>
Very nice cleanup!
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox