Re: [PATCH v6 12/14] crypto: caam - force DMA address to 32-bit on 64-bit i.MX SoCs

[Horia Geanta <horia.geanta@nxp.com>]

On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> i.MX8 SoC still use 32-bit addresses in its CAAM implmentation, so
i.MX8 SoC or i.MX8 mScale?
Looking at the documentation, some i.MX8 parts (for e.g. QM and QXP)
allow for 36-bit addresses.

> change all of the code to be able to handle that.
> 
Shouldn't this case (32-bit CAAM and CONFIG_ARCH_DMA_ADDR_T_64BIT=y) work
for any ARMv8 SoC, i.e. how is this i.MX-specific?

> @@ -603,11 +603,13 @@ static int caam_probe(struct platform_device *pdev)
>  		ret = init_clocks(dev, ctrlpriv, imx_soc_match->data);
>  		if (ret)
>  			return ret;
> +
> +		caam_ptr_sz = sizeof(u32);
> +	} else {
> +		caam_ptr_sz = sizeof(dma_addr_t);
caam_ptr_sz should be deduced by reading MCFGR[PS] bit, i.e. decoupled
from dma_addr_t.

There is another configuration that should be considered
(even though highly unlikely):
caam_ptr_sz=1  - > 32-bit addresses for CAAM
CONFIG_ARCH_DMA_ADDR_T_64BIT=n - 32-bit dma_addr_t
so the logic has to be carefully evaluated.

> @@ -191,7 +191,8 @@ static inline u64 caam_dma64_to_cpu(u64 value)
>  
>  static inline u64 cpu_to_caam_dma(u64 value)
>  {
> -	if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT))
> +	if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
> +	    !caam_imx)
Related to my previous comment (i.MX-specific vs. SoC-generic),
this should probably change to smth. like: caam_ptr_sz == sizeof(u64)

>  		return cpu_to_caam_dma64(value);
>  	else
>  		return cpu_to_caam32(value);
> @@ -199,7 +200,8 @@ static inline u64 cpu_to_caam_dma(u64 value)
>  
>  static inline u64 caam_dma_to_cpu(u64 value)
>  {
> -	if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT))
> +	if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
> +	    !caam_imx)
Same here.

>  		return caam_dma64_to_cpu(value);
>  	else
>  		return caam32_to_cpu(value);
> @@ -213,13 +215,24 @@ static inline u64 caam_dma_to_cpu(u64 value)
>  static inline void jr_outentry_get(void *outring, int hw_idx, dma_addr_t *desc,
>  				   u32 *jrstatus)
>  {
> -	struct {
> -		dma_addr_t desc;/* Pointer to completed descriptor */
> -		u32 jrstatus;	/* Status for completed descriptor */
> -	} __packed *outentry = outring;
>  
> -	*desc = outentry[hw_idx].desc;
> -	*jrstatus = outentry[hw_idx].jrstatus;
> +	if (caam_imx) {
Same here: if (caam_ptr_sz == sizeof(u32))

> +		struct {
> +			u32 desc;
> +			u32 jrstatus;
> +		} __packed *outentry = outring;
> +
> +		*desc = outentry[hw_idx].desc;
> +		*jrstatus = outentry[hw_idx].jrstatus;
> +	} else {
> +		struct {
> +			dma_addr_t desc;/* Pointer to completed descriptor */
> +			u32 jrstatus;	/* Status for completed descriptor */
> +		} __packed *outentry = outring;
> +
> +		*desc = outentry[hw_idx].desc;
> +		*jrstatus = outentry[hw_idx].jrstatus;
> +	}
>  }
>  
>  #define SIZEOF_JR_OUTENTRY	(caam_ptr_sz + sizeof(u32))
> @@ -246,9 +259,15 @@ static inline u32 jr_outentry_jrstatus(void *outring, int hw_idx)
>  
>  static inline void jr_inpentry_set(void *inpring, int hw_idx, dma_addr_t val)
>  {
> -	dma_addr_t *inpentry = inpring;
> +	if (caam_imx) {
And here: if (caam_ptr_sz == sizeof(u32))

> +		u32 *inpentry = inpring;
>  
> -	inpentry[hw_idx] = val;
> +		inpentry[hw_idx] = val;
> +	} else {
> +		dma_addr_t *inpentry = inpring;
> +
> +		inpentry[hw_idx] = val;
> +	}
>  }

Horia