Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1389699 > unrolled thread

[PATCH V5 0/6] Introduce framework for SLIMbus device drivers

Back to article view | Back to linux.kernel

Contents

  [PATCH V5 0/6] Introduce framework for SLIMbus device drivers Sagar Dharia <sdharia@codeaurora.org> - 2016-04-28 02:00 +0200
    [PATCH V5 3/6] slimbus: Add messaging APIs to slimbus framework Sagar Dharia <sdharia@codeaurora.org> - 2016-04-28 02:00 +0200
      Re: [PATCH V5 3/6] slimbus: Add messaging APIs to slimbus framework Arnd Bergmann <arnd@arndb.de> - 2016-04-28 12:00 +0200
    [PATCH V5 6/6] slim: qcom: Add runtime-pm support using clock-pause feature Sagar Dharia <sdharia@codeaurora.org> - 2016-04-28 02:10 +0200
    Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus Arnd Bergmann <arnd@arndb.de> - 2016-04-28 12:10 +0200
      Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus Mark Brown <broonie@kernel.org> - 2016-04-28 14:00 +0200
        Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus Arnd Bergmann <arnd@arndb.de> - 2016-04-28 14:40 +0200
          Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus Mark Brown <broonie@kernel.org> - 2016-04-28 16:40 +0200
            Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus Arnd Bergmann <arnd@arndb.de> - 2016-04-28 17:10 +0200
              Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus Mark Brown <broonie@kernel.org> - 2016-04-28 18:50 +0200
                Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus Arnd Bergmann <arnd@arndb.de> - 2016-04-28 19:00 +0200
                  Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus Mark Brown <broonie@kernel.org> - 2016-04-29 13:20 +0200

#1389699 — [PATCH V5 0/6] Introduce framework for SLIMbus device drivers

SLIMbus (Serial Low Power Interchip Media Bus) is a specification
developed by MIPI (Mobile Industry Processor Interface) alliance.
SLIMbus is a 2-wire implementation, which is used to communicate with
peripheral components like audio-codec.
SLIMbus uses Time-Division-Multiplexing to accommodate multiple data
channels, and control channel. Control channel has messages to do
device-enumeration, messages to send/receive control-data to/from
slimbus devices, messages for port/channel management, and messages to
do bandwidth allocation.
Framework is introduced to support  multiple instances of the bus
(1 controller per bus), and multiple slave devices per controller.
SPI and I2C frameworks, and comments from last time when I submitted
the patches were referred-to while working on this framework.

These patchsets introduce device-management, OF helpers, and messaging
APIs, controller driver for Qualcomm's slimbus controller, and
clock-pause feature for entering/exiting low-power mode for SLIMbus.
Framework patches to do channel, port and bandwidth
management are work-in-progress and will be sent out once these
initial patches are accepted.

These patchsets were tested on Qualcomm Snapdragon processor board
using the controller driver, and a test slave device.

Changes from V4 to V5:
* Addressed inline-code review comments from Mark Brown and Rob Herring.
* Comments to document usage of workqueue while sending device-up/down
  notifications to slave drivers.
* Introduced module_slimbus_driver macro for ease of registering and
  de-registering slimbus client driver.
* Modified framework and controller remove functionality and tested
  bind/unbind for slimbus controller and slimbus device drivers.
* Modified Device Tree compatible string to follow format from other
  discoverable buses, and clarified documentation about when node
  definition and compatible fields should be used.
* Clock-pause initiated by the framework when controller is being
  removed to make sure there are no ongoing transfers.

Sagar Dharia (6):
  SLIMbus: Device management on SLIMbus
  of/slimbus: OF helper for SLIMbus
  slimbus: Add messaging APIs to slimbus framework
  slim: qcom: Add Qualcomm Slimbus controller driver
  slimbus: Add support for 'clock-pause' feature
  slim: qcom: Add runtime-pm support using clock-pause feature

 Documentation/devicetree/bindings/slimbus/bus.txt  |  55 ++
 .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  45 ++
 Documentation/slimbus/summary                      | 109 +++
 drivers/Kconfig                                    |   2 +
 drivers/Makefile                                   |   1 +
 drivers/slimbus/Kconfig                            |  20 +
 drivers/slimbus/Makefile                           |   5 +
 drivers/slimbus/slim-core.c                        | 806 +++++++++++++++++++++
 drivers/slimbus/slim-messaging.c                   | 433 +++++++++++
 drivers/slimbus/slim-qcom-ctrl.c                   | 683 +++++++++++++++++
 drivers/slimbus/slim-qcom.h                        |  64 ++
 drivers/slimbus/slim-sched.c                       | 126 ++++
 include/linux/mod_devicetable.h                    |  13 +
 include/linux/slimbus.h                            | 685 +++++++++++++++++
 14 files changed, 3047 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/slimbus/bus.txt
 create mode 100644 Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
 create mode 100644 Documentation/slimbus/summary
 create mode 100644 drivers/slimbus/Kconfig
 create mode 100644 drivers/slimbus/Makefile
 create mode 100644 drivers/slimbus/slim-core.c
 create mode 100644 drivers/slimbus/slim-messaging.c
 create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
 create mode 100644 drivers/slimbus/slim-qcom.h
 create mode 100644 drivers/slimbus/slim-sched.c
 create mode 100644 include/linux/slimbus.h

-- 
1.8.2.1

[toc] | [next] | [standalone]

#1389702 — [PATCH V5 3/6] slimbus: Add messaging APIs to slimbus framework

Slimbus devices use value-element, and information elements to
control device parameters (e.g. value element is used to represent
gain for codec, information element is used to represent interrupt
status for codec when codec interrupt fires).
Messaging APIs are used to set/get these value and information
elements. Slimbus specification uses 8-bit "transaction IDs" for
messages where a read-value is anticipated. Framework uses a table
of pointers to store those TIDs and responds back to the caller in
O(1).
Caller can opt to do synchronous, or asynchronous reads/writes. For
asynchronous operations, the callback will be called from atomic
context.
TX and RX circular rings are used to allow queuing of multiple
transfers per controller. Controller can choose size of these rings
based of controller HW implementation. The buffers are coerently
mapped so that controller can utilize DMA operations for the
transactions without remapping every transaction buffer.
Statically allocated rings help to improve performance by avoiding
overhead of dynamically allocating transactions on need basis.

Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
Tested-by: Naveen Kaje <nkaje@codeaurora.org>
---
 drivers/slimbus/Makefile         |   2 +-
 drivers/slimbus/slim-core.c      |  50 +++++
 drivers/slimbus/slim-messaging.c | 395 +++++++++++++++++++++++++++++++++++++++
 include/linux/slimbus.h          | 224 ++++++++++++++++++++++
 4 files changed, 670 insertions(+), 1 deletion(-)
 create mode 100644 drivers/slimbus/slim-messaging.c

diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
index aa9c147..d95a0a2 100644
--- a/drivers/slimbus/Makefile
+++ b/drivers/slimbus/Makefile
@@ -1,4 +1,4 @@
 #
 # Makefile for kernel slimbus framework.
 #
-obj-$(CONFIG_SLIMBUS)			+= slim-core.o
+obj-$(CONFIG_SLIMBUS)			+= slim-core.o slim-messaging.o
diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c
index 6aaa08f..7d8b71c 100644
--- a/drivers/slimbus/slim-core.c
+++ b/drivers/slimbus/slim-core.c
@@ -17,6 +17,7 @@
 #include <linux/completion.h>
 #include <linux/idr.h>
 #include <linux/pm_runtime.h>
+#include <linux/dma-mapping.h>
 #include <linux/slimbus.h>
 #include <linux/of.h>
 
@@ -225,6 +226,16 @@ static void slim_dev_release(struct device *dev)
 	kfree(sbdev);
 }
 
+static void slim_ctrl_release(struct device *dev)
+{
+	struct slim_controller *ctrl = to_slim_controller(dev);
+
+	dma_free_coherent(dev->parent, (ctrl->rx.sl_sz * ctrl->rx.n),
+			  ctrl->rx.base, ctrl->rx.phy);
+	dma_free_coherent(dev->parent, (ctrl->tx.sl_sz * ctrl->tx.n),
+			  ctrl->tx.base, ctrl->tx.phy);
+}
+
 /**
  * slim_add_device: Add a new device without register board info.
  * @ctrl: Controller to which this device is to be added to.
@@ -398,6 +409,7 @@ int slim_register_controller(struct slim_controller *ctrl)
 
 	ctrl->nr = id;
 
+	ctrl->dev.release = slim_ctrl_release;
 	dev_set_name(&ctrl->dev, "sb-%d", ctrl->nr);
 	ctrl->num_dev = 0;
 
@@ -407,6 +419,35 @@ int slim_register_controller(struct slim_controller *ctrl)
 		ctrl->max_cg = SLIM_MAX_CLK_GEAR;
 
 	mutex_init(&ctrl->m_ctrl);
+	spin_lock_init(&ctrl->tx.lock);
+	spin_lock_init(&ctrl->rx.lock);
+
+	ctrl->pending_wr = kcalloc((ctrl->tx.n - 1),
+				   sizeof(struct slim_pending),
+				   GFP_KERNEL);
+	if (!ctrl->pending_wr) {
+		ret = -ENOMEM;
+		goto wr_alloc_failed;
+	}
+
+	sema_init(&ctrl->tx_sem, (ctrl->tx.n - 1));
+
+	ctrl->tx.base = dma_alloc_coherent(ctrl->dev.parent,
+					   (ctrl->tx.sl_sz * ctrl->tx.n),
+					   &ctrl->tx.phy, GFP_KERNEL);
+	if (!ctrl->tx.base) {
+		ret = -ENOMEM;
+		goto tx_alloc_failed;
+	}
+
+	ctrl->rx.base = dma_alloc_coherent(ctrl->dev.parent,
+					   (ctrl->rx.sl_sz * ctrl->rx.n),
+					   &ctrl->rx.phy, GFP_KERNEL);
+	if (!ctrl->rx.base) {
+		ret = -ENOMEM;
+		goto rx_alloc_failed;
+	}
+
 	ret = device_register(&ctrl->dev);
 	if (ret)
 		goto dev_reg_failed;
@@ -424,9 +465,18 @@ int slim_register_controller(struct slim_controller *ctrl)
 err_workq_failed:
 	device_unregister(&ctrl->dev);
 dev_reg_failed:
+	dma_free_coherent(ctrl->dev.parent, (ctrl->rx.sl_sz * ctrl->rx.n),
+			  ctrl->rx.base, ctrl->rx.phy);
+rx_alloc_failed:
+	dma_free_coherent(ctrl->dev.parent, (ctrl->tx.sl_sz * ctrl->tx.n),
+			  ctrl->tx.base, ctrl->tx.phy);
+tx_alloc_failed:
+	kfree(ctrl->pending_wr);
+wr_alloc_failed:
 	mutex_lock(&slim_lock);
 	idr_remove(&ctrl_idr, ctrl->nr);
 	mutex_unlock(&slim_lock);
+	dev_err(&ctrl->dev, "slimbus controller registration failed:%d", ret);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(slim_register_controller);
diff --git a/drivers/slimbus/slim-messaging.c b/drivers/slimbus/slim-messaging.c
new file mode 100644
index 0000000..183742d
--- /dev/null
+++ b/drivers/slimbus/slim-messaging.c
@@ -0,0 +1,395 @@
+/* Copyright (c) 2011-2016, 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/slab.h>
+#include <linux/slimbus.h>
+
+/**
+ * slim_msg_response: Deliver Message response received from a device to the
+ *	framework.
+ * @ctrl: Controller handle
+ * @reply: Reply received from the device
+ * @len: Length of the reply
+ * @tid: Transaction ID received with which framework can associate reply.
+ * Called by controller to inform framework about the response received.
+ * This helps in making the API asynchronous, and controller-driver doesn't need
+ * to manage 1 more table other than the one managed by framework mapping TID
+ * with buffers
+ */
+void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len)
+{
+	struct slim_val_inf *msg;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctrl->txn_lock, flags);
+	msg = ctrl->tid_tbl[tid];
+	if (msg == NULL || msg->rbuf == NULL) {
+		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
+		dev_err(&ctrl->dev, "Got response to invalid TID:%d, len:%d\n",
+				tid, len);
+		return;
+	}
+	ctrl->tid_tbl[tid] = NULL;
+	spin_unlock_irqrestore(&ctrl->txn_lock, flags);
+
+	memcpy(msg->rbuf, reply, len);
+	if (msg->comp_cb)
+		msg->comp_cb(msg->ctx, 0);
+}
+EXPORT_SYMBOL_GPL(slim_msg_response);
+
+struct slim_cb_data {
+	struct completion *comp;
+	int ret;
+};
+
+static void slim_sync_default_cb(void *ctx, int err)
+{
+	struct slim_cb_data *cbd = ctx;
+
+	cbd->ret = err;
+	complete(cbd->comp);
+}
+
+int slim_processtxn(struct slim_controller *ctrl,
+				struct slim_msg_txn *txn)
+{
+	int ret, i = 0;
+	unsigned long flags;
+	u8 *buf;
+	bool async = false;
+	struct slim_cb_data cbd;
+	DECLARE_COMPLETION_ONSTACK(done);
+	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
+
+	if (!txn->msg->comp_cb) {
+		txn->msg->comp_cb = slim_sync_default_cb;
+		cbd.comp = &done;
+		txn->msg->ctx = &cbd;
+	} else {
+		async = true;
+	}
+
+	buf = slim_get_tx(ctrl, txn, need_tid);
+	if (!buf)
+		return -ENOMEM;
+
+	if (need_tid) {
+		spin_lock_irqsave(&ctrl->txn_lock, flags);
+		for (i = 0; i < ctrl->last_tid; i++) {
+			if (ctrl->tid_tbl[i] == NULL)
+				break;
+		}
+		if (i >= ctrl->last_tid) {
+			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
+				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
+				slim_return_tx(ctrl, -ENOMEM);
+				return -ENOMEM;
+			}
+			ctrl->last_tid++;
+		}
+		ctrl->tid_tbl[i] = txn->msg;
+		txn->tid = i;
+		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
+	}
+
+	ret = ctrl->xfer_msg(ctrl, txn, buf);
+
+	if (!ret && !async) { /* sync transaction */
+		/* Fine-tune calculation after bandwidth management */
+		unsigned long ms = txn->rl + 100;
+
+		ret = wait_for_completion_timeout(&done,
+						  msecs_to_jiffies(ms));
+		if (!ret)
+			slim_return_tx(ctrl, -ETIMEDOUT);
+
+		ret = cbd.ret;
+	}
+
+	if (ret && need_tid) {
+		spin_lock_irqsave(&ctrl->txn_lock, flags);
+		/* Invalidate the transaction */
+		ctrl->tid_tbl[txn->tid] = NULL;
+		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
+	}
+	if (ret)
+		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
+			txn->mt, txn->mc, txn->la, ret);
+	if (!async) {
+		txn->msg->comp_cb = NULL;
+		txn->msg->ctx = NULL;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(slim_processtxn);
+
+static int slim_val_inf_sanity(struct slim_controller *ctrl,
+			       struct slim_val_inf *msg, u8 mc)
+{
+	if (!msg || msg->num_bytes > 16 ||
+	    (msg->start_offset + msg->num_bytes) > 0xC00)
+		goto reterr;
+	switch (mc) {
+	case SLIM_MSG_MC_REQUEST_VALUE:
+	case SLIM_MSG_MC_REQUEST_INFORMATION:
+		if (msg->rbuf != NULL)
+			return 0;
+		break;
+	case SLIM_MSG_MC_CHANGE_VALUE:
+	case SLIM_MSG_MC_CLEAR_INFORMATION:
+		if (msg->wbuf != NULL)
+			return 0;
+		break;
+	case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
+	case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
+		if (msg->rbuf != NULL && msg->wbuf != NULL)
+			return 0;
+		break;
+	default:
+		break;
+	}
+reterr:
+	dev_err(&ctrl->dev, "Sanity check failed:msg:offset:0x%x, mc:%d\n",
+		msg->start_offset, mc);
+	return -EINVAL;
+}
+
+static u16 slim_slicecodefromsize(u16 req)
+{
+	static const u8 codetosize[8] = {1, 2, 3, 4, 6, 8, 12, 16};
+
+	if (req >= ARRAY_SIZE(codetosize))
+		return 0;
+	else
+		return codetosize[req];
+}
+
+static u16 slim_slicesize(int code)
+{
+	static const u8 sizetocode[16] = {
+		0, 1, 2, 3, 3, 4, 4, 5, 5, 5, 5, 6, 6, 6, 6, 7
+	};
+
+	clamp(code, 1, (int)ARRAY_SIZE(sizetocode));
+	return sizetocode[code - 1];
+}
+
+int slim_xfer_msg(struct slim_controller *ctrl,
+			struct slim_device *sbdev, struct slim_val_inf *msg,
+			u8 mc)
+{
+	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
+	struct slim_msg_txn *txn = &txn_stack;
+	int ret;
+	u16 sl, cur;
+
+	ret = slim_val_inf_sanity(ctrl, msg, mc);
+	if (ret)
+		return ret;
+
+	sl = slim_slicesize(msg->num_bytes);
+
+	dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
+		msg->start_offset, msg->num_bytes, mc, sl);
+
+	cur = slim_slicecodefromsize(sl);
+	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
+
+	switch (mc) {
+	case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
+	case SLIM_MSG_MC_CHANGE_VALUE:
+	case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
+	case SLIM_MSG_MC_CLEAR_INFORMATION:
+		txn->rl += msg->num_bytes;
+	default:
+		break;
+	}
+
+	if (slim_tid_txn(txn->mt, txn->mc))
+		txn->rl++;
+
+	return slim_processtxn(ctrl, txn);
+}
+EXPORT_SYMBOL_GPL(slim_xfer_msg);
+
+/* Message APIs Unicast message APIs used by slimbus slave drivers */
+
+/*
+ * Message API access routines.
+ * @sb: client handle requesting elemental message reads, writes.
+ * @msg: Input structure for start-offset, number of bytes to read.
+ * context: can sleep
+ * Returns:
+ * -EINVAL: Invalid parameters
+ * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
+ *	not being clocked or driven by controller)
+ * -ENOTCONN: If the transmitted message was not ACKed by destination device.
+ */
+int slim_request_val_element(struct slim_device *sb,
+				struct slim_val_inf *msg)
+{
+	struct slim_controller *ctrl = sb->ctrl;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_VALUE);
+}
+EXPORT_SYMBOL_GPL(slim_request_val_element);
+
+int slim_request_inf_element(struct slim_device *sb,
+				struct slim_val_inf *msg)
+{
+	struct slim_controller *ctrl = sb->ctrl;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_INFORMATION);
+}
+EXPORT_SYMBOL_GPL(slim_request_inf_element);
+
+int slim_change_val_element(struct slim_device *sb, struct slim_val_inf *msg)
+{
+	struct slim_controller *ctrl = sb->ctrl;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_CHANGE_VALUE);
+}
+EXPORT_SYMBOL_GPL(slim_change_val_element);
+
+int slim_clear_inf_element(struct slim_device *sb, struct slim_val_inf *msg)
+{
+	struct slim_controller *ctrl = sb->ctrl;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_CLEAR_INFORMATION);
+}
+EXPORT_SYMBOL_GPL(slim_clear_inf_element);
+
+int slim_request_change_val_element(struct slim_device *sb,
+					struct slim_val_inf *msg)
+{
+	struct slim_controller *ctrl = sb->ctrl;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_CHANGE_VALUE);
+}
+EXPORT_SYMBOL_GPL(slim_request_change_val_element);
+
+int slim_request_clear_inf_element(struct slim_device *sb,
+					struct slim_val_inf *msg)
+{
+	struct slim_controller *ctrl = sb->ctrl;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	return slim_xfer_msg(ctrl, sb, msg,
+					SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION);
+}
+EXPORT_SYMBOL_GPL(slim_request_clear_inf_element);
+
+/* Functions to get/return TX, RX buffers for messaging. */
+
+void *slim_get_rx(struct slim_controller *ctrl)
+{
+	unsigned long flags;
+	int idx;
+
+	spin_lock_irqsave(&ctrl->rx.lock, flags);
+	if ((ctrl->rx.tail + 1) % ctrl->rx.n == ctrl->rx.head) {
+		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
+		dev_err(&ctrl->dev, "RX QUEUE full!");
+		return NULL;
+	}
+	idx = ctrl->rx.tail;
+	ctrl->rx.tail = (ctrl->rx.tail + 1) % ctrl->rx.n;
+	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
+
+	return ctrl->rx.base + (idx * ctrl->rx.sl_sz);
+}
+EXPORT_SYMBOL_GPL(slim_get_rx);
+
+int slim_return_rx(struct slim_controller *ctrl, void *buf)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctrl->rx.lock, flags);
+	if (ctrl->rx.tail == ctrl->rx.head) {
+		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
+		return -ENODATA;
+	}
+	memcpy(buf, ctrl->rx.base + (ctrl->rx.head * ctrl->rx.sl_sz),
+				ctrl->rx.sl_sz);
+	ctrl->rx.head = (ctrl->rx.head + 1) % ctrl->rx.n;
+	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(slim_return_rx);
+
+void slim_return_tx(struct slim_controller *ctrl, int err)
+{
+	unsigned long flags;
+	int idx;
+	struct slim_pending cur;
+
+	spin_lock_irqsave(&ctrl->tx.lock, flags);
+	idx = ctrl->tx.head;
+	ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
+	cur = ctrl->pending_wr[idx];
+	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
+
+	if (!cur.cb)
+		dev_err(&ctrl->dev, "NULL Transaction or completion");
+	else
+		cur.cb(cur.ctx, err);
+
+	up(&ctrl->tx_sem);
+}
+EXPORT_SYMBOL_GPL(slim_return_tx);
+
+void *slim_get_tx(struct slim_controller *ctrl, struct slim_msg_txn *txn,
+		bool need_tid)
+{
+	unsigned long flags;
+	int ret, idx;
+
+	ret = down_interruptible(&ctrl->tx_sem);
+	if (ret < 0) {
+		dev_err(&ctrl->dev, "TX semaphore down returned:%d", ret);
+		return NULL;
+	}
+	spin_lock_irqsave(&ctrl->tx.lock, flags);
+	if (((ctrl->tx.head + 1) % ctrl->tx.n) == ctrl->tx.tail) {
+		spin_unlock_irqrestore(&ctrl->tx.lock, flags);
+		dev_err(&ctrl->dev, "controller TX buf unavailable");
+		up(&ctrl->tx_sem);
+		return NULL;
+	}
+	idx = ctrl->tx.tail;
+	ctrl->tx.tail = (ctrl->tx.tail + 1) % ctrl->tx.n;
+	ctrl->pending_wr[idx].cb = txn->msg->comp_cb;
+	ctrl->pending_wr[idx].ctx = txn->msg->ctx;
+	ctrl->pending_wr[idx].need_tid = need_tid;
+	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
+
+	return ctrl->tx.base + (idx * ctrl->tx.sl_sz);
+}
+EXPORT_SYMBOL_GPL(slim_get_tx);
diff --git a/include/linux/slimbus.h b/include/linux/slimbus.h
index e076e25..63d79c2 100644
--- a/include/linux/slimbus.h
+++ b/include/linux/slimbus.h
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/mutex.h>
+#include <linux/semaphore.h>
 #include <linux/mod_devicetable.h>
 
 /**
@@ -34,6 +35,9 @@ extern struct bus_type slimbus_type;
 #define SLIM_FRM_SLOTS_PER_SUPERFRAME	16
 #define SLIM_GDE_SLOTS_PER_SUPERFRAME	2
 
+/* MAX in-flight transactions neededing transaction ID (8-bit, per spec) */
+#define SLIM_MAX_TIDS			256
+
 struct slim_controller;
 struct slim_device;
 
@@ -100,12 +104,115 @@ struct slim_addrt {
 #define SLIM_MSG_MC_ASSIGN_LOGICAL_ADDRESS       0x2
 #define SLIM_MSG_MC_REPORT_ABSENT                0xF
 
+/* Information Element management messages */
+#define SLIM_MSG_MC_REQUEST_INFORMATION          0x20
+#define SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION    0x21
+#define SLIM_MSG_MC_REPLY_INFORMATION            0x24
+#define SLIM_MSG_MC_CLEAR_INFORMATION            0x28
+#define SLIM_MSG_MC_REPORT_INFORMATION           0x29
+
+/* Value Element management messages */
+#define SLIM_MSG_MC_REQUEST_VALUE                0x60
+#define SLIM_MSG_MC_REQUEST_CHANGE_VALUE         0x61
+#define SLIM_MSG_MC_REPLY_VALUE                  0x64
+#define SLIM_MSG_MC_CHANGE_VALUE                 0x68
+
 /* Destination type Values */
 #define SLIM_MSG_DEST_LOGICALADDR	0
 #define SLIM_MSG_DEST_ENUMADDR		1
 #define	SLIM_MSG_DEST_BROADCAST		3
 
 /**
+ * struct slim_val_inf: Slimbus value or information element
+ * @start_offset: Specifies starting offset in information/value element map
+ * @num_bytes: upto 16. This ensures that the message will fit the slicesize
+ *		per slimbus spec
+ * @comp_cb: Callback if this read/write is asynchronous
+ * @ctx: Argument for comp_cb
+ */
+struct slim_val_inf {
+	u16			start_offset;
+	u8			num_bytes;
+	u8			*rbuf;
+	const u8		*wbuf;
+	void			(*comp_cb)(void *ctx, int err);
+	void			*ctx;
+};
+
+/**
+ * struct slim_msg_txn: Message to be sent by the controller.
+ * This structure has packet header, payload and buffer to be filled (if any)
+ * @rl: Header field. remaining length.
+ * @mt: Header field. Message type.
+ * @mc: Header field. LSB is message code for type mt.
+ * @dt: Header field. Destination type.
+ * @ec: Element code. Used for elemental access APIs.
+ * @len: Length of payload. (excludes ec)
+ * @tid: Transaction ID. Used for messages expecting response.
+ *	(relevant for message-codes involving read operation)
+ * @la: Logical address of the device this message is going to.
+ *	(Not used when destination type is broadcast.)
+ * @msg: Elemental access message to be read/written
+ */
+struct slim_msg_txn {
+	u8			rl;
+	u8			mt;
+	u8			mc;
+	u8			dt;
+	u16			ec;
+	u8			tid;
+	u8			la;
+	struct slim_val_inf	*msg;
+};
+
+/* Frequently used message transaction structures */
+#define DEFINE_SLIM_LDEST_TXN(name, mc, rl, la, msg) \
+	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_LOGICALADDR, 0,\
+					0, la, msg, }
+
+#define DEFINE_SLIM_BCAST_TXN(name, mc, rl, la, msg) \
+	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_BROADCAST, 0,\
+					0, la, msg, }
+
+#define DEFINE_SLIM_EDEST_TXN(name, mc, rl, la, msg) \
+	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_ENUMADDR, 0,\
+					0, la, msg, }
+
+/**
+ * struct slim_ctrl_buf: circular buffer used by contoller for TX, RX
+ * @base: virtual base address for this buffer
+ * @phy: physical address for this buffer (this is useful if controller can
+ *	  DMA the buffers for TX and RX to/from controller hardware
+ * @lock: lock protecting head and tail
+ * @head: index where buffer is returned back
+ * @tail: index from where buffer is consumed
+ * @sl_sz: byte-size of each slot in this buffer
+ * @n:	  number of elements in this circular ring, note that this needs to be
+ *	1 more than actual buffers to allow for one open slot
+ */
+struct slim_ctrl_buf {
+	void		*base;
+	phys_addr_t	phy;
+	spinlock_t	lock;
+	int		head;
+	int		tail;
+	int		sl_sz;
+	int		n;
+};
+
+/**
+ * struct slim_pending: context of pending transfers
+ * @cb: callback for this transfer
+ * @ctx: contex for the callback function
+ * @need_tid: True if this transfer need Transaction ID
+ */
+struct slim_pending {
+	void (*cb)(void *ctx, int err);
+	void *ctx;
+	bool need_tid;
+};
+
+/**
  * struct slim_controller: Controls every instance of SLIMbus
  *				(similar to 'master' on SPI)
  *	'Manager device' is responsible for  device management, bandwidth
@@ -139,6 +246,16 @@ struct slim_addrt {
  * @addrt: Logical address table
  * @num_dev: Number of active slimbus slaves on this bus
  * @wq: Workqueue per controller used to notify devices when they report present
+ * @tid_tbl: Table of transactions having transaction ID
+ * @txn_lock: Lock to protect table of transactions
+ * @rx: RX buffers used by controller to receive messages. Ctrl may receive more
+ *	than 1 message (e.g. multiple report-present messages or messages from
+ *	multiple slaves).
+ * @tx: TX buffers used by controller to transmit messages. Ctrl may have
+ *	ability to send/queue multiple messages to HW at once.
+ * @pending_wr: Pending write transactions to be acknowledged by controller
+ * @tx_sem: Semaphore for available TX buffers for this controller
+ * @last_tid: Last used entry for TID transactions
  * @xfer_msg: Transfer a message on this controller (this can be a broadcast
  *	control/status message like data channel setup, or a unicast message
  *	like value element read/write.
@@ -162,6 +279,15 @@ struct slim_controller {
 	struct slim_addrt	*addrt;
 	u8			num_dev;
 	struct workqueue_struct *wq;
+	struct slim_val_inf	*tid_tbl[SLIM_MAX_TIDS];
+	u8			last_tid;
+	spinlock_t		txn_lock;
+	struct slim_ctrl_buf	tx;
+	struct slim_ctrl_buf	rx;
+	struct slim_pending	*pending_wr;
+	struct semaphore	tx_sem;
+	int			(*xfer_msg)(struct slim_controller *ctrl,
+					    struct slim_msg_txn *tx, void *buf);
 	int			(*set_laddr)(struct slim_controller *ctrl,
 					     struct slim_eaddr *ea, u8 laddr);
 	int			(*get_laddr)(struct slim_controller *ctrl,
@@ -391,4 +517,102 @@ static inline void slim_set_clientdata(struct slim_device *dev, void *data)
 	dev_set_drvdata(&dev->dev, data);
 }
 
+/* Message APIs Unicast message APIs used by slimbus slave drivers */
+
+/**
+ * Message API access routines for value elements.
+ * @sb: client handle requesting elemental message reads, writes.
+ * @msg: Input structure for start-offset, number of bytes to read.
+ * context: can sleep
+ * Returns:
+ * -EINVAL: Invalid parameters
+ * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
+ *	not being clocked or driven by controller)
+ * -ENOTCONN: If the transmitted message was not ACKed by destination device.
+ */
+int slim_request_val_element(struct slim_device *sb,
+					struct slim_val_inf *msg);
+int slim_change_val_element(struct slim_device *sb,
+					struct slim_val_inf *msg);
+int slim_request_change_val_element(struct slim_device *sb,
+					struct slim_val_inf *msg);
+
+
+/**
+ * Message API access routines for information elements.
+ * @sb: client handle requesting elemental message reads, writes.
+ * @msg: Input structure for start-offset, number of bytes to read
+ *	wbuf will contain information element(s) bit masks to be cleared.
+ *	rbuf will return what the information element value was
+ */
+
+int slim_request_inf_element(struct slim_device *sb,
+					struct slim_val_inf *msg);
+int slim_clear_inf_element(struct slim_device *sb,
+					struct slim_val_inf *msg);
+int slim_request_clear_inf_element(struct slim_device *sb,
+					struct slim_val_inf *msg);
+
+/**
+ * slim_msg_response: Deliver Message response received from a device to the
+ *	framework.
+ * @ctrl: Controller handle
+ * @reply: Reply received from the device
+ * @len: Length of the reply
+ * @tid: Transaction ID received with which framework can associate reply.
+ * Called by controller to inform framework about the response received.
+ * This helps in making the API asynchronous, and controller-driver doesn't need
+ * to manage 1 more table other than the one managed by framework mapping TID
+ * with buffers
+ */
+void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid,
+				u8 len);
+
+/**
+ * slim_processtxn: Process a slimbus-messaging transaction
+ * @ctrl: Controller handle
+ * @txn: Transaction to be sent over SLIMbus
+ * Called by controller to transmit messaging transactions not dealing with
+ * Interface/Value elements. (e.g. transmittting a message to assign logical
+ * address to a slave device
+ * Returns:
+ * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
+ *	not being clocked or driven by controller)
+ * -ENOTCONN: If the transmitted message was not ACKed by destination device.
+ */
+int slim_processtxn(struct slim_controller *ctrl, struct slim_msg_txn *txn);
+
+/**
+ * Functions to get/put TX, RX buffers for messaging.
+ * @ctrl: Controller handle
+ * These functions are called by controller to process the TX/RX buffers.
+ * TX buffer is requested by controller when it's filled-in and sent to the
+ * HW. When HW has finished processing this buffer, controller should return it
+ * back to the pool.
+ * RX buffer is requested by controller when data is received from HW, but is
+ * not processed (e.g. 'report-present message was sent by HW in ISR and SW
+ * needs more time to process the buffer to assign Logical Address)
+ * RX buffer is returned back to the pool when associated RX action
+ * is taken (e.g. Received message is decoded and client's
+ * response buffer is filled in.)
+ */
+void *slim_get_rx(struct slim_controller *ctrl);
+
+int slim_return_rx(struct slim_controller *ctrl, void *buf);
+
+void *slim_get_tx(struct slim_controller *ctrl, struct slim_msg_txn *txn,
+		bool need_tid);
+
+void slim_return_tx(struct slim_controller *ctrl, int err);
+
+static inline bool slim_tid_txn(u8 mt, u8 mc)
+{
+	return (mt == SLIM_MSG_MT_CORE &&
+		(mc == SLIM_MSG_MC_REQUEST_INFORMATION ||
+		 mc == SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION ||
+		 mc == SLIM_MSG_MC_REQUEST_VALUE ||
+		 mc == SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION));
+}
+/* end of message apis */
+
 #endif /* _LINUX_SLIMBUS_H */
