From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1825F35DA60 for ; Fri, 24 Apr 2026 11:03:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777028593; cv=none; b=V+Vkas7tXx/FRRaQYSxHnm4/GIkZ/gaX6aPka+RToyvct+CZAlFnESjKccXomL2NzsXUYo6og8GM6XD92P0LyS0heLtWf2hAmr8y4dZS+gVWUGoJlov16Y9jFHmByGK4LKKIYm/x8tLgZ9Ep1pb797NDYLf3a4uBXtzsJkbLuxU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777028593; c=relaxed/simple; bh=hVddO3hR5x+R+0P8QmpXd3uom0HB9YCG+B6geFvYRGo=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=WCV4WuLB805motjeKig22EYQ1trSvEI5wuX5j35FrlJk3KMXTYzzlb628Wp7BHlOKvUtN8Mh2xvwrwG1ZuAcwTkRqWsESF0BWr4e3raHZUYbrTpDCijjUeBMNqtaIRCcUV3qO9C9woZ01Rwupa7h9NtTnZj5vnbYBSolbY8Gtqc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=X0fLiEax; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="X0fLiEax" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1777028589; bh=hVddO3hR5x+R+0P8QmpXd3uom0HB9YCG+B6geFvYRGo=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=X0fLiEax058jfyunrk9frdSlcqL0mA43dTcGJRFqHikvCozwEEYEftVsRK4gBH97m uFWPiFKP5WiusxJlIoUIFlEvtJoVI8q2gDoxqs3xSxV7zhXabShE+go3lxjGf1/ZDl HRdL5W+g0TFn6uGonJHnlHCjDVlpe3lhr6kx9ejwySnxdiqyyELmtbsDsNZkaIwr23 RMQuuDFxMpvgah0hbKRJtSVUfRsEQfC8KFMNUFW4v7gKHXoeLEe7LUfdAFI6/0fY77 wTkoWPIq2ky0VLU+L+M+twZ9Sjg4870TV0Xf0Zb0AtTcDGB2gpYp3Co1NB+056zQPH JI9l7DdIgCH7g== Received: from fedora (unknown [100.64.0.11]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id 806B717E1227; Fri, 24 Apr 2026 13:03:09 +0200 (CEST) Date: Fri, 24 Apr 2026 13:03:02 +0200 From: Boris Brezillon To: Steven Price Cc: Karunika Choo , dri-devel@lists.freedesktop.org, nd@arm.com, Liviu Dudau , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 4/8] drm/panthor: Store IRQ register base iomem pointer in panthor_irq Message-ID: <20260424130302.4fb86551@fedora> In-Reply-To: <05bc8e72-a914-4bee-b635-fe1e9c623900@arm.com> References: <20260412142951.2309135-1-karunika.choo@arm.com> <20260412142951.2309135-5-karunika.choo@arm.com> <53a66bbd-7ed5-41d9-a15e-af17f292dea1@arm.com> <05bc8e72-a914-4bee-b635-fe1e9c623900@arm.com> Organization: Collabora X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Fri, 24 Apr 2026 11:38:24 +0100 Steven Price wrote: > On 22/04/2026 17:08, Karunika Choo wrote: > > On 22/04/2026 10:34, Steven Price wrote: > >> On 12/04/2026 15:29, Karunika Choo wrote: > >>> Update common IRQ handling code to work from an IRQ-local iomem base > >>> instead of referencing block-specific interrupt register offsets. > >>> > >>> Store the interrupt base address iomem pointer in struct panthor_irq and > >>> switch the shared IRQ helpers to use generic INT_* offsets from that > >>> local base. This removes the need for each caller to expose absolute IRQ > >>> register addresses while keeping the common IRQ flow unchanged. > >>> > >>> No functional change intended. > >>> > >>> v2: > >>> - Change IRQ request function to accept an iomem pointer instead of > >>> computing it from an offset argument. > >>> > >>> Signed-off-by: Karunika Choo > >> > >> One minor comment below... > >> > >>> --- > >>> drivers/gpu/drm/panthor/panthor_device.h | 32 ++++++++++++++-------- > >>> drivers/gpu/drm/panthor/panthor_fw.c | 5 ++-- > >>> drivers/gpu/drm/panthor/panthor_fw_regs.h | 2 ++ > >>> drivers/gpu/drm/panthor/panthor_gpu.c | 6 ++-- > >>> drivers/gpu/drm/panthor/panthor_gpu_regs.h | 3 ++ > >>> drivers/gpu/drm/panthor/panthor_mmu.c | 5 ++-- > >>> drivers/gpu/drm/panthor/panthor_mmu_regs.h | 3 ++ > >>> drivers/gpu/drm/panthor/panthor_pwr.c | 6 ++-- > >>> 8 files changed, 42 insertions(+), 20 deletions(-) > >>> > >> [...] > >>> @@ -1470,7 +1470,8 @@ int panthor_fw_init(struct panthor_device *ptdev) > >>> if (irq <= 0) > >>> return -ENODEV; > >>> > >>> - ret = panthor_request_job_irq(ptdev, &fw->irq, irq, 0); > >>> + ret = panthor_request_job_irq(ptdev, &fw->irq, irq, 0, > >>> + ptdev->iomem + JOB_INT_BASE); > >>> if (ret) { > >>> drm_err(&ptdev->base, "failed to request job irq"); > >>> return ret; > >> [..] > >>> @@ -162,7 +162,9 @@ int panthor_gpu_init(struct panthor_device *ptdev) > >>> if (irq < 0) > >>> return irq; > >>> > >>> - ret = panthor_request_gpu_irq(ptdev, &ptdev->gpu->irq, irq, GPU_INTERRUPTS_MASK); > >>> + ret = panthor_request_gpu_irq(ptdev, &ptdev->gpu->irq, irq, > >>> + GPU_INTERRUPTS_MASK, > >>> + ptdev->iomem + GPU_INT_BASE); > >>> if (ret) > >>> return ret; > >>> > >> [...] > >>> @@ -3229,7 +3229,8 @@ int panthor_mmu_init(struct panthor_device *ptdev) > >>> return -ENODEV; > >>> > >>> ret = panthor_request_mmu_irq(ptdev, &mmu->irq, irq, > >>> - panthor_mmu_fault_mask(ptdev, ~0)); > >>> + panthor_mmu_fault_mask(ptdev, ~0), > >>> + ptdev->iomem + MMU_INT_BASE); > >>> if (ret) > >>> return ret; > >>> > >> [...] > >>> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c > >>> index aafb0c5c7d23..11c43de1ddd5 100644 > >>> --- a/drivers/gpu/drm/panthor/panthor_pwr.c > >>> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c > >>> @@ -70,7 +70,7 @@ static void panthor_pwr_irq_handler(struct panthor_device *ptdev, u32 status) > >>> } > >>> spin_unlock(&ptdev->pwr->reqs_lock); > >>> } > >>> -PANTHOR_IRQ_HANDLER(pwr, PWR, panthor_pwr_irq_handler); > >>> +PANTHOR_IRQ_HANDLER(pwr, panthor_pwr_irq_handler); > >>> > >>> static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command, u64 args) > >>> { > >>> @@ -464,7 +464,9 @@ int panthor_pwr_init(struct panthor_device *ptdev) > >>> if (irq < 0) > >>> return irq; > >>> > >>> - err = panthor_request_pwr_irq(ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK); > >>> + err = panthor_request_pwr_irq( > >>> + ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK, > >>> + ptdev->iomem + GPU_CONTROL_BASE + PWR_CONTROL_BASE); > >> > >> This one is the odd one out because it adds GPU_CONTROL_BASE put the > >> other panthor_request_xxx_irq() calls don't. Sashiko also points out > >> that there's an argument these should all be using ptdev->gpu->iomem in > >> the final refactor. > >> > > > > I understand that it is slightly different, it is only there to > > illustrate the fact that it is a child of the GPU_CONTROL register page. > > w.r.t using ptdev->gpu->iomem, that would mean that we need reach into > > the panthor_gpu component to access the memory, hence why we used a > > separate iomem for this one. > > Why do you not add GPU_CONTROL_BASE on when accessing JOB_INT_BASE, > GPU_INT_BASE and MMU_INT_BASE though? > > The obvious answer for JOB/MMU is because they're not architecturally in > GPU_CONTROL. But then why don't we have > JOB_CONTROL_BASE/MMU_CONTROL_BASE (my guess is because there isn't > really anything other than INT registers in those blocks). > > So it looks like for JOB_INT_BASE/GPU_INT_BASE/MMU_INT_BASE these are > offsets from the beginning of the GPU registers iomem. But then for some > reason PWR_INT_BASE (introduced in patch 6) is treated as if it's a > relative offset within GPU_CONTROL_BASE. Which is just weirdly inconsistent. > > I'm happy to disagree with Sashiko on the use of ptdev->gpu->iomem - I > think interrupts are special enough that we can use the top-level iomem. > But that only holds as a justification if all interrupts are treated in > the same way. I don't really mind if some intermediate patches use ugly tricks to get there, but in the final state, I'd actually prefer if each block was initializing its own iomem, and then the interrupt iomem was calculated off this block-iomem region. So, in patch, I'd like to see PWR_INT_BASE changed from 0x800 to 0 and panthor_request_pwr_irq(ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK, ptdev->pwr->iomem + PWR_INT_BASE);