Re: [PATCH v4 1/2] iommu/arm-smmu-v3: Add a cmdq_entries module parameter

[Nicolin Chen]

Hi,

I think there can be some small cosmetic changes. I gave it a try, and
it looks overall cleaner to me. Hope you would agree.

On Wed, Sep 02, 2026 at 01:17:23PM +0100, Kiryl Shutsemau (Meta) wrote:
> +/**
> + * arm_smmu_queue_max_n_shift() - pick the log2 depth of a queue
> + * @hw_shift: log2 depth the hardware allows, capped for natural alignment

We can drop "capped for natural alignment" -- evtq/priq don't cap.

> + * @ent_sz_shift: log2 of the queue entry size in bytes
> + * @want: number of entries asked for, or zero to use @hw_shift

Instead, rename to:

@ceiling: default max_n_shift ceiling of the queue
@ent_sz_shift: log2 of the queue entry size in bytes
@new_ceiling: new ceiling to override; use the default if 0

> + * @want is rounded down to a power of two. It never sizes a queue below one
> + * page, because coherent DMA is page granular: a shallower queue occupies the
> + * same memory as one that fills the page, and arm_smmu_init_one_queue() stops
> + * shrinking at a page too.
> + */
> +static u32 arm_smmu_queue_max_n_shift(u32 hw_shift, u32 ent_sz_shift, u32 want)
> +{
> +	u32 page_shift = PAGE_SHIFT - ent_sz_shift;

And let's call it "floor". It fits the comments and commit message.

> +
> +	if (!want)
> +		return hw_shift;
> +
> +	return min(hw_shift, max(ilog2(want), page_shift));
> +}
> +
> +/*
> + * Command queues are also allocated by the Tegra241 CMDQV for its VCMDQs, which
> + * need the same depth decision.
> + */
> +u32 arm_smmu_cmdq_max_n_shift(u32 hw_shift)
> +{
> +	return arm_smmu_queue_max_n_shift(hw_shift, CMDQ_ENT_SZ_SHIFT,
> +					  cmdq_entries);
> +}

Here, move in the natural alignment cap.

u32 arm_smmu_cmdq_max_n_shift(u32 ceiling)
{
	/* Capped to ensure natural alignment */
	ceiling = min_t(u32, CMDQ_MAX_SZ_SHIFT, ceiling);

	return arm_smmu_queue_max_n_shift(
		ceiling, CMDQ_ENT_SZ_SHIFT,
		cmdq_max_entries ? ilog2(cmdq_max_entries) : 0);
}
  
> @@ -662,8 +663,8 @@ static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
>  
>  	/* Cap queue size to SMMU's IDR1.CMDQS and ensure natural alignment */
>  	regval = readl_relaxed(smmu->base + ARM_SMMU_IDR1);
> -	q->llq.max_n_shift =
> -		min_t(u32, CMDQ_MAX_SZ_SHIFT, FIELD_GET(IDR1_CMDQS, regval));
> +	hw_shift = min_t(u32, CMDQ_MAX_SZ_SHIFT, FIELD_GET(IDR1_CMDQS, regval));
> +	q->llq.max_n_shift = arm_smmu_cmdq_max_n_shift(hw_shift);

Then, we can just use FIELD_GET() at the two call sites:

	smmu->cmdq.q.llq.max_n_shift =
		arm_smmu_cmdq_max_n_shift(FIELD_GET(IDR1_CMDQS, reg));

Thanks
Nicolin