[PATCH] [RFC] cmd: i2c: fix default address len for DM_I2C

[Tim Harvey]

On Sat, Aug 13, 2022 at 7:59 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Tim,
>
> On Thu, 11 Aug 2022 at 11:57, Tim Harvey <tharvey at gateworks.com> wrote:
> >
> > According to the comment block "The default {addr} parameter is one byte
> > (.1) which works well for memories and registers with 8 bits of address
> > space."
> >
> > While this is true for legacy I2C a default length of -1 is being passed
> > for DM_I2C which results in a usage error.
> >
> > Restore the documented behavior by always using a default alen of 1.
> >
> > Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> >
> > This is an RFC as I'm unclear if we want to restore the legacy usage or
> > enforce a new usage (in which case the comment block should be updated)
> > and I'm not clear if this is documented in other places. If the decision
> > is to enforce a new usage then it is unclear to me how to specifiy the
> > default alen as there is no command for that (i2c alen [len]?).
> > ---
> >  cmd/i2c.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> >
>
> Can you dig into this a little more on your board? DEFAULT_ADDR_LEN is
> set to -1 so that by default it does not get set by the command, and
> the existing alen is used.
>
> This is necessary for driver model, since the alen can be set by the
> peripheral device and we don't want to overwrite it:
>
> if (!ret && alen != -1)
>     ret = i2c_set_chip_offset_len(dev, alen);
>

Simon,

Here's some annotated debug prints added where chip_offset is passed/set:
Model: Gateworks Venice GW73xx-0x i.MX8MM Development Kit
DRAM:  1 GiB
i2c_chip_of_to_plat gsc at 20 offset_len=1
i2c_chip_of_to_plat pmic at 69 offset_len=1
i2c_get_chip i2c at 30a20000 0x51 offset_len=1
i2c_bind_driver i2c at 30a20000 offset_len=1
i2c_set_chip_offset_len generic_51 offset_len=1
dm_i2c_read generic_51 offset=0 offset_len=1
i2c_setup_offset 0x51 offset=0 offset_len=1
Core:  209 devices, 27 uclasses, devicetree: separate
...
u-boot=> i2c dev 0 && i2c md 0x51 0 50
Setting bus to 0
i2c - I2C sub-system

Usage:
i2c bus [muxtype:muxaddr:muxchannel] - show I2C bus info
...

So the chip I noticed this issue with is 0x51 which an atmel,24c02
compatible eeprom which imx8mm-venice-gw700x.dtsi defines 4 of
(i2c1-0x50, i2c1-0x51, i2c1-0x52, i2c1-0x53). You can see above
i2c1-0x51 (i2c1=i2c at 30a20000) is accessed early as it holds the board
model information and at that point in time it's accessed with alen=1
(which I specify in board/gateworks/venice/eeprom.c:eeprom_read()) but
by the time the 'i2c md 0x51 0 50' comes around
i2c_get_chip_offset_len() returns 0 for this device. The other eeprom
devices on i2c1 are never accessed by a driver so they would never
have a 'default' alen set.

Where is the initial setting of alen supposed to have come?

Best Regards,

Tim

>
> > diff --git a/cmd/i2c.c b/cmd/i2c.c
> > index bd04b14024be..c57271479e81 100644
> > --- a/cmd/i2c.c
> > +++ b/cmd/i2c.c
> > @@ -118,17 +118,7 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES;
> >  #endif
> >
> >  #define DISP_LINE_LEN  16
> > -
> > -/*
> > - * Default for driver model is to use the chip's existing address length.
> > - * For legacy code, this is not stored, so we need to use a suitable
> > - * default.
> > - */
> > -#if CONFIG_IS_ENABLED(DM_I2C)
> > -#define DEFAULT_ADDR_LEN       (-1)
> > -#else
> >  #define DEFAULT_ADDR_LEN       1
> > -#endif
> >
> >  #if CONFIG_IS_ENABLED(DM_I2C)
> >  static struct udevice *i2c_cur_bus;
> > --
> > 2.25.1
> >
>
> Regards,
> Simon