[RFC 0/7] Qualcomm SMEM, SMD, RPM and regulators

[RFC 0/7] Qualcomm SMEM, SMD, RPM and regulators

[Bjorn Andersson]

All Qualcomm platforms implements a shared heap among the processors in the
SoC, used for sharing data with other parts of the system.

One consumer of items from this heap is the "Shared Memory Driver", a ring
buffer based point-to-point communication mechanism used to send either stream
or packet based data to remote processors.

Starting with 8x74 this system is used to talk to the Resource Power Manager
(RPM), a power efficient "coprocessor" with responsibility of aggregate votes
from the various systems in the SoC related to regulators, clocks and bus
frequencies.

The PMIC regulators and root clocks in these platforms are only accessible via
the RPM, so to get access to these we need the full chain of smem, smd, rpm and
a regulator driver implemented. And that is exactly what this series provides.


A key outstanding question is where in the tree we should put the
implementation, for now I dropped them in drivers/soc/qcom but that's only
because I don't know where to put it otherwise. I have not found any equivalent
of the SMEM driver, SMD resembles mailbox and rpmsg - but comments in that
patch on why it's neither.

RPM is a mfd and regulator is a regulator :)


Part from the rpm regulators I've tested this by hacking up a firmware loader
and making some adoptions to the wcn36xx WiFi driver and I can indeed
communicate with this system as well. Missing for that part is the coupling
with remoteproc to reset smd channels when a remote system goes down.  But that
is indeed a separate set of patches.

Bjorn Andersson (7):
  soc: qcom: Add device tree binding for SMEM
  soc: qcom: Add device tree binding for SMD
  mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding
  soc: qcom: Add Shared Memory Manager driver
  soc: qcom: Add Shared Memory Driver
  mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD
  regulator: qcom-smd-rpm: Regulator driver for the Qualcomm RPM

 .../devicetree/bindings/mfd/qcom-rpm-smd.txt       |  122 +++
 .../devicetree/bindings/soc/qcom/qcom,smd.txt      |   82 ++
 .../devicetree/bindings/soc/qcom/qcom,smem.txt     |   34 +
 drivers/mfd/Kconfig                                |   14 +
 drivers/mfd/Makefile                               |    1 +
 drivers/mfd/qcom-smd-rpm.c                         |  299 ++++++
 drivers/regulator/Kconfig                          |   12 +
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/qcom_smd-regulator.c             |  229 +++++
 drivers/soc/qcom/Kconfig                           |   16 +
 drivers/soc/qcom/Makefile                          |    2 +
 drivers/soc/qcom/qcom_smd.c                        | 1043 ++++++++++++++++++++
 drivers/soc/qcom/qcom_smem.c                       |  328 ++++++
 include/dt-bindings/mfd/qcom-rpm.h                 |   36 +
 include/linux/mfd/qcom-smd-rpm.h                   |    9 +
 include/linux/soc/qcom/qcom_smd.h                  |   47 +
 include/linux/soc/qcom/qcom_smem.h                 |   14 +
 17 files changed, 2289 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,smd.txt
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
 create mode 100644 drivers/mfd/qcom-smd-rpm.c
 create mode 100644 drivers/regulator/qcom_smd-regulator.c
 create mode 100644 drivers/soc/qcom/qcom_smd.c
 create mode 100644 drivers/soc/qcom/qcom_smem.c
 create mode 100644 include/linux/mfd/qcom-smd-rpm.h
 create mode 100644 include/linux/soc/qcom/qcom_smd.h
 create mode 100644 include/linux/soc/qcom/qcom_smem.h

-- 
1.7.9.5

[RFC 1/7] soc: qcom: Add device tree binding for SMEM

[Bjorn Andersson]

Add device tree binding documentation for the Qualcom Shared Memory
manager.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

Exposed by this node is a set of items of different sizes. For many things a
standard of_xlate method of referencing the individual nodes would be
preferable, so a #something-cells would make sense. We do however also needs
access to these items without explicitly stating the references in devicetree
(e.g. SMD references 257 of these). I haven't found any good example of how to
implement this, so suggestions are welcome.

Note that the hwspinlock reference is not yet supported in the mainline, but
this will likely need a few iterations so I wanted to get this out.

 .../devicetree/bindings/soc/qcom/qcom,smem.txt     |   34 ++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
new file mode 100644
index 0000000..ddd58c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
@@ -0,0 +1,34 @@
+Qualcomm Shared Memory binding
+
+This binding describes the Qualcomm Shared Memory, used to share data between
+various subsystems and OSes in Qualcomm platforms.
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be:
+		    "qcom,smem"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: base address and size pair for each area representing the
+		    shared memory. The first pair will must represent the "main"
+		    area, where the shared memory header and table-of-content
+		    can be found.
+
+- hwspinlocks:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: reference to a hwspinlock used to protect allocations from
+		    the shared memory
+
+= EXAMPLE
+
+        smem: smem@fa00000 {
+                compatible = "qcom,smem";
+                reg = <0x0fa00000 0x200000>,
+                      <0xfc428000 0x4000>;
+
+                hwspinlocks = <&tcsr_mutex 3>;
+        };
-- 
1.7.9.5

[RFC 2/7] soc: qcom: Add device tree binding for SMD

[Bjorn Andersson]

Add device tree binding documentation for the Qualcomm Shared Memory
Device.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

I was looking at having one smd node per remote processor - e.g flatten the
first and second level. As all the channels, for all the remote processors are
allocated from the same pool this does however not feel very natural.

 .../devicetree/bindings/soc/qcom/qcom,smd.txt      |   82 ++++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,smd.txt

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smd.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,smd.txt
new file mode 100644
index 0000000..c56e4fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smd.txt
@@ -0,0 +1,82 @@
+Qualcomm Shared Memory Driver (SMD) binding
+
+This binding describes the Qualcomm Shared Memory Driver, a fifo based
+communication channel for sending data between the various subsystems in
+Qualcomm platforms.
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "qcom,smd"
+
+- qcom,smem:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: reference to a smem node managing the shared memory items
+		    used for smd
+
+= EDGES
+
+Each subnode of the SMD node represents a remote subsystem, i.e. a remote
+processor of some sort - in SMD language called an "edge". The name of the
+edges are not important.
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: should specify the IRQ used by the remote processor to
+		    signal this processor about communication related updates
+
+- qcom,ipc:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: three entries specifying the outgoing ipc bit used for
+		    signaling the remote processor:
+		    - phandle to a syscon node representing the apcs registers
+		    - u32 representing offset to the register within the syscon
+		    - u32 representing the ipc bit within the register
+
+- qcom,smd-edge:
+	Usage: required
+	Value type: <u32>
+	Definition: an identifier representing the remote processor
+
+= SMD DEVICES
+
+In turn, subnodes of the "edges" represent devices tied to SMD channels on that
+"edge". The names of the devices are not important. The properties of these
+nodes are defined by the individual bindings for the SMD devices - but must
+contain the following property:
+
+- qcom,smd-channels:
+	Usage: required
+	Value type: <stringlist>
+	Definition: a list of channels tied to this device, used for matching
+		    the device to channels
+
+= EXAMPLE
+
+The following example represents a smd node, with one edge representing the
+"rpm" subsystem. For the "rpm" subsystem we have a device tied to the
+"rpm_request" channel.
+
+        smd {
+                compatible = "qcom,smd";
+                qcom,smem = <&smem>;
+
+                rpm {
+                        interrupts = <0 168 1>;
+                        qcom,ipc = <&apcs 8 0>;
+                        qcom,smd-edge = <15>;
+
+                        rpm_requests {
+                                compatible = "qcom,rpm-msm8974";
+                                qcom,smd-channels = "rpm_requests";
+
+                                #address-cells = <1>;
+                                #size-cells = <0>;
+
+				...
+			};
+		};
+	};
-- 
1.7.9.5

[RFC 3/7] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

[Bjorn Andersson]

Add binding documentation for the Qualcomm Resource Power Manager (RPM)
using shared memory (Qualcomm SMD) as transport mechanism. This is found
in 8974 and newer based devices.

The binding currently describes the rpm itself and the regulator
subnodes.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

Note that this patch extends the rpm dt-bindings header from [1].

[1] https://lkml.org/lkml/2014/9/22/733

 .../devicetree/bindings/mfd/qcom-rpm-smd.txt       |  122 ++++++++++++++++++++
 include/dt-bindings/mfd/qcom-rpm.h                 |   36 ++++++
 2 files changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt

diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
new file mode 100644
index 0000000..a846101
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
@@ -0,0 +1,122 @@
+Qualcomm Resource Power Manager (RPM) over SMD
+
+This driver is used to interface with the Resource Power Manager (RPM) found in
+various Qualcomm platforms. The RPM allows each component in the system to vote
+for state of the system resources, such as clocks, regulators and bus
+frequencies.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,rpm-msm8974"
+
+- qcom,smd-channels:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Shared Memory Channel used for communication with the RPM
+
+- #address-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: must be 1
+
+- #size-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: must be 0
+
+= SUBDEVICES
+
+The RPM exposes resources to its subnodes. The below bindings specify the set
+of valid subnodes that can operate on these resources.
+
+== Switch-mode Power Supply regulator
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,rpm-pm8841-smps"
+		    "qcom,rpm-pm8941-smps"
+
+
+- reg:
+	Usage: required
+	Value type: <u32>
+	Definition: resource as defined in <dt-bindings/mfd/qcom-rpm.h>
+		    must be one of:
+		    QCOM_RPM_PM8841_SMPS1 - QCOM_RPM_PM8841_SMPS4,
+		    QCOM_RPM_PM8941_SMPS1 - QCOM_RPM_PM8941_SMPS3
+
+Standard regulator bindings are used inside switch mode power supply subnodes.
+Check Documentation/devicetree/bindings/regulator/regulator.txt for more
+details.
+
+== Low-dropout regulator
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,rpm-pm8941-ldo"
+
+- reg:
+	Usage: required
+	Value type: <u32>
+	Definition: resource as defined in <dt-bindings/mfd/qcom-rpm.h>
+		    must be one of:
+		    QCOM_RPM_PM8941_LDO1 - QCOM_RPM_PM8941_LDO24
+
+Standard regulator bindings are used inside switch low-dropout regulator
+subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
+more details.
+
+== Switch
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,rpm-pm8941-switch"
+
+- reg:
+	Usage: required
+	Value type: <u32>
+	Definition: resource as defined in <dt-bindings/mfd/qcom/qcom-rpm.h>
+		    must be one of:
+		    QCOM_RPM_PM8941_LVS1 - QCOM_RPM_PM8941_LVS3,
+		    QCOM_RPM_PM8941_MVS1 - QCOM_RPM_PM8941_MVS2
+
+Standard regulator bindings are used inside switch regulator subnodes.  Check
+Documentation/devicetree/bindings/regulator/regulator.txt for more details.
+
+= EXAMPLE
+
+	#include <dt-bindings/mfd/qcom-rpm.h>
+
+	rpm@108000 {
+		compatible = "qcom,rpm-msm8960";
+		qcom,smd-channels = "rpm_requests";
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pm8941_s1: regulator-s1 {
+			compatible = "qcom,rpm-pm8941-smps";
+			reg = <QCOM_RPM_PM8941_SMPS1>;
+
+			regulator-min-microvolt = <1300000>;
+			regulator-max-microvolt = <1300000>;
+		};
+
+		pm8941_l3: pm8941-l3 {
+			compatible = "qcom,rpm-pm8941-ldo";
+			reg = <QCOM_RPM_PM8941_LDO3>;
+
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+		};
+
+	};
+
diff --git a/include/dt-bindings/mfd/qcom-rpm.h b/include/dt-bindings/mfd/qcom-rpm.h
index 388a6f3..a5c473e 100644
--- a/include/dt-bindings/mfd/qcom-rpm.h
+++ b/include/dt-bindings/mfd/qcom-rpm.h
@@ -141,6 +141,42 @@
 #define QCOM_RPM_SYS_FABRIC_MODE		131
 #define QCOM_RPM_USB_OTG_SWITCH			132
 #define QCOM_RPM_VDDMIN_GPIO			133
+#define QCOM_RPM_PM8841_SMPS1			134
+#define QCOM_RPM_PM8841_SMPS2			135
+#define QCOM_RPM_PM8841_SMPS3			136
+#define QCOM_RPM_PM8841_SMPS4			137
+#define QCOM_RPM_PM8941_SMPS1			138
+#define QCOM_RPM_PM8941_SMPS2			139
+#define QCOM_RPM_PM8941_SMPS3			140
+#define QCOM_RPM_PM8941_LDO1			141
+#define QCOM_RPM_PM8941_LDO2			142
+#define QCOM_RPM_PM8941_LDO3			143
+#define QCOM_RPM_PM8941_LDO4			144
+#define QCOM_RPM_PM8941_LDO5			145
+#define QCOM_RPM_PM8941_LDO6			146
+#define QCOM_RPM_PM8941_LDO7			147
+#define QCOM_RPM_PM8941_LDO8			148
+#define QCOM_RPM_PM8941_LDO9			149
+#define QCOM_RPM_PM8941_LDO10			150
+#define QCOM_RPM_PM8941_LDO11			151
+#define QCOM_RPM_PM8941_LDO12			152
+#define QCOM_RPM_PM8941_LDO13			153
+#define QCOM_RPM_PM8941_LDO14			154
+#define QCOM_RPM_PM8941_LDO15			155
+#define QCOM_RPM_PM8941_LDO16			156
+#define QCOM_RPM_PM8941_LDO17			157
+#define QCOM_RPM_PM8941_LDO18			158
+#define QCOM_RPM_PM8941_LDO19			159
+#define QCOM_RPM_PM8941_LDO20			160
+#define QCOM_RPM_PM8941_LDO21			161
+#define QCOM_RPM_PM8941_LDO22			162
+#define QCOM_RPM_PM8941_LDO23			163
+#define QCOM_RPM_PM8941_LDO24			164
+#define QCOM_RPM_PM8941_LVS1			165
+#define QCOM_RPM_PM8941_LVS2			166
+#define QCOM_RPM_PM8941_LVS3			167
+#define QCOM_RPM_PM8941_MVS1			168
+#define QCOM_RPM_PM8941_MVS2			169
 
 /*
  * Constants used to select force mode for regulators.
-- 
1.7.9.5

[RFC 4/7] soc: qcom: Add Shared Memory Manager driver

[Bjorn Andersson]

The Shared Memory Manager driver implements an interface for allocating
and accessing items in the memory area shared among all of the
processors in a Qualcomm platform.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

In later platforms this is extended to support "secure smem", I do
unfortunately not have any devices that I could test this with so I have only
implemented the "insecure" version for now.

I was thinking about implementing an of_xlate as this would make sense for many
things. It is however not feasible for SMD, that would require us to list 257
items.

It would make sense to enhance this with a method of keeping track of currently
consumed items, both to take care of "mutual exclusion" between consumers as
well as knowing when it's safe to remove the device and/or unload the driver.

I did consider exposing the items as regmaps, but for many of the items the
regmap context is consuming far more space then the actual data. And we would
end up creating 3-400 regmap contexts in a normal system.


Also note that the hwspinlock framework does not yet support devicetree based
retrieval of spinlock handles, so that part needs to be changed.

 drivers/soc/qcom/Kconfig           |    8 +
 drivers/soc/qcom/Makefile          |    1 +
 drivers/soc/qcom/qcom_smem.c       |  328 ++++++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/qcom_smem.h |   14 ++
 4 files changed, 351 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_smem.c
 create mode 100644 include/linux/soc/qcom/qcom_smem.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 7bd2c94..9e6bc56 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -9,3 +9,11 @@ config QCOM_GSBI
           functions for connecting the underlying serial UART, SPI, and I2C
           devices to the output pins.
 
+config QCOM_SMEM
+	tristate "Qualcomm Shared Memory Interface"
+	depends on ARCH_QCOM
+	help
+	  Say y here to enable support for the Qualcomm Shared Memory Manager.
+	  The driver provides an interface to items in a heap shared among all
+	  processors in a Qualcomm platform and is used for exchanging
+	  information as well as a fifo based communication mechanism (SMD).
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 4389012..b1f7b18 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
+obj-$(CONFIG_QCOM_SMEM) +=	qcom_smem.o
diff --git a/drivers/soc/qcom/qcom_smem.c b/drivers/soc/qcom/qcom_smem.c
new file mode 100644
index 0000000..9766c86
--- /dev/null
+++ b/drivers/soc/qcom/qcom_smem.c
@@ -0,0 +1,328 @@
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB.
+ * Copyright (c) 2012-2013, 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/hwspinlock.h>
+#include <linux/soc/qcom/qcom_smem.h>
+
+#define AUX_BASE_MASK		0xfffffffc
+#define HWSPINLOCK_TIMEOUT	1000
+#define SMEM_MAX_ITEMS		512
+
+/**
+  * struct smem_proc_comm - proc_comm communication struct (legacy)
+  * @command:	current command to be executed
+  * @status:	status of the currently requested command
+  * @params:	parameters to the command
+  */
+struct smem_proc_comm {
+	u32 command;
+	u32 status;
+	u32 params[2];
+};
+
+/**
+ * struct smem_entry - entry to reference smem items on the heap
+ * @allocated:	boolean to indicate if this entry is used
+ * @offset:	offset to the allocated space
+ * @size:	size of the allocated space, 8 byte aligned
+ * @aux_base:	base address for the memory region used by this unit, or 0 for
+ *		the default region. bits 0,1 are reserved
+ */
+struct smem_entry {
+	u32 allocated;
+	u32 offset;
+	u32 size;
+	u32 aux_base; /* bits 1:0 reserved */
+};
+
+/**
+ * struct smem_header - header found in beginning of primary smem region
+ * @proc_comm:		proc_comm communication interface (legacy)
+ * @version:		array of versions for the various subsystems
+ * @smem_initialized:	boolean to indicate that smem is initialized
+ * @free_offset:	index of the first unallocated byte in smem
+ * @available:		number of bytes available for allocation
+ * @unused:		reserved field
+ * toc:			array of references to smem entries
+ */
+struct smem_header {
+	struct smem_proc_comm proc_comm[4];
+	u32 version[32];
+	u32 smem_initialized;
+	u32 free_offset;
+	u32 available;
+	u32 unused;
+	struct smem_entry toc[SMEM_MAX_ITEMS];
+};
+
+/**
+  * struct smem_area - memory region used for smem heap
+  * @aux_base:	physical base address of the region, used for entry lookup
+  * @virt_base:	virtual address of the mapping
+  */
+struct smem_region {
+	u32 aux_base;
+	void __iomem *virt_base;
+};
+
+/**
+  * struct qcom_smem - control struct for the smem driver
+  * @dev:		device pointer
+  * @hwlock:		hwspinlock to be held during heap operations
+  * @num_regions:	number of entires in the regions array
+  * @regions:		array of memory regions, region 0 contains smem_header
+  */
+struct qcom_smem {
+	struct device *dev;
+
+	struct hwspinlock *hwlock;
+
+	unsigned num_regions;
+	struct smem_region regions[0];
+};
+
+/* Pointer to the one and only smem handle */
+static struct qcom_smem *smem_handle;
+
+/**
+ * of_get_qcom_smem - retrieve a smem handle from a phandle
+ * @client_node: of_node of the client
+ *
+ * Resolve the phandle in the property "qcom,smem" of @client_node and use this
+ * to retrieve an smem handle.
+ */
+struct qcom_smem *of_get_qcom_smem(struct device_node *client_node)
+{
+	struct device_node *node;
+
+	node = of_parse_phandle(client_node, "qcom,smem", 0);
+	if (!node)
+		return ERR_PTR(-EINVAL);
+
+	of_node_put(node);
+
+	if (!smem_handle)
+		return ERR_PTR(-EPROBE_DEFER);
+	else if (smem_handle->dev->of_node != node)
+		return ERR_PTR(-EINVAL);
+
+	return smem_handle;
+}
+EXPORT_SYMBOL(of_get_qcom_smem);
+
+/**
+ * qcom_smem_alloc - allocate space for a smem item
+ * @smem:	smem handle
+ * @smem_id:	item to be allocated
+ * @size:	number of bytes to be allocated
+ *
+ * Allocate space for a given smem item of size @size, given that the item is
+ * not yet allocated.
+ */
+int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size)
+{
+	struct smem_header *header = smem->regions[0].virt_base;
+	struct smem_entry *entry;
+	unsigned long flags;
+	int ret;
+
+	size = ALIGN(size, 8);
+
+	if (WARN_ON(smem_id >= SMEM_MAX_ITEMS))
+		return -EINVAL;
+
+	ret = hwspin_lock_timeout_irqsave(smem->hwlock,
+					  HWSPINLOCK_TIMEOUT,
+					  &flags);
+	if (ret)
+		return ret;
+
+	entry = &header->toc[smem_id];
+	if (entry->allocated) {
+		ret = -EEXIST;
+		goto out;
+	}
+
+	if (WARN_ON(size > header->available)) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	entry->offset = header->free_offset;
+	entry->size = size;
+	entry->allocated = 1;
+
+	header->free_offset += size;
+	header->available -= size;
+
+	/* Commit the changes before we release the spin lock */
+	wmb();
+out:
+	hwspin_unlock_irqrestore(smem->hwlock, &flags);
+	return ret;
+}
+EXPORT_SYMBOL(qcom_smem_alloc);
+
+unsigned qcom_smem_get_free_space(struct qcom_smem *smem)
+{
+	struct smem_header *header = smem->regions[0].virt_base;
+
+	return header->available;
+}
+EXPORT_SYMBOL(qcom_smem_get_free_space);
+
+/**
+ * qcom_smem_get - resolve ptr of size of a smem item
+ * @smem:	smem handle
+ * @smem_id:	item id to be resolved
+ * @ptr:	pointer to be filled out with address of the item
+ * @size:	pointer to be filled out with size of the item
+ *
+ * Looks up pointer and size of a smem item.
+ */
+int qcom_smem_get(struct qcom_smem *smem,
+		  unsigned smem_id,
+		  void **ptr,
+		  size_t *size)
+{
+	struct smem_header *header = smem->regions[0].virt_base;
+	struct smem_region *area;
+	struct smem_entry *entry;
+	unsigned long flags;
+	u32 aux_base;
+	unsigned i;
+	int ret;
+
+	if (WARN_ON(smem_id >= SMEM_MAX_ITEMS))
+		return -EINVAL;
+
+	ret = hwspin_lock_timeout_irqsave(smem->hwlock,
+					  HWSPINLOCK_TIMEOUT,
+					  &flags);
+	if (ret)
+		return ret;
+
+	entry = &header->toc[smem_id];
+	if (!entry->allocated) {
+		ret = -ENXIO;
+		goto out;
+	}
+
+	if (ptr != NULL) {
+		aux_base = entry->aux_base & AUX_BASE_MASK;
+
+		for (i = 0; i < smem->num_regions; i++) {
+			area = &smem->regions[i];
+
+			if (area->aux_base == aux_base || !aux_base) {
+				*ptr = area->virt_base + entry->offset;
+				break;
+			}
+		}
+	}
+	if (size != NULL)
+		*size = entry->size;
+
+out:
+	hwspin_unlock_irqrestore(smem->hwlock, &flags);
+	return ret;
+}
+EXPORT_SYMBOL(qcom_smem_get);
+
+static int qcom_smem_probe(struct platform_device *pdev)
+{
+	struct qcom_smem *smem;
+	struct resource *res;
+	size_t array_size;
+	int num_regions = 0;
+	int i;
+
+	for (i = 0; i < pdev->num_resources; i++) {
+		res = &pdev->resource[i];
+
+		if (resource_type(res) == IORESOURCE_MEM)
+			num_regions++;
+	}
+
+	if (num_regions == 0) {
+		dev_err(&pdev->dev, "no smem regions specified\n");
+		return -EINVAL;
+	}
+
+	array_size = num_regions * sizeof(struct smem_region);
+	smem = devm_kzalloc(&pdev->dev, sizeof(*smem) + array_size, GFP_KERNEL);
+	if (!smem)
+		return -ENOMEM;
+
+	smem->dev = &pdev->dev;
+	smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL);
+	if (IS_ERR(smem->hwlock))
+		return PTR_ERR(smem->hwlock);
+
+	smem->num_regions = num_regions;
+
+	for (i = 0; i < num_regions; i++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+
+		smem->regions[i].aux_base = (u32)res->start;
+		smem->regions[i].virt_base = devm_ioremap(&pdev->dev,
+							  res->start,
+							  resource_size(res));
+		if (!smem->regions[i].virt_base)
+			return -ENOMEM;
+	}
+
+	dev_set_drvdata(&pdev->dev, smem);
+	smem_handle = smem;
+
+	dev_info(&pdev->dev, "Qualcomm Shared Memory Interface probed\n");
+
+	return 0;
+}
+
+static const struct of_device_id qcom_smem_of_match[] = {
+	{ .compatible = "qcom,smem" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_smem_of_match);
+
+static struct platform_driver qcom_smem_driver = {
+	.probe          = qcom_smem_probe,
+	.driver  = {
+		.name  = "qcom_smem",
+		.owner = THIS_MODULE,
+		.of_match_table = qcom_smem_of_match,
+	},
+};
+
+static int __init qcom_smem_init(void)
+{
+	return platform_driver_register(&qcom_smem_driver);
+}
+arch_initcall(qcom_smem_init);
+
+static void __exit qcom_smem_exit(void)
+{
+	platform_driver_unregister(&qcom_smem_driver);
+}
+module_exit(qcom_smem_exit)
+
+MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>");
+MODULE_DESCRIPTION("Qualcomm Shared Memory Interface");
+MODULE_LICENSE("GPLv2");
diff --git a/include/linux/soc/qcom/qcom_smem.h b/include/linux/soc/qcom/qcom_smem.h
new file mode 100644
index 0000000..7c0d3fd
--- /dev/null
+++ b/include/linux/soc/qcom/qcom_smem.h
@@ -0,0 +1,14 @@
+#ifndef __QCOM_SMEM_H__
+#define __QCOM_SMEM_H__
+
+struct device_node;
+struct qcom_smem;
+
+struct qcom_smem *of_get_qcom_smem(struct device_node *node);
+
+int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size);
+int qcom_smem_get(struct qcom_smem *smem, unsigned item, void **ptr, size_t *size);
+
+unsigned qcom_smem_get_free_space(struct qcom_smem *smem);
+
+#endif
-- 
1.7.9.5

