Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1259183 > unrolled thread
| Started by | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| First post | 2015-10-30 04:10 +0100 |
| Last post | 2015-10-31 18:20 +0100 |
| Articles | 20 on this page of 48 — 8 participants |
Back to article view | Back to linux.kernel
[PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-30 04:10 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver kbuild test robot <lkp@intel.com> - 2015-10-30 05:40 +0100
[PATCH] dma: fix platform_no_drv_owner.cocci warnings kbuild test robot <lkp@intel.com> - 2015-10-30 05:40 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Arnd Bergmann <arnd@arndb.de> - 2015-10-30 10:40 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-31 08:00 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Timur Tabi <timur@codeaurora.org> - 2015-10-31 14:00 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Timur Tabi <timur@codeaurora.org> - 2015-10-31 14:00 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Arnd Bergmann <arnd@arndb.de> - 2015-11-02 22:40 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-11-03 05:50 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Arnd Bergmann <arnd@arndb.de> - 2015-11-03 13:50 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Timur Tabi <timur@codeaurora.org> - 2015-11-03 15:30 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-11-04 02:10 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Arnd Bergmann <arnd@arndb.de> - 2015-10-30 11:30 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-30 22:50 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Timur Tabi <timur@codeaurora.org> - 2015-10-30 23:00 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Arnd Bergmann <arnd@arndb.de> - 2015-10-30 23:50 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Timur Tabi <timur@codeaurora.org> - 2015-10-30 23:40 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Arnd Bergmann <arnd@arndb.de> - 2015-10-31 00:00 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Arnd Bergmann <arnd@arndb.de> - 2015-10-30 23:40 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-31 03:00 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Sinan Kaya <okaya@codeaurora.org> - 2015-11-01 20:00 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Timur Tabi <timur@codeaurora.org> - 2015-11-01 21:30 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Sinan Kaya <okaya@codeaurora.org> - 2015-11-01 21:30 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Arnd Bergmann <arnd@arndb.de> - 2015-11-02 17:40 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Sinan Kaya <okaya@codeaurora.org> - 2015-11-02 20:30 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Arnd Bergmann <arnd@arndb.de> - 2015-11-02 22:00 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Sinan Kaya <okaya@codeaurora.org> - 2015-11-03 06:30 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Arnd Bergmann <arnd@arndb.de> - 2015-11-03 11:50 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Sinan Kaya <okaya@codeaurora.org> - 2015-11-03 22:10 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Arnd Bergmann <arnd@arndb.de> - 2015-11-03 22:20 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Mark Rutland <mark.rutland@arm.com> - 2015-10-30 16:00 +0100
Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-31 05:30 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Mark Rutland <mark.rutland@arm.com> - 2015-10-30 16:10 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-10-30 19:10 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-30 19:10 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Mark Rutland <mark.rutland@arm.com> - 2015-10-30 19:20 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-10-30 19:20 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Mark Rutland <mark.rutland@arm.com> - 2015-10-30 19:30 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-10-30 19:50 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-30 19:50 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Mark Rutland <mark.rutland@arm.com> - 2015-10-30 20:10 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Al Stone <al.stone@linaro.org> - 2015-10-30 21:10 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Mark Rutland <mark.rutland@arm.com> - 2015-10-30 21:20 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-10-30 21:20 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Jon Masters <jcm@redhat.com> - 2015-10-31 04:40 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-31 18:40 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-30 20:40 +0100
Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Sinan Kaya <okaya@codeaurora.org> - 2015-10-31 18:20 +0100
Page 1 of 3 [1] 2 3 Next page →
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2015-10-30 04:10 +0100 |
| Subject | [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver |
| Message-ID | <qp8eK-oK-5@gated-at.bofh.it> |
The Qualcomm Technologies HIDMA device has been designed
to support virtualization technology. The driver has been
divided into two to follow the hardware design. The management
driver is executed in hypervisor context and is the main
managment for all channels provided by the device. The
channel driver is exected in the guest OS context.
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
.../devicetree/bindings/dma/qcom_hidma_mgmt.txt | 42 +
drivers/dma/Kconfig | 11 +
drivers/dma/Makefile | 1 +
drivers/dma/qcom_hidma_mgmt.c | 868 +++++++++++++++++++++
4 files changed, 922 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
create mode 100644 drivers/dma/qcom_hidma_mgmt.c
diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
new file mode 100644
index 0000000..81674ab
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
@@ -0,0 +1,42 @@
+Qualcomm Technologies HIDMA Management interface
+
+Required properties:
+- compatible: must contain "qcom,hidma_mgmt"
+- reg: Address range for DMA device
+- interrupts: Should contain the one interrupt shared by all channels
+- nr-channels: Number of channels supported by this DMA controller.
+- max-write: Maximum write burst in bytes. A memcpy requested is
+ fragmented to multiples of this amount.
+- max-read: Maximum read burst in bytes. A memcpy request is
+ fragmented to multiples of this amount.
+- max-wxactions: Maximum write transactions to perform in a burst
+- max-rdactions: Maximum read transactions to perform in a burst
+- max-memset-limit: Maximum memset limit
+- ch-priority-#n: Priority of the channel
+- ch-weight-#n: Round robin weight of the channel
+Example:
+
+ hidma-mgmt@f9984000 = {
+ compatible = "qcom,hidma_mgmt";
+ reg = <0xf9984000 0x15000>;
+ interrupts = <0 94 0>;
+ nr-channels = 6;
+ max-write = 1024;
+ max-read = 1024;
+ max-wxactions = 31;
+ max-rdactions = 31;
+ max-memset-limit = 8;
+ ch-priority-0 = 0;
+ ch-priority-1 = 1;
+ ch-priority-2 = 1;
+ ch-priority-3 = 0;
+ ch-priority-4 = 0;
+ ch-priority-5 = 0;
+ ch-weight-0 = 1;
+ ch-weight-1 = 13;
+ ch-weight-2 = 10;
+ ch-weight-3 = 3;
+ ch-weight-4 = 4;
+ ch-weight-5 = 5;
+ };
+
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index b458475..76a5a5e 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -501,6 +501,17 @@ config XGENE_DMA
help
Enable support for the APM X-Gene SoC DMA engine.
+config QCOM_HIDMA_MGMT
+ bool "Qualcomm Technologies HIDMA Managment support"
+ select DMA_ENGINE
+ help
+ Enable support for the Qualcomm Technologies HIDMA Management.
+ Each DMA device requires one management interface driver
+ for basic initialization before QCOM_HIDMA driver can start
+ managing the channels. In a virtualized environment, the guest
+ OS would run QCOM_HIDMA driver and the hypervisor would run
+ the QCOM_HIDMA_MGMT driver.
+
config XILINX_VDMA
tristate "Xilinx AXI VDMA Engine"
depends on (ARCH_ZYNQ || MICROBLAZE)
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 7711a71..3d25ffd 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_PL330_DMA) += pl330.o
obj-$(CONFIG_PPC_BESTCOMM) += bestcomm/
obj-$(CONFIG_PXA_DMA) += pxa_dma.o
obj-$(CONFIG_QCOM_BAM_DMA) += qcom_bam_dma.o
+obj-$(CONFIG_QCOM_HIDMA_MGMT) += qcom_hidma_mgmt.o
obj-$(CONFIG_RENESAS_DMA) += sh/
obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
diff --git a/drivers/dma/qcom_hidma_mgmt.c b/drivers/dma/qcom_hidma_mgmt.c
new file mode 100644
index 0000000..8fcad4d
--- /dev/null
+++ b/drivers/dma/qcom_hidma_mgmt.c
@@ -0,0 +1,868 @@
+/*
+ * Qualcomm Technologies HIDMA DMA engine Management interface
+ *
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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/dmaengine.h>
+#include <linux/acpi.h>
+#include <linux/of.h>
+#include <linux/property.h>
+#include <linux/debugfs.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/pm_runtime.h>
+#include "qcom_hidma.h"
+
+#define MHICFG_OFFSET 0x10
+#define QOS_N_OFFSET 0x300
+#define CFG_OFFSET 0x400
+#define HW_PARAM_OFFSET 0x408
+#define MAX_BUS_REQ_LEN_OFFSET 0x41C
+#define MAX_XACTIONS_OFFSET 0x420
+#define SW_VERSION_OFFSET 0x424
+#define CHRESET_TIMEOUUT_OFFSET 0x500
+#define MEMSET_LIMIT_OFFSET 0x600
+#define MHID_BUS_ERR0_OFFSET 0x1020
+#define MHID_BUS_ERR1_OFFSET 0x1024
+#define MHID_BUS_ERR_CLR_OFFSET 0x102C
+#define EVT_BUS_ERR0_OFFSET 0x1030
+#define EVT_BUS_ERR1_OFFSET 0x1034
+#define EVT_BUS_ERR_CLR_OFFSET 0x103C
+#define IDE_BUS_ERR0_OFFSET 0x1040
+#define IDE_BUS_ERR1_OFFSET 0x1044
+#define IDE_BUS_ERR2_OFFSET 0x1048
+#define IDE_BUS_ERR_CLR_OFFSET 0x104C
+#define ODE_BUS_ERR0_OFFSET 0x1050
+#define ODE_BUS_ERR1_OFFSET 0x1054
+#define ODE_BUS_ERR2_OFFSET 0x1058
+#define ODE_BUS_ERR_CLR_OFFSET 0x105C
+#define MSI_BUS_ERR0_OFFSET 0x1060
+#define MSI_BUS_ERR_CLR_OFFSET 0x106C
+#define TRE_ERR0_OFFSET 0x1070
+#define TRE_ERR_CLR_OFFSET 0x107C
+#define HW_EVENTS_CFG_OFFSET 0x1080
+
+#define HW_EVENTS_CFG_MASK 0xFF
+#define TRE_ERR_TRCHID_MASK 0xF
+#define TRE_ERR_EVRIDX_MASK 0xFF
+#define TRE_ERR_TYPE_MASK 0xFF
+#define MSI_ERR_RESP_MASK 0xFF
+#define MSI_ERR_TRCHID_MASK 0xFF
+#define ODE_ERR_REQLEN_MASK 0xFFFF
+#define ODE_ERR_RESP_MASK 0xFF
+#define ODE_ERR_TRCHID_MASK 0xFF
+#define IDE_ERR_REQLEN_MASK 0xFFFF
+#define IDE_ERR_RESP_MASK 0xFF
+#define IDE_ERR_TRCHID_MASK 0xFF
+#define EVT_ERR_RESP_MASK 0xFF
+#define EVT_ERR_TRCHID_MASK 0xFF
+#define MHID_ERR_RESP_MASK 0xFF
+#define MHID_ERR_TRCHID_MASK 0xFF
+#define MEMSET_LIMIT_MASK 0x1F
+#define MAX_WR_XACTIONS_MASK 0x1F
+#define MAX_RD_XACTIONS_MASK 0x1F
+#define MAX_JOBSIZE_MASK 0xFF
+#define MAX_COIDX_MASK 0xFF
+#define TREQ_CAPACITY_MASK 0xFF
+#define WEIGHT_MASK 0x7F
+#define TREQ_LIMIT_MASK 0x1FF
+#define NR_CHANNEL_MASK 0xFFFF
+#define MAX_BUS_REQ_LEN_MASK 0xFFFF
+#define CHRESET_TIMEOUUT_MASK 0xFFFFF
+
+#define TRE_ERR_TRCHID_BIT_POS 28
+#define TRE_ERR_IEOB_BIT_POS 16
+#define TRE_ERR_EVRIDX_BIT_POS 8
+#define MSI_ERR_RESP_BIT_POS 8
+#define ODE_ERR_REQLEN_BIT_POS 16
+#define ODE_ERR_RESP_BIT_POS 8
+#define IDE_ERR_REQLEN_BIT_POS 16
+#define IDE_ERR_RESP_BIT_POS 8
+#define EVT_ERR_RESP_BIT_POS 8
+#define MHID_ERR_RESP_BIT_POS 8
+#define MAX_WR_XACTIONS_BIT_POS 16
+#define TREQ_CAPACITY_BIT_POS 8
+#define MAX_JOB_SIZE_BIT_POS 16
+#define NR_EV_CHANNEL_BIT_POS 16
+#define MAX_BUS_WR_REQ_BIT_POS 16
+#define WRR_BIT_POS 8
+#define PRIORITY_BIT_POS 15
+#define TREQ_LIMIT_BIT_POS 16
+#define TREQ_LIMIT_EN_BIT_POS 23
+#define STOP_BIT_POS 24
+
+#define MODULE_NAME "hidma-mgmt"
+#define PREFIX MODULE_NAME ": "
+#define AUTOSUSPEND_TIMEOUT 2000
+
+#define HIDMA_RUNTIME_GET(dmadev) \
+do { \
+ atomic_inc(&(dmadev)->pm_counter); \
+ TRC_PM(&(dmadev)->pdev->dev, \
+ "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\
+ atomic_read(&(dmadev)->pm_counter)); \
+ pm_runtime_get_sync(&(dmadev)->pdev->dev); \
+} while (0)
+
+#define HIDMA_RUNTIME_SET(dmadev) \
+do { \
+ atomic_dec(&(dmadev)->pm_counter); \
+ TRC_PM(&(dmadev)->pdev->dev, \
+ "%s:%d pm_runtime_put_autosuspend:%d\n", \
+ __func__, __LINE__, \
+ atomic_read(&(dmadev)->pm_counter)); \
+ pm_runtime_mark_last_busy(&(dmadev)->pdev->dev); \
+ pm_runtime_put_autosuspend(&(dmadev)->pdev->dev); \
+} while (0)
+
+struct qcom_hidma_mgmt_dev {
+ u8 max_wr_xactions;
+ u8 max_rd_xactions;
+ u8 max_memset_limit;
+ u16 max_write_request;
+ u16 max_read_request;
+ u16 nr_channels;
+ u32 chreset_timeout;
+ u32 sw_version;
+ u8 *priority;
+ u8 *weight;
+
+ atomic_t pm_counter;
+ /* Hardware device constants */
+ dma_addr_t dev_physaddr;
+ void __iomem *dev_virtaddr;
+ resource_size_t dev_addrsize;
+
+ struct dentry *debugfs;
+ struct dentry *info;
+ struct dentry *err;
+ struct dentry *mhid_errclr;
+ struct dentry *evt_errclr;
+ struct dentry *ide_errclr;
+ struct dentry *ode_errclr;
+ struct dentry *msi_errclr;
+ struct dentry *tre_errclr;
+ struct dentry *evt_ena;
+ struct platform_device *pdev;
+};
+
+static unsigned int debug_pm;
+module_param(debug_pm, uint, 0644);
+MODULE_PARM_DESC(debug_pm,
+ "debug runtime power management transitions (default: 0)");
+
+#define TRC_PM(...) do { \
+ if (debug_pm) \
+ dev_info(__VA_ARGS__); \
+ } while (0)
+
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+
+#define HIDMA_SHOW(dma, name) \
+ seq_printf(s, #name "=0x%x\n", dma->name)
+
+#define HIDMA_READ_SHOW(dma, name, offset) \
+ do { \
+ u32 val; \
+ val = readl(dma->dev_virtaddr + offset); \
+ seq_printf(s, name "=0x%x\n", val); \
+ } while (0)
+
+/**
+ * qcom_hidma_mgmt_info: display HIDMA device info
+ *
+ * Display the info for the current HIDMA device.
+ */
+static int qcom_hidma_mgmt_info(struct seq_file *s, void *unused)
+{
+ struct qcom_hidma_mgmt_dev *mgmtdev = s->private;
+ u32 val;
+ int i;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ HIDMA_SHOW(mgmtdev, sw_version);
+
+ val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET);
+ seq_printf(s, "ENABLE=%d\n", val & 0x1);
+
+ val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET);
+ seq_printf(s, "reset_timeout=%d\n", val & CHRESET_TIMEOUUT_MASK);
+
+ val = readl(mgmtdev->dev_virtaddr + MHICFG_OFFSET);
+ seq_printf(s, "nr_event_channel=%d\n",
+ (val >> NR_EV_CHANNEL_BIT_POS) & NR_CHANNEL_MASK);
+ seq_printf(s, "nr_tr_channel=%d\n", (val & NR_CHANNEL_MASK));
+ seq_printf(s, "nr_virt_tr_channel=%d\n", mgmtdev->nr_channels);
+ seq_printf(s, "dev_virtaddr=%p\n", &mgmtdev->dev_virtaddr);
+ seq_printf(s, "dev_physaddr=%pap\n", &mgmtdev->dev_physaddr);
+ seq_printf(s, "dev_addrsize=%pap\n", &mgmtdev->dev_addrsize);
+
+ val = readl(mgmtdev->dev_virtaddr + MEMSET_LIMIT_OFFSET);
+ seq_printf(s, "MEMSET_LIMIT_OFFSET=%d\n", val & MEMSET_LIMIT_MASK);
+
+ val = readl(mgmtdev->dev_virtaddr + HW_PARAM_OFFSET);
+ seq_printf(s, "MAX_JOB_SIZE=%d\n",
+ (val >> MAX_JOB_SIZE_BIT_POS) & MAX_JOBSIZE_MASK);
+ seq_printf(s, "TREQ_CAPACITY=%d\n",
+ (val >> TREQ_CAPACITY_BIT_POS) & TREQ_CAPACITY_MASK);
+ seq_printf(s, "MAX_COIDX_DEPTH=%d\n", val & MAX_COIDX_MASK);
+
+ val = readl(mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET);
+ seq_printf(s, "MAX_BUS_WR_REQ_LEN=%d\n",
+ (val >> MAX_BUS_WR_REQ_BIT_POS) & MAX_BUS_REQ_LEN_MASK);
+ seq_printf(s, "MAX_BUS_RD_REQ_LEN=%d\n", val & MAX_BUS_REQ_LEN_MASK);
+
+ val = readl(mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET);
+ seq_printf(s, "MAX_WR_XACTIONS=%d\n",
+ (val >> MAX_WR_XACTIONS_BIT_POS) & MAX_WR_XACTIONS_MASK);
+ seq_printf(s, "MAX_RD_XACTIONS=%d\n", val & MAX_RD_XACTIONS_MASK);
+
+ for (i = 0; i < mgmtdev->nr_channels; i++) {
+ void __iomem *offset;
+
+ offset = mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i);
+ val = readl(offset);
+
+ seq_printf(s, "CH#%d STOP=%d\n",
+ i, (val & (1 << STOP_BIT_POS)) ? 1 : 0);
+ seq_printf(s, "CH#%d TREQ LIMIT EN=%d\n", i,
+ (val & (1 << TREQ_LIMIT_EN_BIT_POS)) ? 1 : 0);
+ seq_printf(s, "CH#%d TREQ LIMIT=%d\n",
+ i, (val >> TREQ_LIMIT_BIT_POS) & TREQ_LIMIT_MASK);
+ seq_printf(s, "CH#%d priority=%d\n", i,
+ (val & (1 << PRIORITY_BIT_POS)) ? 1 : 0);
+ seq_printf(s, "CH#%d WRR=%d\n", i,
+ (val >> WRR_BIT_POS) & WEIGHT_MASK);
+ seq_printf(s, "CH#%d USE_DLA=%d\n", i, (val & 1) ? 1 : 0);
+ }
+ HIDMA_RUNTIME_SET(mgmtdev);
+
+ return 0;
+}
+
+static int qcom_hidma_mgmt_info_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, qcom_hidma_mgmt_info, inode->i_private);
+}
+
+static const struct file_operations qcom_hidma_mgmt_fops = {
+ .open = qcom_hidma_mgmt_info_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+/**
+ * qcom_hidma_mgmt_err: display HIDMA error info
+ *
+ * Display the error info for the current HIDMA device.
+ */
+static int qcom_hidma_mgmt_err(struct seq_file *s, void *unused)
+{
+ u32 val;
+ struct qcom_hidma_mgmt_dev *mgmtdev = s->private;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ val = readl(mgmtdev->dev_virtaddr + MHID_BUS_ERR0_OFFSET);
+ seq_printf(s, "MHID TR_CHID=%d\n", val & MHID_ERR_TRCHID_MASK);
+ seq_printf(s, "MHID RESP_ERROR=%d\n",
+ (val >> MHID_ERR_RESP_BIT_POS) & MHID_ERR_RESP_MASK);
+ HIDMA_READ_SHOW(mgmtdev, "MHID READ_PTR", MHID_BUS_ERR1_OFFSET);
+
+ val = readl(mgmtdev->dev_virtaddr + EVT_BUS_ERR0_OFFSET);
+ seq_printf(s, "EVT TR_CHID=%d\n", val & EVT_ERR_TRCHID_MASK);
+ seq_printf(s, "EVT RESP_ERROR=%d\n",
+ (val >> EVT_ERR_RESP_BIT_POS) & EVT_ERR_RESP_MASK);
+ HIDMA_READ_SHOW(mgmtdev, "EVT WRITE_PTR", EVT_BUS_ERR1_OFFSET);
+
+ val = readl(mgmtdev->dev_virtaddr + IDE_BUS_ERR0_OFFSET);
+ seq_printf(s, "IDE TR_CHID=%d\n", val & IDE_ERR_TRCHID_MASK);
+ seq_printf(s, "IDE RESP_ERROR=%d\n",
+ (val >> IDE_ERR_RESP_BIT_POS) & IDE_ERR_RESP_MASK);
+ seq_printf(s, "IDE REQ_LENGTH=%d\n",
+ (val >> IDE_ERR_REQLEN_BIT_POS) & IDE_ERR_REQLEN_MASK);
+ HIDMA_READ_SHOW(mgmtdev, "IDE ADDR_LSB", IDE_BUS_ERR1_OFFSET);
+ HIDMA_READ_SHOW(mgmtdev, "IDE ADDR_MSB", IDE_BUS_ERR2_OFFSET);
+
+ val = readl(mgmtdev->dev_virtaddr + ODE_BUS_ERR0_OFFSET);
+ seq_printf(s, "ODE TR_CHID=%d\n", val & ODE_ERR_TRCHID_MASK);
+ seq_printf(s, "ODE RESP_ERROR=%d\n",
+ (val >> ODE_ERR_RESP_BIT_POS) & ODE_ERR_RESP_MASK);
+ seq_printf(s, "ODE REQ_LENGTH=%d\n",
+ (val >> ODE_ERR_REQLEN_BIT_POS) & ODE_ERR_REQLEN_MASK);
+ HIDMA_READ_SHOW(mgmtdev, "ODE ADDR_LSB", ODE_BUS_ERR1_OFFSET);
+ HIDMA_READ_SHOW(mgmtdev, "ODE ADDR_MSB", ODE_BUS_ERR2_OFFSET);
+
+ val = readl(mgmtdev->dev_virtaddr + MSI_BUS_ERR0_OFFSET);
+ seq_printf(s, "MSI TR_CHID=%d\n", val & MSI_ERR_TRCHID_MASK);
+ seq_printf(s, "MSI RESP_ERROR=%d\n",
+ (val >> MSI_ERR_RESP_BIT_POS) & MSI_ERR_RESP_MASK);
+
+ val = readl(mgmtdev->dev_virtaddr + TRE_ERR0_OFFSET);
+ seq_printf(s, "TRE TRE_TYPE=%d\n", val & TRE_ERR_TYPE_MASK);
+ seq_printf(s, "TRE TRE_EVRIDX=%d\n",
+ (val >> TRE_ERR_EVRIDX_BIT_POS) & TRE_ERR_EVRIDX_MASK);
+ seq_printf(s, "TRE TRE_IEOB=%d\n",
+ (val >> TRE_ERR_IEOB_BIT_POS) & 1);
+ seq_printf(s, "TRE TRCHID=%d\n",
+ (val >> TRE_ERR_TRCHID_BIT_POS) & TRE_ERR_TRCHID_MASK);
+
+ HIDMA_READ_SHOW(mgmtdev, "HW_EVENTS_CFG_OFFSET",
+ HW_EVENTS_CFG_OFFSET);
+
+ HIDMA_RUNTIME_SET(mgmtdev);
+ return 0;
+}
+
+static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, qcom_hidma_mgmt_err, inode->i_private);
+}
+
+static const struct file_operations qcom_hidma_mgmt_err_fops = {
+ .open = qcom_hidma_mgmt_err_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file,
+ const char __user *user_buf, size_t count, loff_t *ppos)
+{
+ struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET);
+ HIDMA_RUNTIME_SET(mgmtdev);
+ return count;
+}
+
+static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = {
+ .write = qcom_hidma_mgmt_mhiderr_clr,
+};
+
+static ssize_t qcom_hidma_mgmt_evterr_clr(struct file *file,
+ const char __user *user_buf, size_t count, loff_t *ppos)
+{
+ struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ writel(1, mgmtdev->dev_virtaddr + EVT_BUS_ERR_CLR_OFFSET);
+ HIDMA_RUNTIME_SET(mgmtdev);
+ return count;
+}
+
+static const struct file_operations qcom_hidma_mgmt_evterr_clrfops = {
+ .write = qcom_hidma_mgmt_evterr_clr,
+};
+
+static ssize_t qcom_hidma_mgmt_ideerr_clr(struct file *file,
+ const char __user *user_buf, size_t count, loff_t *ppos)
+{
+ struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ writel(1, mgmtdev->dev_virtaddr + IDE_BUS_ERR_CLR_OFFSET);
+ HIDMA_RUNTIME_SET(mgmtdev);
+ return count;
+}
+
+static const struct file_operations qcom_hidma_mgmt_ideerr_clrfops = {
+ .write = qcom_hidma_mgmt_ideerr_clr,
+};
+
+static ssize_t qcom_hidma_mgmt_odeerr_clr(struct file *file,
+ const char __user *user_buf, size_t count, loff_t *ppos)
+{
+ struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ writel(1, mgmtdev->dev_virtaddr + ODE_BUS_ERR_CLR_OFFSET);
+ HIDMA_RUNTIME_SET(mgmtdev);
+ return count;
+}
+
+static const struct file_operations qcom_hidma_mgmt_odeerr_clrfops = {
+ .write = qcom_hidma_mgmt_odeerr_clr,
+};
+
+static ssize_t qcom_hidma_mgmt_msierr_clr(struct file *file,
+ const char __user *user_buf, size_t count, loff_t *ppos)
+{
+ struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ writel(1, mgmtdev->dev_virtaddr + MSI_BUS_ERR_CLR_OFFSET);
+ HIDMA_RUNTIME_SET(mgmtdev);
+ return count;
+}
+
+static const struct file_operations qcom_hidma_mgmt_msierr_clrfops = {
+ .write = qcom_hidma_mgmt_msierr_clr,
+};
+
+static ssize_t qcom_hidma_mgmt_treerr_clr(struct file *file,
+ const char __user *user_buf, size_t count, loff_t *ppos)
+{
+ struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ writel(1, mgmtdev->dev_virtaddr + TRE_ERR_CLR_OFFSET);
+ HIDMA_RUNTIME_SET(mgmtdev);
+ return count;
+}
+
+static const struct file_operations qcom_hidma_mgmt_treerr_clrfops = {
+ .write = qcom_hidma_mgmt_treerr_clr,
+};
+
+static ssize_t qcom_hidma_mgmt_evtena(struct file *file,
+ const char __user *user_buf, size_t count, loff_t *ppos)
+{
+ char temp_buf[16+1];
+ struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
+ u32 event;
+ ssize_t ret;
+ unsigned long val;
+
+ temp_buf[16] = '\0';
+ if (copy_from_user(temp_buf, user_buf, min_t(int, count, 16)))
+ goto out;
+
+ ret = kstrtoul(temp_buf, 16, &val);
+ if (ret) {
+ pr_warn(PREFIX "unknown event\n");
+ goto out;
+ }
+
+ event = (u32)val & HW_EVENTS_CFG_MASK;
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ writel(event, mgmtdev->dev_virtaddr + HW_EVENTS_CFG_OFFSET);
+ HIDMA_RUNTIME_SET(mgmtdev);
+out:
+ return count;
+}
+
+static const struct file_operations qcom_hidma_mgmt_evtena_fops = {
+ .write = qcom_hidma_mgmt_evtena,
+};
+
+static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
+{
+ debugfs_remove(mgmtdev->evt_ena);
+ debugfs_remove(mgmtdev->tre_errclr);
+ debugfs_remove(mgmtdev->msi_errclr);
+ debugfs_remove(mgmtdev->ode_errclr);
+ debugfs_remove(mgmtdev->ide_errclr);
+ debugfs_remove(mgmtdev->evt_errclr);
+ debugfs_remove(mgmtdev->mhid_errclr);
+ debugfs_remove(mgmtdev->err);
+ debugfs_remove(mgmtdev->info);
+ debugfs_remove(mgmtdev->debugfs);
+}
+
+static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
+{
+ int rc = 0;
+
+ mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev),
+ NULL);
+ if (!mgmtdev->debugfs) {
+ rc = -ENODEV;
+ return rc;
+ }
+
+ mgmtdev->info = debugfs_create_file("info", S_IRUGO,
+ mgmtdev->debugfs, mgmtdev, &qcom_hidma_mgmt_fops);
+ if (!mgmtdev->info) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ mgmtdev->err = debugfs_create_file("err", S_IRUGO,
+ mgmtdev->debugfs, mgmtdev,
+ &qcom_hidma_mgmt_err_fops);
+ if (!mgmtdev->err) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ mgmtdev->mhid_errclr = debugfs_create_file("mhiderrclr", S_IWUSR,
+ mgmtdev->debugfs, mgmtdev,
+ &qcom_hidma_mgmt_mhiderr_clrfops);
+ if (!mgmtdev->mhid_errclr) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ mgmtdev->evt_errclr = debugfs_create_file("evterrclr", S_IWUSR,
+ mgmtdev->debugfs, mgmtdev,
+ &qcom_hidma_mgmt_evterr_clrfops);
+ if (!mgmtdev->evt_errclr) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ mgmtdev->ide_errclr = debugfs_create_file("ideerrclr", S_IWUSR,
+ mgmtdev->debugfs, mgmtdev,
+ &qcom_hidma_mgmt_ideerr_clrfops);
+ if (!mgmtdev->ide_errclr) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ mgmtdev->ode_errclr = debugfs_create_file("odeerrclr", S_IWUSR,
+ mgmtdev->debugfs, mgmtdev,
+ &qcom_hidma_mgmt_odeerr_clrfops);
+ if (!mgmtdev->ode_errclr) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ mgmtdev->msi_errclr = debugfs_create_file("msierrclr", S_IWUSR,
+ mgmtdev->debugfs, mgmtdev,
+ &qcom_hidma_mgmt_msierr_clrfops);
+ if (!mgmtdev->msi_errclr) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ mgmtdev->tre_errclr = debugfs_create_file("treerrclr", S_IWUSR,
+ mgmtdev->debugfs, mgmtdev,
+ &qcom_hidma_mgmt_treerr_clrfops);
+ if (!mgmtdev->tre_errclr) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ mgmtdev->evt_ena = debugfs_create_file("evtena", S_IWUSR,
+ mgmtdev->debugfs, mgmtdev,
+ &qcom_hidma_mgmt_evtena_fops);
+ if (!mgmtdev->evt_ena) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ return 0;
+cleanup:
+ qcom_hidma_mgmt_debug_uninit(mgmtdev);
+ return rc;
+}
+#else
+static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
+{
+}
+static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
+{
+ return 0;
+}
+#endif
+
+static irqreturn_t qcom_hidma_mgmt_irq_handler(int irq, void *arg)
+{
+ /* TODO: handle irq here */
+ return IRQ_HANDLED;
+}
+
+static int qcom_hidma_mgmt_setup(struct qcom_hidma_mgmt_dev *mgmtdev)
+{
+ u32 val;
+ int i;
+
+ val = readl(mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET);
+
+ if (mgmtdev->max_write_request) {
+ val = val &
+ ~(MAX_BUS_REQ_LEN_MASK << MAX_BUS_WR_REQ_BIT_POS);
+ val = val |
+ (mgmtdev->max_write_request << MAX_BUS_WR_REQ_BIT_POS);
+ }
+ if (mgmtdev->max_read_request) {
+ val = val & ~(MAX_BUS_REQ_LEN_MASK);
+ val = val | (mgmtdev->max_read_request);
+ }
+ writel(val, mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET);
+
+ val = readl(mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET);
+ if (mgmtdev->max_wr_xactions) {
+ val = val &
+ ~(MAX_WR_XACTIONS_MASK << MAX_WR_XACTIONS_BIT_POS);
+ val = val |
+ (mgmtdev->max_wr_xactions << MAX_WR_XACTIONS_BIT_POS);
+ }
+ if (mgmtdev->max_rd_xactions) {
+ val = val & ~(MAX_RD_XACTIONS_MASK);
+ val = val | (mgmtdev->max_rd_xactions);
+ }
+ writel(val, mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET);
+
+ val = readl(mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET);
+ mgmtdev->max_write_request =
+ (val >> MAX_BUS_WR_REQ_BIT_POS) & MAX_BUS_REQ_LEN_MASK;
+ mgmtdev->max_read_request = val & MAX_BUS_REQ_LEN_MASK;
+
+ val = readl(mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET);
+ mgmtdev->max_wr_xactions =
+ (val >> MAX_WR_XACTIONS_BIT_POS) & MAX_WR_XACTIONS_MASK;
+ mgmtdev->max_rd_xactions = val & MAX_RD_XACTIONS_MASK;
+
+ mgmtdev->sw_version = readl(mgmtdev->dev_virtaddr + SW_VERSION_OFFSET);
+
+ for (i = 0; i < mgmtdev->nr_channels; i++) {
+ val = readl(mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i));
+ val = val & ~(1 << PRIORITY_BIT_POS);
+ val = val |
+ ((mgmtdev->priority[i] & 0x1) << PRIORITY_BIT_POS);
+ val = val & ~(WEIGHT_MASK << WRR_BIT_POS);
+ val = val
+ | ((mgmtdev->weight[i] & WEIGHT_MASK) << WRR_BIT_POS);
+ writel(val, mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i));
+ }
+
+ if (mgmtdev->chreset_timeout > 0) {
+ val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET);
+ val = val & ~CHRESET_TIMEOUUT_MASK;
+ val = val | (mgmtdev->chreset_timeout & CHRESET_TIMEOUUT_MASK);
+ writel(val, mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET);
+ }
+ val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET);
+ mgmtdev->chreset_timeout = val & CHRESET_TIMEOUUT_MASK;
+
+ if (mgmtdev->max_memset_limit > 0) {
+ val = readl(mgmtdev->dev_virtaddr + MEMSET_LIMIT_OFFSET);
+ val = val & ~MEMSET_LIMIT_MASK;
+ val = val | (mgmtdev->max_memset_limit & MEMSET_LIMIT_MASK);
+ writel(val, mgmtdev->dev_virtaddr + MEMSET_LIMIT_OFFSET);
+ }
+ val = readl(mgmtdev->dev_virtaddr + MEMSET_LIMIT_OFFSET);
+ mgmtdev->max_memset_limit = val & MEMSET_LIMIT_MASK;
+
+ val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET);
+ val = val | 1;
+ writel(val, mgmtdev->dev_virtaddr + CFG_OFFSET);
+
+ return 0;
+}
+
+static int qcom_hidma_mgmt_probe(struct platform_device *pdev)
+{
+ struct resource *dma_resource;
+ int irq;
+ int rc, i;
+ struct qcom_hidma_mgmt_dev *mgmtdev;
+
+ pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ dma_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!dma_resource) {
+ dev_err(&pdev->dev, "No memory resources found\n");
+ rc = -ENODEV;
+ goto out;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "irq resources not found\n");
+ rc = -ENODEV;
+ goto out;
+ }
+
+ mgmtdev = devm_kzalloc(&pdev->dev, sizeof(*mgmtdev), GFP_KERNEL);
+ if (!mgmtdev) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ mgmtdev->pdev = pdev;
+ HIDMA_RUNTIME_GET(mgmtdev);
+
+ rc = devm_request_irq(&pdev->dev, irq, qcom_hidma_mgmt_irq_handler,
+ IRQF_SHARED, "qcom-hidmamgmt", mgmtdev);
+ if (rc) {
+ dev_err(&pdev->dev, "irq registration failed: %d\n", irq);
+ goto out;
+ }
+
+ mgmtdev->dev_physaddr = dma_resource->start;
+ mgmtdev->dev_addrsize = resource_size(dma_resource);
+
+ dev_dbg(&pdev->dev, "dev_physaddr:%pa\n", &mgmtdev->dev_physaddr);
+ dev_dbg(&pdev->dev, "dev_addrsize:%pa\n", &mgmtdev->dev_addrsize);
+
+ mgmtdev->dev_virtaddr = devm_ioremap_resource(&pdev->dev,
+ dma_resource);
+ if (IS_ERR(mgmtdev->dev_virtaddr)) {
+ dev_err(&pdev->dev, "can't map i/o memory at %pa\n",
+ &mgmtdev->dev_physaddr);
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ if (device_property_read_u16(&pdev->dev, "nr-channels",
+ &mgmtdev->nr_channels)) {
+ dev_err(&pdev->dev, "number of channels missing\n");
+ rc = -EINVAL;
+ goto out;
+ }
+
+ device_property_read_u16(&pdev->dev, "max-write",
+ &mgmtdev->max_write_request);
+ if ((mgmtdev->max_write_request != 128) &&
+ (mgmtdev->max_write_request != 256) &&
+ (mgmtdev->max_write_request != 512) &&
+ (mgmtdev->max_write_request != 1024)) {
+ dev_err(&pdev->dev, "invalid write request %d\n",
+ mgmtdev->max_write_request);
+ rc = -EINVAL;
+ goto out;
+ }
+
+ device_property_read_u16(&pdev->dev, "max-read",
+ &mgmtdev->max_read_request);
+ if ((mgmtdev->max_read_request != 128) &&
+ (mgmtdev->max_read_request != 256) &&
+ (mgmtdev->max_read_request != 512) &&
+ (mgmtdev->max_read_request != 1024)) {
+ dev_err(&pdev->dev, "invalid read request %d\n",
+ mgmtdev->max_read_request);
+ rc = -EINVAL;
+ goto out;
+ }
+
+ device_property_read_u8(&pdev->dev, "max-wxactions",
+ &mgmtdev->max_wr_xactions);
+
+ device_property_read_u8(&pdev->dev, "max-rdactions",
+ &mgmtdev->max_rd_xactions);
+
+ device_property_read_u8(&pdev->dev, "max-memset-limit",
+ &mgmtdev->max_memset_limit);
+
+ /* needs to be at least one */
+ if (mgmtdev->max_memset_limit == 0)
+ mgmtdev->max_memset_limit = 1;
+
+ mgmtdev->priority = devm_kcalloc(&pdev->dev,
+ mgmtdev->nr_channels, sizeof(*mgmtdev->priority), GFP_KERNEL);
+ if (!mgmtdev->priority) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ mgmtdev->weight = devm_kcalloc(&pdev->dev,
+ mgmtdev->nr_channels, sizeof(*mgmtdev->weight), GFP_KERNEL);
+ if (!mgmtdev->weight) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ for (i = 0; i < mgmtdev->nr_channels; i++) {
+ char name[30];
+
+ sprintf(name, "ch-priority-%d", i);
+ device_property_read_u8(&pdev->dev, name,
+ &mgmtdev->priority[i]);
+
+ sprintf(name, "ch-weight-%d", i);
+ device_property_read_u8(&pdev->dev, name,
+ &mgmtdev->weight[i]);
+
+ if (mgmtdev->weight[i] > 15) {
+ dev_err(&pdev->dev, "max value of weight can be 15.\n");
+ rc = -EINVAL;
+ goto out;
+ }
+
+ /* weight needs to be at least one */
+ if (mgmtdev->weight[i] == 0)
+ mgmtdev->weight[i] = 1;
+ }
+
+ rc = qcom_hidma_mgmt_setup(mgmtdev);
+ if (rc) {
+ dev_err(&pdev->dev, "setup failed\n");
+ goto out;
+ }
+
+ rc = qcom_hidma_mgmt_debug_init(mgmtdev);
+ if (rc) {
+ dev_err(&pdev->dev, "debugfs init failed\n");
+ goto out;
+ }
+
+ dev_info(&pdev->dev,
+ "HI-DMA engine management driver registration complete\n");
+ platform_set_drvdata(pdev, mgmtdev);
+ HIDMA_RUNTIME_SET(mgmtdev);
+ return 0;
+out:
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_sync_suspend(&pdev->dev);
+ return rc;
+}
+
+static int qcom_hidma_mgmt_remove(struct platform_device *pdev)
+{
+ struct qcom_hidma_mgmt_dev *mgmtdev = platform_get_drvdata(pdev);
+
+ HIDMA_RUNTIME_GET(mgmtdev);
+ qcom_hidma_mgmt_debug_uninit(mgmtdev);
+ pm_runtime_put_sync_suspend(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+
+ dev_info(&pdev->dev, "HI-DMA engine management driver removed\n");
+ return 0;
+}
+
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = {
+ {"QCOM8060"},
+ {},
+};
+#endif
+
+static const struct of_device_id qcom_hidma_mgmt_match[] = {
+ { .compatible = "qcom,hidma_mgmt", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, qcom_hidma_mgmt_match);
+
+static struct platform_driver qcom_hidma_mgmt_driver = {
+ .probe = qcom_hidma_mgmt_probe,
+ .remove = qcom_hidma_mgmt_remove,
+ .driver = {
+ .name = MODULE_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(qcom_hidma_mgmt_match),
+ .acpi_match_table = ACPI_PTR(qcom_hidma_mgmt_acpi_ids),
+ },
+};
+
+static int __init qcom_hidma_mgmt_init(void)
+{
+ return platform_driver_register(&qcom_hidma_mgmt_driver);
+}
+device_initcall(qcom_hidma_mgmt_init);
+
+static void __exit qcom_hidma_mgmt_exit(void)
+{
+ platform_driver_unregister(&qcom_hidma_mgmt_driver);
+}
+module_exit(qcom_hidma_mgmt_exit);
--
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | kbuild test robot <lkp@intel.com> |
|---|---|
| Date | 2015-10-30 05:40 +0100 |
| Subject | Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver |
| Message-ID | <qp9DQ-1c0-5@gated-at.bofh.it> |
| In reply to | #1259183 |
Hi Sinan, [auto build test WARNING on lwn/docs-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base] url: https://github.com/0day-ci/linux/commits/Sinan-Kaya/dma-add-Qualcomm-Technologies-HIDMA-management-driver/20151030-111408 coccinelle warnings: (new ones prefixed by >>) >> drivers/dma/qcom_hidma_mgmt.c:852:3-8: No need to set .owner here. The core will do it. Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | kbuild test robot <lkp@intel.com> |
|---|---|
| Date | 2015-10-30 05:40 +0100 |
| Subject | [PATCH] dma: fix platform_no_drv_owner.cocci warnings |
| Message-ID | <qp9DQ-1c0-7@gated-at.bofh.it> |
| In reply to | #1259183 |
drivers/dma/qcom_hidma_mgmt.c:852:3-8: No need to set .owner here. The core will do it.
Remove .owner field if calls are used which set it automatically
Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
CC: Sinan Kaya <okaya@codeaurora.org>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
qcom_hidma_mgmt.c | 1 -
1 file changed, 1 deletion(-)
--- a/drivers/dma/qcom_hidma_mgmt.c
+++ b/drivers/dma/qcom_hidma_mgmt.c
@@ -849,7 +849,6 @@ static struct platform_driver qcom_hidma
.remove = qcom_hidma_mgmt_remove,
.driver = {
.name = MODULE_NAME,
- .owner = THIS_MODULE,
.of_match_table = of_match_ptr(qcom_hidma_mgmt_match),
.acpi_match_table = ACPI_PTR(qcom_hidma_mgmt_acpi_ids),
},
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2015-10-30 10:40 +0100 |
| Message-ID | <qpeka-45t-33@gated-at.bofh.it> |
| In reply to | #1259183 |
On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote:
> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
> new file mode 100644
> index 0000000..81674ab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
> @@ -0,0 +1,42 @@
> +Qualcomm Technologies HIDMA Management interface
> +
> +Required properties:
> +- compatible: must contain "qcom,hidma_mgmt"
No underscores in the name please. Also try to be more specific, in case this
device shows up in more than one SoC and they end up not being 100% compatible
we want to be able to tell them apart.
> +- reg: Address range for DMA device
> +- interrupts: Should contain the one interrupt shared by all channels
> +- nr-channels: Number of channels supported by this DMA controller.
> +- max-write: Maximum write burst in bytes. A memcpy requested is
> + fragmented to multiples of this amount.
> +- max-read: Maximum read burst in bytes. A memcpy request is
> + fragmented to multiples of this amount.
> +- max-wxactions: Maximum write transactions to perform in a burst
> +- max-rdactions: Maximum read transactions to perform in a burst
> +- max-memset-limit: Maximum memset limit
> +- ch-priority-#n: Priority of the channel
> +- ch-weight-#n: Round robin weight of the channel
For the last two, you can just use a multi-cell property, using one
cell per channel.
> +
> +#define MODULE_NAME "hidma-mgmt"
> +#define PREFIX MODULE_NAME ": "
These can be removed, just use dev_info() etc for messages, to include
a unique identifier.
> +#define AUTOSUSPEND_TIMEOUT 2000
> +
> +#define HIDMA_RUNTIME_GET(dmadev) \
> +do { \
> + atomic_inc(&(dmadev)->pm_counter); \
> + TRC_PM(&(dmadev)->pdev->dev, \
> + "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\
> + atomic_read(&(dmadev)->pm_counter)); \
> + pm_runtime_get_sync(&(dmadev)->pdev->dev); \
> +} while (0)
> +
> +#define HIDMA_RUNTIME_SET(dmadev) \
> +do { \
> + atomic_dec(&(dmadev)->pm_counter); \
> + TRC_PM(&(dmadev)->pdev->dev, \
> + "%s:%d pm_runtime_put_autosuspend:%d\n", \
> + __func__, __LINE__, \
> + atomic_read(&(dmadev)->pm_counter)); \
> + pm_runtime_mark_last_busy(&(dmadev)->pdev->dev); \
> + pm_runtime_put_autosuspend(&(dmadev)->pdev->dev); \
> +} while (0)
Better use inline functions.
> +struct qcom_hidma_mgmt_dev {
> + u8 max_wr_xactions;
> + u8 max_rd_xactions;
> + u8 max_memset_limit;
> + u16 max_write_request;
> + u16 max_read_request;
> + u16 nr_channels;
> + u32 chreset_timeout;
> + u32 sw_version;
> + u8 *priority;
> + u8 *weight;
> +
> + atomic_t pm_counter;
> + /* Hardware device constants */
> + dma_addr_t dev_physaddr;
No need to store the physaddr, it's write-only here.
> +
> +static unsigned int debug_pm;
> +module_param(debug_pm, uint, 0644);
> +MODULE_PARM_DESC(debug_pm,
> + "debug runtime power management transitions (default: 0)");
> +
> +#define TRC_PM(...) do { \
> + if (debug_pm) \
> + dev_info(__VA_ARGS__); \
> + } while (0)
> +
Once you are done debugging the driver and have a final version, please
remove these.
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +
> +#define HIDMA_SHOW(dma, name) \
> + seq_printf(s, #name "=0x%x\n", dma->name)
Only used once, just remove.
> +#define HIDMA_READ_SHOW(dma, name, offset) \
> + do { \
> + u32 val; \
> + val = readl(dma->dev_virtaddr + offset); \
> + seq_printf(s, name "=0x%x\n", val); \
> + } while (0)
Inline function.
> +/**
> + * qcom_hidma_mgmt_info: display HIDMA device info
> + *
> + * Display the info for the current HIDMA device.
> + */
> +static int qcom_hidma_mgmt_info(struct seq_file *s, void *unused)
> +{
> + struct qcom_hidma_mgmt_dev *mgmtdev = s->private;
> + u32 val;
> + int i;
> +
> + HIDMA_RUNTIME_GET(mgmtdev);
> + HIDMA_SHOW(mgmtdev, sw_version);
> +
> + val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET);
> + seq_printf(s, "ENABLE=%d\n", val & 0x1);
> +
> + val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET);
> + seq_printf(s, "reset_timeout=%d\n", val & CHRESET_TIMEOUUT_MASK);
> +
> + val = readl(mgmtdev->dev_virtaddr + MHICFG_OFFSET);
> + seq_printf(s, "nr_event_channel=%d\n",
> + (val >> NR_EV_CHANNEL_BIT_POS) & NR_CHANNEL_MASK);
> + seq_printf(s, "nr_tr_channel=%d\n", (val & NR_CHANNEL_MASK));
> + seq_printf(s, "nr_virt_tr_channel=%d\n", mgmtdev->nr_channels);
> + seq_printf(s, "dev_virtaddr=%p\n", &mgmtdev->dev_virtaddr);
> + seq_printf(s, "dev_physaddr=%pap\n", &mgmtdev->dev_physaddr);
> + seq_printf(s, "dev_addrsize=%pap\n", &mgmtdev->dev_addrsize);
printing pointers is a security risk, just remove them if they are
not essential here.
> +
> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, qcom_hidma_mgmt_err, inode->i_private);
> +}
> +
> +static const struct file_operations qcom_hidma_mgmt_err_fops = {
> + .open = qcom_hidma_mgmt_err_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file,
> + const char __user *user_buf, size_t count, loff_t *ppos)
> +{
> + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
> +
> + HIDMA_RUNTIME_GET(mgmtdev);
> + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET);
> + HIDMA_RUNTIME_SET(mgmtdev);
> + return count;
> +}
> +
> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = {
> + .write = qcom_hidma_mgmt_mhiderr_clr,
> +};
Is this really just a debugging interface? If anyone would do this
for normal operation, it needs to be a proper API.
> +
> +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
> +{
> + debugfs_remove(mgmtdev->evt_ena);
> + debugfs_remove(mgmtdev->tre_errclr);
> + debugfs_remove(mgmtdev->msi_errclr);
> + debugfs_remove(mgmtdev->ode_errclr);
> + debugfs_remove(mgmtdev->ide_errclr);
> + debugfs_remove(mgmtdev->evt_errclr);
> + debugfs_remove(mgmtdev->mhid_errclr);
> + debugfs_remove(mgmtdev->err);
> + debugfs_remove(mgmtdev->info);
> + debugfs_remove(mgmtdev->debugfs);
> +}
>
I guess debugfs_remove_recursive() would do the job as well.
> +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
> +{
> + int rc = 0;
> +
> + mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev),
> + NULL);
> + if (!mgmtdev->debugfs) {
> + rc = -ENODEV;
> + return rc;
> + }
> +
> + mgmtdev->info = debugfs_create_file("info", S_IRUGO,
> + mgmtdev->debugfs, mgmtdev, &qcom_hidma_mgmt_fops);
> + if (!mgmtdev->info) {
> + rc = -ENOMEM;
> + goto cleanup;
> + }
> +
> + mgmtdev->err = debugfs_create_file("err", S_IRUGO,
> + mgmtdev->debugfs, mgmtdev,
> + &qcom_hidma_mgmt_err_fops);
> + if (!mgmtdev->err) {
> + rc = -ENOMEM;
> + goto cleanup;
> + }
> +
> + mgmtdev->mhid_errclr = debugfs_create_file("mhiderrclr", S_IWUSR,
> + mgmtdev->debugfs, mgmtdev,
> + &qcom_hidma_mgmt_mhiderr_clrfops);
> + if (!mgmtdev->mhid_errclr) {
> + rc = -ENOMEM;
> + goto cleanup;
> + }
Maybe use a loop around an array here to avoid writing the same thing 10 times?
Also, you could move the debugging code into a separate file and have a separate
Kconfig symbol so users can disable this if they do not debug your driver
but still want to use debugfs for other things.
> +static irqreturn_t qcom_hidma_mgmt_irq_handler(int irq, void *arg)
> +{
> + /* TODO: handle irq here */
> + return IRQ_HANDLED;
> +}
> + rc = devm_request_irq(&pdev->dev, irq, qcom_hidma_mgmt_irq_handler,
> + IRQF_SHARED, "qcom-hidmamgmt", mgmtdev);
> + if (rc) {
> + dev_err(&pdev->dev, "irq registration failed: %d\n", irq);
> + goto out;
> + }
If you create a shared handler, you must check whether there was an
interrupt pending, otherwise you will lose interrupts for all other devices
that share the same IRQ.
Better remove the handler completely here so you don't need to check it.
> +
> + if (device_property_read_u16(&pdev->dev, "nr-channels",
> + &mgmtdev->nr_channels)) {
> + dev_err(&pdev->dev, "number of channels missing\n");
> + rc = -EINVAL;
> + goto out;
> + }
This should be a u32 name 'dma-channels' for consistency with the
generic DMA binding.
Also, try to avoid using u8 and u16 properties everywhere and just
us u32 for consistency.
> +
> + for (i = 0; i < mgmtdev->nr_channels; i++) {
> + char name[30];
> +
> + sprintf(name, "ch-priority-%d", i);
> + device_property_read_u8(&pdev->dev, name,
> + &mgmtdev->priority[i]);
> +
> + sprintf(name, "ch-weight-%d", i);
> + device_property_read_u8(&pdev->dev, name,
> + &mgmtdev->weight[i]);
Per comment above, just read this as an array.
> + dev_info(&pdev->dev,
> + "HI-DMA engine management driver registration complete\n");
> + platform_set_drvdata(pdev, mgmtdev);
> + HIDMA_RUNTIME_SET(mgmtdev);
> + return 0;
> +out:
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_put_sync_suspend(&pdev->dev);
> + return rc;
> +}
The rest of the probe function does not register any user interface aside from
the debugging stuff. Can you explain in the changelog how you expect the
driver to be used in a real system? Is there another driver coming?
> +
> +static const struct of_device_id qcom_hidma_mgmt_match[] = {
> + { .compatible = "qcom,hidma_mgmt", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, qcom_hidma_mgmt_match);
> +
> +static struct platform_driver qcom_hidma_mgmt_driver = {
> + .probe = qcom_hidma_mgmt_probe,
> + .remove = qcom_hidma_mgmt_remove,
> + .driver = {
> + .name = MODULE_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(qcom_hidma_mgmt_match),
drop the .owner field and the of_match_ptr() macro to avoid warnings here.
> +static int __init qcom_hidma_mgmt_init(void)
> +{
> + return platform_driver_register(&qcom_hidma_mgmt_driver);
> +}
> +device_initcall(qcom_hidma_mgmt_init);
> +
> +static void __exit qcom_hidma_mgmt_exit(void)
> +{
> + platform_driver_unregister(&qcom_hidma_mgmt_driver);
> +}
> +module_exit(qcom_hidma_mgmt_exit);
module_platform_driver()
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2015-10-31 08:00 +0100 |
| Subject | Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver |
| Message-ID | <qpyiR-7Zq-3@gated-at.bofh.it> |
| In reply to | #1259357 |
On 10/30/2015 5:34 AM, Arnd Bergmann wrote:
> On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote:
>> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
>> new file mode 100644
>> index 0000000..81674ab
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
>> @@ -0,0 +1,42 @@
>> +Qualcomm Technologies HIDMA Management interface
>> +
>> +Required properties:
>> +- compatible: must contain "qcom,hidma_mgmt"
>
> No underscores in the name please. Also try to be more specific, in case this
> device shows up in more than one SoC and they end up not being 100% compatible
> we want to be able to tell them apart.
ok
>
>> +- reg: Address range for DMA device
>> +- interrupts: Should contain the one interrupt shared by all channels
>> +- nr-channels: Number of channels supported by this DMA controller.
>> +- max-write: Maximum write burst in bytes. A memcpy requested is
>> + fragmented to multiples of this amount.
>> +- max-read: Maximum read burst in bytes. A memcpy request is
>> + fragmented to multiples of this amount.
>> +- max-wxactions: Maximum write transactions to perform in a burst
>> +- max-rdactions: Maximum read transactions to perform in a burst
>> +- max-memset-limit: Maximum memset limit
>> +- ch-priority-#n: Priority of the channel
>> +- ch-weight-#n: Round robin weight of the channel
>
> For the last two, you can just use a multi-cell property, using one
> cell per channel.
will look
>
>> +
>> +#define MODULE_NAME "hidma-mgmt"
>> +#define PREFIX MODULE_NAME ": "
>
> These can be removed, just use dev_info() etc for messages, to include
> a unique identifier.
got rid of them
>
>> +#define AUTOSUSPEND_TIMEOUT 2000
>> +
>> +#define HIDMA_RUNTIME_GET(dmadev) \
>> +do { \
>> + atomic_inc(&(dmadev)->pm_counter); \
>> + TRC_PM(&(dmadev)->pdev->dev, \
>> + "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\
>> + atomic_read(&(dmadev)->pm_counter)); \
>> + pm_runtime_get_sync(&(dmadev)->pdev->dev); \
>> +} while (0)
>> +
>> +#define HIDMA_RUNTIME_SET(dmadev) \
>> +do { \
>> + atomic_dec(&(dmadev)->pm_counter); \
>> + TRC_PM(&(dmadev)->pdev->dev, \
>> + "%s:%d pm_runtime_put_autosuspend:%d\n", \
>> + __func__, __LINE__, \
>> + atomic_read(&(dmadev)->pm_counter)); \
>> + pm_runtime_mark_last_busy(&(dmadev)->pdev->dev); \
>> + pm_runtime_put_autosuspend(&(dmadev)->pdev->dev); \
>> +} while (0)
>
> Better use inline functions.
removed these
>
>> +struct qcom_hidma_mgmt_dev {
>> + u8 max_wr_xactions;
>> + u8 max_rd_xactions;
>> + u8 max_memset_limit;
>> + u16 max_write_request;
>> + u16 max_read_request;
>> + u16 nr_channels;
>> + u32 chreset_timeout;
>> + u32 sw_version;
>> + u8 *priority;
>> + u8 *weight;
>> +
>> + atomic_t pm_counter;
>> + /* Hardware device constants */
>> + dma_addr_t dev_physaddr;
>
> No need to store the physaddr, it's write-only here.
ok
>
>> +
>> +static unsigned int debug_pm;
>> +module_param(debug_pm, uint, 0644);
>> +MODULE_PARM_DESC(debug_pm,
>> + "debug runtime power management transitions (default: 0)");
>> +
>> +#define TRC_PM(...) do { \
>> + if (debug_pm) \
>> + dev_info(__VA_ARGS__); \
>> + } while (0)
>> +
>
> Once you are done debugging the driver and have a final version, please
> remove these.
removed TRC_PM
>
>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>> +
>> +#define HIDMA_SHOW(dma, name) \
>> + seq_printf(s, #name "=0x%x\n", dma->name)
>
> Only used once, just remove.
ok
>
>> +#define HIDMA_READ_SHOW(dma, name, offset) \
>> + do { \
>> + u32 val; \
>> + val = readl(dma->dev_virtaddr + offset); \
>> + seq_printf(s, name "=0x%x\n", val); \
>> + } while (0)
>
> Inline function.
ok
>
>> +/**
>> + * qcom_hidma_mgmt_info: display HIDMA device info
>> + *
>> + * Display the info for the current HIDMA device.
>> + */
>> +static int qcom_hidma_mgmt_info(struct seq_file *s, void *unused)
>> +{
>> + struct qcom_hidma_mgmt_dev *mgmtdev = s->private;
>> + u32 val;
>> + int i;
>> +
>> + HIDMA_RUNTIME_GET(mgmtdev);
>> + HIDMA_SHOW(mgmtdev, sw_version);
>> +
>> + val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET);
>> + seq_printf(s, "ENABLE=%d\n", val & 0x1);
>> +
>> + val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET);
>> + seq_printf(s, "reset_timeout=%d\n", val & CHRESET_TIMEOUUT_MASK);
>> +
>> + val = readl(mgmtdev->dev_virtaddr + MHICFG_OFFSET);
>> + seq_printf(s, "nr_event_channel=%d\n",
>> + (val >> NR_EV_CHANNEL_BIT_POS) & NR_CHANNEL_MASK);
>> + seq_printf(s, "nr_tr_channel=%d\n", (val & NR_CHANNEL_MASK));
>> + seq_printf(s, "nr_virt_tr_channel=%d\n", mgmtdev->nr_channels);
>> + seq_printf(s, "dev_virtaddr=%p\n", &mgmtdev->dev_virtaddr);
>> + seq_printf(s, "dev_physaddr=%pap\n", &mgmtdev->dev_physaddr);
>> + seq_printf(s, "dev_addrsize=%pap\n", &mgmtdev->dev_addrsize);
>
> printing pointers is a security risk, just remove them if they are
> not essential here.
ok
>> +
>> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file)
>> +{
>> + return single_open(file, qcom_hidma_mgmt_err, inode->i_private);
>> +}
>> +
>> +static const struct file_operations qcom_hidma_mgmt_err_fops = {
>> + .open = qcom_hidma_mgmt_err_open,
>> + .read = seq_read,
>> + .llseek = seq_lseek,
>> + .release = single_release,
>> +};
>> +
>> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file,
>> + const char __user *user_buf, size_t count, loff_t *ppos)
>> +{
>> + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
>> +
>> + HIDMA_RUNTIME_GET(mgmtdev);
>> + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET);
>> + HIDMA_RUNTIME_SET(mgmtdev);
>> + return count;
>> +}
>> +
>> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = {
>> + .write = qcom_hidma_mgmt_mhiderr_clr,
>> +};
>
> Is this really just a debugging interface? If anyone would do this
> for normal operation, it needs to be a proper API.
>
This will be used by the system admin to monitor/reset the execution
state of the DMA channels. This will be the management interface.
Debugfs is probably not the right choice. I originally had sysfs but
than had some doubts. I'm open to suggestions.
>> +
>> +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> + debugfs_remove(mgmtdev->evt_ena);
>> + debugfs_remove(mgmtdev->tre_errclr);
>> + debugfs_remove(mgmtdev->msi_errclr);
>> + debugfs_remove(mgmtdev->ode_errclr);
>> + debugfs_remove(mgmtdev->ide_errclr);
>> + debugfs_remove(mgmtdev->evt_errclr);
>> + debugfs_remove(mgmtdev->mhid_errclr);
>> + debugfs_remove(mgmtdev->err);
>> + debugfs_remove(mgmtdev->info);
>> + debugfs_remove(mgmtdev->debugfs);
>> +}
>>
> I guess debugfs_remove_recursive() would do the job as well.
ok
>
>> +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> + int rc = 0;
>> +
>> + mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev),
>> + NULL);
>> + if (!mgmtdev->debugfs) {
>> + rc = -ENODEV;
>> + return rc;
>> + }
>> +
>> + mgmtdev->info = debugfs_create_file("info", S_IRUGO,
>> + mgmtdev->debugfs, mgmtdev, &qcom_hidma_mgmt_fops);
>> + if (!mgmtdev->info) {
>> + rc = -ENOMEM;
>> + goto cleanup;
>> + }
>> +
>> + mgmtdev->err = debugfs_create_file("err", S_IRUGO,
>> + mgmtdev->debugfs, mgmtdev,
>> + &qcom_hidma_mgmt_err_fops);
>> + if (!mgmtdev->err) {
>> + rc = -ENOMEM;
>> + goto cleanup;
>> + }
>> +
>> + mgmtdev->mhid_errclr = debugfs_create_file("mhiderrclr", S_IWUSR,
>> + mgmtdev->debugfs, mgmtdev,
>> + &qcom_hidma_mgmt_mhiderr_clrfops);
>> + if (!mgmtdev->mhid_errclr) {
>> + rc = -ENOMEM;
>> + goto cleanup;
>> + }
>
> Maybe use a loop around an array here to avoid writing the same thing 10 times?
>
will do
> Also, you could move the debugging code into a separate file and have a separate
> Kconfig symbol so users can disable this if they do not debug your driver
> but still want to use debugfs for other things.
>
I need these to be accessible all the time, maybe via sysfs or ioctl.
>> +static irqreturn_t qcom_hidma_mgmt_irq_handler(int irq, void *arg)
>> +{
>> + /* TODO: handle irq here */
>> + return IRQ_HANDLED;
>> +}
>
>
>> + rc = devm_request_irq(&pdev->dev, irq, qcom_hidma_mgmt_irq_handler,
>> + IRQF_SHARED, "qcom-hidmamgmt", mgmtdev);
>> + if (rc) {
>> + dev_err(&pdev->dev, "irq registration failed: %d\n", irq);
>> + goto out;
>> + }
>
> If you create a shared handler, you must check whether there was an
> interrupt pending, otherwise you will lose interrupts for all other devices
> that share the same IRQ.
ok, I'll remove it for now. The interrupt handler is a to-do.
>
> Better remove the handler completely here so you don't need to check it.
>
>> +
>> + if (device_property_read_u16(&pdev->dev, "nr-channels",
>> + &mgmtdev->nr_channels)) {
>> + dev_err(&pdev->dev, "number of channels missing\n");
>> + rc = -EINVAL;
>> + goto out;
>> + }
>
> This should be a u32 name 'dma-channels' for consistency with the
> generic DMA binding.
>
ok
> Also, try to avoid using u8 and u16 properties everywhere and just
> us u32 for consistency.
will do
>
>> +
>> + for (i = 0; i < mgmtdev->nr_channels; i++) {
>> + char name[30];
>> +
>> + sprintf(name, "ch-priority-%d", i);
>> + device_property_read_u8(&pdev->dev, name,
>> + &mgmtdev->priority[i]);
>> +
>> + sprintf(name, "ch-weight-%d", i);
>> + device_property_read_u8(&pdev->dev, name,
>> + &mgmtdev->weight[i]);
>
> Per comment above, just read this as an array.
will look. While device tree syntax for arrays is easy, describing an
array in DSM package looks really ugly. I tried to keep things simple.
I'll spend some time on it.
>
>> + dev_info(&pdev->dev,
>> + "HI-DMA engine management driver registration complete\n");
>> + platform_set_drvdata(pdev, mgmtdev);
>> + HIDMA_RUNTIME_SET(mgmtdev);
>> + return 0;
>> +out:
>> + pm_runtime_disable(&pdev->dev);
>> + pm_runtime_put_sync_suspend(&pdev->dev);
>> + return rc;
>> +}
>
> The rest of the probe function does not register any user interface aside from
> the debugging stuff. Can you explain in the changelog how you expect the
> driver to be used in a real system? Is there another driver coming?
I expect this driver to grow in functionality over time. Right now, it
does the global init for the DMA. After that all channels execute on
their own without depending on each other. Global init has to be done
first before attempting to do any channel initialization.
There is also implied startup ordering requirements. I was doing this by
using channel driver with the late binding to guarantee that.
As soon as I use module_platform_driver, the ordering gets reversed for
some reason.
>
>> +
>> +static const struct of_device_id qcom_hidma_mgmt_match[] = {
>> + { .compatible = "qcom,hidma_mgmt", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_hidma_mgmt_match);
>> +
>> +static struct platform_driver qcom_hidma_mgmt_driver = {
>> + .probe = qcom_hidma_mgmt_probe,
>> + .remove = qcom_hidma_mgmt_remove,
>> + .driver = {
>> + .name = MODULE_NAME,
>> + .owner = THIS_MODULE,
>> + .of_match_table = of_match_ptr(qcom_hidma_mgmt_match),
>
> drop the .owner field and the of_match_ptr() macro to avoid warnings here.
>
>> +static int __init qcom_hidma_mgmt_init(void)
>> +{
>> + return platform_driver_register(&qcom_hidma_mgmt_driver);
>> +}
>> +device_initcall(qcom_hidma_mgmt_init);
>> +
>> +static void __exit qcom_hidma_mgmt_exit(void)
>> +{
>> + platform_driver_unregister(&qcom_hidma_mgmt_driver);
>> +}
>> +module_exit(qcom_hidma_mgmt_exit);
>
> module_platform_driver()
>
> Arnd
>
thanks for the feedback.
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Timur Tabi <timur@codeaurora.org> |
|---|---|
| Date | 2015-10-31 14:00 +0100 |
| Subject | Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver |
| Message-ID | <qpDVh-2Vz-23@gated-at.bofh.it> |
| In reply to | #1259946 |
Sinan Kaya wrote: > > I expect this driver to grow in functionality over time. Right now, it > does the global init for the DMA. After that all channels execute on > their own without depending on each other. Global init has to be done > first before attempting to do any channel initialization. > > There is also implied startup ordering requirements. I was doing this by > using channel driver with the late binding to guarantee that. > > As soon as I use module_platform_driver, the ordering gets reversed for > some reason. If you want to force two probe functions to be called in order, then that's what -EPROBE_DEFER is for. -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Timur Tabi <timur@codeaurora.org> |
|---|---|
| Date | 2015-10-31 14:00 +0100 |
| Subject | Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver |
| Message-ID | <qpDVg-2Vz-19@gated-at.bofh.it> |
| In reply to | #1259946 |
Sinan Kaya wrote: > > I expect this driver to grow in functionality over time. Right now, it > does the global init for the DMA. After that all channels execute on > their own without depending on each other. Global init has to be done > first before attempting to do any channel initialization. > > There is also implied startup ordering requirements. I was doing this by > using channel driver with the late binding to guarantee that. > > As soon as I use module_platform_driver, the ordering gets reversed for > some reason. -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2015-11-02 22:40 +0100 |
| Message-ID | <qquZB-250-35@gated-at.bofh.it> |
| In reply to | #1259946 |
On Saturday 31 October 2015 02:51:46 Sinan Kaya wrote:
> On 10/30/2015 5:34 AM, Arnd Bergmann wrote:
> > On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote:
> >> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
> >> +
> >> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file)
> >> +{
> >> + return single_open(file, qcom_hidma_mgmt_err, inode->i_private);
> >> +}
> >> +
> >> +static const struct file_operations qcom_hidma_mgmt_err_fops = {
> >> + .open = qcom_hidma_mgmt_err_open,
> >> + .read = seq_read,
> >> + .llseek = seq_lseek,
> >> + .release = single_release,
> >> +};
> >> +
> >> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file,
> >> + const char __user *user_buf, size_t count, loff_t *ppos)
> >> +{
> >> + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
> >> +
> >> + HIDMA_RUNTIME_GET(mgmtdev);
> >> + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET);
> >> + HIDMA_RUNTIME_SET(mgmtdev);
> >> + return count;
> >> +}
> >> +
> >> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = {
> >> + .write = qcom_hidma_mgmt_mhiderr_clr,
> >> +};
> >
> > Is this really just a debugging interface? If anyone would do this
> > for normal operation, it needs to be a proper API.
> >
> This will be used by the system admin to monitor/reset the execution
> state of the DMA channels. This will be the management interface.
> Debugfs is probably not the right choice. I originally had sysfs but
> than had some doubts. I'm open to suggestions.
User interface design is unfortunately always hard, and I don't have
an obvious answer for you.
Using debugfs by definition means that you don't expect users to
rely on ABI stability, so they should not write any automated scripts
against the contents of the files.
With sysfs, the opposite is true: you need to maintain compatibility
for as long as anyone might rely on the current interface, and it
needs to be reviewed properly and documented in Documentation/ABI/.
Other options are to use ioctl(), netlink or your own virtual file
system, but each of them has the same ABI requirements as sysfs.
Regardless of what you pick, you also need to consider how other drivers
would use the same interface: If someone else has hardware that does
the same thing, we want to be able to use the same tools to access
it, so you should avoid having any hardware specific data in it and
keep it as generic and extensible as possible. In this particular
case, that probably means you should implement the user interfaces in
the dmaengine core driver, and let the specific DMA driver provide
callback function pointers along with the normal ones to fill that
data.
> >> + dev_info(&pdev->dev,
> >> + "HI-DMA engine management driver registration complete\n");
> >> + platform_set_drvdata(pdev, mgmtdev);
> >> + HIDMA_RUNTIME_SET(mgmtdev);
> >> + return 0;
> >> +out:
> >> + pm_runtime_disable(&pdev->dev);
> >> + pm_runtime_put_sync_suspend(&pdev->dev);
> >> + return rc;
> >> +}
> >
> > The rest of the probe function does not register any user interface aside from
> > the debugging stuff. Can you explain in the changelog how you expect the
> > driver to be used in a real system? Is there another driver coming?
>
> I expect this driver to grow in functionality over time. Right now, it
> does the global init for the DMA. After that all channels execute on
> their own without depending on each other. Global init has to be done
> first before attempting to do any channel initialization.
>
> There is also implied startup ordering requirements. I was doing this by
> using channel driver with the late binding to guarantee that.
>
> As soon as I use module_platform_driver, the ordering gets reversed for
> some reason.
For the ordering requirements, it's probably best to export a symbol
with the entry point and let the normal driver call into that. Using
separate initcall levels is not something you should do in a normal
device driver like this.
What is the relation between the device nodes for the two kinds of
devices? Does it make sense to model the other one as a child device
of this one? That way you would trivially do the ordering by not marking
this one as 'compatible="simple-bus"' and triggering the registration
of the child from the parent probe function.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2015-11-03 05:50 +0100 |
| Subject | Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver |
| Message-ID | <qqBHH-6kT-1@gated-at.bofh.it> |
| In reply to | #1261000 |
On 11/2/2015 4:30 PM, Arnd Bergmann wrote:
> On Saturday 31 October 2015 02:51:46 Sinan Kaya wrote:
>> On 10/30/2015 5:34 AM, Arnd Bergmann wrote:
>>> On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote:
>>>> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
>>>> +
>>>> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file)
>>>> +{
>>>> + return single_open(file, qcom_hidma_mgmt_err, inode->i_private);
>>>> +}
>>>> +
>>>> +static const struct file_operations qcom_hidma_mgmt_err_fops = {
>>>> + .open = qcom_hidma_mgmt_err_open,
>>>> + .read = seq_read,
>>>> + .llseek = seq_lseek,
>>>> + .release = single_release,
>>>> +};
>>>> +
>>>> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file,
>>>> + const char __user *user_buf, size_t count, loff_t *ppos)
>>>> +{
>>>> + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
>>>> +
>>>> + HIDMA_RUNTIME_GET(mgmtdev);
>>>> + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET);
>>>> + HIDMA_RUNTIME_SET(mgmtdev);
>>>> + return count;
>>>> +}
>>>> +
>>>> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = {
>>>> + .write = qcom_hidma_mgmt_mhiderr_clr,
>>>> +};
>>>
>>> Is this really just a debugging interface? If anyone would do this
>>> for normal operation, it needs to be a proper API.
>>>
>> This will be used by the system admin to monitor/reset the execution
>> state of the DMA channels. This will be the management interface.
>> Debugfs is probably not the right choice. I originally had sysfs but
>> than had some doubts. I'm open to suggestions.
>
> User interface design is unfortunately always hard, and I don't have
> an obvious answer for you.
>
> Using debugfs by definition means that you don't expect users to
> rely on ABI stability, so they should not write any automated scripts
> against the contents of the files.
>
> With sysfs, the opposite is true: you need to maintain compatibility
> for as long as anyone might rely on the current interface, and it
> needs to be reviewed properly and documented in Documentation/ABI/.
>
> Other options are to use ioctl(), netlink or your own virtual file
> system, but each of them has the same ABI requirements as sysfs.
>
> Regardless of what you pick, you also need to consider how other drivers
> would use the same interface: If someone else has hardware that does
> the same thing, we want to be able to use the same tools to access
> it, so you should avoid having any hardware specific data in it and
> keep it as generic and extensible as possible. In this particular
> case, that probably means you should implement the user interfaces in
> the dmaengine core driver, and let the specific DMA driver provide
> callback function pointers along with the normal ones to fill that
> data.
>
Thanks, I'll think about this. I'm inclined towards sysfs.
>>>> + dev_info(&pdev->dev,
>>>> + "HI-DMA engine management driver registration complete\n");
>>>> + platform_set_drvdata(pdev, mgmtdev);
>>>> + HIDMA_RUNTIME_SET(mgmtdev);
>>>> + return 0;
>>>> +out:
>>>> + pm_runtime_disable(&pdev->dev);
>>>> + pm_runtime_put_sync_suspend(&pdev->dev);
>>>> + return rc;
>>>> +}
>>>
>>> The rest of the probe function does not register any user interface aside from
>>> the debugging stuff. Can you explain in the changelog how you expect the
>>> driver to be used in a real system? Is there another driver coming?
>>
>> I expect this driver to grow in functionality over time. Right now, it
>> does the global init for the DMA. After that all channels execute on
>> their own without depending on each other. Global init has to be done
>> first before attempting to do any channel initialization.
>>
>> There is also implied startup ordering requirements. I was doing this by
>> using channel driver with the late binding to guarantee that.
>>
>> As soon as I use module_platform_driver, the ordering gets reversed for
>> some reason.
>
> For the ordering requirements, it's probably best to export a symbol
> with the entry point and let the normal driver call into that. Using
> separate initcall levels is not something you should do in a normal
> device driver like this.
>
I figured this out. If the channel driver starts before the management
driver; then channel reset fails. I'm handling this in the channel
driver and am returning -EPROBE_DEFER. After that, management driver
gets its chance to work. Then, the channel driver again. This change is
in the v2 series.
> What is the relation between the device nodes for the two kinds of
> devices? Does it make sense to model the other one as a child device
> of this one? That way you would trivially do the ordering by not marking
> this one as 'compatible="simple-bus"' and triggering the registration
> of the child from the parent probe function.
>
The required order is management driver first, channel drivers next. If
the order is reversed, channel init fails. I handle this with deferred
probing.
I tried to keep loose binding between the management driver due to QEMU.
QEMU auto-generates the devicetree entries. The guest machine just sees
one devicetree object for the DMA channel but guest machine device-tree
kernel does not have any management driver entity.
This requires DMA channel driver to work independently in the guest
machine without dependencies.
> Arnd
>
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2015-11-03 13:50 +0100 |
| Message-ID | <qqJcf-2KO-17@gated-at.bofh.it> |
| In reply to | #1261194 |
On Monday 02 November 2015 23:45:17 Sinan Kaya wrote: > On 11/2/2015 4:30 PM, Arnd Bergmann wrote: > > On Saturday 31 October 2015 02:51:46 Sinan Kaya wrote: > >> On 10/30/2015 5:34 AM, Arnd Bergmann wrote: > >>> On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote: > >> This will be used by the system admin to monitor/reset the execution > >> state of the DMA channels. This will be the management interface. > >> Debugfs is probably not the right choice. I originally had sysfs but > >> than had some doubts. I'm open to suggestions. > > > > User interface design is unfortunately always hard, and I don't have > > an obvious answer for you. > > > > Using debugfs by definition means that you don't expect users to > > rely on ABI stability, so they should not write any automated scripts > > against the contents of the files. > > > > With sysfs, the opposite is true: you need to maintain compatibility > > for as long as anyone might rely on the current interface, and it > > needs to be reviewed properly and documented in Documentation/ABI/. > > > > Other options are to use ioctl(), netlink or your own virtual file > > system, but each of them has the same ABI requirements as sysfs. > > > > Regardless of what you pick, you also need to consider how other drivers > > would use the same interface: If someone else has hardware that does > > the same thing, we want to be able to use the same tools to access > > it, so you should avoid having any hardware specific data in it and > > keep it as generic and extensible as possible. In this particular > > case, that probably means you should implement the user interfaces in > > the dmaengine core driver, and let the specific DMA driver provide > > callback function pointers along with the normal ones to fill that > > data. > > > Thanks, I'll think about this. I'm inclined towards sysfs. Ok. Documentation/sysfs-rules.txt has a good introduction of how this is done. > >>> The rest of the probe function does not register any user interface aside from > >>> the debugging stuff. Can you explain in the changelog how you expect the > >>> driver to be used in a real system? Is there another driver coming? > >> > >> I expect this driver to grow in functionality over time. Right now, it > >> does the global init for the DMA. After that all channels execute on > >> their own without depending on each other. Global init has to be done > >> first before attempting to do any channel initialization. > >> > >> There is also implied startup ordering requirements. I was doing this by > >> using channel driver with the late binding to guarantee that. > >> > >> As soon as I use module_platform_driver, the ordering gets reversed for > >> some reason. > > > > For the ordering requirements, it's probably best to export a symbol > > with the entry point and let the normal driver call into that. Using > > separate initcall levels is not something you should do in a normal > > device driver like this. > > > I figured this out. If the channel driver starts before the management > driver; then channel reset fails. I'm handling this in the channel > driver and am returning -EPROBE_DEFER. After that, management driver > gets its chance to work. Then, the channel driver again. This change is > in the v2 series. If you change the order in the Makefile, the management driver should always get probed first if both are built-in. When the driver is a loadable module, the ordering should work because of the way that the modules are loaded. Using the deferred probing makes sense here, so that would just be an optimization to avoid it normally. Things can still get shuffled around e.g. if the management device is deferred itself and we end up probing the channel driver first. > > What is the relation between the device nodes for the two kinds of > > devices? Does it make sense to model the other one as a child device > > of this one? That way you would trivially do the ordering by not marking > > this one as 'compatible="simple-bus"' and triggering the registration > > of the child from the parent probe function. > > > > The required order is management driver first, channel drivers next. If > the order is reversed, channel init fails. I handle this with deferred > probing. > > I tried to keep loose binding between the management driver due to QEMU. > > QEMU auto-generates the devicetree entries. The guest machine just sees > one devicetree object for the DMA channel but guest machine device-tree > kernel does not have any management driver entity. > > This requires DMA channel driver to work independently in the guest > machine without dependencies. You have a distinct "compatible" string for qemu, right? It sounds like this is not the same device if the dependencies are different, and you could just have two ways to probe the same device. The split between the two drivers still feels a little awkward overall, it might be good to give it some more thought. Would it work to describe the combination of the channel and management registers as a single device with a single driver, but the management parts being optional? That way, the management registers could be intergrated better into the dmaengine framework, to provide a consistent API to user space. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Timur Tabi <timur@codeaurora.org> |
|---|---|
| Date | 2015-11-03 15:30 +0100 |
| Subject | Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver |
| Message-ID | <qqKL0-3QT-7@gated-at.bofh.it> |
| In reply to | #1261456 |
Arnd Bergmann wrote: > If you change the order in the Makefile, the management driver should > always get probed first if both are built-in. When the driver is a > loadable module, the ordering should work because of the way that the > modules are loaded. This sounds like something that should be commented in the Makefile. Even if you use -EPROBE_DEFER and you re-order the Makefile only so that it's less likely to be a problem, that's still worthy of a comment. -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2015-11-04 02:10 +0100 |
| Subject | Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver |
| Message-ID | <qqUKl-20V-15@gated-at.bofh.it> |
| In reply to | #1261456 |
On 11/3/2015 7:42 AM, Arnd Bergmann wrote: > You have a distinct "compatible" string for qemu, right? It sounds like > this is not the same device if the dependencies are different, and > you could just have two ways to probe the same device. > No, it is the same dma channel object that gets probed by the same name like the hypervisor. The channel object gets unbound from the hypervisor, it then gets bound to VFIO. Then, eventually QEMU takes over. The channel driver does not know under which OS it is running and it works in both environments as it is without any code changes at this moment. > The split between the two drivers still feels a little awkward overall, > it might be good to give it some more thought. I see. I'd like to keep the management driver as independent as possible from the channel driver for security and functionality reasons. I need to keep the management addresses and functionality in the hypervisor only. > > Would it work to describe the combination of the channel and management > registers as a single device with a single driver, but the management > parts being optional? That way, the management registers could be > intergrated better into the dmaengine framework, to provide a consistent > API to user space. I can compile both management driver and channel driver into the same module if it sounds better. I can probe one with channel and another with the management name. I just need to be careful about not sharing any kind of data structure between them otherwise virtualization will break. I consider the management driver a client of the DMA engine API at this moment. -- Sinan Kaya Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2015-10-30 11:30 +0100 |
| Subject | Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver |
| Message-ID | <qpf6y-4BH-35@gated-at.bofh.it> |
| In reply to | #1259183 |
On Thursday 29 October 2015 23:08:13 Sinan Kaya wrote:
> This patch adds support for hidma engine. The driver
> consists of two logical blocks. The DMA engine interface
> and the low-level interface. This version of the driver
> does not support virtualization on this release and only
> memcpy interface support is included.
Does that mean you can have slave device support eventually?
If so, please put that into the binding document already.
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
> .../devicetree/bindings/dma/qcom_hidma.txt | 18 +
> drivers/dma/Kconfig | 11 +
> drivers/dma/Makefile | 4 +
> drivers/dma/qcom_hidma.c | 1717 ++++++++++++++++++++
> drivers/dma/qcom_hidma.h | 44 +
> drivers/dma/qcom_hidma_ll.c | 1132 +++++++++++++
> 6 files changed, 2926 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma.txt
> create mode 100644 drivers/dma/qcom_hidma.c
> create mode 100644 drivers/dma/qcom_hidma.h
> create mode 100644 drivers/dma/qcom_hidma_ll.c
>
> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma.txt b/Documentation/devicetree/bindings/dma/qcom_hidma.txt
> new file mode 100644
> index 0000000..9a01635
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma.txt
> @@ -0,0 +1,18 @@
> +Qualcomm Technologies HIDMA Channel driver
> +
> +Required properties:
> +- compatible: must contain "qcom,hidma"
Be more specific here. Also, should this be 'hisilicon,hidma-1.0' rather
than 'qcom'? I'm guessing from the name that this is a device you licensed
from them.
> +- reg: Addresses for the transfer and event channel
> +- interrupts: Should contain the event interrupt
> +- desc-count: Number of asynchronous requests this channel can handle
> +- event-channel: The HW event channel completions will be delivered.
> +Example:
> +
> + hidma_24: hidma@0x5c050000 {
Name should be 'dma-controller' in DT, not 'hidma'.
> + compatible = "qcom,hidma";
> + reg = <0 0x5c050000 0x0 0x1000>,
> + <0 0x5c0b0000 0x0 0x1000>;
> + interrupts = <0 389 0>;
> + desc-count = <10>;
> + event-channel = /bits/ 8 <4>;
> + };
Remove the /bits/.
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 76a5a5e..2645185 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -512,6 +512,17 @@ config QCOM_HIDMA_MGMT
> OS would run QCOM_HIDMA driver and the hypervisor would run
> the QCOM_HIDMA_MGMT driver.
>
> +config QCOM_HIDMA
> + tristate "Qualcomm Technologies HIDMA support"
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Enable support for the Qualcomm Technologies HIDMA controller.
> + The HIDMA controller supports optimized buffer copies
> + (user to kernel, kernel to kernel, etc.). It only supports
> + memcpy/memset interfaces. The core is not intended for general
> + purpose slave DMA.
> +
> config XILINX_VDMA
> tristate "Xilinx AXI VDMA Engine"
> depends on (ARCH_ZYNQ || MICROBLAZE)
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 3d25ffd..5665df2 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -53,6 +53,10 @@ obj-$(CONFIG_PL330_DMA) += pl330.o
> obj-$(CONFIG_PPC_BESTCOMM) += bestcomm/
> obj-$(CONFIG_PXA_DMA) += pxa_dma.o
> obj-$(CONFIG_QCOM_BAM_DMA) += qcom_bam_dma.o
> +obj-$(CONFIG_QCOM_HIDMA) += qcom_hdma.o
> +qcom_hdma-objs := qcom_hidma_ll.o qcom_hidma.o
> +
The driver is linked into a single module, so all the EXPORT_SYMBOL
statements can be dropped.
> +/* Default idle time is 2 seconds. This parameter can
> + * be overridden by changing the following
> + * /sys/bus/platform/devices/QCOM8061:<xy>/power/autosuspend_delay_ms
> + * during kernel boot.
> + */
> +#define AUTOSUSPEND_TIMEOUT 2000
> +#define HIDMA_DEFAULT_DESCRIPTOR_COUNT 16
> +#define MODULE_NAME "hidma"
MODULE_NAME and HIDMA_DEFAULT_DESCRIPTOR_COUNT can be dropped
> +#define HIDMA_RUNTIME_GET(dmadev) \
> +do { \
> + atomic_inc(&(dmadev)->pm_counter); \
> + TRC_PM((dmadev)->ddev.dev, \
> + "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\
> + atomic_read(&(dmadev)->pm_counter)); \
> + pm_runtime_get_sync((dmadev)->ddev.dev); \
> +} while (0)
> +
> +#define HIDMA_RUNTIME_SET(dmadev) \
> +do { \
> + atomic_dec(&(dmadev)->pm_counter); \
> + TRC_PM((dmadev)->ddev.dev, \
> + "%s:%d pm_runtime_put_autosuspend:%d\n", \
> + __func__, __LINE__, \
> + atomic_read(&(dmadev)->pm_counter)); \
> + pm_runtime_mark_last_busy((dmadev)->ddev.dev); \
> + pm_runtime_put_autosuspend((dmadev)->ddev.dev); \
> +} while (0)
Inline functions.
> +struct hidma_test_sync {
> + atomic_t counter;
> + wait_queue_head_t wq;
> +};
Let me guess, you converted this from a semaphore? ;-)
Just put the two members into the containing structure and relete it here.
> +struct hidma_dev {
> + u8 evridx;
> + u32 nr_descriptors;
> +
> + void *lldev;
> + void __iomem *dev_trca;
> + void __iomem *dev_evca;
> + int (*self_test)(struct hidma_dev *device);
> + struct dentry *debugfs;
> + struct dentry *stats;
> +
> + /* used to protect the pending channel list*/
> + spinlock_t lock;
> + dma_addr_t dev_trca_phys;
> + struct dma_device ddev;
> + struct tasklet_struct tasklet;
workqueue maybe?
> + resource_size_t dev_trca_size;
> + dma_addr_t dev_evca_phys;
> + resource_size_t dev_evca_size;
All three look unused and can be removed.
> +static unsigned int debug_pm;
> +module_param(debug_pm, uint, 0644);
> +MODULE_PARM_DESC(debug_pm,
> + "debug runtime power management transitions (default: 0)");
> +
> +#define TRC_PM(...) do { \
> + if (debug_pm) \
> + dev_info(__VA_ARGS__); \
> + } while (0)
Again, remove these after you are done debugging the problem at hand,
we don't need to clutter up the upstream version.
> + /*
> + * It is assumed that the hardware can move the data within 1s
> + * and signal the OS of the completion
> + */
> + ret = wait_event_interruptible_timeout(dmadev->test_result.wq,
> + atomic_read(&dmadev->test_result.counter) == (map_count),
> + msecs_to_jiffies(10000));
> +
> + if (ret <= 0) {
> + dev_err(dmadev->ddev.dev,
> + "Self-test sg copy timed out, disabling\n");
> + err = -ENODEV;
> + goto tx_status;
> + }
Why ENODEV? Could you make this handle restarted system calls?
> +
> +/*
> + * Perform a streaming transaction to verify the HW works.
> + */
> +static int hidma_selftest_streaming(struct hidma_dev *dmadev,
> + struct dma_chan *dma_chanptr, u64 size,
> + unsigned long flags)
> +{
You have a lot of selftest code here. Can you try to move that into a file
that works for all DMA engines? It feels like this should not be part of
a driver.
> +
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +
> +#define SIER_CHAN_SHOW(chan, name) \
> + seq_printf(s, #name "=%u\n", chan->name)
can be open-coded for clarity.
> + ret = dma_mapping_error(dev, dma_src);
> + if (ret) {
> + dev_err(dev, "dma_mapping_error with ret:%d\n", ret);
> + ret = -ENOMEM;
> + } else {
> + phys_addr_t phys;
> +
> + phys = dma_to_phys(dev, dma_src);
> + if (strcmp(__va(phys), "hello world") != 0) {
> + dev_err(dev, "memory content mismatch\n");
> + ret = -EINVAL;
> + } else {
> + dev_dbg(dev, "mapsingle:dma_map_single works\n");
> + }
> + dma_unmap_single(dev, dma_src, buf_size, DMA_TO_DEVICE);
> + }
dma_to_phys() is architecture specific and does not work if you
have an IOMMU. Just use the virtual address you passed into dma_map_*.
> +/**
> + * hidma_dma_info: display HIDMA device info
> + *
> + * Display the info for the current HIDMA device.
> + */
> +static int hidma_dma_info(struct seq_file *s, void *unused)
> +{
> + struct hidma_dev *dmadev = s->private;
> + struct dma_device *dma = &dmadev->ddev;
> +
> + seq_printf(s, "nr_descriptors=%d\n", dmadev->nr_descriptors);
> + seq_printf(s, "dev_trca=%p\n", &dmadev->dev_trca);
> + seq_printf(s, "dev_trca_phys=%pa\n", &dmadev->dev_trca_phys);
> + seq_printf(s, "dev_trca_size=%pa\n", &dmadev->dev_trca_size);
> + seq_printf(s, "dev_evca=%p\n", &dmadev->dev_evca);
> + seq_printf(s, "dev_evca_phys=%pa\n", &dmadev->dev_evca_phys);
> + seq_printf(s, "dev_evca_size=%pa\n", &dmadev->dev_evca_size);
Don't print pointers here.
> + seq_printf(s, "self_test=%u\n",
> + atomic_read(&dmadev->test_result.counter));
> +
> + seq_printf(s, "copy%s%s%s%s%s%s%s%s%s%s%s\n",
> + dma_has_cap(DMA_PQ, dma->cap_mask) ? " pq" : "",
> + dma_has_cap(DMA_PQ_VAL, dma->cap_mask) ? " pq_val" : "",
> + dma_has_cap(DMA_XOR, dma->cap_mask) ? " xor" : "",
> + dma_has_cap(DMA_XOR_VAL, dma->cap_mask) ? " xor_val" : "",
> + dma_has_cap(DMA_INTERRUPT, dma->cap_mask) ? " intr" : "",
> + dma_has_cap(DMA_SG, dma->cap_mask) ? " sg" : "",
> + dma_has_cap(DMA_ASYNC_TX, dma->cap_mask) ? " async" : "",
> + dma_has_cap(DMA_SLAVE, dma->cap_mask) ? " slave" : "",
> + dma_has_cap(DMA_CYCLIC, dma->cap_mask) ? " cyclic" : "",
> + dma_has_cap(DMA_INTERLEAVE, dma->cap_mask) ? " intl" : "",
> + dma_has_cap(DMA_MEMCPY, dma->cap_mask) ? " memcpy" : "");
> +
> + return 0;
> +}
The selftest part and the features could just be separate files in the
core dmaengine code, it doesn't really belong here.
> +}
> +#else
> +static void hidma_debug_uninit(struct hidma_dev *dmadev)
> +{
> +}
> +static int hidma_debug_init(struct hidma_dev *dmadev)
> +{
> + return 0;
> +}
> +#endif
You can remove the #ifdef here. The debugfs code already stubs out all
debugfs_create_*() and turns it all into nops when it is disabled.
> + dma_cap_set(DMA_MEMCPY, dmadev->ddev.cap_mask);
> + /* Apply default dma_mask if needed */
> + if (!pdev->dev.dma_mask) {
> + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> + }
> +
remove the check, or use
if (WARN_ON(!pdev->dev.dma_mask))
return -ENXIO;
The dma mask has to always be set by the platform code before probe()
is called. If it is not set, you are not allowed to perform DMA.
> + dmadev->dev_evca_phys = evca_resource->start;
> + dmadev->dev_evca_size = resource_size(evca_resource);
> +
> + dev_dbg(&pdev->dev, "dev_evca_phys:%pa\n", &dmadev->dev_evca_phys);
> + dev_dbg(&pdev->dev, "dev_evca_size:%pa\n", &dmadev->dev_evca_size);
> +
> + dev_dbg(&pdev->dev, "qcom_hidma: mapped EVCA %pa to %p\n",
> + &dmadev->dev_evca_phys, dmadev->dev_evca);
> + dmadev->dev_trca_phys = trca_resource->start;
> + dmadev->dev_trca_size = resource_size(trca_resource);
> +
> + dev_dbg(&pdev->dev, "dev_trca_phys:%pa\n", &dmadev->dev_trca_phys);
> + dev_dbg(&pdev->dev, "dev_trca_size:%pa\n", &dmadev->dev_trca_size);
Don't print pointers.
> + rc = devm_request_irq(&pdev->dev, chirq, hidma_chirq_handler, 0,
> + "qcom-hidma", &dmadev->lldev);
> + if (rc) {
> + dev_err(&pdev->dev, "chirq registration failed: %d\n", chirq);
> + goto chirq_request_failed;
> + }
> +
> + dev_dbg(&pdev->dev, "initializing DMA channels\n");
> + INIT_LIST_HEAD(&dmadev->ddev.channels);
> + rc = hidma_chan_init(dmadev, 0);
> + if (rc) {
> + dev_err(&pdev->dev, "probe:channel init failed\n");
> + goto channel_init_failed;
> + }
> + dev_dbg(&pdev->dev, "HI-DMA engine driver starting self test\n");
> + rc = dmadev->self_test(dmadev);
> + if (rc) {
> + dev_err(&pdev->dev, "probe: self test failed: %d\n", rc);
> + goto self_test_failed;
> + }
> + dev_info(&pdev->dev, "probe: self test succeeded.\n");
> +
> + dev_dbg(&pdev->dev, "calling dma_async_device_register\n");
> + rc = dma_async_device_register(&dmadev->ddev);
> + if (rc) {
> + dev_err(&pdev->dev,
> + "probe: failed to register slave DMA: %d\n", rc);
> + goto device_register_failed;
> + }
> + dev_dbg(&pdev->dev, "probe: dma_async_device_register done\n");
> +
> + rc = hidma_debug_init(dmadev);
> + if (rc) {
> + dev_err(&pdev->dev,
> + "probe: failed to init debugfs: %d\n", rc);
> + goto debug_init_failed;
> + }
> +
> + dev_info(&pdev->dev, "HI-DMA engine driver registration complete\n");
> + platform_set_drvdata(pdev, dmadev);
> + HIDMA_RUNTIME_SET(dmadev);
> + return 0;
Remove the debug prints when you are done debugging.
> +debug_init_failed:
> +device_register_failed:
> +self_test_failed:
> +channel_init_failed:
> +chirq_request_failed:
> + hidma_ll_uninit(dmadev->lldev);
> +ll_init_failed:
> +evridx_failed:
> +remap_trca_failed:
> +remap_evca_failed:
> + if (dmadev)
> + hidma_free(dmadev);
Rename the labels according to what you do at in the failure case
and remove most of them. This is 'goto', not 'comefrom' ;-)
> +static struct platform_driver hidma_driver = {
> + .probe = hidma_probe,
> + .remove = hidma_remove,
> + .driver = {
> + .name = MODULE_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(hidma_match),
> + .acpi_match_table = ACPI_PTR(hidma_acpi_ids),
Remove .owner and of_match_ptr().
> + },
> +};
> +
> +static int __init hidma_init(void)
> +{
> + return platform_driver_register(&hidma_driver);
> +}
> +late_initcall(hidma_init);
> +
> +static void __exit hidma_exit(void)
> +{
> + platform_driver_unregister(&hidma_driver);
> +}
> +module_exit(hidma_exit);
module_platform_driver()
> +
> + if (unlikely(tre_ch >= lldev->nr_tres)) {
> + dev_err(lldev->dev, "invalid TRE number in free:%d", tre_ch);
> + return;
> + }
> +
> + tre = &lldev->trepool[tre_ch];
> + if (unlikely(atomic_read(&tre->allocated) != true)) {
> + dev_err(lldev->dev, "trying to free an unused TRE:%d",
> + tre_ch);
> + return;
> + }
Remove the 'unlikely' and the redundant '!= true'.
Only use 'likely' or 'unlikely' if you can measure a difference.
> +static int hidma_ll_reset(struct hidma_lldev *lldev)
> +{
> + u32 val;
> + int count;
> +
> + val = readl_relaxed(lldev->trca + TRCA_CTRLSTS_OFFSET);
> + val = val & ~(CH_CONTROL_MASK << 16);
> + val = val | (CH_RESET << 16);
> + writel_relaxed(val, lldev->trca + TRCA_CTRLSTS_OFFSET);
> +
> + /* wait until the reset is performed */
> + wmb();
> +
> + /* Delay 10ms after reset to allow DMA logic to quiesce.*/
> + for (count = 0; count < 10; count++) {
> + val = readl_relaxed(lldev->trca + TRCA_CTRLSTS_OFFSET);
> + lldev->trch_state = (val >> CH_STATE_BIT_POS)
> + & CH_STATE_MASK;
> + if (lldev->trch_state == CH_DISABLED)
> + break;
> + mdelay(1);
> + }
> + val = readl_relaxed(lldev->trca + TRCA_CTRLSTS_OFFSET);
> + lldev->trch_state = (val >> CH_STATE_BIT_POS) & CH_STATE_MASK;
> + if (lldev->trch_state != CH_DISABLED) {
> + dev_err(lldev->dev,
> + "transfer channel did not reset\n");
> + return -ENODEV;
> + }
> +
> + val = readl_relaxed(lldev->evca + EVCA_CTRLSTS_OFFSET);
> + val = val & ~(CH_CONTROL_MASK << 16);
> + val = val | (CH_RESET << 16);
> + writel_relaxed(val, lldev->evca + EVCA_CTRLSTS_OFFSET);
> +
> + /* wait until the reset is performed */
> + wmb();
> +
> + /* Delay 10ms after reset to allow DMA logic to quiesce.*/
> + for (count = 0; count < 10; count++) {
> + val = readl_relaxed(lldev->evca + EVCA_CTRLSTS_OFFSET);
> + lldev->evch_state = (val >> CH_STATE_BIT_POS)
> + & CH_STATE_MASK;
> + if (lldev->evch_state == CH_DISABLED)
> + break;
> + mdelay(1);
> + }
Try using a workqueue to get into a state where you can call msleep()
instead of mdelay().
Also, if you waste CPU cycles for hundreds of milliseconds, it's unlikely
that the function is so performance critical that it requires writel_relaxed().
Just use writel() here.
> +/*
> + * The interrupt handler for HIDMA will try to consume as many pending
> + * EVRE from the event queue as possible. Each EVRE has an associated
> + * TRE that holds the user interface parameters. EVRE reports the
> + * result of the transaction. Hardware guarantees ordering between EVREs
> + * and TREs. We use last processed offset to figure out which TRE is
> + * associated with which EVRE. If two TREs are consumed by HW, the EVREs
> + * are in order in the event ring.
> + * This handler will do a one pass for consuming EVREs. Other EVREs may
> + * be delivered while we are working. It will try to consume incoming
> + * EVREs one more time and return.
> + * For unprocessed EVREs, hardware will trigger another interrupt until
> + * all the interrupt bits are cleared.
> + */
> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
> +{
> + u32 status;
> + u32 enable;
> + u32 cause;
> + int repeat = 2;
> + unsigned long timeout;
> +
> + status = readl_relaxed(lldev->evca + EVCA_IRQ_STAT_OFFSET);
> + enable = readl_relaxed(lldev->evca + EVCA_IRQ_EN_OFFSET);
> + cause = status & enable;
> +
Reading the status probably requires a readl() rather than readl_relaxed()
to guarantee that the DMA data has arrived in memory by the time that the
register data is seen by the CPU. If using readl_relaxed() here is a valid
and required optimization, please add a comment to explain why it works
and how much you gain.
> + /* Another interrupt might have arrived while we are
> + * processing this one. Read the new cause.
> + */
> + status = readl_relaxed(lldev->evca + EVCA_IRQ_STAT_OFFSET);
> + enable = readl_relaxed(lldev->evca + EVCA_IRQ_EN_OFFSET);
> + cause = status & enable;
> +
> + repeat--;
> + }
Same here.
> +}
> +
> +
> +static int hidma_ll_enable(struct hidma_lldev *lldev)
> +{
> + u32 val;
> +
> + val = readl_relaxed(lldev->evca + EVCA_CTRLSTS_OFFSET);
> + val &= ~(CH_CONTROL_MASK << 16);
> + val |= (CH_ENABLE << 16);
> +
> + writel_relaxed(val, lldev->evca + EVCA_CTRLSTS_OFFSET);
> +
> + /* wait until channel is enabled */
> + wmb();
> +
> + mdelay(1);
> +
> + val = readl_relaxed(lldev->evca + EVCA_CTRLSTS_OFFSET);
> + lldev->evch_state = (val >> CH_STATE_BIT_POS) & CH_STATE_MASK;
> + if ((lldev->evch_state != CH_ENABLED) &&
> + (lldev->evch_state != CH_RUNNING)) {
> + dev_err(lldev->dev,
> + "event channel did not get enabled\n");
> + return -ENODEV;
> + }
> +
> + val = readl_relaxed(lldev->trca + TRCA_CTRLSTS_OFFSET);
> + val = val & ~(CH_CONTROL_MASK << 16);
> + val = val | (CH_ENABLE << 16);
> + writel_relaxed(val, lldev->trca + TRCA_CTRLSTS_OFFSET);
> +
> + /* wait until channel is enabled */
> + wmb();
> +
> + mdelay(1);
Another workqueue? You should basically never call mdelay().
> +static int hidma_ll_hw_start(void *llhndl)
> +{
> + int rc = 0;
> + struct hidma_lldev *lldev = llhndl;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(&lldev->lock, irqflags);
> + writel_relaxed(lldev->tre_write_offset,
> + lldev->trca + TRCA_DOORBELL_OFFSET);
> + spin_unlock_irqrestore(&lldev->lock, irqflags);
How does this work? The writel_relaxed() won't synchronize with either
the DMA data or the spinlock.
> +int hidma_ll_init(void **lldevp, struct device *dev, u32 nr_tres,
> + void __iomem *trca, void __iomem *evca,
> + u8 evridx)
How about returning the pointer rather than passing in an indirect pointer?
Also, your abstraction seem to go a little too far if the upper driver
doesn't know what the lower driver calls its main device structure.
Or you can go further and just embed the struct hidma_lldev within the
struct hidma_dev to save one?
> +void hidma_ll_chstats(struct seq_file *s, void *llhndl, u32 tre_ch)
> +{
> + struct hidma_lldev *lldev = llhndl;
> + struct hidma_tre *tre;
> + u32 length;
> + dma_addr_t src_start;
> + dma_addr_t dest_start;
> + u32 *tre_local;
> +
> + if (unlikely(tre_ch >= lldev->nr_tres)) {
> + dev_err(lldev->dev, "invalid TRE number in chstats:%d",
> + tre_ch);
> + return;
> + }
> + tre = &lldev->trepool[tre_ch];
> + seq_printf(s, "------Channel %d -----\n", tre_ch);
> + seq_printf(s, "allocated=%d\n", atomic_read(&tre->allocated));
> + HIDMA_CHAN_SHOW(tre, queued);
> + seq_printf(s, "err_info=0x%x\n",
> + lldev->tx_status_list[tre->chidx].err_info);
> + seq_printf(s, "err_code=0x%x\n",
> + lldev->tx_status_list[tre->chidx].err_code);
> + HIDMA_CHAN_SHOW(tre, status);
> + HIDMA_CHAN_SHOW(tre, chidx);
> + HIDMA_CHAN_SHOW(tre, dma_sig);
> + seq_printf(s, "dev_name=%s\n", tre->dev_name);
> + seq_printf(s, "callback=%p\n", tre->callback);
> + seq_printf(s, "data=%p\n", tre->data);
> + HIDMA_CHAN_SHOW(tre, tre_index);
> +
> + tre_local = &tre->tre_local[0];
> + src_start = tre_local[TRE_SRC_LOW_IDX];
> + src_start = ((u64)(tre_local[TRE_SRC_HI_IDX]) << 32) + src_start;
> + dest_start = tre_local[TRE_DEST_LOW_IDX];
> + dest_start += ((u64)(tre_local[TRE_DEST_HI_IDX]) << 32);
> + length = tre_local[TRE_LEN_IDX];
> +
> + seq_printf(s, "src=%pap\n", &src_start);
> + seq_printf(s, "dest=%pap\n", &dest_start);
> + seq_printf(s, "length=0x%x\n", length);
> +}
> +EXPORT_SYMBOL_GPL(hidma_ll_chstats);
Remove all the pointers here. I guess you can remove the entire debugfs
file really ;-)
This looks like it is better done using ftrace for the low-level internals
of the driver.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2015-10-30 22:50 +0100 |
| Subject | Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver |
| Message-ID | <qppIB-2BU-7@gated-at.bofh.it> |
| In reply to | #1259377 |
thanks for the review. I'll work on these.
On 10/30/2015 6:24 AM, Arnd Bergmann wrote:
> On Thursday 29 October 2015 23:08:13 Sinan Kaya wrote:
>> This patch adds support for hidma engine. The driver
>> consists of two logical blocks. The DMA engine interface
>> and the low-level interface. This version of the driver
>> does not support virtualization on this release and only
>> memcpy interface support is included.
>
> Does that mean you can have slave device support eventually?
>
> If so, please put that into the binding document already.
will do.
>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>> .../devicetree/bindings/dma/qcom_hidma.txt | 18 +
>> drivers/dma/Kconfig | 11 +
>> drivers/dma/Makefile | 4 +
>> drivers/dma/qcom_hidma.c | 1717 ++++++++++++++++++++
>> drivers/dma/qcom_hidma.h | 44 +
>> drivers/dma/qcom_hidma_ll.c | 1132 +++++++++++++
>> 6 files changed, 2926 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma.txt
>> create mode 100644 drivers/dma/qcom_hidma.c
>> create mode 100644 drivers/dma/qcom_hidma.h
>> create mode 100644 drivers/dma/qcom_hidma_ll.c
>>
>> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma.txt b/Documentation/devicetree/bindings/dma/qcom_hidma.txt
>> new file mode 100644
>> index 0000000..9a01635
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma.txt
>> @@ -0,0 +1,18 @@
>> +Qualcomm Technologies HIDMA Channel driver
>> +
>> +Required properties:
>> +- compatible: must contain "qcom,hidma"
>
> Be more specific here. Also, should this be 'hisilicon,hidma-1.0' rather
> than 'qcom'? I'm guessing from the name that this is a device you licensed
> from them.
>
No, this is a QCOM part. I used the driver from hisilicon as a starting
point instead of starting from scratch. That's why, I kept the original
copy rights.
>> +- reg: Addresses for the transfer and event channel
>> +- interrupts: Should contain the event interrupt
>> +- desc-count: Number of asynchronous requests this channel can handle
>> +- event-channel: The HW event channel completions will be delivered.
>> +Example:
>> +
>> + hidma_24: hidma@0x5c050000 {
>
> Name should be 'dma-controller' in DT, not 'hidma'.
will test and change.
>
>> + compatible = "qcom,hidma";
>> + reg = <0 0x5c050000 0x0 0x1000>,
>> + <0 0x5c0b0000 0x0 0x1000>;
>> + interrupts = <0 389 0>;
>> + desc-count = <10>;
>> + event-channel = /bits/ 8 <4>;
>> + };
>
> Remove the /bits/.
ok
>
>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>> index 76a5a5e..2645185 100644
>> --- a/drivers/dma/Kconfig
>> +++ b/drivers/dma/Kconfig
>> @@ -512,6 +512,17 @@ config QCOM_HIDMA_MGMT
>> OS would run QCOM_HIDMA driver and the hypervisor would run
>> the QCOM_HIDMA_MGMT driver.
>>
>> +config QCOM_HIDMA
>> + tristate "Qualcomm Technologies HIDMA support"
>> + select DMA_ENGINE
>> + select DMA_VIRTUAL_CHANNELS
>> + help
>> + Enable support for the Qualcomm Technologies HIDMA controller.
>> + The HIDMA controller supports optimized buffer copies
>> + (user to kernel, kernel to kernel, etc.). It only supports
>> + memcpy/memset interfaces. The core is not intended for general
>> + purpose slave DMA.
>> +
>> config XILINX_VDMA
>> tristate "Xilinx AXI VDMA Engine"
>> depends on (ARCH_ZYNQ || MICROBLAZE)
>> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
>> index 3d25ffd..5665df2 100644
>> --- a/drivers/dma/Makefile
>> +++ b/drivers/dma/Makefile
>> @@ -53,6 +53,10 @@ obj-$(CONFIG_PL330_DMA) += pl330.o
>> obj-$(CONFIG_PPC_BESTCOMM) += bestcomm/
>> obj-$(CONFIG_PXA_DMA) += pxa_dma.o
>> obj-$(CONFIG_QCOM_BAM_DMA) += qcom_bam_dma.o
>> +obj-$(CONFIG_QCOM_HIDMA) += qcom_hdma.o
>> +qcom_hdma-objs := qcom_hidma_ll.o qcom_hidma.o
>> +
>
> The driver is linked into a single module, so all the EXPORT_SYMBOL
> statements can be dropped.
will test and change.
>
>> +/* Default idle time is 2 seconds. This parameter can
>> + * be overridden by changing the following
>> + * /sys/bus/platform/devices/QCOM8061:<xy>/power/autosuspend_delay_ms
>> + * during kernel boot.
>> + */
>> +#define AUTOSUSPEND_TIMEOUT 2000
>> +#define HIDMA_DEFAULT_DESCRIPTOR_COUNT 16
>> +#define MODULE_NAME "hidma"
>
> MODULE_NAME and HIDMA_DEFAULT_DESCRIPTOR_COUNT can be dropped
I'll remove the module_name. The default descriptor is used if
device-tree or acpi table does not have the descriptor count.
>
>
>> +#define HIDMA_RUNTIME_GET(dmadev) \
>> +do { \
>> + atomic_inc(&(dmadev)->pm_counter); \
>> + TRC_PM((dmadev)->ddev.dev, \
>> + "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\
>> + atomic_read(&(dmadev)->pm_counter)); \
>> + pm_runtime_get_sync((dmadev)->ddev.dev); \
>> +} while (0)
>> +
>> +#define HIDMA_RUNTIME_SET(dmadev) \
>> +do { \
>> + atomic_dec(&(dmadev)->pm_counter); \
>> + TRC_PM((dmadev)->ddev.dev, \
>> + "%s:%d pm_runtime_put_autosuspend:%d\n", \
>> + __func__, __LINE__, \
>> + atomic_read(&(dmadev)->pm_counter)); \
>> + pm_runtime_mark_last_busy((dmadev)->ddev.dev); \
>> + pm_runtime_put_autosuspend((dmadev)->ddev.dev); \
>> +} while (0)
>
> Inline functions.
ok, I was hoping to keep the file and line numbers as the PM stuff gets
called from multiple locations. should I call the inline functions with
__func__ and __LINE__ from another macro like
#define HIDMA_RUNTIME_GET(dmadev)
func(dmadev, __func__, __LINE__)
etc.
>
>> +struct hidma_test_sync {
>> + atomic_t counter;
>> + wait_queue_head_t wq;
>> +};
>
> Let me guess, you converted this from a semaphore? ;-)
>
> Just put the two members into the containing structure and relete it here.
Probably some other code that I don't remember. will do.
>
>> +struct hidma_dev {
>> + u8 evridx;
>> + u32 nr_descriptors;
>> +
>> + void *lldev;
>> + void __iomem *dev_trca;
>> + void __iomem *dev_evca;
>> + int (*self_test)(struct hidma_dev *device);
>> + struct dentry *debugfs;
>> + struct dentry *stats;
>> +
>> + /* used to protect the pending channel list*/
>> + spinlock_t lock;
>> + dma_addr_t dev_trca_phys;
>> + struct dma_device ddev;
>> + struct tasklet_struct tasklet;
>
> workqueue maybe?
is there an advantage of workqueue over tasklet? I'm open to suggestions.
>
>> + resource_size_t dev_trca_size;
>> + dma_addr_t dev_evca_phys;
>> + resource_size_t dev_evca_size;
>
> All three look unused and can be removed.
ok.
>
>> +static unsigned int debug_pm;
>> +module_param(debug_pm, uint, 0644);
>> +MODULE_PARM_DESC(debug_pm,
>> + "debug runtime power management transitions (default: 0)");
>> +
>> +#define TRC_PM(...) do { \
>> + if (debug_pm) \
>> + dev_info(__VA_ARGS__); \
>> + } while (0)
>
> Again, remove these after you are done debugging the problem at hand,
> we don't need to clutter up the upstream version.
ok, I'll take them out if that's the norm. I was hoping to keep them in
case something shows up.
>
>> + /*
>> + * It is assumed that the hardware can move the data within 1s
>> + * and signal the OS of the completion
>> + */
>> + ret = wait_event_interruptible_timeout(dmadev->test_result.wq,
>> + atomic_read(&dmadev->test_result.counter) == (map_count),
>> + msecs_to_jiffies(10000));
>> +
>> + if (ret <= 0) {
>> + dev_err(dmadev->ddev.dev,
>> + "Self-test sg copy timed out, disabling\n");
>> + err = -ENODEV;
>> + goto tx_status;
>> + }
>
> Why ENODEV? Could you make this handle restarted system calls?
This is the self test code. It gets called from probe. If there is a
problem with the device or system configuration, I don't want to enable
this device. I can certainly return a different error code though.
What's a good code?
>
>> +
>> +/*
>> + * Perform a streaming transaction to verify the HW works.
>> + */
>> +static int hidma_selftest_streaming(struct hidma_dev *dmadev,
>> + struct dma_chan *dma_chanptr, u64 size,
>> + unsigned long flags)
>> +{
>
> You have a lot of selftest code here. Can you try to move that into a file
> that works for all DMA engines? It feels like this should not be part of
> a driver.
Will try.
>
>> +
>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>> +
>> +#define SIER_CHAN_SHOW(chan, name) \
>> + seq_printf(s, #name "=%u\n", chan->name)
>
> can be open-coded for clarity.
>
I think I copied this from another driver. Would you rather see
seq_printf in the code than a macro.
Sorry, I didn't get what you mean by "open-coded".
>> + ret = dma_mapping_error(dev, dma_src);
>> + if (ret) {
>> + dev_err(dev, "dma_mapping_error with ret:%d\n", ret);
>> + ret = -ENOMEM;
>> + } else {
>> + phys_addr_t phys;
>> +
>> + phys = dma_to_phys(dev, dma_src);
>> + if (strcmp(__va(phys), "hello world") != 0) {
>> + dev_err(dev, "memory content mismatch\n");
>> + ret = -EINVAL;
>> + } else {
>> + dev_dbg(dev, "mapsingle:dma_map_single works\n");
>> + }
>> + dma_unmap_single(dev, dma_src, buf_size, DMA_TO_DEVICE);
>> + }
>
> dma_to_phys() is architecture specific and does not work if you
> have an IOMMU. Just use the virtual address you passed into dma_map_*.
Should we fix the real problem?
I patched dma_to_phys to call IOMMU driver for translation when IOMMU is
present. I was planning to upstream that patch when Robin Murphy's
changes make upstream.
The assumption of phys==dma address doesn't look scalable to me. I was
checking the DMA subsystem for correctness with this test. If use
dma_src, it is going to be a 1==1 test.
>
>> +/**
>> + * hidma_dma_info: display HIDMA device info
>> + *
>> + * Display the info for the current HIDMA device.
>> + */
>> +static int hidma_dma_info(struct seq_file *s, void *unused)
>> +{
>> + struct hidma_dev *dmadev = s->private;
>> + struct dma_device *dma = &dmadev->ddev;
>> +
>> + seq_printf(s, "nr_descriptors=%d\n", dmadev->nr_descriptors);
>> + seq_printf(s, "dev_trca=%p\n", &dmadev->dev_trca);
>> + seq_printf(s, "dev_trca_phys=%pa\n", &dmadev->dev_trca_phys);
>> + seq_printf(s, "dev_trca_size=%pa\n", &dmadev->dev_trca_size);
>> + seq_printf(s, "dev_evca=%p\n", &dmadev->dev_evca);
>> + seq_printf(s, "dev_evca_phys=%pa\n", &dmadev->dev_evca_phys);
>> + seq_printf(s, "dev_evca_size=%pa\n", &dmadev->dev_evca_size);
>
> Don't print pointers here.
ok
>
>> + seq_printf(s, "self_test=%u\n",
>> + atomic_read(&dmadev->test_result.counter));
>> +
>> + seq_printf(s, "copy%s%s%s%s%s%s%s%s%s%s%s\n",
>> + dma_has_cap(DMA_PQ, dma->cap_mask) ? " pq" : "",
>> + dma_has_cap(DMA_PQ_VAL, dma->cap_mask) ? " pq_val" : "",
>> + dma_has_cap(DMA_XOR, dma->cap_mask) ? " xor" : "",
>> + dma_has_cap(DMA_XOR_VAL, dma->cap_mask) ? " xor_val" : "",
>> + dma_has_cap(DMA_INTERRUPT, dma->cap_mask) ? " intr" : "",
>> + dma_has_cap(DMA_SG, dma->cap_mask) ? " sg" : "",
>> + dma_has_cap(DMA_ASYNC_TX, dma->cap_mask) ? " async" : "",
>> + dma_has_cap(DMA_SLAVE, dma->cap_mask) ? " slave" : "",
>> + dma_has_cap(DMA_CYCLIC, dma->cap_mask) ? " cyclic" : "",
>> + dma_has_cap(DMA_INTERLEAVE, dma->cap_mask) ? " intl" : "",
>> + dma_has_cap(DMA_MEMCPY, dma->cap_mask) ? " memcpy" : "");
>> +
>> + return 0;
>> +}
>
> The selftest part and the features could just be separate files in the
> core dmaengine code, it doesn't really belong here.
>
I'll take a look. It could take a couple of iterations to get this right
but I could certainly move my testing to another file for sharing.
>> +}
>> +#else
>> +static void hidma_debug_uninit(struct hidma_dev *dmadev)
>> +{
>> +}
>> +static int hidma_debug_init(struct hidma_dev *dmadev)
>> +{
>> + return 0;
>> +}
>> +#endif
>
> You can remove the #ifdef here. The debugfs code already stubs out all
> debugfs_create_*() and turns it all into nops when it is disabled.
ok, I think I got compilation errors with the data structures but I'll
take a look.
>
>> + dma_cap_set(DMA_MEMCPY, dmadev->ddev.cap_mask);
>> + /* Apply default dma_mask if needed */
>> + if (!pdev->dev.dma_mask) {
>> + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>> + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
>> + }
>> +
>
> remove the check, or use
I got caught on this problem before. Let me go back and revisit. It took
my entire day to figure out that dma_mask pointer was not set.
>
> if (WARN_ON(!pdev->dev.dma_mask))
> return -ENXIO;
>
> The dma mask has to always be set by the platform code before probe()
> is called. If it is not set, you are not allowed to perform DMA.
I tested this on an ACPI platform BTW when I was working on the initial
implementation.
>
>> + dmadev->dev_evca_phys = evca_resource->start;
>> + dmadev->dev_evca_size = resource_size(evca_resource);
>> +
>> + dev_dbg(&pdev->dev, "dev_evca_phys:%pa\n", &dmadev->dev_evca_phys);
>> + dev_dbg(&pdev->dev, "dev_evca_size:%pa\n", &dmadev->dev_evca_size);
>> +
>
>> + dev_dbg(&pdev->dev, "qcom_hidma: mapped EVCA %pa to %p\n",
>> + &dmadev->dev_evca_phys, dmadev->dev_evca);
>
>
>
>> + dmadev->dev_trca_phys = trca_resource->start;
>> + dmadev->dev_trca_size = resource_size(trca_resource);
>> +
>> + dev_dbg(&pdev->dev, "dev_trca_phys:%pa\n", &dmadev->dev_trca_phys);
>> + dev_dbg(&pdev->dev, "dev_trca_size:%pa\n", &dmadev->dev_trca_size);
>
> Don't print pointers.
OK
>
>> + rc = devm_request_irq(&pdev->dev, chirq, hidma_chirq_handler, 0,
>> + "qcom-hidma", &dmadev->lldev);
>> + if (rc) {
>> + dev_err(&pdev->dev, "chirq registration failed: %d\n", chirq);
>> + goto chirq_request_failed;
>> + }
>> +
>> + dev_dbg(&pdev->dev, "initializing DMA channels\n");
>> + INIT_LIST_HEAD(&dmadev->ddev.channels);
>> + rc = hidma_chan_init(dmadev, 0);
>> + if (rc) {
>> + dev_err(&pdev->dev, "probe:channel init failed\n");
>> + goto channel_init_failed;
>> + }
>> + dev_dbg(&pdev->dev, "HI-DMA engine driver starting self test\n");
>> + rc = dmadev->self_test(dmadev);
>> + if (rc) {
>> + dev_err(&pdev->dev, "probe: self test failed: %d\n", rc);
>> + goto self_test_failed;
>> + }
>> + dev_info(&pdev->dev, "probe: self test succeeded.\n");
>> +
>> + dev_dbg(&pdev->dev, "calling dma_async_device_register\n");
>> + rc = dma_async_device_register(&dmadev->ddev);
>> + if (rc) {
>> + dev_err(&pdev->dev,
>> + "probe: failed to register slave DMA: %d\n", rc);
>> + goto device_register_failed;
>> + }
>> + dev_dbg(&pdev->dev, "probe: dma_async_device_register done\n");
>> +
>> + rc = hidma_debug_init(dmadev);
>> + if (rc) {
>> + dev_err(&pdev->dev,
>> + "probe: failed to init debugfs: %d\n", rc);
>> + goto debug_init_failed;
>> + }
>> +
>> + dev_info(&pdev->dev, "HI-DMA engine driver registration complete\n");
>> + platform_set_drvdata(pdev, dmadev);
>> + HIDMA_RUNTIME_SET(dmadev);
>> + return 0;
>
> Remove the debug prints when you are done debugging.
errors or the dev_infos?
>
>> +debug_init_failed:
>> +device_register_failed:
>> +self_test_failed:
>> +channel_init_failed:
>> +chirq_request_failed:
>> + hidma_ll_uninit(dmadev->lldev);
>> +ll_init_failed:
>> +evridx_failed:
>> +remap_trca_failed:
>> +remap_evca_failed:
>> + if (dmadev)
>> + hidma_free(dmadev);
>
> Rename the labels according to what you do at in the failure case
> and remove most of them. This is 'goto', not 'comefrom' ;-)
ok, We had different opinions how gotos should be written.
>
>> +static struct platform_driver hidma_driver = {
>> + .probe = hidma_probe,
>> + .remove = hidma_remove,
>> + .driver = {
>> + .name = MODULE_NAME,
>> + .owner = THIS_MODULE,
>> + .of_match_table = of_match_ptr(hidma_match),
>> + .acpi_match_table = ACPI_PTR(hidma_acpi_ids),
>
>
> Remove .owner and of_match_ptr().
>
ok, will do.
>> + },
>> +};
>> +
>> +static int __init hidma_init(void)
>> +{
>> + return platform_driver_register(&hidma_driver);
>> +}
>> +late_initcall(hidma_init);
>> +
>> +static void __exit hidma_exit(void)
>> +{
>> + platform_driver_unregister(&hidma_driver);
>> +}
>> +module_exit(hidma_exit);
>
> module_platform_driver()
ok
>
>> +
>> + if (unlikely(tre_ch >= lldev->nr_tres)) {
>> + dev_err(lldev->dev, "invalid TRE number in free:%d", tre_ch);
>> + return;
>> + }
>> +
>> + tre = &lldev->trepool[tre_ch];
>> + if (unlikely(atomic_read(&tre->allocated) != true)) {
>> + dev_err(lldev->dev, "trying to free an unused TRE:%d",
>> + tre_ch);
>> + return;
>> + }
>
> Remove the 'unlikely' and the redundant '!= true'.
>
> Only use 'likely' or 'unlikely' if you can measure a difference.
ok
>
>> +static int hidma_ll_reset(struct hidma_lldev *lldev)
>> +{
>> + u32 val;
>> + int count;
>> +
>> + val = readl_relaxed(lldev->trca + TRCA_CTRLSTS_OFFSET);
>> + val = val & ~(CH_CONTROL_MASK << 16);
>> + val = val | (CH_RESET << 16);
>> + writel_relaxed(val, lldev->trca + TRCA_CTRLSTS_OFFSET);
>> +
>> + /* wait until the reset is performed */
>> + wmb();
>> +
>> + /* Delay 10ms after reset to allow DMA logic to quiesce.*/
>> + for (count = 0; count < 10; count++) {
>> + val = readl_relaxed(lldev->trca + TRCA_CTRLSTS_OFFSET);
>> + lldev->trch_state = (val >> CH_STATE_BIT_POS)
>> + & CH_STATE_MASK;
>> + if (lldev->trch_state == CH_DISABLED)
>> + break;
>> + mdelay(1);
>> + }
>> + val = readl_relaxed(lldev->trca + TRCA_CTRLSTS_OFFSET);
>> + lldev->trch_state = (val >> CH_STATE_BIT_POS) & CH_STATE_MASK;
>> + if (lldev->trch_state != CH_DISABLED) {
>> + dev_err(lldev->dev,
>> + "transfer channel did not reset\n");
>> + return -ENODEV;
>> + }
>> +
>> + val = readl_relaxed(lldev->evca + EVCA_CTRLSTS_OFFSET);
>> + val = val & ~(CH_CONTROL_MASK << 16);
>> + val = val | (CH_RESET << 16);
>> + writel_relaxed(val, lldev->evca + EVCA_CTRLSTS_OFFSET);
>> +
>> + /* wait until the reset is performed */
>> + wmb();
>> +
>> + /* Delay 10ms after reset to allow DMA logic to quiesce.*/
>> + for (count = 0; count < 10; count++) {
>> + val = readl_relaxed(lldev->evca + EVCA_CTRLSTS_OFFSET);
>> + lldev->evch_state = (val >> CH_STATE_BIT_POS)
>> + & CH_STATE_MASK;
>> + if (lldev->evch_state == CH_DISABLED)
>> + break;
>> + mdelay(1);
>> + }
>
> Try using a workqueue to get into a state where you can call msleep()
> instead of mdelay().
>
will try
> Also, if you waste CPU cycles for hundreds of milliseconds, it's unlikely
> that the function is so performance critical that it requires writel_relaxed().
>
> Just use writel() here.
The issue is not writel_relaxed vs. writel. After I issue reset, I need
wait for some time to confirm reset was done. I can use readl_polling
instead of mdelay if we don't like mdelay.
>> +/*
>> + * The interrupt handler for HIDMA will try to consume as many pending
>> + * EVRE from the event queue as possible. Each EVRE has an associated
>> + * TRE that holds the user interface parameters. EVRE reports the
>> + * result of the transaction. Hardware guarantees ordering between EVREs
>> + * and TREs. We use last processed offset to figure out which TRE is
>> + * associated with which EVRE. If two TREs are consumed by HW, the EVREs
>> + * are in order in the event ring.
>> + * This handler will do a one pass for consuming EVREs. Other EVREs may
>> + * be delivered while we are working. It will try to consume incoming
>> + * EVREs one more time and return.
>> + * For unprocessed EVREs, hardware will trigger another interrupt until
>> + * all the interrupt bits are cleared.
>> + */
>> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
>> +{
>> + u32 status;
>> + u32 enable;
>> + u32 cause;
>> + int repeat = 2;
>> + unsigned long timeout;
>> +
>> + status = readl_relaxed(lldev->evca + EVCA_IRQ_STAT_OFFSET);
>> + enable = readl_relaxed(lldev->evca + EVCA_IRQ_EN_OFFSET);
>> + cause = status & enable;
>> +
>
> Reading the status probably requires a readl() rather than readl_relaxed()
> to guarantee that the DMA data has arrived in memory by the time that the
> register data is seen by the CPU. If using readl_relaxed() here is a valid
> and required optimization, please add a comment to explain why it works
> and how much you gain.
I will add some description. This is a high speed peripheral. I don't
like spreading barriers as candies inside the readl and writel unless I
have to.
According to the barriers video, I watched on youtube this should be the
rule for ordering.
"if you do two relaxed reads and check the results of the returned
variables, ARM architecture guarantees that these two relaxed variables
will get observed during the check."
this is called implied ordering or something of that sort.
>
>> + /* Another interrupt might have arrived while we are
>> + * processing this one. Read the new cause.
>> + */
>> + status = readl_relaxed(lldev->evca + EVCA_IRQ_STAT_OFFSET);
>> + enable = readl_relaxed(lldev->evca + EVCA_IRQ_EN_OFFSET);
>> + cause = status & enable;
>> +
>> + repeat--;
>> + }
>
> Same here.
>
comment above.
>
>> +}
>> +
>> +
>> +static int hidma_ll_enable(struct hidma_lldev *lldev)
>> +{
>> + u32 val;
>> +
>> + val = readl_relaxed(lldev->evca + EVCA_CTRLSTS_OFFSET);
>> + val &= ~(CH_CONTROL_MASK << 16);
>> + val |= (CH_ENABLE << 16);
>> +
>> + writel_relaxed(val, lldev->evca + EVCA_CTRLSTS_OFFSET);
>> +
>> + /* wait until channel is enabled */
>> + wmb();
>> +
>> + mdelay(1);
>> +
>> + val = readl_relaxed(lldev->evca + EVCA_CTRLSTS_OFFSET);
>> + lldev->evch_state = (val >> CH_STATE_BIT_POS) & CH_STATE_MASK;
>> + if ((lldev->evch_state != CH_ENABLED) &&
>> + (lldev->evch_state != CH_RUNNING)) {
>> + dev_err(lldev->dev,
>> + "event channel did not get enabled\n");
>> + return -ENODEV;
>> + }
>> +
>> + val = readl_relaxed(lldev->trca + TRCA_CTRLSTS_OFFSET);
>> + val = val & ~(CH_CONTROL_MASK << 16);
>> + val = val | (CH_ENABLE << 16);
>> + writel_relaxed(val, lldev->trca + TRCA_CTRLSTS_OFFSET);
>> +
>> + /* wait until channel is enabled */
>> + wmb();
>> +
>> + mdelay(1);
>
> Another workqueue? You should basically never call mdelay().
I'll use polled read instead.
>
>> +static int hidma_ll_hw_start(void *llhndl)
>> +{
>> + int rc = 0;
>> + struct hidma_lldev *lldev = llhndl;
>> + unsigned long irqflags;
>> +
>> + spin_lock_irqsave(&lldev->lock, irqflags);
>> + writel_relaxed(lldev->tre_write_offset,
>> + lldev->trca + TRCA_DOORBELL_OFFSET);
>> + spin_unlock_irqrestore(&lldev->lock, irqflags);
>
> How does this work? The writel_relaxed() won't synchronize with either
> the DMA data or the spinlock.
mutex and spinlocks have barriers inside. See the youtube video.
https://www.youtube.com/watch?v=6ORn6_35kKo
>
>> +int hidma_ll_init(void **lldevp, struct device *dev, u32 nr_tres,
>> + void __iomem *trca, void __iomem *evca,
>> + u8 evridx)
>
> How about returning the pointer rather than passing in an indirect pointer?
ok
>
> Also, your abstraction seem to go a little too far if the upper driver
> doesn't know what the lower driver calls its main device structure.
>
> Or you can go further and just embed the struct hidma_lldev within the
> struct hidma_dev to save one?
That's how it was before. It got too complex and variables/spinlocks got
intermixed. I borrowed the upper layer and it worked as it is. I rather
keep all hardware stuff in another file and do not mix and match for safety.
>
>> +void hidma_ll_chstats(struct seq_file *s, void *llhndl, u32 tre_ch)
>> +{
>> + struct hidma_lldev *lldev = llhndl;
>> + struct hidma_tre *tre;
>> + u32 length;
>> + dma_addr_t src_start;
>> + dma_addr_t dest_start;
>> + u32 *tre_local;
>> +
>> + if (unlikely(tre_ch >= lldev->nr_tres)) {
>> + dev_err(lldev->dev, "invalid TRE number in chstats:%d",
>> + tre_ch);
>> + return;
>> + }
>> + tre = &lldev->trepool[tre_ch];
>> + seq_printf(s, "------Channel %d -----\n", tre_ch);
>> + seq_printf(s, "allocated=%d\n", atomic_read(&tre->allocated));
>> + HIDMA_CHAN_SHOW(tre, queued);
>> + seq_printf(s, "err_info=0x%x\n",
>> + lldev->tx_status_list[tre->chidx].err_info);
>> + seq_printf(s, "err_code=0x%x\n",
>> + lldev->tx_status_list[tre->chidx].err_code);
>> + HIDMA_CHAN_SHOW(tre, status);
>> + HIDMA_CHAN_SHOW(tre, chidx);
>> + HIDMA_CHAN_SHOW(tre, dma_sig);
>> + seq_printf(s, "dev_name=%s\n", tre->dev_name);
>> + seq_printf(s, "callback=%p\n", tre->callback);
>> + seq_printf(s, "data=%p\n", tre->data);
>> + HIDMA_CHAN_SHOW(tre, tre_index);
>> +
>> + tre_local = &tre->tre_local[0];
>> + src_start = tre_local[TRE_SRC_LOW_IDX];
>> + src_start = ((u64)(tre_local[TRE_SRC_HI_IDX]) << 32) + src_start;
>> + dest_start = tre_local[TRE_DEST_LOW_IDX];
>> + dest_start += ((u64)(tre_local[TRE_DEST_HI_IDX]) << 32);
>> + length = tre_local[TRE_LEN_IDX];
>> +
>> + seq_printf(s, "src=%pap\n", &src_start);
>> + seq_printf(s, "dest=%pap\n", &dest_start);
>> + seq_printf(s, "length=0x%x\n", length);
>> +}
>> +EXPORT_SYMBOL_GPL(hidma_ll_chstats);
>
> Remove all the pointers here. I guess you can remove the entire debugfs
> file really ;-)
ok, I need some facility to print out stuff when problems happened.
Would you rather use sysfs?
>
> This looks like it is better done using ftrace for the low-level internals
> of the driver.
I'll look at ftrace.
>
> Arnd
>
thanks for the review again.
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Timur Tabi <timur@codeaurora.org> |
|---|---|
| Date | 2015-10-30 23:00 +0100 |
| Subject | Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver |
| Message-ID | <qppSi-2Fa-1@gated-at.bofh.it> |
| In reply to | #1259809 |
On 10/30/2015 04:42 PM, Sinan Kaya wrote: >> >> if (WARN_ON(!pdev->dev.dma_mask)) >> return -ENXIO; >> >> The dma mask has to always be set by the platform code before probe() >> is called. If it is not set, you are not allowed to perform DMA. > > I tested this on an ACPI platform BTW when I was working on the initial > implementation. PowerPC sets the mask to 32 bits by default: http://lxr.free-electrons.com/ident?i=arch_setup_pdev_archdata Should we do something similar in ARM64? Today, we have to manually set the DMA mask in all drivers. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2015-10-30 23:50 +0100 |
| Subject | Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver |
| Message-ID | <qpqEG-3cv-25@gated-at.bofh.it> |
| In reply to | #1259812 |
On Friday 30 October 2015 16:55:16 Timur Tabi wrote: > > On 10/30/2015 04:42 PM, Sinan Kaya wrote: > >> > >> if (WARN_ON(!pdev->dev.dma_mask)) > >> return -ENXIO; > >> > >> The dma mask has to always be set by the platform code before probe() > >> is called. If it is not set, you are not allowed to perform DMA. > > > > I tested this on an ACPI platform BTW when I was working on the initial > > implementation. > > PowerPC sets the mask to 32 bits by default: > > http://lxr.free-electrons.com/ident?i=arch_setup_pdev_archdata > > Should we do something similar in ARM64? Today, we have to manually set > the DMA mask in all drivers. We set the dma mask from the 'dma-ranges' property of the parent device, but fall back to 32-bit because we did not manage to mandate this property in time for all arm64 machines to use. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Timur Tabi <timur@codeaurora.org> |
|---|---|
| Date | 2015-10-30 23:40 +0100 |
| Subject | Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver |
| Message-ID | <qpqv0-39c-15@gated-at.bofh.it> |
| In reply to | #1259809 |
On 10/30/2015 05:28 PM, Arnd Bergmann wrote: >>> > >Why ENODEV? Could you make this handle restarted system calls? >> > >> >This is the self test code. It gets called from probe. If there is a >> >problem with the device or system configuration, I don't want to enable >> >this device. I can certainly return a different error code though. >> >What's a good code? > I see. probe() is not restartable, so it cannot be -ERESTARTSYS. > > Maybe better use wait_event_timeout and not handle the signals then. > It will eventually time out if something goes wrong. What about -EPROBE_DEFER? Isn't that "restartable"? Granted, it's only supposed to be used if the driver is dependent on another driver to probe, so I'm not sure it applies here. If the self-test fails, then it is possible that it could succeed later? -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2015-10-31 00:00 +0100 |
| Subject | Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver |
| Message-ID | <qpqOl-3fL-1@gated-at.bofh.it> |
| In reply to | #1259829 |
On Friday 30 October 2015 17:36:45 Timur Tabi wrote: > On 10/30/2015 05:28 PM, Arnd Bergmann wrote: > >>> > >Why ENODEV? Could you make this handle restarted system calls? > >> > > >> >This is the self test code. It gets called from probe. If there is a > >> >problem with the device or system configuration, I don't want to enable > >> >this device. I can certainly return a different error code though. > >> >What's a good code? > > I see. probe() is not restartable, so it cannot be -ERESTARTSYS. > > > > Maybe better use wait_event_timeout and not handle the signals then. > > It will eventually time out if something goes wrong. > > What about -EPROBE_DEFER? Isn't that "restartable"? Granted, it's only > supposed to be used if the driver is dependent on another driver to > probe, so I'm not sure it applies here. If the self-test fails, then it > is possible that it could succeed later? No, this is different. The probe function can get called from all sorts of contexts (sys_init_module, device_create, deferred probing), and not all of them go back to user space when returning an error, so we cannot deliver a signal to the calling process this way. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2015-10-30 23:40 +0100 |
| Subject | Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver |
| Message-ID | <qpqv0-39c-17@gated-at.bofh.it> |
| In reply to | #1259809 |
On Friday 30 October 2015 17:42:26 Sinan Kaya wrote:
> On 10/30/2015 6:24 AM, Arnd Bergmann wrote:
> > On Thursday 29 October 2015 23:08:13 Sinan Kaya wrote:
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> ---
> >> .../devicetree/bindings/dma/qcom_hidma.txt | 18 +
> >> drivers/dma/Kconfig | 11 +
> >> drivers/dma/Makefile | 4 +
> >> drivers/dma/qcom_hidma.c | 1717 ++++++++++++++++++++
> >> drivers/dma/qcom_hidma.h | 44 +
> >> drivers/dma/qcom_hidma_ll.c | 1132 +++++++++++++
> >> 6 files changed, 2926 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma.txt
> >> create mode 100644 drivers/dma/qcom_hidma.c
> >> create mode 100644 drivers/dma/qcom_hidma.h
> >> create mode 100644 drivers/dma/qcom_hidma_ll.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma.txt b/Documentation/devicetree/bindings/dma/qcom_hidma.txt
> >> new file mode 100644
> >> index 0000000..9a01635
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma.txt
> >> @@ -0,0 +1,18 @@
> >> +Qualcomm Technologies HIDMA Channel driver
> >> +
> >> +Required properties:
> >> +- compatible: must contain "qcom,hidma"
> >
> > Be more specific here. Also, should this be 'hisilicon,hidma-1.0' rather
> > than 'qcom'? I'm guessing from the name that this is a device you licensed
> > from them.
> >
> No, this is a QCOM part. I used the driver from hisilicon as a starting
> point instead of starting from scratch. That's why, I kept the original
> copy rights.
I meant the 'hidma' name. ;-)
> >> +- reg: Addresses for the transfer and event channel
> >> +- interrupts: Should contain the event interrupt
> >> +- desc-count: Number of asynchronous requests this channel can handle
> >> +- event-channel: The HW event channel completions will be delivered.
> >> +Example:
> >> +
> >> + hidma_24: hidma@0x5c050000 {
> >
> > Name should be 'dma-controller' in DT, not 'hidma'.
> will test and change.
no need to test, Linux ignores the node name.
> >
> > MODULE_NAME and HIDMA_DEFAULT_DESCRIPTOR_COUNT can be dropped
>
> I'll remove the module_name. The default descriptor is used if
> device-tree or acpi table does not have the descriptor count.
I missed that part. If the descriptor count is a hardware feature,
just make that property mandatory. OTOH, if this is an optimization
setting, better drop that property entirely.
> >> +#define HIDMA_RUNTIME_GET(dmadev) \
> >> +do { \
> >> + atomic_inc(&(dmadev)->pm_counter); \
> >> + TRC_PM((dmadev)->ddev.dev, \
> >> + "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\
> >> + atomic_read(&(dmadev)->pm_counter)); \
> >> + pm_runtime_get_sync((dmadev)->ddev.dev); \
> >> +} while (0)
> >> +
> >> +#define HIDMA_RUNTIME_SET(dmadev) \
> >> +do { \
> >> + atomic_dec(&(dmadev)->pm_counter); \
> >> + TRC_PM((dmadev)->ddev.dev, \
> >> + "%s:%d pm_runtime_put_autosuspend:%d\n", \
> >> + __func__, __LINE__, \
> >> + atomic_read(&(dmadev)->pm_counter)); \
> >> + pm_runtime_mark_last_busy((dmadev)->ddev.dev); \
> >> + pm_runtime_put_autosuspend((dmadev)->ddev.dev); \
> >> +} while (0)
> >
> > Inline functions.
>
> ok, I was hoping to keep the file and line numbers as the PM stuff gets
> called from multiple locations. should I call the inline functions with
> __func__ and __LINE__ from another macro like
>
> #define HIDMA_RUNTIME_GET(dmadev)
> func(dmadev, __func__, __LINE__)
>
> etc.
I would just drop the debugging macros entirely here.
> >
> >> +struct hidma_dev {
> >> + u8 evridx;
> >> + u32 nr_descriptors;
> >> +
> >> + void *lldev;
> >> + void __iomem *dev_trca;
> >> + void __iomem *dev_evca;
> >> + int (*self_test)(struct hidma_dev *device);
> >> + struct dentry *debugfs;
> >> + struct dentry *stats;
> >> +
> >> + /* used to protect the pending channel list*/
> >> + spinlock_t lock;
> >> + dma_addr_t dev_trca_phys;
> >> + struct dma_device ddev;
> >> + struct tasklet_struct tasklet;
> >
> > workqueue maybe?
>
> is there an advantage of workqueue over tasklet? I'm open to suggestions.
tasklets are not normally used in new code. They are only marginally
faster than workqueues, but are executed in atomic context and cannot
call sleeping functions.
> >> +static unsigned int debug_pm;
> >> +module_param(debug_pm, uint, 0644);
> >> +MODULE_PARM_DESC(debug_pm,
> >> + "debug runtime power management transitions (default: 0)");
> >> +
> >> +#define TRC_PM(...) do { \
> >> + if (debug_pm) \
> >> + dev_info(__VA_ARGS__); \
> >> + } while (0)
> >
> > Again, remove these after you are done debugging the problem at hand,
> > we don't need to clutter up the upstream version.
>
> ok, I'll take them out if that's the norm. I was hoping to keep them in
> case something shows up.
In my experience, the bugs you have now are different from the bugs
that other people find next, and debugging macros like this end up
not helping then.
> >> + /*
> >> + * It is assumed that the hardware can move the data within 1s
> >> + * and signal the OS of the completion
> >> + */
> >> + ret = wait_event_interruptible_timeout(dmadev->test_result.wq,
> >> + atomic_read(&dmadev->test_result.counter) == (map_count),
> >> + msecs_to_jiffies(10000));
> >> +
> >> + if (ret <= 0) {
> >> + dev_err(dmadev->ddev.dev,
> >> + "Self-test sg copy timed out, disabling\n");
> >> + err = -ENODEV;
> >> + goto tx_status;
> >> + }
> >
> > Why ENODEV? Could you make this handle restarted system calls?
>
> This is the self test code. It gets called from probe. If there is a
> problem with the device or system configuration, I don't want to enable
> this device. I can certainly return a different error code though.
> What's a good code?
I see. probe() is not restartable, so it cannot be -ERESTARTSYS.
Maybe better use wait_event_timeout and not handle the signals then.
It will eventually time out if something goes wrong.
> >> +
> >> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> >> +
> >> +#define SIER_CHAN_SHOW(chan, name) \
> >> + seq_printf(s, #name "=%u\n", chan->name)
> >
> > can be open-coded for clarity.
> >
>
> I think I copied this from another driver. Would you rather see
> seq_printf in the code than a macro.
>
> Sorry, I didn't get what you mean by "open-coded".
Yes, that is what I meant. Keep the code but don't put it into
an inline function or macro.
> >> + ret = dma_mapping_error(dev, dma_src);
> >> + if (ret) {
> >> + dev_err(dev, "dma_mapping_error with ret:%d\n", ret);
> >> + ret = -ENOMEM;
> >> + } else {
> >> + phys_addr_t phys;
> >> +
> >> + phys = dma_to_phys(dev, dma_src);
> >> + if (strcmp(__va(phys), "hello world") != 0) {
> >> + dev_err(dev, "memory content mismatch\n");
> >> + ret = -EINVAL;
> >> + } else {
> >> + dev_dbg(dev, "mapsingle:dma_map_single works\n");
> >> + }
> >> + dma_unmap_single(dev, dma_src, buf_size, DMA_TO_DEVICE);
> >> + }
> >
> > dma_to_phys() is architecture specific and does not work if you
> > have an IOMMU. Just use the virtual address you passed into dma_map_*.
>
> Should we fix the real problem?
>
> I patched dma_to_phys to call IOMMU driver for translation when IOMMU is
> present. I was planning to upstream that patch when Robin Murphy's
> changes make upstream.
>
> The assumption of phys==dma address doesn't look scalable to me. I was
> checking the DMA subsystem for correctness with this test. If use
> dma_src, it is going to be a 1==1 test.
dma_to_phys is the internal helper that is used by the iommu
implementation, drivers should never call it, and I don't see
why you would do that.
> >
> >> + seq_printf(s, "self_test=%u\n",
> >> + atomic_read(&dmadev->test_result.counter));
> >> +
> >> + seq_printf(s, "copy%s%s%s%s%s%s%s%s%s%s%s\n",
> >> + dma_has_cap(DMA_PQ, dma->cap_mask) ? " pq" : "",
> >> + dma_has_cap(DMA_PQ_VAL, dma->cap_mask) ? " pq_val" : "",
> >> + dma_has_cap(DMA_XOR, dma->cap_mask) ? " xor" : "",
> >> + dma_has_cap(DMA_XOR_VAL, dma->cap_mask) ? " xor_val" : "",
> >> + dma_has_cap(DMA_INTERRUPT, dma->cap_mask) ? " intr" : "",
> >> + dma_has_cap(DMA_SG, dma->cap_mask) ? " sg" : "",
> >> + dma_has_cap(DMA_ASYNC_TX, dma->cap_mask) ? " async" : "",
> >> + dma_has_cap(DMA_SLAVE, dma->cap_mask) ? " slave" : "",
> >> + dma_has_cap(DMA_CYCLIC, dma->cap_mask) ? " cyclic" : "",
> >> + dma_has_cap(DMA_INTERLEAVE, dma->cap_mask) ? " intl" : "",
> >> + dma_has_cap(DMA_MEMCPY, dma->cap_mask) ? " memcpy" : "");
> >> +
> >> + return 0;
> >> +}
> >
> > The selftest part and the features could just be separate files in the
> > core dmaengine code, it doesn't really belong here.
> >
> I'll take a look. It could take a couple of iterations to get this right
> but I could certainly move my testing to another file for sharing.
I also missed the part about the selftest being called from probe
rather than through debugfs.
> >> +}
> >> +#else
> >> +static void hidma_debug_uninit(struct hidma_dev *dmadev)
> >> +{
> >> +}
> >> +static int hidma_debug_init(struct hidma_dev *dmadev)
> >> +{
> >> + return 0;
> >> +}
> >> +#endif
> >
> > You can remove the #ifdef here. The debugfs code already stubs out all
> > debugfs_create_*() and turns it all into nops when it is disabled.
>
> ok, I think I got compilation errors with the data structures but I'll
> take a look.
Ok. There may be cases where a structure definition is mistakenly placed
in an #ifdef and should instead be taken out.
> >> + dma_cap_set(DMA_MEMCPY, dmadev->ddev.cap_mask);
> >> + /* Apply default dma_mask if needed */
> >> + if (!pdev->dev.dma_mask) {
> >> + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> >> + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> >> + }
> >> +
> >
> > remove the check, or use
>
> I got caught on this problem before. Let me go back and revisit. It took
> my entire day to figure out that dma_mask pointer was not set.
>
> >
> > if (WARN_ON(!pdev->dev.dma_mask))
> > return -ENXIO;
> >
> > The dma mask has to always be set by the platform code before probe()
> > is called. If it is not set, you are not allowed to perform DMA.
>
> I tested this on an ACPI platform BTW when I was working on the initial
> implementation.
I remember there was a bug in ACPI that it was not setting the dma mask
pointer right. That should be fixed in newer kernels. If not, don't
add another workaround for broken ACPI but instead fix the ACPI code.
> >
> >> + rc = devm_request_irq(&pdev->dev, chirq, hidma_chirq_handler, 0,
> >> + "qcom-hidma", &dmadev->lldev);
> >> + if (rc) {
> >> + dev_err(&pdev->dev, "chirq registration failed: %d\n", chirq);
> >> + goto chirq_request_failed;
> >> + }
> >> +
> >> + dev_dbg(&pdev->dev, "initializing DMA channels\n");
> >> + INIT_LIST_HEAD(&dmadev->ddev.channels);
> >> + rc = hidma_chan_init(dmadev, 0);
> >> + if (rc) {
> >> + dev_err(&pdev->dev, "probe:channel init failed\n");
> >> + goto channel_init_failed;
> >> + }
> >> + dev_dbg(&pdev->dev, "HI-DMA engine driver starting self test\n");
> >> + rc = dmadev->self_test(dmadev);
> >> + if (rc) {
> >> + dev_err(&pdev->dev, "probe: self test failed: %d\n", rc);
> >> + goto self_test_failed;
> >> + }
> >> + dev_info(&pdev->dev, "probe: self test succeeded.\n");
> >> +
> >> + dev_dbg(&pdev->dev, "calling dma_async_device_register\n");
> >> + rc = dma_async_device_register(&dmadev->ddev);
> >> + if (rc) {
> >> + dev_err(&pdev->dev,
> >> + "probe: failed to register slave DMA: %d\n", rc);
> >> + goto device_register_failed;
> >> + }
> >> + dev_dbg(&pdev->dev, "probe: dma_async_device_register done\n");
> >> +
> >> + rc = hidma_debug_init(dmadev);
> >> + if (rc) {
> >> + dev_err(&pdev->dev,
> >> + "probe: failed to init debugfs: %d\n", rc);
> >> + goto debug_init_failed;
> >> + }
> >> +
> >> + dev_info(&pdev->dev, "HI-DMA engine driver registration complete\n");
> >> + platform_set_drvdata(pdev, dmadev);
> >> + HIDMA_RUNTIME_SET(dmadev);
> >> + return 0;
> >
> > Remove the debug prints when you are done debugging.
>
> errors or the dev_infos?
I would remove both. Some might still be useful, but generally you only
want to print messages when something happens that is not triggered by
user action.
>
> > Also, if you waste CPU cycles for hundreds of milliseconds, it's unlikely
> > that the function is so performance critical that it requires writel_relaxed().
> >
> > Just use writel() here.
>
> The issue is not writel_relaxed vs. writel. After I issue reset, I need
> wait for some time to confirm reset was done. I can use readl_polling
> instead of mdelay if we don't like mdelay.
I meant that both _relaxed() and mdelay() are probably wrong here.
readl_polling() would avoid the part with _relaxed(), but if that can
still take more than a few microseconds, you should try to sleep inbetween
rather than burn CPU cycles.
> >> +/*
> >> + * The interrupt handler for HIDMA will try to consume as many pending
> >> + * EVRE from the event queue as possible. Each EVRE has an associated
> >> + * TRE that holds the user interface parameters. EVRE reports the
> >> + * result of the transaction. Hardware guarantees ordering between EVREs
> >> + * and TREs. We use last processed offset to figure out which TRE is
> >> + * associated with which EVRE. If two TREs are consumed by HW, the EVREs
> >> + * are in order in the event ring.
> >> + * This handler will do a one pass for consuming EVREs. Other EVREs may
> >> + * be delivered while we are working. It will try to consume incoming
> >> + * EVREs one more time and return.
> >> + * For unprocessed EVREs, hardware will trigger another interrupt until
> >> + * all the interrupt bits are cleared.
> >> + */
> >> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
> >> +{
> >> + u32 status;
> >> + u32 enable;
> >> + u32 cause;
> >> + int repeat = 2;
> >> + unsigned long timeout;
> >> +
> >> + status = readl_relaxed(lldev->evca + EVCA_IRQ_STAT_OFFSET);
> >> + enable = readl_relaxed(lldev->evca + EVCA_IRQ_EN_OFFSET);
> >> + cause = status & enable;
> >> +
> >
> > Reading the status probably requires a readl() rather than readl_relaxed()
> > to guarantee that the DMA data has arrived in memory by the time that the
> > register data is seen by the CPU. If using readl_relaxed() here is a valid
> > and required optimization, please add a comment to explain why it works
> > and how much you gain.
>
> I will add some description. This is a high speed peripheral. I don't
> like spreading barriers as candies inside the readl and writel unless I
> have to.
>
> According to the barriers video, I watched on youtube this should be the
> rule for ordering.
>
> "if you do two relaxed reads and check the results of the returned
> variables, ARM architecture guarantees that these two relaxed variables
> will get observed during the check."
>
> this is called implied ordering or something of that sort.
My point was a bit different: while it is guaranteed that the
result of the readl_relaxed() is observed in order, they do not
guarantee that a DMA from device to memory that was started by
the device before the readl_relaxed() has arrived in memory
by the time that the readl_relaxed() result is visible to the
CPU and it starts accessing the memory.
In other words, when the hardware sends you data followed by an
interrupt to tell you the data is there, your interrupt handler
can tell the driver that is waiting for this data that the DMA
is complete while the data itself is still in flight, e.g. waiting
for an IOMMU to fetch page table entries.
> >> + wmb();
> >> +
> >> + mdelay(1);
> >
> > Another workqueue? You should basically never call mdelay().
>
> I'll use polled read instead.
Ok, but again make sure that you call msleep() or usleep_range()
between the reads.
> >> +static int hidma_ll_hw_start(void *llhndl)
> >> +{
> >> + int rc = 0;
> >> + struct hidma_lldev *lldev = llhndl;
> >> + unsigned long irqflags;
> >> +
> >> + spin_lock_irqsave(&lldev->lock, irqflags);
> >> + writel_relaxed(lldev->tre_write_offset,
> >> + lldev->trca + TRCA_DOORBELL_OFFSET);
> >> + spin_unlock_irqrestore(&lldev->lock, irqflags);
> >
> > How does this work? The writel_relaxed() won't synchronize with either
> > the DMA data or the spinlock.
>
> mutex and spinlocks have barriers inside. See the youtube video.
>
> https://www.youtube.com/watch?v=6ORn6_35kKo
I'm pretty sure these barriers only make sense to the CPU, so the
spinlock guarantees that the access to lldev->tre_write_offset is
protected, but not the access to lldev->trca, because that write
is posted on the bus and might not complete until after the
unlock. There is no "dsb(st)" in here:
static inline void arch_spin_unlock(arch_spinlock_t *lock)
{
unsigned long tmp;
asm volatile(ARM64_LSE_ATOMIC_INSN(
/* LL/SC */
" ldrh %w1, %0\n"
" add %w1, %w1, #1\n"
" stlrh %w1, %0",
/* LSE atomics */
" mov %w1, #1\n"
" nop\n"
" staddlh %w1, %0")
: "=Q" (lock->owner), "=&r" (tmp)
:
: "memory");
}
> >
> > Also, your abstraction seem to go a little too far if the upper driver
> > doesn't know what the lower driver calls its main device structure.
> >
> > Or you can go further and just embed the struct hidma_lldev within the
> > struct hidma_dev to save one?
>
> That's how it was before. It got too complex and variables/spinlocks got
> intermixed. I borrowed the upper layer and it worked as it is. I rather
> keep all hardware stuff in another file and do not mix and match for safety.
Ok, then just use a forward declaration for the struct name so you can
have a type-safe pointer but don't need to show the members.
> >> +void hidma_ll_chstats(struct seq_file *s, void *llhndl, u32 tre_ch)
> >> +{
> >> + struct hidma_lldev *lldev = llhndl;
> >> + struct hidma_tre *tre;
> >> + u32 length;
> >> + dma_addr_t src_start;
> >> + dma_addr_t dest_start;
> >> + u32 *tre_local;
> >> +
> >> + if (unlikely(tre_ch >= lldev->nr_tres)) {
> >> + dev_err(lldev->dev, "invalid TRE number in chstats:%d",
> >> + tre_ch);
> >> + return;
> >> + }
> >> + tre = &lldev->trepool[tre_ch];
> >> + seq_printf(s, "------Channel %d -----\n", tre_ch);
> >> + seq_printf(s, "allocated=%d\n", atomic_read(&tre->allocated));
> >> + HIDMA_CHAN_SHOW(tre, queued);
> >> + seq_printf(s, "err_info=0x%x\n",
> >> + lldev->tx_status_list[tre->chidx].err_info);
> >> + seq_printf(s, "err_code=0x%x\n",
> >> + lldev->tx_status_list[tre->chidx].err_code);
> >> + HIDMA_CHAN_SHOW(tre, status);
> >> + HIDMA_CHAN_SHOW(tre, chidx);
> >> + HIDMA_CHAN_SHOW(tre, dma_sig);
> >> + seq_printf(s, "dev_name=%s\n", tre->dev_name);
> >> + seq_printf(s, "callback=%p\n", tre->callback);
> >> + seq_printf(s, "data=%p\n", tre->data);
> >> + HIDMA_CHAN_SHOW(tre, tre_index);
> >> +
> >> + tre_local = &tre->tre_local[0];
> >> + src_start = tre_local[TRE_SRC_LOW_IDX];
> >> + src_start = ((u64)(tre_local[TRE_SRC_HI_IDX]) << 32) + src_start;
> >> + dest_start = tre_local[TRE_DEST_LOW_IDX];
> >> + dest_start += ((u64)(tre_local[TRE_DEST_HI_IDX]) << 32);
> >> + length = tre_local[TRE_LEN_IDX];
> >> +
> >> + seq_printf(s, "src=%pap\n", &src_start);
> >> + seq_printf(s, "dest=%pap\n", &dest_start);
> >> + seq_printf(s, "length=0x%x\n", length);
> >> +}
> >> +EXPORT_SYMBOL_GPL(hidma_ll_chstats);
> >
> > Remove all the pointers here. I guess you can remove the entire debugfs
> > file really ;-)
>
> ok, I need some facility to print out stuff when problems happened.
> Would you rather use sysfs?
sysfs would be less appropriate, as that requires providing a stable ABI
for user space. I think ftrace should provide what you need. Let me know
if that doesn't work out.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2015-10-31 03:00 +0100 |
| Subject | Re: [PATCH 2/2] dma: add Qualcomm Technologies HIDMA channel driver |
| Message-ID | <qptCx-4ZD-3@gated-at.bofh.it> |
| In reply to | #1259832 |
On 10/30/2015 6:28 PM, Arnd Bergmann wrote: > I missed that part. If the descriptor count is a hardware feature, > just make that property mandatory. OTOH, if this is an optimization > setting, better drop that property entirely. I'm going to make this a module parameter instead and get rid of the constant. The reason, I have this default parameter today is that QEMU does not support passing device tree arguments for platform devices. QEMU allows you to set memory and interrupt resources only for platform devices. At least now, I can pass the argument via command line before starting QEMU. I'll put checks that the value needs to come from either DTS/ACPI/command line. -- Sinan Kaya Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
Page 1 of 3 [1] 2 3 Next page →
Back to top | Article view | linux.kernel
csiph-web