Re: [PATCH 1/3] ACPI: CPPC: Avoid unnecessary reads for full-width writes

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

On 8/4/2026 5:05 AM, Christian Loehle wrote:
> SystemMemory GAS entries may describe a field within a wider access
> unit, so cpc_write() reads the access unit before updating the field to
> preserve the surrounding bits. It also does this when the field covers
> the complete access unit.
> 
> When the bit offset is zero and the register bit width equals the
> resolved access width, the previous value cannot affect the result. Skip
> the MMIO read and mask operation in that case. Retain rmw_lock because
> another entry in the same _CPC package may share the access unit.
> 
> Signed-off-by: Christian Loehle <christian.loehle@arm.com>

Nice optimization. Looks good to me.

Reviewed-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>


> ---
>   drivers/acpi/cppc_acpi.c | 43 ++++++++++++++++++++++++----------------
>   1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 53d09ca98f06..9b8d68b44ea9 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1162,25 +1162,34 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
>   			return -ENODEV;
>   		}
>   
> +		/*
> +		 * Only partial fields need the previous contents to preserve bits
> +		 * outside the field. Keep serializing full-width writes because
> +		 * another _CPC entry may share the access unit and require RMW.
> +		 */
>   		raw_spin_lock_irqsave(&cpc_desc->rmw_lock, flags);
> -		switch (size) {
> -		case 8:
> -			prev_val = readb_relaxed(vaddr);
> -			break;
> -		case 16:
> -			prev_val = readw_relaxed(vaddr);
> -			break;
> -		case 32:
> -			prev_val = readl_relaxed(vaddr);
> -			break;
> -		case 64:
> -			prev_val = readq_relaxed(vaddr);
> -			break;
> -		default:
> -			raw_spin_unlock_irqrestore(&cpc_desc->rmw_lock, flags);
> -			return -EFAULT;
> +
> +		if (reg->bit_offset || reg->bit_width != size) {
> +			switch (size) {
> +			case 8:
> +				prev_val = readb_relaxed(vaddr);
> +				break;
> +			case 16:
> +				prev_val = readw_relaxed(vaddr);
> +				break;
> +			case 32:
> +				prev_val = readl_relaxed(vaddr);
> +				break;
> +			case 64:
> +				prev_val = readq_relaxed(vaddr);
> +				break;
> +			default:
> +				raw_spin_unlock_irqrestore(&cpc_desc->rmw_lock,
> +							   flags);
> +				return -EFAULT;
> +			}
> +			val = MASK_VAL_WRITE(reg, prev_val, val);
>   		}
> -		val = MASK_VAL_WRITE(reg, prev_val, val);
>   	}
>   
>   	switch (size) {


-- 
Thx and BRs,
Zhongqiu Han