| Message ID | 20180612145516.30897-1-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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 >
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
On Wed, Jun 13, 2018 at 9:21 AM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > 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. > From what you are saying, you were aware of the firmware bug for 2 years as you had work-around from then. I wish the firmware had come up with the fix in those lost 2 years. I understand if it was discovered now, but that doesn't seem to be the case here. Firmware fixes are now becoming inevitable, so the attitude that firmware can't be fixed has to slowing change. I am not saying I am against the quirk, but in general I have seen the firmware support of QCOM parts are really bad. I waited for almost 2 years for PSCI firmware on previous dragonboard where developer are trying to upstream some feature. -- 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 13/06/18 09:45, Sudeep Holla wrote: > On Wed, Jun 13, 2018 at 9:21 AM, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: >> >> >> 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. >> > > From what you are saying, you were aware of the firmware bug for 2 years as > you had work-around from then. I wish the firmware had come up with the fix > in those lost 2 years. I understand if it was discovered now, but that doesn't > seem to be the case here. > > Firmware fixes are now becoming inevitable, so the attitude that firmware can't > be fixed has to slowing change. I am not saying I am against the quirk, but in > general I have seen the firmware support of QCOM parts are really bad. I waited > for almost 2 years for PSCI firmware on previous dragonboard where developer > are trying to upstream some feature. I agree with you! We did wait for more than 2 years for firmware to be fixed. But it never happened and very little hopes to get it fixed. I did try the quirk based on GICD_IIDR and I could boot linus master on DB820c! I will send it out as RFC soon. thanks, srini > > -- > 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
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(-)