| Message ID | 20130412093003.GE30416@pengutronix.de |
|---|---|
| State | RFC |
| Headers | show |
Hi Uwe, please prefer wsa@the-dreams.de for kernel stuff. > - Is it correct to have I2C_FUNC_SMBUS_EMUL in .functionality without > supporting I2C_M_RECV_LEN? Yes. Check the #define and you'll see that it does not contain BLOCK_READ and BLOCK_PROC_CALL. All the best, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 12, 2013 at 05:37:57PM +0200, Wolfram Sang wrote: > please prefer wsa@the-dreams.de for kernel stuff. The reason I used wolfram@... is probably: $ for addr in sa olfram; do printf "%s: " $addr; git log --oneline --grep=w$addr@the-dreams.de v3.9-rc6 | wc -l; done sa: 7 olfram: 35 But I see the recent commits use "sa" so probably I will do that next time, too. > > - Is it correct to have I2C_FUNC_SMBUS_EMUL in .functionality without > > supporting I2C_M_RECV_LEN? > > Yes. Check the #define and you'll see that it does not contain > BLOCK_READ and BLOCK_PROC_CALL. Ah, ok. But then there is a different problem: Even though "my" driver only advertises I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL calling i2c_smbus_read_block_data in userspace results in .master_xfer being called with I2C_M_RECV_LEN set. Best regards Uwe
Hi Uwe, > The reason I used wolfram@... is probably: Looking up MAINTAINERS would have done it :) > Ah, ok. But then there is a different problem: Even though "my" driver > only advertises I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL calling > i2c_smbus_read_block_data in userspace results in .master_xfer being > called with I2C_M_RECV_LEN set. From Documentation/i2c/functionality: Because not every I2C or SMBus adapter implements everything in the I2C specifications, a client can not trust that everything it needs is implemented when it is given the option to attach to an adapter: the client needs some way to check whether an adapter has the needed functionality... Regards, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Uwe, CCing Lucas Stach too, are you two not in the same office now ? ;-) > Hello, > > I'm currently trying to teach the i2c-mxs driver to handle the > I2C_M_RECV_LEN flag but failed up to now. Maybe someone has an good idea > how to make it running? > > My current patch looks as follows: Try basing your patch on top of Lucas's, they looked very reasonable. I think WS should have applied them by now. Wolfram, what's the status of those three mxs- i2c patches from Lucas, can you tell? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Wolfram, On Sun, Apr 14, 2013 at 01:57:58PM +0200, Wolfram Sang wrote: > > Ah, ok. But then there is a different problem: Even though "my" driver > > only advertises I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL calling > > i2c_smbus_read_block_data in userspace results in .master_xfer being > > called with I2C_M_RECV_LEN set. > > From Documentation/i2c/functionality: > > Because not every I2C or SMBus adapter implements everything in the > I2C specifications, a client can not trust that everything it needs > is implemented when it is given the option to attach to an adapter: > the client needs some way to check whether an adapter has the needed > functionality... While add support for I2C_M_RECV_LEN I forgot to write the length data to the first byte in the message buffer which happend to be initialized with 0xff. This made i2c_smbus_xfer_emulated copy 255 bytes to data->block overflowing the array and so resulting in stack curruption. I think the same could be accomplished with a non-broken driver (e.g. by calling i2c_smbus_read_block_data for an eeprom that is interpreted as a 1 byte read by the i2c bus driver. If the read byte is big enough the same stack curruption occurs). So IMHO the i2c core should be a bit more careful here and either not let i2c_smbus_xfer_emulated call the xfer callback of a driver that is not capable to handle I2C_M_RECV_LEN with a message that has this bit set or at least assert that data->block isn't written to out of bounds. Best regards Uwe
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -1,3 +1,4 @@ +#define DEBUG /* * Freescale MXS I2C bus driver * @@ -289,14 +290,23 @@ write_init_pio_fail: static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c) { unsigned long timeout = jiffies + msecs_to_jiffies(1000); + u32 reg, reg_old; + int first = 1; - while (!(readl(i2c->regs + MXS_I2C_DEBUG0) & + while (!(reg = readl(i2c->regs + MXS_I2C_DEBUG0) & MXS_I2C_DEBUG0_DMAREQ)) { + if (first || reg != reg_old) { + pr_debug("%s: I2C_DEBUG = 0x%08x\n", __func__, reg); + first = 0; + reg_old = reg; + } if (time_after(jiffies, timeout)) return -ETIMEDOUT; cond_resched(); } + pr_debug("%s: I2C_DEBUG = 0x%08x\n", __func__, reg); + writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR); return 0; @@ -338,6 +348,9 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR); if (msg->flags & I2C_M_RD) { + int readlen = msg->len; + size_t buf_offset = 0; + addr_data |= I2C_SMBUS_READ; /* SELECT command. */ @@ -354,19 +367,39 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, if (ret) return ret; + if (msg->flags & I2C_M_RECV_LEN) { + pr_debug("read first byte\n"); + writel(MXS_I2C_CTRL0_RUN | MXS_I2C_CTRL0_MASTER_MODE | + MXS_I2C_CTRL0_XFER_COUNT(1), + i2c->regs + MXS_I2C_CTRL0); + + ret = mxs_i2c_pio_wait_dmareq(i2c); + if (ret) + return ret; + + data = readl(i2c->regs + MXS_I2C_DATA) & 0xff; + if (data > I2C_SMBUS_BLOCK_MAX) + return -EPROTO; + + msg->len += data; + readlen += data - 1; + } + + pr_debug("%s: data = %u, msg->len = %u, readlen = %u\n", __func__, data, msg->len, readlen); + /* READ command. */ writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ | flags | - MXS_I2C_CTRL0_XFER_COUNT(msg->len), + MXS_I2C_CTRL0_XFER_COUNT(readlen), i2c->regs + MXS_I2C_CTRL0); - for (i = 0; i < msg->len; i++) { + for (i = 0; i < readlen; i++) { if ((i & 3) == 0) { ret = mxs_i2c_pio_wait_dmareq(i2c); if (ret) return ret; data = readl(i2c->regs + MXS_I2C_DATA); } - msg->buf[i] = data & 0xff; + msg->buf[buf_offset + i] = data & 0xff; data >>= 8; } } else { @@ -438,6 +471,9 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, * is set to 8 bytes, transfers shorter than 8 bytes are transfered * using PIO mode while longer transfers use DMA. The 8 byte border is * based on this empirical measurement and a lot of previous frobbing. + * + * Note that only mxs_i2c_pio_setup_xfer has support for I2C_M_RECV_LEN. + * That is OK as msg->len is <= 2 when that flag is set. */ if (msg->len < 8) { ret = mxs_i2c_pio_setup_xfer(adap, msg, flags); @@ -497,6 +533,8 @@ static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id) struct mxs_i2c_dev *i2c = dev_id; u32 stat = readl(i2c->regs + MXS_I2C_CTRL1) & MXS_I2C_IRQ_MASK; + pr_debug("%s: stat = 0x%08x\n", __func__, stat); + if (!stat) return IRQ_NONE;
Hello, I'm currently trying to teach the i2c-mxs driver to handle the I2C_M_RECV_LEN flag but failed up to now. Maybe someone has an good idea how to make it running? My current patch looks as follows: The problem is that mxs_i2c_pio_wait_dmareq that is called in the if (msg->flags & I2C_M_RECV_LEN) block times out, i.e. the hardware doesn't set MXS_I2C_DEBUG0_DMAREQ after the read request for the length byte. The same problem happens when I use the dma path to implement I2C_M_RECV_LEN support. In that case mxs_i2c_isr is called, but the dma callback is not. On the wire the following happens for a i2c_smbus_read_block_data call (from userspace): S $clientaddr W A $command A Sr $clientaddr R A 0x01 as expected and then the i.MX28 starts sending an ack: SDA ..____________/¯¯¯\____ SCL .._/¯\_/¯\_/¯\_/¯\_/¯¯¯ .. 5 6 7 8 9 slave sending | mxs starts length byte (1) | to send A but MXS_I2C_DEBUG0 stays 0 while mxs_i2c_pio_wait_dmareq waits. After that a P is put on the wire. That fits to what I see when using dma: HW_APBX_CH6_DEBUG1.STATEMACHINE == 0x15 meaning: WAIT_END — When the Wait for Command End bit is set, the state machine enters this state until the DMA device indicates that the command is complete. So the problem seems to be that the i2c core fails to tell the dma core that it's done. Other things I wonder about in the i2c-mxs driver: - I don't understand why RETAIN_CLOCK is added to MXS_CMD_I2C_SELECT. From my understanding of i2c and the hardware manual I'd say this is only relevant for slave mode? Setting RETAIN_CLOCK for the cycle that reads the length byte is broken in a different way. (After reading the 1 the mxs pulls low SDA for acking but doesn't pulse SCL. Then another 9 clock pulses are sent with SDA high. After the 9th falling edge SDA is pulled low by the i.MX28 and then both lines go back to 1 at the same time. I will debug a bit more here.) So it seems RETAIN_CLOCK has a meaning for master mode, too. If you have a more concrete perception of its meaning it would be great if you could share it. - Is it correct to have I2C_FUNC_SMBUS_EMUL in .functionality without supporting I2C_M_RECV_LEN? Best regards Uwe