[Intel-xe] [PATCH v2 08/17] drm/xe/guc: Convert GuC registers to REG_FIELD/REG_BIT

Sat Apr 22 15:03:04 UTC 2023

On Fri, Apr 21, 2023 at 05:09:50PM -0700, Matt Roper wrote:
>On Fri, Apr 21, 2023 at 03:32:49PM -0700, Lucas De Marchi wrote:
>> Cleanup GuC register declarations by converting them to use REG_FIELD,
>> REG_BIT and REG_GENMASK. While converting, also reorder the bitfields
>> so they follow the convention of declaring the higher bits first.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/regs/xe_guc_regs.h | 168 +++++++++++++-------------
>>  drivers/gpu/drm/xe/xe_ggtt.c          |   6 +-
>>  drivers/gpu/drm/xe/xe_guc.c           |   8 +-
>>  drivers/gpu/drm/xe/xe_guc_ads.c       |   3 +-
>>  4 files changed, 89 insertions(+), 96 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_guc_regs.h b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> index 011868ff38aa..facb1f0a55b6 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> @@ -14,23 +14,18 @@
>>  /* Definitions of GuC H/W registers, bits, etc */
>>
>>  #define GUC_STATUS			_MMIO(0xc000)
>> -#define   GS_RESET_SHIFT		0
>> -#define   GS_MIA_IN_RESET		  (0x01 << GS_RESET_SHIFT)
>> -#define   GS_BOOTROM_SHIFT		1
>> -#define   GS_BOOTROM_MASK		  (0x7F << GS_BOOTROM_SHIFT)
>> -#define   GS_BOOTROM_RSA_FAILED		  (0x50 << GS_BOOTROM_SHIFT)
>> -#define   GS_BOOTROM_JUMP_PASSED	  (0x76 << GS_BOOTROM_SHIFT)
>> -#define   GS_UKERNEL_SHIFT		8
>> -#define   GS_UKERNEL_MASK		  (0xFF << GS_UKERNEL_SHIFT)
>> -#define   GS_MIA_SHIFT			16
>> -#define   GS_MIA_MASK			  (0x07 << GS_MIA_SHIFT)
>> -#define   GS_MIA_CORE_STATE		  (0x01 << GS_MIA_SHIFT)
>> -#define   GS_MIA_HALT_REQUESTED		  (0x02 << GS_MIA_SHIFT)
>> -#define   GS_MIA_ISR_ENTRY		  (0x04 << GS_MIA_SHIFT)
>> -#define   GS_AUTH_STATUS_SHIFT		30
>> -#define   GS_AUTH_STATUS_MASK		  (0x03 << GS_AUTH_STATUS_SHIFT)
>> -#define   GS_AUTH_STATUS_BAD		  (0x01 << GS_AUTH_STATUS_SHIFT)
>> -#define   GS_AUTH_STATUS_GOOD		  (0x02 << GS_AUTH_STATUS_SHIFT)
>> +#define   GS_AUTH_STATUS_MASK		REG_GENMASK(31, 30)
>> +#define   GS_AUTH_STATUS_BAD		REG_FIELD_PREP(GS_AUTH_STATUS_MASK, 0x1)
>> +#define   GS_AUTH_STATUS_GOOD		REG_FIELD_PREP(GS_AUTH_STATUS_MASK, 0x2)
>> +#define   GS_MIA_MASK			REG_GENMASK(18, 16)
>> +#define   GS_MIA_CORE_STATE		REG_FIELD_PREP(GS_MIA_MASK, 0x1)
>> +#define   GS_MIA_HALT_REQUESTED		REG_FIELD_PREP(GS_MIA_MASK, 0x2)
>> +#define   GS_MIA_ISR_ENTRY		REG_FIELD_PREP(GS_MIA_MASK, 0x4)
>> +#define   GS_UKERNEL_MASK		REG_GENMASK(15, 8)
>> +#define   GS_BOOTROM_MASK		REG_GENMASK(7, 1)
>> +#define   GS_BOOTROM_RSA_FAILED		REG_FIELD_PREP(GS_BOOTROM_MASK, 0x50)
>> +#define   GS_BOOTROM_JUMP_PASSED	REG_FIELD_PREP(GS_BOOTROM_MASK, 0x76)
>> +#define   GS_MIA_IN_RESET		REG_BIT(0)
>>
>>  #define SOFT_SCRATCH(n)			_MMIO(0xc180 + (n) * 4)
>>  #define SOFT_SCRATCH_COUNT		16
>> @@ -42,90 +37,89 @@
>>  #define DMA_ADDR_0_HIGH			_MMIO(0xc304)
>>  #define DMA_ADDR_1_LOW			_MMIO(0xc308)
>>  #define DMA_ADDR_1_HIGH			_MMIO(0xc30c)
>> -#define   DMA_ADDRESS_SPACE_WOPCM	  (7 << 16)
>> -#define   DMA_ADDRESS_SPACE_GTT		  (8 << 16)
>> +#define   DMA_ADDR_SPACE_MASK		REG_GENMASK(20, 16)
>> +#define   DMA_ADDRESS_SPACE_WOPCM	REG_FIELD_PREP(DMA_ADDR_SPACE_MASK, 7)
>> +#define   DMA_ADDRESS_SPACE_GTT		REG_FIEDL_PREP(DMA_ADDR_SPACE_MASK, 8)
>
>s/FIEDL/FIELD/.  Seems like his field isn't actually used anywhere
>(which is why we avoided a build failure).  Should we just drop it?