[RFC 5/7] soc: qcom: Add Shared Memory Driver

[Bjorn Andersson]

This adds the Qualcomm Shared Memory Driver (SMD) providing
communication channels to remote processors, ontop of SMEM.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

As we've already seen this is fuel for discussions, so I hope to start these
off by giving some insight in my research here.

== Codeaurora implementation ==
The codeaurora implementation originates from an implementation that was based
purely on initcall and global state. Clients are platform_drivers, they request
channels, has an "event callback" to indicate when to do things and handles the
fifo logic in the client. That means that every client driver duplicates this
logic - in comparison to e.g. rpmsg where this is completely abstracted away.

My proposal is following the rpmsg way of providing all these things, in an
attempt to reduce the code duplication and create a cleaner abstraction between
the two pieces. The side effect of this is that smd devices will follow the
life cycle of the corresponding smd channel, i.e. devices will be removed when
the other side goes away.

In the msm-3.10 branch there are 19 calls to smd_open_named_on_edge() but only
one of these cases have multiple channels per "smd client". So I've followed
rpmsg and optimized the implementation for the 1:1 channel vs device setup. The
idea is to follow rpmsg and provide an actual "open" api, that will attach
further channels to an already existing device.

== RPMSG ==

The rpmsg_driver fits nicely into what I want to do, so that could be used
straight off. Data is transmitted by calling rpmsg_send() and incoming data is
delivered through the registered callback on a channel.  It's possible to
request additional channels for a rpmsg device, although this is probably never
tested as there is no way to send data on this additional endpoint.

Endpoints are sort of equivalent of a smd channel although the life cycle is
slightly different, but a major issue with the rpmsg interface is that channels
are identified by a single u32, while SMD channels are identified by a u32 and
the channel name.

So to be able to use the rpmsg api we would need to create additional apis that
handles the custom address format of the smd channels. Furthermore all the "off
channel" functions would be invalid.
Furthermore most of the channel and endpoint structures would be "invalid".


But these are the problems with the actual api, rpmsg is not only the api. It's
a direct implementation of these functions, defining how services are
discovered, how channels are represented as well as the actual "wire" protocol.

Or to put it in another way: rpmsg is an implementation of a packet protocol
on-top of virtio, it is not a framework or api for abstracting packet transport
logic.

As there are discussions regarding replacing SMD with a new wire protocol, it
would probably be convenient to create a generic version of my proposed "api"
that can be re-used between different implementations and hopefully provide
re-use of parts of the code. Maybe we can make rpmsg an implementation under
such a generic api.

== Mailbox "framework" ==
The recently proposed mailbox "framework" have already come up numerous times
in discussing this.

The proposed mailbox framework consists of a notion of a mailbox client and a
mailbox controller. The controller exposes a predefined number of allways
available channels and there is a set of apis and callbacks related to how to
interact between the two sides.

A client, e.g. a platform_driver, requests a handle to a channel and use this
for communication. In SMD channels have life cycles, so for this to work I
would need to implement some sort of life cycle management around this. 

SMD can operate in packet or streaming mode, in both cases we have dynamically
sized data chunks. The mailbox api presents the mbox_send_message() function,
where the data chunk is defined by a "void *mssg" - i.e. a pointer to some data
of size magically known by both sides of the api.

The rx_callback in mbox_client would be used for handing data to the SMD client
similarly to below. I think the rest of tx_done, mbox_client_txdone and
mbox_client_peek_data are implementing the rest of my receive loop. But at the
cost of spreading this code in all clients.

So, if the mailbox framework had the life cycle management of channels,
supported variable size data and I move half the implementation of the message
pump to the clients (spread out over 4-5 callbacks/function calls), then we
should be able to use the framework.

The benefit would be to say that we're using the framework, the drawback would
be that the api between the smd implementation and it's clients would be much
more complicated.

We would not save a single line of code and we would not be able to reuse any
existing controller as the api does not abstract much of this.

== Firmware loader and crashes ==
I believe we can generalize remoteproc (align it with the documentation) and
get the Qualcomm peripheral image loader in there. There are several ways in
the Qualcomm system to get notified that a remote processor has crashed and we
can tie those into methods of e.g. dumping the ram associated with the remote
processor. But when this is done the remoteproc must trigger a reset on all
channels associated with the remote processor, as these will in most cases
remain "open".
In codeaurora this is done by an explicit call resetting all channels when this
happen, the question for our mainline version is how to tie the two components
together to trigger this action. (remoteproc would be notified by an raising
gpio/irq)

== Error handling ==
In the codeaurora version of this driver faulty state of a channel info is
cause for a BUG() and incoming packets that are greater than the ring buffer
causes triggers a restart of that remote processor (and then a BUG if that
fails). It might in some cases be enough to just close the faulty channel and
hope for the other side to clean up the state and restart, or we need to
trigger remoteproc to restart the remote processor, but part of RPM failing
these things should not be considered fatal for the Linux processor.

 drivers/soc/qcom/Kconfig          |    8 +
 drivers/soc/qcom/Makefile         |    1 +
 drivers/soc/qcom/qcom_smd.c       | 1043 +++++++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/qcom_smd.h |   47 ++
 4 files changed, 1099 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_smd.c
 create mode 100644 include/linux/soc/qcom/qcom_smd.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 9e6bc56..9ac1541 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -9,6 +9,14 @@ config QCOM_GSBI
           functions for connecting the underlying serial UART, SPI, and I2C
           devices to the output pins.
 
+config QCOM_SMD
+	tristate "Qualcomm Shared Memory Driver"
+	depends on QCOM_SMEM
+	help
+	  Say y here to enable support for the Qualcomm Shared Memory Driver
+	  providing communication channels to remote processors in Qualcomm
+	  platforms.
+
 config QCOM_SMEM
 	tristate "Qualcomm Shared Memory Interface"
 	depends on ARCH_QCOM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index b1f7b18..0e89532 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
+obj-$(CONFIG_QCOM_SMD) +=	qcom_smd.o
 obj-$(CONFIG_QCOM_SMEM) +=	qcom_smem.o
