[PATCH] drm/amdgpu: Remove redundant I2C EEPROM address

[Luben Tuikov]

On 2022-11-08 09:31, Alex Deucher wrote:
> On Mon, Nov 7, 2022 at 12:23 PM Luben Tuikov <luben.tuikov at amd.com> wrote:
>>
>> Remove redundant EEPROM_I2C_MADDR_54H address, since we already have it
>> represented (ARCTURUS), and since we don't include the I2C device type
>> identifier in EEPROM memory addresses, i.e. that high up in the device
>> abstraction--we only use EEPROM memory addresses, as memory is continuously
>> represented by EEPROM device(s) on the I2C bus.
>>
>> Add a comment describing what these memory addresses are, how they come
>> about and how they're usually extracted from the device address byte.
>>
>> Cc: Candice Li <candice.li at amd.com>
>> Cc: Tao Zhou <tao.zhou1 at amd.com>
>> Cc: Alex Deucher <Alexander.Deucher at amd.com>
>> Fixes: 367a1ebddde5d0 ("drm/amdgpu: Add EEPROM I2C address support for ip discovery")
>> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c    |  2 ++
>>  .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c    | 24 ++++++++++++++++---
>>  2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
>> index 4d9eb0137f8c43..d6c4293829aab1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
>> @@ -79,6 +79,8 @@
>>   * That is, for an I2C EEPROM driver everything is controlled by
>>   * the "eeprom_addr".
>>   *
>> + * See also top of amdgpu_ras_eeprom.c.
>> + *
>>   * P.S. If you need to write, lock and read the Identification Page,
>>   * (M24M02-DR device only, which we do not use), change the "7" to
>>   * "0xF" in the macro below, and let the client set bit 20 to 1 in
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
>> index 7268ae65c140c1..1bb92a64f24afc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
>> @@ -33,12 +33,30 @@
>>
>>  #include "amdgpu_reset.h"
>>
>> +/* These are memory addresses as would be seen by one or more EEPROM
>> + * chips strung on the I2C bus, usually by manipulating pins 1-3 of a
>> + * set of EEPROM devices. They form a continuous memory space.
>> + *
>> + * The I2C device address includes the device type identifier, 1010b,
>> + * which is a reserved value and indicates that this is an I2C EEPROM
>> + * device. It also includes the top 3 bits of the 19 bit EEPROM memory
>> + * address, namely bits 18, 17, and 16. This makes up the 7 bit
>> + * address sent on the I2C bus with bit 0 being the direction bit,
>> + * which is not represented here, and sent by the hardware directly.
>> + *
>> + * For instance,
>> + *   50h = 1010000b => device type identifier 1010b, bits 18:16 = 000b, address 0.
>> + *   54h = 1010100b => --"--, bits 18:16 = 100b, address 40000h.
>> + *   56h = 1010110b => --"--, bits 18:16 = 110b, address 60000h.
>> + * Depending on the size of the I2C EEPROM device(s), bits 18:16 may
>> + * address memory in a device or a device on the I2C bus, depending on
>> + * the status of pins 1-3. See top of amdgpu_eeprom.c.
>> + */
>>  #define EEPROM_I2C_MADDR_VEGA20         0x0
>>  #define EEPROM_I2C_MADDR_ARCTURUS       0x40000
>>  #define EEPROM_I2C_MADDR_ARCTURUS_D342  0x0
>>  #define EEPROM_I2C_MADDR_SIENNA_CICHLID 0x0
>>  #define EEPROM_I2C_MADDR_ALDEBARAN      0x0
> 
> As a follow on patch maybe we can clean up the rest of these
> duplicates?  And rather than using the asic type, maybe just switch to
> the define to a more descriptive name?  E.g.,
> #define EEPROM_I2C_MADDR_0H       0x00000
> #define EEPROM_I2C_MADDR_4H       0x40000
> #define EEPROM_I2C_MADDR_6H       0x60000
> 
> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
> 

I sent a patch doing exactly that 22 minutes after sending this one.
Kent already reviewed that patch. Check your inbox, you're CC-ed on this and
that one.

Regards,
Luben


>> -#define EEPROM_I2C_MADDR_54H            (0x54UL << 16)
>>
>>  /*
>>   * The 2 macros bellow represent the actual size in bytes that
>> @@ -130,7 +148,7 @@ static bool __get_eeprom_i2c_addr_ip_discovery(struct amdgpu_device *adev,
>>         switch (adev->ip_versions[MP1_HWIP][0]) {
>>         case IP_VERSION(13, 0, 0):
>>         case IP_VERSION(13, 0, 10):
>> -               control->i2c_address = EEPROM_I2C_MADDR_54H;
>> +               control->i2c_address = EEPROM_I2C_MADDR_ARCTURUS;
>>                 return true;
>>         default:
>>                 return false;
>> @@ -185,7 +203,7 @@ static bool __get_eeprom_i2c_addr(struct amdgpu_device *adev,
>>
>>         switch (adev->ip_versions[MP1_HWIP][0]) {
>>         case IP_VERSION(13, 0, 0):
>> -               control->i2c_address = EEPROM_I2C_MADDR_54H;
>> +               control->i2c_address = EEPROM_I2C_MADDR_ARCTURUS;
>>                 break;
>>
>>         default:
>>
>> base-commit: 03b61a92efbaf17ac3d9f82ae81aa4cf8ed40608
>> --
>> 2.38.1
>>