Re: [PATCH v1 i2c/for-next] i2c-i801: recover from hardware PEC errors
Hi Ellen, On Wed, 15 Apr 2015 13:46:17 -0700, Ellen Wang wrote: > On 4/15/2015 5:08 AM, Jean Delvare wrote: > > On Mon, 13 Apr 2015 17:11:59 -0700, Ellen Wang wrote: > >> On a CRC error while using hardware-supported PEC, an additional > >> error bit is set in the auxiliary status register. If this bit > >> isn't cleared, all subsequent operations will fail, essentially > >> hanging the controller. > >> > >> The fix is simple: clear the bit at the same time we clear > >> the error bits in the main status register. > >> > >> Signed-off-by: Ellen Wang > >> --- > >> drivers/i2c/busses/i2c-i801.c | 21 + > >> 1 file changed, 21 insertions(+) > > > > Thanks for the patch. I noticed the issue over a year ago and wrote a > > patch on my own, but then couldn't find the time to test it and finally > > forgot about it :( so I never posted it. You can read it here for > > reference: > > http://jdelvare.nerim.net/devel/misc/i2c-i801-07-check-pec-status.patch > > > > I am currently working on other matters but I'll look into this ASAP. A > > quick comparison between our patches suggests that yours only clears > > the PEC status bit while mine also reports the error properly to the > > caller, so mine might be a better working base. Maybe you could review > > and/or test my patch as a replacement of yours? > > My version does actually report the error, because the main status > register error bit is also set in this case, and that gets detected and > reported. It just doesn't doesn't return a specific errno for PEC error. Correct, I did not remember this, sorry. > The main difference between our versions is that you check and clear the > CRC error bit in i801_check_pre() and i801_checkpost(), while I do it in > i801_isr(). i801_isr() is also where it checks and clears the main > status register for errors, so it seems natural to do it also for the > auxiliary status. The check in i801_check_pre() is in case the CRC error bit is set at the time the driver is loaded. If it is, and we don't clear it, the driver won't work (for PEC transactions or even for all transactions, I'm not sure.) So I think it is needed either way. At the end of the transaction, the main status register is cleared in both i801_isr() and i801_check_post(). The latter is needed in case the driver operates in polling (non-interrupt) mode. That was the only mode available originally and my patch is so old that it is entirely possible that I wrote it before interrupt support was added to the driver. In fact that might even be the reason why I never submitted it, it may not support interrupt mode properly. So it is possible that the aux status register must be cleared in both i801_isr() and i801_check_post() to be on the safe side. > Probably what we should do is combine the two versions, especially if we > want to return a PEC-specific errno. I think to do that properly in the > current structure of the code, we should save the auxiliary status in > priv->auxsts in i801_isr(), the same way it saves priv->status. This > ways, isr() can clear the error in the hardware and check_post() can > still see it and process accordingly. What do you think? (I actually > thought of this, but opted for a minimal fix.) This is more Sounds like a good idea, but I still believe that i801_check_post() will also have to clear the CRC error bit, for the non-interrupt case. > complicated, and functions like i801_wait_intr() return the status and > it can't return two values easily. I don't think we actually need to deal with the aux status register value in i801_wait_intr(). According to my notes, whenever the CRC error bit is set, SMBHSTSTS_DEV_ERR is also set in the main status register. So it's enough to check for that bit in i801_wait_intr(). If you need to pass the aux register status to i801_check_post(), in the non-interrupt case you could simply do: return i801_check_post(priv, status, inb_p(SMBAUXSTS(priv))); > If we decide to go with your version, I can certainly test it. We have > a machine with the right hardware combination, and it generates PEC > errors deterministically. Well as I wrote above, it is possible that my patch doesn't work at all in the interrupt case, or that it works but is racy. So most probably we have to do as you said and combine both patches into one. > By the way, I noticed a small bug in i801_transaction(). At line 391, it > has "status = -ETIMEOUT", but "status" at that point should be the > register value not -errno. It probably should be "return -ETIMEOUT", > but I'm not sure whether hardware cleanup is necessary at that point. I can't see any bug there. status can be either the (positive) register value or a negative error code. It is passed to i801_check_post() which can deal with both. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordo
[PULL REQUEST] i2c for 4.1
Linus, here are two buildfixes for I2C based on your tree from the day before yesterday. Sadly, these build errors never reached me while the patches were sitting in -next. Need to fix that. Please pull. Thanks, Wolfram The following changes since commit bb0fd7ab0986105765d11baa82e619c618a235aa: Merge branch 'for-linus' of git://ftp.arm.linux.org.uk/~rmk/linux-arm (2015-04-14 21:03:26 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next for you to fetch changes up to c1c21f4e60ed4523292f1a89ff45a208bddd3849: i2c: core: Export bus recovery functions (2015-04-15 21:02:07 +0200) Guenter Roeck (1): i2c: jz4780: Fix build for m68k and sparc64 Mark Brown (1): i2c: core: Export bus recovery functions drivers/i2c/busses/i2c-jz4780.c | 1 + drivers/i2c/i2c-core.c | 3 +++ 2 files changed, 4 insertions(+) signature.asc Description: Digital signature
RE: [RFC PATCH 1/1] i2c: core: Add support for best effort block read emulation
> -Original Message-
> From: Wolfram Sang [mailto:w...@the-dreams.de]
> Sent: 15 April, 2015 18:55
> To: Tirdea, Irina
> Cc: linux-i2c@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: Re: [RFC PATCH 1/1] i2c: core: Add support for best effort block
> read emulation
>
> On Wed, Mar 25, 2015 at 04:33:20PM +0200, Irina Tirdea wrote:
> > There are devices that need to handle block transactions
> > regardless of the capabilities exported by the adapter.
> > For performance reasons, they need to use i2c read blocks
> > if available, otherwise emulate the block transaction with word
> > or byte transactions.
> >
> > Add support for a helper function that would read a data block
> > using the best transfer available: I2C_FUNC_SMBUS_READ_I2C_BLOCK,
> > I2C_FUNC_SMBUS_READ_WORD_DATA or I2C_FUNC_SMBUS_READ_BYTE_DATA.
> >
> > Signed-off-by: Irina Tirdea
> > ---
> >
> > Hi,
> >
> > This is a new API proposal to handle i2c block emulation in the
> > core instead of the driver code.
> >
> > This is needed for a set of iio sensor changes ([1], [2], [3])
> > that would otherwise duplicate this code. There are also some
> > usages of this functionality in the kernel (e.g. eeprom driver at24).
> >
> > Please let me know what you think.
>
> I am open to add something like this. One change I'd like to request is
> to introduce a user, e.g. convert at24.
>
Thanks for the review!
Sure, I'll send a new version with the fixes you suggested and include a user
as well.
> >
> > Thanks,
> > Irina
> >
> > [1] https://lkml.org/lkml/2015/2/16/408
> > [2] https://lkml.org/lkml/2015/2/16/413
> > [3] https://lkml.org/lkml/2015/2/16/402
> >
> > drivers/i2c/i2c-core.c | 62
> > ++
> > include/linux/i2c.h| 3 +++
> > 2 files changed, 65 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index fe80f85..2579f7d 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -2907,6 +2907,68 @@ trace:
> > }
> > EXPORT_SYMBOL(i2c_smbus_xfer);
> >
> > +/**
> > + * i2c_smbus_read_i2c_block_data_or_emulated - read block or emulate
> > + * @client: Handle to slave device
> > + * @command: Byte interpreted by slave
> > + * @length: Size of data block; SMBus allows at most 32 bytes
> > + * @values: Byte array into which data will be read; big enough to hold
> > + * the data returned by the slave. SMBus allows at most 32 bytes.
>
> Sidenote: SMBus3 allows 255 byte, but we don't support that (yet), so
> this is okay for now.
>
> > + *
> > + * This executes the SMBus "block read" protocol if supported by the
> > adapter.
> > + * If block read is not supported, it emulates it using either word or byte
> > + * read protocols depending on availability.
>
> Here I'd like to see a warning that people should double-check if their
> I2C slave does support that. Sometimes one can't exchange a block
> transfer with a byte transfer.
>
Ok.
> > + */
> > +s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client
> > *client,
> > + u8 command, u8 length, u8 *values)
> > +{
> > + u8 i;
> > + int status;
> > +
> > + if (length > I2C_SMBUS_BLOCK_MAX)
> > + length = I2C_SMBUS_BLOCK_MAX;
> > +
> > + if (i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> > + return i2c_smbus_read_i2c_block_data(client, command,
> > +length, values);
> > + } else if (i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_WORD_DATA |
> > + I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
>
> What about skipping the need for READ_BYTE_DATA and dump the byte which
> was maybe read too much?
>
Yes, that will simply the code.
> > + for (i = 0; (i + 2) <= length; i += 2) {
> > + status = i2c_smbus_read_word_data(client, command + i);
> > + if (status < 0)
> > + return status;
> > + values[i] = status & 0xff;
> > + values[i+1] = status >> 8;
> > + }
> > + if (i < length) {
> > + status = i2c_smbus_read_byte_data(client, command + i);
> > + if (status < 0)
> > + return status;
> > + values[i] = status;
> > + i++;
> > + }
> > + return i;
> > + } else if (i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
> > + for (i = 0; i < length; i++) {
> > + status = i2c_smbus_read_byte_data(client, command + i);
> > + if (status < 0)
> > + return status;
> > + values[i] = status;
> > + }
> > + retu
[PATCH] omap: i2c: Add calls for pinctrl state select
From: Pascal Huerst This adds calls to pinctrl subsystem in order to switch pin states on suspend/resume if you provide a "sleep" state in DT. Signed-off-by: Pascal Huerst --- drivers/i2c/busses/i2c-omap.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 0e89419..6b5d4bd 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1423,6 +1423,8 @@ static int omap_i2c_runtime_suspend(struct device *dev) omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG); } + pinctrl_pm_select_sleep_state(dev); + return 0; } @@ -1431,6 +1433,8 @@ static int omap_i2c_runtime_resume(struct device *dev) struct platform_device *pdev = to_platform_device(dev); struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); + pinctrl_pm_select_default_state(dev); + if (!_dev->regs) return 0; -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Need some guidance on i2c-ocores driver
Resend to LKML Lee, This question is actually more about MFD. Can you point me to the possible causes for my failure below? Thanks. York On 04/16/2015 05:02 PM, York Sun wrote: > Julia and other experts, > > I am seeking for help on my device driver. > > I am working on a module driver for a FPGA design with open core I2C > controller > memory-mapped to BAR2. I have searched up and down and found only one driver > (drivers/mfd/timberdale.c) with similar implementation. Following timberdale > driver, I am able to load the driver, but blocked by devm_ioremap_resource(). > It > is always in conflict with my BAR2. I wonder if I did something wrong. Here is > the flow I tracked so far. (By the way, I am using an older kernel 3.12. The > new > 4.0 kernel crashes when booting on my platform. I plan to move to newer kernel > later). > > mfd_add_devices() > | > |--mfd_add_device() > | > |--platform_device_add() > | > |--insert_resource() /* this passed */ > | | > | |--insert_resource_conflict() > | > |--device_add() > | > |--bus_probe_device() > | > |--devm_ioremap_resource() > | > |--devm_request_mem_region() > | > |--__request_region() /* this always conflicts */ > | > |--__request_resource() > > My driver is called RedStone DMA. Here is my debug output > >> root@p1022ds:~# insmod redstone_mfd.ko >> RedStone DMA 0002:05:00.0: pci_enable_device() successful >> RedStone DMA 0002:05:00.0: MSI-X init successful >> York kernel: Calling devm_ioremap_resource() >> York kernel kernel/resource.c __request_region 977: conflict=[??? >> 0xc0008-0xc00087fff flags 0x8000] >> ocores-i2c ocores-i2c: can't request region for resource [mem >> 0xc00086000-0xc0008601f] >> ocores-i2c: probe of ocores-i2c failed with error -16 >> RedStone DMA 0002:05:00.0: BAR[0] 0x000c-0x000c0007 >> flags 0x0014220c, length 524288 >> RedStone DMA 0002:05:00.0: BAR[1] 0x-0x >> flags 0x, length 0 >> RedStone DMA 0002:05:00.0: BAR[2] 0x000c0008-0x000c00087fff >> flags 0x00040200, length 32768 >> RedStone DMA 0002:05:00.0: BAR[3] 0x-0x >> flags 0x, length 0 >> RedStone DMA 0002:05:00.0: BAR[4] 0x-0x >> flags 0x, length 0 >> RedStone DMA 0002:05:00.0: BAR[5] 0x-0x >> flags 0x, length 0 >> RedStone DMA 0002:05:00.0: Version 1.4, built on 4-16-15, id 0 >> root@p1022ds:~# > > Can you shed some light on this? > > Thanks. > > York > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V2 1/2] i2c: brcmstb: Add Broadcom settop SoC i2c controller driver
On 16/04/15 22:52, rajeev kumar wrote: +#define bsc_readl(_dev, reg) \ + __bsc_readl(_dev, offsetof(struct bsc_regs, reg)) + > > use readl/writel This peripheral is used on chips that can run in either little or big-endian, and in both cases, this is a board-level strap that dictates the entire endianess of the system, such that on-chip peripherals should always be accessed in the host (native) endianess. readl/writel introduce an implicit endian conversion for kernels compiled in big-endian, that we do not want here. Depending on the architecture, there is also an additional barrier that is not needed, since the underlying bus serving this peripheral (GISB) does not re-order transactions, and writes are not posted. A better abstraction however could be something like this: #ifdef CONFIG_CPU_BIG_ENDIAN #define __bsc_readl(_dev, reg) ioread32be(...) #else #define __bsc_readl(_dev, reg) ioread32(...) #endif Such that we preserve the host endianess access, but we clearly identify the differences in running in a big or little endian configuration? -- Florian -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/3] i2c: davinci improvements and fixes
On 04/10/2015 06:59 PM, Wolfram Sang wrote: On Mon, Apr 06, 2015 at 03:38:38PM +0300, grygorii.stras...@linaro.org wrote: From: Grygorii Strashko This series converts driver to use I2C bus recovery infrastructure and adds Davinci I2C bus recovery mechanizm based on using ICPFUNC registers. These patches are combination of two patches from Ben Gardiner [1] and Mike Looijmans [2] which i've reworked to use I2C bus recovery infrastructure Applied to for-next, thanks for keeping at it and providing useful info! Thanks -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] Add support for best effort block read emulation
This is the second version for adding i2c_smbus_read_i2c_block_data_or_emulated to i2c-core. It contains mostly fixes suggested by Wolfram. Changes since v1: - dropped the RFC tag - changed at24 to use i2c_smbus_read_i2c_block_data_or_emulated - when reading an odd number of bytes using word emulation, read an even number of bytes and drop the last one - add a comment that this might not be suitable for all I2C slaves Irina Tirdea (2): i2c: core: Add support for best effort block read emulation eeprom: at24: use i2c_smbus_read_i2c_block_data_or_emulated drivers/i2c/i2c-core.c | 60 ++ drivers/misc/eeprom/at24.c | 40 +++ include/linux/i2c.h| 3 +++ 3 files changed, 72 insertions(+), 31 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] eeprom: at24: use i2c_smbus_read_i2c_block_data_or_emulated
For i2c busses that support only SMBUS extensions, the eeprom at24
driver reads data from the device using the SMBus block, word or byte
read protocols depending on availability.
Replace the block read emulation from the driver with the
i2c_smbus_read_i2c_block_data_or_emulated call from i2c core.
Signed-off-by: Irina Tirdea
---
drivers/misc/eeprom/at24.c | 40 +---
1 file changed, 9 insertions(+), 31 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 2d3db81..d13795a 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -186,19 +186,11 @@ static ssize_t at24_eeprom_read(struct at24_data *at24,
char *buf,
if (count > io_limit)
count = io_limit;
- switch (at24->use_smbus) {
- case I2C_SMBUS_I2C_BLOCK_DATA:
+ if (at24->use_smbus) {
/* Smaller eeproms can work given some SMBus extension calls */
if (count > I2C_SMBUS_BLOCK_MAX)
count = I2C_SMBUS_BLOCK_MAX;
- break;
- case I2C_SMBUS_WORD_DATA:
- count = 2;
- break;
- case I2C_SMBUS_BYTE_DATA:
- count = 1;
- break;
- default:
+ } else {
/*
* When we have a better choice than SMBus calls, use a
* combined I2C message. Write address; then read up to
@@ -229,27 +221,13 @@ static ssize_t at24_eeprom_read(struct at24_data *at24,
char *buf,
timeout = jiffies + msecs_to_jiffies(write_timeout);
do {
read_time = jiffies;
- switch (at24->use_smbus) {
- case I2C_SMBUS_I2C_BLOCK_DATA:
- status = i2c_smbus_read_i2c_block_data(client, offset,
- count, buf);
- break;
- case I2C_SMBUS_WORD_DATA:
- status = i2c_smbus_read_word_data(client, offset);
- if (status >= 0) {
- buf[0] = status & 0xff;
- buf[1] = status >> 8;
- status = count;
- }
- break;
- case I2C_SMBUS_BYTE_DATA:
- status = i2c_smbus_read_byte_data(client, offset);
- if (status >= 0) {
- buf[0] = status;
- status = count;
- }
- break;
- default:
+ if (at24->use_smbus) {
+ status =
+ i2c_smbus_read_i2c_block_data_or_emulated(client,
+ offset,
+ count,
+ buf);
+ } else {
status = i2c_transfer(client->adapter, msg, 2);
if (status == 2)
status = count;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] i2c: core: Add support for best effort block read emulation
There are devices that need to handle block transactions
regardless of the capabilities exported by the adapter.
For performance reasons, they need to use i2c read blocks
if available, otherwise emulate the block transaction with word
or byte transactions.
Add support for a helper function that would read a data block
using the best transfer available: I2C_FUNC_SMBUS_READ_I2C_BLOCK,
I2C_FUNC_SMBUS_READ_WORD_DATA or I2C_FUNC_SMBUS_READ_BYTE_DATA.
Signed-off-by: Irina Tirdea
---
drivers/i2c/i2c-core.c | 60 ++
include/linux/i2c.h| 3 +++
2 files changed, 63 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 098f698..5ceebc4 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2907,6 +2907,66 @@ trace:
}
EXPORT_SYMBOL(i2c_smbus_xfer);
+/**
+ * i2c_smbus_read_i2c_block_data_or_emulated - read block or emulate
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @length: Size of data block; SMBus allows at most 32 bytes
+ * @values: Byte array into which data will be read; big enough to hold
+ * the data returned by the slave. SMBus allows at most 32 bytes.
+ *
+ * This executes the SMBus "block read" protocol if supported by the adapter.
+ * If block read is not supported, it emulates it using either word or byte
+ * read protocols depending on availability.
+ *
+ * Before using this function you must double-check if the I2C slave does
+ * support exchanging a block transfer with a byte transfer.
+ */
+s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
+ u8 command, u8 length, u8 *values)
+{
+ u8 i;
+ int status;
+
+ if (length > I2C_SMBUS_BLOCK_MAX)
+ length = I2C_SMBUS_BLOCK_MAX;
+
+ if (i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+ return i2c_smbus_read_i2c_block_data(client, command,
+length, values);
+ } else if (i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_WORD_DATA)) {
+ for (i = 0; i < length; i += 2) {
+ status = i2c_smbus_read_word_data(client, command + i);
+ if (status < 0)
+ return status;
+ values[i] = status & 0xff;
+ if ((i + 1) < length)
+ values[i + 1] = status >> 8;
+ }
+ if (i > length)
+ return length;
+ return i;
+ } else if (i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
+ for (i = 0; i < length; i++) {
+ status = i2c_smbus_read_byte_data(client, command + i);
+ if (status < 0)
+ return status;
+ values[i] = status;
+ }
+ return i;
+ }
+
+ dev_err(&client->adapter->dev, "Unsupported transactions: %d,%d,%d\n",
+ I2C_SMBUS_I2C_BLOCK_DATA, I2C_SMBUS_WORD_DATA,
+ I2C_SMBUS_BYTE_DATA);
+
+ return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data_or_emulated);
+
#if IS_ENABLED(CONFIG_I2C_SLAVE)
int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e83a738..faf518d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -121,6 +121,9 @@ extern s32 i2c_smbus_read_i2c_block_data(const struct
i2c_client *client,
extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
u8 command, u8 length,
const u8 *values);
+extern s32
+i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
+ u8 command, u8 length, u8 *values);
#endif /* I2C */
/**
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