oops, thanks.

>
>
>>  #define DMA_COPY_SIZE			_MMIO(0xc310)
>>  #define DMA_CTRL			_MMIO(0xc314)
>> -#define   HUC_UKERNEL			  (1<<9)
>> -#define   UOS_MOVE			  (1<<4)
>> -#define   START_DMA			  (1<<0)
>> +#define   HUC_UKERNEL			REG_BIT(9)
>> +#define   UOS_MOVE			REG_BIT(4)
>> +#define   START_DMA			REG_BIT(0)
>>  #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
>> -#define   GUC_WOPCM_OFFSET_VALID	  (1<<0)
>> -#define   HUC_LOADING_AGENT_VCR		  (0<<1)
>> -#define   HUC_LOADING_AGENT_GUC		  (1<<1)
>>  #define   GUC_WOPCM_OFFSET_SHIFT	14
>
>Can we drop the _SHIFT...

that's what I attempted to do in this commit. However this one I left
here since it's used in non-obvious ways:

	drivers/gpu/drm/xe/xe_wopcm.c:#define GUC_WOPCM_OFFSET_ALIGNMENT        (1UL << GUC_WOPCM_OFFSET_SHIFT)

It seems that it uses the same shift in the register to later align
guc_wopcm_base. I'm not sure if this was really intended or not.

>
>> -#define   GUC_WOPCM_OFFSET_MASK		  (0x3ffff << GUC_WOPCM_OFFSET_SHIFT)
>> -#define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
>> +#define   GUC_WOPCM_OFFSET_MASK		REG_GENMASK(31, GUC_WOPCM_OFFSET_SHIFT)
>
>...and hardcode the 14 here?
>
>> +#define   HUC_LOADING_AGENT_MASK	REG_BIT(1)
>> +#define   HUC_LOADING_AGENT_VCR		REG_FIELD_PREP(HUC_LOADING_AGENT_MASK, 0)
>
>HUC_LOADING_AGENT_VCR is never used as far as I can see.  And single-bit
>"masks" with REG_FIELD_PREP are kind of silly; I think we can just
>define HUC_LOADING_AGENT_GUC as REG_BIT(1).

sounds good. I will change that in the next version

>
>
>At some point we should also add another patch that sorts this file by
>offset.  Doesn't need to be part of this series though.

yeah, I sorted some but not alls. I also want to have a lint checker
that prevents it going wrong over time. We could add that as a CI hook
check.

thanks
Lucas De Marchi