Messages in this thread | | | Subject | Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 | From | Mikko Perttunen <> | Date | Fri, 7 Apr 2017 09:49:49 +0300 |
| |
On 06.04.2017 12:26, Marc Zyngier wrote: > On 06/04/17 09:17, Mikko Perttunen wrote: >> From: Matt Craighead <mcraighead@nvidia.com> >> >> 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 <mcraighead@nvidia.com> >> [mperttunen@nvidia.com: commit message rewritten] >> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> >> --- >> 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.
> > Thanks, > > M. >
Thanks for reviewing!
Mikko
| |