Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 25 Feb 2022 15:27:28 +0000
Message-ID: <87lexz2cgv.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Hector Martin <marcan@marcan.st>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Rob Herring <robh+dt@kernel.org>,
        Sven Peter <sven@svenpeter.dev>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Mark Kettenis <mark.kettenis@xs4all.nl>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v2 7/7] irqchip/apple-aic: Add support for AICv2
In-Reply-To: <20220224130741.63924-8-marcan@marcan.st>
References: <20220224130741.63924-1-marcan@marcan.st>
        <20220224130741.63924-8-marcan@marcan.st>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 24 Feb 2022 13:07:41 +0000,
Hector Martin <marcan@marcan.st> wrote:
> 
> Introduce support for the new AICv2 hardware block in t6000/t6001 SoCs.
> 
> It seems these blocks are missing the information required to compute
> the event register offset in the capability registers, so we specify
> that in the DT.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/irqchip/irq-apple-aic.c | 148 ++++++++++++++++++++++++++++----
>  1 file changed, 129 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
> index 93c622435ba2..602c8b274170 100644
> --- a/drivers/irqchip/irq-apple-aic.c
> +++ b/drivers/irqchip/irq-apple-aic.c
> @@ -103,6 +103,57 @@
>  
>  #define AIC_MAX_IRQ		0x400
>  
> +/*
> + * AIC v2 registers (MMIO)
> + */
> +
> +#define AIC2_VERSION		0x0000
> +#define AIC2_VERSION_VER	GENMASK(7, 0)
> +
> +#define AIC2_INFO1		0x0004
> +#define AIC2_INFO1_NR_IRQ	GENMASK(15, 0)
> +#define AIC2_INFO1_LAST_DIE	GENMASK(27, 24)
> +
> +#define AIC2_INFO2		0x0008
> +
> +#define AIC2_INFO3		0x000c
> +#define AIC2_INFO3_MAX_IRQ	GENMASK(15, 0)
> +#define AIC2_INFO3_MAX_DIE	GENMASK(27, 24)
> +
> +#define AIC2_RESET		0x0010
> +#define AIC2_RESET_RESET	BIT(0)
> +
> +#define AIC2_CONFIG		0x0014
> +#define AIC2_CONFIG_ENABLE	BIT(0)
> +#define AIC2_CONFIG_PREFER_PCPU	BIT(28)
> +
> +#define AIC2_TIMEOUT		0x0028
> +#define AIC2_CLUSTER_PRIO	0x0030
> +#define AIC2_DELAY_GROUPS	0x0100
> +
> +#define AIC2_IRQ_CFG		0x2000
> +
> +/*
> + * AIC2 registers are laid out like this, starting at AIC2_IRQ_CFG:
> + *
> + * Repeat for each die:
> + *   IRQ_CFG: u32 * MAX_IRQS
> + *   SW_SET: u32 * (MAX_IRQS / 32)
> + *   SW_CLR: u32 * (MAX_IRQS / 32)
> + *   MASK_SET: u32 * (MAX_IRQS / 32)
> + *   MASK_CLR: u32 * (MAX_IRQS / 32)
> + *   HW_STATE: u32 * (MAX_IRQS / 32)
> + *
> + * This is followed by a set of event registers, each 16K page aligned.
> + * The first one is the AP event register we will use. Unfortunately,
> + * the actual implemented die count is not specified anywhere in the
> + * capability registers, so we have to explicitly specify the event
> + * register offset in the device tree to remain forward-compatible.
> + */
> +
> +#define AIC2_IRQ_CFG_TARGET	GENMASK(3, 0)
> +#define AIC2_IRQ_CFG_DELAY_IDX	GENMASK(7, 5)
> +
>  #define MASK_REG(x)		(4 * ((x) >> 5))
>  #define MASK_BIT(x)		BIT((x) & GENMASK(4, 0))
>  
> @@ -193,6 +244,7 @@ struct aic_info {
>  	/* Register offsets */
>  	u32 event;
>  	u32 target_cpu;
> +	u32 irq_cfg;
>  	u32 sw_set;
>  	u32 sw_clr;
>  	u32 mask_set;
> @@ -220,6 +272,14 @@ static const struct aic_info aic1_fipi_info = {
>  	.fast_ipi	= true,
>  };
>  
> +static const struct aic_info aic2_info = {
> +	.version	= 2,
> +
> +	.irq_cfg	= AIC2_IRQ_CFG,
> +
> +	.fast_ipi	= true,
> +};
> +
>  static const struct of_device_id aic_info_match[] = {
>  	{
>  		.compatible = "apple,t8103-aic",
> @@ -229,6 +289,10 @@ static const struct of_device_id aic_info_match[] = {
>  		.compatible = "apple,aic",
>  		.data = &aic1_info,
>  	},
> +	{
> +		.compatible = "apple,aic2",
> +		.data = &aic2_info,
> +	},
>  	{}
>  };
>  
> @@ -373,6 +437,14 @@ static struct irq_chip aic_chip = {
>  	.irq_set_type = aic_irq_set_type,
>  };
>  
> +static struct irq_chip aic2_chip = {
> +	.name = "AIC2",
> +	.irq_mask = aic_irq_mask,
> +	.irq_unmask = aic_irq_unmask,
> +	.irq_eoi = aic_irq_eoi,
> +	.irq_set_type = aic_irq_set_type,
> +};
> +
>  /*
>   * FIQ irqchip
>   */
> @@ -529,10 +601,15 @@ static struct irq_chip fiq_chip = {
>  static int aic_irq_domain_map(struct irq_domain *id, unsigned int irq,
>  			      irq_hw_number_t hw)
>  {
> +	struct aic_irq_chip *ic = id->host_data;
>  	u32 type = FIELD_GET(AIC_EVENT_TYPE, hw);
> +	struct irq_chip *chip = &aic_chip;
> +
> +	if (ic->info.version == 2)
> +		chip = &aic2_chip;
>  
>  	if (type == AIC_EVENT_TYPE_IRQ) {
> -		irq_domain_set_info(id, irq, hw, &aic_chip, id->host_data,
> +		irq_domain_set_info(id, irq, hw, chip, id->host_data,
>  				    handle_fasteoi_irq, NULL, NULL);
>  		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
>  	} else {
> @@ -888,24 +965,26 @@ static int aic_init_cpu(unsigned int cpu)
>  	/* Commit all of the above */
>  	isb();
>  
> -	/*
> -	 * Make sure the kernel's idea of logical CPU order is the same as AIC's
> -	 * If we ever end up with a mismatch here, we will have to introduce
> -	 * a mapping table similar to what other irqchip drivers do.
> -	 */
> -	WARN_ON(aic_ic_read(aic_irqc, AIC_WHOAMI) != smp_processor_id());
> +	if (aic_irqc->info.version == 1) {
> +		/*
> +		 * Make sure the kernel's idea of logical CPU order is the same as AIC's
> +		 * If we ever end up with a mismatch here, we will have to introduce
> +		 * a mapping table similar to what other irqchip drivers do.
> +		 */
> +		WARN_ON(aic_ic_read(aic_irqc, AIC_WHOAMI) != smp_processor_id());

Don't you have a similar issue with AICv2?  Or is it that AICv2
doesn't have this register?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.