* [PATCH v2 1/2] iommu/arm-smmu-v3: Manage teardown with devm
From: Shameer Kolothum @ 2026-06-11 8:42 UTC (permalink / raw)
To: iommu, linux-kernel, linux-arm-kernel
Cc: nicolinc, jgg, joro, will, robin.murphy, nathanc, mochs,
skolothumtho
In-Reply-To: <20260611084205.686559-1-skolothumtho@nvidia.com>
arm_smmu_device_remove() manually frees the IOPF queue, destroys the
vmid_map and disables the device, while the IRQs and queues are devm
managed. devm unwinds only after remove() returns, so the cleanup runs
in the wrong order. The IOPF queue is freed before the event-queue IRQ
whose handler uses it.
Manage all of it with devm so the unwind order is correct. Free the IOPF
queue and vmid_map via devm actions, and disable the device from one
registered after arm_smmu_device_reset().
This is also a prerequisite for fixing a Tegra241 CMDQV CMD_SYNC
use-after-free in the subsequent patch.
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 ++++++++++++++-------
1 file changed, 34 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index e8d7dbe495f0..00261e77e7bc 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -4464,6 +4464,16 @@ int arm_smmu_cmdq_init(struct arm_smmu_device *smmu,
return 0;
}
+static void arm_smmu_free_iopf_action(void *data)
+{
+ iopf_queue_free(data);
+}
+
+static void arm_smmu_destroy_vmid_map(void *data)
+{
+ ida_destroy(data);
+}
+
static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
{
int ret;
@@ -4491,6 +4501,11 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
smmu->evtq.iopf = iopf_queue_alloc(dev_name(smmu->dev));
if (!smmu->evtq.iopf)
return -ENOMEM;
+ ret = devm_add_action_or_reset(smmu->dev,
+ arm_smmu_free_iopf_action,
+ smmu->evtq.iopf);
+ if (ret)
+ return ret;
}
/* priq */
@@ -4569,7 +4584,8 @@ static int arm_smmu_init_strtab(struct arm_smmu_device *smmu)
ida_init(&smmu->vmid_map);
- return 0;
+ return devm_add_action_or_reset(smmu->dev, arm_smmu_destroy_vmid_map,
+ &smmu->vmid_map);
}
static int arm_smmu_init_structures(struct arm_smmu_device *smmu)
@@ -4782,6 +4798,11 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
return ret;
}
+static void arm_smmu_disable_action(void *data)
+{
+ arm_smmu_device_disable(data);
+}
+
static void arm_smmu_write_strtab(struct arm_smmu_device *smmu)
{
struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
@@ -5540,7 +5561,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
/* Initialise in-memory data structures */
ret = arm_smmu_init_structures(smmu);
if (ret)
- goto err_free_iopf;
+ return ret;
/* Record our private device structure */
platform_set_drvdata(pdev, smmu);
@@ -5550,30 +5571,30 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
/* Reset the device */
ret = arm_smmu_device_reset(smmu);
+ if (ret) {
+ arm_smmu_device_disable(smmu);
+ return ret;
+ }
+
+ /* Register last so it unwinds first, while the CMDQ is still up. */
+ ret = devm_add_action_or_reset(smmu->dev, arm_smmu_disable_action, smmu);
if (ret)
- goto err_disable;
+ return ret;
/* And we're up. Go go go! */
ret = iommu_device_sysfs_add(&smmu->iommu, dev, NULL,
"smmu3.%pa", &ioaddr);
if (ret)
- goto err_disable;
+ return ret;
ret = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
if (ret) {
dev_err(dev, "Failed to register iommu\n");
- goto err_free_sysfs;
+ iommu_device_sysfs_remove(&smmu->iommu);
+ return ret;
}
return 0;
-
-err_free_sysfs:
- iommu_device_sysfs_remove(&smmu->iommu);
-err_disable:
- arm_smmu_device_disable(smmu);
-err_free_iopf:
- iopf_queue_free(smmu->evtq.iopf);
- return ret;
}
static void arm_smmu_device_remove(struct platform_device *pdev)
@@ -5582,9 +5603,6 @@ static void arm_smmu_device_remove(struct platform_device *pdev)
iommu_device_unregister(&smmu->iommu);
iommu_device_sysfs_remove(&smmu->iommu);
- arm_smmu_device_disable(smmu);
- iopf_queue_free(smmu->evtq.iopf);
- ida_destroy(&smmu->vmid_map);
}
static void arm_smmu_device_shutdown(struct platform_device *pdev)
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/2] iommu/arm-smmu-v3: Fix Tegra241 CMDQV CMD_SYNC use-after-free
From: Shameer Kolothum @ 2026-06-11 8:42 UTC (permalink / raw)
To: iommu, linux-kernel, linux-arm-kernel
Cc: nicolinc, jgg, joro, will, robin.murphy, nathanc, mochs,
skolothumtho
Hi,
The arm-smmu-v3 probe teardown mixes devm and manual cleanup, so resources
unwind in the wrong order. The IOPF queue is freed before the event-queue
IRQ whose handler uses it. On Tegra241 this is worse: devres frees
smmu->cmdq.q.base before arm_smmu_impl_remove() runs, and the CMDQV
teardown then issues a CMD_SYNC on the freed queue, a use-after-free.
Patch 1 moves the remaining manual teardown (IOPF queue, vmid_map, device
disable) onto devm so the unwind order is correct. Patch 2 adds a
device_disable() impl op and uses it to quiesce the Tegra241 VINTFs
while the CMDQ is still up, fixing the UAF.
v1 is here:
https://lore.kernel.org/linux-iommu/20260529091052.317102-1-skolothumtho@nvidia.com/
Please take a look and let me know.
Thanks,
Shameer
Shameer Kolothum (2):
iommu/arm-smmu-v3: Manage teardown with devm
iommu/tegra241-cmdqv: Fix CMD_SYNC use-after-free on teardown
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 54 +++++++++++++------
.../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 15 +++++-
3 files changed, 52 insertions(+), 18 deletions(-)
--
2.43.0
^ permalink raw reply
* RE: [PATCH v32 0/5] Add ASPEED AST2600 I2C controller driver
From: Ryan Chen @ 2026-06-11 8:42 UTC (permalink / raw)
To: Ryan Chen, jk@codeconstruct.com.au,
andriy.shevchenko@linux.intel.com, Andi Shyti, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Joel Stanley, Andrew Jeffery,
Benjamin Herrenschmidt, Philipp Zabel, william@wkennington.com,
wsa+renesas@sang-engineering.com
Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org,
openbmc@lists.ozlabs.org, Conor Dooley
In-Reply-To: <20260611-upstream_i2c-v32-0-b66eba921d01@aspeedtech.com>
Hello Andi, Wolfram,
Sorry to bother you, can this i2c patch series can be review?
Ryan
> Subject: [PATCH v32 0/5] Add ASPEED AST2600 I2C controller driver
>
> This series adds support for the AST2600 I2C controller “new register set”
> implementation.
>
> The AST2600 I2C controller introduces a revised register layout which
> separates controller and target functionality into distinct register blocks, and
> extends clock divider configuration and packet-based transfer support
> compared to the legacy mixed register layout used on earlier ASPEED SoCs.
>
> The current driver implementation for the AST2600 I2C peripheral is through
> the hardware's "compatibility mode", which exposes a register set that
> matches the previous generation hardware (AST2500 and earlier).
>
> Instead, add a driver that works in new-register-set mode, to allow the new
> features, and will provide support for future hardware that will not implement
> compatibility mode.
>
> In order to support the new mode, we need a DT binding change to reflect the
> reference to the global register set. Since the binding still represents the same
> (AST2600 SoC) physical hardware, we continue to use the existing compatible
> string of "aspeed,ast2600-i2c-bus".
>
> However: since we're changing semantics for an existing binding, we allow
> backwards compatibility by selecting on presence/absence of the newly-added
> properties, and fall back to the old driver (ie., in compatibility mode) when we
> detect a DT using the old binding spec.
>
> Specifically:
>
> - ast2600-i2c-bus nodes that provide the `aspeed,global-regs` property
> (present in the new binding and absent in the legacy binding) will be
> successfully probed by the new driver
>
> - ast2600-i2c-bus nodes without `aspeed,global-regs` continue to use the
> existing driver (in legacy register mode), ensuring that platforms
> with the current DTBs remain functional
>
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
> Changes in v32:
> - 1/5: add MAINTAINERS entry for aspeed,ast2600-i2c.yaml in the same
> patch that creates the file.
> - 3/5: add if/then conditional schema: when aspeed,global-regs is
> present, require reg to have at least two items. The new driver
> unconditionally maps resource index 1 (the buffer SRAM region); a DT
> with one reg entry and aspeed,global-regs passes schema validation but
> fails probe. The constraint makes the schema consistent with driver
> behaviour.
> - 4/5: address follow-on code review issues:
> - add MAINTAINERS entry for drivers/i2c/busses/i2c-ast2600.c.
> - fix interrupt storm when msgs is NULL: clear PKT_DONE in the IRQ
> handler; per the datasheet this auto-clears all associated status
> bits.
> - fix out-of-bounds: guard msgs_index against msgs_count before
> indexing msgs array in ast2600_i2c_controller_packet_irq().
> - fix use-after-free: WRITE_ONCE() null msgs before complete() in
> all IRQ completion paths so trailing IRQs bail out immediately.
> - fix race in timeout path: null msgs before re-enabling IER so a
> late IRQ cannot access the caller's freed message buffer.
> - fix 0-length SMBus block read hanging the bus: issue STOP via
> CONTROLLER_TRIGGER_LAST_STOP, set stop_pending, poll for
> NORMAL_STOP.
> - initialise clk_div_reg to I2CCG_DIV_CTRL and global_ctrl to 0
> to avoid uninitialized values if regmap_read() fails.
> - guard against clock-frequency = <0> in DT; default to 100 kHz
> to prevent divide-by-zero in ast2600_i2c_ac_timing_config().
> - remove AST2600_I2CM_BUS_RECOVER_FAIL from IER writes; bit 15 is
> Reserved in I2CM10 (IER) and only exists as a status bit in I2CM14.
> - 5/5: address follow-on code review issues:
> - fix target RX data loss in master-abort path: remove BUFF_CTRL
> zeroing that discarded pending target RX data stored in bits [29:24].
> - fix use-after-free in master-abort path: null msgs and re-enable
> IER before complete(), not after, preventing stale IRQ from touching
> the newly-installed msgs of the next transfer.
> - fix shared-buffer corruption on coalesced STOP+SLAVE_MATCH IRQ:
> restore the SLAVE_PENDING guard on target_active = false.
> SLAVE_PENDING
> (bit 29) is set when a new address-match is queued before the previous
> DMA receive completes; clearing target_active in that case allows the
> controller to overwrite the shared Tx/Rx buffer.
> - use READ_ONCE() for all process-context reads of target_active;
> the IRQ path writes it with WRITE_ONCE() and plain loads allow the
> compiler to cache a stale value across the IER-disable window.
> - Link to v31:
> https://lore.kernel.org/r/20260603-upstream_i2c-v31-0-ba7a02714f22@aspee
> dtech.com
> Changes in v31:
> - 1/5: clarify in the commit message that the second reg region is
> optional (minItems: 1), matching the schema change from v30.
> - 2/5: zero-initialise struct i2c_timings so the bus-frequency fallback
> correctly triggers when clock-frequency is absent in the DT.
> - 4/5: fix zero-length RX: ast2600_i2c_setup_buff_rx() now returns
> -EINVAL for xfer_len <= 0, propagated through the controller packet
> IRQ handler to abort the transfer instead of hanging until SW timeout.
> - 4/5: address follow-on code review issues:
> - Guard controller_packet_irq() against NULL msgs (post-timeout UAF).
> - Clamp HW-reported xfer_len via ast2600_i2c_clamp_len() in TX_ACK
> and RX_DONE to prevent out-of-bounds writes on HW length glitches.
> - Use regmap_update_bits() for I2CG_CTRL to avoid clobbering shared
> global bits across parallel bus probes (TOCTOU fix).
> - Fix SMBus block read with recv_len == 0: set controller_xfer_cnt =
> msg->len to satisfy the "msg done" check without an extra 1-byte RX.
> - Mirror the controller timeout sequence in recover_bus() timeout path
> (disable IER, synchronize_irq(), W1C ISR, reset master, restore IER).
> - Remove unused #include <linux/of_device.h>.
> - Remove dead adap.algo_data assignment in probe().
> - 5/5: address follow-on target-mode code review issues:
> - Clear target_active on any STOP (not just STOP without SLAVE_PENDING),
> fixing a deadlock under coalesced IRQ events.
> - Enable target IER in reg_target() rather than unconditionally in
> probe(), matching the disable in unreg_target().
> - Re-arm HW in SLAVE_PENDING|RX_DONE|WAIT_TX_DMA|STOP ISR case
> (missing CMD_STS write left bus SCL-stretched until INACTIVE_TO).
> - Default target ISR case: write TARGET_TRIGGER_CMD instead of
> silently breaking, preventing bus hang on unhandled states.
> - W1C-clear ADDR1/2/3_NAK bits in HW in target_irq() to prevent
> stale NAK bits from bouncing controller transfers with -EBUSY.
> - unreg_target(): write 0 to ADDR_CTRL instead of masking with
> ADDR1_MASK, which left ADDR1_ENABLE (BIT(7)) set after unregister.
> - Link to v30:
> https://lore.kernel.org/r/20260528-upstream_i2c-v30-0-5d4f9adc3530@aspee
> dtech.com
>
> Changes in v30:
> - 1/5: aspeed,ast2600-i2c.yaml: keep backward compatibility for
> existing in-tree AST2600 device trees (Sashiko AI review).
> - reg: add minItems: 1 so legacy single-reg DTs still validate.
> - retain bus-frequency as a deprecated property so DTs that still
> use it are not rejected by unevaluatedProperties: false.
> - 2/5: new patch "i2c: aspeed: Read clock-frequency via
> i2c_parse_fw_timings()". The legacy i2c-aspeed driver now reads
> the standard clock-frequency property first and falls back to
> bus-frequency, avoiding a silent 100 kHz downgrade when a DT
> follows the updated binding but still binds to the legacy
> driver (Sashiko AI review).
> - 4/5: address Sashiko AI code review feedback:
> - Use manual i2c_add_adapter() / i2c_del_adapter() instead of
> devm_i2c_add_adapter() so the adapter is torn down before the
> hardware is disabled in remove(); otherwise client .remove()
> callbacks can fail or hang after FUN_CTRL/IER have been cleared.
> - synchronize_irq() and clear pending IRQ status on the controller
> timeout path to avoid the ISR racing with the next transfer and
> touching freed msgs.
> - Use clamp_t() for AC TIMING divisor / scl_low / scl_high so
> extreme clock-frequency values cannot underflow into the unsigned
> domain and corrupt the AC TIMING register.
> - Derive the RX buffer offset from buf_size instead of hardcoding
> 0x10, since the dual-pool split is configurable.
> - Clamp i2c-scl-clk-low-timeout-us to the TTIMEOUT field's 5-bit
> range (max 31 * 1024us) and emit a dev_warn() instead of letting
> AST2600_I2CC_TTIMEOUT()'s mask silently truncate larger values.
> - Return -EBUSY (not -ENOMEM) for every ast2600_i2c_do_start()
> failure path in the controller packet IRQ handler (NORMAL_STOP,
> TX_ACK, and RX_DONE branches).
> - Advertise I2C_AQ_NO_ZERO_LEN_READ via i2c_adapter_quirks so the
> i2c-core rejects zero-byte reads before they reach the driver.
> The AST2600 packet engine cannot encode a zero-length RX command
> and would otherwise stall waiting for an RX_DONE that never
> arrives.
> - 5/5: address Sashiko AI code review feedback:
> - Force-stop path (target IRQ aborting an in-flight controller
> transfer): disable the controller IER and W1C-clear pending ISR
> before calling complete(), then restore the IER after the
> wake-up. Without the disable/clear sequence the controller IRQ
> handler can race with the target abort path and double-complete
> or touch freed msgs.
> - unreg_target() teardown ordering: disable the target IER first,
> then disable SLAVE_EN / clear ADDR_CTRL, synchronize_irq(), W1C
> pending ISR, and only then NULL i2c_bus->target and clear
> target_active. The old order left IER enabled while target was
> being cleared, allowing an in-flight handler to dereference a
> target pointer the caller had already freed.
> - reg_target() bring-up ordering: assign i2c_bus->target before
> enabling SLAVE_EN. Otherwise an IRQ that fires after SLAVE_EN
> is set but before the pointer is stored finds target == NULL,
> exits without clearing the ISR, and the unmasked event re-fires
> as an IRQ storm.
> - Use writel() instead of writeb() when staging a TX byte into
> the target buffer. The AST2600 buffer SRAM only supports 32-bit
> accesses; byte writes are silently dropped (or, on some
> revisions, raise a bus fault), so a SLAVE_READ_REQUESTED reply
> never reaches the master.
> - reg_target() rejects 10-bit client addresses with
> -EAFNOSUPPORT. AST2600_I2CS_ADDR1 is only a 7-bit field;
> without the check, the high bits of a 10-bit address overflow
> into the adjacent ADDR2 field and silently corrupt a second
> target slot.
> - Initialise the local `u8 value` to 0 in the target packet IRQ
> handler. Its address is passed to i2c_slave_event() for events
> such as I2C_SLAVE_STOP / I2C_SLAVE_READ_REQUESTED; a slave
> backend that reads the byte before writing would otherwise leak
> uninitialised kernel stack.
> - Link to v29:
> https://lore.kernel.org/r/20260415-upstream_i2c-v29-0-317c1a905ae1@aspee
> dtech.com
>
> Changes in v29:
> - 2/4: remove aspeed,enable-dma properties.
> - 3/4: update commit message remove transfer mode selection.
> - 3/4: remove sysfs file.
> - 3/4: remove define I2C_TARGET_MSG_BUF_SIZE and
> AST2600_I2C_DMA_SIZE.
> - 3/4: remove buf_index in struct ast2600_i2c_bus.
> - 3/4, 4/4: remove dma/byte mode, use buffer mode only.
> - 4/4: fix race between unreg_target and IRQ handler.
> - 4/4: move i2cs ier enable from ast2600_i2c_init to probe after master ier
> enable.
> - Link to v28:
> https://lore.kernel.org/r/20260330-upstream_i2c-v28-0-17bdae39c5cb@aspee
> dtech.com
>
> Changes in v28:
> - 2/4: update commit message correspond with aspeed,enable-dma.
> - 2/4: remove aspeed,transfer-mode and add aspeed,enable-dma property
> and description.
> - 2/4: Fix aspeed,enable-dma description to reflect hardware capability
> rather than software behavior.
> - 3/4: Separate xfer_mode_store into distinct parse and availability-check
> steps by introducing ast2600_i2c_xfer_mode_check().
> - 3/4: fix tx dma memcpy source point address.
> - 3/4: Use a temporary variable for
> devm_platform_get_and_ioremap_resource()
> to avoid storing an ERR_PTR in i2c_bus->buf_base; drop the redundant
> NULL assignment in the error path since i2c_bus is kzalloc()ed.
> - 3/4: Add ABI documentation file
> Documentation/ABI/testing/sysfs-driver-ast2600-i2c.
> - 4/4: fix typo condication -> condition.
> - 4/4: fix compile error, when disable CONFIG_I2C_SLAVE.
> - Link to v27:
> https://lore.kernel.org/r/20260324-upstream_i2c-v27-0-f19b511c8c28@aspee
> dtech.com
>
> Changes in v27:
> - 1/4 use aspeed,enable-dma instead aspeed,transfer-mode.
> - 2/4 remove aspeed,transfer-mode selection instad aspeed,transfer-mode
> - 2/4 add sysfs for xfer mode.
> - Link to v26:
> https://lore.kernel.org/r/20260309-upstream_i2c-v26-0-5fedcff8ffe8@aspeedt
> ech.com
>
> Changes in v26:
> - 1/4: binding reworks based on review feedback
> - Link to v25:
> https://lore.kernel.org/r/20260225-upstream_i2c-v25-0-9f4bdd954f3f@aspeed
> tech.com
>
> Changes in v25:
> - Use b4 to send series.
> - Rebase on v7.0-rc1.
> - Clarify cover letter and commit logs based on review feedback.
> - Remove the i2c-aspeed-core multiplexer infrastructure and
> implement driver selection via conditional -ENODEV handling
> in individual probe() functions.
> - 3/4: incorporate review feedback and refactor new driver
> - Link to v24:
> https://lore.kernel.org/r/20251118014034.820988-1-ryan_chen@aspeedtech.c
> om
>
> Changes in v24:
> - aspeed,ast2600-i2c.yaml
> - fix make dt_binding_check blank warning.
> - Link to v23:
> https://lore.kernel.org/all/20251117025040.3622984-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v23:
> - update typo patch (1/4) commit message.
> - aspeed,ast2600-i2c.yaml
> - update reg and description.
> - i2c-ast2600.c controller
> - replace ast2600_select_i2c_clock to ast2600_i2c_ac_timing_config.
> - i2c-ast2600.c target
> - I2C_TARGET_MSG_BUF_SIZE 256 to 4096
> - remove blank line.
> - refine Master comment description to controller
> - Link to v22:
> https://lore.kernel.org/all/20251112085649.1903631-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v22:
> - update patch (1/4) commit message add dts example reason.
> - aspeed,ast2600-i2c.yaml @patch (1/4)
> - rename ast2600-i2c.yaml to aspeed,ast2600-i2c.yaml.
> - update reg, clock-frequency description.
> - aspeed,ast2600-i2c.yaml @patch (2/4)
> - aspeed,transfer-mode, aspeed,transfer-mode add for ast2600.
> - i2c-aspeed-core.c,h @patch (3/4)
> - add i2c-aspeed-core allow both old and new device trees using the
> same compatible string "aspeed,ast2600-i2c-bus".
> - Link to v21:
> https://lore.kernel.org/all/20251027061240.3427875-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v21:
> - update patch (1/4) commit message
> - i2c-ast2600.c
> - move rst to local variable in ast2600_i2c_probe().
> - Link to v20:
> https://lore.kernel.org/all/20251021013548.2375190-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v20:
> - ast2600-i2c.yaml
> - fix warning at make dt_binding_check.
> - Link to v19:
> https://lore.kernel.org/all/20251020013200.1858325-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v19:
> - Split AST2600 binding into its own YAML file
> - Removed `aspeed,ast2600-i2c-bus` from `aspeed,i2c.yaml`
> - Added `aspeed,global-regs` and `aspeed,transfer-mode` to AST2600 binding
> - Link to v18:
> https://lore.kernel.org/all/20250820051832.3605405-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v18:
> - refine patch (1/3) commit message (reason for commit not list.)
> - i2c-ast2600.c
> - remove redundant reset_control_deassert in driver probe.
> - remove reset_control_assert(i2c_bus->rst) in driver remove.
> - Link to v17:
> https://lore.kernel.org/all/20250814084156.1650432-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v17:
> - move i2c new mode register and feature into driver commit message.
> - aspeed,i2c.yaml
> - remove multi-master properties.
> - use aspeed,transfer-mode properties for aspeed,enable-byte/enable-dma.
> -i2c-ast2600.c
> - rename dma_safe_buf to controller_dma_safe_buf.
> - fix ast2600_i2c_recover_bus return overflow warnings.
> - add ast2600_i2c_target_packet_buff_irq unhandle case.
> - add parameter "cmd" in ast2600_i2c_setup_dma_rx,
> ast2600_i2c_setup_buff_rx, ast2600_i2c_setup_byte_rx
> - use reset_control_deassert replace
> devm_reset_control_get_shared_deasserted.
> - useaspeed,transfer-mode properties for transfer mode setting.
> - change compatible = "aspeed,ast2600-i2cv2" to "aspeed,ast2600-i2c-bus".
> - Link to v16:
> https://lore.kernel.org/all/20250224055936.1804279-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v16:
> - aspeed,i2c.yaml: add aspeed,enable-byte properties for force byte mode.
> - i2c-ast2600.c
> - change include asm/unaligned.h to linux/unaligned.h.
> - add reset timeout councter when slave active timeout.
> - modify issue i2c_recovery_bus before slave re-enable.
> - add aspeed,enable-byte properties.
> - Link to v15:
> https://lore.kernel.org/all/20241007035235.2254138-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v15:
> - i2c-ast2600.c
> - add include unaligned.h
> - rename all master -> controller, slave -> target.
> - keep multi-master to align property.
> - remove no used element in ast2600_i2c_bus.
> - Link to v14:
> https://lore.kernel.org/all/20241002070213.1165263-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v14:
> - aspeed,i2c.yaml
> - v13 change people reviewed-by tag, v14 fixed to original people tag,
> modify to Reviewed-by: Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>
> - struct ast2600_i2c_bus layout optimal.
> - ast2600_select_i2c_clock refine.
> - ast2600_i2c_recover_bus overridden fix.
> - dma_mapping_error() returned error code shadowed modify.
> - buffer register in a 4-byte aligned simplified
> - remove smbus alert
> - Link to v13:
> https://lore.kernel.org/all/20240819092850.1590758-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v13:
> - separate i2c master and slave driver to be two patchs.
> - modify include header list, add bits.h include. remove of*.h
> - modify (((x) >> 24) & GENMASK(5, 0)) to (((x) & GENMASK(29, 24)) >> 24)
> - modify ast2600_select_i2c_clock function implement.
> - modify ast2600_i2c_recover_bus function u32 claim to
> u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> - Link to v12:
> https://lore.kernel.org/all/20230714074522.23827-1-ryan_chen@aspeedtech.c
> om/
>
> Changes in v12:
> - aspeed,i2c.yaml
> - add Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> - i2c-ast2600.c
> - update include by alphabetical order
> - make just a one TAB and put the last two lines on the single one
> - remove no used timing_table structre
> - remove enum explicit assinment
> - rewritten to avoid this and using loop in ast2600_select_i2c_clock
> - use GENMASK for most 0xffff
> - remove too many parentheses
> - use str_read_write replace read write string
> - remove redundant blank line after ast2600_i2c_bus_of_table
> - fix wrong multi-line style of the comment
> - use macro for i2c standard speeds
> - remove useless noise dev_info
> - Link to v11:
> https://lore.kernel.org/all/20230430041712.3247998-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v11:
> - aspeed,i2c.yaml
> - no change, the same with v10.
> - i2c-ast2600.c
> - modify alert_enable from int -> boolean.
> - modify dbg string recovery -> recover.
> - remove no need to init 0.
> - remove new line after break.
> - remove unneeded empty line.
> - modify dma_alloc_coherent to dmam_alloc_coherent
> - modify probe nomem return dev_err_probe
> - modify i2c_add_adapter to devm_i2c_adapter
> - modify checkpatch: Alignment should match open parenthesis
> - modify checkpatch: braces {} should be used on all arms of this statement
> - modify checkpatch: Unbalanced braces around else statement
> - Link to v10:
> https://lore.kernel.org/all/20230415012848.1777768-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v10:
> - aspeed,i2c.yaml
> - move unevaluatedProperties after allOf.
> - remove extra one blank line.
> - i2c-ast2600.c
> - no change, the same with v8.
> - Link to v9:
> https://lore.kernel.org/all/20230405022825.333246-1-ryan_chen@aspeedtech.
> com/
>
> Changes in v9:
> - aspeed,i2c.yaml
> - backoff to v7.
> - no fix typo in maintainer's name and email. this would be another patch.
> - no remove address-cells, size-cells, this would be another patch.
> - use aspeed,enable-dma property instead of aspeed,xfer-mode selection.
> - fix allOf and else false properties for aspeed,ast2600-i2cv2.
> - i2c-ast2600.c
> - no change, the same with v8
> - Link to v8:
> https://lore.kernel.org/all/20230330073259.485606-1-ryan_chen@aspeedtech.
> com/
>
> Changes in v8:
> - aspeed,i2c.yaml
> - modify commit message.
> - Fix typo in maintainer's name and email.
> - remove address-cells, size-cells.
> - i2c-ast2600.c
> - move "i2c timeout counter" comment description before property_read.
> - remove redundant code "return ret" in probe end.
> - Link to v7:
> https://lore.kernel.org/all/20230327092524.3916389-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v7:
> - aspeed,i2c.yaml
> - Update ASPEED I2C maintainers email.
> - use aspeed,enable-dma property instead of aspeed,xfer-mode selection.
> - fix allOf and else false properties for aspeed,ast2600-i2cv2.
> - i2c-ast2600.c
> - remove aspeed,xfer-mode instead of aspeed,enable-dma mode. buffer
> mode
> is default.
> - remove aspeed,timeout instead of i2c-scl-clk-low-timeout-us for
> timeout setting.
> - Link to v6:
> https://lore.kernel.org/all/20230226031321.3126756-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v6:
> - remove aspeed,i2cv2.yaml, merge to aspeed,i2c.yaml -add support for
> i2cv2 properites.
> - i2c-ast2600.c
> - fix ast2600_i2c_remove ordering.
> - remove ast2600_i2c_probe goto labels, and add dev_err_probe -remove
> redundant deb_dbg debug message.
> - rename gr_regmap -> global_regs
> - Link to v5:
> https://lore.kernel.org/all/20230220061745.1973981-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v5:
> - remove ast2600-i2c-global.yaml, i2c-ast2600-global.c.
> - i2c-ast2600.c
> - remove legacy clock divide, all go for new clock divide.
> - remove duplicated read isr.
> - remove no used driver match
> - fix probe return for each labels return.
> - global use mfd driver, driver use phandle to regmap read/write.
> - rename aspeed,i2c-ast2600.yaml to aspeed,i2cv2.yaml -remove
> bus-frequency.
> - add required aspeed,gr
> - add timeout, byte-mode, buff-mode properites.
> - Link to v4:
> https://lore.kernel.org/all/20230201103359.1742140-1-ryan_chen@aspeedtec
> h.com/
>
> Changes in v4:
> - fix i2c-ast2600.c driver buffer mode use single buffer conflit in
> master slave mode both enable.
> - fix kmemleak issue when use dma mode.
> - fix typo aspeed,i2c-ast2600.yaml compatible is "aspeed,ast2600-i2c"
> - fix typo aspeed,i2c-ast2600.ymal to aspeed,i2c-ast2600.yaml
> - Link to v3:
> https://lore.kernel.org/all/20220516064900.30517-1-ryan_chen@aspeedtech.c
> om/
>
> Changes in v3:
> - fix i2c global clock divide default value.
> - remove i2c slave no used dev_dbg info.
> - Link to v2:
> https://lore.kernel.org/all/20220413101735.27678-1-ryan_chen@aspeedtech.c
> om/
>
> Changes in v2:
> - add i2c global ymal file commit.
> - rename file name from new to ast2600.
> aspeed-i2c-new-global.c -> i2c-ast2600-global.c
> aspeed-i2c-new-global.h -> i2c-ast2600-global.h
> i2c-new-aspeed.c -> i2c-ast2600.c
> - rename all driver function name to ast2600.
> - Link to v1:
> https://lore.kernel.org/all/20220323004009.943298-1-ryan_chen@aspeedtech.
> com/
>
> ---
> Ryan Chen (5):
> dt-bindings: i2c: Split AST2600 binding into a new YAML
> i2c: aspeed: Read clock-frequency via i2c_parse_fw_timings()
> dt-bindings: i2c: ast2600-i2c.yaml: Add global-regs properties
> i2c: ast2600: Add controller driver for AST2600 new register set
> i2c: ast2600: Add target mode support
>
> .../bindings/i2c/aspeed,ast2600-i2c.yaml | 88 ++
> .../devicetree/bindings/i2c/aspeed,i2c.yaml | 3 +-
> MAINTAINERS | 2 +
> drivers/i2c/busses/Makefile | 2 +-
> drivers/i2c/busses/i2c-aspeed.c | 24 +-
> drivers/i2c/busses/i2c-ast2600.c | 1290
> ++++++++++++++++++++
> 6 files changed, 1400 insertions(+), 9 deletions(-)
> ---
> base-commit: a293ec25d59dd96309058c70df5a4dd0f889a1e4
> change-id: 20260223-upstream_i2c-ebd07f89739c
>
> Best regards,
> --
> Ryan Chen <ryan_chen@aspeedtech.com>
^ permalink raw reply
* Re: [PATCH v2 2/2] soc: aspeed: add host-side PCIe BMC device driver
From: Grégoire Layet @ 2026-06-11 8:41 UTC (permalink / raw)
To: Andrew Jeffery
Cc: joel, andrew, jacky_chou, yh_chung, ninad, linux-aspeed,
linux-arm-kernel, linux-kernel
In-Reply-To: <66df26f7ec827a0f48cd44c454bfd36968ca4dd0.camel@codeconstruct.com.au>
Hello Andrew,
> Again, I'd rather we avoid drivers/soc/aspeed.
If creating a new specific driver is the right move, where should it go if
not in drivers/soc/aspeed ?
But again, considering all you remarks, maybe this driver is unnecessary.
As initially the driver was doing all the PCI BMC device functionality
provided by ASPEED (shared memory, VUART, message queue and doorbell),
it made sense to have everything in a driver for this specific case.
However for a TTY-only driver, that might be excessive.
When I trimmed down the driver, I didn't consider whether the driver itself
was still necessary.
I think adding a new driver is not the right solution for this.
From my research, the ASPEED PCIe device could be added to the 8250_pci driver.
The ASPEED PCIe device has a specific device ID and Vendor ID.
What do you think about this?
Thanks,
Grégoire
On Wed, 10 Jun 2026 at 14:51, Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> On Mon, 2026-06-08 at 14:51 +0000, Grégoire Layet wrote:
> > Taken from ASPEED 6.18 Kernel SDK
> >
> > Add support for VUART over PCIe between BMC and host.
> > This add host side driver.
> >
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> > Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
> > Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
> > Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
> > ---
> > drivers/soc/aspeed/Kconfig | 8 +
> > drivers/soc/aspeed/Makefile | 1 +
> > drivers/soc/aspeed/aspeed-host-bmc-dev.c | 249 +++++++++++++++++++++++
>
> Again, I'd rather we avoid drivers/soc/aspeed.
>
> > 3 files changed, 258 insertions(+)
> > create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c
> >
> > diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> > index 3e1fcf3c3268..5deefb64e8c7 100644
> > --- a/drivers/soc/aspeed/Kconfig
> > +++ b/drivers/soc/aspeed/Kconfig
> > @@ -11,6 +11,14 @@ config ASPEED_BMC_DEV
> > Enable support for the ASPEED AST2600 BMC Device.
> > This exposes the PCIe-to-LPC bridge of the BMC to the host over PCIe.
> >
> > +config ASPEED_HOST_BMC_DEV
> > + tristate "ASPEED Host BMC Device"
> > + depends on PCI
> > + depends on SERIAL_8250
> > + help
> > + Enable support for the ASPEED AST2600 BMC Device on the Host.
> > + This configure the PCIe and setup two 8250 compatible VUART ports.
> > +
> > config ASPEED_LPC_CTRL
> > tristate "ASPEED LPC firmware cycle control"
> > select REGMAP
> > diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> > index fab0d247df66..3fd3f6d8d36e 100644
> > --- a/drivers/soc/aspeed/Makefile
> > +++ b/drivers/soc/aspeed/Makefile
> > @@ -1,5 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > obj-$(CONFIG_ASPEED_BMC_DEV) += aspeed-bmc-dev.o
> > +obj-$(CONFIG_ASPEED_HOST_BMC_DEV) += aspeed-host-bmc-dev.o
> > obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
> > obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
> > obj-$(CONFIG_ASPEED_UART_ROUTING) += aspeed-uart-routing.o
> > diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> > new file mode 100644
> > index 000000000000..7cb52a770fb6
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +// Copyright (C) ASPEED Technology Inc.
> > +
> > +#include <linux/init.h>
> > +#include <linux/version.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/pci.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/serial_8250.h>
> > +
> > +static DEFINE_IDA(bmc_device_ida);
> > +
> > +#define VUART_MAX_PARMS 2
>
> Given the one supported piece of hardware we could avoid the associated
> loops and rather extract loop bodies to functions and call the function
> twice.
>
> > +#define MAX_MSI_NUM 8
> > +#define BMC_MULTI_MSI 32
> > +
> > +#define DRIVER_NAME "aspeed-host-bmc-dev"
> > +
> > +enum aspeed_platform_id {
> > + ASPEED,
> > +};
> > +
> > +enum msi_index {
> > + VUART0_MSI,
> > + VUART1_MSI,
> > +};
> > +
> > +/* Match msi_index */
> > +static int ast2600_msi_idx_table[MAX_MSI_NUM] = { 16, 15 };
> > +
> > +struct aspeed_platform {
> > + int (*setup)(struct pci_dev *pdev);
> > +};
> > +
> > +struct aspeed_pci_bmc_dev {
> > + struct device *dev;
> > + struct aspeed_platform *platform;
> > + kernel_ulong_t driver_data;
> > + int id;
> > +
> > + unsigned long message_bar_base;
> > + unsigned long message_bar_size;
> > + void __iomem *msg_bar_reg;
> > +
> > + struct uart_8250_port uart[VUART_MAX_PARMS];
> > + int uart_line[VUART_MAX_PARMS];
> > +
> > + /* Interrupt
> > + * The index of array is using to enum msi_index
> > + */
> > + int *msi_idx_table;
> > +};
> > +
> > +static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
> > +{
> > + struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > +
> > + /* Assign static msi index table by platform */
> > + pci_bmc_dev->msi_idx_table = ast2600_msi_idx_table;
> > +
> > + if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)
> > + /* Set all msi index to the first vector */
> > + memset(pci_bmc_dev->msi_idx_table, 0, sizeof(int) * MAX_MSI_NUM);
> > +}
> > +
> > +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev)
> > +{
> > + struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > + struct device *dev = &pdev->dev;
> > + u16 vuart_ioport;
> > + int ret, i;
> > +
> > + for (i = 0; i < VUART_MAX_PARMS; i++) {
> > + /* Assign the line to non-exist device */
> > + pci_bmc_dev->uart_line[i] = -ENOENT;
> > + vuart_ioport = 0x3F8 - (i * 0x100);
> > + pci_bmc_dev->uart[i].port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
> > + pci_bmc_dev->uart[i].port.uartclk = 115200 * 16;
> > + pci_bmc_dev->uart[i].port.irq =
> > + pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[VUART0_MSI + i]);
> > + pci_bmc_dev->uart[i].port.dev = dev;
> > + pci_bmc_dev->uart[i].port.iotype = UPIO_MEM32;
> > + pci_bmc_dev->uart[i].port.iobase = 0;
> > + pci_bmc_dev->uart[i].port.mapbase =
> > + pci_bmc_dev->message_bar_base + (vuart_ioport << 2);
> > + pci_bmc_dev->uart[i].port.membase = 0;
> > + pci_bmc_dev->uart[i].port.type = PORT_16550A;
> > + pci_bmc_dev->uart[i].port.flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
> > + pci_bmc_dev->uart[i].port.regshift = 2;
> > + ret = serial8250_register_8250_port(&pci_bmc_dev->uart[i]);
> > + if (ret < 0) {
> > + dev_err_probe(dev, ret, "Can't setup PCIe VUART\n");
> > + return ret;
> > + }
> > + pci_bmc_dev->uart_line[i] = ret;
> > + }
> > + return 0;
> > +}
> > +
> > +static void aspeed_pci_host_bmc_device_release_vuart(struct pci_dev *pdev)
> > +{
> > + struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > + int i;
> > +
> > + for (i = 0; i < VUART_MAX_PARMS; i++) {
> > + if (pci_bmc_dev->uart_line[i] >= 0)
> > + serial8250_unregister_port(pci_bmc_dev->uart_line[i]);
> > + }
> > +}
> > +
> > +static int aspeed_pci_host_setup(struct pci_dev *pdev)
> > +{
> > + struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > + int rc = 0;
> > +
> > + /* Get Message BAR */
> > + pci_bmc_dev->message_bar_base = pci_resource_start(pdev, 1);
> > + pci_bmc_dev->message_bar_size = pci_resource_len(pdev, 1);
> > + pci_bmc_dev->msg_bar_reg = pci_ioremap_bar(pdev, 1);
> > + if (!pci_bmc_dev->msg_bar_reg)
> > + return -ENOMEM;
> > +
> > + if (pdev->revision < 0x27) {
> > + /* AST2600 ERRTA40: dummy read */
>
> Can you please rather document what problem this is actually solving.
>
> > + (void)__raw_readl((void __iomem *)pci_bmc_dev->msg_bar_reg);
> > + } else {
> > + /* AST2700 not supported */
> > + pr_err("AST2700 detected but not supported");
>
> This logs an error but rc = 0 on return. Perhaps drop the log message
> and return an appropriate error code?
>
> > + goto out_free0;
> > + }
> > +
> > + rc = aspeed_pci_bmc_device_setup_vuart(pdev);
> > + if (rc) {
> > + pr_err("Cannot setup Virtual UART");
> > + goto out_free0;
> > + }
> > +
> > + return 0;
> > +
> > +out_free0:
> > + pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
> > +
> > + return rc;
> > +}
> > +
> > +static struct aspeed_platform aspeed_pcie_host[] = {
> > + { .setup = aspeed_pci_host_setup },
> > + { 0 }
> > +};
> > +
> > +static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > +{
> > + struct aspeed_pci_bmc_dev *pci_bmc_dev;
> > + int rc = 0;
> > +
> > + pr_info("ASPEED BMC PCI ID %04x:%04x, IRQ=%u\n", pdev->vendor, pdev->device, pdev->irq);
>
> I think we could do without this.
>
> > +
> > + pci_bmc_dev = devm_kzalloc(&pdev->dev, sizeof(*pci_bmc_dev), GFP_KERNEL);
> > + if (!pci_bmc_dev)
> > + return -ENOMEM;
> > +
> > + /* Get platform id */
> > + pci_bmc_dev->driver_data = ent->driver_data;
> > + pci_bmc_dev->platform = &aspeed_pcie_host[ent->driver_data];
> > +
> > + pci_bmc_dev->id = ida_alloc(&bmc_device_ida, GFP_KERNEL);
>
> This seems unnecessary.
>
> > + if (pci_bmc_dev->id < 0)
> > + return pci_bmc_dev->id;
> > +
> > + rc = pci_enable_device(pdev);
> > + if (rc) {
> > + dev_err(&pdev->dev, "pci_enable_device() returned error %d\n", rc);
> > + return rc;
> > + }
> > +
> > + pci_set_master(pdev);
> > + pci_set_drvdata(pdev, pci_bmc_dev);
> > +
> > + /* Prepare IRQ resource */
> > + aspeed_pci_setup_irq_resource(pdev);
> > +
> > + /* Setup BMC PCI device */
> > + rc = pci_bmc_dev->platform->setup(pdev);
>
> As with patch 1 this indirection seems unnecessary.
>
> > + if (rc) {
> > + dev_err(&pdev->dev, "ASPEED PCIe Host device returned error %d\n", rc);
> > + pci_free_irq_vectors(pdev);
> > + pci_disable_device(pdev);
> > + return rc;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_pci_host_bmc_device_remove(struct pci_dev *pdev)
> > +{
> > + struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > +
> > + if (pci_bmc_dev->driver_data == ASPEED)
>
> This condition seems unnecessary as the value shouldn't be anything
> else.
>
> > + aspeed_pci_host_bmc_device_release_vuart(pdev);
> > +
> > + ida_free(&bmc_device_ida, pci_bmc_dev->id);
> > +
> > + pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
> > +
> > + pci_free_irq_vectors(pdev);
> > + pci_disable_device(pdev);
> > +}
> > +
> > +/**
> > + * This table holds the list of (VendorID,DeviceID) supported by this driver
> > + *
> > + */
>
> I think that's self-evident and prefer the comment be removed.
>
> > +static struct pci_device_id aspeed_host_bmc_dev_pci_ids[] = {
> > + /* ASPEED BMC Device */
> > + { PCI_DEVICE(0x1A03, 0x2402), .class = 0xFF0000, .class_mask = 0xFFFF00,
> > + .driver_data = ASPEED },
> > + {
> > + 0,
> > + }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, aspeed_host_bmc_dev_pci_ids);
> > +
> > +static struct pci_driver aspeed_host_bmc_dev_driver = {
> > + .name = DRIVER_NAME,
> > + .id_table = aspeed_host_bmc_dev_pci_ids,
> > + .probe = aspeed_pci_host_bmc_device_probe,
> > + .remove = aspeed_pci_host_bmc_device_remove,
> > +};
> > +
> > +static int __init aspeed_host_bmc_device_init(void)
> > +{
> > + return pci_register_driver(&aspeed_host_bmc_dev_driver);
> > +}
> > +
> > +static void aspeed_host_bmc_device_exit(void)
> > +{
> > + /* unregister pci driver */
> > + pci_unregister_driver(&aspeed_host_bmc_dev_driver);
> > +}
> > +
> > +late_initcall(aspeed_host_bmc_device_init);
> > +module_exit(aspeed_host_bmc_device_exit);
>
> module_driver() could be used here.
>
> > +
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> > +MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
> > +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver
From: Grégoire Layet @ 2026-06-11 8:40 UTC (permalink / raw)
To: Andrew Jeffery
Cc: joel, andrew, jacky_chou, yh_chung, ninad, linux-aspeed,
linux-arm-kernel, linux-kernel
In-Reply-To: <4839c31f666b612799a795bb47c884901fd2a903.camel@codeconstruct.com.au>
Hello Andrew,
> It's probably best to use ASPEED's SDK as a source of inspiration for
> fixing obscure bugs, but not send drivers directly extracted from it.
I would like to discuss the different options then.
If we decide to continue on the current path, I'll modify the code according
to your remarks.
> We should avoid adding more drivers in drivers/soc/aspeed where we can.
> Is this really necessary?
This driver (for the BMC side) only enables some configuration on the SCU to
make MSI interrupts work. It is a very specific configuration, which is why I
thought it was OK as a separate driver.
Fundamentally, the BMC side driver is not necessary. During my testing,
I successfully made data flow in both directions without the BMC side driver,
but this was using polling mode (IRQ = 0), it's not ideal.
It is also possible to put the SCU initialisation on the
8250_aspeed_vuart driver
directly. This could be activated with a specific flag added to VUART nodes
('pcie2vuart' for example) on the DeviceTree.
Putting this configuration in the VUART driver directly would make the setup
on the devicetree easier and more understandable. Because in this series, the
bmc side driver needs a bmc_device node to be loaded. But the bmc_dev node
doesn't hold any meaningful information.
If we decide to take this approach, I think we should also add the
differentiation for the ast2600 compatibility. Currently, the aspeed-g6.dtsi
uses the "aspeed,ast2500-vuart" compatibility entry for the four VUARTs.
What do you think about this solution ?
Thanks,
Grégoire
On Wed, 10 Jun 2026 at 14:33, Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> Hello Grégoire,
>
> On Mon, 2026-06-08 at 14:51 +0000, Grégoire Layet wrote:
> > Taken from ASPEED 6.18 Kernel SDK
>
> It's probably best to use ASPEED's SDK as a source of inspiration for
> fixing obscure bugs, but not send drivers directly extracted from it.
>
> >
> > Add support for VUART over PCIe between BMC and host.
> > This add BMC side driver.
> >
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> > Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
> > Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
> > Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
> > ---
> > drivers/soc/aspeed/Kconfig | 7 ++
> > drivers/soc/aspeed/Makefile | 1 +
> > drivers/soc/aspeed/aspeed-bmc-dev.c | 187 ++++++++++++++++++++++++++++
>
> We should avoid adding more drivers in drivers/soc/aspeed where we can.
>
> Is this really necessary?
>
> > 3 files changed, 195 insertions(+)
> > create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c
> >
> > diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> > index f579ee0b5afa..3e1fcf3c3268 100644
> > --- a/drivers/soc/aspeed/Kconfig
> > +++ b/drivers/soc/aspeed/Kconfig
> > @@ -4,6 +4,13 @@ if ARCH_ASPEED || COMPILE_TEST
> >
> > menu "ASPEED SoC drivers"
> >
> > +config ASPEED_BMC_DEV
> > + tristate "ASPEED BMC Device"
> > + default n
> > + help
> > + Enable support for the ASPEED AST2600 BMC Device.
> > + This exposes the PCIe-to-LPC bridge of the BMC to the host over PCIe.
> > +
> > config ASPEED_LPC_CTRL
> > tristate "ASPEED LPC firmware cycle control"
> > select REGMAP
> > diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> > index b35d74592964..fab0d247df66 100644
> > --- a/drivers/soc/aspeed/Makefile
> > +++ b/drivers/soc/aspeed/Makefile
> > @@ -1,4 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_ASPEED_BMC_DEV) += aspeed-bmc-dev.o
> > obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
> > obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
> > obj-$(CONFIG_ASPEED_UART_ROUTING) += aspeed-uart-routing.o
> > diff --git a/drivers/soc/aspeed/aspeed-bmc-dev.c b/drivers/soc/aspeed/aspeed-bmc-dev.c
> > new file mode 100644
> > index 000000000000..7a204b543c97
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/aspeed-bmc-dev.c
> > @@ -0,0 +1,187 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +// Copyright (C) ASPEED Technology Inc.
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/regmap.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#define SCU_TRIGGER_MSI
> > +
> > +/* AST2600 SCU */
> > +#define ASPEED_SCU04 0x04
> > +#define AST2600A3_SCU04 0x05030303
> > +#define ASPEED_SCUC20 0xC20
> > +#define ASPEED_SCUC24 0xC24
>
> These could all use properly descriptive names.
>
> Pinctrl is an exception because of how the documentation is structured.
>
> > +#define MSI_ROUTING_MASK GENMASK(11, 10)
> > +#define PCIDEV1_INTX_MSI_HOST2BMC_EN BIT(18)
> > +#define MSI_ROUTING_PCIe2LPC_PCIDEV0 (0x1 << 10)
> > +#define MSI_ROUTING_PCIe2LPC_PCIDEV1 (0x2 << 10)
> > +
> > +#define ASPEED_SCU_PCIE_CONF_CTRL 0xC20
> > +#define SCU_PCIE_CONF_BMC_DEV_EN BIT(8)
> > +#define SCU_PCIE_CONF_BMC_DEV_EN_MMIO BIT(9)
> > +#define SCU_PCIE_CONF_BMC_DEV_EN_MSI BIT(11)
> > +#define SCU_PCIE_CONF_BMC_DEV_EN_IRQ BIT(13)
> > +#define SCU_PCIE_CONF_BMC_DEV_EN_DMA BIT(14)
> > +#define SCU_PCIE_CONF_BMC_DEV_EN_E2L BIT(15)
> > +#define SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE BIT(21)
> > +
> > +#define ASPEED_SCU_BMC_DEV_CLASS 0xC68
> > +
> > +
> > +struct aspeed_platform {
> > + int (*init)(struct platform_device *pdev);
> > +};
> > +
> > +struct aspeed_bmc_device {
> > + struct device *dev;
> > + int id;
> > + void __iomem *reg_base;
> > +
> > + int pcie2lpc;
> > + int irq;
> > +
> > + const struct aspeed_platform *platform;
> > +
> > + struct regmap *scu;
> > + int pcie_irq;
> > +};
> > +
> > +
> > +static int aspeed_ast2600_init(struct platform_device *pdev)
> > +{
> > + struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
> > + struct device *dev = &pdev->dev;
> > + u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
> > + SCU_PCIE_CONF_BMC_DEV_EN_MMIO | SCU_PCIE_CONF_BMC_DEV_EN;
> > + u32 scu_id;
> > +
> > + bmc_device->scu = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,scu");
>
> We should rather look at auxbus for the SCU.
>
> > + if (IS_ERR(bmc_device->scu)) {
> > + dev_err(&pdev->dev, "failed to find SCU regmap\n");
> > + return PTR_ERR(bmc_device->scu);
> > + }
> > +
> > + if (bmc_device->pcie2lpc)
> > + pcie_config_ctl |= SCU_PCIE_CONF_BMC_DEV_EN_E2L |
> > + SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE;
> > +
> > + regmap_update_bits(bmc_device->scu, ASPEED_SCU_PCIE_CONF_CTRL,
> > + pcie_config_ctl, pcie_config_ctl);
> > +
> > + /* update class code to others as it is a MFD device */
> > + regmap_write(bmc_device->scu, ASPEED_SCU_BMC_DEV_CLASS, 0xff000000);
> > +
> > +#ifdef SCU_TRIGGER_MSI
>
> I don't see that this needs to be a CPP test. This could be a C test.
> The construct would be optimised because of the constant and we'd get
> compile time coverage of both sides without additional configuration.
>
> Have you tested both sides?
>
> > + //SCUC24[17]: Enable PCI device 1 INTx/MSI from SCU560[15]. Will be added in next version
> > + regmap_update_bits(bmc_device->scu, ASPEED_SCUC20, BIT(11) | BIT(14), BIT(11) | BIT(14));
>
> These bits need descriptive macros.
>
> > +
> > + regmap_read(bmc_device->scu, ASPEED_SCU04, &scu_id);
> > + if (scu_id == AST2600A3_SCU04)
> > + regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
> > + PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_MASK,
> > + PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_PCIe2LPC_PCIDEV1);
> > + else
> > + regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
> > + BIT(17) | BIT(14) | BIT(11), BIT(17) | BIT(14) | BIT(11));
>
> As do these
>
> > +#else
> > + //SCUC24[18]: Enable PCI device 1 INTx/MSI from Host-to-BMC controller.
> > + regmap_update_bits(bmc_device->scu, 0xc24, BIT(18) | BIT(14), BIT(18) | BIT(14));
>
> And these.
>
> > +#endif
> > +
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static struct aspeed_platform ast2600_plaform = {
> > + .init = aspeed_ast2600_init
> > +};
> > +
> > +
> > +static const struct of_device_id aspeed_bmc_device_of_matches[] = {
> > + { .compatible = "aspeed,ast2600-bmc-device", .data = &ast2600_plaform },
>
> This compatible isn't documented in this series and isn't present in
> linux-next at a87737435cfa ("Add linux-next specific files for
> 20260608"). You'll need to address that if it's reasonable to continue
> down this path. I expect you'll want to avoid it, and define any
> necessary properties on the SCU node rather than add further children.
>
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_bmc_device_of_matches);
> > +
> > +static int aspeed_bmc_device_probe(struct platform_device *pdev)
> > +{
> > + struct aspeed_bmc_device *bmc_device;
> > + struct device *dev = &pdev->dev;
>
> This shortcut is defined but inconsistently used.
>
> > + const void *md = of_device_get_match_data(dev);
>
> I think we can do without this, see below.
>
> > + int ret = 0;
> > +
> > + if (!md)
> > + return -ENODEV;
> > +
> > + bmc_device = devm_kzalloc(&pdev->dev, sizeof(struct aspeed_bmc_device), GFP_KERNEL);
> > + if (!bmc_device)
> > + return -ENOMEM;
> > + dev_set_drvdata(dev, bmc_device);
> > +
> > + bmc_device->platform = md;
> > +
> > + bmc_device->id = of_alias_get_id(dev->of_node, "bmcdev");
> > + if (bmc_device->id < 0)
> > + bmc_device->id = 0;
> > +
> > + bmc_device->dev = dev;
> > + bmc_device->reg_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(bmc_device->reg_base))
> > + return PTR_ERR(bmc_device->reg_base);
> > +
> > + bmc_device->irq = platform_get_irq(pdev, 0);
>
> This seems unnecessary.
>
> > + if (bmc_device->irq < 0) {
> > + dev_err(&pdev->dev, "platform get of irq[=%d] failed!\n", bmc_device->irq);
> > + return bmc_device->irq;
> > + }
> > +
> > + if (of_property_read_bool(dev->of_node, "pcie2lpc"))
>
> This property isn't documented.
>
> > + bmc_device->pcie2lpc = 1;
> > +
> > + ret = bmc_device->platform->init(pdev);
>
> The driver only supports one SoC, this indirection seems unnecessary
> right now. We can add that later when there's a need to differentiate.
> I'd rather you call the setup function directly for now.
>
> > + if (ret) {
> > + dev_err(dev, "Initialize bmc device failed\n");
> > + goto out;
> > + }
> > +
> > + dev_info(dev, "aspeed bmc device: driver successfully loaded.\n");
> > +
> > + return 0;
> > +
> > +out:
> > + dev_warn(dev, "aspeed bmc device: driver init failed (ret=%d)!\n", ret);
> > + return ret;
> > +}
> > +
> > +static void aspeed_bmc_device_remove(struct platform_device *pdev)
> > +{
> > + struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
> > +
> > + devm_free_irq(&pdev->dev, bmc_device->irq, bmc_device);
> > + devm_kfree(&pdev->dev, bmc_device);
>
> These are unnecessary due to cleanup of devres on release.
>
> Andrew
^ permalink raw reply
* Re: [PATCH v2 05/16] usb: hub: Associate port@ fwnode with USB port device
From: Andy Shevchenko @ 2026-06-11 8:37 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Greg Kroah-Hartman, Daniel Scally, Heikki Krogerus, Sakari Ailus,
Rafael J. Wysocki, Danilo Krummrich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Alan Stern, linux-acpi, driver-core,
linux-pm, linux-usb, devicetree, linux-mediatek, linux-arm-kernel,
linux-kernel, Manivannan Sadhasivam, Chen-Yu Tsai
In-Reply-To: <CAMRc=MdiwQM6yk8FXcc+RisVP2iqWKWzVsn2-Yy6dyJXt-1X=Q@mail.gmail.com>
On Thu, Jun 11, 2026 at 04:20:58AM -0400, Bartosz Golaszewski wrote:
> On Wed, 10 Jun 2026 16:16:12 +0200, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> said:
> > On Wed, Jun 10, 2026 at 04:40:39PM +0800, Chen-Yu Tsai wrote:
> >> When a USB hub port is connected to a connector in a firmware node
> >> graph, the port itself has a node in the graph.
> >>
> >> Associate the port's firmware node with the USB port's device,
> >> usb_port::dev. This is used in later changes for the M.2 slot power
> >> sequencing provider to match against the requesting port.
> >
> > Okay, would this affect ACPI-based systems? if so, how?
> > Can you elaborate on that, please?
>
> Is it possible that there's an ACPI device node associated with the port like
> on some DT systems? I don't think so and there should be no impact IMO but I
> also don't know enough about ACPI.
The API is agnostic. There is a possibility to have software nodes associated
with the port. I think the best is to be sure that ACPI-aware people who are
experts in USB will check this (Heikki?).
Also note Sashiko complain on reference count leakage.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 0/3] tty: serial: Add Cortina-Access UART driver and platform support
From: Arnd Bergmann @ 2026-06-11 8:35 UTC (permalink / raw)
To: Jason Li, Jason Li, Greg Kroah-Hartman, Jiri Slaby
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Catalin Marinas,
Will Deacon, linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <JH0PR01MB5777B84BE8D9987329ABA54BA21B2@JH0PR01MB5777.apcprd01.prod.exchangelabs.com>
On Thu, Jun 11, 2026, at 07:27, Jason Li wrote:
> Hi Arnd,
>
> Your memory is truly amazing; you even remember a submission from a few
> years ago.
No, I just looked up your previous submissions when I saw the new one,
lore.kernel.org never forgets anything ;-)
> Yes, we expect actual end-user products based on these SoCs, and our
> intention is to provide complete upstream support over time. The UART
> driver and DTS support submitted in this series are the first step in
> that effort.
>
> Cortina-System and Cortina-Access are now totally different company.
> Current aarch64 chipset are totally different with legacy gemini
> processor.
> Realtek has many business unit, different BU may have upstream plan but
> they are individual.
> Although Cortina-Access is a wholly-owned subsidiary of Realtek, our
> product development is entirely independent.
Thanks for the information. Please make sure to add something along
these into the changeset text for the initial arm64 patch, along
with a brief description of what type of chip this is
Arnd
^ permalink raw reply
* [PATCH] firmware: arm_scmi: Use common error handling code in __scmi_device_create()
From: Markus Elfring @ 2026-06-11 8:34 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, Cristian Marussi, Sudeep Holla
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 11 Jun 2026 10:27:45 +0200
Use an additional label so that a bit of exception handling can be better
reused at the end of an if branch.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/firmware/arm_scmi/bus.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 793be9eabaed..8c20ed1a8243 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -450,14 +450,13 @@ __scmi_device_create(struct device_node *np, struct device *parent,
return NULL;
scmi_dev->name = kstrdup_const(name ?: "unknown", GFP_KERNEL);
- if (!scmi_dev->name) {
- kfree(scmi_dev);
- return NULL;
- }
+ if (!scmi_dev->name)
+ goto free_scmi_dev;
id = ida_alloc_min(&scmi_bus_id, 1, GFP_KERNEL);
if (id < 0) {
kfree_const(scmi_dev->name);
+free_scmi_dev:
kfree(scmi_dev);
return NULL;
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one
From: Jerome Brunet @ 2026-06-11 8:28 UTC (permalink / raw)
To: Valerio Setti
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Neil Armstrong, Kevin Hilman, Martin Blumenstingl, linux-kernel,
linux-sound, linux-arm-kernel, linux-amlogic
In-Reply-To: <20260610-reshape-aiu-as-axg-v2-0-cac3663a8b51@baylibre.com>
On mer. 10 juin 2026 at 23:29, Valerio Setti <vsetti@baylibre.com> wrote:
> The goal of this series is to reshape Amlogic GX's AIU implementation for
> I2S to let it follow the same design as in AXG's TDM. Keeping the same
> design allows for unifying the two platform implementations in the future
> and it also allows for an easy addition of I2S input.
>
> The first commit introduces gx-formatter as the basic block which takes
> care of properly formatting audio data. Formatters are DAPM widgets
> (c.f. axg-tdm-formatter in AXG) which are dynamically attached/detached
> to the streams when the latters starts/stop, respectively.
> aiu-formatter-i2s is introduced as formatter implementation for the i2s
> output.
>
> By the end aiu-encoder-i2s will only need to handle interface clocks and
> enforce interface wide rate symmetry (c.f axg-tdm-interface on the AXG
> platform). Right now rate symmetry is not relevant because only i2s output
> is supported, but it will become useful when following patch series will
> introduce the i2s input part.
>
> This series was tested on an OdroidC2 board (Amlogic S905 SOC) both with
> HDMI output and with NXP SGTL5000 codec connected to the I2S pins.
> This series was also verified using "pcm-test" test tool and all tests
> are passing.
>
> Changes in v2:
> - Fixed most of the weaknesses found by Sashiko review tool [1].
> - Resolved testing failures with "pcm-test" as reported by Mark Brown
> (thanks for the heads up!). I left a comment in
> "aiu_encoder_i2s_startup" to explain the fix.
>
> Link to v1: https://lore.kernel.org/r/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3@baylibre.com
>
> [1]: https://sashiko.dev/#/patchset/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3%40baylibre.com
>
> Signed-off-by: Valerio Setti <vsetti@baylibre.com>
Nice work. Thanks for taking the time to understand the existing drivers
and build upon them !
Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
Feel free to add yourself as maintainer of the Amlogic audio drivers, if
you fancy doing so ;)
> ---
> Valerio Setti (4):
> ASoC: meson: gx: add gx-formatter and gx-interface
> ASoC: meson: aiu-encoder-i2s: prepare for multiple streams
> ASoC: meson: aiu: introduce I2S output formatter
> ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data
>
> sound/soc/meson/Makefile | 2 +
> sound/soc/meson/aiu-encoder-i2s.c | 281 +++++++++++++++++++++++++----------
> sound/soc/meson/aiu-formatter-i2s.c | 104 +++++++++++++
> sound/soc/meson/aiu.c | 32 +++-
> sound/soc/meson/aiu.h | 4 +
> sound/soc/meson/gx-formatter.c | 282 ++++++++++++++++++++++++++++++++++++
> sound/soc/meson/gx-formatter.h | 56 +++++++
> sound/soc/meson/gx-interface.h | 48 ++++++
> 8 files changed, 731 insertions(+), 78 deletions(-)
> ---
> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> change-id: 20260515-reshape-aiu-as-axg-1dac9037cad3
>
> Best regards,
--
Jerome
^ permalink raw reply
* Re: [PATCH v2 06/16] usb: hub: Pass |struct usb_port*| to usb_port_is_power_on()
From: Bartosz Golaszewski @ 2026-06-11 8:25 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Bartosz Golaszewski, Greg Kroah-Hartman, Andy Shevchenko,
Daniel Scally, Heikki Krogerus, Sakari Ailus, Rafael J. Wysocki,
Danilo Krummrich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Alan Stern,
linux-acpi, driver-core, linux-pm, linux-usb, devicetree,
linux-mediatek, linux-arm-kernel, linux-kernel,
Manivannan Sadhasivam
In-Reply-To: <20260610084053.2059858-7-wenst@chromium.org>
On Wed, 10 Jun 2026 10:40:40 +0200, Chen-Yu Tsai <wenst@chromium.org> said:
> usb_port_is_power_on() currently takes |struct usb_hub*|, but only needs
> it to tell if the hub/port is SuperSpeed or not.
>
> In a subsequent change, usb_port_is_power_on() needs access to a pwrseq
> state tracking field in |struct usb_port|. Either structure can be used
> to identify whether a port/hub is SuperSpeed or not, as the field in
> |struct usb_port| is inherited from the hub:
>
> port->is_superspeed = hub_is_superspeed(hub)
>
> Replace usb_port_is_power_on()'s |struct usb_hub*| parameter with
> |struct usb_port*| so a subsequent change can use it.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
Makes sense.
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
^ permalink raw reply
* Re: [PATCH v2 05/16] usb: hub: Associate port@ fwnode with USB port device
From: Bartosz Golaszewski @ 2026-06-11 8:20 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bartosz Golaszewski, Greg Kroah-Hartman, Daniel Scally,
Heikki Krogerus, Sakari Ailus, Rafael J. Wysocki,
Danilo Krummrich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Alan Stern,
linux-acpi, driver-core, linux-pm, linux-usb, devicetree,
linux-mediatek, linux-arm-kernel, linux-kernel,
Manivannan Sadhasivam, Chen-Yu Tsai
In-Reply-To: <ailxrP-_9_NL8qnN@ashevche-desk.local>
On Wed, 10 Jun 2026 16:16:12 +0200, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> said:
> On Wed, Jun 10, 2026 at 04:40:39PM +0800, Chen-Yu Tsai wrote:
>> When a USB hub port is connected to a connector in a firmware node
>> graph, the port itself has a node in the graph.
>>
>> Associate the port's firmware node with the USB port's device,
>> usb_port::dev. This is used in later changes for the M.2 slot power
>> sequencing provider to match against the requesting port.
>
> Okay, would this affect ACPI-based systems? if so, how?
> Can you elaborate on that, please?
>
Is it possible that there's an ACPI device node associated with the port like
on some DT systems? I don't think so and there should be no impact IMO but I
also don't know enough about ACPI.
Bart
^ permalink raw reply
* [PATCH] arm64: dts: imx8mp-frdm: Add missing HDMI DDC pinctrl
From: Philipp Zabel @ 2026-06-11 8:18 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
Cc: devicetree, imx, linux-arm-kernel, linux-kernel, Philipp Zabel
Configure HDMI DDC SCL/SDA pins to support reading EDID.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm64/boot/dts/freescale/imx8mp-frdm.dts | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-frdm.dts b/arch/arm64/boot/dts/freescale/imx8mp-frdm.dts
index 5fb9714215bf..f43330d1ff8b 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-frdm.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-frdm.dts
@@ -562,6 +562,8 @@ MX8MP_IOMUXC_SAI1_RXD0__GPIO4_IO02 0x10
pinctrl_hdmi: hdmigrp {
fsl,pins = <
+ MX8MP_IOMUXC_HDMI_DDC_SCL__HDMIMIX_HDMI_SCL 0x1c2
+ MX8MP_IOMUXC_HDMI_DDC_SDA__HDMIMIX_HDMI_SDA 0x1c2
MX8MP_IOMUXC_HDMI_CEC__HDMIMIX_HDMI_CEC 0x10
>;
};
---
base-commit: 4549871118cf616eecdd2d939f78e3b9e1dddc48
change-id: 20260609-imx8mp-frdm-hdmi-ddc-715a3cd5a9ff
Best regards,
--
Philipp Zabel <p.zabel@pengutronix.de>
^ permalink raw reply related
* Re: [PATCH v2 04/16] usb: hub: Return actual error from hub_configure() in hub_probe()
From: Bartosz Golaszewski @ 2026-06-11 8:17 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Alan Stern, linux-acpi, driver-core, linux-pm, linux-usb,
devicetree, linux-mediatek, linux-arm-kernel, linux-kernel,
Manivannan Sadhasivam, Bartosz Golaszewski, Greg Kroah-Hartman,
Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus,
Rafael J. Wysocki, Danilo Krummrich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno
In-Reply-To: <20260610084053.2059858-5-wenst@chromium.org>
On Wed, 10 Jun 2026 10:40:38 +0200, Chen-Yu Tsai <wenst@chromium.org> said:
> The addition of power sequencing descriptor handling in the USB hub code
> requires dealing with deferred probing from pwrseq_get(). The power
> sequencing provider may not yet be available when the USB hub probes.
>
> Return the actual error code from hub_configure() when it fails, so that
> the driver core can notice the deferred probe request.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
^ permalink raw reply
* Re: [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams
From: Jerome Brunet @ 2026-06-11 8:16 UTC (permalink / raw)
To: Valerio Setti
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Neil Armstrong, Kevin Hilman, Martin Blumenstingl, linux-kernel,
linux-sound, linux-arm-kernel, linux-amlogic
In-Reply-To: <20260610-reshape-aiu-as-axg-v2-2-cac3663a8b51@baylibre.com>
On mer. 10 juin 2026 at 23:29, Valerio Setti <vsetti@baylibre.com> wrote:
> aiu-encoder-i2s is going to be the interface that handles both playback
> and capture, so this commit does all the required changes to prepare
> for that since so far it only handled playback:
> - probe/remove functions are added to allocate/free per stream data,
> respectively.
> - 'struc gx_iface' and 'struct gx_stream' are used to store interface or
> stream associated data, respecively.
> - interface wide rate symmetry is enforced.
> - quirks on bclk are also enforced if/when necessary.
>
> Clock-wise instead of bulk enabling all the clocks on startup and disabling
> them on shutdown, only the peripheral's internal ones are enabled/disabled
> in those functions, whereas MCLK and I2S clock divider are handled in
> prepare/hw_free.
>
> Finally a trigger() callback is also added to start/stop the associated
> I2S data formatter.
>
> Signed-off-by: Valerio Setti <vsetti@baylibre.com>
> ---
> sound/soc/meson/aiu-encoder-i2s.c | 207 ++++++++++++++++++++++++++++++++++----
> sound/soc/meson/aiu.h | 3 +
> 2 files changed, 193 insertions(+), 17 deletions(-)
>
> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index 3b4061508c18047fe8d6f3f98061720f8ce238f2..f50b03824ad280afabb31eecc20ccb855defa11e 100644
> --- a/sound/soc/meson/aiu-encoder-i2s.c
> +++ b/sound/soc/meson/aiu-encoder-i2s.c
> @@ -10,6 +10,8 @@
> #include <sound/soc-dai.h>
>
> #include "aiu.h"
> +#include "gx-formatter.h"
> +#include "gx-interface.h"
>
> #define AIU_I2S_SOURCE_DESC_MODE_8CH BIT(0)
> #define AIU_I2S_SOURCE_DESC_MODE_24BIT BIT(5)
> @@ -112,6 +114,9 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
> struct snd_pcm_hw_params *params,
> unsigned int bs)
> {
> + struct aiu *aiu = snd_soc_component_get_drvdata(component);
> + struct gx_iface *iface = &aiu->i2s.iface;
> +
> /*
> * NOTE: this HW is odd.
> * In most configuration, the i2s divider is 'mclk / blck'.
> @@ -126,6 +131,18 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
> return -EINVAL;
> }
> bs += bs / 2;
> + iface->bs_quirk = true;
> + } else {
> + /*
> + * If the bs quirk is currently applied for one stream and another
> + * ones tries to setup a configuration for which the quirk is
> + * not required, then fail.
> + */
> + if (iface->bs_quirk) {
> + dev_err(component->dev,
> + "bclk requirements are incompatible with active stream\n");
> + return -EINVAL;
This comment is not blocking thing IMO, more a request for future
improvments.
What I do not like here is the user may get an unexpected failure while
setting up the stream, because of something that did not show up the
contrainst we provided. IOW, it is kind of too late.
The problem is not new to this driver, the same applies to the error
returned if bs is not a multiple of 2 :(
It would be nice if this problem could be reflected in the contraints
to avoid returning an error this late in the stream setup.
> + }
> }
>
> /* Use CLK_MORE for mclk to bclk divider */
> @@ -145,14 +162,15 @@ static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
> struct snd_pcm_hw_params *params)
> {
> struct aiu *aiu = snd_soc_component_get_drvdata(component);
> + struct gx_iface *iface = &aiu->i2s.iface;
> unsigned int srate = params_rate(params);
> unsigned int fs, bs;
> int ret;
>
> /* Get the oversampling factor */
> - fs = DIV_ROUND_CLOSEST(clk_get_rate(aiu->i2s.clks[MCLK].clk), srate);
> + fs = DIV_ROUND_CLOSEST(iface->mclk_rate, srate);
>
> - if (fs % 64)
> + if ((fs % 64) || (fs == 0))
> return -EINVAL;
>
> /* Send data MSB first */
> @@ -188,24 +206,59 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params,
> struct snd_soc_dai *dai)
> {
> + struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> + struct gx_iface *iface = ts->iface;
> struct snd_soc_component *component = dai->component;
> int ret;
>
> - /* Disable the clock while changing the settings */
> - aiu_encoder_i2s_divider_enable(component, false);
> + /*
> + * Enforce interface wide rate symmetry only if there is more than
> + * 1 stream active.
> + */
> + if (snd_soc_dai_active(dai) > 1) {
> + if (iface->rate && iface->rate != params_rate(params)) {
> + dev_err(dai->dev, "can't set iface rate (%d != %d)\n",
> + iface->rate, params_rate(params));
> + return -EINVAL;
> + }
> + }
>
> ret = aiu_encoder_i2s_setup_desc(component, params);
> if (ret) {
> - dev_err(dai->dev, "setting i2s desc failed\n");
> + dev_err(dai->dev, "setting i2s desc failed: %d\n", ret);
> return ret;
> }
>
> ret = aiu_encoder_i2s_set_clocks(component, params);
> if (ret) {
> - dev_err(dai->dev, "setting i2s clocks failed\n");
> + dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret);
> return ret;
> }
>
> + iface->rate = params_rate(params);
> + ts->physical_width = params_physical_width(params);
> + ts->width = params_width(params);
> + ts->channels = params_channels(params);
> +
> + return 0;
> +}
> +
> +static int aiu_encoder_i2s_prepare(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> + struct snd_soc_component *component = dai->component;
> + int ret;
> +
> + if (ts->clk_enabled)
> + return 0;
> +
> + ret = clk_prepare_enable(ts->iface->mclk);
> + if (ret)
> + return ret;
> +
> + ts->clk_enabled = true;
> +
> aiu_encoder_i2s_divider_enable(component, true);
>
> return 0;
> @@ -214,9 +267,24 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
> static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
> struct snd_soc_dai *dai)
> {
> + struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> + struct gx_iface *iface = ts->iface;
> struct snd_soc_component *component = dai->component;
>
> - aiu_encoder_i2s_divider_enable(component, false);
> + /*
> + * If this is the last substream being closed then disable the i2s
> + * clock divider and clear 'iface->rate'.
> + */
> + if (snd_soc_dai_active(dai) <= 1) {
> + aiu_encoder_i2s_divider_enable(component, 0);
> + iface->rate = 0;
> + iface->bs_quirk = false;
> + }
> +
> + if (ts->clk_enabled) {
> + clk_disable_unprepare(ts->iface->mclk);
> + ts->clk_enabled = false;
> + }
>
> return 0;
> }
> @@ -224,6 +292,8 @@ static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
> static int aiu_encoder_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> {
> struct snd_soc_component *component = dai->component;
> + struct aiu *aiu = snd_soc_component_get_drvdata(component);
> + struct gx_iface *iface = &aiu->i2s.iface;
> unsigned int inv = fmt & SND_SOC_DAIFMT_INV_MASK;
> unsigned int val = 0;
> unsigned int skew;
> @@ -255,9 +325,12 @@ static int aiu_encoder_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> skew = 0;
> break;
> default:
> + dev_err(dai->dev, "unsupported dai format\n");
> return -EINVAL;
> }
>
> + iface->fmt = fmt;
> +
> val |= FIELD_PREP(AIU_CLK_CTRL_LRCLK_SKEW, skew);
> snd_soc_component_update_bits(component, AIU_CLK_CTRL,
> AIU_CLK_CTRL_LRCLK_INVERT |
> @@ -272,6 +345,7 @@ static int aiu_encoder_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> unsigned int freq, int dir)
> {
> struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
> + struct gx_iface *iface = &aiu->i2s.iface;
> int ret;
>
> if (WARN_ON(clk_id != 0))
> @@ -280,11 +354,15 @@ static int aiu_encoder_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> if (dir == SND_SOC_CLOCK_IN)
> return 0;
>
> - ret = clk_set_rate(aiu->i2s.clks[MCLK].clk, freq);
> - if (ret)
> - dev_err(dai->dev, "Failed to set sysclk to %uHz", freq);
> + ret = clk_set_rate(iface->mclk, freq);
> + if (ret) {
> + dev_err(dai->dev, "Failed to set sysclk to %uHz: %d", freq, ret);
> + return ret;
> + }
>
> - return ret;
> + iface->mclk_rate = freq;
> +
> + return 0;
> }
>
> static const unsigned int hw_channels[] = {2, 8};
> @@ -305,15 +383,35 @@ static int aiu_encoder_i2s_startup(struct snd_pcm_substream *substream,
> SNDRV_PCM_HW_PARAM_CHANNELS,
> &hw_channel_constraints);
> if (ret) {
> - dev_err(dai->dev, "adding channels constraints failed\n");
> + dev_err(dai->dev, "adding channels constraints failed: %d\n", ret);
> return ret;
> }
>
> - ret = clk_bulk_prepare_enable(aiu->i2s.clk_num, aiu->i2s.clks);
> - if (ret)
> - dev_err(dai->dev, "failed to enable i2s clocks\n");
> + /*
> + * Enable only clocks which are required for the interface internal
> + * logic. MCLK is enabled/disabled from the formatter and the I2S
> + * divider is enabled/disabled in "hw_params"/"hw_free", respectively.
> + */
> + ret = clk_prepare_enable(aiu->i2s.clks[PCLK].clk);
> + if (ret) {
> + dev_err(dai->dev, "failed to enable PCLK: %d\n", ret);
> + return ret;
> + }
> + ret = clk_prepare_enable(aiu->i2s.clks[MIXER].clk);
> + if (ret) {
> + dev_err(dai->dev, "failed to enable MIXER: %d\n", ret);
> + clk_disable_unprepare(aiu->i2s.clks[PCLK].clk);
> + return ret;
> + }
> + ret = clk_prepare_enable(aiu->i2s.clks[AOCLK].clk);
> + if (ret) {
> + dev_err(dai->dev, "failed to enable AOCLK: %d\n", ret);
> + clk_disable_unprepare(aiu->i2s.clks[MIXER].clk);
> + clk_disable_unprepare(aiu->i2s.clks[PCLK].clk);
> + return ret;
> + }
>
> - return ret;
> + return 0;
> }
>
> static void aiu_encoder_i2s_shutdown(struct snd_pcm_substream *substream,
> @@ -321,14 +419,89 @@ static void aiu_encoder_i2s_shutdown(struct snd_pcm_substream *substream,
> {
> struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
>
> - clk_bulk_disable_unprepare(aiu->i2s.clk_num, aiu->i2s.clks);
> + clk_disable_unprepare(aiu->i2s.clks[AOCLK].clk);
> + clk_disable_unprepare(aiu->i2s.clks[MIXER].clk);
> + clk_disable_unprepare(aiu->i2s.clks[PCLK].clk);
> +}
> +
> +static int aiu_encoder_i2s_trigger(struct snd_pcm_substream *substream,
> + int cmd,
> + struct snd_soc_dai *dai)
> +{
> + struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> + int ret;
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + ret = gx_stream_start(ts);
> + break;
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + case SNDRV_PCM_TRIGGER_STOP:
> + gx_stream_stop(ts);
> + ret = 0;
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +static int aiu_encoder_i2s_remove_dai(struct snd_soc_dai *dai)
> +{
> + int stream;
> +
> + for_each_pcm_streams(stream) {
> + struct gx_stream *ts;
> +
> + ts = snd_soc_dai_dma_data_get(dai, stream);
> + if (ts)
> + gx_stream_free(ts);
> +
> + snd_soc_dai_dma_data_set(dai, stream, NULL);
> + }
> +
> + return 0;
> +}
> +
> +static int aiu_encoder_i2s_probe_dai(struct snd_soc_dai *dai)
> +{
> + struct aiu *aiu = snd_soc_dai_get_drvdata(dai);
> + struct gx_iface *iface = &aiu->i2s.iface;
> + int stream;
> +
> + for_each_pcm_streams(stream) {
> + struct gx_stream *ts;
> +
> + if (!snd_soc_dai_get_widget(dai, stream))
> + continue;
> +
> + ts = gx_stream_alloc(iface);
> + if (!ts) {
> + aiu_encoder_i2s_remove_dai(dai);
> + return -ENOMEM;
> + }
> + snd_soc_dai_dma_data_set(dai, stream, ts);
> + }
> +
> + iface->mclk = aiu->i2s.clks[MCLK].clk;
> + iface->mclk_rate = clk_get_rate(iface->mclk);
> +
> + return 0;
> }
>
> const struct snd_soc_dai_ops aiu_encoder_i2s_dai_ops = {
> + .probe = aiu_encoder_i2s_probe_dai,
> + .remove = aiu_encoder_i2s_remove_dai,
> .hw_params = aiu_encoder_i2s_hw_params,
> + .prepare = aiu_encoder_i2s_prepare,
> .hw_free = aiu_encoder_i2s_hw_free,
> .set_fmt = aiu_encoder_i2s_set_fmt,
> .set_sysclk = aiu_encoder_i2s_set_sysclk,
> .startup = aiu_encoder_i2s_startup,
> .shutdown = aiu_encoder_i2s_shutdown,
> + .trigger = aiu_encoder_i2s_trigger,
> };
> diff --git a/sound/soc/meson/aiu.h b/sound/soc/meson/aiu.h
> index 0f94c8bf608181112d78402532b832eb50c2d409..68310de0bdf7a97d8de2ff306c159248ee9b0ede 100644
> --- a/sound/soc/meson/aiu.h
> +++ b/sound/soc/meson/aiu.h
> @@ -7,6 +7,8 @@
> #ifndef _MESON_AIU_H
> #define _MESON_AIU_H
>
> +#include "gx-formatter.h"
> +
> struct clk;
> struct clk_bulk_data;
> struct device;
> @@ -25,6 +27,7 @@ struct aiu_interface {
> struct clk_bulk_data *clks;
> unsigned int clk_num;
> int irq;
> + struct gx_iface iface;
> };
>
> struct aiu_platform_data {
--
Jerome
^ permalink raw reply
* Re: [PATCH v2 01/16] device property: Add fwnode_graph_get_port_by_id()
From: Bartosz Golaszewski @ 2026-06-11 8:15 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Alan Stern, linux-acpi, driver-core, linux-pm, linux-usb,
devicetree, linux-mediatek, linux-arm-kernel, linux-kernel,
Manivannan Sadhasivam, Bartosz Golaszewski, Greg Kroah-Hartman,
Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus,
Rafael J. Wysocki, Danilo Krummrich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno
In-Reply-To: <20260610084053.2059858-2-wenst@chromium.org>
On Wed, 10 Jun 2026 10:40:35 +0200, Chen-Yu Tsai <wenst@chromium.org> said:
> In some cases the driver needs a reference to the port firmware node.
> Once such case is the upcoming USB power sequencing integration. The
> USB hub port is tied to the corresponding port firmware node if it
> exists.
>
> Provide a helper for this.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
^ permalink raw reply
* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
From: Jerome Brunet @ 2026-06-11 8:10 UTC (permalink / raw)
To: Bui Duc Phuc
Cc: Mark Brown, Liam Girdwood, Neil Armstrong, Kevin Hilman,
Martin Blumenstingl, Jaroslav Kysela, Takashi Iwai, linux-sound,
linux-arm-kernel, linux-amlogic, linux-kernel
In-Reply-To: <CAABR9nGC=f_hO7FX2RnRHLb5D2Tt9=qT_yv-Fv2y32AAoQy+sw@mail.gmail.com>
On mer. 10 juin 2026 at 23:27, Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
> Hi Jerome,
>
> Thank you for your feedback,
>
>>
>> I suppose it is OK but it does not seem to really clean anything and
>> make the code easier to follow in that instance, from my perspective at
>> least.
>>
>> If there is policy to systematically use guard() whenever
>> possible then OK, otherwise it seems unnecessary.
>>
>
> I have noticed that guard() has been adopted in several subsystems.
> Since this appears to be the only place in the Meson ASoC code currently using
> mutex_lock()/mutex_unlock(), I converted it for consistency with the
> newer style.
>
> Going forward, should new Meson ASoC code use guard(), or should it continue
> using the traditional mutex_lock()/mutex_unlock() pattern?
Can't say if there is such policy either. IMO it should be more a
case-by-case thing
The code is not better or worse with the change but you went through the
trouble of doing so, if Mark is fine with it, let's have it
Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
>
> Best regards,
> Phuc
--
Jerome
^ permalink raw reply
* Re: [PATCH] rtc: meson: fix refcount leak in meson_rtc_get_bus
From: Philipp Zabel @ 2026-06-11 8:08 UTC (permalink / raw)
To: WenTao Liang
Cc: alexandre.belloni, neil.armstrong, khilman, jbrunet,
martin.blumenstingl, ben.dooks, linux-rtc, linux-arm-kernel,
linux-amlogic, linux-kernel, stable
In-Reply-To: <20260611035605.59906-1-vulab@iscas.ac.cn>
On Thu, Jun 11, 2026 at 11:56:05AM +0800, WenTao Liang wrote:
> In meson_rtc_get_bus(), reset_control_reset() is called to trigger
> a hardware reset when the serial bus is not ready. The function may
> retry up to three times, but neither the successful nor the failure
> path calls reset_control_rearm() to balance the reference count,
> leaking the triggered_count on shared reset controls.
Wrong, this driver uses exclusive reset control, which does not do any
refcounting. Arguably, it should request the reset control via
devm_regulator_get_exclusive() instead of devm_regulator_get() to
make this clear.
> Fix this by adding reset_control_rearm() after reset_control_reset()
> on both the error return path and the success path within the retry
> loop, ensuring the reset control can be re-triggered on subsequent
> bus acquisition attempts.
This doesn't fix anything, reset_control_rearm() does nothing and
should not be used with exclusive reset controls.
regards
Philipp
^ permalink raw reply
* Re: [PATCH v3] media: bcm2835-unicam: Fix log status runtime access
From: Laurent Pinchart @ 2026-06-11 8:03 UTC (permalink / raw)
To: Eugen Hristev
Cc: Raspberry Pi Kernel Maintenance, Mauro Carvalho Chehab,
Florian Fainelli, Broadcom internal kernel review list, Ray Jui,
Scott Branden, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Jean-Michel Hautbois, Naushir Patuck, linux-media,
linux-rpi-kernel, linux-arm-kernel, linux-kernel
In-Reply-To: <20260611-bcmpipm-v3-1-c609dacb029f@kernel.org>
On Thu, Jun 11, 2026 at 08:29:55AM +0300, Eugen Hristev wrote:
> When requesting log status, the block might be powered off, but registers
> are being read.
> Avoid reading the registers if the device is not resumed, thus also avoid
> powering up the device just for log status.
>
> Fixes: 392cd78d495f ("media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface")
> Signed-off-by: Eugen Hristev <ehristev@kernel.org>
> ---
> Changes in v3:
> - Changed to check return value of pm_runtime_get_if_active() and only call
> pm_runtime_put() if the device is active.
> - Link to v2: https://patch.msgid.link/20260522-bcmpipm-v2-1-a3da66cbc9f0@kernel.org
>
> Changes in v2:
> - changed to use pm_runtime_get_if_active()
> - add corresponding put()
> - Link to v1: https://patch.msgid.link/20260521-bcmpipm-v1-1-3eba88d88045@kernel.org
>
> To: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> To: Mauro Carvalho Chehab <mchehab@kernel.org>
> To: Florian Fainelli <florian.fainelli@broadcom.com>
> To: Ray Jui <rjui@broadcom.com>
> To: Scott Branden <sbranden@broadcom.com>
> To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
> To: Sakari Ailus <sakari.ailus@linux.intel.com>
> To: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> To: Hans Verkuil <hverkuil@kernel.org>
> To: Naushir Patuck <naush@raspberrypi.com>
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: linux-media@vger.kernel.org
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/media/platform/broadcom/bcm2835-unicam.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index 8d28ba0b59a3..96b51e29bba4 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -2043,6 +2043,7 @@ static int unicam_log_status(struct file *file, void *fh)
> struct unicam_node *node = video_drvdata(file);
> struct unicam_device *unicam = node->dev;
> u32 reg;
> + int pm_active;
>
> /* status for sub devices */
> v4l2_device_call_all(&unicam->v4l2_dev, 0, core, log_status);
> @@ -2052,6 +2053,14 @@ static int unicam_log_status(struct file *file, void *fh)
> node->fmt.fmt.pix.width, node->fmt.fmt.pix.height);
> dev_info(unicam->dev, "V4L2 format: %08x\n",
> node->fmt.fmt.pix.pixelformat);
> +
> + pm_active = pm_runtime_get_if_active(unicam->dev);
> + if (!pm_active) {
> + dev_info(unicam->dev,
> + "Live data N/A due to device inactive\n");
> + return 0;
> + }
> +
> reg = unicam_reg_read(unicam, UNICAM_IPIPE);
> dev_info(unicam->dev, "Unpacking/packing: %u / %u\n",
> unicam_get_field(reg, UNICAM_PUM_MASK),
> @@ -2065,6 +2074,9 @@ static int unicam_log_status(struct file *file, void *fh)
> dev_info(unicam->dev, "Write pointer: %08x\n",
> unicam_reg_read(unicam, UNICAM_IBWP));
>
> + if (pm_active == 1)
> + pm_runtime_put(unicam->dev);
As far as I understand, the discussion on v2 concluded there was no need
to test pm_active here. Did I miss anything ?
> +
> return 0;
> }
>
>
> ---
> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> change-id: 20260521-bcmpipm-6c578e73239c
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 00/39] Add i.MX95 DPU/DSI/LVDS support
From: Liu Ying @ 2026-06-11 8:01 UTC (permalink / raw)
To: Marek Vasut
Cc: Piyush Patle, dri-devel, imx, linux-arm-kernel, linux-clk,
devicetree, Shawn Guo, Fabio Estevam, Peng Fan, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lucas Stach, Laurent Pinchart,
Thomas Zimmermann, Abel Vesa, Pengutronix Kernel Team
In-Reply-To: <7fc8f50f-7f6d-4b45-b172-a83d97164b40@mailbox.org>
On Wed, Jun 10, 2026 at 06:31:02PM +0200, Marek Vasut wrote:
> On 6/9/26 10:26 AM, Liu Ying wrote:
>
> Hello Liu,
Hello Marek,
>
> > > > > I brought this series up on the i.MX95 15x15 FRDM (IT6263 LVDS-to-HDMI on
> > > > > LVDS ch1). It mostly works, but I ran into a few issues around DI routing,
> > > > > LVDS format handling, and DC enable sequencing which needed rework before
> > > > > HDMI would come up reliably on the board.
> > > > >
> > > > > I don't see a v2 of the series and things seem to have been quiet since
> > > > > November. Are you planning to post an updated version?
> > > >
> > > > My plan was to enable prefetch engine support[1] for i.MX8QXP display
> > > > controller and add device tree for a whole i.MX8QXP LVDS display pipeline,
> > > > before adding i.MX95 display controller support.
> > > >
> > > > Unfortunately, it seems that Marek is not a big fan of [1]
> > >
> > > I am fine with [1] as long as it can be isolated and does not affect every
> > > SoC that might reuse this driver, which I think it can be done.
> >
> > How can it be isolated?
>
> if (compatible("mx8q"))
> something->prefetch_op = somefunction;
I certainly cannot add this to [1], because currently the driver only supports
i.MX8QXP display controller.
>
> And then wherever is prefetch used, do
>
> if (something->prefetch_op)
> something->prefetch_op()
These 'if' checks can be avoided, if i.MX95 display controller is supported
by a separate driver. The checks are just telling us that the i.MX8QXP and
i.MX95 display controllers are different from H/W point of view - i.MX8QXP
SoC has prefetch engines, while i.MX95 SoC has not. Of course, there are a
lot more H/W differences, hence it's worth separate drivers. Without
prefetch engines to do tile to linear resolving for framebuffers, i.MX95
display controller's DRM planes cannot advertise tile specific modifiers.
The separate driver's IOCTL designs for blit-engine could also be different,
because of the prefetch engine and no Linear Tile Store(LTS) [2] attached to
i.MX95 display controller Store unit's output. Userspace could easily check
the DRM driver names to get the IOCTLS that a particular driver supports.
[2] https://elixir.bootlin.com/linux/v7.0/source/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-store.yaml#L73-L76
>
> Or something along those lines ?
>
> > > > and I'm busy
> > > > with downstream development so the plan doesn't move forward well. I still
> > > > think [1] makes sense(maybe I need to rebase it on latest drm-misc-next),
> > > > so I'd like to see review comments on [1] and hopefully people think that
> > > > the overall idea of [1] is ok.
> > >
> > > My only concern is, to keep it isolated to MX8Q, so this driver can be
> > > reused by MX95.
> > >
> > > > > I've accumulated a fair amount of rework while getting this running on the
> > > > > FRDM. If you're not planning a v2, I can clean things up and send one based
> > > > > on the current series.
> > > >
> > > > I still think that i.MX95 display controller driver should be in a separate
> > > > driver, rather than sharing the same driver with i.MX8QXP display controller
> > > > like this patch series does, because the two display controllers are quite
> > > > different as I mentioned in comments on this patch series and in discussion
> > > > in [1]. Also, the common part between the two display controllers should
> > > > be extracted to a common helper library as I mentioned there too.
> > > Are they really? It seems this series adds support for the MX95 DC without
> > > that many changes, so are the DCs really that different ? It seems the MX95
> > > DC is simply a reuse/evolution of the MX8Q DC blocks, so duplicating the
> > > code seems like the wrong direction, it will only lead to disparate sets of
> > > bugs in two drivers, which isn't desired.
> >
> > I pointed out a lot of H/W differences between the two display controllers
> > during the discussions for this patch series and my i.MX8QXP prefetch engine
> > patch series[1]. Please take a look at [1], which clearly shows that the
> > prefetch engine would considerably impact CRTC/plane atomic callback
> > implementations.
>
> Is the prefetch engine actually grown into the CRTC/DE or not ? I suspect it
> is separate and instead part of the built-in DMA, right ?
It is separate and not grown into CRTC/DE.
It sits between DRAM and pixel engine's or blit engine's fetchunit.
>
> > Display controller internal blocks would also impact
> > the implementations, e.g., DomainBlend block in i.MX95 display controller
> > doesn't present in i.MX8QXP display controller. It makes sense to use
> > separate drivers for the two display controllers instead of adding 'if/else'
> > checks to a single driver's atomic callbacks or introducing two pairs of
> > atomic callbacks to that single driver. I mentioned before, the code to
> > simply add a DRM driver(struct drm_driver) is fairly limited.
>
> Can't we simply have two sets of ops (one for mx8q and one for mx95) for
> those ops which are too complicated to implement as a single op with if/else
> statements ?
Technically, we can, but using separate drivers is a better design:
- no 'if/else' checks to advertise those tile specific modifiers or not.
- better IOCTL designs for blit-engine with different DRM driver names.
- again, the code to add a DRM driver(struct drm_driver) is fairly limited.
>
> > I also mentioned before that separate drivers make them easier to maintain:
> > we don't have to test both i.MX8QXP and i.MX95 if only one display controller
> > specific code is changed.
>
> The downside is lack of code reuse, which leads to disparate sets of bugs in
> these two drivers and code duplication. And it seems to me, that large parts
> of the MX8Q and MX95 DC are effectively identical.
A common helper library achieves code reuse, so that's not a downside.
I'd say a lot of display controller internal units are identical, but H/W
differences would significantly impact driver implementations:
- i.MX95-only display controller DomainBlend unit supports 4 different modes,
which impacts plane/CRTC drivers.
- i.MX8QXP-only prefetch engine impacts plane/CRTC drivers and blit-engine
driver/IOCTLs.
- i.MX8QXP-only LTS impacts blit-engine driver/IOCTLs.
>
> > > (I might not fully understand what you have in mind with the helper library
> > > though?)
> >
> > I said this could be something like imx-ldb-helper.c and plus perhaps some
> > callbacks like fg->dc_fg_cfg_videomode().
> Do you perceive that the DC driver cannot be parametrized easily enough that
> it has to be turned into a library like that ? When I look at this patchset,
> esp. the first half which updates the various blocks, it does not seem to me
> that way.
Yes, I do perceive that. Your patchset touches mostly the identical display
controller units(DomainBlend is an exception and could impact plane/CRTC
drivers a lot) and enables kind of minimal features. That's probaly why you
feel that way. If think more about the different part, probably it will make
you feel the other way around.
--
Regards,
Liu Ying
^ permalink raw reply
* [PATCH v7 8/8] perf test: Add Arm CoreSight callchain test
From: Leo Yan @ 2026-06-11 7:57 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, John Garry, Will Deacon, James Clark,
Mike Leach, Suzuki K Poulose, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Al Grant, Paschalis Mpeis, Amir Ayupov
Cc: linux-arm-kernel, coresight, linux-perf-users, Leo Yan
In-Reply-To: <20260611-b4-arm_cs_callchain_support_v1-v7-0-1ba770c862ae@arm.com>
Add a CoreSight shell test for synthesized callchains.
The test uses the new callchain workload to generate trace and decodes
it with synthesis callchain. It then verifies that the instruction
samples show the expected callchain push and pop.
Use control FIFOs so tracing starts only around the workload, which
keeps the trace data small. The test is limited to arm64 systems with
the cs_etm event available.
After:
perf test 136 -vvv
136: CoreSight synthesized callchain:
--- start ---
test child forked, pid 3539
---- end(0) ----
136: CoreSight synthesized callchain : Ok
Assisted-by: Codex:GPT-5.5
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
tools/perf/Documentation/perf-test.txt | 6 +-
tools/perf/tests/builtin-test.c | 1 +
tools/perf/tests/shell/coresight/callchain.sh | 168 ++++++++++++++++++++++++++
tools/perf/tests/tests.h | 1 +
tools/perf/tests/workloads/Build | 2 +
tools/perf/tests/workloads/callchain.c | 24 ++++
6 files changed, 200 insertions(+), 2 deletions(-)
diff --git a/tools/perf/Documentation/perf-test.txt b/tools/perf/Documentation/perf-test.txt
index 81c8525f594680d814f80e6f88bcce8d867bb350..859df74e62efc4b1e80da13ae8e053356f68ae54 100644
--- a/tools/perf/Documentation/perf-test.txt
+++ b/tools/perf/Documentation/perf-test.txt
@@ -57,7 +57,8 @@ OPTIONS
--workload=::
Run a built-in workload, to list them use '--list-workloads', current
ones include: noploop, thloop, leafloop, sqrtloop, brstack, datasym,
- context_switch_loop, deterministic, named_threads and landlock.
+ context_switch_loop, deterministic, named_threads, landlock and
+ callchain.
Used with the shell script regression tests.
@@ -69,7 +70,8 @@ OPTIONS
'named_threads' accepts the number of threads and the number of loops to
do in each thread.
- The datasym, landlock and deterministic workloads don't accept any.
+ The datasym, landlock, deterministic and callchain workloads don't accept
+ any.
--list-workloads::
List the available workloads to use with -w/--workload.
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index afc06cec49546d29d86b94840c7021c5bf5c88e3..8994488cc206863ba77f7e7e5803e62f18e151ba 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -166,6 +166,7 @@ static struct test_workload *workloads[] = {
&workload__jitdump,
&workload__context_switch_loop,
&workload__deterministic,
+ &workload__callchain,
#ifdef HAVE_RUST_SUPPORT
&workload__code_with_type,
diff --git a/tools/perf/tests/shell/coresight/callchain.sh b/tools/perf/tests/shell/coresight/callchain.sh
new file mode 100755
index 0000000000000000000000000000000000000000..e9f907f60e3a4574f13a9c651d541c675f07a4ad
--- /dev/null
+++ b/tools/perf/tests/shell/coresight/callchain.sh
@@ -0,0 +1,168 @@
+#!/bin/bash
+# CoreSight synthesized callchain (exclusive)
+# SPDX-License-Identifier: GPL-2.0
+
+glb_err=1
+
+if ! tmpdir=$(mktemp -d /tmp/perf-cs-callchain-test.XXXXXX); then
+ echo "mktemp failed"
+ exit 1
+fi
+
+cleanup_files()
+{
+ rm -rf "$tmpdir"
+}
+
+trap cleanup_files EXIT
+trap 'cleanup_files; exit $glb_err' TERM INT
+
+skip_if_system_is_not_ready()
+{
+ [ "$(uname -m)" = "aarch64" ] || {
+ echo "Skip: arm64 only test" >&2
+ return 2
+ }
+
+ perf list | grep -Pzq 'cs_etm//' || {
+ echo "Skip: cs_etm event is not available" >&2
+ return 2
+ }
+
+ return 0
+}
+
+record_trace()
+{
+ local data=$1
+ local script=$2
+
+ local cf="$tmpdir/ctl"
+ local af="$tmpdir/ack"
+
+ mkfifo "$cf" "$af"
+
+ perf record -o "$data" -e cs_etm// --per-thread -D -1 --control fifo:"$cf","$af" -- \
+ perf test --record-ctl fifo:"$cf","$af" -w callchain >/dev/null 2>&1 &&
+
+ # It is safe to use 'i3i' with a three-instruction interval, since the
+ # workload is compiled with -O0.
+ perf script --itrace=g16i3il64 -i "$data" > "$script"
+}
+
+callchain_regex_1()
+{
+ printf '%s' \
+'perf[[:space:]]+[0-9]+[[:space:]]+\[[0-9]+\][[:space:]]+([0-9.]+:[[:space:]]+)?[0-9]+ instructions:[[:space:]]*\n'\
+'[[:space:]]+[[:xdigit:]]+ foo\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'[[:space:]]+[[:xdigit:]]+ callchain\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'([[:space:]]+[[:xdigit:]]+ .*\n)*'
+}
+
+callchain_regex_2()
+{
+ printf '%s' \
+'perf[[:space:]]+[0-9]+[[:space:]]+\[[0-9]+\][[:space:]]+([0-9.]+:[[:space:]]+)?[0-9]+ instructions:[[:space:]]*\n'\
+'[[:space:]]+[[:xdigit:]]+ do_syscall\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'[[:space:]]+[[:xdigit:]]+ foo\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'[[:space:]]+[[:xdigit:]]+ callchain\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'([[:space:]]+[[:xdigit:]]+ .*\n)*'
+}
+
+callchain_regex_3()
+{
+ printf '%s' \
+'perf[[:space:]]+[0-9]+[[:space:]]+\[[0-9]+\][[:space:]]+([0-9.]+:[[:space:]]+)?[0-9]+ instructions:[[:space:]]*\n'\
+'[[:space:]]+[[:xdigit:]]+ syscall(@plt)?\+0x[[:xdigit:]]+ \(.*\)\n'\
+'[[:space:]]+[[:xdigit:]]+ do_syscall\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'[[:space:]]+[[:xdigit:]]+ foo\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'[[:space:]]+[[:xdigit:]]+ callchain\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'([[:space:]]+[[:xdigit:]]+ .*\n)*'
+}
+
+callchain_regex_4()
+{
+ printf '%s' \
+'perf[[:space:]]+[0-9]+[[:space:]]+\[[0-9]+\][[:space:]]+([0-9.]+:[[:space:]]+)?[0-9]+ instructions:[[:space:]]*\n'\
+'[[:space:]]+[[:xdigit:]]+ .*\+0x[[:xdigit:]]+ \(\[kernel\.kallsyms\]\)\n'\
+'[[:space:]]+[[:xdigit:]]+ syscall(@plt)?\+0x[[:xdigit:]]+ \(.*\)\n'\
+'[[:space:]]+[[:xdigit:]]+ do_syscall\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'[[:space:]]+[[:xdigit:]]+ foo\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'[[:space:]]+[[:xdigit:]]+ callchain\+0x[[:xdigit:]]+ \(.*/perf\)\n'\
+'([[:space:]]+[[:xdigit:]]+ .*\n)*'
+}
+
+find_after_line()
+{
+ local regex="$1"
+ local file="$2"
+ local start="$3"
+ local offset
+ local line
+
+ # Search in byte offset
+ offset=$(
+ tail -n +"$start" "$file" |
+ grep -Pzob -m1 "$regex" |
+ tr '\0' '\n' |
+ sed -n 's/^\([0-9][0-9]*\):.*/\1/p;q'
+ )
+
+ if [ -z "$offset" ]; then
+ echo "Failed to match regex after line $start" >&2
+ echo "Regex:" >&2
+ printf '%s\n' "$regex" >&2
+ echo "Context from line $start:" >&2
+ sed -n "${start},$((start + 100))p" "$file" >&2
+ return 1
+ fi
+
+ # Conver from offset to line
+ line=$(
+ tail -n +"$start" "$file" |
+ head -c "$offset" |
+ wc -l
+ )
+
+ echo "$((start + line))"
+}
+
+check_callchain_flow()
+{
+ local file="$1"
+ local l1 l2 l3 l4 l5 l6 l7
+
+ # Callchain push
+ l1=$(find_after_line "$(callchain_regex_1)" "$file" 1) || return 1
+ l2=$(find_after_line "$(callchain_regex_2)" "$file" "$((l1 + 1))") || return 1
+ l3=$(find_after_line "$(callchain_regex_3)" "$file" "$((l2 + 1))") || return 1
+ l4=$(find_after_line "$(callchain_regex_4)" "$file" "$((l3 + 1))") || return 1
+
+ # Callchain pop
+ l5=$(find_after_line "$(callchain_regex_3)" "$file" "$((l4 + 1))") || return 1
+ l6=$(find_after_line "$(callchain_regex_2)" "$file" "$((l5 + 1))") || return 1
+ l7=$(find_after_line "$(callchain_regex_1)" "$file" "$((l6 + 1))") || return 1
+
+ return 0
+}
+
+run_test()
+{
+ local data=$tmpdir/perf.data
+ local script=$tmpdir/perf.script
+
+ if ! record_trace "$data" "$script"; then
+ echo "$name: perf record/script failed"
+ return
+ fi
+
+ check_callchain_flow "$script" || return
+
+ glb_err=0
+}
+
+skip_if_system_is_not_ready || exit 2
+
+run_test
+
+exit $glb_err
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 7cedf05be544ad79a99e86d30dfa4f7b01ca0837..cee9e6b62dcc838c864bbe76efe3b638ed75b134 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -248,6 +248,7 @@ DECLARE_WORKLOAD(inlineloop);
DECLARE_WORKLOAD(jitdump);
DECLARE_WORKLOAD(context_switch_loop);
DECLARE_WORKLOAD(deterministic);
+DECLARE_WORKLOAD(callchain);
#ifdef HAVE_RUST_SUPPORT
DECLARE_WORKLOAD(code_with_type);
diff --git a/tools/perf/tests/workloads/Build b/tools/perf/tests/workloads/Build
index 75b377934a0e62b9ac1fec245520ea0978ac957e..dfdf9a2720b22f67a3d7b53d0ed14e0654059c8f 100644
--- a/tools/perf/tests/workloads/Build
+++ b/tools/perf/tests/workloads/Build
@@ -13,6 +13,7 @@ perf-test-y += inlineloop.o
perf-test-y += jitdump.o
perf-test-y += context_switch_loop.o
perf-test-y += deterministic.o
+perf-test-y += callchain.o
ifeq ($(CONFIG_RUST_SUPPORT),y)
perf-test-y += code_with_type.o
@@ -26,3 +27,4 @@ CFLAGS_datasym.o = -g -O0 -fno-inline -U_FORTIFY_SOURCE
CFLAGS_traploop.o = -g -O0 -fno-inline -U_FORTIFY_SOURCE
CFLAGS_inlineloop.o = -g -O2
CFLAGS_deterministic.o = -g -O0 -fno-inline -U_FORTIFY_SOURCE
+CFLAGS_callchain.o = -g -O0 -fno-inline -U_FORTIFY_SOURCE
diff --git a/tools/perf/tests/workloads/callchain.c b/tools/perf/tests/workloads/callchain.c
new file mode 100644
index 0000000000000000000000000000000000000000..a7eca77277496eec2f47d6f1a646552c298a533f
--- /dev/null
+++ b/tools/perf/tests/workloads/callchain.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/compiler.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+#include "../tests.h"
+
+static void do_syscall(void)
+{
+ syscall(SYS_getpid);
+}
+
+static void foo(void)
+{
+ do_syscall();
+}
+
+static int callchain(int argc __maybe_unused, const char **argv __maybe_unused)
+{
+ foo();
+
+ return 0;
+}
+
+DEFINE_WORKLOAD(callchain);
--
2.34.1
^ permalink raw reply related
* [PATCH v7 7/8] perf cs-etm: Synthesize callchains for instruction samples
From: Leo Yan @ 2026-06-11 7:57 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, John Garry, Will Deacon, James Clark,
Mike Leach, Suzuki K Poulose, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Al Grant, Paschalis Mpeis, Amir Ayupov
Cc: linux-arm-kernel, coresight, linux-perf-users, Leo Yan, Leo Yan
In-Reply-To: <20260611-b4-arm_cs_callchain_support_v1-v7-0-1ba770c862ae@arm.com>
From: Leo Yan <leo.yan@linaro.org>
CS ETM already records branches into the thread stack, but instruction
samples do not carry synthesized callchains. It misses to support the
callchain and no output with the itrace option 'g'.
Allocate a callchain buffer per queue and use thread_stack__sample()
when synthesizing instruction samples.
Advertise PERF_SAMPLE_CALLCHAIN on the synthetic instruction event.
Allocate one extra callchain entry than requested, as the first entry
is reserved for storing context information.
cs_etm__context() is introduced for handling context packet and update
the thread info and start kernel address for frontend decoding.
After:
perf script --itrace=g16l64i1i
callchain_test 6543 [002] 1 instructions:
ffff800080010c14 vectors+0x414 ([kernel.kallsyms])
aaaad6b60784 do_svc+0x1c (/home/kernel/leoy/test_cs_callchain/callchain_test)
aaaad6b60798 print+0xc (/home/kernel/leoy/test_cs_callchain/callchain_test)
aaaad6b607b0 foo+0xc (/home/kernel/leoy/test_cs_callchain/callchain_test)
aaaad6b607c8 main+0xc (/home/kernel/leoy/test_cs_callchain/callchain_test)
ffff9325225c __libc_start_call_main+0x7c (/usr/lib/aarch64-linux-gnu/libc.so.6)
ffff9325233c call_init+0x9c (inlined)
ffff9325233c __libc_start_main_impl+0x9c (inlined)
aaaad6b60670 _start+0x30 (/home/kernel/leoy/test_cs_callchain/callchain_test)
ffff800080012290 ret_to_user+0x120 ([kernel.kallsyms])
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: James Clark <james.clark@linaro.org>
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
tools/perf/util/cs-etm.c | 83 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 78 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 830618763d8b1bdcc015c492d7b2354d862566ca..f37aa41b3587aad063ea464bc460fe3438bd039d 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -17,6 +17,7 @@
#include <stdlib.h>
#include "auxtrace.h"
+#include "callchain.h"
#include "color.h"
#include "cs-etm.h"
#include "cs-etm-decoder/cs-etm-decoder.h"
@@ -86,9 +87,11 @@ struct cs_etm_auxtrace {
struct cs_etm_traceid_queue {
u8 trace_chan_id;
u64 period_instructions;
+ u64 kernel_start;
union perf_event *event_buf;
unsigned int br_stack_sz;
struct branch_stack *last_branch;
+ struct ip_callchain *callchain;
struct cs_etm_packet *prev_packet;
struct cs_etm_packet *packet;
struct cs_etm_packet_queue packet_queue;
@@ -649,6 +652,15 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
tidq->br_stack_sz = etm->synth_opts.last_branch_sz;
}
+ if (etm->synth_opts.callchain) {
+ /* Add 1 to callchain_sz for callchain context */
+ tidq->callchain =
+ zalloc(struct_size(tidq->callchain, ips,
+ etm->synth_opts.callchain_sz + 1));
+ if (!tidq->callchain)
+ goto out_free;
+ }
+
tidq->event_buf = malloc(PERF_SAMPLE_MAX_SIZE);
if (!tidq->event_buf)
goto out_free;
@@ -656,6 +668,7 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
return 0;
out_free:
+ zfree(&tidq->callchain);
zfree(&tidq->last_branch);
zfree(&tidq->prev_packet);
zfree(&tidq->packet);
@@ -937,6 +950,7 @@ static void cs_etm__free_traceid_queues(struct cs_etm_queue *etmq)
thread__zput(tidq->frontend_thread);
thread__zput(tidq->decode_thread);
zfree(&tidq->event_buf);
+ zfree(&tidq->callchain);
zfree(&tidq->last_branch);
zfree(&tidq->prev_packet);
zfree(&tidq->packet);
@@ -1602,6 +1616,26 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
sample.branch_stack = tidq->last_branch;
}
+ if (etm->synth_opts.callchain) {
+ if (tidq->kernel_start)
+ thread_stack__sample(tidq->frontend_thread,
+ tidq->packet->cpu,
+ tidq->callchain,
+ etm->synth_opts.callchain_sz + 1,
+ sample.ip, tidq->kernel_start);
+ else
+ /*
+ * Clear the callchain when the kernel start address is
+ * not available yet. The empty callchain can then be
+ * consumed by cs_etm__inject_event().
+ */
+ memset(tidq->callchain, 0,
+ struct_size(tidq->callchain, ips,
+ etm->synth_opts.callchain_sz + 1));
+
+ sample.callchain = tidq->callchain;
+ }
+
if (etm->synth_opts.inject) {
ret = cs_etm__inject_event(etm, event, &sample,
etm->instructions_sample_type);
@@ -1764,6 +1798,9 @@ static int cs_etm__synth_events(struct cs_etm_auxtrace *etm,
attr.branch_sample_type |= PERF_SAMPLE_BRANCH_HW_INDEX;
}
+ if (etm->synth_opts.callchain)
+ attr.sample_type |= PERF_SAMPLE_CALLCHAIN;
+
if (etm->synth_opts.instructions) {
attr.config = PERF_COUNT_HW_INSTRUCTIONS;
attr.sample_period = etm->synth_opts.period;
@@ -1895,6 +1932,34 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
return 0;
}
+static int cs_etm__context(struct cs_etm_queue *etmq,
+ struct cs_etm_traceid_queue *tidq)
+{
+ ocsd_ex_level el = tidq->packet->el;
+ struct machine *machine;
+ int ret;
+
+ machine = cs_etm__get_machine(etmq, el);
+ if (!machine) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ tidq->kernel_start = machine__kernel_start(machine);
+
+ ret = cs_etm__etmq_update_thread(etmq, el, tidq->packet->tid,
+ &tidq->frontend_thread);
+ if (ret)
+ goto err;
+
+ return 0;
+
+err:
+ tidq->frontend_thread = NULL;
+ tidq->kernel_start = 0;
+ return ret;
+}
+
static int cs_etm__exception(struct cs_etm_traceid_queue *tidq)
{
/*
@@ -2487,9 +2552,7 @@ static int cs_etm__process_traceid_queue(struct cs_etm_queue *etmq,
* tracing the kernel the context packet will be emitted
* between two ranges.
*/
- ret = cs_etm__etmq_update_thread(etmq, tidq->packet->el,
- tidq->packet->tid,
- &tidq->frontend_thread);
+ ret = cs_etm__context(etmq, tidq);
if (ret)
goto out;
break;
@@ -3507,6 +3570,14 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
PERF_IP_FLAG_TRACE_BEGIN |
PERF_IP_FLAG_TRACE_END;
+ if (etm->synth_opts.callchain && !symbol_conf.use_callchain) {
+ symbol_conf.use_callchain = true;
+ if (callchain_register_param(&callchain_param) < 0) {
+ symbol_conf.use_callchain = false;
+ etm->synth_opts.callchain = false;
+ }
+ }
+
etm->session = session;
etm->num_cpu = num_cpu;
@@ -3558,9 +3629,11 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
}
etm->use_thread_stack = etm->synth_opts.thread_stack ||
- etm->synth_opts.last_branch;
+ etm->synth_opts.last_branch ||
+ etm->synth_opts.callchain;
- etm->use_callchain = etm->synth_opts.thread_stack;
+ etm->use_callchain = etm->synth_opts.thread_stack ||
+ etm->synth_opts.callchain;
err = cs_etm__synth_events(etm, session);
if (err)
--
2.34.1
^ permalink raw reply related
* [PATCH v7 6/8] perf cs-etm: Support call indentation
From: Leo Yan @ 2026-06-11 7:57 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, John Garry, Will Deacon, James Clark,
Mike Leach, Suzuki K Poulose, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Al Grant, Paschalis Mpeis, Amir Ayupov
Cc: linux-arm-kernel, coresight, linux-perf-users, Leo Yan, Leo Yan
In-Reply-To: <20260611-b4-arm_cs_callchain_support_v1-v7-0-1ba770c862ae@arm.com>
From: Leo Yan <leo.yan@linaro.org>
The perf script callindent is derived from call stack in thread context,
CS ETM ignores the requirement for callindent without pushing and poping
call stack.
Enable thread-stack when either itrace thread-stack support or last branch
entries are requested, allocate the branch stack storage accordingly, and
feed taken branches to thread_stack__event() whenever thread-stack state
is needed.
When callindent is requested, pass callstack=true to thread_stack__event()
so the common thread-stack code maintains call depth for branch samples.
Before:
perf script -F +callindent
callchain_test 6543 [002] 1 branches: main ffff93252258 __libc_start_call_main+0x78 (/usr/lib/aarch64-linux-gnu/libc.so.6)
callchain_test 6543 [002] 1 branches: foo aaaad6b607c4 main+0x8 (/home/kernel/leoy/test_cs_callchain/callchain_test)
callchain_test 6543 [002] 1 branches: print aaaad6b607ac foo+0x8 (/home/kernel/leoy/test_cs_callchain/callchain_test)
callchain_test 6543 [002] 1 branches: do_svc aaaad6b60794 print+0x8 (/home/kernel/leoy/test_cs_callchain/callchain_test)
callchain_test 6543 [002] 1 branches: vectors aaaad6b60780 do_svc+0x18 (/home/kernel/leoy/test_cs_callchain/callchain_test)
callchain_test 6543 [002] 1 branches: el0t_64_sync_handler ffff80008001159c el0t_64_sync+0x194 ([kernel.kallsyms])
callchain_test 6543 [002] 1 branches: el0_svc ffff800081829194 el0t_64_sync_handler+0x9c ([kernel.kallsyms])
callchain_test 6543 [002] 1 branches: lockdep_hardirqs_off ffff800081828794 el0_svc+0x24 ([kernel.kallsyms])
callchain_test 6543 [002] 1 branches: __this_cpu_preempt_check ffff80008182b348 lockdep_hardirqs_off+0xf0 ([kernel.kallsyms])
After:
callchain_test 6543 [002] 1 branches: main ffff93252258 __libc_start_call_main+0x78 (/usr/lib/aarch64-linux-gnu/libc.so.6)
callchain_test 6543 [002] 1 branches: foo aaaad6b607c4 main+0x8 (/home/kernel/leoy/test_cs_callchain/callchain_test)
callchain_test 6543 [002] 1 branches: print aaaad6b607ac foo+0x8 (/home/kernel/leoy/test_cs_callchain/callchain_test)
callchain_test 6543 [002] 1 branches: do_svc aaaad6b60794 print+0x8 (/home/kernel/leoy/test_cs_callchain/callchain_test)
callchain_test 6543 [002] 1 branches: vectors aaaad6b60780 do_svc+0x18 (/home/kernel/leoy/test_cs_callchain/callchain_test)
callchain_test 6543 [002] 1 branches: el0t_64_sync_handler ffff80008001159c el0t_64_sync+0x194 ([kernel.kallsyms])
callchain_test 6543 [002] 1 branches: el0_svc ffff800081829194 el0t_64_sync_handler+0x9c ([kernel.kallsyms])
callchain_test 6543 [002] 1 branches: lockdep_hardirqs_off ffff800081828794 el0_svc+0x24 ([kernel.kallsyms])
callchain_test 6543 [002] 1 branches: __this_cpu_preempt_check ffff80008182b348 lockdep_hardirqs_off+0xf0 ([kernel.kallsyms])
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: James Clark <james.clark@linaro.org>
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
tools/perf/util/cs-etm.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 7069b4990e6107fdece3cc5451142714f1d627ef..830618763d8b1bdcc015c492d7b2354d862566ca 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -66,6 +66,8 @@ struct cs_etm_auxtrace {
bool snapshot_mode;
bool data_queued;
bool has_virtual_ts; /* Virtual/Kernel timestamps in the trace. */
+ bool use_thread_stack;
+ bool use_callchain;
int num_cpu;
u64 latest_kernel_timestamp;
@@ -635,7 +637,7 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
if (!tidq->prev_packet)
goto out_free;
- if (etm->synth_opts.last_branch) {
+ if (etm->use_thread_stack) {
size_t sz = sizeof(struct branch_stack);
sz += etm->synth_opts.last_branch_sz *
@@ -1545,7 +1547,7 @@ static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
if (!cs_etm__packet_has_taken_branch(tidq->prev_packet))
return;
- if (etmq->etm->synth_opts.last_branch) {
+ if (etmq->etm->use_thread_stack) {
from = cs_etm__last_executed_instr(tidq->prev_packet);
to = cs_etm__first_executed_instr(tidq->packet);
@@ -1554,7 +1556,8 @@ static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
/* Enable callchain so thread stack entry can be allocated */
thread_stack__event(tidq->frontend_thread, tidq->prev_packet->cpu,
tidq->prev_packet->flags, from, to, size,
- etmq->buffer->buffer_nr + 1, false,
+ etmq->buffer->buffer_nr + 1,
+ etmq->etm->use_callchain,
tidq->br_stack_sz, 0);
} else {
thread_stack__set_trace_nr(tidq->frontend_thread,
@@ -1955,7 +1958,7 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
cs_etm__packet_swap(etm, tidq);
/* Reset last branches after flush the trace */
- if (etm->synth_opts.last_branch)
+ if (etm->use_thread_stack)
thread_stack__flush(tidq->frontend_thread);
return err;
@@ -2018,7 +2021,7 @@ static void cs_etm__flush_all_stack(struct cs_etm_queue *etmq)
{
enum cs_etm_pid_fmt pid_fmt = cs_etm__get_pid_fmt(etmq);
- if (!etmq->etm->synth_opts.last_branch)
+ if (!etmq->etm->use_thread_stack)
return;
cs_etm__flush_machine_stack(etmq, HOST_KERNEL_ID);
@@ -3491,6 +3494,7 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
itrace_synth_opts__set_default(&etm->synth_opts,
session->itrace_synth_opts->default_no_sample);
etm->synth_opts.callchain = false;
+ etm->synth_opts.thread_stack = session->itrace_synth_opts->thread_stack;
}
if (etm->synth_opts.calls)
@@ -3552,6 +3556,12 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
etm->tc.cap_user_time_zero = tc->cap_user_time_zero;
etm->tc.cap_user_time_short = tc->cap_user_time_short;
}
+
+ etm->use_thread_stack = etm->synth_opts.thread_stack ||
+ etm->synth_opts.last_branch;
+
+ etm->use_callchain = etm->synth_opts.thread_stack;
+
err = cs_etm__synth_events(etm, session);
if (err)
goto err_free_queues;
--
2.34.1
^ permalink raw reply related
* [PATCH v7 5/8] perf cs-etm: Flush thread stacks after decoder reset
From: Leo Yan @ 2026-06-11 7:56 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, John Garry, Will Deacon, James Clark,
Mike Leach, Suzuki K Poulose, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Al Grant, Paschalis Mpeis, Amir Ayupov
Cc: linux-arm-kernel, coresight, linux-perf-users, Leo Yan
In-Reply-To: <20260611-b4-arm_cs_callchain_support_v1-v7-0-1ba770c862ae@arm.com>
Perf resets the CoreSight decoder when moving to a new AUX trace buffer,
this causes trace discontinunity globally.
For callchain synthesis, keeping thread-stack state after decoder reset
can leave stale call/return history attached to threads that are decoded
later, producing incorrect synthesized callchains.
Flush all host thread stacks after a decoder reset. When virtualization
is present, flush the guest thread stacks as well.
Reviewed-by: James Clark <james.clark@linaro.org>
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
tools/perf/util/cs-etm.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 8798bf0471faf3b1813780b45c588263ff6b4416..7069b4990e6107fdece3cc5451142714f1d627ef 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -1997,6 +1997,37 @@ static int cs_etm__end_block(struct cs_etm_queue *etmq,
return 0;
}
+
+static int cs_etm__flush_stack_cb(struct thread *thread,
+ void *data __maybe_unused)
+{
+ thread_stack__flush(thread);
+ return 0;
+}
+
+static void cs_etm__flush_machine_stack(struct cs_etm_queue *etmq, pid_t pid)
+{
+ struct machine *machine;
+
+ machine = machines__find(&etmq->etm->session->machines, pid);
+ if (machine)
+ machine__for_each_thread(machine, cs_etm__flush_stack_cb, NULL);
+}
+
+static void cs_etm__flush_all_stack(struct cs_etm_queue *etmq)
+{
+ enum cs_etm_pid_fmt pid_fmt = cs_etm__get_pid_fmt(etmq);
+
+ if (!etmq->etm->synth_opts.last_branch)
+ return;
+
+ cs_etm__flush_machine_stack(etmq, HOST_KERNEL_ID);
+
+ /* Clear the guest stack if virtualization is supported */
+ if (pid_fmt == CS_ETM_PIDFMT_CTXTID2)
+ cs_etm__flush_machine_stack(etmq, DEFAULT_GUEST_KERNEL_ID);
+}
+
/*
* cs_etm__get_data_block: Fetch a block from the auxtrace_buffer queue
* if need be.
@@ -2019,6 +2050,12 @@ static int cs_etm__get_data_block(struct cs_etm_queue *etmq)
ret = cs_etm_decoder__reset(etmq->decoder);
if (ret)
return ret;
+
+ /*
+ * Since the decoder is reset, this causes a global trace
+ * discontinuity. Flush all thread stacks.
+ */
+ cs_etm__flush_all_stack(etmq);
}
return etmq->buf_len;
--
2.34.1
^ permalink raw reply related
* [PATCH v7 4/8] perf cs-etm: Use thread-stack for last branch entries
From: Leo Yan @ 2026-06-11 7:56 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, John Garry, Will Deacon, James Clark,
Mike Leach, Suzuki K Poulose, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Al Grant, Paschalis Mpeis, Amir Ayupov
Cc: linux-arm-kernel, coresight, linux-perf-users, Leo Yan
In-Reply-To: <20260611-b4-arm_cs_callchain_support_v1-v7-0-1ba770c862ae@arm.com>
CS ETM maintains its own circular array for last branch entries, with
local helpers to update, copy and reset the branch stack. This
duplicates logic already provided by the common code.
Record taken branches with thread_stack__event() and synthesize
PERF_SAMPLE_BRANCH_STACK data with thread_stack__br_sample(). This
removes the private last_branch_rb buffer and its position tracking.
This also makes the branch history state belong to the thread rather
than the trace queue. That is a better fit for CoreSight traces where
a trace queue can effectively be CPU scoped, while call/return history
is per thread.
Keep the buffer number updated via thread_stack__set_trace_nr(), which
is used when exporting samples to Python scripts. Pass callstack=false
for now; synthesized callchains are added by a later patch.
The output should remain same, except that be->flags.predicted is no
longer set. Since CoreSight trace does not provide branch prediction
information, clearing the flag avoids confusion.
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
tools/perf/util/cs-etm.c | 159 ++++++++++++++---------------------------------
1 file changed, 46 insertions(+), 113 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 4127120459418389ca7aabb9a49dead2b50e7533..8798bf0471faf3b1813780b45c588263ff6b4416 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -84,10 +84,9 @@ struct cs_etm_auxtrace {
struct cs_etm_traceid_queue {
u8 trace_chan_id;
u64 period_instructions;
- size_t last_branch_pos;
union perf_event *event_buf;
+ unsigned int br_stack_sz;
struct branch_stack *last_branch;
- struct branch_stack *last_branch_rb;
struct cs_etm_packet *prev_packet;
struct cs_etm_packet *packet;
struct cs_etm_packet_queue packet_queue;
@@ -644,9 +643,8 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
tidq->last_branch = zalloc(sz);
if (!tidq->last_branch)
goto out_free;
- tidq->last_branch_rb = zalloc(sz);
- if (!tidq->last_branch_rb)
- goto out_free;
+
+ tidq->br_stack_sz = etm->synth_opts.last_branch_sz;
}
tidq->event_buf = malloc(PERF_SAMPLE_MAX_SIZE);
@@ -656,7 +654,6 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
return 0;
out_free:
- zfree(&tidq->last_branch_rb);
zfree(&tidq->last_branch);
zfree(&tidq->prev_packet);
zfree(&tidq->packet);
@@ -939,7 +936,6 @@ static void cs_etm__free_traceid_queues(struct cs_etm_queue *etmq)
thread__zput(tidq->decode_thread);
zfree(&tidq->event_buf);
zfree(&tidq->last_branch);
- zfree(&tidq->last_branch_rb);
zfree(&tidq->prev_packet);
zfree(&tidq->packet);
zfree(&tidq);
@@ -1299,57 +1295,6 @@ static int cs_etm__queue_first_cs_timestamp(struct cs_etm_auxtrace *etm,
return ret;
}
-static inline
-void cs_etm__copy_last_branch_rb(struct cs_etm_queue *etmq,
- struct cs_etm_traceid_queue *tidq)
-{
- struct branch_stack *bs_src = tidq->last_branch_rb;
- struct branch_stack *bs_dst = tidq->last_branch;
- size_t nr = 0;
-
- /*
- * Set the number of records before early exit: ->nr is used to
- * determine how many branches to copy from ->entries.
- */
- bs_dst->nr = bs_src->nr;
-
- /*
- * Early exit when there is nothing to copy.
- */
- if (!bs_src->nr)
- return;
-
- /*
- * As bs_src->entries is a circular buffer, we need to copy from it in
- * two steps. First, copy the branches from the most recently inserted
- * branch ->last_branch_pos until the end of bs_src->entries buffer.
- */
- nr = etmq->etm->synth_opts.last_branch_sz - tidq->last_branch_pos;
- memcpy(&bs_dst->entries[0],
- &bs_src->entries[tidq->last_branch_pos],
- sizeof(struct branch_entry) * nr);
-
- /*
- * If we wrapped around at least once, the branches from the beginning
- * of the bs_src->entries buffer and until the ->last_branch_pos element
- * are older valid branches: copy them over. The total number of
- * branches copied over will be equal to the number of branches asked by
- * the user in last_branch_sz.
- */
- if (bs_src->nr >= etmq->etm->synth_opts.last_branch_sz) {
- memcpy(&bs_dst->entries[nr],
- &bs_src->entries[0],
- sizeof(struct branch_entry) * tidq->last_branch_pos);
- }
-}
-
-static inline
-void cs_etm__reset_last_branch_rb(struct cs_etm_traceid_queue *tidq)
-{
- tidq->last_branch_pos = 0;
- tidq->last_branch_rb->nr = 0;
-}
-
static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,
struct cs_etm_traceid_queue *tidq,
struct cs_etm_packet *packet, u64 addr)
@@ -1419,38 +1364,6 @@ static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,
return addr;
}
-static void cs_etm__update_last_branch_rb(struct cs_etm_queue *etmq,
- struct cs_etm_traceid_queue *tidq)
-{
- struct branch_stack *bs = tidq->last_branch_rb;
- struct branch_entry *be;
-
- /*
- * The branches are recorded in a circular buffer in reverse
- * chronological order: we start recording from the last element of the
- * buffer down. After writing the first element of the stack, move the
- * insert position back to the end of the buffer.
- */
- if (!tidq->last_branch_pos)
- tidq->last_branch_pos = etmq->etm->synth_opts.last_branch_sz;
-
- tidq->last_branch_pos -= 1;
-
- be = &bs->entries[tidq->last_branch_pos];
- be->from = cs_etm__last_executed_instr(tidq->prev_packet);
- be->to = cs_etm__first_executed_instr(tidq->packet);
- /* No support for mispredict */
- be->flags.mispred = 0;
- be->flags.predicted = 1;
-
- /*
- * Increment bs->nr until reaching the number of last branches asked by
- * the user on the command line.
- */
- if (bs->nr < etmq->etm->synth_opts.last_branch_sz)
- bs->nr += 1;
-}
-
static int cs_etm__inject_event(struct cs_etm_auxtrace *etm, union perf_event *event,
struct perf_sample *sample, u64 type)
{
@@ -1614,6 +1527,42 @@ static inline u64 cs_etm__resolve_sample_time(struct cs_etm_queue *etmq,
return etm->latest_kernel_timestamp;
}
+static bool cs_etm__packet_has_taken_branch(struct cs_etm_packet *packet)
+{
+ if (packet->sample_type == CS_ETM_RANGE &&
+ packet->last_instr_taken_branch)
+ return true;
+
+ return false;
+}
+
+static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
+ struct cs_etm_traceid_queue *tidq)
+{
+ u64 from, to;
+ int size;
+
+ if (!cs_etm__packet_has_taken_branch(tidq->prev_packet))
+ return;
+
+ if (etmq->etm->synth_opts.last_branch) {
+ from = cs_etm__last_executed_instr(tidq->prev_packet);
+ to = cs_etm__first_executed_instr(tidq->packet);
+
+ size = cs_etm__instr_size(etmq, tidq, tidq->prev_packet, from);
+
+ /* Enable callchain so thread stack entry can be allocated */
+ thread_stack__event(tidq->frontend_thread, tidq->prev_packet->cpu,
+ tidq->prev_packet->flags, from, to, size,
+ etmq->buffer->buffer_nr + 1, false,
+ tidq->br_stack_sz, 0);
+ } else {
+ thread_stack__set_trace_nr(tidq->frontend_thread,
+ tidq->prev_packet->cpu,
+ etmq->buffer->buffer_nr + 1);
+ }
+}
+
static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
struct cs_etm_traceid_queue *tidq,
struct cs_etm_packet *packet,
@@ -1644,8 +1593,11 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
cs_etm__copy_insn(etmq, tidq, packet, &sample);
- if (etm->synth_opts.last_branch)
+ if (etm->synth_opts.last_branch) {
+ thread_stack__br_sample(tidq->frontend_thread, tidq->packet->cpu,
+ tidq->last_branch, tidq->br_stack_sz);
sample.branch_stack = tidq->last_branch;
+ }
if (etm->synth_opts.inject) {
ret = cs_etm__inject_event(etm, event, &sample,
@@ -1836,14 +1788,7 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
tidq->period_instructions += tidq->packet->instr_count;
- /*
- * Record a branch when the last instruction in
- * PREV_PACKET is a branch.
- */
- if (etm->synth_opts.last_branch &&
- tidq->prev_packet->sample_type == CS_ETM_RANGE &&
- tidq->prev_packet->last_instr_taken_branch)
- cs_etm__update_last_branch_rb(etmq, tidq);
+ cs_etm__add_stack_event(etmq, tidq);
if (etm->synth_opts.instructions &&
tidq->period_instructions >= etm->instructions_sample_period) {
@@ -1902,10 +1847,6 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
u64 offset = etm->instructions_sample_period - instrs_prev;
u64 addr;
- /* Prepare last branches for instruction sample */
- if (etm->synth_opts.last_branch)
- cs_etm__copy_last_branch_rb(etmq, tidq);
-
while (tidq->period_instructions >=
etm->instructions_sample_period) {
/*
@@ -1936,8 +1877,7 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
generate_sample = true;
/* Generate sample for branch taken packet */
- if (tidq->prev_packet->sample_type == CS_ETM_RANGE &&
- tidq->prev_packet->last_instr_taken_branch)
+ if (cs_etm__packet_has_taken_branch(tidq->prev_packet))
generate_sample = true;
if (generate_sample) {
@@ -1985,10 +1925,6 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
etmq->etm->synth_opts.instructions &&
tidq->prev_packet->sample_type == CS_ETM_RANGE) {
u64 addr;
-
- /* Prepare last branches for instruction sample */
- cs_etm__copy_last_branch_rb(etmq, tidq);
-
/*
* Generate a last branch event for the branches left in the
* circular buffer at the end of the trace.
@@ -2020,7 +1956,7 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
/* Reset last branches after flush the trace */
if (etm->synth_opts.last_branch)
- cs_etm__reset_last_branch_rb(tidq);
+ thread_stack__flush(tidq->frontend_thread);
return err;
}
@@ -2044,9 +1980,6 @@ static int cs_etm__end_block(struct cs_etm_queue *etmq,
tidq->prev_packet->sample_type == CS_ETM_RANGE) {
u64 addr;
- /* Prepare last branches for instruction sample */
- cs_etm__copy_last_branch_rb(etmq, tidq);
-
/*
* Use the address of the end of the last reported execution
* range.
--
2.34.1
^ permalink raw reply related
* [PATCH v7 3/8] perf cs-etm: Refactor instruction size handling
From: Leo Yan @ 2026-06-11 7:56 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, John Garry, Will Deacon, James Clark,
Mike Leach, Suzuki K Poulose, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Al Grant, Paschalis Mpeis, Amir Ayupov
Cc: linux-arm-kernel, coresight, linux-perf-users, Leo Yan, Leo Yan
In-Reply-To: <20260611-b4-arm_cs_callchain_support_v1-v7-0-1ba770c862ae@arm.com>
From: Leo Yan <leo.yan@linaro.org>
This patch introduces a new function cs_etm__instr_size() to calculate
the instruction size based on ISA type and instruction address.
Given the trace data can be MB and most likely that will be A64/A32 on
a lot of platforms, cs_etm__instr_addr() keeps a single ISA type check
for A64/A32 and executes an optimized calculation (addr + offset * 4).
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: James Clark <james.clark@linaro.org>
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
tools/perf/util/cs-etm.c | 43 ++++++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index b4d598ccabbd2551affdc8feed5c63bac4fee98d..4127120459418389ca7aabb9a49dead2b50e7533 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -1366,6 +1366,18 @@ static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,
return ((instrBytes[1] & 0xF8) >= 0xE8) ? 4 : 2;
}
+static inline int cs_etm__instr_size(struct cs_etm_queue *etmq,
+ struct cs_etm_traceid_queue *tidq,
+ struct cs_etm_packet *packet,
+ u64 addr)
+{
+ if (packet->isa == CS_ETM_ISA_T32)
+ return cs_etm__t32_instr_size(etmq, tidq, packet, addr);
+
+ /* Otherwise, 4-byte instruction size for A32/A64 */
+ return 4;
+}
+
static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)
{
/*
@@ -1394,19 +1406,17 @@ static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,
struct cs_etm_packet *packet,
u64 offset)
{
- if (packet->isa == CS_ETM_ISA_T32) {
- u64 addr = packet->start_addr;
+ u64 addr = packet->start_addr;
- while (offset) {
- addr += cs_etm__t32_instr_size(etmq, tidq, packet,
- addr);
- offset--;
- }
- return addr;
- }
+ /* 4-byte instruction size for A32/A64 */
+ if (packet->isa == CS_ETM_ISA_A64 || packet->isa == CS_ETM_ISA_A32)
+ return addr + offset * 4;
- /* Assume a 4 byte instruction size (A32/A64) */
- return packet->start_addr + offset * 4;
+ while (offset) {
+ addr += cs_etm__instr_size(etmq, tidq, packet, addr);
+ offset--;
+ }
+ return addr;
}
static void cs_etm__update_last_branch_rb(struct cs_etm_queue *etmq,
@@ -1576,16 +1586,7 @@ static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
return;
}
- /*
- * T32 instruction size might be 32-bit or 16-bit, decide by calling
- * cs_etm__t32_instr_size().
- */
- if (packet->isa == CS_ETM_ISA_T32)
- sample->insn_len = cs_etm__t32_instr_size(etmq, tidq, packet,
- sample->ip);
- /* Otherwise, A64 and A32 instruction size are always 32-bit. */
- else
- sample->insn_len = 4;
+ sample->insn_len = cs_etm__instr_size(etmq, tidq, packet, sample->ip);
cs_etm__frontend_mem_access(etmq, tidq, packet, sample->ip,
sample->insn_len, (void *)sample->insn);
--
2.34.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox