Re: [PATCH v2 2/9] drm/panthor: Use 64-bit and poll register accessors
From: Karunika Choo
Date: Wed Apr 09 2025 - 09:07:49 EST
On 21/03/2025 07:53, Boris Brezillon wrote:
> On Thu, 20 Mar 2025 11:17:34 +0000
> Karunika Choo <karunika.choo@xxxxxxx> wrote:
>
>> This patch updates Panthor to use the new 64-bit accessors and poll
>> functions.
>
> nit: I don't think it makes sense to dissociate the introduction of the
> new helpers and their use. Could we squash this patch into the previous
> one?
It was previously requested that I split the patches into two to ease
review. I can merge it back into the previous one in v3.
Kind regards,
Karunika Choo
>
>>
>> Signed-off-by: Karunika Choo <karunika.choo@xxxxxxx>
>> ---
>> drivers/gpu/drm/panthor/panthor_fw.c | 9 +-
>> drivers/gpu/drm/panthor/panthor_gpu.c | 142 +++++++-------------------
>> drivers/gpu/drm/panthor/panthor_mmu.c | 34 ++----
>> 3 files changed, 53 insertions(+), 132 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
>> index 0f52766a3120..ecfbe0456f89 100644
>> --- a/drivers/gpu/drm/panthor/panthor_fw.c
>> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
>> @@ -1059,8 +1059,8 @@ static void panthor_fw_stop(struct panthor_device *ptdev)
>> u32 status;
>>
>> gpu_write(ptdev, MCU_CONTROL, MCU_CONTROL_DISABLE);
>> - if (readl_poll_timeout(ptdev->iomem + MCU_STATUS, status,
>> - status == MCU_STATUS_DISABLED, 10, 100000))
>> + if (gpu_read_poll_timeout(ptdev, MCU_STATUS, status,
>> + status == MCU_STATUS_DISABLED, 10, 100000))
>> drm_err(&ptdev->base, "Failed to stop MCU");
>> }
>>
>> @@ -1085,8 +1085,9 @@ void panthor_fw_pre_reset(struct panthor_device *ptdev, bool on_hang)
>>
>> panthor_fw_update_reqs(glb_iface, req, GLB_HALT, GLB_HALT);
>> gpu_write(ptdev, CSF_DOORBELL(CSF_GLB_DOORBELL_ID), 1);
>> - if (!readl_poll_timeout(ptdev->iomem + MCU_STATUS, status,
>> - status == MCU_STATUS_HALT, 10, 100000)) {
>> + if (!gpu_read_poll_timeout(ptdev, MCU_STATUS, status,
>> + status == MCU_STATUS_HALT, 10,
>> + 100000)) {
>> ptdev->reset.fast = true;
>> } else {
>> drm_warn(&ptdev->base, "Failed to cleanly suspend MCU");
>> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
>> index 671049020afa..0dee011fe2e9 100644
>> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
>> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
>> @@ -108,14 +108,9 @@ static void panthor_gpu_init_info(struct panthor_device *ptdev)
>>
>> ptdev->gpu_info.as_present = gpu_read(ptdev, GPU_AS_PRESENT);
>>
>> - ptdev->gpu_info.shader_present = gpu_read(ptdev, GPU_SHADER_PRESENT_LO);
>> - ptdev->gpu_info.shader_present |= (u64)gpu_read(ptdev, GPU_SHADER_PRESENT_HI) << 32;
>> -
>> - ptdev->gpu_info.tiler_present = gpu_read(ptdev, GPU_TILER_PRESENT_LO);
>> - ptdev->gpu_info.tiler_present |= (u64)gpu_read(ptdev, GPU_TILER_PRESENT_HI) << 32;
>> -
>> - ptdev->gpu_info.l2_present = gpu_read(ptdev, GPU_L2_PRESENT_LO);
>> - ptdev->gpu_info.l2_present |= (u64)gpu_read(ptdev, GPU_L2_PRESENT_HI) << 32;
>> + ptdev->gpu_info.shader_present = gpu_read64(ptdev, GPU_SHADER_PRESENT_LO);
>> + ptdev->gpu_info.tiler_present = gpu_read64(ptdev, GPU_TILER_PRESENT_LO);
>> + ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO);
>>
>> arch_major = GPU_ARCH_MAJOR(ptdev->gpu_info.gpu_id);
>> product_major = GPU_PROD_MAJOR(ptdev->gpu_info.gpu_id);
>> @@ -152,8 +147,7 @@ static void panthor_gpu_irq_handler(struct panthor_device *ptdev, u32 status)
>> {
>> if (status & GPU_IRQ_FAULT) {
>> u32 fault_status = gpu_read(ptdev, GPU_FAULT_STATUS);
>> - u64 address = ((u64)gpu_read(ptdev, GPU_FAULT_ADDR_HI) << 32) |
>> - gpu_read(ptdev, GPU_FAULT_ADDR_LO);
>> + u64 address = gpu_read64(ptdev, GPU_FAULT_ADDR_LO);
>>
>> drm_warn(&ptdev->base, "GPU Fault 0x%08x (%s) at 0x%016llx\n",
>> fault_status, panthor_exception_name(ptdev, fault_status & 0xFF),
>> @@ -244,45 +238,27 @@ int panthor_gpu_block_power_off(struct panthor_device *ptdev,
>> u32 pwroff_reg, u32 pwrtrans_reg,
>> u64 mask, u32 timeout_us)
>> {
>> - u32 val, i;
>> + u32 val;
>> int ret;
>>
>> - for (i = 0; i < 2; i++) {
>> - u32 mask32 = mask >> (i * 32);
>> -
>> - if (!mask32)
>> - continue;
>> -
>> - ret = readl_relaxed_poll_timeout(ptdev->iomem + pwrtrans_reg + (i * 4),
>> - val, !(mask32 & val),
>> - 100, timeout_us);
>> - if (ret) {
>> - drm_err(&ptdev->base, "timeout waiting on %s:%llx power transition",
>> - blk_name, mask);
>> - return ret;
>> - }
>> + ret = gpu_read64_relaxed_poll_timeout(ptdev, pwrtrans_reg, val, !val,
>> + 100, timeout_us);
>> + if (ret) {
>> + drm_err(&ptdev->base,
>> + "timeout waiting on %s:%llx power transition", blk_name,
>> + mask);
>> + return ret;
>> }
>>
>> - if (mask & GENMASK(31, 0))
>> - gpu_write(ptdev, pwroff_reg, mask);
>> -
>> - if (mask >> 32)
>> - gpu_write(ptdev, pwroff_reg + 4, mask >> 32);
>> -
>> - for (i = 0; i < 2; i++) {
>> - u32 mask32 = mask >> (i * 32);
>> + gpu_write64(ptdev, pwroff_reg, mask);
>>
>> - if (!mask32)
>> - continue;
>> -
>> - ret = readl_relaxed_poll_timeout(ptdev->iomem + pwrtrans_reg + (i * 4),
>> - val, !(mask32 & val),
>> - 100, timeout_us);
>> - if (ret) {
>> - drm_err(&ptdev->base, "timeout waiting on %s:%llx power transition",
>> - blk_name, mask);
>> - return ret;
>> - }
>> + ret = gpu_read64_relaxed_poll_timeout(ptdev, pwrtrans_reg, val, !val,
>> + 100, timeout_us);
>> + if (ret) {
>> + drm_err(&ptdev->base,
>> + "timeout waiting on %s:%llx power transition", blk_name,
>> + mask);
>> + return ret;
>> }
>>
>> return 0;
>> @@ -305,45 +281,26 @@ int panthor_gpu_block_power_on(struct panthor_device *ptdev,
>> u32 pwron_reg, u32 pwrtrans_reg,
>> u32 rdy_reg, u64 mask, u32 timeout_us)
>> {
>> - u32 val, i;
>> + u32 val;
>> int ret;
>>
>> - for (i = 0; i < 2; i++) {
>> - u32 mask32 = mask >> (i * 32);
>> -
>> - if (!mask32)
>> - continue;
>> -
>> - ret = readl_relaxed_poll_timeout(ptdev->iomem + pwrtrans_reg + (i * 4),
>> - val, !(mask32 & val),
>> - 100, timeout_us);
>> - if (ret) {
>> - drm_err(&ptdev->base, "timeout waiting on %s:%llx power transition",
>> - blk_name, mask);
>> - return ret;
>> - }
>> + ret = gpu_read64_relaxed_poll_timeout(ptdev, pwrtrans_reg, val, !val,
>> + 100, timeout_us);
>> + if (ret) {
>> + drm_err(&ptdev->base,
>> + "timeout waiting on %s:%llx power transition", blk_name,
>> + mask);
>> + return ret;
>> }
>>
>> - if (mask & GENMASK(31, 0))
>> - gpu_write(ptdev, pwron_reg, mask);
>> -
>> - if (mask >> 32)
>> - gpu_write(ptdev, pwron_reg + 4, mask >> 32);
>> -
>> - for (i = 0; i < 2; i++) {
>> - u32 mask32 = mask >> (i * 32);
>> + gpu_write64(ptdev, pwron_reg, mask);
>>
>> - if (!mask32)
>> - continue;
>> -
>> - ret = readl_relaxed_poll_timeout(ptdev->iomem + rdy_reg + (i * 4),
>> - val, (mask32 & val) == mask32,
>> - 100, timeout_us);
>> - if (ret) {
>> - drm_err(&ptdev->base, "timeout waiting on %s:%llx readiness",
>> - blk_name, mask);
>> - return ret;
>> - }
>> + ret = gpu_read64_relaxed_poll_timeout(ptdev, pwrtrans_reg, val, !val,
>> + 100, timeout_us);
>> + if (ret) {
>> + drm_err(&ptdev->base, "timeout waiting on %s:%llx readiness",
>> + blk_name, mask);
>> + return ret;
>> }
>>
>> return 0;
>> @@ -492,26 +449,6 @@ void panthor_gpu_resume(struct panthor_device *ptdev)
>> panthor_gpu_l2_power_on(ptdev);
>> }
>>
>> -/**
>> - * panthor_gpu_read_64bit_counter() - Read a 64-bit counter at a given offset.
>> - * @ptdev: Device.
>> - * @reg: The offset of the register to read.
>> - *
>> - * Return: The counter value.
>> - */
>> -static u64
>> -panthor_gpu_read_64bit_counter(struct panthor_device *ptdev, u32 reg)
>> -{
>> - u32 hi, lo;
>> -
>> - do {
>> - hi = gpu_read(ptdev, reg + 0x4);
>> - lo = gpu_read(ptdev, reg);
>> - } while (hi != gpu_read(ptdev, reg + 0x4));
>> -
>> - return ((u64)hi << 32) | lo;
>> -}
>> -
>> /**
>> * panthor_gpu_read_timestamp() - Read the timestamp register.
>> * @ptdev: Device.
>> @@ -520,7 +457,7 @@ panthor_gpu_read_64bit_counter(struct panthor_device *ptdev, u32 reg)
>> */
>> u64 panthor_gpu_read_timestamp(struct panthor_device *ptdev)
>> {
>> - return panthor_gpu_read_64bit_counter(ptdev, GPU_TIMESTAMP_LO);
>> + return gpu_read64_sync(ptdev, GPU_TIMESTAMP_LO);
>> }
>>
>> /**
>> @@ -531,10 +468,5 @@ u64 panthor_gpu_read_timestamp(struct panthor_device *ptdev)
>> */
>> u64 panthor_gpu_read_timestamp_offset(struct panthor_device *ptdev)
>> {
>> - u32 hi, lo;
>> -
>> - hi = gpu_read(ptdev, GPU_TIMESTAMP_OFFSET_HI);
>> - lo = gpu_read(ptdev, GPU_TIMESTAMP_OFFSET_LO);
>> -
>> - return ((u64)hi << 32) | lo;
>> + return gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET_LO);
>> }
>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
>> index 12a02e28f50f..a0a79f19bdea 100644
>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
>> @@ -510,9 +510,9 @@ static int wait_ready(struct panthor_device *ptdev, u32 as_nr)
>> /* Wait for the MMU status to indicate there is no active command, in
>> * case one is pending.
>> */
>> - ret = readl_relaxed_poll_timeout_atomic(ptdev->iomem + AS_STATUS(as_nr),
>> - val, !(val & AS_STATUS_AS_ACTIVE),
>> - 10, 100000);
>> + ret = gpu_read_relaxed_poll_timeout_atomic(ptdev, AS_STATUS(as_nr), val,
>> + !(val & AS_STATUS_AS_ACTIVE),
>> + 10, 100000);
>>
>> if (ret) {
>> panthor_device_schedule_reset(ptdev);
>> @@ -564,8 +564,7 @@ static void lock_region(struct panthor_device *ptdev, u32 as_nr,
>> region = region_width | region_start;
>>
>> /* Lock the region that needs to be updated */
>> - gpu_write(ptdev, AS_LOCKADDR_LO(as_nr), lower_32_bits(region));
>> - gpu_write(ptdev, AS_LOCKADDR_HI(as_nr), upper_32_bits(region));
>> + gpu_write64(ptdev, AS_LOCKADDR_LO(as_nr), region);
>> write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
>> }
>>
>> @@ -615,14 +614,9 @@ static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
>> if (ret)
>> return ret;
>>
>> - gpu_write(ptdev, AS_TRANSTAB_LO(as_nr), lower_32_bits(transtab));
>> - gpu_write(ptdev, AS_TRANSTAB_HI(as_nr), upper_32_bits(transtab));
>> -
>> - gpu_write(ptdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
>> - gpu_write(ptdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
>> -
>> - gpu_write(ptdev, AS_TRANSCFG_LO(as_nr), lower_32_bits(transcfg));
>> - gpu_write(ptdev, AS_TRANSCFG_HI(as_nr), upper_32_bits(transcfg));
>> + gpu_write64(ptdev, AS_TRANSTAB_LO(as_nr), transtab);
>> + gpu_write64(ptdev, AS_MEMATTR_LO(as_nr), memattr);
>> + gpu_write64(ptdev, AS_TRANSCFG_LO(as_nr), transcfg);
>>
>> return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
>> }
>> @@ -635,14 +629,9 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
>> if (ret)
>> return ret;
>>
>> - gpu_write(ptdev, AS_TRANSTAB_LO(as_nr), 0);
>> - gpu_write(ptdev, AS_TRANSTAB_HI(as_nr), 0);
>> -
>> - gpu_write(ptdev, AS_MEMATTR_LO(as_nr), 0);
>> - gpu_write(ptdev, AS_MEMATTR_HI(as_nr), 0);
>> -
>> - gpu_write(ptdev, AS_TRANSCFG_LO(as_nr), AS_TRANSCFG_ADRMODE_UNMAPPED);
>> - gpu_write(ptdev, AS_TRANSCFG_HI(as_nr), 0);
>> + gpu_write64(ptdev, AS_TRANSTAB_LO(as_nr), 0);
>> + gpu_write64(ptdev, AS_MEMATTR_LO(as_nr), 0);
>> + gpu_write64(ptdev, AS_TRANSCFG_LO(as_nr), AS_TRANSCFG_ADRMODE_UNMAPPED);
>>
>> return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
>> }
>> @@ -1680,8 +1669,7 @@ static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
>> u32 source_id;
>>
>> fault_status = gpu_read(ptdev, AS_FAULTSTATUS(as));
>> - addr = gpu_read(ptdev, AS_FAULTADDRESS_LO(as));
>> - addr |= (u64)gpu_read(ptdev, AS_FAULTADDRESS_HI(as)) << 32;
>> + addr = gpu_read64(ptdev, AS_FAULTADDRESS_LO(as));
>>
>> /* decode the fault status */
>> exception_type = fault_status & 0xFF;
>