2023-11-18 04:29:25

by Georgi Djakov

[permalink] [raw]
Subject: [PATCH v2 0/6] Add support for Translation Buffer Units

The TCUs (Translation Control Units) and TBUs (Translation Buffer
Units) are key components of the MMU-500. Multiple TBUs are connected
to a single TCU over an interconnect. Each TBU contains a TLB that
caches page tables. The MMU-500 implements a TBU for each connected
master, and the TBU is designed, so that it is local to the master.

The Qualcomm SDM845 platform has an implementation of the SMMU-500,
that has multiple TBUs. A DT schema is added to describe the resources
for each TBU (register space, power-domains, interconnects and clocks).

The TBU driver will manage the resources and allow the system to
operate the TBUs during a context fault to obtain details by doing
s1 inv, software + hardware page table walks etc. This is implemented
with ATOS/eCATs as the ATS feature is not supported. Being able to
query the TBUs is useful for debugging various hardware/software
issues on these platforms.

v2:
- Improve DT binding description, add full example. (Konrad)
- Drop Qcom specific stuff from the generic binding. (Rob)
- Unconditionally try to populate subnodes. (Konrad)
- Improve TBU driver commit text, remove memory barriers. (Bjorn)
- Move TBU stuff into separate file. Make the driver builtin.
- TODO: Evaluate whether to keep TBU support as a separate driver
or just instantiate things from qcom_smmu_impl_init()

v1: https://lore.kernel.org/r/[email protected]

Georgi Djakov (6):
dt-bindings: iommu: Add Translation Buffer Unit bindings
iommu/arm-smmu-qcom: Add support for TBUs
iommu/arm-smmu-qcom: Add Qualcomm TBU driver
iommu/arm-smmu: Allow using a threaded handler for context interrupts
iommu/arm-smmu-qcom: Use a custom context fault handler for sdm845
arm64: dts: qcom: sdm845: Add DT nodes for the TBUs

.../devicetree/bindings/iommu/arm,smmu.yaml | 25 +
.../bindings/iommu/qcom,qsmmuv500-tbu.yaml | 89 ++++
arch/arm64/boot/dts/qcom/sdm845.dtsi | 82 +++
drivers/iommu/Kconfig | 8 +
drivers/iommu/arm/arm-smmu/Makefile | 1 +
.../iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c | 504 ++++++++++++++++++
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 12 +
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 6 +-
drivers/iommu/arm/arm-smmu/arm-smmu.c | 12 +-
drivers/iommu/arm/arm-smmu/arm-smmu.h | 3 +
10 files changed, 739 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
create mode 100644 drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c


2023-11-18 04:29:34

by Georgi Djakov

[permalink] [raw]
Subject: [PATCH v2 5/6] iommu/arm-smmu-qcom: Use a custom context fault handler for sdm845

The sdm845 platform now supports TBUs, so let's get additional debug
info from the TBUs when a context fault occurs. Implement a custom
context fault handler that does both software + hardware page table
walks and TLB Invalidate All.

Signed-off-by: Georgi Djakov <[email protected]>
---
.../iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c | 138 ++++++++++++++++++
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 4 +
2 files changed, 142 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c
index 2ab60f0a4ec2..a4c7db53067c 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c
@@ -52,6 +52,11 @@ static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
return container_of(smmu, struct qcom_smmu, smmu);
}

