Re: [PATCH 6/8] irqchip/gic-v3-its: add ability to resend MAPC on resume

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

On Fri, 12 Jan 2018 21:24:20 +0000,
Derek Basehore wrote:
> 
> This adds a DT-binding to resend the MAPC command to an ITS node on

This isn't a DT binding. That's the driver implementation. The binding
is what you put in Documentation/device-tree...

> resume. If the ITS is powered down during suspend and the collections
> are not backed by memory, the ITS will lose that state. This just sets
> up the known state for the collections after the ITS is restored.
> 
> Change-Id: I4fe7f3f16500e1d3b14368262f03a43509c0049b
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 85 ++++++++++++++++++++++------------------
>  1 file changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 5cb808e3d0bf..6d2688a2ee67 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -46,6 +46,7 @@
>  #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
> +#define ITS_FLAGS_RESEND_MAPC_ON_RESUME		(1ULL << 3)
>  
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>  
> @@ -1949,52 +1950,53 @@ static void its_cpu_init_lpis(void)
>  	dsb(sy);
>  }
>  
> -static void its_cpu_init_collection(void)
> +static void its_cpu_init_collection(struct its_node *its)
>  {
> -	struct its_node *its;
> -	int cpu;
> -
> -	spin_lock(&its_lock);
> -	cpu = smp_processor_id();
> -
> -	list_for_each_entry(its, &its_nodes, entry) {
> -		u64 target;
> +	int cpu = smp_processor_id();
> +	u64 target;
>  
> -		/* avoid cross node collections and its mapping */
> -		if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
> -			struct device_node *cpu_node;
> +	/* avoid cross node collections and its mapping */
> +	if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
> +		struct device_node *cpu_node;
>  
> -			cpu_node = of_get_cpu_node(cpu, NULL);
> -			if (its->numa_node != NUMA_NO_NODE &&
> -				its->numa_node != of_node_to_nid(cpu_node))
> -				continue;
> -		}
> +		cpu_node = of_get_cpu_node(cpu, NULL);
> +		if (its->numa_node != NUMA_NO_NODE &&
> +			its->numa_node != of_node_to_nid(cpu_node))
> +			return;
> +	}
>  
> +	/*
> +	 * We now have to bind each collection to its target
> +	 * redistributor.
> +	 */
> +	if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
>  		/*
> -		 * We now have to bind each collection to its target
> +		 * This ITS wants the physical address of the
>  		 * redistributor.
>  		 */
> -		if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
> -			/*
> -			 * This ITS wants the physical address of the
> -			 * redistributor.
> -			 */
> -			target = gic_data_rdist()->phys_base;
> -		} else {
> -			/*
> -			 * This ITS wants a linear CPU number.
> -			 */
> -			target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
> -			target = GICR_TYPER_CPU_NUMBER(target) << 16;
> -		}
> +		target = gic_data_rdist()->phys_base;
> +	} else {
> +		/* This ITS wants a linear CPU number. */
> +		target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
> +		target = GICR_TYPER_CPU_NUMBER(target) << 16;
> +	}
>  
> -		/* Perform collection mapping */
> -		its->collections[cpu].target_address = target;
> -		its->collections[cpu].col_id = cpu;
> +	/* Perform collection mapping */
> +	its->collections[cpu].target_address = target;
> +	its->collections[cpu].col_id = cpu;
>  
> -		its_send_mapc(its, &its->collections[cpu], 1);
> -		its_send_invall(its, &its->collections[cpu]);
> -	}
> +	its_send_mapc(its, &its->collections[cpu], 1);
> +	its_send_invall(its, &its->collections[cpu]);
> +}
> +
> +static void its_cpu_init_collections(void)
> +{
> +	struct its_node *its;
> +
> +	spin_lock(&its_lock);
> +
> +	list_for_each_entry(its, &its_nodes, entry)
> +		its_cpu_init_collection(its);
>  
>  	spin_unlock(&its_lock);
>  }
> @@ -3089,6 +3091,9 @@ void its_restore_enable(void)
>  			its_write_baser(its, &its->tables[i],
>  					its->tables[i].val);
>  		writel_relaxed(ctx->ctlr, base + GITS_CTLR);
> +
> +		if (its->flags & ITS_FLAGS_RESEND_MAPC_ON_RESUME)
> +			its_cpu_init_collection(its);
>  	}
>  	spin_unlock(&its_lock);
>  }
> @@ -3312,6 +3317,10 @@ static int __init its_probe_one(struct resource *res,
>  		ctlr |= GITS_CTLR_ImDe;
>  	writel_relaxed(ctlr, its->base + GITS_CTLR);
>  
> +	if (fwnode_property_present(its->fwnode_handle,
> +				    "resend-mapc-on-resume"))
> +		its->flags |= ITS_FLAGS_RESEND_MAPC_ON_RESUME;

This function is firmware agnostic, and I really don't want to make
this thing a general purpose property.

Please set this as a quirk when you detect some particular combination
of HW and firmware (in your case, GIC500 + this property). And again,
the property name is totally backward. IT should be something like
"collection-state-lost-on-suspend".

> +
>  	err = its_init_domain(handle, its);
>  	if (err)
>  		goto out_free_tables;
> @@ -3347,7 +3356,7 @@ int its_cpu_init(void)
>  			return -ENXIO;
>  		}
>  		its_cpu_init_lpis();
> -		its_cpu_init_collection();
> +		its_cpu_init_collections();
>  	}
>  
>  	return 0;
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 

Thanks,

	M.