-- 
1.8.2.1

[toc] | [prev] | [next] | [standalone]

#1389953 — Re: [PATCH V5 3/6] slimbus: Add messaging APIs to slimbus framework

On Wednesday 27 April 2016 17:58:06 Sagar Dharia wrote:

> +	ctrl->tx.base = dma_alloc_coherent(ctrl->dev.parent,
> +					   (ctrl->tx.sl_sz * ctrl->tx.n),
> +					   &ctrl->tx.phy, GFP_KERNEL);
> +	if (!ctrl->tx.base) {
> +		ret = -ENOMEM;
> +		goto tx_alloc_failed;
> +	}

This looks controller specific, and hints that some parts of the "common"
framework should actually be part of the controller driver instead.

In particular, the use of "ctrl->dev.parent" suggests that this should
have been allocated by the controller.

> @@ -162,6 +279,15 @@ struct slim_controller {
>  	struct slim_addrt	*addrt;
>  	u8			num_dev;
>  	struct workqueue_struct *wq;
> +	struct slim_val_inf	*tid_tbl[SLIM_MAX_TIDS];
> +	u8			last_tid;
> +	spinlock_t		txn_lock;
> +	struct slim_ctrl_buf	tx;
> +	struct slim_ctrl_buf	rx;
> +	struct slim_pending	*pending_wr;
> +	struct semaphore	tx_sem;

Please try to avoid the use of semaphores in new code.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1389704 — [PATCH V5 6/6] slim: qcom: Add runtime-pm support using clock-pause feature

Slimbus HW mandates that clock-pause sequence has to be executed
before disabling relevant interface and core clocks.
Runtime-PM's autosuspend feature is used here to enter/exit low
power mode for Qualcomm's Slimbus controller. Autosuspend feature
enables driver to avoid changing power-modes too frequently since
entering clock-pause is an expensive sequence

Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
---
 drivers/slimbus/slim-qcom-ctrl.c | 128 +++++++++++++++++++++++++++++++++++++--
 drivers/slimbus/slim-qcom.h      |   1 +
 2 files changed, 125 insertions(+), 4 deletions(-)

diff --git a/drivers/slimbus/slim-qcom-ctrl.c b/drivers/slimbus/slim-qcom-ctrl.c
index 3609526..f18c4a7 100644
--- a/drivers/slimbus/slim-qcom-ctrl.c
+++ b/drivers/slimbus/slim-qcom-ctrl.c
@@ -20,6 +20,7 @@
 #include <linux/clk.h>
 #include <linux/of.h>
 #include <linux/slimbus.h>
+#include <linux/pm_runtime.h>
 #include "slim-qcom.h"
 
 #define MSM_SLIM_NAME	"msm_slim_ctrl"
@@ -216,6 +217,30 @@ rx_ret_irq:
 	return ret;
 }
 
