* [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 @ 2017-04-06 8:17 Mikko Perttunen 2017-04-06 9:26 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Mikko Perttunen @ 2017-04-06 8:17 UTC (permalink / raw) To: tglx, jason, marc.zyngier Cc: linux-kernel, talho, aniruddhab, Matt Craighead, Mikko Perttunen 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. 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); for (i = 0; i < DIV_ROUND_UP(32, 4); i++) -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 2017-04-06 8:17 [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 Mikko Perttunen @ 2017-04-06 9:26 ` Marc Zyngier 2017-04-07 6:49 ` Mikko Perttunen 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2017-04-06 9:26 UTC (permalink / raw) To: Mikko Perttunen, tglx, jason Cc: linux-kernel, talho, aniruddhab, Matt Craighead 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? > > 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? 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. 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? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 2017-04-06 9:26 ` Marc Zyngier @ 2017-04-07 6:49 ` Mikko Perttunen 2017-04-07 7:32 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Mikko Perttunen @ 2017-04-07 6:49 UTC (permalink / raw) To: Marc Zyngier, tglx, jason; +Cc: linux-kernel, talho, aniruddhab, Matt Craighead 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 2017-04-07 6:49 ` Mikko Perttunen @ 2017-04-07 7:32 ` Marc Zyngier 2017-04-10 10:32 ` Mikko Perttunen 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2017-04-07 7:32 UTC (permalink / raw) To: Mikko Perttunen, tglx, jason Cc: linux-kernel, talho, aniruddhab, Matt Craighead 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 <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. 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? M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 2017-04-07 7:32 ` Marc Zyngier @ 2017-04-10 10:32 ` Mikko Perttunen 0 siblings, 0 replies; 5+ messages in thread From: Mikko Perttunen @ 2017-04-10 10:32 UTC (permalink / raw) To: Marc Zyngier, tglx, jason; +Cc: linux-kernel, talho, aniruddhab, Matt Craighead 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 <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. > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-10 10:43 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-06 8:17 [PATCH] irqchip/gic: Don't write to GICD_ICFGR0 Mikko Perttunen 2017-04-06 9:26 ` Marc Zyngier 2017-04-07 6:49 ` Mikko Perttunen 2017-04-07 7:32 ` Marc Zyngier 2017-04-10 10:32 ` Mikko Perttunen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.