Re: [PATCH Review 1/1] drm/amdgpu: add umc query error status function

Fri, 08 Apr 2022 03:08:02 -0700 

Dear Stanley,


Thank you for the patch.

Am 08.04.22 um 10:10 schrieb Stanley.Yang:

Please remove the dot/period from the name.


In order to debug ras error, driver will print IPID/SYND/MISC0
register value if detect correctable or uncorrectable error.


… if it detects …


Provide umc_query_error_status_helper function to reduce code
redundancy.
Can you please make this two patches. First refactoring, and then adding new call sites. (The current commit message summary, does not say anything about new call-sites.) 


Change-Id: I09a2aae85cde3ab2cb6b042b973da6839ad024ec


Without the Gerrit review instance, the Change-Id is not of any use.


Signed-off-by: Stanley.Yang <stanley.y...@amd.com>


Please remove the . from the name.


Kind regards,

Paul



---
  drivers/gpu/drm/amd/amdgpu/umc_v6_7.c | 106 ++++++++++++--------------
  1 file changed, 48 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c 
b/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
index c45d9c14ecbc..9d3b54778417 100644
--- a/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
+++ b/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
@@ -64,21 +64,62 @@ static inline uint32_t get_umc_v6_7_channel_index(struct 
amdgpu_device *adev,
        return adev->umc.channel_idx_tbl[umc_inst * adev->umc.channel_inst_num 
+ ch_inst];
  }
+static void umc_query_error_status_helper(struct amdgpu_device *adev, 
+                                                 uint64_t mc_umc_status, 
uint32_t umc_reg_offset)
+{
+       uint32_t mc_umc_addr;
+       uint64_t reg_value;
+
+       if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Deferred) 
== 1)
+               dev_info(adev->dev, "Deferred error, no user action is 
needed.\n");
+
+       if (mc_umc_status)
+               dev_info(adev->dev, "MCA STATUS 0x%llx, umc_reg_offset 0x%x\n", 
mc_umc_status, umc_reg_offset);
+
+       /* print IPID registers value */
+       mc_umc_addr =
+               SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_IPIDT0);
+       reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
+       if (reg_value)
+               dev_info(adev->dev, "MCA IPID 0x%llx, umc_reg_offset 0x%x\n", 
reg_value, umc_reg_offset);
+
+       /* print SYND registers value */
+       mc_umc_addr =
+               SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_SYNDT0);
+       reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
+       if (reg_value)
+               dev_info(adev->dev, "MCA SYND 0x%llx, umc_reg_offset 0x%x\n", 
reg_value, umc_reg_offset);
+
+       /* print MISC0 registers value */
+       mc_umc_addr =
+               SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_MISC0T0);
+       reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
+       if (reg_value)
+               dev_info(adev->dev, "MCA MISC0 0x%llx, umc_reg_offset 0x%x\n", 
reg_value, umc_reg_offset);
+}
+
  static void umc_v6_7_ecc_info_query_correctable_error_count(struct 
amdgpu_device *adev,
                                                   uint32_t umc_inst, uint32_t 
ch_inst,
                                                   unsigned long *error_count)
  {
        uint64_t mc_umc_status;
        uint32_t eccinfo_table_idx;
+       uint32_t umc_reg_offset;
        struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
+ umc_reg_offset = get_umc_v6_7_reg_offset(adev, 
+                                               umc_inst, ch_inst);
+
        eccinfo_table_idx = umc_inst * adev->umc.channel_inst_num + ch_inst;
        /* check for SRAM correctable error
          MCUMC_STATUS is a 64 bit register */
        mc_umc_status = ras->umc_ecc.ecc[eccinfo_table_idx].mca_umc_status;
        if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1 
&&
-           REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 
1)
+           REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 
1) {
                *error_count += 1;
+
+               umc_query_error_status_helper(adev, mc_umc_status, 
umc_reg_offset);
+       }
  }
