Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0

[Mikko Perttunen]

On 06.04.2017 12:26, Marc Zyngier wrote:
> On 06/04/17 09:17, Mikko Perttunen wrote:
> > From: Matt Craighead <mcraighead@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > [mperttunen@xxxxxxxxxx: commit message rewritten]
> > Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > ---
> >  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