Re: [PATCH v3 2/2] dmaengine: apple-admac: Add M3 generation ADMACs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional overwrite of global configuration registers with a potentially hardcoded channel mask during per-channel setup.
--

--- Patch [2]: [PATCH v3 2/2] dmaengine: apple-admac: Add M3 generation ADMACs ---
commit 14231adca1c04d18dabe6aedf727a8ccef7b20a1
Author: Sasha Finkelstein <[email protected]>

dmaengine: apple-admac: Add M3 generation ADMACs
    
The admacs present on t8122 and t603x SoCs need additional writes in
order to operate correctly. The exact purpose of this register
is unknown.

> diff --git a/drivers/dma/apple-admac.c b/drivers/dma/apple-admac.c
> --- a/drivers/dma/apple-admac.c
> +++ b/drivers/dma/apple-admac.c
[ ... ]
> @@ -747,6 +756,11 @@ static int admac_device_config(struct dma_chan *chan,
>  	u32 bus_width = readl_relaxed(ad->base + REG_BUS_WIDTH(adchan->no)) &
>  		~(BUS_WIDTH_WORD_SIZE | BUS_WIDTH_FRAME_SIZE);
>  
> +	if (ad->set_unk28) {
> +		writel_relaxed(T8122_UNK_28_VAL, ad->base + REG_UNK_28);
> +		writel_relaxed(T8122_UNK_28_VAL, ad->base + REG_UNK_2C);
> +	}
> +

[Severity: High]
Does this code overwrite global configuration state for all channels?

Since admac_device_config() is called per-channel during setup, writing a
hardcoded value (0x200000, which is BIT(21)) directly to global controller
registers without a read-modify-write will unconditionally clear any existing
state for other active channels.

Is T8122_UNK_28_VAL a global magic value, or might it be a channel-specific
bitmask captured from a trace that should be applied via a read-modify-write
incorporating the channel number?

>  	switch (is_tx ? config->dst_addr_width : config->src_addr_width) {
>  	case DMA_SLAVE_BUSWIDTH_1_BYTE:
>  		wordsize = 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2