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