[PATCH v2 0/2] i2c: Add Tegra BPMP I2C proxy driver

[PATCH v2 0/2] i2c: Add Tegra BPMP I2C proxy driver

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi Wolfram,

I had originally sent these in for the last merge window but then got
bounces indicating that your email server was not accepting emails. At
the time it seemed like I wasn't the only one having these problems.

Looking at my mailbox that situation now seems to be resolved, so here's
hoping it'll work this time. Note that because of the above I've carried
this in the Tegra tree for a while now to get it more compile testing.
If you end up applying the set, please let me know so I can drop them
from the Tegra tree and avoid conflicts in linux-next.

On the bright side the dependencies for this were merged during the last
release cycle, so these should be good to go in without further
complications.

Thanks,
Thierry

Shardar Shariff Md (1):
  i2c: Add Tegra BPMP I2C proxy driver

Stephen Warren (1):
  dt-bindings: Add Tegra186 BPMP I2C binding

 .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      |  42 +++
 drivers/i2c/busses/Kconfig                         |  11 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-tegra-bpmp.c                | 365 +++++++++++++++++++++
 4 files changed, 419 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
 create mode 100644 drivers/i2c/busses/i2c-tegra-bpmp.c

-- 
2.11.0

[PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding

[Thierry Reding]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
HW devices, such as the I2C controller for the power management I2C bus.
Software running on other CPUs must perform IPC to the BPMP in order to
execute transactions on that I2C bus. This binding describes an I2C bus
that is accessed in such a fashion.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Tom Warren <twarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt

diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
new file mode 100644
index 000000000000..ab240e10debc
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
@@ -0,0 +1,42 @@
+NVIDIA Tegra186 BPMP I2C controller
+
+In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
+devices, such as the I2C controller for the power management I2C bus. Software
+running on other CPUs must perform IPC to the BPMP in order to execute
+transactions on that I2C bus. This binding describes an I2C bus that is
+accessed in such a fashion.
+
+The BPMP I2C node must be located directly inside the main BPMP node. See
+../firmware/nvidia,tegra186-bpmp.txt for details of the BPMP binding.
+
+This node represents an I2C controller. See ../i2c/i2c.txt for details of the
+core I2C binding.
+
+Required properties:
+- compatible:
+    Array of strings.
+    One of:
+    - "nvidia,tegra186-bpmp-i2c".
+- #address-cells: Address cells for I2C device address.
+    Single-cell integer.
+    Must be <1>.
+- #size-cells:
+    Single-cell integer.
+    Must be <0>.
+- nvidia,bpmp-bus-id:
+    Single-cell integer.
+    Indicates the I2C bus number this DT node represent, as defined by the
+    BPMP firmware.
+
+Example:
+
+bpmp {
+	...
+
+	i2c {
+		compatible = "nvidia,tegra186-bpmp-i2c";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		nvidia,bpmp-bus-id = <5>;
+	};
+};
-- 
2.11.0

[PATCH v2 2/2] i2c: Add Tegra BPMP I2C proxy driver

[Thierry Reding]

From: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Add Tegra BPMP I2C driver. The BPMP is the boot and power management
processor embedded in Tegra SoCs. In newer SoC versions, access to one
of the I2C busses goes via the BPMP, requiring a different "proxy" I2C
driver that accesses the bus via the real I2C driver embedded in the
BPMP firmware.

Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/Kconfig          |  11 ++
 drivers/i2c/busses/Makefile         |   1 +
 drivers/i2c/busses/i2c-tegra-bpmp.c | 365 ++++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-tegra-bpmp.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 0cdc8443deab..e4a603e5e90a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -919,6 +919,17 @@ config I2C_TEGRA
 	  If you say yes to this option, support will be included for the
 	  I2C controller embedded in NVIDIA Tegra SOCs
 
+config I2C_TEGRA_BPMP
+	tristate "NVIDIA Tegra BPMP I2C controller"
+	depends on TEGRA_BPMP
+	help
+	  If you say yes to this option, support will be included for the I2C
+	  controller embedded in NVIDIA Tegra SoCs accessed via the BPMP.
+
+	  This I2C driver is a 'virtual' I2C driver. The real driver is part
+	  of the BPMP firmware, and this driver merely communicates with that
+	  real driver.
+
 config I2C_UNIPHIER
 	tristate "UniPhier FIFO-less I2C controller"
 	depends on ARCH_UNIPHIER || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 1c1bac87a9db..beb4809cfd47 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_I2C_ST)		+= i2c-st.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
 obj-$(CONFIG_I2C_SUN6I_P2WI)	+= i2c-sun6i-p2wi.o
 obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
