Re: [PATCH 4/4] drm/imagination: Simplify MIPS PTE definitions

[Alessio Belle <Alessio.Belle@imgtec.com>]

Hi Alexandru,

On Wed, 2026-10-14 at 11:13 +0300, Alexandru Dadu wrote:
> From: Matt Coster <matt.coster@imgtec.com>
> 
> Use BIT() and GENMASK() in the definitions of the MIPS PTE format, and
> remove some unused definitions that add no value.
> 
> Signed-off-by: Matt Coster <matt.coster@imgtec.com>
> Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
> ---
>  drivers/gpu/drm/imagination/pvr_mips_internal.h | 72 ++++++++-----------------
>  drivers/gpu/drm/imagination/pvr_vm_mips.c       | 25 ++++-----
>  2 files changed, 34 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_mips_internal.h b/drivers/gpu/drm/imagination/pvr_mips_internal.h
> index 732730d64bf5..2b25a69003b3 100644
> --- a/drivers/gpu/drm/imagination/pvr_mips_internal.h
> +++ b/drivers/gpu/drm/imagination/pvr_mips_internal.h
> @@ -39,63 +39,33 @@
>  #define PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES (4)
>  /* Total number of TLB entries. */
>  #define PVR_MIPSFW_NUMBER_OF_TLB_ENTRIES (16)
> -/* "Uncached" caching policy. */
> -#define PVR_MIPSFW_UNCACHED_CACHE_POLICY (2)
> -/* "Write-back write-allocate" caching policy. */
> -#define PVR_MIPSFW_WRITEBACK_CACHE_POLICY (3)
> -/* "Write-through no write-allocate" caching policy. */
> -#define PVR_MIPSFW_WRITETHROUGH_CACHE_POLICY (1)
> +
> +enum pvr_fw_mips_cache_policy {
> +	/* "Write-through no write-allocate" caching policy. */
> +	PVR_MIPSFW_CACHE_POLICY_WRITETHROUGH = 1,
> +	/* "Uncached" caching policy. */
> +	PVR_MIPSFW_CACHE_POLICY_UNCACHED = 2,
> +	/* "Write-back write-allocate" caching policy. */
> +	PVR_MIPSFW_CACHE_POLICY_WRITEBACK = 3,
> +};
> +
>  /* Cached policy used by MIPS in case of physical bus on 32 bit. */
> -#define PVR_MIPSFW_CACHED_POLICY (PVR_MIPSFW_WRITEBACK_CACHE_POLICY)
> +#define PVR_MIPSFW_CACHE_POLICY_CACHED (PVR_MIPSFW_CACHE_POLICY_WRITEBACK)
>  /* Cached policy used by MIPS in case of physical bus on more than 32 bit. */
> -#define PVR_MIPSFW_CACHED_POLICY_ABOVE_32BIT (PVR_MIPSFW_WRITETHROUGH_CACHE_POLICY)
> +#define PVR_MIPSFW_CACHE_POLICY_CACHED_ABOVE_32BIT (PVR_MIPSFW_CACHE_POLICY_WRITETHROUGH)
> +
>  /* Total number of Remap entries. */
>  #define PVR_MIPSFW_NUMBER_OF_REMAP_ENTRIES (2 * PVR_MIPSFW_NUMBER_OF_TLB_ENTRIES)
>  
>  /* MIPS EntryLo/PTE format. */
> -
> -#define PVR_MIPSFW_ENTRYLO_READ_INHIBIT_SHIFT (31U)
> -#define PVR_MIPSFW_ENTRYLO_READ_INHIBIT_CLRMSK (0X7FFFFFFF)
> -#define PVR_MIPSFW_ENTRYLO_READ_INHIBIT_EN (0X80000000)
> -
> -#define PVR_MIPSFW_ENTRYLO_EXEC_INHIBIT_SHIFT (30U)
> -#define PVR_MIPSFW_ENTRYLO_EXEC_INHIBIT_CLRMSK (0XBFFFFFFF)
> -#define PVR_MIPSFW_ENTRYLO_EXEC_INHIBIT_EN (0X40000000)
> -
> -/* Page Frame Number */
> -#define PVR_MIPSFW_ENTRYLO_PFN_SHIFT (6)
> -#define PVR_MIPSFW_ENTRYLO_PFN_ALIGNSHIFT (12)
> -/* Mask used for the MIPS Page Table in case of physical bus on 32 bit. */
> -#define PVR_MIPSFW_ENTRYLO_PFN_MASK (0x03FFFFC0)
> -#define PVR_MIPSFW_ENTRYLO_PFN_SIZE (20)
> -/* Mask used for the MIPS Page Table in case of physical bus on more than 32 bit. */
> -#define PVR_MIPSFW_ENTRYLO_PFN_MASK_ABOVE_32BIT (0x3FFFFFC0)
> -#define PVR_MIPSFW_ENTRYLO_PFN_SIZE_ABOVE_32BIT (24)
> -#define PVR_MIPSFW_ADDR_TO_ENTRYLO_PFN_RSHIFT (PVR_MIPSFW_ENTRYLO_PFN_ALIGNSHIFT - \
> -					       PVR_MIPSFW_ENTRYLO_PFN_SHIFT)
> -
> -#define PVR_MIPSFW_ENTRYLO_CACHE_POLICY_SHIFT (3U)
> -#define PVR_MIPSFW_ENTRYLO_CACHE_POLICY_CLRMSK (0XFFFFFFC7)
> -
> -#define PVR_MIPSFW_ENTRYLO_DIRTY_SHIFT (2U)
> -#define PVR_MIPSFW_ENTRYLO_DIRTY_CLRMSK (0XFFFFFFFB)
> -#define PVR_MIPSFW_ENTRYLO_DIRTY_EN (0X00000004)
> -
> -#define PVR_MIPSFW_ENTRYLO_VALID_SHIFT (1U)
> -#define PVR_MIPSFW_ENTRYLO_VALID_CLRMSK (0XFFFFFFFD)
> -#define PVR_MIPSFW_ENTRYLO_VALID_EN (0X00000002)
> -
> -#define PVR_MIPSFW_ENTRYLO_GLOBAL_SHIFT (0U)
> -#define PVR_MIPSFW_ENTRYLO_GLOBAL_CLRMSK (0XFFFFFFFE)
> -#define PVR_MIPSFW_ENTRYLO_GLOBAL_EN (0X00000001)
> -
> -#define PVR_MIPSFW_ENTRYLO_DVG (PVR_MIPSFW_ENTRYLO_DIRTY_EN | \
> -				PVR_MIPSFW_ENTRYLO_VALID_EN | \
> -				PVR_MIPSFW_ENTRYLO_GLOBAL_EN)
> -#define PVR_MIPSFW_ENTRYLO_UNCACHED (PVR_MIPSFW_UNCACHED_CACHE_POLICY << \
> -				     PVR_MIPSFW_ENTRYLO_CACHE_POLICY_SHIFT)
> -#define PVR_MIPSFW_ENTRYLO_DVG_UNCACHED (PVR_MIPSFW_ENTRYLO_DVG | \
> -					 PVR_MIPSFW_ENTRYLO_UNCACHED)
> +#define PVR_MIPSFW_ENTRYLO_READ_INHIBIT BIT_U32(31)
> +#define PVR_MIPSFW_ENTRYLO_EXEC_INHIBIT BIT_U32(30)
> +#define PVR_MIPSFW_ENTRYLO_PFN GENMASK_U32(25, 6)
> +#define PVR_MIPSFW_ENTRYLO_PFN_ABOVE_32BIT GENMASK_U32(29, 6)

Following what Sashiko pointed out, could you propagate this renaming to
PVR_MIPSFW_TLB_GET_PA()?

With that fixed,

Reviewed-by: Alessio Belle <alessio.belle@imgtec.com>

Thanks,
Alessio

> +#define PVR_MIPSFW_ENTRYLO_CACHE_POLICY GENMASK_U32(5, 3)
> +#define PVR_MIPSFW_ENTRYLO_DIRTY BIT_U32(2)
> +#define PVR_MIPSFW_ENTRYLO_VALID BIT_U32(1)
> +#define PVR_MIPSFW_ENTRYLO_GLOBAL BIT_U32(0)
>  
>  /* Remap Range Config Addr Out. */
>  /* These defines refer to the upper half of the Remap Range Config register. */
> diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> index 40495b1486a2..7f49314bc6be 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> @@ -97,11 +97,13 @@ pvr_vm_mips_init(struct pvr_device *pvr_dev)
>  		goto err_free_pages;
>  	}
>  
> -	vm_data->pfn_mask = (phys_bus_width > 32) ? PVR_MIPSFW_ENTRYLO_PFN_MASK_ABOVE_32BIT :
> -						    PVR_MIPSFW_ENTRYLO_PFN_MASK;
> -
> -	vm_data->cache_policy = (phys_bus_width > 32) ? PVR_MIPSFW_CACHED_POLICY_ABOVE_32BIT :
> -							PVR_MIPSFW_CACHED_POLICY;
> +	if (phys_bus_width > 32) {
> +		vm_data->pfn_mask = PVR_MIPSFW_ENTRYLO_PFN_ABOVE_32BIT;
> +		vm_data->cache_policy = PVR_MIPSFW_CACHE_POLICY_CACHED_ABOVE_32BIT;
> +	} else {
> +		vm_data->pfn_mask = PVR_MIPSFW_ENTRYLO_PFN;
> +		vm_data->cache_policy = PVR_MIPSFW_CACHE_POLICY_CACHED;
> +	}
>  
>  	pvr_dev->fw_dev.processor_data.mips_data.vm_data = vm_data;
>  
> @@ -145,15 +147,15 @@ get_mips_pte_flags(bool read, bool write, u32 cache_policy)
>  	u32 flags = 0;
>  
>  	if (read && write) /* Read/write. */
> -		flags |= PVR_MIPSFW_ENTRYLO_DIRTY_EN;
> +		flags |= PVR_MIPSFW_ENTRYLO_DIRTY;
>  	else if (write)    /* Write only. */
> -		flags |= PVR_MIPSFW_ENTRYLO_READ_INHIBIT_EN;
> +		flags |= PVR_MIPSFW_ENTRYLO_READ_INHIBIT;
>  	else
>  		WARN_ON(!read);
>  
> -	flags |= cache_policy << PVR_MIPSFW_ENTRYLO_CACHE_POLICY_SHIFT;
> +	flags |= FIELD_PREP(PVR_MIPSFW_ENTRYLO_CACHE_POLICY, cache_policy);
>  
> -	flags |= PVR_MIPSFW_ENTRYLO_VALID_EN | PVR_MIPSFW_ENTRYLO_GLOBAL_EN;
> +	flags |= PVR_MIPSFW_ENTRYLO_VALID | PVR_MIPSFW_ENTRYLO_GLOBAL;
>  
>  	return flags;
>  }
> @@ -199,7 +201,7 @@ pvr_vm_mips_map(struct pvr_device *pvr_dev, struct pvr_fw_object *fw_obj)
>  	end_pfn = (end & fw_dev->fw_heap_info.offset_mask) >> PVR_MIPSFW_LOG2_PAGE_SIZE_4K;
>  
>  	if (pvr_obj->flags & PVR_BO_FW_FLAGS_DEVICE_UNCACHED)
> -		cache_policy = PVR_MIPSFW_UNCACHED_CACHE_POLICY;
> +		cache_policy = PVR_MIPSFW_CACHE_POLICY_UNCACHED;
>  	else
>  		cache_policy = vm_data->cache_policy;
>  
> @@ -216,8 +218,7 @@ pvr_vm_mips_map(struct pvr_device *pvr_dev, struct pvr_fw_object *fw_obj)
>  		if (err)
>  			goto err_unmap_pages;
>  
> -		pte = ((dma_addr >> PVR_MIPSFW_LOG2_PAGE_SIZE_4K)
> -		       << PVR_MIPSFW_ENTRYLO_PFN_SHIFT) & vm_data->pfn_mask;
> +		pte = field_prep(vm_data->pfn_mask, dma_addr >> PVR_MIPSFW_LOG2_PAGE_SIZE_4K);
>  		pte |= pte_flags;
>  
>  		WRITE_ONCE(vm_data->pt[pfn], pte);
>