Re: [RFC PATCH v3 3/4] irqchip: gic-v3: Add quirk for msm8996 secured registers

[Marc Zyngier <marc.zyngier@arm.com>]

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@linaro.org>
> ---
>  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...