static void umc_v6_7_ecc_info_querry_uncorrectable_error_count(struct amdgpu_device *adev, 
@@ -88,8 +129,6 @@ static void 
umc_v6_7_ecc_info_querry_uncorrectable_error_count(struct amdgpu_dev
        uint64_t mc_umc_status;
        uint32_t eccinfo_table_idx;
        uint32_t umc_reg_offset;
-       uint32_t mc_umc_addr;
-       uint64_t reg_value;
        struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
umc_reg_offset = get_umc_v6_7_reg_offset(adev, 
@@ -106,32 +145,7 @@ static void 
umc_v6_7_ecc_info_querry_uncorrectable_error_count(struct amdgpu_dev
            REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, TCC) == 
1)) {
                *error_count += 1;
- if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Deferred) == 1) 
-                       dev_info(adev->dev, "Deferred error, no user action is 
needed.\n");
-
-               if (mc_umc_status)
-                       dev_info(adev->dev, "MCA STATUS 0x%llx, umc_reg_offset 
0x%x\n", mc_umc_status, umc_reg_offset);
-
-               /* print IPID registers value */
-               mc_umc_addr =
-                       SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_IPIDT0);
-               reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
-               if (reg_value)
-                       dev_info(adev->dev, "MCA IPID 0x%llx, umc_reg_offset 
0x%x\n", reg_value, umc_reg_offset);
-
-               /* print SYND registers value */
-               mc_umc_addr =
-                       SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_SYNDT0);
-               reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
-               if (reg_value)
-                       dev_info(adev->dev, "MCA SYND 0x%llx, umc_reg_offset 
0x%x\n", reg_value, umc_reg_offset);
-
-               /* print MISC0 registers value */
-               mc_umc_addr =
-                       SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_MISC0T0);
-               reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
-               if (reg_value)
-                       dev_info(adev->dev, "MCA MISC0 0x%llx, umc_reg_offset 
0x%x\n", reg_value, umc_reg_offset);
+               umc_query_error_status_helper(adev, mc_umc_status, 
umc_reg_offset);
        }
  }
@@ -277,8 +291,11 @@ static void umc_v6_7_query_correctable_error_count(struct amdgpu_device *adev, 
          MCUMC_STATUS is a 64 bit register */
        mc_umc_status = RREG64_PCIE((mc_umc_status_addr + umc_reg_offset) * 4);
        if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1 
&&
-           REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 
1)
+           REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 
1) {
                *error_count += 1;
+
+               umc_query_error_status_helper(adev, mc_umc_status, 
umc_reg_offset);
+       }
  }
static void umc_v6_7_querry_uncorrectable_error_count(struct amdgpu_device *adev, 
@@ -287,8 +304,6 @@ static void 
umc_v6_7_querry_uncorrectable_error_count(struct amdgpu_device *adev
  {
        uint64_t mc_umc_status;
        uint32_t mc_umc_status_addr;
-       uint32_t mc_umc_addr;
-       uint64_t reg_value;
mc_umc_status_addr = 
                SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_STATUST0);
@@ -303,32 +318,7 @@ static void 
umc_v6_7_querry_uncorrectable_error_count(struct amdgpu_device *adev
            REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, TCC) == 
1)) {
                *error_count += 1;
- if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Deferred) == 1) 
-                       dev_info(adev->dev, "Deferred error, no user action is 
needed.\n");
-
-               if (mc_umc_status)
-                       dev_info(adev->dev, "MCA STATUS 0x%llx, umc_reg_offset 
0x%x\n", mc_umc_status, umc_reg_offset);
-
-               /* print IPID registers value */
-               mc_umc_addr =
-                       SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_IPIDT0);
-               reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
-               if (reg_value)
-                       dev_info(adev->dev, "MCA IPID 0x%llx, umc_reg_offset 
0x%x\n", reg_value, umc_reg_offset);
-
-               /* print SYND registers value */
-               mc_umc_addr =
-                       SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_SYNDT0);
-               reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
-               if (reg_value)
-                       dev_info(adev->dev, "MCA SYND 0x%llx, umc_reg_offset 
0x%x\n", reg_value, umc_reg_offset);
-
-               /* print MISC0 registers value */
-               mc_umc_addr =
-                       SOC15_REG_OFFSET(UMC, 0, regMCA_UMC_UMC0_MCUMC_MISC0T0);
-               reg_value = RREG64_PCIE((mc_umc_addr + umc_reg_offset) * 4);
-               if (reg_value)
-                       dev_info(adev->dev, "MCA MISC0 0x%llx, umc_reg_offset 
0x%x\n", reg_value, umc_reg_offset);
+               umc_query_error_status_helper(adev, mc_umc_status, 
umc_reg_offset);
        }
  }

Reply via email to