Re: [PATCH v5 2/4] memory: tegra: Group register and fields

[Jon Hunter <jonathanh@nvidia.com>]

On 19/12/2025 11:43, Ketan Patil wrote:
> The current register definitions are not in sorted order. Sort these
> registers according to their address. Put bit fields of the
> corresponding registers below the register definitions to clearly
> identify which fields belongs to which registers.
> 
> Signed-off-by: Ketan Patil <ketanp@nvidia.com>
> ---
>   drivers/memory/tegra/mc.h | 49 +++++++++++++++++++++------------------
>   1 file changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> index a7f20850741f..482f836f7816 100644
> --- a/drivers/memory/tegra/mc.h
> +++ b/drivers/memory/tegra/mc.h
> @@ -13,13 +13,31 @@
>   #include <soc/tegra/mc.h>
>   
>   #define MC_INTSTATUS					0x00
> +/* Bit field of MC_INTSTATUS register */
> +#define MC_INT_DECERR_EMEM				BIT(6)
> +#define MC_INT_INVALID_GART_PAGE			BIT(7)
> +#define MC_INT_SECURITY_VIOLATION			BIT(8)
> +#define MC_INT_ARBITRATION_EMEM				BIT(9)
> +#define MC_INT_INVALID_SMMU_PAGE			BIT(10)
> +#define MC_INT_INVALID_APB_ASID_UPDATE			BIT(11)
> +#define MC_INT_DECERR_VPR				BIT(12)
> +#define MC_INT_SECERR_SEC				BIT(13)
> +#define MC_INT_DECERR_MTS				BIT(16)
> +#define MC_INT_DECERR_GENERALIZED_CARVEOUT		BIT(17)
> +#define MC_INT_DECERR_ROUTE_SANITY			BIT(20)
> +
>   #define MC_INTMASK					0x04
>   #define MC_GART_ERROR_REQ				0x30
>   #define MC_EMEM_ADR_CFG					0x54
> +#define MC_EMEM_ADR_CFG_EMEM_NUMDEV			BIT(0)
> +
>   #define MC_DECERR_EMEM_OTHERS_STATUS			0x58
>   #define MC_SECURITY_VIOLATION_STATUS			0x74
>   #define MC_EMEM_ARB_CFG					0x90
>   #define MC_EMEM_ARB_OUTSTANDING_REQ			0x94
> +#define MC_EMEM_ARB_OUTSTANDING_REQ_HOLDOFF_OVERRIDE	BIT(30)
> +#define MC_EMEM_ARB_OUTSTANDING_REQ_LIMIT_ENABLE	BIT(31)
> +
>   #define MC_EMEM_ARB_TIMING_RCD				0x98
>   #define MC_EMEM_ARB_TIMING_RP				0x9c
>   #define MC_EMEM_ARB_TIMING_RC				0xa0
> @@ -41,44 +59,31 @@
>   #define MC_EMEM_ARB_OVERRIDE				0xe8
>   #define MC_TIMING_CONTROL_DBG				0xf8
>   #define MC_TIMING_CONTROL				0xfc
> +#define MC_TIMING_UPDATE				BIT(0)
> +
>   #define MC_GLOBAL_INTSTATUS				0xf24
>   
> -#define MC_INT_DECERR_ROUTE_SANITY			BIT(20)
> -#define MC_INT_DECERR_GENERALIZED_CARVEOUT		BIT(17)
> -#define MC_INT_DECERR_MTS				BIT(16)
> -#define MC_INT_SECERR_SEC				BIT(13)
> -#define MC_INT_DECERR_VPR				BIT(12)
> -#define MC_INT_INVALID_APB_ASID_UPDATE			BIT(11)
> -#define MC_INT_INVALID_SMMU_PAGE			BIT(10)
> -#define MC_INT_ARBITRATION_EMEM				BIT(9)
> -#define MC_INT_SECURITY_VIOLATION			BIT(8)
> -#define MC_INT_INVALID_GART_PAGE			BIT(7)
> -#define MC_INT_DECERR_EMEM				BIT(6)
> +/* Bit field of MC_ERR_STATUS_0 register */
> +#define MC_ERR_STATUS_RW				BIT(16)
> +#define MC_ERR_STATUS_SECURITY				BIT(17)
> +#define MC_ERR_STATUS_NONSECURE				BIT(25)
> +#define MC_ERR_STATUS_WRITABLE				BIT(26)
> +#define MC_ERR_STATUS_READABLE				BIT(27)
>   
>   #define MC_ERR_STATUS_TYPE_SHIFT			28
>   #define MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE		(0x6 << 28)
>   #define MC_ERR_STATUS_TYPE_MASK				(0x7 << 28)
> -#define MC_ERR_STATUS_READABLE				BIT(27)
> -#define MC_ERR_STATUS_WRITABLE				BIT(26)
> -#define MC_ERR_STATUS_NONSECURE				BIT(25)
> +
>   #define MC_ERR_STATUS_ADR_HI_SHIFT			20
>   #define MC_ERR_STATUS_ADR_HI_MASK			0x3
> -#define MC_ERR_STATUS_SECURITY				BIT(17)
> -#define MC_ERR_STATUS_RW				BIT(16)
> -
> -#define MC_EMEM_ADR_CFG_EMEM_NUMDEV			BIT(0)
>   
>   #define MC_EMEM_ARB_CFG_CYCLES_PER_UPDATE(x)		((x) & 0x1ff)
>   #define MC_EMEM_ARB_CFG_CYCLES_PER_UPDATE_MASK		0x1ff
>   
>   #define MC_EMEM_ARB_OUTSTANDING_REQ_MAX_MASK		0x1ff

Shouldn't the above masks be moved as well? Typically we put both the 
masks and bits next to the associated registers.

> -#define MC_EMEM_ARB_OUTSTANDING_REQ_HOLDOFF_OVERRIDE	BIT(30)
> -#define MC_EMEM_ARB_OUTSTANDING_REQ_LIMIT_ENABLE	BIT(31)
>   
>   #define MC_EMEM_ARB_OVERRIDE_EACK_MASK			0x3

And this one.

Jon

-- 
nvpublic