Re: [PATCH 7/9] ARM: dts: bcm2711-rpi: Use 40-bit DMA for HDMI audio

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] ARM: dts: bcm2711: hdmi audio dma40 truncates FIFO address on !LPAE
--

commit 849f01a77c8d05d1bc02c231948e58143158b712
Author: Peter Robinson <pbrobinson@gmail.com>

ARM: dts: bcm2711-rpi: Use 40-bit DMA for HDMI audio

This commit enables the 40-bit DMA controller (dma40) on the Raspberry Pi 4
and switches hdmi0 and hdmi1 audio DMA to use dma40 channels 10 and 17.

> diff --git a/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi b/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
> index 1eb6406449d19..a49a732d3f64e 100644
> --- a/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
> +++ b/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
> @@ -42,6 +42,12 @@ i2c0_1: i2c@1 {
>  	};
>  };
>  
> +&dma40 {
> +	/* The VPU firmware uses DMA channel 11 for VCHIQ */
> +	brcm,dma-channel-mask = <0x7000>;
> +	status = "okay";
> +};
> +
>  &firmware {
>  	expgpio: gpio {
>  		compatible = "raspberrypi,firmware-gpio";
> @@ -59,12 +65,14 @@ reset: reset {
>  &hdmi0 {
>  	clocks = <&firmware_clocks 13>, <&firmware_clocks 14>, <&dvp 0>, <&clk_27MHz>;
>  	clock-names = "hdmi", "bvb", "audio", "cec";
> +	dmas = <&dma40 10>;
>  	wifi-2.4ghz-coexistence;
>  };
>  
>  &hdmi1 {
>  	clocks = <&firmware_clocks 13>, <&firmware_clocks 14>, <&dvp 1>, <&clk_27MHz>;
>  	clock-names = "hdmi", "bvb", "audio", "cec";
> +	dmas = <&dma40 17>;

[Severity: Critical]
Will switching hdmi0 and hdmi1 in arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
to dma40 cause a regression on 32-bit ARM kernels built without
CONFIG_ARM_LPAE (such as bcm2835_defconfig or multi_v7_defconfig)?

When playing HDMI audio, vc4_hdmi_audio_init() configures
dma_slave_config.dst_addr with the VPU bus address of the MAI_DATA FIFO
(0x7ef200xx). With hdmi0 and hdmi1 moved to dma40,
bcm2835_dma_prep_dma_cyclic() calls bcm2835_dma_periph_addr() to translate the
0x7c000000..0x7fffffff peripheral window to the 35-bit SCB bus window
(0x4_7c000000..0x4_7fffffff) by adding c->periph_offset (0x400000000ULL):

drivers/dma/bcm2835-dma.c:bcm2835_dma_periph_addr() {
	if (addr >= BCM2835_DMA_PERIPH_BASE && addr < BCM2835_DMA_PERIPH_END)
		addr += c->periph_offset;

	return addr;
}

However, CONFIG_ARCH_BCM2835 does not select CONFIG_ARM_LPAE or
CONFIG_ARCH_DMA_ADDR_T_64BIT, so dma_addr_t is a 32-bit u32 on non-LPAE 32-bit
ARM builds. Because bcm2835_dma_periph_addr() takes and returns dma_addr_t
(and dst in bcm2835_dma_prep_dma_cyclic(), bcm2835_dma_prep_slave_sg(),
bcm2835_dma_create_cb_chain(), bcm2835_dma_cb_init(), and
bcm2835_dma_cb_set_dst() is also typed as dma_addr_t rather than u64), adding
0x400000000ULL truncates the upper 32 bits, leaving addr as 0x7ef200xx.

When bcm2835_dma_cb_set_dst() then programs the 40-bit control block:

drivers/dma/bcm2835-dma.c:bcm2835_dma_cb_set_dst() {
	if (c->is_40bit_channel) {
		e->cb40->dst = lower_32_bits(addr);
		FIELD_MODIFY(BCM2711_DMA40_ADDR_HI, &e->cb40->dsti,
			     upper_32_bits(addr));
		return;
	}

upper_32_bits(addr) evaluates to 0 instead of 0x4. Doesn't this program the
DMA40 engine to write HDMI audio samples to SCB address 0x0_7ef200xx (physical
SDRAM around the 2 GiB mark) instead of the HDMI MAI FIFO at 0x4_7ef200xx,
breaking HDMI audio and corrupting system memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102090713.896324-1-pbrobinson@gmail.com?part=7