* Re: [PATCH 6/7] hwmon: adm1275: Support ROHM BD12790
From: Matti Vaittinen @ 2026-06-17 5:56 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: <e1673147-a51f-460c-ad09-21f54d3d6803@roeck-us.net>
On 16/06/2026 17:08, Guenter Roeck wrote:
> On 6/15/26 23:44, Matti Vaittinen wrote:
>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> Add support for ROHM BD12790 hot-swap controller which is largely
>> similar to Analog Devices adm1272.
>>
>> The BD12790 uses the same selectable 60V/100V voltage ranges and
>> 15mV/30mV current-sense ranges as the ADM1272, and the same VRANGE
>> (bit 5) and IRANGE (bit 0) layout in PMON_CONFIG. It therefore uses
>> a dedicated coefficient table that mirrors adm1272_coefficients, with
>> the following differences derived from BD12790 datasheet Table 1 (p.18):
>> - power 60V/30mV: m=17560 (vs. 17561)
>> - power 100V/30mV: m=10536 (vs. 10535)
>> - temperature: b=31880 (vs. 31871, reflecting T[11:0] = 4.2*T + 3188)
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> Assisted-by: GitHub Copilot:claude-sonnet-4.6
>>
>> ---
>> Originally this patch was AI-generated. I did pretty much re-write the
>> probe changes by hand, and also fixed some of the coefficient math
>> afterwards :/ But yeah, this one was AI "assisted". :)
>>
>> drivers/hwmon/pmbus/Kconfig | 4 +--
>> drivers/hwmon/pmbus/adm1275.c | 53 +++++++++++++++++++++++++++++------
>> 2 files changed, 47 insertions(+), 10 deletions(-)
// snip
>> @@ -655,12 +681,23 @@ static int adm1275_probe(struct i2c_client *client)
>> break;
>> case adm1272:
>> case adm1273:
>> + case bd12790:
>
> Please don't overload the existing case statements.
> Just add separate case statements for the new chips.
>
Hmm. Ok, although, here, same as with the BD12780, I would like the code
to clearly show that the BD12790 is very very similar to another IC
(adm1272). When we have own case for it, this information gets kind of
lost as these cases are a tad too long to easily spot the differences.
If there are any ideas how to ease spotting this while having own cases
- I am keen to hear.
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
* RE: [PATCH v6 3/3] PCI: imx6: Add root port reset to support link recovery
From: Hongxing Zhu (OSS) @ 2026-06-17 5:49 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev, Hongxing Zhu (OSS)
Cc: devicetree@vger.kernel.org, imx@lists.linux.dev, robh@kernel.org,
conor+dt@kernel.org, Frank.Li@kernel.org,
linux-pci@vger.kernel.org
In-Reply-To: <20260603070533.641461F00893@smtp.kernel.org>
> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Wednesday, June 3, 2026 3:06 PM
> To: Hongxing Zhu (OSS) <hongxing.zhu@oss.nxp.com>
> Cc: devicetree@vger.kernel.org; imx@lists.linux.dev; robh@kernel.org;
> conor+dt@kernel.org; Frank.Li@kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH v6 3/3] PCI: imx6: Add root port reset to support link recovery
>
> Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to
> consider:
> - [High] Read-modify-write operations on mixed RW/W1C register silently drop
> interrupts.
> - [High] Shared interrupt handler checks status but fails to verify if the interrupt is
> enabled.
> - [High] Ignored return value of `dw_pcie_wait_for_link()` hides downstream link
> recovery failures.
> - [High] Lockless traversal of PCI bus devices in an asynchronous IRQ thread
> exposes the kernel to Use-After-Free and list corruption.
> - [High] Link down interrupt is not disabled during system shutdown, causing an
> AXI bus fault and system hang.
> - [High] Failed root port reset leaves the controller unclocked while still active,
> causing a crash on subsequent PM suspend.
> - [High] Unprotected hardware indirect register access during root port reset.
> - [Medium] Ignored negative error codes from
> `platform_get_irq_byname_optional()` break probe deferral.
> - [Low] Build failure due to undefined functions and struct members.
> --
>
> commit 5bee25b25210b0bc28349bc8474d10204a25cd72
> Author: Richard Zhu <hongxing.zhu@nxp.com>
>
> PCI: imx6: Add root port reset to support link recovery
>
> The PCIe link can go down due to various unexpected circumstances. Add root
> port reset support to enable link recovery for the i.MX PCIe controller when the
> optional "intr" interrupt is present.
>
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 773ab65b2afac..aa5c90be23df8 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
>
> [ ... ]
>
> > @@ -1394,6 +1401,13 @@ static int imx_pcie_host_init(struct dw_pcie_rp
> > *pp)
> >
> > imx_setup_phy_mpll(imx_pcie);
> >
> > + /*
> > + * Callback invoked by PCI core when link down is detected and
> > + * recovery is needed.
> > + */
> > + if (pp->bridge)
> > + pp->bridge->reset_root_port = imx_pcie_reset_root_port;
>
> [Severity: Low]
> Does the reset_root_port member actually exist in struct pci_host_bridge?
> This appears to cause a build failure since the struct member is undefined.
>
[Richard] the `reset_root_port` member and `pci_host_handle_link_down()`
function is introduced in a prerequisite patch that will be merged before
this one.
> [ ... ]
>
> > @@ -1661,6 +1675,9 @@ static int imx_pcie_suspend_noirq(struct device
> *dev)
> > if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND))
> > return 0;
> >
> > + if (imx_pcie->lnk_intr > 0)
> > + regmap_clear_bits(imx_pcie->iomuxc_gpr,
> IMX95_LINK_INT_CTRL_STS,
> > + IMX95_LINK_DOWN_INT_EN);
>
> [Severity: High]
> Since IMX95_LINK_INT_CTRL_STS mixes an RW enable bit and a write-1-to-clear
> status bit, will using regmap_clear_bits perform a read-modify-write cycle that
> inadvertently clears pending status bits if they are latched during the read?
>
[Richard] This is acceptable in the suspend path. Even if a pending interrupt
status bit is inadvertently cleared during the regmap_clear_bits() operation,
the link would be down at the end of suspend_noirq(), and retrained during
resume_noirq(). Same to the contents of link recovery.
> [ ... ]
>
> > +static irqreturn_t imx_pcie_lnk_irq_isr(int irq, void *priv) {
> > + struct imx_pcie *imx_pcie = priv;
> > + struct dw_pcie *pci = imx_pcie->pci;
> > + struct device *dev = pci->dev;
> > + u32 val;
> > +
> > + regmap_read(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS, &val);
> > + if (val & IMX95_LINK_DOWN_INT_STS) {
>
> [Severity: High]
> Is it possible for this shared interrupt handler to be invoked while the interrupt is
> disabled, see the latched status bit, and erroneously return IRQ_WAKE_THREAD?
>
> If a PME interrupt fires while the link down interrupt is intentionally disabled, the
> ISR will still claim it. Would it be safer to check if IMX95_LINK_DOWN_INT_EN is
> also set before proceeding?
[Richard]Good catch. I will add a check for IMX95_LINK_DOWN_INT_EN to ensure
the handler only processes the interrupt when it is enabled. This makes
the code more robust against spurious shared interrupt events.
>
> > + dev_dbg(dev, "PCIe link down detected, initiating recovery\n");
> > + /* Clear link down interrupt status by writing 1b'1 to it */
> > + regmap_set_bits(imx_pcie->iomuxc_gpr,
> IMX95_LINK_INT_CTRL_STS,
> > + IMX95_LINK_DOWN_INT_STS);
>
> [Severity: High]
> Similar to the regmap_clear_bits call above, could using regmap_set_bits here
> perform a read-modify-write cycle that inadvertently clears other pending write-
> 1-to-clear bits in the same register?
>
[Richard] This is safe here. The status bit is cleared immediately after being
read in the ISR, so there are no other pending W1C bits in this register that
could be inadvertently cleared by the read-modify-write operation.
> > + regmap_clear_bits(imx_pcie->iomuxc_gpr,
> IMX95_LINK_INT_CTRL_STS,
> > + IMX95_LINK_DOWN_INT_EN);
> > +
> > + return IRQ_WAKE_THREAD;
> > + }
> > +
> > + return IRQ_NONE;
> > +}
> > +
> > +static irqreturn_t imx_pcie_lnk_irq_thread(int irq, void *priv) {
> > + struct imx_pcie *imx_pcie = priv;
> > + struct dw_pcie *pci = imx_pcie->pci;
> > + struct dw_pcie_rp *pp = &pci->pp;
> > + struct pci_dev *port;
> > +
> > + for_each_pci_bridge(port, pp->bridge->bus)
>
> [Severity: High]
> Is it safe to iterate the bus list locklessly in this IRQ thread?
> Could concurrent modifications to the PCI bus list cause a use-after-free since the
> pci_bus_sem is not held?
>
[Richard] The PCI bus structure is stable at this point because devices are
only added/removed during enumeration or hotplug events, both of which hold
the appropriate locks. The threaded IRQ handler does not race with these
operations, so lockless traversal is safe here.
> > + if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> > + pci_host_handle_link_down(port);
>
> [Severity: Low]
> Does the pci_host_handle_link_down function exist in the tree? This appears to
> be an undefined function call which will cause a build failure.
>
> > +
> > + regmap_set_bits(imx_pcie->iomuxc_gpr, IMX95_LINK_INT_CTRL_STS,
> > + IMX95_LINK_DOWN_INT_EN);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int imx_pcie_reset_root_port(struct pci_host_bridge *bridge,
> > + struct pci_dev *pdev)
> > +{
> > + struct pci_bus *bus = bridge->bus;
> > + struct dw_pcie_rp *pp = bus->sysdata;
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> > + int ret;
> > +
> > + imx_pcie_msi_save_restore(imx_pcie, true);
> > + if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_LUT))
> > + imx_pcie_lut_save(imx_pcie);
>
> [Severity: High]
> The i.MX95 PCIe hardware uses an indirect access mechanism for LUTs. Since this
> reset executes asynchronously in a threaded IRQ, should imx_pcie->lock be
> acquired before calling imx_pcie_lut_save to prevent concurrent operations from
> clobbering the shared indirect address indices?
>
[Richard] The threaded IRQ handler is serialized by the kernel's IRQ subsystem,
so concurrent invocations of the same handler are not possible. Additionally,
no other code path accesses the LUT registers during link recovery, so there
is no race condition here.
> > + imx_pcie_stop_link(pci);
> > + imx_pcie_host_exit(pp);
> > +
> > + ret = imx_pcie_host_init(pp);
> > + if (ret) {
> > + dev_err(pci->dev, "Failed to re-init PCIe\n");
> > + return ret;
> > + }
> > + ret = dw_pcie_setup_rc(pp);
> > + if (ret)
> > + goto err_host_deinit;
> > +
> > + imx_pcie_start_link(pci);
> > + dw_pcie_wait_for_link(pci);
>
> [Severity: High]
> Should the return value of dw_pcie_wait_for_link be checked here?
> By ignoring it, the function might falsely report success to the PCI core even if
> downstream link recovery fails, which could lead to unsupported request aborts
> when accessing downstream devices.
>
[Richard] The return value of dw_pcie_wait_for_link() is intentionally not
checked here. Link recovery is best-effort; even if the link does not come
back up immediately, the MSI and LUT state has been restored, and the system
remains stable. Subsequent link training attempts may succeed.
> > +
> > + if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_LUT))
> > + imx_pcie_lut_restore(imx_pcie);
> > + imx_pcie_msi_save_restore(imx_pcie, false);
> > +
> > + dev_dbg(pci->dev, "Root port reset completed\n");
> > + return 0;
> > +
> > +err_host_deinit:
> > + imx_pcie_host_exit(pp);
>
> [Severity: High]
> If the re-initialization fails and imx_pcie_host_exit disables the clocks, the device
> still remains bound. If the system later initiates a suspend, will
> imx_pcie_suspend_noirq execute unconditionally and trigger a fatal external
> abort by attempting to read from the unclocked DBI registers?
[Richard] You're right that this is a potential issue. If re-initialization
fails, the clocks are disabled but the device remains bound. Because that
host_deinit and host_init are already symmetrical, the host_deinit shouldn't
be in the error exit path again. Would leave the clocks enabled on failure
to maintain a safe state.
>
> > +
> > + return ret;
> > +}
> > +
>
> [ ... ]
>
> > @@ -1919,9 +2018,33 @@ static int imx_pcie_probe(struct platform_device
> *pdev)
> > val |= PCI_MSI_FLAGS_ENABLE;
> > dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS, val);
> > }
> > +
> > + /* Get link event irq if it is present */
> > + imx_pcie->lnk_intr = platform_get_irq_byname_optional(pdev,
> "intr");
> > + if (imx_pcie->lnk_intr > 0) {
>
> [Severity: Medium]
> If the interrupt controller is not yet ready, the platform irq function returns -
> EPROBE_DEFER. Does checking strictly for > 0 cause the driver to silently ignore
> this error code and fail to defer the probe, permanently disabling the link recovery
> functionality?
>
> [ ... ]
[Richard] Good point. I will update the code to explicitly handle -EPROBE_DEFER:
imx_pcie->lnk_intr = platform_get_irq_byname_optional(pdev, "intr");
if (imx_pcie->lnk_intr == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (imx_pcie->lnk_intr > 0) {
/* setup interrupt handler */
}
>
> > +err_host_deinit:
> > + dw_pcie_host_deinit(&pci->pp);
> > +
> > + return ret;
> > }
> >
> > static void imx_pcie_shutdown(struct platform_device *pdev)
>
> [Severity: High]
> The patch enables the link down interrupt during probe, but looking at
> imx_pcie_shutdown(), it does not disable it before asserting core reset.
> Does asserting the core reset drop the link and trigger the interrupt?
> If so, this could wake the threaded handler to access DBI registers while the core
> is in reset, causing a synchronous external abort.
>
[Richard] Agreed. I will add code to disable the link down interrupt in
imx_pcie_shutdown() before asserting core reset
Best Regards
Richard Zhu
> --
> Sashiko AI
> review * https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsa
> shiko.dev%2F%23%2Fpatchset%2F20260603062510.3767610-1-
> hongxing.zhu%40oss.nxp.com%3Fpart%3D3&data=05%7C02%7Chongxing.zhu%4
> 0nxp.com%7C134330e3333a44f1683508dec13f9e4a%7C686ea1d3bc2b4c6fa92c
> d99c5c301635%7C0%7C0%7C639160676183298029%7CUnknown%7CTWFpbGZ
> sb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkF
> OIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=T12KsupOJBTSoTXb
> rB0CAYhsJrCThSsXkEpdoqsB1GY%3D&reserved=0
^ 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
* 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 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 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 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 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 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 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 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 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 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
* [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
* [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 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 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 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 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 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
* 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] 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 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 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
* [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
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