* [PATCH v4 2/2] pwm: Add Nuvoton MA35D1 PWM controller support
From: Chi-Wen Weng @ 2026-06-17 2:59 UTC (permalink / raw)
To: ukleinek, robh, krzk+dt, conor+dt
Cc: linux-arm-kernel, linux-pwm, devicetree, linux-kernel, cwweng,
cwweng.linux, Trevor Gamblin
In-Reply-To: <20260617025925.2539334-1-cwweng.linux@gmail.com>
From: Chi-Wen Weng <cwweng@nuvoton.com>
Add a PWM framework driver for the Nuvoton MA35D1 PWM controller.
The MA35D1 PWM controller provides 6 PWM channels. The hardware supports
up, down and up-down counter types, auto-reload and one-shot modes, and
independent and complementary output modes. This driver configures all
channels to up-counting mode, auto-reload mode and independent output mode.
The waveform generator is configured to drive the output high at the zero
point and low at the compare-up point. In up-counting mode the counter
counts from 0 to PERIOD inclusive, so the PWM period is PERIOD + 1 cycles.
With the selected waveform actions, CMPDAT = 0 generates 0% duty cycle and
CMPDAT > PERIOD generates 100% duty cycle. Limit PERIOD to 0xfffe so that
CMPDAT = 0xffff can be used for the full-duty case.
PERIOD and CMPDAT updates are buffered by the hardware and take effect at
the end of the current period because IMMLDENn is left disabled. When the
PWM output is disabled, POENn is cleared and the output pin is put into
tri-state.
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
---
drivers/pwm/Kconfig | 9 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-ma35d1.c | 344 +++++++++++++++++++++++++++++++++++++++
3 files changed, 354 insertions(+)
create mode 100644 drivers/pwm/pwm-ma35d1.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e8886a9b64d9..355131e6efac 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -463,6 +463,15 @@ config PWM_LPSS_PLATFORM
To compile this driver as a module, choose M here: the module
will be called pwm-lpss-platform.
+config PWM_MA35D1
+ tristate "Nuvoton MA35D1 PWM support"
+ depends on ARCH_MA35 || COMPILE_TEST
+ help
+ Generic PWM framework driver for Nuvoton MA35D1.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-ma35d1.
+
config PWM_MAX7360
tristate "MAX7360 PWMs"
depends on MFD_MAX7360
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 5630a521a7cf..7ad761ea27d1 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MA35D1) += pwm-ma35d1.o
obj-$(CONFIG_PWM_MAX7360) += pwm-max7360.o
obj-$(CONFIG_PWM_MC33XS2410) += pwm-mc33xs2410.o
obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o
diff --git a/drivers/pwm/pwm-ma35d1.c b/drivers/pwm/pwm-ma35d1.c
new file mode 100644
index 000000000000..c07eedeca035
--- /dev/null
+++ b/drivers/pwm/pwm-ma35d1.c
@@ -0,0 +1,344 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the Nuvoton MA35D1 PWM controller
+ *
+ * Copyright (C) 2026 Nuvoton Corporation
+ * Chi-Wen Weng <cwweng@nuvoton.com>
+ *
+ * Reference Manual:
+ * https://www.nuvoton.com.cn/resource-download.jsp?tp_GUID=DA05-MA35D16
+ *
+ * Limitations:
+ * - The hardware supports 6 PWM channels.
+ * - The hardware supports up, down and up-down counter types. This driver
+ * configures all channels to up-counting mode.
+ * - The hardware supports auto-reload and one-shot counter modes. This driver
+ * configures all channels to auto-reload mode.
+ * - The hardware supports independent and complementary output modes. This
+ * driver configures all channels to independent output mode.
+ * - The hardware supports programmable waveform actions at zero, period and
+ * compare points. This driver uses zero point high and compare-up point low
+ * actions for normal PWM output.
+ * - In up-counting mode, the counter counts from 0 to PERIOD inclusive. With
+ * zero point high and compare-up point low actions, CMPDAT = 0 produces 0%
+ * duty and CMPDAT > PERIOD produces 100% duty.
+ * - The driver limits PERIOD to 0xfffe so that CMPDAT can be set greater than
+ * PERIOD to generate a 100% duty cycle.
+ * - Period and duty cycle changes are buffered by hardware and take effect at
+ * the end of the current period because IMMLDENn is left disabled.
+ * - Polarity changes are applied directly and may cause a transient output
+ * change if the PWM output is running.
+ * - When disabled, the output pin is put in tri-state by clearing POENn.
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/math64.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#define MA35D1_REG_PWM_CTL0 0x00
+#define MA35D1_REG_PWM_CTL1 0x04
+#define MA35D1_REG_PWM_CNTEN 0x20
+#define MA35D1_REG_PWM_PERIOD(ch) (0x30 + 4 * (ch))
+#define MA35D1_REG_PWM_CMPDAT(ch) (0x50 + 4 * (ch))
+#define MA35D1_REG_PWM_WGCTL0 0xb0
+#define MA35D1_REG_PWM_WGCTL1 0xb4
+#define MA35D1_REG_PWM_POLCTL 0xd4
+#define MA35D1_REG_PWM_POEN 0xd8
+
+#define MA35D1_PWM_CTL1_CNTMODE_MASK(ch) BIT(16 + (ch))
+#define MA35D1_PWM_CTL1_OUTMODE_MASK(ch) BIT(24 + ((ch) / 2))
+
+#define MA35D1_PWM_WGCTL_ACTION_MASK 0x3
+#define MA35D1_PWM_WGCTL_ACTION_LOW 1
+#define MA35D1_PWM_WGCTL_ACTION_HIGH 2
+
+#define MA35D1_PWM_WGCTL_ZERO_HIGH(ch) \
+ (MA35D1_PWM_WGCTL_ACTION_HIGH << (2 * (ch)))
+#define MA35D1_PWM_WGCTL_CMP_UP_LOW(ch) \
+ (MA35D1_PWM_WGCTL_ACTION_LOW << (2 * (ch)))
+
+#define MA35D1_PWM_CNTEN_EN(ch) BIT(ch)
+#define MA35D1_PWM_POEN_EN(ch) BIT(ch)
+#define MA35D1_PWM_POLCTL_INV(ch) BIT(ch)
+
+#define MA35D1_PWM_MAX_CMPDAT 0xffff
+#define MA35D1_PWM_MAX_PERIOD 0xfffe
+#define MA35D1_PWM_MAX_PERIOD_CYCLES (MA35D1_PWM_MAX_PERIOD + 1)
+#define MA35D1_PWM_NUM_CHANNELS 6
+
+struct nuvoton_pwm {
+ void __iomem *base;
+ unsigned long clkrate;
+};
+
+static inline struct nuvoton_pwm *nuvoton_pwm_from_chip(struct pwm_chip *chip)
+{
+ return pwmchip_get_drvdata(chip);
+}
+
+static inline u32 nuvoton_pwm_ctl1_cnttype_mask(unsigned int ch)
+{
+ return MA35D1_PWM_WGCTL_ACTION_MASK << (2 * ch);
+}
+
+static inline u32 nuvoton_pwm_wgctl_zero_mask(unsigned int ch)
+{
+ return MA35D1_PWM_WGCTL_ACTION_MASK << (2 * ch);
+}
+
+static inline u32 nuvoton_pwm_wgctl_period_mask(unsigned int ch)
+{
+ return MA35D1_PWM_WGCTL_ACTION_MASK << (16 + 2 * ch);
+}
+
+static inline u32 nuvoton_pwm_wgctl_cmp_up_mask(unsigned int ch)
+{
+ return MA35D1_PWM_WGCTL_ACTION_MASK << (2 * ch);
+}
+
+static inline u32 nuvoton_pwm_wgctl_cmp_down_mask(unsigned int ch)
+{
+ return MA35D1_PWM_WGCTL_ACTION_MASK << (16 + 2 * ch);
+}
+
+static inline u32 nuvoton_pwm_readl(struct nuvoton_pwm *nvtpwm,
+ unsigned int offset)
+{
+ return readl(nvtpwm->base + offset);
+}
+
+static inline void nuvoton_pwm_writel(struct nuvoton_pwm *nvtpwm,
+ unsigned int offset, u32 value)
+{
+ writel(value, nvtpwm->base + offset);
+}
+
+static inline void nuvoton_pwm_rmw(struct nuvoton_pwm *nvtpwm,
+ unsigned int offset, u32 mask, u32 value)
+{
+ u32 reg;
+
+ reg = nuvoton_pwm_readl(nvtpwm, offset);
+ reg &= ~mask;
+ reg |= value & mask;
+ nuvoton_pwm_writel(nvtpwm, offset, reg);
+}
+
+static void nuvoton_pwm_init(struct nuvoton_pwm *nvtpwm)
+{
+ u32 ctl1_mask = 0;
+ u32 wgctl0_mask = 0;
+ u32 wgctl0_val = 0;
+ u32 wgctl1_mask = 0;
+ u32 wgctl1_val = 0;
+ int ch;
+
+ for (ch = 0; ch < MA35D1_PWM_NUM_CHANNELS; ch++) {
+ /* CNTTYPEn = 00: up counter type */
+ ctl1_mask |= nuvoton_pwm_ctl1_cnttype_mask(ch);
+
+ /* CNTMODEn = 0: auto-reload mode */
+ ctl1_mask |= MA35D1_PWM_CTL1_CNTMODE_MASK(ch);
+
+ /* ZPCTLn = 10: output high at zero point */
+ wgctl0_mask |= nuvoton_pwm_wgctl_zero_mask(ch);
+ wgctl0_val |= MA35D1_PWM_WGCTL_ZERO_HIGH(ch);
+
+ /* PRDPCTLn = 00: do nothing at period point */
+ wgctl0_mask |= nuvoton_pwm_wgctl_period_mask(ch);
+
+ /* CMPUCTLn = 01: output low at compare up point */
+ wgctl1_mask |= nuvoton_pwm_wgctl_cmp_up_mask(ch);
+ wgctl1_val |= MA35D1_PWM_WGCTL_CMP_UP_LOW(ch);
+
+ /* CMPDCTLn = 00: do nothing at compare down point */
+ wgctl1_mask |= nuvoton_pwm_wgctl_cmp_down_mask(ch);
+ }
+
+ for (ch = 0; ch < MA35D1_PWM_NUM_CHANNELS; ch += 2) {
+ /* OUTMODEn = 0: independent mode */
+ ctl1_mask |= MA35D1_PWM_CTL1_OUTMODE_MASK(ch);
+ }
+
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_CTL1, ctl1_mask, 0);
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_WGCTL0,
+ wgctl0_mask, wgctl0_val);
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_WGCTL1,
+ wgctl1_mask, wgctl1_val);
+}
+
+static int nuvoton_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct nuvoton_pwm *nvtpwm = nuvoton_pwm_from_chip(chip);
+ u32 ch = pwm->hwpwm;
+ u64 duty_cycles, period_cycles;
+ u32 cmpdat, period;
+
+ if (!state->enabled) {
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POEN,
+ MA35D1_PWM_POEN_EN(ch), 0);
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_CNTEN,
+ MA35D1_PWM_CNTEN_EN(ch), 0);
+
+ return 0;
+ }
+
+ period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
+ state->period,
+ NSEC_PER_SEC);
+ if (!period_cycles)
+ return -EINVAL;
+
+ if (period_cycles > MA35D1_PWM_MAX_PERIOD_CYCLES)
+ period_cycles = MA35D1_PWM_MAX_PERIOD_CYCLES;
+
+ duty_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
+ state->duty_cycle,
+ NSEC_PER_SEC);
+ if (duty_cycles > period_cycles)
+ duty_cycles = period_cycles;
+
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POLCTL,
+ MA35D1_PWM_POLCTL_INV(ch), 0);
+ else
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POLCTL,
+ MA35D1_PWM_POLCTL_INV(ch),
+ MA35D1_PWM_POLCTL_INV(ch));
+
+ /*
+ * In up-counting mode the counter counts from 0 to PERIOD inclusive.
+ * With zero point high and compare-up point low actions:
+ * - CMPDAT = 0 produces 0% duty.
+ * - CMPDAT > PERIOD produces 100% duty.
+ * PERIOD is limited to 0xfffe, so duty_cycles can be written directly
+ * to CMPDAT and still fit in the 16-bit compare field for 100% duty.
+ */
+ period = period_cycles - 1;
+ cmpdat = duty_cycles;
+
+ nuvoton_pwm_writel(nvtpwm, MA35D1_REG_PWM_PERIOD(ch), period);
+ nuvoton_pwm_writel(nvtpwm, MA35D1_REG_PWM_CMPDAT(ch), cmpdat);
+
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_CNTEN,
+ MA35D1_PWM_CNTEN_EN(ch), MA35D1_PWM_CNTEN_EN(ch));
+ nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POEN,
+ MA35D1_PWM_POEN_EN(ch), MA35D1_PWM_POEN_EN(ch));
+
+ return 0;
+}
+
+static int nuvoton_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct nuvoton_pwm *nvtpwm = nuvoton_pwm_from_chip(chip);
+ u32 ch = pwm->hwpwm;
+ u32 cmpdat, cnten, period, poen, polctl;
+ u64 duty_cycles, period_cycles;
+
+ cnten = nuvoton_pwm_readl(nvtpwm, MA35D1_REG_PWM_CNTEN);
+ poen = nuvoton_pwm_readl(nvtpwm, MA35D1_REG_PWM_POEN);
+ polctl = nuvoton_pwm_readl(nvtpwm, MA35D1_REG_PWM_POLCTL);
+ period = nuvoton_pwm_readl(nvtpwm, MA35D1_REG_PWM_PERIOD(ch)) &
+ MA35D1_PWM_MAX_CMPDAT;
+ cmpdat = nuvoton_pwm_readl(nvtpwm, MA35D1_REG_PWM_CMPDAT(ch)) &
+ MA35D1_PWM_MAX_CMPDAT;
+
+ period_cycles = period + 1;
+ if (cmpdat > period)
+ duty_cycles = period_cycles;
+ else
+ duty_cycles = cmpdat;
+
+ state->enabled = (cnten & MA35D1_PWM_CNTEN_EN(ch)) &&
+ (poen & MA35D1_PWM_POEN_EN(ch));
+ state->polarity = (polctl & MA35D1_PWM_POLCTL_INV(ch)) ?
+ PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+ state->period = DIV64_U64_ROUND_UP(period_cycles * NSEC_PER_SEC,
+ nvtpwm->clkrate);
+ state->duty_cycle = DIV64_U64_ROUND_UP(duty_cycles * NSEC_PER_SEC,
+ nvtpwm->clkrate);
+
+ return 0;
+}
+
+static const struct pwm_ops nuvoton_pwm_ops = {
+ .apply = nuvoton_pwm_apply,
+ .get_state = nuvoton_pwm_get_state,
+};
+
+static int nuvoton_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pwm_chip *chip;
+ struct nuvoton_pwm *nvtpwm;
+ struct clk *clk;
+ int ret;
+
+ chip = devm_pwmchip_alloc(dev, MA35D1_PWM_NUM_CHANNELS,
+ sizeof(*nvtpwm));
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
+
+ nvtpwm = nuvoton_pwm_from_chip(chip);
+
+ nvtpwm->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(nvtpwm->base))
+ return PTR_ERR(nvtpwm->base);
+
+ clk = devm_clk_get_enabled(dev, NULL);
+ if (IS_ERR(clk))
+ return dev_err_probe(dev, PTR_ERR(clk),
+ "Unable to get the clock\n");
+
+ ret = devm_clk_rate_exclusive_get(dev, clk);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Unable to get exclusive clock rate\n");
+
+ nvtpwm->clkrate = clk_get_rate(clk);
+ if (!nvtpwm->clkrate)
+ return dev_err_probe(dev, -EINVAL,
+ "PWM clock rate is zero\n");
+
+ if (nvtpwm->clkrate > NSEC_PER_SEC)
+ return dev_err_probe(dev, -EINVAL,
+ "PWM clock out of range (%lu)\n",
+ nvtpwm->clkrate);
+
+ nuvoton_pwm_init(nvtpwm);
+
+ chip->ops = &nuvoton_pwm_ops;
+ chip->atomic = true;
+
+ ret = devm_pwmchip_add(dev, chip);
+ if (ret)
+ return dev_err_probe(dev, ret, "Unable to add PWM chip\n");
+
+ return 0;
+}
+
+static const struct of_device_id nuvoton_pwm_of_match[] = {
+ { .compatible = "nuvoton,ma35d1-pwm" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, nuvoton_pwm_of_match);
+
+static struct platform_driver nuvoton_pwm_driver = {
+ .probe = nuvoton_pwm_probe,
+ .driver = {
+ .name = "nuvoton-pwm",
+ .of_match_table = nuvoton_pwm_of_match,
+ },
+};
+module_platform_driver(nuvoton_pwm_driver);
+
+MODULE_AUTHOR("Chi-Wen Weng <cwweng@nuvoton.com>");
+MODULE_DESCRIPTION("Nuvoton MA35D1 PWM driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related
* [PATCH 1/3] PCI: rcar-gen4: Configure AXIINTC if iMSI-RX not used
From: Marek Vasut @ 2026-06-17 2:59 UTC (permalink / raw)
To: linux-pci
Cc: Marek Vasut, Yoshihiro Shimoda, Krzysztof Wilczyński,
Bjorn Helgaas, Catalin Marinas, Conor Dooley, Geert Uytterhoeven,
Krzysztof Kozlowski, Lorenzo Pieralisi, Manivannan Sadhasivam,
Marc Zyngier, Rob Herring, devicetree, linux-arm-kernel,
linux-doc, linux-kernel, linux-renesas-soc
In case MSI are enabled, but DWC built-in iMSI-RX is not in use, the
MSI are handled via GIC ITS. Configure all controller MSI registers
fully.
Set or clear MSI capability register MSICAP0 MSI enable MSIE bit and
PCIe Interrupt Status 0 Enable register PCIEINTSTS0EN MSI interrupt
enable MSI_CTRL_INT bit according to MSI enable state, set both bits
if MSI are enabled, clear both bits if MSI are disabled.
If MSI are disabled, or MSI are enabled and iMSI-RX is used, then
deconfigure AXIINTCADDR and AXIINTCCONT to 0, which disables any
pass through of MSI TLPs onto the AXI bus and then further into
GIC ITS translation registers.
If MSI are enabled and iMSI-RX is not used, the configure AXIINTCADDR
with target address of GIC ITS translation registers, and configure
AXIINTCCONT to enable MSI TLP pass through onto AXI bus and into the
GIC ITS. This specific configuration allows handling of MSI via the
GIC ITS instead of integrated iMSI-RX.
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
NOTE: This would not be possible without prior work from Shimoda-san
---
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
drivers/pci/controller/dwc/pcie-rcar-gen4.c | 53 +++++++++++++++++++--
1 file changed, 48 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index 485cfa8bd9692..ba6e3bedd6d0a 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -31,6 +31,10 @@
#define DEVICE_TYPE_RC BIT(4)
#define BIFUR_MOD_SET_ON BIT(0)
+/* MSI Capability */
+#define MSICAP0 0x0050
+#define MSICAP0_MSIE BIT(16)
+
/* PCIe Interrupt Status 0 */
#define PCIEINTSTS0 0x0084
@@ -55,6 +59,16 @@
#define APP_HOLD_PHY_RST BIT(16)
#define APP_LTSSM_ENABLE BIT(0)
+/* INTC address */
+#define AXIINTCADDR 0x0a00
+/* GITS GIC ITS translation register */
+#define AXIINTCADDR_VAL 0xf1050000
+
+/* INTC control & mask */
+#define AXIINTCCONT 0x0a04
+#define INTC_EN BIT(31)
+#define INTC_MASK GENMASK(11, 2)
+
/* PCIe Power Management Control */
#define PCIEPWRMNGCTRL 0x0070
#define APP_CLK_REQ_N BIT(11)
@@ -305,6 +319,39 @@ static struct rcar_gen4_pcie *rcar_gen4_pcie_alloc(struct platform_device *pdev)
return rcar;
}
+static void rcar_gen4_pcie_host_msi_init(struct dw_pcie_rp *pp)
+{
+ struct dw_pcie *dw = to_dw_pcie_from_pp(pp);
+ struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
+ u32 val;
+
+ /* Make sure MSICAP0 MSIE is configured. */
+ val = dw_pcie_readl_dbi(dw, MSICAP0);
+ if (pci_msi_enabled())
+ val |= MSICAP0_MSIE;
+ else
+ val &= ~MSICAP0_MSIE;
+ dw_pcie_writel_dbi(dw, MSICAP0, val);
+
+ if (!pci_msi_enabled() || pp->use_imsi_rx) {
+ /* Clear AXIINTC mapping. */
+ writel(0, rcar->base + AXIINTCADDR);
+ writel(0, rcar->base + AXIINTCCONT);
+ } else {
+ /* Point AXIINTC to GIC ITS and enable. */
+ writel(AXIINTCADDR_VAL, rcar->base + AXIINTCADDR);
+ writel(INTC_EN | INTC_MASK, rcar->base + AXIINTCCONT);
+ }
+
+ /* Configure MSI interrupt signal */
+ val = readl(rcar->base + PCIEINTSTS0EN);
+ if (pci_msi_enabled())
+ val |= MSI_CTRL_INT;
+ else
+ val &= ~MSI_CTRL_INT;
+ writel(val, rcar->base + PCIEINTSTS0EN);
+}
+
static int rcar_gen4_pcie_enable_device(struct pci_host_bridge *bridge,
struct pci_dev *dev)
{
@@ -359,7 +406,6 @@ static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
struct dw_pcie *dw = to_dw_pcie_from_pp(pp);
struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
int ret;
- u32 val;
if (pp->bridge)
pp->bridge->enable_device = rcar_gen4_pcie_enable_device;
@@ -379,10 +425,7 @@ static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
dw_pcie_writel_dbi2(dw, PCI_BASE_ADDRESS_0, 0x0);
dw_pcie_writel_dbi2(dw, PCI_BASE_ADDRESS_1, 0x0);
- /* Enable MSI interrupt signal */
- val = readl(rcar->base + PCIEINTSTS0EN);
- val |= MSI_CTRL_INT;
- writel(val, rcar->base + PCIEINTSTS0EN);
+ rcar_gen4_pcie_host_msi_init(pp);
msleep(PCIE_T_PVPERL_MS); /* pe_rst requires 100msec delay */
--
2.53.0
^ permalink raw reply related
* [PATCH 2/3] irqchip/gic-v3: Add Renesas R-Car Gen4 erratum workaround
From: Marek Vasut @ 2026-06-17 2:59 UTC (permalink / raw)
To: linux-pci
Cc: Marek Vasut, Yoshihiro Shimoda, Krzysztof Wilczyński,
Bjorn Helgaas, Catalin Marinas, Conor Dooley, Geert Uytterhoeven,
Krzysztof Kozlowski, Lorenzo Pieralisi, Manivannan Sadhasivam,
Marc Zyngier, Rob Herring, devicetree, linux-arm-kernel,
linux-doc, linux-kernel, linux-renesas-soc
In-Reply-To: <20260617030008.154449-1-marek.vasut+renesas@mailbox.org>
Renesas R-Car S4/V4H/V4M GIC600 integration has address width for AXI
or APB interface configured to 32 bit, it can therefore access only
the first 4 GiB of physical address space. This information comes from
R-Car V4H Interface Specification sheet, there is currently no technical
update number assigned to this limitation. Further input from hardware
engineer indicates that this limitation also applies to R-Car S4 and V4M.
Name the limitation GEN4GICITS1, and add a driver quirk to mitigate this
limitation.
Note that the 0x0201743b GIC600 ID is not Renesas-specific, it is
common for many ARM GICv3 implementations. Therefore, add an extra
of_machine_is_compatible() check.
The GIC600 implementation in R-Car S4/V4H/V4M is r1p6.
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
NOTE: This would not be possible without prior work from Shimoda-san
https://lore.kernel.org/all/20240214052050.1966439-1-yoshihiro.shimoda.uh@renesas.com/
---
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
Documentation/arch/arm64/silicon-errata.rst | 1 +
arch/arm64/Kconfig | 9 +++++++++
drivers/irqchip/irq-gic-v3-its.c | 20 ++++++++++++++++++++
3 files changed, 30 insertions(+)
diff --git a/Documentation/arch/arm64/silicon-errata.rst b/Documentation/arch/arm64/silicon-errata.rst
index 014aa1c215a16..b0c68b64f5ac2 100644
--- a/Documentation/arch/arm64/silicon-errata.rst
+++ b/Documentation/arch/arm64/silicon-errata.rst
@@ -352,6 +352,7 @@ stable kernels.
+----------------+-----------------+-----------------+-----------------------------+
| Qualcomm Tech. | Kryo4xx Gold | N/A | ARM64_ERRATUM_1286807 |
+----------------+-----------------+-----------------+-----------------------------+
+| Renesas | S4/V4H/V4M | N/A | RENESAS_ERRATUM_GEN4GICITS1 |
+----------------+-----------------+-----------------+-----------------------------+
| Rockchip | RK3588 | #3588001 | ROCKCHIP_ERRATUM_3588001 |
+----------------+-----------------+-----------------+-----------------------------+
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b3afe0688919b..b9e17ce475e61 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1382,6 +1382,15 @@ config NVIDIA_CARMEL_CNP_ERRATUM
If unsure, say Y.
+config RENESAS_ERRATUM_GEN4GICITS1
+ bool "Renesas R-Car Gen4: GIC600 can not access physical addresses above 4 GiB"
+ default y
+ help
+ The Renesas R-Car Gen4 S4/V4H/V4M GIC600 SoC integrations have AXI
+ addressing limited to the first 32-bit of physical address space.
+
+ If unsure, say Y.
+
config ROCKCHIP_ERRATUM_3568002
bool "Rockchip 3568002: GIC600 can not access physical addresses higher than 4GB"
default y
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index b57d81ad33a0a..ec3756f29cf1a 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4901,6 +4901,18 @@ static bool __maybe_unused its_enable_rk3568002(void *data)
return true;
}
+static bool __maybe_unused its_enable_renesas_gen4(void *data)
+{
+ if (!of_machine_is_compatible("renesas,r8a779f0") &&
+ !of_machine_is_compatible("renesas,r8a779g0") &&
+ !of_machine_is_compatible("renesas,r8a779h0"))
+ return false;
+
+ gfp_flags_quirk |= GFP_DMA32;
+
+ return true;
+}
+
static const struct gic_quirk its_quirks[] = {
#ifdef CONFIG_CAVIUM_ERRATUM_22375
{
@@ -4975,6 +4987,14 @@ static const struct gic_quirk its_quirks[] = {
.mask = 0xffffffff,
.init = its_enable_rk3568002,
},
+#endif
+#ifdef CONFIG_RENESAS_ERRATUM_GEN4GICITS1
+ {
+ .desc = "ITS: Renesas R-Car Gen4 GIC600 32-bit limit",
+ .iidr = 0x0201743b,
+ .mask = 0xffffffff,
+ .init = its_enable_renesas_gen4,
+ },
#endif
{
}
--
2.53.0
^ permalink raw reply related
* [PATCH 3/3] arm64: dts: renesas: r8a779g0: Add GICv3 ITS and update PCIe nodes
From: Marek Vasut @ 2026-06-17 2:59 UTC (permalink / raw)
To: linux-pci
Cc: Marek Vasut, Yoshihiro Shimoda, Krzysztof Wilczyński,
Bjorn Helgaas, Catalin Marinas, Conor Dooley, Geert Uytterhoeven,
Krzysztof Kozlowski, Lorenzo Pieralisi, Manivannan Sadhasivam,
Marc Zyngier, Rob Herring, devicetree, linux-arm-kernel,
linux-doc, linux-kernel, linux-renesas-soc
In-Reply-To: <20260617030008.154449-1-marek.vasut+renesas@mailbox.org>
This SoC implements GIC600 with GICv3 ITS and PCIe host mode on this
SoC can use it. Add GIC ITS node into GIC node, update interrupt-map
and add msi-map into PCIe controller node.
The GIC ITS does have master interface to issue transactions to RAM.
The interface does support cacheable transactions, however, it does
not support shareable attribute, because the AXI port signals are tied
to inactive in this implementation. Therefore, add "dma-noncoherent"
DT property into the GIC ITS subnode.
The GIC redistributor does not have cacheable/shareable, therefore
add "dma-noncoherent" DT property into the GIC node.
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
NOTE: This would not be possible without prior work from Shimoda-san
https://lore.kernel.org/all/20240214052144.1966569-1-yoshihiro.shimoda.uh@renesas.com/
---
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
arch/arm64/boot/dts/renesas/r8a779g0.dtsi | 31 ++++++++++++++++-------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
index 3a8af825bb253..82e864acf2601 100644
--- a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
@@ -792,6 +792,7 @@ pciec0: pcie@e65d0000 {
resets = <&cpg 624>;
reset-names = "pwr";
max-link-speed = <4>;
+ msi-parent = <&its>;
num-lanes = <2>;
#address-cells = <3>;
#size-cells = <2>;
@@ -802,10 +803,10 @@ pciec0: pcie@e65d0000 {
dma-ranges = <0x42000000 0 0x00000000 0 0x00000000 1 0x00000000>;
#interrupt-cells = <1>;
interrupt-map-mask = <0 0 0 7>;
- interrupt-map = <0 0 0 1 &gic GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
- <0 0 0 2 &gic GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
- <0 0 0 3 &gic GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
- <0 0 0 4 &gic GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-map = <0 0 0 1 &gic 0 0 GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 2 &gic 0 0 GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 3 &gic 0 0 GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 4 &gic 0 0 GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>;
snps,enable-cdm-check;
status = "disabled";
@@ -839,6 +840,7 @@ pciec1: pcie@e65d8000 {
resets = <&cpg 625>;
reset-names = "pwr";
max-link-speed = <4>;
+ msi-parent = <&its>;
num-lanes = <2>;
#address-cells = <3>;
#size-cells = <2>;
@@ -849,10 +851,10 @@ pciec1: pcie@e65d8000 {
dma-ranges = <0x42000000 0 0x00000000 0 0x00000000 1 0x00000000>;
#interrupt-cells = <1>;
interrupt-map-mask = <0 0 0 7>;
- interrupt-map = <0 0 0 1 &gic GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
- <0 0 0 2 &gic GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
- <0 0 0 3 &gic GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
- <0 0 0 4 &gic GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-map = <0 0 0 1 &gic 0 0 GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 2 &gic 0 0 GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 3 &gic 0 0 GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 4 &gic 0 0 GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>;
snps,enable-cdm-check;
status = "disabled";
@@ -2131,11 +2133,22 @@ ipmmu_mm: iommu@eefc0000 {
gic: interrupt-controller@f1000000 {
compatible = "arm,gic-v3";
#interrupt-cells = <3>;
- #address-cells = <0>;
+ #address-cells = <2>;
+ #size-cells = <2>;
interrupt-controller;
reg = <0x0 0xf1000000 0 0x20000>,
<0x0 0xf1060000 0 0x110000>;
interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ dma-noncoherent;
+
+ ranges = <0x0 0x0 0x0 0xf1000000 0x0 0x200000>;
+
+ its: msi-controller@40000 {
+ compatible = "arm,gic-v3-its";
+ reg = <0x0 0x40000 0x0 0x20000>;
+ dma-noncoherent;
+ msi-controller;
+ };
};
gpu: gpu@fd000000 {
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v4 2/2] pwm: Add Nuvoton MA35D1 PWM controller support
From: sashiko-bot @ 2026-06-17 3:09 UTC (permalink / raw)
To: Chi-Wen Weng; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260617025925.2539334-3-cwweng.linux@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver unconditionally clobbers PWM waveform generator and control registers in probe(), breaking seamless handover for active PWM channels.
--
commit 1c5d12cc5f94bf9ab4e36bc34c306ce883661f7e
Author: Chi-Wen Weng <cwweng@nuvoton.com>
pwm: Add Nuvoton MA35D1 PWM controller support
Add a PWM framework driver for the Nuvoton MA35D1 PWM controller.
> diff --git a/drivers/pwm/pwm-ma35d1.c b/drivers/pwm/pwm-ma35d1.c
> new file mode 100644
> index 0000000000000..c07eedeca035b
> --- /dev/null
> +++ b/drivers/pwm/pwm-ma35d1.c
[ ... ]
> +static void nuvoton_pwm_init(struct nuvoton_pwm *nvtpwm)
> +{
> + u32 ctl1_mask = 0;
> + u32 wgctl0_mask = 0;
> + u32 wgctl0_val = 0;
> + u32 wgctl1_mask = 0;
> + u32 wgctl1_val = 0;
> + int ch;
> +
> + for (ch = 0; ch < MA35D1_PWM_NUM_CHANNELS; ch++) {
> + /* CNTTYPEn = 00: up counter type */
> + ctl1_mask |= nuvoton_pwm_ctl1_cnttype_mask(ch);
> +
> + /* CNTMODEn = 0: auto-reload mode */
> + ctl1_mask |= MA35D1_PWM_CTL1_CNTMODE_MASK(ch);
> +
> + /* ZPCTLn = 10: output high at zero point */
> + wgctl0_mask |= nuvoton_pwm_wgctl_zero_mask(ch);
> + wgctl0_val |= MA35D1_PWM_WGCTL_ZERO_HIGH(ch);
> +
> + /* PRDPCTLn = 00: do nothing at period point */
> + wgctl0_mask |= nuvoton_pwm_wgctl_period_mask(ch);
> +
> + /* CMPUCTLn = 01: output low at compare up point */
> + wgctl1_mask |= nuvoton_pwm_wgctl_cmp_up_mask(ch);
> + wgctl1_val |= MA35D1_PWM_WGCTL_CMP_UP_LOW(ch);
> +
> + /* CMPDCTLn = 00: do nothing at compare down point */
> + wgctl1_mask |= nuvoton_pwm_wgctl_cmp_down_mask(ch);
> + }
> +
> + for (ch = 0; ch < MA35D1_PWM_NUM_CHANNELS; ch += 2) {
> + /* OUTMODEn = 0: independent mode */
> + ctl1_mask |= MA35D1_PWM_CTL1_OUTMODE_MASK(ch);
> + }
> +
> + nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_CTL1, ctl1_mask, 0);
> + nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_WGCTL0,
> + wgctl0_mask, wgctl0_val);
> + nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_WGCTL1,
> + wgctl1_mask, wgctl1_val);
> +}
[ ... ]
> +static int nuvoton_pwm_probe(struct platform_device *pdev)
> +{
[ ... ]
> + if (nvtpwm->clkrate > NSEC_PER_SEC)
> + return dev_err_probe(dev, -EINVAL,
> + "PWM clock out of range (%lu)\n",
> + nvtpwm->clkrate);
> +
> + nuvoton_pwm_init(nvtpwm);
[Severity: Medium]
Will this unconditionally clobber the waveform generator and control
registers for all channels during probe()?
If the bootloader has already enabled a PWM channel (such as for a regulator
or display backlight) with a different waveform configuration, altering these
global control registers while the channel is running might corrupt its output.
Also, since nuvoton_pwm_get_state() relies on PERIOD and CMPDAT values,
altering their hardware interpretation here means the PWM core will inherit
an incorrect initial state.
Should the driver check if channels are already active via CNTEN and POEN
before modifying their configuration to allow for a seamless handover?
> +
> + chip->ops = &nuvoton_pwm_ops;
> + chip->atomic = true;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617025925.2539334-1-cwweng.linux@gmail.com?part=2
^ permalink raw reply
* [PATCH] dt-bindings: thermal: qcom-tsens: Document the maili Temperature Sensor
From: Haritha S K via B4 Relay @ 2026-06-17 4:34 UTC (permalink / raw)
To: Amit Kucheria, Thara Gopinath, Rafael J. Wysocki, Daniel Lezcano,
Zhang Rui, Lukasz Luba, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-pm, linux-arm-msm, devicetree, linux-kernel,
manaf.pallikunhi, gaurav.kohli, Haritha S K
From: Haritha S K <haritha.k@oss.qualcomm.com>
Document the Temperature Sensor (TSENS) on the Qualcomm maili SoC.
Signed-off-by: Haritha S K <haritha.k@oss.qualcomm.com>
---
Documentation/devicetree/bindings/thermal/qcom-tsens.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
index f0efaa8349ee..5a8f7673e730 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
@@ -58,6 +58,7 @@ properties:
- qcom,glymur-tsens
- qcom,hawi-tsens
- qcom,kaanapali-tsens
+ - qcom,maili-tsens
- qcom,milos-tsens
- qcom,nord-tsens
- qcom,msm8953-tsens
---
base-commit: a76326dca329a7a738ba367529868dfbafb4a403
change-id: 20260616-b4-maili-upstream-3-b198aaa8969f
Best regards,
--
Haritha S K <haritha.k@oss.qualcomm.com>
^ permalink raw reply related
* Re: [PATCH 1/2] dt-bindings: soc: qcom: Document CDSP Power Management
From: Krzysztof Kozlowski @ 2026-06-17 4:39 UTC (permalink / raw)
To: Vignesh Viswanathan, Bjorn Andersson
Cc: Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel
In-Reply-To: <3cbcffee-a2d1-494d-b4d4-378c2ae395e6@oss.qualcomm.com>
On 26/05/2026 09:57, Vignesh Viswanathan wrote:
>>
>>> +
>>> + vdd-mx-supply:
>>> + description:
>>> + Phandle to the MX voltage regulator. This is the actual hardware regulator
>>> + (e.g., from MP8899 PMIC) that supplies power to the CDSP MX rail. Optional
>>> + on boards where MX rail is always-on or not present.
>>> +
>>> + regulators:
>>> + type: object
>>> + description:
>>> + Virtual regulators provided by this driver for consumption by the CDSP
>>> + remoteproc driver. These virtual regulators pass through enable/disable
>>> + requests to the actual hardware regulators (vdd-cx-supply, vdd-mx-supply).
>>
>> These regulators doesn't exist in reality, they are only here because
>> you choose to split the description of your remoteproc implementation in
>> two.
>
> Yes, this is because the actual regulator can be controlled by two independent entities,
> the standard PAS driver and this CDSP driver when CDSP requests via the SMEM channel.
There is no such entity as PAS driver and CDSP driver. You are mixing
the hardware with Linux drivers.
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v7 0/6] Enable I2C on SA8255p Qualcomm platforms
From: Praveen Talari @ 2026-06-17 4:50 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Bjorn Andersson, Mukesh Kumar Savaliya,
Viken Dadhaniya
Cc: Mattijs Korpershoek, Praveen Talari, linux-arm-msm, linux-i2c,
devicetree, linux-kernel, bjorn.andersson, konrad.dybcio,
aniket.randive, chandana.chiluveru, prasad.sodagudi,
Krzysztof Kozlowski, Nikunj Kela
The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
The driver requests resources operations over SCMI using power
and performance protocols.
The SCMI power protocol enables or disables resources like clocks,
interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
such as resume/suspend, to control power states(on/off).
The SCMI performance protocol manages I2C frequency, with each
frequency rate represented by a performance level. The driver uses
geni_se_set_perf_opp() API to request the desired frequency rate..
As part of geni_se_set_perf_opp(), the OPP for the requested frequency
is obtained using dev_pm_opp_find_freq_floor() and the performance
level is set using dev_pm_opp_set_opp().
Tested-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
Changes in v7:
- Rebased all patches on latest tip
- Removed SOC related patches which were merged and available in
linux-next
https://lore.kernel.org/all/177950280353.1097700.8285469548487310751.b4-ty@kernel.org/
- Link to v6: https://lore.kernel.org/all/20260227061544.1785978-1-praveen.talari@oss.qualcomm.com
Changes in v4:
- Added a new patch(4/13) to handle core clk as part of
geni_se_clks_off/on().
To: Andi Shyti <andi.shyti@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Konrad Dybcio <konradybcio@kernel.org>
To: Bjorn Andersson <andersson@kernel.org>
To: Praveen Talari <praveen.talari@oss.qualcomm.com>
To: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
To: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-i2c@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: bjorn.andersson@oss.qualcomm.com
Cc: konrad.dybcio@oss.qualcomm.com
Cc: aniket.randive@oss.qualcomm.com
Cc: chandana.chiluveru@oss.qualcomm.com
Cc: prasad.sodagudi@oss.qualcomm.com
---
Praveen Talari (6):
dt-bindings: i2c: Describe SA8255p
i2c: qcom-geni: Isolate serial engine setup
i2c: qcom-geni: Move resource initialization to separate function
i2c: qcom-geni: Use resources helper APIs in runtime PM functions
i2c: qcom-geni: Store of_device_id data in driver private struct
i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms
.../bindings/i2c/qcom,sa8255p-geni-i2c.yaml | 64 ++++
drivers/i2c/busses/i2c-qcom-geni.c | 322 ++++++++++-----------
2 files changed, 220 insertions(+), 166 deletions(-)
---
base-commit: 8d6dbbbe3ba62de0a63e962ee004afb848c8e3ac
change-id: 20260616-enable-i2c-on-sa8255p-432b23838e18
Best regards,
--
Praveen Talari <praveen.talari@oss.qualcomm.com>
^ permalink raw reply
* [PATCH v7 1/6] dt-bindings: i2c: Describe SA8255p
From: Praveen Talari @ 2026-06-17 4:50 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Bjorn Andersson, Mukesh Kumar Savaliya,
Viken Dadhaniya
Cc: Mattijs Korpershoek, Praveen Talari, linux-arm-msm, linux-i2c,
devicetree, linux-kernel, bjorn.andersson, konrad.dybcio,
aniket.randive, chandana.chiluveru, prasad.sodagudi,
Krzysztof Kozlowski, Nikunj Kela
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com>
Add DT bindings for the QUP GENI I2C controller on sa8255p platforms.
SA8255p platform abstracts resources such as clocks, interconnect and
GPIO pins configuration in Firmware. SCMI power and perf protocol
are utilized to request resource configurations.
SA8255p platform does not require the Serial Engine (SE) common properties
as the SE firmware is loaded and managed by the TrustZone (TZ) secure
environment.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Co-developed-by: Nikunj Kela <quic_nkela@quicinc.com>
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
v2->v3:
- Added Reviewed-by tag
v1->v2:
Krzysztof:
- Added dma properties in example node
- Removed minItems from power-domains property
- Added in commit text about common property
---
.../bindings/i2c/qcom,sa8255p-geni-i2c.yaml | 64 ++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml b/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml
new file mode 100644
index 000000000000..a61e40b5cbc1
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/qcom,sa8255p-geni-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SA8255p QUP GENI I2C Controller
+
+maintainers:
+ - Praveen Talari <praveen.talari@oss.qualcomm.com>
+
+properties:
+ compatible:
+ const: qcom,sa8255p-geni-i2c
+
+ reg:
+ maxItems: 1
+
+ dmas:
+ maxItems: 2
+
+ dma-names:
+ items:
+ - const: tx
+ - const: rx
+
+ interrupts:
+ maxItems: 1
+
+ power-domains:
+ maxItems: 2
+
+ power-domain-names:
+ items:
+ - const: power
+ - const: perf
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - power-domains
+
+allOf:
+ - $ref: /schemas/i2c/i2c-controller.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/dma/qcom-gpi.h>
+
+ i2c@a90000 {
+ compatible = "qcom,sa8255p-geni-i2c";
+ reg = <0xa90000 0x4000>;
+ interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
+ dmas = <&gpi_dma0 0 0 QCOM_GPI_I2C>,
+ <&gpi_dma0 1 0 QCOM_GPI_I2C>;
+ dma-names = "tx", "rx";
+ power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
+ power-domain-names = "power", "perf";
+ };
+...
--
2.34.1
^ permalink raw reply related
* [PATCH v7 2/6] i2c: qcom-geni: Isolate serial engine setup
From: Praveen Talari @ 2026-06-17 4:50 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Bjorn Andersson, Mukesh Kumar Savaliya,
Viken Dadhaniya
Cc: Mattijs Korpershoek, Praveen Talari, linux-arm-msm, linux-i2c,
devicetree, linux-kernel, bjorn.andersson, konrad.dybcio,
aniket.randive, chandana.chiluveru, prasad.sodagudi
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com>
Moving the serial engine setup to geni_i2c_init() API for a cleaner
probe function and utilizes the PM runtime API to control resources
instead of direct clock-related APIs for better resource management.
Enables reusability of the serial engine initialization like
hibernation and deep sleep features where hardware context is lost.
Acked-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
v6->v7:
- Added Reviewed-by tag from konrad and mukesh.
v4->v5:
- Added Reviewed-by tag.
v3->v4:
viken:
- Added Acked-by tag
- Removed extra space before invoke of geni_i2c_init().
v1->v2:
Bjorn:
- Updated commit text.
---
drivers/i2c/busses/i2c-qcom-geni.c | 158 ++++++++++++++++++-------------------
1 file changed, 75 insertions(+), 83 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index d2f5055b0b10..3ee0be228d7c 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -990,10 +990,77 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
return ret;
}
+static int geni_i2c_init(struct geni_i2c_dev *gi2c)
+{
+ const struct geni_i2c_desc *desc = NULL;
+ u32 proto, tx_depth;
+ bool fifo_disable;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(gi2c->se.dev);
+ if (ret < 0) {
+ dev_err(gi2c->se.dev, "error turning on device :%d\n", ret);
+ return ret;
+ }
+
+ proto = geni_se_read_proto(&gi2c->se);
+ if (proto == GENI_SE_INVALID_PROTO) {
+ ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
+ if (ret) {
+ dev_err_probe(gi2c->se.dev, ret, "i2c firmware load failed ret: %d\n", ret);
+ goto err;
+ }
+ } else if (proto != GENI_SE_I2C) {
+ ret = dev_err_probe(gi2c->se.dev, -ENXIO, "Invalid proto %d\n", proto);
+ goto err;
+ }
+
+ desc = device_get_match_data(gi2c->se.dev);
+ if (desc && desc->no_dma_support) {
+ fifo_disable = false;
+ gi2c->no_dma = true;
+ } else {
+ fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
+ }
+
+ if (fifo_disable) {
+ /* FIFO is disabled, so we can only use GPI DMA */
+ gi2c->gpi_mode = true;
+ ret = setup_gpi_dma(gi2c);
+ if (ret)
+ goto err;
+
+ dev_dbg(gi2c->se.dev, "Using GPI DMA mode for I2C\n");
+ } else {
+ gi2c->gpi_mode = false;
+ tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
+
+ /* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
+ if (!tx_depth && desc)
+ tx_depth = desc->tx_fifo_depth;
+
+ if (!tx_depth) {
+ ret = dev_err_probe(gi2c->se.dev, -EINVAL,
+ "Invalid TX FIFO depth\n");
+ goto err;
+ }
+
+ gi2c->tx_wm = tx_depth - 1;
+ geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
+ geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
+ PACKING_BYTES_PW, true, true, true);
+
+ dev_dbg(gi2c->se.dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
+ }
+
+err:
+ pm_runtime_put(gi2c->se.dev);
+ return ret;
+}
+
static int geni_i2c_probe(struct platform_device *pdev)
{
struct geni_i2c_dev *gi2c;
- u32 proto, tx_depth, fifo_disable;
int ret;
struct device *dev = &pdev->dev;
const struct geni_i2c_desc *desc = NULL;
@@ -1073,101 +1140,26 @@ static int geni_i2c_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = clk_prepare_enable(gi2c->core_clk);
- if (ret)
- return ret;
-
- ret = geni_se_resources_on(&gi2c->se);
- if (ret) {
- dev_err_probe(dev, ret, "Error turning on resources\n");
- goto err_clk;
- }
- proto = geni_se_read_proto(&gi2c->se);
- if (proto == GENI_SE_INVALID_PROTO) {
- ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
- if (ret) {
- dev_err_probe(dev, ret, "i2c firmware load failed ret: %d\n", ret);
- goto err_resources;
- }
- } else if (proto != GENI_SE_I2C) {
- ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
- goto err_resources;
- }
-
- if (desc && desc->no_dma_support) {
- fifo_disable = false;
- gi2c->no_dma = true;
- } else {
- fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
- }
-
- if (fifo_disable) {
- /* FIFO is disabled, so we can only use GPI DMA */
- gi2c->gpi_mode = true;
- ret = setup_gpi_dma(gi2c);
- if (ret)
- goto err_resources;
-
- dev_dbg(dev, "Using GPI DMA mode for I2C\n");
- } else {
- gi2c->gpi_mode = false;
- tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
-
- /* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
- if (!tx_depth && desc)
- tx_depth = desc->tx_fifo_depth;
-
- if (!tx_depth) {
- ret = dev_err_probe(dev, -EINVAL,
- "Invalid TX FIFO depth\n");
- goto err_resources;
- }
-
- gi2c->tx_wm = tx_depth - 1;
- geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
- geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
- PACKING_BYTES_PW, true, true, true);
-
- dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
- }
-
- clk_disable_unprepare(gi2c->core_clk);
- ret = geni_se_resources_off(&gi2c->se);
- if (ret) {
- dev_err_probe(dev, ret, "Error turning off resources\n");
- goto err_dma;
- }
-
- ret = geni_icc_disable(&gi2c->se);
- if (ret)
- goto err_dma;
-
pm_runtime_set_suspended(gi2c->se.dev);
pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
pm_runtime_use_autosuspend(gi2c->se.dev);
pm_runtime_enable(gi2c->se.dev);
+ ret = geni_i2c_init(gi2c);
+ if (ret < 0) {
+ pm_runtime_disable(gi2c->se.dev);
+ return ret;
+ }
+
ret = i2c_add_adapter(&gi2c->adap);
if (ret) {
dev_err_probe(dev, ret, "Error adding i2c adapter\n");
pm_runtime_disable(gi2c->se.dev);
- goto err_dma;
+ return ret;
}
dev_dbg(dev, "Geni-I2C adaptor successfully added\n");
- return ret;
-
-err_resources:
- geni_se_resources_off(&gi2c->se);
-err_clk:
- clk_disable_unprepare(gi2c->core_clk);
-
- return ret;
-
-err_dma:
- release_gpi_dma(gi2c);
-
return ret;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v7 3/6] i2c: qcom-geni: Move resource initialization to separate function
From: Praveen Talari @ 2026-06-17 4:50 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Bjorn Andersson, Mukesh Kumar Savaliya,
Viken Dadhaniya
Cc: Mattijs Korpershoek, Praveen Talari, linux-arm-msm, linux-i2c,
devicetree, linux-kernel, bjorn.andersson, konrad.dybcio,
aniket.randive, chandana.chiluveru, prasad.sodagudi
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com>
Refactor the resource initialization in geni_i2c_probe() by introducing
a new geni_i2c_resources_init() function and utilizing the common
geni_se_resources_init() framework and clock frequency mapping, making the
probe function cleaner.
Acked-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
v4->v5:
- Added a Reviewed-by tag.
- Removed core clk, has_core_clk and icc_ddr variable.
v3->v4:
- Added Acked-by tag.
v1->v2:
- Updated commit text.
---
drivers/i2c/busses/i2c-qcom-geni.c | 64 ++++++++++++++------------------------
1 file changed, 24 insertions(+), 40 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 3ee0be228d7c..a7672b6c2bc0 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -107,7 +107,6 @@ struct geni_i2c_dev {
int cur_wr;
int cur_rd;
spinlock_t lock;
- struct clk *core_clk;
u32 clk_freq_out;
const struct geni_i2c_clk_fld *clk_fld;
void *dma_buf;
@@ -124,8 +123,6 @@ struct geni_i2c_dev {
};
struct geni_i2c_desc {
- bool has_core_clk;
- char *icc_ddr;
bool no_dma_support;
unsigned int tx_fifo_depth;
};
@@ -1058,6 +1055,23 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
return ret;
}
+static int geni_i2c_resources_init(struct geni_i2c_dev *gi2c)
+{
+ int ret;
+
+ ret = geni_se_resources_init(&gi2c->se);
+ if (ret)
+ return ret;
+
+ ret = geni_i2c_clk_map_idx(gi2c);
+ if (ret)
+ return dev_err_probe(gi2c->se.dev, ret, "Invalid clk frequency %d Hz\n",
+ gi2c->clk_freq_out);
+
+ return geni_icc_set_bw_ab(&gi2c->se, GENI_DEFAULT_BW, GENI_DEFAULT_BW,
+ Bps_to_icc(gi2c->clk_freq_out));
+}
+
static int geni_i2c_probe(struct platform_device *pdev)
{
struct geni_i2c_dev *gi2c;
@@ -1077,16 +1091,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
desc = device_get_match_data(&pdev->dev);
- if (desc && desc->has_core_clk) {
- gi2c->core_clk = devm_clk_get(dev, "core");
- if (IS_ERR(gi2c->core_clk))
- return PTR_ERR(gi2c->core_clk);
- }
-
- gi2c->se.clk = devm_clk_get(dev, "se");
- if (IS_ERR(gi2c->se.clk) && !has_acpi_companion(dev))
- return PTR_ERR(gi2c->se.clk);
-
ret = device_property_read_u32(dev, "clock-frequency",
&gi2c->clk_freq_out);
if (ret) {
@@ -1101,16 +1105,15 @@ static int geni_i2c_probe(struct platform_device *pdev)
if (gi2c->irq < 0)
return gi2c->irq;
- ret = geni_i2c_clk_map_idx(gi2c);
- if (ret)
- return dev_err_probe(dev, ret, "Invalid clk frequency %d Hz\n",
- gi2c->clk_freq_out);
-
gi2c->adap.algo = &geni_i2c_algo;
init_completion(&gi2c->done);
spin_lock_init(&gi2c->lock);
platform_set_drvdata(pdev, gi2c);
+ ret = geni_i2c_resources_init(gi2c);
+ if (ret)
+ return ret;
+
/* Keep interrupts disabled initially to allow for low-power modes */
ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN,
dev_name(dev), gi2c);
@@ -1123,23 +1126,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
gi2c->adap.dev.of_node = dev->of_node;
strscpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name));
- ret = geni_icc_get(&gi2c->se, desc ? desc->icc_ddr : "qup-memory");
- if (ret)
- return ret;
- /*
- * Set the bus quota for core and cpu to a reasonable value for
- * register access.
- * Set quota for DDR based on bus speed.
- */
- gi2c->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
- gi2c->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
- if (!desc || desc->icc_ddr)
- gi2c->se.icc_paths[GENI_TO_DDR].avg_bw = Bps_to_icc(gi2c->clk_freq_out);
-
- ret = geni_icc_set_bw(&gi2c->se);
- if (ret)
- return ret;
-
pm_runtime_set_suspended(gi2c->se.dev);
pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
pm_runtime_use_autosuspend(gi2c->se.dev);
@@ -1192,7 +1178,7 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
return ret;
}
- clk_disable_unprepare(gi2c->core_clk);
+ clk_disable_unprepare(gi2c->se.core_clk);
return geni_icc_disable(&gi2c->se);
}
@@ -1206,7 +1192,7 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
if (ret)
return ret;
- ret = clk_prepare_enable(gi2c->core_clk);
+ ret = clk_prepare_enable(gi2c->se.core_clk);
if (ret)
goto out_icc_disable;
@@ -1219,7 +1205,7 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
return 0;
out_clk_disable:
- clk_disable_unprepare(gi2c->core_clk);
+ clk_disable_unprepare(gi2c->se.core_clk);
out_icc_disable:
geni_icc_disable(&gi2c->se);
@@ -1260,8 +1246,6 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
};
static const struct geni_i2c_desc i2c_master_hub = {
- .has_core_clk = true,
- .icc_ddr = NULL,
.no_dma_support = true,
.tx_fifo_depth = 16,
};
--
2.34.1
^ permalink raw reply related
* [PATCH v7 4/6] i2c: qcom-geni: Use resources helper APIs in runtime PM functions
From: Praveen Talari @ 2026-06-17 4:50 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Bjorn Andersson, Mukesh Kumar Savaliya,
Viken Dadhaniya
Cc: Mattijs Korpershoek, Praveen Talari, linux-arm-msm, linux-i2c,
devicetree, linux-kernel, bjorn.andersson, konrad.dybcio,
aniket.randive, chandana.chiluveru, prasad.sodagudi
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com>
To manage GENI serial engine resources during runtime power management,
drivers currently need to call functions for ICC, clock, and
SE resource operations in both suspend and resume paths, resulting in
code duplication across drivers.
The new geni_se_resources_activate() and geni_se_resources_deactivate()
helper APIs addresses this issue by providing a streamlined method to
enable or disable all resources based, thereby eliminating redundancy
across drivers.
Acked-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
v5->v6:
Konrad:
- Updated last return value as "0".
- Added Reviewed-by
v3->v4:
- Added Acked-by tag.
v1->v2:
Bjorn:
- Remove geni_se_resources_state() API.
- Used geni_se_resources_activate() and geni_se_resources_deactivate()
to enable/disable resources.
---
drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index a7672b6c2bc0..8f8b74c4e88a 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -1172,15 +1172,14 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
disable_irq(gi2c->irq);
- ret = geni_se_resources_off(&gi2c->se);
+
+ ret = geni_se_resources_deactivate(&gi2c->se);
if (ret) {
enable_irq(gi2c->irq);
return ret;
}
- clk_disable_unprepare(gi2c->se.core_clk);
-
- return geni_icc_disable(&gi2c->se);
+ return 0;
}
static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
@@ -1188,28 +1187,13 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
int ret;
struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
- ret = geni_icc_enable(&gi2c->se);
+ ret = geni_se_resources_activate(&gi2c->se);
if (ret)
return ret;
- ret = clk_prepare_enable(gi2c->se.core_clk);
- if (ret)
- goto out_icc_disable;
-
- ret = geni_se_resources_on(&gi2c->se);
- if (ret)
- goto out_clk_disable;
-
enable_irq(gi2c->irq);
return 0;
-
-out_clk_disable:
- clk_disable_unprepare(gi2c->se.core_clk);
-out_icc_disable:
- geni_icc_disable(&gi2c->se);
-
- return ret;
}
static int __maybe_unused geni_i2c_suspend_noirq(struct device *dev)
--
2.34.1
^ permalink raw reply related
* [PATCH v7 5/6] i2c: qcom-geni: Store of_device_id data in driver private struct
From: Praveen Talari @ 2026-06-17 4:50 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Bjorn Andersson, Mukesh Kumar Savaliya,
Viken Dadhaniya
Cc: Mattijs Korpershoek, Praveen Talari, linux-arm-msm, linux-i2c,
devicetree, linux-kernel, bjorn.andersson, konrad.dybcio,
aniket.randive, chandana.chiluveru, prasad.sodagudi
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com>
To avoid repeatedly fetching and checking platform data across various
functions, store the struct of_device_id data directly in the i2c
private structure. This change enhances code maintainability and reduces
redundancy.
Acked-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
v6->v7
- Added Reviewed-by from konrad.
v5->v6
Konrad
- Updated in geni_i2c_acpi_match() as suggested
- Moved geni_i2c_acpi_match below to avoid errors of geni-i2c
v4->v5
Konrad
- Added a null check after platform data struct.
v3->v4
- Added Acked-by tag.
Konrad
- Removed icc_ddr from platfrom data struct
---
drivers/i2c/busses/i2c-qcom-geni.c | 46 ++++++++++++++++++++------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 8f8b74c4e88a..92cccb1cce69 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -77,6 +77,11 @@ enum geni_i2c_err_code {
#define XFER_TIMEOUT HZ
#define RST_TIMEOUT HZ
+struct geni_i2c_desc {
+ bool no_dma_support;
+ unsigned int tx_fifo_depth;
+};
+
#define QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC 2
/**
@@ -120,11 +125,7 @@ struct geni_i2c_dev {
bool is_tx_multi_desc_xfer;
u32 num_msgs;
struct geni_i2c_gpi_multi_desc_xfer i2c_multi_desc_config;
-};
-
-struct geni_i2c_desc {
- bool no_dma_support;
- unsigned int tx_fifo_depth;
+ const struct geni_i2c_desc *dev_data;
};
struct geni_i2c_err_log {
@@ -941,15 +942,6 @@ static const struct i2c_algorithm geni_i2c_algo = {
.functionality = geni_i2c_func,
};
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id geni_i2c_acpi_match[] = {
- { "QCOM0220"},
- { "QCOM0411" },
- { }
-};
-MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
-#endif
-
static void release_gpi_dma(struct geni_i2c_dev *gi2c)
{
if (gi2c->rx_c)
@@ -989,7 +981,6 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
static int geni_i2c_init(struct geni_i2c_dev *gi2c)
{
- const struct geni_i2c_desc *desc = NULL;
u32 proto, tx_depth;
bool fifo_disable;
int ret;
@@ -1012,8 +1003,7 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
goto err;
}
- desc = device_get_match_data(gi2c->se.dev);
- if (desc && desc->no_dma_support) {
+ if (gi2c->dev_data->no_dma_support) {
fifo_disable = false;
gi2c->no_dma = true;
} else {
@@ -1033,8 +1023,8 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
- if (!tx_depth && desc)
- tx_depth = desc->tx_fifo_depth;
+ if (!tx_depth && gi2c->se.core_clk)
+ tx_depth = gi2c->dev_data->tx_fifo_depth;
if (!tx_depth) {
ret = dev_err_probe(gi2c->se.dev, -EINVAL,
@@ -1077,7 +1067,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
struct geni_i2c_dev *gi2c;
int ret;
struct device *dev = &pdev->dev;
- const struct geni_i2c_desc *desc = NULL;
gi2c = devm_kzalloc(dev, sizeof(*gi2c), GFP_KERNEL);
if (!gi2c)
@@ -1089,7 +1078,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
if (IS_ERR(gi2c->se.base))
return PTR_ERR(gi2c->se.base);
- desc = device_get_match_data(&pdev->dev);
+ gi2c->dev_data = device_get_match_data(&pdev->dev);
+ if (!gi2c->dev_data)
+ return -EINVAL;
ret = device_property_read_u32(dev, "clock-frequency",
&gi2c->clk_freq_out);
@@ -1229,13 +1220,24 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
NULL)
};
+static const struct geni_i2c_desc geni_i2c = {};
+
static const struct geni_i2c_desc i2c_master_hub = {
.no_dma_support = true,
.tx_fifo_depth = 16,
};
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id geni_i2c_acpi_match[] = {
+ { "QCOM0220", (kernel_ulong_t)&geni_i2c},
+ { "QCOM0411", (kernel_ulong_t)&geni_i2c},
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
+#endif
+
static const struct of_device_id geni_i2c_dt_match[] = {
- { .compatible = "qcom,geni-i2c" },
+ { .compatible = "qcom,geni-i2c", .data = &geni_i2c },
{ .compatible = "qcom,geni-i2c-master-hub", .data = &i2c_master_hub },
{}
};
--
2.34.1
^ permalink raw reply related
* [PATCH v7 6/6] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms
From: Praveen Talari @ 2026-06-17 4:50 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Bjorn Andersson, Mukesh Kumar Savaliya,
Viken Dadhaniya
Cc: Mattijs Korpershoek, Praveen Talari, linux-arm-msm, linux-i2c,
devicetree, linux-kernel, bjorn.andersson, konrad.dybcio,
aniket.randive, chandana.chiluveru, prasad.sodagudi
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com>
The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
The driver requests resources operations over SCMI using power
and performance protocols.
The SCMI power protocol enables or disables resources like clocks,
interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
such as resume/suspend, to control power on/off.
The SCMI performance protocol manages I2C frequency, with each
frequency rate represented by a performance level. The driver uses
geni_se_set_perf_opp() API to request the desired frequency rate..
As part of geni_se_set_perf_opp(), the OPP for the requested frequency
is obtained using dev_pm_opp_find_freq_floor() and the performance
level is set using dev_pm_opp_set_opp().
Acked-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
v6->v7
- Added Reviewed-by from konrad.
v3->v4:
- Added Acked-by tag.
V1->v2:
From kernel test robot:
- Initialized ret to "0" in resume/suspend callbacks.
Bjorn:
- Used seperate APIs for the resouces enable/disable.
---
drivers/i2c/busses/i2c-qcom-geni.c | 56 ++++++++++++++++++++++++++++----------
1 file changed, 42 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 92cccb1cce69..96dbf04138be 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -80,6 +80,10 @@ enum geni_i2c_err_code {
struct geni_i2c_desc {
bool no_dma_support;
unsigned int tx_fifo_depth;
+ int (*resources_init)(struct geni_se *se);
+ int (*set_rate)(struct geni_se *se, unsigned long freq);
+ int (*power_on)(struct geni_se *se);
+ int (*power_off)(struct geni_se *se);
};
#define QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC 2
@@ -200,8 +204,9 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
return -EINVAL;
}
-static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
+static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
{
+ struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
u32 val;
@@ -214,6 +219,7 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
val |= itr->t_low_cnt << LOW_COUNTER_SHFT;
val |= itr->t_cycle_cnt;
writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
+ return 0;
}
static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
@@ -919,7 +925,9 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
return ret;
}
- qcom_geni_i2c_conf(gi2c);
+ ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
+ if (ret)
+ return ret;
if (gi2c->gpi_mode)
ret = geni_i2c_gpi_xfer(gi2c, msgs, num);
@@ -1045,8 +1053,9 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
return ret;
}
-static int geni_i2c_resources_init(struct geni_i2c_dev *gi2c)
+static int geni_i2c_resources_init(struct geni_se *se)
{
+ struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
int ret;
ret = geni_se_resources_init(&gi2c->se);
@@ -1101,7 +1110,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
spin_lock_init(&gi2c->lock);
platform_set_drvdata(pdev, gi2c);
- ret = geni_i2c_resources_init(gi2c);
+ ret = gi2c->dev_data->resources_init(&gi2c->se);
if (ret)
return ret;
@@ -1159,15 +1168,17 @@ static void geni_i2c_shutdown(struct platform_device *pdev)
static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
{
- int ret;
+ int ret = 0;
struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
disable_irq(gi2c->irq);
- ret = geni_se_resources_deactivate(&gi2c->se);
- if (ret) {
- enable_irq(gi2c->irq);
- return ret;
+ if (gi2c->dev_data->power_off) {
+ ret = gi2c->dev_data->power_off(&gi2c->se);
+ if (ret) {
+ enable_irq(gi2c->irq);
+ return ret;
+ }
}
return 0;
@@ -1175,12 +1186,14 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
{
- int ret;
+ int ret = 0;
struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
- ret = geni_se_resources_activate(&gi2c->se);
- if (ret)
- return ret;
+ if (gi2c->dev_data->power_on) {
+ ret = gi2c->dev_data->power_on(&gi2c->se);
+ if (ret)
+ return ret;
+ }
enable_irq(gi2c->irq);
@@ -1220,11 +1233,25 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
NULL)
};
-static const struct geni_i2c_desc geni_i2c = {};
+static const struct geni_i2c_desc geni_i2c = {
+ .resources_init = geni_i2c_resources_init,
+ .set_rate = qcom_geni_i2c_conf,
+ .power_on = geni_se_resources_activate,
+ .power_off = geni_se_resources_deactivate,
+};
static const struct geni_i2c_desc i2c_master_hub = {
.no_dma_support = true,
.tx_fifo_depth = 16,
+ .resources_init = geni_i2c_resources_init,
+ .set_rate = qcom_geni_i2c_conf,
+ .power_on = geni_se_resources_activate,
+ .power_off = geni_se_resources_deactivate,
+};
+
+static const struct geni_i2c_desc sa8255p_geni_i2c = {
+ .resources_init = geni_se_domain_attach,
+ .set_rate = geni_se_set_perf_opp,
};
#ifdef CONFIG_ACPI
@@ -1239,6 +1266,7 @@ MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
static const struct of_device_id geni_i2c_dt_match[] = {
{ .compatible = "qcom,geni-i2c", .data = &geni_i2c },
{ .compatible = "qcom,geni-i2c-master-hub", .data = &i2c_master_hub },
+ { .compatible = "qcom,sa8255p-geni-i2c", .data = &sa8255p_geni_i2c },
{}
};
MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
--
2.34.1
^ permalink raw reply related
* RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
From: Selvamani Rajagopal @ 2026-06-17 4:54 UTC (permalink / raw)
To: Parthiban.Veerasooran@microchip.com, andrew+netdev@lunn.ch,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, robh@kernel.org, krzk+dt@kernel.org,
conor+dt@kernel.org, Piergiorgio Beruto
Cc: andrew@lunn.ch, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Conor.Dooley@microchip.com,
devicetree@vger.kernel.org
In-Reply-To: <7c89df6b-32ac-46c8-8400-945879037f2e@microchip.com>
> Subject: Re: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level
> triggered.
>
>
> Hi Selvamani,
>
> I did a quick test by connecting Mikroe LAN8651 Click to a Raspberry Pi
> 4 and shared the feedback below. Please let me know if you need any
> further details.
Parthiban,
Thanks for testing this.
Though the NULL pointer reference after skb_put is a clue, I am working with our team to see we can see this crash in our setup.
Will keep you updated.
>
> [ 8276.691064] eth1: Receive buffer overflow error
> [ 8281.662600] Unable to handle kernel NULL pointer dereference at
> virtual address 0000000000000074> drm_panel_orientation_quirks backlight nfnetlink
> [ 8281.839427] pc : skb_put+0x14/0x80
> [ 8281.842864] lr : oa_tc6_macphy_threaded_irq+0x428/0x880 [lan865x_t1s]
^ permalink raw reply
* Re: [PATCH v7 2/6] i2c: qcom-geni: Isolate serial engine setup
From: sashiko-bot @ 2026-06-17 5:00 UTC (permalink / raw)
To: Praveen Talari; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-2-ad736dbeab57@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Asynchronous pm_runtime_put() followed immediately by pm_runtime_disable() leaves hardware permanently powered on during probe failures.
- [Medium] DMA channels allocated during hardware initialization are permanently leaked if I2C adapter registration fails.
--
commit 111d823f58de8c30c14ab44aa8466c66a5e8d761
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
i2c: qcom-geni: Isolate serial engine setup
Moving the serial engine setup to geni_i2c_init() API for a cleaner
probe function and utilizes the PM runtime API to control resources
instead of direct clock-related APIs for better resource management.
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index d2f5055b0b108..3ee0be228d7c1 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -990,10 +990,77 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
> return ret;
> }
>
> +static int geni_i2c_init(struct geni_i2c_dev *gi2c)
> +{
> + const struct geni_i2c_desc *desc = NULL;
> + u32 proto, tx_depth;
> + bool fifo_disable;
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(gi2c->se.dev);
[ ... ]
> + if (fifo_disable) {
> + /* FIFO is disabled, so we can only use GPI DMA */
> + gi2c->gpi_mode = true;
> + ret = setup_gpi_dma(gi2c);
> + if (ret)
> + goto err;
[ ... ]
> +err:
> + pm_runtime_put(gi2c->se.dev);
> + return ret;
> +}
> +
> static int geni_i2c_probe(struct platform_device *pdev)
> {
[ ... ]
> pm_runtime_set_suspended(gi2c->se.dev);
> pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
> pm_runtime_use_autosuspend(gi2c->se.dev);
> pm_runtime_enable(gi2c->se.dev);
>
> + ret = geni_i2c_init(gi2c);
> + if (ret < 0) {
> + pm_runtime_disable(gi2c->se.dev);
[Severity: Medium]
If geni_i2c_init() fails, it drops its runtime PM reference using the
asynchronous pm_runtime_put(). Will calling pm_runtime_disable() right
afterwards cancel the pending suspend work, leaving the device in the
RPM_ACTIVE state and leaking power resources?
> + return ret;
> + }
> +
> ret = i2c_add_adapter(&gi2c->adap);
> if (ret) {
> dev_err_probe(dev, ret, "Error adding i2c adapter\n");
> pm_runtime_disable(gi2c->se.dev);
> - goto err_dma;
> + return ret;
[Severity: Medium]
Does this code leak the DMA channels if i2c_add_adapter() fails?
Before this patch, a failure here would jump to err_dma which called
release_gpi_dma(gi2c). Now it returns directly, but geni_i2c_init()
may have successfully allocated the DMA channels via setup_gpi_dma().
Additionally, similar to the check above, will pm_runtime_disable() cancel
the async suspend from the successful geni_i2c_init() path, leaving the
hardware powered on?
> }
>
> dev_dbg(dev, "Geni-I2C adaptor successfully added\n");
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=2
^ permalink raw reply
* Re: [PATCH v7 5/6] i2c: qcom-geni: Store of_device_id data in driver private struct
From: sashiko-bot @ 2026-06-17 5:02 UTC (permalink / raw)
To: Praveen Talari; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-5-ad736dbeab57@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Accessing a non-existent member `core_clk` in `struct geni_se` causes a build failure and alters fallback logic.
- [High] DMA channels are permanently leaked if `i2c_add_adapter()` fails during probe.
Pre-existing issues:
- [High] Multi-descriptor DMA buffer arrays are leaked on early transfer errors or partial allocation failures.
--
commit 1f9fcc815478494b91c04d32de4b9e7efa6fac42
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
i2c: qcom-geni: Store of_device_id data in driver private struct
This commit updates the driver to store the struct of_device_id data
directly in the i2c private structure. The goal is to improve code
maintainability and avoid repeatedly fetching and checking platform
data across different functions.
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 8f8b74c4e88a4..92cccb1cce698 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
[ ... ]
> @@ -1033,8 +1023,8 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
> tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>
> /* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
> - if (!tx_depth && desc)
> - tx_depth = desc->tx_fifo_depth;
> + if (!tx_depth && gi2c->se.core_clk)
> + tx_depth = gi2c->dev_data->tx_fifo_depth;
[Severity: High]
Does struct geni_se have a core_clk member? Looking at geni-se.h, it appears
this will cause a build failure since core_clk doesn't exist.
Additionally, if this is changed to gi2c->se.clk, will it alter the original
fallback logic? Previously, if a master-hub device lacked this clock, it
correctly fell back to using desc->tx_fifo_depth. With this change, if the
clock is missing, tx_depth might remain 0 and cause a probe failure.
[ ... ]
> @@ -1089,7 +1078,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
> if (IS_ERR(gi2c->se.base))
> return PTR_ERR(gi2c->se.base);
>
> - desc = device_get_match_data(&pdev->dev);
> + gi2c->dev_data = device_get_match_data(&pdev->dev);
> + if (!gi2c->dev_data)
> + return -EINVAL;
[Severity: High]
During the refactoring in this patch series, did we lose the cleanup for
DMA channels in geni_i2c_probe()?
If i2c_add_adapter() fails after geni_i2c_init() has successfully requested
GPI DMA channels via setup_gpi_dma(), release_gpi_dma() is not called:
geni_i2c_probe() {
...
ret = geni_i2c_init(gi2c);
if (ret < 0) { ... }
ret = i2c_add_adapter(&gi2c->adap);
if (ret) {
dev_err_probe(dev, ret, "Error adding i2c adapter\n");
pm_runtime_disable(gi2c->se.dev);
return ret;
}
...
}
Are tx_c and rx_c permanently leaked here on failure?
[ ... ]
[Severity: High]
This is a pre-existing issue, but do multi-descriptor DMA buffer arrays
leak on early transfer errors or partial allocation failures?
In geni_i2c_gpi_xfer(), if the second allocation fails, it jumps to the
error label:
geni_i2c_gpi_xfer() {
...
tx_multi_xfer->dma_buf = kcalloc(gi2c->num_msgs, ...);
tx_multi_xfer->dma_addr = kcalloc(gi2c->num_msgs, ...);
if (!tx_multi_xfer->dma_buf || !tx_multi_xfer->dma_addr) {
ret = -ENOMEM;
goto err;
}
...
err:
geni_i2c_gpi_multi_desc_unmap(gi2c, msgs, &i);
}
Inside geni_i2c_gpi_multi_desc_unmap(), the arrays are only freed if
the loop reaches the end:
geni_i2c_gpi_multi_desc_unmap() {
...
if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs - 1) {
kfree(tx_multi_xfer->dma_buf);
kfree(tx_multi_xfer->dma_addr);
break;
}
...
}
During an early abort or partial allocation failure, the arrays are not
freed. Should they be cleaned up unconditionally?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=5
^ permalink raw reply
* Re: [PATCH v7 1/6] dt-bindings: i2c: Describe SA8255p
From: sashiko-bot @ 2026-06-17 5:04 UTC (permalink / raw)
To: Praveen Talari; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-1-ad736dbeab57@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incomplete schema requirements for power domains.
- [Low] Missing `#address-cells` and `#size-cells` in the example block.
--
commit 028881c69d7286ec4ecd4e0f30436fe9b7b8594c
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
dt-bindings: i2c: Describe SA8255p
Add DT bindings for the QUP GENI I2C controller on sa8255p platforms.
The platform abstracts resources in firmware via SCMI and uses TZ for
Serial Engine management, omitting standard SE properties.
> diff --git a/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml b/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml
[ ... ]
> +properties:
> + compatible:
> + const: qcom,sa8255p-geni-i2c
[ ... ]
> + power-domains:
> + maxItems: 2
[Severity: Medium]
Since the SA8255p platform requires both the power and perf domains to
function properly, should this also specify minItems: 2? Currently,
maxItems: 2 implicitly defaults to minItems: 1, allowing a configuration
with missing domains to pass schema validation.
> +
> + power-domain-names:
> + items:
> + - const: power
> + - const: perf
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - power-domains
[Severity: Medium]
Should power-domain-names also be included in the required block?
Without it, an incomplete device tree node could pass schema validation but
fail at runtime when the driver attempts to request the missing domains by
name.
> +
> +allOf:
> + - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/dma/qcom-gpi.h>
> +
> + i2c@a90000 {
> + compatible = "qcom,sa8255p-geni-i2c";
[Severity: Low]
Does this example need #address-cells and #size-cells properties?
Because the schema inherits from i2c-controller.yaml, an I2C controller
node is inherently treated as a bus requiring #address-cells = <1> and
#size-cells = <0>. Omitting these might trigger a dt_binding_check schema
validation error.
> + reg = <0xa90000 0x4000>;
> + interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=1
^ permalink raw reply
* Re: [PATCH v7 3/6] i2c: qcom-geni: Move resource initialization to separate function
From: sashiko-bot @ 2026-06-17 5:05 UTC (permalink / raw)
To: Praveen Talari; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-3-ad736dbeab57@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Critical] Build failure due to undefined GENI SE resource initialization and PM functions
- [High] GPI DMA channels are permanently leaked on probe failure.
- [High] Asynchronous pm_runtime_put() is canceled by pm_runtime_disable() on probe error paths, leaking power.
Pre-existing issues:
- [High] Missing synchronous suspend before disabling runtime PM during device removal.
--
commit 3d26b1870da68dc53ac527a03242c49f148d835f
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
i2c: qcom-geni: Move resource initialization to separate function
Refactor the resource initialization in geni_i2c_probe() by introducing
a new geni_i2c_resources_init() function and utilizing the common
geni_se_resources_init() framework and clock frequency mapping, making the
probe function cleaner.
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 3ee0be228d7c1..a7672b6c2bc07 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
[ ... ]
> @@ -1058,6 +1055,23 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
> return ret;
> }
>
> +static int geni_i2c_resources_init(struct geni_i2c_dev *gi2c)
> +{
> + int ret;
> +
> + ret = geni_se_resources_init(&gi2c->se);
[Severity: Critical]
Are we sure this function is available in the current tree? It appears
geni_se_resources_init() is undefined, which might cause a build failure.
> + if (ret)
> + return ret;
> +
> + ret = geni_i2c_clk_map_idx(gi2c);
> + if (ret)
> + return dev_err_probe(gi2c->se.dev, ret, "Invalid clk frequency %d Hz\n",
> + gi2c->clk_freq_out);
> +
> + return geni_icc_set_bw_ab(&gi2c->se, GENI_DEFAULT_BW, GENI_DEFAULT_BW,
> + Bps_to_icc(gi2c->clk_freq_out));
[Severity: Critical]
Is geni_icc_set_bw_ab() defined? Similar to the call above, it looks like this
is an unresolved symbol that could break the build.
> +}
> +
> static int geni_i2c_probe(struct platform_device *pdev)
> {
> struct geni_i2c_dev *gi2c;
[ ... ]
> @@ -1101,16 +1105,15 @@ static int geni_i2c_probe(struct platform_device *pdev)
> if (gi2c->irq < 0)
> return gi2c->irq;
>
> - ret = geni_i2c_clk_map_idx(gi2c);
> - if (ret)
> - return dev_err_probe(dev, ret, "Invalid clk frequency %d Hz\n",
> - gi2c->clk_freq_out);
> -
> gi2c->adap.algo = &geni_i2c_algo;
> init_completion(&gi2c->done);
> spin_lock_init(&gi2c->lock);
> platform_set_drvdata(pdev, gi2c);
>
> + ret = geni_i2c_resources_init(gi2c);
> + if (ret)
> + return ret;
> +
[Severity: High]
Does this probe function properly clean up resources if it fails later on?
If the subsequent call to i2c_add_adapter() fails later in geni_i2c_probe(),
the function returns without freeing the DMA channels that were allocated
during geni_i2c_init() via setup_gpi_dma(). Could this permanently leak the
GPI DMA channels?
[Severity: High]
Is there a risk of leaking power during probe error handling?
If geni_i2c_init() or i2c_add_adapter() fails, geni_i2c_probe() calls
pm_runtime_disable() immediately. However, geni_i2c_init() queues an async
pm_runtime_put() in its own error path. Since pm_runtime_disable() cancels
any pending async suspend work, does this leave the hardware indefinitely
powered on in the RPM_ACTIVE state?
[ ... ]
> @@ -1260,8 +1246,6 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
> };
>
> static const struct geni_i2c_desc i2c_master_hub = {
> - .has_core_clk = true,
> - .icc_ddr = NULL,
> .no_dma_support = true,
> .tx_fifo_depth = 16,
> };
[Severity: Critical]
Does this patch series introduce undefined power management functions here?
It appears the i2c_master_hub struct is assigned geni_se_resources_activate
and geni_se_resources_deactivate for its power management fields, but these
functions do not seem to be defined anywhere in the kernel tree.
[Severity: High]
This is a pre-existing issue, but does geni_i2c_remove() properly quiesce
the device before tearing down power management?
The geni_i2c_remove() function calls pm_runtime_disable() without a prior
synchronization barrier. If an asynchronous suspend is pending from a
recently completed I2C transaction, the immediate pm_runtime_disable() could
cancel it. Should a call to pm_runtime_put_sync() or
pm_runtime_force_suspend() be added to ensure it is fully suspended?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=3
^ permalink raw reply
* Re: [PATCH v7 4/6] i2c: qcom-geni: Use resources helper APIs in runtime PM functions
From: sashiko-bot @ 2026-06-17 5:06 UTC (permalink / raw)
To: Praveen Talari; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-4-ad736dbeab57@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [Low] The patch calls undefined helper functions `geni_se_resources_activate`, `geni_se_resources_deactivate`, and `geni_se_resources_init`, leading to implicit function declarations and link failures.
Pre-existing issues:
- [Critical] Stale `gi2c->dma_buf` pointer causes use-after-free and double-free on DMA mapping failure.
- [High] Pending asynchronous runtime suspend is cancelled by `pm_runtime_disable()`, permanently leaking hardware resources (clocks, interconnects).
- [High] Memory leak of multi-descriptor DMA arrays on transfer error.
- [High] The system suspend callback executes a sleeping function while interrupts are disabled, causing a BUG.
--
commit 495d087d0c5bbbc5ad2b355b642cfdc5ae9439b6
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
i2c: qcom-geni: Use resources helper APIs in runtime PM functions
To manage GENI serial engine resources during runtime power management,
drivers currently need to call functions for ICC, clock, and
SE resource operations in both suspend and resume paths, resulting in
code duplication across drivers.
The new geni_se_resources_activate() and geni_se_resources_deactivate()
helper APIs addresses this issue by providing a streamlined method to
enable or disable all resources based, thereby eliminating redundancy
across drivers.
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index a7672b6c2bc07..8f8b74c4e88a4 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -1172,15 +1172,14 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
> struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>
> disable_irq(gi2c->irq);
[Severity: High]
This isn't a bug introduced by this patch, but does calling disable_irq() here
cause a BUG when suspending the system?
Looking at the suspend paths, geni_i2c_suspend_noirq() runs in the noirq
phase where local interrupts are globally disabled. It calls
pm_runtime_force_suspend(), which invokes this geni_i2c_runtime_suspend()
function.
Calling disable_irq() explicitly invokes might_sleep(), which triggers a
"BUG: sleeping function called from invalid context" when called with
interrupts disabled.
> - ret = geni_se_resources_off(&gi2c->se);
> + ret = geni_se_resources_deactivate(&gi2c->se);
[Severity: Low]
Are the implementations for geni_se_resources_deactivate() and
geni_se_resources_activate() missing from this patch series?
These functions appear to be completely missing from the kernel tree at this
revision. Does this lead to implicit function declarations and build failures?
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
geni_i2c_probe() and the driver removal path in geni_i2c_remove()
permanently leak hardware resources?
In geni_i2c_init(), an asynchronous pm_runtime_put() is called. If an error
occurs later in geni_i2c_probe(), or during geni_i2c_remove(), the code
immediately calls pm_runtime_disable():
drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_probe() {
...
pm_runtime_disable(gi2c->se.dev);
...
}
Calling pm_runtime_disable() invokes __pm_runtime_barrier(), which cancels
any pending asynchronous suspend requests instead of waiting for them to
complete. Since the suspend is cancelled and the device remains in the
RPM_ACTIVE state after the driver unbinds, do its clocks and power domains
leak indefinitely?
[Severity: Critical]
This is a pre-existing issue, but can a stale dma_buf pointer cause a
use-after-free and double-free if DMA mapping fails?
If geni_se_rx_dma_prep() fails in geni_i2c_rx_one_msg(), the local dma_buf
is freed and set to NULL, but the device struct's gi2c->dma_buf retains the
stale pointer from a previous successful transfer:
drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_rx_one_msg() {
...
if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) {
geni_se_select_mode(se, GENI_SE_FIFO);
i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
dma_buf = NULL;
} else {
...
}
During cleanup, the stale pointer is evaluated as true:
drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_rx_msg_cleanup() {
...
if (gi2c->dma_buf) {
if (gi2c->err)
geni_i2c_rx_fsm_rst(gi2c);
geni_se_rx_dma_unprep(&gi2c->se, gi2c->dma_addr, gi2c->xfer_len);
i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
}
...
}
Does this unmap the stale DMA address, copy data from the freed bounce buffer,
and then double-free the kalloc'd bounce buffer?
[Severity: High]
This is a pre-existing issue, but is there a memory leak of the
multi-descriptor DMA arrays when a transfer error occurs?
In geni_i2c_gpi_multi_desc_unmap(), the arrays are only freed if
unmap_msg_cnt reaches gi2c->num_msgs - 1:
drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_gpi_multi_desc_unmap() {
...
if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs - 1) {
kfree(tx_multi_xfer->dma_buf);
kfree(tx_multi_xfer->dma_addr);
break;
}
...
}
If an error occurs during the multi-message transfer, msg_xfer_cnt is less
than num_msgs, so the loop terminates early. Does this bypass the kfree()
calls and leak both tracking arrays, potentially allowing an attacker to
cause an Out-Of-Memory condition?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=4
^ permalink raw reply
* Re: [PATCH v7 6/6] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms
From: sashiko-bot @ 2026-06-17 5:06 UTC (permalink / raw)
To: Praveen Talari; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260617-enable-i2c-on-sa8255p-v7-6-ad736dbeab57@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Uninitialized `gi2c->clk_fld` pointer triggers a deterministic NULL pointer dereference in `geni_i2c_gpi_xfer` for the SA8255p platform.
- [Medium] An early error return from the new `set_rate()` callback in `geni_i2c_xfer()` irreversibly leaks a PM runtime usage counter.
- [Low] The newly added SA8255p descriptor references undeclared functions `geni_se_domain_attach` and `geni_se_set_perf_opp`, causing a hard build failure.
--
commit c33b9259904d2d29eb39c245dff8c21c943e3fb1
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms
The Qualcomm automotive SA8255p SoC relies on firmware to configure platform
resources. This adds support for SA8255p by using SCMI power and performance
protocols to manage resources like clocks and interconnects.
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 92cccb1cce698..96dbf04138bee 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
[ ... ]
> @@ -919,7 +925,9 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
> return ret;
> }
>
> - qcom_geni_i2c_conf(gi2c);
> + ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
> + if (ret)
> + return ret;
[Severity: Medium]
Does this early return leak the PM runtime reference?
Earlier in geni_i2c_xfer(), the driver increments the PM usage counter via
pm_runtime_get_sync(gi2c->se.dev). If set_rate() returns an error, this
returns immediately without calling pm_runtime_put_autosuspend() to release
the reference.
[ ... ]
> @@ -1220,11 +1233,25 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
> NULL)
> };
[ ... ]
> +static const struct geni_i2c_desc sa8255p_geni_i2c = {
> + .resources_init = geni_se_domain_attach,
> + .set_rate = geni_se_set_perf_opp,
> +};
[Severity: Low]
Are the functions geni_se_domain_attach and geni_se_set_perf_opp missing
declarations? They do not appear to be defined anywhere or declared in the
included headers, which causes a build failure.
[Severity: High]
Does bypassing geni_i2c_resources_init() here lead to a NULL pointer
dereference in GPI mode?
For the qcom,sa8255p-geni-i2c compatible, resources_init is set to
geni_se_domain_attach instead of geni_i2c_resources_init. This means
geni_i2c_clk_map_idx() is never called to initialize gi2c->clk_fld.
Since no_dma_support is false for SA8255p, if the hardware register indicates
the FIFO is disabled, the driver will fall back to GPI DMA mode.
In geni_i2c_gpi_xfer(), the uninitialized gi2c->clk_fld pointer is
unconditionally dereferenced:
const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
...
peripheral.cycle_count = itr->t_cycle_cnt;
Could this cause a panic during transfer if GPI mode is used?
>
> #ifdef CONFIG_ACPI
> @@ -1239,6 +1266,7 @@ MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
> static const struct of_device_id geni_i2c_dt_match[] = {
> { .compatible = "qcom,geni-i2c", .data = &geni_i2c },
> { .compatible = "qcom,geni-i2c-master-hub", .data = &i2c_master_hub },
> + { .compatible = "qcom,sa8255p-geni-i2c", .data = &sa8255p_geni_i2c },
> {}
> };
> MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=6
^ permalink raw reply
* RE: [PATCH v32 5/5] i2c: ast2600: Add target mode support
From: Ryan Chen @ 2026-06-17 5:27 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev
Cc: robh@kernel.org, conor+dt@kernel.org, devicetree@vger.kernel.org
In-Reply-To: <20260611054645.3FEEF1F00893@smtp.kernel.org>
Hello,
> Subject: Re: [PATCH v32 5/5] i2c: ast2600: Add target mode support
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to
> consider:
> - [High] Incorrect chronological ordering of I2C slave events and spurious event
> signaling in the target interrupt handler breaks backend state machines.
> - [High] Controller mode can be permanently locked out because
> `target_active` is not reliably cleared upon target transaction STOP.
> - [High] Race condition in the target mode master-abort fast path leaves stale
> controller interrupts pending, leading to use-after-free and data corruption of
> subsequent I2C transfers.
> - [High] Failure to W1C-clear target ISR error bits causes a permanent
> deadlock where all subsequent controller transfers fail with -EBUSY.
> --
>
> commit e5417daca64552c41b25f31b14fb3eb562d3e134
> Author: Ryan Chen <ryan_chen@aspeedtech.com>
>
> i2c: ast2600: Add target mode support
>
> Add target mode support to the AST2600 I2C driver, enabling dual-role
> capability. This includes adding target interrupt handling and address match
> response logic.
>
> > diff --git a/drivers/i2c/busses/i2c-ast2600.c
> > b/drivers/i2c/busses/i2c-ast2600.c
> > index 70f37dc77468a..aace0c7eeb721 100644
> > --- a/drivers/i2c/busses/i2c-ast2600.c
> > +++ b/drivers/i2c/busses/i2c-ast2600.c
> > @@ -348,6 +353,253 @@ static int ast2600_i2c_recover_bus(struct
> > ast2600_i2c_bus *i2c_bus)
> [ ... ]
> > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > +static void ast2600_i2c_target_packet_buff_irq(struct ast2600_i2c_bus
> > +*i2c_bus, u32 sts) {
> > + u8 value = 0;
> > + int target_rx_len = 0;
> > + u32 cmd = 0;
> > + int i;
> > +
> > + /* due to controller target is common buffer, need force the master stop
> not issue */
> > + if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & GENMASK(15,
> 0)) {
> > + writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> > + writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> > + writel(readl(i2c_bus->reg_base + AST2600_I2CM_ISR),
> > + i2c_bus->reg_base + AST2600_I2CM_ISR);
> > + i2c_bus->cmd_err = -EBUSY;
> > + WRITE_ONCE(i2c_bus->msgs, NULL);
> > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> > + i2c_bus->reg_base + AST2600_I2CM_IER);
> > + complete(&i2c_bus->cmd_complete);
> > + }
>
> [Severity: High]
> Could this fast path abort leave stale controller interrupts pending?
>
> If the hardware asynchronously asserts PKT_DONE or PKT_ERROR shortly after
> the W1C ISR read here, the complete() call wakes the waiting thread, which
> might immediately start a new controller transfer.
>
> Would the stale interrupt then fire and incorrectly apply to the new transfer,
> potentially leading to a use-after-free on the new msgs buffer?
The abort path disables the controller IER, W1C-clears I2CM ISR, and
sets i2c_bus->msgs = NULL before complete(). Per the datasheet the controller ISR
is not re-asserted asynchronously once cleared, so the next transfer starts with I2CM_ISR == 0.
Even if a late IRQ were delivered, the controller handler bails out immediately because
msgs is NULL, so it cannot touch a subsequent transfer's buffer. There is no use-after-free.
>
> > +
> > + /* Handle i2c target timeout condition */
> > + if (sts & AST2600_I2CS_INACTIVE_TO) {
> > + /* Reset timeout counter */
> > + u32 ac_timing = readl(i2c_bus->reg_base +
> AST2600_I2CC_AC_TIMING) &
> > + AST2600_I2CC_AC_TIMING_MASK;
> > +
> > + writel(ac_timing, i2c_bus->reg_base + AST2600_I2CC_AC_TIMING);
> > + ac_timing |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout);
> > + writel(ac_timing, i2c_bus->reg_base + AST2600_I2CC_AC_TIMING);
> > + writel(TARGET_TRIGGER_CMD, i2c_bus->reg_base +
> AST2600_I2CS_CMD_STS);
> > + writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_ISR);
> > + i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
> > + WRITE_ONCE(i2c_bus->target_active, false);
> > + return;
> > + }
>
> [Severity: High]
> Does this early return fail to clear the AST2600_I2CS_INACTIVE_TO bit from
> the hardware register?
>
> Since it is never W1C-cleared, the ISR register might remain non-zero
> permanently.
Per I2CS24[16], in packet mode clearing PKT_DONE also clears bits 0, 2, [5:4],
7, 15 and 17 — and bit 15 is INACTIVE_TO.
The handler does writel(I2CS_PKT_DONE, I2CS_ISR) on the timeout path,
which clears INACTIVE_TO along with PKT_DONE...
>
> > +
> > + sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
> > +
> > + if (sts & AST2600_I2CS_SLAVE_MATCH)
> > + WRITE_ONCE(i2c_bus->target_active, true);
> > +
> > + switch (sts) {
> > + case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
> > + AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_STOP:
> > + case AST2600_I2CS_SLAVE_PENDING |
> > + AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_STOP:
> > + case AST2600_I2CS_SLAVE_PENDING |
> > + AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
> > + i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
> > + fallthrough;
>
> [Severity: High]
> Is the event ordering inverted here?
>
> This unconditionally emits I2C_SLAVE_STOP first, and then falls through to emit
> I2C_SLAVE_WRITE_REQUESTED and I2C_SLAVE_WRITE_RECEIVED.
>
> Emitting the STOP condition prior to receiving the write data leaves backend
> state machines (like i2c-slave-eeprom) without a terminating event for the
> data, which typically causes writes to be dropped.
The cases in question all carry both SLAVE_PENDING and SLAVE_MATCH ... I2CS24[29]
(SLAVE_PENDING) is read-only and means "a previous slave-receive has not been
serviced yet while a new address match is already pending".
So this single interrupt spans two back-to-back transactions:
the STOP belongs to the previous one, while SLAVE_MATCH and the following data
belong to the new one...
>
> [ ... ]
> > + case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_TX_DMA |
> AST2600_I2CS_RX_DONE:
> > + case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
> > + case AST2600_I2CS_WAIT_TX_DMA:
> > + if (sts & AST2600_I2CS_SLAVE_MATCH)
> > + i2c_slave_event(i2c_bus->target,
> I2C_SLAVE_WRITE_REQUESTED,
> > +&value);
>
> [Severity: High]
> Can this spuriously emit I2C_SLAVE_WRITE_REQUESTED during a repeated
> start transitioning to a Master Read (WAIT_TX_DMA)?
>
> If so, it would reset the backend's internal write pointers, corrupting previously
> received write data before I2C_SLAVE_READ_REQUESTED is processed.
The related WAIT_TX_DMA concern is the same: WRITE_REQUESTED is only
emitted when SLAVE_MATCH is also set (a fresh address match),
which is the offset-write phase of a combined register-read.
A plain repeated-start-to-read (no SLAVE_MATCH) does not enter that emit path,
so the backend write pointer is not reset.
>
> [ ... ]
> > + if (cmd)
> > + writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> > +
> > + writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_ISR);
> > + readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
> > +
> > + if ((sts & AST2600_I2CS_STOP) && !(sts &
> AST2600_I2CS_SLAVE_PENDING))
> > + WRITE_ONCE(i2c_bus->target_active, false); }
>
> [Severity: High]
> Is this condition too strict?
>
> For standard target write transactions, the hardware explicitly combines
> AST2600_I2CS_STOP and AST2600_I2CS_SLAVE_PENDING in the same
> interrupt event.
>
> If this evaluates to false in those normal cases, target_active would remain
> permanently true, locking out all subsequent controller mode transfers with
> -EBUSY.
This is intentional. STOP combined with SLAVE_PENDING means the previous
transaction ended but a new back-to-back transaction is already pending,
so the target is still in use and must stay active. A standalone target transaction ends
with STOP without SLAVE_PENDING and is cleared normally.
The timeout path (INACTIVE_TO) also clears target_active explicitly...
>
> > @@ -682,6 +954,21 @@ static int ast2600_i2c_controller_xfer(struct
> i2c_adapter *adap, struct i2c_msg
> > return ret;
> > }
> >
> > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > + if (READ_ONCE(i2c_bus->target_active))
> > + return -EBUSY;
> > + /*
> > + * Controller and target share the same buffer register. A target
> > + * transaction can update buffer state asynchronously via IRQ, so block
> > + * controller transfers while target is active to avoid buffer corruption.
> > + */
> > + writel(0, i2c_bus->reg_base + AST2600_I2CS_IER);
> > + if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR) ||
> READ_ONCE(i2c_bus->target_active)) {
> > + writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_IER);
> > + return -EBUSY;
> > + }
> > +#endif
>
> [Severity: High]
> If AST2600_I2CS_INACTIVE_TO is left uncleared in the target interrupt handler,
> will this check permanently return -EBUSY for all subsequent controller
> transfers?
>
Consequently the readl(I2CS_ISR) interlock at the top of the controller xfer path
does not see a stale INACTIVE_TO, so it does not return -EBUSY for subsequent transfers.
This check itself is intentional and correct: controller and target share the same buffer
registers, so we must confirm no unserviced target interrupt is pending before starting
a controller transfer.
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/20260611-upstream_i2c-v32-0-b66eba921d01
> @aspeedtech.com?part=5
^ permalink raw reply
* RE: [PATCH v2 3/5] pinctrl: samsung: Add Exynos8855 pinctrl configuration
From: Alim Akhtar @ 2026-06-17 5:38 UTC (permalink / raw)
To: 'Peter Griffin'
Cc: krzk, robh, conor+dt, linusw, linux-samsung-soc, linux-kernel,
devicetree, linux-gpio, hajun.sung
In-Reply-To: <CADrjBPpyNymPXtYgdeOGtp1KDkdp9gTrBAEcJR89B+wwF8uUXw@mail.gmail.com>
> -----Original Message-----
> From: Peter Griffin <peter.griffin@linaro.org>
> Sent: Monday, June 15, 2026 7:45 PM
> To: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: krzk@kernel.org; robh@kernel.org; conor+dt@kernel.org;
> linusw@kernel.org; linux-samsung-soc@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> gpio@vger.kernel.org; hajun.sung@samsung.com
> Subject: Re: [PATCH v2 3/5] pinctrl: samsung: Add Exynos8855 pinctrl
> configuration
>
> Hi Alim,
>
> Thanks for your patch. It's great to see more Exynos SoCs being supported!
>
Thanks Peter, more patches to follow.
> On Mon, 15 Jun 2026 at 09:34, Alim Akhtar <alim.akhtar@samsung.com>
> wrote:
> >
> > Add pinctrl configuration for Exynos8855. The bank type macros are
> > reused from Exynos850 SoC.
> >
> > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> > ---
> > .../pinctrl/samsung/pinctrl-exynos-arm64.c | 123 ++++++++++++++++++
> > drivers/pinctrl/samsung/pinctrl-samsung.c | 2 +
> > drivers/pinctrl/samsung/pinctrl-samsung.h | 1 +
> > 3 files changed, 126 insertions(+)
> >
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > index fe9f92cb037e..db120ae4d847 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > @@ -943,6 +943,129 @@ const struct samsung_pinctrl_of_match_data
> exynos850_of_data __initconst = {
> > .num_ctrl = ARRAY_SIZE(exynos850_pin_ctrl),
> > };
> >
>
> Are you sure you want to use E850 pinctrl macros and not the GS101 ones?
> The GS101 macros allow the fltcon offset to be specified, which I think is
> required for all Exynos9 (including e850 SoC). Youngmin sent a series
> previously https://lore.kernel.org/lkml/20251202093613.852109-1-
> youngmin.nam@samsung.com/
> fixing up some of this but it hasn't been re-spun in a while. In particular this
> patch https://lore.kernel.org/lkml/20251202093613.852109-4-
> youngmin.nam@samsung.com/.
>
Thanks for pointing out, let me re-look into this, according will change.
> > +/* pin banks of exynos8855 pin-controller 0 (ALIVE) */ static const
> > +struct samsung_pin_bank_data exynos8855_pin_banks0[] __initconst = {
> > + /* Must start with EINTG banks, ordered by EINT group number. */
[Snip]
> > +static const struct samsung_pin_ctrl exynos8855_pin_ctrl[] __initconst = {
> > + {
> > + /* pin-controller instance 0 ALIVE data */
> > + .pin_banks = exynos8855_pin_banks0,
> > + .nr_banks = ARRAY_SIZE(exynos8855_pin_banks0),
> > + .eint_wkup_init = exynos_eint_wkup_init,
> > + .eint_gpio_init = exynos_eint_gpio_init,
> > + }, {
>
> With fltcon_offset specified, you could then use
> gs101_pinctrl_suspend/gs101_pinctrl_resume callbacks here.
>
Let me cross check with UM and will add accordingly.
^ permalink raw reply
* RE: [PATCH v2 4/5] arm64: dts: exynos: add initial support for Samsung Exynos8855 smdk
From: Alim Akhtar @ 2026-06-17 5:41 UTC (permalink / raw)
To: 'Peter Griffin'
Cc: krzk, robh, conor+dt, linusw, linux-samsung-soc, linux-kernel,
devicetree, linux-gpio, hajun.sung
In-Reply-To: <CADrjBPqS=d2Q02UhdpkSxHJ-RYe-hvurB-1meurTOLoUcUidFQ@mail.gmail.com>
Hi Peter
> -----Original Message-----
> From: Peter Griffin <peter.griffin@linaro.org>
> Sent: Monday, June 15, 2026 8:07 PM
> To: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: krzk@kernel.org; robh@kernel.org; conor+dt@kernel.org;
> linusw@kernel.org; linux-samsung-soc@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> gpio@vger.kernel.org; hajun.sung@samsung.com
> Subject: Re: [PATCH v2 4/5] arm64: dts: exynos: add initial support for
> Samsung Exynos8855 smdk
>
> Hi Alim,
>
> On Mon, 15 Jun 2026 at 09:34, Alim Akhtar <alim.akhtar@samsung.com>
> wrote:
> >
> > Add initial devicetree support for Samsung smdk board using
> > Exynos8855 SoC.
>
> I think it would be worthwhile adding a more verbose description of the
> Exynos8855 SoC in the commit message e.g. a brief list of the major IPs on
> the SoC.
>
Sure will add more details here
> >
> > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> [..]
> > diff --git a/arch/arm64/boot/dts/exynos/exynos8855-smdk.dts
> > b/arch/arm64/boot/dts/exynos/exynos8855-smdk.dts
> > new file mode 100644
> > index 000000000000..f5132bcaa47c
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/exynos/exynos8855-smdk.dts
> > @@ -0,0 +1,32 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Samsung Exynos8855 SMDK board device tree source
> > + *
> > + * Copyright (C) 2026 Samsung Electronics Co., Ltd.
> > + *
> > + * Device tree source file for WinLink's E850-96 board which is based
> > +on
> > + * Samsung Exynos8855 SoC.
>
> E850-96 isn't based on the Exynos8855 SoC. I guess it's leftover from a
> copy/paste.
>
Ah! My bad, will remove these, Thanks
> regards,
>
> Peter
>
>
^ permalink raw reply
* Re: [PATCH 3/7] hwmon: adm1275: Support ROHM BD12780
From: Matti Vaittinen @ 2026-06-17 5:48 UTC (permalink / raw)
To: Guenter Roeck, Matti Vaittinen, Matti Vaittinen
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Shuah Khan, Wensheng Wang, Ashish Yadav, Kim Seer Paller,
Cedric Encarnacion, Chris Packham, Yuxi Wang, Charles Hsu,
ChiShih Tsai, linux-hwmon, devicetree, linux-kernel, linux-doc
In-Reply-To: <67bec6ae-4f21-441a-8c5e-b56bd1a2ab84@roeck-us.net>
Thanks for taking the time to review this! Feedback is appreciated :)
On 16/06/2026 17:13, Guenter Roeck wrote:
> On 6/15/26 23:36, Matti Vaittinen wrote:
>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ROHM BD12780 and BD12780A are hot-swap controllers. They are largely
>> similar to Analog Devices ADM1278. Besides the ID registers and some
>> added functionality, the BD12780 and BD12780A mark PMON_CONFIG bits
>> [15:14] as reserved. Hence TSFILT setting must be omitted on these ICs.
>>
>> The BD12780 has 3 pins usable for configuring the I2C address. The
>> BD12780A lists the ADDR3-pin as "not connect".
>>
>> Support ROHM BD12780 and BD12780A controllers.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>> drivers/hwmon/pmbus/Kconfig | 2 +-
>> drivers/hwmon/pmbus/adm1275.c | 46 +++++++++++++++++++++++++++++------
>> 2 files changed, 39 insertions(+), 9 deletions(-)
>>
// snip
>> @@ -487,6 +489,21 @@ static const struct i2c_device_id adm1275_id[] = {
>> { "adm1281", adm1281 },
>> { "adm1293", adm1293 },
>> { "adm1294", adm1294 },
>> + /*
>> + * The BD12780a is functionally identical to BD12780(*). Even the
>> pmbus ID
>> + * register contents are same. When instantiated from the DT, it
>> is required
>> + * to have the bd12780 as a fall-back. We still need the bd12780a
>> ID here,
>> + * because the i2c_device_id is created from the first
>> compatible, not from
>> + * the fall-back entry.
>> + * (*)Until proven to differ. I prefer having own compatible for
>> these
>> + * variants for that day. Please note that even though the probe
>> is called
>> + * based on the 'bd12780a' -entry, the ID is picked at probe
>> based on the
>> + * pmbus register contents and not by DT entry. Thus, if the
>> bd12780 and
>> + * bd12780a are found to require different handling, then this
>> needs to be
>> + * changed, or bd12780a is handled as bd12780.
>> + */
>> + { "bd12780", bd12780 },
>> + { "bd12780a", /* driver data unused, see --^ */ },
>
> We don't usually do that. There are various A/B/C variants for many chips,
> and we just use the base name unless a difference is warranted. Either this
> is needed, and driver data is needed as well, or it isn't. If it is not
> needed,
> it should be dropped.
At the moment the only difference I know is reduced amount of I2C slave
addresses. This shouldn't be visible to this driver.
My problem is that I don't know for sure if we later notice something
that requires differentiating. Thus I would like to have different DT
compatibles (or other source of I2C IDs if those are used to instantiate
the driver). If we don't do this, then we have problems if we later find
out that these ICs require different handling as users because we can't
differentiate these ICs if they are described with same compatible/I2C ID.
The "fun" thing is that both variants have exactly same MFR_MODEL and
MFR_REVISION register contents. Thus, these ICs can't be differentiated
by reading PMBus registers.
This is also why the driver data entry gets unused. The existing probe
mechanism in this driver, scans the names in this ID list and compares
it to the PMBus MFR_MODEL, and then picks-up the driver data to use. Now
because BD12780 and BD12780A both have same MFR_MODEL, the scan in probe
will always pick the same driver data entry, no matter what ID was
matched by bus code. That's why I added the comment here.
If I drop the { "bd12780a", /* driver data unused, see --^ */ } -entry
from the ID list and add the of_device_ids, then I think this problem is
solved for the DT-platforms. As far as I understand, this would still
cause any non DT platform to describe both variants as "bd12780", making
it impossible to later differentiate ICs in the driver, right? I can do
this, but for me it feels a bit like asking for problems...
My thinking was to have different IDs for these variants so hardware
description could have different IDs for different ICs. Then, if we
later need to differentiate these ICs, we still have an option to change
the probe to trust the i2c_get_match_data() when PMBus indicates the
bd12780.
This however is some extra complexity, and I would like to add it to the
probe only if it really is required.
But yeah, having an ID entry in the list and driver data not used even
when the matching IC is found, is unusual and would have caught me off
guard. Hence I added the (long) comment.
>
>> { "mc09c", sq24905c },
>> { }
>> };
// snip
>> @@ -712,7 +732,16 @@ static int adm1275_probe(struct i2c_client *client)
>> break;
>> case adm1278:
>> case adm1281:
>> + case bd12780:
>> case sq24905c:
>> + {
>> + u16 defconfig;
>> +
>> + if (data->id == bd12780)
>> + defconfig = BD12780_PMON_DEFCONFIG;
>> + else
>> + defconfig = ADM1278_PMON_DEFCONFIG;
>> +
>
> Please add a separate case statement for the new chip
> and do not overload existing chip data.
I originally did just that. I, however, was not happy with this as it
resulted quite long own case this IC, which is almost identical to this
other (adm1278, adm1281, sq24905c) case. I wanted the code to shout that
the DB12780 is indeed (almost) identical to the adm1278. But yeah, I
agree, having "if (foo)" in "switch (foo)" case can get confusing.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox