Re: [PATCH v5] dma: qcom: bam_dma: Fix command element mask field for BAM v1.6.0+

[Vinod Koul]

On 14-05-26, 12:09, Varadarajan Narayanan wrote:
> From: Md Sadre Alam <md.alam@xxxxxxxxxxxxxxxx>
> 
> BAM version 1.6.0 and later changed the behavior of the mask field in
> command elements for read operations. In newer BAM versions, the mask
> field for read commands contains the upper 4 bits of the destination
> address to support 36-bit addressing, while for write commands it
> continues to function as a traditional write mask.

But this changes behaviour for all versions. What happens to folks on older
versions, wont this break for them, if not what am I missing

> 
> This change causes NAND enumeration failures on platforms like IPQ5424
> that use BAM v1.6.0+, because the current code sets mask=0xffffffff
> for all commands. For read commands on newer BAM versions, this results
> in the hardware interpreting the destination address as 0xf_xxxxxxxx
> (invalid high memory) instead of the intended 0x0_xxxxxxxx address.
> 
> Fixed this issue by:
> 1. Updating the bam_cmd_element structure documentation to reflect the
>    dual purpose of the mask field
> 2. Modifying bam_prep_ce_le32() to set appropriate mask values based on
>    command type:
>    - For read commands: mask = 0 (32-bit addressing, upper bits = 0)
>    - For write commands: mask = 0xffffffff (traditional write mask)
> 3. Maintaining backward compatibility with older BAM versions
> 
> This fix enables proper NAND functionality on IPQ5424 and other platforms
> using BAM v1.6.0+ while preserving compatibility with existing systems.
> 
> Fixes: dfebb055f73a2 ("dmaengine: qcom: bam_dma: wrapper functions for command descriptor")
> 

No blanks here

> Tested-by: Lakshmi Sowjanya D <quic_laksd@xxxxxxxxxxx>
> Signed-off-by: Md Sadre Alam <md.alam@xxxxxxxxxxxxxxxx>
> Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@xxxxxxxxxxxxxxxx>
> ---
> Change in [v5]
> 	- Split the driver change into a separate patch
> 	- Update commit log with 'Fixes' tag
> 
> Change in [v4] - https://lore.kernel.org/linux-arm-msm/20260206100202.413834-2-quic_mdalam@xxxxxxxxxxx/
> 
> * No change
> 
> Change in [v3]
> 
> * Added Tested-by tag
> 
> Change in [v2]
> 
> * No change
> 
> Change in [v1]
> 
> * Updated bam_prep_ce_le32() to set the mask field conditionally based on
>   command type
> 
> * Enhanced kernel-doc comments to clarify mask behavior for BAM v1.6.0+
> ---
>  include/linux/dma/qcom_bam_dma.h | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/dma/qcom_bam_dma.h b/include/linux/dma/qcom_bam_dma.h
> index 68fc0e643b1b..d9d07a9ab313 100644
> --- a/include/linux/dma/qcom_bam_dma.h
> +++ b/include/linux/dma/qcom_bam_dma.h
> @@ -13,9 +13,12 @@
>   * supported by BAM DMA Engine.
>   *
>   * @cmd_and_addr - upper 8 bits command and lower 24 bits register address.
> - * @data - for write command: content to be written into peripheral register.
> - *	   for read command: dest addr to write peripheral register value.
> - * @mask - register mask.
> + * @data - For write command: content to be written into peripheral register.
> + *	   For read command: lower 32 bits of destination address.
> + * @mask - For write command: register write mask.
> + *	   For read command on BAM v1.6.0+: upper 4 bits of destination address.
> + *	   For read command on BAM < v1.6.0: ignored by hardware.
> + *	   Setting to 0 ensures 32-bit addressing compatibility.
>   * @reserved - for future usage.
>   *
>   */
> @@ -42,6 +45,10 @@ enum bam_command_type {
>   * @addr: target address
>   * @cmd: BAM command
>   * @data: actual data for write and dest addr for read in le32
> + *
> + * For BAM v1.6.0+, the mask field behavior depends on command type:
> + * - Write commands: mask = write mask (typically 0xffffffff)
> + * - Read commands: mask = upper 4 bits of destination address (0 for 32-bit)
>   */
>  static inline void
>  bam_prep_ce_le32(struct bam_cmd_element *bam_ce, u32 addr,
> @@ -50,7 +57,11 @@ bam_prep_ce_le32(struct bam_cmd_element *bam_ce, u32 addr,
>  	bam_ce->cmd_and_addr =
>  		cpu_to_le32((addr & 0xffffff) | ((cmd & 0xff) << 24));
>  	bam_ce->data = data;
> -	bam_ce->mask = cpu_to_le32(0xffffffff);
> +	if (cmd == BAM_READ_COMMAND)
> +		bam_ce->mask = cpu_to_le32(0x0); /* 32-bit addressing */
> +	else
> +		bam_ce->mask = cpu_to_le32(0xffffffff); /* Write mask */
> +	bam_ce->reserved = 0;
>  }
>  
>  /*
> @@ -60,7 +71,7 @@ bam_prep_ce_le32(struct bam_cmd_element *bam_ce, u32 addr,
>   * @bam_ce: BAM command element
>   * @addr: target address
>   * @cmd: BAM command
> - * @data: actual data for write and dest addr for read
> + * @data: actual data for write and destination address for read
>   */
>  static inline void
>  bam_prep_ce(struct bam_cmd_element *bam_ce, u32 addr,
> 
> ---
> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> change-id: 20260514-bam-fix-142a0ee8057e
> 
> Best regards,
> -- 
> Varadarajan Narayanan <varadarajan.narayanan@xxxxxxxxxxxxxxxx>

-- 
~Vinod