diff --git a/drivers/soc/qcom/qcom_smd.c b/drivers/soc/qcom/qcom_smd.c
new file mode 100644
index 0000000..0175293
--- /dev/null
+++ b/drivers/soc/qcom/qcom_smd.c
@@ -0,0 +1,1043 @@
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB.
+ * Copyright (c) 2012-2013, 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/memblock.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/hwspinlock.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/delay.h>
+#include <linux/soc/qcom/qcom_smd.h>
+#include <linux/soc/qcom/qcom_smem.h>
+
+/*
+ * The Qualcomm Shared Memory communication solution provides point-to-point
+ * channels for clients to send and receive streaming or packet based data.
+ *
+ * Each channel consists of a control item (channel info) and a ring buffer
+ * pair. The channel info carry information related to channel state, flow
+ * control and the offsets within the ring buffer.
+ *
+ * All allocated channels are listed in an allocation table, identifying the
+ * pair of items by name, type and remote processor.
+ *
+ * Upon creating a new channel the remote processor allocates channel info and
+ * ring buffer items from the smem heap and populate the allocation table. An
+ * interrupt is sent to the other end of the channel and a scan for new
+ * channels should be done. A channel never goes away, it will only change
+ * state.
+ *
+ * The remote processor signals it intent for bring up the communication
+ * channel by setting the state of its end of the channel to "opening" and
+ * sends out an interrupt. We detect this change and register a smd device to
+ * consume the channel. Upon finding a consumer we finish the handshake and the
+ * channel is up.
+ *
+ * Upon closing a channel, the remote processor will update the state of its
+ * end of the channel and signal us, we will then unregister any attached
+ * device and close our end of the channel.
+ *
+ * Devices attached to a channel can use the qcom_smd_send function to push
+ * data to the channel, this is done by copying the data into the tx ring
+ * buffer, updating the pointers in the channel info and signaling the remote
+ * processor.
+ *
+ * The remote processor does the equivalent when it transfer data and upon
+ * receiving the interrupt we check the channel info for new data and delivers
+ * this to the attached device. If the device is not ready to receive the data
+ * we leave it in the ring buffer for now.
+ */
+
+#define SMEM_CHANNEL_ALLOC_TBL	13
+#define SMEM_SMD_INFO_BASE_ID	14
+#define SMEM_SMD_FIFO_BASE_ID	338
+
+#define SMD_CHANNEL_NAME_LEN	20
+#define SMD_NUM_CHANNELS        64
+
+struct smd_channel_info;
+struct smd_channel_info_word;
+
+/**
+ * struct qcom_smd_edge - representing a remote processor
+ * @smd:		handle to qcom_smd
+ * @of_node:		of_node handle for information related to this edge
+ * @edge_id:		identifier of this edge
+ * @ipc_regmap:		regmap handle holding the outgoing ipc register
+ * @ipc_offset:		offset within @ipc_regmap of the register for ipc
+ * @ipc_bit:		bit in the register at @ipc_offset of @ipc_regmap
+ * @channels:		list of all channels detected on this edge
+ * @smem_available:	last available amount of smem triggering a channel scan
+ * @channel_scan_work:	work item for channel scanning
+ * @state_change_work:	work item for state changes
+ */
+struct qcom_smd_edge {
+	struct qcom_smd *smd;
+	struct device_node *of_node;
+	unsigned edge_id;
+
+	struct regmap *ipc_regmap;
+	int ipc_offset;
+	int ipc_bit;
+
+	struct list_head channels;
+
+	unsigned smem_available;
+	struct work_struct channel_scan_work;
+	struct work_struct state_change_work;
+};
+
+/**
+ * struct qcom_smd_channel - smd channel struct
+ * @smd:	handle to qcom_smd
+ * @edge:	qcom_smd_edge this channel is living on
+ * @qsdev:	reference to a associated smd device
+ * @name:	name of the channel
+ * @state:	local state of the channel
+ * @remote_state:	remote state of the channel
+ * @tx_info:	byte aligned outgoing channel info
+ * @rx_info:	byte aligned incoming channel info
+ * @tx_info_word:	word aligned outgoing channel info
+ * @rx_info_word:	word aligned incoming channel info
+ * @tx_lock:	lock to make writes to the channel mutually exclusive
+ * @tx_fifo:	pointer to the outgoing ring buffer
+ * @rx_fifo:	pointer to the incoming ring buffer
+ * @fifo_size:	size of the ring buffers
+ * @bounce_buffer: bounce buffer for reading wrapped packets
+ * @cb:		callback function registered for this channel
+ * @cb_lock:	guard for modifying @cb while handling incoming data
+ * @pkt_size:	size of the currently handled packet
+ * @list:	lite entry for @channels in qcom_smd_edge
+ */
+struct qcom_smd_channel {
+	struct qcom_smd *smd;
+	struct qcom_smd_edge *edge;
+
+	struct qcom_smd_device *qsdev;
+
+	char *name;
+	unsigned state;
+	unsigned remote_state;
+
+	struct smd_channel_info *tx_info;
+	struct smd_channel_info *rx_info;
+
+	struct smd_channel_info_word *tx_info_word;
+	struct smd_channel_info_word *rx_info_word;
+
+	struct mutex tx_lock;
+
+	void *tx_fifo;
+	void *rx_fifo;
+	int fifo_size;
+
+	void *bounce_buffer;
+	int (*cb)(struct qcom_smd_device *, void *, size_t);
+	spinlock_t cb_lock;
+
+	int pkt_size;
+
+	struct list_head list;
+};
+
+/**
+ * struct qcom_smd - smd struct
+ * @dev:	device struct
+ * @smem:	reference to smem handler
+ * @allocated:	bitmap representing already allocated channels
+ * @num_edges:	number of entries in @edges
+ * @edges:	array of edges to be handled
+ */
+struct qcom_smd {
+	struct device *dev;
+
+	struct qcom_smem *smem;
+
+	DECLARE_BITMAP(allocated, SMD_NUM_CHANNELS);
+
+	unsigned num_edges;
+	struct qcom_smd_edge edges[0];
+};
+
+struct smd_channel_info {
+	u32 state;
+	u8  fDSR;
+	u8  fCTS;
+	u8  fCD;
+	u8  fRI;
+	u8  fHEAD;
+	u8  fTAIL;
+	u8  fSTATE;
+	u8  fBLOCKREADINTR;
+	u32 tail;
+	u32 head;
+};
+
+struct smd_channel_info_word {
+	u32 state;
+	u32 fDSR;
+	u32 fCTS;
+	u32 fCD;
+	u32 fRI;
+	u32 fHEAD;
+	u32 fTAIL;
+	u32 fSTATE;
+	u32 fBLOCKREADINTR;
+	u32 tail;
+	u32 head;
+};
+
+enum {
+	SMD_CHANNEL_CLOSED,
+	SMD_CHANNEL_OPENING,
+	SMD_CHANNEL_OPENED,
+	SMD_CHANNEL_FLUSHING,
+	SMD_CHANNEL_CLOSING,
+	SMD_CHANNEL_RESET,
+	SMD_CHANNEL_RESET_OPENING
+};
+
+#define GET_RX_CHANNEL_INFO(channel, param) \
+	(channel->rx_info_word ? \
+		channel->rx_info_word->param : \
+		channel->rx_info->param)
+
+#define GET_TX_CHANNEL_INFO(channel, param) \
+	(channel->rx_info_word ? \
+		channel->tx_info_word->param : \
+		channel->tx_info->param)
+
+#define SET_RX_CHANNEL_INFO(channel, param, value) \
+	(channel->rx_info_word ? \
+		(channel->rx_info_word->param = value) : \
+		(channel->rx_info->param = value))
+
+#define SET_TX_CHANNEL_INFO(channel, param, value) \
+	(channel->rx_info_word ? \
+		(channel->tx_info_word->param = value) : \
+		(channel->tx_info->param = value))
+
+/**
+ * struct qcom_smd_alloc_entry - channel allocation entry
+ * @name:	channel name
+ * @cid:	channel index
+ * @flags:	channel flags and type
+ * @ref_count:	reference count of the channel
+ */
+struct qcom_smd_alloc_entry {
+	u8 name[SMD_CHANNEL_NAME_LEN];
+	u32 cid;
+	u32 flags;
+	u32 ref_count;
+} __packed;
+
+#define SMD_CHANNEL_FLAGS_EDGE		0xff
+#define SMD_CHANNEL_FLAGS_STREAM	BIT(8)
+#define SMD_CHANNEL_FLAGS_PACKET	BIT(9)
+
+static void qcom_smd_signal_channel(struct qcom_smd_channel *channel)
+{
+	struct qcom_smd_edge *edge = channel->edge;
+
+	regmap_write(edge->ipc_regmap, edge->ipc_offset, BIT(edge->ipc_bit));
+}
+
+/*
+ * Initialize the tx channel info
+ */
+static void qcom_smd_channel_reset(struct qcom_smd_channel *channel)
+{
+	SET_TX_CHANNEL_INFO(channel, state, SMD_CHANNEL_CLOSED);
+	SET_TX_CHANNEL_INFO(channel, fDSR, 0);
+	SET_TX_CHANNEL_INFO(channel, fCTS, 0);
+	SET_TX_CHANNEL_INFO(channel, fCD, 0);
+	SET_TX_CHANNEL_INFO(channel, fRI, 0);
+	SET_TX_CHANNEL_INFO(channel, fHEAD, 0);
+	SET_TX_CHANNEL_INFO(channel, fTAIL, 0);
+	SET_TX_CHANNEL_INFO(channel, fSTATE, 1);
+	SET_TX_CHANNEL_INFO(channel, fBLOCKREADINTR, 0);
+	SET_TX_CHANNEL_INFO(channel, head, 0);
+	SET_TX_CHANNEL_INFO(channel, tail, 0);
+
+	qcom_smd_signal_channel(channel);
+
+	channel->state = SMD_CHANNEL_CLOSED;
+	channel->pkt_size = 0;
+}
+
+/*
+ * Calculate the amount of data available in the rx fifo
+ */
+static size_t qcom_smd_channel_get_rx_avail(struct qcom_smd_channel *channel)
+{
+	unsigned head;
+	unsigned tail;
+
+	head = GET_RX_CHANNEL_INFO(channel, head);
+	tail = GET_RX_CHANNEL_INFO(channel, tail);
+
+	return (head - tail) & (channel->fifo_size - 1);
+}
+
+/*
+ * Set tx channel state and inform the remote processor
+ */
+static void qcom_smd_channel_set_state(struct qcom_smd_channel *channel,
+				       int state)
+{
+	bool is_open = state == SMD_CHANNEL_OPENED;
+
+	if (channel->state == state)
+		return;
+
+	dev_dbg(channel->smd->dev, "set_state(%s, %d)\n", channel->name, state);
+
+	SET_TX_CHANNEL_INFO(channel, fDSR, is_open);
+	SET_TX_CHANNEL_INFO(channel, fCTS, is_open);
+	SET_TX_CHANNEL_INFO(channel, fCD, is_open);
+
+	SET_TX_CHANNEL_INFO(channel, state, state);
+	SET_TX_CHANNEL_INFO(channel, fSTATE, 1);
+
+	channel->state = state;
+	qcom_smd_signal_channel(channel);
+}
+
+static size_t qcom_smd_channel_peek(struct qcom_smd_channel *channel,
+				    void *buf, size_t count)
+{
+	unsigned tail;
+	size_t len;
+
+	tail = GET_RX_CHANNEL_INFO(channel, tail);
+
+	len = min(count, channel->fifo_size - tail);
+	if (len)
+		memcpy(buf, channel->rx_fifo + tail, len);
+
+	if (len != count)
+		memcpy(buf + len, channel->rx_fifo, count - len);
+
+	return count;
+}
+
+static size_t qcom_smd_channel_read(struct qcom_smd_channel *channel,
+				    void *buf, size_t count)
+{
+	unsigned tail;
+	size_t ret;
+
+	tail = GET_RX_CHANNEL_INFO(channel, tail);
+
+	ret = qcom_smd_channel_peek(channel, buf, count);
+
+	tail += count;
+	tail &= (channel->fifo_size - 1);
+	SET_RX_CHANNEL_INFO(channel, tail, tail);
+
+	return ret;
+}
+
+/*
+ * Read out a single packet from the rx fifo and deliver it to the device
+ */
+static int qcom_smd_channel_recv_single(struct qcom_smd_channel *channel)
+{
+	struct qcom_smd_device *qsdev = channel->qsdev;
+	unsigned tail;
+	size_t len;
+	void *ptr;
+	int ret;
+
+	tail = GET_RX_CHANNEL_INFO(channel, tail);
+
+	/* Use bounce buffer if the data wraps */
+	if (tail + channel->pkt_size >= channel->fifo_size) {
+		ptr = channel->bounce_buffer;
+		len = qcom_smd_channel_peek(channel, ptr, channel->pkt_size);
+	} else {
+		ptr = channel->rx_fifo + tail;
+		len = channel->pkt_size;
+	}
+
+	spin_lock(&channel->cb_lock);
+	if (channel->cb)
+		ret = channel->cb(qsdev, ptr, len);
+	else
+		ret = 0;
+	spin_unlock(&channel->cb_lock);
+
+	/* Only forward the tail if the device consumed the data */
+	if (!ret) {
+		tail += len;
+		tail &= (channel->fifo_size - 1);
+		SET_RX_CHANNEL_INFO(channel, tail, tail);
+
+		channel->pkt_size = 0;
+	}
+
+	return ret;
+}
+
+static void qcom_smd_channel_intr(struct qcom_smd_channel *channel)
+{
+	int remote_state;
+	u32 pkt_header[5];
+	int avail;
+	int ret;
+
+	/* Handle state changes */
+	remote_state = GET_RX_CHANNEL_INFO(channel, state);
+	if (remote_state != channel->remote_state) {
+		schedule_work(&channel->edge->state_change_work);
+		channel->remote_state = remote_state;
+	}
+	/* Indicate that we have seen any state change */
+	SET_RX_CHANNEL_INFO(channel, fSTATE, 0);
+
+	/* Don't consume the data until we've opened the channel */
+	if (channel->state != SMD_CHANNEL_OPENED)
+		return;
+
+	/* Indicate that we've seen the new data */
+	SET_RX_CHANNEL_INFO(channel, fHEAD, 0);
+
+	/* Consume data */
+	for (;;) {
+		avail = qcom_smd_channel_get_rx_avail(channel);
+
+		if (channel->pkt_size == 0 && avail >= sizeof(pkt_header)) {
+			qcom_smd_channel_read(channel,
+					      pkt_header, sizeof(pkt_header));
+			channel->pkt_size = pkt_header[0];
+		} else if (channel->pkt_size && avail >= channel->pkt_size) {
+			ret = qcom_smd_channel_recv_single(channel);
+			if (ret)
+				break;
+		} else {
+			break;
+		}
+	}
+
+	/* Indicate that we have seen the updated tail */
+	SET_RX_CHANNEL_INFO(channel, fTAIL, 1);
+
+	if (!GET_RX_CHANNEL_INFO(channel, fBLOCKREADINTR)) {
+		/* Ensure ordering of channel info updates */
+		wmb();
+
+		qcom_smd_signal_channel(channel);
+	}
+}
+
+/*
+ * Calculate how much space is available in the tx fifo.
+ */
+static size_t qcom_smd_get_tx_avail(struct qcom_smd_channel *channel)
+{
+	unsigned head;
+	unsigned tail;
+	unsigned mask = channel->fifo_size - 1;
+
+	head = GET_TX_CHANNEL_INFO(channel, head);
+	tail = GET_TX_CHANNEL_INFO(channel, tail);
+
+	return mask - ((head - tail) & mask);
+}
+
+/*
+ * Write count bytes of data into channel, possibly wrapping in the ring buffer
+ */
+static int qcom_smd_write_fifo(struct qcom_smd_channel *channel,
+			       void *data,
+			       size_t count)
+{
+	unsigned head;
+	size_t len;
+
+	head = GET_TX_CHANNEL_INFO(channel, head);
+
+	len = min(count, channel->fifo_size - head);
+	if (len)
+		memcpy(channel->tx_fifo + head, data, len);
+
+	if (len != count)
+		memcpy(channel->tx_fifo, data + len, count - len);
+
+	head += count;
+	head &= (channel->fifo_size - 1);
+	SET_TX_CHANNEL_INFO(channel, head, head);
+
+	return count;
+}
+
+/**
+ * qcom_smd_send - write data to smd channel
+ * @channel:	channel handle
+ * @data:	buffer of data to write
+ * @len:	number of bytes to write
+ */
+int qcom_smd_send(struct qcom_smd_channel *channel, void *data, int len)
+{
+	u32 hdr[5] = {len,};
+
+	mutex_lock(&channel->tx_lock);
+
+	/* Wait for enough space in tx fifo */
+	while (qcom_smd_get_tx_avail(channel) < sizeof(hdr) + len)
+		usleep_range(1000, 5000);
+
+	SET_TX_CHANNEL_INFO(channel, fTAIL, 0);
+
+	qcom_smd_write_fifo(channel, hdr, sizeof(hdr));
+	qcom_smd_write_fifo(channel, data, len);
+
+	SET_TX_CHANNEL_INFO(channel, fHEAD, 1);
+
+	/* Ensure ordering of channel info updates */
+	wmb();
+
+	qcom_smd_signal_channel(channel);
+
+	mutex_unlock(&channel->tx_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(qcom_smd_send);
+
+static struct qcom_smd_device *to_smd_device(struct device *dev)
+{
+	return container_of(dev, struct qcom_smd_device, dev);
+}
+
+static struct qcom_smd_driver *to_smd_driver(struct device *dev)
+{
+	struct qcom_smd_device *qsdev = to_smd_device(dev);
+
+	return container_of(qsdev->dev.driver, struct qcom_smd_driver, driver);
+}
+
+static int qcom_smd_dev_match(struct device *dev, struct device_driver *drv)
+{
+	return of_driver_match_device(dev, drv);
+}
+
+static int qcom_smd_dev_probe(struct device *dev)
+{
+	struct qcom_smd_device *qsdev = to_smd_device(dev);
+	struct qcom_smd_driver *qsdrv = to_smd_driver(dev);
+	struct qcom_smd_channel *channel = qsdev->channel;
+	size_t bb_size;
+	int ret;
+
+	/*
+	 * Packets are maximum 4k, but reduce if the fifo is smaller
+	 */
+	bb_size = min(channel->fifo_size, 4096);
+	channel->bounce_buffer = kmalloc(bb_size, GFP_KERNEL);
+	if (!channel->bounce_buffer)
+		return -ENOMEM;
+
+	channel->cb = qsdrv->callback;
+
+	qcom_smd_channel_set_state(channel, SMD_CHANNEL_OPENING);
+
+	qcom_smd_channel_set_state(channel, SMD_CHANNEL_OPENED);
+
+	ret = qsdrv->probe(qsdev);
+	if (ret) {
+		dev_err(&qsdev->dev, "probe failed\n");
+
+		channel->cb = NULL;
+		kfree(channel->bounce_buffer);
+		channel->bounce_buffer = NULL;
+
+		qcom_smd_channel_set_state(channel, SMD_CHANNEL_CLOSED);
+	}
+
+	return ret;
+}
+
+static int qcom_smd_dev_remove(struct device *dev)
+{
+	struct qcom_smd_device *qsdev = to_smd_device(dev);
+	struct qcom_smd_driver *qsdrv = to_smd_driver(dev);
+	struct qcom_smd_channel *channel = qsdev->channel;
+	unsigned long flags;
+
+	qcom_smd_channel_set_state(channel, SMD_CHANNEL_CLOSING);
+
+	spin_lock_irqsave(&channel->cb_lock, flags);
+	channel->cb = NULL;
+	spin_unlock_irqrestore(&channel->cb_lock, flags);
+
+	if (qsdrv->remove)
+		qsdrv->remove(qsdev);
+
+	channel->qsdev = NULL;
+	kfree(channel->bounce_buffer);
+	channel->bounce_buffer = NULL;
+
+	qcom_smd_channel_set_state(channel, SMD_CHANNEL_CLOSED);
+
+	qcom_smd_channel_reset(channel);
+
+	return 0;
+}
+
+static struct bus_type qcom_smd_bus = {
+	.name = "qcom_smd",
+	.match = qcom_smd_dev_match,
+	.probe = qcom_smd_dev_probe,
+	.remove = qcom_smd_dev_remove,
+};
+
+/**
+ * qcom_smd_driver_register - register a smd driver
+ * @qsdrv:	qcom_smd_driver struct
+ */
+int qcom_smd_driver_register(struct qcom_smd_driver *qsdrv)
+{
+	qsdrv->driver.bus = &qcom_smd_bus;
+	return driver_register(&qsdrv->driver);
+}
+EXPORT_SYMBOL(qcom_smd_driver_register);
+
+/**
+ * qcom_smd_driver_unregister - unregister a smd driver
+ * @qsdrv:	qcom_smd_driver struct
+ */
+void qcom_smd_driver_unregister(struct qcom_smd_driver *qsdrv)
+{
+	driver_unregister(&qsdrv->driver);
+}
+EXPORT_SYMBOL(qcom_smd_driver_unregister);
+
+static struct qcom_smd_channel *qcom_smd_create_channel(struct qcom_smd *smd,
+							struct qcom_smd_edge *edge,
+							int channel_idx,
+							char *name)
+{
+	struct qcom_smd_channel *channel;
+	size_t fifo_size;
+	size_t info_size;
+	void *fifo_base;
+	void *info;
+	int ret;
+
+	channel = devm_kzalloc(smd->dev, sizeof(*channel), GFP_KERNEL);
+	if (!channel)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&channel->tx_lock);
+	spin_lock_init(&channel->cb_lock);
+
+	channel->name = kstrdup(name, GFP_KERNEL);
+	channel->smd = smd;
+	channel->edge = edge;
+
+	ret = qcom_smem_get(smd->smem, SMEM_SMD_INFO_BASE_ID + channel_idx,
+			    (void **)&info, &info_size);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/*
+	 * Use the size of the item to figure out which channel info struct to
+	 * use.
+	 */
+	if (info_size == 2 * sizeof(struct smd_channel_info_word)) {
+		channel->tx_info_word = info;
+		channel->rx_info_word = info + sizeof(struct smd_channel_info_word);
+	} else if (info_size == 2 * sizeof(struct smd_channel_info)) {
+		channel->tx_info = info;
+		channel->rx_info = info + sizeof(struct smd_channel_info);
+	} else {
+		dev_err(smd->dev,
+			"channel info of size %d not supported\n", info_size);
+		return ERR_PTR(-EINVAL);
+	}
+
+	ret = qcom_smem_get(smd->smem, SMEM_SMD_FIFO_BASE_ID + channel_idx,
+			    &fifo_base, &fifo_size);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/* The channel consist of a rx and tx fifo of equal size */
+	fifo_size /= 2;
+
+	dev_dbg(smd->dev, "channel '%s' info-size: %d fifo-size: %d\n",
+			  name, info_size, fifo_size);
+
+	channel->tx_fifo = fifo_base;
+	channel->rx_fifo = fifo_base + fifo_size;
+	channel->fifo_size = fifo_size;
+
+	qcom_smd_channel_reset(channel);
+
+	return channel;
+}
+
+static struct device_node *qcom_smd_match_channel(struct device_node *edge_node,
+						  const char *channel)
+{
+	struct device_node *child;
+	const char *name;
+	const char *key;
+	int ret;
+
+	for_each_available_child_of_node(edge_node, child) {
+		key = "qcom,smd-channels";
+		ret = of_property_read_string(child, key, &name);
+		if (ret)
+			continue;
+
+		if (strcmp(name, channel) == 0)
+			return child;
+	}
+
+	return NULL;
+}
+
+static void qcom_smd_release_device(struct device *dev)
+{
+	struct qcom_smd_device *qsdev = to_smd_device(dev);
+
+	kfree(qsdev);
+}
+
+static int qcom_smd_create_device(struct qcom_smd_channel *channel)
+{
+	struct qcom_smd_device *qsdev;
+	struct qcom_smd_edge *edge = channel->edge;
+	struct device_node *node;
+	struct qcom_smd *smd = channel->smd;
+	int ret;
+
+	if (channel->qsdev)
+		return -EEXIST;
+
+	node = qcom_smd_match_channel(edge->of_node, channel->name);
+	if (!node) {
+		dev_dbg(smd->dev, "no match for '%s'\n", channel->name);
+		return -ENXIO;
+	}
+
+	dev_dbg(smd->dev, "registering '%s'\n", channel->name);
+
+	qsdev = kzalloc(sizeof(*qsdev), GFP_KERNEL);
+	if (!qsdev)
+		return -ENOMEM;
+
+	dev_set_name(&qsdev->dev, "%s.%s", edge->of_node->name, node->name);
+	qsdev->dev.parent = smd->dev;
+	qsdev->dev.bus = &qcom_smd_bus;
+	qsdev->dev.release = qcom_smd_release_device;
+	qsdev->dev.of_node = node;
+
+	qsdev->channel = channel;
+
+	channel->qsdev = qsdev;
+
+	ret = device_register(&qsdev->dev);
+	if (ret) {
+		dev_err(smd->dev, "device_register failed: %d\n", ret);
+		put_device(&qsdev->dev);
+	}
+
+	return ret;
+}
+
+static void qcom_smd_destroy_device(struct qcom_smd_channel *channel)
+{
+	struct device *dev;
+
+	BUG_ON(!channel->qsdev);
+
+	dev = &channel->qsdev->dev;
+
+	device_unregister(dev);
+	put_device(dev);
+}
+
+static void qcom_channel_scan_worker(struct work_struct *work)
+{
+	struct qcom_smd_edge *edge = container_of(work,
+						  struct qcom_smd_edge,
+						  channel_scan_work);
+	struct qcom_smd_channel *channel;
+	struct qcom_smd_alloc_entry *entry;
+	struct qcom_smd *smd = edge->smd;
+	int ret;
+	int i;
+
+	ret = qcom_smem_get(smd->smem, SMEM_CHANNEL_ALLOC_TBL,
+			    (void **)&entry, NULL);
+	if (ret < 0) {
+		dev_err(smd->dev,
+			"smem item %d not allocated\n", SMEM_CHANNEL_ALLOC_TBL);
+		return;
+	};
+
+	for (i = 0; i < SMD_NUM_CHANNELS; i++) {
+		if (test_bit(i, smd->allocated))
+			continue;
+
+		if (entry[i].ref_count == 0)
+			continue;
+
+		if (!entry[i].name[0])
+			continue;
+
+		if (!(entry[i].flags & SMD_CHANNEL_FLAGS_PACKET))
+			continue;
+
+		if ((entry[i].flags & SMD_CHANNEL_FLAGS_EDGE) != edge->edge_id)
+			continue;
+
+		channel = qcom_smd_create_channel(smd, edge,
+						  entry[i].cid,
+						  entry[i].name);
+		if (IS_ERR(channel))
+			continue;
+
+		list_add(&channel->list, &edge->channels);
+
+		dev_dbg(smd->dev, "new channel found: '%s'\n", channel->name);
+		set_bit(i, smd->allocated);
+	}
+
+	schedule_work(&edge->state_change_work);
+}
+
+static void qcom_channel_state_worker(struct work_struct *work)
+{
+	struct qcom_smd_channel *channel;
+	struct qcom_smd_edge *edge = container_of(work,
+						  struct qcom_smd_edge,
+						  state_change_work);
+	unsigned remote_state;
+
+	/*
+	 * Register a device for any closed channel where the remote processor
+	 * is showing interest in opening the channel.
+	 */
+	list_for_each_entry(channel, &edge->channels, list) {
+		if (channel->state != SMD_CHANNEL_CLOSED)
+			continue;
+
+		remote_state = GET_RX_CHANNEL_INFO(channel, state);
+		if (remote_state != SMD_CHANNEL_OPENING &&
+		    remote_state != SMD_CHANNEL_OPENED)
+			continue;
+
+		qcom_smd_create_device(channel);
+	}
+
+	/*
+	 * Unregister the device for any channel that is opened where the
+	 * remote processor is closing the channel.
+	 */
+	list_for_each_entry(channel, &edge->channels, list) {
+		if (channel->state != SMD_CHANNEL_OPENING &&
+		    channel->state != SMD_CHANNEL_OPENED)
+			continue;
+
+		remote_state = GET_RX_CHANNEL_INFO(channel, state);
+		if (remote_state == SMD_CHANNEL_OPENING ||
+		    remote_state == SMD_CHANNEL_OPENED)
+			continue;
+
+		qcom_smd_destroy_device(channel);
+	}
+}
+
+/*
+ * The edge interrupts are triggered by the remote processor on state changes,
+ * channel info updates or when new channels are created.
+ */
+static irqreturn_t qcom_smd_edge_intr(int irq, void *data)
+{
+	struct qcom_smd_edge *edge = data;
+	struct qcom_smd_channel *channel;
+	unsigned available;
+
+	/*
+	 * Handle state changes or data on each of the channels on this edge
+	 */
+	list_for_each_entry(channel, &edge->channels, list)
+		qcom_smd_channel_intr(channel);
+
+	/*
+	 * Creating a new channel requires allocating an smem entry, so we only
+	 * have to scan if the amount of available space in smem have changed
+	 * since last scan.
+	 */
+	available = qcom_smem_get_free_space(edge->smd->smem);
+	if (available != edge->smem_available) {
+		edge->smem_available = available;
+		schedule_work(&edge->channel_scan_work);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int qcom_smd_parse_edge(struct device *dev,
+			       struct device_node *node,
+			       struct qcom_smd_edge *edge)
+{
+	struct device_node *syscon_np;
+	const char *key;
+	int irq;
+	int ret;
+
+	INIT_LIST_HEAD(&edge->channels);
+
+	INIT_WORK(&edge->channel_scan_work, qcom_channel_scan_worker);
+	INIT_WORK(&edge->state_change_work, qcom_channel_state_worker);
+
+	edge->of_node = of_node_get(node);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq < 0) {
+		dev_err(dev, "required smd interrupt missing\n");
+		return -EINVAL;
+	}
+
+	ret = devm_request_irq(dev, irq,
+			       qcom_smd_edge_intr, IRQF_TRIGGER_RISING,
+			       node->name, edge);
+	if (ret) {
+		dev_err(dev, "failed to request smd irq\n");
+		return ret;
+	}
+
+	key = "qcom,smd-edge";
+	ret = of_property_read_u32(node, key, &edge->edge_id);
+	if (ret) {
+		dev_err(dev, "edge missing %s property\n", key);
+		return -EINVAL;
+	}
+
+	syscon_np = of_parse_phandle(node, "qcom,ipc", 0);
+	if (!syscon_np) {
+		dev_err(dev, "no qcom,ipc node\n");
+		return -ENODEV;
+	}
+
+	edge->ipc_regmap = syscon_node_to_regmap(syscon_np);
+	if (IS_ERR(edge->ipc_regmap))
+		return PTR_ERR(edge->ipc_regmap);
+
+	key = "qcom,ipc";
+	ret = of_property_read_u32_index(node, key, 1, &edge->ipc_offset);
+	if (ret < 0) {
+		dev_err(dev, "no offset in %s\n", key);
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32_index(node, key, 2, &edge->ipc_bit);
+	if (ret < 0) {
+		dev_err(dev, "no bit in %s\n", key);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_smd_probe(struct platform_device *pdev)
+{
+	struct qcom_smd_edge *edge;
+	struct device_node *node;
+	struct qcom_smd *smd;
+	size_t array_size;
+	int count;
+	int ret;
+	int i = 0;
+
+	count = of_get_child_count(pdev->dev.of_node);
+	array_size = sizeof(*smd) + count * sizeof(struct qcom_smd_edge);
+	smd = devm_kzalloc(&pdev->dev, array_size, GFP_KERNEL);
+	if (!smd)
+		return -ENOMEM;
+	smd->dev = &pdev->dev;
+
+	smd->smem = of_get_qcom_smem(pdev->dev.of_node);
+	if (IS_ERR(smd->smem)) {
+		if (PTR_ERR(smd->smem) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to acquire smem handle\n");
+		return PTR_ERR(smd->smem);
+	}
+
+	dev_set_drvdata(&pdev->dev, smd);
+
+	smd->num_edges = count;
+	for_each_child_of_node(pdev->dev.of_node, node) {
+		edge = &smd->edges[i++];
+		edge->smd = smd;
+
+		ret = qcom_smd_parse_edge(&pdev->dev, node, edge);
+		if (ret)
+			continue;
+
+		schedule_work(&edge->channel_scan_work);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id qcom_smd_of_match[] = {
+	{ .compatible = "qcom,smd" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_smd_of_match);
+
+static struct platform_driver qcom_smd_driver = {
+	.probe = qcom_smd_probe,
+	.driver = {
+		.name = "qcom_smd",
+		.owner = THIS_MODULE,
+		.of_match_table = qcom_smd_of_match,
+	},
+};
+
+static int __init qcom_smd_init(void)
+{
+	int ret;
+
+	ret = bus_register(&qcom_smd_bus);
+	if (ret) {
+		pr_err("failed to register smd bus: %d\n", ret);
+		return ret;
+	}
+
+	return platform_driver_register(&qcom_smd_driver);
+}
+arch_initcall(qcom_smd_init);
+
+static void __exit qcom_smd_exit(void)
+{
+	platform_driver_unregister(&qcom_smd_driver);
+	bus_unregister(&qcom_smd_bus);
+}
+module_exit(qcom_smd_exit);
+
+MODULE_DESCRIPTION("Qualcomm Shared Memory Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/qcom/qcom_smd.h b/include/linux/soc/qcom/qcom_smd.h
new file mode 100644
index 0000000..dad2c86
--- /dev/null
+++ b/include/linux/soc/qcom/qcom_smd.h
@@ -0,0 +1,47 @@
+#ifndef __LINUX_MSM_SMEM_H__
+#define __LINUX_MSM_SMEM_H__
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+
+struct qcom_smd;
+struct qcom_smd_channel;
+struct qcom_smd_lookup;
+
+/**
+ * struct qcom_smd_device - smd device struct
+ * @dev:	the device struct
+ * @channel:	handle to the smd channel for this device
+ */
+struct qcom_smd_device {
+	struct device dev;
+	struct qcom_smd_channel *channel;
+};
+
+/**
+ * struct qcom_smd_driver - smd driver struct
+ * @driver:	underlying device driver
+ * @probe:	invoked when the smd channel is found
+ * @remove:	invoked when the smd channel is closed
+ * @callback:	invoked when an inbound message is received on the channel,
+ * 		should return 0 on success or -EBUSY if the data cannot be
+ *		consumed at this time
+ */
+struct qcom_smd_driver {
+	struct device_driver driver;
+	int (*probe)(struct qcom_smd_device *dev);
+	void (*remove)(struct qcom_smd_device *dev);
+	int (*callback)(struct qcom_smd_device *, void *, size_t);
+};
+
+int qcom_smd_driver_register(struct qcom_smd_driver *drv);
+void qcom_smd_driver_unregister(struct qcom_smd_driver *drv);
+
+#define module_qcom_smd_driver(__smd_driver) \
+	module_driver(__smd_driver, qcom_smd_driver_register, \
+		      qcom_smd_driver_unregister)
+
+int qcom_smd_send(struct qcom_smd_channel *channel, void *data, int len);
+
+#endif
+
-- 
1.7.9.5

[RFC 6/7] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD

[Bjorn Andersson]

Driver for the Resource Power Manager (RPM) found in Qualcomm 8974 based
devices.
The driver exposes resources that child drivers can operate on; to
implementing regulator, clock and bus frequency drivers.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

Note that the qcom_rpm_smd_write is equivalent of qcom_rpm_write and there is a
possibility of re-using at least the clock implementation on top of this. This
would however require some logic for calling the right implementation so I have
not done it at this time to keep things as clean as possible.

An idea for improvement is that in qcom_rpm_smd_write we put the ack_status and
completion on the stack and register this with idr using the message id, upon
receiving the interrupt we would find the right client and complete this.
Allowing for multiple requests to be in flight at any given time.

I did not implement this because I haven't done any measurements on what kind
of improvements this could give and it would be a clean iteration ontop of
this.

 drivers/mfd/Kconfig              |   14 ++
 drivers/mfd/Makefile             |    1 +
 drivers/mfd/qcom-smd-rpm.c       |  299 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/qcom-smd-rpm.h |    9 ++
 4 files changed, 323 insertions(+)
 create mode 100644 drivers/mfd/qcom-smd-rpm.c
 create mode 100644 include/linux/mfd/qcom-smd-rpm.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6743e88..c62c7f5 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -553,6 +553,20 @@ config MFD_QCOM_RPM
 	  Say M here if you want to include support for the Qualcomm RPM as a
 	  module. This will build a module called "qcom_rpm".
 
+config MFD_QCOM_SMD_RPM
+	tristate "Qualcomm Resource Power Manager (RPM) over SMD"
+	depends on QCOM_SMD && OF
+	help
+	  If you say yes to this option, support will be included for the
+	  Resource Power Manager system found in the Qualcomm 8974 based
+	  devices.
+
+	  This is required to access many regulators, clocks and bus
+	  frequencies controlled by the RPM on these devices.
+
+	  Say M here if you want to include support for the Qualcomm RPM as a
+	  module. This will build a module called "qcom-smd-rpm".
+
 config MFD_RDC321X
 	tristate "RDC R-321x southbridge"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 3f2fc89..e19ab12 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -154,6 +154,7 @@ obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
 obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
 obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o ssbi.o
 obj-$(CONFIG_MFD_QCOM_RPM)	+= qcom_rpm.o
+obj-$(CONFIG_MFD_QCOM_SMD_RPM)	+= qcom-smd-rpm.o
 obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
 obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c
new file mode 100644
index 0000000..3f77f87
--- /dev/null
+++ b/drivers/mfd/qcom-smd-rpm.c
@@ -0,0 +1,299 @@
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB.
+ * Copyright (c) 2012-2013, 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+
+#include <linux/soc/qcom/qcom_smd.h>
+#include <linux/mfd/qcom-smd-rpm.h>
+
+#include <dt-bindings/mfd/qcom-rpm.h>
+
+#define RPM_REQUEST_TIMEOUT     (5 * HZ)
+
+struct qcom_rpm_resource {
+	u32 resource_type;
+	u32 resource_id;
+};
+
+struct qcom_rpm_data {
+	const struct qcom_rpm_resource *resource_table;
+	unsigned nresources;
+};
+
+struct qcom_smd_rpm {
+	struct device *dev;
+	struct qcom_smd_channel *rpm_channel;
+
+	struct completion ack;
+	struct mutex lock;
+	int ack_status;
+
+	const struct qcom_rpm_data *data;
+};
+
+struct qcom_rpm_header {
+	u32 service_type;
+	u32 length;
+};
+
+struct qcom_rpm_request {
+	u32 msg_id;
+	u32 flags;
+	u32 resource_type;
+	u32 resource_id;
+	u32 data_len;
+};
+
+struct qcom_rpm_message {
+	u32 msg_type;
+	u32 length;
+	union {
+		u32 msg_id;
+		u8 message[0];
+	};
+};
+
+#define RPM_SERVICE_TYPE_REQUEST	0x00716572 /* "req\0" */
+
+#define RPM_MSG_TYPE_ERR		0x00727265 /* "err\0" */
+#define RPM_MSG_TYPE_MSG_ID		0x2367736d /* "msg#" */
+
+#define RESOURCE_TYPE_SMPA		0x61706d73 /* "smpa" */
+#define RESOURCE_TYPE_SMPB		0x62706d73 /* "smpb" */
+#define RESOURCE_TYPE_LDOA		0x616f646c /* "ldoa" */
+#define RESOURCE_TYPE_VSA		0x00617376 /* "vsa\0" */
+
+#define RPM_MSG_FLAGS_SET_ACTIVE_MODE	BIT(0)
+#define RPM_MSG_FLAGS_SET_SLEEP_MODE	BIT(1)
+
+static const struct qcom_rpm_resource msm8x74_resource_table[] = {
+	[QCOM_RPM_PM8841_SMPS1] = { RESOURCE_TYPE_SMPB, 1 },
+	[QCOM_RPM_PM8841_SMPS2] = { RESOURCE_TYPE_SMPB, 2 },
+	[QCOM_RPM_PM8841_SMPS3] = { RESOURCE_TYPE_SMPB, 3 },
+	[QCOM_RPM_PM8841_SMPS4] = { RESOURCE_TYPE_SMPB, 4 },
+
+	[QCOM_RPM_PM8941_SMPS1] = { RESOURCE_TYPE_SMPA, 1 },
+	[QCOM_RPM_PM8941_SMPS2] = { RESOURCE_TYPE_SMPA, 2 },
+	[QCOM_RPM_PM8941_SMPS3] = { RESOURCE_TYPE_SMPA, 3 },
+
+	[QCOM_RPM_PM8941_LDO1] =  { RESOURCE_TYPE_LDOA, 1 },
+	[QCOM_RPM_PM8941_LDO2] =  { RESOURCE_TYPE_LDOA, 2 },
+	[QCOM_RPM_PM8941_LDO3] =  { RESOURCE_TYPE_LDOA, 3 },
+	[QCOM_RPM_PM8941_LDO4] =  { RESOURCE_TYPE_LDOA, 4 },
+	[QCOM_RPM_PM8941_LDO5] =  { RESOURCE_TYPE_LDOA, 5 },
+	[QCOM_RPM_PM8941_LDO6] =  { RESOURCE_TYPE_LDOA, 6 },
+	[QCOM_RPM_PM8941_LDO7] =  { RESOURCE_TYPE_LDOA, 7 },
+	[QCOM_RPM_PM8941_LDO8] =  { RESOURCE_TYPE_LDOA, 8 },
+	[QCOM_RPM_PM8941_LDO9] =  { RESOURCE_TYPE_LDOA, 9 },
+	[QCOM_RPM_PM8941_LDO10] = { RESOURCE_TYPE_LDOA, 10 },
+	[QCOM_RPM_PM8941_LDO11] = { RESOURCE_TYPE_LDOA, 11 },
+	[QCOM_RPM_PM8941_LDO12] = { RESOURCE_TYPE_LDOA, 12 },
+	[QCOM_RPM_PM8941_LDO13] = { RESOURCE_TYPE_LDOA, 13 },
+	[QCOM_RPM_PM8941_LDO14] = { RESOURCE_TYPE_LDOA, 14 },
+	[QCOM_RPM_PM8941_LDO15] = { RESOURCE_TYPE_LDOA, 15 },
+	[QCOM_RPM_PM8941_LDO16] = { RESOURCE_TYPE_LDOA, 16 },
+	[QCOM_RPM_PM8941_LDO17] = { RESOURCE_TYPE_LDOA, 17 },
+	[QCOM_RPM_PM8941_LDO18] = { RESOURCE_TYPE_LDOA, 18 },
+	[QCOM_RPM_PM8941_LDO19] = { RESOURCE_TYPE_LDOA, 19 },
+	[QCOM_RPM_PM8941_LDO20] = { RESOURCE_TYPE_LDOA, 20 },
+	[QCOM_RPM_PM8941_LDO21] = { RESOURCE_TYPE_LDOA, 21 },
+	[QCOM_RPM_PM8941_LDO22] = { RESOURCE_TYPE_LDOA, 22 },
+	[QCOM_RPM_PM8941_LDO23] = { RESOURCE_TYPE_LDOA, 23 },
+	[QCOM_RPM_PM8941_LDO24] = { RESOURCE_TYPE_LDOA, 24 },
+
+	[QCOM_RPM_PM8941_LVS1] =  { RESOURCE_TYPE_VSA, 1 },
+	[QCOM_RPM_PM8941_LVS2] =  { RESOURCE_TYPE_VSA, 2 },
+	[QCOM_RPM_PM8941_LVS3] =  { RESOURCE_TYPE_VSA, 3 },
+
+	[QCOM_RPM_PM8941_MVS1] =  { RESOURCE_TYPE_VSA, 4 },
+	[QCOM_RPM_PM8941_MVS2] =  { RESOURCE_TYPE_VSA, 5 },
+};
+
+static const struct qcom_rpm_data msm8x74_template = {
+	.resource_table = msm8x74_resource_table,
+	.nresources = ARRAY_SIZE(msm8x74_resource_table),
+};
+
+static const struct of_device_id qcom_smd_rpm_of_match[] = {
+	{ .compatible = "qcom,rpm-msm8974", .data = &msm8x74_template },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_smd_rpm_of_match);
+
+/**
+ * qcom_rpm_smd_write - write @buf to @resource
+ * @rpm:	rpm handle
+ * @resource:	resource identifier
+ * @buf:	the data to be written
+ * @count:	number of bytes in @buf
+ */
+int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm,
+		       int resource,
+		       void *buf,
+		       size_t count)
+{
+	const struct qcom_rpm_resource *res;
+	const struct qcom_rpm_data *data = rpm->data;
+	static unsigned msg_id = 1;
+	int left;
+	int ret;
+
+	struct {
+		struct qcom_rpm_header hdr;
+		struct qcom_rpm_request req;
+		u8 payload[count];
+	} pkt;
+
+	/* SMD packets to the RPM may not exceed 256 bytes */
+	if (WARN_ON(sizeof(pkt) >= 256))
+		return -EINVAL;
+
+	if (WARN_ON(resource < 0 || resource >= data->nresources))
+		return -EINVAL;
+
+	res = &data->resource_table[resource];
+	if (WARN_ON(!res->resource_id || !res->resource_type))
+		return -EINVAL;
+
+	mutex_lock(&rpm->lock);
+
+	pkt.hdr.service_type = RPM_SERVICE_TYPE_REQUEST;
+	pkt.hdr.length = sizeof(struct qcom_rpm_request) + count;
+
+	pkt.req.msg_id = msg_id++;
+	pkt.req.flags = RPM_MSG_FLAGS_SET_ACTIVE_MODE;
+	pkt.req.resource_type = res->resource_type;
+	pkt.req.resource_id = res->resource_id;
+	pkt.req.data_len = count;
+	memcpy(pkt.payload, buf, count);
+
+	ret = qcom_smd_send(rpm->rpm_channel, &pkt, sizeof(pkt));
+	if (ret)
+		goto out;
+
+	left = wait_for_completion_timeout(&rpm->ack, RPM_REQUEST_TIMEOUT);
+	if (!left)
+		ret = -ETIMEDOUT;
+	else
+		ret = rpm->ack_status;
+
+out:
+	mutex_unlock(&rpm->lock);
+	return ret;
+}
+EXPORT_SYMBOL(qcom_rpm_smd_write);
+
+#define RPM_ERR_INVALID_RESOURCE "resource does not exist"
+
+static bool qcom_rpm_msg_is_invalid_resource(struct qcom_rpm_message *msg)
+{
+	size_t msg_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1;
+
+	if (msg->length != msg_len)
+		return false;
+
+	if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE, msg_len))
+		return false;
+
+	return true;
+}
+
+static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev,
+				 void *data,
+				 size_t count)
+{
+	struct qcom_rpm_header *hdr = data;
+	struct qcom_rpm_message *msg;
+	struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev);
+	u8 *buf = data + sizeof(struct qcom_rpm_header);
+	u8 *end = buf + hdr->length;
+	int status = 0;
+
+	if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST ||
+	    hdr->length < sizeof(struct qcom_rpm_message)) {
+		dev_err(rpm->dev, "invalid request\n");
+		return 0;
+	}
+
+	while (buf < end) {
+		msg = (struct qcom_rpm_message *)buf;
+		switch (msg->msg_type) {
+		case RPM_MSG_TYPE_MSG_ID:
+			break;
+		case RPM_MSG_TYPE_ERR:
+			if (qcom_rpm_msg_is_invalid_resource(msg))
+				status = -ENXIO;
+			else
+				status = -EIO;
+			break;
+		}
+
+		buf = PTR_ALIGN(buf + 2 * sizeof(u32) + msg->length, 4);
+	}
+
+	rpm->ack_status = status;
+	complete(&rpm->ack);
+	return 0;
+}
+
+static int qcom_smd_rpm_probe(struct qcom_smd_device *sdev)
+{
+	const struct of_device_id *match;
+	struct qcom_smd_rpm *rpm;
+
+	rpm = devm_kzalloc(&sdev->dev, sizeof(*rpm), GFP_KERNEL);
+	if (!rpm)
+		return -ENOMEM;
+
+	rpm->dev = &sdev->dev;
+	mutex_init(&rpm->lock);
+	init_completion(&rpm->ack);
+
+	match = of_match_device(qcom_smd_rpm_of_match, &sdev->dev);
+	rpm->data = match->data;
+	rpm->rpm_channel = sdev->channel;
+
+	dev_set_drvdata(&sdev->dev, rpm);
+
+	dev_info(&sdev->dev, "Qualcomm SMD RPM driver probed\n");
+
+	return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
+}
+
+static void qcom_smd_rpm_remove(struct qcom_smd_device *sdev)
+{
+	dev_set_drvdata(&sdev->dev, NULL);
+	of_platform_depopulate(&sdev->dev);
+}
+
+static struct qcom_smd_driver qcom_smd_rpm_driver = {
+	.probe = qcom_smd_rpm_probe,
+	.remove = qcom_smd_rpm_remove,
+	.callback = qcom_smd_rpm_callback,
+	.driver  = {
+		.name  = "qcom_smd_rpm",
+		.owner = THIS_MODULE,
+		.of_match_table = qcom_smd_rpm_of_match,
+	},
+};
+
+module_qcom_smd_driver(qcom_smd_rpm_driver);
+
+MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>");
+MODULE_DESCRIPTION("Qualcomm SMD backed RPM driver");
+MODULE_LICENSE("GPLv2");
diff --git a/include/linux/mfd/qcom-smd-rpm.h b/include/linux/mfd/qcom-smd-rpm.h
new file mode 100644
index 0000000..59b9425
--- /dev/null
+++ b/include/linux/mfd/qcom-smd-rpm.h
@@ -0,0 +1,9 @@
+#ifndef __QCOM_SMD_RPM_H__
+#define __QCOM_SMD_RPM_H__
+
+struct qcom_smd_rpm;
+
+int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm, int resource,
+		       void *buf, size_t count);
+
+#endif
-- 
1.7.9.5

[RFC 7/7] regulator: qcom-smd-rpm: Regulator driver for the Qualcomm RPM

[Bjorn Andersson]

Driver for regulators exposed by the Resource Power Manager (RPM) found
in Qualcomm 8974 based devices.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

According to the datasheet for the PMIC the regulators are indeed programmed in
steps, but the steps seems to vary between different regulators and the details
are hidden by the RPM that exposes contiguous voltage ranges.

Either we run with this, add a few more compatibles to encode the steps or have
the step coming from devicetree. I prefer the current implementation as that is
the cleanest of these.

 drivers/regulator/Kconfig              |   12 ++
 drivers/regulator/Makefile             |    1 +
 drivers/regulator/qcom_smd-regulator.c |  229 ++++++++++++++++++++++++++++++++
 3 files changed, 242 insertions(+)
 create mode 100644 drivers/regulator/qcom_smd-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 0e59754..ad01acf 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -461,6 +461,18 @@ config REGULATOR_QCOM_RPM
 	  Qualcomm RPM as a module. The module will be named
 	  "qcom_rpm-regulator".
 
+config REGULATOR_QCOM_SMD_RPM
+	tristate "Qualcomm SMD based RPM regulator driver"
+	depends on MFD_QCOM_SMD_RPM
+	help
+	  If you say yes to this option, support will be included for the
+	  regulators exposed by the Resource Power Manager found in Qualcomm
+	  8974 based devices.
+
+	  Say M here if you want to include support for the regulators on the
+	  Qualcomm RPM as a module. The module will be named
+	  "qcom_smd-regulator".
+
 config REGULATOR_RC5T583
 	tristate "RICOH RC5T583 Power regulators"
 	depends on MFD_RC5T583
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 9c50dc6..b022270 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
 obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
 obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
 obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
 obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
 obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o
 obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
new file mode 100644
index 0000000..510cb44
--- /dev/null
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -0,0 +1,229 @@
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB.
+ * Copyright (c) 2012-2013, 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/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/mfd/qcom-smd-rpm.h>
+#include <dt-bindings/mfd/qcom-rpm.h>
+
+struct qcom_rpm_reg {
+	struct qcom_smd_rpm *rpm;
+	unsigned resource;
+
+	struct regulator_desc desc;
+
+	int is_enabled;
+	int uV;
+};
+
+struct rpm_regulator_req {
+	u32 key;
+	u32 nbytes;
+	u32 value;
+};
+
+#define RPM_KEY_SWEN	0x6e657773 /* "swen" */
+#define RPM_KEY_UV	0x00007675 /* "uv" */
+#define RPM_KEY_MV	0x0000616d /* "ma" */
+
+static int rpm_reg_enable(struct regulator_dev *rdev)
+{
+	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+	struct rpm_regulator_req req;
+	int ret;
+
+	req.key = RPM_KEY_SWEN;
+	req.nbytes = sizeof(u32);
+	req.value = 1;
+
+	ret = qcom_rpm_smd_write(vreg->rpm, vreg->resource, &req, sizeof(req));
+	if (!ret)
+		vreg->is_enabled = 1;
+
+	return ret;
+}
+
+static int rpm_reg_is_enabled(struct regulator_dev *rdev)
+{
+	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+
+	return vreg->is_enabled;
+}
+
+static int rpm_reg_disable(struct regulator_dev *rdev)
+{
+	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+	struct rpm_regulator_req req;
+	int ret;
+
+	req.key = RPM_KEY_SWEN;
+	req.nbytes = sizeof(u32);
+	req.value = 0;
+
+	ret = qcom_rpm_smd_write(vreg->rpm, vreg->resource, &req, sizeof(req));
+	if (!ret)
+		vreg->is_enabled = 0;
+
+	return ret;
+}
+
+static int rpm_reg_get_voltage(struct regulator_dev *rdev)
+{
+	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+
+	return vreg->uV;
+}
+
+static int rpm_reg_set_voltage(struct regulator_dev *rdev,
+			       int min_uV,
+			       int max_uV,
+			       unsigned *selector)
+{
+	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+	struct rpm_regulator_req req;
+	int ret = 0;
+
+	req.key = RPM_KEY_UV;
+	req.nbytes = sizeof(u32);
+	req.value = min_uV;
+
+	ret = qcom_rpm_smd_write(vreg->rpm, vreg->resource, &req, sizeof(req));
+	if (!ret)
+		vreg->uV = min_uV;
+
+	return ret;
+}
+
+static struct regulator_ops rpm_smps_ops = {
+	.enable = rpm_reg_enable,
+	.disable = rpm_reg_disable,
+	.is_enabled = rpm_reg_is_enabled,
+
+	.get_voltage = rpm_reg_get_voltage,
+	.set_voltage = rpm_reg_set_voltage,
+};
+
+static struct regulator_ops rpm_ldo_ops = {
+	.enable = rpm_reg_enable,
+	.disable = rpm_reg_disable,
+	.is_enabled = rpm_reg_is_enabled,
+
+	.get_voltage = rpm_reg_get_voltage,
+	.set_voltage = rpm_reg_set_voltage,
+};
+
+static struct regulator_ops rpm_switch_ops = {
+	.enable = rpm_reg_enable,
+	.disable = rpm_reg_disable,
+	.is_enabled = rpm_reg_is_enabled,
+};
+
+static const struct of_device_id rpm_of_match[] = {
+	{ .compatible = "qcom,rpm-pm8841-smps", .data = &rpm_smps_ops },
+	{ .compatible = "qcom,rpm-pm8941-smps", .data = &rpm_smps_ops },
+	{ .compatible = "qcom,rpm-pm8941-ldo", .data = &rpm_ldo_ops},
+	{ .compatible = "qcom,rpm-pm8941-switch", .data = &rpm_switch_ops },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rpm_of_match);
+
+static int rpm_reg_probe(struct platform_device *pdev)
+{
+	struct regulator_init_data *initdata;
+	const struct of_device_id *match;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+	struct qcom_rpm_reg *vreg;
+	const char *key;
+	u32 val;
+	int ret;
+
+	match = of_match_device(rpm_of_match, &pdev->dev);
+
+	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node);
+	if (!initdata)
+		return -EINVAL;
+
+	vreg = devm_kzalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
+	if (!vreg)
+		return -ENOMEM;
+
+	vreg->desc.continuous_voltage_range = true;
+	vreg->desc.id = -1;
+	vreg->desc.name = pdev->dev.of_node->name;
+	vreg->desc.ops = (struct regulator_ops *)match->data;
+	vreg->desc.owner = THIS_MODULE;
+	vreg->desc.type = REGULATOR_VOLTAGE;
+
+	vreg->rpm = dev_get_drvdata(pdev->dev.parent);
+	if (!vreg->rpm) {
+		dev_err(&pdev->dev, "unable to get rpm handle\n");
+		return -ENXIO;
+	}
+
+	key = "reg";
+	ret = of_property_read_u32(pdev->dev.of_node, key, &val);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to read %s\n", key);
+		return ret;
+	}
+	vreg->resource = val;
+
+	if (vreg->desc.ops->set_voltage &&
+	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
+		dev_err(&pdev->dev, "no voltage specified for regulator\n");
+		return -EINVAL;
+	}
+
+	config.dev = &pdev->dev;
+	config.init_data = initdata;
+	config.driver_data = vreg;
+	config.of_node = pdev->dev.of_node;
+	rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "can't register regulator\n");
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+static struct platform_driver rpm_reg_driver = {
+	.probe = rpm_reg_probe,
+	.driver = {
+		.name  = "qcom_rpm_smd_regulator",
+		.owner = THIS_MODULE,
+		.of_match_table = rpm_of_match,
+	},
+};
+
+static int __init rpm_reg_init(void)
+{
+	return platform_driver_register(&rpm_reg_driver);
+}
+subsys_initcall(rpm_reg_init);
+
+static void __exit rpm_reg_exit(void)
+{
+	platform_driver_unregister(&rpm_reg_driver);
+}
+module_exit(rpm_reg_exit)
+
+MODULE_DESCRIPTION("Qualcomm RPM regulator driver");
+MODULE_LICENSE("GPLv2");
-- 
1.7.9.5

Re: [RFC 4/7] soc: qcom: Add Shared Memory Manager driver

[Kiran Padwal]

Hi Bjorn,

On Tuesday 30 September 2014 06:04 AM, Bjorn Andersson wrote:
> The Shared Memory Manager driver implements an interface for allocating
> and accessing items in the memory area shared among all of the
> processors in a Qualcomm platform.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---

<snip>

> +
> +static int qcom_smem_probe(struct platform_device *pdev)
> +{
> +	struct qcom_smem *smem;
> +	struct resource *res;
> +	size_t array_size;
> +	int num_regions = 0;
> +	int i;
> +
> +	for (i = 0; i < pdev->num_resources; i++) {
> +		res = &pdev->resource[i];
> +
> +		if (resource_type(res) == IORESOURCE_MEM)
> +			num_regions++;
> +	}
> +
> +	if (num_regions == 0) {
> +		dev_err(&pdev->dev, "no smem regions specified\n");
> +		return -EINVAL;
> +	}
> +
> +	array_size = num_regions * sizeof(struct smem_region);
> +	smem = devm_kzalloc(&pdev->dev, sizeof(*smem) + array_size, GFP_KERNEL);
> +	if (!smem)
> +		return -ENOMEM;
> +
> +	smem->dev = &pdev->dev;
> +	smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL);

Compilation breaks while I try to compile with this patch.
Do I missing anything? Below is the error I am getting,

drivers/soc/qcom/qcom_smem.c: In function ‘qcom_smem_probe’:
drivers/soc/qcom/qcom_smem.c:274:2: error: implicit declaration of function ‘of_hwspin_lock_request’ [-Werror=implicit-function-declaration]
  smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL)

