* Re: [PATCH v4 00/16] i3c: renesas: Suspend to RAM with power loss and runtime PM
From: Frank Li @ 2026-06-12 20:17 UTC (permalink / raw)
To: Claudiu Beznea
Cc: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel, linux-i3c, linux-kernel, claudiu.beznea, Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
On Fri, Jun 12, 2026 at 07:04:42PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Hi,
>
> This series adjusts the suspend to RAM code to handle cases where power
> to the connected devices is lost during suspend to RAM. The fixes
> included in this series are required for that support.
>
> Along with suspend to RAM support, runtime PM support is also added.
> Cleanup patches were included to prepare for clean runtime PM support.
>
> Please note that series need to be applied on top of [1] since
> patch "i3c: renesas: Perform Dynamic Address Assignment on resume"
> uses i3c_master_reattach_i3c_dev_locked().
Please sashiko AI review result, except pre-existing problem you can fix
later, other problem need be fixed or provide your judegement.
Frank
>
> Thank you,
> Claudiu
>
> Changes in v4:
> - dropped patch "i3c: renesas: Do not attach devices if xfer failed"
> - fixed the swap in renesas_i3c_group_devs_in_slots() for i3c->addr[]
> renesas_i3c_irqs_mask_and_clear();
> - in patch 16/16 renamed renesas_i3c_abort_xfer() from v3 to
>
> Changes in v3:
> - re-based on top of series [1] to be able to use
> i3c_master_reattach_i3c_dev_locked()
> - used i3c_master_reattach_i3c_dev_locked() on patch
> "i3c: renesas: Perform Dynamic Address Assignment on resume" along with
> grouping the I2C and I3C devices in the driver slot
> - collected tags
>
> Changes in v2:
> - dropped patch "i3c: renesas: Use the divider 128"
> - adjusted the patches title and description where requested in the
> review process
> - adjusted the DAA procedure after resume to still properly re-configure
> the controller in case the bus was full before a suspend
> - added patch "i3c: renesas: Do not attach devices if xfer failed"
> - collected tags
>
> [1] https://lore.kernel.org/linux-i3c/20260525064209.2263045-1-lakshay.piplani@nxp.com/T/#m74ddb24e1e3a8006d933bd6106cc0c4508a11d2c
>
> Claudiu Beznea (16):
> i3c: renesas: Check that the transfer is valid before accessing it
> i3c: renesas: Restore STDBR and EXTBR registers on resume
> i3c: renesas: Follow the reset deassert order used in probe
> i3c: renesas: Reconfigure the DATBAS register on re-attach
> i3c: renesas: Reset the controller on resume
> i3c: renesas: Perform Dynamic Address Assignment on resume
> i3c: renesas: Clean DATBAS register on detach
> i3c: renesas: Use reset_control_bulk_{assert, deassert}()
> i3c: renesas: Return immediately if there is no transfer
> i3c: renesas: Follow a unified pattern for transfer and command
> initialization
> i3c: renesas: Drop the explicit memset() call
> i3c: renesas: Update HW registers after SW computations are done
> i3c: renesas: Organize structures to avoid unnecessary padding
> i3c: renesas: Use the "dev_name:irq_name" format for the interrupt
> name
> i3c: renesas: Drop unnecessary tab
> i3c: renesas: Add runtime PM support
>
> drivers/i3c/master/renesas-i3c.c | 541 +++++++++++++++++++++++--------
> 1 file changed, 413 insertions(+), 128 deletions(-)
>
> --
> 2.43.0
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v11 8/9] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
From: Frank Li @ 2026-06-12 20:14 UTC (permalink / raw)
To: sashiko-reviews
Cc: Lakshay Piplani, Frank.Li, linux-i3c, devicetree, conor+dt, robh
In-Reply-To: <20260612113948.F33BF1F000E9@smtp.kernel.org>
On Fri, Jun 12, 2026 at 11:39:48AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
> - [High] Regulator devres cleanup actions are erroneously attached to the parent device instead of the child device being probed.
> - [High] SMBus I2C adapters are not unregistered on probe error paths, causing a Use-After-Free.
> - [High] In-Band Interrupt (IBI) remains requested and enabled on probe failure, causing a Use-After-Free.
> - [High] Unsafe reliance on non-deterministic delays without polling the SMBus finish flag causes premature data reads.
> - [High] The driver dynamically overwrites the shared platform device's `of_node` pointer, corrupting the core device tree state.
> - [High] `regmap_update_bits` uses the configuration value as the bitmask, failing to clear stale bits.
> - [High] DAA is performed on the upstream bus before the downstream target ports are physically enabled.
> - [Medium] Target port device tree node references acquired during probe are leaked on error paths.
> - [Medium] Default configuration structures are populated with enum values instead of Ohms, causing misconfiguration.
> - [Low] Dead code due to incorrect `IS_ERR` check on `devm_kzalloc` return value.
> --
>
> commit 8d18a29a52d0297c45635edaf397ce828e709ef9
> Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
>
> i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
>
> Add I3C hub functionality for the NXP P3H2x4x family of multiport hubs.
> These devices support downstream target ports that can be configured
> as I3C, I2C, or SMBus.
>
> > diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> > new file mode 100644
> > index 0000000000000..fedcb978f07ee
> > --- /dev/null
> > +++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
>
> [ ... ]
>
> > +static int p3h2x4x_configure_ldo(struct device *dev)
> > +{
> > + static const char * const supplies[] = {
> > + "vcc1",
> > + "vcc2",
> > + "vcc3",
> > + "vcc4"
> > + };
> > + int ret, i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(supplies); i++) {
> > + ret = devm_regulator_get_enable_optional(dev->parent, supplies[i]);
>
> [Severity: High]
> Could this leak devres memory on the parent device? By passing dev->parent
> (the MFD parent device) instead of dev (the child hub device), the regulator
> cleanup actions are attached to the parent. If the hub driver fails probe
> or is repeatedly bound/unbound, it looks like these regulators won't be
> disabled and the references will be leaked.
look like make sense, please check it.
>
> [ ... ]
>
> > +static int p3h2x4x_configure_tp(struct device *dev)
> > +{
> > + struct p3h2x4x_i3c_hub_dev *hub = dev_get_drvdata(dev);
> > + u8 mode = 0, smbus = 0, pullup = 0, target_port = 0;
> > + int tp, ret;
> > +
> > + for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> > + pullup |= hub->hub_config.tp_config[tp].pullup_en ? P3H2X4X_SET_BIT(tp) : 0;
>
> [ ... ]
>
> > + ret = regmap_update_bits(hub->regmap, P3H2X4X_TP_PULLUP_EN, pullup, pullup);
>
> [Severity: High]
> Does this fail to clear stale bits? The third argument to regmap_update_bits()
> is the mask. By using 'pullup' as the mask, it seems this only applies
> operations to bits that are 1. Any bit that is 0 in 'pullup' is ignored,
> which might leave stale hardware state (like pullups enabled by the
> bootloader) unmodified. Should this use a proper mask covering all relevant
> bits?
Make sense, please check it.
>
> [ ... ]
>
> > +static void p3h2x4x_get_target_port_dt_conf(struct device *dev,
> > + const struct device_node *node)
> > +{
> > + struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> > + u64 tp_port;
> > +
> > + for_each_available_child_of_node_scoped(node, dev_node) {
> > + if (of_property_read_reg(dev_node, 0, &tp_port, NULL))
> > + continue;
> > +
> > + if (tp_port < P3H2X4X_TP_MAX_COUNT) {
> > + p3h2x4x_i3c_hub->tp_bus[tp_port].of_node = of_node_get(dev_node);
>
> [Severity: Medium]
> Is there a resource leak here on probe error paths? This manually increments
> the reference count of the child nodes using of_node_get(). While
> p3h2x4x_i3c_hub_remove() correctly drops these references, if
> p3h2x4x_i3c_hub_probe() fails and returns an error, the remove callback
> isn't executed. Without a devm cleanup action, could these OF nodes leak
> memory?
>
> [ ... ]
>
> > +static void p3h2x4x_default_configuration(struct device *dev)
> > +{
> > + struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> > + int tp_count;
> > +
> > + p3h2x4x_i3c_hub->hub_config.tp0145_pullup = P3H2X4X_TP_PULLUP_500R;
>
> [Severity: Medium]
> Are these default configuration values being set correctly? These fields
> appear to be designed to store physical Ohm measurements, but they are
> initialized to macros like P3H2X4X_TP_PULLUP_500R which evaluate to enum
> indices (e.g., 1). When the default value 1 is later processed by
> p3h2x4x_pullup_dt_to_reg() using find_closest(), it would resolve to the
> closest table entry 250 instead of the intended 500. Should these initialize
> with real-world units?
>
> [ ... ]
>
> > +static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
> > +{
>
> [ ... ]
>
> > + /* Register virtual I2C adapters for SMBus target ports */
> > + ret = p3h2x4x_tp_smbus_algo(p3h2x4x_i3c_hub);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to add i2c adapter\n");
> > +
> > + /* Lock access to protected registers */
> > + ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> > + P3H2X4X_REGISTERS_LOCK_CODE);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to lock HUB's protected registers\n");
>
> [Severity: High]
> Are the SMBus I2C adapters cleaned up if this final regmap_write() fails?
> p3h2x4x_tp_smbus_algo() registers adapters using i2c_add_adapter(), which
> requires an explicit i2c_del_adapter(). If the probe fails here, the adapters
> are never unregistered, which leaves the I2C core with active references to
> devm-freed memory. Could this cause a use-after-free?
>
> > +
> > + return 0;
> > +}
>
> > diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> > new file mode 100644
> > index 0000000000000..38505dda0e81b
> > --- /dev/null
> > +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
>
> [ ... ]
>
> > +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> > +{
>
> [ ... ]
>
> > + p3h2x4x_hub->hub = devm_kzalloc(p3h2x4x_hub->dev,
> > + sizeof(*p3h2x4x_hub->hub),
> > + GFP_KERNEL);
> > +
> > + if (!p3h2x4x_hub->hub)
> > + return -ENOMEM;
> > +
> > + i3c_hub_init(p3h2x4x_hub->hub,
> > + &p3h2x4x_hub_ops,
> > + p3h2x4x_hub->i3cdev);
> > +
> > + if (IS_ERR(p3h2x4x_hub->hub))
> > + return PTR_ERR(p3h2x4x_hub->hub);
>
> [Severity: Low]
> Is this IS_ERR() check necessary? devm_kzalloc() returns either a valid
> pointer or NULL on failure, never an error pointer. It seems this block of
> code might be dead.
Make sense, please fix it
Frank
>
> [ ... ]
>
> > + for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> > + if (!p3h2x4x_hub->tp_bus[tp].of_node ||
> > + p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C)
> > + continue;
> > +
> > + /* Assign DT node for this TP */
> > + p3h2x4x_hub->dev->of_node = p3h2x4x_hub->tp_bus[tp].of_node;
>
> [Severity: High]
> Can dynamically overwriting the platform device's of_node pointer corrupt the
> core device tree state? By changing p3h2x4x_hub->dev->of_node in a loop
> without saving and restoring the original, it looks like this permanently
> changes the node the device points to. Could this break device tree links,
> sysfs topologies, or devres lookups that rely on the original parent hub
> of_node?
>
> [ ... ]
>
> > + /* Perform DAA */
> > + ret = i3c_master_do_daa(parent);
> > + if (ret)
> > + return ret;
> > +
> > + ntwk_mask |= p3h2x4x_hub->tp_bus[tp].tp_mask;
> > + p3h2x4x_hub->tp_bus[tp].is_registered = true;
> > + p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
> > + }
>
> [ ... ]
>
> > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > + ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
> > + if (ret)
> > + return ret;
> > +
> > + ret = i3c_device_enable_ibi(p3h2x4x_hub->i3cdev);
>
> [Severity: High]
> If a subsequent step in p3h2x4x_i3c_hub_probe() fails, are the IBIs safely
> cleaned up? The devm framework would free the p3h2x4x_i3c_hub structure,
> but the IBI might remain active on the master. If an interrupt fires later
> and accesses the freed structure via i3cdev_get_drvdata(i3cdev), could this
> trigger a use-after-free?
>
> > + if (ret) {
> > + i3c_device_free_ibi(p3h2x4x_hub->i3cdev);
> > + return ret;
> > + }
> > +#endif
> > + ret = regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
>
> [Severity: High]
> Does performing DAA here actually reach the downstream targets? The DAA is
> executed earlier on the upstream bus inside the target port loop, but the
> downstream target ports don't appear to be logically connected to the upstream
> bus until ntwk_mask is written to P3H2X4X_TP_NET_CON_CONF at the end of the
> function. Will downstream targets receive the ENTDAA command if DAA is done
> before the physical ports are enabled?
>
> > diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> > new file mode 100644
> > index 0000000000000..edb75f790e92e
> > --- /dev/null
> > +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
>
> [ ... ]
>
> > +static int p3h2x4x_read_smbus_transaction_status(struct p3h2x4x_i3c_hub_dev *hub,
> > + u8 target_port_status,
> > + u8 data_length)
> > +{
> > + u32 status_read;
> > + u8 status;
> > + int ret;
> > +
> > + fsleep(P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(data_length));
> > +
> > + ret = regmap_read(hub->regmap, target_port_status, &status_read);
>
> [Severity: High]
> Could relying on a fixed timeout instead of polling the hardware finish flag
> cause the driver to read stale data? Because the top 4 bits of the status
> register are initialized to 0 before the transaction begins, they remain 0
> while the hardware is busy. If the transaction exceeds the timeout (e.g., due
> to clock stretching), status evaluates to 0
> (P3H2X4X_SMBUS_CNTRL_STATUS_TXN_OK). Should this code poll the
> P3H2X4X_SMBUS_TRANSACTION_FINISH_FLAG instead to ensure completion?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=8
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v11 5/9] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
From: Frank Li @ 2026-06-12 20:02 UTC (permalink / raw)
To: Lakshay Piplani
Cc: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood,
vikash.bansal, priyanka.jain, aman.kumarpandey
In-Reply-To: <20260612111816.3688240-6-lakshay.piplani@nxp.com>
On Fri, Jun 12, 2026 at 04:48:12PM +0530, Lakshay Piplani wrote:
> From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
>
> Add core MFD support for the NXP P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841)
> family of multiport I3C hub devices. These devices connect to a host via
> I3C/I2C/SMBus and expose multiple downstream target ports.
>
> Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
> Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> ---
> Changes in v11:
> - Use MFD_CELL_NAME() for child device registration
> - Rename local variables for consistency
> - Rename driver names to follow subsystem conventions:
> - Use '-' instead of '_' in driver names
> - Drop the "_drv" suffix from driver names
>
> Changes in v10:
> - Drop redundant is_p3h2x4x_in_i3c flag
>
> Changes in v9:
> - Renamed macros to follow consistent uppercase naming conventions
> - Made REGMAP selects in the P3H2X4X MFD Kconfig conditional,
> to avoid I3C/I2C dependency issues
>
> Changes in v8:
> - No change
>
> Changes in v7:
> - Use new config I3C_OR_I2C
>
> Changes in v6:
> - No change
>
> Changes in v5:
> - Corrected the ordering in the Makefile and Kconfig for MFD_P3H2X4X
> - Updated dev_err_probe() for regmap_init failure.
> - Updated module description
>
> Changes in v4:
> - Split the driver into three separate patches(mfd, regulator and I3C hub)
> - Added support for NXP P3H2x4x MFD functionality
> ---
> ---
> MAINTAINERS | 2 +
> drivers/mfd/Kconfig | 13 ++++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/p3h2840.c | 119 ++++++++++++++++++++++++++++++++++++
> include/linux/i3c/device.h | 1 +
> include/linux/mfd/p3h2840.h | 26 ++++++++
> 6 files changed, 162 insertions(+)
> create mode 100644 drivers/mfd/p3h2840.c
> create mode 100644 include/linux/mfd/p3h2840.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ba65ae5a008b..3420701a75c5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19311,6 +19311,8 @@ L: linux-kernel@vger.kernel.org
> L: linux-i3c@lists.infradead.org
> S: Maintained
> F: Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> +F: drivers/mfd/p3h2840.c
> +F: include/linux/mfd/p3h2840.h
>
> NXP PF5300/PF5301/PF5302 PMIC REGULATOR DEVICE DRIVER
> M: Woodrow Douglass <wdouglass@carnegierobotics.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 7192c9d1d268..405b50c3c77b 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -617,6 +617,19 @@ config MFD_MX25_TSADC
> i.MX25 processors. They consist of a conversion queue for general
> purpose ADC and a queue for Touchscreens.
>
> +config MFD_P3H2X4X
> + tristate "NXP P3H2X4X I3C Hub Device"
> + depends on I3C_OR_I2C
> + select MFD_CORE
> + select REGMAP_I3C if I3C
> + select REGMAP_I2C if I2C
> + help
> + Enable Support for NXP P3H244x/P3H284x I3C HUB device using I3C/I2C
> + communication interface.
> +
> + This driver provides support for I3C hub and regulator, each subdriver
> + can be enabled independently depending on the required functionality.
> +
> config MFD_PF1550
> tristate "NXP PF1550 PMIC Support"
> depends on I2C=y && OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e75e8045c28a..a284b22c7b13 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -122,6 +122,7 @@ obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o
> obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o
> obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o
>
> +obj-$(CONFIG_MFD_P3H2X4X) += p3h2840.o
> obj-$(CONFIG_MFD_PF1550) += pf1550.o
>
> obj-$(CONFIG_MFD_NCT6694) += nct6694.o
> diff --git a/drivers/mfd/p3h2840.c b/drivers/mfd/p3h2840.c
> new file mode 100644
> index 000000000000..0a80992d6250
> --- /dev/null
> +++ b/drivers/mfd/p3h2840.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025-2026 NXP
> + * P3H2X4X i3c hub and regulator device.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/i3c/master.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/p3h2840.h>
> +#include <linux/regmap.h>
> +
> +static const struct mfd_cell p3h2x4x_devs[] = {
> + MFD_CELL_NAME("p3h2x4x-regulator"),
> + MFD_CELL_NAME("p3h2x4x-i3c-hub"),
> +};
> +
> +static const struct regmap_config p3h2x4x_regmap_config = {
> + .reg_bits = P3H2X4X_REG_BITS,
> + .val_bits = P3H2X4X_VAL_BITS,
> + .max_register = 0xFF,
> +};
> +
> +static int p3h2x4x_device_probe_i3c(struct i3c_device *i3cdev)
> +{
> + struct p3h2x4x_dev *ddata;
> + int ret;
> +
> + ddata = devm_kzalloc(&i3cdev->dev, sizeof(*ddata), GFP_KERNEL);
> + if (!ddata)
> + return -ENOMEM;
> +
> + i3cdev_set_drvdata(i3cdev, ddata);
> +
> + ddata->regmap = devm_regmap_init_i3c(i3cdev, &p3h2x4x_regmap_config);
> + if (IS_ERR(ddata->regmap))
> + return dev_err_probe(&i3cdev->dev, PTR_ERR(ddata->regmap),
> + "Failed to register HUB regmap\n");
> +
> + // The hub child driver retrieves information from i3cdev
> + ddata->i3cdev = i3cdev;
> +
> + ret = devm_mfd_add_devices(&i3cdev->dev, PLATFORM_DEVID_AUTO,
> + p3h2x4x_devs, ARRAY_SIZE(p3h2x4x_devs),
> + NULL, 0, NULL);
> + if (ret)
> + return dev_err_probe(&i3cdev->dev, ret, "Failed to add sub devices\n");
> +
> + return 0;
> +}
> +
> +static int p3h2x4x_device_probe_i2c(struct i2c_client *client)
> +{
> + struct p3h2x4x_dev *ddata;
> + int ret;
> +
> + ddata = devm_kzalloc(&client->dev, sizeof(*ddata), GFP_KERNEL);
> + if (!ddata)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, ddata);
> +
> + ddata->regmap = devm_regmap_init_i2c(client, &p3h2x4x_regmap_config);
> + if (IS_ERR(ddata->regmap))
> + return dev_err_probe(&client->dev, PTR_ERR(ddata->regmap),
> + "Failed to register HUB regmap\n");
> +
> + ddata->i3cdev = NULL;
> +
> + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
> + p3h2x4x_devs, ARRAY_SIZE(p3h2x4x_devs),
> + NULL, 0, NULL);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "Failed to add sub devices\n");
> +
> + return 0;
> +}
> +
> +static const struct i3c_device_id p3h2x4x_i3c_ids[] = {
> + I3C_CLASS(I3C_DCR_HUB, NULL),
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i3c, p3h2x4x_i3c_ids);
> +
> +static const struct i2c_device_id p3h2x4x_i2c_id_table[] = {
> + { "nxp-i3c-hub" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, p3h2x4x_i2c_id_table);
> +
> +static const struct of_device_id p3h2x4x_i2c_of_match[] = {
> + { .compatible = "nxp,p3h2840", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, p3h2x4x_i2c_of_match);
> +
> +static struct i3c_driver p3h2x4x_i3c = {
> + .driver = {
> + .name = "p3h2x4x-i3c",
> + },
> + .probe = p3h2x4x_device_probe_i3c,
> + .id_table = p3h2x4x_i3c_ids,
> +};
> +
> +static struct i2c_driver p3h2x4x_i2c = {
> + .driver = {
> + .name = "p3h2x4x-i2c",
> + .of_match_table = p3h2x4x_i2c_of_match,
> + },
> + .probe = p3h2x4x_device_probe_i2c,
> + .id_table = p3h2x4x_i2c_id_table,
> +};
> +module_i3c_i2c_driver(p3h2x4x_i3c, &p3h2x4x_i2c);
> +
> +MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
> +MODULE_AUTHOR("Vikash Bansal <vikash.bansal@nxp.com>");
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB multi function driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
> index 971d53349b6f..6188082599dd 100644
> --- a/include/linux/i3c/device.h
> +++ b/include/linux/i3c/device.h
> @@ -85,6 +85,7 @@ struct i3c_xfer {
> */
> enum i3c_dcr {
> I3C_DCR_GENERIC_DEVICE = 0,
> + I3C_DCR_HUB = 194,
> };
>
> #define I3C_PID_MANUF_ID(pid) (((pid) & GENMASK_ULL(47, 33)) >> 33)
> diff --git a/include/linux/mfd/p3h2840.h b/include/linux/mfd/p3h2840.h
> new file mode 100644
> index 000000000000..2d4fae2176ae
> --- /dev/null
> +++ b/include/linux/mfd/p3h2840.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2025-2026 NXP
> + * This header file contains register definitions and bit masks for the P3H2X4X.
> + */
> +
> +#ifndef _LINUX_MFD_P3H2840_H
> +#define _LINUX_MFD_P3H2840_H
> +
> +#include <linux/types.h>
> +
> +/* Device Configuration Registers */
> +#define P3H2X4X_DEV_REG_PROTECTION_CODE 0x10
> +#define P3H2X4X_REGISTERS_LOCK_CODE 0x00
> +#define P3H2X4X_REGISTERS_UNLOCK_CODE 0x69
> +#define P3H2X4X_CP1_REGISTERS_UNLOCK_CODE 0x6a
> +
> +/* Reg config for Regmap */
> +#define P3H2X4X_REG_BITS 8
> +#define P3H2X4X_VAL_BITS 8
> +
> +struct p3h2x4x_dev {
> + struct i3c_device *i3cdev;
> + struct regmap *regmap;
> +};
> +#endif /* _LINUX_MFD_P3H2840_H */
> --
> 2.25.1
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v11 4/9] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
From: Frank Li @ 2026-06-12 20:00 UTC (permalink / raw)
To: Lakshay Piplani
Cc: linux-kernel, linux-i3c, alexandre.belloni, krzk+dt, robh,
conor+dt, devicetree, broonie, lee, Frank.Li, lgirdwood,
vikash.bansal, priyanka.jain, aman.kumarpandey
In-Reply-To: <20260612111816.3688240-5-lakshay.piplani@nxp.com>
On Fri, Jun 12, 2026 at 04:48:11PM +0530, Lakshay Piplani wrote:
> From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
>
> Add bindings for the NXP P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841)
> multiport I3C hub family. These devices connect to a host via
> I3C/I2C/SMBus and allow communication with multiple downstream
> peripherals.
>
> Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
> Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> ---
> Changes in v11:
> - No change
>
> Changes in v10:
> - No change, added Reviewed-By tag
>
> Changes in v9:
> - Referenced i3c.yaml and i2c-controller.yaml for child nodes
> - Dropped unnecessary #address-cells and #size-cells from child nodes
>
> Changes in v8:
> - Add compatible in i3c example
>
> Changes in v7:
> - Fix schema validation issues
> - Adjust required properties
> - Add I2C example
>
> Changes in v6:
> - Use a vendor prefix for the attributes
>
> Changes in v5:
> - Removed SW properties: cp0-ldo-microvolt,cp1-ldo-microvolt,
> tp0145-ldo-microvolt, tp2367-ldo-microvolt
> - Changed supply entries and its descriptions
>
> Changes in v4:
> - Fixed DT binding check warning
> - Removed SW properties: ibi-enable, local-dev, and always-enable
>
> Changes in v3:
> - Added MFD (Multi-Function Device) support for I3C hub and on-die regulator
> - Added Regulator supply node
>
> Changes in v2:
> - Fixed DT binding check warning
> - Revised logic for parsing DTS nodes
> ---
> ---
> .../devicetree/bindings/i3c/nxp,p3h2840.yaml | 291 ++++++++++++++++++
> MAINTAINERS | 9 +
> 2 files changed, 300 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
>
> diff --git a/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> new file mode 100644
> index 000000000000..c080eeb0eeaa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> @@ -0,0 +1,291 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2025 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/nxp,p3h2840.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP P3H2X4X I3C HUB
> +
> +maintainers:
> + - Aman Kumar Pandey <aman.kumarpandey@nxp.com>
> + - Vikash Bansal <vikash.bansal@nxp.com>
> + - Lakshay Piplani <lakshay.piplani@nxp.com>
> +
> +description: |
> + P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841) is a family of multiport I3C
> + hub devices that connect to:-
> + 1. A host CPU via I3C/I2C/SMBus bus on upstream side and connect to multiple
> + peripheral devices on the downstream side.
> + 2. Have two Controller Ports which can support either
> + I2C/SMBus or I3C buses and connect to a CPU, BMC or SOC.
> + 3. P3H2840/ P3H2841 are 8 port I3C hub with eight I3C/I2C Target Port.
> + 4. P3H2440/ P3H2441 are 4 port I3C hub with four I3C/I2C Target Port.
> + Target ports can be configured as I2C/SMBus, I3C or GPIO and connect to
> + peripherals.
> +
> +properties:
> + compatible:
> + const: nxp,p3h2840
> +
> + reg:
> + maxItems: 1
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> + assigned-address:
> + maximum: 0x7f
> +
> + nxp,tp0145-pullup-ohms:
> + description:
> + Selects the pull up resistance for target Port 0/1/4/5, in ohms.
> + enum: [250, 500, 1000, 2000]
> + default: 500
> +
> + nxp,tp2367-pullup-ohms:
> + description:
> + Selects the pull up resistance for target Port 2/3/6/7, in ohms.
> + enum: [250, 500, 1000, 2000]
> + default: 500
> +
> + nxp,cp0-io-strength-ohms:
> + description:
> + Selects the IO drive strength for controller Port 0, in ohms.
> + enum: [20, 30, 40, 50]
> + default: 20
> +
> + nxp,cp1-io-strength-ohms:
> + description:
> + Selects the IO drive strength for controller Port 1, in ohms.
> + enum: [20, 30, 40, 50]
> + default: 20
> +
> + nxp,tp0145-io-strength-ohms:
> + description:
> + Selects the IO drive strength for target port 0/1/4/5, in ohms.
> + enum: [20, 30, 40, 50]
> + default: 20
> +
> + nxp,tp2367-io-strength-ohms:
> + description:
> + Selects the IO drive strength for target port 2/3/6/7, in ohms.
> + enum: [20, 30, 40, 50]
> + default: 20
> +
> + vcc1-supply:
> + description: Controller port 0 power supply.
> +
> + vcc2-supply:
> + description: Controller port 1 power supply.
> +
> + vcc3-supply:
> + description: Target port 0/1/4/5 power supply.
> +
> + vcc4-supply:
> + description: Target port 2/3/6/7 power supply.
> +
> + regulators:
> + type: object
> + additionalProperties: false
> +
> + properties:
> + ldo-cp0:
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
> + unevaluatedProperties: false
> +
> + ldo-cp1:
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
> + unevaluatedProperties: false
> +
> + ldo-tpg0:
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
> + unevaluatedProperties: false
> +
> + ldo-tpg1:
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
> + unevaluatedProperties: false
> +
> +required:
> + - reg
> +
> +patternProperties:
> + "^i3c@[0-7]$":
> + type: object
> + $ref: /schemas/i3c/i3c.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description:
> + The I3C HUB Target Port number.
> + maximum: 7
> +
> + nxp,pullup-enable:
> + type: boolean
> + description:
> + Enables the on-die pull-up for Target Port.
> +
> + required:
> + - reg
> +
> + "^(i2c|smbus)@[0-7]$":
> + type: object
> + $ref: /schemas/i2c/i2c-controller.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description:
> + The I3C HUB Target Port number.
> + maximum: 7
> +
> + nxp,pullup-enable:
> + type: boolean
> + description:
> + Enables the on-die pull-up for Target Port.
> +
> + required:
> + - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + i3c {
> + #address-cells = <3>;
> + #size-cells = <0>;
> +
> + hub@70,236153000c2 {
> + reg = <0x70 0x236 0x3000c2>;
> + compatible = "nxp,p3h2840";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + assigned-address = <0x50>;
> +
> + nxp,tp0145-pullup-ohms = <1000>;
> + nxp,tp2367-pullup-ohms = <1000>;
> + nxp,cp0-io-strength-ohms = <50>;
> + nxp,cp1-io-strength-ohms = <50>;
> + nxp,tp0145-io-strength-ohms = <50>;
> + nxp,tp2367-io-strength-ohms = <50>;
> + vcc3-supply = <®_tpg0>;
> + vcc4-supply = <®_tpg1>;
> +
> + regulators {
> + reg_cp0: ldo-cp0 {
> + regulator-name = "ldo-cp0";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +
> + reg_cp1: ldo-cp1 {
> + regulator-name = "ldo-cp1";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +
> + reg_tpg0: ldo-tpg0 {
> + regulator-name = "ldo-tpg0";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +
> + reg_tpg1: ldo-tpg1 {
> + regulator-name = "ldo-tpg1";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> + };
> +
> + smbus@0 {
> + reg = <0x0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + nxp,pullup-enable;
> + };
> +
> + i2c@1 {
> + reg = <0x1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + nxp,pullup-enable;
> + };
> +
> + i3c@2 {
> + reg = <0x2>;
> + #address-cells = <3>;
> + #size-cells = <0>;
> + nxp,pullup-enable;
> + };
> + };
> + };
> +
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + hub@70 {
> + reg = <0x70>;
> + compatible = "nxp,p3h2840";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + nxp,tp0145-pullup-ohms = <1000>;
> + nxp,tp2367-pullup-ohms = <1000>;
> + nxp,cp0-io-strength-ohms = <50>;
> + nxp,cp1-io-strength-ohms = <50>;
> + nxp,tp0145-io-strength-ohms = <50>;
> + nxp,tp2367-io-strength-ohms = <50>;
> + vcc3-supply = <®_tpg0_i2c>;
> + vcc4-supply = <®_tpg1_i2c>;
> +
> + regulators {
> + reg_cp0_i2c: ldo-cp0 {
> + regulator-name = "ldo-cp0";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +
> + reg_cp1_i2c: ldo-cp1 {
> + regulator-name = "ldo-cp1";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +
> + reg_tpg0_i2c: ldo-tpg0 {
> + regulator-name = "ldo-tpg0";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +
> + reg_tpg1_i2c: ldo-tpg1 {
> + regulator-name = "ldo-tpg1";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> + };
> +
> + smbus@0 {
> + reg = <0x0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + nxp,pullup-enable;
> + };
> +
> + i2c@1 {
> + reg = <0x1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + nxp,pullup-enable;
> + };
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c2c6d79275c6..ba65ae5a008b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19303,6 +19303,15 @@ S: Maintained
> F: Documentation/devicetree/bindings/ptp/nxp,ptp-netc.yaml
> F: drivers/ptp/ptp_netc.c
>
> +NXP P3H2X4X I3C-HUB DRIVER
> +M: Vikash Bansal <vikash.bansal@nxp.com>
> +M: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
> +M: Lakshay Piplani <lakshay.piplani@nxp.com>
> +L: linux-kernel@vger.kernel.org
> +L: linux-i3c@lists.infradead.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> +
> NXP PF5300/PF5301/PF5302 PMIC REGULATOR DEVICE DRIVER
> M: Woodrow Douglass <wdouglass@carnegierobotics.com>
> S: Maintained
> --
> 2.25.1
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v11 3/9] i3c: master: Add APIs for I3C hub support
From: Frank Li @ 2026-06-12 19:59 UTC (permalink / raw)
To: sashiko-reviews
Cc: Lakshay Piplani, linux-i3c, devicetree, conor+dt, Frank.Li, robh
In-Reply-To: <20260612114844.5D7B71F000E9@smtp.kernel.org>
On Fri, Jun 12, 2026 at 11:48:43AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] i3c_master_direct_detach_i3c_dev_locked lacks the unaddressable device check present in its attach counterpart, leading to a NULL pointer dereference in controller drivers.
> --
>
> commit a7f74140d15672b8f2824666071ce06e3051e0d8
> Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
>
> i3c: master: Add APIs for I3C hub support
>
> Add helpers for attaching and detaching I3C devices and CCC helpers
> to check CCC support and send CCC commands to prepare for I3C hub support.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index ec3547f75469e..59e0f82de6ebd 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
>
> [ ... ]
>
> > +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> > + struct i3c_dev_desc *dev)
> > +{
> > + int ret = 0;
> > +
> > + /*
> > + * We don't attach devices to the controller until they are
> > + * addressable on the bus.
> > + */
> > +
> > + if (!dev->info.static_addr && !dev->info.dyn_addr)
> > + return 0;
> > +
> > + /* Do not attach the master device itself. */
> > + if (master->this != dev && master->ops->attach_i3c_dev)
> > + ret = master->ops->attach_i3c_dev(dev);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
>
> [ ... ]
>
> > +void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev)
> > +{
> > + struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > +
> > + /* Do not detach the master device itself. */
> > + if (master->this != dev && master->ops->detach_i3c_dev)
> > + master->ops->detach_i3c_dev(dev);
>
> [Severity: High]
> Does this missing address check in the detach helper lead to a NULL pointer
> dereference in controller drivers?
>
> The attach helper i3c_master_direct_attach_i3c_dev_locked() gracefully skips
> attaching devices without a static or dynamic address and returns 0 (success).
> Because it returns success, callers might naturally invoke this companion
> detach API during teardown for these same devices.
>
> However, since this helper lacks the corresponding address check, it
> unconditionally invokes the controller's detach callback. Controller drivers
> often retrieve the device's private data and immediately dereference it. Since
> the attach phase was skipped, this private data would be NULL.
>
> Should this function also check if the device has an address before calling
> the detach callback?
Make sense, please fix it.
Frank
>
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_detach_i3c_dev_locked);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=3
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v11 2/9] i3c: master: Expose the APIs to support I3C hub
From: Frank Li @ 2026-06-12 19:52 UTC (permalink / raw)
To: sashiko-reviews
Cc: Lakshay Piplani, devicetree, Frank.Li, linux-i3c, conor+dt, robh
In-Reply-To: <20260612114421.2BEE31F000E9@smtp.kernel.org>
On Fri, Jun 12, 2026 at 11:44:20AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [Medium] API documentation mismatch: i3c_master_reattach_i3c_dev_locked() claims to return -EBUSY on collision, but unconditionally overwrites slot status.
> - [Low] Invalid lock context expressions and typos in newly added IBI kernel-doc.
>
> Pre-existing issues:
> - [Critical] Multiple Use-After-Free vulnerabilities in i3c_dev_free_ibi_locked() due to improper synchronization and teardown order.
> - [High] Unbalanced release of address slots in i3c_master_reattach_i3c_dev_locked() failure path leads to bus corruption.
> --
>
> commit 13a1ff5c64b48ceff03545ae3bebd5e60a136c1e
> Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
>
> i3c: master: Expose the APIs to support I3C hub
>
> This patch exposes several internal I3C master functions to allow new I3C
> hub drivers to use them. It adds kernel-doc comments and exports symbols
> like i3c_master_reattach_i3c_dev_locked and various IBI handling functions.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index c45b450af319d..ec3547f75469e 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1652,8 +1652,23 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
> > return 0;
> > }
> >
> > -static int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
> > - u8 old_dyn_addr)
> > +/**
> > + * i3c_master_reattach_i3c_dev_locked() - reattach an I3C device with a new address
> > + * @dev: I3C device descriptor to reattach
> > + * @old_dyn_addr: previous dynamic address of the device
> > + *
> > + * This function reattaches an existing I3C device to the bus when its dynamic
> > + * address has changed. It updates the bus address slot status accordingly:
> > + * - Marks the new dynamic address as occupied by an I3C device.
> > + * - Frees the old dynamic address slot if applicable.
> > + *
> > + * This function must be called with the bus lock held in write mode.
> > + *
> > + * Return: 0 on success, or a negative error code if reattachment fails
> > + * (e.g. -EBUSY if the new address slot is not free).
> > + */
> > +int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
> > + u8 old_dyn_addr)
> > {
>
> [Severity: Medium]
> The kernel-doc states this returns -EBUSY if the new address slot is not free,
> but does the implementation actually check this?
>
> Looking at the rest of i3c_master_reattach_i3c_dev_locked(), it appears
> i3c_bus_set_addr_slot_status() is called unconditionally to set
> I3C_ADDR_SLOT_I3C_DEV without first checking if the slot is available.
>
> Could this silently overwrite an existing occupied slot if an external driver
> relies on the documented failure behavior?
>
> [Severity: High]
> This is a pre-existing issue, but does the error path safely handle a failure
> from master->ops->reattach_i3c_dev()?
>
> When the reattach operation fails, i3c_master_put_i3c_addrs(dev) is called,
> which marks the address slots as free. However, it doesn't appear to revert
> the hardware device's assigned address.
>
> Since callers like i3c_master_add_i3c_dev_locked() completely ignore the
> return value of this function, could this leave the bus manager out of sync
> with the hardware state and cause severe address collisions if the slot is
> subsequently reassigned?
>
> [ ... ]
>
> > @@ -3195,6 +3211,16 @@ int i3c_dev_do_xfers_locked(struct i3c_dev_desc *dev, struct i3c_xfer *xfers,
> > return master->ops->i3c_xfers(dev, xfers, nxfers, mode);
> > }
> >
> > +/**
> > + * i3c_dev_disable_ibi_locked() - Disable IBIs coming from a specific device
> > + * @dev: device on which IBIs should be disabled
> > + *
> > + * This function disable IBIs coming from a specific device and wait for
> > + * all pending IBIs to be processed.
> > + *
> > + * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
> > + * Return: 0 in case of success, a negative error core otherwise.
> > + */
>
> [Severity: Low]
> Is the lock context expression correct here?
>
> The parameter is of type struct i3c_dev_desc, which does not have a desc
> member, so &dev->desc->ibi_lock appears to be structurally invalid.
> Should this be &dev->ibi_lock instead?
Make sense, please fix it
>
> Also, there is a minor typo: "error core" should likely be "error code".
Please this typo
Frank
>
> [ ... ]
>
> > +/**
> > + * i3c_dev_free_ibi_locked() - Free all resources needed for IBI handling
> > + * @dev: device on which you want to release IBI resources
> > + *
> > + * This function is responsible for de-allocating resources previously
> > + * allocated by i3c_device_request_ibi(). It should be called after disabling
> > + * IBIs with i3c_device_disable_ibi().
> > + *
> > + * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
> > + */
> > void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
>
> [Severity: Critical]
> This is a pre-existing issue, but does this teardown order introduce a
> use-after-free regression when exposed to external drivers?
>
> Looking at the body of i3c_dev_free_ibi_locked(), the function calls
> master->ops->free_ibi(dev) before destroy_workqueue(dev->ibi->wq).
>
> If free_ibi() frees the underlying IBI pool memory, and destroy_workqueue()
> subsequently flushes pending async work items, could those work items
> execute and dereference the already freed pool?
>
> Additionally, if i3c_dev_disable_ibi_locked() had failed to disable the
> hardware interrupt, could a firing interrupt access dev->ibi after it is
> unconditionally freed by kfree(dev->ibi) at the end of this function?
>
> > {
> > struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > @@ -3301,6 +3366,7 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> > kfree(dev->ibi);
> > dev->ibi = NULL;
> > }
> > +EXPORT_SYMBOL_GPL(i3c_dev_free_ibi_locked);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=2
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 16/16] i3c: renesas: Add runtime PM support
From: sashiko-bot @ 2026-06-12 16:34 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-i3c, Frank.Li
In-Reply-To: <20260612160458.3102106-17-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Timeout handler fails to abort hardware transfers because it clears the BCTL_ABT bit instead of setting it.
Pre-existing issues:
- [High] Transfer loops do not break on timeout and unconditionally return success, leading to silent failures, race conditions, and memory corruption.
--
commit d8976043a71981cb4634aa1e9eafca1113e44ec6
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Add runtime PM support
This commit adds runtime PM support to the Renesas I3C driver, guarding
subsystem operations with pm_runtime APIs. It also drops explicit clock
handling, modifies wait_xfer to return unsigned long, and introduces a
function to clear pending IRQs on transfer aborts.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -443,7 +442,24 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
> }
> }
>
> -static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> +static void renesas_i3c_irqs_mask_and_clear(struct renesas_i3c *i3c)
> +{
> + guard(spinlock_irqsave)(&i3c->xferqueue.lock);
> +
> + /* Disable all the interrupts */
> + renesas_writel(i3c->regs, BIE, 0);
> + renesas_writel(i3c->regs, NTIE, 0);
> +
> + /* Clear normal transfer status flags. */
> + renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0 | NTST_RSPQFF |
> + NTST_TEF | NTST_TABTF);
> + /* Clear bus status flags. */
> + renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF | BST_SPCNDDF);
> + /* Clear error flags. */
> + renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
> +}
[Severity: High]
Does this actually abort the hardware transfer? The commit message mentions
disabling interrupts and clearing pending IRQ status bits when a completion
timeout occurs.
However, clearing the BCTL_ABT bit is usually how an abort is acknowledged.
To actually initiate a hardware abort, the host typically must set the
BCTL_ABT bit, or explicitly reset the controller.
By only clearing an already-zero bit, does the controller remain in a busy
or hung state, potentially corrupting subsequent transfers?
[ ... ]
> @@ -777,7 +812,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
> NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
> NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> + time_left = renesas_i3c_wait_xfer(i3c, xfer);
> + if (!time_left)
> + renesas_i3c_irqs_mask_and_clear(i3c);
>
> newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
[Severity: High]
This is a pre-existing issue, but I wanted to ask about the timeout handling
here in renesas_i3c_daa().
If a timeout occurs (!time_left), the code proceeds without breaking early
or returning an error. Because cmd->rx_count might be empty on a timeout,
does this end up parsing a full newdevs mask and erroneously attaching
phantom devices for all remaining address slots?
[ ... ]
> @@ -970,9 +1022,14 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
> renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
> }
>
> - renesas_i3c_wait_xfer(i3c, xfer);
> + time_left = renesas_i3c_wait_xfer(i3c, xfer);
> + if (!time_left)
> + xfer_failed = true;
> }
>
> + if (xfer_failed)
> + renesas_i3c_irqs_mask_and_clear(i3c);
> +
> return 0;
> }
[Severity: High]
This is also a pre-existing issue, but does this loop in
renesas_i3c_i3c_xfers() need to break when a timeout occurs?
By continuing the loop after xfer_failed is set to true, it seems
subsequent commands are enqueued to a controller that might still be hung
from the timed-out transfer.
Additionally, this function unconditionally returns 0 (success) at the end,
completely ignoring the timeout and any hardware errors recorded in
xfer->ret. Would it be safer to return an error code and break out of the
loop immediately?
[ ... ]
> @@ -1097,7 +1175,9 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>
> renesas_set_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
>
> - wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
> + time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
> + if (!time_left)
> + xfer_failed = true;
>
> if (cmd->err)
> break;
[Severity: High]
This is a pre-existing issue, but similar to the i3c transfers above, the
i2c transfer loop in renesas_i3c_i2c_xfers() fails to break on a timeout.
Because it continues enqueuing commands to a potentially hung controller,
could this cause a direct data race on cmd->i2c_buf and ultimately result in
returning a false success status?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=16
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 04/16] i3c: renesas: Reconfigure the DATBAS register on re-attach
From: sashiko-bot @ 2026-06-12 16:24 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: Frank.Li, linux-i3c
In-Reply-To: <20260612160458.3102106-5-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Hardware transfers are not aborted on software timeout, permanently desynchronizing the bus state and precipitating severe memory corruption.
- [High] Hardware DAA implementation erroneously assumes free device slots are contiguous, assigning already-in-use addresses to new devices under fragmented slot conditions.
- [High] Ignored completion timeout returns in `renesas_i3c_i2c_xfers` lead to silent operational failures.
- [High] Undefined behavior in dynamic `GENMASK` generation causes false phantom device attachment.
--
commit 80894950fcbc5a0242a4c816117f647e2715af0b
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Reconfigure the DATBAS register on re-attach
This commit reconfigures the DATBAS register during re-attach to handle
cases where a device changes its position in the i3c->addrs[] array. It
ensures the correct Device Address Table Basic Register is used and
updates software caches.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 7d448936b74c1..4b81c32b1fdec 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -892,10 +892,26 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
> struct i3c_master_controller *m = i3c_dev_get_master(dev);
> struct renesas_i3c *i3c = to_renesas_i3c(m);
> struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> + int pos;
> +
> + pos = renesas_i3c_get_free_pos(i3c);
> +
> + if (data->index > pos && pos >= 0) {
> + renesas_writel(i3c->regs, DATBAS(data->index), 0);
> + i3c->addrs[data->index] = 0;
> + i3c->free_pos |= BIT(data->index);
> +
> + data->index = pos;
> + i3c->free_pos &= ~BIT(data->index);
> + }
>
> i3c->addrs[data->index] = dev->info.dyn_addr ? dev->info.dyn_addr :
> dev->info.static_addr;
>
> + renesas_writel(i3c->regs, DATBAS(data->index),
> + DATBAS_DVSTAD(dev->info.static_addr) |
> + datbas_dvdyad_with_parity(i3c->addrs[data->index]));
> +
> return 0;
> }
[Severity: High]
This isn't a bug introduced by this patch, but looking at renesas_i3c_daa()
in this file, is it possible for the hardware to overwrite an active device
if slots are fragmented?
In renesas_i3c_daa():
cmd->cmd0 = NCMDQP_CMD_ATTR(NCMDQP_ADDR_ASSGN) | NCMDQP_ROC |
NCMDQP_TID(I3C_COMMAND_ADDRESS_ASSIGNMENT) |
NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
The code passes i3c->maxdevs - ret to NCMDQP_DEV_COUNT, assuming all
subsequent slots are free. If slot 1 is free but slot 2 is in use, could the
hardware increment its internal DATBAS index and overwrite the active device
in slot 2 when multiple new devices respond to ENTDAA?
[Severity: Critical]
This is a pre-existing issue, but I have a question about how hardware aborts
are handled on software timeout.
When wait_for_completion_timeout() expires in renesas_i3c_wait_xfer():
time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
if (!time_left)
renesas_i3c_dequeue_xfer(i3c, xfer);
The xfer is dequeued, but the hardware transfer doesn't appear to be aborted.
If a new transfer is enqueued, and the hardware eventually completes the
timed-out transfer, could renesas_i3c_resp_isr() blindly apply the late
response to the new transfer buffer?
xfer = i3c->xferqueue.cur;
...
if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
bytes_remaining = data_len - cmd->rx_count;
i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
Does this create a scenario where i3c_readl_fifo() copies data into the new
cmd->rx_buf based on the old data_len, potentially causing a buffer overflow?
[Severity: High]
This isn't a bug introduced by this patch, but does the I2C transfer logic
mask completion timeouts?
In renesas_i3c_i2c_xfers():
time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
if (!time_left)
xfer_failed = true;
if (cmd->err)
break;
If the timeout expires, xfer_failed is set to true but cmd->err remains 0
because the ISR never executed. Since cmd->err is 0, the loop does not break,
which seems like it could cause the driver to continue to the next message
and eventually return 0, masking the timeout.
[Severity: High]
This is a pre-existing issue, but can the dynamic mask generation in
renesas_i3c_daa() result in phantom device attachments?
In renesas_i3c_daa(), when configuring the mask for new devices:
newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
If no devices respond to the ENTDAA command, cmd->rx_count is set to
i3c->maxdevs - ret. If ret is 0, this makes the upper bound of GENMASK
evaluate to -1. Would executing GENMASK(-1, 0) evaluate to ~0UL and cause
the driver to iterate over all bits, falsely registering phantom devices
via i3c_master_add_i3c_dev_locked()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=4
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 09/16] i3c: renesas: Return immediately if there is no transfer
From: sashiko-bot @ 2026-06-12 16:23 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-i3c, Frank.Li
In-Reply-To: <20260612160458.3102106-10-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Hardware Desynchronization and Memory Corruption on timeout
- [Critical] Missing Bounds Check causing Kernel Heap Buffer Overflow in Receive ISRs
- [High] Unchecked timeout leading to hardware desynchronization and incorrect success reporting in `renesas_i3c_i2c_xfers`
- [High] Missing Error Handling in `renesas_i3c_i3c_xfers` unconditionally returns success
- [High] Out-of-Bounds Read and Endianness violation on short TX transfers
--
commit e7c9bbe000eba8d228d206b4d05e155779672539
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Return immediately if there is no transfer
There is no need to allocate a transfer structure when i2c_nxfers is zero.
Return immediately instead of unnecessarily allocating memory.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 9edd2dc961073..fa9c15381ed6a 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1061,13 +1061,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
> u8 start_bit = CNDCTL_STCND;
> int i;
>
> + if (!i2c_nxfers)
> + return 0;
> +
> struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
> if (!xfer)
> return -ENOMEM;
>
> - if (!i2c_nxfers)
> - return 0;
> -
> renesas_i3c_bus_enable(m, false);
>
> init_completion(&xfer->comp);
[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with
unchecked timeouts later in this same function:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
...
wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
if (cmd->err)
break;
...
If wait_for_completion_timeout() times out, time_left will be 0 but cmd->err
remains 0 (its initialization value, since the ISR did not run to update it).
The check evaluates to false, causing the driver to continue the loop and issue
the next I2C message while the hardware may still be executing the previous one.
Finally, it unconditionally returns cmd->err (0), falsely reporting success to
the I2C subsystem on timeout. Should this explicitly check the timeout return
value?
[Severity: High]
This isn't a bug introduced by this patch, but renesas_i3c_i3c_xfers() appears
to unconditionally return success even on error:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
...
renesas_i3c_wait_xfer(i3c, xfer);
}
return 0;
}
The function iterates through I3C transfers but ignores the error status
(xfer->ret) populated by the ISR on hardware failures (e.g., NACK, CRC errors).
Even if a timeout occurs, the function unconditionally returns 0 at the end,
masking all I3C transfer failures from the subsystem. Could it return xfer->ret
or an appropriate error code instead?
[Severity: Critical]
This is a pre-existing issue, but I noticed a potential problem with hardware
desynchronization and memory corruption on timeout.
If a transfer timeout occurs, renesas_i3c_wait_xfer() removes the transfer from
the software queue, but the driver fails to abort the hardware operation. In
the ISR:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
...
xfer = i3c->xferqueue.cur;
if (!xfer) {
/* Disable interrupts. */
renesas_clear_bit(i3c->regs, NTIE, NTIE_TDBEIE0 | NTIE_RDBFIE0);
/* Clear any error flags. */
renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
...
The driver clears the BCTL_ABT bit instead of setting it. When a subsequent
transfer starts, interrupts are re-enabled and the hardware may fire a delayed
completion interrupt for the old transfer. The ISR will attribute this interrupt
to the newly enqueued transfer (i3c->xferqueue.cur), potentially causing data
corruption and out-of-bounds writes into the new transfer's buffer. Does
BCTL_ABT need to be set instead to properly abort?
[Severity: Critical]
This isn't a bug introduced by this patch, but there appears to be a missing
bounds check causing a kernel heap buffer overflow in the receive ISRs.
In both renesas_i3c_resp_isr() and renesas_i3c_rx_isr(), the length of incoming
data is calculated directly from hardware-provided registers without being
validated against the allocated buffer size (cmd->len).
For example, in renesas_i3c_resp_isr():
drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
...
if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
bytes_remaining = data_len - cmd->rx_count;
i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
...
And in renesas_i3c_rx_isr():
drivers/i3c/master/renesas-i3c.c:renesas_i3c_rx_isr() {
...
read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
...
A malfunctioning or malicious target device sending an oversized response could
cause i3c_readl_fifo() to write past the bounds of cmd->rx_buf. Can this be
safeguarded by checking against cmd->len?
[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds read and
endianness violation on short TX transfers.
In renesas_i3c_start_xfer_locked(), immediate transfers of 1 to 3 bytes cast
the tx_buf pointer to a u32 * and dereference it:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_start_xfer_locked() {
...
if (cmd->len <= 4) {
cmd->cmd0 |= NCMDQP_CMD_ATTR(NCMDQP_IMMED_XFER);
cmd->cmd0 |= NCMDQP_BYTE_CNT(cmd->len);
cmd->tx_count = cmd->len;
cmd1 = cmd->len == 0 ? 0 : *(u32 *)cmd->tx_buf;
} else {
...
Since the caller may only provide a 1-byte array on the stack, this reads
out-of-bounds memory, potentially triggering KASAN or causing unaligned memory
access faults. Additionally, it reads the bytes in CPU host byte order, causing
the data sent over the I3C wire to be incorrectly ordered on Big Endian systems.
Would it be better to copy the bytes safely into a temporary u32 and convert to
the correct endianness?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=9
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 10/16] i3c: renesas: Follow a unified pattern for transfer and command initialization
From: sashiko-bot @ 2026-06-12 16:21 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-i3c, Frank.Li
In-Reply-To: <20260612160458.3102106-11-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
Pre-existing issues:
- [Critical] The `renesas_i3c_i3c_xfers` function silently ignores all transfer errors and timeouts, always returning success.
- [High] The driver does not abort or reset the underlying hardware controller when an I2C or I3C transfer times out. This leaves the hardware active in the background, which will corrupt subsequent transfers.
- [High] In `renesas_i3c_i2c_xfers`, the return value of `wait_for_completion_timeout` is ignored, leading to erroneous state progression and reporting success when transfers actually timed out.
- [High] The driver unconditionally terminates every I3C private transfer with a STOP condition, failing to support atomic transactions that require Repeated Start (Sr).
- [High] I3C write transfers larger than 64 bytes will overflow the hardware FIFO and repeatedly corrupt data.
- [High] I3C read transfers larger than 64 bytes will overwrite the beginning of the receive buffer.
- [High] Dynamic Address Assignment (DAA) ignores transfer failures and timeouts.
--
commit 172b123b52edbe37fd1e66252700d27e875fa822
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Follow a unified pattern for transfer and command initialization
This patch unifies the pattern for transfer and command initialization
across the renesas i3c driver. It ensures the transfer data structure is
allocated, completion initialized, and command properties set before enabling
the I3C bus.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index fa9c15381ed6a..883be4326e430 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -726,6 +726,10 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
> if (!xfer)
> return -ENOMEM;
>
> + init_completion(&xfer->comp);
> + cmd = xfer->cmds;
> + cmd->rx_count = 0;
> +
> /* Enable I3C bus. */
> renesas_i3c_bus_enable(m, true);
[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
renesas_i3c_daa():
time_left = renesas_i3c_wait_xfer(i3c, xfer);
if (!time_left)
renesas_i3c_irqs_mask_and_clear(i3c);
newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
The result of the ENTDAA transfer is unverified before trusting
cmd->rx_count. If the assignment command fails or times out, does the
driver blindly proceed to calculate device masks using a stale or zero
rx_count? Could this lead to incorrect device attachment logic and
state desynchronization?
[ ... ]
> @@ -934,15 +934,15 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
> struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> int i;
>
> - /* Enable I3C bus. */
> - renesas_i3c_bus_enable(m, true);
> -
> struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
> if (!xfer)
> return -ENOMEM;
>
> init_completion(&xfer->comp);
>
> + /* Enable I3C bus. */
> + renesas_i3c_bus_enable(m, true);
> +
> for (i = 0; i < i3c_nxfers; i++) {
> struct renesas_i3c_cmd *cmd = xfer->cmds;
[Severity: High]
This isn't a bug introduced by this patch, but looking at the loop in
renesas_i3c_i3c_xfers() where it sets up the command descriptor:
cmd->cmd0 = NCMDQP_DEV_INDEX(data->index) | NCMDQP_MODE(0) |
NCMDQP_RNW(cmd->rnw) | NCMDQP_ROC | NCMDQP_TOC;
Does setting NCMDQP_TOC unconditionally inside the loop break support for
Repeated Start (Sr) sequences? By forcing a STOP condition after each
message, doesn't this prevent the driver from executing atomic
transactions?
[Severity: Critical]
This is also a pre-existing issue, but further down in
renesas_i3c_i3c_xfers(), the function ignores the transfer result:
if (xfer_failed)
renesas_i3c_irqs_mask_and_clear(i3c);
return 0;
Is it intentional to silently ignore all transfer errors and timeouts here?
Because the function always returns 0, upper layers will assume writes
succeeded and might process uninitialized or corrupted read buffers.
[Severity: High]
Another pre-existing issue in this area: does this function properly handle
transfers larger than 64 bytes?
Looking at the write path, the code pushes cmd->len bytes regardless of the
NTDTBP0_DEPTH limit (16 words). Additionally, in renesas_i3c_tx_isr(), the
TX interrupt repeatedly writes the entire cmd->len buffer from the base
address instead of advancing the pointer:
i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
Will this reliably overflow the hardware FIFO and repeatedly corrupt data?
[Severity: High]
Similarly for the read path, this is a pre-existing issue, but does the
driver overwrite the beginning of the receive buffer for large reads?
In renesas_i3c_rx_isr() and renesas_i3c_resp_isr(), data is read using:
i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
cmd->rx_count = read_bytes;
Because cmd->rx_buf is not offset by the bytes already read, and
cmd->rx_count is overwritten instead of accumulated, won't subsequent
interrupt reads write into the very beginning of the user's buffer?
[ ... ]
> @@ -1068,12 +1068,12 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
> if (!xfer)
> return -ENOMEM;
>
> - renesas_i3c_bus_enable(m, false);
> -
> init_completion(&xfer->comp);
> xfer->is_i2c_xfer = true;
> cmd = xfer->cmds;
>
> + renesas_i3c_bus_enable(m, false);
> +
> if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
> cmd->err = -EBUSY;
> return cmd->err;
[Severity: High]
This is a pre-existing issue, but looking at the timeout handling in
renesas_i3c_i2c_xfers():
time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
if (!time_left)
xfer_failed = true;
if (cmd->err)
break;
If the transfer times out, time_left is 0 but cmd->err remains 0. Does this
cause the loop to continue and overwrite the cmd state while the hardware
might still be stuck, eventually returning success (0)?
[Severity: High]
This is another pre-existing issue related to timeouts. When
wait_for_completion_timeout() times out here, the software removes the xfer
object from the queue and masks interrupts in
renesas_i3c_irqs_mask_and_clear():
renesas_writel(i3c->regs, BIE, 0);
renesas_writel(i3c->regs, NTIE, 0);
However, it does not send an abort command or reset the hardware.
If a new transfer is initiated, the hardware continues operation in the
background. When the slow hardware eventually completes the original
timed-out transfer, it triggers an interrupt. Will renesas_i3c_resp_isr()
then fetch the new i3c->xferqueue.cur and blindly apply the old transfer's
completion status and read data into the new transfer's buffers, causing
cross-transfer memory corruption?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=10
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 07/16] i3c: renesas: Clean DATBAS register on detach
From: sashiko-bot @ 2026-06-12 16:20 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: Frank.Li, linux-i3c
In-Reply-To: <20260612160458.3102106-8-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Critical] `renesas_i3c_group_devs_in_slots` assumes the target swap slot is occupied by an I3C device, which crashes if the slot is free. It also fails to update the free slot bitmask.
Pre-existing issues:
- [High] The DATBAS registers and device address trackers for unused slots are left dirty after Dynamic Address Assignment (DAA), which can cause hardware TX/RX logic issues.
--
commit bcdea49e3d9ced6f5b774a208065f0889dbb7c12
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Clean DATBAS register on detach
The controller uses DATBAS registers on TX/RX logic. Clean the DATBAS
register for the detached I3C device to avoid issues.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index a63edddbc9fe0..abe24e563d217 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1042,6 +1042,8 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
> struct i3c_master_controller *m = i3c_dev_get_master(dev);
> struct renesas_i3c *i3c = to_renesas_i3c(m);
>
> + renesas_writel(i3c->regs, DATBAS(data->index), 0);
> +
> i3c_dev_set_master_data(dev, NULL);
> i3c->addrs[data->index].addr = 0;
> i3c->addrs[data->index].i3c_dev = NULL;
[Severity: Critical]
Since we are allowing I3C devices to be detached and free their slots here,
will this cause a null pointer dereference in
renesas_i3c_group_devs_in_slots() during resume?
If a slot at i2c_pos was previously detached, i3c_addr.i3c_dev can be
completely free. In this scenario, it seems i3c_dev is null and we
dereference it:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_group_devs_in_slots() {
...
/*
* Swap the I3C device on i2c_pos slot with the I2C dev
* on pos slot.
*/
i3c_addr = i3c->addrs[i2c_pos];
i3c_dev = i3c_addr.i3c_dev;
i2c_addr = i3c->addrs[pos];
i2c_dev = i2c_addr.i2c_dev;
new_i2c_data = i3c_dev_get_master_data(i3c_dev);
...
}
It also looks like the function moves the I2C device to i2c_pos but fails
to update the i3c->free_pos bitmask, causing state corruption where the
controller considers the new I2C slot as free. Does this bitmask need to
be updated when swapping devices?
[Severity: High]
This isn't a bug introduced by this patch, but since we are cleaning the
DATBAS registers on detach to avoid tx/rx logic issues, do we also need to
clean them after Dynamic Address Assignment for unassigned slots?
In renesas_i3c_daa(), all free DATBAS registers are pre-configured before
the ENTDAA command:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
...
ret = i3c_master_get_free_addr(m, last_addr + 1);
if (ret < 0)
return -ENOSPC;
i3c->addrs[pos].addr = ret;
last_addr = ret;
renesas_writel(i3c->regs, DATBAS(pos),
datbas_dvdyad_with_parity(ret));
}
...
}
After the ENTDAA command completes, it uses the remaining count
(cmd->rx_count) to determine how many devices were actually assigned.
However, it appears the driver never clears DATBAS(pos) and
i3c->addrs[pos].addr for the remaining unassigned slots.
Could leaving invalid dynamic addresses in the DATBAS registers cause the
controller's tx/rx logic to misbehave?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=7
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 01/16] i3c: renesas: Check that the transfer is valid before accessing it
From: sashiko-bot @ 2026-06-12 16:20 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: Frank.Li, linux-i3c
In-Reply-To: <20260612160458.3102106-2-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
New issues:
- [High] The `!xfer` early return path in `renesas_i3c_rx_isr` fails to disable the `NTIE_RDBFIE0` interrupt, causing an interrupt storm.
- [High] A typo in the `!xfer` early return path of `renesas_i3c_tend_isr` duplicates the `BIE_TENDIE` flag, failing to mask the NACK interrupt.
- [High] The `!xfer` early return path in `renesas_i3c_start_isr` fails to disable the `BIE_STCNDDIE` interrupt.
Pre-existing issues:
- [Critical] The transfer functions `renesas_i3c_i3c_xfers` and `renesas_i3c_i2c_xfers` silently ignore timeouts and hardware errors, falsely returning success.
- [Critical] Hardware State Desync causing cross-transfer corruption due to failure to abort hardware on timeout.
- [Critical] FIFO overflow in I3C writes due to unconditionally writing the entire transfer length without progress tracking.
- [Critical] Out-of-bounds write and pointer offset corruption in `renesas_i3c_rx_isr` during I3C reads.
- [High] Hardware interrupt registers are modified via non-atomic read-modify-write operations from process context without locking, causing lost mask updates.
--
commit 80958e2863c878df9b87948c2774499050e47aee
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Check that the transfer is valid before accessing it
The Renesas I3C driver uses an asynchronous model to transfer data. It
prepares a struct renesas_i3c_xfer, enqueues it, and waits for completion.
The interrupt handler dequeues the transfer, updates/uses it, and signals
the waiting thread.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca2..bc1a0ae1d12d8 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -1138,6 +1153,15 @@ static irqreturn_t renesas_i3c_tend_isr(int irq, void *data)
>
> scoped_guard(spinlock, &i3c->xferqueue.lock) {
> xfer = i3c->xferqueue.cur;
> + if (!xfer) {
> + /* Disable interrupts. */
> + renesas_clear_bit(i3c->regs, BIE, BIE_TENDIE | BIE_TENDIE);
[Severity: High]
Does this duplicated flag fail to mask the NACK interrupt?
It appears the code is meant to mask both the Transmit End and NACK
interrupts (as the next line clears BST_NACKDF | BST_TENDF).
The failure to include BIE_NACKDIE leaves the NACK interrupt unmasked,
which could cause an interrupt storm if a NACK triggered the ISR.
> + renesas_clear_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
> + /* Clear any status flag. */
> + renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF);
> + return IRQ_HANDLED;
> + }
[ ... ]
> @@ -1184,6 +1208,14 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
>
> scoped_guard(spinlock, &i3c->xferqueue.lock) {
> xfer = i3c->xferqueue.cur;
> + if (!xfer) {
> + /* Clear any status registers. */
> + renesas_clear_bit(i3c->regs, BST, BST_SPCNDDF);
> + /* Clear the Read Buffer Full status flag. */
> + renesas_clear_bit(i3c->regs, NTST, NTST_RDBFF0);
> + return IRQ_HANDLED;
> + }
[Severity: High]
Can this early return path cause an interrupt storm?
When a Read Buffer Full interrupt fires, this attempts to handle an aborted
transfer by simply clearing the status flag NTST_RDBFF0.
Because the hardware fifo remains full and the interrupt enable bit
NTIE_RDBFIE0 is not disabled, the interrupt will immediately re-trigger
when the ISR returns.
[ ... ]
> @@ -1259,6 +1293,12 @@ static irqreturn_t renesas_i3c_start_isr(int irq, void *data)
>
> scoped_guard(spinlock, &i3c->xferqueue.lock) {
> xfer = i3c->xferqueue.cur;
> + if (!xfer) {
> + /* Clear any status registers. */
> + renesas_clear_bit(i3c->regs, BST, BST_STCNDDF);
> + return IRQ_HANDLED;
> + }
[Severity: High]
Does this early return path fail to disable the Start Condition interrupt?
The early return clears the BST_STCNDDF status flag but doesn't disable the
Start Condition interrupt via BIE_STCNDDIE.
This could leave the interrupt permanently enabled, leading to stray
interrupts whenever a Start condition is detected on the bus.
Additionally, while reviewing this file, I noticed a few pre-existing regressions:
[Severity: High]
This is a pre-existing issue, but are hardware interrupt registers modified
without locking, potentially causing lost mask updates?
In drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers():
renesas_set_bit(i3c->regs, BIE, BIE_NACKDIE);
renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
renesas_set_bit(i3c->regs, BIE, BIE_STCNDDIE);
These configure hardware interrupt registers using renesas_set_bit() from
process context without holding the i3c->xferqueue.lock spinlock.
renesas_set_bit() performs a non-atomic read-modify-write sequence. If a
hardware interrupt fires between the read and the write, the ISR's interrupt
disabling operations (which properly hold the lock) could be entirely
overwritten and lost when the process context writes its stale value back.
[Severity: High]
This is a pre-existing issue, but does renesas_i3c_i3c_xfers() silently ignore
timeouts and hardware errors?
In drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers():
time_left = renesas_i3c_wait_xfer(i3c, xfer);
if (!time_left)
xfer_failed = true;
if (xfer_failed)
renesas_i3c_irqs_mask_and_clear(i3c);
return 0;
If renesas_i3c_wait_xfer() times out, xfer_failed is set to true, but the
loop continues processing subsequent transfers, and the function
unconditionally returns 0 at the end.
Hardware errors propagated via xfer->ret are never checked or returned,
which could lead to silent data corruption as subsystems assume transfers
succeeded.
[Severity: High]
This is a pre-existing issue, but can a timeout cause hardware state desync
and cross-transfer corruption?
In drivers/i3c/master/renesas-i3c.c:renesas_i3c_wait_xfer():
time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
if (!time_left)
renesas_i3c_dequeue_xfer(i3c, xfer);
When this times out, it dequeues xfer from the software queue but does not
instruct the hardware controller to abort the ongoing physical transaction.
The caller loop in renesas_i3c_i3c_xfers() then immediately queues the next
transfer using the exact same xfer memory allocation:
struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
Because the hardware is still processing the first transfer, its delayed
interrupt will match !xfer == false (as cur now points to the same re-queued
xfer structure). The ISR could complete the second transfer prematurely using
the results of the first.
[Severity: High]
This is a pre-existing issue, but can I3C writes overflow the hardware fifo
due to unconditionally writing the entire transfer length?
In drivers/i3c/master/renesas-i3c.c:renesas_i3c_tx_isr():
} else {
i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
}
Both the initial enqueue and the tx interrupt handler blindly call
i3c_writel_fifo() with cmd->len bytes. The hardware fifo depth is fixed at 16
words (64 bytes).
If cmd->len is larger than 64, the fifo could immediately overflow and
corrupt the transfer and hardware state. Also, since cmd->tx_count is
neither used nor updated, each subsequent tx interrupt would repeatedly
write the entire buffer from offset 0.
[Severity: High]
This is a pre-existing issue, but can I3C reads cause an out-of-bounds write
and pointer offset corruption?
In drivers/i3c/master/renesas-i3c.c:renesas_i3c_rx_isr():
} else {
read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
cmd->rx_count = read_bytes;
}
When an I3C read triggers, it calculates read_bytes by multiplying the number
of 32-bit words in the hardware fifo by sizeof(u32).
If the requested read length is unaligned (e.g., 2 bytes), reading 4 bytes
could corrupt the adjacent heap memory. Moreover, the pointer cmd->rx_buf is
never advanced, and cmd->rx_count is assigned rather than incremented,
meaning multi-part transfers could repeatedly overwrite the beginning of the
buffer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 12/16] i3c: renesas: Update HW registers after SW computations are done
From: sashiko-bot @ 2026-06-12 16:19 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-i3c, Frank.Li
In-Reply-To: <20260612160458.3102106-13-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Division by zero crash if `i2c-scl-hz` property is omitted in pure mode.
- [Medium] Potential division by zero crash and precision loss in open-drain timing calculations.
--
commit a5b09dc4623aa6e4464b99ec9c493bf28137c24d
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Update HW registers after SW computations are done
The patch defers hardware register writes until after software calculations
and cache updates are complete in renesas_i3c_bus_init(), aiming to
support future runtime PM enhancements.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index d5247e841a838..96d4fed32bf95 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -560,10 +560,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
> if (!i3c->rate)
> return -EINVAL;
>
> - ret = renesas_i3c_reset(i3c);
> - if (ret)
> - return ret;
> -
> i2c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i2c);
[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but can
this cause a division by zero crash?
If the i2c-scl-hz device tree property is not present, of_populate_i3c_bus()
leaves master->bus.scl_rate.i2c as 0. In pure mode (I3C_BUS_MODE_PURE),
i3c_bus_set_mode() explicitly does not initialize scl_rate.i2c, which
would leave it as 0 here.
[ ... ]
> ret = i3c_master_get_free_addr(m, 0);
> if (ret < 0)
> return ret;
>
> + info.dyn_addr = ret;
> i3c->dyn_addr = ret;
> - renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(ret) | MSDVAD_MDYADV);
> + i3c->refclk_div = cks;
> +
> + ret = renesas_i3c_reset(i3c);
> + if (ret)
> + return ret;
[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but does
the timing calculation in this function risk a division by zero or precision
loss?
In renesas_i3c_bus_init(), there are calculations similar to:
od_low_ticks -= t.scl_fall_ns / (NSEC_PER_SEC / i3c->rate) + 1;
If the input clock rate i3c->rate exceeds 1 GHz (NSEC_PER_SEC), the inner
division evaluates to 0, which would cause a divide-by-zero kernel panic.
Even for rates under 1 GHz, this pattern causes severe precision loss for
non-integer multiples of 1 GHz.
Would using a pattern like DIV_ROUND_UP_ULL((u64)ns * rate, NSEC_PER_SEC)
be safer here to prevent both the crash and precision loss?
> + renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
> + renesas_writel(i3c->regs, EXTBR, i3c->extbr);
> + renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
> + renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(i3c->dyn_addr) | MSDVAD_MDYADV);
> +
> + /* I3C hw init*/
> + renesas_i3c_hw_init(i3c);
>
> - info.dyn_addr = ret;
> return i3c_master_set_info(&i3c->base, &info);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=12
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 06/16] i3c: renesas: Perform Dynamic Address Assignment on resume
From: sashiko-bot @ 2026-06-12 16:19 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-i3c, Frank.Li
In-Reply-To: <20260612160458.3102106-7-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `renesas_i3c_group_devs_in_slots()` crashes due to NULL pointer dereference when swapping an I2C device into a previously empty slot.
- [High] Slot state corruption and hardware configuration overwrite during device grouping.
- [High] Build failure and state corruption due to calling undefined function `i3c_master_reattach_i3c_dev_locked()` and bypassing PID matching.
--
commit e2b35e6a6eb1e4e88dd575a2f0b9fe6f7bf5f7c5
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Perform Dynamic Address Assignment on resume
The Renesas RZ/G3S SoC supports a power saving mode where power to most
SoC components, including I3C, is turned off. Running DAA in the controller
resume path restores communication for devices that also lose power.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 67b9a31300d09..a63edddbc9fe0 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -637,12 +646,80 @@ static void renesas_i3c_bus_cleanup(struct i3c_master_controller *m)
> renesas_i3c_reset(i3c);
> }
>
> +static u8 renesas_i3c_group_devs_in_slots(struct renesas_i3c *i3c)
> +{
> + struct renesas_i3c_i2c_dev_data *new_i2c_data, *new_i3c_data;
> + struct renesas_i3c_addr i2c_addr, i3c_addr;
> + struct i3c_dev_desc *i3c_dev;
> + struct i2c_dev_desc *i2c_dev;
> + u8 i2c_pos = 0;
[ ... ]
> + for (u8 pos = 0; pos < i3c->maxdevs; pos++) {
> + if (i3c->free_pos & BIT(pos))
> + continue;
> +
> + if (!i3c->addrs[pos].is_i2c)
> + continue;
> +
> + if (pos == i2c_pos) {
> + i2c_pos++;
> + continue;
> + }
> +
> + /*
> + * Swap the I3C device on i2c_pos slot with the I2C dev
> + * on pos slot.
> + */
> + i3c_addr = i3c->addrs[i2c_pos];
> + i3c_dev = i3c_addr.i3c_dev;
> +
> + i2c_addr = i3c->addrs[pos];
> + i2c_dev = i2c_addr.i2c_dev;
> +
> + new_i2c_data = i3c_dev_get_master_data(i3c_dev);
[Severity: High]
Does this crash when swapping an I2C device into a previously empty slot?
When i2c_pos does not advance over empty slots, i3c_dev will be NULL if
the destination slot is empty. Since i3c_dev_get_master_data() unconditionally
dereferences the pointer (dev->common.master_priv), it seems this would cause
a kernel panic when swapping into an empty slot.
> + new_i3c_data = i2c_dev_get_master_data(i2c_dev);
> +
> + i3c->addrs[i2c_pos] = i2c_addr;
> + i3c->addrs[i2c_pos].is_i2c = true;
> + i3c->addrs[i2c_pos].i2c_dev = i2c_dev;
> +
> + i3c->addrs[pos] = i3c_addr;
> + i3c->addrs[pos].is_i2c = false;
> + i3c->addrs[pos].i3c_dev = i3c_dev;
> +
> + new_i2c_data->index = i2c_pos++;
> + new_i3c_data->index = pos;
> +
> + i2c_dev_set_master_data(i2c_dev, new_i2c_data);
> + i3c_dev_set_master_data(i3c_dev, new_i3c_data);
> + }
[ ... ]
> @@ -655,16 +732,25 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
> olddevs = ~(i3c->free_pos);
> i3c->internal_state = I3C_INTERNAL_STATE_CONTROLLER_ENTDAA;
>
> + if (i3c->resuming)
> + first_i3c_pos = renesas_i3c_group_devs_in_slots(i3c);
> +
> /* Setting DATBASn registers for target devices. */
> for (pos = 0; pos < i3c->maxdevs; pos++) {
> - if (olddevs & BIT(pos))
> - continue;
> + if (olddevs & BIT(pos)) {
[Severity: High]
Does this clobber the configuration of I2C devices swapped into empty slots?
Since renesas_i3c_group_devs_in_slots() does not update i3c->free_pos during
a swap, the olddevs mask calculated before the grouping is now stale.
If an I2C device was moved to a previously empty slot, olddevs & BIT(pos) will
evaluate to false. This bypasses the skip logic, treats the newly moved I2C
device as an unassigned slot, and generates a new dynamic address for it,
blindly overwriting its DATBAS hardware configuration.
[ ... ]
> @@ -689,12 +780,38 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>
> renesas_i3c_wait_xfer(i3c, xfer);
>
> - newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> - newdevs &= ~olddevs;
> + newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
> + /* Re-attach all the I3C devices on resume. */
> + if (!i3c->resuming)
> + newdevs &= ~olddevs;
>
> for (pos = 0; pos < i3c->maxdevs; pos++) {
> - if (newdevs & BIT(pos))
> - i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
> + if (!(newdevs & BIT(pos)))
> + continue;
> +
> + /*
> + * If the bus was fully occupied before suspend, re-attach the
> + * devices and update the addresses tracked by the subsystem.
> + * Without this, i3c_master_add_i3c_dev_locked() returns errors
> + * due to a lack of free slots (as reported by
> + * renesas_i3c_get_free_pos()), causing the attachment to fail.
> + *
> + * Otherwise, follow the normal
> + * i3c_master_add_i3c_dev_locked() path, which will re-attach the
> + * devices, keep the subsystem state and driver addresses in sync,
> + * and perform all the required internal bookkeeping.
> + */
> + if (!i3c->free_pos && i3c->resuming) {
> + struct i3c_dev_desc *dev = i3c->addrs[pos].i3c_dev;
> + u8 old_dyn_addr;
> +
> + old_dyn_addr = dev->info.dyn_addr;
> + dev->info.dyn_addr = i3c->addrs[pos].addr;
> +
> + i3c_master_reattach_i3c_dev_locked(dev, old_dyn_addr);
[Severity: High]
This calls i3c_master_reattach_i3c_dev_locked() which does not appear to exist
in the kernel, causing a build failure.
Also, does this logic cross-wire physical devices with the wrong software
structures?
It blindly assigns newly generated dynamic addresses to the pre-suspend
device structures based solely on their slot index (pos). However, ENTDAA
assigns addresses based on Provisioned ID (PID) arbitration order, not the
previous slot indices. If the bus topology changes or devices arbitrate in a
different order, commands might be sent to the incorrect targets, potentially
causing data corruption.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=6
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 05/16] i3c: renesas: Reset the controller on resume
From: sashiko-bot @ 2026-06-12 16:18 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-i3c, Frank.Li
In-Reply-To: <20260612160458.3102106-6-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Missing hardware cancellation on transfer timeout causes use-after-free and memory corruption.
- [High] The driver ignores transfer timeouts and errors, silently reporting success.
--
commit 06142498cb9164f084dca924bf2625138121a705
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Reset the controller on resume
This commit updates renesas_i3c_reset() to use read_poll_timeout_atomic()
since the resume callback runs in the noirq phase. It also resets the
controller on resume to follow the probe sequence and avoid ordering
related failures.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 4b81c32b1fdec..67b9a31300d09 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -1504,6 +1508,8 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>
> return 0;
>
> +err_clks_disable:
> + clk_bulk_disable(i3c->num_clks, i3c->clks);
> err_presetn:
> reset_control_assert(i3c->presetn);
> err_tresetn:
[Severity: Critical]
This is a pre-existing issue, but does the driver have a structural flaw
with transfer timeouts causing memory corruption?
When a transfer times out in renesas_i3c_wait_xfer(), the transfer is
dequeued and can be freed:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_wait_xfer() {
...
if (!time_left)
renesas_i3c_dequeue_xfer(i3c, xfer);
...
}
However, the active hardware transaction is never aborted. If the target
eventually completes the transaction, the hardware will write a response
descriptor to NRSPQP.
When a subsequent transfer is initiated, the driver re-enables the interrupt,
and the ISR will fire, picking up the new transfer:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
...
xfer = i3c->xferqueue.cur;
...
data_len = NRSPQP_DATA_LEN(resp_descriptor);
...
i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
...
}
Will the ISR blindly read the stale data from the old completed hardware
transaction into the new transfer's rx_buf?
If the old transfer's length exceeds the new transfer's buffer size, couldn't
this overflow the buffer and cause memory corruption?
[Severity: High]
This is a pre-existing issue, but does the driver ignore transfer timeouts
and errors, silently reporting success?
In renesas_i3c_i3c_xfers(), the function ignores transfer timeouts and errors,
returning 0 unconditionally:
drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
...
if (xfer_failed)
renesas_i3c_irqs_mask_and_clear(i3c);
return 0;
}
Similarly, in renesas_i3c_i2c_xfers():
drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
...
if (xfer_failed)
renesas_i3c_irqs_mask_and_clear(i3c);
return cmd->err;
}
Since cmd->err is only updated to a negative code by the ISR upon an explicit
NACK, it remains 0 on timeout. Doesn't this falsely claim that the message
was successfully delivered to the core I3C subsystem?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=5
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 03/16] i3c: renesas: Follow the reset deassert order used in probe
From: sashiko-bot @ 2026-06-12 16:17 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: Frank.Li, linux-i3c
In-Reply-To: <20260612160458.3102106-4-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] In `renesas_i3c_i2c_xfers`, unhandled timeouts cause the loop to proceed, risking data corruption and silent failures.
- [High] In `renesas_i3c_i3c_xfers`, transfer statuses and errors are completely ignored, unconditionally returning 0.
- [High] Missing hardware abort sequence on transfer timeout risks controller lockup.
--
commit 85523970e99a91763d831b25e5f1d70eaf5d402f
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Follow the reset deassert order used in probe
Use the same reset deassert order in the resume and probe paths to avoid
potential failures due to ordering differences.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index e70db3a17bc72..7d448936b74c1 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1457,17 +1457,17 @@ static int renesas_i3c_resume_noirq(struct device *dev)
> struct renesas_i3c *i3c = dev_get_drvdata(dev);
> int i, ret;
>
> - ret = reset_control_deassert(i3c->presetn);
> + ret = reset_control_deassert(i3c->tresetn);
> if (ret)
> return ret;
[ ... ]
While reviewing this file, I noticed a few pre-existing issues that might need
your attention:
[Severity: High]
This is a pre-existing issue, but does this code properly handle timeouts
during I2C transfers?
drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
...
renesas_set_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
if (cmd->err)
break;
start_bit = CNDCTL_SRCND;
...
}
If wait_for_completion_timeout() times out, its return value is discarded and
cmd->err remains 0. The loop condition if (cmd->err) break; does not trigger,
and the loop proceeds to the next iteration.
Can this overwrite shared state and issue a new START condition while the
hardware might still be processing the previous request? This could lead to
silent failures and controller desynchronization since the function eventually
returns success despite the timeout.
[Severity: High]
This is a pre-existing issue, but does renesas_i3c_i3c_xfers() ignore hardware
errors and timeouts?
drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
...
if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
if (cmd->len > NTDTBP0_DEPTH * sizeof(u32))
renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
}
renesas_i3c_wait_xfer(i3c, xfer);
}
return 0;
}
The loop iterates over i3c_nxfers and waits for each transfer using
renesas_i3c_wait_xfer(), but completely ignores xfer->ret which is populated
by the ISR with actual hardware error statuses like NACK or parity errors.
The function then unconditionally returns 0. Could masking all hardware errors
and timeouts from the upper layers lead to subsequent silent failures?
[Severity: High]
This is a pre-existing issue, but is there a missing hardware abort sequence
when a transfer times out?
drivers/i3c/master/renesas-i3c.c:renesas_i3c_wait_xfer() {
...
static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
{
unsigned long time_left;
renesas_i3c_enqueue_xfer(i3c, xfer);
time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
if (!time_left)
renesas_i3c_dequeue_xfer(i3c, xfer);
}
...
}
When a transfer times out, the software state is dequeued via
renesas_i3c_dequeue_xfer(). However, it does not appear to set the BCTL_ABT
bit or reset the controller, leaving the hardware in an indeterminate busy
state.
When the next transfer is enqueued, renesas_i3c_start_xfer_locked()
indiscriminately writes new commands to the hardware command queue (NCMDQP).
Could this cause a bus collision or controller lockup?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=3
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* Re: [PATCH v4 02/16] i3c: renesas: Restore STDBR and EXTBR registers on resume
From: sashiko-bot @ 2026-06-12 16:17 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-i3c, Frank.Li
In-Reply-To: <20260612160458.3102106-3-claudiu.beznea@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Silent data truncation in EXTBR register configuration for slow clock speeds.
--
commit decdc939209046d74f5ae66361c115e1c225a379
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Restore STDBR and EXTBR registers on resume
This commit restores the STDBR and EXTBR registers during the resume process
for the Renesas RZ/G3S. This ensures the controller maintains the same state
after a suspend with power loss.
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -607,10 +608,9 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
> renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
>
> /* Extended Bit Rate setting */
> - renesas_writel(i3c->regs, EXTBR, EXTBR_EBRLO(od_low_ticks) |
> - EXTBR_EBRHO(od_high_ticks) |
> - EXTBR_EBRLP(pp_low_ticks) |
> - EXTBR_EBRHP(pp_high_ticks));
> + i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
> + EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
[Severity: High]
This is a pre-existing issue, but does this code silently truncate
od_low_ticks and od_high_ticks when the clock is slow?
For slow I2C speeds, od_low_ticks and od_high_ticks can reach up to 510
because the clock divider loop only ensures (od_low_ticks / 2) <= 0xFF.
The STDBR register configuration applies a right shift when double_SBR is
set to avoid overflow.
Since the EXTBR_EBRLO and EXTBR_EBRHO macros use 8-bit masks, and this line
doesn't apply the double_SBR right shift, won't any tick value greater than
255 get truncated by the bitwise AND and result in incorrect extended bit
rate timings?
> + renesas_writel(i3c->regs, EXTBR, i3c->extbr);
>
> renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
> i3c->refclk_div = cks;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=2
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply
* [PATCH v4 16/16] i3c: renesas: Add runtime PM support
From: Claudiu Beznea @ 2026-06-12 16:04 UTC (permalink / raw)
To: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel
Cc: claudiu.beznea, linux-i3c, linux-kernel, claudiu.beznea,
Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
On the SoCs where the Renesas I3C driver is enabled (RZ/G3S and RZ/G3E),
the clocks of the IP are managed through a clock PM domain. To keep the
I3C code simpler, the explicit clock handling was dropped along with the
addition of runtime PM support, in favor of the runtime PM APIs. Only the
code for getting tclk was preserved, as it is necessary to compute the
I3C clock rate.
All the APIs provided to the I3C subsystem through struct
i3c_master_controller_ops are guarded with runtime PM APIs to
enable/disable the controller at runtime.
As the Renesas I3C driver implements an asynchronous transmit model by
preparing a transfer and waiting for its completion through the ISR,
renesas_i3c_abort_xfer() was added to disable interrupts and clear any
pending IRQ status bits when there is no completion in the defined
timeout. Along with this, renesas_i3c_wait_xfer() return type was changed
to unsigned long.
Add runtime PM support for the Renesas I3C driver.
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v4:
- renamed renesas_i3c_abort_xfer() from v3 to
renesas_i3c_irqs_mask_and_clear(); along with it renamed local varibles
abort_xfer to xfer_failed
Changes in v3:
- dropped the RPM resume/suspend in renesas_i3c_suspend() to read the
DATBASn registers as the DATBASn register are not used on suspend/resume
path anymore in this version
Changes in v2:
- dropped the runtime suspend/resume functions as for now, there will be
no pinctrl sleep state described in DT
- do not synchronize the IRQs in renesas_i3c_abort_xfer() as some handlers
may re-enable interrupts; instead just disable the interrupts and clean
any status bits that the IRQ handlers are touching; with this the
struct renesas_i3c::{irqs, num_irqs} and the associated code was removed
- dropped the renesas_i3c_dont_use_autosuspend() along with the
devm_add_action_or_reset() call to set it since the same operation is
done by the devres cleanup helper of devm_pm_runtime_enable()
- adjusted the renesas_i3c_suspend() with RPM calls to save the DATBAS
registers
- use pm_runtime_resume_and_get() in renesas_i3c_resume() to avoid
mixing gotos with cleanup helpers
- adjusted the patch description to reflect these changes
drivers/i3c/master/renesas-i3c.c | 150 +++++++++++++++++++++++++------
1 file changed, 122 insertions(+), 28 deletions(-)
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 1bed2304b5ee..e0e75144440e 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -22,6 +22,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/reset.h>
#include <linux/slab.h>
#include "../internals.h"
@@ -199,8 +200,6 @@
#define RENESAS_I3C_MAX_DEVS 8
#define I2C_INIT_MSG -1
-#define RENESAS_I3C_TCLK_IDX 1
-
enum i3c_internal_state {
I3C_INTERNAL_STATE_DISABLED,
I3C_INTERNAL_STATE_CONTROLLER_IDLE,
@@ -263,9 +262,10 @@ struct renesas_i3c_addr {
struct renesas_i3c {
void __iomem *regs;
- struct clk_bulk_data *clks;
+ struct clk *tclk;
struct reset_control *presetn;
struct reset_control *tresetn;
+ struct device *dev;
struct renesas_i3c_xferqueue xferqueue;
struct i3c_master_controller base;
struct renesas_i3c_addr addrs[RENESAS_I3C_MAX_DEVS];
@@ -278,7 +278,6 @@ struct renesas_i3c {
u32 i3c_STDBR;
u32 extbr;
u16 maxdevs;
- u8 num_clks;
u8 refclk_div;
};
@@ -443,7 +442,24 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
}
}
-static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
+static void renesas_i3c_irqs_mask_and_clear(struct renesas_i3c *i3c)
+{
+ guard(spinlock_irqsave)(&i3c->xferqueue.lock);
+
+ /* Disable all the interrupts */
+ renesas_writel(i3c->regs, BIE, 0);
+ renesas_writel(i3c->regs, NTIE, 0);
+
+ /* Clear normal transfer status flags. */
+ renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0 | NTST_RSPQFF |
+ NTST_TEF | NTST_TABTF);
+ /* Clear bus status flags. */
+ renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF | BST_SPCNDDF);
+ /* Clear error flags. */
+ renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
+}
+
+static unsigned long renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
{
unsigned long time_left;
@@ -452,6 +468,8 @@ static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xf
time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
if (!time_left)
renesas_i3c_dequeue_xfer(i3c, xfer);
+
+ return time_left;
}
static void renesas_i3c_set_prts(struct renesas_i3c *i3c, u32 val)
@@ -485,6 +503,12 @@ static void renesas_i3c_bus_enable(struct i3c_master_controller *m, bool i3c_mod
static int renesas_i3c_reset(struct renesas_i3c *i3c)
{
u32 val;
+ int ret;
+
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (ret)
+ return ret;
renesas_writel(i3c->regs, BCTL, 0);
renesas_set_bit(i3c->regs, RSTCTL, RSTCTL_RI3CRST);
@@ -556,7 +580,7 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
int od_high_ticks, od_low_ticks, i2c_total_ticks;
int ret;
- i3c->rate = clk_get_rate(i3c->clks[RENESAS_I3C_TCLK_IDX].clk);
+ i3c->rate = clk_get_rate(i3c->tclk);
if (!i3c->rate)
return -EINVAL;
@@ -627,6 +651,11 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
if (ret)
return ret;
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (ret)
+ return ret;
+
renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
renesas_writel(i3c->regs, EXTBR, i3c->extbr);
renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
@@ -716,6 +745,7 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
{
struct renesas_i3c *i3c = to_renesas_i3c(m);
struct renesas_i3c_cmd *cmd;
+ unsigned long time_left;
u32 olddevs, newdevs;
u8 last_addr = 0, pos;
u8 first_i3c_pos = 0;
@@ -729,6 +759,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
cmd = xfer->cmds;
cmd->rx_count = 0;
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (ret)
+ return ret;
+
/* Enable I3C bus. */
renesas_i3c_bus_enable(m, true);
@@ -777,7 +812,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
- renesas_i3c_wait_xfer(i3c, xfer);
+ time_left = renesas_i3c_wait_xfer(i3c, xfer);
+ if (!time_left)
+ renesas_i3c_irqs_mask_and_clear(i3c);
newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
/* Re-attach all the I3C devices on resume. */
@@ -865,6 +902,7 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
{
struct renesas_i3c *i3c = to_renesas_i3c(m);
struct renesas_i3c_cmd *cmd;
+ unsigned long time_left;
int ret, pos = 0;
if (ccc->id & I3C_CCC_DIRECT) {
@@ -882,6 +920,11 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
cmd->rnw = ccc->rnw;
cmd->cmd0 = 0;
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (ret)
+ return ret;
+
renesas_i3c_bus_enable(m, true);
/* Calculate the command descriptor. */
@@ -916,7 +959,9 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
}
}
- renesas_i3c_wait_xfer(i3c, xfer);
+ time_left = renesas_i3c_wait_xfer(i3c, xfer);
+ if (!time_left)
+ renesas_i3c_irqs_mask_and_clear(i3c);
ret = xfer->ret;
if (ret)
@@ -931,7 +976,9 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
struct i3c_master_controller *m = i3c_dev_get_master(dev);
struct renesas_i3c *i3c = to_renesas_i3c(m);
struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
- int i;
+ unsigned long time_left;
+ bool xfer_failed = false;
+ int i, ret;
struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
if (!xfer)
@@ -939,6 +986,11 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
init_completion(&xfer->comp);
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (ret)
+ return ret;
+
/* Enable I3C bus. */
renesas_i3c_bus_enable(m, true);
@@ -970,9 +1022,14 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
}
- renesas_i3c_wait_xfer(i3c, xfer);
+ time_left = renesas_i3c_wait_xfer(i3c, xfer);
+ if (!time_left)
+ xfer_failed = true;
}
+ if (xfer_failed)
+ renesas_i3c_irqs_mask_and_clear(i3c);
+
return 0;
}
@@ -981,12 +1038,17 @@ static int renesas_i3c_attach_i3c_dev(struct i3c_dev_desc *dev)
struct i3c_master_controller *m = i3c_dev_get_master(dev);
struct renesas_i3c *i3c = to_renesas_i3c(m);
struct renesas_i3c_i2c_dev_data *data;
- int pos;
+ int pos, ret;
pos = renesas_i3c_get_free_pos(i3c);
if (pos < 0)
return pos;
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (ret)
+ return ret;
+
data = kzalloc_obj(*data);
if (!data)
return -ENOMEM;
@@ -1010,7 +1072,12 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
struct renesas_i3c *i3c = to_renesas_i3c(m);
struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
struct i3c_dev_desc *tmp_dev = i3c->addrs[data->index].i3c_dev;
- int pos;
+ int pos, ret;
+
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (ret)
+ return ret;
pos = renesas_i3c_get_free_pos(i3c);
@@ -1040,8 +1107,12 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
struct i3c_master_controller *m = i3c_dev_get_master(dev);
struct renesas_i3c *i3c = to_renesas_i3c(m);
+ int ret;
- renesas_writel(i3c->regs, DATBAS(data->index), 0);
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (!ret)
+ renesas_writel(i3c->regs, DATBAS(data->index), 0);
i3c_dev_set_master_data(dev, NULL);
i3c->addrs[data->index].addr = 0;
@@ -1058,7 +1129,9 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
struct renesas_i3c *i3c = to_renesas_i3c(m);
struct renesas_i3c_cmd *cmd;
u8 start_bit = CNDCTL_STCND;
- int i;
+ unsigned long time_left;
+ bool xfer_failed = false;
+ int i, ret;
if (!i2c_nxfers)
return 0;
@@ -1071,6 +1144,11 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
xfer->is_i2c_xfer = true;
cmd = xfer->cmds;
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+ ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (ret)
+ return ret;
+
renesas_i3c_bus_enable(m, false);
if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
@@ -1097,7 +1175,9 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
renesas_set_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
- wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
+ time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
+ if (!time_left)
+ xfer_failed = true;
if (cmd->err)
break;
@@ -1106,6 +1186,10 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
}
renesas_i3c_dequeue_xfer(i3c, xfer);
+
+ if (xfer_failed)
+ renesas_i3c_irqs_mask_and_clear(i3c);
+
return cmd->err;
}
@@ -1508,12 +1592,16 @@ static int renesas_i3c_probe(struct platform_device *pdev)
if (IS_ERR(i3c->regs))
return PTR_ERR(i3c->regs);
- ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &i3c->clks);
- if (ret <= RENESAS_I3C_TCLK_IDX)
- return dev_err_probe(&pdev->dev, ret < 0 ? ret : -EINVAL,
- "Failed to get clocks (need > %d, got %d)\n",
- RENESAS_I3C_TCLK_IDX, ret);
- i3c->num_clks = ret;
+ i3c->tclk = devm_clk_get(&pdev->dev, "tclk");
+ if (IS_ERR(i3c->tclk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(i3c->tclk), "Failed to get tclk");
+
+ i3c->dev = &pdev->dev;
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 300);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ ret = devm_pm_runtime_enable(&pdev->dev);
+ if (ret)
+ return ret;
i3c->tresetn = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev, "tresetn");
if (IS_ERR(i3c->tresetn))
@@ -1580,8 +1668,6 @@ static int renesas_i3c_suspend(struct device *dev)
if (ret)
goto err_mark_resumed;
- clk_bulk_disable(i3c->num_clks, i3c->clks);
-
return 0;
err_mark_resumed:
@@ -1603,13 +1689,13 @@ static int renesas_i3c_resume(struct device *dev)
if (ret)
return ret;
- ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
+ ret = renesas_i3c_reset(i3c);
if (ret)
goto err_resets_asserted;
- ret = renesas_i3c_reset(i3c);
+ ret = pm_runtime_resume_and_get(dev);
if (ret)
- goto err_clks_disable;
+ goto err_resets_asserted;
/* Re-store I3C registers value. */
renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
@@ -1632,15 +1718,23 @@ static int renesas_i3c_resume(struct device *dev)
i2c_mark_adapter_resumed(&i3c->base.i2c);
+ pm_runtime_put_autosuspend(dev);
+
/*
* I3C devices may have retained their dynamic address anyway. Do not
* fail the resume because of DAA error.
*/
return 0;
-err_clks_disable:
- clk_bulk_disable(i3c->num_clks, i3c->clks);
err_resets_asserted:
+ /*
+ * If this happens, there is no way to recover from this state without
+ * reloading the driver. We want to avoid keeping the reset line
+ * deasserted unnecessarily. The runtime paths will still work correctly
+ * even if the IP registers are accessed while reset is asserted (e.g.
+ * if a runtime path is triggered after a failed resume). Checked on
+ * RZ/G3S.
+ */
reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
return ret;
}
--
2.43.0
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related
* [PATCH v4 15/16] i3c: renesas: Drop unnecessary tab
From: Claudiu Beznea @ 2026-06-12 16:04 UTC (permalink / raw)
To: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel
Cc: claudiu.beznea, linux-i3c, linux-kernel, claudiu.beznea,
Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Remove an unnecessary tab to make the code cleaner.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- collected tags
drivers/i3c/master/renesas-i3c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index a5384c0f71e3..1bed2304b5ee 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -109,7 +109,7 @@
#define NCMDQP_DATA_LENGTH(x) FIELD_PREP(GENMASK(31, 16), x)
#define NRSPQP 0x154 /* Normal Respone Queue */
-#define NRSPQP_NO_ERROR 0
+#define NRSPQP_NO_ERROR 0
#define NRSPQP_ERROR_CRC 1
#define NRSPQP_ERROR_PARITY 2
#define NRSPQP_ERROR_FRAME 3
--
2.43.0
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related
* [PATCH v4 14/16] i3c: renesas: Use the "dev_name:irq_name" format for the interrupt name
From: Claudiu Beznea @ 2026-06-12 16:04 UTC (permalink / raw)
To: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel
Cc: claudiu.beznea, linux-i3c, linux-kernel, claudiu.beznea,
Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Use the "dev_name:irq_name" format for the interrupt names. This makes it
easier to identify interrupts in systems where multiple devices may request
interrupts with the same name.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- collected tags
drivers/i3c/master/renesas-i3c.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index ab73fee1813b..a5384c0f71e3 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1533,12 +1533,19 @@ static int renesas_i3c_probe(struct platform_device *pdev)
return ret;
for (i = 0; i < ARRAY_SIZE(renesas_i3c_irqs); i++) {
+ const char *irqname;
+
ret = platform_get_irq_byname(pdev, renesas_i3c_irqs[i].name);
if (ret < 0)
return ret;
+ irqname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s:%s", dev_name(&pdev->dev),
+ renesas_i3c_irqs[i].desc);
+ if (!irqname)
+ return -ENOMEM;
+
ret = devm_request_irq(&pdev->dev, ret, renesas_i3c_irqs[i].isr,
- 0, renesas_i3c_irqs[i].desc, i3c);
+ 0, irqname, i3c);
if (ret)
return ret;
}
--
2.43.0
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related
* [PATCH v4 13/16] i3c: renesas: Organize structures to avoid unnecessary padding
From: Claudiu Beznea @ 2026-06-12 16:04 UTC (permalink / raw)
To: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel
Cc: claudiu.beznea, linux-i3c, linux-kernel, claudiu.beznea,
Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reorder structure members to reduce padding and improve memory layout.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- collected tags
drivers/i3c/master/renesas-i3c.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 96d4fed32bf9..ab73fee1813b 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -221,19 +221,19 @@ enum renesas_i3c_event {
};
struct renesas_i3c_cmd {
+ const void *tx_buf;
+ void *rx_buf;
+ /* i2c xfer */
+ u8 *i2c_buf;
+ const struct i2c_msg *msg;
+ int i2c_bytes_left;
+ int i2c_is_last;
u32 cmd0;
u32 len;
- const void *tx_buf;
u32 tx_count;
- void *rx_buf;
u32 rx_count;
u32 err;
u8 rnw;
- /* i2c xfer */
- int i2c_bytes_left;
- int i2c_is_last;
- u8 *i2c_buf;
- const struct i2c_msg *msg;
};
struct renesas_i3c_xfer {
@@ -262,22 +262,22 @@ struct renesas_i3c_addr {
};
struct renesas_i3c {
+ void __iomem *regs;
+ struct clk_bulk_data *clks;
+ struct reset_control *presetn;
+ struct reset_control *tresetn;
+ struct renesas_i3c_xferqueue xferqueue;
struct i3c_master_controller base;
+ struct renesas_i3c_addr addrs[RENESAS_I3C_MAX_DEVS];
+ unsigned long rate;
enum i3c_internal_state internal_state;
- u16 maxdevs;
+ bool resuming;
u32 free_pos;
u32 dyn_addr;
u32 i2c_STDBR;
u32 i3c_STDBR;
u32 extbr;
- unsigned long rate;
- struct renesas_i3c_addr addrs[RENESAS_I3C_MAX_DEVS];
- struct renesas_i3c_xferqueue xferqueue;
- void __iomem *regs;
- struct clk_bulk_data *clks;
- struct reset_control *presetn;
- struct reset_control *tresetn;
- bool resuming;
+ u16 maxdevs;
u8 num_clks;
u8 refclk_div;
};
--
2.43.0
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related
* [PATCH v4 12/16] i3c: renesas: Update HW registers after SW computations are done
From: Claudiu Beznea @ 2026-06-12 16:04 UTC (permalink / raw)
To: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel
Cc: claudiu.beznea, linux-i3c, linux-kernel, claudiu.beznea,
Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
renesas_i3c_bus_init() performs a number of computations and software
cache updates, interleaving them with hardware register writes. While
this works today, it makes it harder to minimize the time the controller
must remain powered when runtime PM is introduced.
Perform all software computations and cache updates first, then update
the hardware registers. This prepares for future runtime PM support.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- collected tags
drivers/i3c/master/renesas-i3c.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index d5247e841a83..96d4fed32bf9 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -560,10 +560,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
if (!i3c->rate)
return -EINVAL;
- ret = renesas_i3c_reset(i3c);
- if (ret)
- return ret;
-
i2c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i2c);
i3c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i3c);
@@ -614,27 +610,31 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
STDBR_SBRHO(double_SBR, od_high_ticks) |
STDBR_SBRLP(pp_low_ticks) |
STDBR_SBRHP(pp_high_ticks);
- renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
/* Extended Bit Rate setting */
i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
- renesas_writel(i3c->regs, EXTBR, i3c->extbr);
-
- renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
- i3c->refclk_div = cks;
-
- /* I3C hw init*/
- renesas_i3c_hw_init(i3c);
ret = i3c_master_get_free_addr(m, 0);
if (ret < 0)
return ret;
+ info.dyn_addr = ret;
i3c->dyn_addr = ret;
- renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(ret) | MSDVAD_MDYADV);
+ i3c->refclk_div = cks;
+
+ ret = renesas_i3c_reset(i3c);
+ if (ret)
+ return ret;
+
+ renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
+ renesas_writel(i3c->regs, EXTBR, i3c->extbr);
+ renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
+ renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(i3c->dyn_addr) | MSDVAD_MDYADV);
+
+ /* I3C hw init*/
+ renesas_i3c_hw_init(i3c);
- info.dyn_addr = ret;
return i3c_master_set_info(&i3c->base, &info);
}
--
2.43.0
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related
* [PATCH v4 11/16] i3c: renesas: Drop the explicit memset() call
From: Claudiu Beznea @ 2026-06-12 16:04 UTC (permalink / raw)
To: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel
Cc: claudiu.beznea, linux-i3c, linux-kernel, claudiu.beznea,
Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Drop the explicit memset() call on struct i3c_device_info object, as it is
already initialized at declaration through compiler initialization.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- collected tags
drivers/i3c/master/renesas-i3c.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 883be4326e43..d5247e841a83 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -634,7 +634,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
i3c->dyn_addr = ret;
renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(ret) | MSDVAD_MDYADV);
- memset(&info, 0, sizeof(info));
info.dyn_addr = ret;
return i3c_master_set_info(&i3c->base, &info);
}
--
2.43.0
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related
* [PATCH v4 10/16] i3c: renesas: Follow a unified pattern for transfer and command initialization
From: Claudiu Beznea @ 2026-06-12 16:04 UTC (permalink / raw)
To: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel
Cc: claudiu.beznea, linux-i3c, linux-kernel, claudiu.beznea,
Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Follow a unified pattern for transfer and command initialization across
the driver. This keeps the code cleaner and easier to follow. Also, in
some cases the I3C device was enabled before the transfer data structure
was even allocated.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- collected tags
drivers/i3c/master/renesas-i3c.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index fa9c15381ed6..883be4326e43 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -726,6 +726,10 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
if (!xfer)
return -ENOMEM;
+ init_completion(&xfer->comp);
+ cmd = xfer->cmds;
+ cmd->rx_count = 0;
+
/* Enable I3C bus. */
renesas_i3c_bus_enable(m, true);
@@ -756,10 +760,6 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
}
- init_completion(&xfer->comp);
- cmd = xfer->cmds;
- cmd->rx_count = 0;
-
if (i3c->resuming) {
/* Run ENTDAA for all the I3C devices on the bus, if resuming. */
ret = first_i3c_pos;
@@ -878,13 +878,13 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
if (!xfer)
return -ENOMEM;
- renesas_i3c_bus_enable(m, true);
-
init_completion(&xfer->comp);
cmd = xfer->cmds;
cmd->rnw = ccc->rnw;
cmd->cmd0 = 0;
+ renesas_i3c_bus_enable(m, true);
+
/* Calculate the command descriptor. */
switch (ccc->id) {
case I3C_CCC_SETDASA:
@@ -934,15 +934,15 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
int i;
- /* Enable I3C bus. */
- renesas_i3c_bus_enable(m, true);
-
struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
if (!xfer)
return -ENOMEM;
init_completion(&xfer->comp);
+ /* Enable I3C bus. */
+ renesas_i3c_bus_enable(m, true);
+
for (i = 0; i < i3c_nxfers; i++) {
struct renesas_i3c_cmd *cmd = xfer->cmds;
@@ -1068,12 +1068,12 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
if (!xfer)
return -ENOMEM;
- renesas_i3c_bus_enable(m, false);
-
init_completion(&xfer->comp);
xfer->is_i2c_xfer = true;
cmd = xfer->cmds;
+ renesas_i3c_bus_enable(m, false);
+
if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
cmd->err = -EBUSY;
return cmd->err;
--
2.43.0
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related
* [PATCH v4 09/16] i3c: renesas: Return immediately if there is no transfer
From: Claudiu Beznea @ 2026-06-12 16:04 UTC (permalink / raw)
To: wsa+renesas, tommaso.merciai.xr, alexandre.belloni, Frank.Li,
p.zabel
Cc: claudiu.beznea, linux-i3c, linux-kernel, claudiu.beznea,
Claudiu Beznea
In-Reply-To: <20260612160458.3102106-1-claudiu.beznea@kernel.org>
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
There is no need to allocate a transfer structure when i2c_nxfers is zero.
Return immediately instead of unnecessarily allocating memory.
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- updated patch title
drivers/i3c/master/renesas-i3c.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 9edd2dc96107..fa9c15381ed6 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1061,13 +1061,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
u8 start_bit = CNDCTL_STCND;
int i;
+ if (!i2c_nxfers)
+ return 0;
+
struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
if (!xfer)
return -ENOMEM;
- if (!i2c_nxfers)
- return 0;
-
renesas_i3c_bus_enable(m, false);
init_completion(&xfer->comp);
--
2.43.0
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related
page: next (older)
- 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