From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753173AbdDJKnU (ORCPT ); Mon, 10 Apr 2017 06:43:20 -0400 Received: from hqemgate15.nvidia.com ([216.228.121.64]:12063 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752856AbdDJKnT (ORCPT ); Mon, 10 Apr 2017 06:43:19 -0400 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Mon, 10 Apr 2017 03:43:18 -0700 Subject: Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 To: Marc Zyngier , , References: <1491466631-11206-1-git-send-email-mperttunen@nvidia.com> <3c8d9248-39c2-9b70-7a41-0b51e6ba5a3c@arm.com> <34a426a8-d077-b52f-9c2e-60ac444f4d66@arm.com> CC: , , , Matt Craighead From: Mikko Perttunen Message-ID: <853c58ba-d733-3a70-39f6-d9d62035677f@nvidia.com> Date: Mon, 10 Apr 2017 13:32:08 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <34a426a8-d077-b52f-9c2e-60ac444f4d66@arm.com> X-Originating-IP: [10.21.26.175] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL102.nvidia.com (10.26.138.15) Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07.04.2017 10:32, Marc Zyngier wrote: > On 07/04/17 07:49, Mikko Perttunen wrote: >> On 06.04.2017 12:26, Marc Zyngier wrote: >>> On 06/04/17 09:17, Mikko Perttunen wrote: >>>> From: Matt Craighead >>>> >>>> According to the GICv2 specification, the GICD_ICFGR0, >>>> or GIC_DIST_CONFIG[0] register is read-only. Therefore >>>> avoid writing to it. >>> >>> Have you verified that this also applies to pre-v2 GICs? >> >> I had not, but I just looked up the GICv1 specification and this also >> applies to GICv1. >> >>> >>>> >>>> Signed-off-by: Matt Craighead >>>> [mperttunen@nvidia.com: commit message rewritten] >>>> Signed-off-by: Mikko Perttunen >>>> --- >>>> drivers/irqchip/irq-gic.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index 1b1df4f770bd..d9c0000050e0 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -609,7 +609,7 @@ void gic_dist_restore(struct gic_chip_data *gic) >>>> >>>> writel_relaxed(GICD_DISABLE, dist_base + GIC_DIST_CTRL); >>>> >>>> - for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++) >>>> + for (i = 1; i < DIV_ROUND_UP(gic_irqs, 16); i++) >>>> writel_relaxed(gic->saved_spi_conf[i], >>>> dist_base + GIC_DIST_CONFIG + i * 4); >>>> >>>> @@ -699,7 +699,7 @@ void gic_cpu_restore(struct gic_chip_data *gic) >>>> } >>>> >>>> ptr = raw_cpu_ptr(gic->saved_ppi_conf); >>>> - for (i = 0; i < DIV_ROUND_UP(32, 16); i++) >>>> + for (i = 1; i < DIV_ROUND_UP(32, 16); i++) >>>> writel_relaxed(ptr[i], dist_base + GIC_DIST_CONFIG + i * 4); >>> >>> Assuming that the above stands for all GICs, it feels like there is room >>> for simplification here. But you haven't dealt with the save side, so >>> what's the point? >>> >> >> Yes, with this we could also drop saving the value when saving, and >> that's probably worth doing. We could also just shift the indexing to be >> one higher always. >> >>> Also, you're missing out some other stuff which is (by definition) RO as >>> well, such as the target registers for SGIs and PPIs. Finally, there is >>> the question of the allocated memory for these registers. >> >> At least for the target register, the driver already seems to have code >> to skip the fields defined as read-only. I havent looked for other >> read-only registers, but this is the only registers we are having issues >> with (see below). >> >>> >>> Overall, I'm not sure what this patch is trying to achieve. It doesn't >>> fix a bug, and is not complete enough to do something useful (even >>> though it would only be saving a handful of bytes). >>> >>> Maybe you can explain what you're trying to do here? >> >> Sure. Our simulation environment enforces the read-only-ness of these >> registers, so the driver as is doesn't work in simulation. As far as I >> understand, the register being read-only means that the model is allowed >> to do this. > > I'm not sure this is a valid model for a GICv2. Some other parts of the > documentation hint at registers being RO/WI, but more crucially, there > is the case of GICD_ICFGR1. It is implementation defined whether it is > RO or not, and SW has no way to find out other than writing to it. What > would you do in this case? My position is that GICD_GICR0 should have a > similar behaviour. > > Thoughts? We could add some device data that specifies this, but that that would be serious overkill just to support a simulation implementation. I do think we could still make the change for ICFGR0, as that is guaranteed to be read-only and the patch is very small and correct regardless of the interpretation of 'read-only'. However, I do see your point of keeping it uniform. Of course, it's your decision :) > > M. > Thanks, Mikko