On 4/11/2024 2:23 PM, Vanshidhar Konda wrote: > commit 2f4a4d63a193be6fd530d180bb13c3592052904c modified > cpc_read/cpc_write to use access_width to read CPC registers. For PCC > registers the access width field in the ACPI register macro specifies > the PCC subspace id. For non-zero PCC subspace id the access width is > incorrectly treated as access width. This causes errors when reading > from PCC registers in the CPPC driver. > > For PCC registers base the size of read/write on the bit width field. > The debug message in cpc_read/cpc_write is updated to print relevant > information for the address space type used to read the register. > > Signed-off-by: Vanshidhar Konda <vanshikonda@xxxxxxxxxxxxxxxxxxxxxx> > Tested-by: Jarred White <jarredwhite@xxxxxxxxxxxxxxxxxxx> > Reviewed-by: Jarred White <jarredwhite@xxxxxxxxxxxxxxxxxxx> > Cc: 5.15+ <stable@xxxxxxxxxxxxxxx> # 5.15+ > --- > > When testing v6.9-rc1 kernel on AmpereOne system dmesg showed that > cpufreq policy had failed to initialize on some cores during boot because > cpufreq->get() always returned 0. On this system CPPC registers are in PCC > subspace index 2 that are 32 bits wide. With this patch the CPPC driver > interpreted the access width field as 16 bits, causing the register read > to roll over too quickly to provide valid values during frequency > computation. > > v2: > - Use size variable in debug print message > - Use size instead of reg->bit_width for acpi_os_read_memory and > acpi_os_write_memory > > drivers/acpi/cppc_acpi.c | 53 ++++++++++++++++++++++++++++------------ > 1 file changed, 37 insertions(+), 16 deletions(-) Thanks for adding the CC: stable tag. Couple of nits, assuming those are fixed: Reviewed-by: Easwar Hariharan <eahariha@xxxxxxxxxxxxxxxxxxx> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 4bfbe55553f4..a037e9d15f48 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -1002,14 +1002,14 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) > } > > *val = 0; > + size = GET_BIT_WIDTH(reg); > > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > - u32 width = GET_BIT_WIDTH(reg); > u32 val_u32; > acpi_status status; > > status = acpi_os_read_port((acpi_io_address)reg->address, > - &val_u32, width); > + &val_u32, size); > if (ACPI_FAILURE(status)) { > pr_debug("Error: Failed to read SystemIO port %llx\n", > reg->address); > @@ -1018,17 +1018,22 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) > > *val = val_u32; > return 0; > - } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) > + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) { > + /* > + * For registers in PCC space, the register size is determined > + * by the bit width field; the access size is used to indicate > + * the PCC subspace id. > + */ > + size = reg->bit_width; > vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id); > + } > else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) > vaddr = reg_res->sys_mem_vaddr; > else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) > return cpc_read_ffh(cpu, reg, val); > else > return acpi_os_read_memory((acpi_physical_address)reg->address, > - val, reg->bit_width); > - > - size = GET_BIT_WIDTH(reg); > + val, size); > > switch (size) { > case 8: > @@ -1044,8 +1049,13 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) > *val = readq_relaxed(vaddr); > break; > default: > - pr_debug("Error: Cannot read %u bit width from PCC for ss: %d\n", > - reg->bit_width, pcc_ss_id); > + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) { > + pr_debug("Error: Cannot read %u width from for system memory: 0x%llx\n", > + size, reg->address); Nit: from for? There might be a missing word there, or just an extra. Ditto for cpc_write() below. > + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) { > + pr_debug("Error: Cannot read %u bit width to PCC for ss: %d\n", > + size, pcc_ss_id); > + } > return -EFAULT; > } > > @@ -1063,12 +1073,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); > struct cpc_reg *reg = ®_res->cpc_entry.reg; > > + size = GET_BIT_WIDTH(reg); > + > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > - u32 width = GET_BIT_WIDTH(reg); > acpi_status status; > > status = acpi_os_write_port((acpi_io_address)reg->address, > - (u32)val, width); > + (u32)val, size); > if (ACPI_FAILURE(status)) { > pr_debug("Error: Failed to write SystemIO port %llx\n", > reg->address); > @@ -1076,17 +1087,22 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > } > > return 0; > - } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) > + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) { > + /* > + * For registers in PCC space, the register size is determined > + * by the bit width field; the access size is used to indicate > + * the PCC subspace id. > + */ > + size = reg->bit_width; > vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id); > + } > else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) > vaddr = reg_res->sys_mem_vaddr; > else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) > return cpc_write_ffh(cpu, reg, val); > else > return acpi_os_write_memory((acpi_physical_address)reg->address, > - val, reg->bit_width); > - > - size = GET_BIT_WIDTH(reg); > + val, size); > > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) > val = MASK_VAL(reg, val); > @@ -1105,8 +1121,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > writeq_relaxed(val, vaddr); > break; > default: > - pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n", > - reg->bit_width, pcc_ss_id); > + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) { > + pr_debug("Error: Cannot write %u width from for system memory: 0x%llx\n", > + size, reg->address); > + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) { > + pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n", > + size, pcc_ss_id); > + } > ret_val = -EFAULT; > break; > }