[PATCH] drm/amdgpu: simplify amdgpu_ras_eeprom.c

[Luben Tuikov]

On 2023-03-31 15:30, Alex Deucher wrote:
> On Tue, Mar 28, 2023 at 12:30 PM Luben Tuikov <luben.tuikov at amd.com> 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 at amd.com>
>>> Cc: Luben Tuikov <luben.tuikov at amd.com>
>>> ---
>>>  .../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.

Okay, so we can guarantee that atom_ctx is not NULL at this point.
Add my,

Reviewed-by: Luben Tuikov <luben.tuikov at amd.com>

And if in the wild we see that it is, it'll be an easy fix.

Regards,
Luben


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