From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 175B03F7888 for ; Tue, 26 May 2026 12:23:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779798214; cv=none; b=Vr0OBXpgiFKxcfUuV0EpSgUN1CWosBxT9bDeSMoYIsth8Q1scV8GK5h80LRjnV7xM1OOjC1cK/RcXjcIoh7X91ruVjbWCowZ9EB/dUh0JOjpB1ry4S4KFp4RufEF6B5N9mzcSV8d51j9jjMqkWdt0ZlTLh8b8Zbu/LleuZ+ceP8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779798214; c=relaxed/simple; bh=iGDQmirXwsVXqAciJSjXGrEGgNfDX2hiOOLxRSpT0f0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=OTkswSNVjXP1m6mzmfnyYXGJliQPxzOLNI70hbVQdsdEVRrIpwXAnWb17q0tFIUUAqxTAHkQCNWx6CqYwm+rX/cT0nxBDa5fQ7PmzTOvn7r6D9cZ7acKdY/LtomYOkmfEvJ8QYHk2hHoahbcIBk3suJLyeysVkaSArI1HPX0YS4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=eZPR3riL; arc=none smtp.client-ip=209.85.128.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="eZPR3riL" Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-490388fd0dbso49600185e9.0 for ; Tue, 26 May 2026 05:23:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1779798210; x=1780403010; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ZCIY9sU4f3NHfQKfeX8dod9XT1LJXx2xAP5zQxt3BVI=; b=eZPR3riL+W3qzANtKqmOvNbGUks1goO8ojCdqBbVgQKNnexrOZ/10JxbUBr9dsJTIH ogkU9zT9/Qdtl7BCSsNZHDrG0MNwJmwBd0ZKos+BrB9ZHzVQeay4g8PsBgdvaVX/vIQ3 xCmGrIW7IEnFtik3bIZkE+g76i4XNlVkczMIxGcLKKskwvemjd97gyWwxwHLzW45GwEd TeVypgBZSz/SzDGxqCblGMAw1AZhkMq5Vf9NNFVxcvUdAN1PawnP8O06FksdHOS/H9rv K8+7QGcFfhE+sa3rUATMM6AolENy5WtTN9QpghsOgykaLuTP6uGxl8L0DJehJu3kqOke lzIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779798210; x=1780403010; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ZCIY9sU4f3NHfQKfeX8dod9XT1LJXx2xAP5zQxt3BVI=; b=QYVarofqhrHp1n5inR4odUt01Tq2ObR9Yx5Jw3HWHcfox3ouuCCZ5bRfOjhEQLasDk 1ABLgPQxxkBTLjMYUDm/dYgfkbSXqUGxYA2FPT+A2Ljjf+cRbWxl+A9CIo2hxGVrsuOh a9Cz+QThcgZGOQwXD6LcEwMHh/2Q8u4kQB08l4WaSDXVymLD5FOGrA+SttJW3lRuG9tm h4ZSC5jYnVtSg4ZTQbcivAo2HNHs1WMQSXLQQuqUkXioXaWZWhn6mj/ugD+pL5WIifi4 PTjDE6Rrl0dE+EUL8DIC9Ho0Omk7i3xLZYyQ+9dzw2bK1QOf8MsfwsEzatv55nnIc2eU GZtw== X-Forwarded-Encrypted: i=1; AFNElJ+fLvIqhy5IzF3t4BdhlsioB1N36kv9hiVkEqHzwzMYS/dyj6fohbnMpiQoV2k5IVaLBkOwY6LewXiB@vger.kernel.org X-Gm-Message-State: AOJu0Yw1Xe05M6QGPfiiyRjiwUE1OxoGX68Z7LVNv9qfxDuKyA5UD0KU DehQkwYXyZ7Essqmcv3DHa93AveFPY8Rg8jx4le/b1MlM0vcBqRtCmvzugIqvgnjmbk= X-Gm-Gg: Acq92OGlPL+lHVw1k+t6hR/K3VMknSsphNsWCDLtePacr2szOJPHmR06a9ca33q0TAn Akg/UycxmyG3JRxJhmrDR56Pcyb71f8/3GP5WuUwOT5s9Y3rjMaFJPNjGb8tdUpMLGam24qN+zn iAw8HEe4RFjuV9vFc7Gi8V1GtgquN5KqyztdKqLM+w9f9+2iAc9RbtL1E2YVOll2UhVcDkDm/DO rDfl97bQ8a1GDSPOzbx++Vm8GGjr0uHYv83sEOj3V3Ex8TBRMej5qKrGRUXA8p4F/yhPcSsBctd VEC54VolLDV5mLUnwsQf0fBM9Knzq0pSZDbUJq+t5zwr1JkL4zMifYfSoimEYkwYGcDQjvDGv+0 lk1/L6JxbhY3NpsuGzYuf0TvAZr0nBajZJN20PKCryMUimvDTjP7VA4tnt4mCEDI3I3HlBZRHkw ZPLUWNhhTWcy8BNP9ZSlpXN/eJhfRGE/NJayhGz6E= X-Received: by 2002:a05:600c:4510:b0:48f:e230:c3fa with SMTP id 5b1f17b1804b1-490428ddc9fmr289822275e9.32.1779798210262; Tue, 26 May 2026 05:23:30 -0700 (PDT) Received: from linaro.org ([77.64.147.108]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-45eb6d4850dsm33487487f8f.17.2026.05.26.05.23.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 May 2026 05:23:29 -0700 (PDT) Date: Tue, 26 May 2026 14:22:49 +0200 From: Stephan Gerhold To: Maulik Shah Cc: Bjorn Andersson , Konrad Dybcio , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Thomas Gleixner , Linus Walleij , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, Sneh Mankad Subject: Re: [PATCH v2 5/8] irqchip/qcom-pdc: Configure PDC to pass through mode Message-ID: References: <20260526-hamoa_pdc-v2-0-f6857af1ce91@oss.qualcomm.com> <20260526-hamoa_pdc-v2-5-f6857af1ce91@oss.qualcomm.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260526-hamoa_pdc-v2-5-f6857af1ce91@oss.qualcomm.com> On Tue, May 26, 2026 at 04:24:41PM +0530, Maulik Shah wrote: > All PDC irqchip supports pass through mode in which both Direct SPIs and > GPIO IRQs (as SPIs) are sent to GIC without latching at PDC. > > Newer PDCs (v3.0 onwards) also support additional secondary controller mode > where PDC latches GPIO IRQs and sends to GIC as level type IRQ. Direct SPIs > still works same as pass through mode without latching at PDC even in > secondary controller mode. > > All the SoCs so far default uses pass through mode with the exception of > x1e. x1e PDC may be set to secondary controller mode for builds on CRD > boards whereas it may be set to pass through mode for IoT-EVK boards. > The mode configuration is done in firmware and initially shipped windows > firmware did not have SCM interface to read or modify the PDC mode. > Later only write access is opened up for non secure world. > > Using the write access available add changes to modify the PDC mode to > pass through mode via SCM write. When the write fails (on older firmware) > assume to work in secondary mode. > > Co-developed-by: Sneh Mankad > Signed-off-by: Sneh Mankad > Signed-off-by: Maulik Shah > --- > drivers/irqchip/qcom-pdc.c | 109 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 106 insertions(+), 3 deletions(-) > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index 86379dddc5be..69ddd7d89a83 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > [...] > @@ -135,6 +151,8 @@ static const struct pdc_regs pdc_v3_2 = { > }; > > static const struct pdc_cfg pdc_cfg_v3_2 = { > + .gpio_irq_sts = GENMASK(5, 5), > + .gpio_irq_mask = GENMASK(4, 4), BIT(5) / BIT(4) would be clearer here in my opinion. > .irq_enable = GENMASK(3, 3), > .irq_type = GENMASK(2, 0), > }; > [...] > @@ -184,6 +204,14 @@ static u32 pdc_reg_read(int reg, u32 i) > return readl_relaxed(pdc->base + reg + i * sizeof(u32)); > } > > +static inline bool pdc_pin_uses_seconary_mode(int pin_out) > +{ > + if (pdc->mode == PDC_SECONDARY_MODE && pin_out >= pdc->num_spis) > + return true; > + > + return false; Can put this in one line: return pdc->mode == PDC_SECONDARY_MODE && pin_out >= pdc->num_spis; > +} > + > static void pdc_x1e_irq_enable_write(u32 bank, u32 enable) > { > void __iomem *base; > @@ -232,6 +260,36 @@ static void pdc_enable_intr_bank(int pin_out, bool on) > pdc_reg_write(pdc->regs->irq_en_reg, index, enable); > > raw_spin_unlock_irqrestore(&pdc->lock, flags); > + > + if (pdc_pin_uses_seconary_mode(pin_out)) > + pdc->unmask_gpio(pin_out, on); > +} > + > +static void pdc_clear_gpio_cfg(int pin_out) > +{ > + unsigned long gpio_sts; > + > + if (pdc->version < PDC_VERSION_3_0) > + return; > + > + gpio_sts = pdc_reg_read(pdc->regs->irq_cfg_reg, pin_out); > + gpio_sts &= ~pdc->cfg->gpio_irq_sts; > + pdc_reg_write(pdc->regs->irq_cfg_reg, pin_out, gpio_sts); Is this guaranteed to be called sequentially, i.e. not in parallel on another CPU? Otherwise, you need to add the lock here to make sure the read-modify-write doesn't race with another CPU. Note that since the irq_cfg_reg is also used in qcom_pdc_gic_set_type() it would be safest to add the lock there as well (although since PDC has IRQCHIP_SET_TYPE_MASKED it's probably unlikely to be called in parallel with another irqchip operation for the same IRQ). In my patch, I handled this for all users using a new pdc_update_irq_cfg() function [1]. [1]: https://github.com/stephan-gh/linux/commit/59ca2a7335ede83e4a7cf02704dd7c469c725c14 > +} > + > +static void pdc_unmask_gpio_cfg(int pin_out, bool unmask) > +{ > + unsigned long gpio_mask; > + > + if (pdc->version < PDC_VERSION_3_0) > + return; > + > + gpio_mask = pdc_reg_read(pdc->regs->irq_cfg_reg, pin_out); > + if (unmask) > + gpio_mask &= ~pdc->cfg->gpio_irq_mask; > + else > + gpio_mask |= pdc->cfg->gpio_irq_mask; > + pdc_reg_write(pdc->regs->irq_cfg_reg, pin_out, gpio_mask); > } > > static void pdc_enable_intr_cfg(int pin_out, bool on) > [...] > @@ -258,6 +319,20 @@ static void qcom_pdc_gic_enable(struct irq_data *d) > irq_chip_enable_parent(d); > } > > +static void qcom_pdc_ack(struct irq_data *d) > +{ > + if (pdc_pin_uses_seconary_mode(d->hwirq) && !irqd_is_level_type(d)) > + pdc->clear_gpio(d->hwirq); > +} You might need a write memory barrier here and/or read-back here to make sure the write is complete before the interrupt is unmasked in the GIC. IIRC I added this in my patch after seeing some test tlmm-test failure.. > + > +static void qcom_pdc_gic_eoi(struct irq_data *d) > +{ > + if (pdc_pin_uses_seconary_mode(d->hwirq) && irqd_is_level_type(d)) > + pdc->clear_gpio(d->hwirq); > + > + irq_chip_eoi_parent(d); > +} > + > /* > * GIC does not handle falling edge or active low. To allow falling edge and > * active low interrupts to be handled at GIC, PDC has an inverter that inverts > [...] > @@ -510,6 +600,10 @@ static int qcom_pdc_probe(struct platform_device *pdev, struct device_node *pare > pdc->enable_intr = pdc_enable_intr_bank; > } > > + pdc->unmask_gpio = pdc_unmask_gpio_cfg; > + pdc->clear_gpio = pdc_clear_gpio_cfg; What is the purpose of these function pointers if you always assign the same function? Thanks, Stephan