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 ABD684594A for ; Fri, 24 Apr 2026 12:09:09 +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=1777032551; cv=none; b=P5mMO5OzSDnRoardZotu/2dRFl424UD1XGpUJWWYszU2pxcMcIZzw6/MO4Ju5vBjCEcqLqcTJQ5baN4E7zdd5eG+EeKdTBkjgnqdAWgowR61kHMAh1/7evQ0us0S/Z3L7dOs9yHhQcUhc0dPM7PNwz7XLmHuP937Ju/FK+rZ4UE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777032551; c=relaxed/simple; bh=bAtkoYstXQt4BRxKwP2U4Ooxgnt99sDfcSdFJjFTetc=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=lGTD2cbe3XZakGaxcVNxeSAx1OdeLbzDONuZCFsxUHYVKRbInGWsaeC9MivqQdwN5QpZdPM7TR13jpyQtRBVGAjjB5mAl9c4WYoOjVt0yWa1manZZKYQr9r8bfhR0BVihD9C88Mkairq2WvN+o9JI+gl7QhIXyf6DA4u96dbWGw= 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=bHcgaOOS; 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="bHcgaOOS" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1777032547; bh=bAtkoYstXQt4BRxKwP2U4Ooxgnt99sDfcSdFJjFTetc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=bHcgaOOS/mAFd771CKIfLiEoLJUoG3lZQyF2Ond/qEmqpOl+ArIYpcyRaOfe+yOc6 3CdEB0PiVkjoHwFkIGLX8CINiADO5htPlNUbdwWX+HDraP+5XfGewahxaHoBXmhmxa gOqXOeg93EXaRn9OjwafNPPjmjJ+rHpmvTkVINEPmoXItfS2YplU61kUw/2AQeTV7i 4da7da3nhbahX7BiutQUsCGge3pDfj2RAzJOFaQM1p8J/9wUZd64deDe74ZOvo/O7g RGaAlP+XrI0W/5afRO7+I385IcBrwk9Py3jpW5hJMHuWddTE007x130rNzCuSarBmY oOYElh2KMkpuA== 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 680F917E14D4; Fri, 24 Apr 2026 14:09:07 +0200 (CEST) Date: Fri, 24 Apr 2026 14:09:03 +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: <20260424140903.26b03ea2@fedora> In-Reply-To: <5087bbe4-4898-4fe1-9ea2-17e967bfeec0@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> <20260424130302.4fb86551@fedora> <5087bbe4-4898-4fe1-9ea2-17e967bfeec0@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 12:20:07 +0100 Steven Price wrote: > On 24/04/2026 12:03, Boris Brezillon wrote: > > 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, > > Sorry, I should have been clearer on that - the intermediate patches can > be a big ugly if necessary (it's clearly better to split things up > rather than change everything in one go). I picked this patch to comment > on because it's the one changing the IRQ handling. > > > 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); > > Yes, using the relevant iomem pointer for each IRQ request also makes > sense. What I disagreed with Sashiko on was picking on just the GPU > instance ;) No worries. That was more a comment addressed to Karunika actually, since I see: err = panthor_request_pwr_irq( ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK, ptdev->iomem + GPU_CONTROL_BASE + PWR_INT_BASE); in patch 6, which is not what I'd expect there.