Messages in this thread | | | Subject | Re: [RFC PATCH v2] irqchip/gic-v3: Add quirk for msm8996 secured registers | From | Marc Zyngier <> | Date | Tue, 11 Sep 2018 15:14:20 +0100 |
| |
On 04/09/18 15:05, Srinivas Kandagatla wrote: > Access to GICR_WAKER is restricted on msm8996 SoC in Hypervisor. > Its been more than 2 years of wait for this to be fixed, which has > no hopes to be fixed. This change was introduced for the "lead device" > on msm8996 platform. It looks like all publicly available msm8996 > devices have this implementation. > > So add a quirk to not access this register on msm8996. > > With this quirk MSM8996 can at least boot out of mainline, > which can help community to work with boards based on MSM8996. > > Without this patch Qualcomm DB820c board reboots when GICR_WAKER > is accessed. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > Hi Marc, > > There is no errata associated with this quirk, so I could not add > any entries into Documentation/arm64/silicon-errata.txt Or add > any number to the quirk description.
Well, we still need something. Specially given that this seems to affect more than just the piece of HW you mention (see Craig's follow-up). This is quite important, given that the IIDR stuff doesn't seem to work at all (see below).
> > Changes since v1: > - renamed gic_v3 references to gic > - added full mask for iidr register match > > thanks, > srini > > drivers/irqchip/irq-gic-v3.c | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index d5912f1ec884..406d4a44c887 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -41,6 +41,8 @@ > > #include "irq-gic-common.h" > > +#define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0) > + > struct redist_region { > void __iomem *redist_base; > phys_addr_t phys_base; > @@ -55,6 +57,7 @@ struct gic_chip_data { > struct irq_domain *domain; > u64 redist_stride; > u32 nr_redist_regions; > + u64 flags; > bool has_rss; > unsigned int irq_nr; > struct partition_desc *ppi_descs[16]; > @@ -139,6 +142,9 @@ static void gic_enable_redist(bool enable) > u32 count = 1000000; /* 1s! */ > u32 val; > > + if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996) > + return; > + > rbase = gic_data_rdist_rd_base(); > > val = readl_relaxed(rbase + GICR_WAKER); > @@ -1068,13 +1074,31 @@ static const struct irq_domain_ops partition_domain_ops = { > .select = gic_irq_domain_select, > }; > > +static bool __maybe_unused gic_enable_quirk_msm8996(void *data) > +{ > + struct gic_chip_data *d = data; > + > + d->flags |= FLAGS_WORKAROUND_GICR_WAKER_MSM8996; > + > + return true; > +} > + > +static const struct gic_quirk gic_quirks[] = { > + { > + .desc = "GICv3: Qualcomm MSM8996 skip GICR_WAKER Read/Write", > + .iidr = 0x00001070, /* MSM8996 */
I just realized that this is the exact same ID as QDF2400, which is a perfectly functional platform. So on top of being fairly broken, the firmware also hijacks the ID of another implementation. I don't know if I should laugh or cry. Needless to say, I'm not going to willingly break a perfectly working platform.
> + .mask = 0xffffffff, > + .init = gic_enable_quirk_msm8996, > + }, > +}; > + > static int __init gic_init_bases(void __iomem *dist_base, > struct redist_region *rdist_regs, > u32 nr_redist_regions, > u64 redist_stride, > struct fwnode_handle *handle) > { > - u32 typer; > + u32 typer, iidr; > int gic_irqs; > int err; > > @@ -1130,6 +1154,9 @@ static int __init gic_init_bases(void __iomem *dist_base, > if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis()) > its_init(handle, &gic_data.rdists, gic_data.domain); > > + iidr = readl_relaxed(dist_base + GICD_IIDR); > + gic_enable_quirks(iidr, gic_quirks, &gic_data); > + > gic_smp_init(); > gic_dist_init(); > gic_cpu_init(); >
Given the above, I'm not taking this patch any time soon. Please consider alternative methods such as a different (non-)compatible string to flag affected implementations.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |