| Message ID | 1509378506-30851-1-git-send-email-svendev@arcx.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | at24: support eeproms that do not roll over page reads. | expand |
Hi Sven,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.14-rc7]
[cannot apply to next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sven-Van-Asbroeck/at24-support-eeproms-that-do-not-roll-over-page-reads/20171101-114231
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All warnings (new ones prefixed by >>):
In file included from drivers/misc/eeprom/at24.c:12:0:
drivers/misc/eeprom/at24.c: In function 'at24_translate_offset':
include/linux/kernel.h:790:16: warning: comparison of distinct pointer types lacks a cast
(void) (&min1 == &min2); \
^
include/linux/kernel.h:799:2: note: in expansion of macro '__min'
__min(typeof(x), typeof(y), \
^~~~~
>> drivers/misc/eeprom/at24.c:210:12: note: in expansion of macro 'min'
*count = min(*count, remainder);
^~~
vim +/min +210 drivers/misc/eeprom/at24.c
> 12 #include <linux/kernel.h>
13 #include <linux/init.h>
14 #include <linux/module.h>
15 #include <linux/slab.h>
16 #include <linux/delay.h>
17 #include <linux/mutex.h>
18 #include <linux/mod_devicetable.h>
19 #include <linux/log2.h>
20 #include <linux/bitops.h>
21 #include <linux/jiffies.h>
22 #include <linux/property.h>
23 #include <linux/acpi.h>
24 #include <linux/i2c.h>
25 #include <linux/nvmem-provider.h>
26 #include <linux/platform_data/at24.h>
27
28 /*
29 * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
30 * Differences between different vendor product lines (like Atmel AT24C or
31 * MicroChip 24LC, etc) won't much matter for typical read/write access.
32 * There are also I2C RAM chips, likewise interchangeable. One example
33 * would be the PCF8570, which acts like a 24c02 EEPROM (256 bytes).
34 *
35 * However, misconfiguration can lose data. "Set 16-bit memory address"
36 * to a part with 8-bit addressing will overwrite data. Writing with too
37 * big a page size also loses data. And it's not safe to assume that the
38 * conventional addresses 0x50..0x57 only hold eeproms; a PCF8563 RTC
39 * uses 0x51, for just one example.
40 *
41 * Accordingly, explicit board-specific configuration data should be used
42 * in almost all cases. (One partial exception is an SMBus used to access
43 * "SPD" data for DRAM sticks. Those only use 24c02 EEPROMs.)
44 *
45 * So this driver uses "new style" I2C driver binding, expecting to be
46 * told what devices exist. That may be in arch/X/mach-Y/board-Z.c or
47 * similar kernel-resident tables; or, configuration data coming from
48 * a bootloader.
49 *
50 * Other than binding model, current differences from "eeprom" driver are
51 * that this one handles write access and isn't restricted to 24c02 devices.
52 * It also handles larger devices (32 kbit and up) with two-byte addresses,
53 * which won't work on pure SMBus systems.
54 */
55
56 struct at24_data {
57 struct at24_platform_data chip;
58 int use_smbus;
59 int use_smbus_write;
60
61 ssize_t (*read_func)(struct at24_data *, char *, unsigned int, size_t);
62 ssize_t (*write_func)(struct at24_data *,
63 const char *, unsigned int, size_t);
64
65 /*
66 * Lock protects against activities from other Linux tasks,
67 * but not from changes by other I2C masters.
68 */
69 struct mutex lock;
70
71 u8 *writebuf;
72 unsigned write_max;
73 unsigned num_addresses;
74
75 struct nvmem_config nvmem_config;
76 struct nvmem_device *nvmem;
77
78 /*
79 * Some chips tie up multiple I2C addresses; dummy devices reserve
80 * them for us, and we'll use them with SMBus calls.
81 */
82 struct i2c_client *client[];
83 };
84
85 /*
86 * This parameter is to help this driver avoid blocking other drivers out
87 * of I2C for potentially troublesome amounts of time. With a 100 kHz I2C
88 * clock, one 256 byte read takes about 1/43 second which is excessive;
89 * but the 1/170 second it takes at 400 kHz may be quite reasonable; and
90 * at 1 MHz (Fm+) a 1/430 second delay could easily be invisible.
91 *
92 * This value is forced to be a power of two so that writes align on pages.
93 */
94 static unsigned io_limit = 128;
95 module_param(io_limit, uint, 0);
96 MODULE_PARM_DESC(io_limit, "Maximum bytes per I/O (default 128)");
97
98 /*
99 * Specs often allow 5 msec for a page write, sometimes 20 msec;
100 * it's important to recover from write timeouts.
101 */
102 static unsigned write_timeout = 25;
103 module_param(write_timeout, uint, 0);
104 MODULE_PARM_DESC(write_timeout, "Time (in ms) to try writes (default 25)");
105
106 #define AT24_SIZE_BYTELEN 5
107 #define AT24_SIZE_FLAGS 8
108
109 #define AT24_BITMASK(x) (BIT(x) - 1)
110
111 /* create non-zero magic value for given eeprom parameters */
112 #define AT24_DEVICE_MAGIC(_len, _flags) \
113 ((1 << AT24_SIZE_FLAGS | (_flags)) \
114 << AT24_SIZE_BYTELEN | ilog2(_len))
115
116 /*
117 * Both reads and writes fail if the previous write didn't complete yet. This
118 * macro loops a few times waiting at least long enough for one entire page
119 * write to work while making sure that at least one iteration is run before
120 * checking the break condition.
121 *
122 * It takes two parameters: a variable in which the future timeout in jiffies
123 * will be stored and a temporary variable holding the time of the last
124 * iteration of processing the request. Both should be unsigned integers
125 * holding at least 32 bits.
126 */
127 #define loop_until_timeout(tout, op_time) \
128 for (tout = jiffies + msecs_to_jiffies(write_timeout), op_time = 0; \
129 op_time ? time_before(op_time, tout) : true; \
130 usleep_range(1000, 1500), op_time = jiffies)
131
132 static const struct i2c_device_id at24_ids[] = {
133 /* needs 8 addresses as A0-A2 are ignored */
134 { "24c00", AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) },
135 /* old variants can't be handled with this generic entry! */
136 { "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
137 { "24cs01", AT24_DEVICE_MAGIC(16,
138 AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
139 { "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
140 { "24cs02", AT24_DEVICE_MAGIC(16,
141 AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
142 { "24mac402", AT24_DEVICE_MAGIC(48 / 8,
143 AT24_FLAG_MAC | AT24_FLAG_READONLY) },
144 { "24mac602", AT24_DEVICE_MAGIC(64 / 8,
145 AT24_FLAG_MAC | AT24_FLAG_READONLY) },
146 /* spd is a 24c02 in memory DIMMs */
147 { "spd", AT24_DEVICE_MAGIC(2048 / 8,
148 AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
149 { "24c04", AT24_DEVICE_MAGIC(4096 / 8, 0) },
150 { "24cs04", AT24_DEVICE_MAGIC(16,
151 AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
152 /* 24rf08 quirk is handled at i2c-core */
153 { "24c08", AT24_DEVICE_MAGIC(8192 / 8, 0) },
154 { "24cs08", AT24_DEVICE_MAGIC(16,
155 AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
156 { "24c16", AT24_DEVICE_MAGIC(16384 / 8, 0) },
157 { "24cs16", AT24_DEVICE_MAGIC(16,
158 AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
159 { "24c32", AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) },
160 { "24cs32", AT24_DEVICE_MAGIC(16,
161 AT24_FLAG_ADDR16 |
162 AT24_FLAG_SERIAL |
163 AT24_FLAG_READONLY) },
164 { "24c64", AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) },
165 { "24cs64", AT24_DEVICE_MAGIC(16,
166 AT24_FLAG_ADDR16 |
167 AT24_FLAG_SERIAL |
168 AT24_FLAG_READONLY) },
169 { "24c128", AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) },
170 { "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
171 { "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
172 { "24c1024", AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16) },
173 { "at24", 0 },
174 { /* END OF LIST */ }
175 };
176 MODULE_DEVICE_TABLE(i2c, at24_ids);
177
178 static const struct acpi_device_id at24_acpi_ids[] = {
179 { "INT3499", AT24_DEVICE_MAGIC(8192 / 8, 0) },
180 { }
181 };
182 MODULE_DEVICE_TABLE(acpi, at24_acpi_ids);
183
184 /*-------------------------------------------------------------------------*/
185
186 /*
187 * This routine supports chips which consume multiple I2C addresses. It
188 * computes the addressing information to be used for a given r/w request.
189 * Assumes that sanity checks for offset happened at sysfs-layer.
190 *
191 * Slave address and byte offset derive from the offset. Always
192 * set the byte address; on a multi-master board, another master
193 * may have changed the chip's "current" address pointer.
194 *
195 * In case of chips that don't rollover page reads, truncate the count
196 * to the nearest page boundary. This might result in the
197 * at24_eeprom_read_XXX functions reading fewer bytes than requested,
198 * but this is compensated for in at24_read().
199 */
200 static struct i2c_client *at24_translate_offset(struct at24_data *at24,
201 unsigned int *offset, size_t *count)
202 {
203 unsigned int i, bits, remainder;
204
205 bits = (at24->chip.flags & AT24_FLAG_ADDR16) ? 16 : 8;
206 i = *offset >> bits;
207 *offset &= AT24_BITMASK(bits);
208 if ((at24->chip.flags & AT24_FLAG_NO_RDROL) && count) {
209 remainder = BIT(bits) - *offset;
> 210 *count = min(*count, remainder);
211 }
212
213 return at24->client[i];
214 }
215
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Sven,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.14-rc7]
[cannot apply to next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sven-Van-Asbroeck/at24-support-eeproms-that-do-not-roll-over-page-reads/20171101-114231
config: tile-allyesconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile
All warnings (new ones prefixed by >>):
drivers/misc/eeprom/at24.c: In function 'at24_translate_offset':
>> drivers/misc/eeprom/at24.c:210:12: warning: comparison of distinct pointer types lacks a cast [enabled by default]
vim +210 drivers/misc/eeprom/at24.c
185
186 /*
187 * This routine supports chips which consume multiple I2C addresses. It
188 * computes the addressing information to be used for a given r/w request.
189 * Assumes that sanity checks for offset happened at sysfs-layer.
190 *
191 * Slave address and byte offset derive from the offset. Always
192 * set the byte address; on a multi-master board, another master
193 * may have changed the chip's "current" address pointer.
194 *
195 * In case of chips that don't rollover page reads, truncate the count
196 * to the nearest page boundary. This might result in the
197 * at24_eeprom_read_XXX functions reading fewer bytes than requested,
198 * but this is compensated for in at24_read().
199 */
200 static struct i2c_client *at24_translate_offset(struct at24_data *at24,
201 unsigned int *offset, size_t *count)
202 {
203 unsigned int i, bits, remainder;
204
205 bits = (at24->chip.flags & AT24_FLAG_ADDR16) ? 16 : 8;
206 i = *offset >> bits;
207 *offset &= AT24_BITMASK(bits);
208 if ((at24->chip.flags & AT24_FLAG_NO_RDROL) && count) {
209 remainder = BIT(bits) - *offset;
> 210 *count = min(*count, remainder);
211 }
212
213 return at24->client[i];
214 }
215
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Sven,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.14-rc7]
[cannot apply to next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sven-Van-Asbroeck/at24-support-eeproms-that-do-not-roll-over-page-reads/20171101-114231
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
vim +210 drivers/misc/eeprom/at24.c
185
186 /*
187 * This routine supports chips which consume multiple I2C addresses. It
188 * computes the addressing information to be used for a given r/w request.
189 * Assumes that sanity checks for offset happened at sysfs-layer.
190 *
191 * Slave address and byte offset derive from the offset. Always
192 * set the byte address; on a multi-master board, another master
193 * may have changed the chip's "current" address pointer.
194 *
195 * In case of chips that don't rollover page reads, truncate the count
196 * to the nearest page boundary. This might result in the
197 * at24_eeprom_read_XXX functions reading fewer bytes than requested,
198 * but this is compensated for in at24_read().
199 */
200 static struct i2c_client *at24_translate_offset(struct at24_data *at24,
201 unsigned int *offset, size_t *count)
202 {
203 unsigned int i, bits, remainder;
204
205 bits = (at24->chip.flags & AT24_FLAG_ADDR16) ? 16 : 8;
206 i = *offset >> bits;
207 *offset &= AT24_BITMASK(bits);
208 if ((at24->chip.flags & AT24_FLAG_NO_RDROL) && count) {
209 remainder = BIT(bits) - *offset;
> 210 *count = min(*count, remainder);
211 }
212
213 return at24->client[i];
214 }
215
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt index afc0458..301bc7e 100644 --- a/Documentation/devicetree/bindings/eeprom/eeprom.txt +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt @@ -36,6 +36,11 @@ Optional properties: - read-only: this parameterless property disables writes to the eeprom + - no-read-rollover: supported on the at24 eeprom family only. + This parameterless property indicates that the + eeprom does not support auto read rollover. Please consult + the manual of your device. + Example: eeprom@52 { diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index 764ff5df..ef4197c 100644 --- a/drivers/misc/eeprom/at24.c +++ b/drivers/misc/eeprom/at24.c @@ -192,26 +192,22 @@ struct at24_data { * set the byte address; on a multi-master board, another master * may have changed the chip's "current" address pointer. * - * REVISIT some multi-address chips don't rollover page reads to - * the next slave address, so we may need to truncate the count. - * Those chips might need another quirk flag. - * - * If the real hardware used four adjacent 24c02 chips and that - * were misconfigured as one 24c08, that would be a similar effect: - * one "eeprom" file not four, but larger reads would fail when - * they crossed certain pages. + * In case of chips that don't rollover page reads, truncate the count + * to the nearest page boundary. This might result in the + * at24_eeprom_read_XXX functions reading fewer bytes than requested, + * but this is compensated for in at24_read(). */ static struct i2c_client *at24_translate_offset(struct at24_data *at24, - unsigned int *offset) + unsigned int *offset, size_t *count) { - unsigned i; - - if (at24->chip.flags & AT24_FLAG_ADDR16) { - i = *offset >> 16; - *offset &= 0xffff; - } else { - i = *offset >> 8; - *offset &= 0xff; + unsigned int i, bits, remainder; + + bits = (at24->chip.flags & AT24_FLAG_ADDR16) ? 16 : 8; + i = *offset >> bits; + *offset &= AT24_BITMASK(bits); + if ((at24->chip.flags & AT24_FLAG_NO_RDROL) && count) { + remainder = BIT(bits) - *offset; + *count = min(*count, remainder); } return at24->client[i]; @@ -224,7 +220,7 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf, struct i2c_client *client; int status; - client = at24_translate_offset(at24, &offset); + client = at24_translate_offset(at24, &offset, &count); if (count > io_limit) count = io_limit; @@ -258,7 +254,7 @@ static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf, u8 msgbuf[2]; memset(msg, 0, sizeof(msg)); - client = at24_translate_offset(at24, &offset); + client = at24_translate_offset(at24, &offset, &count); if (count > io_limit) count = io_limit; @@ -307,7 +303,7 @@ static ssize_t at24_eeprom_read_serial(struct at24_data *at24, char *buf, u8 addrbuf[2]; int status; - client = at24_translate_offset(at24, &offset); + client = at24_translate_offset(at24, &offset, &count); memset(msg, 0, sizeof(msg)); msg[0].addr = client->addr; @@ -360,7 +356,7 @@ static ssize_t at24_eeprom_read_mac(struct at24_data *at24, char *buf, u8 addrbuf[2]; int status; - client = at24_translate_offset(at24, &offset); + client = at24_translate_offset(at24, &offset, &count); memset(msg, 0, sizeof(msg)); msg[0].addr = client->addr; @@ -415,7 +411,7 @@ static ssize_t at24_eeprom_write_smbus_block(struct at24_data *at24, struct i2c_client *client; ssize_t status = 0; - client = at24_translate_offset(at24, &offset); + client = at24_translate_offset(at24, &offset, NULL); count = at24_adjust_write_count(at24, offset, count); loop_until_timeout(timeout, write_time) { @@ -442,7 +438,7 @@ static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24, struct i2c_client *client; ssize_t status = 0; - client = at24_translate_offset(at24, &offset); + client = at24_translate_offset(at24, &offset, &count); loop_until_timeout(timeout, write_time) { status = i2c_smbus_write_byte_data(client, offset, buf[0]); @@ -468,7 +464,7 @@ static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf, ssize_t status = 0; int i = 0; - client = at24_translate_offset(at24, &offset); + client = at24_translate_offset(at24, &offset, NULL); count = at24_adjust_write_count(at24, offset, count); msg.addr = client->addr; @@ -569,6 +565,8 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip) if (device_property_present(dev, "read-only")) chip->flags |= AT24_FLAG_READONLY; + if (device_property_present(dev, "no-read-rollover")) + chip->flags |= AT24_FLAG_NO_RDROL; err = device_property_read_u32(dev, "pagesize", &val); if (!err) { diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h index 271a4e2..a5804f1 100644 --- a/include/linux/platform_data/at24.h +++ b/include/linux/platform_data/at24.h @@ -50,6 +50,7 @@ struct at24_platform_data { #define AT24_FLAG_TAKE8ADDR BIT(4) /* take always 8 addresses (24c00) */ #define AT24_FLAG_SERIAL BIT(3) /* factory-programmed serial number */ #define AT24_FLAG_MAC BIT(2) /* factory-programmed mac address */ +#define AT24_FLAG_NO_RDROL BIT(1) /* chip does not rollover page reads */ void (*setup)(struct nvmem_device *nvmem, void *context); void *context;
Some eeproms in the at24 family do not roll over page reads, e.g. the Microchip 24AA16/24LC16B. On those eeproms, reads that straddle block boundaries will not work correctly. Solution: Implement read rollover in the driver. To enable it, add the AT24_FLAG_NO_RDROL flag to the eeprom entry in the device_id table, or add 'no-read-rollover' to the eeprom devicetree entry. Signed-off-by: Sven Van Asbroeck <svendev@arcx.com> --- .../devicetree/bindings/eeprom/eeprom.txt | 5 +++ drivers/misc/eeprom/at24.c | 46 +++++++++++----------- include/linux/platform_data/at24.h | 1 + 3 files changed, 28 insertions(+), 24 deletions(-)