* [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-13 23:45 ` Suman Anna
0 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-13 23:45 UTC (permalink / raw)
To: linux-arm-kernel
The Texas Instrument's Keystone 2 family of SoCs has 1 or more
TMS320C66x DSP Core Subsystems (C66x CorePacs). Each subsystem has
a C66x Fixed/Floating-Point DSP Core, with 32KB of L1P and L1D SRAMs,
that can be configured and partitioned as either RAM and/or Cache,
and 1 MB of L2 SRAM. The CorePac also includes an Internal DMA (IDMA),
External Memory Controller (EMC), Extended Memory Controller (XMC)
with a Memory Protection and Address Extension (MPAX) unit, a Bandwidth
Management (BWM) unit, an Interrupt Controller (INTC) and a Powerdown
Controller (PDC).
A new remoteproc module is added to perform the device management of
these DSP devices. The driver expects the firmware names to be of the
form "keystone-dsp<X>-fw", where X is the corresponding DSP number, and
uses the standard remoteproc core ELF loader. The support is limited
to images only using the DSP internal memories at the moment. This
remoteproc driver is also designed to work with virtio, and uses the
IPC Generation registers for performing the virtio signalling and
getting notified of exceptions.
The driver currently supports the 66AK2H/66AK2K, 66AK2L and 66AK2E
SoCs.
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Sam Nelson <sam.nelson@ti.com>
Signed-off-by: Andrew F. Davis <afd@ti.com>
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>
---
v3: No code changes, picked up Santosh's Ack
v2: https://patchwork.kernel.org/patch/9773685/
drivers/remoteproc/Kconfig | 13 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/keystone_remoteproc.c | 515 +++++++++++++++++++++++++++++++
3 files changed, 529 insertions(+)
create mode 100644 drivers/remoteproc/keystone_remoteproc.c
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..9839b35b6ab3 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -71,6 +71,19 @@ config DA8XX_REMOTEPROC
It's safe to say n here if you're not interested in multimedia
offloading.
+config KEYSTONE_REMOTEPROC
+ tristate "Keystone Remoteproc support"
+ depends on ARCH_KEYSTONE
+ depends on RESET_CONTROLLER
+ depends on REMOTEPROC
+ select RPMSG_VIRTIO
+ help
+ Say Y here here to support Keystone remote processors (DSP)
+ via the remote processor framework.
+
+ It's safe to say N here if you're not interested in the Keystone
+ DSPs or just want to use a bare minimum kernel.
+
config QCOM_ADSP_PIL
tristate "Qualcomm ADSP Peripheral Image Loader"
depends on OF && ARCH_QCOM
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..f1ce5fc8a2f3 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -11,6 +11,7 @@ remoteproc-y += remoteproc_elf_loader.o
obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
obj-$(CONFIG_WKUP_M3_RPROC) += wkup_m3_rproc.o
obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o
+obj-$(CONFIG_KEYSTONE_REMOTEPROC) += keystone_remoteproc.o
obj-$(CONFIG_QCOM_ADSP_PIL) += qcom_adsp_pil.o
obj-$(CONFIG_QCOM_RPROC_COMMON) += qcom_common.o
obj-$(CONFIG_QCOM_Q6V5_PIL) += qcom_q6v5_pil.o
diff --git a/drivers/remoteproc/keystone_remoteproc.c b/drivers/remoteproc/keystone_remoteproc.c
new file mode 100644
index 000000000000..6e09ef76f7c7
--- /dev/null
+++ b/drivers/remoteproc/keystone_remoteproc.c
@@ -0,0 +1,515 @@
+/*
+ * TI Keystone DSP remoteproc driver
+ *
+ * Copyright (C) 2015-2017 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/workqueue.h>
+#include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/remoteproc.h>
+#include <linux/reset.h>
+
+#include "remoteproc_internal.h"
+
+#define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK (SZ_16M - 1)
+
+/**
+ * struct keystone_rproc_mem - internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @dev_addr: Device address of the memory region from DSP view
+ * @size: Size of the memory region
+ */
+struct keystone_rproc_mem {
+ void __iomem *cpu_addr;
+ phys_addr_t bus_addr;
+ u32 dev_addr;
+ size_t size;
+};
+
+/**
+ * struct keystone_rproc - keystone remote processor driver structure
+ * @dev: cached device pointer
+ * @rproc: remoteproc device handle
+ * @mem: internal memory regions data
+ * @num_mems: number of internal memory regions
+ * @dev_ctrl: device control regmap handle
+ * @reset: reset control handle
+ * @boot_offset: boot register offset in @dev_ctrl regmap
+ * @irq_ring: irq entry for vring
+ * @irq_fault: irq entry for exception
+ * @kick_gpio: gpio used for virtio kicks
+ * @workqueue: workqueue for processing virtio interrupts
+ */
+struct keystone_rproc {
+ struct device *dev;
+ struct rproc *rproc;
+ struct keystone_rproc_mem *mem;
+ int num_mems;
+ struct regmap *dev_ctrl;
+ struct reset_control *reset;
+ u32 boot_offset;
+ int irq_ring;
+ int irq_fault;
+ int kick_gpio;
+ struct work_struct workqueue;
+};
+
+/* Put the DSP processor into reset */
+static void keystone_rproc_dsp_reset(struct keystone_rproc *ksproc)
+{
+ reset_control_assert(ksproc->reset);
+}
+
+/* Configure the boot address and boot the DSP processor */
+static int keystone_rproc_dsp_boot(struct keystone_rproc *ksproc, u32 boot_addr)
+{
+ int ret;
+
+ if (boot_addr & (SZ_1K - 1)) {
+ dev_err(ksproc->dev, "invalid boot address 0x%x, must be aligned on a 1KB boundary\n",
+ boot_addr);
+ return -EINVAL;
+ }
+
+ ret = regmap_write(ksproc->dev_ctrl, ksproc->boot_offset, boot_addr);
+ if (ret) {
+ dev_err(ksproc->dev, "regmap_write of boot address failed, status = %d\n",
+ ret);
+ return ret;
+ }
+
+ reset_control_deassert(ksproc->reset);
+
+ return 0;
+}
+
+/*
+ * Process the remoteproc exceptions
+ *
+ * The exception reporting on Keystone DSP remote processors is very simple
+ * compared to the equivalent processors on the OMAP family, it is notified
+ * through a software-designed specific interrupt source in the IPC interrupt
+ * generation register.
+ *
+ * This function just invokes the rproc_report_crash to report the exception
+ * to the remoteproc driver core, to trigger a recovery.
+ */
+static irqreturn_t keystone_rproc_exception_interrupt(int irq, void *dev_id)
+{
+ struct keystone_rproc *ksproc = dev_id;
+
+ rproc_report_crash(ksproc->rproc, RPROC_FATAL_ERROR);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Main virtqueue message workqueue function
+ *
+ * This function is executed upon scheduling of the keystone remoteproc
+ * driver's workqueue. The workqueue is scheduled by the vring ISR handler.
+ *
+ * There is no payload message indicating the virtqueue index as is the
+ * case with mailbox-based implementations on OMAP family. As such, this
+ * handler processes both the Tx and Rx virtqueue indices on every invocation.
+ * The rproc_vq_interrupt function can detect if there are new unprocessed
+ * messages or not (returns IRQ_NONE vs IRQ_HANDLED), but there is no need
+ * to check for these return values. The index 0 triggering will process all
+ * pending Rx buffers, and the index 1 triggering will process all newly
+ * available Tx buffers and will wakeup any potentially blocked senders.
+ *
+ * NOTE:
+ * 1. A payload could be added by using some of the source bits in the
+ * IPC interrupt generation registers, but this would need additional
+ * changes to the overall IPC stack, and currently there are no benefits
+ * of adapting that approach.
+ * 2. The current logic is based on an inherent design assumption of supporting
+ * only 2 vrings, but this can be changed if needed.
+ */
+static void handle_event(struct work_struct *work)
+{
+ struct keystone_rproc *ksproc =
+ container_of(work, struct keystone_rproc, workqueue);
+
+ rproc_vq_interrupt(ksproc->rproc, 0);
+ rproc_vq_interrupt(ksproc->rproc, 1);
+}
+
+/*
+ * Interrupt handler for processing vring kicks from remote processor
+ */
+static irqreturn_t keystone_rproc_vring_interrupt(int irq, void *dev_id)
+{
+ struct keystone_rproc *ksproc = dev_id;
+
+ schedule_work(&ksproc->workqueue);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Power up the DSP remote processor.
+ *
+ * This function will be invoked only after the firmware for this rproc
+ * was loaded, parsed successfully, and all of its resource requirements
+ * were met.
+ */
+static int keystone_rproc_start(struct rproc *rproc)
+{
+ struct keystone_rproc *ksproc = rproc->priv;
+ int ret;
+
+ INIT_WORK(&ksproc->workqueue, handle_event);
+
+ ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
+ dev_name(ksproc->dev), ksproc);
+ if (ret) {
+ dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
+ ret);
+ goto out;
+ }
+
+ ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
+ 0, dev_name(ksproc->dev), ksproc);
+ if (ret) {
+ dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
+ ret);
+ goto free_vring_irq;
+ }
+
+ ret = keystone_rproc_dsp_boot(ksproc, rproc->bootaddr);
+ if (ret)
+ goto free_exc_irq;
+
+ return 0;
+
+free_exc_irq:
+ free_irq(ksproc->irq_fault, ksproc);
+free_vring_irq:
+ free_irq(ksproc->irq_ring, ksproc);
+ flush_work(&ksproc->workqueue);
+out:
+ return ret;
+}
+
+/*
+ * Stop the DSP remote processor.
+ *
+ * This function puts the DSP processor into reset, and finishes processing
+ * of any pending messages.
+ */
+static int keystone_rproc_stop(struct rproc *rproc)
+{
+ struct keystone_rproc *ksproc = rproc->priv;
+
+ keystone_rproc_dsp_reset(ksproc);
+ free_irq(ksproc->irq_fault, ksproc);
+ free_irq(ksproc->irq_ring, ksproc);
+ flush_work(&ksproc->workqueue);
+
+ return 0;
+}
+
+/*
+ * Kick the remote processor to notify about pending unprocessed messages.
+ * The vqid usage is not used and is inconsequential, as the kick is performed
+ * through a simulated GPIO (a bit in an IPC interrupt-triggering register),
+ * the remote processor is expected to process both its Tx and Rx virtqueues.
+ */
+static void keystone_rproc_kick(struct rproc *rproc, int vqid)
+{
+ struct keystone_rproc *ksproc = rproc->priv;
+
+ if (WARN_ON(ksproc->kick_gpio < 0))
+ return;
+
+ gpio_set_value(ksproc->kick_gpio, 1);
+}
+
+/*
+ * Custom function to translate a DSP device address (internal RAMs only) to a
+ * kernel virtual address. The DSPs can access their RAMs at either an internal
+ * address visible only from a DSP, or at the SoC-level bus address. Both these
+ * addresses need to be looked through for translation. The translated addresses
+ * can be used either by the remoteproc core for loading (when using kernel
+ * remoteproc loader), or by any rpmsg bus drivers.
+ */
+static void *keystone_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+ struct keystone_rproc *ksproc = rproc->priv;
+ void __iomem *va = NULL;
+ phys_addr_t bus_addr;
+ u32 dev_addr, offset;
+ size_t size;
+ int i;
+
+ if (len <= 0)
+ return NULL;
+
+ for (i = 0; i < ksproc->num_mems; i++) {
+ bus_addr = ksproc->mem[i].bus_addr;
+ dev_addr = ksproc->mem[i].dev_addr;
+ size = ksproc->mem[i].size;
+
+ if (da < KEYSTONE_RPROC_LOCAL_ADDRESS_MASK) {
+ /* handle DSP-view addresses */
+ if ((da >= dev_addr) &&
+ ((da + len) <= (dev_addr + size))) {
+ offset = da - dev_addr;
+ va = ksproc->mem[i].cpu_addr + offset;
+ break;
+ }
+ } else {
+ /* handle SoC-view addresses */
+ if ((da >= bus_addr) &&
+ (da + len) <= (bus_addr + size)) {
+ offset = da - bus_addr;
+ va = ksproc->mem[i].cpu_addr + offset;
+ break;
+ }
+ }
+ }
+
+ return (__force void *)va;
+}
+
+static const struct rproc_ops keystone_rproc_ops = {
+ .start = keystone_rproc_start,
+ .stop = keystone_rproc_stop,
+ .kick = keystone_rproc_kick,
+ .da_to_va = keystone_rproc_da_to_va,
+};
+
+static int keystone_rproc_of_get_memories(struct platform_device *pdev,
+ struct keystone_rproc *ksproc)
+{
+ static const char * const mem_names[] = {"l2sram", "l1pram", "l1dram"};
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ int num_mems = 0;
+ int i;
+
+ num_mems = ARRAY_SIZE(mem_names);
+ ksproc->mem = devm_kcalloc(ksproc->dev, num_mems,
+ sizeof(*ksproc->mem), GFP_KERNEL);
+ if (!ksproc->mem)
+ return -ENOMEM;
+
+ for (i = 0; i < num_mems; i++) {
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ mem_names[i]);
+ ksproc->mem[i].cpu_addr = devm_ioremap_resource(dev, res);
+ if (IS_ERR(ksproc->mem[i].cpu_addr)) {
+ dev_err(dev, "failed to parse and map %s memory\n",
+ mem_names[i]);
+ return PTR_ERR(ksproc->mem[i].cpu_addr);
+ }
+ ksproc->mem[i].bus_addr = res->start;
+ ksproc->mem[i].dev_addr =
+ res->start & KEYSTONE_RPROC_LOCAL_ADDRESS_MASK;
+ ksproc->mem[i].size = resource_size(res);
+
+ /* zero out memories to start in a pristine state */
+ memset((__force void *)ksproc->mem[i].cpu_addr, 0,
+ ksproc->mem[i].size);
+ }
+ ksproc->num_mems = num_mems;
+
+ return 0;
+}
+
+static int keystone_rproc_of_get_dev_syscon(struct platform_device *pdev,
+ struct keystone_rproc *ksproc)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ if (!of_property_read_bool(np, "ti,syscon-dev")) {
+ dev_err(dev, "ti,syscon-dev property is absent\n");
+ return -EINVAL;
+ }
+
+ ksproc->dev_ctrl =
+ syscon_regmap_lookup_by_phandle(np, "ti,syscon-dev");
+ if (IS_ERR(ksproc->dev_ctrl)) {
+ ret = PTR_ERR(ksproc->dev_ctrl);
+ return ret;
+ }
+
+ if (of_property_read_u32_index(np, "ti,syscon-dev", 1,
+ &ksproc->boot_offset)) {
+ dev_err(dev, "couldn't read the boot register offset\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int keystone_rproc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct keystone_rproc *ksproc;
+ struct rproc *rproc;
+ int dsp_id;
+ char *fw_name = NULL;
+ char *template = "keystone-dsp%d-fw";
+ int name_len = 0;
+ int ret = 0;
+
+ if (!np) {
+ dev_err(dev, "only DT-based devices are supported\n");
+ return -ENODEV;
+ }
+
+ dsp_id = of_alias_get_id(np, "rproc");
+ if (dsp_id < 0) {
+ dev_warn(dev, "device does not have an alias id\n");
+ return dsp_id;
+ }
+
+ /* construct a custom default fw name - subject to change in future */
+ name_len = strlen(template); /* assuming a single digit alias */
+ fw_name = devm_kzalloc(dev, name_len, GFP_KERNEL);
+ if (!fw_name)
+ return -ENOMEM;
+ snprintf(fw_name, name_len, template, dsp_id);
+
+ rproc = rproc_alloc(dev, dev_name(dev), &keystone_rproc_ops, fw_name,
+ sizeof(*ksproc));
+ if (!rproc)
+ return -ENOMEM;
+
+ rproc->has_iommu = false;
+ ksproc = rproc->priv;
+ ksproc->rproc = rproc;
+ ksproc->dev = dev;
+
+ ret = keystone_rproc_of_get_dev_syscon(pdev, ksproc);
+ if (ret)
+ goto free_rproc;
+
+ ksproc->reset = devm_reset_control_get(dev, NULL);
+ if (IS_ERR(ksproc->reset)) {
+ ret = PTR_ERR(ksproc->reset);
+ goto free_rproc;
+ }
+
+ /* enable clock for accessing DSP internal memories */
+ pm_runtime_enable(dev);
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable clock, status = %d\n", ret);
+ pm_runtime_put_noidle(dev);
+ goto disable_rpm;
+ }
+
+ ret = keystone_rproc_of_get_memories(pdev, ksproc);
+ if (ret)
+ goto disable_clk;
+
+ ksproc->irq_ring = platform_get_irq_byname(pdev, "vring");
+ if (ksproc->irq_ring < 0) {
+ ret = ksproc->irq_ring;
+ dev_err(dev, "failed to get vring interrupt, status = %d\n",
+ ret);
+ goto disable_clk;
+ }
+
+ ksproc->irq_fault = platform_get_irq_byname(pdev, "exception");
+ if (ksproc->irq_fault < 0) {
+ ret = ksproc->irq_fault;
+ dev_err(dev, "failed to get exception interrupt, status = %d\n",
+ ret);
+ goto disable_clk;
+ }
+
+ ksproc->kick_gpio = of_get_named_gpio_flags(np, "kick-gpios", 0, NULL);
+ if (ksproc->kick_gpio < 0) {
+ ret = ksproc->kick_gpio;
+ dev_err(dev, "failed to get gpio for virtio kicks, status = %d\n",
+ ret);
+ goto disable_clk;
+ }
+
+ if (of_reserved_mem_device_init(dev))
+ dev_warn(dev, "device does not have specific CMA pool\n");
+
+ ret = rproc_add(rproc);
+ if (ret) {
+ dev_err(dev, "failed to add register device with remoteproc core, status = %d\n",
+ ret);
+ goto release_mem;
+ }
+
+ platform_set_drvdata(pdev, ksproc);
+
+ return 0;
+
+release_mem:
+ of_reserved_mem_device_release(dev);
+disable_clk:
+ pm_runtime_put_sync(dev);
+disable_rpm:
+ pm_runtime_disable(dev);
+free_rproc:
+ rproc_free(rproc);
+ return ret;
+}
+
+static int keystone_rproc_remove(struct platform_device *pdev)
+{
+ struct keystone_rproc *ksproc = platform_get_drvdata(pdev);
+
+ rproc_del(ksproc->rproc);
+ pm_runtime_put_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+ rproc_free(ksproc->rproc);
+ of_reserved_mem_device_release(&pdev->dev);
+
+ return 0;
+}
+
+static const struct of_device_id keystone_rproc_of_match[] = {
+ { .compatible = "ti,k2hk-dsp", },
+ { .compatible = "ti,k2l-dsp", },
+ { .compatible = "ti,k2e-dsp", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, keystone_rproc_of_match);
+
+static struct platform_driver keystone_rproc_driver = {
+ .probe = keystone_rproc_probe,
+ .remove = keystone_rproc_remove,
+ .driver = {
+ .name = "keystone-rproc",
+ .of_match_table = keystone_rproc_of_match,
+ },
+};
+
+module_platform_driver(keystone_rproc_driver);
+
+MODULE_AUTHOR("Suman Anna <s-anna@ti.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TI Keystone DSP Remoteproc driver");
--
2.13.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-13 23:45 ` Suman Anna
0 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-13 23:45 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen
Cc: Rob Herring, Santosh Shilimkar, Mark Rutland, linux-remoteproc,
linux-arm-kernel, devicetree, linux-kernel, Suman Anna,
Andrew F. Davis, Sam Nelson
The Texas Instrument's Keystone 2 family of SoCs has 1 or more
TMS320C66x DSP Core Subsystems (C66x CorePacs). Each subsystem has
a C66x Fixed/Floating-Point DSP Core, with 32KB of L1P and L1D SRAMs,
that can be configured and partitioned as either RAM and/or Cache,
and 1 MB of L2 SRAM. The CorePac also includes an Internal DMA (IDMA),
External Memory Controller (EMC), Extended Memory Controller (XMC)
with a Memory Protection and Address Extension (MPAX) unit, a Bandwidth
Management (BWM) unit, an Interrupt Controller (INTC) and a Powerdown
Controller (PDC).
A new remoteproc module is added to perform the device management of
these DSP devices. The driver expects the firmware names to be of the
form "keystone-dsp<X>-fw", where X is the corresponding DSP number, and
uses the standard remoteproc core ELF loader. The support is limited
to images only using the DSP internal memories at the moment. This
remoteproc driver is also designed to work with virtio, and uses the
IPC Generation registers for performing the virtio signalling and
getting notified of exceptions.
The driver currently supports the 66AK2H/66AK2K, 66AK2L and 66AK2E
SoCs.
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Sam Nelson <sam.nelson@ti.com>
Signed-off-by: Andrew F. Davis <afd@ti.com>
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>
---
v3: No code changes, picked up Santosh's Ack
v2: https://patchwork.kernel.org/patch/9773685/
drivers/remoteproc/Kconfig | 13 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/keystone_remoteproc.c | 515 +++++++++++++++++++++++++++++++
3 files changed, 529 insertions(+)
create mode 100644 drivers/remoteproc/keystone_remoteproc.c
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..9839b35b6ab3 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -71,6 +71,19 @@ config DA8XX_REMOTEPROC
It's safe to say n here if you're not interested in multimedia
offloading.
+config KEYSTONE_REMOTEPROC
+ tristate "Keystone Remoteproc support"
+ depends on ARCH_KEYSTONE
+ depends on RESET_CONTROLLER
+ depends on REMOTEPROC
+ select RPMSG_VIRTIO
+ help
+ Say Y here here to support Keystone remote processors (DSP)
+ via the remote processor framework.
+
+ It's safe to say N here if you're not interested in the Keystone
+ DSPs or just want to use a bare minimum kernel.
+
config QCOM_ADSP_PIL
tristate "Qualcomm ADSP Peripheral Image Loader"
depends on OF && ARCH_QCOM
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..f1ce5fc8a2f3 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -11,6 +11,7 @@ remoteproc-y += remoteproc_elf_loader.o
obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
obj-$(CONFIG_WKUP_M3_RPROC) += wkup_m3_rproc.o
obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o
+obj-$(CONFIG_KEYSTONE_REMOTEPROC) += keystone_remoteproc.o
obj-$(CONFIG_QCOM_ADSP_PIL) += qcom_adsp_pil.o
obj-$(CONFIG_QCOM_RPROC_COMMON) += qcom_common.o
obj-$(CONFIG_QCOM_Q6V5_PIL) += qcom_q6v5_pil.o
diff --git a/drivers/remoteproc/keystone_remoteproc.c b/drivers/remoteproc/keystone_remoteproc.c
new file mode 100644
index 000000000000..6e09ef76f7c7
--- /dev/null
+++ b/drivers/remoteproc/keystone_remoteproc.c
@@ -0,0 +1,515 @@
+/*
+ * TI Keystone DSP remoteproc driver
+ *
+ * Copyright (C) 2015-2017 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/workqueue.h>
+#include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/remoteproc.h>
+#include <linux/reset.h>
+
+#include "remoteproc_internal.h"
+
+#define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK (SZ_16M - 1)
+
+/**
+ * struct keystone_rproc_mem - internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @dev_addr: Device address of the memory region from DSP view
+ * @size: Size of the memory region
+ */
+struct keystone_rproc_mem {
+ void __iomem *cpu_addr;
+ phys_addr_t bus_addr;
+ u32 dev_addr;
+ size_t size;
+};
+
+/**
+ * struct keystone_rproc - keystone remote processor driver structure
+ * @dev: cached device pointer
+ * @rproc: remoteproc device handle
+ * @mem: internal memory regions data
+ * @num_mems: number of internal memory regions
+ * @dev_ctrl: device control regmap handle
+ * @reset: reset control handle
+ * @boot_offset: boot register offset in @dev_ctrl regmap
+ * @irq_ring: irq entry for vring
+ * @irq_fault: irq entry for exception
+ * @kick_gpio: gpio used for virtio kicks
+ * @workqueue: workqueue for processing virtio interrupts
+ */
+struct keystone_rproc {
+ struct device *dev;
+ struct rproc *rproc;
+ struct keystone_rproc_mem *mem;
+ int num_mems;
+ struct regmap *dev_ctrl;
+ struct reset_control *reset;
+ u32 boot_offset;
+ int irq_ring;
+ int irq_fault;
+ int kick_gpio;
+ struct work_struct workqueue;
+};
+
+/* Put the DSP processor into reset */
+static void keystone_rproc_dsp_reset(struct keystone_rproc *ksproc)
+{
+ reset_control_assert(ksproc->reset);
+}
+
+/* Configure the boot address and boot the DSP processor */
+static int keystone_rproc_dsp_boot(struct keystone_rproc *ksproc, u32 boot_addr)
+{
+ int ret;
+
+ if (boot_addr & (SZ_1K - 1)) {
+ dev_err(ksproc->dev, "invalid boot address 0x%x, must be aligned on a 1KB boundary\n",
+ boot_addr);
+ return -EINVAL;
+ }
+
+ ret = regmap_write(ksproc->dev_ctrl, ksproc->boot_offset, boot_addr);
+ if (ret) {
+ dev_err(ksproc->dev, "regmap_write of boot address failed, status = %d\n",
+ ret);
+ return ret;
+ }
+
+ reset_control_deassert(ksproc->reset);
+
+ return 0;
+}
+
+/*
+ * Process the remoteproc exceptions
+ *
+ * The exception reporting on Keystone DSP remote processors is very simple
+ * compared to the equivalent processors on the OMAP family, it is notified
+ * through a software-designed specific interrupt source in the IPC interrupt
+ * generation register.
+ *
+ * This function just invokes the rproc_report_crash to report the exception
+ * to the remoteproc driver core, to trigger a recovery.
+ */
+static irqreturn_t keystone_rproc_exception_interrupt(int irq, void *dev_id)
+{
+ struct keystone_rproc *ksproc = dev_id;
+
+ rproc_report_crash(ksproc->rproc, RPROC_FATAL_ERROR);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Main virtqueue message workqueue function
+ *
+ * This function is executed upon scheduling of the keystone remoteproc
+ * driver's workqueue. The workqueue is scheduled by the vring ISR handler.
+ *
+ * There is no payload message indicating the virtqueue index as is the
+ * case with mailbox-based implementations on OMAP family. As such, this
+ * handler processes both the Tx and Rx virtqueue indices on every invocation.
+ * The rproc_vq_interrupt function can detect if there are new unprocessed
+ * messages or not (returns IRQ_NONE vs IRQ_HANDLED), but there is no need
+ * to check for these return values. The index 0 triggering will process all
+ * pending Rx buffers, and the index 1 triggering will process all newly
+ * available Tx buffers and will wakeup any potentially blocked senders.
+ *
+ * NOTE:
+ * 1. A payload could be added by using some of the source bits in the
+ * IPC interrupt generation registers, but this would need additional
+ * changes to the overall IPC stack, and currently there are no benefits
+ * of adapting that approach.
+ * 2. The current logic is based on an inherent design assumption of supporting
+ * only 2 vrings, but this can be changed if needed.
+ */
+static void handle_event(struct work_struct *work)
+{
+ struct keystone_rproc *ksproc =
+ container_of(work, struct keystone_rproc, workqueue);
+
+ rproc_vq_interrupt(ksproc->rproc, 0);
+ rproc_vq_interrupt(ksproc->rproc, 1);
+}
+
+/*
+ * Interrupt handler for processing vring kicks from remote processor
+ */
+static irqreturn_t keystone_rproc_vring_interrupt(int irq, void *dev_id)
+{
+ struct keystone_rproc *ksproc = dev_id;
+
+ schedule_work(&ksproc->workqueue);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Power up the DSP remote processor.
+ *
+ * This function will be invoked only after the firmware for this rproc
+ * was loaded, parsed successfully, and all of its resource requirements
+ * were met.
+ */
+static int keystone_rproc_start(struct rproc *rproc)
+{
+ struct keystone_rproc *ksproc = rproc->priv;
+ int ret;
+
+ INIT_WORK(&ksproc->workqueue, handle_event);
+
+ ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
+ dev_name(ksproc->dev), ksproc);
+ if (ret) {
+ dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
+ ret);
+ goto out;
+ }
+
+ ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
+ 0, dev_name(ksproc->dev), ksproc);
+ if (ret) {
+ dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
+ ret);
+ goto free_vring_irq;
+ }
+
+ ret = keystone_rproc_dsp_boot(ksproc, rproc->bootaddr);
+ if (ret)
+ goto free_exc_irq;
+
+ return 0;
+
+free_exc_irq:
+ free_irq(ksproc->irq_fault, ksproc);
+free_vring_irq:
+ free_irq(ksproc->irq_ring, ksproc);
+ flush_work(&ksproc->workqueue);
+out:
+ return ret;
+}
+
+/*
+ * Stop the DSP remote processor.
+ *
+ * This function puts the DSP processor into reset, and finishes processing
+ * of any pending messages.
+ */
+static int keystone_rproc_stop(struct rproc *rproc)
+{
+ struct keystone_rproc *ksproc = rproc->priv;
+
+ keystone_rproc_dsp_reset(ksproc);
+ free_irq(ksproc->irq_fault, ksproc);
+ free_irq(ksproc->irq_ring, ksproc);
+ flush_work(&ksproc->workqueue);
+
+ return 0;
+}
+
+/*
+ * Kick the remote processor to notify about pending unprocessed messages.
+ * The vqid usage is not used and is inconsequential, as the kick is performed
+ * through a simulated GPIO (a bit in an IPC interrupt-triggering register),
+ * the remote processor is expected to process both its Tx and Rx virtqueues.
+ */
+static void keystone_rproc_kick(struct rproc *rproc, int vqid)
+{
+ struct keystone_rproc *ksproc = rproc->priv;
+
+ if (WARN_ON(ksproc->kick_gpio < 0))
+ return;
+
+ gpio_set_value(ksproc->kick_gpio, 1);
+}
+
+/*
+ * Custom function to translate a DSP device address (internal RAMs only) to a
+ * kernel virtual address. The DSPs can access their RAMs at either an internal
+ * address visible only from a DSP, or at the SoC-level bus address. Both these
+ * addresses need to be looked through for translation. The translated addresses
+ * can be used either by the remoteproc core for loading (when using kernel
+ * remoteproc loader), or by any rpmsg bus drivers.
+ */
+static void *keystone_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+ struct keystone_rproc *ksproc = rproc->priv;
+ void __iomem *va = NULL;
+ phys_addr_t bus_addr;
+ u32 dev_addr, offset;
+ size_t size;
+ int i;
+
+ if (len <= 0)
+ return NULL;
+
+ for (i = 0; i < ksproc->num_mems; i++) {
+ bus_addr = ksproc->mem[i].bus_addr;
+ dev_addr = ksproc->mem[i].dev_addr;
+ size = ksproc->mem[i].size;
+
+ if (da < KEYSTONE_RPROC_LOCAL_ADDRESS_MASK) {
+ /* handle DSP-view addresses */
+ if ((da >= dev_addr) &&
+ ((da + len) <= (dev_addr + size))) {
+ offset = da - dev_addr;
+ va = ksproc->mem[i].cpu_addr + offset;
+ break;
+ }
+ } else {
+ /* handle SoC-view addresses */
+ if ((da >= bus_addr) &&
+ (da + len) <= (bus_addr + size)) {
+ offset = da - bus_addr;
+ va = ksproc->mem[i].cpu_addr + offset;
+ break;
+ }
+ }
+ }
+
+ return (__force void *)va;
+}
+
+static const struct rproc_ops keystone_rproc_ops = {
+ .start = keystone_rproc_start,
+ .stop = keystone_rproc_stop,
+ .kick = keystone_rproc_kick,
+ .da_to_va = keystone_rproc_da_to_va,
+};
+
+static int keystone_rproc_of_get_memories(struct platform_device *pdev,
+ struct keystone_rproc *ksproc)
+{
+ static const char * const mem_names[] = {"l2sram", "l1pram", "l1dram"};
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ int num_mems = 0;
+ int i;
+
+ num_mems = ARRAY_SIZE(mem_names);
+ ksproc->mem = devm_kcalloc(ksproc->dev, num_mems,
+ sizeof(*ksproc->mem), GFP_KERNEL);
+ if (!ksproc->mem)
+ return -ENOMEM;
+
+ for (i = 0; i < num_mems; i++) {
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ mem_names[i]);
+ ksproc->mem[i].cpu_addr = devm_ioremap_resource(dev, res);
+ if (IS_ERR(ksproc->mem[i].cpu_addr)) {
+ dev_err(dev, "failed to parse and map %s memory\n",
+ mem_names[i]);
+ return PTR_ERR(ksproc->mem[i].cpu_addr);
+ }
+ ksproc->mem[i].bus_addr = res->start;
+ ksproc->mem[i].dev_addr =
+ res->start & KEYSTONE_RPROC_LOCAL_ADDRESS_MASK;
+ ksproc->mem[i].size = resource_size(res);
+
+ /* zero out memories to start in a pristine state */
+ memset((__force void *)ksproc->mem[i].cpu_addr, 0,
+ ksproc->mem[i].size);
+ }
+ ksproc->num_mems = num_mems;
+
+ return 0;
+}
+
+static int keystone_rproc_of_get_dev_syscon(struct platform_device *pdev,
+ struct keystone_rproc *ksproc)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ if (!of_property_read_bool(np, "ti,syscon-dev")) {
+ dev_err(dev, "ti,syscon-dev property is absent\n");
+ return -EINVAL;
+ }
+
+ ksproc->dev_ctrl =
+ syscon_regmap_lookup_by_phandle(np, "ti,syscon-dev");
+ if (IS_ERR(ksproc->dev_ctrl)) {
+ ret = PTR_ERR(ksproc->dev_ctrl);
+ return ret;
+ }
+
+ if (of_property_read_u32_index(np, "ti,syscon-dev", 1,
+ &ksproc->boot_offset)) {
+ dev_err(dev, "couldn't read the boot register offset\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int keystone_rproc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct keystone_rproc *ksproc;
+ struct rproc *rproc;
+ int dsp_id;
+ char *fw_name = NULL;
+ char *template = "keystone-dsp%d-fw";
+ int name_len = 0;
+ int ret = 0;
+
+ if (!np) {
+ dev_err(dev, "only DT-based devices are supported\n");
+ return -ENODEV;
+ }
+
+ dsp_id = of_alias_get_id(np, "rproc");
+ if (dsp_id < 0) {
+ dev_warn(dev, "device does not have an alias id\n");
+ return dsp_id;
+ }
+
+ /* construct a custom default fw name - subject to change in future */
+ name_len = strlen(template); /* assuming a single digit alias */
+ fw_name = devm_kzalloc(dev, name_len, GFP_KERNEL);
+ if (!fw_name)
+ return -ENOMEM;
+ snprintf(fw_name, name_len, template, dsp_id);
+
+ rproc = rproc_alloc(dev, dev_name(dev), &keystone_rproc_ops, fw_name,
+ sizeof(*ksproc));
+ if (!rproc)
+ return -ENOMEM;
+
+ rproc->has_iommu = false;
+ ksproc = rproc->priv;
+ ksproc->rproc = rproc;
+ ksproc->dev = dev;
+
+ ret = keystone_rproc_of_get_dev_syscon(pdev, ksproc);
+ if (ret)
+ goto free_rproc;
+
+ ksproc->reset = devm_reset_control_get(dev, NULL);
+ if (IS_ERR(ksproc->reset)) {
+ ret = PTR_ERR(ksproc->reset);
+ goto free_rproc;
+ }
+
+ /* enable clock for accessing DSP internal memories */
+ pm_runtime_enable(dev);
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable clock, status = %d\n", ret);
+ pm_runtime_put_noidle(dev);
+ goto disable_rpm;
+ }
+
+ ret = keystone_rproc_of_get_memories(pdev, ksproc);
+ if (ret)
+ goto disable_clk;
+
+ ksproc->irq_ring = platform_get_irq_byname(pdev, "vring");
+ if (ksproc->irq_ring < 0) {
+ ret = ksproc->irq_ring;
+ dev_err(dev, "failed to get vring interrupt, status = %d\n",
+ ret);
+ goto disable_clk;
+ }
+
+ ksproc->irq_fault = platform_get_irq_byname(pdev, "exception");
+ if (ksproc->irq_fault < 0) {
+ ret = ksproc->irq_fault;
+ dev_err(dev, "failed to get exception interrupt, status = %d\n",
+ ret);
+ goto disable_clk;
+ }
+
+ ksproc->kick_gpio = of_get_named_gpio_flags(np, "kick-gpios", 0, NULL);
+ if (ksproc->kick_gpio < 0) {
+ ret = ksproc->kick_gpio;
+ dev_err(dev, "failed to get gpio for virtio kicks, status = %d\n",
+ ret);
+ goto disable_clk;
+ }
+
+ if (of_reserved_mem_device_init(dev))
+ dev_warn(dev, "device does not have specific CMA pool\n");
+
+ ret = rproc_add(rproc);
+ if (ret) {
+ dev_err(dev, "failed to add register device with remoteproc core, status = %d\n",
+ ret);
+ goto release_mem;
+ }
+
+ platform_set_drvdata(pdev, ksproc);
+
+ return 0;
+
+release_mem:
+ of_reserved_mem_device_release(dev);
+disable_clk:
+ pm_runtime_put_sync(dev);
+disable_rpm:
+ pm_runtime_disable(dev);
+free_rproc:
+ rproc_free(rproc);
+ return ret;
+}
+
+static int keystone_rproc_remove(struct platform_device *pdev)
+{
+ struct keystone_rproc *ksproc = platform_get_drvdata(pdev);
+
+ rproc_del(ksproc->rproc);
+ pm_runtime_put_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+ rproc_free(ksproc->rproc);
+ of_reserved_mem_device_release(&pdev->dev);
+
+ return 0;
+}
+
+static const struct of_device_id keystone_rproc_of_match[] = {
+ { .compatible = "ti,k2hk-dsp", },
+ { .compatible = "ti,k2l-dsp", },
+ { .compatible = "ti,k2e-dsp", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, keystone_rproc_of_match);
+
+static struct platform_driver keystone_rproc_driver = {
+ .probe = keystone_rproc_probe,
+ .remove = keystone_rproc_remove,
+ .driver = {
+ .name = "keystone-rproc",
+ .of_match_table = keystone_rproc_of_match,
+ },
+};
+
+module_platform_driver(keystone_rproc_driver);
+
+MODULE_AUTHOR("Suman Anna <s-anna@ti.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TI Keystone DSP Remoteproc driver");
--
2.13.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-25 20:15 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-25 20:15 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc, linux-arm-kernel, devicetree, linux-kernel,
Andrew F. Davis, Sam Nelson
On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> +static int keystone_rproc_start(struct rproc *rproc)
> +{
> + struct keystone_rproc *ksproc = rproc->priv;
> + int ret;
> +
> + INIT_WORK(&ksproc->workqueue, handle_event);
> +
> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> + dev_name(ksproc->dev), ksproc);
> + if (ret) {
> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> + ret);
> + goto out;
> + }
> +
> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> + 0, dev_name(ksproc->dev), ksproc);
> + if (ret) {
> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> + ret);
> + goto free_vring_irq;
> + }
I do prefer that your request any resources during probe() and
potentially enable/disable them here. If below concern about using a
GPIO driver is cleared already I'll take it as is though.
[..]
> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> +{
> + struct keystone_rproc *ksproc = rproc->priv;
> +
> + if (WARN_ON(ksproc->kick_gpio < 0))
> + return;
> +
> + gpio_set_value(ksproc->kick_gpio, 1);
> +}
> +
This doesn't sound like a gpio-controller and the GPIO maintainer did
reject an attempt by me to use the GPIO framework to abstract a similar
thing. Do you already have this driver upstream or have you clarified
with the maintainer that the GPIO framework is an acceptable abstraction
for this?
It looks equivalent to the "APCS IPC" register found in Qualcomm
platforms, previously implemented through a syscon but in v4.13 being
pushed to being a mailbox driver.
Apart from this I think the series looks good.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-25 20:15 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-25 20:15 UTC (permalink / raw)
To: linux-arm-kernel
On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> +static int keystone_rproc_start(struct rproc *rproc)
> +{
> + struct keystone_rproc *ksproc = rproc->priv;
> + int ret;
> +
> + INIT_WORK(&ksproc->workqueue, handle_event);
> +
> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> + dev_name(ksproc->dev), ksproc);
> + if (ret) {
> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> + ret);
> + goto out;
> + }
> +
> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> + 0, dev_name(ksproc->dev), ksproc);
> + if (ret) {
> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> + ret);
> + goto free_vring_irq;
> + }
I do prefer that your request any resources during probe() and
potentially enable/disable them here. If below concern about using a
GPIO driver is cleared already I'll take it as is though.
[..]
> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> +{
> + struct keystone_rproc *ksproc = rproc->priv;
> +
> + if (WARN_ON(ksproc->kick_gpio < 0))
> + return;
> +
> + gpio_set_value(ksproc->kick_gpio, 1);
> +}
> +
This doesn't sound like a gpio-controller and the GPIO maintainer did
reject an attempt by me to use the GPIO framework to abstract a similar
thing. Do you already have this driver upstream or have you clarified
with the maintainer that the GPIO framework is an acceptable abstraction
for this?
It looks equivalent to the "APCS IPC" register found in Qualcomm
platforms, previously implemented through a syscon but in v4.13 being
pushed to being a mailbox driver.
Apart from this I think the series looks good.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-25 20:15 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-25 20:15 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew F. Davis, Sam Nelson
On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> +static int keystone_rproc_start(struct rproc *rproc)
> +{
> + struct keystone_rproc *ksproc = rproc->priv;
> + int ret;
> +
> + INIT_WORK(&ksproc->workqueue, handle_event);
> +
> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> + dev_name(ksproc->dev), ksproc);
> + if (ret) {
> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> + ret);
> + goto out;
> + }
> +
> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> + 0, dev_name(ksproc->dev), ksproc);
> + if (ret) {
> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> + ret);
> + goto free_vring_irq;
> + }
I do prefer that your request any resources during probe() and
potentially enable/disable them here. If below concern about using a
GPIO driver is cleared already I'll take it as is though.
[..]
> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> +{
> + struct keystone_rproc *ksproc = rproc->priv;
> +
> + if (WARN_ON(ksproc->kick_gpio < 0))
> + return;
> +
> + gpio_set_value(ksproc->kick_gpio, 1);
> +}
> +
This doesn't sound like a gpio-controller and the GPIO maintainer did
reject an attempt by me to use the GPIO framework to abstract a similar
thing. Do you already have this driver upstream or have you clarified
with the maintainer that the GPIO framework is an acceptable abstraction
for this?
It looks equivalent to the "APCS IPC" register found in Qualcomm
platforms, previously implemented through a syscon but in v4.13 being
pushed to being a mailbox driver.
Apart from this I think the series looks good.
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
2017-06-25 20:15 ` Bjorn Andersson
(?)
(?)
@ 2017-06-26 15:54 ` Suman Anna
-1 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-26 15:54 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc, linux-arm-kernel, devicetree, linux-kernel,
Andrew F. Davis <afd@ti.com>,
Sam Nelson, Grygorii Strashko
Hi Bjorn,
On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>
>> +static int keystone_rproc_start(struct rproc *rproc)
>> +{
>> + struct keystone_rproc *ksproc = rproc->priv;
>> + int ret;
>> +
>> + INIT_WORK(&ksproc->workqueue, handle_event);
>> +
>> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>> + dev_name(ksproc->dev), ksproc);
>> + if (ret) {
>> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>> + ret);
>> + goto out;
>> + }
>> +
>> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>> + 0, dev_name(ksproc->dev), ksproc);
>> + if (ret) {
>> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>> + ret);
>> + goto free_vring_irq;
>> + }
>
> I do prefer that your request any resources during probe() and
> potentially enable/disable them here. If below concern about using a
> GPIO driver is cleared already I'll take it as is though.
>
> [..]
>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> + struct keystone_rproc *ksproc = rproc->priv;
>> +
>> + if (WARN_ON(ksproc->kick_gpio < 0))
>> + return;
>> +
>> + gpio_set_value(ksproc->kick_gpio, 1);
>> +}
>> +
>
> This doesn't sound like a gpio-controller and the GPIO maintainer did
> reject an attempt by me to use the GPIO framework to abstract a similar
> thing. Do you already have this driver upstream or have you clarified
> with the maintainer that the GPIO framework is an acceptable abstraction
> for this?
Yeah, this has been upstream since quite some time. See commit
2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
regards
Suman
>
> It looks equivalent to the "APCS IPC" register found in Qualcomm
> platforms, previously implemented through a syscon but in v4.13 being
> pushed to being a mailbox driver.
>
>
> Apart from this I think the series looks good.
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 15:54 ` Suman Anna
0 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-26 15:54 UTC (permalink / raw)
To: linux-arm-kernel
Hi Bjorn,
On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>
>> +static int keystone_rproc_start(struct rproc *rproc)
>> +{
>> + struct keystone_rproc *ksproc = rproc->priv;
>> + int ret;
>> +
>> + INIT_WORK(&ksproc->workqueue, handle_event);
>> +
>> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>> + dev_name(ksproc->dev), ksproc);
>> + if (ret) {
>> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>> + ret);
>> + goto out;
>> + }
>> +
>> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>> + 0, dev_name(ksproc->dev), ksproc);
>> + if (ret) {
>> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>> + ret);
>> + goto free_vring_irq;
>> + }
>
> I do prefer that your request any resources during probe() and
> potentially enable/disable them here. If below concern about using a
> GPIO driver is cleared already I'll take it as is though.
>
> [..]
>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> + struct keystone_rproc *ksproc = rproc->priv;
>> +
>> + if (WARN_ON(ksproc->kick_gpio < 0))
>> + return;
>> +
>> + gpio_set_value(ksproc->kick_gpio, 1);
>> +}
>> +
>
> This doesn't sound like a gpio-controller and the GPIO maintainer did
> reject an attempt by me to use the GPIO framework to abstract a similar
> thing. Do you already have this driver upstream or have you clarified
> with the maintainer that the GPIO framework is an acceptable abstraction
> for this?
Yeah, this has been upstream since quite some time. See commit
2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
regards
Suman
>
> It looks equivalent to the "APCS IPC" register found in Qualcomm
> platforms, previously implemented through a syscon but in v4.13 being
> pushed to being a mailbox driver.
>
>
> Apart from this I think the series looks good.
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 15:54 ` Suman Anna
0 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-26 15:54 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Mark Rutland, Grygorii Strashko, devicetree,
linux-remoteproc, linux-kernel, Andrew F. Davis, Rob Herring,
Santosh Shilimkar, linux-arm-kernel, Sam Nelson
Hi Bjorn,
On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>
>> +static int keystone_rproc_start(struct rproc *rproc)
>> +{
>> + struct keystone_rproc *ksproc = rproc->priv;
>> + int ret;
>> +
>> + INIT_WORK(&ksproc->workqueue, handle_event);
>> +
>> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>> + dev_name(ksproc->dev), ksproc);
>> + if (ret) {
>> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>> + ret);
>> + goto out;
>> + }
>> +
>> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>> + 0, dev_name(ksproc->dev), ksproc);
>> + if (ret) {
>> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>> + ret);
>> + goto free_vring_irq;
>> + }
>
> I do prefer that your request any resources during probe() and
> potentially enable/disable them here. If below concern about using a
> GPIO driver is cleared already I'll take it as is though.
>
> [..]
>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> + struct keystone_rproc *ksproc = rproc->priv;
>> +
>> + if (WARN_ON(ksproc->kick_gpio < 0))
>> + return;
>> +
>> + gpio_set_value(ksproc->kick_gpio, 1);
>> +}
>> +
>
> This doesn't sound like a gpio-controller and the GPIO maintainer did
> reject an attempt by me to use the GPIO framework to abstract a similar
> thing. Do you already have this driver upstream or have you clarified
> with the maintainer that the GPIO framework is an acceptable abstraction
> for this?
Yeah, this has been upstream since quite some time. See commit
2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
regards
Suman
>
> It looks equivalent to the "APCS IPC" register found in Qualcomm
> platforms, previously implemented through a syscon but in v4.13 being
> pushed to being a mailbox driver.
>
>
> Apart from this I think the series looks good.
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 15:54 ` Suman Anna
0 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-26 15:54 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc, linux-arm-kernel, devicetree, linux-kernel,
Andrew F. Davis, Sam Nelson, Grygorii Strashko
Hi Bjorn,
On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>
>> +static int keystone_rproc_start(struct rproc *rproc)
>> +{
>> + struct keystone_rproc *ksproc = rproc->priv;
>> + int ret;
>> +
>> + INIT_WORK(&ksproc->workqueue, handle_event);
>> +
>> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>> + dev_name(ksproc->dev), ksproc);
>> + if (ret) {
>> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>> + ret);
>> + goto out;
>> + }
>> +
>> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>> + 0, dev_name(ksproc->dev), ksproc);
>> + if (ret) {
>> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>> + ret);
>> + goto free_vring_irq;
>> + }
>
> I do prefer that your request any resources during probe() and
> potentially enable/disable them here. If below concern about using a
> GPIO driver is cleared already I'll take it as is though.
>
> [..]
>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> + struct keystone_rproc *ksproc = rproc->priv;
>> +
>> + if (WARN_ON(ksproc->kick_gpio < 0))
>> + return;
>> +
>> + gpio_set_value(ksproc->kick_gpio, 1);
>> +}
>> +
>
> This doesn't sound like a gpio-controller and the GPIO maintainer did
> reject an attempt by me to use the GPIO framework to abstract a similar
> thing. Do you already have this driver upstream or have you clarified
> with the maintainer that the GPIO framework is an acceptable abstraction
> for this?
Yeah, this has been upstream since quite some time. See commit
2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
regards
Suman
>
> It looks equivalent to the "APCS IPC" register found in Qualcomm
> platforms, previously implemented through a syscon but in v4.13 being
> pushed to being a mailbox driver.
>
>
> Apart from this I think the series looks good.
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 20:06 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-26 20:06 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc, linux-arm-kernel, devicetree, linux-kernel,
Andrew F. Davis, Sam Nelson, Grygorii Strashko
On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
> Hi Bjorn,
>
> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> > On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> >
> >> +static int keystone_rproc_start(struct rproc *rproc)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> + int ret;
> >> +
> >> + INIT_WORK(&ksproc->workqueue, handle_event);
> >> +
> >> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> >> + dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> >> + ret);
> >> + goto out;
> >> + }
> >> +
> >> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> >> + 0, dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> >> + ret);
> >> + goto free_vring_irq;
> >> + }
> >
> > I do prefer that your request any resources during probe() and
> > potentially enable/disable them here. If below concern about using a
> > GPIO driver is cleared already I'll take it as is though.
> >
> > [..]
> >> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> +
> >> + if (WARN_ON(ksproc->kick_gpio < 0))
> >> + return;
> >> +
> >> + gpio_set_value(ksproc->kick_gpio, 1);
> >> +}
> >> +
> >
> > This doesn't sound like a gpio-controller and the GPIO maintainer did
> > reject an attempt by me to use the GPIO framework to abstract a similar
> > thing. Do you already have this driver upstream or have you clarified
> > with the maintainer that the GPIO framework is an acceptable abstraction
> > for this?
>
> Yeah, this has been upstream since quite some time. See commit
> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
>
Okay, sounds good. I have merged the series.
I still would like to have resources allocated at probe() time, so I
would appreciate a follow up patch moving the request_irq()s to probe,
per above comment (but we can take that after v4.13).
Regards,
Bjorn
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 20:06 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-26 20:06 UTC (permalink / raw)
To: linux-arm-kernel
On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
> Hi Bjorn,
>
> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> > On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> >
> >> +static int keystone_rproc_start(struct rproc *rproc)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> + int ret;
> >> +
> >> + INIT_WORK(&ksproc->workqueue, handle_event);
> >> +
> >> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> >> + dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> >> + ret);
> >> + goto out;
> >> + }
> >> +
> >> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> >> + 0, dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> >> + ret);
> >> + goto free_vring_irq;
> >> + }
> >
> > I do prefer that your request any resources during probe() and
> > potentially enable/disable them here. If below concern about using a
> > GPIO driver is cleared already I'll take it as is though.
> >
> > [..]
> >> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> +
> >> + if (WARN_ON(ksproc->kick_gpio < 0))
> >> + return;
> >> +
> >> + gpio_set_value(ksproc->kick_gpio, 1);
> >> +}
> >> +
> >
> > This doesn't sound like a gpio-controller and the GPIO maintainer did
> > reject an attempt by me to use the GPIO framework to abstract a similar
> > thing. Do you already have this driver upstream or have you clarified
> > with the maintainer that the GPIO framework is an acceptable abstraction
> > for this?
>
> Yeah, this has been upstream since quite some time. See commit
> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
>
Okay, sounds good. I have merged the series.
I still would like to have resources allocated at probe() time, so I
would appreciate a follow up patch moving the request_irq()s to probe,
per above comment (but we can take that after v4.13).
Regards,
Bjorn
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 20:06 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-26 20:06 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew F. Davis, Sam Nelson,
Grygorii Strashko
On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
> Hi Bjorn,
>
> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> > On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> >
> >> +static int keystone_rproc_start(struct rproc *rproc)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> + int ret;
> >> +
> >> + INIT_WORK(&ksproc->workqueue, handle_event);
> >> +
> >> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> >> + dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> >> + ret);
> >> + goto out;
> >> + }
> >> +
> >> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> >> + 0, dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> >> + ret);
> >> + goto free_vring_irq;
> >> + }
> >
> > I do prefer that your request any resources during probe() and
> > potentially enable/disable them here. If below concern about using a
> > GPIO driver is cleared already I'll take it as is though.
> >
> > [..]
> >> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> +
> >> + if (WARN_ON(ksproc->kick_gpio < 0))
> >> + return;
> >> +
> >> + gpio_set_value(ksproc->kick_gpio, 1);
> >> +}
> >> +
> >
> > This doesn't sound like a gpio-controller and the GPIO maintainer did
> > reject an attempt by me to use the GPIO framework to abstract a similar
> > thing. Do you already have this driver upstream or have you clarified
> > with the maintainer that the GPIO framework is an acceptable abstraction
> > for this?
>
> Yeah, this has been upstream since quite some time. See commit
> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
>
Okay, sounds good. I have merged the series.
I still would like to have resources allocated at probe() time, so I
would appreciate a follow up patch moving the request_irq()s to probe,
per above comment (but we can take that after v4.13).
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
2017-06-26 20:06 ` Bjorn Andersson
(?)
(?)
@ 2017-06-26 20:20 ` Suman Anna
-1 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-26 20:20 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc, linux-arm-kernel, devicetree, linux-kernel,
Andrew F. Davis <afd@ti.com>,
Sam Nelson, Grygorii Strashko
On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
>
>> Hi Bjorn,
>>
>> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
>>> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>>>
>>>> +static int keystone_rproc_start(struct rproc *rproc)
>>>> +{
>>>> + struct keystone_rproc *ksproc = rproc->priv;
>>>> + int ret;
>>>> +
>>>> + INIT_WORK(&ksproc->workqueue, handle_event);
>>>> +
>>>> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>>>> + dev_name(ksproc->dev), ksproc);
>>>> + if (ret) {
>>>> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>>>> + ret);
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>>>> + 0, dev_name(ksproc->dev), ksproc);
>>>> + if (ret) {
>>>> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>>>> + ret);
>>>> + goto free_vring_irq;
>>>> + }
>>>
>>> I do prefer that your request any resources during probe() and
>>> potentially enable/disable them here. If below concern about using a
>>> GPIO driver is cleared already I'll take it as is though.
>>>
>>> [..]
>>>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>>>> +{
>>>> + struct keystone_rproc *ksproc = rproc->priv;
>>>> +
>>>> + if (WARN_ON(ksproc->kick_gpio < 0))
>>>> + return;
>>>> +
>>>> + gpio_set_value(ksproc->kick_gpio, 1);
>>>> +}
>>>> +
>>>
>>> This doesn't sound like a gpio-controller and the GPIO maintainer did
>>> reject an attempt by me to use the GPIO framework to abstract a similar
>>> thing. Do you already have this driver upstream or have you clarified
>>> with the maintainer that the GPIO framework is an acceptable abstraction
>>> for this?
>>
>> Yeah, this has been upstream since quite some time. See commit
>> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
>>
>
> Okay, sounds good. I have merged the series.
>
>
> I still would like to have resources allocated at probe() time, so I
> would appreciate a follow up patch moving the request_irq()s to probe,
> per above comment (but we can take that after v4.13).
OK thanks. This is a common theme across all the remoteproc drivers
supporting rpmsg, and I definitely need to disable them in probe since
the boot or the virtio/rpmsg devices are not guaranteed to be present in
the probe.
regards
Suman
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 20:20 ` Suman Anna
0 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-26 20:20 UTC (permalink / raw)
To: linux-arm-kernel
On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
>
>> Hi Bjorn,
>>
>> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
>>> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>>>
>>>> +static int keystone_rproc_start(struct rproc *rproc)
>>>> +{
>>>> + struct keystone_rproc *ksproc = rproc->priv;
>>>> + int ret;
>>>> +
>>>> + INIT_WORK(&ksproc->workqueue, handle_event);
>>>> +
>>>> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>>>> + dev_name(ksproc->dev), ksproc);
>>>> + if (ret) {
>>>> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>>>> + ret);
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>>>> + 0, dev_name(ksproc->dev), ksproc);
>>>> + if (ret) {
>>>> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>>>> + ret);
>>>> + goto free_vring_irq;
>>>> + }
>>>
>>> I do prefer that your request any resources during probe() and
>>> potentially enable/disable them here. If below concern about using a
>>> GPIO driver is cleared already I'll take it as is though.
>>>
>>> [..]
>>>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>>>> +{
>>>> + struct keystone_rproc *ksproc = rproc->priv;
>>>> +
>>>> + if (WARN_ON(ksproc->kick_gpio < 0))
>>>> + return;
>>>> +
>>>> + gpio_set_value(ksproc->kick_gpio, 1);
>>>> +}
>>>> +
>>>
>>> This doesn't sound like a gpio-controller and the GPIO maintainer did
>>> reject an attempt by me to use the GPIO framework to abstract a similar
>>> thing. Do you already have this driver upstream or have you clarified
>>> with the maintainer that the GPIO framework is an acceptable abstraction
>>> for this?
>>
>> Yeah, this has been upstream since quite some time. See commit
>> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
>>
>
> Okay, sounds good. I have merged the series.
>
>
> I still would like to have resources allocated at probe() time, so I
> would appreciate a follow up patch moving the request_irq()s to probe,
> per above comment (but we can take that after v4.13).
OK thanks. This is a common theme across all the remoteproc drivers
supporting rpmsg, and I definitely need to disable them in probe since
the boot or the virtio/rpmsg devices are not guaranteed to be present in
the probe.
regards
Suman
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 20:20 ` Suman Anna
0 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-26 20:20 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Mark Rutland, Grygorii Strashko, devicetree,
linux-remoteproc, linux-kernel, Andrew F. Davis, Rob Herring,
Santosh Shilimkar, linux-arm-kernel, Sam Nelson
On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
>
>> Hi Bjorn,
>>
>> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
>>> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>>>
>>>> +static int keystone_rproc_start(struct rproc *rproc)
>>>> +{
>>>> + struct keystone_rproc *ksproc = rproc->priv;
>>>> + int ret;
>>>> +
>>>> + INIT_WORK(&ksproc->workqueue, handle_event);
>>>> +
>>>> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>>>> + dev_name(ksproc->dev), ksproc);
>>>> + if (ret) {
>>>> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>>>> + ret);
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>>>> + 0, dev_name(ksproc->dev), ksproc);
>>>> + if (ret) {
>>>> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>>>> + ret);
>>>> + goto free_vring_irq;
>>>> + }
>>>
>>> I do prefer that your request any resources during probe() and
>>> potentially enable/disable them here. If below concern about using a
>>> GPIO driver is cleared already I'll take it as is though.
>>>
>>> [..]
>>>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>>>> +{
>>>> + struct keystone_rproc *ksproc = rproc->priv;
>>>> +
>>>> + if (WARN_ON(ksproc->kick_gpio < 0))
>>>> + return;
>>>> +
>>>> + gpio_set_value(ksproc->kick_gpio, 1);
>>>> +}
>>>> +
>>>
>>> This doesn't sound like a gpio-controller and the GPIO maintainer did
>>> reject an attempt by me to use the GPIO framework to abstract a similar
>>> thing. Do you already have this driver upstream or have you clarified
>>> with the maintainer that the GPIO framework is an acceptable abstraction
>>> for this?
>>
>> Yeah, this has been upstream since quite some time. See commit
>> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
>>
>
> Okay, sounds good. I have merged the series.
>
>
> I still would like to have resources allocated at probe() time, so I
> would appreciate a follow up patch moving the request_irq()s to probe,
> per above comment (but we can take that after v4.13).
OK thanks. This is a common theme across all the remoteproc drivers
supporting rpmsg, and I definitely need to disable them in probe since
the boot or the virtio/rpmsg devices are not guaranteed to be present in
the probe.
regards
Suman
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 20:20 ` Suman Anna
0 siblings, 0 replies; 37+ messages in thread
From: Suman Anna @ 2017-06-26 20:20 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc, linux-arm-kernel, devicetree, linux-kernel,
Andrew F. Davis, Sam Nelson, Grygorii Strashko
On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
>
>> Hi Bjorn,
>>
>> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
>>> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>>>
>>>> +static int keystone_rproc_start(struct rproc *rproc)
>>>> +{
>>>> + struct keystone_rproc *ksproc = rproc->priv;
>>>> + int ret;
>>>> +
>>>> + INIT_WORK(&ksproc->workqueue, handle_event);
>>>> +
>>>> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>>>> + dev_name(ksproc->dev), ksproc);
>>>> + if (ret) {
>>>> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>>>> + ret);
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>>>> + 0, dev_name(ksproc->dev), ksproc);
>>>> + if (ret) {
>>>> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>>>> + ret);
>>>> + goto free_vring_irq;
>>>> + }
>>>
>>> I do prefer that your request any resources during probe() and
>>> potentially enable/disable them here. If below concern about using a
>>> GPIO driver is cleared already I'll take it as is though.
>>>
>>> [..]
>>>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>>>> +{
>>>> + struct keystone_rproc *ksproc = rproc->priv;
>>>> +
>>>> + if (WARN_ON(ksproc->kick_gpio < 0))
>>>> + return;
>>>> +
>>>> + gpio_set_value(ksproc->kick_gpio, 1);
>>>> +}
>>>> +
>>>
>>> This doesn't sound like a gpio-controller and the GPIO maintainer did
>>> reject an attempt by me to use the GPIO framework to abstract a similar
>>> thing. Do you already have this driver upstream or have you clarified
>>> with the maintainer that the GPIO framework is an acceptable abstraction
>>> for this?
>>
>> Yeah, this has been upstream since quite some time. See commit
>> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
>>
>
> Okay, sounds good. I have merged the series.
>
>
> I still would like to have resources allocated at probe() time, so I
> would appreciate a follow up patch moving the request_irq()s to probe,
> per above comment (but we can take that after v4.13).
OK thanks. This is a common theme across all the remoteproc drivers
supporting rpmsg, and I definitely need to disable them in probe since
the boot or the virtio/rpmsg devices are not guaranteed to be present in
the probe.
regards
Suman
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 21:19 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-26 21:19 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc, linux-arm-kernel, devicetree, linux-kernel,
Andrew F. Davis, Sam Nelson, Grygorii Strashko
On Mon 26 Jun 13:20 PDT 2017, Suman Anna wrote:
> On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> > On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
> >> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
[..]
> > I still would like to have resources allocated at probe() time, so I
> > would appreciate a follow up patch moving the request_irq()s to probe,
> > per above comment (but we can take that after v4.13).
>
> OK thanks. This is a common theme across all the remoteproc drivers
> supporting rpmsg, and I definitely need to disable them in probe since
> the boot or the virtio/rpmsg devices are not guaranteed to be present in
> the probe.
>
We have a consistent struct rproc after rproc_alloc(). So before the
virtio device (rpmsg in your case) calls virtio_find_vqs() rvring->vq
will be NULL and rproc_vq_interrupt() is a nop.
So AFAICT it should be fine to register this at probe time and leave it
enabled...but there's a lot of moving parts involved here, so it's
possible that I'm missing something.
But in for both interrupts it's important to disable them before calling
rproc_free(), as this will free the memory passed to the interrupt
handler.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 21:19 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-26 21:19 UTC (permalink / raw)
To: linux-arm-kernel
On Mon 26 Jun 13:20 PDT 2017, Suman Anna wrote:
> On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> > On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
> >> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
[..]
> > I still would like to have resources allocated at probe() time, so I
> > would appreciate a follow up patch moving the request_irq()s to probe,
> > per above comment (but we can take that after v4.13).
>
> OK thanks. This is a common theme across all the remoteproc drivers
> supporting rpmsg, and I definitely need to disable them in probe since
> the boot or the virtio/rpmsg devices are not guaranteed to be present in
> the probe.
>
We have a consistent struct rproc after rproc_alloc(). So before the
virtio device (rpmsg in your case) calls virtio_find_vqs() rvring->vq
will be NULL and rproc_vq_interrupt() is a nop.
So AFAICT it should be fine to register this at probe time and leave it
enabled...but there's a lot of moving parts involved here, so it's
possible that I'm missing something.
But in for both interrupts it's important to disable them before calling
rproc_free(), as this will free the memory passed to the interrupt
handler.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2017-06-26 21:19 ` Bjorn Andersson
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Andersson @ 2017-06-26 21:19 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Rob Herring, Santosh Shilimkar, Mark Rutland,
linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew F. Davis, Sam Nelson,
Grygorii Strashko
On Mon 26 Jun 13:20 PDT 2017, Suman Anna wrote:
> On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> > On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
> >> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
[..]
> > I still would like to have resources allocated at probe() time, so I
> > would appreciate a follow up patch moving the request_irq()s to probe,
> > per above comment (but we can take that after v4.13).
>
> OK thanks. This is a common theme across all the remoteproc drivers
> supporting rpmsg, and I definitely need to disable them in probe since
> the boot or the virtio/rpmsg devices are not guaranteed to be present in
> the probe.
>
We have a consistent struct rproc after rproc_alloc(). So before the
virtio device (rpmsg in your case) calls virtio_find_vqs() rvring->vq
will be NULL and rproc_vq_interrupt() is a nop.
So AFAICT it should be fine to register this at probe time and leave it
enabled...but there's a lot of moving parts involved here, so it's
possible that I'm missing something.
But in for both interrupts it's important to disable them before calling
rproc_free(), as this will free the memory passed to the interrupt
handler.
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
2017-06-26 15:54 ` Suman Anna
@ 2022-09-06 3:55 ` Dmitry Torokhov
-1 siblings, 0 replies; 37+ messages in thread
From: Dmitry Torokhov @ 2022-09-06 3:55 UTC (permalink / raw)
To: Suman Anna
Cc: Bjorn Andersson, Ohad Ben-Cohen, Mark Rutland, Grygorii Strashko,
DTML, linux-remoteproc, lkml, Andrew F. Davis, Rob Herring,
Santosh Shilimkar, linux-arm-kernel, Sam Nelson
Hi,
On Mon, Jun 26, 2017 at 8:55 AM Suman Anna <s-anna@ti.com> wrote:
>
> Hi Bjorn,
>
> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> > On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> >
> >> +static int keystone_rproc_start(struct rproc *rproc)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> + int ret;
> >> +
> >> + INIT_WORK(&ksproc->workqueue, handle_event);
> >> +
> >> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> >> + dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> >> + ret);
> >> + goto out;
> >> + }
> >> +
> >> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> >> + 0, dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> >> + ret);
> >> + goto free_vring_irq;
> >> + }
> >
> > I do prefer that your request any resources during probe() and
> > potentially enable/disable them here. If below concern about using a
> > GPIO driver is cleared already I'll take it as is though.
> >
> > [..]
> >> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> +
> >> + if (WARN_ON(ksproc->kick_gpio < 0))
> >> + return;
> >> +
> >> + gpio_set_value(ksproc->kick_gpio, 1);
> >> +}
> >> +
> >
> > This doesn't sound like a gpio-controller and the GPIO maintainer did
> > reject an attempt by me to use the GPIO framework to abstract a similar
> > thing. Do you already have this driver upstream or have you clarified
> > with the maintainer that the GPIO framework is an acceptable abstraction
> > for this?
>
> Yeah, this has been upstream since quite some time. See commit
> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
Sorry for the thread necromancy, but was it intentional that the
driver only parses DT to find this "GPIO" but never requests/reserves
it? I would like to drop of_get_named_gpio_flags() in favor of a more
standard dev_get_gpiod().
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
@ 2022-09-06 3:55 ` Dmitry Torokhov
0 siblings, 0 replies; 37+ messages in thread
From: Dmitry Torokhov @ 2022-09-06 3:55 UTC (permalink / raw)
To: Suman Anna
Cc: Bjorn Andersson, Ohad Ben-Cohen, Mark Rutland, Grygorii Strashko,
DTML, linux-remoteproc, lkml, Andrew F. Davis, Rob Herring,
Santosh Shilimkar, linux-arm-kernel, Sam Nelson
Hi,
On Mon, Jun 26, 2017 at 8:55 AM Suman Anna <s-anna@ti.com> wrote:
>
> Hi Bjorn,
>
> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> > On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> >
> >> +static int keystone_rproc_start(struct rproc *rproc)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> + int ret;
> >> +
> >> + INIT_WORK(&ksproc->workqueue, handle_event);
> >> +
> >> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> >> + dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> >> + ret);
> >> + goto out;
> >> + }
> >> +
> >> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> >> + 0, dev_name(ksproc->dev), ksproc);
> >> + if (ret) {
> >> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> >> + ret);
> >> + goto free_vring_irq;
> >> + }
> >
> > I do prefer that your request any resources during probe() and
> > potentially enable/disable them here. If below concern about using a
> > GPIO driver is cleared already I'll take it as is though.
> >
> > [..]
> >> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> >> +{
> >> + struct keystone_rproc *ksproc = rproc->priv;
> >> +
> >> + if (WARN_ON(ksproc->kick_gpio < 0))
> >> + return;
> >> +
> >> + gpio_set_value(ksproc->kick_gpio, 1);
> >> +}
> >> +
> >
> > This doesn't sound like a gpio-controller and the GPIO maintainer did
> > reject an attempt by me to use the GPIO framework to abstract a similar
> > thing. Do you already have this driver upstream or have you clarified
> > with the maintainer that the GPIO framework is an acceptable abstraction
> > for this?
>
> Yeah, this has been upstream since quite some time. See commit
> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
Sorry for the thread necromancy, but was it intentional that the
driver only parses DT to find this "GPIO" but never requests/reserves
it? I would like to drop of_get_named_gpio_flags() in favor of a more
standard dev_get_gpiod().
Thanks.
--
Dmitry
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 37+ messages in thread