| Message ID | 1416326695-13083-3-git-send-email-wsa@the-dreams.de |
|---|---|
| State | Accepted |
| Delegated to: | Wolfram Sang |
| Headers | show |
On Tue, Nov 18, 2014 at 5:04 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > The first user of the i2c-slave interface is an eeprom simulator. It is > a shared memory which can be accessed by the remote master via I2C and > locally via sysfs. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Changes since RFC: > * fix build error for modules > * don't hardcode size > * add boundary checks for sysfs access > * use a spinlock > * s/at24s/eeprom/g > * add some docs > * clean up exit paths > * use size-variable instead of driver_data > > drivers/i2c/Kconfig | 10 +++ > drivers/i2c/Makefile | 1 + > drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 181 insertions(+) > create mode 100644 drivers/i2c/i2c-slave-eeprom.c > > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig > index b51a402752c4..8c9e619f3026 100644 > --- a/drivers/i2c/Kconfig > +++ b/drivers/i2c/Kconfig > @@ -110,6 +110,16 @@ config I2C_STUB > > If you don't know what to do here, definitely say N. > > +config I2C_SLAVE > + bool "I2C slave support" > + > +if I2C_SLAVE > + > +config I2C_SLAVE_EEPROM > + tristate "I2C eeprom slave driver" > + > +endif > + > config I2C_DEBUG_CORE > bool "I2C Core debugging messages" > help > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile > index 1722f50f2473..45095b3d16a9 100644 > --- a/drivers/i2c/Makefile > +++ b/drivers/i2c/Makefile > @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o > obj-$(CONFIG_I2C_MUX) += i2c-mux.o > obj-y += algos/ busses/ muxes/ > obj-$(CONFIG_I2C_STUB) += i2c-stub.o > +obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o > > ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG > CFLAGS_i2c-core.o := -Wno-deprecated-declarations > diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c > new file mode 100644 > index 000000000000..6631400b5f02 > --- /dev/null > +++ b/drivers/i2c/i2c-slave-eeprom.c > @@ -0,0 +1,170 @@ > +/* > + * I2C slave mode EEPROM simulator > + * > + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <wsa@sang-engineering.com> > + * Copyright (C) 2014 by Renesas Electronics Corporation > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; version 2 of the License. > + * > + * Because most IP blocks can only detect one I2C slave address anyhow, this > + * driver does not support simulating EEPROM types which take more than one > + * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit > + * pointer, yet implementation is deferred until the need actually arises. > + */ > + > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/sysfs.h> > + > +struct eeprom_data { > + struct bin_attribute bin; > + bool first_write; > + spinlock_t buffer_lock; > + u8 buffer_idx; > + u8 buffer[]; > +}; > + > +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, > + enum i2c_slave_event event, u8 *val) > +{ > + struct eeprom_data *eeprom = i2c_get_clientdata(client); > + > + switch (event) { > + case I2C_SLAVE_REQ_WRITE_END: > + if (eeprom->first_write) { > + eeprom->buffer_idx = *val; > + eeprom->first_write = false; > + } else { > + spin_lock(&eeprom->buffer_lock); > + eeprom->buffer[eeprom->buffer_idx++] = *val; > + spin_unlock(&eeprom->buffer_lock); > + } > + break; > + > + case I2C_SLAVE_REQ_READ_START: > + spin_lock(&eeprom->buffer_lock); > + *val = eeprom->buffer[eeprom->buffer_idx]; > + spin_unlock(&eeprom->buffer_lock); > + break; > + > + case I2C_SLAVE_REQ_READ_END: > + eeprom->buffer_idx++; > + break; > + > + case I2C_SLAVE_STOP: > + eeprom->first_write = true; > + break; > + > + default: > + break; > + } > + > + return 0; > +} Would it make sense to have: WRITE_START WRITE_NEXT WRITE_STOP WRITE_REPEAT_START READ_START READ_NEXT READ_STOP READ_REPEAT_START Some devices may want different behavior for subsequent reads when they are separate transactions, compared to a single larger transaction. e.g. a single transaction may wraparound inside a >8bit register (e.g. for 16bit: msb, lsb, msb, lsb, ...), but step to the next register when a separate read/write is issued. Alternatively, a WRITE/READ_NEXT may be implemented more efficiently. This may not matter for current systems compared to the low-frequency bus, but who knows what IoT devices may bring to the table. Also, behavior may be different for repeat start versus stop/start, although a repeat start could be a start without a previous stop as well... Of course, if an I2C adapter cannot distinguish these events, this is probably a futile attempt at adding semantics that will silently break depending on the actual hardware/driver used. Regards, Stijn > + > +static ssize_t i2c_slave_eeprom_bin_read(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, char *buf, loff_t off, size_t count) > +{ > + struct eeprom_data *eeprom; > + unsigned long flags; > + > + if (off + count >= attr->size) > + return -EFBIG; > + > + eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj)); > + > + spin_lock_irqsave(&eeprom->buffer_lock, flags); > + memcpy(buf, &eeprom->buffer[off], count); > + spin_unlock_irqrestore(&eeprom->buffer_lock, flags); > + > + return count; > +} > + > +static ssize_t i2c_slave_eeprom_bin_write(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, char *buf, loff_t off, size_t count) > +{ > + struct eeprom_data *eeprom; > + unsigned long flags; > + > + if (off + count >= attr->size) > + return -EFBIG; > + > + eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj)); > + > + spin_lock_irqsave(&eeprom->buffer_lock, flags); > + memcpy(&eeprom->buffer[off], buf, count); > + spin_unlock_irqrestore(&eeprom->buffer_lock, flags); > + > + return count; > +} > + > +static int i2c_slave_eeprom_probe(struct i2c_client *client, const struct i2c_device_id *id) > +{ > + struct eeprom_data *eeprom; > + int ret; > + unsigned size = id->driver_data; > + > + eeprom = devm_kzalloc(&client->dev, sizeof(struct eeprom_data) + size, GFP_KERNEL); > + if (!eeprom) > + return -ENOMEM; > + > + eeprom->first_write = true; > + spin_lock_init(&eeprom->buffer_lock); > + i2c_set_clientdata(client, eeprom); > + > + sysfs_bin_attr_init(&eeprom->bin); > + eeprom->bin.attr.name = "slave-eeprom"; > + eeprom->bin.attr.mode = S_IRUSR | S_IWUSR; > + eeprom->bin.read = i2c_slave_eeprom_bin_read; > + eeprom->bin.write = i2c_slave_eeprom_bin_write; > + eeprom->bin.size = size; > + > + ret = sysfs_create_bin_file(&client->dev.kobj, &eeprom->bin); > + if (ret) > + return ret; > + > + ret = i2c_slave_register(client, i2c_slave_eeprom_slave_cb); > + if (ret) { > + sysfs_remove_bin_file(&client->dev.kobj, &eeprom->bin); > + return ret; > + } > + > + return 0; > +}; > + > +static int i2c_slave_eeprom_remove(struct i2c_client *client) > +{ > + struct eeprom_data *eeprom = i2c_get_clientdata(client); > + > + i2c_slave_unregister(client); > + sysfs_remove_bin_file(&client->dev.kobj, &eeprom->bin); > + > + return 0; > +} > + > +static const struct i2c_device_id i2c_slave_eeprom_id[] = { > + { "slave-24c02", 2048 / 8 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, i2c_slave_eeprom_id); > + > +static struct i2c_driver i2c_slave_eeprom_driver = { > + .driver = { > + .name = "i2c-slave-eeprom", > + .owner = THIS_MODULE, > + }, > + .probe = i2c_slave_eeprom_probe, > + .remove = i2c_slave_eeprom_remove, > + .id_table = i2c_slave_eeprom_id, > +}; > +module_i2c_driver(i2c_slave_eeprom_driver); > + > +MODULE_AUTHOR("Wolfram Sang <wsa@sang-engineering.com>"); > +MODULE_DESCRIPTION("I2C slave mode EEPROM simulator"); > +MODULE_LICENSE("GPL v2"); > -- > 2.1.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Wolfram, this mail is thematically more a reply to patch 1 and maybe just serves my understanding of the slave support. On Tue, Nov 18, 2014 at 05:04:54PM +0100, Wolfram Sang wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > The first user of the i2c-slave interface is an eeprom simulator. It is > a shared memory which can be accessed by the remote master via I2C and > locally via sysfs. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Changes since RFC: > * fix build error for modules > * don't hardcode size > * add boundary checks for sysfs access > * use a spinlock > * s/at24s/eeprom/g > * add some docs > * clean up exit paths > * use size-variable instead of driver_data > > drivers/i2c/Kconfig | 10 +++ > drivers/i2c/Makefile | 1 + > drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 181 insertions(+) > create mode 100644 drivers/i2c/i2c-slave-eeprom.c > > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig > index b51a402752c4..8c9e619f3026 100644 > --- a/drivers/i2c/Kconfig > +++ b/drivers/i2c/Kconfig > @@ -110,6 +110,16 @@ config I2C_STUB > > If you don't know what to do here, definitely say N. > > +config I2C_SLAVE > + bool "I2C slave support" > + > +if I2C_SLAVE > + > +config I2C_SLAVE_EEPROM > + tristate "I2C eeprom slave driver" > + > +endif > + > config I2C_DEBUG_CORE > bool "I2C Core debugging messages" > help > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile > index 1722f50f2473..45095b3d16a9 100644 > --- a/drivers/i2c/Makefile > +++ b/drivers/i2c/Makefile > @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o > obj-$(CONFIG_I2C_MUX) += i2c-mux.o > obj-y += algos/ busses/ muxes/ > obj-$(CONFIG_I2C_STUB) += i2c-stub.o > +obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o > > ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG > CFLAGS_i2c-core.o := -Wno-deprecated-declarations > diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c > new file mode 100644 > index 000000000000..6631400b5f02 > --- /dev/null > +++ b/drivers/i2c/i2c-slave-eeprom.c > @@ -0,0 +1,170 @@ > +/* > + * I2C slave mode EEPROM simulator > + * > + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <wsa@sang-engineering.com> > + * Copyright (C) 2014 by Renesas Electronics Corporation > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; version 2 of the License. > + * > + * Because most IP blocks can only detect one I2C slave address anyhow, this > + * driver does not support simulating EEPROM types which take more than one > + * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit > + * pointer, yet implementation is deferred until the need actually arises. > + */ > + > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/sysfs.h> > + > +struct eeprom_data { > + struct bin_attribute bin; > + bool first_write; > + spinlock_t buffer_lock; > + u8 buffer_idx; > + u8 buffer[]; > +}; > + > +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, > + enum i2c_slave_event event, u8 *val) > +{ > + struct eeprom_data *eeprom = i2c_get_clientdata(client); > + > + switch (event) { > + case I2C_SLAVE_REQ_WRITE_END: > + if (eeprom->first_write) { > + eeprom->buffer_idx = *val; > + eeprom->first_write = false; > + } else { > + spin_lock(&eeprom->buffer_lock); > + eeprom->buffer[eeprom->buffer_idx++] = *val; > + spin_unlock(&eeprom->buffer_lock); > + } > + break; > + > + case I2C_SLAVE_REQ_READ_START: > + spin_lock(&eeprom->buffer_lock); > + *val = eeprom->buffer[eeprom->buffer_idx]; > + spin_unlock(&eeprom->buffer_lock); > + break; > + > + case I2C_SLAVE_REQ_READ_END: > + eeprom->buffer_idx++; You don't check here for buffer_idx >= ARRAY_SIZE(buffer)? Ditto in the I2C_SLAVE_REQ_WRITE_END case. > + break; > + > + case I2C_SLAVE_STOP: > + eeprom->first_write = true; > + break; > + > + default: > + break; > + } > + > + return 0; > +} This is the most interesting function here because it uses the new interface, the functions below are only to update and show the simulated eeprom contents and driver boilerplate, right? When the eeprom driver is probed and the adapter driver notices a read request for the respective i2c address, this callback is called with event=I2C_SLAVE_REQ_READ_START. Returning 0 here and provide the first byte to send make the adapter ack the read request and send the data provided. If something != 0 is returned a NAK is sent? How is the next byte requested from the slave driver? I assume with two additional calls to the callback, first with event=I2C_SLAVE_REQ_READ_END, then event=I2C_SLAVE_REQ_READ_START once more. Would it make sense to reduce this to a single call? Does the driver at READ_END time already know if its write got acked? If so, how? This means that for each byte the callback is called. Would it make sense to make the API more flexible and allow the slave driver to return a buffer? This would remove some callback overhead and might allow to let the adapter driver make use of its DMA mechanism. Best regards Uwe
On Fri, Nov 21, 2014 at 08:19:41AM +0100, Uwe Kleine-König wrote: > Hello Wolfram, > > this mail is thematically more a reply to patch 1 and maybe just serves > my understanding of the slave support. > > On Tue, Nov 18, 2014 at 05:04:54PM +0100, Wolfram Sang wrote: > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > > The first user of the i2c-slave interface is an eeprom simulator. It is > > a shared memory which can be accessed by the remote master via I2C and > > locally via sysfs. > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > > > Changes since RFC: > > * fix build error for modules > > * don't hardcode size > > * add boundary checks for sysfs access > > * use a spinlock > > * s/at24s/eeprom/g > > * add some docs > > * clean up exit paths > > * use size-variable instead of driver_data > > > > drivers/i2c/Kconfig | 10 +++ > > drivers/i2c/Makefile | 1 + > > drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 181 insertions(+) > > create mode 100644 drivers/i2c/i2c-slave-eeprom.c > > > > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig > > index b51a402752c4..8c9e619f3026 100644 > > --- a/drivers/i2c/Kconfig > > +++ b/drivers/i2c/Kconfig > > @@ -110,6 +110,16 @@ config I2C_STUB > > > > If you don't know what to do here, definitely say N. > > > > +config I2C_SLAVE > > + bool "I2C slave support" > > + > > +if I2C_SLAVE > > + > > +config I2C_SLAVE_EEPROM > > + tristate "I2C eeprom slave driver" > > + > > +endif > > + > > config I2C_DEBUG_CORE > > bool "I2C Core debugging messages" > > help > > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile > > index 1722f50f2473..45095b3d16a9 100644 > > --- a/drivers/i2c/Makefile > > +++ b/drivers/i2c/Makefile > > @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o > > obj-$(CONFIG_I2C_MUX) += i2c-mux.o > > obj-y += algos/ busses/ muxes/ > > obj-$(CONFIG_I2C_STUB) += i2c-stub.o > > +obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o > > > > ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG > > CFLAGS_i2c-core.o := -Wno-deprecated-declarations > > diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c > > new file mode 100644 > > index 000000000000..6631400b5f02 > > --- /dev/null > > +++ b/drivers/i2c/i2c-slave-eeprom.c > > @@ -0,0 +1,170 @@ > > +/* > > + * I2C slave mode EEPROM simulator > > + * > > + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <wsa@sang-engineering.com> > > + * Copyright (C) 2014 by Renesas Electronics Corporation > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; version 2 of the License. > > + * > > + * Because most IP blocks can only detect one I2C slave address anyhow, this > > + * driver does not support simulating EEPROM types which take more than one > > + * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit > > + * pointer, yet implementation is deferred until the need actually arises. > > + */ > > + > > +#include <linux/i2c.h> > > +#include <linux/init.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/slab.h> > > +#include <linux/spinlock.h> > > +#include <linux/sysfs.h> > > + > > +struct eeprom_data { > > + struct bin_attribute bin; > > + bool first_write; > > + spinlock_t buffer_lock; > > + u8 buffer_idx; > > + u8 buffer[]; > > +}; > > + > > +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, > > + enum i2c_slave_event event, u8 *val) > > +{ > > + struct eeprom_data *eeprom = i2c_get_clientdata(client); > > + > > + switch (event) { > > + case I2C_SLAVE_REQ_WRITE_END: > > + if (eeprom->first_write) { > > + eeprom->buffer_idx = *val; > > + eeprom->first_write = false; > > + } else { > > + spin_lock(&eeprom->buffer_lock); > > + eeprom->buffer[eeprom->buffer_idx++] = *val; > > + spin_unlock(&eeprom->buffer_lock); > > + } > > + break; > > + > > + case I2C_SLAVE_REQ_READ_START: > > + spin_lock(&eeprom->buffer_lock); > > + *val = eeprom->buffer[eeprom->buffer_idx]; > > + spin_unlock(&eeprom->buffer_lock); > > + break; > > + > > + case I2C_SLAVE_REQ_READ_END: > > + eeprom->buffer_idx++; > You don't check here for buffer_idx >= ARRAY_SIZE(buffer)? > Ditto in the I2C_SLAVE_REQ_WRITE_END case. I just noticed that buffer_idx is an u8, so it overflows at 255+1. So the probe routine should error out if size is bigger than 256. Best regards Uwe
Hi, Please quote only relevant parts of the message (like I did). This improves readability a lot. > > +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, > > + enum i2c_slave_event event, u8 *val) > > +{ > > + struct eeprom_data *eeprom = i2c_get_clientdata(client); > > + > > + switch (event) { > > + case I2C_SLAVE_REQ_WRITE_END: > > + if (eeprom->first_write) { > > + eeprom->buffer_idx = *val; > > + eeprom->first_write = false; > > + } else { > > + spin_lock(&eeprom->buffer_lock); > > + eeprom->buffer[eeprom->buffer_idx++] = *val; > > + spin_unlock(&eeprom->buffer_lock); > > + } > > + break; > > + > > + case I2C_SLAVE_REQ_READ_START: > > + spin_lock(&eeprom->buffer_lock); > > + *val = eeprom->buffer[eeprom->buffer_idx]; > > + spin_unlock(&eeprom->buffer_lock); > > + break; > > + > > + case I2C_SLAVE_REQ_READ_END: > > + eeprom->buffer_idx++; > > + break; > > + > > + case I2C_SLAVE_STOP: > > + eeprom->first_write = true; > > + break; > > + > > + default: > > + break; > > + } > > + > > + return 0; > > +} > > > Would it make sense to have: > WRITE_START > WRITE_NEXT > WRITE_STOP > WRITE_REPEAT_START > READ_START > READ_NEXT > READ_STOP > READ_REPEAT_START > > Some devices may want different behavior for subsequent > reads when they are separate transactions, compared to > a single larger transaction. This can all be handled with I2C_SLAVE_STOP. > e.g. a single transaction may wraparound inside a >8bit > register (e.g. for 16bit: msb, lsb, msb, lsb, ...), but step > to the next register when a separate read/write is issued. > Alternatively, a WRITE/READ_NEXT may be implemented > more efficiently. This may not matter for current systems > compared to the low-frequency bus, but who knows what > IoT devices may bring to the table. Let's start simple until we have more use cases. WRITE_NEXT may be useful when you have an internal u8 pointer to be set before actually sending data, but already less useful if this is u16. Also, slaves may decide to handle stops at unexpected places differently. Furthermore, my feeling is that slave drivers will be more robust if they handle those simple primitives properly. What I could imagine, though, is that somewhen the eeprom simulator will be able to get data via other means than the shared memory, maybe via some callback. Then, the callback really only has to deal with "read this byte" or "write that byte" while for the outer world we have a proper well-known EEPROM like I2C interface. That being said, the list of I2C_SLAVE_* events is extensible, of course. Though, I rather see low-level stuff there like "General Call Adress received". > > Also, behavior may be different for repeat start versus > stop/start, although a repeat start could be a start > without a previous stop as well... IMO a repeated start is to ensure that two messages arrive at the slave without interruption from another master. I can't think why a slave should know which type of start that was. In fact, if it does that would raise an eyebrow for me. Do you have an example? > Of course, if an I2C adapter cannot distinguish these > events, this is probably a futile attempt at adding > semantics that will silently break depending on the > actual hardware/driver used. That as well. I have not seen an I2C adapter which could provide this information so far. Thanks, Wolfram
Hi Uwe, please don't quote so much :) > > > + case I2C_SLAVE_REQ_READ_END: > > > + eeprom->buffer_idx++; > > You don't check here for buffer_idx >= ARRAY_SIZE(buffer)? > > Ditto in the I2C_SLAVE_REQ_WRITE_END case. > I just noticed that buffer_idx is an u8, so it overflows at 255+1. So > the probe routine should error out if size is bigger than 256. But size is currently fixed to 256, so all is fine. Yes, if we extend it for bigger sizes, all that stuff needs to be taken care of, right.
> this mail is thematically more a reply to patch 1 and maybe just serves > my understanding of the slave support. Sure. This shows how badly needed the documentation is :) ... > > + break; > > + > > + case I2C_SLAVE_STOP: > > + eeprom->first_write = true; > > + break; > > + > > + default: > > + break; > > + } > > + > > + return 0; > > +} > This is the most interesting function here because it uses the new > interface, the functions below are only to update and show the simulated > eeprom contents and driver boilerplate, right? Yes. > When the eeprom driver is probed and the adapter driver notices a read > request for the respective i2c address, this callback is called with > event=I2C_SLAVE_REQ_READ_START. Returning 0 here and provide the first > byte to send make the adapter ack the read request and send the data > provided. If something != 0 is returned a NAK is sent? We only send NAK on write requests (I use read/write from the master perspective). Then, we have to say if the received byte was successfully processed. When reading, the master has to ack the successful reception of the byte. > How is the next byte requested from the slave driver? I assume with two > additional calls to the callback, first with > event=I2C_SLAVE_REQ_READ_END, then event=I2C_SLAVE_REQ_READ_START once > more. Would it make sense to reduce this to a single call? Does the > driver at READ_END time already know if its write got acked? If so, how? No single call. I had this first, but my experiments showed that it is important for the EEPROM driver to only increase the internal pointer when the byte was ACKed. Otherwise, I was off-by-one. Ideally, I2C_SLAVE_REQ_READ_END should be used when the master ACKed the byte, right. However, the rcar hardware doesn't have an interrupt for this, so I imply that the start of a new read request ends the old one. I probably should add a comment for that. > This means that for each byte the callback is called. Would it make > sense to make the API more flexible and allow the slave driver to return > a buffer? This would remove some callback overhead and might allow to > let the adapter driver make use of its DMA mechanism. For DMA, I haven't seen DMA slave support yet. Makes sense to me, we wouldn't know the transfer size, since the master can send a stop anytime. This makes possible gains of using a buffer also speculative. Also, I2C is still a low-bandwith bus, so usually we have a high number of small transfers. For now, I'd skip this idea. As I said in another thread, we need more use cases. If the need arises, we can come up with something. I don't think the current design prevents such an addition? Thanks, Wolfram
Hallo Wolfram, On Sat, Nov 22, 2014 at 07:14:06PM +0100, Wolfram Sang wrote: > > > > + case I2C_SLAVE_REQ_READ_END: > > > > + eeprom->buffer_idx++; > > > You don't check here for buffer_idx >= ARRAY_SIZE(buffer)? > > > Ditto in the I2C_SLAVE_REQ_WRITE_END case. > > I just noticed that buffer_idx is an u8, so it overflows at 255+1. So > > the probe routine should error out if size is bigger than 256. > > But size is currently fixed to 256, so all is fine. Yes, if we extend it > for bigger sizes, all that stuff needs to be taken care of, right. yeah, it's well hidden, but true. So IMHO worth a comment. Best regards Uwe
Hello Wolfram, On Sat, Nov 22, 2014 at 07:26:30PM +0100, Wolfram Sang wrote: > > > this mail is thematically more a reply to patch 1 and maybe just serves > > my understanding of the slave support. > > Sure. This shows how badly needed the documentation is :) > > ... > > > + break; > > > + > > > + case I2C_SLAVE_STOP: > > > + eeprom->first_write = true; > > > + break; > > > + > > > + default: > > > + break; > > > + } > > > + > > > + return 0; > > > +} > > This is the most interesting function here because it uses the new > > interface, the functions below are only to update and show the simulated > > eeprom contents and driver boilerplate, right? > > Yes. > > > When the eeprom driver is probed and the adapter driver notices a read > > request for the respective i2c address, this callback is called with > > event=I2C_SLAVE_REQ_READ_START. Returning 0 here and provide the first > > byte to send make the adapter ack the read request and send the data > > provided. If something != 0 is returned a NAK is sent? > > We only send NAK on write requests (I use read/write from the master I don't understand this. Who is "we" in this case? > perspective). Then, we have to say if the received byte was successfully > processed. When reading, the master has to ack the successful reception > of the byte. Right, I got this wrong in my question. On a read request (as seen from the master) the master has to ack. > > How is the next byte requested from the slave driver? I assume with two > > additional calls to the callback, first with > > event=I2C_SLAVE_REQ_READ_END, then event=I2C_SLAVE_REQ_READ_START once > > more. Would it make sense to reduce this to a single call? Does the > > driver at READ_END time already know if its write got acked? If so, how? > > No single call. I had this first, but my experiments showed that it is > important for the EEPROM driver to only increase the internal pointer > when the byte was ACKed. Otherwise, I was off-by-one. Sure, the driver has to know how his read response was received by the master. But assuming I understand your abstraction right there is some redundancy. There are only three cases on a read request (well plus error handling): - master sends ACK, reads next byte in this case the slave must provide another word In your abstraction this implies callback(I2C_SLAVE_REQ_READ_END); <-- this is redundant callback(I2C_SLAVE_REQ_READ_START); - master sends ACK, then P or Sr callback(I2C_SLAVE_REQ_READ_END); maybe callback(I2C_SLAVE_STOP) - master sends NACK in this case the message ends and the master has to send Sr or P. In your case this results in: nothing for the NACK? maybe callback(I2C_SLAVE_STOP) The situations where the slave has to react are: - slave was addressed with R input: address output: NAK or (ACK + data byte) - slave was addressed with #W: input: address output: NAK or ACK - data sent by slave-transmitter was acked output: next data byte (maybe unused because master sends Sr or P) - data sent by slave-transmitter was nacked output: void (unless we want to support IGNORE_NAK :-) - slave received a data byte (write) input: data output: NAK or ACK This looks like a better model in my eyes. In this model the slave driver doesn't even need to be informed about P. Not entirely sure about "data sent by slave-transmitter was nacked". > Ideally, I2C_SLAVE_REQ_READ_END should be used when the master ACKed the > byte, right. However, the rcar hardware doesn't have an interrupt for > this, so I imply that the start of a new read request ends the old one. > I probably should add a comment for that. > > > This means that for each byte the callback is called. Would it make > > sense to make the API more flexible and allow the slave driver to return > > a buffer? This would remove some callback overhead and might allow to > > let the adapter driver make use of its DMA mechanism. > > For DMA, I haven't seen DMA slave support yet. Makes sense to me, we haha, there is only a single slave driver yet and you're the author. > wouldn't know the transfer size, since the master can send a stop > anytime. This makes possible gains of using a buffer also speculative. > Also, I2C is still a low-bandwith bus, so usually we have a high number > of small transfers. > > For now, I'd skip this idea. As I said in another thread, we need more > use cases. If the need arises, we can come up with something. I don't > think the current design prevents such an addition? It would change the API, but starting to get experience with byte banging is probably OK. Best regards Uwe
> > > When the eeprom driver is probed and the adapter driver notices a read > > > request for the respective i2c address, this callback is called with > > > event=I2C_SLAVE_REQ_READ_START. Returning 0 here and provide the first > > > byte to send make the adapter ack the read request and send the data > > > provided. If something != 0 is returned a NAK is sent? > > > > We only send NAK on write requests (I use read/write from the master > I don't understand this. Who is "we" in this case? We as the slave serving the request of a remote master. > > No single call. I had this first, but my experiments showed that it is > > important for the EEPROM driver to only increase the internal pointer > > when the byte was ACKed. Otherwise, I was off-by-one. > Sure, the driver has to know how his read response was received by the > master. But assuming I understand your abstraction right there is some > redundancy. There are only three cases on a read request (well plus error > handling): > > - master sends ACK, reads next byte > in this case the slave must provide another word > In your abstraction this implies > callback(I2C_SLAVE_REQ_READ_END); <-- this is redundant Well, in the way that a new start of something means automatically the end of the previous something, like some closing html tags which can be left out. However, I think being explicit here isn't much of a drawback. You lose a little bit of performance and gain flexibility. A sensor-like device could decide that on READ_END it is safe to discard the old value and start acquiring the new one, so it will be available on next READ_START. Be aware that the master decides how much time there will be between END and START (although it will be very close in most cases). > callback(I2C_SLAVE_REQ_READ_START); > > - master sends ACK, then P or Sr > callback(I2C_SLAVE_REQ_READ_END); > maybe callback(I2C_SLAVE_STOP) > > - master sends NACK > in this case the message ends and the master has to send Sr or P. > In your case this results in: > nothing for the NACK? > maybe callback(I2C_SLAVE_STOP) > > The situations where the slave has to react are: > > - slave was addressed with R > input: address > output: NAK or (ACK + data byte) > - slave was addressed with #W: > input: address > output: NAK or ACK On most slave IP cores I have seen so far, there is nothing to do for a slave driver for those two. Address recognition and sending ACK are all done by the hardware and you are notified of that by an interrupt. For the read case, you have to deliver the first byte, of course. > - data sent by slave-transmitter was acked > output: next data byte (maybe unused because master sends Sr or P) The driver I modified in the next patch cannot know if the master acked/nacked something. It just knows if the output buffer is empty or if a stop was signalled (which really should come after NAK from the master). So, we can't depend on this difference. > - data sent by slave-transmitter was nacked > output: void (unless we want to support IGNORE_NAK :-) :) > - slave received a data byte (write) > input: data > output: NAK or ACK > > This looks like a better model in my eyes. In this model the slave > driver doesn't even need to be informed about P. Not entirely sure about > "data sent by slave-transmitter was nacked". I think we definately need a STOP notification. Devices might want to do something like reducing power etc after stop. What I like about my version is that it reflects pretty closely what happens on the bus. I can't currently tell what somebody could do with WRITE_START (precharging something?). But it feels better to me to directly reflect what a real slave device would see, too, for flexibility. > > > For DMA, I haven't seen DMA slave support yet. Makes sense to me, we > haha, there is only a single slave driver yet and you're the author. I meant IP cores here which support DMA for slave transmissions. Regardings buffers: I wouldn't like all bus drivers to have code handling the buffer when all they have to do is to put one byte on the bus. So, buffer handling should go into the core then probably. So, there'd be still one level of indirection? Also, since we don't know when the master will collect the data, there is a chance it might get stale? To me that sounds like too much complexity for too litle gain. At this moment, at least. Regards, Wolfram
On Sat, Nov 22, 2014 at 7:12 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > Hi, > > Please quote only relevant parts of the message (like I did). This > improves readability a lot. > My bad, will minimize the overhead in the future. >> >> Would it make sense to have: >> WRITE_START >> WRITE_NEXT >> WRITE_STOP >> WRITE_REPEAT_START >> READ_START >> READ_NEXT >> READ_STOP >> READ_REPEAT_START >> >> Some devices may want different behavior for subsequent >> reads when they are separate transactions, compared to >> a single larger transaction. > > This can all be handled with I2C_SLAVE_STOP. > I think the goal is probably better covered by Uwe's idea for returning a buffer. See below... >> e.g. a single transaction may wraparound inside a >8bit >> register (e.g. for 16bit: msb, lsb, msb, lsb, ...), but step >> to the next register when a separate read/write is issued. >> Alternatively, a WRITE/READ_NEXT may be implemented >> more efficiently. This may not matter for current systems >> compared to the low-frequency bus, but who knows what >> IoT devices may bring to the table. > > Let's start simple until we have more use cases. WRITE_NEXT may be > useful when you have an internal u8 pointer to be set before actually > sending data, but already less useful if this is u16. Also, slaves may > decide to handle stops at unexpected places differently. Furthermore, my > feeling is that slave drivers will be more robust if they handle those > simple primitives properly. > I think usable semantics could be that a slave driver can indicate at any point during an I2C read transaction that the next X bytes should be returned from a buffer and for a write transaction, that the next X bytes should be buffered before passed on to the driver. When the transaction ends or the adapter reaches the end of the buffer, it indicates this to the driver which can then either cleanup or otherwise act accordingly. For the EEPROM driver, this could mean that after the address is received, the driver can pass the pointer + remaining size and have the adapter driver take care of transmit. Upon end-of-buffer, it could just handle the wraparound and let the adapter take care of the rest. But as you say, this can be added later on when the need arises. > What I could imagine, though, is that somewhen the eeprom simulator will > be able to get data via other means than the shared memory, maybe via > some callback. Then, the callback really only has to deal with "read > this byte" or "write that byte" while for the outer world we have a > proper well-known EEPROM like I2C interface. > > That being said, the list of I2C_SLAVE_* events is extensible, of > course. Though, I rather see low-level stuff there like "General Call > Adress received". > >> >> Also, behavior may be different for repeat start versus >> stop/start, although a repeat start could be a start >> without a previous stop as well... > > IMO a repeated start is to ensure that two messages arrive at the slave > without interruption from another master. I can't think why a slave > should know which type of start that was. In fact, if it does that would > raise an eyebrow for me. Do you have an example? > No, I'll have to pass on that one. Plenty of odd I2C behavior, but that's not one of them. You're right on repeat start, so it would be quite erratic behavior. But even if, it can probably be added later on. >> Of course, if an I2C adapter cannot distinguish these >> events, this is probably a futile attempt at adding >> semantics that will silently break depending on the >> actual hardware/driver used. > > That as well. I have not seen an I2C adapter which could provide this > information so far. > Just checked components we're using and stop/repeated-start both transition into the same state. Regards, Stijn > Thanks, > > Wolfram > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> My bad, will minimize the overhead in the future. Thanks! > I think usable semantics could be that a slave driver can indicate at any > point during an I2C read transaction that the next X bytes should be > returned from a buffer and for a write transaction, that the next X bytes > should be buffered before passed on to the driver. When the transaction Yes, if we have something like that it should definately be that the slave driver offers a buffer and the bus driver may accept the buffer or not. Buffers should be opt-in and slave drivers will always have to support byte-based transactions as the ultimate fallback because this is how the vast majority of HW works (at least those I checked). That makes me really wonder if there will be a case where performance is so important that buffer handling is coded on top of the always needed byte based handling? More so, I'd think most slaves will have stuff like registers. So, buffered writes usually won't make sense because setting bits should cause an action immediately. And reading bits will have the problem of data getting stale. I'd think this is the biggest use case of I2C. Except for reading an EEPROM _once_ ;) > ends or the adapter reaches the end of the buffer, it indicates this to > the driver which can then either cleanup or otherwise act accordingly. Or the adapter encounters an unexpected STOP. Cleanups need to handle interrupted transactions as well. I'd think this is a bit more error prone, because people would need to check a return value and compare it to the number of bytes they wanted. Not a show stopper, but also nothing I want to throw in too easily. > But as you say, this can be added later on when the need arises. I'd really say so. > Just checked components we're using and stop/repeated-start both > transition into the same state. Good, thanks for checking! May I ask which components you are using? Wolfram
22 нояб. 2014 г., в 21:12, Wolfram Sang <wsa@the-dreams.de> написал(а): > IMO a repeated start is to ensure that two messages arrive at the slave > without interruption from another master. I can't think why a slave > should know which type of start that was. In fact, if it does that would > raise an eyebrow for me. Do you have an example? It is used to implement Device ID reading. Not sure, that the feature is really needed for the first release. See [1] "3.1.17 Device ID" chapter on page "20 of 64" See [2] "7.2.2 Device ID (PCA9671 ID field)" chapter [1] UM10204 I2C-bus specification and user manual (http://www.nxp.com/documents/user_manual/UM10204.pdf) [2] PCA9671 (http://www.nxp.com/documents/data_sheet/PCA9671.pdf) Alexander. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 26, 2014 at 03:25:29PM +0300, Alexander Kochetkov wrote: > > 22 нояб. 2014 г., в 21:12, Wolfram Sang <wsa@the-dreams.de> написал(а): > > > IMO a repeated start is to ensure that two messages arrive at the slave > > without interruption from another master. I can't think why a slave > > should know which type of start that was. In fact, if it does that would > > raise an eyebrow for me. Do you have an example? > > It is used to implement Device ID reading. Yes and no, I'd say :) Technically, it needs repeated start. But really, the hardware should handle this (and I know one IP core which has support for it). One could try to simulate the behaviour in the bus driver IF a second slave address AND detection of repeated start is available, but I doubt this combination exists. Still, all the _slave driver_ needs to handle is an I2C_SLAVE_EVENT_DEVICE_ID which should return the apropriate value. That being said, I have never seen querying Device ID in action. > Not sure, that the feature is really needed for the first release. I am sure it is not :D Thanks, Wolfram
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index b51a402752c4..8c9e619f3026 100644 --- a/drivers/i2c/Kconfig +++ b/drivers/i2c/Kconfig @@ -110,6 +110,16 @@ config I2C_STUB If you don't know what to do here, definitely say N. +config I2C_SLAVE + bool "I2C slave support" + +if I2C_SLAVE + +config I2C_SLAVE_EEPROM + tristate "I2C eeprom slave driver" + +endif + config I2C_DEBUG_CORE bool "I2C Core debugging messages" help diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index 1722f50f2473..45095b3d16a9 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o obj-$(CONFIG_I2C_MUX) += i2c-mux.o obj-y += algos/ busses/ muxes/ obj-$(CONFIG_I2C_STUB) += i2c-stub.o +obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG CFLAGS_i2c-core.o := -Wno-deprecated-declarations diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c new file mode 100644 index 000000000000..6631400b5f02 --- /dev/null +++ b/drivers/i2c/i2c-slave-eeprom.c @@ -0,0 +1,170 @@ +/* + * I2C slave mode EEPROM simulator + * + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <wsa@sang-engineering.com> + * Copyright (C) 2014 by Renesas Electronics Corporation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; version 2 of the License. + * + * Because most IP blocks can only detect one I2C slave address anyhow, this + * driver does not support simulating EEPROM types which take more than one + * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit + * pointer, yet implementation is deferred until the need actually arises. + */ + +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/sysfs.h> + +struct eeprom_data { + struct bin_attribute bin; + bool first_write; + spinlock_t buffer_lock; + u8 buffer_idx; + u8 buffer[]; +}; + +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, + enum i2c_slave_event event, u8 *val) +{ + struct eeprom_data *eeprom = i2c_get_clientdata(client); + + switch (event) { + case I2C_SLAVE_REQ_WRITE_END: + if (eeprom->first_write) { + eeprom->buffer_idx = *val; + eeprom->first_write = false; + } else { + spin_lock(&eeprom->buffer_lock); + eeprom->buffer[eeprom->buffer_idx++] = *val; + spin_unlock(&eeprom->buffer_lock); + } + break; + + case I2C_SLAVE_REQ_READ_START: + spin_lock(&eeprom->buffer_lock); + *val = eeprom->buffer[eeprom->buffer_idx]; + spin_unlock(&eeprom->buffer_lock); + break; + + case I2C_SLAVE_REQ_READ_END: + eeprom->buffer_idx++; + break; + + case I2C_SLAVE_STOP: + eeprom->first_write = true; + break; + + default: + break; + } + + return 0; +} + +static ssize_t i2c_slave_eeprom_bin_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, char *buf, loff_t off, size_t count) +{ + struct eeprom_data *eeprom; + unsigned long flags; + + if (off + count >= attr->size) + return -EFBIG; + + eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj)); + + spin_lock_irqsave(&eeprom->buffer_lock, flags); + memcpy(buf, &eeprom->buffer[off], count); + spin_unlock_irqrestore(&eeprom->buffer_lock, flags); + + return count; +} + +static ssize_t i2c_slave_eeprom_bin_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, char *buf, loff_t off, size_t count) +{ + struct eeprom_data *eeprom; + unsigned long flags; + + if (off + count >= attr->size) + return -EFBIG; + + eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj)); + + spin_lock_irqsave(&eeprom->buffer_lock, flags); + memcpy(&eeprom->buffer[off], buf, count); + spin_unlock_irqrestore(&eeprom->buffer_lock, flags); + + return count; +} + +static int i2c_slave_eeprom_probe(struct i2c_client *client, const struct i2c_device_id *id) +{ + struct eeprom_data *eeprom; + int ret; + unsigned size = id->driver_data; + + eeprom = devm_kzalloc(&client->dev, sizeof(struct eeprom_data) + size, GFP_KERNEL); + if (!eeprom) + return -ENOMEM; + + eeprom->first_write = true; + spin_lock_init(&eeprom->buffer_lock); + i2c_set_clientdata(client, eeprom); + + sysfs_bin_attr_init(&eeprom->bin); + eeprom->bin.attr.name = "slave-eeprom"; + eeprom->bin.attr.mode = S_IRUSR | S_IWUSR; + eeprom->bin.read = i2c_slave_eeprom_bin_read; + eeprom->bin.write = i2c_slave_eeprom_bin_write; + eeprom->bin.size = size; + + ret = sysfs_create_bin_file(&client->dev.kobj, &eeprom->bin); + if (ret) + return ret; + + ret = i2c_slave_register(client, i2c_slave_eeprom_slave_cb); + if (ret) { + sysfs_remove_bin_file(&client->dev.kobj, &eeprom->bin); + return ret; + } + + return 0; +}; + +static int i2c_slave_eeprom_remove(struct i2c_client *client) +{ + struct eeprom_data *eeprom = i2c_get_clientdata(client); + + i2c_slave_unregister(client); + sysfs_remove_bin_file(&client->dev.kobj, &eeprom->bin); + + return 0; +} + +static const struct i2c_device_id i2c_slave_eeprom_id[] = { + { "slave-24c02", 2048 / 8 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, i2c_slave_eeprom_id); + +static struct i2c_driver i2c_slave_eeprom_driver = { + .driver = { + .name = "i2c-slave-eeprom", + .owner = THIS_MODULE, + }, + .probe = i2c_slave_eeprom_probe, + .remove = i2c_slave_eeprom_remove, + .id_table = i2c_slave_eeprom_id, +}; +module_i2c_driver(i2c_slave_eeprom_driver); + +MODULE_AUTHOR("Wolfram Sang <wsa@sang-engineering.com>"); +MODULE_DESCRIPTION("I2C slave mode EEPROM simulator"); +MODULE_LICENSE("GPL v2");