* [RFC 0/2] misc: bt: added Block Transfer over I2C
@ 2016-09-08 21:28 Brendan Higgins
2016-09-08 21:28 ` [RFC 1/2] misc: bt: added IPMI Block Transfer over I2C master and slave Brendan Higgins
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Brendan Higgins @ 2016-09-08 21:28 UTC (permalink / raw)
To: openbmc
Looking for feedback on my new bt-i2c (Block Transfer over I2C) interface.
This is not a standard IPMI hardware interface. The IPMI specified hardware
interface for I2C/SMBus is SSIF (SMBus System Interface); however, SSIF is not a
very good solution for what we are trying to do:
First off, SSIF does not support sequence numbers, which makes multiple
in-flight messages difficult to implement (the IPMI spec explicitly leaves out
"multithreaded support" for SSIF, but does say it can be supported by an
implementor); this is a major issue for us as we would like to support multiple
in-flight messages. It also looks like OpenBMC makes use of the sequence number
in several places; on OpenBMC, it may be possible to address this by adding in a
sequence field to incoming SSIF messages that is always zero, but this would
not address the problem of multiple in-flight messages.
The second main limitation to SSIF is that it requires the slave side (BMC) to
NACK until it is ready to either send or receive data (technically this is not a
hard requirement, but the spec does say the BMC must NACK until ready to provide
a response unless SMBus Alert is available/enabled; this is effectively a
requirement because SSIF allows SMBus Alert to be turned off and is to be turned
off by default). However, the Aspeed 24XX and Aspeed 25XX I2C controllers do not
support arbitrary NACKing in slave mode; the only way to NACK is to disable
slave mode, which would also prevent the slave from responding to incoming
messages when it is unable to provide an outgoing message.
Although we could modify SSIF to suit our needs, some of the desired
modifications would be substantial (such as adding in a sequence field; it would
completely change the framing of the message); so I decided that SSIF was
probably not the best route to go for IPMI over I2C.
Here I introduce the bt-i2c (Block Transfer over I2C) interface; it is designed
such that it can be supported by any I2C slave device (using the standard Linux
I2C core slave mode framework), but I designed it such that it can make use of
SMBus Alert to notify the master (CPU) of available responses (not in this patch
set because it requires a change to I2C core; I was planning on using this
driver as the initial user). I wrote a precise specification of bt-i2c in the
second patch.
I based the slave side (BMC) driver on the bt-host driver; the file system
interface is fully compatible and has been tested with btbridged and ipmid
running on an ast2500 evb.
Work still to do:
- Add support for SMBus Alert (currently in progrees).
- Support O_NONBLOCK on the master side; I was planning on having this depend
on SMBus Alert support since polling is required if it is not available.
- Because bt-host and bt-i2c-slave share an identical file system interface; I
was thinking that we could pull the file system interface out so that they
share a single one.
Cheers!
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC 1/2] misc: bt: added IPMI Block Transfer over I2C master and slave
2016-09-08 21:28 [RFC 0/2] misc: bt: added Block Transfer over I2C Brendan Higgins
@ 2016-09-08 21:28 ` Brendan Higgins
2016-09-09 5:51 ` Joel Stanley
2016-09-08 21:28 ` [RFC 2/2] misc: bt: added documentation for bt-i2c drivers Brendan Higgins
2016-09-09 5:22 ` [RFC 0/2] misc: bt: added Block Transfer over I2C Joel Stanley
2 siblings, 1 reply; 8+ messages in thread
From: Brendan Higgins @ 2016-09-08 21:28 UTC (permalink / raw)
To: openbmc; +Cc: Brendan Higgins
The IPMI definition of the Block Transfer protocol defines the hardware
registers and behavior in addition to the message format and messaging
semantics. This implements a new protocol that uses IPMI Block Transfer
messages and semantics on top of a standard I2C interface. This protocol
has the same BMC side file system interface as "bt-host".
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
drivers/misc/Kconfig | 23 ++++
drivers/misc/Makefile | 2 +
drivers/misc/bt-i2c-master.c | 199 +++++++++++++++++++++++++++
drivers/misc/bt-i2c-slave.c | 317 +++++++++++++++++++++++++++++++++++++++++++
drivers/misc/bt.h | 19 +++
5 files changed, 560 insertions(+)
create mode 100644 drivers/misc/bt-i2c-master.c
create mode 100644 drivers/misc/bt-i2c-slave.c
create mode 100644 drivers/misc/bt.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 4617ddc..1c7e3a9 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -809,6 +809,29 @@ config ASPEED_BT_IPMI_HOST
help
Support for the Aspeed BT ipmi host.
+config BT_I2C_SLAVE
+ depends on !ASPEED_BT_IPMI_HOST && I2C_SLAVE
+ tristate "BT I2C IPMI slave driver"
+ default "n"
+ ---help---
+ Block Transfer over I2C defines a new IPMI compatible interface that
+ uses Block Transfer messages and semantics on top of plain old I2C.
+
+ This adds support for the slave side of the protocol (only really
+ useful for BMCs (Baseboard Management Controllers)).
+
+config BT_I2C_MASTER
+ depends on I2C
+ tristate "BT I2C IPMI master driver"
+ default "n"
+ ---help---
+ Block Transfer over I2C defines a new IPMI compatible interface that
+ uses Block Transfer messages and semantics on top of plain old I2C.
+
+ This adds support for master side of the protocol (this is most likely
+ what you want if you are compiling for a regular CPU, only useful if
+ your computer has a BMC that speaks the slave side of this protocol).
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 724861b..b642ed8 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -58,3 +58,5 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
obj-$(CONFIG_CXL_BASE) += cxl/
obj-$(CONFIG_PANEL) += panel.o
obj-$(CONFIG_ASPEED_BT_IPMI_HOST) += bt-host.o
+obj-$(CONFIG_BT_I2C_SLAVE) += bt-i2c-slave.o
+obj-$(CONFIG_BT_I2C_MASTER) += bt-i2c-master.o
diff --git a/drivers/misc/bt-i2c-master.c b/drivers/misc/bt-i2c-master.c
new file mode 100644
index 0000000..22142d6
--- /dev/null
+++ b/drivers/misc/bt-i2c-master.c
@@ -0,0 +1,199 @@
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+
+#include "bt.h"
+
+#define DEVICE_NAME "bt-master"
+
+struct bt_i2c_master {
+ struct i2c_client *client;
+ struct miscdevice miscdev;
+ struct mutex file_mutex;
+};
+
+static const unsigned long write_timeout = 25;
+
+static int send_bt_request(struct bt_i2c_master *bt_master,
+ struct bt_msg *bt_request)
+{
+ struct i2c_client *client = bt_master->client;
+ u8 *buf = (u8 *) bt_request;
+ unsigned long timeout, read_time;
+ int ret;
+
+ timeout = jiffies + msecs_to_jiffies(write_timeout);
+ do {
+ read_time = jiffies;
+ ret = i2c_master_send(client, buf, bt_msg_len(bt_request));
+ if (ret >= 0)
+ return 0;
+ usleep_range(1000, 1500);
+ } while (time_before(read_time, timeout));
+ return ret;
+}
+
+static int receive_bt_response(struct bt_i2c_master *bt_master,
+ struct bt_msg *bt_response)
+{
+ struct i2c_client *client = bt_master->client;
+ u8 *buf = (u8 *) bt_response;
+ u8 len = 0;
+ unsigned long timeout, read_time;
+ int ret;
+
+ /*
+ * Slave may not NACK when not ready, so we peek at the first byte to
+ * see if it is a valid length.
+ */
+ while (len == 0) {
+ i2c_master_recv(client, &len, 1);
+ if (len != 0)
+ break;
+ usleep_range(1000, 1500);
+
+ /* Signal received: quit syscall. */
+ if (signal_pending(current))
+ return -ERESTARTSYS;
+ }
+
+ timeout = jiffies + msecs_to_jiffies(write_timeout);
+ do {
+ read_time = jiffies;
+ ret = i2c_master_recv(client, buf, len + 1);
+ if (ret >= 0)
+ return 0;
+ usleep_range(1000, 1500);
+ } while (time_before(read_time, timeout));
+ return ret;
+}
+
+static inline struct bt_i2c_master *to_bt_i2c_master(struct file *file)
+{
+ return container_of(file->private_data, struct bt_i2c_master, miscdev);
+}
+
+static ssize_t bt_read(struct file *file, char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ struct bt_i2c_master *bt_master = to_bt_i2c_master(file);
+ struct bt_msg msg;
+ int ret;
+
+ mutex_lock(&bt_master->file_mutex);
+ ret = receive_bt_response(bt_master, &msg);
+ if (ret < 0)
+ goto out;
+ count = min(count, bt_msg_len(&msg));
+ if (copy_to_user(buf, &msg, count)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+out:
+ mutex_unlock(&bt_master->file_mutex);
+ if (ret < 0)
+ return ret;
+ else
+ return count;
+}
+
+static ssize_t bt_write(struct file *file, const char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ struct bt_i2c_master *bt_master = to_bt_i2c_master(file);
+ struct bt_msg msg;
+ int ret;
+
+ mutex_lock(&bt_master->file_mutex);
+ if (count > sizeof(struct bt_msg)) {
+ ret = -EFAULT;
+ goto out;
+ }
+ if (copy_from_user(&msg, buf, count) || count < bt_msg_len(&msg)) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = send_bt_request(bt_master, &msg);
+ if (ret < 0)
+ goto out;
+
+out:
+ mutex_unlock(&bt_master->file_mutex);
+ if (ret < 0)
+ return ret;
+ else
+ return count;
+}
+
+static const struct file_operations bt_fops = {
+ .owner = THIS_MODULE,
+ .read = bt_read,
+ .write = bt_write,
+};
+
+static int bt_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct bt_i2c_master *bt_master;
+ int ret;
+
+ bt_master = devm_kzalloc(&client->dev, sizeof(struct bt_i2c_master),
+ GFP_KERNEL);
+ if (!bt_master)
+ return -ENOMEM;
+
+ mutex_init(&bt_master->file_mutex);
+ bt_master->client = client;
+ i2c_set_clientdata(client, bt_master);
+
+ bt_master->miscdev.minor = MISC_DYNAMIC_MINOR;
+ bt_master->miscdev.name = DEVICE_NAME;
+ bt_master->miscdev.fops = &bt_fops;
+ bt_master->miscdev.parent = &client->dev;
+ ret = misc_register(&bt_master->miscdev);
+ if (ret < 0)
+ return ret;
+ return 0;
+}
+
+static int bt_i2c_remove(struct i2c_client *client)
+{
+ struct bt_i2c_master *bt_master = i2c_get_clientdata(client);
+
+ misc_deregister(&bt_master->miscdev);
+ return 0;
+}
+
+static const struct i2c_device_id bt_i2c_id[] = {
+ {"bt-i2c-master", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, bt_i2c_id);
+
+static struct i2c_driver bt_i2c_driver = {
+ .driver = {
+ .name = "bt-i2c-master",
+ },
+ .probe = bt_i2c_probe,
+ .remove = bt_i2c_remove,
+ .id_table = bt_i2c_id,
+};
+module_i2c_driver(bt_i2c_driver);
+
+MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
+MODULE_DESCRIPTION("IPMI Block Transfer over I2C master.");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/misc/bt-i2c-slave.c b/drivers/misc/bt-i2c-slave.c
new file mode 100644
index 0000000..1a90e05
--- /dev/null
+++ b/drivers/misc/bt-i2c-slave.c
@@ -0,0 +1,317 @@
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+
+#include "bt.h"
+
+/*
+ * TODO: This is "bt-host" to match the bt-host driver; however, I think this is
+ * unclear in the context of a CPU side driver. Should probably name this
+ * and the DEVICE_NAME in bt-host to something like "bt-bmc" or "bt-slave".
+ */
+#define DEVICE_NAME "bt-host"
+
+static const unsigned long request_queue_max_len = 256;
+
+struct bt_request_elem {
+ struct list_head list;
+ struct bt_msg request;
+};
+
+struct bt_i2c_slave {
+ struct i2c_client *client;
+ struct miscdevice miscdev;
+ struct bt_msg request;
+ struct list_head request_queue;
+ atomic_t request_queue_len;
+ struct bt_msg response;
+ atomic_t response_in_progress;
+ size_t msg_idx;
+ spinlock_t lock;
+ wait_queue_head_t wait_queue;
+ struct mutex file_mutex;
+};
+
+static int receive_bt_request(struct bt_i2c_slave *bt_slave, bool non_blocking,
+ struct bt_msg *bt_request) {
+ unsigned long flags;
+ struct bt_request_elem *queue_elem;
+
+ if (!atomic_read(&bt_slave->request_queue_len) && non_blocking)
+ return -EAGAIN;
+ else if (!non_blocking)
+ wait_event_interruptible(
+ bt_slave->wait_queue,
+ atomic_read(&bt_slave->request_queue_len));
+
+ spin_lock_irqsave(&bt_slave->lock, flags);
+ queue_elem = list_first_entry(&bt_slave->request_queue,
+ struct bt_request_elem, list);
+ memcpy(bt_request, &queue_elem->request, sizeof(struct bt_msg));
+ list_del(&queue_elem->list);
+ kfree(queue_elem);
+ atomic_dec(&bt_slave->request_queue_len);
+ spin_unlock_irqrestore(&bt_slave->lock, flags);
+ return 0;
+}
+
+static int send_bt_response(struct bt_i2c_slave *bt_slave, bool non_blocking,
+ struct bt_msg *bt_response) {
+ unsigned long flags;
+
+ if (non_blocking && atomic_read(&bt_slave->response_in_progress))
+ return -EAGAIN;
+ else if (!non_blocking)
+ wait_event_interruptible(
+ bt_slave->wait_queue,
+ !atomic_read(&bt_slave->response_in_progress));
+
+ spin_lock_irqsave(&bt_slave->lock, flags);
+ memcpy(&bt_slave->response, bt_response, sizeof(struct bt_msg));
+ atomic_set(&bt_slave->response_in_progress, 0);
+ spin_unlock_irqrestore(&bt_slave->lock, flags);
+ return 0;
+}
+
+static inline struct bt_i2c_slave *to_bt_i2c_slave(struct file *file)
+{
+ return container_of(file->private_data, struct bt_i2c_slave, miscdev);
+}
+
+static ssize_t bt_read(struct file *file, char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
+ struct bt_msg msg;
+ ssize_t ret;
+
+ mutex_lock(&bt_slave->file_mutex);
+ ret = receive_bt_request(bt_slave, file->f_flags & O_NONBLOCK, &msg);
+ if (ret < 0)
+ goto out;
+ count = min(count, bt_msg_len(&msg));
+ if (copy_to_user(buf, &msg, count)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+out:
+ mutex_unlock(&bt_slave->file_mutex);
+ if (ret < 0)
+ return ret;
+ else
+ return count;
+}
+
+static ssize_t bt_write(struct file *file, const char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
+ struct bt_msg msg;
+ ssize_t ret;
+
+ if (count > sizeof(struct bt_msg))
+ return -EFAULT;
+
+ if (copy_from_user(&msg, buf, count) || count < bt_msg_len(&msg))
+ return -EFAULT;
+
+ mutex_lock(&bt_slave->file_mutex);
+ ret = send_bt_response(bt_slave, file->f_flags & O_NONBLOCK, &msg);
+ mutex_unlock(&bt_slave->file_mutex);
+
+ if (ret < 0)
+ return ret;
+ else
+ return count;
+}
+
+static long bt_ioctl(struct file *file, unsigned int cmd, unsigned long param)
+{
+ return 0;
+}
+
+static unsigned int bt_poll(struct file *file, poll_table *wait)
+{
+ struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
+ unsigned int mask = 0;
+
+ mutex_lock(&bt_slave->file_mutex);
+ poll_wait(file, &bt_slave->wait_queue, wait);
+
+ if (atomic_read(&bt_slave->request_queue_len))
+ mask |= POLLIN;
+ if (!atomic_read(&bt_slave->response_in_progress))
+ mask |= POLLOUT;
+ mutex_unlock(&bt_slave->file_mutex);
+ return mask;
+}
+
+static const struct file_operations bt_fops = {
+ .owner = THIS_MODULE,
+ .read = bt_read,
+ .write = bt_write,
+ .poll = bt_poll,
+ .unlocked_ioctl = bt_ioctl,
+};
+
+static int handle_request(struct bt_i2c_slave *bt_slave)
+{
+ struct bt_request_elem *queue_elem;
+
+ if (atomic_read(&bt_slave->request_queue_len) >= request_queue_max_len)
+ return -EFAULT;
+ queue_elem = kmalloc(sizeof(struct bt_request_elem), GFP_KERNEL);
+ memcpy(&queue_elem->request, &bt_slave->request, sizeof(struct bt_msg));
+ list_add(&queue_elem->list, &bt_slave->request_queue);
+ atomic_inc(&bt_slave->request_queue_len);
+ wake_up_all(&bt_slave->wait_queue);
+ return 0;
+}
+
+static int complete_response(struct bt_i2c_slave *bt_slave)
+{
+ /* Invalidate response in buffer to denote it having been sent. */
+ bt_slave->response.len = 0;
+ atomic_set(&bt_slave->response_in_progress, 1);
+ wake_up_all(&bt_slave->wait_queue);
+ return 0;
+}
+
+static int bt_i2c_slave_cb(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ struct bt_i2c_slave *bt_slave = i2c_get_clientdata(client);
+ u8 *buf;
+
+ spin_lock(&bt_slave->lock);
+ switch (event) {
+ case I2C_SLAVE_WRITE_REQUESTED:
+ bt_slave->msg_idx = 0;
+ break;
+
+ case I2C_SLAVE_WRITE_RECEIVED:
+ buf = (u8 *) &bt_slave->request;
+ if (bt_slave->msg_idx >= sizeof(struct bt_i2c_slave))
+ break;
+
+ buf[bt_slave->msg_idx++] = *val;
+ if (bt_slave->msg_idx >= bt_msg_len(&bt_slave->request))
+ handle_request(bt_slave);
+ break;
+
+ case I2C_SLAVE_READ_REQUESTED:
+ buf = (u8 *) &bt_slave->response;
+ bt_slave->msg_idx = 0;
+ *val = buf[bt_slave->msg_idx];
+ /*
+ * Do not increment buffer_idx here, because we don't know if
+ * this byte will be actually used. Read Linux I2C slave docs
+ * for details.
+ */
+ break;
+
+ case I2C_SLAVE_READ_PROCESSED:
+ buf = (u8 *) &bt_slave->response;
+ if (bt_slave->response.len &&
+ bt_slave->msg_idx < bt_msg_len(&bt_slave->response)) {
+ *val = buf[++bt_slave->msg_idx];
+ } else {
+ *val = 0;
+ }
+ if (bt_slave->msg_idx + 1 >= bt_msg_len(&bt_slave->response))
+ complete_response(bt_slave);
+ break;
+
+ case I2C_SLAVE_STOP:
+ bt_slave->msg_idx = 0;
+ break;
+
+ default:
+ break;
+ }
+ spin_unlock(&bt_slave->lock);
+
+ return 0;
+}
+
+static int bt_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct bt_i2c_slave *bt_slave;
+ int ret;
+
+ bt_slave = devm_kzalloc(&client->dev, sizeof(struct bt_i2c_slave),
+ GFP_KERNEL);
+ if (!bt_slave)
+ return -ENOMEM;
+
+ spin_lock_init(&bt_slave->lock);
+ init_waitqueue_head(&bt_slave->wait_queue);
+ atomic_set(&bt_slave->request_queue_len, 0);
+ atomic_set(&bt_slave->response_in_progress, 0);
+ INIT_LIST_HEAD(&bt_slave->request_queue);
+
+ mutex_init(&bt_slave->file_mutex);
+
+ bt_slave->miscdev.minor = MISC_DYNAMIC_MINOR;
+ bt_slave->miscdev.name = DEVICE_NAME;
+ bt_slave->miscdev.fops = &bt_fops;
+ bt_slave->miscdev.parent = &client->dev;
+ ret = misc_register(&bt_slave->miscdev);
+ if (ret < 0)
+ return ret;
+
+ bt_slave->client = client;
+ i2c_set_clientdata(client, bt_slave);
+ ret = i2c_slave_register(client, bt_i2c_slave_cb);
+ if (ret < 0) {
+ misc_deregister(&bt_slave->miscdev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int bt_i2c_remove(struct i2c_client *client)
+{
+ struct bt_i2c_slave *bt_slave = i2c_get_clientdata(client);
+
+ i2c_slave_unregister(client);
+ misc_deregister(&bt_slave->miscdev);
+ return 0;
+}
+
+static const struct i2c_device_id bt_i2c_id[] = {
+ {"bt-i2c-slave", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, bt_i2c_id);
+
+static struct i2c_driver bt_i2c_driver = {
+ .driver = {
+ .name = "bt-i2c-slave",
+ },
+ .probe = bt_i2c_probe,
+ .remove = bt_i2c_remove,
+ .id_table = bt_i2c_id,
+};
+module_i2c_driver(bt_i2c_driver);
+
+MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
+MODULE_DESCRIPTION("IPMI Block Transfer over I2C slave.");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/misc/bt.h b/drivers/misc/bt.h
new file mode 100644
index 0000000..c0c2881
--- /dev/null
+++ b/drivers/misc/bt.h
@@ -0,0 +1,19 @@
+#ifndef _DRIVERS_MISC_BT_H
+#define _DRIVERS_MISC_BT_H
+
+#include <linux/types.h>
+
+struct bt_msg {
+ u8 len;
+ u8 netfn_lun;
+ u8 seq;
+ u8 cmd;
+ u8 payload[252];
+};
+
+static inline u32 bt_msg_len(struct bt_msg *bt_request)
+{
+ return bt_request->len + 1;
+}
+
+#endif /* _DRIVERS_MISC_BT_H */
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RFC 2/2] misc: bt: added documentation for bt-i2c drivers
2016-09-08 21:28 [RFC 0/2] misc: bt: added Block Transfer over I2C Brendan Higgins
2016-09-08 21:28 ` [RFC 1/2] misc: bt: added IPMI Block Transfer over I2C master and slave Brendan Higgins
@ 2016-09-08 21:28 ` Brendan Higgins
2016-09-09 5:22 ` [RFC 0/2] misc: bt: added Block Transfer over I2C Joel Stanley
2 siblings, 0 replies; 8+ messages in thread
From: Brendan Higgins @ 2016-09-08 21:28 UTC (permalink / raw)
To: openbmc; +Cc: Brendan Higgins
Added device tree binding documentation for bt-i2c-master and
bt-i2c-slave and documentation for the Block Transfer over I2C (bt-i2c)
protocol.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
.../devicetree/bindings/misc/bt-i2c-master.txt | 22 ++++
.../devicetree/bindings/misc/bt-i2c-slave.txt | 22 ++++
Documentation/misc-devices/bt-i2c.txt | 123 +++++++++++++++++++++
3 files changed, 167 insertions(+)
create mode 100644 Documentation/devicetree/bindings/misc/bt-i2c-master.txt
create mode 100644 Documentation/devicetree/bindings/misc/bt-i2c-slave.txt
create mode 100644 Documentation/misc-devices/bt-i2c.txt
diff --git a/Documentation/devicetree/bindings/misc/bt-i2c-master.txt b/Documentation/devicetree/bindings/misc/bt-i2c-master.txt
new file mode 100644
index 0000000..d418fe2
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/bt-i2c-master.txt
@@ -0,0 +1,22 @@
+Device tree configuration for the bt-i2c-master driver.
+
+Required Properties:
+- compatible : should be "bt-i2c-master"
+- reg : the I2C slave address of the bt-i2c-slave
+
+i2c0: i2c-bus@40 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x40 0x40>;
+ compatible = "aspeed,ast2400-i2c-bus";
+ bus = <0>;
+ clock-frequency = <100000>;
+ status = "disabled";
+ interrupts = <0>;
+ interrupt-parent = <&i2c>;
+
+ bt-i2c-master@41 {
+ compatible = "bt-i2c-master";
+ reg = <0x41>;
+ };
+};
diff --git a/Documentation/devicetree/bindings/misc/bt-i2c-slave.txt b/Documentation/devicetree/bindings/misc/bt-i2c-slave.txt
new file mode 100644
index 0000000..f4e67ae
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/bt-i2c-slave.txt
@@ -0,0 +1,22 @@
+Device tree configuration for the bt-i2c-slave driver.
+
+Required Properties:
+- compatible : should be "bt-i2c-slave"
+- reg : the I2C slave address of this driver.
+
+i2c0: i2c-bus@40 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x40 0x40>;
+ compatible = "aspeed,ast2400-i2c-bus";
+ bus = <0>;
+ clock-frequency = <100000>;
+ status = "disabled";
+ interrupts = <0>;
+ interrupt-parent = <&i2c>;
+
+ bt-i2c-slave@41 {
+ compatible = "bt-i2c-slave";
+ reg = <0x41>;
+ };
+};
diff --git a/Documentation/misc-devices/bt-i2c.txt b/Documentation/misc-devices/bt-i2c.txt
new file mode 100644
index 0000000..15c6ee5
--- /dev/null
+++ b/Documentation/misc-devices/bt-i2c.txt
@@ -0,0 +1,123 @@
+Linux Block Transfer over I2C (bt-i2c) interface description
+============================================================
+
+by Brendan Higgins <brendanhiggins@google.com> in 2016
+
+Introduction
+------------
+
+IPMI defines an interface for communication between a CPU, a BMC (Baseboard
+Management Controller), and sensors and various other peripherals. For a more
+complete description of IPMI please see:
+http://www.intel.com/content/www/us/en/servers/ipmi/ipmi-second-gen-interface-spec-v2-rev1-1.html
+
+IPMI defines a *common* message format, as in a set of fields that are common
+across all IPMI messages; they could be viewed as part of the framing
+information for an IPMI message. They include:
+
+ - netfn
+ - lun
+ - cmd
+
+netfn and cmd together define the type of the message; netfn can be viewed as a
+message class and cmd is a subtype of sorts. lun (logical unit number) is used
+for routing between messages between different interfaces. After the last field
+there is usually a variable length payload. Despite these common fields, the
+remainder of the framing varies widely between the IPMI defined hardware
+interfaces; some specify a length as part of the framing which is never more
+than a byte in length; others use a special signal to denote the end of message.
+Some IPMI hardware interfaces, the Block Transfer interface in particular,
+support a sequence number that aids in support of multiple in-flight IPMI
+messages.
+
+IPMI defines SSIF (SMBus System Interface) as the IPMI hardware interface for
+SMBus/I2C. It supports a maximum total message length of 255 bytes that is
+broken up across several SMBus block operations. It does not define a sequence
+field in the IPMI framing making it very difficult to support multiple in flight
+messages (it is also intentionally left out of the specification). SSIF also
+requires the slave device to NACK until it is ready to accept data (technically
+it only specifies that it may NACK until it is ready, but must NACK on attempted
+reads if it does not support SMBus Alert; however, this is an effective
+requirements since a slave device is supposed to start with SMBus Alert
+disabled); this again makes SSIF very difficult to support for some slave
+devices which may not support NACKing arbitrary messages; indeed, at the time of
+writing, the Linux I2C slave driver framework did not have support for sending
+NACKs.
+
+Block Transfer over I2C defines a new IPMI compatible interface that uses Block
+Transfer messages and semantics on top of plain old I2C; it does not assume that
+the I2C slave is capable of NACKing arbitrary messages; however, it is designed
+such that it could take advantage of SMBus Alert so that the master does not
+have to poll (the Linux I2C core slave mode does not currently support SMBus
+Alert, but a patch adding this support is currently on the way).
+
+Protocol Definition
+-------------------
+
+Block Transfer over I2C uses the IPMI defined Block Transfer message format; it
+supports variable length messages with a maximum length of 255 bytes (limited by
+the IPMI Block Transfer length byte).
+
+A Block Transfer over I2C Request is structured as follows:
+
+------------------------------------------------------------------------------------------------------
+| I2C start | slave address / RW bit unset | Block Transfer message | ... (another message or stop ) |
+------------------------------------------------------------------------------------------------------
+
+Multiple requests can be sent before any responses are received. Sequence
+numbers are to be handled by the users of the drivers; thus, no semantics are
+prescribed to their usage; however, the slave driver is required to buffer at
+least 256 requests before dropping requests; this can be used in conjunction
+with sequence numbers to prevent messages from being dropped by the slave.
+
+A Block Transfer over I2C Response is structured as follows:
+
+----------------------------------------------------------------------------------------------------
+| I2C start | slave address / RW bit set | Block Transfer message | ... (another message or stop ) |
+----------------------------------------------------------------------------------------------------
+
+Until a response is ready to send, the slave will respond only with zero bytes.
+If the slave receives a start or a stop before it was done sending a response,
+it will resend the entire response the next time a read is requested; in this
+way, an I2C master may poll for responses by reading a single byte until it is
+non-zero and then perform the read transaction shown above.
+
+In the future, it is planned that the slave will support using SMBus Alert to
+notify the master of an available response, but will never be required.
+
+Driver Interface
+----------------
+
+The slave side of the device file interface is modeled after bt-host, a driver
+for the Aspeed 24xx/255xx IPMI Block Transfer controller.
+
+A read (request) should be large enough to fit a Block Transfer message
+(including the length byte) of 256 bytes; if the provided buffer is smaller, the
+message will be truncated.
+
+A write (response) must be no greater than the maximum valid length of a Block
+Transfer message (including the length byte), 256 bytes; if the provided buffer
+is larger, the write will fail with EFAULT. The driver also enforces a valid
+length byte which must contain the total length of the message not including the
+length byte; thus, the write will fail with EFAULT if the buffer length is less
+than the length field plus one. The driver will also only send length field plus
+one bytes.
+
+The slave device file interface also supports the poll system call; it will
+report when a request is available to read or it is ready to accept a response.
+
+The master side of the device file interface is pretty similar to the slave
+side, accept that it does not currently support poll (to be implemented along
+with SMBus Alert support).
+
+A read (response) should be large enough to fit a Block Transfer message
+(including the length byte) of 256 bytes; if the provided buffer is smaller, the
+message will be truncated.
+
+A write (request) must be no greater than the maximum valid length of a Block
+Transfer message (including the length byte), 256 bytes; if the provided buffer
+is larger, the write will fail with EFAULT. The driver also enforces a valid
+length byte which must contain the total length of the message not including the
+length byte; thus, the write will fail with EFAULT if the buffer length is less
+than the length field plus one. The driver will also only send length field plus
+one bytes.
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC 0/2] misc: bt: added Block Transfer over I2C
2016-09-08 21:28 [RFC 0/2] misc: bt: added Block Transfer over I2C Brendan Higgins
2016-09-08 21:28 ` [RFC 1/2] misc: bt: added IPMI Block Transfer over I2C master and slave Brendan Higgins
2016-09-08 21:28 ` [RFC 2/2] misc: bt: added documentation for bt-i2c drivers Brendan Higgins
@ 2016-09-09 5:22 ` Joel Stanley
2016-09-09 16:30 ` Cédric Le Goater
2 siblings, 1 reply; 8+ messages in thread
From: Joel Stanley @ 2016-09-09 5:22 UTC (permalink / raw)
To: Brendan Higgins; +Cc: OpenBMC Maillist, Cédric Le Goater
Hey Brendan,
On Fri, Sep 9, 2016 at 6:58 AM, Brendan Higgins
<brendanhiggins@google.com> wrote:
> Looking for feedback on my new bt-i2c (Block Transfer over I2C) interface.
Thanks for sending this out. You're setting a great example for the rest of us!
I don't have an specific feedback for you, other than to say I think
what you are doing is reasonable.
> I based the slave side (BMC) driver on the bt-host driver; the file system
> interface is fully compatible and has been tested with btbridged and ipmid
> running on an ast2500 evb.
This is timely. Cedric has recently sent out the bt-host patches[1].
One of the questions raised was the suitability of the userspace API
for other users. It would be useful for you to put forward your
thoughts on that discussion.
[1] http://www.spinics.net/lists/arm-kernel/msg527834.html
Cheers,
Joel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 1/2] misc: bt: added IPMI Block Transfer over I2C master and slave
2016-09-08 21:28 ` [RFC 1/2] misc: bt: added IPMI Block Transfer over I2C master and slave Brendan Higgins
@ 2016-09-09 5:51 ` Joel Stanley
2016-09-09 21:37 ` Brendan Higgins
0 siblings, 1 reply; 8+ messages in thread
From: Joel Stanley @ 2016-09-09 5:51 UTC (permalink / raw)
To: Brendan Higgins; +Cc: OpenBMC Maillist
On Fri, Sep 9, 2016 at 6:58 AM, Brendan Higgins
<brendanhiggins@google.com> wrote:
> The IPMI definition of the Block Transfer protocol defines the hardware
> registers and behavior in addition to the message format and messaging
> semantics. This implements a new protocol that uses IPMI Block Transfer
> messages and semantics on top of a standard I2C interface. This protocol
> has the same BMC side file system interface as "bt-host".
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> drivers/misc/Kconfig | 23 ++++
> drivers/misc/Makefile | 2 +
> drivers/misc/bt-i2c-master.c | 199 +++++++++++++++++++++++++++
> drivers/misc/bt-i2c-slave.c | 317 +++++++++++++++++++++++++++++++++++++++++++
> drivers/misc/bt.h | 19 +++
> 5 files changed, 560 insertions(+)
> create mode 100644 drivers/misc/bt-i2c-master.c
> create mode 100644 drivers/misc/bt-i2c-slave.c
> create mode 100644 drivers/misc/bt.h
>
There is talk of putting the bt-host driver into drivers/char/ipmi. I
imagine that will affect your work.
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 4617ddc..1c7e3a9 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -809,6 +809,29 @@ config ASPEED_BT_IPMI_HOST
> help
> Support for the Aspeed BT ipmi host.
>
> +config BT_I2C_SLAVE
> + depends on !ASPEED_BT_IPMI_HOST && I2C_SLAVE
> + tristate "BT I2C IPMI slave driver"
> + default "n"
> + ---help---
> + Block Transfer over I2C defines a new IPMI compatible interface that
> + uses Block Transfer messages and semantics on top of plain old I2C.
> +
> + This adds support for the slave side of the protocol (only really
> + useful for BMCs (Baseboard Management Controllers)).
> +
> +config BT_I2C_MASTER
> + depends on I2C
> + tristate "BT I2C IPMI master driver"
> + default "n"
> + ---help---
> + Block Transfer over I2C defines a new IPMI compatible interface that
> + uses Block Transfer messages and semantics on top of plain old I2C.
> +
> + This adds support for master side of the protocol (this is most likely
> + what you want if you are compiling for a regular CPU, only useful if
> + your computer has a BMC that speaks the slave side of this protocol).
I didn't pick up on this when reading your cover letter. The host side
will read the messages straight into the kernel and userspace, not
pass them through firmware?
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 724861b..b642ed8 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -58,3 +58,5 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
> obj-$(CONFIG_CXL_BASE) += cxl/
> obj-$(CONFIG_PANEL) += panel.o
> obj-$(CONFIG_ASPEED_BT_IPMI_HOST) += bt-host.o
> +obj-$(CONFIG_BT_I2C_SLAVE) += bt-i2c-slave.o
> +obj-$(CONFIG_BT_I2C_MASTER) += bt-i2c-master.o
> diff --git a/drivers/misc/bt-i2c-master.c b/drivers/misc/bt-i2c-master.c
> new file mode 100644
> index 0000000..22142d6
> --- /dev/null
> +++ b/drivers/misc/bt-i2c-master.c
> @@ -0,0 +1,199 @@
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/errno.h>
> +#include <linux/poll.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/miscdevice.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +
> +#include "bt.h"
> +
> +#define DEVICE_NAME "bt-master"
> +
> +struct bt_i2c_master {
> + struct i2c_client *client;
> + struct miscdevice miscdev;
> + struct mutex file_mutex;
> +};
> +
> +static const unsigned long write_timeout = 25;
> +
> +static int send_bt_request(struct bt_i2c_master *bt_master,
> + struct bt_msg *bt_request)
> +{
> + struct i2c_client *client = bt_master->client;
> + u8 *buf = (u8 *) bt_request;
> + unsigned long timeout, read_time;
> + int ret;
> +
> + timeout = jiffies + msecs_to_jiffies(write_timeout);
> + do {
> + read_time = jiffies;
> + ret = i2c_master_send(client, buf, bt_msg_len(bt_request));
> + if (ret >= 0)
> + return 0;
> + usleep_range(1000, 1500);
> + } while (time_before(read_time, timeout));
It feels like there should be a neater way of doing this read/write
and wait operation that the driver does. I don't have a specific
recommendation though.
> + return ret;
> +}
> +
> +static int receive_bt_response(struct bt_i2c_master *bt_master,
> + struct bt_msg *bt_response)
> +{
> + struct i2c_client *client = bt_master->client;
> + u8 *buf = (u8 *) bt_response;
> + u8 len = 0;
> + unsigned long timeout, read_time;
> + int ret;
> +
> + /*
> + * Slave may not NACK when not ready, so we peek at the first byte to
> + * see if it is a valid length.
> + */
> + while (len == 0) {
> + i2c_master_recv(client, &len, 1);
> + if (len != 0)
> + break;
> + usleep_range(1000, 1500);
> +
> + /* Signal received: quit syscall. */
> + if (signal_pending(current))
> + return -ERESTARTSYS;
> + }
> +
> + timeout = jiffies + msecs_to_jiffies(write_timeout);
> + do {
> + read_time = jiffies;
> + ret = i2c_master_recv(client, buf, len + 1);
> + if (ret >= 0)
> + return 0;
> + usleep_range(1000, 1500);
> + } while (time_before(read_time, timeout));
> + return ret;
> +}
> +
> +static inline struct bt_i2c_master *to_bt_i2c_master(struct file *file)
> +{
> + return container_of(file->private_data, struct bt_i2c_master, miscdev);
> +}
> +
> +static ssize_t bt_read(struct file *file, char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + struct bt_i2c_master *bt_master = to_bt_i2c_master(file);
> + struct bt_msg msg;
> + int ret;
> +
> + mutex_lock(&bt_master->file_mutex);
> + ret = receive_bt_response(bt_master, &msg);
> + if (ret < 0)
> + goto out;
> + count = min(count, bt_msg_len(&msg));
> + if (copy_to_user(buf, &msg, count)) {
> + ret = -EFAULT;
> + goto out;
The goto doesn't do much :)
> + }
> +
> +out:
> + mutex_unlock(&bt_master->file_mutex);
> + if (ret < 0)
> + return ret;
> + else
> + return count;
> +}
> +
> +static ssize_t bt_write(struct file *file, const char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + struct bt_i2c_master *bt_master = to_bt_i2c_master(file);
> + struct bt_msg msg;
> + int ret;
> +
> + mutex_lock(&bt_master->file_mutex);
> + if (count > sizeof(struct bt_msg)) {
> + ret = -EFAULT;
> + goto out;
> + }
> + if (copy_from_user(&msg, buf, count) || count < bt_msg_len(&msg)) {
> + ret = -EFAULT;
> + goto out;
> + }
> + ret = send_bt_request(bt_master, &msg);
> + if (ret < 0)
> + goto out;
> +
> +out:
> + mutex_unlock(&bt_master->file_mutex);
> + if (ret < 0)
> + return ret;
> + else
> + return count;
> +}
> +
> +static const struct file_operations bt_fops = {
> + .owner = THIS_MODULE,
> + .read = bt_read,
> + .write = bt_write,
> +};
> +
> +static int bt_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct bt_i2c_master *bt_master;
> + int ret;
> +
> + bt_master = devm_kzalloc(&client->dev, sizeof(struct bt_i2c_master),
> + GFP_KERNEL);
> + if (!bt_master)
> + return -ENOMEM;
> +
> + mutex_init(&bt_master->file_mutex);
> + bt_master->client = client;
> + i2c_set_clientdata(client, bt_master);
> +
> + bt_master->miscdev.minor = MISC_DYNAMIC_MINOR;
> + bt_master->miscdev.name = DEVICE_NAME;
> + bt_master->miscdev.fops = &bt_fops;
> + bt_master->miscdev.parent = &client->dev;
> + ret = misc_register(&bt_master->miscdev);
> + if (ret < 0)
> + return ret;
> + return 0;
> +}
> +
> +static int bt_i2c_remove(struct i2c_client *client)
> +{
> + struct bt_i2c_master *bt_master = i2c_get_clientdata(client);
> +
> + misc_deregister(&bt_master->miscdev);
> + return 0;
> +}
> +
> +static const struct i2c_device_id bt_i2c_id[] = {
> + {"bt-i2c-master", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(i2c, bt_i2c_id);
> +
> +static struct i2c_driver bt_i2c_driver = {
> + .driver = {
> + .name = "bt-i2c-master",
> + },
> + .probe = bt_i2c_probe,
> + .remove = bt_i2c_remove,
> + .id_table = bt_i2c_id,
> +};
> +module_i2c_driver(bt_i2c_driver);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> +MODULE_DESCRIPTION("IPMI Block Transfer over I2C master.");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/misc/bt-i2c-slave.c b/drivers/misc/bt-i2c-slave.c
> new file mode 100644
> index 0000000..1a90e05
> --- /dev/null
> +++ b/drivers/misc/bt-i2c-slave.c
> @@ -0,0 +1,317 @@
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/errno.h>
> +#include <linux/poll.h>
> +#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/miscdevice.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +
> +#include "bt.h"
> +
> +/*
> + * TODO: This is "bt-host" to match the bt-host driver; however, I think this is
> + * unclear in the context of a CPU side driver. Should probably name this
> + * and the DEVICE_NAME in bt-host to something like "bt-bmc" or "bt-slave".
> + */
> +#define DEVICE_NAME "bt-host"
> +
> +static const unsigned long request_queue_max_len = 256;
> +
> +struct bt_request_elem {
> + struct list_head list;
> + struct bt_msg request;
> +};
> +
> +struct bt_i2c_slave {
> + struct i2c_client *client;
> + struct miscdevice miscdev;
> + struct bt_msg request;
> + struct list_head request_queue;
> + atomic_t request_queue_len;
> + struct bt_msg response;
> + atomic_t response_in_progress;
> + size_t msg_idx;
> + spinlock_t lock;
> + wait_queue_head_t wait_queue;
> + struct mutex file_mutex;
> +};
> +
> +static int receive_bt_request(struct bt_i2c_slave *bt_slave, bool non_blocking,
> + struct bt_msg *bt_request) {
> + unsigned long flags;
> + struct bt_request_elem *queue_elem;
> +
> + if (!atomic_read(&bt_slave->request_queue_len) && non_blocking)
> + return -EAGAIN;
> + else if (!non_blocking)
> + wait_event_interruptible(
> + bt_slave->wait_queue,
> + atomic_read(&bt_slave->request_queue_len));
> +
> + spin_lock_irqsave(&bt_slave->lock, flags);
> + queue_elem = list_first_entry(&bt_slave->request_queue,
> + struct bt_request_elem, list);
> + memcpy(bt_request, &queue_elem->request, sizeof(struct bt_msg));
> + list_del(&queue_elem->list);
> + kfree(queue_elem);
> + atomic_dec(&bt_slave->request_queue_len);
> + spin_unlock_irqrestore(&bt_slave->lock, flags);
> + return 0;
> +}
> +
> +static int send_bt_response(struct bt_i2c_slave *bt_slave, bool non_blocking,
> + struct bt_msg *bt_response) {
> + unsigned long flags;
> +
> + if (non_blocking && atomic_read(&bt_slave->response_in_progress))
> + return -EAGAIN;
> + else if (!non_blocking)
> + wait_event_interruptible(
> + bt_slave->wait_queue,
> + !atomic_read(&bt_slave->response_in_progress));
> +
> + spin_lock_irqsave(&bt_slave->lock, flags);
> + memcpy(&bt_slave->response, bt_response, sizeof(struct bt_msg));
> + atomic_set(&bt_slave->response_in_progress, 0);
> + spin_unlock_irqrestore(&bt_slave->lock, flags);
> + return 0;
> +}
> +
> +static inline struct bt_i2c_slave *to_bt_i2c_slave(struct file *file)
> +{
> + return container_of(file->private_data, struct bt_i2c_slave, miscdev);
> +}
> +
> +static ssize_t bt_read(struct file *file, char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
> + struct bt_msg msg;
> + ssize_t ret;
> +
> + mutex_lock(&bt_slave->file_mutex);
> + ret = receive_bt_request(bt_slave, file->f_flags & O_NONBLOCK, &msg);
> + if (ret < 0)
> + goto out;
> + count = min(count, bt_msg_len(&msg));
> + if (copy_to_user(buf, &msg, count)) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> +out:
> + mutex_unlock(&bt_slave->file_mutex);
> + if (ret < 0)
> + return ret;
> + else
> + return count;
> +}
> +
> +static ssize_t bt_write(struct file *file, const char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
> + struct bt_msg msg;
> + ssize_t ret;
> +
> + if (count > sizeof(struct bt_msg))
> + return -EFAULT;
> +
> + if (copy_from_user(&msg, buf, count) || count < bt_msg_len(&msg))
> + return -EFAULT;
> +
> + mutex_lock(&bt_slave->file_mutex);
> + ret = send_bt_response(bt_slave, file->f_flags & O_NONBLOCK, &msg);
> + mutex_unlock(&bt_slave->file_mutex);
> +
> + if (ret < 0)
> + return ret;
> + else
> + return count;
> +}
> +
> +static long bt_ioctl(struct file *file, unsigned int cmd, unsigned long param)
> +{
> + return 0;
> +}
> +
> +static unsigned int bt_poll(struct file *file, poll_table *wait)
> +{
> + struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
> + unsigned int mask = 0;
> +
> + mutex_lock(&bt_slave->file_mutex);
> + poll_wait(file, &bt_slave->wait_queue, wait);
> +
> + if (atomic_read(&bt_slave->request_queue_len))
> + mask |= POLLIN;
> + if (!atomic_read(&bt_slave->response_in_progress))
> + mask |= POLLOUT;
> + mutex_unlock(&bt_slave->file_mutex);
> + return mask;
> +}
> +
> +static const struct file_operations bt_fops = {
> + .owner = THIS_MODULE,
> + .read = bt_read,
> + .write = bt_write,
> + .poll = bt_poll,
> + .unlocked_ioctl = bt_ioctl,
> +};
> +
> +static int handle_request(struct bt_i2c_slave *bt_slave)
> +{
> + struct bt_request_elem *queue_elem;
> +
> + if (atomic_read(&bt_slave->request_queue_len) >= request_queue_max_len)
> + return -EFAULT;
> + queue_elem = kmalloc(sizeof(struct bt_request_elem), GFP_KERNEL);
> + memcpy(&queue_elem->request, &bt_slave->request, sizeof(struct bt_msg));
> + list_add(&queue_elem->list, &bt_slave->request_queue);
> + atomic_inc(&bt_slave->request_queue_len);
> + wake_up_all(&bt_slave->wait_queue);
> + return 0;
> +}
> +
> +static int complete_response(struct bt_i2c_slave *bt_slave)
> +{
> + /* Invalidate response in buffer to denote it having been sent. */
> + bt_slave->response.len = 0;
> + atomic_set(&bt_slave->response_in_progress, 1);
> + wake_up_all(&bt_slave->wait_queue);
> + return 0;
> +}
> +
> +static int bt_i2c_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct bt_i2c_slave *bt_slave = i2c_get_clientdata(client);
> + u8 *buf;
> +
> + spin_lock(&bt_slave->lock);
> + switch (event) {
> + case I2C_SLAVE_WRITE_REQUESTED:
> + bt_slave->msg_idx = 0;
> + break;
> +
> + case I2C_SLAVE_WRITE_RECEIVED:
> + buf = (u8 *) &bt_slave->request;
> + if (bt_slave->msg_idx >= sizeof(struct bt_i2c_slave))
> + break;
> +
> + buf[bt_slave->msg_idx++] = *val;
> + if (bt_slave->msg_idx >= bt_msg_len(&bt_slave->request))
> + handle_request(bt_slave);
> + break;
> +
> + case I2C_SLAVE_READ_REQUESTED:
> + buf = (u8 *) &bt_slave->response;
> + bt_slave->msg_idx = 0;
> + *val = buf[bt_slave->msg_idx];
> + /*
> + * Do not increment buffer_idx here, because we don't know if
> + * this byte will be actually used. Read Linux I2C slave docs
> + * for details.
> + */
> + break;
> +
> + case I2C_SLAVE_READ_PROCESSED:
> + buf = (u8 *) &bt_slave->response;
> + if (bt_slave->response.len &&
> + bt_slave->msg_idx < bt_msg_len(&bt_slave->response)) {
> + *val = buf[++bt_slave->msg_idx];
> + } else {
> + *val = 0;
> + }
> + if (bt_slave->msg_idx + 1 >= bt_msg_len(&bt_slave->response))
> + complete_response(bt_slave);
> + break;
> +
> + case I2C_SLAVE_STOP:
> + bt_slave->msg_idx = 0;
> + break;
> +
> + default:
> + break;
> + }
> + spin_unlock(&bt_slave->lock);
> +
> + return 0;
> +}
> +
> +static int bt_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct bt_i2c_slave *bt_slave;
> + int ret;
> +
> + bt_slave = devm_kzalloc(&client->dev, sizeof(struct bt_i2c_slave),
> + GFP_KERNEL);
> + if (!bt_slave)
> + return -ENOMEM;
> +
> + spin_lock_init(&bt_slave->lock);
> + init_waitqueue_head(&bt_slave->wait_queue);
> + atomic_set(&bt_slave->request_queue_len, 0);
> + atomic_set(&bt_slave->response_in_progress, 0);
> + INIT_LIST_HEAD(&bt_slave->request_queue);
> +
> + mutex_init(&bt_slave->file_mutex);
> +
> + bt_slave->miscdev.minor = MISC_DYNAMIC_MINOR;
> + bt_slave->miscdev.name = DEVICE_NAME;
> + bt_slave->miscdev.fops = &bt_fops;
> + bt_slave->miscdev.parent = &client->dev;
> + ret = misc_register(&bt_slave->miscdev);
> + if (ret < 0)
> + return ret;
> +
> + bt_slave->client = client;
> + i2c_set_clientdata(client, bt_slave);
> + ret = i2c_slave_register(client, bt_i2c_slave_cb);
> + if (ret < 0) {
> + misc_deregister(&bt_slave->miscdev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int bt_i2c_remove(struct i2c_client *client)
> +{
> + struct bt_i2c_slave *bt_slave = i2c_get_clientdata(client);
> +
> + i2c_slave_unregister(client);
> + misc_deregister(&bt_slave->miscdev);
> + return 0;
> +}
> +
> +static const struct i2c_device_id bt_i2c_id[] = {
> + {"bt-i2c-slave", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(i2c, bt_i2c_id);
> +
> +static struct i2c_driver bt_i2c_driver = {
> + .driver = {
> + .name = "bt-i2c-slave",
> + },
> + .probe = bt_i2c_probe,
> + .remove = bt_i2c_remove,
> + .id_table = bt_i2c_id,
> +};
> +module_i2c_driver(bt_i2c_driver);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> +MODULE_DESCRIPTION("IPMI Block Transfer over I2C slave.");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/misc/bt.h b/drivers/misc/bt.h
> new file mode 100644
> index 0000000..c0c2881
> --- /dev/null
> +++ b/drivers/misc/bt.h
> @@ -0,0 +1,19 @@
> +#ifndef _DRIVERS_MISC_BT_H
> +#define _DRIVERS_MISC_BT_H
> +
> +#include <linux/types.h>
> +
> +struct bt_msg {
> + u8 len;
> + u8 netfn_lun;
> + u8 seq;
> + u8 cmd;
> + u8 payload[252];
> +};
> +
> +static inline u32 bt_msg_len(struct bt_msg *bt_request)
> +{
> + return bt_request->len + 1;
> +}
> +
> +#endif /* _DRIVERS_MISC_BT_H */
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 0/2] misc: bt: added Block Transfer over I2C
2016-09-09 5:22 ` [RFC 0/2] misc: bt: added Block Transfer over I2C Joel Stanley
@ 2016-09-09 16:30 ` Cédric Le Goater
2016-09-09 21:59 ` Brendan Higgins
0 siblings, 1 reply; 8+ messages in thread
From: Cédric Le Goater @ 2016-09-09 16:30 UTC (permalink / raw)
To: Joel Stanley, Brendan Higgins; +Cc: OpenBMC Maillist
On 09/09/2016 07:22 AM, Joel Stanley wrote:
> Hey Brendan,
>
> On Fri, Sep 9, 2016 at 6:58 AM, Brendan Higgins
> <brendanhiggins@google.com> wrote:
>> Looking for feedback on my new bt-i2c (Block Transfer over I2C) interface.
>
> Thanks for sending this out. You're setting a great example for the rest of us!
>
> I don't have an specific feedback for you, other than to say I think
> what you are doing is reasonable.
>
>> I based the slave side (BMC) driver on the bt-host driver; the file system
>> interface is fully compatible and has been tested with btbridged and ipmid
>> running on an ast2500 evb.
>
> This is timely. Cedric has recently sent out the bt-host patches[1].
> One of the questions raised was the suitability of the userspace API
> for other users. It would be useful for you to put forward your
> thoughts on that discussion.
>
> [1] http://www.spinics.net/lists/arm-kernel/msg527834.html
yes. The main question is about the use of the ioctl to propagate
the SMS ATN event to the host. I think this is probably the best
method but as this is a user API, we should be careful with what we
merge. I don't know any other implementation of a IPMI BT driver
for the BMC side.
Also, the drivers should probably be under char/ipmi. I will change
that.
Finaly, the device name should contain "ipmi" IMO. Let's find a
name ! or a pattern at least.
Thanks,
C.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 1/2] misc: bt: added IPMI Block Transfer over I2C master and slave
2016-09-09 5:51 ` Joel Stanley
@ 2016-09-09 21:37 ` Brendan Higgins
0 siblings, 0 replies; 8+ messages in thread
From: Brendan Higgins @ 2016-09-09 21:37 UTC (permalink / raw)
To: Joel Stanley; +Cc: OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 24804 bytes --]
> There is talk of putting the bt-host driver into drivers/char/ipmi. I
> imagine that will affect your work.
Yes it would; this was talked about more on the cover letter, so I will
continue this discussion there.
> I didn't pick up on this when reading your cover letter. The host side
> will read the messages straight into the kernel and userspace, not
> pass them through firmware?
That is the plan at this time. If the BIOS were to want to communicate with
the BMC they would obviously have to implement the master side of this
driver. I think it is useful to
a) provide a reference implementation in the kernel
b) not make any assumptions about something like BIOS runtime services
In any case, the slave side does not care how the master organizes its
communication with the BMC.
> It feels like there should be a neater way of doing this read/write
> and wait operation that the driver does. I don't have a specific
> recommendation though.
I do not think the write is particularly messy, maybe I am being
presumptuous about the amount of errors that an I2C bus may experience. I
can appreciate your concern about the read though; I was not thrilled about
having to poll for a non-zero length, but without SMBus Alert we do not
have a way of knowing whether a read is ready without polling (I fully
intend on adding support for that later on, but that requires a change to
I2C core and I would really like to keep that separate :-) ).
> The goto doesn't do much :)
No it does not, but I thought it made it clear that it was intentionally
failing out rather than proceeding normally. I guess I thought it better
fit past criticism about explicit fail out (I am thinking of "goto out"
over "cleanup return" in the I2C driver). In any case, I do not feel
strongly about it, but I guess I would like an explanation.
Thanks!
On Thu, Sep 8, 2016 at 10:51 PM Joel Stanley <joel@jms.id.au> wrote:
> On Fri, Sep 9, 2016 at 6:58 AM, Brendan Higgins
> <brendanhiggins@google.com> wrote:
> > The IPMI definition of the Block Transfer protocol defines the hardware
> > registers and behavior in addition to the message format and messaging
> > semantics. This implements a new protocol that uses IPMI Block Transfer
> > messages and semantics on top of a standard I2C interface. This protocol
> > has the same BMC side file system interface as "bt-host".
> >
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
> > drivers/misc/Kconfig | 23 ++++
> > drivers/misc/Makefile | 2 +
> > drivers/misc/bt-i2c-master.c | 199 +++++++++++++++++++++++++++
> > drivers/misc/bt-i2c-slave.c | 317
> +++++++++++++++++++++++++++++++++++++++++++
> > drivers/misc/bt.h | 19 +++
> > 5 files changed, 560 insertions(+)
> > create mode 100644 drivers/misc/bt-i2c-master.c
> > create mode 100644 drivers/misc/bt-i2c-slave.c
> > create mode 100644 drivers/misc/bt.h
> >
>
> There is talk of putting the bt-host driver into drivers/char/ipmi. I
> imagine that will affect your work.
>
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 4617ddc..1c7e3a9 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -809,6 +809,29 @@ config ASPEED_BT_IPMI_HOST
> > help
> > Support for the Aspeed BT ipmi host.
> >
> > +config BT_I2C_SLAVE
> > + depends on !ASPEED_BT_IPMI_HOST && I2C_SLAVE
> > + tristate "BT I2C IPMI slave driver"
> > + default "n"
> > + ---help---
> > + Block Transfer over I2C defines a new IPMI compatible
> interface that
> > + uses Block Transfer messages and semantics on top of plain old
> I2C.
> > +
> > + This adds support for the slave side of the protocol (only
> really
> > + useful for BMCs (Baseboard Management Controllers)).
> > +
> > +config BT_I2C_MASTER
> > + depends on I2C
> > + tristate "BT I2C IPMI master driver"
> > + default "n"
> > + ---help---
> > + Block Transfer over I2C defines a new IPMI compatible
> interface that
> > + uses Block Transfer messages and semantics on top of plain old
> I2C.
> > +
> > + This adds support for master side of the protocol (this is
> most likely
> > + what you want if you are compiling for a regular CPU, only
> useful if
> > + your computer has a BMC that speaks the slave side of this
> protocol).
>
> I didn't pick up on this when reading your cover letter. The host side
> will read the messages straight into the kernel and userspace, not
> pass them through firmware?
>
> > +
> > source "drivers/misc/c2port/Kconfig"
> > source "drivers/misc/eeprom/Kconfig"
> > source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index 724861b..b642ed8 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -58,3 +58,5 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
> > obj-$(CONFIG_CXL_BASE) += cxl/
> > obj-$(CONFIG_PANEL) += panel.o
> > obj-$(CONFIG_ASPEED_BT_IPMI_HOST) += bt-host.o
> > +obj-$(CONFIG_BT_I2C_SLAVE) += bt-i2c-slave.o
> > +obj-$(CONFIG_BT_I2C_MASTER) += bt-i2c-master.o
> > diff --git a/drivers/misc/bt-i2c-master.c b/drivers/misc/bt-i2c-master.c
> > new file mode 100644
> > index 0000000..22142d6
> > --- /dev/null
> > +++ b/drivers/misc/bt-i2c-master.c
> > @@ -0,0 +1,199 @@
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/errno.h>
> > +#include <linux/poll.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/timer.h>
> > +#include <linux/jiffies.h>
> > +
> > +#include "bt.h"
> > +
> > +#define DEVICE_NAME "bt-master"
> > +
> > +struct bt_i2c_master {
> > + struct i2c_client *client;
> > + struct miscdevice miscdev;
> > + struct mutex file_mutex;
> > +};
> > +
> > +static const unsigned long write_timeout = 25;
> > +
> > +static int send_bt_request(struct bt_i2c_master *bt_master,
> > + struct bt_msg *bt_request)
> > +{
> > + struct i2c_client *client = bt_master->client;
> > + u8 *buf = (u8 *) bt_request;
> > + unsigned long timeout, read_time;
> > + int ret;
> > +
> > + timeout = jiffies + msecs_to_jiffies(write_timeout);
> > + do {
> > + read_time = jiffies;
> > + ret = i2c_master_send(client, buf,
> bt_msg_len(bt_request));
> > + if (ret >= 0)
> > + return 0;
> > + usleep_range(1000, 1500);
> > + } while (time_before(read_time, timeout));
>
> It feels like there should be a neater way of doing this read/write
> and wait operation that the driver does. I don't have a specific
> recommendation though.
>
> > + return ret;
> > +}
> > +
> > +static int receive_bt_response(struct bt_i2c_master *bt_master,
> > + struct bt_msg *bt_response)
> > +{
> > + struct i2c_client *client = bt_master->client;
> > + u8 *buf = (u8 *) bt_response;
> > + u8 len = 0;
> > + unsigned long timeout, read_time;
> > + int ret;
> > +
> > + /*
> > + * Slave may not NACK when not ready, so we peek at the first
> byte to
> > + * see if it is a valid length.
> > + */
> > + while (len == 0) {
> > + i2c_master_recv(client, &len, 1);
> > + if (len != 0)
> > + break;
> > + usleep_range(1000, 1500);
> > +
> > + /* Signal received: quit syscall. */
> > + if (signal_pending(current))
> > + return -ERESTARTSYS;
> > + }
> > +
> > + timeout = jiffies + msecs_to_jiffies(write_timeout);
> > + do {
> > + read_time = jiffies;
> > + ret = i2c_master_recv(client, buf, len + 1);
> > + if (ret >= 0)
> > + return 0;
> > + usleep_range(1000, 1500);
> > + } while (time_before(read_time, timeout));
> > + return ret;
> > +}
> > +
> > +static inline struct bt_i2c_master *to_bt_i2c_master(struct file *file)
> > +{
> > + return container_of(file->private_data, struct bt_i2c_master,
> miscdev);
> > +}
> > +
> > +static ssize_t bt_read(struct file *file, char __user *buf, size_t
> count,
> > + loff_t *ppos)
> > +{
> > + struct bt_i2c_master *bt_master = to_bt_i2c_master(file);
> > + struct bt_msg msg;
> > + int ret;
> > +
> > + mutex_lock(&bt_master->file_mutex);
> > + ret = receive_bt_response(bt_master, &msg);
> > + if (ret < 0)
> > + goto out;
> > + count = min(count, bt_msg_len(&msg));
> > + if (copy_to_user(buf, &msg, count)) {
> > + ret = -EFAULT;
> > + goto out;
>
> The goto doesn't do much :)
>
> > + }
> > +
> > +out:
> > + mutex_unlock(&bt_master->file_mutex);
> > + if (ret < 0)
> > + return ret;
> > + else
> > + return count;
> > +}
> > +
> > +static ssize_t bt_write(struct file *file, const char __user *buf,
> size_t count,
> > + loff_t *ppos)
> > +{
> > + struct bt_i2c_master *bt_master = to_bt_i2c_master(file);
> > + struct bt_msg msg;
> > + int ret;
> > +
> > + mutex_lock(&bt_master->file_mutex);
> > + if (count > sizeof(struct bt_msg)) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > + if (copy_from_user(&msg, buf, count) || count <
> bt_msg_len(&msg)) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > + ret = send_bt_request(bt_master, &msg);
> > + if (ret < 0)
> > + goto out;
> > +
> > +out:
> > + mutex_unlock(&bt_master->file_mutex);
> > + if (ret < 0)
> > + return ret;
> > + else
> > + return count;
> > +}
> > +
> > +static const struct file_operations bt_fops = {
> > + .owner = THIS_MODULE,
> > + .read = bt_read,
> > + .write = bt_write,
> > +};
> > +
> > +static int bt_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct bt_i2c_master *bt_master;
> > + int ret;
> > +
> > + bt_master = devm_kzalloc(&client->dev, sizeof(struct
> bt_i2c_master),
> > + GFP_KERNEL);
> > + if (!bt_master)
> > + return -ENOMEM;
> > +
> > + mutex_init(&bt_master->file_mutex);
> > + bt_master->client = client;
> > + i2c_set_clientdata(client, bt_master);
> > +
> > + bt_master->miscdev.minor = MISC_DYNAMIC_MINOR;
> > + bt_master->miscdev.name = DEVICE_NAME;
> > + bt_master->miscdev.fops = &bt_fops;
> > + bt_master->miscdev.parent = &client->dev;
> > + ret = misc_register(&bt_master->miscdev);
> > + if (ret < 0)
> > + return ret;
> > + return 0;
> > +}
> > +
> > +static int bt_i2c_remove(struct i2c_client *client)
> > +{
> > + struct bt_i2c_master *bt_master = i2c_get_clientdata(client);
> > +
> > + misc_deregister(&bt_master->miscdev);
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id bt_i2c_id[] = {
> > + {"bt-i2c-master", 0},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(i2c, bt_i2c_id);
> > +
> > +static struct i2c_driver bt_i2c_driver = {
> > + .driver = {
> > + .name = "bt-i2c-master",
> > + },
> > + .probe = bt_i2c_probe,
> > + .remove = bt_i2c_remove,
> > + .id_table = bt_i2c_id,
> > +};
> > +module_i2c_driver(bt_i2c_driver);
> > +
> > +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> > +MODULE_DESCRIPTION("IPMI Block Transfer over I2C master.");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/misc/bt-i2c-slave.c b/drivers/misc/bt-i2c-slave.c
> > new file mode 100644
> > index 0000000..1a90e05
> > --- /dev/null
> > +++ b/drivers/misc/bt-i2c-slave.c
> > @@ -0,0 +1,317 @@
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/errno.h>
> > +#include <linux/poll.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/timer.h>
> > +#include <linux/jiffies.h>
> > +
> > +#include "bt.h"
> > +
> > +/*
> > + * TODO: This is "bt-host" to match the bt-host driver; however, I
> think this is
> > + * unclear in the context of a CPU side driver. Should probably name
> this
> > + * and the DEVICE_NAME in bt-host to something like "bt-bmc" or
> "bt-slave".
> > + */
> > +#define DEVICE_NAME "bt-host"
> > +
> > +static const unsigned long request_queue_max_len = 256;
> > +
> > +struct bt_request_elem {
> > + struct list_head list;
> > + struct bt_msg request;
> > +};
> > +
> > +struct bt_i2c_slave {
> > + struct i2c_client *client;
> > + struct miscdevice miscdev;
> > + struct bt_msg request;
> > + struct list_head request_queue;
> > + atomic_t request_queue_len;
> > + struct bt_msg response;
> > + atomic_t response_in_progress;
> > + size_t msg_idx;
> > + spinlock_t lock;
> > + wait_queue_head_t wait_queue;
> > + struct mutex file_mutex;
> > +};
> > +
> > +static int receive_bt_request(struct bt_i2c_slave *bt_slave, bool
> non_blocking,
> > + struct bt_msg *bt_request) {
> > + unsigned long flags;
> > + struct bt_request_elem *queue_elem;
> > +
> > + if (!atomic_read(&bt_slave->request_queue_len) && non_blocking)
> > + return -EAGAIN;
> > + else if (!non_blocking)
> > + wait_event_interruptible(
> > + bt_slave->wait_queue,
> > +
> atomic_read(&bt_slave->request_queue_len));
> > +
> > + spin_lock_irqsave(&bt_slave->lock, flags);
> > + queue_elem = list_first_entry(&bt_slave->request_queue,
> > + struct bt_request_elem, list);
> > + memcpy(bt_request, &queue_elem->request, sizeof(struct bt_msg));
> > + list_del(&queue_elem->list);
> > + kfree(queue_elem);
> > + atomic_dec(&bt_slave->request_queue_len);
> > + spin_unlock_irqrestore(&bt_slave->lock, flags);
> > + return 0;
> > +}
> > +
> > +static int send_bt_response(struct bt_i2c_slave *bt_slave, bool
> non_blocking,
> > + struct bt_msg *bt_response) {
> > + unsigned long flags;
> > +
> > + if (non_blocking && atomic_read(&bt_slave->response_in_progress))
> > + return -EAGAIN;
> > + else if (!non_blocking)
> > + wait_event_interruptible(
> > + bt_slave->wait_queue,
> > +
> !atomic_read(&bt_slave->response_in_progress));
> > +
> > + spin_lock_irqsave(&bt_slave->lock, flags);
> > + memcpy(&bt_slave->response, bt_response, sizeof(struct bt_msg));
> > + atomic_set(&bt_slave->response_in_progress, 0);
> > + spin_unlock_irqrestore(&bt_slave->lock, flags);
> > + return 0;
> > +}
> > +
> > +static inline struct bt_i2c_slave *to_bt_i2c_slave(struct file *file)
> > +{
> > + return container_of(file->private_data, struct bt_i2c_slave,
> miscdev);
> > +}
> > +
> > +static ssize_t bt_read(struct file *file, char __user *buf, size_t
> count,
> > + loff_t *ppos)
> > +{
> > + struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
> > + struct bt_msg msg;
> > + ssize_t ret;
> > +
> > + mutex_lock(&bt_slave->file_mutex);
> > + ret = receive_bt_request(bt_slave, file->f_flags & O_NONBLOCK,
> &msg);
> > + if (ret < 0)
> > + goto out;
> > + count = min(count, bt_msg_len(&msg));
> > + if (copy_to_user(buf, &msg, count)) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > +out:
> > + mutex_unlock(&bt_slave->file_mutex);
> > + if (ret < 0)
> > + return ret;
> > + else
> > + return count;
> > +}
> > +
> > +static ssize_t bt_write(struct file *file, const char __user *buf,
> size_t count,
> > + loff_t *ppos)
> > +{
> > + struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
> > + struct bt_msg msg;
> > + ssize_t ret;
> > +
> > + if (count > sizeof(struct bt_msg))
> > + return -EFAULT;
> > +
> > + if (copy_from_user(&msg, buf, count) || count < bt_msg_len(&msg))
> > + return -EFAULT;
> > +
> > + mutex_lock(&bt_slave->file_mutex);
> > + ret = send_bt_response(bt_slave, file->f_flags & O_NONBLOCK,
> &msg);
> > + mutex_unlock(&bt_slave->file_mutex);
> > +
> > + if (ret < 0)
> > + return ret;
> > + else
> > + return count;
> > +}
> > +
> > +static long bt_ioctl(struct file *file, unsigned int cmd, unsigned long
> param)
> > +{
> > + return 0;
> > +}
> > +
> > +static unsigned int bt_poll(struct file *file, poll_table *wait)
> > +{
> > + struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
> > + unsigned int mask = 0;
> > +
> > + mutex_lock(&bt_slave->file_mutex);
> > + poll_wait(file, &bt_slave->wait_queue, wait);
> > +
> > + if (atomic_read(&bt_slave->request_queue_len))
> > + mask |= POLLIN;
> > + if (!atomic_read(&bt_slave->response_in_progress))
> > + mask |= POLLOUT;
> > + mutex_unlock(&bt_slave->file_mutex);
> > + return mask;
> > +}
> > +
> > +static const struct file_operations bt_fops = {
> > + .owner = THIS_MODULE,
> > + .read = bt_read,
> > + .write = bt_write,
> > + .poll = bt_poll,
> > + .unlocked_ioctl = bt_ioctl,
> > +};
> > +
> > +static int handle_request(struct bt_i2c_slave *bt_slave)
> > +{
> > + struct bt_request_elem *queue_elem;
> > +
> > + if (atomic_read(&bt_slave->request_queue_len) >=
> request_queue_max_len)
> > + return -EFAULT;
> > + queue_elem = kmalloc(sizeof(struct bt_request_elem), GFP_KERNEL);
> > + memcpy(&queue_elem->request, &bt_slave->request, sizeof(struct
> bt_msg));
> > + list_add(&queue_elem->list, &bt_slave->request_queue);
> > + atomic_inc(&bt_slave->request_queue_len);
> > + wake_up_all(&bt_slave->wait_queue);
> > + return 0;
> > +}
> > +
> > +static int complete_response(struct bt_i2c_slave *bt_slave)
> > +{
> > + /* Invalidate response in buffer to denote it having been sent.
> */
> > + bt_slave->response.len = 0;
> > + atomic_set(&bt_slave->response_in_progress, 1);
> > + wake_up_all(&bt_slave->wait_queue);
> > + return 0;
> > +}
> > +
> > +static int bt_i2c_slave_cb(struct i2c_client *client,
> > + enum i2c_slave_event event, u8 *val)
> > +{
> > + struct bt_i2c_slave *bt_slave = i2c_get_clientdata(client);
> > + u8 *buf;
> > +
> > + spin_lock(&bt_slave->lock);
> > + switch (event) {
> > + case I2C_SLAVE_WRITE_REQUESTED:
> > + bt_slave->msg_idx = 0;
> > + break;
> > +
> > + case I2C_SLAVE_WRITE_RECEIVED:
> > + buf = (u8 *) &bt_slave->request;
> > + if (bt_slave->msg_idx >= sizeof(struct bt_i2c_slave))
> > + break;
> > +
> > + buf[bt_slave->msg_idx++] = *val;
> > + if (bt_slave->msg_idx >= bt_msg_len(&bt_slave->request))
> > + handle_request(bt_slave);
> > + break;
> > +
> > + case I2C_SLAVE_READ_REQUESTED:
> > + buf = (u8 *) &bt_slave->response;
> > + bt_slave->msg_idx = 0;
> > + *val = buf[bt_slave->msg_idx];
> > + /*
> > + * Do not increment buffer_idx here, because we don't
> know if
> > + * this byte will be actually used. Read Linux I2C slave
> docs
> > + * for details.
> > + */
> > + break;
> > +
> > + case I2C_SLAVE_READ_PROCESSED:
> > + buf = (u8 *) &bt_slave->response;
> > + if (bt_slave->response.len &&
> > + bt_slave->msg_idx < bt_msg_len(&bt_slave->response))
> {
> > + *val = buf[++bt_slave->msg_idx];
> > + } else {
> > + *val = 0;
> > + }
> > + if (bt_slave->msg_idx + 1 >=
> bt_msg_len(&bt_slave->response))
> > + complete_response(bt_slave);
> > + break;
> > +
> > + case I2C_SLAVE_STOP:
> > + bt_slave->msg_idx = 0;
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > + spin_unlock(&bt_slave->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int bt_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct bt_i2c_slave *bt_slave;
> > + int ret;
> > +
> > + bt_slave = devm_kzalloc(&client->dev, sizeof(struct
> bt_i2c_slave),
> > + GFP_KERNEL);
> > + if (!bt_slave)
> > + return -ENOMEM;
> > +
> > + spin_lock_init(&bt_slave->lock);
> > + init_waitqueue_head(&bt_slave->wait_queue);
> > + atomic_set(&bt_slave->request_queue_len, 0);
> > + atomic_set(&bt_slave->response_in_progress, 0);
> > + INIT_LIST_HEAD(&bt_slave->request_queue);
> > +
> > + mutex_init(&bt_slave->file_mutex);
> > +
> > + bt_slave->miscdev.minor = MISC_DYNAMIC_MINOR;
> > + bt_slave->miscdev.name = DEVICE_NAME;
> > + bt_slave->miscdev.fops = &bt_fops;
> > + bt_slave->miscdev.parent = &client->dev;
> > + ret = misc_register(&bt_slave->miscdev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + bt_slave->client = client;
> > + i2c_set_clientdata(client, bt_slave);
> > + ret = i2c_slave_register(client, bt_i2c_slave_cb);
> > + if (ret < 0) {
> > + misc_deregister(&bt_slave->miscdev);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int bt_i2c_remove(struct i2c_client *client)
> > +{
> > + struct bt_i2c_slave *bt_slave = i2c_get_clientdata(client);
> > +
> > + i2c_slave_unregister(client);
> > + misc_deregister(&bt_slave->miscdev);
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id bt_i2c_id[] = {
> > + {"bt-i2c-slave", 0},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(i2c, bt_i2c_id);
> > +
> > +static struct i2c_driver bt_i2c_driver = {
> > + .driver = {
> > + .name = "bt-i2c-slave",
> > + },
> > + .probe = bt_i2c_probe,
> > + .remove = bt_i2c_remove,
> > + .id_table = bt_i2c_id,
> > +};
> > +module_i2c_driver(bt_i2c_driver);
> > +
> > +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> > +MODULE_DESCRIPTION("IPMI Block Transfer over I2C slave.");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/misc/bt.h b/drivers/misc/bt.h
> > new file mode 100644
> > index 0000000..c0c2881
> > --- /dev/null
> > +++ b/drivers/misc/bt.h
> > @@ -0,0 +1,19 @@
> > +#ifndef _DRIVERS_MISC_BT_H
> > +#define _DRIVERS_MISC_BT_H
> > +
> > +#include <linux/types.h>
> > +
> > +struct bt_msg {
> > + u8 len;
> > + u8 netfn_lun;
> > + u8 seq;
> > + u8 cmd;
> > + u8 payload[252];
> > +};
> > +
> > +static inline u32 bt_msg_len(struct bt_msg *bt_request)
> > +{
> > + return bt_request->len + 1;
> > +}
> > +
> > +#endif /* _DRIVERS_MISC_BT_H */
> > --
> > 2.8.0.rc3.226.g39d4020
> >
> > _______________________________________________
> > openbmc mailing list
> > openbmc@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/openbmc
>
[-- Attachment #2: Type: text/html, Size: 48068 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 0/2] misc: bt: added Block Transfer over I2C
2016-09-09 16:30 ` Cédric Le Goater
@ 2016-09-09 21:59 ` Brendan Higgins
0 siblings, 0 replies; 8+ messages in thread
From: Brendan Higgins @ 2016-09-09 21:59 UTC (permalink / raw)
To: Cédric Le Goater, Joel Stanley; +Cc: OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]
@Joel, thanks! I appreciate that :-)
> yes. The main question is about the use of the ioctl to propagate
> the SMS ATN event to the host. I think this is probably the best
You are changing the behavior of the interface in some respect, so I think
ioctl makes sense.
> method but as this is a user API, we should be careful with what we
> merge. I don't know any other implementation of a IPMI BT driver
> for the BMC side.
Fair point, that is something I am not sure about either. I think
upstreaming my patch would be contingent on whether we wanted to upstream
yours. On the other hand, if people want your IPMI BT driver from the BMC
side, it would be worth seeing if upstream wanted mine: bt-i2c addresses an
Aspeed specific issue (cannot NACK arbitrary messages makes SSIF support
possibly impossible), but we will see. I am more interested in seeing how
you guys feel about using Block Transfer for more things in a possibly
somewhat modified manner.
It seems like OpenBMC assumes all IPMI messages are assumed to be framed as
BT messages; maybe we want to consider formalizing around them? Just a
thought.
> Also, the drivers should probably be under char/ipmi. I will change
> that.
Makes sense, I will fix this in my next update.
> Finaly, the device name should contain "ipmi" IMO. Let's find a
> name ! or a pattern at least.
Also makes sense; I definitely want to follow the same pattern as you since
this is intended to provide the same API to the user. To throw my 2 cents
in, I do not like the "host" part of "bt-host", just because it feels
confusing. I get it from the standpoint of the userland (it is a port to
the host), but from a driver standpoint it seems confusing. I like maybe
use the term "bmc" or "slave" instead. Maybe the prefix could be
"ipmi-bt-xxx-cpu" and "ipmi-bt-xxx-bmc"?
Cheers
On Fri, Sep 9, 2016 at 9:30 AM Cédric Le Goater <clg@kaod.org> wrote:
> On 09/09/2016 07:22 AM, Joel Stanley wrote:
> > Hey Brendan,
> >
> > On Fri, Sep 9, 2016 at 6:58 AM, Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> >> Looking for feedback on my new bt-i2c (Block Transfer over I2C)
> interface.
> >
> > Thanks for sending this out. You're setting a great example for the rest
> of us!
> >
> > I don't have an specific feedback for you, other than to say I think
> > what you are doing is reasonable.
> >
> >> I based the slave side (BMC) driver on the bt-host driver; the file
> system
> >> interface is fully compatible and has been tested with btbridged and
> ipmid
> >> running on an ast2500 evb.
> >
> > This is timely. Cedric has recently sent out the bt-host patches[1].
> > One of the questions raised was the suitability of the userspace API
> > for other users. It would be useful for you to put forward your
> > thoughts on that discussion.
> >
> > [1] http://www.spinics.net/lists/arm-kernel/msg527834.html
>
> yes. The main question is about the use of the ioctl to propagate
> the SMS ATN event to the host. I think this is probably the best
> method but as this is a user API, we should be careful with what we
> merge. I don't know any other implementation of a IPMI BT driver
> for the BMC side.
>
> Also, the drivers should probably be under char/ipmi. I will change
> that.
>
> Finaly, the device name should contain "ipmi" IMO. Let's find a
> name ! or a pattern at least.
>
> Thanks,
>
> C.
>
>
[-- Attachment #2: Type: text/html, Size: 7473 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-09-09 21:59 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-08 21:28 [RFC 0/2] misc: bt: added Block Transfer over I2C Brendan Higgins
2016-09-08 21:28 ` [RFC 1/2] misc: bt: added IPMI Block Transfer over I2C master and slave Brendan Higgins
2016-09-09 5:51 ` Joel Stanley
2016-09-09 21:37 ` Brendan Higgins
2016-09-08 21:28 ` [RFC 2/2] misc: bt: added documentation for bt-i2c drivers Brendan Higgins
2016-09-09 5:22 ` [RFC 0/2] misc: bt: added Block Transfer over I2C Joel Stanley
2016-09-09 16:30 ` Cédric Le Goater
2016-09-09 21:59 ` Brendan Higgins
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.