Re: [PATCH 20/40] drm/amdgpu: EEPROM respects I2C quirks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2021-06-11 1:01 p.m., Alex Deucher wrote:
> On Tue, Jun 8, 2021 at 5:40 PM Luben Tuikov <luben.tuikov@xxxxxxx> wrote:
>> Consult the i2c_adapter.quirks table for
>> the maximum read/write data length per bus
>> transaction. Do not exceed this transaction
>> limit.
>>
>> Cc: Jean Delvare <jdelvare@xxxxxxx>
>> Cc: Alexander Deucher <Alexander.Deucher@xxxxxxx>
>> Cc: Andrey Grodzovsky <Andrey.Grodzovsky@xxxxxxx>
>> Cc: Lijo Lazar <Lijo.Lazar@xxxxxxx>
>> Cc: Stanley Yang <Stanley.Yang@xxxxxxx>
>> Cc: Hawking Zhang <Hawking.Zhang@xxxxxxx>
>> Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c | 80 +++++++++++++++++-----
>>  1 file changed, 64 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
>> index 7fdb5bd2fc8bc8..94aeda1c7f8ca0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
>> @@ -32,20 +32,9 @@
>>
>>  #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 buf_size, bool read)
>> +static int __amdgpu_eeprom_xfer(struct i2c_adapter *i2c_adap,
>> +                               u16 slave_addr, u16 eeprom_addr,
>> +                               u8 *eeprom_buf, u16 buf_size, bool read)
>>  {
>>         u8 eeprom_offset_buf[EEPROM_OFFSET_SIZE];
>>         struct i2c_msg msgs[] = {
>> @@ -65,8 +54,8 @@ int amdgpu_eeprom_xfer(struct i2c_adapter *i2c_adap,
>>         u16 len;
>>
>>         r = 0;
>> -       for (len = 0; buf_size > 0;
>> -            buf_size -= len, eeprom_addr += len, eeprom_buf += len) {
>> +       for ( ; 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;
>> @@ -120,3 +109,62 @@ int amdgpu_eeprom_xfer(struct i2c_adapter *i2c_adap,
>>
>>         return r < 0 ? r : eeprom_buf - p;
>>  }
>> +
>> +/**
>> + * 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 buf_size, bool read)
>> +{
>> +       const struct i2c_adapter_quirks *quirks = i2c_adap->quirks;
>> +       u16 limit;
>> +
>> +       if (!quirks)
>> +               limit = 0;
>> +       else if (read)
>> +               limit = quirks->max_read_len;
>> +       else
>> +               limit = quirks->max_write_len;
>> +
>> +       if (limit == 0) {
>> +               return __amdgpu_eeprom_xfer(i2c_adap, slave_addr, eeprom_addr,
>> +                                           eeprom_buf, buf_size, read);
>> +       } else if (limit <= EEPROM_OFFSET_SIZE) {
>> +               dev_err_ratelimited(&i2c_adap->dev,
>> +                                   "maddr:0x%04X size:0x%02X:quirk max_%s_len must be > %d",
>> +                                   eeprom_addr, buf_size,
>> +                                   read ? "read" : "write", EEPROM_OFFSET_SIZE);
>> +               return -EINVAL;
> I presume we handle this case properly at higher levels (i.e., split
> up EEPROM updates into smaller transactions)?

Absolutely we do.
(We break it down twice: once per this limit and again per page size and page boundary. It'll work always. :-) )

But this is different--this means that the user has set a limit less than 2, which means we can't even send a set-address phase to set the EEPROM memory address offset we want to read or write from, and thus the chattiness.

I just noticed that it is less-than-or-equal, which means the smallest limit the user can set which would work is 3. But 2 would also work, then all transfers would be 2 bytes long. Does it matter? I guess I can change this from LTE to LT, to mean that a minimum transfer of 2 is the smallest we support. I've changed it to LT. :-)

Regards,
Luben

>
> Alex
>
>
>> +       } else {
>> +               u16 ps; /* Partial size */
>> +               int res = 0, r;
>> +
>> +               /* The "limit" includes all data bytes sent/received,
>> +                * which would include the EEPROM_OFFSET_SIZE bytes.
>> +                * Account for them here.
>> +                */
>> +               limit -= EEPROM_OFFSET_SIZE;
>> +               for ( ; buf_size > 0;
>> +                     buf_size -= ps, eeprom_addr += ps, eeprom_buf += ps) {
>> +                       ps = min(limit, buf_size);
>> +
>> +                       r = __amdgpu_eeprom_xfer(i2c_adap,
>> +                                                slave_addr, eeprom_addr,
>> +                                                eeprom_buf, ps, read);
>> +                       if (r < 0)
>> +                               return r;
>> +                       res += r;
>> +               }
>> +
>> +               return res;
>> +       }
>> +}
>> --
>> 2.32.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cluben.tuikov%40amd.com%7Cc8502a7f4dd94666468408d92cfa95e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637590277035962948%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=UsBaf7trds%2BjmJ8yhIaMoLNdq2Rxk3EXY5jztgzjFL0%3D&amp;reserved=0

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux