On 2023-03-31 15:30, Alex Deucher wrote:
> 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.
Okay, so we can guarantee that atom_ctx is not NULL at this point.
Add my,
Reviewed-by: Luben Tuikov <luben.tuikov@xxxxxxx>
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
>>
