[PATCH] drm/amdgpu: simplify amdgpu_ras_eeprom.c

[Alex Deucher]

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.

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
>