+static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
+{
+ return container_of(dom, struct arm_smmu_domain, domain);
+}
+
static struct qsmmuv500_tbu *qsmmuv500_find_tbu(struct qcom_smmu *qsmmu, u32 sid)
{
struct qsmmuv500_tbu *tbu = NULL;
@@ -301,6 +306,139 @@ static phys_addr_t qsmmuv500_iova_to_phys(struct arm_smmu_domain *smmu_domain,
return phys;
}

+static phys_addr_t qcom_smmu_iova_to_phys_hard(struct iommu_domain *domain, dma_addr_t iova)
+{
+ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ struct arm_smmu_device *smmu = smmu_domain->smmu;
+ int idx = smmu_domain->cfg.cbndx;
+ u32 frsynra;
+ u16 sid;
+
+ frsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
+ sid = FIELD_GET(ARM_SMMU_CBFRSYNRA_SID, frsynra);
+
+ return qsmmuv500_iova_to_phys(smmu_domain, iova, sid);
+}
+
+static phys_addr_t qcom_smmu_verify_fault(struct iommu_domain *domain, dma_addr_t iova, u32 fsr)
+{
+ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ struct io_pgtable *iop = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
+ struct arm_smmu_device *smmu = smmu_domain->smmu;
+ phys_addr_t phys_post_tlbiall;
+ phys_addr_t phys;
+
+ phys = qcom_smmu_iova_to_phys_hard(domain, iova);
+ io_pgtable_tlb_flush_all(iop);
+ phys_post_tlbiall = qcom_smmu_iova_to_phys_hard(domain, iova);
+
+ if (phys != phys_post_tlbiall) {
+ dev_err(smmu->dev,
+ "ATOS results differed across TLBIALL... (before: %pa after: %pa)\n",
+ &phys, &phys_post_tlbiall);
+ }
+
+ return (phys == 0 ? phys_post_tlbiall : phys);
+}
+
+irqreturn_t qcom_smmu_context_fault(int irq, void *dev)
+{
+ struct iommu_domain *domain = dev;
+ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
+ struct arm_smmu_device *smmu = smmu_domain->smmu;
+ u32 fsr, fsynr, cbfrsynra, resume = 0;
+ int idx = smmu_domain->cfg.cbndx;
+ phys_addr_t phys_soft;
+ unsigned long iova;
+ int ret, tmp;
+
+ static DEFINE_RATELIMIT_STATE(_rs,
+ DEFAULT_RATELIMIT_INTERVAL,
+ DEFAULT_RATELIMIT_BURST);
+
+ fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR);
+ if (!(fsr & ARM_SMMU_FSR_FAULT))
+ return IRQ_NONE;
+
+ fsynr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSYNR0);
+ iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
+ cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
+
+ phys_soft = ops->iova_to_phys(ops, iova);
+
+ tmp = report_iommu_fault(domain, NULL, iova,
+ fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);
+ if (!tmp || tmp == -EBUSY) {
+ dev_dbg(smmu->dev,
+ "Context fault handled by client: iova=0x%08lx, fsr=0x%x, fsynr=0x%x, cb=%d\n",
+ iova, fsr, fsynr, idx);
+ dev_dbg(smmu->dev, "soft iova-to-phys=%pa\n", &phys_soft);
+ ret = IRQ_HANDLED;
+ resume = ARM_SMMU_RESUME_TERMINATE;
+ } else {
+ phys_addr_t phys_atos = qcom_smmu_verify_fault(domain, iova, fsr);
+
+ if (__ratelimit(&_rs)) {
+ dev_err(smmu->dev,
+ "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
+ fsr, iova, fsynr, cbfrsynra, idx);
+ dev_err(smmu->dev,
+ "FSR = %08x [%s%s%s%s%s%s%s%s%s], SID=0x%x\n",
+ fsr,
+ (fsr & 0x02) ? "TF " : "",
+ (fsr & 0x04) ? "AFF " : "",
+ (fsr & 0x08) ? "PF " : "",
+ (fsr & 0x10) ? "EF " : "",
+ (fsr & 0x20) ? "TLBMCF " : "",
+ (fsr & 0x40) ? "TLBLKF " : "",
+ (fsr & 0x80) ? "MHF " : "",
+ (fsr & 0x40000000) ? "SS " : "",
+ (fsr & 0x80000000) ? "MULTI " : "",
+ cbfrsynra);
+
+ dev_err(smmu->dev,
+ "soft iova-to-phys=%pa\n", &phys_soft);
+ if (!phys_soft)
+ dev_err(smmu->dev,
+ "SOFTWARE TABLE WALK FAILED! Looks like %s accessed an unmapped address!\n",
+ dev_name(smmu->dev));
+ if (phys_atos)
+ dev_err(smmu->dev, "hard iova-to-phys (ATOS)=%pa\n",
+ &phys_atos);
+ else
+ dev_err(smmu->dev, "hard iova-to-phys (ATOS) failed\n");
+ }
+ ret = IRQ_NONE;
+ resume = ARM_SMMU_RESUME_TERMINATE;
+ }
+
+ /*
+ * If the client returns -EBUSY, do not clear FSR and do not RESUME
+ * if stalled. This is required to keep the IOMMU client stalled on
+ * the outstanding fault. This gives the client a chance to take any
+ * debug action and then terminate the stalled transaction.
+ * So, the sequence in case of stall on fault should be:
+ * 1) Do not clear FSR or write to RESUME here
+ * 2) Client takes any debug action
+ * 3) Client terminates the stalled transaction and resumes the IOMMU
+ * 4) Client clears FSR. The FSR should only be cleared after 3) and
+ * not before so that the fault remains outstanding. This ensures
+ * SCTLR.HUPCF has the desired effect if subsequent transactions also
+ * need to be terminated.
+ */
+ if (tmp != -EBUSY) {
+ /* Clear the faulting FSR */
+ arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_FSR, fsr);
+
+ /* Retry or terminate any stalled transactions */
+ if (fsr & ARM_SMMU_FSR_SS)
+ arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_RESUME, resume);
+ }
+
+ return ret;
+}
+
static int qsmmuv500_tbu_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 1622abace484..1af3d4a7fe1d 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -421,6 +421,10 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
.reset = qcom_sdm845_smmu500_reset,
.write_s2cr = qcom_smmu_write_s2cr,
.tlb_sync = qcom_smmu_tlb_sync,
+#ifdef CONFIG_ARM_SMMU_QCOM_TBU
+ .context_fault = qcom_smmu_context_fault,
+ .context_fault_needs_threaded_irq = true,
+#endif
};

static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {

2023-11-18 04:29:37

by Georgi Djakov

[permalink] [raw]
Subject: [PATCH v2 1/6] dt-bindings: iommu: Add Translation Buffer Unit bindings

The "apps_smmu" on the Qualcomm sdm845 platform is an implementation
of the SMMU-500, that consists of a single TCU (Translation Control
Unit) and multiple TBUs (Translation Buffer Units). The TCU is already
being described in the generic SMMU DT schema. Add also bindings for
the TBUs to describe their properties and resources that needs to be
managed in order to operate them.

In this DT schema, the TBUs are modelled as child devices of the TCU
and each of them is described with it's register space, clocks, power
domains, interconnects etc.

Signed-off-by: Georgi Djakov <[email protected]>
---
.../devicetree/bindings/iommu/arm,smmu.yaml | 25 ++++++
.../bindings/iommu/qcom,qsmmuv500-tbu.yaml | 89 +++++++++++++++++++
2 files changed, 114 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
index aa9e1c0895a5..f7f89be5f7a3 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
@@ -231,6 +231,18 @@ properties:
enabled for any given device.
$ref: /schemas/types.yaml#/definitions/phandle

+ '#address-cells':
+ enum: [ 1, 2 ]
+
+ '#size-cells':
+ enum: [ 1, 2 ]
+
+ ranges: true
+
+patternProperties:
+ "^tbu@[0-9a-f]*":
+ type: object
+
required:
- compatible
- reg
@@ -453,6 +465,19 @@ allOf:
- description: Voter clock required for HLOS SMMU access
- description: Interface clock required for register access

+ - if:
+ properties:
+ compatible:
+ contains:
+ const: qcom,smmu-500
+ then:
+ patternProperties:
+ "^tbu@[0-9a-f]*":
+ $ref: qcom,qsmmuv500-tbu.yaml
+ unevaluatedProperties: false
+ properties:
+ ranges: true
+
# Disallow clocks for all other platforms with specific compatibles
- if:
properties:
diff --git a/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
new file mode 100644
index 000000000000..4dc9d0ca33c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/qcom,qsmmuv500-tbu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm TBU (Translation Buffer Unit)
+
+maintainers:
+ - Georgi Djakov <[email protected]>
+
+description:
+ The Qualcomm SMMU500 implementation consists of TCU and TBU. The TBU contains
+ a Translation Lookaside Buffer (TLB) that caches page tables. TBUs provides
+ debug features to trace and trigger debug transactions. There are multiple TBU
+ instances distributes with each client core.
+
+properties:
+ $nodename:
+ pattern: "^tbu@[0-9a-f]+$"
+
+ compatible:
+ const: qcom,qsmmuv500-tbu
+
+ reg:
+ items:
+ - description: Address and size of the TBU's register space.
+
+ reg-names:
+ items:
+ - const: base
+
+ clocks:
+ maxItems: 1
+
+ interconnects:
+ maxItems: 1
+
+ power-domains:
+ maxItems: 1
+
+ qcom,stream-id-range:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: Stream ID range (address and size) that is assigned by the TBU
+ items:
+ minItems: 2
+ maxItems: 2
+
+required:
+ - compatible
+ - reg
+ - interconnects
+ - qcom,stream-id-range
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+ #include <dt-bindings/interconnect/qcom,icc.h>
+ #include <dt-bindings/interconnect/qcom,sdm845.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/power/qcom-rpmpd.h>
+
+ apps_smmu: iommu@15000000 {
+ compatible = "qcom,sdm845-smmu-500", "arm,mmu-500";
+ reg = <0x15000000 0x80000>;
+ ranges = <0 0 0 0 0xffffffff>;
+ #iommu-cells = <2>;
+ #global-interrupts = <1>;
+ interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 343 IRQ_TYPE_LEVEL_HIGH>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ anoc_1_pcie_tbu: tbu@150e1000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150e1000 0x0 0x1000>;
+ reg-names = "base";
+ clocks = <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>;
+ interconnects = <&system_noc MASTER_GNOC_SNOC QCOM_ICC_TAG_ACTIVE_ONLY
+ &config_noc SLAVE_IMEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_AGGRE_NOC_MMU_PCIE_TBU_GDSC>;
+ qcom,stream-id-range = <0x1c00 0x400>;
+ };
+ };
+
+...

2023-11-18 04:29:39

by Georgi Djakov

[permalink] [raw]
Subject: [PATCH v2 6/6] arm64: dts: qcom: sdm845: Add DT nodes for the TBUs

Add the device-tree nodes for the TBUs (translation buffer units) that
are present on the sdm845 platforms. The TBUs can be used debug the
kernel and provide additional information when a context faults occur.

Describe the all registers, clocks, interconnects and power-domain
resources that are needed for each of the TBUs.

Signed-off-by: Georgi Djakov <[email protected]>
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 82 ++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index bf5e6eb9d313..5c7174415ae9 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -15,6 +15,7 @@
#include <dt-bindings/dma/qcom-gpi.h>
#include <dt-bindings/firmware/qcom,scm.h>
#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/interconnect/qcom,icc.h>
#include <dt-bindings/interconnect/qcom,osm-l3.h>
#include <dt-bindings/interconnect/qcom,sdm845.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -5000,6 +5001,7 @@ pil-reloc@94c {
apps_smmu: iommu@15000000 {
compatible = "qcom,sdm845-smmu-500", "arm,mmu-500";
reg = <0 0x15000000 0 0x80000>;
+ ranges;
#iommu-cells = <2>;
#global-interrupts = <1>;
interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>,
@@ -5067,6 +5069,86 @@ apps_smmu: iommu@15000000 {
<GIC_SPI 341 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 342 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 343 IRQ_TYPE_LEVEL_HIGH>;
+
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ anoc_1_tbu: tbu@150c5000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150c5000 0x0 0x1000>;
+ reg-names = "base";
+ interconnects = <&system_noc MASTER_GNOC_SNOC QCOM_ICC_TAG_ACTIVE_ONLY
+ &config_noc SLAVE_IMEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_AGGRE_NOC_MMU_TBU1_GDSC>;
+ qcom,stream-id-range = <0x0 0x400>;
+ };
+
+ anoc_2_tbu: tbu@150c9000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150c9000 0x0 0x1000>;
+ reg-names = "base";
+ interconnects = <&system_noc MASTER_GNOC_SNOC QCOM_ICC_TAG_ACTIVE_ONLY
+ &config_noc SLAVE_IMEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_AGGRE_NOC_MMU_TBU2_GDSC>;
+ qcom,stream-id-range = <0x400 0x400>;
+ };
+
+ mnoc_hf_0_tbu: tbu@150cd000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150cd000 0x0 0x1000>;
+ reg-names = "base";
+ interconnects = <&mmss_noc MASTER_MDP0 QCOM_ICC_TAG_ACTIVE_ONLY
+ &mmss_noc SLAVE_MNOC_HF_MEM_NOC QCOM_ICC_TAG_ACTIVE_ONLY>;
+ qcom,stream-id-range = <0x800 0x400>;
+ };
+
+ mnoc_hf_1_tbu: tbu@150d1000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150d1000 0x0 0x1000>;
+ reg-names = "base";
+ interconnects = <&mmss_noc MASTER_MDP0 QCOM_ICC_TAG_ACTIVE_ONLY
+ &mmss_noc SLAVE_MNOC_HF_MEM_NOC QCOM_ICC_TAG_ACTIVE_ONLY>;
+ qcom,stream-id-range = <0xc00 0x400>;
+ };
+
+ mnoc_sf_0_tbu: tbu@150d5000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150d5000 0x0 0x1000>;
+ reg-names = "base";
+ interconnects = <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ACTIVE_ONLY
+ &mmss_noc SLAVE_MNOC_SF_MEM_NOC QCOM_ICC_TAG_ACTIVE_ONLY>;
+ qcom,stream-id-range = <0x1000 0x400>;
+ };
+
+ compute_dsp_tbu: tbu@150d9000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150d9000 0x0 0x1000>;
+ reg-names = "base";
+ interconnects = <&system_noc MASTER_GNOC_SNOC QCOM_ICC_TAG_ACTIVE_ONLY
+ &config_noc SLAVE_IMEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
+ qcom,stream-id-range = <0x1400 0x400>;
+ };
+
+ adsp_tbu: tbu@150dd000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150dd000 0x0 0x1000>;
+ reg-names = "base";
+ interconnects = <&system_noc MASTER_GNOC_SNOC QCOM_ICC_TAG_ACTIVE_ONLY
+ &config_noc SLAVE_IMEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_AGGRE_NOC_MMU_AUDIO_TBU_GDSC>;
+ qcom,stream-id-range = <0x1800 0x400>;
+ };
+
+ anoc_1_pcie_tbu: tbu@150e1000 {
+ compatible = "qcom,qsmmuv500-tbu";
+ reg = <0x0 0x150e1000 0x0 0x1000>;
+ reg-names = "base";
+ clocks = <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>;
+ interconnects = <&system_noc MASTER_GNOC_SNOC QCOM_ICC_TAG_ACTIVE_ONLY
+ &config_noc SLAVE_IMEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_AGGRE_NOC_MMU_PCIE_TBU_GDSC>;
+ qcom,stream-id-range = <0x1c00 0x400>;
+ };
};

lpasscc: clock-controller@17014000 {

2023-11-18 11:21:25

by Bryan O'Donoghue

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Add support for Translation Buffer Units

On 18/11/2023 04:27, Georgi Djakov wrote:
> The TCUs (Translation Control Units) and TBUs (Translation Buffer
> Units) are key components of the MMU-500. Multiple TBUs are connected
> to a single TCU over an interconnect. Each TBU contains a TLB that
> caches page tables. The MMU-500 implements a TBU for each connected
> master, and the TBU is designed, so that it is local to the master.
>
> The Qualcomm SDM845 platform has an implementation of the SMMU-500,
> that has multiple TBUs. A DT schema is added to describe the resources
> for each TBU (register space, power-domains, interconnects and clocks).
>
> The TBU driver will manage the resources and allow the system to
> operate the TBUs during a context fault to obtain details by doing
> s1 inv, software + hardware page table walks etc. This is implemented
> with ATOS/eCATs as the ATS feature is not supported. Being able to
> query the TBUs is useful for debugging various hardware/software
> issues on these platforms.
>
> v2:
> - Improve DT binding description, add full example. (Konrad)
> - Drop Qcom specific stuff from the generic binding. (Rob)
> - Unconditionally try to populate subnodes. (Konrad)
> - Improve TBU driver commit text, remove memory barriers. (Bjorn)
> - Move TBU stuff into separate file. Make the driver builtin.
> - TODO: Evaluate whether to keep TBU support as a separate driver
> or just instantiate things from qcom_smmu_impl_init()
>
> v1: https://lore.kernel.org/r/[email protected]

What is your suggested way to test this series ?

---
bod

2023-11-20 15:36:51

by Andrew Halaney

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] dt-bindings: iommu: Add Translation Buffer Unit bindings

On Fri, Nov 17, 2023 at 08:27:25PM -0800, Georgi Djakov wrote:
> The "apps_smmu" on the Qualcomm sdm845 platform is an implementation
> of the SMMU-500, that consists of a single TCU (Translation Control
> Unit) and multiple TBUs (Translation Buffer Units). The TCU is already
> being described in the generic SMMU DT schema. Add also bindings for

nit for if you respin: s/Add also/Add/ or similar :)

> the TBUs to describe their properties and resources that needs to be
> managed in order to operate them.
>
> In this DT schema, the TBUs are modelled as child devices of the TCU
> and each of them is described with it's register space, clocks, power
> domains, interconnects etc.
>
> Signed-off-by: Georgi Djakov <[email protected]>
> ---
> .../devicetree/bindings/iommu/arm,smmu.yaml | 25 ++++++
> .../bindings/iommu/qcom,qsmmuv500-tbu.yaml | 89 +++++++++++++++++++
> 2 files changed, 114 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index aa9e1c0895a5..f7f89be5f7a3 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -231,6 +231,18 @@ properties:
> enabled for any given device.
> $ref: /schemas/types.yaml#/definitions/phandle
>
> + '#address-cells':
> + enum: [ 1, 2 ]
> +
> + '#size-cells':
> + enum: [ 1, 2 ]
> +
> + ranges: true
> +
> +patternProperties:
> + "^tbu@[0-9a-f]*":
> + type: object
> +
> required:
> - compatible
> - reg
> @@ -453,6 +465,19 @@ allOf:
> - description: Voter clock required for HLOS SMMU access
> - description: Interface clock required for register access
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: qcom,smmu-500
> + then:
> + patternProperties:
> + "^tbu@[0-9a-f]*":
> + $ref: qcom,qsmmuv500-tbu.yaml
> + unevaluatedProperties: false
> + properties:
> + ranges: true
> +
> # Disallow clocks for all other platforms with specific compatibles
> - if:
> properties:
> diff --git a/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
> new file mode 100644
> index 000000000000..4dc9d0ca33c9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/qcom,qsmmuv500-tbu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm TBU (Translation Buffer Unit)
> +
> +maintainers:
> + - Georgi Djakov <[email protected]>
> +
> +description:
> + The Qualcomm SMMU500 implementation consists of TCU and TBU. The TBU contains
> + a Translation Lookaside Buffer (TLB) that caches page tables. TBUs provides
> + debug features to trace and trigger debug transactions. There are multiple TBU
> + instances distributes with each client core.
> +
> +properties:
> + $nodename:
> + pattern: "^tbu@[0-9a-f]+$"
> +
> + compatible:
> + const: qcom,qsmmuv500-tbu
> +
> + reg:
> + items:
> + - description: Address and size of the TBU's register space.
> +
> + reg-names:
> + items:
> + - const: base
> +
> + clocks:
> + maxItems: 1
> +
> + interconnects:
> + maxItems: 1
> +
> + power-domains:
> + maxItems: 1
> +
> + qcom,stream-id-range:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: Stream ID range (address and size) that is assigned by the TBU
> + items:
> + minItems: 2
> + maxItems: 2
> +
> +required:
> + - compatible
> + - reg
> + - interconnects
> + - qcom,stream-id-range
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> + #include <dt-bindings/interconnect/qcom,icc.h>
> + #include <dt-bindings/interconnect/qcom,sdm845.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/power/qcom-rpmpd.h>
> +
> + apps_smmu: iommu@15000000 {
> + compatible = "qcom,sdm845-smmu-500", "arm,mmu-500";
> + reg = <0x15000000 0x80000>;
> + ranges = <0 0 0 0 0xffffffff>;
> + #iommu-cells = <2>;
> + #global-interrupts = <1>;
> + interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 343 IRQ_TYPE_LEVEL_HIGH>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + anoc_1_pcie_tbu: tbu@150e1000 {
> + compatible = "qcom,qsmmuv500-tbu";
> + reg = <0x0 0x150e1000 0x0 0x1000>;
> + reg-names = "base";
> + clocks = <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>;
> + interconnects = <&system_noc MASTER_GNOC_SNOC QCOM_ICC_TAG_ACTIVE_ONLY
> + &config_noc SLAVE_IMEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
> + power-domains = <&gcc HLOS1_VOTE_AGGRE_NOC_MMU_PCIE_TBU_GDSC>;
> + qcom,stream-id-range = <0x1c00 0x400>;
> + };
> + };
> +
> +...
>

2023-11-27 18:14:05

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] dt-bindings: iommu: Add Translation Buffer Unit bindings

On Fri, Nov 17, 2023 at 08:27:25PM -0800, Georgi Djakov wrote:
> The "apps_smmu" on the Qualcomm sdm845 platform is an implementation
> of the SMMU-500, that consists of a single TCU (Translation Control
> Unit) and multiple TBUs (Translation Buffer Units). The TCU is already
> being described in the generic SMMU DT schema. Add also bindings for
> the TBUs to describe their properties and resources that needs to be
> managed in order to operate them.
>
> In this DT schema, the TBUs are modelled as child devices of the TCU
> and each of them is described with it's register space, clocks, power
> domains, interconnects etc.
>
> Signed-off-by: Georgi Djakov <[email protected]>
> ---
> .../devicetree/bindings/iommu/arm,smmu.yaml | 25 ++++++
> .../bindings/iommu/qcom,qsmmuv500-tbu.yaml | 89 +++++++++++++++++++
> 2 files changed, 114 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index aa9e1c0895a5..f7f89be5f7a3 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -231,6 +231,18 @@ properties:
> enabled for any given device.
> $ref: /schemas/types.yaml#/definitions/phandle
>
> + '#address-cells':
> + enum: [ 1, 2 ]
> +
> + '#size-cells':
> + enum: [ 1, 2 ]
> +
> + ranges: true
> +
> +patternProperties:
> + "^tbu@[0-9a-f]*":
> + type: object
> +
> required:
> - compatible
> - reg
> @@ -453,6 +465,19 @@ allOf:
> - description: Voter clock required for HLOS SMMU access
> - description: Interface clock required for register access
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: qcom,smmu-500

