> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@xxxxxxx>
> Sent: 2019年8月9日 14:42
> To: Zhou1, Tao <Tao.Zhou1@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
> Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Zhang, Hawking
> <Hawking.Zhang@xxxxxxx>
> Subject: Re: [PATCH 1/2] drm/amdgpu: implement UMC 64 bits REG
> operations
>
> Am 09.08.19 um 06:27 schrieb Tao Zhou:
> > implement 64 bits operations via 32 bits interface
> >
> > Signed-off-by: Tao Zhou <tao.zhou1@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h | 9 +++++++++
> > drivers/gpu/drm/amd/amdgpu/umc_v6_1.c | 10 +++++-----
> > 2 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > index 9efdd66279e5..a617dcc9d257 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > @@ -21,6 +21,15 @@
> > #ifndef __AMDGPU_UMC_H__
> > #define __AMDGPU_UMC_H__
> >
> > +/* implement 64 bits REG operations via 32 bits interface */
> > +#define RREG64_UMC(reg) (RREG32(reg) | \
> > + ((uint64_t)RREG32((reg) + 1) << 32))
> > +#define WREG64_UMC(reg, v) \
>
> Maybe call this WREG_LO_HI and RREG_LO_HI to explicitly note what they
> are doing. This way we can probably keep them in amdgpu.h as well.
[Tao] The two macros are non-atomic, putting them in amdgpu.h may lead to their abuse in the furture.
>
> > + do { \
> > + WREG32((reg), (uint32_t)((v) & 0xffffffff)); \
> > + WREG32((reg) + 1, (uint32_t)((v) >> 32)); \
>
> I think I now understand why you wanted to have this macro.
>
> We have explicit lower_32_bits() and upper_32_bits() functions to avoid the
> masking and shifting and make the code more readable.
>
> For example see the UVD code:
> > WREG32_SOC15(UVD, 0,
> > mmUVD_LMI_VCPU_CACHE_64BIT_BAR_LOW,
> > lower_32_bits(adev->vcn.gpu_addr));
> > WREG32_SOC15(UVD, 0,
> > mmUVD_LMI_VCPU_CACHE_64BIT_BAR_HIGH,
> > upper_32_bits(adev->vcn.gpu_addr));
>
> I suggest to use those in your macro here as well, apart from that the patch
> looks good to me.
[Tao] OK, I'll change to use lower_32_bits() and upper_32_bits().
>
> Thanks for taking care of this,
> Christian.
>
> > + } while (0)
> > +
> > /*
> > * void (*func)(struct amdgpu_device *adev, struct ras_err_data *err_data,
> > * uint32_t umc_reg_offset, uint32_t
> channel_index)
> > diff --git a/drivers/gpu/drm/amd/amdgpu/umc_v6_1.c
> > b/drivers/gpu/drm/amd/amdgpu/umc_v6_1.c
> > index 64df37b860dd..8502e736f721 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/umc_v6_1.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/umc_v6_1.c
> > @@ -116,7 +116,7 @@ static void
> > umc_v6_1_query_correctable_error_count(struct amdgpu_device *adev,
> >
> > /* check for SRAM correctable error
> > MCUMC_STATUS is a 64 bit register */
> > - mc_umc_status = RREG64(mc_umc_status_addr + umc_reg_offset);
> > + mc_umc_status = RREG64_UMC(mc_umc_status_addr +
> umc_reg_offset);
> > if (REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, ErrorCodeExt) == 6 &&
> > 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) @@ -134,7 +134,7 @@ static void
> umc_v6_1_querry_uncorrectable_error_count(struct amdgpu_device *adev
> > SOC15_REG_OFFSET(UMC, 0,
> > mmMCA_UMC_UMC0_MCUMC_STATUST0);
> >
> > /* check the MCUMC_STATUS */
> > - mc_umc_status = RREG64(mc_umc_status_addr + umc_reg_offset);
> > + mc_umc_status = RREG64_UMC(mc_umc_status_addr +
> umc_reg_offset);
> > 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, Deferred) == 1 ||
> > REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, UECC)
> > == 1 || @@ -173,11 +173,11 @@ static void
> umc_v6_1_query_error_address(struct amdgpu_device *adev,
> > /* skip error address process if -ENOMEM */
> > if (!err_data->err_addr) {
> > /* clear umc status */
> > - WREG64(mc_umc_status_addr + umc_reg_offset, 0x0ULL);
> > + WREG64_UMC(mc_umc_status_addr + umc_reg_offset,
> 0x0ULL);
> > return;
> > }
> >
> > - mc_umc_status = RREG64(mc_umc_status_addr + umc_reg_offset);
> > + mc_umc_status = RREG64_UMC(mc_umc_status_addr +
> umc_reg_offset);
> >
> > /* calculate error address if ue/ce error is detected */
> > if (REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, Val)
> > == 1 && @@ -200,7 +200,7 @@ static void
> umc_v6_1_query_error_address(struct amdgpu_device *adev,
> > }
> >
> > /* clear umc status */
> > - WREG64(mc_umc_status_addr + umc_reg_offset, 0x0ULL);
> > + WREG64_UMC(mc_umc_status_addr + umc_reg_offset, 0x0ULL);
> > }
> >
> > static void umc_v6_1_query_ras_error_address(struct amdgpu_device
> > *adev,
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
