Message-ID: <8634bndaoa.wl-maz@kernel.org>
Date: Wed, 25 Jun 2025 17:54:13 +0100
From: Marc Zyngier <maz@...nel.org>
To: Will Deacon <will@...nel.org>
Cc: Xueqi Zhang <xueqi.zhang@...iatek.com>,
	Yong Wu <yong.wu@...iatek.com>,
	Robin Murphy <robin.murphy@....com>,
	Joerg Roedel <joro@...tes.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Project_Global_Chrome_Upstream_Group@...iatek.com,
	Ning li <ning.li@...iatek.com>,
	linux-mediatek@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	devicetree@...r.kernel.org,
	iommu@...ts.linux.dev
Subject: Re: [RFC PATCH 6/8] iommu/arm-smmu-v3: mediatek: Add wrapper handle for IRQ

On Tue, 24 Jun 2025 12:22:00 +0100,
Will Deacon <will@...nel.org> wrote:
> 
> [+Marc for irqchip question at the end]
> 
> On Mon, Jun 16, 2025 at 10:56:12AM +0800, Xueqi Zhang wrote:
> > Mediatek SMMU interrupt is low level active rather than the standard
> > edge.Process Mediatek SMMU wrapper interrupt and dump detailed
> > information when a translation fault occurs.
> > 
> > Signed-off-by: Xueqi Zhang <xueqi.zhang@...iatek.com>
> > ---
> >  .../arm/arm-smmu-v3/arm-smmu-v3-mediatek.c    | 349 +++++++++++++++++-
> >  1 file changed, 347 insertions(+), 2 deletions(-)
> 
> I think this probably needs splitting in two parts so that the low-level
> IRQ handling is separate from the fault diagnostic reporting.
> 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-mediatek.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-mediatek.c
> > index 48290366e596..448166c1ca64 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-mediatek.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-mediatek.c
> > @@ -14,7 +14,122 @@
> >  
> >  #include "arm-smmu-v3.h"
> >  
> > +#include <linux/soc/mediatek/mtk_sip_svc.h>
> > +#include <linux/arm-smccc.h>
> > +
> > +#define SMMUWP_GLB_CTL0			(0x0)
> > +#define CTL0_STD_AXI_MODE_DIS		BIT(0)
> > +#define CTL0_MON_DIS			BIT(1)
> > +#define CTL0_DCM_EN			BIT(2)
> > +#define CTL0_WRAPPER_CK_AOEN		BIT(3)
> > +#define CTL0_AUTO_AXDOMAIN_EN		BIT(4)
> > +#define CTL0_IRQ_BUSY_EN		BIT(5)
> > +#define CTL0_ABT_CNT_CLR		BIT(6)
> > +#define CTL0_LEGACY_AXCACHE		BIT(7)
> > +#define CTL0_COMMIT_DIS			BIT(8)
> > +#define CTL0_AUTO_SLP_DIS		BIT(9)
> > +#define CTL0_STTSL_DIS			BIT(10)
> > +#define CTL0_CFG_TAB_DCM_EN		BIT(11)
> > +#define CTL0_CPU_PARTID_DIS		BIT(14)
> > +/* New bits of SMMU wrapper extension */
> > +#define CTL0_TCU2SLC_DCM_EN		BIT(18)
> > +#define CTL0_APB_DCM_EN			BIT(19)
> > +#define CTL0_DVM_DCM_EN			BIT(20)
> > +#define CTL0_CPU_TBU_PARTID_DIS		BIT(21)
> > +
> > +#define SMMUWP_IRQ_STA			(0x80)
> > +#define STA_TCU_GLB_INTR		BIT(0)
> > +#define STA_TCU_CMD_SYNC_INTR		BIT(1)
> > +#define STA_TCU_EVTQ_INTR		BIT(2)
> > +#define STA_TCU_PRI_INTR		BIT(3)
> > +#define STA_TCU_PMU_INTR		BIT(4)
> > +#define STA_TCU_RAS_CRI			BIT(5)
> > +#define STA_TCU_RAS_ERI			BIT(6)
> > +#define STA_TCU_RAS_FHI			BIT(7)
> > +
> > +#define SMMUWP_IRQ_ACK			(0x84)
> > +
> > +#define SMMUWP_IRQ_ACK_CNT		(0x88)
> > +#define IRQ_ACK_CNT_MSK			GENMASK(7, 0)
> > +
> > +/* SMMU non-secure interrupt pending count register, count 20 */
> > +#define SMMUWP_IRQ_CNTx(cnt)		(0x100 + 0x4 * (cnt))
> > +
> > +#define SMMU_TCU_CTL1_AXSLC		(0x204)
> > +#define AXSLC_BIT_FIELD			GENMASK(8, 4)
> > +#define AXSLC_CACHE			BIT(5)
> > +#define AXSLC_ALLOCATE			BIT(6)
> > +#define AXSLC_SPECULATIVE		BIT(7)
> > +#define AXSLC_SET			(AXSLC_CACHE | AXSLC_ALLOCATE | AXSLC_SPECULATIVE)
> > +#define SLC_SB_ONLY_EN			BIT(1)
> > +
> > +/* SMMU TBUx read translation fault monitor0 */
> > +#define SMMUWP_TBUx_RTFM0(tbu)		(0x380 + 0x100 * (tbu))
> > +#define RTFM0_FAULT_AXI_ID		GENMASK_ULL(19, 0)
> > +#define RTFM0_FAULT_DET			BIT(31)
> > +
> > +/* SMMU TBUx read translation fault monitor1 */
> > +#define SMMUWP_TBUx_RTFM1(tbu)		(0x384 + 0x100 * (tbu))
> > +#define RTFM1_FAULT_VA_35_32		GENMASK_ULL(3, 0)
> > +#define RTFM1_FAULT_VA_31_12		GENMASK_ULL(31, 12)
> > +
> > +/* SMMU TBUx read translation fault monitor2 */
> > +#define SMMUWP_TBUx_RTFM2(tbu)		(0x388 + 0x100 * (tbu))
> > +#define RTFM2_FAULT_SID			GENMASK_ULL(7, 0)
> > +#define RTFM2_FAULT_SSID		GENMASK_ULL(15, 8)
> > +#define RTFM2_FAULT_SSIDV		BIT(16)
> > +#define RTFM2_FAULT_SECSID		BIT(17)
> > +
> > +/* SMMU TBUx write translation fault monitor0 */
> > +#define SMMUWP_TBUx_WTFM0(tbu)		(0x390 + 0x100 * (tbu))
> > +#define WTFM0_FAULT_AXI_ID		GENMASK_ULL(19, 0)
> > +#define WTFM0_FAULT_DET			BIT(31)
> > +
> > +/* SMMU TBUx write translation fault monitor1 */
> > +#define SMMUWP_TBUx_WTFM1(tbu)		(0x394 + 0x100 * (tbu))
> > +#define WTFM1_FAULT_VA_35_32		GENMASK_ULL(3, 0)
> > +#define WTFM1_FAULT_VA_31_12		GENMASK_ULL(31, 12)
> > +
> > +/* SMMU TBUx write translation fault monitor2 */
> > +#define SMMUWP_TBUx_WTFM2(tbu)		(0x398 + 0x100 * (tbu))
> > +#define WTFM2_FAULT_SID			GENMASK_ULL(7, 0)
> > +#define WTFM2_FAULT_SSID		GENMASK_ULL(15, 8)
> > +#define WTFM2_FAULT_SSIDV		BIT(16)
> > +#define WTFM2_FAULT_SECSID		BIT(17)
> > +
> > +/* SMMU TBU Manual OG Control High Register0 */
> > +#define SMMUWP_TBU0_MOGH0		(0x3b4)
> > +#define MOGH_EN				BIT(29)
> > +#define MOGH_RW				BIT(28)
> > +
> > +/* SMMU translation fault TBUx */
> > +#define SMMUWP_TF_TBU_MSK		GENMASK(26, 24)
> > +#define SMMUWP_TF_TBU(tbu)		FIELD_PREP(SMMUWP_TF_TBU_MSK, tbu)
> > +
> > +#define SMMU_FAULT_RS_INTERVAL		DEFAULT_RATELIMIT_INTERVAL
> > +#define SMMU_FAULT_RS_BURST		(1)
> > +
> > +#define STRSEC(sec)			((sec) ? "SECURE" : "NORMAL")
> > +
> > +#define WP_OFFSET_MT8196		0x1e0000
> > +
> >  #define MTK_SMMU_COMP_STR_LEN		64
> > +
> > +#define MTK_SMMU_FAULT_IOVA(low, high) ((low) | (((u64)(high) & 0xf) << 32))
> > +
> > +#define SMMU_SUCCESS			(0)
> > +#define SMMU_ID_ERR			(1)
> > +#define SMMU_CMD_ERR			(2)
> > +#define SMMU_PARA_INVALID		(3)
> > +#define SMMU_NEED			(4)
> > +#define SMMU_NONEED			(5)
> > +
> > +/* plat flags: */
> > +#define SMMU_SKIP_PM_CLK		BIT(0)
> > +#define SMMU_CLK_AO_EN			BIT(1)
> > +#define SMMU_AXSLC_EN			BIT(2)
> > +#define SMMU_DIS_CPU_PARTID		BIT(3)
> > +#define SMMU_DIS_CPU_TBU_PARTID		BIT(4)
> >  #define SMMU_REQUIRE_PARENT		BIT(5)
> >  #define MTK_SMMU_HAS_FLAG(pdata, _x)    (!!(((pdata)->flags) & (_x)))
> >  
> > @@ -25,22 +140,30 @@ enum mtk_smmu_type {
> >  };
> >  
> >  struct mtk_smmu_v3_plat {
> > +	u32			wp_offset;
> > +	unsigned int		tbu_cnt;
> >  	enum mtk_smmu_type	smmu_type;
> >  	u32			flags;
> >  };
> >  
> >  struct mtk_smmu_v3 {
> >  	struct arm_smmu_device	smmu;
> > +	void __iomem			*wp_base;
> >  	const struct mtk_smmu_v3_plat *plat_data;
> >  };
> >  
> >  static const struct mtk_smmu_v3_plat mt8196_data_mm = {
> > +	.wp_offset		= WP_OFFSET_MT8196,
> > +	.tbu_cnt		= 3,
> >  	.smmu_type		= MTK_SMMU_MM,
> > +	.flags			= SMMU_AXSLC_EN,
> >  };
> >  
> >  static const struct mtk_smmu_v3_plat mt8196_data_apu = {
> > +	.wp_offset		= WP_OFFSET_MT8196,
> > +	.tbu_cnt		= 3,
> >  	.smmu_type		= MTK_SMMU_APU,
> > -	.flags			= SMMU_REQUIRE_PARENT,
> > +	.flags			= SMMU_AXSLC_EN | SMMU_REQUIRE_PARENT,
> >  };
> >  
> >  struct mtk_smmu_v3_of_device_data {
> > @@ -70,17 +193,228 @@ static const struct mtk_smmu_v3_plat *mtk_smmu_v3_get_plat_data(const struct dev
> >  	return NULL;
> >  }
> >  
> > +static inline void smmu_write_field(void __iomem *base,
> > +				    unsigned int reg,
> > +				    unsigned int mask,
> > +				    unsigned int val)
> > +{
> > +	unsigned int regval;
> > +
> > +	regval = readl_relaxed(base + reg);
> > +	regval = (regval & (~mask)) | val;
> > +	writel_relaxed(regval, base + reg);
> > +}
> > +
> > +static void smmu_init_wpcfg(struct arm_smmu_device *smmu)
> > +{
> > +	struct mtk_smmu_v3 *mtk_smmu_v3 = to_mtk_smmu_v3(smmu);
> > +	void __iomem *wp_base = mtk_smmu_v3->wp_base;
> > +
> > +	/* DCM basic setting */
> > +	smmu_write_field(wp_base, SMMUWP_GLB_CTL0, CTL0_DCM_EN, CTL0_DCM_EN);
> > +	smmu_write_field(wp_base, SMMUWP_GLB_CTL0, CTL0_CFG_TAB_DCM_EN,
> > +			 CTL0_CFG_TAB_DCM_EN);
> > +
> > +	smmu_write_field(wp_base, SMMUWP_GLB_CTL0,
> > +			 CTL0_TCU2SLC_DCM_EN | CTL0_APB_DCM_EN |
> > +			 CTL0_DVM_DCM_EN,
> > +			 CTL0_TCU2SLC_DCM_EN | CTL0_APB_DCM_EN |
> > +			 CTL0_DVM_DCM_EN);
> > +
> > +	if (MTK_SMMU_HAS_FLAG(mtk_smmu_v3->plat_data, SMMU_DIS_CPU_PARTID))
> > +		smmu_write_field(wp_base, SMMUWP_GLB_CTL0, CTL0_CPU_PARTID_DIS,
> > +				 CTL0_CPU_PARTID_DIS);
> > +	if (MTK_SMMU_HAS_FLAG(mtk_smmu_v3->plat_data, SMMU_DIS_CPU_TBU_PARTID))
> > +		smmu_write_field(wp_base, SMMUWP_GLB_CTL0,
> > +				 CTL0_CPU_TBU_PARTID_DIS, CTL0_CPU_TBU_PARTID_DIS);
> > +
> > +	/* Used for MM_SMMMU read command overtaking */
> > +	if (mtk_smmu_v3->plat_data->smmu_type == MTK_SMMU_MM)
> > +		smmu_write_field(wp_base, SMMUWP_GLB_CTL0, CTL0_STD_AXI_MODE_DIS,
> > +				 CTL0_STD_AXI_MODE_DIS);
> > +
> > +	/* Set AXSLC */
> > +	if (MTK_SMMU_HAS_FLAG(mtk_smmu_v3->plat_data, SMMU_AXSLC_EN)) {
> > +		smmu_write_field(wp_base, SMMUWP_GLB_CTL0,
> > +				 CTL0_STD_AXI_MODE_DIS, CTL0_STD_AXI_MODE_DIS);
> > +		smmu_write_field(wp_base, SMMU_TCU_CTL1_AXSLC, AXSLC_BIT_FIELD,
> > +				 AXSLC_SET);
> > +		smmu_write_field(wp_base, SMMU_TCU_CTL1_AXSLC, SLC_SB_ONLY_EN,
> > +				 SLC_SB_ONLY_EN);
> > +	}
> > +}
> > +
> > +/* Consume SMMU wrapper interrupt bit */
> > +static unsigned int
> > +smmuwp_consume_intr(void __iomem *wp_base, unsigned int irq_bit)
> > +{
> > +	unsigned int pend_cnt;
> > +
> > +	pend_cnt = readl_relaxed(wp_base + SMMUWP_IRQ_CNTx(__ffs(irq_bit)));
> > +	smmu_write_field(wp_base, SMMUWP_IRQ_ACK_CNT, IRQ_ACK_CNT_MSK, pend_cnt);
> > +	writel_relaxed(irq_bit, wp_base + SMMUWP_IRQ_ACK);
> > +
> > +	return pend_cnt;
> > +}
> > +
> > +/* clear translation fault mark */
> > +static void smmuwp_clear_tf(void __iomem *wp_base)
> > +{
> > +	smmu_write_field(wp_base, SMMUWP_GLB_CTL0, CTL0_ABT_CNT_CLR, CTL0_ABT_CNT_CLR);
> > +	smmu_write_field(wp_base, SMMUWP_GLB_CTL0, CTL0_ABT_CNT_CLR, 0);
> > +}
> > +
> > +static u32 smmuwp_fault_id(u32 axi_id, u32 tbu_id)
> > +{
> > +	u32 fault_id = (axi_id & ~SMMUWP_TF_TBU_MSK) | (SMMUWP_TF_TBU(tbu_id));
> > +
> > +	return fault_id;
> > +}
> > +
> > +/* Process TBU translation fault Monitor */
> > +static bool smmuwp_process_tf(struct arm_smmu_device *smmu)
> > +{
> > +	struct mtk_smmu_v3 *mtk_smmu_v3 = to_mtk_smmu_v3(smmu);
> > +	void __iomem *wp_base = mtk_smmu_v3->wp_base;
> > +	unsigned int sid, ssid, secsidv, ssidv;
> > +	u32 i, regval, va35_32, axiid, fault_id;
> > +	u64 fault_iova;
> > +	bool tf_det = false;
> > +
> > +	for (i = 0; i < mtk_smmu_v3->plat_data->tbu_cnt; i++) {
> > +		regval = readl_relaxed(wp_base + SMMUWP_TBUx_RTFM0(i));
> > +		if (!(regval & RTFM0_FAULT_DET))
> > +			goto write;
> > +
> > +		tf_det = true;
> > +		axiid = FIELD_GET(RTFM0_FAULT_AXI_ID, regval);
> > +		fault_id = smmuwp_fault_id(axiid, i);
> > +
> > +		regval = readl_relaxed(wp_base + SMMUWP_TBUx_RTFM1(i));
> > +		va35_32 = FIELD_GET(RTFM1_FAULT_VA_35_32, regval);
> > +		fault_iova = MTK_SMMU_FAULT_IOVA(regval & RTFM1_FAULT_VA_31_12, va35_32);
> > +
> > +		regval = readl_relaxed(wp_base + SMMUWP_TBUx_RTFM2(i));
> > +		sid = FIELD_GET(RTFM2_FAULT_SID, regval);
> > +		ssid = FIELD_GET(RTFM2_FAULT_SSID, regval);
> 
> We already print a bunch of this stuff in arm_smmu_dump_event() now that
> Pranjal has improved the logic. I don't see the value in printing the
> same information twice, so we should trim down the extra diagnostics here
> so that (a) the information is distinct from that provided by the core
> driuver code and (b) it's relevant to Linux (e.g. there's no need to
> print information about secure transactions).
> 
> The core driver code should also print some delimiters around the
> implementation-defined data as well. Out of curiosity, is the TBU
> monitor part of Arm's IP or is this additional hardware from MTK?
> 
> > +		ssidv = FIELD_GET(RTFM2_FAULT_SSIDV, regval);
> > +		secsidv = FIELD_GET(RTFM2_FAULT_SECSID, regval);
> > +		dev_err_ratelimited(smmu->dev, "TF read in %s world, TBU_id-%d-fault_id:0x%x(0x%x)\n",
> > +				    STRSEC(secsidv), i, fault_id, axiid);
> > +		dev_err_ratelimited(smmu->dev,
> > +				    "iova:0x%llx, sid:%d, ssid:%d, ssidv:%d, secsidv:%d\n",
> > +				    fault_iova, sid, ssid, ssidv, secsidv);
> > +
> > +write:
> > +		regval = readl_relaxed(wp_base + SMMUWP_TBUx_WTFM0(i));
> > +		if (!(regval & WTFM0_FAULT_DET))
> > +			continue;
> > +
> > +		tf_det = true;
> > +		axiid = FIELD_GET(WTFM0_FAULT_AXI_ID, regval);
> > +		fault_id = smmuwp_fault_id(axiid, i);
> > +
> > +		regval = readl_relaxed(wp_base + SMMUWP_TBUx_WTFM1(i));
> > +		va35_32 = FIELD_GET(WTFM1_FAULT_VA_35_32, regval);
> > +		fault_iova = MTK_SMMU_FAULT_IOVA(regval & RTFM1_FAULT_VA_31_12, va35_32);
> > +
> > +		regval = readl_relaxed(wp_base + SMMUWP_TBUx_WTFM2(i));
> > +		sid = FIELD_GET(WTFM2_FAULT_SID, regval);
> > +		ssid = FIELD_GET(WTFM2_FAULT_SSID, regval);
> > +		ssidv = FIELD_GET(WTFM2_FAULT_SSIDV, regval);
> > +		secsidv = FIELD_GET(WTFM2_FAULT_SECSID, regval);
> > +		dev_err_ratelimited(smmu->dev, "TF write in %s world, TBU_id-%d-fault_id:0x%x(0x%x)\n",
> > +				    STRSEC(secsidv), i, fault_id, axiid);
> > +		dev_err_ratelimited(smmu->dev,
> > +				    "iova:0x%llx, sid:%d, ssid:%d, ssidv:%d, secsidv:%d\n",
> > +				    fault_iova, sid, ssid, ssidv, secsidv);
> 
> nit: but I don't think we should use the _ratelimited() prints here as it
> could end up with random lines being dropped.
> 
> > +/* Process SMMU wrapper interrupt */
> > +static int mtk_smmu_v3_smmuwp_irq_handler(int irq, struct arm_smmu_device *smmu)
> > +{
> > +	struct mtk_smmu_v3 *mtk_smmuv3 = to_mtk_smmu_v3(smmu);
> > +	void __iomem *wp_base = mtk_smmuv3->wp_base;
> > +	unsigned int irq_sta, pend_cnt;
> > +
> > +	irq_sta = readl_relaxed(wp_base + SMMUWP_IRQ_STA);
> > +	if (irq_sta == 0)
> > +		return 0;
> > +
> > +	if (irq_sta & STA_TCU_GLB_INTR) {
> > +		pend_cnt = smmuwp_consume_intr(wp_base, STA_TCU_GLB_INTR);
> > +		dev_dbg(smmu->dev,
> > +			"IRQ_STA:0x%x, Non-secure TCU global interrupt detected pending_cnt: %d\n",
> > +			irq_sta, pend_cnt);
> > +	}
> > +
> > +	if (irq_sta & STA_TCU_CMD_SYNC_INTR) {
> > +		pend_cnt = smmuwp_consume_intr(wp_base, STA_TCU_CMD_SYNC_INTR);
> > +		dev_dbg(smmu->dev,
> > +			"IRQ_STA:0x%x, Non-secure TCU CMD_SYNC interrupt detected pending_cnt: %d\n",
> > +			irq_sta, pend_cnt);
> > +	}
> > +
> > +	if (irq_sta & STA_TCU_EVTQ_INTR) {
> > +		pend_cnt = smmuwp_consume_intr(wp_base, STA_TCU_EVTQ_INTR);
> > +		dev_dbg(smmu->dev,
> > +			"IRQ_STA:0x%x, Non-secure TCU EVTQ interrupt detected pending_cnt: %d\n",
> > +			irq_sta, pend_cnt);
> > +	}
> > +
> > +	if (irq_sta & STA_TCU_PRI_INTR) {
> > +		pend_cnt = smmuwp_consume_intr(wp_base, STA_TCU_PRI_INTR);
> > +		dev_dbg(smmu->dev, "IRQ_STA:0x%x, TCU PRI interrupt detected pending_cnt: %d\n",
> > +			irq_sta, pend_cnt);
> > +	}
> > +
> > +	if (irq_sta & STA_TCU_PMU_INTR) {
> > +		pend_cnt = smmuwp_consume_intr(wp_base, STA_TCU_PMU_INTR);
> > +		dev_dbg(smmu->dev, "IRQ_STA:0x%x, TCU PMU interrupt detected pending_cnt: %d\n",
> > +			irq_sta, pend_cnt);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Hrm. I wonder whether this would be better off treated as a chained irqchip
> rather than hiding the logic inside the SMMU driver? It effectively looks
> like a demuxer to me.

Yeah, that's effectively what this is.

However, it isn't clear this has the basic mask/unmask logic that we'd
expect for an irqchip, and I doubt you'd want the irqchip to pry into
the SMMU code for that to mask/unmask the individual interrupt
sources.

The other thing is that I don't see any actual handling here.
smmuwp_consume_intr() ACKs the interrupt, and that's it. I'm not even
sure why this code exists at all (but I haven't read the series, TBH).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists