| Message ID | 1421274213-3544-3-git-send-email-rjui@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote: > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/sched.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/slab.h> > +#include <linux/delay.h> some of them are not needed. I tested on amd64 and efm32 and could drop linux/device.h, linux/sched.h, linux/clk.h. (BTW, I wonder that you don't need clk handling.) > +#define TIM_CFG_OFFSET 0x04 > +#define TIME_CFG_MODE_400_SHIFT 31 Is the register name and the bit name prefix really different or is this a typo? > +static int __wait_for_bus_idle(struct bcm_iproc_i2c_dev *iproc_i2c) A bcm_iproc_i2c prefix would be nice here. > +static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c, > + struct i2c_msg *msg, u8 *addr) > +{ > + > + if (msg->flags & I2C_M_TEN) { > + dev_err(iproc_i2c->device, "no support for 10-bit address\n"); > + return -EINVAL; > + } > + > + *addr = (msg->addr << 1); You can also drop the parentheses. > + switch (val) { > + case M_CMD_STATUS_SUCCESS: > + return 0; > + > + case M_CMD_STATUS_LOST_ARB: > + dev_err(iproc_i2c->device, "lost bus arbitration\n"); > + return -EREMOTEIO; > + > + case M_CMD_STATUS_NACK_ADDR: > + dev_err(iproc_i2c->device, "NAK addr:0x%02x\n", > + iproc_i2c->msg->addr); > + return -EREMOTEIO; > + > + case M_CMD_STATUS_NACK_DATA: > + dev_err(iproc_i2c->device, "NAK data\n"); > + return -EREMOTEIO; > + > + case M_CMD_STATUS_TIMEOUT: > + dev_err(iproc_i2c->device, "bus timeout\n"); > + return -ETIMEDOUT; > + > + default: > + dev_err(iproc_i2c->device, "unknown error code=%d\n", val); > + return -EREMOTEIO; > + } > + > + return -EREMOTEIO; This is not reached. > +} > + > +static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, > + struct i2c_msg *msg) > +{ > + int ret, i; > + u8 addr; > + u32 val; > + unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC); > + > + if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) { Is the < 1 a hardware or a software limitation? That means your driver doesn't support I2C_SMBUS_QUICK which is used for example by i2cdetect. > + dev_err(iproc_i2c->device, > + "supported data length is 1 - %u bytes\n", > + M_TX_RX_FIFO_SIZE - 1); > + return -EINVAL; > + } > + > + iproc_i2c->msg = msg; Can it happen that iproc_i2c->msg still holds an uncompleted message here or is this serialized by the core? Wolfram? Either here something like: if (iproc_i2c->msg) return -EBUSY; and iproc_i2c->msg = NULL; when a transfer is completed is needed, or the respective code can be dropped from other drivers (e.g. i2c-efm32). On the other hand .msg is only used in bcm_iproc_i2c_check_status() to give a diagnostic message. Maybe you can drop .msg and instead give it as an additional parameter to bcm_iproc_i2c_check_status(). > + ret = __wait_for_bus_idle(iproc_i2c); > + if (ret) > + return ret; I would still prefer to have something like: if (bcm_iproc_i2c_bus_busy()) return -EBUSY; instead of a tight loop here. > + ret = bcm_iproc_i2c_format_addr(iproc_i2c, msg, &addr); > + if (ret) > + return ret; > + > + /* load slave address into the TX FIFO */ > + writel(addr, iproc_i2c->base + M_TX_OFFSET); > + > + /* for a write transaction, load data into the TX FIFO */ > + if (!(msg->flags & I2C_M_RD)) { > + for (i = 0; i < msg->len; i++) { > + val = msg->buf[i]; > + > + /* mark the last byte */ > + if (i == msg->len - 1) > + val |= 1 << M_TX_WR_STATUS_SHIFT; What happens if you don't mark this last byte? Could this be used to support transfers bigger than the fifo size? > + /* > + * Enable the "start busy" interrupt, which will be triggered after > + * the transaction is done, i.e., the internal start_busy bit s/\.,/./ I think > + * transitions from 1 to 0 s/$/./ > + */ > + writel(1 << IE_M_START_BUSY_SHIFT, iproc_i2c->base + IE_OFFSET); > + > + /* > + * Now we can activate the transfer. For a read operation, specify the > + * number of bytes to read s/$/./ > + */ > + val = 1 << M_CMD_START_BUSY_SHIFT; > + if (msg->flags & I2C_M_RD) { > + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | > + (msg->len << M_CMD_RD_CNT_SHIFT); > + } else { > + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); > + } > + writel(val, iproc_i2c->base + M_CMD_OFFSET); > + > + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); When the interrupt fires here after the complete timed out and before you disable the irq you still throw the result away. > + > + /* disable all interrupts */ > + writel(0, iproc_i2c->base + IE_OFFSET); > + > + if (!time_left) { > + dev_err(iproc_i2c->device, "transaction times out\n"); s/times/timed/ > +static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap) > +{ > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; Note that I2C_FUNC_SMBUS_EMUL includes I2C_FUNC_SMBUS_QUICK, so your driver claims to support transfers of length 0. > +static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) > +{ > + unsigned int bus_speed, speed_bit; > + u32 val; > + int ret = of_property_read_u32(iproc_i2c->device->of_node, > + "clock-frequency", &bus_speed); > + if (ret < 0) { > + dev_err(iproc_i2c->device, > + "missing clock-frequency property\n"); > + return -ENODEV; Is a missing property the only situation where of_property_read_u32 returns an error? Would it be sane to default to 100 kHz? > +static int bcm_iproc_i2c_remove(struct platform_device *pdev) > +{ > + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); > + > + i2c_del_adapter(&iproc_i2c->adapter); You need to free the irq before i2c_del_adapter. > + free_irq(iproc_i2c->irq, iproc_i2c); > + bcm_iproc_i2c_disable(iproc_i2c); > + > + return 0; > +} > + > +static const struct of_device_id bcm_iproc_i2c_of_match[] = { > + {.compatible = "brcm,iproc-i2c",}, Not sure this is specified to be a must, but I'd add spaces after { and before }. > + {}, It's a good habit to write this as { /* sentinel */ } without trailing comma. Best regards Uwe
> > + iproc_i2c->msg = msg; > Can it happen that iproc_i2c->msg still holds an uncompleted message > here or is this serialized by the core? Wolfram? Either here something We have per-adapter locks serializing transfers, if you mean that? > > +static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) > > +{ > > + unsigned int bus_speed, speed_bit; > > + u32 val; > > + int ret = of_property_read_u32(iproc_i2c->device->of_node, > > + "clock-frequency", &bus_speed); > > + if (ret < 0) { > > + dev_err(iproc_i2c->device, > > + "missing clock-frequency property\n"); > > + return -ENODEV; > Is a missing property the only situation where of_property_read_u32 > returns an error? Would it be sane to default to 100 kHz? Default of 100kHz instead of -ENODEV sounds very reasonable. > > +static int bcm_iproc_i2c_remove(struct platform_device *pdev) > > +{ > > + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); > > + > > + i2c_del_adapter(&iproc_i2c->adapter); > You need to free the irq before i2c_del_adapter. One could also keep using devm_request_irq and disable all interrupts sources here? Thanks for the reviews, Uwe! Wolfram
Hello, On Thu, Jan 15, 2015 at 01:07:05PM +0100, Wolfram Sang wrote: > > > + iproc_i2c->msg = msg; > > Can it happen that iproc_i2c->msg still holds an uncompleted message > > here or is this serialized by the core? Wolfram? Either here something > > We have per-adapter locks serializing transfers, if you mean that? ok, so in the efm32 driver the if (ddata->msgs) condition in efm32_i2c_master_xfer can never be true, right? > > > +static int bcm_iproc_i2c_remove(struct platform_device *pdev) > > > +{ > > > + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); > > > + > > > + i2c_del_adapter(&iproc_i2c->adapter); > > You need to free the irq before i2c_del_adapter. > > One could also keep using devm_request_irq and disable all interrupts > sources here? calling devm_free_irq would work or writel(0, iproc_i2c->base + IE_OFFSET) + synchronize_irq(). Best regards Uwe
On 1/15/2015 12:41 AM, Uwe Kleine-König wrote: > Hello, > > On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote: >> +#include <linux/device.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/sched.h> >> +#include <linux/i2c.h> >> +#include <linux/interrupt.h> >> +#include <linux/platform_device.h> >> +#include <linux/clk.h> >> +#include <linux/io.h> >> +#include <linux/slab.h> >> +#include <linux/delay.h> > some of them are not needed. I tested on amd64 and efm32 and could drop > linux/device.h, linux/sched.h, linux/clk.h. (BTW, I wonder that you > don't need clk handling.) Thanks. Will delete redundant header includes. I haven't found any clocks in Cygnus clock data sheet that are labeled i2c. I suspect the I2C clock is derived directly from the crystal and therefore we have no gating control. As you can see, the rates of 100K and 400K are set directly in the I2C block internal registers. That implies the I2C core clock is fixed. > >> +#define TIM_CFG_OFFSET 0x04 >> +#define TIME_CFG_MODE_400_SHIFT 31 > Is the register name and the bit name prefix really different or is this > a typo? > Yeah, typo. Will fix. >> +static int __wait_for_bus_idle(struct bcm_iproc_i2c_dev *iproc_i2c) > A bcm_iproc_i2c prefix would be nice here. > >> +static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c, >> + struct i2c_msg *msg, u8 *addr) >> +{ >> + >> + if (msg->flags & I2C_M_TEN) { >> + dev_err(iproc_i2c->device, "no support for 10-bit address\n"); >> + return -EINVAL; >> + } >> + >> + *addr = (msg->addr << 1); > You can also drop the parentheses. > Yes >> + switch (val) { >> + case M_CMD_STATUS_SUCCESS: >> + return 0; >> + >> + case M_CMD_STATUS_LOST_ARB: >> + dev_err(iproc_i2c->device, "lost bus arbitration\n"); >> + return -EREMOTEIO; >> + >> + case M_CMD_STATUS_NACK_ADDR: >> + dev_err(iproc_i2c->device, "NAK addr:0x%02x\n", >> + iproc_i2c->msg->addr); >> + return -EREMOTEIO; >> + >> + case M_CMD_STATUS_NACK_DATA: >> + dev_err(iproc_i2c->device, "NAK data\n"); >> + return -EREMOTEIO; >> + >> + case M_CMD_STATUS_TIMEOUT: >> + dev_err(iproc_i2c->device, "bus timeout\n"); >> + return -ETIMEDOUT; >> + >> + default: >> + dev_err(iproc_i2c->device, "unknown error code=%d\n", val); >> + return -EREMOTEIO; >> + } >> + >> + return -EREMOTEIO; > This is not reached. > Will delete. >> +} >> + >> +static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, >> + struct i2c_msg *msg) >> +{ >> + int ret, i; >> + u8 addr; >> + u32 val; >> + unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC); >> + >> + if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) { > Is the < 1 a hardware or a software limitation? That means your driver > doesn't support I2C_SMBUS_QUICK which is used for example by i2cdetect. > Actually a SW issue. Will fix. >> + dev_err(iproc_i2c->device, >> + "supported data length is 1 - %u bytes\n", >> + M_TX_RX_FIFO_SIZE - 1); >> + return -EINVAL; >> + } >> + >> + iproc_i2c->msg = msg; > Can it happen that iproc_i2c->msg still holds an uncompleted message > here or is this serialized by the core? Wolfram? Either here something > like: > > if (iproc_i2c->msg) > return -EBUSY; > > and > > iproc_i2c->msg = NULL; > > when a transfer is completed is needed, or the respective code can be > dropped from other drivers (e.g. i2c-efm32). > On the other hand .msg is only used in bcm_iproc_i2c_check_status() to > give a diagnostic message. Maybe you can drop .msg and instead give it > as an additional parameter to bcm_iproc_i2c_check_status(). > Yes, I'll drop .msg in iproc_i2c. >> + ret = __wait_for_bus_idle(iproc_i2c); >> + if (ret) >> + return ret; > I would still prefer to have something like: > > if (bcm_iproc_i2c_bus_busy()) > return -EBUSY; > > instead of a tight loop here. > Okay. Will do. >> + ret = bcm_iproc_i2c_format_addr(iproc_i2c, msg, &addr); >> + if (ret) >> + return ret; >> + >> + /* load slave address into the TX FIFO */ >> + writel(addr, iproc_i2c->base + M_TX_OFFSET); >> + >> + /* for a write transaction, load data into the TX FIFO */ >> + if (!(msg->flags & I2C_M_RD)) { >> + for (i = 0; i < msg->len; i++) { >> + val = msg->buf[i]; >> + >> + /* mark the last byte */ >> + if (i == msg->len - 1) >> + val |= 1 << M_TX_WR_STATUS_SHIFT; > What happens if you don't mark this last byte? Could this be used to > support transfers bigger than the fifo size? > I do not think so. According to the iProc I2C block programming guide, one always needs to mark the last byte in a write operation. >> + /* >> + * Enable the "start busy" interrupt, which will be triggered after >> + * the transaction is done, i.e., the internal start_busy bit > s/\.,/./ I think > >> + * transitions from 1 to 0 > s/$/./ Thanks. >> + */ >> + writel(1 << IE_M_START_BUSY_SHIFT, iproc_i2c->base + IE_OFFSET); >> + >> + /* >> + * Now we can activate the transfer. For a read operation, specify the >> + * number of bytes to read > s/$/./ > >> + */ >> + val = 1 << M_CMD_START_BUSY_SHIFT; >> + if (msg->flags & I2C_M_RD) { >> + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | >> + (msg->len << M_CMD_RD_CNT_SHIFT); >> + } else { >> + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); >> + } >> + writel(val, iproc_i2c->base + M_CMD_OFFSET); >> + >> + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); > > When the interrupt fires here after the complete timed out and before > you disable the irq you still throw the result away. Yes, but then this comes down to the fact that if it has reached the point that is determined to be a timeout condition in the driver, one should really treat it as timeout error. In a normal condition, time_left should never reach zero. >> + >> + /* disable all interrupts */ >> + writel(0, iproc_i2c->base + IE_OFFSET); >> + >> + if (!time_left) { >> + dev_err(iproc_i2c->device, "transaction times out\n"); > s/times/timed/ > Thanks. >> +static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap) >> +{ >> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; > Note that I2C_FUNC_SMBUS_EMUL includes I2C_FUNC_SMBUS_QUICK, so your > driver claims to support transfers of length 0. > Yes. Fix the driver to support length 0 for slave address query. >> +static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) >> +{ >> + unsigned int bus_speed, speed_bit; >> + u32 val; >> + int ret = of_property_read_u32(iproc_i2c->device->of_node, >> + "clock-frequency", &bus_speed); >> + if (ret < 0) { >> + dev_err(iproc_i2c->device, >> + "missing clock-frequency property\n"); >> + return -ENODEV; > Is a missing property the only situation where of_property_read_u32 > returns an error? Would it be sane to default to 100 kHz? > Okay, agreed with you and Wolfram. Will default to 100 KHz. >> +static int bcm_iproc_i2c_remove(struct platform_device *pdev) >> +{ >> + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); >> + >> + i2c_del_adapter(&iproc_i2c->adapter); > You need to free the irq before i2c_del_adapter. > Yes. Thanks. Change back to use devm_request_irq, and use disable_irq here before removing the adapter. >> + free_irq(iproc_i2c->irq, iproc_i2c); >> + bcm_iproc_i2c_disable(iproc_i2c); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id bcm_iproc_i2c_of_match[] = { >> + {.compatible = "brcm,iproc-i2c",}, > Not sure this is specified to be a must, but I'd add spaces after { and > before }. > Yes. >> + {}, > It's a good habit to write this as > > { /* sentinel */ } > > without trailing comma. Okay. > > Best regards > Uwe > Thanks for the review!
On 1/15/2015 4:07 AM, Wolfram Sang wrote: >>> +static int bcm_iproc_i2c_remove(struct platform_device *pdev) >>> +{ >>> + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); >>> + >>> + i2c_del_adapter(&iproc_i2c->adapter); >> You need to free the irq before i2c_del_adapter. > > One could also keep using devm_request_irq and disable all interrupts > sources here? > Okay this makes sense. Will use devm_request_irq and disable interrupt in the remove function. Thanks. > Thanks for the reviews, Uwe! > > Wolfram >
Hello, On Fri, Jan 16, 2015 at 02:09:28PM -0800, Ray Jui wrote: > On 1/15/2015 12:41 AM, Uwe Kleine-König wrote: > > On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote: > >> + */ > >> + val = 1 << M_CMD_START_BUSY_SHIFT; > >> + if (msg->flags & I2C_M_RD) { > >> + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | > >> + (msg->len << M_CMD_RD_CNT_SHIFT); > >> + } else { > >> + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); > >> + } > >> + writel(val, iproc_i2c->base + M_CMD_OFFSET); > >> + > >> + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); > > > > When the interrupt fires here after the complete timed out and before > > you disable the irq you still throw the result away. > Yes, but then this comes down to the fact that if it has reached the > point that is determined to be a timeout condition in the driver, one > should really treat it as timeout error. In a normal condition, > time_left should never reach zero. I don't agree here. I'm not sure there is a real technical reason, though. But still if you're in a "success after timeout already over" situation it's IMHO better to interpret it as success, not timeout. > >> +static int bcm_iproc_i2c_remove(struct platform_device *pdev) > >> +{ > >> + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); > >> + > >> + i2c_del_adapter(&iproc_i2c->adapter); > > You need to free the irq before i2c_del_adapter. > > > Yes. Thanks. Change back to use devm_request_irq, and use disable_irq > here before removing the adapter. The more lightweight approach is to set your device's irq-enable register to zero and call synchronize_irq. (For a shared irq calling disable_irq is even wrong here.) Best regards Uwe
On 1/17/2015 8:01 AM, Uwe Kleine-König wrote: > Hello, > > On Fri, Jan 16, 2015 at 02:09:28PM -0800, Ray Jui wrote: >> On 1/15/2015 12:41 AM, Uwe Kleine-König wrote: >>> On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote: >>>> + */ >>>> + val = 1 << M_CMD_START_BUSY_SHIFT; >>>> + if (msg->flags & I2C_M_RD) { >>>> + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | >>>> + (msg->len << M_CMD_RD_CNT_SHIFT); >>>> + } else { >>>> + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); >>>> + } >>>> + writel(val, iproc_i2c->base + M_CMD_OFFSET); >>>> + >>>> + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); >>> >>> When the interrupt fires here after the complete timed out and before >>> you disable the irq you still throw the result away. >> Yes, but then this comes down to the fact that if it has reached the >> point that is determined to be a timeout condition in the driver, one >> should really treat it as timeout error. In a normal condition, >> time_left should never reach zero. > I don't agree here. I'm not sure there is a real technical reason, > though. But still if you're in a "success after timeout already over" > situation it's IMHO better to interpret it as success, not timeout. > The thing is, the interrupt should never fire after wait_for_completion_timeout returns zero here. If it does, then the issue is really that the timeout value set in the driver is probably not long enough. I just checked other I2C drivers. I think the way how timeout is handled here is consistent with other I2C drivers. >>>> +static int bcm_iproc_i2c_remove(struct platform_device *pdev) >>>> +{ >>>> + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); >>>> + >>>> + i2c_del_adapter(&iproc_i2c->adapter); >>> You need to free the irq before i2c_del_adapter. >>> >> Yes. Thanks. Change back to use devm_request_irq, and use disable_irq >> here before removing the adapter. > The more lightweight approach is to set your device's irq-enable > register to zero and call synchronize_irq. (For a shared irq calling > disable_irq is even wrong here.) > The fact that IRQF_SHARED flag is not set indicates this is a dedicated IRQ line, so I thought using disable_irq here makes sense. But if both you and Wolfram think masking all I2C interrupts at the block level + synchronize_irq is a better approach, I can change to that. Thanks! > Best regards > Uwe >
Hello, On Sat, Jan 17, 2015 at 11:58:33AM -0800, Ray Jui wrote: > On 1/17/2015 8:01 AM, Uwe Kleine-König wrote: > > On Fri, Jan 16, 2015 at 02:09:28PM -0800, Ray Jui wrote: > >> On 1/15/2015 12:41 AM, Uwe Kleine-König wrote: > >>> On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote: > >>>> + */ > >>>> + val = 1 << M_CMD_START_BUSY_SHIFT; > >>>> + if (msg->flags & I2C_M_RD) { > >>>> + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | > >>>> + (msg->len << M_CMD_RD_CNT_SHIFT); > >>>> + } else { > >>>> + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); > >>>> + } > >>>> + writel(val, iproc_i2c->base + M_CMD_OFFSET); > >>>> + > >>>> + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); > >>> > >>> When the interrupt fires here after the complete timed out and before > >>> you disable the irq you still throw the result away. > >> Yes, but then this comes down to the fact that if it has reached the > >> point that is determined to be a timeout condition in the driver, one > >> should really treat it as timeout error. In a normal condition, > >> time_left should never reach zero. > > I don't agree here. I'm not sure there is a real technical reason, > > though. But still if you're in a "success after timeout already over" > > situation it's IMHO better to interpret it as success, not timeout. > > > The thing is, the interrupt should never fire after > wait_for_completion_timeout returns zero here. If it does, then the > issue is really that the timeout value set in the driver is probably not > long enough. I just checked other I2C drivers. I think the way how > timeout is handled here is consistent with other I2C drivers. In the presence of Clock stretching there is no (theorethical) upper limit for the time needed to transfer a given message, is there? So (theoretically) you can never be sure not to interrupt an ongoing transfer. And other drivers doing the same is only an excuse to start similar, but not to not improve :-) > >>>> +static int bcm_iproc_i2c_remove(struct platform_device *pdev) > >>>> +{ > >>>> + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); > >>>> + > >>>> + i2c_del_adapter(&iproc_i2c->adapter); > >>> You need to free the irq before i2c_del_adapter. > >>> > >> Yes. Thanks. Change back to use devm_request_irq, and use disable_irq > >> here before removing the adapter. > > The more lightweight approach is to set your device's irq-enable > > register to zero and call synchronize_irq. (For a shared irq calling > > disable_irq is even wrong here.) > > > The fact that IRQF_SHARED flag is not set indicates this is a dedicated > IRQ line, so I thought using disable_irq here makes sense. But if both > you and Wolfram think masking all I2C interrupts at the block level + > synchronize_irq is a better approach, I can change to that. Thanks! I don't care much. Using synchronize_irq is the more universal approach and so more likely correct for someone copying from your driver. Best regards Uwe
On 1/17/2015 12:18 PM, Uwe Kleine-König wrote: > Hello, > > On Sat, Jan 17, 2015 at 11:58:33AM -0800, Ray Jui wrote: >> On 1/17/2015 8:01 AM, Uwe Kleine-König wrote: >>> On Fri, Jan 16, 2015 at 02:09:28PM -0800, Ray Jui wrote: >>>> On 1/15/2015 12:41 AM, Uwe Kleine-König wrote: >>>>> On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote: >>>>>> + */ >>>>>> + val = 1 << M_CMD_START_BUSY_SHIFT; >>>>>> + if (msg->flags & I2C_M_RD) { >>>>>> + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | >>>>>> + (msg->len << M_CMD_RD_CNT_SHIFT); >>>>>> + } else { >>>>>> + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); >>>>>> + } >>>>>> + writel(val, iproc_i2c->base + M_CMD_OFFSET); >>>>>> + >>>>>> + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); >>>>> >>>>> When the interrupt fires here after the complete timed out and before >>>>> you disable the irq you still throw the result away. >>>> Yes, but then this comes down to the fact that if it has reached the >>>> point that is determined to be a timeout condition in the driver, one >>>> should really treat it as timeout error. In a normal condition, >>>> time_left should never reach zero. >>> I don't agree here. I'm not sure there is a real technical reason, >>> though. But still if you're in a "success after timeout already over" >>> situation it's IMHO better to interpret it as success, not timeout. >>> >> The thing is, the interrupt should never fire after >> wait_for_completion_timeout returns zero here. If it does, then the >> issue is really that the timeout value set in the driver is probably not >> long enough. I just checked other I2C drivers. I think the way how >> timeout is handled here is consistent with other I2C drivers. > In the presence of Clock stretching there is no (theorethical) upper > limit for the time needed to transfer a given message, is there? So > (theoretically) you can never be sure not to interrupt an ongoing > transfer. > Yes. No theoretical upper limit in the case when clock is stretched by the slave. But how would adding an additional interrupt completion check below help? I assume you want the the check to be like the following? time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); /* disable all interrupts */ writel(0, iproc_i2c->base + IE_OFFSET); if (!time_left && !completion_done()) { dev_err(iproc_i2c->device, "transaction timed out\n"); /* flush FIFOs */ val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT); writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET); return -ETIMEDOUT; } Does the above code make sense logically? That is, wait_for_completion_timeout has timed out, and we are doing an additional check below to make sure it really has timed out? > And other drivers doing the same is only an excuse to start similar, but > not to not improve :-) > >>>>>> +static int bcm_iproc_i2c_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); >>>>>> + >>>>>> + i2c_del_adapter(&iproc_i2c->adapter); >>>>> You need to free the irq before i2c_del_adapter. >>>>> >>>> Yes. Thanks. Change back to use devm_request_irq, and use disable_irq >>>> here before removing the adapter. >>> The more lightweight approach is to set your device's irq-enable >>> register to zero and call synchronize_irq. (For a shared irq calling >>> disable_irq is even wrong here.) >>> >> The fact that IRQF_SHARED flag is not set indicates this is a dedicated >> IRQ line, so I thought using disable_irq here makes sense. But if both >> you and Wolfram think masking all I2C interrupts at the block level + >> synchronize_irq is a better approach, I can change to that. Thanks! > I don't care much. Using synchronize_irq is the more universal approach > and so more likely correct for someone copying from your driver. > Sure, more universal approach and a good example for others. It takes care of both cases of dedicated and shared interrupt. I will make that change. Thanks. > Best regards > Uwe >
Hello, On Sat, Jan 17, 2015 at 12:51:50PM -0800, Ray Jui wrote: > On 1/17/2015 12:18 PM, Uwe Kleine-König wrote: > > Hello, > > > > On Sat, Jan 17, 2015 at 11:58:33AM -0800, Ray Jui wrote: > >> On 1/17/2015 8:01 AM, Uwe Kleine-König wrote: > >>> On Fri, Jan 16, 2015 at 02:09:28PM -0800, Ray Jui wrote: > >>>> On 1/15/2015 12:41 AM, Uwe Kleine-König wrote: > >>>>> On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote: > >>>>>> + */ > >>>>>> + val = 1 << M_CMD_START_BUSY_SHIFT; > >>>>>> + if (msg->flags & I2C_M_RD) { > >>>>>> + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | > >>>>>> + (msg->len << M_CMD_RD_CNT_SHIFT); > >>>>>> + } else { > >>>>>> + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); > >>>>>> + } > >>>>>> + writel(val, iproc_i2c->base + M_CMD_OFFSET); > >>>>>> + > >>>>>> + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); > >>>>> > >>>>> When the interrupt fires here after the complete timed out and before > >>>>> you disable the irq you still throw the result away. > >>>> Yes, but then this comes down to the fact that if it has reached the > >>>> point that is determined to be a timeout condition in the driver, one > >>>> should really treat it as timeout error. In a normal condition, > >>>> time_left should never reach zero. > >>> I don't agree here. I'm not sure there is a real technical reason, > >>> though. But still if you're in a "success after timeout already over" > >>> situation it's IMHO better to interpret it as success, not timeout. > >>> > >> The thing is, the interrupt should never fire after > >> wait_for_completion_timeout returns zero here. If it does, then the > >> issue is really that the timeout value set in the driver is probably not > >> long enough. I just checked other I2C drivers. I think the way how > >> timeout is handled here is consistent with other I2C drivers. > > In the presence of Clock stretching there is no (theorethical) upper > > limit for the time needed to transfer a given message, is there? So > > (theoretically) you can never be sure not to interrupt an ongoing > > transfer. > > > Yes. No theoretical upper limit in the case when clock is stretched by > the slave. But how would adding an additional interrupt completion check > below help? I assume you want the the check to be like the following? > > time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); > > /* disable all interrupts */ > writel(0, iproc_i2c->base + IE_OFFSET); > > if (!time_left && !completion_done()) { > dev_err(iproc_i2c->device, "transaction timed out\n"); > > /* flush FIFOs */ > val = (1 << M_FIFO_RX_FLUSH_SHIFT) | > (1 << M_FIFO_TX_FLUSH_SHIFT); > writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET); > return -ETIMEDOUT; > } No, I want: time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); if (!transfer_was_complete) { handle_error(); ... } handle_successful_transfer(); and time_left == 0 is not a reliable indicator that the transfer failed. Best regards Uwe
On 1/17/2015 1:10 PM, Uwe Kleine-König wrote: > Hello, > > On Sat, Jan 17, 2015 at 12:51:50PM -0800, Ray Jui wrote: >> On 1/17/2015 12:18 PM, Uwe Kleine-König wrote: >>> Hello, >>> >>> On Sat, Jan 17, 2015 at 11:58:33AM -0800, Ray Jui wrote: >>>> On 1/17/2015 8:01 AM, Uwe Kleine-König wrote: >>>>> On Fri, Jan 16, 2015 at 02:09:28PM -0800, Ray Jui wrote: >>>>>> On 1/15/2015 12:41 AM, Uwe Kleine-König wrote: >>>>>>> On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote: >>>>>>>> + */ >>>>>>>> + val = 1 << M_CMD_START_BUSY_SHIFT; >>>>>>>> + if (msg->flags & I2C_M_RD) { >>>>>>>> + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | >>>>>>>> + (msg->len << M_CMD_RD_CNT_SHIFT); >>>>>>>> + } else { >>>>>>>> + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); >>>>>>>> + } >>>>>>>> + writel(val, iproc_i2c->base + M_CMD_OFFSET); >>>>>>>> + >>>>>>>> + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); >>>>>>> >>>>>>> When the interrupt fires here after the complete timed out and before >>>>>>> you disable the irq you still throw the result away. >>>>>> Yes, but then this comes down to the fact that if it has reached the >>>>>> point that is determined to be a timeout condition in the driver, one >>>>>> should really treat it as timeout error. In a normal condition, >>>>>> time_left should never reach zero. >>>>> I don't agree here. I'm not sure there is a real technical reason, >>>>> though. But still if you're in a "success after timeout already over" >>>>> situation it's IMHO better to interpret it as success, not timeout. >>>>> >>>> The thing is, the interrupt should never fire after >>>> wait_for_completion_timeout returns zero here. If it does, then the >>>> issue is really that the timeout value set in the driver is probably not >>>> long enough. I just checked other I2C drivers. I think the way how >>>> timeout is handled here is consistent with other I2C drivers. >>> In the presence of Clock stretching there is no (theorethical) upper >>> limit for the time needed to transfer a given message, is there? So >>> (theoretically) you can never be sure not to interrupt an ongoing >>> transfer. >>> >> Yes. No theoretical upper limit in the case when clock is stretched by >> the slave. But how would adding an additional interrupt completion check >> below help? I assume you want the the check to be like the following? >> >> time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); >> >> /* disable all interrupts */ >> writel(0, iproc_i2c->base + IE_OFFSET); >> >> if (!time_left && !completion_done()) { >> dev_err(iproc_i2c->device, "transaction timed out\n"); >> >> /* flush FIFOs */ >> val = (1 << M_FIFO_RX_FLUSH_SHIFT) | >> (1 << M_FIFO_TX_FLUSH_SHIFT); >> writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET); >> return -ETIMEDOUT; >> } > No, I want: > > time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); > > if (!transfer_was_complete) { > handle_error(); > ... > > } > > handle_successful_transfer(); > > and time_left == 0 is not a reliable indicator that the transfer failed. > > Best regards > Uwe > Okay I'll check both time_left and transfer_was_done: time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); /* disable all interrupts */ writel(0, iproc_i2c->base + IE_OFFSET); if (!time_left && !atomic_read(&iproc_i2c->transfer_is_successful)) { dev_err(iproc_i2c->device, "transaction timed out\n"); /* flush FIFOs */ val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT); writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET); return -ETIMEDOUT; }
On Sat, Jan 17, 2015 at 01:26:41PM -0800, Ray Jui wrote: > time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); > > /* disable all interrupts */ > writel(0, iproc_i2c->base + IE_OFFSET); > > if (!time_left && !atomic_read(&iproc_i2c->transfer_is_successful)) { Why are you using atomic_read() here?
On 1/17/2015 2:40 PM, Russell King - ARM Linux wrote: > On Sat, Jan 17, 2015 at 01:26:41PM -0800, Ray Jui wrote: >> time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); >> >> /* disable all interrupts */ >> writel(0, iproc_i2c->base + IE_OFFSET); >> >> if (!time_left && !atomic_read(&iproc_i2c->transfer_is_successful)) { > > Why are you using atomic_read() here? > transfer_is_successful 1) will be reset to 0 in this function (before kick start the I2C transfer), 2) will be set to 1 in the ISR (to signal completion of the I2C transfer), and 3) will be checked in this function here. I thought that means I should declare it volatile, because it can be modified in both the process context and interrupt context (and I use atomic because I remember Linux checkpatch warns against using volatile)?
On Sat, Jan 17, 2015 at 04:30:33PM -0800, Ray Jui wrote: > > > On 1/17/2015 2:40 PM, Russell King - ARM Linux wrote: > > On Sat, Jan 17, 2015 at 01:26:41PM -0800, Ray Jui wrote: > >> time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); > >> > >> /* disable all interrupts */ > >> writel(0, iproc_i2c->base + IE_OFFSET); > >> > >> if (!time_left && !atomic_read(&iproc_i2c->transfer_is_successful)) { > > > > Why are you using atomic_read() here? > > > transfer_is_successful 1) will be reset to 0 in this function (before > kick start the I2C transfer), 2) will be set to 1 in the ISR (to signal > completion of the I2C transfer), and 3) will be checked in this function > here. I thought that means I should declare it volatile, because it can > be modified in both the process context and interrupt context (and I use > atomic because I remember Linux checkpatch warns against using volatile)? You don't need volatile or atomic_t for that. Rather than switching to atomic_t when seeing the checkpatch warning, you'd do better to read Documentation/volatile-considered-harmful.txt to understand why checkpatch issues the warning, and realise that you don't need it for the above. Note that in the above code, the compiler can't make an assumption about iproc_i2c->transfer_is_successful because it can't tell whether a called function (eg, wait_for_completion_timeout()) could modify it. Another possible issue with the above code are these lines: /* disable all interrupts */ writel(0, iproc_i2c->base + IE_OFFSET); It would be nice to think that would hit the hardware immediately, but that's making assumptions about hardware which are not necessary true. Your interrupt handler could even be running on another CPU after you've asked for that register to be written. Depending on what you're trying to achieve here, you may need: /* disable all interrupts */ writel(0, iproc_i2c->base + IE_OFFSET); /* read it back to ensure the write has hit */ readl(iproc_i2c->base + IE_OFFSET); /* make sure the interrupt handler isn't running */ synchronize_irq(...->irq); if what you're trying to do is to ensure that the interrupt handler has finished running.
On 1/19/2015 11:28 AM, Russell King - ARM Linux wrote: > On Sat, Jan 17, 2015 at 04:30:33PM -0800, Ray Jui wrote: >> >> >> On 1/17/2015 2:40 PM, Russell King - ARM Linux wrote: >>> On Sat, Jan 17, 2015 at 01:26:41PM -0800, Ray Jui wrote: >>>> time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); >>>> >>>> /* disable all interrupts */ >>>> writel(0, iproc_i2c->base + IE_OFFSET); >>>> >>>> if (!time_left && !atomic_read(&iproc_i2c->transfer_is_successful)) { >>> >>> Why are you using atomic_read() here? >>> >> transfer_is_successful 1) will be reset to 0 in this function (before >> kick start the I2C transfer), 2) will be set to 1 in the ISR (to signal >> completion of the I2C transfer), and 3) will be checked in this function >> here. I thought that means I should declare it volatile, because it can >> be modified in both the process context and interrupt context (and I use >> atomic because I remember Linux checkpatch warns against using volatile)? > > You don't need volatile or atomic_t for that. > > Rather than switching to atomic_t when seeing the checkpatch warning, > you'd do better to read Documentation/volatile-considered-harmful.txt > to understand why checkpatch issues the warning, and realise that you > don't need it for the above. > > Note that in the above code, the compiler can't make an assumption > about iproc_i2c->transfer_is_successful because it can't tell whether > a called function (eg, wait_for_completion_timeout()) could modify it. > Got it. Thanks. > Another possible issue with the above code are these lines: > > /* disable all interrupts */ > writel(0, iproc_i2c->base + IE_OFFSET); > > It would be nice to think that would hit the hardware immediately, but > that's making assumptions about hardware which are not necessary true. > Your interrupt handler could even be running on another CPU after you've > asked for that register to be written. > > Depending on what you're trying to achieve here, you may need: > > /* disable all interrupts */ > writel(0, iproc_i2c->base + IE_OFFSET); > /* read it back to ensure the write has hit */ > readl(iproc_i2c->base + IE_OFFSET); > > /* make sure the interrupt handler isn't running */ > synchronize_irq(...->irq); > > if what you're trying to do is to ensure that the interrupt handler has > finished running. > This will be the most robust way of handling this. Given that we've added an additional flag to check to make sure there's no interrupt missed after wait_for_completion_timeout times out, it makes sense to ensure that by the time when we check the flag there's no pending irq. I'll add this to the driver and make 'xfer_is_done' an 'int' instead of 'atomic_t'. I will also add the call to readl to flush the write in the remove function after interrupts are disabled.
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 31e8308..af76d23 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -372,6 +372,16 @@ config I2C_BCM2835 This support is also available as a module. If so, the module will be called i2c-bcm2835. +config I2C_BCM_IPROC + tristate "Broadcom iProc I2C controller" + depends on ARCH_BCM_IPROC || COMPILE_TEST + default ARCH_BCM_IPROC + help + If you say yes to this option, support will be included for the + Broadcom iProc I2C controller. + + If you don't know what to do here, say N. + config I2C_BCM_KONA tristate "BCM Kona I2C adapter" depends on ARCH_BCM_MOBILE diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 56388f6..d93b509 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_AT91) += i2c-at91.o obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o obj-$(CONFIG_I2C_AXXIA) += i2c-axxia.o obj-$(CONFIG_I2C_BCM2835) += i2c-bcm2835.o +obj-$(CONFIG_I2C_BCM_IPROC) += i2c-bcm-iproc.o obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o obj-$(CONFIG_I2C_CADENCE) += i2c-cadence.o obj-$(CONFIG_I2C_CBUS_GPIO) += i2c-cbus-gpio.o diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c new file mode 100644 index 0000000..7d9ed4e --- /dev/null +++ b/drivers/i2c/busses/i2c-bcm-iproc.c @@ -0,0 +1,503 @@ +/* + * Copyright (C) 2014 Broadcom 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. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/sched.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/delay.h> + +#define CFG_OFFSET 0x00 +#define CFG_RESET_SHIFT 31 +#define CFG_EN_SHIFT 30 +#define CFG_M_RETRY_CNT_SHIFT 16 +#define CFG_M_RETRY_CNT_MASK 0x0f + +#define TIM_CFG_OFFSET 0x04 +#define TIME_CFG_MODE_400_SHIFT 31 + +#define M_FIFO_CTRL_OFFSET 0x0c +#define M_FIFO_RX_FLUSH_SHIFT 31 +#define M_FIFO_TX_FLUSH_SHIFT 30 +#define M_FIFO_RX_CNT_SHIFT 16 +#define M_FIFO_RX_CNT_MASK 0x7f +#define M_FIFO_RX_THLD_SHIFT 8 +#define M_FIFO_RX_THLD_MASK 0x3f + +#define M_CMD_OFFSET 0x30 +#define M_CMD_START_BUSY_SHIFT 31 +#define M_CMD_STATUS_SHIFT 25 +#define M_CMD_STATUS_MASK 0x07 +#define M_CMD_STATUS_SUCCESS 0x0 +#define M_CMD_STATUS_LOST_ARB 0x1 +#define M_CMD_STATUS_NACK_ADDR 0x2 +#define M_CMD_STATUS_NACK_DATA 0x3 +#define M_CMD_STATUS_TIMEOUT 0x4 +#define M_CMD_PROTOCOL_SHIFT 9 +#define M_CMD_PROTOCOL_MASK 0xf +#define M_CMD_PROTOCOL_BLK_WR 0x7 +#define M_CMD_PROTOCOL_BLK_RD 0x8 +#define M_CMD_PEC_SHIFT 8 +#define M_CMD_RD_CNT_SHIFT 0 +#define M_CMD_RD_CNT_MASK 0xff + +#define IE_OFFSET 0x38 +#define IE_M_RX_FIFO_FULL_SHIFT 31 +#define IE_M_RX_THLD_SHIFT 30 +#define IE_M_START_BUSY_SHIFT 28 + +#define IS_OFFSET 0x3c +#define IS_M_RX_FIFO_FULL_SHIFT 31 +#define IS_M_RX_THLD_SHIFT 30 +#define IS_M_START_BUSY_SHIFT 28 + +#define M_TX_OFFSET 0x40 +#define M_TX_WR_STATUS_SHIFT 31 +#define M_TX_DATA_SHIFT 0 +#define M_TX_DATA_MASK 0xff + +#define M_RX_OFFSET 0x44 +#define M_RX_STATUS_SHIFT 30 +#define M_RX_STATUS_MASK 0x03 +#define M_RX_PEC_ERR_SHIFT 29 +#define M_RX_DATA_SHIFT 0 +#define M_RX_DATA_MASK 0xff + +#define I2C_TIMEOUT_MESC 100 +#define M_TX_RX_FIFO_SIZE 64 + +enum bus_speed_index { + I2C_SPD_100K = 0, + I2C_SPD_400K, +}; + +struct bcm_iproc_i2c_dev { + struct device *device; + int irq; + + void __iomem *base; + struct i2c_msg *msg; + + struct i2c_adapter adapter; + + struct completion done; +}; + +/* + * Can be expanded in the future if more interrupt status bits are utilized + */ +#define ISR_MASK (1 << IS_M_START_BUSY_SHIFT) + +static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data) +{ + struct bcm_iproc_i2c_dev *iproc_i2c = data; + u32 status = readl(iproc_i2c->base + IS_OFFSET); + + status &= ISR_MASK; + + if (!status) + return IRQ_NONE; + + writel(status, iproc_i2c->base + IS_OFFSET); + complete_all(&iproc_i2c->done); + + return IRQ_HANDLED; +} + +static int __wait_for_bus_idle(struct bcm_iproc_i2c_dev *iproc_i2c) +{ + unsigned long timeout = jiffies + msecs_to_jiffies(I2C_TIMEOUT_MESC); + + while (readl(iproc_i2c->base + M_CMD_OFFSET) & + (1 << M_CMD_START_BUSY_SHIFT)) { + if (time_after(jiffies, timeout)) { + dev_err(iproc_i2c->device, + "wait for bus idle timeout\n"); + return -ETIMEDOUT; + } + cpu_relax(); + } + + return 0; +} + +static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c, + struct i2c_msg *msg, u8 *addr) +{ + + if (msg->flags & I2C_M_TEN) { + dev_err(iproc_i2c->device, "no support for 10-bit address\n"); + return -EINVAL; + } + + *addr = (msg->addr << 1); + + if (msg->flags & I2C_M_RD) + *addr |= 1; + + return 0; +} + +static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c) +{ + u32 val; + + val = readl(iproc_i2c->base + M_CMD_OFFSET); + val = (val >> M_CMD_STATUS_SHIFT) & M_CMD_STATUS_MASK; + + switch (val) { + case M_CMD_STATUS_SUCCESS: + return 0; + + case M_CMD_STATUS_LOST_ARB: + dev_err(iproc_i2c->device, "lost bus arbitration\n"); + return -EREMOTEIO; + + case M_CMD_STATUS_NACK_ADDR: + dev_err(iproc_i2c->device, "NAK addr:0x%02x\n", + iproc_i2c->msg->addr); + return -EREMOTEIO; + + case M_CMD_STATUS_NACK_DATA: + dev_err(iproc_i2c->device, "NAK data\n"); + return -EREMOTEIO; + + case M_CMD_STATUS_TIMEOUT: + dev_err(iproc_i2c->device, "bus timeout\n"); + return -ETIMEDOUT; + + default: + dev_err(iproc_i2c->device, "unknown error code=%d\n", val); + return -EREMOTEIO; + } + + return -EREMOTEIO; +} + +static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, + struct i2c_msg *msg) +{ + int ret, i; + u8 addr; + u32 val; + unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC); + + if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) { + dev_err(iproc_i2c->device, + "supported data length is 1 - %u bytes\n", + M_TX_RX_FIFO_SIZE - 1); + return -EINVAL; + } + + iproc_i2c->msg = msg; + ret = __wait_for_bus_idle(iproc_i2c); + if (ret) + return ret; + + ret = bcm_iproc_i2c_format_addr(iproc_i2c, msg, &addr); + if (ret) + return ret; + + /* load slave address into the TX FIFO */ + writel(addr, iproc_i2c->base + M_TX_OFFSET); + + /* for a write transaction, load data into the TX FIFO */ + if (!(msg->flags & I2C_M_RD)) { + for (i = 0; i < msg->len; i++) { + val = msg->buf[i]; + + /* mark the last byte */ + if (i == msg->len - 1) + val |= 1 << M_TX_WR_STATUS_SHIFT; + + writel(val, iproc_i2c->base + M_TX_OFFSET); + } + } + + /* mark as incomplete before starting the transaction */ + reinit_completion(&iproc_i2c->done); + + /* + * Enable the "start busy" interrupt, which will be triggered after + * the transaction is done, i.e., the internal start_busy bit + * transitions from 1 to 0 + */ + writel(1 << IE_M_START_BUSY_SHIFT, iproc_i2c->base + IE_OFFSET); + + /* + * Now we can activate the transfer. For a read operation, specify the + * number of bytes to read + */ + val = 1 << M_CMD_START_BUSY_SHIFT; + if (msg->flags & I2C_M_RD) { + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | + (msg->len << M_CMD_RD_CNT_SHIFT); + } else { + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); + } + writel(val, iproc_i2c->base + M_CMD_OFFSET); + + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left); + + /* disable all interrupts */ + writel(0, iproc_i2c->base + IE_OFFSET); + + if (!time_left) { + dev_err(iproc_i2c->device, "transaction times out\n"); + + /* flush FIFOs */ + val = (1 << M_FIFO_RX_FLUSH_SHIFT) | + (1 << M_FIFO_TX_FLUSH_SHIFT); + writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET); + return -EREMOTEIO; + } + + ret = bcm_iproc_i2c_check_status(iproc_i2c); + if (ret) { + /* flush both TX/RX FIFOs */ + val = (1 << M_FIFO_RX_FLUSH_SHIFT) | + (1 << M_FIFO_TX_FLUSH_SHIFT); + writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET); + return ret; + } + + /* + * For a read operation, we now need to load the data from FIFO + * into the memory buffer + */ + if (msg->flags & I2C_M_RD) { + for (i = 0; i < msg->len; i++) { + msg->buf[i] = (readl(iproc_i2c->base + M_RX_OFFSET) >> + M_RX_DATA_SHIFT) & M_RX_DATA_MASK; + } + } + + dev_dbg(iproc_i2c->device, "xfer %c, addr=0x%02x, len=%d\n", + (msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr, + msg->len); + dev_dbg(iproc_i2c->device, "**** data start ****\n"); + for (i = 0; i < msg->len; i++) + dev_dbg(iproc_i2c->device, "0x%02x ", msg->buf[i]); + dev_dbg(iproc_i2c->device, "**** data end ****\n"); + + return 0; +} + +static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter, + struct i2c_msg msgs[], int num) +{ + struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adapter); + int ret, i; + + /* go through all messages */ + for (i = 0; i < num; i++) { + ret = bcm_iproc_i2c_xfer_single_msg(iproc_i2c, &msgs[i]); + if (ret) { + dev_err(iproc_i2c->device, "xfer failed\n"); + return ret; + } + } + + return num; +} + +static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; +} + +static const struct i2c_algorithm bcm_iproc_algo = { + .master_xfer = bcm_iproc_i2c_xfer, + .functionality = bcm_iproc_i2c_functionality, +}; + +static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) +{ + unsigned int bus_speed, speed_bit; + u32 val; + int ret = of_property_read_u32(iproc_i2c->device->of_node, + "clock-frequency", &bus_speed); + if (ret < 0) { + dev_err(iproc_i2c->device, + "missing clock-frequency property\n"); + return -ENODEV; + } + + if (bus_speed < 100000) { + dev_err(iproc_i2c->device, "%d Hz bus speed not supported\n", + bus_speed); + dev_err(iproc_i2c->device, + "valid speeds are 100khz and 400khz\n"); + return -EINVAL; + } else if (bus_speed < 400000) { + speed_bit = 0; + } else { + /* bus_speed >= 400000 */ + speed_bit = 1; + } + + val = readl(iproc_i2c->base + TIM_CFG_OFFSET); + val &= ~(1 << TIME_CFG_MODE_400_SHIFT); + val |= speed_bit << TIME_CFG_MODE_400_SHIFT; + writel(val, iproc_i2c->base + TIM_CFG_OFFSET); + + dev_info(iproc_i2c->device, "bus set to %u Hz\n", bus_speed); + + return 0; +} + +static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c) +{ + u32 val; + + /* put controller in reset */ + val = readl(iproc_i2c->base + CFG_OFFSET); + val |= 1 << CFG_RESET_SHIFT; + val &= ~(1 << CFG_EN_SHIFT); + writel(val, iproc_i2c->base + CFG_OFFSET); + + /* wait 100 usec per spec */ + udelay(100); + + /* bring controller out of reset */ + val &= ~(1 << CFG_RESET_SHIFT); + writel(val, iproc_i2c->base + CFG_OFFSET); + + /* flush TX/RX FIFOs and set RX FIFO threshold to zero */ + val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT); + writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET); + + /* disable all interrupts */ + writel(0, iproc_i2c->base + IE_OFFSET); + + /* clear all pending interrupts */ + writel(0xffffffff, iproc_i2c->base + IS_OFFSET); + + return 0; +} + +static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *iproc_i2c) +{ + u32 val; + + val = readl(iproc_i2c->base + CFG_OFFSET); + val |= 1 << CFG_EN_SHIFT; + writel(val, iproc_i2c->base + CFG_OFFSET); +} + +static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *iproc_i2c) +{ + u32 val; + + val = readl(iproc_i2c->base + CFG_OFFSET); + val &= ~(1 << CFG_EN_SHIFT); + writel(val, iproc_i2c->base + CFG_OFFSET); +} + +static int bcm_iproc_i2c_probe(struct platform_device *pdev) +{ + int irq, ret = 0; + struct bcm_iproc_i2c_dev *iproc_i2c; + struct i2c_adapter *adap; + struct resource *res; + + iproc_i2c = devm_kzalloc(&pdev->dev, sizeof(*iproc_i2c), + GFP_KERNEL); + if (!iproc_i2c) + return -ENOMEM; + + platform_set_drvdata(pdev, iproc_i2c); + iproc_i2c->device = &pdev->dev; + init_completion(&iproc_i2c->done); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + iproc_i2c->base = devm_ioremap_resource(iproc_i2c->device, res); + if (IS_ERR(iproc_i2c->base)) + return PTR_ERR(iproc_i2c->base); + + ret = bcm_iproc_i2c_init(iproc_i2c); + if (ret) + return ret; + + ret = bcm_iproc_i2c_cfg_speed(iproc_i2c); + if (ret) + return ret; + + irq = platform_get_irq(pdev, 0); + if (irq <= 0) { + dev_err(iproc_i2c->device, "no irq resource\n"); + return irq; + } + iproc_i2c->irq = irq; + + ret = request_irq(irq, bcm_iproc_i2c_isr, 0, pdev->name, iproc_i2c); + if (ret < 0) { + dev_err(iproc_i2c->device, "unable to request irq %i\n", irq); + return ret; + } + + bcm_iproc_i2c_enable(iproc_i2c); + + adap = &iproc_i2c->adapter; + i2c_set_adapdata(adap, iproc_i2c); + strlcpy(adap->name, "Broadcom iProc I2C adapter", sizeof(adap->name)); + adap->algo = &bcm_iproc_algo; + adap->dev.parent = &pdev->dev; + adap->dev.of_node = pdev->dev.of_node; + + ret = i2c_add_adapter(adap); + if (ret) { + dev_err(iproc_i2c->device, "failed to add adapter\n"); + free_irq(iproc_i2c->irq, iproc_i2c); + return ret; + } + + return 0; +} + +static int bcm_iproc_i2c_remove(struct platform_device *pdev) +{ + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); + + i2c_del_adapter(&iproc_i2c->adapter); + free_irq(iproc_i2c->irq, iproc_i2c); + bcm_iproc_i2c_disable(iproc_i2c); + + return 0; +} + +static const struct of_device_id bcm_iproc_i2c_of_match[] = { + {.compatible = "brcm,iproc-i2c",}, + {}, +}; +MODULE_DEVICE_TABLE(of, bcm_iproc_i2c_of_match); + +static struct platform_driver bcm_iproc_i2c_driver = { + .driver = { + .name = "bcm-iproc-i2c", + .of_match_table = bcm_iproc_i2c_of_match, + }, + .probe = bcm_iproc_i2c_probe, + .remove = bcm_iproc_i2c_remove, +}; +module_platform_driver(bcm_iproc_i2c_driver); + +MODULE_AUTHOR("Ray Jui <rjui@broadcom.com>"); +MODULE_DESCRIPTION("Broadcom iProc I2C Driver"); +MODULE_LICENSE("GPL v2");