[U-Boot] [PATCH 2/2] i2c: mxc_i2c: Fix read and read->write xfers in DM mode
Heiko Schocher
hs at denx.de
Tue Apr 30 04:34:26 UTC 2019
Hello Trent,
Am 16.04.2019 um 00:02 schrieb Trent Piepho:
> This is an old driver that supports both device mapped and non-mapped
> mode, and covers a wide range of hardware. It's hard to change without
> risking breaking something. I have to tried to be exceedingly detailed
> in this patch, so please excuse the length of the commit essay that
> follows.
>
> In device mapped mode the I2C xfer function does not handle plain read,
> and some other, transfers correctly.
>
> What it can't handle are transactions that:
> Start with a read, or,
> Have a write followed by a read, or,
> Have more than one read in a row.
>
> The common I2C/SMBUS read register and write register transactions
> always start with a write, followed by a write or a read, and then end.
> These work, so the bug is not apparent for most I2C slaves that only use
> these common xfer forms.
>
> The existing xfer loop initializes by sending the chip address in write
> mode after it deals with bus arbitration and master setup. When
> processing each message, if the next message will be a read, it sends a
> repeated start followed by the chip address in read mode after the
> current message.
>
> Obviously, this does not work if the first message is a read, as the
> chip is always addressed in write mode initially by i2c_init_transfer().
>
> A write following a read does not work because the repeated start is
> only sent when the next message is a read. There is no logic to send it
> when the current message is a read and next is write. It should be sent
> every time the bus changes direction.
>
> The ability to use a plain read was added to this driver in
> commit 2feec4eafd40 ("imx: mxc_i2c: tweak the i2c transfer method"),
> but this applied only the non-DM code path.
>
> This patch fixes the DM code path. The xfer function will call
> i2c_init_transfer() with an alen of -1 to avoid sending the chip
> address. The same way the non-DM code achieves this. The xfer
> function's message loop will send the address and mode before each
> message if the bus changes direction, and on the first message.
>
> When reading data, the master hardware is one byte ahead of what we
> receive. I.e., reading a byte from the data register returns a byte
> *already received* by the master, and causes the master to start the RX
> of the *next* byte. Therefor, before we read the final byte of a
> message, we must tell the master what to do next. I add a "last" flag
> to i2c_read_data() to tell it if the message is to be followed by a stop
> or a repeated start. When last == true it acts exactly as before.
>
> The non-DM code can only create an xfer where the read, if any, is the
> final message of the xfer. And so the only callsite of i2c_read_data()
> in the non-DM code has the "last" parameter as true. Therefore, this
> change has no effect on the non-DM code. As all other changes are in
> the DM xfer function, which is not even compiled in non-DM code, I am
> confident that this patch has no effect on boards not using I2C_DM.
> This greatly reduces the change of hardware that could be affected.
>
> For DM boards, I have verified every transaction the "i2c" command can
> create on a scope and they are all exactly as they are supposed to be.
> I also tested write->read->write, which isn't possible with the i2c
> command, and it works as well. I didn't fix multiple reads in a row, as
> it's a lot more invasive and obviously no one has every wanted them
> since they've never worked. It didn't seem like the extra complexity
> was justified to support something no one uses.
>
> Cc: Nandor Han <nandor.han at ge.com>
> Cc: Heiko Schocher <hs at denx.de>
> Cc: Stefano Babic <sbabic at denx.de>
> Cc: Fabio Estevam <festevam at gmail.com>
> Cc: Breno Matheus Lima <brenomatheus at gmail.com>
> Signed-off-by: Trent Piepho <tpiepho at impinj.com>
> ---
> drivers/i2c/mxc_i2c.c | 95 ++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 64 insertions(+), 31 deletions(-)
Thanks for this work!
Your patch has
total: 64 errors, 2 warnings, 0 checks, 145 lines checked
Please fix and send a v2.
It would be good to become here some Tested by tags ...
Reviewed-by: Heiko Schocher <hs at denx.de>
bye,
Heiko
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index f17a47f600..23119cce65 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
> @@ -482,8 +482,13 @@ static int i2c_write_data(struct mxc_i2c_bus *i2c_bus, u8 chip, const u8 *buf,
> return ret;
> }
>
> +/* Will generate a STOP after the last byte if "last" is true, i.e. this is the
> + * final message of a transaction. If not, it switches the bus back to TX mode
> + * and does not send a STOP, leaving the bus in a state where a repeated start
> + * and address can be sent for another message.
> + */
> static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
> - int len)
> + int len, bool last)
> {
> int ret;
> unsigned int temp;
> @@ -513,17 +518,31 @@ static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
> return ret;
> }
>
> - /*
> - * It must generate STOP before read I2DR to prevent
> - * controller from generating another clock cycle
> - */
> if (i == (len - 1)) {
> - i2c_imx_stop(i2c_bus);
> + /* Final byte has already been received by master! When
> + * we read it from I2DR, the master will start another
> + * cycle. We must program it first to send a STOP or
> + * switch to TX to avoid this.
> + */
> + if (last) {
> + i2c_imx_stop(i2c_bus);
> + } else {
> + /* Final read, no stop, switch back to tx */
> + temp = readb(base + (I2CR << reg_shift));
> + temp |= I2CR_MTX | I2CR_TX_NO_AK;
> + writeb(temp, base + (I2CR << reg_shift));
> + }
> } else if (i == (len - 2)) {
> + /* Master has already recevied penultimate byte. When
> + * we read it from I2DR, master will start RX of final
> + * byte. We must set TX_NO_AK now so it does not ACK
> + * that final byte.
> + */
> temp = readb(base + (I2CR << reg_shift));
> temp |= I2CR_TX_NO_AK;
> writeb(temp, base + (I2CR << reg_shift));
> }
> +
> writeb(I2SR_IIF_CLEAR, base + (I2SR << reg_shift));
> buf[i] = readb(base + (I2DR << reg_shift));
> }
> @@ -533,7 +552,9 @@ static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
> debug(" 0x%02x", buf[ret]);
> debug("\n");
>
> - i2c_imx_stop(i2c_bus);
> + /* It is not clear to me that this is necessary */
> + if (last)
> + i2c_imx_stop(i2c_bus);
> return 0;
> }
>
> @@ -585,7 +606,7 @@ static int bus_i2c_read(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr,
> return ret;
> }
>
> - ret = i2c_read_data(i2c_bus, chip, buf, len);
> + ret = i2c_read_data(i2c_bus, chip, buf, len, true);
>
> i2c_imx_stop(i2c_bus);
> return ret;
> @@ -939,42 +960,54 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
> ulong base = i2c_bus->base;
> int reg_shift = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
> VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
> + int read_mode;
>
> - /*
> - * Here the 3rd parameter addr and the 4th one alen are set to 0,
> - * because here we only want to send out chip address. The register
> - * address is wrapped in msg.
> + /* Here address len is set to -1 to not send any address at first.
> + * Otherwise i2c_init_transfer will send the chip address with write
> + * mode set. This is wrong if the 1st message is read.
> */
> - ret = i2c_init_transfer(i2c_bus, msg->addr, 0, 0);
> + ret = i2c_init_transfer(i2c_bus, msg->addr, 0, -1);
> if (ret < 0) {
> debug("i2c_init_transfer error: %d\n", ret);
> return ret;
> }
>
> + read_mode = -1; /* So it's always different on the first message */
> for (; nmsgs > 0; nmsgs--, msg++) {
> - bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD);
> - debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
> - if (msg->flags & I2C_M_RD)
> - ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
> - msg->len);
> - else {
> - ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
> - msg->len);
> - if (ret)
> - break;
> - if (next_is_read) {
> - /* Reuse ret */
> + const int msg_is_read = !!(msg->flags & I2C_M_RD);
> +
> + debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n", msg->addr,
> + msg->len, msg_is_read ? 'R' : 'W');
> +
> + if (msg_is_read != read_mode) {
> + /* Send repeated start if not 1st message */
> + if (read_mode != -1) {
> + debug("i2c_xfer: [RSTART]\n");
> ret = readb(base + (I2CR << reg_shift));
> ret |= I2CR_RSTA;
> writeb(ret, base + (I2CR << reg_shift));
> -
> - ret = tx_byte(i2c_bus, (msg->addr << 1) | 1);
> - if (ret < 0) {
> - i2c_imx_stop(i2c_bus);
> - break;
> - }
> }
> + debug("i2c_xfer: [ADDR %02x | %c]\n", msg->addr,
> + msg_is_read ? 'R' : 'W');
> + ret = tx_byte(i2c_bus, (msg->addr << 1) | msg_is_read);
> + if (ret < 0) {
> + debug("i2c_xfer: [STOP]\n");
> + i2c_imx_stop(i2c_bus);
> + break;
> + }
> + read_mode = msg_is_read;
> }
> +
> + if (msg->flags & I2C_M_RD)
> + ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
> + msg->len, nmsgs == 1 ||
> + (msg->flags & I2C_M_STOP));
> + else
> + ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
> + msg->len);
> +
> + if (ret < 0)
> + break;
> }
>
> if (ret)
>
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de
More information about the U-Boot
mailing list