On Tue, Mar 28, 2023 at 12:30 PM Luben Tuikov <luben.tuikov@xxxxxxx> wrote: > > On 2023-03-27 20:11, Alex Deucher wrote: > > All chips that support RAS also support IP discovery, so > > use the IP versions rather than a mix of IP versions and > > asic types. > > > > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx> > > Cc: Luben Tuikov <luben.tuikov@xxxxxxx> > > --- > > .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 72 ++++++------------- > > 1 file changed, 20 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c > > index 3106fa8a15ef..c2ef2b1456bc 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c > > @@ -106,48 +106,13 @@ > > #define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras, eeprom_control))->adev > > > > static bool __is_ras_eeprom_supported(struct amdgpu_device *adev) > > -{ > > - if (adev->asic_type == CHIP_IP_DISCOVERY) { > > - switch (adev->ip_versions[MP1_HWIP][0]) { > > - case IP_VERSION(13, 0, 0): > > - case IP_VERSION(13, 0, 10): > > - return true; > > - default: > > - return false; > > - } > > - } > > - > > - return adev->asic_type == CHIP_VEGA20 || > > - adev->asic_type == CHIP_ARCTURUS || > > - adev->asic_type == CHIP_SIENNA_CICHLID || > > - adev->asic_type == CHIP_ALDEBARAN; > > -} > > - > > -static bool __get_eeprom_i2c_addr_arct(struct amdgpu_device *adev, > > - struct amdgpu_ras_eeprom_control *control) > > -{ > > - struct atom_context *atom_ctx = adev->mode_info.atom_context; > > - > > - if (!control || !atom_ctx) > > - return false; > > - > > - if (strnstr(atom_ctx->vbios_version, > > - "D342", > > - sizeof(atom_ctx->vbios_version))) > > - control->i2c_address = EEPROM_I2C_MADDR_0; > > - else > > - control->i2c_address = EEPROM_I2C_MADDR_4; > > - > > - return true; > > -} > > - > > -static bool __get_eeprom_i2c_addr_ip_discovery(struct amdgpu_device *adev, > > - struct amdgpu_ras_eeprom_control *control) > > { > > switch (adev->ip_versions[MP1_HWIP][0]) { > > + case IP_VERSION(11, 0, 2): /* VEGA20 and ARCTURUS */ > > + case IP_VERSION(11, 0, 7): > > case IP_VERSION(13, 0, 0): > > + case IP_VERSION(13, 0, 2): > > case IP_VERSION(13, 0, 10): > > I'd add the rest of the proper names here which are being deleted by this change, > so as to not lose this information by this commit: Sienna Cichlid and Aldebaran, > the rest can be left blank as per the current state of the code. Fixed. > > > - control->i2c_address = EEPROM_I2C_MADDR_4; > > return true; > > default: > > return false; > > @@ -178,29 +143,32 @@ static bool __get_eeprom_i2c_addr(struct amdgpu_device *adev, > > return true; > > } > > > > - switch (adev->asic_type) { > > - case CHIP_VEGA20: > > - control->i2c_address = EEPROM_I2C_MADDR_0; > > + switch (adev->ip_versions[MP1_HWIP][0]) { > > + case IP_VERSION(11, 0, 2): > > + /* VEGA20 and ARCTURUS */ > > + if (adev->asic_type == CHIP_VEGA20) > > + control->i2c_address = EEPROM_I2C_MADDR_0; > > + else if (strnstr(atom_ctx->vbios_version, > > In the code this is qualified with atom_ctx != NULL; and if it is, > then we return false. So, this is fine, iff we can guarantee that > "atom_ctx" will never be NULL. If, OTOH, we cannot guarantee that, > then we need to add, > else if (!atom_ctx) > return false; > else if (strnstr(... > > Although, I do recognize that for Aldebaran below, we do not qualify > atom_ctx, so we should probably qualify there too. This function is called after the vbios is initialized so I think we can drop the check. vbios is fetched in amdgpu_device_ip_early_init() and ras is initialized in amdgpu_device_ip_init() which is called much later. Alex > > > + "D342", > > + sizeof(atom_ctx->vbios_version))) > > + control->i2c_address = EEPROM_I2C_MADDR_0; > > + else > > + control->i2c_address = EEPROM_I2C_MADDR_4; > > return true; > > - > > - case CHIP_ARCTURUS: > > - return __get_eeprom_i2c_addr_arct(adev, control); > > - > > - case CHIP_SIENNA_CICHLID: > > + case IP_VERSION(11, 0, 7): > > control->i2c_address = EEPROM_I2C_MADDR_0; > > return true; > > - > > - case CHIP_ALDEBARAN: > > + case IP_VERSION(13, 0, 2): > > if (strnstr(atom_ctx->vbios_version, "D673", > > sizeof(atom_ctx->vbios_version))) > > control->i2c_address = EEPROM_I2C_MADDR_4; > > else > > control->i2c_address = EEPROM_I2C_MADDR_0; > > return true; > > - > > - case CHIP_IP_DISCOVERY: > > - return __get_eeprom_i2c_addr_ip_discovery(adev, control); > > - > > + case IP_VERSION(13, 0, 0): > > + case IP_VERSION(13, 0, 10): > > + control->i2c_address = EEPROM_I2C_MADDR_4; > > + return true; > > default: > > return false; > > } > > -- > Regards, > Luben >