[PATCH 2/5] drm/amdgpu: Use the dynamic IP based offset for register access for SOC15

[Liu, Shaoyun]

Thanks for the review .  The reason to use define here is the original function didn't take the adev as parameter .  If I change it here , all the caller of this function need to be changed . If you think that's  proper way  I can change it.  But do you think it's  better to put it in another change so this patch  will looks much more cleaner . 

Regards
Shaoyun.liu

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken at gmail.com] 
Sent: Wednesday, November 29, 2017 3:05 PM
To: Liu, Shaoyun; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/amdgpu: Use the dynamic IP based offset for register access for SOC15

Am 29.11.2017 um 20:09 schrieb Shaoyun Liu:
> Change-Id: I29f33ee3b4bbd6737f3426385a9e8452fb528a67
> Signed-off-by: Shaoyun Liu <Shaoyun.Liu at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c    | 21 +++----------------
>   drivers/gpu/drm/amd/amdgpu/soc15_common.h | 34 ++++++++-----------------------
>   2 files changed, 11 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c 
> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 4c55f21..e324c66 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -107,24 +107,9 @@
>   	SOC15_REG_OFFSET(SDMA0, 0, mmSDMA0_GB_ADDR_CONFIG_READ), 0x0018773f, 0x00000002
>   };
>   
> -static u32 sdma_v4_0_get_reg_offset(u32 instance, u32 
> internal_offset) -{
> -	u32 base = 0;
> -
> -	switch (instance) {
> -	case 0:
> -		base = SDMA0_BASE.instance[0].segment[0];
> -		break;
> -	case 1:
> -		base = SDMA1_BASE.instance[0].segment[0];
> -		break;
> -	default:
> -		BUG();
> -		break;
> -	}
> -
> -	return base + internal_offset;
> -}
> +#define sdma_v4_0_get_reg_offset(inst, offset) ( 0 == inst ? \
> +	(adev->reg_offset[SDMA0_HWIP][0][0] + offset) : \
> +	(adev->reg_offset[SDMA1_HWIP][0][0] + offset))

Please keep that a function, not a define.

Apart from that looks really good to me.

Christian.

>   
>   static void sdma_v4_0_init_golden_registers(struct amdgpu_device *adev)
>   {
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15_common.h 
> b/drivers/gpu/drm/amd/amdgpu/soc15_common.h
> index 7a8e4e28..62a6e21 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15_common.h
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15_common.h
> @@ -54,42 +54,24 @@ struct nbio_pcie_index_data {
>                                                               
> (ip##_BASE__INST##inst##_SEG4 + reg)))))
>   
>   #define WREG32_FIELD15(ip, idx, reg, field, val)	\
> -	WREG32(SOC15_REG_OFFSET(ip, idx, mm##reg), (RREG32(SOC15_REG_OFFSET(ip, idx, mm##reg)) & ~REG_FIELD_MASK(reg, field)) | (val) << REG_FIELD_SHIFT(reg, field))
> +	WREG32(adev->reg_offset[ip##_HWIP][idx][mm##reg##_BASE_IDX] + mm##reg,	\
> +	(RREG32(adev->reg_offset[ip##_HWIP][idx][mm##reg##_BASE_IDX] + mm##reg)	\
> +	& ~REG_FIELD_MASK(reg, field)) | (val) << REG_FIELD_SHIFT(reg, 
> +field))
>   
>   #define RREG32_SOC15(ip, inst, reg) \
> -	RREG32( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
> -		(1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
> -		(2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
> -		(3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
> -		(ip##_BASE__INST##inst##_SEG4 + reg))))))
> +	RREG32(adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + reg)
>   
>   #define RREG32_SOC15_OFFSET(ip, inst, reg, offset) \
> -	RREG32( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
> -		(1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
> -		(2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
> -		(3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
> -		(ip##_BASE__INST##inst##_SEG4 + reg))))) + offset)
> +	RREG32((adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + reg) + 
> +offset)
>   
>   #define WREG32_SOC15(ip, inst, reg, value) \
> -	WREG32( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
> -		(1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
> -		(2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
> -		(3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
> -		(ip##_BASE__INST##inst##_SEG4 + reg))))), value)
> +	WREG32((adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + reg), 
> +value)
>   
>   #define WREG32_SOC15_NO_KIQ(ip, inst, reg, value) \
> -	WREG32_NO_KIQ( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
> -		(1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
> -		(2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
> -		(3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
> -		(ip##_BASE__INST##inst##_SEG4 + reg))))), value)
> +	WREG32_NO_KIQ((adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + 
> +reg), value)
>   
>   #define WREG32_SOC15_OFFSET(ip, inst, reg, offset, value) \
> -	WREG32( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
> -		(1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
> -		(2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
> -		(3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
> -		(ip##_BASE__INST##inst##_SEG4 + reg))))) + offset, value)
> +	WREG32((adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + reg) + 
> +offset, value)
>   
>   #endif
>