[U-Boot] [PATCH 16/37] i2c: add support for offset overflow in to address
Robert Beckett
bob.beckett at collabora.com
Thu Oct 17 14:35:01 UTC 2019
On Tue, 2019-10-15 at 21:40 -0600, Simon Glass wrote:
> Hi Robert,
>
> On Tue, 15 Oct 2019 at 09:55, Robert Beckett <
> bob.beckett at collabora.com> wrote:
> > Some devices (2 wire eeproms for example) use some bits from the
> > chip
> > address to represent the high bits of the offset instead of or as
> > well
> > as using multiple bytes for the offset, effectively stealing chip
> > addresses on the bus.
> >
> > Add a chip offset mask that can be set for any i2c chip which gets
> > filled with the offset overflow during offset setup.
> >
> > Signed-off-by: Robert Beckett <bob.beckett at collabora.com>
> > Signed-off-by: Ian Ray <ian.ray at ge.com>
> > ---
> > drivers/i2c/i2c-uclass.c | 32 ++++++++++++++++++++++++++------
> > include/i2c.h | 24 ++++++++++++++++++++++++
> > 2 files changed, 50 insertions(+), 6 deletions(-)
>
> Please can you update the i2c tests to cover this new feature?
Sure, will do.
Having looked at the i2c testing available in test/dm/i2c.c, I'm
struggling to see how the current tests are valid.
The changes in offset leng for 3 and 4 byte offsets don't set the
correct offset length in dm_test_i2c_offset.
Also, there is no visibility of the effect under test to verify the
correct behaviour. It seems to be relying on predictable offset mapping
in to an assumed 256 byte storage in drivers/misc/i2c_eeprom_emul.c,
but as long as the mapping is consistent between read and write, it
could be all sorts of wrong and the test would not show any issue.
Also there appears to be a bug in the loop for offset mapping, I think
it should be ">=", rather than ">". Currently wouldn't it overflow the
storage if an offset of 256 is used?
Am I missing something in how it is meant to work? If not, I can modify
the tests while I'm adding a new one for the new addressing mode, I
just wanted to check first.
Thanks for the review, Ill fix the rest up for v2.
>
> > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> > index e47abf1833..7580867dc1 100644
> > --- a/drivers/i2c/i2c-uclass.c
> > +++ b/drivers/i2c/i2c-uclass.c
> > @@ -52,16 +52,19 @@ void i2c_dump_msgs(struct i2c_msg *msg, int
> > nmsgs)
> > static int i2c_setup_offset(struct dm_i2c_chip *chip, uint offset,
> > uint8_t offset_buf[], struct i2c_msg
> > *msg)
> > {
> > - int offset_len;
> > + int offset_len = chip->offset_len;
> >
> > msg->addr = chip->chip_addr;
> > + if (chip->chip_addr_offset_mask)
> > + msg->addr |= (offset >> (8 * offset_len)) &
> > + chip->chip_addr_offset_mask;
> > msg->flags = chip->flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN :
> > 0;
> > msg->len = chip->offset_len;
> > msg->buf = offset_buf;
> > - if (!chip->offset_len)
> > + if (!offset_len)
> > return -EADDRNOTAVAIL;
> > - assert(chip->offset_len <= I2C_MAX_OFFSET_LEN);
> > - offset_len = chip->offset_len;
> > + assert(offset_len <= I2C_MAX_OFFSET_LEN);
> > +
> > while (offset_len--)
> > *offset_buf++ = offset >> (8 * offset_len);
> >
> > @@ -83,7 +86,7 @@ static int i2c_read_bytewise(struct udevice *dev,
> > uint offset,
> > if (i2c_setup_offset(chip, offset + i, offset_buf,
> > msg))
> > return -EINVAL;
> > ptr = msg + 1;
> > - ptr->addr = chip->chip_addr;
> > + ptr->addr = msg->addr;
> > ptr->flags = msg->flags | I2C_M_RD;
> > ptr->len = 1;
> > ptr->buf = &buffer[i];
> > @@ -132,6 +135,7 @@ int dm_i2c_read(struct udevice *dev, uint
> > offset, uint8_t *buffer, int len)
> >
> > if (!ops->xfer)
> > return -ENOSYS;
> > +
>
> Unrelated change
>
> > if (chip->flags & DM_I2C_CHIP_RD_ADDRESS)
> > return i2c_read_bytewise(dev, offset, buffer, len);
> > ptr = msg;
> > @@ -139,7 +143,7 @@ int dm_i2c_read(struct udevice *dev, uint
> > offset, uint8_t *buffer, int len)
> > ptr++;
> >
> > if (len) {
> > - ptr->addr = chip->chip_addr;
> > + ptr->addr = msg->addr;
> > ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ?
> > I2C_M_TEN : 0;
> > ptr->flags |= I2C_M_RD;
> > ptr->len = len;
> > @@ -465,6 +469,22 @@ int i2c_get_chip_offset_len(struct udevice
> > *dev)
> > return chip->offset_len;
> > }
> >
> > +int i2c_set_chip_addr_offset_mask(struct udevice *dev, uint mask)
> > +{
> > + struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
> > +
> > + chip->chip_addr_offset_mask = mask;
> > +
> > + return 0;
> > +}
> > +
> > +uint i2c_get_chip_addr_offset_mask(struct udevice *dev)
> > +{
> > + struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
> > +
> > + return chip->chip_addr_offset_mask;
> > +}
> > +
> > #ifdef CONFIG_DM_GPIO
> > static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit)
> > {
> > diff --git a/include/i2c.h b/include/i2c.h
> > index 33570f5404..3c927340da 100644
> > --- a/include/i2c.h
> > +++ b/include/i2c.h
> > @@ -45,12 +45,17 @@ struct udevice;
> > * represent up to 256 bytes. A value larger than 1
> > may be
> > * needed for larger devices.
> > * @flags: Flags for this chip (dm_i2c_chip_flags)
> > + * @chip_addr_offset_mask: Mask of offset bits within chip_addr.
> > Used for
> > + * devices which steal addresses as part of
> > offset.
> > + * If offset_len is zero, then the offset
> > is encoded
> > + * completely within the chip address
> > itself.
>
> Can you add an example value here, or point to some documentation on
> this? There is not enough info here to figure out what is going on.
>
>
> > * @emul: Emulator for this chip address (only used for emulation)
> > */
> > struct dm_i2c_chip {
> > uint chip_addr;
> > uint offset_len;
> > uint flags;
> > + uint chip_addr_offset_mask;
> > #ifdef CONFIG_SANDBOX
> > struct udevice *emul;
> > bool test_mode;
> > @@ -261,6 +266,25 @@ int i2c_set_chip_offset_len(struct udevice
> > *dev, uint offset_len);
> > */
> > int i2c_get_chip_offset_len(struct udevice *dev);
> >
> > +/**
> > + * i2c_set_chip_addr_offset_mask() - set mask of address bits
> > usable by offset
> > + *
> > + * Some devices listen on multiple chip addresses to achieve
> > larger offsets
> > + * than their single or multiple byte offsets would allow for. You
> > can use this
> > + * function to set the bits that are valid to be used for offset
> > overflow.
> > + *
> > + * @mask: The mask to be used for high offset bits within address
> > + * @return 0 if OK, other -ve value on error
> > + */
> > +int i2c_set_chip_addr_offset_mask(struct udevice *dev, uint mask);
> > +
> > +/*
> > + * i2c_get_chip_addr_offset_mask() - get mask of address bits
> > usable by offset
> > + *
> > + * @return current chip addr offset mask
> > + */
> > +uint i2c_get_chip_addr_offset_mask(struct udevice *dev);
> > +
> > /**
> > * i2c_deblock() - recover a bus that is in an unknown state
> > *
> > --
> > 2.20.1
> >
More information about the U-Boot
mailing list