Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The patch writes 0x2 to RK_MMU_AUTO_GATING to clear bit 1, but this value actually sets bit 1 and clears bit 0, doing the exact opposite of the intended behavior. - [High] An SoC-specific workaround is applied unconditionally to all Rockchip IOMMUs using a brittle hardcoded register overwrite. --
commit 86bf3e3b9a68d419b8e6ba9d4d3cf8afb0c324bb Author: Midgy BALON <[email protected]> iommu/rockchip: Clear AUTO_GATING bit 1 on the RK356x v1 IOMMU This commit aims to fix a hardware lockup on the RK356x v1 IOMMU by clearing bit 1 of the RK_MMU_AUTO_GATING register. This change is intended to prevent auto clock-gating of the page-table walker. > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index 0013cf196c573..7d40ecb26123f 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -952,6 +952,18 @@ static int rk_iommu_enable(struct rk_iommu *iommu) > > ret = rk_iommu_enable_paging(iommu); > > + if (!ret) { > + /* > + * RK356x v1 IOMMU: RK_MMU_AUTO_GATING bit 1 enables page-walker > + * auto clock-gating; the walker's AXI master then loses its > clock > + * between transactions and a TLB-miss page walk never > completes, > + * leaving the IOMMU stuck (PAGING_ENABLED, never IDLE). Clear > + * bit 1 (keep bit 0, the slave-port gate) once paging is > enabled. > + */ > + for (i = 0; i < iommu->num_mmu; i++) [Severity: High] Will this loop unconditionally apply the RK356x v1 workaround to all SoCs supported by the rockchip-iommu driver? It seems older variants like the RK3288 or RK3399 might unexpectedly receive this register write in rk_iommu_enable() without a check for the specific SoC version. > + rk_iommu_write(iommu->bases[i], RK_MMU_AUTO_GATING, > 0x2); [Severity: High] Does writing 0x2 here achieve the intended result? The comment mentions clearing bit 1 and keeping bit 0. Since the register resets to 0x3 (binary 11), writing 0x2 (binary 10) actually sets bit 1 and clears bit 0. This seems to do the exact opposite of what is described, keeping the buggy page-walker auto clock-gating enabled and inadvertently disabling the working slave-port gate. Should this be 0x1 instead? Additionally, because rk_iommu_write() completely overwrites the register, could this clobber other bits in RK_MMU_AUTO_GATING? Would it be safer to use a read-modify-write pattern here? > + } > + > out_disable_stall: > rk_iommu_disable_stall(iommu); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
