Re: [PATCH 19/40] drm/amdgpu: Fixes to the AMDGPU EEPROM driver

[Alex Deucher <alexdeucher@gmail.com>]

On Tue, Jun 8, 2021 at 5:40 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>
> * When reading from the EEPROM device, there is no
>   device limitation on the number of bytes
>   read--they're simply sequenced out. Thus, read
>   the whole data requested in one go.
>
> * When writing to the EEPROM device, there is a
>   256-byte page limit to write to before having to
>   generate a STOP on the bus, as well as the
>   address written to mustn't cross over the page
>   boundary (it actually rolls over). Maximize the
>   data written to per bus acquisition.
>
> * Return the number of bytes read/written, or -errno.
>
> * Add kernel doc.
>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Alexander Deucher <Alexander.Deucher@amd.com>
> Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> Cc: Lijo Lazar <Lijo.Lazar@amd.com>
> Cc: Stanley Yang <Stanley.Yang@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c | 96 +++++++++++++++-------
>  1 file changed, 68 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
> index d02ea083a6c69b..7fdb5bd2fc8bc8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
> @@ -24,59 +24,99 @@
>  #include "amdgpu_eeprom.h"
>  #include "amdgpu.h"
>
> -#define EEPROM_OFFSET_LENGTH 2
> +/* AT24CM02 has a 256-byte write page size.
> + */
> +#define EEPROM_PAGE_BITS   8
> +#define EEPROM_PAGE_SIZE   (1U << EEPROM_PAGE_BITS)
> +#define EEPROM_PAGE_MASK   (EEPROM_PAGE_SIZE - 1)
> +
> +#define EEPROM_OFFSET_SIZE 2
>
> +/**
> + * amdgpu_eeprom_xfer -- Read/write from/to an I2C EEPROM device
> + * @i2c_adap: pointer to the I2C adapter to use
> + * @slave_addr: I2C address of the slave device
> + * @eeprom_addr: EEPROM address from which to read/write
> + * @eeprom_buf: pointer to data buffer to read into/write from
> + * @buf_size: the size of @eeprom_buf
> + * @read: True if reading from the EEPROM, false if writing
> + *
> + * Returns the number of bytes read/written; -errno on error.
> + */
>  int amdgpu_eeprom_xfer(struct i2c_adapter *i2c_adap,
>                        u16 slave_addr, u16 eeprom_addr,
> -                      u8 *eeprom_buf, u16 bytes, bool read)
> +                      u8 *eeprom_buf, u16 buf_size, bool read)
>  {
> -       u8 eeprom_offset_buf[2];
> -       u16 bytes_transferred;
> +       u8 eeprom_offset_buf[EEPROM_OFFSET_SIZE];
>         struct i2c_msg msgs[] = {
>                 {
>                         .addr = slave_addr,
>                         .flags = 0,
> -                       .len = EEPROM_OFFSET_LENGTH,
> +                       .len = EEPROM_OFFSET_SIZE,
>                         .buf = eeprom_offset_buf,
>                 },
>                 {
>                         .addr = slave_addr,
>                         .flags = read ? I2C_M_RD : 0,
> -                       .len = bytes,
> -                       .buf = eeprom_buf,
>                 },
>         };
> +       const u8 *p = eeprom_buf;
>         int r;
> +       u16 len;
> +
> +       r = 0;
> +       for (len = 0; buf_size > 0;
> +            buf_size -= len, eeprom_addr += len, eeprom_buf += len) {
> +               /* Set the EEPROM address we want to write to/read from.
> +                */
> +               msgs[0].buf[0] = (eeprom_addr >> 8) & 0xff;
> +               msgs[0].buf[1] = eeprom_addr & 0xff;
>
> -       msgs[0].buf[0] = ((eeprom_addr >> 8) & 0xff);
> -       msgs[0].buf[1] = (eeprom_addr & 0xff);
> +               if (!read) {
> +                       /* Write the maximum amount of data, without
> +                        * crossing the device's page boundary, as per
> +                        * its spec. Partial page writes are allowed,
> +                        * starting at any location within the page,
> +                        * so long as the page boundary isn't crossed
> +                        * over (actually the page pointer rolls
> +                        * over).
> +                        *
> +                        * As per the AT24CM02 EEPROM spec, after
> +                        * writing into a page, the I2C driver MUST
> +                        * terminate the transfer, i.e. in
> +                        * "i2c_transfer()" below, with a STOP
> +                        * condition, so that the self-timed write
> +                        * cycle begins. This is implied for the
> +                        * "i2c_transfer()" abstraction.
> +                        */
> +                       len = min(EEPROM_PAGE_SIZE - (eeprom_addr &
> +                                                     EEPROM_PAGE_MASK),
> +                                 (u32)buf_size);
> +               } else {
> +                       /* Reading from the EEPROM has no limitation
> +                        * on the number of bytes read from the EEPROM
> +                        * device--they are simply sequenced out.
> +                        */
> +                       len = buf_size;
> +               }
> +               msgs[1].len = len;
> +               msgs[1].buf = eeprom_buf;
>
> -       while (msgs[1].len) {
>                 r = i2c_transfer(i2c_adap, msgs, ARRAY_SIZE(msgs));
> -               if (r <= 0)
> -                       return r;
> +               if (r < ARRAY_SIZE(msgs))
> +                       break;
>
> -               /* Only for write data */
> -               if (!msgs[1].flags)
> -                       /*
> -                        * According to EEPROM spec there is a MAX of 10 ms required for
> -                        * EEPROM to flush internal RX buffer after STOP was issued at the
> -                        * end of write transaction. During this time the EEPROM will not be
> -                        * responsive to any more commands - so wait a bit more.
> +               if (!read) {
> +                       /* According to the AT24CM02 EEPROM spec the
> +                        * length of the self-writing cycle, tWR, is
> +                        * 10 ms.
>                          *
>                          * TODO Improve to wait for first ACK for slave address after
>                          * internal write cycle done.
>                          */
>                         msleep(10);
> -
> -
> -               bytes_transferred = r - EEPROM_OFFSET_LENGTH;
> -               eeprom_addr += bytes_transferred;
> -               msgs[0].buf[0] = ((eeprom_addr >> 8) & 0xff);
> -               msgs[0].buf[1] = (eeprom_addr & 0xff);
> -               msgs[1].buf += bytes_transferred;
> -               msgs[1].len -= bytes_transferred;
> +               }
>         }
>
> -       return 0;
> +       return r < 0 ? r : eeprom_buf - p;
>  }
> --
> 2.32.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx