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:[email protected]] Sent: Wednesday, November 29, 2017 3:05 PM To: Liu, Shaoyun; [email protected] 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 <[email protected]> > --- > 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 > _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
