| Message ID | 20180612145516.30897-1-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | irqchip/gic-v3: do not access GICR_WAKER if its secured register. | expand |
On Tue, Jun 12, 2018 at 3:55 PM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > GICR_WAKER can be a secured register, check this before accessing it > as its done in power management code. > > Without this patch Qualcomm DB820c board crashes. > Are you sure this is the one causing the crash ? As per GIC specification: "When GICD_CTLR.DS==1, this register is always accessible. When GICD_CTLR.DS==0, this is a Secure register. This register is RAZ/WI to Non-secure accesses." -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/18 16:24, Sudeep Holla wrote: > On Tue, Jun 12, 2018 at 3:55 PM, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: >> GICR_WAKER can be a secured register, check this before accessing it >> as its done in power management code. >> >> Without this patch Qualcomm DB820c board crashes. >> > > Are you sure this is the one causing the crash ? > Yep, am 100% sure its the write in gic_enable_redist(). Very first zero write to GICR_WAKER is the one which is crashing my system. If I add return before writing then I can boot my system fine. Not sure why writes are not ignored? Also why does other parts of the code have checks while accessing this register? thanks, srini > As per GIC specification: > "When GICD_CTLR.DS==1, this register is always accessible. > When GICD_CTLR.DS==0, this is a Secure register. This register is RAZ/WI > to Non-secure accesses." > > > -- > Regards, > Sudeep > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018 15:55:16 +0100, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > GICR_WAKER can be a secured register, check this before accessing it > as its done in power management code. NAK. From the GICv3 spec: * When GICD_CTLR.DS==1, this register is always accessible. * When GICD_CTLR.DS==0, this is a Secure register. This register is RAZ/WI to Non-secure accesses. > Without this patch Qualcomm DB820c board crashes. I suggest you find out how the GIC has been integrated on this platform. If you take a fault on accessing this register, this very much looks like an integration bug, and it should be quirked as such. Thanks, M. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/irqchip/irq-gic-v3.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 5a67ec084588..38136d6e9ca5 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -656,6 +656,12 @@ static int gic_dist_supports_lpis(void) > return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) && !gicv3_nolpi; > } > > +/* Check whether it's single security state view */ > +static bool gic_dist_security_disabled(void) > +{ > + return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS; > +} > + > static void gic_cpu_init(void) > { > void __iomem *rbase; > @@ -664,7 +670,8 @@ static void gic_cpu_init(void) > if (gic_populate_rdist()) > return; > > - gic_enable_redist(true); > + if (gic_dist_security_disabled()) > + gic_enable_redist(true); > > rbase = gic_data_rdist_sgi_base(); > > @@ -819,11 +826,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > #endif > > #ifdef CONFIG_CPU_PM > -/* Check whether it's single security state view */ > -static bool gic_dist_security_disabled(void) > -{ > - return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS; > -} > > static int gic_cpu_pm_notifier(struct notifier_block *self, > unsigned long cmd, void *v) > -- > 2.16.2 > -- Jazz is not dead, it just smell funny. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/18 17:34, Marc Zyngier wrote: > I suggest you find out how the GIC has been integrated on this > platform. If you take a fault on accessing this register, this very > much looks like an integration bug, and it should be quirked as such. Thanks for the suggestion, This is a bug in the firmware which is restricting access to this register. We have been working around this bug for more than 2 years due to this. Now this platform support is in mainline and We/I have no hope that this will be fixed in near future atleast for this platform. I will try to come up with a Quirk specific for this SoC. thanks, srini -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 5a67ec084588..38136d6e9ca5 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -656,6 +656,12 @@ static int gic_dist_supports_lpis(void) return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) && !gicv3_nolpi; } +/* Check whether it's single security state view */ +static bool gic_dist_security_disabled(void) +{ + return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS; +} + static void gic_cpu_init(void) { void __iomem *rbase; @@ -664,7 +670,8 @@ static void gic_cpu_init(void) if (gic_populate_rdist()) return; - gic_enable_redist(true); + if (gic_dist_security_disabled()) + gic_enable_redist(true); rbase = gic_data_rdist_sgi_base(); @@ -819,11 +826,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, #endif #ifdef CONFIG_CPU_PM -/* Check whether it's single security state view */ -static bool gic_dist_security_disabled(void) -{ - return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS; -} static int gic_cpu_pm_notifier(struct notifier_block *self, unsigned long cmd, void *v)
GICR_WAKER can be a secured register, check this before accessing it as its done in power management code. Without this patch Qualcomm DB820c board crashes. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/irqchip/irq-gic-v3.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) -- 2.16.2 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html