Thanks,
--Kiran

Re: [RFC 4/7] soc: qcom: Add Shared Memory Manager driver

[Kiran Padwal]

Hi,

On Tuesday 30 September 2014 11:47 AM, Kiran Padwal wrote:
> Hi Bjorn,
> 
> On Tuesday 30 September 2014 06:04 AM, Bjorn Andersson wrote:
>> The Shared Memory Manager driver implements an interface for allocating
>> and accessing items in the memory area shared among all of the
>> processors in a Qualcomm platform.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> ---
> 
> <snip>
> 
>> +
>> +static int qcom_smem_probe(struct platform_device *pdev)
>> +{
>> +	struct qcom_smem *smem;
>> +	struct resource *res;
>> +	size_t array_size;
>> +	int num_regions = 0;
>> +	int i;
>> +
>> +	for (i = 0; i < pdev->num_resources; i++) {
>> +		res = &pdev->resource[i];
>> +
>> +		if (resource_type(res) == IORESOURCE_MEM)
>> +			num_regions++;
>> +	}
>> +
>> +	if (num_regions == 0) {
>> +		dev_err(&pdev->dev, "no smem regions specified\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	array_size = num_regions * sizeof(struct smem_region);
>> +	smem = devm_kzalloc(&pdev->dev, sizeof(*smem) + array_size, GFP_KERNEL);
>> +	if (!smem)
>> +		return -ENOMEM;
>> +
>> +	smem->dev = &pdev->dev;
>> +	smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL);
> 
> Compilation breaks while I try to compile with this patch.
> Do I missing anything? Below is the error I am getting,
> 
> drivers/soc/qcom/qcom_smem.c: In function ‘qcom_smem_probe’:
> drivers/soc/qcom/qcom_smem.c:274:2: error: implicit declaration of function ‘of_hwspin_lock_request’ [-Werror=implicit-function-declaration]
>   smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL)

I am sorry I missed the note in commit message of this patch. I did look for any such note in cover letter 
and also googled around for any change to find this function.
Could not find and hence posted comment. Please ignore.

> 
> Thanks,
> --Kiran
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 3/7] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