+static int msm_clk_pause_wakeup(struct slim_controller *ctrl)
+{
+	struct msm_slim_ctrl *dev = slim_get_ctrldata(ctrl);
+
+	clk_prepare_enable(dev->hclk);
+	clk_prepare_enable(dev->rclk);
+	enable_irq(dev->irq);
+
+	writel_relaxed(1, dev->base + FRM_WAKEUP);
+	/* Make sure framer wakeup write goes through before ISR fires */
+	mb();
+	/**
+	 * HW Workaround: Currently, slave is reporting lost-sync messages
+	 * after slimbus comes out of clock pause.
+	 * Transaction with slave fail before slave reports that message
+	 * Give some time for that report to come
+	 * Slimbus wakes up in clock gear 10 at 24.576MHz. With each superframe
+	 * being 250 usecs, we wait for 5-10 superframes here to ensure
+	 * we get the message
+	 */
+	usleep_range(1250, 2500);
+	return 0;
+}
+
 static int msm_xfer_msg(struct slim_controller *ctrl, struct slim_msg_txn *txn,
 			void *pbuf)
 {
@@ -286,7 +311,6 @@ static int msm_set_laddr(struct slim_controller *ctrl,
 	 */
 	msg.wbuf = buf;
 	msg.num_bytes = 7;
-
 	ret = slim_processtxn(&dev->ctrl, &txn);
 
 	if (ret)
@@ -417,6 +441,8 @@ static int msm_slim_probe(struct platform_device *pdev)
 
 	dev->ctrl.set_laddr = msm_set_laddr;
 	dev->ctrl.xfer_msg = msm_xfer_msg;
+	dev->ctrl.wakeup =  msm_clk_pause_wakeup;
+
 	dev->ctrl.tx.n = MSM_TX_MSGS;
 	dev->ctrl.rx.n = MSM_RX_MSGS;
 	dev->ctrl.tx.sl_sz = SLIM_MSGQ_BUF_LEN;
@@ -514,6 +540,12 @@ static int msm_slim_probe(struct platform_device *pdev)
 	 */
 	mb();
 
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, MSM_SLIM_AUTOSUSPEND);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	dev_dbg(dev->dev, "MSM SB controller is up:ver:0x%x!\n", dev->ver);
 	return 0;
 
@@ -531,14 +563,101 @@ static int msm_slim_remove(struct platform_device *pdev)
 {
 	struct msm_slim_ctrl *dev = platform_get_drvdata(pdev);
 
-	disable_irq(dev->irq);
-	clk_disable_unprepare(dev->rclk);
-	clk_disable_unprepare(dev->hclk);
+	pm_runtime_disable(&pdev->dev);
 	slim_del_controller(&dev->ctrl);
 	destroy_workqueue(dev->rxwq);
 	return 0;
 }
 
+/**
+ * If PM_RUNTIME is not defined, these 2 functions become helper
+ * functions to be called from system suspend/resume.
+ */
+#ifdef CONFIG_PM
+static int msm_slim_runtime_suspend(struct device *device)
+{
+	struct platform_device *pdev = to_platform_device(device);
+	struct msm_slim_ctrl *dev = platform_get_drvdata(pdev);
+	int ret;
+
+	dev_dbg(device, "pm_runtime: suspending...\n");
+	ret = slim_ctrl_clk_pause(&dev->ctrl, false, SLIM_CLK_UNSPECIFIED);
+	if (ret) {
+		dev_err(device, "clk pause not entered:%d", ret);
+	} else {
+		disable_irq(dev->irq);
+		clk_disable_unprepare(dev->hclk);
+		clk_disable_unprepare(dev->rclk);
+	}
+	return ret;
+}
+
+static int msm_slim_runtime_resume(struct device *device)
+{
+	struct platform_device *pdev = to_platform_device(device);
+	struct msm_slim_ctrl *dev = platform_get_drvdata(pdev);
+	int ret = 0;
+
+	dev_dbg(device, "pm_runtime: resuming...\n");
+	ret = slim_ctrl_clk_pause(&dev->ctrl, true, 0);
+	if (ret)
+		dev_err(device, "clk pause not exited:%d", ret);
+	return ret;
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int msm_slim_suspend(struct device *dev)
+{
+	int ret = 0;
+
+	if (!pm_runtime_enabled(dev) ||
+		(!pm_runtime_suspended(dev))) {
+		dev_dbg(dev, "system suspend");
+		ret = msm_slim_runtime_suspend(dev);
+	}
+	if (ret == -EISCONN) {
+		/**
+		* If the clock pause failed due to active channels, there is
+		* a possibility that some audio stream is active during suspend.
+		* (e.g. modem usecase during suspend)
+		* We dont want to return suspend failure in that case so that
+		* display and relevant components can still go to suspend.
+		* If there is some other error, then it should prevent
+		* system level suspend
+		*/
+		ret = 0;
+	}
+	return ret;
+}
+
+static int msm_slim_resume(struct device *dev)
+{
+	if (!pm_runtime_enabled(dev) || !pm_runtime_suspended(dev)) {
+		int ret;
+
+		dev_dbg(dev, "system resume");
+		ret = msm_slim_runtime_resume(dev);
+		if (!ret) {
+			pm_runtime_mark_last_busy(dev);
+			pm_request_autosuspend(dev);
+		}
+		return ret;
+
+	}
+	return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops msm_slim_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(msm_slim_suspend, msm_slim_resume)
+	SET_RUNTIME_PM_OPS(
+			   msm_slim_runtime_suspend,
+			   msm_slim_runtime_resume,
+			   NULL
+	)
+};
+
 static const struct of_device_id msm_slim_dt_match[] = {
 	{
 		.compatible = "qcom,slim-msm",
@@ -553,6 +672,7 @@ static struct platform_driver msm_slim_driver = {
 		.name = MSM_SLIM_NAME,
 		.owner = THIS_MODULE,
 		.of_match_table = msm_slim_dt_match,
+		.pm = &msm_slim_dev_pm_ops,
 	},
 };
 module_platform_driver(msm_slim_driver);
diff --git a/drivers/slimbus/slim-qcom.h b/drivers/slimbus/slim-qcom.h
index 0ad59c3..8b1d649 100644
--- a/drivers/slimbus/slim-qcom.h
+++ b/drivers/slimbus/slim-qcom.h
@@ -23,6 +23,7 @@
 		((l) | ((mt) << 5) | ((mc) << 8) | ((dt) << 15) | ((ad) << 16))
 
 #define SLIM_ROOT_FREQ 24576000
+#define MSM_SLIM_AUTOSUSPEND 1000
 
 /* MAX message size over control channel */
 #define SLIM_MSGQ_BUF_LEN	40
-- 
1.8.2.1

[toc] | [prev] | [next] | [standalone]

#1389957 — Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus

On Wednesday 27 April 2016 17:58:04 Sagar Dharia wrote:

> +/**
> + * slim_driver_register: Client driver registration with slimbus
> + * @drv:Client driver to be associated with client-device.
> + * This API will register the client driver with the slimbus
> + * It is called from the driver's module-init function.
> + */
> +int slim_driver_register(struct slim_driver *drv)
> +{
> +	drv->driver.bus = &slimbus_type;
> +
> +	return driver_register(&drv->driver);
> +}
> +EXPORT_SYMBOL_GPL(slim_driver_register);

Please make this use the same trick as platform_driver_register() to implicitly
set the .owner field of the driver to THIS_MODULE.

> +/**
> + * slim_add_device: Add a new device without register board info.
> + * @ctrl: Controller to which this device is to be added to.
> + * Called when device doesn't have an explicit client-driver to be probed, or
> + * the client-driver is a module installed dynamically.
> + */
> +int slim_add_device(struct slim_controller *ctrl, struct slim_device *sbdev)

This looks like an artifact of ancient pre-DT times. I'd say kill it off before
someone starts using it.

> +struct sbi_boardinfo {
> +	struct list_head	list;
> +	struct slim_boardinfo	board_info;
> +};
> +
> +static LIST_HEAD(board_list);
> +static LIST_HEAD(slim_ctrl_list);
> +static DEFINE_MUTEX(board_lock);
> +

> +/**
> + * slim_register_board_info: Board-initialization routine.
> + * @info: List of all devices on all controllers present on the board.
> + * @n: number of entries.
> + * API enumerates respective devices on corresponding controller.
> + * Called from board-init function.
> + */
> +int slim_register_board_info(struct slim_boardinfo const *info, unsigned n)
> +{

Same for all of this.

> +struct slim_device_id {
> +	__u16 manf_id, prod_code;
> +	__u8 dev_index, instance;
> +
> +	/* Data private to the driver */
> +	kernel_ulong_t driver_data;
> +};

Can you add explicit padding here to avoid having the anonymous two bytes in the middle?

Also, I think a void pointer instead of a kernel_ulong_t makes it more useful,
but there are always cases where one or the other fits better.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1390078 — Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus

[Multipart message — attachments visible in raw view] — view raw

On Thu, Apr 28, 2016 at 12:00:26PM +0200, Arnd Bergmann wrote:
> On Wednesday 27 April 2016 17:58:04 Sagar Dharia wrote:

> > +int slim_add_device(struct slim_controller *ctrl, struct slim_device *sbdev)

> This looks like an artifact of ancient pre-DT times. I'd say kill it off before
> someone starts using it.

Not every architecture uses DT, and even on architectures with DT
support it isn't always the only firmware.  In this specific case it's
questionable how many people are going to implement Slimbus at this
point but in general insisting that we go DT only doesn't seem great.

[toc] | [prev] | [next] | [standalone]

#1390127 — Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus

On Thursday 28 April 2016 12:53:37 Mark Brown wrote:
> On Thu, Apr 28, 2016 at 12:00:26PM +0200, Arnd Bergmann wrote:
> > On Wednesday 27 April 2016 17:58:04 Sagar Dharia wrote:
> 
> > > +int slim_add_device(struct slim_controller *ctrl, struct slim_device *sbdev)
> 
> > This looks like an artifact of ancient pre-DT times. I'd say kill it off before
> > someone starts using it.
> 
> Not every architecture uses DT, and even on architectures with DT
> support it isn't always the only firmware.  In this specific case it's
> questionable how many people are going to implement Slimbus at this
> point but in general insisting that we go DT only doesn't seem great.
> 

Nothing wrong with adding support for manual board files later if
we have a good reason for it, but at the moment, this seems completely
ARM/ARM64 specific.

I don't foresee mobile phones with ACPI using this subsystem, but even
if we got them, it would be a horrible idea to use hardcoded board
specific tables in a platform file, and we should insist that whatever
firmware is present has a way to describe the slimbus devices.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1390267 — Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus

[Multipart message — attachments visible in raw view] — view raw

On Thu, Apr 28, 2016 at 02:33:41PM +0200, Arnd Bergmann wrote:
> On Thursday 28 April 2016 12:53:37 Mark Brown wrote:
> > On Thu, Apr 28, 2016 at 12:00:26PM +0200, Arnd Bergmann wrote:

> > > This looks like an artifact of ancient pre-DT times. I'd say kill it off before
> > > someone starts using it.

> > Not every architecture uses DT, and even on architectures with DT
> > support it isn't always the only firmware.  In this specific case it's
> > questionable how many people are going to implement Slimbus at this
> > point but in general insisting that we go DT only doesn't seem great.

> Nothing wrong with adding support for manual board files later if
> we have a good reason for it, but at the moment, this seems completely
> ARM/ARM64 specific.

It's not in theory, but in practice nobody other that Qualcomm is ever
likely to release a controller.

> I don't foresee mobile phones with ACPI using this subsystem, but even
> if we got them, it would be a horrible idea to use hardcoded board
> specific tables in a platform file, and we should insist that whatever
> firmware is present has a way to describe the slimbus devices.

Right, in this particular case I don't think it makes a huge difference
but what you were talking about was "ancient pre-DT times" rather than
something specific to this particular case.  That's definitely a thing
that people keep thinking and it's good to push back on it since we do
have non-DT cases to worry about (some architectures, other firmwares,
things like PCI cards with other components on them and so on).

[toc] | [prev] | [next] | [standalone]

#1390290 — Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus

On Thursday 28 April 2016 15:38:01 Mark Brown wrote:
> On Thu, Apr 28, 2016 at 02:33:41PM +0200, Arnd Bergmann wrote:
> > On Thursday 28 April 2016 12:53:37 Mark Brown wrote:
> > I don't foresee mobile phones with ACPI using this subsystem, but even
> > if we got them, it would be a horrible idea to use hardcoded board
> > specific tables in a platform file, and we should insist that whatever
> > firmware is present has a way to describe the slimbus devices.
> 
> Right, in this particular case I don't think it makes a huge difference
> but what you were talking about was "ancient pre-DT times" rather than
> something specific to this particular case.  That's definitely a thing
> that people keep thinking and it's good to push back on it since we do
> have non-DT cases to worry about (some architectures, other firmwares,
> things like PCI cards with other components on them and so on).

Ok, I see what you mean. It turns out I made the exact same
comment on the first review five years ago (phrased more
nicely back then):

http://thread.gmane.org/gmane.linux.documentation/3192/focus=3193

My comment this time was for the particular driver, but I'd
still also maintain that a new subsystem in general should not
start out by addressing the needs of traditional board files.
I don't think we have merge new platform support on any
architecture that would need this in the past years and
stuff like spi_board_info and i2c_board_info is only really
used on really old machines (but not going away any time soon
either).

	Arnd

[toc] | [prev] | [next] | [standalone]

#1390387 — Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus

[Multipart message — attachments visible in raw view] — view raw

On Thu, Apr 28, 2016 at 04:59:02PM +0200, Arnd Bergmann wrote:

> My comment this time was for the particular driver, but I'd
> still also maintain that a new subsystem in general should not
> start out by addressing the needs of traditional board files.

I certainly wouldn't insist that people add it but equally pushing to
remove it too strongly seems like the wrong thing, at least as a general
comment rather than a specific one.

> I don't think we have merge new platform support on any
> architecture that would need this in the past years and
> stuff like spi_board_info and i2c_board_info is only really
> used on really old machines (but not going away any time soon
> either).

It's not just platforms that use these things though - there's things
like the SolarFlare NICs where the firmware update mechanism essentially
involves exposing a SPI flash as part of a PCI device and we just merged
an ASoC driver for a video card which was reusing some existing IPs and
chips.

[toc] | [prev] | [next] | [standalone]

#1390399 — Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus

On Thursday 28 April 2016 17:39:20 Mark Brown wrote:
> > I don't think we have merge new platform support on any
> > architecture that would need this in the past years and
> > stuff like spi_board_info and i2c_board_info is only really
> > used on really old machines (but not going away any time soon
> > either).
> 
> It's not just platforms that use these things though - there's things
> like the SolarFlare NICs where the firmware update mechanism essentially
> involves exposing a SPI flash as part of a PCI device and we just merged
> an ASoC driver for a video card which was reusing some existing IPs and
> chips.
> 

That's of course fine: you essentially have a discoverable bus there,
and if we need something like that, we can always add it later to
any subsystem.

In contrast, the interface in the proposed slimbus subsystem seems
designed for board files, and is in the best case just dead code
that can be removed, or has a risk of being misused e.g. if some
device manufacturer decides to use a board file for this instead
of describing the slimbus slaves in DT.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1391015 — Re: [PATCH V5 1/6] SLIMbus: Device management on SLIMbus

[Multipart message — attachments visible in raw view] — view raw

On Thu, Apr 28, 2016 at 06:49:23PM +0200, Arnd Bergmann wrote:
> On Thursday 28 April 2016 17:39:20 Mark Brown wrote:

> > It's not just platforms that use these things though - there's things
> > like the SolarFlare NICs where the firmware update mechanism essentially
> > involves exposing a SPI flash as part of a PCI device and we just merged
> > an ASoC driver for a video card which was reusing some existing IPs and
> > chips.

> That's of course fine: you essentially have a discoverable bus there,
> and if we need something like that, we can always add it later to
> any subsystem.

No, the issue with those cases is that there are devices on discoverable
buses that instantiate non-discoverable buses as part of the
discoverable device so the implementation of the discoverable device is
in part essentially a board file.

> In contrast, the interface in the proposed slimbus subsystem seems
> designed for board files, and is in the best case just dead code
> that can be removed, or has a risk of being misused e.g. if some
> device manufacturer decides to use a board file for this instead
> of describing the slimbus slaves in DT.

Which can be more of a pain than it should be if the code has been
written to assume that all the world is DT.  That's the most problematic
bit of this meme - I've seen people doing things like duplicating IP
drivers inside larger devices because they don't think they're allowed
to reuse the IP driver.  We can fix this sort of thing up when we notice
it but we should also try to avoid people making the mistakes in the
first place since it saves everyone a lot of effort.  With subsystems
and to a lesser extent off SoC drivers we need to be careful about just
blindly removing non-DT code and writing things so they're hard to use
without DT.

Like I say in this case I think the board file stuff is probably never
going to get used so it's probably sensible to remove that code but it
does need to be thought about.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web