+obj-$(CONFIG_I2C_TEGRA_BPMP)	+= i2c-tegra-bpmp.o
 obj-$(CONFIG_I2C_UNIPHIER)	+= i2c-uniphier.o
 obj-$(CONFIG_I2C_UNIPHIER_F)	+= i2c-uniphier-f.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
diff --git a/drivers/i2c/busses/i2c-tegra-bpmp.c b/drivers/i2c/busses/i2c-tegra-bpmp.c
new file mode 100644
index 000000000000..480251cce238
--- /dev/null
+++ b/drivers/i2c/busses/i2c-tegra-bpmp.c
@@ -0,0 +1,365 @@
+/*
+ * drivers/i2c/busses/i2c-tegra-bpmp.c
+ *
+ * Copyright (c) 2016 NVIDIA Corporation.  All rights reserved.
+ *
+ * Author: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#include <soc/tegra/bpmp-abi.h>
+#include <soc/tegra/bpmp.h>
+
+/*
+ * Serialized I2C message header size is 6 bytes and includes address, flags
+ * and length
+ */
+#define SERIALI2C_HDR_SIZE 6
+
+struct tegra_bpmp_i2c {
+	struct i2c_adapter adapter;
+	struct device *dev;
+
+	struct tegra_bpmp *bpmp;
+	unsigned int bus;
+};
+
+/*
+ * Linux flags are translated to BPMP defined I2C flags that are used in BPMP
+ * firmware I2C driver to avoid any issues in future if Linux I2C flags are
+ * changed.
+ */
+static int tegra_bpmp_xlate_flags(u16 flags, u16 *out)
+{
+	if (flags & I2C_M_TEN) {
+		*out |= SERIALI2C_TEN;
+		flags &= ~I2C_M_TEN;
+	}
+
+	if (flags & I2C_M_RD) {
+		*out |= SERIALI2C_RD;
+		flags &= ~I2C_M_RD;
+	}
+
+	if (flags & I2C_M_STOP) {
+		*out |= SERIALI2C_STOP;
+		flags &= ~I2C_M_STOP;
+	}
+
+	if (flags & I2C_M_NOSTART) {
+		*out |= SERIALI2C_NOSTART;
+		flags &= ~I2C_M_NOSTART;
+	}
+
+	if (flags & I2C_M_REV_DIR_ADDR) {
+		*out |= SERIALI2C_REV_DIR_ADDR;
+		flags &= ~I2C_M_REV_DIR_ADDR;
+	}
+
+	if (flags & I2C_M_IGNORE_NAK) {
+		*out |= SERIALI2C_IGNORE_NAK;
+		flags &= ~I2C_M_IGNORE_NAK;
+	}
+
+	if (flags & I2C_M_NO_RD_ACK) {
+		*out |= SERIALI2C_NO_RD_ACK;
+		flags &= ~I2C_M_NO_RD_ACK;
+	}
+
+	if (flags & I2C_M_RECV_LEN) {
+		*out |= SERIALI2C_RECV_LEN;
+		flags &= ~I2C_M_RECV_LEN;
+	}
+
+	return (flags != 0) ? -EINVAL : 0;
+}
+
+/**
+ * The serialized I2C format is simply the following:
+ * [addr little-endian][flags little-endian][len little-endian][data if write]
+ * [addr little-endian][flags little-endian][len little-endian][data if write]
+ *  ...
+ *
+ * The flags are translated from Linux kernel representation to seriali2c
+ * representation. Any undefined flag being set causes an error.
+ *
+ * The data is there only for writes. Reads have the data transferred in the
+ * other direction, and thus data is not present.
+ *
+ * See deserialize_i2c documentation for the data format in the other direction.
+ */
+static int tegra_bpmp_serialize_i2c_msg(struct tegra_bpmp_i2c *i2c,
+					struct mrq_i2c_request *request,
+					struct i2c_msg *msgs,
+					unsigned int num)
+{
+	char *buf = request->xfer.data_buf;
+	unsigned int i, j, pos = 0;
+	int err;
+
+	for (i = 0; i < num; i++) {
+		struct i2c_msg *msg = &msgs[i];
+		u16 flags = 0;
+
+		err = tegra_bpmp_xlate_flags(msg->flags, &flags);
+		if (err < 0)
+			return err;
+
+		buf[pos++] = msg->addr & 0xff;
+		buf[pos++] = (msg->addr & 0xff00) >> 8;
+		buf[pos++] = flags & 0xff;
+		buf[pos++] = (flags & 0xff00) >> 8;
+		buf[pos++] = msg->len & 0xff;
+		buf[pos++] = (msg->len & 0xff00) >> 8;
+
+		if ((flags & SERIALI2C_RD) == 0) {
+			for (j = 0; j < msg->len; j++)
+				buf[pos++] = msg->buf[j];
+		}
+	}
+
+	request->xfer.data_size = pos;
+
+	return 0;
+}
+
+/**
+ * The data in the BPMP -> CPU direction is composed of sequential blocks for
+ * those messages that have I2C_M_RD. So, for example, if you have:
+ *
+ * - !I2C_M_RD, len == 5, data == a0 01 02 03 04
+ * - !I2C_M_RD, len == 1, data == a0
+ * - I2C_M_RD, len == 2, data == [uninitialized buffer 1]
+ * - !I2C_M_RD, len == 1, data == a2
+ * - I2C_M_RD, len == 2, data == [uninitialized buffer 2]
+ *
+ * ...then the data in the BPMP -> CPU direction would be 4 bytes total, and
+ * would contain 2 bytes that will go to uninitialized buffer 1, and 2 bytes
+ * that will go to uninitialized buffer 2.
+ */
+static int tegra_bpmp_i2c_deserialize(struct tegra_bpmp_i2c *i2c, char *buf,
+				      size_t size, struct i2c_msg *msgs,
+				      unsigned int num)
+{
+	size_t len = 0, pos = 0;
+	unsigned int i;
+
+	for (i = 0; i < num; i++)
+		if (msgs[i].flags & I2C_M_RD)
+			len += msgs[i].len;
+
+	if (len != size)
+		return -EINVAL;
+
+	for (i = 0; i < num; i++) {
+		if (msgs[i].flags & I2C_M_RD) {
+			memcpy(msgs[i].buf, buf + pos, msgs[i].len);
+			pos += msgs[i].len;
+		}
+	}
+
+	return 0;
+}
+
+static int tegra_bpmp_i2c_msg_len_check(struct i2c_msg *msgs, unsigned int num)
+{
+	size_t tx_len = 0, rx_len = 0;
+	unsigned int i;
+
+	for (i = 0; i < num; i++)
+		if (!(msgs[i].flags & I2C_M_RD))
+			tx_len += SERIALI2C_HDR_SIZE + msgs[i].len;
+
+	if (tx_len > TEGRA_I2C_IPC_MAX_IN_BUF_SIZE)
+		return -EINVAL;
+
+	for (i = 0; i < num; i++)
+		if ((msgs[i].flags & I2C_M_RD))
+			rx_len += msgs[i].len;
+
+	if (rx_len > TEGRA_I2C_IPC_MAX_OUT_BUF_SIZE)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int tegra_bpmp_i2c_msg_xfer(struct tegra_bpmp_i2c *i2c,
+				   struct mrq_i2c_request *request,
+				   struct mrq_i2c_response *response)
+{
+	struct tegra_bpmp_message msg;
+	int err;
+
+	request->cmd = CMD_I2C_XFER;
+	request->xfer.bus_id = i2c->bus;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_I2C;
+	msg.tx.data = request;
+	msg.tx.size = sizeof(*request);
+	msg.rx.data = response;
+	msg.rx.size = sizeof(*response);
+
+	if (irqs_disabled())
+		err = tegra_bpmp_transfer_atomic(i2c->bpmp, &msg);
+	else
+		err = tegra_bpmp_transfer(i2c->bpmp, &msg);
+
+	return err;
+}
+
+static int tegra_bpmp_i2c_xfer(struct i2c_adapter *adapter,
+			       struct i2c_msg *msgs, int num)
+{
+	struct tegra_bpmp_i2c *i2c = i2c_get_adapdata(adapter);
+	struct mrq_i2c_response response;
+	struct mrq_i2c_request request;
+	int err;
+
+	err = tegra_bpmp_i2c_msg_len_check(msgs, num);
+	if (err < 0) {
+		dev_err(i2c->dev, "unsupported message length\n");
+		return err;
+	}
+
+	/* TODO: move this somewhere else */
+	memset(&request, 0, sizeof(request));
+	memset(&response, 0, sizeof(response));
+
+	err = tegra_bpmp_serialize_i2c_msg(i2c, &request, msgs, num);
+	if (err < 0) {
+		dev_err(i2c->dev, "failed to serialize message: %d\n", err);
+		return err;
+	}
+
+	err = tegra_bpmp_i2c_msg_xfer(i2c, &request, &response);
+	if (err < 0) {
+		dev_err(i2c->dev, "failed to transfer message: %d\n", err);
+		return err;
+	}
+
+	err = tegra_bpmp_i2c_deserialize(i2c, response.xfer.data_buf,
+					 response.xfer.data_size,
+					 msgs, num);
+	if (err < 0) {
+		dev_err(i2c->dev, "failed to deserialize message: %d\n", err);
+		return err;
+	}
+
+	return num;
+}
+
+static u32 tegra_bpmp_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
+	       I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_NOSTART;
+}
+
+static const struct i2c_algorithm tegra_bpmp_i2c_algo = {
+	.master_xfer = tegra_bpmp_i2c_xfer,
+	.functionality = tegra_bpmp_i2c_func,
+};
+
+static int tegra_bpmp_i2c_probe(struct platform_device *pdev)
+{
+	struct tegra_bpmp_i2c *i2c;
+	u32 value;
+	int err;
+
+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->dev = &pdev->dev;
+
+	i2c->bpmp = dev_get_drvdata(pdev->dev.parent);
+	if (!i2c->bpmp)
+		return -ENODEV;
+
+	err = of_property_read_u32(pdev->dev.of_node, "nvidia,bpmp-bus-id",
+				   &value);
+	if (err < 0)
+		return err;
+
+	i2c->bus = value;
+
+	i2c_set_adapdata(&i2c->adapter, i2c);
+	i2c->adapter.owner = THIS_MODULE;
+	i2c->adapter.class = I2C_CLASS_HWMON;
+	strlcpy(i2c->adapter.name, "Tegra BPMP I2C adapter",
+		sizeof(i2c->adapter.name));
+	i2c->adapter.algo = &tegra_bpmp_i2c_algo;
+	i2c->adapter.dev.parent = &pdev->dev;
+	i2c->adapter.dev.of_node = pdev->dev.of_node;
+
+	platform_set_drvdata(pdev, i2c);
+
+	err = i2c_add_adapter(&i2c->adapter);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to add I2C adapter: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int tegra_bpmp_i2c_remove(struct platform_device *pdev)
+{
+	struct tegra_bpmp_i2c *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adapter);
+
+	return 0;
+}
+
+static const struct of_device_id tegra_bpmp_i2c_of_match[] = {
+	{ .compatible = "nvidia,tegra186-bpmp-i2c", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tegra_bpmp_i2c_of_match);
+
+static struct platform_driver tegra_bpmp_i2c_driver = {
+	.driver = {
+		.name = "tegra-bpmp-i2c",
+		.of_match_table = tegra_bpmp_i2c_of_match,
+	},
+	.probe = tegra_bpmp_i2c_probe,
+	.remove = tegra_bpmp_i2c_remove,
+};
+
+static int __init tegra_bpmp_i2c_init_driver(void)
+{
+	return platform_driver_register(&tegra_bpmp_i2c_driver);
+}
+subsys_initcall(tegra_bpmp_i2c_init_driver);
+
+static void __exit tegra_bpmp_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&tegra_bpmp_i2c_driver);
+}
+module_exit(tegra_bpmp_i2c_exit_driver);
+
+MODULE_DESCRIPTION("NVIDIA Tegra BPMP I2C bus contoller driver");
+MODULE_AUTHOR("Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
+MODULE_AUTHOR("Juha-Matti Tilli");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding

[Wolfram Sang]

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


> +- nvidia,bpmp-bus-id:
> +    Single-cell integer.
> +    Indicates the I2C bus number this DT node represent, as defined by the
> +    BPMP firmware.

I'd like to get an ack from Rob for this one if vendor-specific bindings
are still the way to go to encode firmware data. I simply don't know.

> +
> +Example:
> +
> +bpmp {
> +	...
> +
> +	i2c {
> +		compatible = "nvidia,tegra186-bpmp-i2c";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		nvidia,bpmp-bus-id = <5>;
> +	};
> +};

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding

[Thierry Reding]

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

On Fri, Jan 27, 2017 at 09:39:38AM +0100, Thierry Reding wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
> HW devices, such as the I2C controller for the power management I2C bus.
> Software running on other CPUs must perform IPC to the BPMP in order to
> execute transactions on that I2C bus. This binding describes an I2C bus
> that is accessed in such a fashion.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Tom Warren <twarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt

Rob, sorry for not Cc'ing you earlier on this, but I had thought this
binding had already been reviewed and acked since it is merged in
U-Boot.

Wolfram was concerned in particular about the nvidia,bpmp-bus-id
property below.

Can you give this a quick look, please?

Thanks,
Thierry

> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> new file mode 100644
> index 000000000000..ab240e10debc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> @@ -0,0 +1,42 @@
> +NVIDIA Tegra186 BPMP I2C controller
> +
> +In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
> +devices, such as the I2C controller for the power management I2C bus. Software
> +running on other CPUs must perform IPC to the BPMP in order to execute
> +transactions on that I2C bus. This binding describes an I2C bus that is
> +accessed in such a fashion.
> +
> +The BPMP I2C node must be located directly inside the main BPMP node. See
> +../firmware/nvidia,tegra186-bpmp.txt for details of the BPMP binding.
> +
> +This node represents an I2C controller. See ../i2c/i2c.txt for details of the
> +core I2C binding.
> +
> +Required properties:
> +- compatible:
> +    Array of strings.
> +    One of:
> +    - "nvidia,tegra186-bpmp-i2c".
> +- #address-cells: Address cells for I2C device address.
> +    Single-cell integer.
> +    Must be <1>.
> +- #size-cells:
> +    Single-cell integer.
> +    Must be <0>.
> +- nvidia,bpmp-bus-id:
> +    Single-cell integer.
> +    Indicates the I2C bus number this DT node represent, as defined by the
> +    BPMP firmware.
> +
> +Example:
> +
> +bpmp {
> +	...
> +
> +	i2c {
> +		compatible = "nvidia,tegra186-bpmp-i2c";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		nvidia,bpmp-bus-id = <5>;
> +	};
> +};
> -- 
> 2.11.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding

[Rob Herring]

On Fri, Jan 27, 2017 at 4:19 PM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Jan 27, 2017 at 09:39:38AM +0100, Thierry Reding wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
>> HW devices, such as the I2C controller for the power management I2C bus.
>> Software running on other CPUs must perform IPC to the BPMP in order to
>> execute transactions on that I2C bus. This binding describes an I2C bus
>> that is accessed in such a fashion.
>>
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Tom Warren <twarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 42 ++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>
> Rob, sorry for not Cc'ing you earlier on this, but I had thought this
> binding had already been reviewed and acked since it is merged in
> U-Boot.

Ha! Good one. :(

>
> Wolfram was concerned in particular about the nvidia,bpmp-bus-id
> property below.
>
> Can you give this a quick look, please?
>
> Thanks,
> Thierry
>
>> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>> new file mode 100644
>> index 000000000000..ab240e10debc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>> @@ -0,0 +1,42 @@
>> +NVIDIA Tegra186 BPMP I2C controller
>> +
>> +In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
>> +devices, such as the I2C controller for the power management I2C bus. Software
>> +running on other CPUs must perform IPC to the BPMP in order to execute
>> +transactions on that I2C bus. This binding describes an I2C bus that is
>> +accessed in such a fashion.
>> +
>> +The BPMP I2C node must be located directly inside the main BPMP node. See
>> +../firmware/nvidia,tegra186-bpmp.txt for details of the BPMP binding.
>> +
>> +This node represents an I2C controller. See ../i2c/i2c.txt for details of the
>> +core I2C binding.
>> +
>> +Required properties:
>> +- compatible:
>> +    Array of strings.
>> +    One of:
>> +    - "nvidia,tegra186-bpmp-i2c".
>> +- #address-cells: Address cells for I2C device address.
>> +    Single-cell integer.
>> +    Must be <1>.
>> +- #size-cells:
>> +    Single-cell integer.
>> +    Must be <0>.
>> +- nvidia,bpmp-bus-id:
>> +    Single-cell integer.
>> +    Indicates the I2C bus number this DT node represent, as defined by the
>> +    BPMP firmware.

This seems okay to me given it's a pretty specific use and dictated by
the firmware. If there were multiple, then perhaps we should use reg,
but that may collide with other BPMP child nodes.

Rob

Re: [PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding

[Stephen Warren]

On 01/27/2017 03:19 PM, Thierry Reding wrote:
> On Fri, Jan 27, 2017 at 09:39:38AM +0100, Thierry Reding wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
>> HW devices, such as the I2C controller for the power management I2C bus.
>> Software running on other CPUs must perform IPC to the BPMP in order to
>> execute transactions on that I2C bus. This binding describes an I2C bus
>> that is accessed in such a fashion.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>> Acked-by: Jon Hunter <jonathanh@nvidia.com>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>>  .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 42 ++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>
> Rob, sorry for not Cc'ing you earlier on this, but I had thought this
> binding had already been reviewed and acked since it is merged in
> U-Boot.

I'm pretty sure all the DT binding patches for Tegra186 that I submitted 
were ack'd by Rob. They were certainly all posted to relevant Linux 
lists and discussed and ack'd by someone before I pushed them into 
U-Boot. Probably 2nd quarter last year, or maybe 3rd.

Re: [PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding

[Thierry Reding]

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

On Fri, Jan 27, 2017 at 04:52:37PM -0600, Rob Herring wrote:
> On Fri, Jan 27, 2017 at 4:19 PM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Fri, Jan 27, 2017 at 09:39:38AM +0100, Thierry Reding wrote:
> >> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>
> >> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
> >> HW devices, such as the I2C controller for the power management I2C bus.
> >> Software running on other CPUs must perform IPC to the BPMP in order to
> >> execute transactions on that I2C bus. This binding describes an I2C bus
> >> that is accessed in such a fashion.
> >>
> >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> Signed-off-by: Tom Warren <twarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 42 ++++++++++++++++++++++
> >>  1 file changed, 42 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> >
> > Rob, sorry for not Cc'ing you earlier on this, but I had thought this
> > binding had already been reviewed and acked since it is merged in
> > U-Boot.
> 
> Ha! Good one. :(

So I went looking for earlier discussion on this patch because I
distinctly remembered you being part of it, and found this:

	https://patchwork.ozlabs.org/patch/650385/

You already gave an Acked-by, but I failed to pick it up for some
reason. Given what you said below I'm going to assume that it still
applies.

Wolfram, do you want me to resend with Rob's Acked-by from back in
July or do you want to add it yourself when applying?

Thanks,
Thierry

> > Wolfram was concerned in particular about the nvidia,bpmp-bus-id
> > property below.
> >
> > Can you give this a quick look, please?
> >
> > Thanks,
> > Thierry
> >
> >> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> >> new file mode 100644
> >> index 000000000000..ab240e10debc
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> >> @@ -0,0 +1,42 @@
> >> +NVIDIA Tegra186 BPMP I2C controller
> >> +
> >> +In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
> >> +devices, such as the I2C controller for the power management I2C bus. Software
> >> +running on other CPUs must perform IPC to the BPMP in order to execute
> >> +transactions on that I2C bus. This binding describes an I2C bus that is
> >> +accessed in such a fashion.
> >> +
> >> +The BPMP I2C node must be located directly inside the main BPMP node. See
> >> +../firmware/nvidia,tegra186-bpmp.txt for details of the BPMP binding.
> >> +
> >> +This node represents an I2C controller. See ../i2c/i2c.txt for details of the
> >> +core I2C binding.
> >> +
> >> +Required properties:
> >> +- compatible:
> >> +    Array of strings.
> >> +    One of:
> >> +    - "nvidia,tegra186-bpmp-i2c".
> >> +- #address-cells: Address cells for I2C device address.
> >> +    Single-cell integer.
> >> +    Must be <1>.
> >> +- #size-cells:
> >> +    Single-cell integer.
> >> +    Must be <0>.
> >> +- nvidia,bpmp-bus-id:
> >> +    Single-cell integer.
> >> +    Indicates the I2C bus number this DT node represent, as defined by the
> >> +    BPMP firmware.
> 
> This seems okay to me given it's a pretty specific use and dictated by
> the firmware. If there were multiple, then perhaps we should use reg,
> but that may collide with other BPMP child nodes.
> 
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding

[Thierry Reding]

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

On Fri, Jan 27, 2017 at 10:42:27PM -0700, Stephen Warren wrote:
> On 01/27/2017 03:19 PM, Thierry Reding wrote:
> > On Fri, Jan 27, 2017 at 09:39:38AM +0100, Thierry Reding wrote:
> > > From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > 
> > > In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
> > > HW devices, such as the I2C controller for the power management I2C bus.
> > > Software running on other CPUs must perform IPC to the BPMP in order to
> > > execute transactions on that I2C bus. This binding describes an I2C bus
> > > that is accessed in such a fashion.
> > > 
> > > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > Signed-off-by: Tom Warren <twarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 42 ++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> > 
> > Rob, sorry for not Cc'ing you earlier on this, but I had thought this
> > binding had already been reviewed and acked since it is merged in
> > U-Boot.
> 
> I'm pretty sure all the DT binding patches for Tegra186 that I submitted
> were ack'd by Rob. They were certainly all posted to relevant Linux lists
> and discussed and ack'd by someone before I pushed them into U-Boot.
> Probably 2nd quarter last year, or maybe 3rd.

Yes, I thought I had seen an Acked-by myself, and I know that you're
very diligent about this. And true enough, it's in patchwork. I probably
missed it because I was cherry-picking it directly from U-Boot where the
Acked-by wasn't present.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] i2c: Add Tegra BPMP I2C proxy driver

[Wolfram Sang]

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

Hi there,

> +static int tegra_bpmp_i2c_msg_len_check(struct i2c_msg *msgs, unsigned int num)
> +{
> +	size_t tx_len = 0, rx_len = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < num; i++)
> +		if (!(msgs[i].flags & I2C_M_RD))
> +			tx_len += SERIALI2C_HDR_SIZE + msgs[i].len;
> +
> +	if (tx_len > TEGRA_I2C_IPC_MAX_IN_BUF_SIZE)
> +		return -EINVAL;
> +
> +	for (i = 0; i < num; i++)
> +		if ((msgs[i].flags & I2C_M_RD))
> +			rx_len += msgs[i].len;

You could have done with iterating over all msgs only once, but no need
to fix. I think I'll update the i2c core quirks feature to support your
case and rework that incrementally.

> +	/* TODO: move this somewhere else */
> +	memset(&request, 0, sizeof(request));
> +	memset(&response, 0, sizeof(response));

So? :)

> +	i2c->adapter.class = I2C_CLASS_HWMON;

Do you really need that?

> +	err = i2c_add_adapter(&i2c->adapter);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "failed to add I2C adapter: %d\n", err);

No error message here, the core will do that.

> +static int __init tegra_bpmp_i2c_init_driver(void)
> +{
> +	return platform_driver_register(&tegra_bpmp_i2c_driver);
> +}
> +subsys_initcall(tegra_bpmp_i2c_init_driver);

Have you tried if subsys_initcall is really really necessary?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding

[Thierry Reding]

From: Stephen Warren <swarren@nvidia.com>

In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
HW devices, such as the I2C controller for the power management I2C bus.
Software running on other CPUs must perform IPC to the BPMP in order to
execute transactions on that I2C bus. This binding describes an I2C bus
that is accessed in such a fashion.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Tom Warren <twarren@nvidia.com>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt

diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
new file mode 100644
index 000000000000..ab240e10debc
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
@@ -0,0 +1,42 @@
+NVIDIA Tegra186 BPMP I2C controller
+
+In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
+devices, such as the I2C controller for the power management I2C bus. Software
+running on other CPUs must perform IPC to the BPMP in order to execute
+transactions on that I2C bus. This binding describes an I2C bus that is
+accessed in such a fashion.
+
+The BPMP I2C node must be located directly inside the main BPMP node. See
+../firmware/nvidia,tegra186-bpmp.txt for details of the BPMP binding.
+
+This node represents an I2C controller. See ../i2c/i2c.txt for details of the
+core I2C binding.
+
+Required properties:
+- compatible:
+    Array of strings.
+    One of:
+    - "nvidia,tegra186-bpmp-i2c".
+- #address-cells: Address cells for I2C device address.
+    Single-cell integer.
+    Must be <1>.
+- #size-cells:
+    Single-cell integer.
+    Must be <0>.
+- nvidia,bpmp-bus-id:
+    Single-cell integer.
+    Indicates the I2C bus number this DT node represent, as defined by the
+    BPMP firmware.
+
+Example:
+
+bpmp {
+	...
+
+	i2c {
+		compatible = "nvidia,tegra186-bpmp-i2c";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		nvidia,bpmp-bus-id = <5>;
+	};
+};
-- 
2.10.2