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

[Alex Deucher]

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>

> -#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
>