On 2022-11-08 09:31, Alex Deucher wrote:
> On Mon, Nov 7, 2022 at 12:23 PM Luben Tuikov <luben.tuikov@xxxxxxx> 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@xxxxxxx>
>> Cc: Tao Zhou <tao.zhou1@xxxxxxx>
>> Cc: Alex Deucher <Alexander.Deucher@xxxxxxx>
>> Fixes: 367a1ebddde5d0 ("drm/amdgpu: Add EEPROM I2C address support for ip discovery")
>> Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
>> ---
>> 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@xxxxxxx>
>
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
>>