Doesn't match your example. No failure either, so there's some problem
with your schema. The issue is the tbu@ entry above has no constraint on
child properties. Dropping it would solve the issue. However, having a
TBU is not QCom specific, so that doesn't feel right.


> + then:
> + patternProperties:
> + "^tbu@[0-9a-f]*":

'+' rather than '*' as 1 is the minimum, not 0.

> + $ref: qcom,qsmmuv500-tbu.yaml
> + unevaluatedProperties: false
> + properties:
> + ranges: true
> +
> # Disallow clocks for all other platforms with specific compatibles
> - if:
> properties:
> diff --git a/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
> new file mode 100644
> index 000000000000..4dc9d0ca33c9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/qcom,qsmmuv500-tbu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm TBU (Translation Buffer Unit)
> +
> +maintainers:
> + - Georgi Djakov <[email protected]>
> +
> +description:
> + The Qualcomm SMMU500 implementation consists of TCU and TBU. The TBU contains
> + a Translation Lookaside Buffer (TLB) that caches page tables. TBUs provides
> + debug features to trace and trigger debug transactions. There are multiple TBU
> + instances distributes with each client core.
> +
> +properties:
> + $nodename:
> + pattern: "^tbu@[0-9a-f]+$"

Drop. You defined this in the parent already.

> +
> + compatible:
> + const: qcom,qsmmuv500-tbu
> +
> + reg:
> + items:
> + - description: Address and size of the TBU's register space.
> +
> + reg-names:
> + items:
> + - const: base

Not a useful name. Drop.

> +
> + clocks:
> + maxItems: 1
> +
> + interconnects:
> + maxItems: 1
> +
> + power-domains:
> + maxItems: 1
> +
> + qcom,stream-id-range:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: Stream ID range (address and size) that is assigned by the TBU
> + items:
> + minItems: 2
> + maxItems: 2

Perhaps implementations other than QCom's needs this?

Rob

2023-11-30 15:20:05

by Georgi Djakov

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Add support for Translation Buffer Units

On 11/18/2023 1:21 PM, Bryan O'Donoghue wrote:
> On 18/11/2023 04:27, Georgi Djakov wrote:
>> The TCUs (Translation Control Units) and TBUs (Translation Buffer
>> Units) are key components of the MMU-500. Multiple TBUs are connected
>> to a single TCU over an interconnect. Each TBU contains a TLB that
>> caches page tables. The MMU-500 implements a TBU for each connected
>> master, and the TBU is designed, so that it is local to the master.
>>
>> The Qualcomm SDM845 platform has an implementation of the SMMU-500,
>> that has multiple TBUs. A DT schema is added to describe the resources
>> for each TBU (register space, power-domains, interconnects and clocks).
>>
>> The TBU driver will manage the resources and allow the system to
>> operate the TBUs during a context fault to obtain details by doing
>> s1 inv, software + hardware page table walks etc. This is implemented
>> with ATOS/eCATs as the ATS feature is not supported. Being able to
>> query the TBUs is useful for debugging various hardware/software
>> issues on these platforms.
>>
>> v2:
>> - Improve DT binding description, add full example. (Konrad)
>> - Drop Qcom specific stuff from the generic binding. (Rob)
>> - Unconditionally try to populate subnodes. (Konrad)
>> - Improve TBU driver commit text, remove memory barriers. (Bjorn)
>> - Move TBU stuff into separate file. Make the driver builtin.
>> - TODO: Evaluate whether to keep TBU support as a separate driver
>>    or just instantiate things from qcom_smmu_impl_init()
>>
>> v1: https://lore.kernel.org/r/[email protected]
>
> What is your suggested way to test this series ?

Hi Bryan,

Just break some driver to initiate a memory transaction with no valid context. I used venus for that. Or the simplest would be to just specify an invalid stream ID in DT for some device.

Thanks,
Georgi

2023-11-30 23:25:30

by Georgi Djakov

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] dt-bindings: iommu: Add Translation Buffer Unit bindings

Hi Rob,

Thanks for the feedback!

On 11/27/2023 8:13 PM, Rob Herring wrote:
> On Fri, Nov 17, 2023 at 08:27:25PM -0800, Georgi Djakov wrote:
>> The "apps_smmu" on the Qualcomm sdm845 platform is an implementation
>> of the SMMU-500, that consists of a single TCU (Translation Control
>> Unit) and multiple TBUs (Translation Buffer Units). The TCU is already
>> being described in the generic SMMU DT schema. Add also bindings for
>> the TBUs to describe their properties and resources that needs to be
>> managed in order to operate them.
>>
>> In this DT schema, the TBUs are modelled as child devices of the TCU
>> and each of them is described with it's register space, clocks, power
>> domains, interconnects etc.
>>
>> Signed-off-by: Georgi Djakov <[email protected]>
>> ---
>> .../devicetree/bindings/iommu/arm,smmu.yaml | 25 ++++++
>> .../bindings/iommu/qcom,qsmmuv500-tbu.yaml | 89 +++++++++++++++++++
>> 2 files changed, 114 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> index aa9e1c0895a5..f7f89be5f7a3 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> @@ -231,6 +231,18 @@ properties:
>> enabled for any given device.
>> $ref: /schemas/types.yaml#/definitions/phandle
>>
>> + '#address-cells':
>> + enum: [ 1, 2 ]
>> +
>> + '#size-cells':
>> + enum: [ 1, 2 ]
>> +
>> + ranges: true
>> +
>> +patternProperties:
>> + "^tbu@[0-9a-f]*":
>> + type: object
>> +
>> required:
>> - compatible
>> - reg
>> @@ -453,6 +465,19 @@ allOf:
>> - description: Voter clock required for HLOS SMMU access
>> - description: Interface clock required for register access
>>
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: qcom,smmu-500
>
> Doesn't match your example. No failure either, so there's some problem
> with your schema. The issue is the tbu@ entry above has no constraint on
> child properties. Dropping it would solve the issue. However, having a
> TBU is not QCom specific, so that doesn't feel right.

Having a TBU is not Qcom specific. The ARM MMU-500 implementation for example has TBUs, but the registers are within the SMMU address space, there are no clocks, no power-domains or other resources. Not sure about other implementations. So should we just allow empty tbu child nodes with no properties?

>> + then:
>> + patternProperties:
>> + "^tbu@[0-9a-f]*":
>
> '+' rather than '*' as 1 is the minimum, not 0.

Ok.

>> + $ref: qcom,qsmmuv500-tbu.yaml
>> + unevaluatedProperties: false
>> + properties:
>> + ranges: true
>> +
>> # Disallow clocks for all other platforms with specific compatibles
>> - if:
>> properties:
>> diff --git a/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
>> new file mode 100644
>> index 000000000000..4dc9d0ca33c9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
>> @@ -0,0 +1,89 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iommu/qcom,qsmmuv500-tbu.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm TBU (Translation Buffer Unit)
>> +
>> +maintainers:
>> + - Georgi Djakov <[email protected]>
>> +
>> +description:
>> + The Qualcomm SMMU500 implementation consists of TCU and TBU. The TBU contains
>> + a Translation Lookaside Buffer (TLB) that caches page tables. TBUs provides
>> + debug features to trace and trigger debug transactions. There are multiple TBU
>> + instances distributes with each client core.
>> +
>> +properties:
>> + $nodename:
>> + pattern: "^tbu@[0-9a-f]+$"
>
> Drop. You defined this in the parent already.

Ok.

>> +
>> + compatible:
>> + const: qcom,qsmmuv500-tbu
>> +
>> + reg:
>> + items:
>> + - description: Address and size of the TBU's register space.
>> +
>> + reg-names:
>> + items:
>> + - const: base
>
> Not a useful name. Drop.

Agree.

>> +
>> + clocks:
>> + maxItems: 1
>> +
>> + interconnects:
>> + maxItems: 1
>> +
>> + power-domains:
>> + maxItems: 1
>> +
>> + qcom,stream-id-range:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description: Stream ID range (address and size) that is assigned by the TBU
>> + items:
>> + minItems: 2
>> + maxItems: 2
>
> Perhaps implementations other than QCom's needs this?

Yes, maybe. A TBU can service a fixed amount of stream IDs and this looks like something common for all TBUs. I'll drop the vendor prefix.

Thanks,
Georgi