[Kumar Gala]

On Sep 29, 2014, at 7:34 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:

> Add binding documentation for the Qualcomm Resource Power Manager (RPM)
> using shared memory (Qualcomm SMD) as transport mechanism. This is found
> in 8974 and newer based devices.
> 
> The binding currently describes the rpm itself and the regulator
> subnodes.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> Note that this patch extends the rpm dt-bindings header from [1].
> 
> [1] https://lkml.org/lkml/2014/9/22/733
> 
> .../devicetree/bindings/mfd/qcom-rpm-smd.txt       |  122 ++++++++++++++++++++
> include/dt-bindings/mfd/qcom-rpm.h                 |   36 ++++++
> 2 files changed, 158 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> new file mode 100644
> index 0000000..a846101
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> @@ -0,0 +1,122 @@
> +Qualcomm Resource Power Manager (RPM) over SMD
> +
> +This driver is used to interface with the Resource Power Manager (RPM) found in
> +various Qualcomm platforms. The RPM allows each component in the system to vote
> +for state of the system resources, such as clocks, regulators and bus
> +frequencies.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,rpm-msm8974”

Why not “qcom,rpm-smd”.  I’d like to get Jeff H’s input on how what we do here for compat and distinguish the A-family RPM support vs B-family/RPM-SMD support.

> +
> +- qcom,smd-channels:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: Shared Memory Channel used for communication with the RPM
> +

This needs more details.

> +- #address-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: must be 1
> +
> +- #size-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: must be 0
> +
> += SUBDEVICES

As I mentioned for the the RPM binding on a-family, we should split out the devices into their own binding specs.

> +
> +The RPM exposes resources to its subnodes. The below bindings specify the set
> +of valid subnodes that can operate on these resources.
> +
> +== Switch-mode Power Supply regulator
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,rpm-pm8841-smps"
> +		    "qcom,rpm-pm8941-smps"
> +
> +
> +- reg:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: resource as defined in <dt-bindings/mfd/qcom-rpm.h>
> +		    must be one of:
> +		    QCOM_RPM_PM8841_SMPS1 - QCOM_RPM_PM8841_SMPS4,
> +		    QCOM_RPM_PM8941_SMPS1 - QCOM_RPM_PM8941_SMPS3
> +
> +Standard regulator bindings are used inside switch mode power supply subnodes.
> +Check Documentation/devicetree/bindings/regulator/regulator.txt for more
> +details.
> +
> +== Low-dropout regulator
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,rpm-pm8941-ldo"
> +
> +- reg:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: resource as defined in <dt-bindings/mfd/qcom-rpm.h>
> +		    must be one of:
> +		    QCOM_RPM_PM8941_LDO1 - QCOM_RPM_PM8941_LDO24
> +
> +Standard regulator bindings are used inside switch low-dropout regulator
> +subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
> +more details.
> +
> +== Switch
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,rpm-pm8941-switch"
> +
> +- reg:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: resource as defined in <dt-bindings/mfd/qcom/qcom-rpm.h>
> +		    must be one of:
> +		    QCOM_RPM_PM8941_LVS1 - QCOM_RPM_PM8941_LVS3,
> +		    QCOM_RPM_PM8941_MVS1 - QCOM_RPM_PM8941_MVS2
> +
> +Standard regulator bindings are used inside switch regulator subnodes.  Check
> +Documentation/devicetree/bindings/regulator/regulator.txt for more details.
> +
> += EXAMPLE
> +
> +	#include <dt-bindings/mfd/qcom-rpm.h>
> +
> +	rpm@108000 {
> +		compatible = "qcom,rpm-msm8960";
> +		qcom,smd-channels = "rpm_requests";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		pm8941_s1: regulator-s1 {
> +			compatible = "qcom,rpm-pm8941-smps";
> +			reg = <QCOM_RPM_PM8941_SMPS1>;
> +
> +			regulator-min-microvolt = <1300000>;
> +			regulator-max-microvolt = <1300000>;
> +		};
> +
> +		pm8941_l3: pm8941-l3 {
> +			compatible = "qcom,rpm-pm8941-ldo";
> +			reg = <QCOM_RPM_PM8941_LDO3>;
> +
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +		};
> +
> +	};
> +

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC 0/7] Qualcomm SMEM, SMD, RPM and regulators

[Kumar Gala]

On Sep 29, 2014, at 7:34 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:

> All Qualcomm platforms implements a shared heap among the processors in the
> SoC, used for sharing data with other parts of the system.
> 
> One consumer of items from this heap is the "Shared Memory Driver", a ring
> buffer based point-to-point communication mechanism used to send either stream
> or packet based data to remote processors.
> 
> Starting with 8x74 this system is used to talk to the Resource Power Manager
> (RPM), a power efficient "coprocessor" with responsibility of aggregate votes
> from the various systems in the SoC related to regulators, clocks and bus
> frequencies.
> 
> The PMIC regulators and root clocks in these platforms are only accessible via
> the RPM, so to get access to these we need the full chain of smem, smd, rpm and
> a regulator driver implemented. And that is exactly what this series provides.
> 
> 
> A key outstanding question is where in the tree we should put the
> implementation, for now I dropped them in drivers/soc/qcom but that's only
> because I don't know where to put it otherwise. I have not found any equivalent
> of the SMEM driver, SMD resembles mailbox and rpmsg - but comments in that
> patch on why it's neither.
> 
> RPM is a mfd and regulator is a regulator :)

I still don’t see why RPM support for either A-family or B-family should exist in MFD vis drivers/soc/qcom.  What benefit is there in putting this in MFD?

I think both A and B-family support should be in drivers/soc/qcom for the current time being until we determine there is some framework that makes more sense in the future.  I almost see RPM more like a bus controller than anything else.  Something like an I2C bus controller that than has some set of devices off of that bus.

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC 1/7] soc: qcom: Add device tree binding for SMEM

[Kumar Gala]

On Sep 29, 2014, at 7:34 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:

> Add device tree binding documentation for the Qualcom Shared Memory
> manager.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> Exposed by this node is a set of items of different sizes. For many things a
> standard of_xlate method of referencing the individual nodes would be
> preferable, so a #something-cells would make sense. We do however also needs
> access to these items without explicitly stating the references in devicetree
> (e.g. SMD references 257 of these). I haven't found any good example of how to
> implement this, so suggestions are welcome.
> 
> Note that the hwspinlock reference is not yet supported in the mainline, but
> this will likely need a few iterations so I wanted to get this out.
> 
> .../devicetree/bindings/soc/qcom/qcom,smem.txt     |   34 ++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
> new file mode 100644
> index 0000000..ddd58c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
> @@ -0,0 +1,34 @@
> +Qualcomm Shared Memory binding
> +
> +This binding describes the Qualcomm Shared Memory, used to share data between
> +various subsystems and OSes in Qualcomm platforms.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be:
> +		    "qcom,smem"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address and size pair for each area representing the
> +		    shared memory. The first pair will must represent the "main"
> +		    area, where the shared memory header and table-of-content
> +		    can be found.
> +
> +- hwspinlocks:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: reference to a hwspinlock used to protect allocations from
> +		    the shared memory
> +
> += EXAMPLE
> +
> +        smem: smem@fa00000 {
> +                compatible = "qcom,smem";
> +                reg = <0x0fa00000 0x200000>,
> +                      <0xfc428000 0x4000>;
> +
> +                hwspinlocks = <&tcsr_mutex 3>;
> +        };
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC 4/7] soc: qcom: Add Shared Memory Manager driver

[Bjorn Andersson]

On Mon 29 Sep 23:28 PDT 2014, Kiran Padwal wrote:

> 
> Hi,
> 
> On Tuesday 30 September 2014 11:47 AM, Kiran Padwal wrote:
> > Hi Bjorn,
> > 
> > On Tuesday 30 September 2014 06:04 AM, Bjorn Andersson wrote:
[..]
> >> +	smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL);
> > 
> > Compilation breaks while I try to compile with this patch.
> > Do I missing anything? Below is the error I am getting,
> > 
> > drivers/soc/qcom/qcom_smem.c: In function ‘qcom_smem_probe’:
> > drivers/soc/qcom/qcom_smem.c:274:2: error: implicit declaration of function ‘of_hwspin_lock_request’ [-Werror=implicit-function-declaration]
> >   smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL)
> 
> I am sorry I missed the note in commit message of this patch. I did look for any such note in cover letter 
> and also googled around for any change to find this function.
> Could not find and hence posted comment. Please ignore.
> 

I respun this ontop of Suman Anna's latest hwspinlock patches [1] and [2].

The above code should now be:

        hwlock_id = of_hwspin_lock_get_id(pdev->dev.of_node, 0);
	smem->hwlock = hwspin_lock_request_specific(hwlock_id);

Will include this in the next spin.

[1] https://patchwork.kernel.org/patch/4898051/
[2] https://patchwork.kernel.org/patch/4898071/

Regards,
Bjorn

Re: [RFC 3/7] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

[Bjorn Andersson]

On Tue 30 Sep 06:46 PDT 2014, Kumar Gala wrote:

> 
> On Sep 29, 2014, at 7:34 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
> 
> > diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> > new file mode 100644
> > index 0000000..a846101
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> > @@ -0,0 +1,122 @@
> > +Qualcomm Resource Power Manager (RPM) over SMD
> > +
> > +This driver is used to interface with the Resource Power Manager (RPM) found in
> > +various Qualcomm platforms. The RPM allows each component in the system to vote
> > +for state of the system resources, such as clocks, regulators and bus
> > +frequencies.
> > +
> > +- compatible:
> > +	Usage: required
> > +	Value type: <string>
> > +	Definition: must be one of:
> > +		    "qcom,rpm-msm8974”
> 
> Why not “qcom,rpm-smd”.  I’d like to get Jeff H’s input on how
> what we do here for compat and distinguish the A-family RPM support vs
> B-family/RPM-SMD support.
> 

I don't see anything indicating changes in the actual communication, but as
this also encodes what resources are exposed we have to keep this specific.

I'm not sure what you mean with distinguish the A and B family, they are
completely different and there are no overlap in compatibles or implementation.

The overlap is in how children are communicating with the rpm, but this is an
implementation detail - and noted in that patch as a future improvement.


I forgot to add Jeff, but did send him a separate email asking for his input on
all this.

> > +
> > +- qcom,smd-channels:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: Shared Memory Channel used for communication with the RPM
> > +
> 
> This needs more details.
> 

Not sure what more to add here. It should contain the name of the channel used
for communication with the rpm. For all current platforms this would be
"rpm_requests".

> > +- #address-cells:
> > +	Usage: required
> > +	Value type: <u32>
> > +	Definition: must be 1
> > +
> > +- #size-cells:
> > +	Usage: required
> > +	Value type: <u32>
> > +	Definition: must be 0
> > +
> > += SUBDEVICES
> 
> As I mentioned for the the RPM binding on a-family, we should split out the
> devices into their own binding specs.
> 

Please actually read https://lkml.org/lkml/2014/3/10/567, it's now the third
time I send you that link. If you don't like it then ask Rob to revise his
statement.

For me it makes sense to consolidate the RPM binding in one document rather
than spreading it out in 10 different documents with some implicit
dependencies.

Regards,
Bjorn

Re: [RFC 0/7] Qualcomm SMEM, SMD, RPM and regulators

[Bjorn Andersson]

On Tue 30 Sep 06:49 PDT 2014, Kumar Gala wrote:

> 
> On Sep 29, 2014, at 7:34 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
> 
> > All Qualcomm platforms implements a shared heap among the processors in the
> > SoC, used for sharing data with other parts of the system.
> > 
> > One consumer of items from this heap is the "Shared Memory Driver", a ring
> > buffer based point-to-point communication mechanism used to send either stream
> > or packet based data to remote processors.
> > 
> > Starting with 8x74 this system is used to talk to the Resource Power Manager
> > (RPM), a power efficient "coprocessor" with responsibility of aggregate votes
> > from the various systems in the SoC related to regulators, clocks and bus
> > frequencies.
> > 
> > The PMIC regulators and root clocks in these platforms are only accessible via
> > the RPM, so to get access to these we need the full chain of smem, smd, rpm and
> > a regulator driver implemented. And that is exactly what this series provides.
> > 
> > 
> > A key outstanding question is where in the tree we should put the
> > implementation, for now I dropped them in drivers/soc/qcom but that's only
> > because I don't know where to put it otherwise. I have not found any equivalent
> > of the SMEM driver, SMD resembles mailbox and rpmsg - but comments in that
> > patch on why it's neither.
> > 
> > RPM is a mfd and regulator is a regulator :)
> 
> I still don’t see why RPM support for either A-family or B-family should
> exist in MFD vis drivers/soc/qcom.  What benefit is there in putting this in
> MFD?
> 
> I think both A and B-family support should be in drivers/soc/qcom for the
> current time being until we determine there is some framework that makes more
> sense in the future.  I almost see RPM more like a bus controller than
> anything else.  Something like an I2C bus controller that than has some set
> of devices off of that bus.
> 

When you look at what functionality the RPM exposes it has very much in common
with a PMIC. So after looking at this back and forth for months I think MFD is
a nice fit.

As with all the other pmics we could create a new subsystem (drivers/pmic?) for
this kind of devices that exposes variable size registers for children to read
and write.

But if you can convince the maintainers about that then we have a whole bunch
of stuff in mfd etc that we should move out, so let's not put this in
qcom-staging just for the sake of it.

Regards,
Bjorn

Re: [RFC 1/7] soc: qcom: Add device tree binding for SMEM

[Stephen Boyd]

On 09/29/14 17:34, Bjorn Andersson wrote:
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address and size pair for each area representing the
> +		    shared memory. The first pair will must represent the "main"
> +		    area, where the shared memory header and table-of-content
> +		    can be found.
>
> +
> += EXAMPLE
> +
> +        smem: smem@fa00000 {
> +                compatible = "qcom,smem";
> +                reg = <0x0fa00000 0x200000>,
> +                      <0xfc428000 0x4000>;

Isn't this second entry rpm message ram? That isn't the same as smem.
Plus smem is part of ram (and rpm message ram is not) so we need to do
memory reservations or something.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC 1/7] soc: qcom: Add device tree binding for SMEM

[Bjorn Andersson]

On Tue 30 Sep 12:03 PDT 2014, Stephen Boyd wrote:

> On 09/29/14 17:34, Bjorn Andersson wrote:
> > +
> > +- reg:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: base address and size pair for each area representing the
> > +		    shared memory. The first pair will must represent the "main"
> > +		    area, where the shared memory header and table-of-content
> > +		    can be found.
> >
> > +
> > += EXAMPLE
> > +
> > +        smem: smem@fa00000 {
> > +                compatible = "qcom,smem";
> > +                reg = <0x0fa00000 0x200000>,
> > +                      <0xfc428000 0x4000>;
> 
> Isn't this second entry rpm message ram? That isn't the same as smem.
> Plus smem is part of ram (and rpm message ram is not) so we need to do
> memory reservations or something.
> 

Correct they are different, but smem covers both of those and allocations are
only supposed to be done in the first of these.

And I forgot to mention that I have the following in my dt:

/ {
        reserved-memory {
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;

                smem@fa00000 {
                        #memory-region-cells = <0>;
                        reg = <0x0fa00000 0x200000>;
                        no-map;
                };
        };
};

Regards,
Bjorn

Re: [RFC 1/7] soc: qcom: Add device tree binding for SMEM

[Suman Anna]

Hi Bjorn,

On 09/29/2014 07:34 PM, Bjorn Andersson wrote:
> Add device tree binding documentation for the Qualcom Shared Memory
> manager.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> Exposed by this node is a set of items of different sizes. For many things a
> standard of_xlate method of referencing the individual nodes would be
> preferable, so a #something-cells would make sense. We do however also needs
> access to these items without explicitly stating the references in devicetree
> (e.g. SMD references 257 of these). I haven't found any good example of how to
> implement this, so suggestions are welcome.
> 
> Note that the hwspinlock reference is not yet supported in the mainline, but
> this will likely need a few iterations so I wanted to get this out.
> 
>  .../devicetree/bindings/soc/qcom/qcom,smem.txt     |   34 ++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
> new file mode 100644
> index 0000000..ddd58c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.txt
> @@ -0,0 +1,34 @@
> +Qualcomm Shared Memory binding
> +
> +This binding describes the Qualcomm Shared Memory, used to share data between
> +various subsystems and OSes in Qualcomm platforms.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be:
> +		    "qcom,smem"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address and size pair for each area representing the
> +		    shared memory. The first pair will must represent the "main"
> +		    area, where the shared memory header and table-of-content
> +		    can be found.
> +
> +- hwspinlocks:

The property name to use should be "hwlocks" and not "hwspinlocks". This
is what the hwspinlock driver core expects from client users.

regards
Suman

> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: reference to a hwspinlock used to protect allocations from
> +		    the shared memory
> +
> += EXAMPLE
> +
> +        smem: smem@fa00000 {
> +                compatible = "qcom,smem";
> +                reg = <0x0fa00000 0x200000>,
> +                      <0xfc428000 0x4000>;
> +
> +                hwspinlocks = <&tcsr_mutex 3>;


> +        };
> 

Re: [RFC 3/7] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

[Jeffrey Hugo]

On 9/30/2014 8:37 AM, Bjorn Andersson wrote:
> On Tue 30 Sep 06:46 PDT 2014, Kumar Gala wrote:
>
>>
>> On Sep 29, 2014, at 7:34 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
>>> new file mode 100644
>>> index 0000000..a846101
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
>>> @@ -0,0 +1,122 @@
>>> +Qualcomm Resource Power Manager (RPM) over SMD
>>> +
>>> +This driver is used to interface with the Resource Power Manager (RPM) found in
>>> +various Qualcomm platforms. The RPM allows each component in the system to vote
>>> +for state of the system resources, such as clocks, regulators and bus
>>> +frequencies.
>>> +
>>> +- compatible:
>>> +	Usage: required
>>> +	Value type: <string>
>>> +	Definition: must be one of:
>>> +		    "qcom,rpm-msm8974”
>>
>> Why not “qcom,rpm-smd”.  I’d like to get Jeff H’s input on how
>> what we do here for compat and distinguish the A-family RPM support vs
>> B-family/RPM-SMD support.
>>
>
> I don't see anything indicating changes in the actual communication, but as
> this also encodes what resources are exposed we have to keep this specific.
>
> I'm not sure what you mean with distinguish the A and B family, they are
> completely different and there are no overlap in compatibles or implementation.
>
> The overlap is in how children are communicating with the rpm, but this is an
> implementation detail - and noted in that patch as a future improvement.
>
>
> I forgot to add Jeff, but did send him a separate email asking for his input on
> all this.
>

Yep.  I saw the series.  Still doing a deep dive.

The rpm-smd driver (and the A family equivalent) is outside of my area 
of expertise, but I have some familiarity with it as it is a SMD client. 
  Internally we have a singular compat for all of B family, but I 
haven't been able to figure out how that works to expose all of the 
resources.  I'll talk to my contact later in the week to see what the 
differences are.

Is the per target compat the way to do this though?  The way its 
currently done means we'll have atleast a dozen tables in 
qcom-smd-rpm.c, and I can't imagine they will have anything more than 
minor differences from the msm8x74_resource_table that currently exists 
in patch 6 of the series.  Why wouldn't one table that is a superset of 
all supported targets work?

>>> +
>>> +- qcom,smd-channels:
>>> +	Usage: required
>>> +	Value type: <stringlist>
>>> +	Definition: Shared Memory Channel used for communication with the RPM
>>> +
>>
>> This needs more details.
>>
>
> Not sure what more to add here. It should contain the name of the channel used
> for communication with the rpm. For all current platforms this would be
> "rpm_requests".
>
>>> +- #address-cells:
>>> +	Usage: required
>>> +	Value type: <u32>
>>> +	Definition: must be 1
>>> +
>>> +- #size-cells:
>>> +	Usage: required
>>> +	Value type: <u32>
>>> +	Definition: must be 0
>>> +
>>> += SUBDEVICES
>>
>> As I mentioned for the the RPM binding on a-family, we should split out the
>> devices into their own binding specs.
>>
>
> Please actually read https://lkml.org/lkml/2014/3/10/567, it's now the third
> time I send you that link. If you don't like it then ask Rob to revise his
> statement.
>
> For me it makes sense to consolidate the RPM binding in one document rather
> than spreading it out in 10 different documents with some implicit
> dependencies.
>
> Regards,
> Bjorn
>


Jeffrey Hugo
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation.

Re: [RFC 3/7] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

[Bjorn Andersson]

On Tue 30 Sep 16:16 PDT 2014, Jeffrey Hugo wrote:

> On 9/30/2014 8:37 AM, Bjorn Andersson wrote:
> > On Tue 30 Sep 06:46 PDT 2014, Kumar Gala wrote:
> >
> >>
> >> On Sep 29, 2014, at 7:34 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
> >>
> >>> diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> >>> new file mode 100644
> >>> index 0000000..a846101
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> >>> @@ -0,0 +1,122 @@
> >>> +Qualcomm Resource Power Manager (RPM) over SMD
> >>> +
> >>> +This driver is used to interface with the Resource Power Manager (RPM) found in
> >>> +various Qualcomm platforms. The RPM allows each component in the system to vote
> >>> +for state of the system resources, such as clocks, regulators and bus
> >>> +frequencies.
> >>> +
> >>> +- compatible:
> >>> +	Usage: required
> >>> +	Value type: <string>
> >>> +	Definition: must be one of:
> >>> +		    "qcom,rpm-msm8974”
> >>
> >> Why not “qcom,rpm-smd”.  I’d like to get Jeff H’s input on how
> >> what we do here for compat and distinguish the A-family RPM support vs
> >> B-family/RPM-SMD support.
> >>
> >
> > I don't see anything indicating changes in the actual communication, but as
> > this also encodes what resources are exposed we have to keep this specific.
> >
> > I'm not sure what you mean with distinguish the A and B family, they are
> > completely different and there are no overlap in compatibles or implementation.
> >
> > The overlap is in how children are communicating with the rpm, but this is an
> > implementation detail - and noted in that patch as a future improvement.
> >
> >
> > I forgot to add Jeff, but did send him a separate email asking for his input on
> > all this.
> >
> 
> Yep.  I saw the series.  Still doing a deep dive.
> 
> The rpm-smd driver (and the A family equivalent) is outside of my area 
> of expertise, but I have some familiarity with it as it is a SMD client. 
>   Internally we have a singular compat for all of B family, but I 
> haven't been able to figure out how that works to expose all of the 
> resources.  I'll talk to my contact later in the week to see what the 
> differences are.
> 

That's right, because you have these tables in devicetree in the caf version.
You have to have this information somewhere!

> Is the per target compat the way to do this though?  The way its 
> currently done means we'll have atleast a dozen tables in 
> qcom-smd-rpm.c, and I can't imagine they will have anything more than 
> minor differences from the msm8x74_resource_table that currently exists 
> in patch 6 of the series.  Why wouldn't one table that is a superset of 
> all supported targets work?
> 

It would work as long as e.g. QCOM_RPM_PM8941_MVS1 always is vsa number 4.

But sure, it's a much better fit than the family a and this rpm would reject
invalid resources, so it might work. But this works without us knowing about
all possible platforms.


But if the case is that multiple platforms expose the same table we can always
tie the same table to multiple compatibles.

Regards,
Bjorn

Re: [RFC 7/7] regulator: qcom-smd-rpm: Regulator driver for the Qualcomm RPM

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Mon, Sep 29, 2014 at 05:34:51PM -0700, Bjorn Andersson wrote:

> According to the datasheet for the PMIC the regulators are indeed programmed in
> steps, but the steps seems to vary between different regulators and the details
> are hidden by the RPM that exposes contiguous voltage ranges.

> Either we run with this, add a few more compatibles to encode the steps or have
> the step coming from devicetree. I prefer the current implementation as that is
> the cleanest of these.

We have support for this sort of regulator in the core anyway so just
keep on doing what you're doing.

> +	if (vreg->desc.ops->set_voltage &&
> +	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
> +		dev_err(&pdev->dev, "no voltage specified for regulator\n");
> +		return -EINVAL;
> +	}

You shouldn't need to do this - it should be perfectly legal to have the
ability to set voltages but not use that ability.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 5/7] soc: qcom: Add Shared Memory Driver

[Stephen Boyd]

On 09/29/14 17:34, Bjorn Andersson wrote:
> +
> +#define GET_RX_CHANNEL_INFO(channel, param) \
> +	(channel->rx_info_word ? \
> +		channel->rx_info_word->param : \
> +		channel->rx_info->param)
> +
> +#define GET_TX_CHANNEL_INFO(channel, param) \
> +	(channel->rx_info_word ? \
> +		channel->tx_info_word->param : \
> +		channel->tx_info->param)
> +
> +#define SET_RX_CHANNEL_INFO(channel, param, value) \
> +	(channel->rx_info_word ? \
> +		(channel->rx_info_word->param = value) : \
> +		(channel->rx_info->param = value))
> +
> +#define SET_TX_CHANNEL_INFO(channel, param, value) \
> +	(channel->rx_info_word ? \

Drive-by review: Should this be tx_info_word? Given that it works I
wonder why not just have a flag indicating if we should use word aligned
read/write vs. byte aligned.

> +		(channel->tx_info_word->param = value) : \
> +		(channel->tx_info->param = value))
> +


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC 5/7] soc: qcom: Add Shared Memory Driver

[Bjorn Andersson]

On Thu 02 Oct 15:38 PDT 2014, Stephen Boyd wrote:

> On 09/29/14 17:34, Bjorn Andersson wrote:
> > +
> > +#define GET_RX_CHANNEL_INFO(channel, param) \
> > +	(channel->rx_info_word ? \
> > +		channel->rx_info_word->param : \
> > +		channel->rx_info->param)
> > +
> > +#define GET_TX_CHANNEL_INFO(channel, param) \
> > +	(channel->rx_info_word ? \
> > +		channel->tx_info_word->param : \
> > +		channel->tx_info->param)
> > +
> > +#define SET_RX_CHANNEL_INFO(channel, param, value) \
> > +	(channel->rx_info_word ? \
> > +		(channel->rx_info_word->param = value) : \
> > +		(channel->rx_info->param = value))
> > +
> > +#define SET_TX_CHANNEL_INFO(channel, param, value) \
> > +	(channel->rx_info_word ? \
> 
> Drive-by review: Should this be tx_info_word? Given that it works I
> wonder why not just have a flag indicating if we should use word aligned
> read/write vs. byte aligned.
> 

You're right, that should be tx - but from the way things both channels will
always be of the same type, so it will simply work.

I had a separate flag, but instead of having 4 members in the struct to
indicate if I was dealing with word aligned access I had 5. So I dropped it.

> > +		(channel->tx_info_word->param = value) : \
> > +		(channel->tx_info->param = value))
> > +
> 

Regards,
Bjorn

Re: [RFC 6/7] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD

[Lee Jones]

On Mon, 29 Sep 2014, Bjorn Andersson wrote:

> Driver for the Resource Power Manager (RPM) found in Qualcomm 8974 based
> devices.
> The driver exposes resources that child drivers can operate on; to
> implementing regulator, clock and bus frequency drivers.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> Note that the qcom_rpm_smd_write is equivalent of qcom_rpm_write and there is a
> possibility of re-using at least the clock implementation on top of this. This
> would however require some logic for calling the right implementation so I have
> not done it at this time to keep things as clean as possible.
> 
> An idea for improvement is that in qcom_rpm_smd_write we put the ack_status and
> completion on the stack and register this with idr using the message id, upon
> receiving the interrupt we would find the right client and complete this.
> Allowing for multiple requests to be in flight at any given time.
> 
> I did not implement this because I haven't done any measurements on what kind
> of improvements this could give and it would be a clean iteration ontop of
> this.
> 
>  drivers/mfd/Kconfig              |   14 ++
>  drivers/mfd/Makefile             |    1 +
>  drivers/mfd/qcom-smd-rpm.c       |  299 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/qcom-smd-rpm.h |    9 ++
>  4 files changed, 323 insertions(+)
>  create mode 100644 drivers/mfd/qcom-smd-rpm.c
>  create mode 100644 include/linux/mfd/qcom-smd-rpm.h


> +#define RPM_ERR_INVALID_RESOURCE "resource does not exist"
> +
> +static bool qcom_rpm_msg_is_invalid_resource(struct qcom_rpm_message *msg)
> +{
> +	size_t msg_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1;
> +
> +	if (msg->length != msg_len)
> +		return false;
> +
> +	if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE, msg_len))
> +		return false;
> +
> +	return true;
> +}

You can save yourself a hell of a lot of code by just doing:

if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE,
           min(msg_len, sizeof(RPM_ERR_INVALID_RESOURCE))))

... in qcom_smd_rpm_callback().

[...]

> +static int qcom_smd_rpm_probe(struct qcom_smd_device *sdev)
> +{
> +	const struct of_device_id *match;
> +	struct qcom_smd_rpm *rpm;
> +
> +	rpm = devm_kzalloc(&sdev->dev, sizeof(*rpm), GFP_KERNEL);
> +	if (!rpm)
> +		return -ENOMEM;
> +
> +	rpm->dev = &sdev->dev;
> +	mutex_init(&rpm->lock);
> +	init_completion(&rpm->ack);
> +
> +	match = of_match_device(qcom_smd_rpm_of_match, &sdev->dev);

You need to check the return value here.

> +	rpm->data = match->data;
> +	rpm->rpm_channel = sdev->channel;
> +
> +	dev_set_drvdata(&sdev->dev, rpm);
> +
> +	dev_info(&sdev->dev, "Qualcomm SMD RPM driver probed\n");

Please remove this line.

> +	return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> +}
> +
> +static void qcom_smd_rpm_remove(struct qcom_smd_device *sdev)
> +{
> +	dev_set_drvdata(&sdev->dev, NULL);

If you use the proper platform device interface you don't have to do
this.

> +	of_platform_depopulate(&sdev->dev);
> +}
> +
> +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> +	.probe = qcom_smd_rpm_probe,
> +	.remove = qcom_smd_rpm_remove,
> +	.callback = qcom_smd_rpm_callback,
> +	.driver  = {
> +		.name  = "qcom_smd_rpm",
> +		.owner = THIS_MODULE,
> +		.of_match_table = qcom_smd_rpm_of_match,
> +	},
> +};
> +
> +module_qcom_smd_driver(qcom_smd_rpm_driver);

I don't like this.  What's wrong with the existing platform driver
code?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 4/7] soc: qcom: Add Shared Memory Manager driver

[Jeffrey Hugo]

Here in my initial detailed pass.  I still have some "issues" that I 
want to clarify on my end, but I think I have plenty of comments to 
start with.

On 9/29/2014 6:34 PM, Bjorn Andersson wrote:
> The Shared Memory Manager driver implements an interface for allocating
> and accessing items in the memory area shared among all of the
> processors in a Qualcomm platform.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>
> In later platforms this is extended to support "secure smem", I do
> unfortunately not have any devices that I could test this with so I have only
> implemented the "insecure" version for now.
>
> I was thinking about implementing an of_xlate as this would make sense for many
> things. It is however not feasible for SMD, that would require us to list 257
> items.
>
> It would make sense to enhance this with a method of keeping track of currently
> consumed items, both to take care of "mutual exclusion" between consumers as
> well as knowing when it's safe to remove the device and/or unload the driver.

By "mutual exclusion" I assume you mean that two different entities are 
using the same smem id.  Such a usecase is outside the purview of this 
driver, and is essentially unsupported.  Any such usecases would need to 
be handled by the client.  In short, smem items support one direct client.

I'm not sure it makes sense to unload the driver.  I understand this is 
a qcom specific driver, but it is critical to our devices.  Any 
situation I can think of in which this driver would be unloaded, would 
ultimately result in a reboot of the device.

>
> I did consider exposing the items as regmaps, but for many of the items the
> regmap context is consuming far more space then the actual data. And we would
> end up creating 3-400 regmap contexts in a normal system.
>
>
> Also note that the hwspinlock framework does not yet support devicetree based
> retrieval of spinlock handles, so that part needs to be changed.
>
>   drivers/soc/qcom/Kconfig           |    8 +
>   drivers/soc/qcom/Makefile          |    1 +
>   drivers/soc/qcom/qcom_smem.c       |  328 ++++++++++++++++++++++++++++++++++++
>   include/linux/soc/qcom/qcom_smem.h |   14 ++
>   4 files changed, 351 insertions(+)
>   create mode 100644 drivers/soc/qcom/qcom_smem.c
>   create mode 100644 include/linux/soc/qcom/qcom_smem.h
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 7bd2c94..9e6bc56 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -9,3 +9,11 @@ config QCOM_GSBI
>             functions for connecting the underlying serial UART, SPI, and I2C
>             devices to the output pins.
>
> +config QCOM_SMEM
> +	tristate "Qualcomm Shared Memory Interface"
> +	depends on ARCH_QCOM
> +	help
> +	  Say y here to enable support for the Qualcomm Shared Memory Manager.
> +	  The driver provides an interface to items in a heap shared among all
> +	  processors in a Qualcomm platform and is used for exchanging
> +	  information as well as a fifo based communication mechanism (SMD).

I don't think the reference to SMD is appropriate here.  SMD is one of 
many clients that make use of SMEM.

> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 4389012..b1f7b18 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1 +1,2 @@
>   obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
> +obj-$(CONFIG_QCOM_SMEM) +=	qcom_smem.o
> diff --git a/drivers/soc/qcom/qcom_smem.c b/drivers/soc/qcom/qcom_smem.c
> new file mode 100644
> index 0000000..9766c86
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_smem.c

I see there appears to be a very small ammount of precidence (one other 
file), but it seems pointlessly redundant to prefix "qcom_" to the file 
when it exists in a "qcom" directory.  What is the other point of view 
that I am missing?

> @@ -0,0 +1,328 @@
> +/*
> + * Copyright (c) 2014, Sony Mobile Communications AB.
> + * Copyright (c) 2012-2013, 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/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/soc/qcom/qcom_smem.h>

Alphabetized please.

> +
> +#define AUX_BASE_MASK		0xfffffffc
> +#define HWSPINLOCK_TIMEOUT	1000
> +#define SMEM_MAX_ITEMS		512
> +
> +/**
> +  * struct smem_proc_comm - proc_comm communication struct (legacy)
> +  * @command:	current command to be executed
> +  * @status:	status of the currently requested command
> +  * @params:	parameters to the command
> +  */
> +struct smem_proc_comm {
> +	u32 command;
> +	u32 status;
> +	u32 params[2];
> +};
> +
> +/**
> + * struct smem_entry - entry to reference smem items on the heap
> + * @allocated:	boolean to indicate if this entry is used
> + * @offset:	offset to the allocated space
> + * @size:	size of the allocated space, 8 byte aligned
> + * @aux_base:	base address for the memory region used by this unit, or 0 for
> + *		the default region. bits 0,1 are reserved
> + */
> +struct smem_entry {
> +	u32 allocated;
> +	u32 offset;
> +	u32 size;
> +	u32 aux_base; /* bits 1:0 reserved */

Inline comment appears redundant since its addressed in the comment 
block above

> +};
> +
> +/**
> + * struct smem_header - header found in beginning of primary smem region
> + * @proc_comm:		proc_comm communication interface (legacy)
> + * @version:		array of versions for the various subsystems
> + * @smem_initialized:	boolean to indicate that smem is initialized
> + * @free_offset:	index of the first unallocated byte in smem
> + * @available:		number of bytes available for allocation
> + * @unused:		reserved field
> + * toc:			array of references to smem entries

"@toc"?

> + */
> +struct smem_header {
> +	struct smem_proc_comm proc_comm[4];
> +	u32 version[32];
> +	u32 smem_initialized;
> +	u32 free_offset;
> +	u32 available;
> +	u32 unused;

I see that you inlined the smem_heap_info struct.  That is slightly 
problematic since we have some uses of that structure, and without it, 
accessing id 1 becomes complicated.  I would prefer you reintroduce it.

> +	struct smem_entry toc[SMEM_MAX_ITEMS];
> +};
> +
> +/**
> +  * struct smem_area - memory region used for smem heap

smem_region?

> +  * @aux_base:	physical base address of the region, used for entry lookup
> +  * @virt_base:	virtual address of the mapping
> +  */
> +struct smem_region {
> +	u32 aux_base;
> +	void __iomem *virt_base;
> +};
> +
> +/**
> +  * struct qcom_smem - control struct for the smem driver
> +  * @dev:		device pointer
> +  * @hwlock:		hwspinlock to be held during heap operations
> +  * @num_regions:	number of entires in the regions array
> +  * @regions:		array of memory regions, region 0 contains smem_header
> +  */
> +struct qcom_smem {
> +	struct device *dev;
> +
> +	struct hwspinlock *hwlock;
> +
> +	unsigned num_regions;
> +	struct smem_region regions[0];
> +};
> +
> +/* Pointer to the one and only smem handle */
> +static struct qcom_smem *smem_handle;
> +
> +/**
> + * of_get_qcom_smem - retrieve a smem handle from a phandle

"of_get_qcom_smem()"?

> + * @client_node: of_node of the client
> + *
> + * Resolve the phandle in the property "qcom,smem" of @client_node and use this
> + * to retrieve an smem handle.
> + */
> +struct qcom_smem *of_get_qcom_smem(struct device_node *client_node)
> +{
> +	struct device_node *node;
> +
> +	node = of_parse_phandle(client_node, "qcom,smem", 0);
> +	if (!node)
> +		return ERR_PTR(-EINVAL);
> +
> +	of_node_put(node);
> +
> +	if (!smem_handle)
> +		return ERR_PTR(-EPROBE_DEFER);
> +	else if (smem_handle->dev->of_node != node)
> +		return ERR_PTR(-EINVAL);
> +
> +	return smem_handle;
> +}
> +EXPORT_SYMBOL(of_get_qcom_smem);

Why?  I understand what this does in a technical sense, but I don't 
understand the reasoning behind it.  This forces every entity that is a 
client of smem to have some kind of existance in DT, when for a lot of 
them, they won't have a need or justification to be in DT other than to 
have a phandle to smem.

Smem is a special memory allocator.  I don't recall seeing a DT hook for 
kmalloc/dma_alloc_coherent/alloc_skb/etc so I'm not seeing why we have 
one here.

All this does is get the clients the qcom_smem handle so that they can 
pass it back to smem for alloc and get, but there doesn't seem to be a 
need for it.  The handle is a singleton which this driver already has a 
reference to, so giving it out to the clients and having them hand it 
back seems Rube Goldberg-esq.  It also gives the clients the ability to 
screw with the handle, which I would prefer they never even touch since 
they have no business doing.  You don't even sanity check the handle 
when it gets passed in.  What happens when a client screws up and passes 
in NULL or a random pointer?

The only positive thing about this that I see is it centralizes 
EPROBE_DEFER, but only for well behaved clients.  It doesn't appear that 
it would be too difficult to have that functionality in the simplified 
API you've presented.

> +
> +/**
> + * qcom_smem_alloc - allocate space for a smem item

"qcom_smem_alloc()"?

> + * @smem:	smem handle
> + * @smem_id:	item to be allocated
> + * @size:	number of bytes to be allocated
> + *
> + * Allocate space for a given smem item of size @size, given that the item is
> + * not yet allocated.

Return?

> + */
> +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size)
> +{
> +	struct smem_header *header = smem->regions[0].virt_base;
> +	struct smem_entry *entry;
> +	unsigned long flags;
> +	int ret;
> +
> +	size = ALIGN(size, 8);
> +
> +	if (WARN_ON(smem_id >= SMEM_MAX_ITEMS))
> +		return -EINVAL;
> +
> +	ret = hwspin_lock_timeout_irqsave(smem->hwlock,
> +					  HWSPINLOCK_TIMEOUT,
> +					  &flags);
> +	if (ret)
> +		return ret;
> +
> +	entry = &header->toc[smem_id];
> +	if (entry->allocated) {
> +		ret = -EEXIST;
> +		goto out;
> +	}
> +
> +	if (WARN_ON(size > header->available)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	entry->offset = header->free_offset;
> +	entry->size = size;

wmb() is needed here.  We need to ensure the remote side sees the offset 
and size before seeing allocated because they may be doing a lookup 
without allocation without the lock.

> +	entry->allocated = 1;
> +
> +	header->free_offset += size;
> +	header->available -= size;
> +
> +	/* Commit the changes before we release the spin lock */
> +	wmb();
> +out:
> +	hwspin_unlock_irqrestore(smem->hwlock, &flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL(qcom_smem_alloc);
> +

qcom_smem_get_free_space() kerneldoc?

> +unsigned qcom_smem_get_free_space(struct qcom_smem *smem)
> +{
> +	struct smem_header *header = smem->regions[0].virt_base;
> +
> +	return header->available;
> +}
> +EXPORT_SYMBOL(qcom_smem_get_free_space);
> +
> +/**
> + * qcom_smem_get - resolve ptr of size of a smem item

"qcom_smem_get()"?

> + * @smem:	smem handle
> + * @smem_id:	item id to be resolved
> + * @ptr:	pointer to be filled out with address of the item
> + * @size:	pointer to be filled out with size of the item
> + *
> + * Looks up pointer and size of a smem item.

Return?

> + */
> +int qcom_smem_get(struct qcom_smem *smem,
> +		  unsigned smem_id,
> +		  void **ptr,
> +		  size_t *size)
> +{
> +	struct smem_header *header = smem->regions[0].virt_base;
> +	struct smem_region *area;
> +	struct smem_entry *entry;
> +	unsigned long flags;
> +	u32 aux_base;
> +	unsigned i;
> +	int ret;
> +
> +	if (WARN_ON(smem_id >= SMEM_MAX_ITEMS))
> +		return -EINVAL;
> +
> +	ret = hwspin_lock_timeout_irqsave(smem->hwlock,
> +					  HWSPINLOCK_TIMEOUT,
> +					  &flags);
> +	if (ret)
> +		return ret;
> +
> +	entry = &header->toc[smem_id];
> +	if (!entry->allocated) {
> +		ret = -ENXIO;
> +		goto out;
> +	}
> +
> +	if (ptr != NULL) {
> +		aux_base = entry->aux_base & AUX_BASE_MASK;
> +
> +		for (i = 0; i < smem->num_regions; i++) {
> +			area = &smem->regions[i];
> +
> +			if (area->aux_base == aux_base || !aux_base) {
> +				*ptr = area->virt_base + entry->offset;
> +				break;
> +			}
> +		}
> +	}
> +	if (size != NULL)
> +		*size = entry->size;
> +
> +out:
> +	hwspin_unlock_irqrestore(smem->hwlock, &flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL(qcom_smem_get);
> +
> +static int qcom_smem_probe(struct platform_device *pdev)
> +{
> +	struct qcom_smem *smem;
> +	struct resource *res;
> +	size_t array_size;
> +	int num_regions = 0;
> +	int i;
> +
> +	for (i = 0; i < pdev->num_resources; i++) {
> +		res = &pdev->resource[i];
> +
> +		if (resource_type(res) == IORESOURCE_MEM)
> +			num_regions++;
> +	}
> +
> +	if (num_regions == 0) {
> +		dev_err(&pdev->dev, "no smem regions specified\n");
> +		return -EINVAL;
> +	}
> +
> +	array_size = num_regions * sizeof(struct smem_region);
> +	smem = devm_kzalloc(&pdev->dev, sizeof(*smem) + array_size, GFP_KERNEL);
> +	if (!smem)
> +		return -ENOMEM;
> +
> +	smem->dev = &pdev->dev;
> +	smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL);
> +	if (IS_ERR(smem->hwlock))
> +		return PTR_ERR(smem->hwlock);
> +
> +	smem->num_regions = num_regions;
> +
> +	for (i = 0; i < num_regions; i++) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +
> +		smem->regions[i].aux_base = (u32)res->start;
> +		smem->regions[i].virt_base = devm_ioremap(&pdev->dev,
> +							  res->start,
> +							  resource_size(res));
> +		if (!smem->regions[i].virt_base)
> +			return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, smem);
> +	smem_handle = smem;
> +
> +	dev_info(&pdev->dev, "Qualcomm Shared Memory Interface probed\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_smem_of_match[] = {
> +	{ .compatible = "qcom,smem" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_smem_of_match);
> +
> +static struct platform_driver qcom_smem_driver = {
> +	.probe          = qcom_smem_probe,
> +	.driver  = {
> +		.name  = "qcom_smem",
> +		.owner = THIS_MODULE,
> +		.of_match_table = qcom_smem_of_match,
> +	},
> +};
> +
> +static int __init qcom_smem_init(void)
> +{
> +	return platform_driver_register(&qcom_smem_driver);
> +}
> +arch_initcall(qcom_smem_init);
> +
> +static void __exit qcom_smem_exit(void)
> +{
> +	platform_driver_unregister(&qcom_smem_driver);
> +}
> +module_exit(qcom_smem_exit)
> +
> +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>");
> +MODULE_DESCRIPTION("Qualcomm Shared Memory Interface");
> +MODULE_LICENSE("GPLv2");
> diff --git a/include/linux/soc/qcom/qcom_smem.h b/include/linux/soc/qcom/qcom_smem.h

include/soc/ already exists, why add include/linux/soc?

> new file mode 100644
> index 0000000..7c0d3fd
> --- /dev/null
> +++ b/include/linux/soc/qcom/qcom_smem.h
> @@ -0,0 +1,14 @@
> +#ifndef __QCOM_SMEM_H__
> +#define __QCOM_SMEM_H__
> +

Why are the item definitions missing?

> +struct device_node;
> +struct qcom_smem;
> +
> +struct qcom_smem *of_get_qcom_smem(struct device_node *node);
> +
> +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size);
> +int qcom_smem_get(struct qcom_smem *smem, unsigned item, void **ptr, size_t *size);
> +
> +unsigned qcom_smem_get_free_space(struct qcom_smem *smem);

The above functions should be stubbed out when !CONFIG_QCOM_SMEM, no?

> +
> +#endif
>


-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [RFC 3/7] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

[Jeffrey Hugo]

On 9/30/2014 6:08 PM, Bjorn Andersson wrote:
> On Tue 30 Sep 16:16 PDT 2014, Jeffrey Hugo wrote:
>
>> On 9/30/2014 8:37 AM, Bjorn Andersson wrote:
>>> On Tue 30 Sep 06:46 PDT 2014, Kumar Gala wrote:
>>>
>>>>
>>>> On Sep 29, 2014, at 7:34 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
>>>>> new file mode 100644
>>>>> index 0000000..a846101
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
>>>>> @@ -0,0 +1,122 @@
>>>>> +Qualcomm Resource Power Manager (RPM) over SMD
>>>>> +
>>>>> +This driver is used to interface with the Resource Power Manager (RPM) found in
>>>>> +various Qualcomm platforms. The RPM allows each component in the system to vote
>>>>> +for state of the system resources, such as clocks, regulators and bus
>>>>> +frequencies.
>>>>> +
>>>>> +- compatible:
>>>>> +	Usage: required
>>>>> +	Value type: <string>
>>>>> +	Definition: must be one of:
>>>>> +		    "qcom,rpm-msm8974”
>>>>
>>>> Why not “qcom,rpm-smd”.  I’d like to get Jeff H’s input on how
>>>> what we do here for compat and distinguish the A-family RPM support vs
>>>> B-family/RPM-SMD support.
>>>>
>>>
>>> I don't see anything indicating changes in the actual communication, but as
>>> this also encodes what resources are exposed we have to keep this specific.
>>>
>>> I'm not sure what you mean with distinguish the A and B family, they are
>>> completely different and there are no overlap in compatibles or implementation.
>>>
>>> The overlap is in how children are communicating with the rpm, but this is an
>>> implementation detail - and noted in that patch as a future improvement.
>>>
>>>
>>> I forgot to add Jeff, but did send him a separate email asking for his input on
>>> all this.
>>>
>>
>> Yep.  I saw the series.  Still doing a deep dive.
>>
>> The rpm-smd driver (and the A family equivalent) is outside of my area
>> of expertise, but I have some familiarity with it as it is a SMD client.
>>    Internally we have a singular compat for all of B family, but I
>> haven't been able to figure out how that works to expose all of the
>> resources.  I'll talk to my contact later in the week to see what the
>> differences are.
>>
>
> That's right, because you have these tables in devicetree in the caf version.
> You have to have this information somewhere!

True, it must exist somewhere.  However since its information tied 
directly to the hardware, it seems like its information that would fit 
right in DT.

I talked to my contact, and it seems like the table attributes vary more 
than I thought, target to target, so the single table design seems less 
plausible.  Its my understanding you've had an offline discussion with 
him and some of our regulator guys to discuss the specifics of our 
needs.  I'll let them take over as the experts.

>
>> Is the per target compat the way to do this though?  The way its
>> currently done means we'll have atleast a dozen tables in
>> qcom-smd-rpm.c, and I can't imagine they will have anything more than
>> minor differences from the msm8x74_resource_table that currently exists
>> in patch 6 of the series.  Why wouldn't one table that is a superset of
>> all supported targets work?
>>
>
> It would work as long as e.g. QCOM_RPM_PM8941_MVS1 always is vsa number 4.
>
> But sure, it's a much better fit than the family a and this rpm would reject
> invalid resources, so it might work. But this works without us knowing about
> all possible platforms.
>
>
> But if the case is that multiple platforms expose the same table we can always
> tie the same table to multiple compatibles.
>
> Regards,
> Bjorn
>


-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [RFC 6/7] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD

[Bjorn Andersson]

On Wed 08 Oct 01:40 PDT 2014, Lee Jones wrote:

Hi Lee,

Thanks for your review.

> On Mon, 29 Sep 2014, Bjorn Andersson wrote:
> 
[..]

> > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist"
> > +
> > +static bool qcom_rpm_msg_is_invalid_resource(struct qcom_rpm_message *msg)
> > +{
> > +	size_t msg_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1;
> > +
> > +	if (msg->length != msg_len)
> > +		return false;
> > +
> > +	if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE, msg_len))
> > +		return false;
> > +
> > +	return true;
> > +}
> 
> You can save yourself a hell of a lot of code by just doing:
> 
> if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE,
>            min(msg_len, sizeof(RPM_ERR_INVALID_RESOURCE))))
> 
> ... in qcom_smd_rpm_callback().
> 

I can agree with you that there will be less code, but not "a hell of a lot". I
made the choise because I had something like the snippet you suggest and I
wanted to make it cleaner - I'll fold it back in.

> [...]
> 
> > +static int qcom_smd_rpm_probe(struct qcom_smd_device *sdev)
> > +{
> > +	const struct of_device_id *match;
> > +	struct qcom_smd_rpm *rpm;
> > +
> > +	rpm = devm_kzalloc(&sdev->dev, sizeof(*rpm), GFP_KERNEL);
> > +	if (!rpm)
> > +		return -ENOMEM;
> > +
> > +	rpm->dev = &sdev->dev;
> > +	mutex_init(&rpm->lock);
> > +	init_completion(&rpm->ack);
> > +
> > +	match = of_match_device(qcom_smd_rpm_of_match, &sdev->dev);
> 
> You need to check the return value here.
> 

As long as we only support device tree probing of this match will never return
NULL. I can add a check to fail on non-dt boards if someone chooses to ever
implement one of those.

> > +	rpm->data = match->data;
> > +	rpm->rpm_channel = sdev->channel;
> > +
> > +	dev_set_drvdata(&sdev->dev, rpm);
> > +
> > +	dev_info(&sdev->dev, "Qualcomm SMD RPM driver probed\n");
> 
> Please remove this line.
> 

Ok

> > +	return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> > +}
> > +
> > +static void qcom_smd_rpm_remove(struct qcom_smd_device *sdev)
> > +{
> > +	dev_set_drvdata(&sdev->dev, NULL);
> 
> If you use the proper platform device interface you don't have to do
> this.
> 

Ok

> > +	of_platform_depopulate(&sdev->dev);
> > +}
> > +
> > +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> > +	.probe = qcom_smd_rpm_probe,
> > +	.remove = qcom_smd_rpm_remove,
> > +	.callback = qcom_smd_rpm_callback,
> > +	.driver  = {
> > +		.name  = "qcom_smd_rpm",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = qcom_smd_rpm_of_match,
> > +	},
> > +};
> > +
> > +module_qcom_smd_driver(qcom_smd_rpm_driver);
> 
> I don't like this.  What's wrong with the existing platform driver
> code?
> 

I started off with having smd child devices as platform drivers and had some
accessor functions to find the open handles that triggered the probe() and
register the callback with those. But this didn't feel very sane, so I did
implemented a custom driver struct and probe prototype to simplify writing
drivers.

May I ask why you dislike this? This is how it's done in so many other places
in the kernel...

Regards,
Bjorn

Re: [RFC 4/7] soc: qcom: Add Shared Memory Manager driver

[Bjorn Andersson]

On Wed 08 Oct 14:33 PDT 2014, Jeffrey Hugo wrote:

> Here in my initial detailed pass.  I still have some "issues" that I
> want to clarify on my end, but I think I have plenty of comments to
> start with.
> 

Thanks Jeff!

> On 9/29/2014 6:34 PM, Bjorn Andersson wrote:
> > The Shared Memory Manager driver implements an interface for allocating
> > and accessing items in the memory area shared among all of the
> > processors in a Qualcomm platform.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > ---
> >
> > In later platforms this is extended to support "secure smem", I do
> > unfortunately not have any devices that I could test this with so I have only
> > implemented the "insecure" version for now.
> >
> > I was thinking about implementing an of_xlate as this would make sense for many
> > things. It is however not feasible for SMD, that would require us to list 257
> > items.
> >
> > It would make sense to enhance this with a method of keeping track of currently
> > consumed items, both to take care of "mutual exclusion" between consumers as
> > well as knowing when it's safe to remove the device and/or unload the driver.
> 
> By "mutual exclusion" I assume you mean that two different entities are
> using the same smem id.  Such a usecase is outside the purview of this
> driver, and is essentially unsupported.  Any such usecases would need to
> be handled by the client.  In short, smem items support one direct client.
> 

Good, I figured that that was the case.

So either we ignore the problem (just hand out pointers to drivers that want
them) or we enforce unique ownership. In the latter case we would have to have
a model for "releasing" items, e.g. when a consumer is removed. I can't think
of any practical case when this would actually matter.

> I'm not sure it makes sense to unload the driver.  I understand this is
> a qcom specific driver, but it is critical to our devices.  Any
> situation I can think of in which this driver would be unloaded, would
> ultimately result in a reboot of the device.
> 

In the case when you compile all parts of this series as modules you could
unload them, but I agree that it makes little sense. But either I need to
handle it or prevent it I guess.

[..]
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index 7bd2c94..9e6bc56 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -9,3 +9,11 @@ config QCOM_GSBI
> >             functions for connecting the underlying serial UART, SPI, and I2C
> >             devices to the output pins.
> >
> > +config QCOM_SMEM
> > +     tristate "Qualcomm Shared Memory Interface"
> > +     depends on ARCH_QCOM
> > +     help
> > +       Say y here to enable support for the Qualcomm Shared Memory Manager.
> > +       The driver provides an interface to items in a heap shared among all
> > +       processors in a Qualcomm platform and is used for exchanging
> > +       information as well as a fifo based communication mechanism (SMD).
> 
> I don't think the reference to SMD is appropriate here.  SMD is one of
> many clients that make use of SMEM.
> 

Ok

> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index 4389012..b1f7b18 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -1 +1,2 @@
> >   obj-$(CONFIG_QCOM_GSBI)     +=      qcom_gsbi.o
> > +obj-$(CONFIG_QCOM_SMEM) +=   qcom_smem.o
> > diff --git a/drivers/soc/qcom/qcom_smem.c b/drivers/soc/qcom/qcom_smem.c
> > new file mode 100644
> > index 0000000..9766c86
> > --- /dev/null
> > +++ b/drivers/soc/qcom/qcom_smem.c
> 
> I see there appears to be a very small ammount of precidence (one other
> file), but it seems pointlessly redundant to prefix "qcom_" to the file
> when it exists in a "qcom" directory.  What is the other point of view
> that I am missing?
> 

I don't know where in the tree this driver should be, but your point is very
much valid.

> > @@ -0,0 +1,328 @@
> > +/*
> > + * Copyright (c) 2014, Sony Mobile Communications AB.
> > + * Copyright (c) 2012-2013, 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/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> > +#include <linux/hwspinlock.h>
> > +#include <linux/soc/qcom/qcom_smem.h>
> 
> Alphabetized please.
> 

Ok

[..]

> > +
> > +/**
> > + * struct smem_entry - entry to reference smem items on the heap
> > + * @allocated:       boolean to indicate if this entry is used
> > + * @offset:  offset to the allocated space
> > + * @size:    size of the allocated space, 8 byte aligned
> > + * @aux_base:        base address for the memory region used by this unit, or 0 for
> > + *           the default region. bits 0,1 are reserved
> > + */
> > +struct smem_entry {
> > +     u32 allocated;
> > +     u32 offset;
> > +     u32 size;
> > +     u32 aux_base; /* bits 1:0 reserved */
> 
> Inline comment appears redundant since its addressed in the comment
> block above
> 

Oops, thanks

> > +};
> > +
> > +/**
> > + * struct smem_header - header found in beginning of primary smem region
> > + * @proc_comm:               proc_comm communication interface (legacy)
> > + * @version:         array of versions for the various subsystems
> > + * @smem_initialized:        boolean to indicate that smem is initialized
> > + * @free_offset:     index of the first unallocated byte in smem
> > + * @available:               number of bytes available for allocation
> > + * @unused:          reserved field
> > + * toc:                      array of references to smem entries
> 
> "@toc"?
> 

Indeed, thanks

> > + */
> > +struct smem_header {
> > +     struct smem_proc_comm proc_comm[4];
> > +     u32 version[32];
> > +     u32 smem_initialized;
> > +     u32 free_offset;
> > +     u32 available;
> > +     u32 unused;
> 
> I see that you inlined the smem_heap_info struct.  That is slightly
> problematic since we have some uses of that structure, and without it,
> accessing id 1 becomes complicated.  I would prefer you reintroduce it.
> 

I need to look into the code to better understand what you're referring to, but
I can reinstate it.

> > +     struct smem_entry toc[SMEM_MAX_ITEMS];
> > +};
> > +
> > +/**
> > +  * struct smem_area - memory region used for smem heap
> 
> smem_region?
> 

Of course

> > +  * @aux_base:       physical base address of the region, used for entry lookup
> > +  * @virt_base:      virtual address of the mapping
> > +  */
> > +struct smem_region {
> > +     u32 aux_base;
> > +     void __iomem *virt_base;
> > +};
> > +
> > +/**
> > +  * struct qcom_smem - control struct for the smem driver
> > +  * @dev:            device pointer
> > +  * @hwlock:         hwspinlock to be held during heap operations
> > +  * @num_regions:    number of entires in the regions array
> > +  * @regions:                array of memory regions, region 0 contains smem_header
> > +  */
> > +struct qcom_smem {
> > +     struct device *dev;
> > +
> > +     struct hwspinlock *hwlock;
> > +
> > +     unsigned num_regions;
> > +     struct smem_region regions[0];
> > +};
> > +
> > +/* Pointer to the one and only smem handle */
> > +static struct qcom_smem *smem_handle;
> > +
> > +/**
> > + * of_get_qcom_smem - retrieve a smem handle from a phandle
> 
> "of_get_qcom_smem()"?
> 

Ok

> > + * @client_node: of_node of the client
> > + *
> > + * Resolve the phandle in the property "qcom,smem" of @client_node and use this
> > + * to retrieve an smem handle.
> > + */
> > +struct qcom_smem *of_get_qcom_smem(struct device_node *client_node)
> > +{
> > +     struct device_node *node;
> > +
> > +     node = of_parse_phandle(client_node, "qcom,smem", 0);
> > +     if (!node)
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     of_node_put(node);
> > +
> > +     if (!smem_handle)
> > +             return ERR_PTR(-EPROBE_DEFER);
> > +     else if (smem_handle->dev->of_node != node)
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     return smem_handle;
> > +}
> > +EXPORT_SYMBOL(of_get_qcom_smem);
> 
> Why?  I understand what this does in a technical sense, but I don't
> understand the reasoning behind it.  This forces every entity that is a
> client of smem to have some kind of existance in DT, when for a lot of
> them, they won't have a need or justification to be in DT other than to
> have a phandle to smem.
> 

By modelling this as a device, with a "proper" life cycle we get around many of
the quirks related to clients accessing the api before this driver is
initialized.

> Smem is a special memory allocator.  I don't recall seeing a DT hook for
> kmalloc/dma_alloc_coherent/alloc_skb/etc so I'm not seeing why we have
> one here.
> 

The problem with smem is that it has dependencies to another device
(hwspinlock), so we have to have some sort of life cycle management (e.g. probe
deferral).

> All this does is get the clients the qcom_smem handle so that they can
> pass it back to smem for alloc and get, but there doesn't seem to be a
> need for it.  The handle is a singleton which this driver already has a
> reference to, so giving it out to the clients and having them hand it
> back seems Rube Goldberg-esq.  It also gives the clients the ability to
> screw with the handle, which I would prefer they never even touch since
> they have no business doing.  You don't even sanity check the handle
> when it gets passed in.  What happens when a client screws up and passes
> in NULL or a random pointer?
> 

I guess there will never be more than one smem instance, so you're right in
that it feels a little bit strange to have to explicitly get a handle to smem.

This api is defined to only take sane pointers, this is the kernel. If you pass
a random pointer I guess you get random behavior.

> The only positive thing about this that I see is it centralizes
> EPROBE_DEFER, but only for well behaved clients.  It doesn't appear that
> it would be too difficult to have that functionality in the simplified
> API you've presented.
> 

I don't see much reason to support anything but "well behaved clients".

Moving the probe defer to get/alloc would in my eyes force clients to implement
strange logic to do late deferal of operations.

> > +
> > +/**
> > + * qcom_smem_alloc - allocate space for a smem item
> 
> "qcom_smem_alloc()"?
> 

Ok

> > + * @smem:    smem handle
> > + * @smem_id: item to be allocated
> > + * @size:    number of bytes to be allocated
> > + *
> > + * Allocate space for a given smem item of size @size, given that the item is
> > + * not yet allocated.
> 
> Return?
> 

That sounds reasonable to document here :)

> > + */
> > +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size)
> > +{
> > +     struct smem_header *header = smem->regions[0].virt_base;
> > +     struct smem_entry *entry;
> > +     unsigned long flags;
> > +     int ret;
> > +
> > +     size = ALIGN(size, 8);
> > +
> > +     if (WARN_ON(smem_id >= SMEM_MAX_ITEMS))
> > +             return -EINVAL;
> > +
> > +     ret = hwspin_lock_timeout_irqsave(smem->hwlock,
> > +                                       HWSPINLOCK_TIMEOUT,
> > +                                       &flags);
> > +     if (ret)
> > +             return ret;
> > +
> > +     entry = &header->toc[smem_id];
> > +     if (entry->allocated) {
> > +             ret = -EEXIST;
> > +             goto out;
> > +     }
> > +
> > +     if (WARN_ON(size > header->available)) {
> > +             ret = -ENOMEM;
> > +             goto out;
> > +     }
> > +
> > +     entry->offset = header->free_offset;
> > +     entry->size = size;
> 
> wmb() is needed here.  We need to ensure the remote side sees the offset
> and size before seeing allocated because they may be doing a lookup
> without allocation without the lock.
> 

Ahh of course, thanks!


But if that's specified in the "protocol", then does that mean that we don't
need to lock at all in get() as the order will make sure that an item is not
allocated before it's structure is fully filled in?

> > +     entry->allocated = 1;
> > +
> > +     header->free_offset += size;
> > +     header->available -= size;
> > +
> > +     /* Commit the changes before we release the spin lock */
> > +     wmb();
> > +out:
> > +     hwspin_unlock_irqrestore(smem->hwlock, &flags);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(qcom_smem_alloc);
> > +
> 
> qcom_smem_get_free_space() kerneldoc?
> 

Ok

> > +unsigned qcom_smem_get_free_space(struct qcom_smem *smem)
> > +{
> > +     struct smem_header *header = smem->regions[0].virt_base;
> > +
> > +     return header->available;
> > +}
> > +EXPORT_SYMBOL(qcom_smem_get_free_space);
> > +
> > +/**
> > + * qcom_smem_get - resolve ptr of size of a smem item
> 
> "qcom_smem_get()"?
> 

Ok

> > + * @smem:    smem handle
> > + * @smem_id: item id to be resolved
> > + * @ptr:     pointer to be filled out with address of the item
> > + * @size:    pointer to be filled out with size of the item
> > + *
> > + * Looks up pointer and size of a smem item.
> 
> Return?
> 

Ok

> > + */
> > +int qcom_smem_get(struct qcom_smem *smem,
> > +               unsigned smem_id,
> > +               void **ptr,
> > +               size_t *size)
> > +{
[..]
> > diff --git a/include/linux/soc/qcom/qcom_smem.h b/include/linux/soc/qcom/qcom_smem.h
> 
> include/soc/ already exists, why add include/linux/soc?
> 

I have to admit I don't really understand why the tegra stuff is in
include/soc/, will have to do some more investigation.

> > new file mode 100644
> > index 0000000..7c0d3fd
> > --- /dev/null
> > +++ b/include/linux/soc/qcom/qcom_smem.h
> > @@ -0,0 +1,14 @@
> > +#ifndef __QCOM_SMEM_H__
> > +#define __QCOM_SMEM_H__
> > +
> 
> Why are the item definitions missing?
> 

Sorry, I don't understand what you're looking for.

> > +struct device_node;
> > +struct qcom_smem;
> > +
> > +struct qcom_smem *of_get_qcom_smem(struct device_node *node);
> > +
> > +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size);
> > +int qcom_smem_get(struct qcom_smem *smem, unsigned item, void **ptr, size_t *size);
> > +
> > +unsigned qcom_smem_get_free_space(struct qcom_smem *smem);
> 
> The above functions should be stubbed out when !CONFIG_QCOM_SMEM, no?
> 

Either that or we just have the clients depend on CONFIG_QCOM_SMEM.

> > +
> > +#endif
> >
> 

Regards,
Bjorn

Re: [RFC 6/7] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD

[Lee Jones]

On Fri, 17 Oct 2014, Bjorn Andersson wrote:
> On Wed 08 Oct 01:40 PDT 2014, Lee Jones wrote:

[...]

> > > +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> > > +	.probe = qcom_smd_rpm_probe,
> > > +	.remove = qcom_smd_rpm_remove,
> > > +	.callback = qcom_smd_rpm_callback,
> > > +	.driver  = {
> > > +		.name  = "qcom_smd_rpm",
> > > +		.owner = THIS_MODULE,
> > > +		.of_match_table = qcom_smd_rpm_of_match,
> > > +	},
> > > +};
> > > +
> > > +module_qcom_smd_driver(qcom_smd_rpm_driver);
> > 
> > I don't like this.  What's wrong with the existing platform driver
> > code?
> > 
> 
> I started off with having smd child devices as platform drivers and had some
> accessor functions to find the open handles that triggered the probe() and
> register the callback with those. But this didn't feel very sane, so I did
> implemented a custom driver struct and probe prototype to simplify writing
> drivers.
> 
> May I ask why you dislike this? This is how it's done in so many other places
> in the kernel...

I don't believe that's the case.  All owners of their own
module_*_driver() registration calls are busses (see below), whereas
'qcom_smd' is just a driver.  Things would soon get out of control if
we allowed every driver in the kernel to supply their own driver
registration information variants.

$ git grep "^module_.*_driver(" | \
  cut -d: -f2 | cut -d'(' -f1 | sort | uniq

module_acpi_driver
module_amba_driver
module_comedi_driver
module_comedi_pci_driver
module_comedi_pcmcia_driver
module_comedi_usb_driver
module_gameport_driver
module_hid_driver
module_i2c_driver
module_mcb_driver
module_mipi_dsi_driver
module_pci_driver
module_pcmcia_driver
module_platform_driver
module_serio_driver
module_spi_driver
module_spmi_driver
module_usb_composite_driver
module_usb_driver
module_usb_serial_driver
module_virtio_driver

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 3/7] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

[Bjorn Andersson]

On Wed 08 Oct 14:47 PDT 2014, Jeffrey Hugo wrote:

> On 9/30/2014 6:08 PM, Bjorn Andersson wrote:
> > On Tue 30 Sep 16:16 PDT 2014, Jeffrey Hugo wrote:
> >
> >> On 9/30/2014 8:37 AM, Bjorn Andersson wrote:
[..]
> >
> > That's right, because you have these tables in devicetree in the caf version.
> > You have to have this information somewhere!
> 
> True, it must exist somewhere.  However since its information tied 
> directly to the hardware, it seems like its information that would fit 
> right in DT.
> 

For family B it would be reasonable to move this to device tree, replacing my
"reg" property with a "resource" property taking type and id for the resource
(e.g resource = <0x616f646c 17>; for ldo 17).

For family A this does not work thought, but maybe we should just skip the idea
of having a common look in favour of not having the tables.

> I talked to my contact, and it seems like the table attributes vary more 
> than I thought, target to target, so the single table design seems less 
> plausible.  Its my understanding you've had an offline discussion with 
> him and some of our regulator guys to discuss the specifics of our 
> needs.  I'll let them take over as the experts.
> 

Yes, I met with Stephen, David and Mahesh to discuss the matter. The issues
that we discussed was:

1) It must be possible to set the active and sleep state of a resource
independently.

2) Deferring less important writes (e.g. writes to sleep state) to reduce
communication

3) With deferred writes, we need to flush the data when going to sleep and that
requires some special handling - so that we don't wake up again or crash the
rpm.

4) The scalability of the table


I'm working on figuring out how to do 1) in a sane way, as this is needed.

2) gives 3) which gives a bunch of implementation awkwardness and I'm still not
convinced that it is a problem beyond someones KPI, so I will have to do some
measurements. If nothing else it's "just" an optimization that we should be
able to add incrementally later.

I guess 4) is a matter of taste, with the table the client reference a
resource, with all the data in DT it also describes part of how the rpm works.

Regards,
Bjorn

Re: [RFC 6/7] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD

[Bjorn Andersson]

On Mon 20 Oct 00:22 PDT 2014, Lee Jones wrote:

> On Fri, 17 Oct 2014, Bjorn Andersson wrote:
> > On Wed 08 Oct 01:40 PDT 2014, Lee Jones wrote:
> 
> [...]
> 
> > > > +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> > > > +	.probe = qcom_smd_rpm_probe,
> > > > +	.remove = qcom_smd_rpm_remove,
> > > > +	.callback = qcom_smd_rpm_callback,
> > > > +	.driver  = {
> > > > +		.name  = "qcom_smd_rpm",
> > > > +		.owner = THIS_MODULE,
> > > > +		.of_match_table = qcom_smd_rpm_of_match,
> > > > +	},
> > > > +};
> > > > +
> > > > +module_qcom_smd_driver(qcom_smd_rpm_driver);
> > > 
> > > I don't like this.  What's wrong with the existing platform driver
> > > code?
> > > 
> > 
> > I started off with having smd child devices as platform drivers and had some
> > accessor functions to find the open handles that triggered the probe() and
> > register the callback with those. But this didn't feel very sane, so I did
> > implemented a custom driver struct and probe prototype to simplify writing
> > drivers.
> > 
> > May I ask why you dislike this? This is how it's done in so many other places
> > in the kernel...
> 
> I don't believe that's the case.  All owners of their own
> module_*_driver() registration calls are busses (see below), whereas
> 'qcom_smd' is just a driver.  Things would soon get out of control if
> we allowed every driver in the kernel to supply their own driver
> registration information variants.
> 

I modelled this after rpmsg, with the intention of having qcom_smd provide a
"smd bus" and all client drivers sitting on that bus being probed and removed
as the remote services appear and disappear.

I'm afraid I don't understand what part I missed that makes my smd driver "just
a driver". I will reread the documentation and try to figure out what I might
have missed.

Regards,
Bjorn

Re: [RFC 4/7] soc: qcom: Add Shared Memory Manager driver

[Andreas Färber]

Hi,

Am 17.10.2014 um 16:51 schrieb Bjorn Andersson:
> On Wed 08 Oct 14:33 PDT 2014, Jeffrey Hugo wrote:
>> On 9/29/2014 6:34 PM, Bjorn Andersson wrote:
>>> diff --git a/include/linux/soc/qcom/qcom_smem.h b/include/linux/soc/qcom/qcom_smem.h
>>
>> include/soc/ already exists, why add include/linux/soc?
>>
> 
> I have to admit I don't really understand why the tegra stuff is in
> include/soc/, will have to do some more investigation.

AFAIR soc/ was mach-* stuff that is shared between arm and arm64.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [RFC 4/7] soc: qcom: Add Shared Memory Manager driver

[Bjorn Andersson]

On Wed 08 Oct 14:33 PDT 2014, Jeffrey Hugo wrote:

[..]
> > + */
> > +struct smem_header {
> > +     struct smem_proc_comm proc_comm[4];
> > +     u32 version[32];
> > +     u32 smem_initialized;
> > +     u32 free_offset;
> > +     u32 available;
> > +     u32 unused;
> 
> I see that you inlined the smem_heap_info struct.  That is slightly
> problematic since we have some uses of that structure, and without it,
> accessing id 1 becomes complicated.  I would prefer you reintroduce it.
> 

Could you help me better understand what you mean here? I've scanned through
msm-3.4 and msm-3.10 and read your comment numerous times, but I can't find any
uses that needs it nor figure out why it would be difficult to access item 1.

> > +     struct smem_entry toc[SMEM_MAX_ITEMS];
> > +};

Thanks,
Bjorn

Re: [RFC 5/7] soc: qcom: Add Shared Memory Driver

[Ohad Ben-Cohen]

[Thanks Kevin for the heads up on this]

Hi Bjorn,

On Tue, Sep 30, 2014 at 3:34 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> == Codeaurora implementation ==
> The codeaurora implementation originates from an implementation that was based purely on initcall and global state

Do you refer to the in-tree arch/arm/mach-msm/smd.c ? it seems similar
in some ways to the newly proposed smd driver.

> == RPMSG ==
>
> The rpmsg_driver fits nicely into what I want to do, so that could be used
> straight off. Data is transmitted by calling rpmsg_send() and incoming data is
> delivered through the registered callback on a channel.  It's possible to
> request additional channels for a rpmsg device, although this is probably never
> tested as there is no way to send data on this additional endpoint.

This is inaccurate - rpmsg does have offchannel sending API, some of
which are being used by the rpmsg core itself. Please check out
rpmsg_sendto() and rpmsg_send_offchannel().

> Endpoints are sort of equivalent of a smd channel although the life cycle is
> slightly different, but a major issue with the rpmsg interface is that channels
> are identified by a single u32, while SMD channels are identified by a u32 and
> the channel name.

Honestly this sounds like a small technical difference that can
probably be fixed with some effort. Rpmsg is designed to satisfy the
requirements of its current users, and is not set in stone. Not even
the wire protocol is.

If another requirement shows up, we can adopt rpmsg so new users could
use it instead of merging additional frameworks that basically do the
same thing.

But new requirements must be well described so we can technically be
convinced a core change is indeed needed.

> So to be able to use the rpmsg api we would need to create additional apis that
> handles the custom address format of the smd channels. Furthermore all the "off
> channel" functions would be invalid.
> Furthermore most of the channel and endpoint structures would be "invalid".

Not sure I'm following this one. Can you please explain the smd
addressing? what part of it is set in stone and what part of it can
still be changed?

> But these are the problems with the actual api, rpmsg is not only the api. It's
> a direct implementation of these functions, defining how services are
> discovered, how channels are represented as well as the actual "wire" protocol.
>
> Or to put it in another way: rpmsg is an implementation of a packet protocol
> on-top of virtio, it is not a framework or api for abstracting packet transport
> logic.

This is inaccurate. If there's justification for another wire
protocol, it can be added. Even virtio itself was once only an
abstraction over the virtqueue wire protocol:

commit 7c5e9ed0c84e7d70d887878574590638d5572659
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Mon Apr 12 16:19:07 2010 +0300

    virtio_ring: remove a level of indirection

    We have a single virtqueue_ops implementation,
    and it seems unlikely we'll get another one
    at this point. So let's remove an unnecessary
    level of indirection: it would be very easy to
    re-add it if another implementation surfaces.

    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

> As there are discussions regarding replacing SMD with a new wire protocol, it
> would probably be convenient to create a generic version of my proposed "api"
> that can be re-used between different implementations and hopefully provide
> re-use of parts of the code. Maybe we can make rpmsg an implementation under
> such a generic api.

If the SMD wire protocol can still be changed that's great. Why don't
you pick up the vrings structures and play with it? You then get rpmsg
for free. If some of the APIs aren't exactly what you need, please
explain and we could change it to fit your requirements.

It's always easier to merge new code rather than understand the
existing one and change it to fit all users, but usually it just means
more work later.

Thanks,
Ohad.

Re: [RFC 5/7] soc: qcom: Add Shared Memory Driver

[Bjorn Andersson]

On Wed 29 Oct 07:28 PDT 2014, Ohad Ben-Cohen wrote:

> [Thanks Kevin for the heads up on this]
> 
> Hi Bjorn,
> 

Hi Ohad,

> On Tue, Sep 30, 2014 at 3:34 AM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
> > == Codeaurora implementation ==
> > The codeaurora implementation originates from an implementation that was based purely on initcall and global state
> 
> Do you refer to the in-tree arch/arm/mach-msm/smd.c ? it seems similar
> in some ways to the newly proposed smd driver.
> 

Correct, that's how the codeaurora driver looked 7 years ago, but there's so
much that changed since then. Wire format is more or less the same though.

As per Arnd's request I have been looking at ways to migrate mach-msm to this
implementation, but I haven't come to any good conclusion yet.

> > == RPMSG ==
> >
> > The rpmsg_driver fits nicely into what I want to do, so that could be used
> > straight off. Data is transmitted by calling rpmsg_send() and incoming data is
> > delivered through the registered callback on a channel.  It's possible to
> > request additional channels for a rpmsg device, although this is probably never
> > tested as there is no way to send data on this additional endpoint.
> 
> This is inaccurate - rpmsg does have offchannel sending API, some of
> which are being used by the rpmsg core itself. Please check out
> rpmsg_sendto() and rpmsg_send_offchannel().
> 

You're correct, I just found the api so awkward that I couldn't imagine that it
was supposed to be used that way.

  ept = rpmsg_create_ept(..., 42);
  rpmsg_sendto(..., 42);
  rpmsg_destroy_ept(ept);

is not a very clean way of doing things... So if anyone would be using it I
would assume that the rpmsg_send() function would start taking and ept as
argument instead of a channel.

> > Endpoints are sort of equivalent of a smd channel although the life cycle is
> > slightly different, but a major issue with the rpmsg interface is that channels
> > are identified by a single u32, while SMD channels are identified by a u32 and
> > the channel name.
> 
> Honestly this sounds like a small technical difference that can
> probably be fixed with some effort. Rpmsg is designed to satisfy the
> requirements of its current users, and is not set in stone. Not even
> the wire protocol is.
> 

We could turn the address into a union and pass that along. But it does not
change the fact that the entire channel management in virtio_rpmsg_bus.c would
have to be rewritten.

> If another requirement shows up, we can adopt rpmsg so new users could
> use it instead of merging additional frameworks that basically do the
> same thing.
> 

I've only spend 2-3 weeks trying to figure out how to get the two to play along
in the same core. For me there are to many fundamental implementation details
that differs for it to be a fit.

> But new requirements must be well described so we can technically be
> convinced a core change is indeed needed.
> 

For starters, there is no rpmsg core, there is a virtio based bus
implementation of rpmsg. So we would need to get rid of the strong ties to virtio.

Compare rpmsg_recv_done() with qcom_smd_channel_intr() from my patch, those two
functions are (on a powerpoint level) doing exactly the same task - looping
over incoming data and calling rpmsg_recv_single() and
qcom_smd_channel_recv_single() respectively. Also look at qcom_smd_edge_intr(),
that is how we are notified that there might be data in the buffers.

Compare how rpmsg allocates fixed size buffers from a ringbuffer, while smd
have a ringbuffer pair per channel, where the variable size packages are stored
packed and the rx/tx index pointers are updated.

Compare how the life cycle of a channel in rpmsg is compared to smd. In rpmsg
we get a call to rpmsg_ns_cb() telling us that a channel was registered or went
away. In smd this is first handled by someone allocating memory for the
channel, which qcom_channel_scan_worker() picks up; when this channel changes
state because the remote side is opening it this is detected in
qcom_channel_state_worker(). If the other side closed the channel simply the
state of the channel changes (to closed) and qcom_channel_state_worker() picks
this up - there are no notification events related to this.

> > So to be able to use the rpmsg api we would need to create additional apis that
> > handles the custom address format of the smd channels. Furthermore all the "off
> > channel" functions would be invalid.
> > Furthermore most of the channel and endpoint structures would be "invalid".
> 
> Not sure I'm following this one. Can you please explain the smd
> addressing? what part of it is set in stone and what part of it can
> still be changed?
> 

A SMD channel consists of one pair of control structures smd_channel_info or
smd_channel_info_word and one pair of ring buffers - one for tx, one for rx.
The channel info structures are used to indicate to the other side of the
channel:
*) what the local state is (closed/opening/open/closing)
*) where write and read pointers are within each ringbuffer
*) flow control bits

Channels are allocated on first use and stay allocated until you reboot the
platform, the both sides uses the state to indicate if there's an
implementation available to communicate with.

References to the channels are kept in a lookup table - consisting of
qcom_smd_alloc_entry - and are identified by remote processor identifier (u32)
and a channel name (char[]).


Each of these listed "items" are represented as smem items - a item heap shared
among all the processors in a Qualcomm platform.

> > But these are the problems with the actual api, rpmsg is not only the api. It's
> > a direct implementation of these functions, defining how services are
> > discovered, how channels are represented as well as the actual "wire" protocol.
> >
> > Or to put it in another way: rpmsg is an implementation of a packet protocol
> > on-top of virtio, it is not a framework or api for abstracting packet transport
> > logic.
> 
> This is inaccurate. If there's justification for another wire
> protocol, it can be added. Even virtio itself was once only an
> abstraction over the virtqueue wire protocol:
> 

Correct, what I'm saying is that we need to add another wire protocol, we need
to add another memory management model, we need to add another channel life
cycle model and we need to make modifications to the api.

Or in other words, we need to change everything that defines rpmsg today.
That's why I don't think it's a wise path to take.

> commit 7c5e9ed0c84e7d70d887878574590638d5572659
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Mon Apr 12 16:19:07 2010 +0300
> 
>     virtio_ring: remove a level of indirection
> 
>     We have a single virtqueue_ops implementation,
>     and it seems unlikely we'll get another one
>     at this point. So let's remove an unnecessary
>     level of indirection: it would be very easy to
>     re-add it if another implementation surfaces.
> 
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 

Upon sending a packet to a remote processor in SMD we get some sort of pointer
and size from a client, the work to be done is to write that into the tx
ringbuffer at the index specified in the channel info struct, increment that
index and then kick the other side.

As far as I can see this is not a good fit for virtio or virtqueue, so I was
hoping that we won't complicate either of those implementations for the sake of
fitting into a framework.

> > As there are discussions regarding replacing SMD with a new wire protocol, it
> > would probably be convenient to create a generic version of my proposed "api"
> > that can be re-used between different implementations and hopefully provide
> > re-use of parts of the code. Maybe we can make rpmsg an implementation under
> > such a generic api.
> 
> If the SMD wire protocol can still be changed that's great. Why don't
> you pick up the vrings structures and play with it? You then get rpmsg
> for free. If some of the APIs aren't exactly what you need, please
> explain and we could change it to fit your requirements.
> 

Qualcomm recently announced that they now shipped 1 billion Snapdragon based
Android phones - they are all using the SMD wire protocol and they will never
support anything else.

I've of course poked Qualcomm engineers regarding using rpmsg for future
platforms and althought there are work ongoing to replace SMD I would be
surprised if they picked rpmsg as their next solution...

> It's always easier to merge new code rather than understand the
> existing one and change it to fit all users, but usually it just means
> more work later.
> 

My concern is that merging SMD into rpmsg will still give us two different
solutions, except that they will be deeply tangled.

Thanks for your feedback Ohad!

Regards,
Bjorn

Re: [RFC 5/7] soc: qcom: Add Shared Memory Driver

[Ohad Ben-Cohen]

Hi Bjorn,

On Thu, Oct 30, 2014 at 2:38 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> My concern is that merging SMD into rpmsg will still give us two different
> solutions, except that they will be deeply tangled.

I agree.

It's very clear that SMD and rpmsg are different. But fundamentally,
they do exactly the same: new messages are placed in a ring buffer and
then an interrupt is triggered.

If Qualcomm wanted to adopt rpmsg I'm confident there would have been
no technical barriers to do so.

But given that SMD is not going anywhere, I personally hope that you
could at least avoid having two different implementations of it merged
upstream.

Good luck,
Ohad.

Re: [RFC 5/7] soc: qcom: Add Shared Memory Driver

[Bjorn Andersson]

On Thu 30 Oct 06:34 PDT 2014, Ohad Ben-Cohen wrote:

> Hi Bjorn,
> 
> On Thu, Oct 30, 2014 at 2:38 AM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
> > My concern is that merging SMD into rpmsg will still give us two different
> > solutions, except that they will be deeply tangled.
> 
> I agree.
> 
> It's very clear that SMD and rpmsg are different. But fundamentally,
> they do exactly the same: new messages are placed in a ring buffer and
> then an interrupt is triggered.
> 
> If Qualcomm wanted to adopt rpmsg I'm confident there would have been
> no technical barriers to do so.
> 

Their wish list seems long, but I think you're right :)

> But given that SMD is not going anywhere, I personally hope that you
> could at least avoid having two different implementations of it merged
> upstream.
> 

Looking at the lack of functionality in mach-msm it seems like it's quite a lot
of work getting to a point where anyone could actually test smd on those devices.

If anyone have any input on how I get a piece of user space that will run with
mainline and communicate with the modem to test this I'm happily listening.

Regards,
Bjorn