Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Aug 2023 13:30:30 +0100
Message-ID: <86zg2ado9l.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Neil Armstrong <neil.armstrong@linaro.org>
Subject: Re: [PATCH v3 1/2] irqchip/qcom-pdc: Add support for v3.2 HW
In-Reply-To: <20230829092119.1017194-2-dmitry.baryshkov@linaro.org>
References: <20230829092119.1017194-1-dmitry.baryshkov@linaro.org>
        <20230829092119.1017194-2-dmitry.baryshkov@linaro.org>
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/28.2
 (aarch64-unknown-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 Tue, 29 Aug 2023 10:21:18 +0100,
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
> From: Neil Armstrong <neil.armstrong@linaro.org>
> 
> Starting from HW version 3.2 the IRQ_ENABLE bit has moved to the
> IRQ_i_CFG register and requires a change of the driver to avoid
> writing into an undefined register address.
> 
> Get the HW version from registers and set the IRQ_ENABLE bit to the
> correct register depending on the HW version.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> [DB: fix crash on sm8150 DTs which listed short PDC region]
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/irqchip/qcom-pdc.c | 73 ++++++++++++++++++++++++++++++--------
>  1 file changed, 59 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index a32c0d28d038..f9f44b494b1d 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -22,9 +22,22 @@
>  
>  #define PDC_MAX_GPIO_IRQS	256
>  
> +/* Valid only on HW version < 3.2 */
>  #define IRQ_ENABLE_BANK		0x10
>  #define IRQ_i_CFG		0x110
>  
> +/* Valid only on HW version >= 3.2 */
> +#define IRQ_i_CFG_IRQ_ENABLE	3
> +
> +#define IRQ_i_CFG_TYPE_MASK	GENMASK(2, 0)
> +
> +#define PDC_VERSION		0x1000

That's an offset, right? Maybe spelling it as such would make this
more readable...

> +
> +/* Notable PDC versions */
> +enum {
> +	PDC_VERSION_3_2	= 0x30200,

... specially when reading this (why is it all of a sudden an enum?).

> +};
> +
>  struct pdc_pin_region {
>  	u32 pin_base;
>  	u32 parent_base;
> @@ -37,6 +50,7 @@ static DEFINE_RAW_SPINLOCK(pdc_lock);
>  static void __iomem *pdc_base;
>  static struct pdc_pin_region *pdc_region;
>  static int pdc_region_cnt;
> +static unsigned int pdc_version;
>  
>  static void pdc_reg_write(int reg, u32 i, u32 val)
>  {
> @@ -53,15 +67,22 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>  	int pin_out = d->hwirq;
>  	unsigned long enable;
>  	unsigned long flags;
> -	u32 index, mask;
> -
> -	index = pin_out / 32;
> -	mask = pin_out % 32;
>  
>  	raw_spin_lock_irqsave(&pdc_lock, flags);
> -	enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
> -	__assign_bit(mask, &enable, on);
> -	pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
> +	if (pdc_version < PDC_VERSION_3_2) {
> +		u32 index, mask;
> +
> +		index = pin_out / 32;
> +		mask = pin_out % 32;
> +
> +		enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
> +		__assign_bit(mask, &enable, on);
> +		pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
> +	} else {
> +		enable = pdc_reg_read(IRQ_i_CFG, pin_out);
> +		__assign_bit(IRQ_i_CFG_IRQ_ENABLE, &enable, on);
> +		pdc_reg_write(IRQ_i_CFG, pin_out, enable);
> +	}
>  	raw_spin_unlock_irqrestore(&pdc_lock, flags);
>  }
>  
> @@ -142,6 +163,7 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>  	}
>  
>  	old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
> +	pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
>  	pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>  
>  	ret = irq_chip_set_type_parent(d, type);
> @@ -246,7 +268,7 @@ static const struct irq_domain_ops qcom_pdc_ops = {
>  static int pdc_setup_pin_mapping(struct device_node *np)
>  {
>  	int ret, n, i;
> -	u32 irq_index, reg_index, val;
> +	unsigned long val;
>  
>  	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
>  	if (n <= 0 || n % 3)
> @@ -277,28 +299,51 @@ static int pdc_setup_pin_mapping(struct device_node *np)
>  			return ret;
>  
>  		for (i = 0; i < pdc_region[n].cnt; i++) {
> -			reg_index = (i + pdc_region[n].pin_base) >> 5;
> -			irq_index = (i + pdc_region[n].pin_base) & 0x1f;
> -			val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index);
> -			val &= ~BIT(irq_index);
> -			pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val);
> +			if (pdc_version < PDC_VERSION_3_2) {
> +				u32 irq_index, reg_index;
> +
> +				reg_index = (i + pdc_region[n].pin_base) >> 5;
> +				irq_index = (i + pdc_region[n].pin_base) & 0x1f;
> +				val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index);
> +				__assign_bit(irq_index, &val, 0);
> +				pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val);
> +			} else {
> +				u32 irq;
> +
> +				irq = i + pdc_region[n].pin_base;
> +				val = pdc_reg_read(IRQ_i_CFG, irq);
> +				__assign_bit(IRQ_i_CFG_IRQ_ENABLE, &val,  0);
> +				pdc_reg_write(IRQ_i_CFG, irq, val);
> +			}

This is a bit backwards. The PDC version doesn't change within the
loop. But more importantly, this is a rewrite of the pdc_enable_intr()
helper, only taking raw indices instead of an irq_data pointer.

Surely this can be written in a better way.

>  		}
>  	}
>  
>  	return 0;
>  }
>  
> +#define QCOM_PDC_SIZE 0x30000
> +
>  static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>  {
>  	struct irq_domain *parent_domain, *pdc_domain;
> +	struct resource res;
> +	resource_size_t res_size;

nit: swapping these two lines will make things vaguely more readable.

>  	int ret;
>  
> -	pdc_base = of_iomap(node, 0);
> +	/* compat with old sm8150 DT which had very small region for PDC */
> +	if (of_address_to_resource(node, 0, &res))
> +		return -EINVAL;
> +
> +	res_size = max_t(resource_size_t, resource_size(&res), QCOM_PDC_SIZE);

This probably deserves a warning so that DTs that do not have the
correct size get fixed.

Thanks,

	M.

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