On 12/11/2018 10:33, 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 and
> other qcom SoCs have this implementation.
s/qcom/Qualcomm/
>
> 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 and other
> SoCs with have this restrictions. This Quirk is based on device tree
> compatible string.
>
> Without this patch Qualcomm DB820c board reboots when GICR_WAKER
> is accessed.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
> drivers/irqchip/irq-gic-v3.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 8f87f40c9460..4bd3bbe1b7ce 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);
> @@ -1067,6 +1073,23 @@ static const struct irq_domain_ops partition_domain_ops = {
> .select = gic_irq_domain_select,
> };
>
> +static bool __maybe_unused gic_enable_quirk_msm8996(void *data)
Why __maybe_unused? It is referenced in the quirk table, right?
> +{
> + 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",
This should read "GICv3: Qualcomm MSM8996 broken firmware". Nobody knows
what GICR_WAKER is, but people do understand that they run buggy software.
> + .compatible = "qcom,msm8996-gic-v3", /* MSM8996 */
Drop this comment, it doesn't add anything.
> + .init = gic_enable_quirk_msm8996,
> + },
> +};
> +
> static int __init gic_init_bases(void __iomem *dist_base,
> struct redist_region *rdist_regs,
> u32 nr_redist_regions,
> @@ -1126,6 +1149,9 @@ static int __init gic_init_bases(void __iomem *dist_base,
>
> gic_update_vlpi_properties();
>
> + if (is_of_node(handle))
> + gic_enable_of_quirks(to_of_node(handle), gic_quirks, &gic_data);
> +
Please move this as early as possible. Actually, given that this is DT
only, it should be done in gic_of_init. It should also be moved to the
previous patch, with gic_quirks being an empty array.
> gic_smp_init();
> gic_dist_init();
> gic_cpu_init();
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
