From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 33F71C83F14 for ; Tue, 29 Aug 2023 12:31:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235422AbjH2Mak (ORCPT ); Tue, 29 Aug 2023 08:30:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56180 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233660AbjH2Mag (ORCPT ); Tue, 29 Aug 2023 08:30:36 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DCEBEB9; Tue, 29 Aug 2023 05:30:33 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 7C5F06562E; Tue, 29 Aug 2023 12:30:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DEA8FC433C8; Tue, 29 Aug 2023 12:30:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1693312232; bh=kQz8n0/MK1lL1kYyZbVzmV8RmOo+hRfUy0GSC0bUaLQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Y4fdCOfAWo8eRPY7F6klj8SyB1mb49QMznl94AnQT1Og8Uq8R4s2VzwO4dlywgOtu 8pK6pPTa5z3cKd2nPwe/Q3UV3bhcIiAj3tjjecl1wtTTe+FsRJhHcWKzuPhexpToQK DGfpwaQAAWTCZOPH1VyaM61LB3mj6pCh2tDfHj664gSC2ALWZWQ8KXSskIBH7WH+9y NBowIkJJbZhl0n+Jw6Y1N6MgTHhb7Y6G43utREly4r0jfqm4JBohyxSlD3sIPuO4Z+ lRTRQLlY6K7xZWzkCL/q+J/GKsw15CARANOJJxzCT8Bcfot8frVJLgEguLeX1zz3mW nMyBcWywpsvQA== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1qaxra-0092gH-C8; Tue, 29 Aug 2023 13:30:30 +0100 Date: Tue, 29 Aug 2023 13:30:30 +0100 Message-ID: <86zg2ado9l.wl-maz@kernel.org> From: Marc Zyngier To: Dmitry Baryshkov Cc: Andy Gross , Bjorn Andersson , Konrad Dybcio , Thomas Gleixner , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, Neil Armstrong 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 X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: dmitry.baryshkov@linaro.org, agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org, tglx@linutronix.de, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, neil.armstrong@linaro.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 29 Aug 2023 10:21:18 +0100, Dmitry Baryshkov wrote: > > From: Neil Armstrong > > 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 > [DB: fix crash on sm8150 DTs which listed short PDC region] > Signed-off-by: Dmitry Baryshkov > --- > 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.