Re: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART

[Vincent Jardin <vjardin@free.fr>]

Hi Carlos,

Thanks a lot for the review, and no problem for the delay.

> This is a such rare i2c frame design in the Realtek RTL8366SE SMI read
> frame, right?

It is rare, but I have been inspired for I2C_M_REV_DIR_ADDR by the source
of two client drivers that use it.

Note that I2C_M_NOSTART is not rare at all. It is what regmap uses for a real
gather write. regmap_i2c_gather_write() opens with

        if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_NOSTART))
                return -ENOTSUPP;

FYI, some clients that need REV_DIR_ADDR and NOSTART,

  drivers/input/joystick/as5011.c
  drivers/video/fbdev/matrox/matroxfb_maven.c

both build the identical 2-message pattern,

  i2c_check_functionality(adapter,
                                I2C_FUNC_NOSTART |
                                I2C_FUNC_PROTOCOL_MANGLING)

I do not have those devices, so I did not check it beside code readings.

Some clients that need NOSTART alone,

  drivers/base/regmap/regmap-i2c.c
  drivers/infiniband/hw/hfi1/qsfp.c
  drivers/gpu/drm/i915/display/dvo_ivch.c

About the i2c masters, that use I2C_M_REV_DIR_ADDR in code,

  drivers/i2c/algos/i2c-algo-bit.c
  drivers/i2c/algos/i2c-algo-pcf.c
  drivers/i2c/busses/i2c-s3c2410.c <- the model I did investigate
  drivers/i2c/busses/i2c-tegra-bpmp.c
  drivers/media/pci/cobalt/cobalt-i2c.c

So a taxnonomy can be,

  adapter            REV_DIR impl   MANGLING adv   NOSTART adv   usable
  i2c-algo-bit           yes            yes            yes         yes
  i2c-algo-pcf           yes            yes            no          no
  i2c-s3c2410            yes            yes            yes         yes
  i2c-tegra-bpmp         yes            yes            yes         yes
  cobalt-i2c             yes         (private adapter, not exposed)
  i2c-brcmstb            no             yes            yes         no
  i2c-pxa                no             yes            yes         no
  i2c-tegra              no             yes         yes (cond)     no
  i2c-imx (this patch)   yes            yes            yes         yes

> Is Realtek RTL8366SE SMI driver upstream? Can I found the driver?

Not yet, and I cannot point you at a tree today. It is Realtek's
"Unmanaged Switch" DSA driver, which is not public yet.

What I can share is the message construction, which is the part you
asked about and is not Realtek-specific.

        static int as5011_i2c_read(struct i2c_client *client,
                                   uint8_t aregaddr, signed char *value)
        {
                uint8_t data[2] = { aregaddr };
                struct i2c_msg msg_set[2] = {
                        {
                                .addr = client->addr,
                                .flags = I2C_M_REV_DIR_ADDR,
                                .len = 1,
                                .buf = (uint8_t *)data
                        },
                        {
                                .addr = client->addr,
                                .flags = I2C_M_RD | I2C_M_NOSTART,
                                .len = 1,
                                .buf = (uint8_t *)data
                        }
                };
                int error;

                error = i2c_transfer(client->adapter, msg_set, 2);
                if (error < 0)
                        return error;

                *value = data[0] & 0x80 ? -1 * (1 + ~data[0]) : data[0];
                return 0;
        }

And the Realtek accessor, which is the same two messages with a 2-byte
register and 2-byte data instead of 1 and 1:

        u8 ra[2]   = { reg & 0xff, (reg >> 8) & 0xff };
        u8 data[2] = { 0xff, 0xff };
        struct i2c_msg msgs[2] = {
                {
                        .addr  = client->addr,
                        .flags = I2C_M_REV_DIR_ADDR,
                        .len   = sizeof(ra),
                        .buf   = ra,
                }, {
                        .addr  = client->addr,
                        .flags = I2C_M_RD | I2C_M_NOSTART,
                        .len   = sizeof(data),
                        .buf   = data,
                },
        };

        ret = i2c_transfer(client->adapter, msgs, 2);
        if (ret != 2)
                return ret < 0 ? ret : -EIO;

        *val = data[0] | (data[1] << 8);

msgs[0] is a write message carrying I2C_M_REV_DIR_ADDR, so the address
byte goes out with the read bit set while the master keeps transmitting
the two register-address bytes. msgs[1] is the read half with
I2C_M_NOSTART, so no repeated start is emitted and the controller simply
turns the bus around. Writes are an ordinary unflagged 4-byte write and
need nothing from this patch.

> ... Have you test this i2c-imx feature in your LS board with RTL8366SE?

Yes, on an LX2160A board carrying four RTL8366SE-CG. Two are strapped to
the chip's 2-wire "EEPROM SMI" mode and hang off hardware i2c-imx
controllers; the other two are on MDIO, for both the same switch registers
are reachable both ways and should answer the same values.

First, the failure on an unpatched kernel, which is reproducible with
nothing but i2ctools and is I think the clearest way to see the problem.
These two commands are byte-identical:

        # i2ctransfer -y -f -a 1 w2@0x7c 0x00 0x13 r4@0x7c
        0xff 0xff 0x00 0x00
        # i2ctransfer -y -f -a 1 r6@0x7c        # no preceding write at all
        0xff 0xff 0x00 0x00 0x00 0x00

Then, with this patch both chips answer correctly. For testing this patch,
I did use the following that I have just pushed to help, for the record:

        https://github.com/vjardin/smi-probe

        # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c id
        chip_num    0x6980  CHIP_RTL8367E (inside RTL8366SE-CG)
        chip_ver    0x0030
        svlan_tpid  0x88a8  reset default, as expected

        # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c rd 0x1300
        reg 0x1300 = 0x6980
        # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c rd 0x1202
        reg 0x1202 = 0x88a8

        # smi-probe -t i2c -b /dev/i2c-2 -a 0x5c rd 0x1202 # U19, IIC3
        reg 0x1202 = 0x88a8
        # smi-probe -t mdio -b 0x8b97000 -a 0x1a rd 0x1202 # MDIO control
        reg 0x1202 = 0x88a8

Then it binds as a DSA switch and enumerates its four user ports:

        realtek-US-switch-dsa-i2c 2-005c: RTK DSA unit 0 (EEPROM SMI, addr 0x5c)

I have many other i2c devices on this board, no regression.

Thanks again for your review,
  Vincent