* [PATCH v7 0/4] Add IOMMU driver for rk356x @ 2021-05-25 12:15 Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 1/4] dt-bindings: iommu: rockchip: Convert IOMMU to DT schema Benjamin Gaignard ` (4 more replies) 0 siblings, 5 replies; 14+ messages in thread From: Benjamin Gaignard @ 2021-05-25 12:15 UTC (permalink / raw) To: joro, will, robh+dt, heiko, xxm, robin.murphy Cc: devicetree, Benjamin Gaignard, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel This series adds the IOMMU driver for rk356x SoC. Since a new compatible is needed to distinguish this second version of IOMMU hardware block from the first one, it is an opportunity to convert the binding to DT schema. version 7: - Set DMA mask - Fix rk_iommu_enable() - rebased on v5.13-rc3 tag version 6: - Remove #include <module.h> - Remove pt_address_mask field - Only use once of_device_get_match_data - Return an error if ops don't match version 5: - Add internal ops inside the driver to be able to add variants. - Add support of v2 variant. - Stop using 'version' field - Use GENMASK macro version 4: - Add description for reg items - Remove useless interrupt-names properties - Add description for interrupts items - Remove interrupt-names properties from DST files version 3: - Rename compatible with soc prefix - Rebase on v5.12 tag version 2: - Fix iommu-cells typo in rk322x.dtsi - Change maintainer - Change reg maxItems - Add power-domains property Benjamin Gaignard (4): dt-bindings: iommu: rockchip: Convert IOMMU to DT schema dt-bindings: iommu: rockchip: Add compatible for v2 iommu: rockchip: Add internal ops to handle variants iommu: rockchip: Add support for iommu v2 .../bindings/iommu/rockchip,iommu.txt | 38 ---- .../bindings/iommu/rockchip,iommu.yaml | 85 +++++++++ drivers/iommu/rockchip-iommu.c | 166 +++++++++++++++--- 3 files changed, 229 insertions(+), 60 deletions(-) delete mode 100644 Documentation/devicetree/bindings/iommu/rockchip,iommu.txt create mode 100644 Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml -- 2.25.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v7 1/4] dt-bindings: iommu: rockchip: Convert IOMMU to DT schema 2021-05-25 12:15 [PATCH v7 0/4] Add IOMMU driver for rk356x Benjamin Gaignard @ 2021-05-25 12:15 ` Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 2/4] dt-bindings: iommu: rockchip: Add compatible for v2 Benjamin Gaignard ` (3 subsequent siblings) 4 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2021-05-25 12:15 UTC (permalink / raw) To: joro, will, robh+dt, heiko, xxm, robin.murphy Cc: devicetree, Benjamin Gaignard, Rob Herring, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel Convert Rockchip IOMMU to DT schema Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Rob Herring <robh@kernel.org> Reviewed-by: Heiko Stuebner <heiko@sntech.de> --- .../bindings/iommu/rockchip,iommu.txt | 38 --------- .../bindings/iommu/rockchip,iommu.yaml | 80 +++++++++++++++++++ 2 files changed, 80 insertions(+), 38 deletions(-) delete mode 100644 Documentation/devicetree/bindings/iommu/rockchip,iommu.txt create mode 100644 Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml diff --git a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt deleted file mode 100644 index 6ecefea1c6f9..000000000000 --- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt +++ /dev/null @@ -1,38 +0,0 @@ -Rockchip IOMMU -============== - -A Rockchip DRM iommu translates io virtual addresses to physical addresses for -its master device. Each slave device is bound to a single master device, and -shares its clocks, power domain and irq. - -Required properties: -- compatible : Should be "rockchip,iommu" -- reg : Address space for the configuration registers -- interrupts : Interrupt specifier for the IOMMU instance -- interrupt-names : Interrupt name for the IOMMU instance -- #iommu-cells : Should be <0>. This indicates the iommu is a - "single-master" device, and needs no additional information - to associate with its master device. See: - Documentation/devicetree/bindings/iommu/iommu.txt -- clocks : A list of clocks required for the IOMMU to be accessible by - the host CPU. -- clock-names : Should contain the following: - "iface" - Main peripheral bus clock (PCLK/HCL) (required) - "aclk" - AXI bus clock (required) - -Optional properties: -- rockchip,disable-mmu-reset : Don't use the mmu reset operation. - Some mmu instances may produce unexpected results - when the reset operation is used. - -Example: - - vopl_mmu: iommu@ff940300 { - compatible = "rockchip,iommu"; - reg = <0xff940300 0x100>; - interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; - interrupt-names = "vopl_mmu"; - clocks = <&cru ACLK_VOP1>, <&cru HCLK_VOP1>; - clock-names = "aclk", "iface"; - #iommu-cells = <0>; - }; diff --git a/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml b/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml new file mode 100644 index 000000000000..099fc2578b54 --- /dev/null +++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml @@ -0,0 +1,80 @@ +# SPDX-License-Identifier: GPL-2.0-only +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iommu/rockchip,iommu.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Rockchip IOMMU + +maintainers: + - Heiko Stuebner <heiko@sntech.de> + +description: |+ + A Rockchip DRM iommu translates io virtual addresses to physical addresses for + its master device. Each slave device is bound to a single master device and + shares its clocks, power domain and irq. + + For information on assigning IOMMU controller to its peripheral devices, + see generic IOMMU bindings. + +properties: + compatible: + const: rockchip,iommu + + reg: + items: + - description: configuration registers for MMU instance 0 + - description: configuration registers for MMU instance 1 + minItems: 1 + maxItems: 2 + + interrupts: + items: + - description: interruption for MMU instance 0 + - description: interruption for MMU instance 1 + minItems: 1 + maxItems: 2 + + clocks: + items: + - description: Core clock + - description: Interface clock + + clock-names: + items: + - const: aclk + - const: iface + + "#iommu-cells": + const: 0 + + rockchip,disable-mmu-reset: + $ref: /schemas/types.yaml#/definitions/flag + description: | + Do not use the mmu reset operation. + Some mmu instances may produce unexpected results + when the reset operation is used. + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - "#iommu-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/rk3399-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + vopl_mmu: iommu@ff940300 { + compatible = "rockchip,iommu"; + reg = <0xff940300 0x100>; + interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cru ACLK_VOP1>, <&cru HCLK_VOP1>; + clock-names = "aclk", "iface"; + #iommu-cells = <0>; + }; -- 2.25.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v7 2/4] dt-bindings: iommu: rockchip: Add compatible for v2 2021-05-25 12:15 [PATCH v7 0/4] Add IOMMU driver for rk356x Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 1/4] dt-bindings: iommu: rockchip: Convert IOMMU to DT schema Benjamin Gaignard @ 2021-05-25 12:15 ` Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants Benjamin Gaignard ` (2 subsequent siblings) 4 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2021-05-25 12:15 UTC (permalink / raw) To: joro, will, robh+dt, heiko, xxm, robin.murphy Cc: devicetree, Benjamin Gaignard, Rob Herring, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel Add compatible for the second version of IOMMU hardware block. RK356x IOMMU can also be link to a power domain. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Rob Herring <robh@kernel.org> Reviewed-by: Heiko Stuebner <heiko@sntech.de> --- .../devicetree/bindings/iommu/rockchip,iommu.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml b/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml index 099fc2578b54..d2e28a9e3545 100644 --- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml +++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml @@ -19,7 +19,9 @@ description: |+ properties: compatible: - const: rockchip,iommu + enum: + - rockchip,iommu + - rockchip,rk3568-iommu reg: items: @@ -48,6 +50,9 @@ properties: "#iommu-cells": const: 0 + power-domains: + maxItems: 1 + rockchip,disable-mmu-reset: $ref: /schemas/types.yaml#/definitions/flag description: | -- 2.25.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants 2021-05-25 12:15 [PATCH v7 0/4] Add IOMMU driver for rk356x Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 1/4] dt-bindings: iommu: rockchip: Convert IOMMU to DT schema Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 2/4] dt-bindings: iommu: rockchip: Add compatible for v2 Benjamin Gaignard @ 2021-05-25 12:15 ` Benjamin Gaignard 2021-07-29 15:59 ` Dafna Hirschfeld 2021-05-25 12:15 ` [PATCH v7 4/4] iommu: rockchip: Add support for iommu v2 Benjamin Gaignard 2021-06-04 14:54 ` [PATCH v7 0/4] Add IOMMU driver for rk356x Joerg Roedel 4 siblings, 1 reply; 14+ messages in thread From: Benjamin Gaignard @ 2021-05-25 12:15 UTC (permalink / raw) To: joro, will, robh+dt, heiko, xxm, robin.murphy Cc: devicetree, Benjamin Gaignard, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel Add internal ops to be able to handle incoming variant v2. The goal is to keep the overall structure of the framework but to allow to add the evolution of this hardware block. The ops are global for a SoC because iommu domains are not attached to a specific devices if they are for a virtuel device like drm. Use a global variable shouldn't be since SoC usually doesn't embedded different versions of the iommu hardware block. If that happen one day a WARN_ON will be displayed at probe time. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Heiko Stuebner <heiko@sntech.de> --- version 7: - Set DMA mask - Add function to convert dma address to dte version 6: - Remove #include <module.h> - Remove pt_address_mask field - Only use once of_device_get_match_data - Return an error if ops don't match version 5: - Use of_device_get_match_data() - Add internal ops inside the driver drivers/iommu/rockchip-iommu.c | 86 +++++++++++++++++++++++++--------- 1 file changed, 64 insertions(+), 22 deletions(-) diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 7a2932772fdf..bd2cf7f08c71 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -96,6 +96,15 @@ static const char * const rk_iommu_clocks[] = { "aclk", "iface", }; +struct rk_iommu_ops { + phys_addr_t (*pt_address)(u32 dte); + u32 (*mk_dtentries)(dma_addr_t pt_dma); + u32 (*mk_ptentries)(phys_addr_t page, int prot); + phys_addr_t (*dte_addr_phys)(u32 addr); + u32 (*dma_addr_dte)(dma_addr_t dt_dma); + u64 dma_bit_mask; +}; + struct rk_iommu { struct device *dev; void __iomem **bases; @@ -116,6 +125,7 @@ struct rk_iommudata { }; static struct device *dma_dev; +static const struct rk_iommu_ops *rk_ops; static inline void rk_table_flush(struct rk_iommu_domain *dom, dma_addr_t dma, unsigned int count) @@ -215,11 +225,6 @@ static inline u32 rk_mk_dte(dma_addr_t pt_dma) #define RK_PTE_PAGE_READABLE BIT(1) #define RK_PTE_PAGE_VALID BIT(0) -static inline phys_addr_t rk_pte_page_address(u32 pte) -{ - return (phys_addr_t)pte & RK_PTE_PAGE_ADDRESS_MASK; -} - static inline bool rk_pte_is_page_valid(u32 pte) { return pte & RK_PTE_PAGE_VALID; @@ -448,10 +453,10 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu) * and verifying that upper 5 nybbles are read back. */ for (i = 0; i < iommu->num_mmu; i++) { - rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, DTE_ADDR_DUMMY); + dte_addr = rk_ops->pt_address(DTE_ADDR_DUMMY); + rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, dte_addr); - dte_addr = rk_iommu_read(iommu->bases[i], RK_MMU_DTE_ADDR); - if (dte_addr != (DTE_ADDR_DUMMY & RK_DTE_PT_ADDRESS_MASK)) { + if (dte_addr != rk_iommu_read(iommu->bases[i], RK_MMU_DTE_ADDR)) { dev_err(iommu->dev, "Error during raw reset. MMU_DTE_ADDR is not functioning\n"); return -EFAULT; } @@ -470,6 +475,16 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu) return 0; } +static inline phys_addr_t rk_dte_addr_phys(u32 addr) +{ + return (phys_addr_t)addr; +} + +static inline u32 rk_dma_addr_dte(dma_addr_t dt_dma) +{ + return dt_dma; +} + static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova) { void __iomem *base = iommu->bases[index]; @@ -489,7 +504,7 @@ static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova) page_offset = rk_iova_page_offset(iova); mmu_dte_addr = rk_iommu_read(base, RK_MMU_DTE_ADDR); - mmu_dte_addr_phys = (phys_addr_t)mmu_dte_addr; + mmu_dte_addr_phys = rk_ops->dte_addr_phys(mmu_dte_addr); dte_addr_phys = mmu_dte_addr_phys + (4 * dte_index); dte_addr = phys_to_virt(dte_addr_phys); @@ -498,14 +513,14 @@ static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova) if (!rk_dte_is_pt_valid(dte)) goto print_it; - pte_addr_phys = rk_dte_pt_address(dte) + (pte_index * 4); + pte_addr_phys = rk_ops->pt_address(dte) + (pte_index * 4); pte_addr = phys_to_virt(pte_addr_phys); pte = *pte_addr; if (!rk_pte_is_page_valid(pte)) goto print_it; - page_addr_phys = rk_pte_page_address(pte) + page_offset; + page_addr_phys = rk_ops->pt_address(pte) + page_offset; page_flags = pte & RK_PTE_PAGE_FLAGS_MASK; print_it: @@ -601,13 +616,13 @@ static phys_addr_t rk_iommu_iova_to_phys(struct iommu_domain *domain, if (!rk_dte_is_pt_valid(dte)) goto out; - pt_phys = rk_dte_pt_address(dte); + pt_phys = rk_ops->pt_address(dte); page_table = (u32 *)phys_to_virt(pt_phys); pte = page_table[rk_iova_pte_index(iova)]; if (!rk_pte_is_page_valid(pte)) goto out; - phys = rk_pte_page_address(pte) + rk_iova_page_offset(iova); + phys = rk_ops->pt_address(pte) + rk_iova_page_offset(iova); out: spin_unlock_irqrestore(&rk_domain->dt_lock, flags); @@ -679,14 +694,14 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain, return ERR_PTR(-ENOMEM); } - dte = rk_mk_dte(pt_dma); + dte = rk_ops->mk_dtentries(pt_dma); *dte_addr = dte; rk_table_flush(rk_domain, pt_dma, NUM_PT_ENTRIES); rk_table_flush(rk_domain, rk_domain->dt_dma + dte_index * sizeof(u32), 1); done: - pt_phys = rk_dte_pt_address(dte); + pt_phys = rk_ops->pt_address(dte); return (u32 *)phys_to_virt(pt_phys); } @@ -728,7 +743,7 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr, if (rk_pte_is_page_valid(pte)) goto unwind; - pte_addr[pte_count] = rk_mk_pte(paddr, prot); + pte_addr[pte_count] = rk_ops->mk_ptentries(paddr, prot); paddr += SPAGE_SIZE; } @@ -750,7 +765,7 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr, pte_count * SPAGE_SIZE); iova += pte_count * SPAGE_SIZE; - page_phys = rk_pte_page_address(pte_addr[pte_count]); + page_phys = rk_ops->pt_address(pte_addr[pte_count]); pr_err("iova: %pad already mapped to %pa cannot remap to phys: %pa prot: %#x\n", &iova, &page_phys, &paddr, prot); @@ -785,7 +800,8 @@ static int rk_iommu_map(struct iommu_domain *domain, unsigned long _iova, dte_index = rk_domain->dt[rk_iova_dte_index(iova)]; pte_index = rk_iova_pte_index(iova); pte_addr = &page_table[pte_index]; - pte_dma = rk_dte_pt_address(dte_index) + pte_index * sizeof(u32); + + pte_dma = rk_ops->pt_address(dte_index) + pte_index * sizeof(u32); ret = rk_iommu_map_iova(rk_domain, pte_addr, pte_dma, iova, paddr, size, prot); @@ -821,7 +837,7 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova, return 0; } - pt_phys = rk_dte_pt_address(dte); + pt_phys = rk_ops->pt_address(dte); pte_addr = (u32 *)phys_to_virt(pt_phys) + rk_iova_pte_index(iova); pte_dma = pt_phys + rk_iova_pte_index(iova) * sizeof(u32); unmap_size = rk_iommu_unmap_iova(rk_domain, pte_addr, pte_dma, size); @@ -879,7 +895,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu) for (i = 0; i < iommu->num_mmu; i++) { rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, - rk_domain->dt_dma); + rk_ops->dma_addr_dte(rk_domain->dt_dma)); rk_iommu_base_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE); rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK); } @@ -1037,7 +1053,7 @@ static void rk_iommu_domain_free(struct iommu_domain *domain) for (i = 0; i < NUM_DT_ENTRIES; i++) { u32 dte = rk_domain->dt[i]; if (rk_dte_is_pt_valid(dte)) { - phys_addr_t pt_phys = rk_dte_pt_address(dte); + phys_addr_t pt_phys = rk_ops->pt_address(dte); u32 *page_table = phys_to_virt(pt_phys); dma_unmap_single(dma_dev, pt_phys, SPAGE_SIZE, DMA_TO_DEVICE); @@ -1127,6 +1143,7 @@ static int rk_iommu_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct rk_iommu *iommu; struct resource *res; + const struct rk_iommu_ops *ops; int num_res = pdev->num_resources; int err, i; @@ -1138,6 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev) iommu->dev = dev; iommu->num_mmu = 0; + ops = of_device_get_match_data(dev); + if (!rk_ops) + rk_ops = ops; + + /* + * That should not happen unless different versions of the + * hardware block are embedded the same SoC + */ + if (WARN_ON(rk_ops != ops)) + return -EINVAL; + iommu->bases = devm_kcalloc(dev, num_res, sizeof(*iommu->bases), GFP_KERNEL); if (!iommu->bases) @@ -1226,6 +1254,8 @@ static int rk_iommu_probe(struct platform_device *pdev) } } + dma_set_mask_and_coherent(dev, rk_ops->dma_bit_mask); + return 0; err_remove_sysfs: iommu_device_sysfs_remove(&iommu->iommu); @@ -1277,8 +1307,20 @@ static const struct dev_pm_ops rk_iommu_pm_ops = { pm_runtime_force_resume) }; +static struct rk_iommu_ops iommu_data_ops_v1 = { + .pt_address = &rk_dte_pt_address, + .mk_dtentries = &rk_mk_dte, + .mk_ptentries = &rk_mk_pte, + .dte_addr_phys = &rk_dte_addr_phys, + .dma_addr_dte = &rk_dma_addr_dte, + .dma_bit_mask = DMA_BIT_MASK(32), +}; + + static const struct of_device_id rk_iommu_dt_ids[] = { - { .compatible = "rockchip,iommu" }, + { .compatible = "rockchip,iommu", + .data = &iommu_data_ops_v1, + }, { /* sentinel */ } }; -- 2.25.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants 2021-05-25 12:15 ` [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants Benjamin Gaignard @ 2021-07-29 15:59 ` Dafna Hirschfeld 2021-07-29 16:08 ` Heiko Stübner 0 siblings, 1 reply; 14+ messages in thread From: Dafna Hirschfeld @ 2021-07-29 15:59 UTC (permalink / raw) To: Benjamin Gaignard, joro, will, robh+dt, heiko, xxm, robin.murphy, Ezequiel Garcia Cc: devicetree, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel On 25.05.21 14:15, Benjamin Gaignard wrote: > Add internal ops to be able to handle incoming variant v2. > The goal is to keep the overall structure of the framework but > to allow to add the evolution of this hardware block. > > The ops are global for a SoC because iommu domains are not > attached to a specific devices if they are for a virtuel device like > drm. Use a global variable shouldn't be since SoC usually doesn't > embedded different versions of the iommu hardware block. > If that happen one day a WARN_ON will be displayed at probe time. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > --- > version 7: > - Set DMA mask > - Add function to convert dma address to dte > > version 6: > - Remove #include <module.h> > - Remove pt_address_mask field > - Only use once of_device_get_match_data > - Return an error if ops don't match > > version 5: > - Use of_device_get_match_data() > - Add internal ops inside the driver > > drivers/iommu/rockchip-iommu.c | 86 +++++++++++++++++++++++++--------- > 1 file changed, 64 insertions(+), 22 deletions(-) > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index 7a2932772fdf..bd2cf7f08c71 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -96,6 +96,15 @@ static const char * const rk_iommu_clocks[] = { > "aclk", "iface", > }; > > +struct rk_iommu_ops { > + phys_addr_t (*pt_address)(u32 dte); > + u32 (*mk_dtentries)(dma_addr_t pt_dma); > + u32 (*mk_ptentries)(phys_addr_t page, int prot); > + phys_addr_t (*dte_addr_phys)(u32 addr); > + u32 (*dma_addr_dte)(dma_addr_t dt_dma); > + u64 dma_bit_mask; > +}; > + > struct rk_iommu { > struct device *dev; > void __iomem **bases; > @@ -116,6 +125,7 @@ struct rk_iommudata { > }; > > static struct device *dma_dev; > +static const struct rk_iommu_ops *rk_ops; > > static inline void rk_table_flush(struct rk_iommu_domain *dom, dma_addr_t dma, > unsigned int count) > @@ -215,11 +225,6 @@ static inline u32 rk_mk_dte(dma_addr_t pt_dma) > #define RK_PTE_PAGE_READABLE BIT(1) > #define RK_PTE_PAGE_VALID BIT(0) > > -static inline phys_addr_t rk_pte_page_address(u32 pte) > -{ > - return (phys_addr_t)pte & RK_PTE_PAGE_ADDRESS_MASK; > -} > - > static inline bool rk_pte_is_page_valid(u32 pte) > { > return pte & RK_PTE_PAGE_VALID; > @@ -448,10 +453,10 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu) > * and verifying that upper 5 nybbles are read back. > */ > for (i = 0; i < iommu->num_mmu; i++) { > - rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, DTE_ADDR_DUMMY); > + dte_addr = rk_ops->pt_address(DTE_ADDR_DUMMY); > + rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, dte_addr); > > - dte_addr = rk_iommu_read(iommu->bases[i], RK_MMU_DTE_ADDR); > - if (dte_addr != (DTE_ADDR_DUMMY & RK_DTE_PT_ADDRESS_MASK)) { > + if (dte_addr != rk_iommu_read(iommu->bases[i], RK_MMU_DTE_ADDR)) { > dev_err(iommu->dev, "Error during raw reset. MMU_DTE_ADDR is not functioning\n"); > return -EFAULT; > } > @@ -470,6 +475,16 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu) > return 0; > } > > +static inline phys_addr_t rk_dte_addr_phys(u32 addr) > +{ > + return (phys_addr_t)addr; > +} > + > +static inline u32 rk_dma_addr_dte(dma_addr_t dt_dma) > +{ > + return dt_dma; > +} > + > static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova) > { > void __iomem *base = iommu->bases[index]; > @@ -489,7 +504,7 @@ static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova) > page_offset = rk_iova_page_offset(iova); > > mmu_dte_addr = rk_iommu_read(base, RK_MMU_DTE_ADDR); > - mmu_dte_addr_phys = (phys_addr_t)mmu_dte_addr; > + mmu_dte_addr_phys = rk_ops->dte_addr_phys(mmu_dte_addr); > > dte_addr_phys = mmu_dte_addr_phys + (4 * dte_index); > dte_addr = phys_to_virt(dte_addr_phys); > @@ -498,14 +513,14 @@ static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova) > if (!rk_dte_is_pt_valid(dte)) > goto print_it; > > - pte_addr_phys = rk_dte_pt_address(dte) + (pte_index * 4); > + pte_addr_phys = rk_ops->pt_address(dte) + (pte_index * 4); > pte_addr = phys_to_virt(pte_addr_phys); > pte = *pte_addr; > > if (!rk_pte_is_page_valid(pte)) > goto print_it; > > - page_addr_phys = rk_pte_page_address(pte) + page_offset; > + page_addr_phys = rk_ops->pt_address(pte) + page_offset; > page_flags = pte & RK_PTE_PAGE_FLAGS_MASK; > > print_it: > @@ -601,13 +616,13 @@ static phys_addr_t rk_iommu_iova_to_phys(struct iommu_domain *domain, > if (!rk_dte_is_pt_valid(dte)) > goto out; > > - pt_phys = rk_dte_pt_address(dte); > + pt_phys = rk_ops->pt_address(dte); > page_table = (u32 *)phys_to_virt(pt_phys); > pte = page_table[rk_iova_pte_index(iova)]; > if (!rk_pte_is_page_valid(pte)) > goto out; > > - phys = rk_pte_page_address(pte) + rk_iova_page_offset(iova); > + phys = rk_ops->pt_address(pte) + rk_iova_page_offset(iova); > out: > spin_unlock_irqrestore(&rk_domain->dt_lock, flags); > > @@ -679,14 +694,14 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain, > return ERR_PTR(-ENOMEM); > } > > - dte = rk_mk_dte(pt_dma); > + dte = rk_ops->mk_dtentries(pt_dma); > *dte_addr = dte; > > rk_table_flush(rk_domain, pt_dma, NUM_PT_ENTRIES); > rk_table_flush(rk_domain, > rk_domain->dt_dma + dte_index * sizeof(u32), 1); > done: > - pt_phys = rk_dte_pt_address(dte); > + pt_phys = rk_ops->pt_address(dte); > return (u32 *)phys_to_virt(pt_phys); > } > > @@ -728,7 +743,7 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr, > if (rk_pte_is_page_valid(pte)) > goto unwind; > > - pte_addr[pte_count] = rk_mk_pte(paddr, prot); > + pte_addr[pte_count] = rk_ops->mk_ptentries(paddr, prot); > > paddr += SPAGE_SIZE; > } > @@ -750,7 +765,7 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr, > pte_count * SPAGE_SIZE); > > iova += pte_count * SPAGE_SIZE; > - page_phys = rk_pte_page_address(pte_addr[pte_count]); > + page_phys = rk_ops->pt_address(pte_addr[pte_count]); > pr_err("iova: %pad already mapped to %pa cannot remap to phys: %pa prot: %#x\n", > &iova, &page_phys, &paddr, prot); > > @@ -785,7 +800,8 @@ static int rk_iommu_map(struct iommu_domain *domain, unsigned long _iova, > dte_index = rk_domain->dt[rk_iova_dte_index(iova)]; > pte_index = rk_iova_pte_index(iova); > pte_addr = &page_table[pte_index]; > - pte_dma = rk_dte_pt_address(dte_index) + pte_index * sizeof(u32); > + > + pte_dma = rk_ops->pt_address(dte_index) + pte_index * sizeof(u32); > ret = rk_iommu_map_iova(rk_domain, pte_addr, pte_dma, iova, > paddr, size, prot); > > @@ -821,7 +837,7 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova, > return 0; > } > > - pt_phys = rk_dte_pt_address(dte); > + pt_phys = rk_ops->pt_address(dte); > pte_addr = (u32 *)phys_to_virt(pt_phys) + rk_iova_pte_index(iova); > pte_dma = pt_phys + rk_iova_pte_index(iova) * sizeof(u32); > unmap_size = rk_iommu_unmap_iova(rk_domain, pte_addr, pte_dma, size); > @@ -879,7 +895,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu) > > for (i = 0; i < iommu->num_mmu; i++) { > rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, > - rk_domain->dt_dma); > + rk_ops->dma_addr_dte(rk_domain->dt_dma)); Hi, This is not related to that patch, I was wondring why are all mmu devices initialized with the same dt_dma? I see for example that the isp0_mmu in rk3399.dtsi has two resources. Can't each resource be initialized with different dt_dma and this way there are two dt tables instead of the two mmus pointing to the same dt table. Thanks, Dafna > rk_iommu_base_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE); > rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK); > } > @@ -1037,7 +1053,7 @@ static void rk_iommu_domain_free(struct iommu_domain *domain) > for (i = 0; i < NUM_DT_ENTRIES; i++) { > u32 dte = rk_domain->dt[i]; > if (rk_dte_is_pt_valid(dte)) { > - phys_addr_t pt_phys = rk_dte_pt_address(dte); > + phys_addr_t pt_phys = rk_ops->pt_address(dte); > u32 *page_table = phys_to_virt(pt_phys); > dma_unmap_single(dma_dev, pt_phys, > SPAGE_SIZE, DMA_TO_DEVICE); > @@ -1127,6 +1143,7 @@ static int rk_iommu_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct rk_iommu *iommu; > struct resource *res; > + const struct rk_iommu_ops *ops; > int num_res = pdev->num_resources; > int err, i; > > @@ -1138,6 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev) > iommu->dev = dev; > iommu->num_mmu = 0; > > + ops = of_device_get_match_data(dev); > + if (!rk_ops) > + rk_ops = ops; > + > + /* > + * That should not happen unless different versions of the > + * hardware block are embedded the same SoC > + */ > + if (WARN_ON(rk_ops != ops)) > + return -EINVAL; > + > iommu->bases = devm_kcalloc(dev, num_res, sizeof(*iommu->bases), > GFP_KERNEL); > if (!iommu->bases) > @@ -1226,6 +1254,8 @@ static int rk_iommu_probe(struct platform_device *pdev) > } > } > > + dma_set_mask_and_coherent(dev, rk_ops->dma_bit_mask); > + > return 0; > err_remove_sysfs: > iommu_device_sysfs_remove(&iommu->iommu); > @@ -1277,8 +1307,20 @@ static const struct dev_pm_ops rk_iommu_pm_ops = { > pm_runtime_force_resume) > }; > > +static struct rk_iommu_ops iommu_data_ops_v1 = { > + .pt_address = &rk_dte_pt_address, > + .mk_dtentries = &rk_mk_dte, > + .mk_ptentries = &rk_mk_pte, > + .dte_addr_phys = &rk_dte_addr_phys, > + .dma_addr_dte = &rk_dma_addr_dte, > + .dma_bit_mask = DMA_BIT_MASK(32), > +}; > + > + > static const struct of_device_id rk_iommu_dt_ids[] = { > - { .compatible = "rockchip,iommu" }, > + { .compatible = "rockchip,iommu", > + .data = &iommu_data_ops_v1, > + }, > { /* sentinel */ } > }; > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants 2021-07-29 15:59 ` Dafna Hirschfeld @ 2021-07-29 16:08 ` Heiko Stübner 2021-07-29 16:58 ` Robin Murphy 0 siblings, 1 reply; 14+ messages in thread From: Heiko Stübner @ 2021-07-29 16:08 UTC (permalink / raw) To: Benjamin Gaignard, joro, will, robh+dt, xxm, robin.murphy, Ezequiel Garcia, Dafna Hirschfeld Cc: devicetree, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel Hi Dafna, Am Donnerstag, 29. Juli 2021, 17:59:26 CEST schrieb Dafna Hirschfeld: > On 25.05.21 14:15, Benjamin Gaignard wrote: > > @@ -879,7 +895,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu) > > > > for (i = 0; i < iommu->num_mmu; i++) { > > rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, > > - rk_domain->dt_dma); > > + rk_ops->dma_addr_dte(rk_domain->dt_dma)); > > Hi, > This is not related to that patch, I was wondring why are all mmu devices initialized > with the same dt_dma? > I see for example that the isp0_mmu in rk3399.dtsi has two resources. Can't each resource > be initialized with different dt_dma and this way there are two dt tables instead of the two mmus pointing > to the same dt table. maybe git log -1 cd6438c5f8446691afa4829fe1a9d7b656204f11 "iommu/rockchip: Reconstruct to support multi slaves There are some IPs, such as video encoder/decoder, contains 2 slave iommus, one for reading and the other for writing. They share the same irq and clock with master. This patch reconstructs to support this case by making them share the same Page Directory, Page Tables and even the register operations. That means every instruction to the reading MMU registers would be duplicated to the writing MMU and vice versa." Heiko > > Thanks, > Dafna > > > rk_iommu_base_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE); > > rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK); > > } > > @@ -1037,7 +1053,7 @@ static void rk_iommu_domain_free(struct iommu_domain *domain) > > for (i = 0; i < NUM_DT_ENTRIES; i++) { > > u32 dte = rk_domain->dt[i]; > > if (rk_dte_is_pt_valid(dte)) { > > - phys_addr_t pt_phys = rk_dte_pt_address(dte); > > + phys_addr_t pt_phys = rk_ops->pt_address(dte); > > u32 *page_table = phys_to_virt(pt_phys); > > dma_unmap_single(dma_dev, pt_phys, > > SPAGE_SIZE, DMA_TO_DEVICE); > > @@ -1127,6 +1143,7 @@ static int rk_iommu_probe(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct rk_iommu *iommu; > > struct resource *res; > > + const struct rk_iommu_ops *ops; > > int num_res = pdev->num_resources; > > int err, i; > > > > @@ -1138,6 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev) > > iommu->dev = dev; > > iommu->num_mmu = 0; > > > > + ops = of_device_get_match_data(dev); > > + if (!rk_ops) > > + rk_ops = ops; > > + > > + /* > > + * That should not happen unless different versions of the > > + * hardware block are embedded the same SoC > > + */ > > + if (WARN_ON(rk_ops != ops)) > > + return -EINVAL; > > + > > iommu->bases = devm_kcalloc(dev, num_res, sizeof(*iommu->bases), > > GFP_KERNEL); > > if (!iommu->bases) > > @@ -1226,6 +1254,8 @@ static int rk_iommu_probe(struct platform_device *pdev) > > } > > } > > > > + dma_set_mask_and_coherent(dev, rk_ops->dma_bit_mask); > > + > > return 0; > > err_remove_sysfs: > > iommu_device_sysfs_remove(&iommu->iommu); > > @@ -1277,8 +1307,20 @@ static const struct dev_pm_ops rk_iommu_pm_ops = { > > pm_runtime_force_resume) > > }; > > > > +static struct rk_iommu_ops iommu_data_ops_v1 = { > > + .pt_address = &rk_dte_pt_address, > > + .mk_dtentries = &rk_mk_dte, > > + .mk_ptentries = &rk_mk_pte, > > + .dte_addr_phys = &rk_dte_addr_phys, > > + .dma_addr_dte = &rk_dma_addr_dte, > > + .dma_bit_mask = DMA_BIT_MASK(32), > > +}; > > + > > + > > static const struct of_device_id rk_iommu_dt_ids[] = { > > - { .compatible = "rockchip,iommu" }, > > + { .compatible = "rockchip,iommu", > > + .data = &iommu_data_ops_v1, > > + }, > > { /* sentinel */ } > > }; > > > > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants 2021-07-29 16:08 ` Heiko Stübner @ 2021-07-29 16:58 ` Robin Murphy 2021-07-30 12:52 ` Dafna Hirschfeld 0 siblings, 1 reply; 14+ messages in thread From: Robin Murphy @ 2021-07-29 16:58 UTC (permalink / raw) To: Heiko Stübner, Benjamin Gaignard, joro, will, robh+dt, xxm, Ezequiel Garcia, Dafna Hirschfeld Cc: devicetree, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel On 2021-07-29 17:08, Heiko Stübner wrote: > Hi Dafna, > > Am Donnerstag, 29. Juli 2021, 17:59:26 CEST schrieb Dafna Hirschfeld: >> On 25.05.21 14:15, Benjamin Gaignard wrote: >>> @@ -879,7 +895,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu) >>> >>> for (i = 0; i < iommu->num_mmu; i++) { >>> rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, >>> - rk_domain->dt_dma); >>> + rk_ops->dma_addr_dte(rk_domain->dt_dma)); >> >> Hi, >> This is not related to that patch, I was wondring why are all mmu devices initialized >> with the same dt_dma? >> I see for example that the isp0_mmu in rk3399.dtsi has two resources. Can't each resource >> be initialized with different dt_dma and this way there are two dt tables instead of the two mmus pointing >> to the same dt table. > > maybe > git log -1 cd6438c5f8446691afa4829fe1a9d7b656204f11 > > "iommu/rockchip: Reconstruct to support multi slaves > > There are some IPs, such as video encoder/decoder, contains 2 slave iommus, > one for reading and the other for writing. They share the same irq and > clock with master. > > This patch reconstructs to support this case by making them share the same > Page Directory, Page Tables and even the register operations. > That means every instruction to the reading MMU registers would be > duplicated to the writing MMU and vice versa." Right. In theory we *could* maintain a separate pagetable for each IOMMU instance, but it would just lead to a load of complexity and overhead. For a map request, we'd have to do extra work to decide which table(s) need modifying, and duplicate all the work of the actual mapping if it's more than one. For an unmap request, we'd have no choice but to walk *all* the tables backing that domain to figure out which (if any) actually had it mapped in the first place. Given that we already have distinct read and write permissions for mappings within a single table, there's not even any functional benefit that could be gained in this case (and in the more general case where the device might emit all kinds of transactions from all its interfaces you'd have to maintain identical mappings for all its IOMMUs anyway). Saving memory and code complexity by physically sharing one pagetable and not worrying about trying to do selective TLB maintenance is a bigger win than anything else could be. Robin. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants 2021-07-29 16:58 ` Robin Murphy @ 2021-07-30 12:52 ` Dafna Hirschfeld 2021-07-30 13:29 ` Robin Murphy 0 siblings, 1 reply; 14+ messages in thread From: Dafna Hirschfeld @ 2021-07-30 12:52 UTC (permalink / raw) To: Robin Murphy, Heiko Stübner, Benjamin Gaignard, joro, will, robh+dt, xxm, Ezequiel Garcia Cc: devicetree, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel On 29.07.21 18:58, Robin Murphy wrote: > On 2021-07-29 17:08, Heiko Stübner wrote: >> Hi Dafna, >> >> Am Donnerstag, 29. Juli 2021, 17:59:26 CEST schrieb Dafna Hirschfeld: >>> On 25.05.21 14:15, Benjamin Gaignard wrote: >>>> @@ -879,7 +895,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu) >>>> for (i = 0; i < iommu->num_mmu; i++) { >>>> rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, >>>> - rk_domain->dt_dma); >>>> + rk_ops->dma_addr_dte(rk_domain->dt_dma)); >>> >>> Hi, >>> This is not related to that patch, I was wondring why are all mmu devices initialized >>> with the same dt_dma? >>> I see for example that the isp0_mmu in rk3399.dtsi has two resources. Can't each resource >>> be initialized with different dt_dma and this way there are two dt tables instead of the two mmus pointing >>> to the same dt table. >> >> maybe >> git log -1 cd6438c5f8446691afa4829fe1a9d7b656204f11 >> >> "iommu/rockchip: Reconstruct to support multi slaves >> There are some IPs, such as video encoder/decoder, contains 2 slave iommus, >> one for reading and the other for writing. They share the same irq and >> clock with master. >> This patch reconstructs to support this case by making them share the same >> Page Directory, Page Tables and even the register operations. >> That means every instruction to the reading MMU registers would be >> duplicated to the writing MMU and vice versa." > > Right. In theory we *could* maintain a separate pagetable for each IOMMU instance, but it would just lead to a load of complexity and overhead. For a map request, we'd have to do extra work to decide which table(s) need modifying, and duplicate all the work of the actual mapping if it's more than one. For an unmap request, we'd have no choice but to walk *all* the tables backing that domain to figure out which (if any) actually had it mapped in the first place. > > Given that we already have distinct read and write permissions for mappings within a single table, there's not even any functional benefit that could be gained in this case (and in the more general case where the device might emit all kinds of transactions from all its interfaces you'd have to maintain identical mappings for all its IOMMUs anyway). Saving memory and code complexity by physically sharing one pagetable and not worrying about trying to do selective TLB maintenance is a bigger win than anything else could be. > > Robin. Hi, I just try to understand how this iommu hardware/software works. I have two questions, 1. So we currently miss a potential mapping of the hardware right? I mean , each mmu can map 1024*1024*4K = 4G addresses, so two mmus can potentially map 8G. But since we set them to identical values, we can map only up to 4G. 2. What is the benefit of setting all mmus if they are all set to the same values? Can't we just work with the first mmu like it was done before that patch cd6438c5f8446691afa4829fe1a9d7b656204f11 Thanks, Dafna _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants 2021-07-30 12:52 ` Dafna Hirschfeld @ 2021-07-30 13:29 ` Robin Murphy 0 siblings, 0 replies; 14+ messages in thread From: Robin Murphy @ 2021-07-30 13:29 UTC (permalink / raw) To: Dafna Hirschfeld, Heiko Stübner, Benjamin Gaignard, joro, will, robh+dt, xxm, Ezequiel Garcia Cc: devicetree, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel On 2021-07-30 13:52, Dafna Hirschfeld wrote: > > > On 29.07.21 18:58, Robin Murphy wrote: >> On 2021-07-29 17:08, Heiko Stübner wrote: >>> Hi Dafna, >>> >>> Am Donnerstag, 29. Juli 2021, 17:59:26 CEST schrieb Dafna Hirschfeld: >>>> On 25.05.21 14:15, Benjamin Gaignard wrote: >>>>> @@ -879,7 +895,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu) >>>>> for (i = 0; i < iommu->num_mmu; i++) { >>>>> rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, >>>>> - rk_domain->dt_dma); >>>>> + rk_ops->dma_addr_dte(rk_domain->dt_dma)); >>>> >>>> Hi, >>>> This is not related to that patch, I was wondring why are all mmu >>>> devices initialized >>>> with the same dt_dma? >>>> I see for example that the isp0_mmu in rk3399.dtsi has two >>>> resources. Can't each resource >>>> be initialized with different dt_dma and this way there are two dt >>>> tables instead of the two mmus pointing >>>> to the same dt table. >>> >>> maybe >>> git log -1 cd6438c5f8446691afa4829fe1a9d7b656204f11 >>> >>> "iommu/rockchip: Reconstruct to support multi slaves >>> There are some IPs, such as video encoder/decoder, contains 2 slave >>> iommus, >>> one for reading and the other for writing. They share the same irq and >>> clock with master. >>> This patch reconstructs to support this case by making them share the >>> same >>> Page Directory, Page Tables and even the register operations. >>> That means every instruction to the reading MMU registers would be >>> duplicated to the writing MMU and vice versa." >> >> Right. In theory we *could* maintain a separate pagetable for each >> IOMMU instance, but it would just lead to a load of complexity and >> overhead. For a map request, we'd have to do extra work to decide >> which table(s) need modifying, and duplicate all the work of the >> actual mapping if it's more than one. For an unmap request, we'd have >> no choice but to walk *all* the tables backing that domain to figure >> out which (if any) actually had it mapped in the first place. >> >> Given that we already have distinct read and write permissions for >> mappings within a single table, there's not even any functional >> benefit that could be gained in this case (and in the more general >> case where the device might emit all kinds of transactions from all >> its interfaces you'd have to maintain identical mappings for all its >> IOMMUs anyway). Saving memory and code complexity by physically >> sharing one pagetable and not worrying about trying to do selective >> TLB maintenance is a bigger win than anything else could be. >> >> Robin. > > Hi, I just try to understand how this iommu hardware/software works. I > have two questions, > > 1. So we currently miss a potential mapping of the hardware right? I > mean , each mmu can map 1024*1024*4K = 4G addresses, so two mmus can > potentially map 8G. But since > we set them to identical values, we can map only up to 4G. Not quite. We have 4GB of address space in which read transaction operate, and 4GB of address space in which write transactions operate, but it's hopefully obvious why those are not interchangeable. Technically you *could* map a piece of physical memory to be read and written via different virtual addresses, but you could do that with permissions in a single table anyway, and mostly it would just break any device which expects a single buffer address to both read and write. > 2. What is the benefit of setting all mmus if they are all set to the > same values? Can't we just work with the first mmu like it was done > before that patch > cd6438c5f8446691afa4829fe1a9d7b656204f11 The hardware has two physical interfaces through which it issues transactions - if we only program the IOMMU on one of those interfaces, different transactions will have inconsistent views of memory as above, and the device will probably not function correctly. Before that patch, the cases where just the "first" MMU was programmed were the ones where it was also the only MMU, as they still are. The IOMMUs for these multi-interface devices weren't enabled at all because it would have just broken things. Robin. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v7 4/4] iommu: rockchip: Add support for iommu v2 2021-05-25 12:15 [PATCH v7 0/4] Add IOMMU driver for rk356x Benjamin Gaignard ` (2 preceding siblings ...) 2021-05-25 12:15 ` [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants Benjamin Gaignard @ 2021-05-25 12:15 ` Benjamin Gaignard 2021-05-25 16:51 ` kernel test robot 2021-05-25 21:21 ` kernel test robot 2021-06-04 14:54 ` [PATCH v7 0/4] Add IOMMU driver for rk356x Joerg Roedel 4 siblings, 2 replies; 14+ messages in thread From: Benjamin Gaignard @ 2021-05-25 12:15 UTC (permalink / raw) To: joro, will, robh+dt, heiko, xxm, robin.murphy Cc: devicetree, Benjamin Gaignard, linux-kernel, linux-rockchip, iommu, kernel, linux-arm-kernel This second version of the hardware block has a different bits mapping for page table entries. Add the ops matching to this new mapping. Define a new compatible to distinguish it from the first version. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Heiko Stuebner <heiko@sntech.de> --- version 7: - Set dma_bit_mask field. - Add rk_dma_addr_dte_v2 function version 5: - Use internal ops to support v2 hardware block - Use GENMASK macro. - Keep rk_dte_pt_address() and rk_dte_pt_address_v2() separated because I believe that is more readable like this. - Do not duplicate code. drivers/iommu/rockchip-iommu.c | 84 +++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index bd2cf7f08c71..edd05e488aa7 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -189,6 +189,33 @@ static inline phys_addr_t rk_dte_pt_address(u32 dte) return (phys_addr_t)dte & RK_DTE_PT_ADDRESS_MASK; } +/* + * In v2: + * 31:12 - PT address bit 31:0 + * 11: 8 - PT address bit 35:32 + * 7: 4 - PT address bit 39:36 + * 3: 1 - Reserved + * 0 - 1 if PT @ PT address is valid + */ +#define RK_DTE_PT_ADDRESS_MASK_V2 GENMASK_ULL(31, 4) +#define DTE_HI_MASK1 GENMASK(11, 8) +#define DTE_HI_MASK2 GENMASK(7, 4) +#define DTE_HI_SHIFT1 24 /* shift bit 8 to bit 32 */ +#define DTE_HI_SHIFT2 32 /* shift bit 4 to bit 36 */ +#define PAGE_DESC_HI_MASK1 GENMASK_ULL(39, 36) +#define PAGE_DESC_HI_MASK2 GENMASK_ULL(35, 32) + +static inline phys_addr_t rk_dte_pt_address_v2(u32 dte) +{ + u64 dte_v2 = dte; + + dte_v2 = ((dte_v2 & DTE_HI_MASK2) << DTE_HI_SHIFT2) | + ((dte_v2 & DTE_HI_MASK1) << DTE_HI_SHIFT1) | + (dte_v2 & RK_DTE_PT_ADDRESS_MASK); + + return (phys_addr_t)dte_v2; +} + static inline bool rk_dte_is_pt_valid(u32 dte) { return dte & RK_DTE_PT_VALID; @@ -199,6 +226,15 @@ static inline u32 rk_mk_dte(dma_addr_t pt_dma) return (pt_dma & RK_DTE_PT_ADDRESS_MASK) | RK_DTE_PT_VALID; } +static inline u32 rk_mk_dte_v2(dma_addr_t pt_dma) +{ + pt_dma = (pt_dma & RK_DTE_PT_ADDRESS_MASK) | + ((pt_dma & PAGE_DESC_HI_MASK1) >> DTE_HI_SHIFT1) | + (pt_dma & PAGE_DESC_HI_MASK2) >> DTE_HI_SHIFT2; + + return (pt_dma & RK_DTE_PT_ADDRESS_MASK_V2) | RK_DTE_PT_VALID; +} + /* * Each PTE has a Page address, some flags and a valid bit: * +---------------------+---+-------+-+ @@ -240,6 +276,29 @@ static u32 rk_mk_pte(phys_addr_t page, int prot) return page | flags | RK_PTE_PAGE_VALID; } +/* + * In v2: + * 31:12 - Page address bit 31:0 + * 11:9 - Page address bit 34:32 + * 8:4 - Page address bit 39:35 + * 3 - Security + * 2 - Readable + * 1 - Writable + * 0 - 1 if Page @ Page address is valid + */ +#define RK_PTE_PAGE_READABLE_V2 BIT(2) +#define RK_PTE_PAGE_WRITABLE_V2 BIT(1) + +static u32 rk_mk_pte_v2(phys_addr_t page, int prot) +{ + u32 flags = 0; + + flags |= (prot & IOMMU_READ) ? RK_PTE_PAGE_READABLE_V2 : 0; + flags |= (prot & IOMMU_WRITE) ? RK_PTE_PAGE_WRITABLE_V2 : 0; + + return rk_mk_dte_v2(page) | flags; +} + static u32 rk_mk_pte_invalid(u32 pte) { return pte & ~RK_PTE_PAGE_VALID; @@ -480,9 +539,19 @@ static inline phys_addr_t rk_dte_addr_phys(u32 addr) return (phys_addr_t)addr; } -static inline u32 rk_dma_addr_dte(dma_addr_t dt_dma) +#define DT_HI_MASK GENMASK_ULL(39, 32) +#define DT_SHIFT 28 + +static inline phys_addr_t rk_dte_addr_phys_v2(u32 addr) +{ + return (phys_addr_t)(addr & RK_DTE_PT_ADDRESS_MASK) | + ((addr & DT_HI_MASK) << DT_SHIFT); +} + +static inline u32 rk_dma_addr_dte_v2(dma_addr_t dt_dma) { - return dt_dma; + return (dt_dma & RK_DTE_PT_ADDRESS_MASK) | + ((dt_dma & DT_HI_MASK) >> DT_SHIFT); } static void log_iova(struct rk_iommu *iommu, int index, dma_addr_t iova) @@ -1316,11 +1385,22 @@ static struct rk_iommu_ops iommu_data_ops_v1 = { .dma_bit_mask = DMA_BIT_MASK(32), }; +static struct rk_iommu_ops iommu_data_ops_v2 = { + .pt_address = &rk_dte_pt_address_v2, + .mk_dtentries = &rk_mk_dte_v2, + .mk_ptentries = &rk_mk_pte_v2, + .dte_addr_phys = &rk_dte_addr_phys_v2, + .dma_addr_dte = &rk_dma_addr_dte_v2, + .dma_bit_mask = DMA_BIT_MASK(40), +}; static const struct of_device_id rk_iommu_dt_ids[] = { { .compatible = "rockchip,iommu", .data = &iommu_data_ops_v1, }, + { .compatible = "rockchip,rk3568-iommu", + .data = &iommu_data_ops_v2, + }, { /* sentinel */ } }; -- 2.25.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v7 4/4] iommu: rockchip: Add support for iommu v2 2021-05-25 12:15 ` [PATCH v7 4/4] iommu: rockchip: Add support for iommu v2 Benjamin Gaignard @ 2021-05-25 16:51 ` kernel test robot 2021-05-25 21:21 ` kernel test robot 1 sibling, 0 replies; 14+ messages in thread From: kernel test robot @ 2021-05-25 16:51 UTC (permalink / raw) To: Benjamin Gaignard, joro, will, robh+dt, heiko, xxm, robin.murphy Cc: devicetree, iommu, kbuild-all, linux-arm-kernel, linux-rockchip [-- Attachment #1: Type: text/plain, Size: 2776 bytes --] Hi Benjamin, I love your patch! Yet something to improve: [auto build test ERROR on iommu/next] [also build test ERROR on rockchip/for-next robh/for-next v5.13-rc3 next-20210525] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Benjamin-Gaignard/Add-IOMMU-driver-for-rk356x/20210525-201749 base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next config: powerpc-randconfig-s031-20210525 (attached as .config) compiler: powerpc64-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.3-341-g8af24329-dirty # https://github.com/0day-ci/linux/commit/bd6c989c628be7c8ac1f8aeb9f301bd3a6e0a078 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Benjamin-Gaignard/Add-IOMMU-driver-for-rk356x/20210525-201749 git checkout bd6c989c628be7c8ac1f8aeb9f301bd3a6e0a078 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> drivers/iommu/rockchip-iommu.c:1384:19: error: 'rk_dma_addr_dte' undeclared here (not in a function); did you mean 'rk_dma_addr_dte_v2'? 1384 | .dma_addr_dte = &rk_dma_addr_dte, | ^~~~~~~~~~~~~~~ | rk_dma_addr_dte_v2 vim +1384 drivers/iommu/rockchip-iommu.c 0f181d3cf7d984b Jeffy Chen 2018-03-23 1378 ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1379 static struct rk_iommu_ops iommu_data_ops_v1 = { ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1380 .pt_address = &rk_dte_pt_address, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1381 .mk_dtentries = &rk_mk_dte, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1382 .mk_ptentries = &rk_mk_pte, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1383 .dte_addr_phys = &rk_dte_addr_phys, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 @1384 .dma_addr_dte = &rk_dma_addr_dte, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1385 .dma_bit_mask = DMA_BIT_MASK(32), ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1386 }; ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1387 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 41569 bytes --] [-- Attachment #3: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 4/4] iommu: rockchip: Add support for iommu v2 2021-05-25 12:15 ` [PATCH v7 4/4] iommu: rockchip: Add support for iommu v2 Benjamin Gaignard 2021-05-25 16:51 ` kernel test robot @ 2021-05-25 21:21 ` kernel test robot 1 sibling, 0 replies; 14+ messages in thread From: kernel test robot @ 2021-05-25 21:21 UTC (permalink / raw) To: Benjamin Gaignard, joro, will, robh+dt, heiko, xxm, robin.murphy Cc: devicetree, kbuild-all, clang-built-linux, linux-rockchip, iommu, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 3006 bytes --] Hi Benjamin, I love your patch! Yet something to improve: [auto build test ERROR on iommu/next] [also build test ERROR on rockchip/for-next robh/for-next v5.13-rc3 next-20210525] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Benjamin-Gaignard/Add-IOMMU-driver-for-rk356x/20210525-201749 base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next config: arm-randconfig-r025-20210525 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 99155e913e9bad5f7f8a247f8bb3a3ff3da74af1) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi # https://github.com/0day-ci/linux/commit/bd6c989c628be7c8ac1f8aeb9f301bd3a6e0a078 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Benjamin-Gaignard/Add-IOMMU-driver-for-rk356x/20210525-201749 git checkout bd6c989c628be7c8ac1f8aeb9f301bd3a6e0a078 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> drivers/iommu/rockchip-iommu.c:1384:19: error: use of undeclared identifier 'rk_dma_addr_dte'; did you mean 'rk_dma_addr_dte_v2'? .dma_addr_dte = &rk_dma_addr_dte, ^~~~~~~~~~~~~~~ rk_dma_addr_dte_v2 drivers/iommu/rockchip-iommu.c:551:19: note: 'rk_dma_addr_dte_v2' declared here static inline u32 rk_dma_addr_dte_v2(dma_addr_t dt_dma) ^ 1 error generated. vim +1384 drivers/iommu/rockchip-iommu.c 0f181d3cf7d984b Jeffy Chen 2018-03-23 1378 ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1379 static struct rk_iommu_ops iommu_data_ops_v1 = { ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1380 .pt_address = &rk_dte_pt_address, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1381 .mk_dtentries = &rk_mk_dte, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1382 .mk_ptentries = &rk_mk_pte, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1383 .dte_addr_phys = &rk_dte_addr_phys, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 @1384 .dma_addr_dte = &rk_dma_addr_dte, ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1385 .dma_bit_mask = DMA_BIT_MASK(32), ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1386 }; ce0eeece4c9ef42 Benjamin Gaignard 2021-05-25 1387 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 37646 bytes --] [-- Attachment #3: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 0/4] Add IOMMU driver for rk356x 2021-05-25 12:15 [PATCH v7 0/4] Add IOMMU driver for rk356x Benjamin Gaignard ` (3 preceding siblings ...) 2021-05-25 12:15 ` [PATCH v7 4/4] iommu: rockchip: Add support for iommu v2 Benjamin Gaignard @ 2021-06-04 14:54 ` Joerg Roedel 2021-06-04 14:56 ` Joerg Roedel 4 siblings, 1 reply; 14+ messages in thread From: Joerg Roedel @ 2021-06-04 14:54 UTC (permalink / raw) To: Benjamin Gaignard Cc: devicetree, heiko, will, linux-kernel, linux-rockchip, iommu, robh+dt, kernel, robin.murphy, linux-arm-kernel On Tue, May 25, 2021 at 02:15:47PM +0200, Benjamin Gaignard wrote: > Benjamin Gaignard (4): > dt-bindings: iommu: rockchip: Convert IOMMU to DT schema > dt-bindings: iommu: rockchip: Add compatible for v2 > iommu: rockchip: Add internal ops to handle variants Applied patches 1-3, thanks. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v7 0/4] Add IOMMU driver for rk356x 2021-06-04 14:54 ` [PATCH v7 0/4] Add IOMMU driver for rk356x Joerg Roedel @ 2021-06-04 14:56 ` Joerg Roedel 0 siblings, 0 replies; 14+ messages in thread From: Joerg Roedel @ 2021-06-04 14:56 UTC (permalink / raw) To: Benjamin Gaignard Cc: devicetree, heiko, will, linux-kernel, linux-rockchip, iommu, robh+dt, kernel, robin.murphy, linux-arm-kernel On Fri, Jun 04, 2021 at 04:54:58PM +0200, Joerg Roedel wrote: > On Tue, May 25, 2021 at 02:15:47PM +0200, Benjamin Gaignard wrote: > > Benjamin Gaignard (4): > > dt-bindings: iommu: rockchip: Convert IOMMU to DT schema > > dt-bindings: iommu: rockchip: Add compatible for v2 > > iommu: rockchip: Add internal ops to handle variants > > Applied patches 1-3, thanks. Hmm, no, unapplied. Your separate patch 4 came before the kbuild-bot reports. Can you fix those please and submit a v8? Thanks, Joerg _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-07-30 13:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-25 12:15 [PATCH v7 0/4] Add IOMMU driver for rk356x Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 1/4] dt-bindings: iommu: rockchip: Convert IOMMU to DT schema Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 2/4] dt-bindings: iommu: rockchip: Add compatible for v2 Benjamin Gaignard 2021-05-25 12:15 ` [PATCH v7 3/4] iommu: rockchip: Add internal ops to handle variants Benjamin Gaignard 2021-07-29 15:59 ` Dafna Hirschfeld 2021-07-29 16:08 ` Heiko Stübner 2021-07-29 16:58 ` Robin Murphy 2021-07-30 12:52 ` Dafna Hirschfeld 2021-07-30 13:29 ` Robin Murphy 2021-05-25 12:15 ` [PATCH v7 4/4] iommu: rockchip: Add support for iommu v2 Benjamin Gaignard 2021-05-25 16:51 ` kernel test robot 2021-05-25 21:21 ` kernel test robot 2021-06-04 14:54 ` [PATCH v7 0/4] Add IOMMU driver for rk356x Joerg Roedel 2021-06-04 14:56 ` Joerg Roedel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).