* [PATCH v3 0/6] MT8173 IOMMU SUPPORT
@ 2015-07-16 9:04 Yong Wu
2015-07-16 9:04 ` [PATCH v3 1/6] dt-bindings: iommu: Add binding for mediatek IOMMU Yong Wu
` (5 more replies)
0 siblings, 6 replies; 34+ messages in thread
From: Yong Wu @ 2015-07-16 9:04 UTC (permalink / raw)
To: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger
Cc: Robin Murphy, Will Deacon, Daniel Kurtz, Tomasz Figa,
Lucas Stach, Rob Herring, Catalin Marinas, linux-mediatek,
Sasha Hauer, srv_heupstream, devicetree, linux-kernel,
linux-arm-kernel, iommu, pebolle, arnd, mitchelh, cloud.chou,
frederic.chen, yong.wu
This patch adds support for m4u(Multimedia Memory Management Unit),
Currently it only support the m4u with 2 levels of pagetable on mt8173.
It is based on Robin Murphy's arm64 DMA v3: IOMMU-backed DMA mapping[1].
Please check the hardware block diagram of Mediatek IOMMU.
EMI (External Memory Interface)
|
m4u (Multimedia Memory Management Unit)
|
smi (Smart Multimedia Interface)
|
+---------------+-------
| |
| |
vdec larb disp larb ... SoCs have different local arbiter(larb).
| |
| |
+----+----+ +-----+-----+
| | | | | | ...
| | | | | | ...
| | | | | | ...
MC PP VLD OVL0 RDMA0 WDMA0 ... There are different ports in each larb.
Normally we specify a local arbiter(larb) for each multimedia hardware like
display, video decode, video encode and camera. And there are different ports in
each larb. Take a example, there are some ports like MC, PP, UFO, VLD, AVC_MV,
PRED_RD in video larb, all the ports are according to the video hardware.
From the diagram, all the multimedia module connect with m4u via smi.
SMI is responsible to enable/disable iommu and control the clocks for each local
arbiter. If we should enable the iommu of video decode, it should config the
video's ports. And if the video hardware work whether enable/disable iommu,
it should enable the clock of its larb's clock. And smi also help bandwidth
control for each local arbiter. So we add a special driver for smi and locate
it drivers/memory.
[1] http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013597.html
v3:
-rebased onto v4.2-rc1
-improve iommu flow based on the Robin's DMA v3.
-change mtk iommu-cells from 1 to 2.
-about Short-Descriptor: add split function; add self-test; add some other bits like nG,
XN according to the spec; add SUPERSECTION and MTK quirk; move io_pgtable_ops_to_pgtable
out from LPAE to the header file.
-about SMI: move from driver/soc/mediatek to driver/memory; change the clocks from
clk[2] to clk_apb and clk_smi; add pm.
-add iommu suspend/resume to backup/restore register.
v2: http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013028.html
-add arm short descriptor support.
-seperate smi common from smi and change the clock-names according
to smi HW.
-delete the hardcode of the port-names in mt8173.
replace this with larb-portes-nr in dtsi.
-fix some coding style issues.
v1: http://lists.infradead.org/pipermail/linux-mediatek/2015-March/000058.html
-initial version.
Yong Wu (6):
dt-bindings: iommu: Add binding for mediatek IOMMU
dt-bindings: mediatek: Add smi dts binding
iommu: add ARM short descriptor page table allocator.
memory: mediatek: Add SMI driver
iommu/mediatek: Add mt8173 IOMMU driver
dts: mt8173: Add iommu/smi nodes for mt8173
.../devicetree/bindings/iommu/mediatek,iommu.txt | 61 ++
.../memory-controllers/mediatek,smi-larb.txt | 25 +
.../bindings/memory-controllers/mediatek,smi.txt | 24 +
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 81 +++
drivers/iommu/Kconfig | 31 +
drivers/iommu/Makefile | 2 +
drivers/iommu/io-pgtable-arm-short.c | 742 ++++++++++++++++++++
drivers/iommu/io-pgtable-arm.c | 3 -
drivers/iommu/io-pgtable.c | 4 +
drivers/iommu/io-pgtable.h | 13 +
drivers/iommu/mtk_iommu.c | 725 +++++++++++++++++++
drivers/memory/Kconfig | 8 +
drivers/memory/Makefile | 1 +
drivers/memory/mtk-smi.c | 285 ++++++++
include/dt-bindings/memory/mt8173-larb-port.h | 105 +++
include/soc/mediatek/smi.h | 60 ++
16 files changed, 2167 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,smi.txt
create mode 100644 drivers/iommu/io-pgtable-arm-short.c
create mode 100644 drivers/iommu/mtk_iommu.c
create mode 100644 drivers/memory/mtk-smi.c
create mode 100644 include/dt-bindings/memory/mt8173-larb-port.h
create mode 100644 include/soc/mediatek/smi.h
--
1.7.9.5
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 1/6] dt-bindings: iommu: Add binding for mediatek IOMMU
2015-07-16 9:04 [PATCH v3 0/6] MT8173 IOMMU SUPPORT Yong Wu
@ 2015-07-16 9:04 ` Yong Wu
2015-07-16 9:04 ` [PATCH v3 2/6] dt-bindings: mediatek: Add smi dts binding Yong Wu
` (4 subsequent siblings)
5 siblings, 0 replies; 34+ messages in thread
From: Yong Wu @ 2015-07-16 9:04 UTC (permalink / raw)
To: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger
Cc: Robin Murphy, Will Deacon, Daniel Kurtz, Tomasz Figa,
Lucas Stach, Rob Herring, Catalin Marinas, linux-mediatek,
Sasha Hauer, srv_heupstream, devicetree, linux-kernel,
linux-arm-kernel, iommu, pebolle, arnd, mitchelh, cloud.chou,
frederic.chen, yong.wu
This patch add mediatek iommu dts binding document.
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
.../devicetree/bindings/iommu/mediatek,iommu.txt | 61 ++++++++++++
include/dt-bindings/memory/mt8173-larb-port.h | 105 ++++++++++++++++++++
2 files changed, 166 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
create mode 100644 include/dt-bindings/memory/mt8173-larb-port.h
diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
new file mode 100644
index 0000000..ffdc130
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
@@ -0,0 +1,61 @@
+* Mediatek IOMMU Architecture Implementation
+
+ Mediatek Socs may contain a implementation of Multimedia Memory
+Management Unit(M4U),which use ARM Short-descriptor translation table
+to achieve address translation.
+
+ The IOMMU Hardware Block Diagram, please check below:
+
+ EMI (External Memory Interface)
+ |
+ m4u (Multimedia Memory Management Unit)
+ |
+ smi (Smart Multimedia Interface)
+ |
+ +---------------+-------
+ | |
+ | |
+ vdec larb disp larb ... SoCs have different local arbiter(larb).
+ | |
+ | |
+ +----+----+ +-----+-----+
+ | | | | | | ...
+ | | | | | | ...
+ | | | | | | ...
+ MC PP VLD OVL0 RDMA0 WDMA0 ... There are different ports in each larb.
+
+ As above, The Multimedia HW will go through SMI and M4U while it
+access EMI. SMI is a brige between m4u and the Multimedia HW. It contain
+smi local arbiter and smi common. It will control whether the Multimedia
+HW should go though the m4u for translation or bypass it and talk
+directly with EMI. And also SMI help control the clocks for each
+local arbiter.
+ Normally we specify a local arbiter(larb) for each multimedia HW
+like display, video decode, and camera. And there are different ports
+in each larb. Take a example, There are some ports like MC, PP, VLD in the
+video decode local arbiter, all the ports are according to the video HW.
+
+Required properties:
+- compatible : must be "mediatek,mt8173-m4u".
+- reg : m4u register base and size.
+- interrupts : the interrupt of m4u.
+- clocks : must contain one entry for each clock-names.
+- clock-names : must be "bclk", It is the block clock of m4u.
+- mediatek,larb : List of phandle to the local arbiters in the current Socs.
+ Refer to bindings/memory-controllers/mediatek,smi-larb.txt. It must sort
+ according to the local arbiter index, like larb0, larb1, larb2...
+- iommu-cells : must be 2. There are 2 cells needed to enable/disable iommu.
+ The first one is local arbiter index(larbid), and the other is port
+ index(portid) within local arbiter. Specifies the larbid and portid as
+ defined in dt-binding/memory/mt8173-larb-port.h.
+
+Example:
+ iommu: mmsys_iommu@10205000 {
+ compatible = "mediatek,mt8173-m4u";
+ reg = <0 0x10205000 0 0x1000>;
+ interrupts = <GIC_SPI 139 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&infracfg CLK_INFRA_M4U>;
+ clock-names = "bclk";
+ mediatek,larb = <&larb0 &larb1 &larb2 &larb3 &larb4 &larb5>;
+ #iommu-cells = <2>;
+ };
\ No newline at end of file
diff --git a/include/dt-bindings/memory/mt8173-larb-port.h b/include/dt-bindings/memory/mt8173-larb-port.h
new file mode 100644
index 0000000..7517087
--- /dev/null
+++ b/include/dt-bindings/memory/mt8173-larb-port.h
@@ -0,0 +1,105 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu <yong.wu@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#ifndef __DTS_IOMMU_PORT_MT8173_H
+#define __DTS_IOMMU_PORT_MT8173_H
+
+#define M4U_LARB0_ID 0
+#define M4U_LARB1_ID 1
+#define M4U_LARB2_ID 2
+#define M4U_LARB3_ID 3
+#define M4U_LARB4_ID 4
+#define M4U_LARB5_ID 5
+
+/* larb0 */
+#define M4U_PORT_DISP_OVL0 0
+#define M4U_PORT_DISP_RDMA0 1
+#define M4U_PORT_DISP_WDMA0 2
+#define M4U_PORT_DISP_OD_R 3
+#define M4U_PORT_DISP_OD_W 4
+#define M4U_PORT_MDP_RDMA0 5
+#define M4U_PORT_MDP_WDMA 6
+#define M4U_PORT_MDP_WROT0 7
+
+/* larb1 */
+#define M4U_PORT_HW_VDEC_MC_EXT 0
+#define M4U_PORT_HW_VDEC_PP_EXT 1
+#define M4U_PORT_HW_VDEC_UFO_EXT 2
+#define M4U_PORT_HW_VDEC_VLD_EXT 3
+#define M4U_PORT_HW_VDEC_VLD2_EXT 4
+#define M4U_PORT_HW_VDEC_AVC_MV_EXT 5
+#define M4U_PORT_HW_VDEC_PRED_RD_EXT 6
+#define M4U_PORT_HW_VDEC_PRED_WR_EXT 7
+#define M4U_PORT_HW_VDEC_PPWRAP_EXT 8
+#define M4U_PORT_HW_VDEC_TILE 9
+
+/* larb2 */
+#define M4U_PORT_IMGO 0
+#define M4U_PORT_RRZO 1
+#define M4U_PORT_AAO 2
+#define M4U_PORT_LCSO 3
+#define M4U_PORT_ESFKO 4
+#define M4U_PORT_IMGO_D 5
+#define M4U_PORT_LSCI 6
+#define M4U_PORT_LSCI_D 7
+#define M4U_PORT_BPCI 8
+#define M4U_PORT_BPCI_D 9
+#define M4U_PORT_UFDI 10
+#define M4U_PORT_IMGI 11
+#define M4U_PORT_IMG2O 12
+#define M4U_PORT_IMG3O 13
+#define M4U_PORT_VIPI 14
+#define M4U_PORT_VIP2I 15
+#define M4U_PORT_VIP3I 16
+#define M4U_PORT_LCEI 17
+#define M4U_PORT_RB 18
+#define M4U_PORT_RP 19
+#define M4U_PORT_WR 20
+
+/* larb3 */
+#define M4U_PORT_VENC_RCPU 0
+#define M4U_PORT_VENC_REC 1
+#define M4U_PORT_VENC_BSDMA 2
+#define M4U_PORT_VENC_SV_COMV 3
+#define M4U_PORT_VENC_RD_COMV 4
+#define M4U_PORT_JPGENC_RDMA 5
+#define M4U_PORT_JPGENC_BSDMA 6
+#define M4U_PORT_JPGDEC_WDMA 7
+#define M4U_PORT_JPGDEC_BSDMA 8
+#define M4U_PORT_VENC_CUR_LUMA 9
+#define M4U_PORT_VENC_CUR_CHROMA 10
+#define M4U_PORT_VENC_REF_LUMA 11
+#define M4U_PORT_VENC_REF_CHROMA 12
+#define M4U_PORT_VENC_NBM_RDMA 13
+#define M4U_PORT_VENC_NBM_WDMA 14
+
+/* larb4 */
+#define M4U_PORT_DISP_OVL1 0
+#define M4U_PORT_DISP_RDMA1 1
+#define M4U_PORT_DISP_RDMA2 2
+#define M4U_PORT_DISP_WDMA1 3
+#define M4U_PORT_MDP_RDMA1 4
+#define M4U_PORT_MDP_WROT1 5
+
+/* larb5 */
+#define M4U_PORT_VENC_RCPU_SET2 0
+#define M4U_PORT_VENC_REC_FRM_SET2 1
+#define M4U_PORT_VENC_REF_LUMA_SET2 2
+#define M4U_PORT_VENC_REC_CHROMA_SET2 3
+#define M4U_PORT_VENC_BSDMA_SET2 4
+#define M4U_PORT_VENC_CUR_LUMA_SET2 5
+#define M4U_PORT_VENC_CUR_CHROMA_SET2 6
+#define M4U_PORT_VENC_RD_COMA_SET2 7
+#define M4U_PORT_VENC_SV_COMA_SET2 8
+
+#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 2/6] dt-bindings: mediatek: Add smi dts binding
2015-07-16 9:04 [PATCH v3 0/6] MT8173 IOMMU SUPPORT Yong Wu
2015-07-16 9:04 ` [PATCH v3 1/6] dt-bindings: iommu: Add binding for mediatek IOMMU Yong Wu
@ 2015-07-16 9:04 ` Yong Wu
2015-07-16 9:04 ` [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator Yong Wu
` (3 subsequent siblings)
5 siblings, 0 replies; 34+ messages in thread
From: Yong Wu @ 2015-07-16 9:04 UTC (permalink / raw)
To: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger
Cc: Robin Murphy, Will Deacon, Daniel Kurtz, Tomasz Figa,
Lucas Stach, Rob Herring, Catalin Marinas, linux-mediatek,
Sasha Hauer, srv_heupstream, devicetree, linux-kernel,
linux-arm-kernel, iommu, pebolle, arnd, mitchelh, cloud.chou,
frederic.chen, yong.wu
This patch add smi binding document.
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
.../memory-controllers/mediatek,smi-larb.txt | 25 ++++++++++++++++++++
.../bindings/memory-controllers/mediatek,smi.txt | 24 +++++++++++++++++++
2 files changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,smi.txt
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
new file mode 100644
index 0000000..ae11dcf
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
@@ -0,0 +1,25 @@
+SMI (Smart Multimedia Interface) Local Arbiter
+
+The hardware block diagram please check bindings/iommu/mediatek,iommu.txt
+
+Required properties:
+- compatible : must be "mediatek,mt8173-smi-larb"
+- reg : the register and size of this local arbiter.
+- mediatek,smi : a phandle to the smi_common node.
+- power-domains : a phandle to the power domain of this local arbiter.
+- clocks : Must contain an entry for each entry in clock-names.
+- clock-names: must contain 2 entries, as follows:
+ - "apb" : Advanced Peripheral Bus clock, It's the clock for setting
+ the register.
+ - "smi" : It's the clock for transfer data and command.
+
+Example:
+ larb1: larb@16010000 {
+ compatible = "mediatek,mt8173-smi-larb";
+ reg = <0 0x16010000 0 0x1000>;
+ mediatek,smi = <&smi_common>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
+ clocks = <&vdecsys CLK_VDEC_CKEN>,
+ <&vdecsys CLK_VDEC_LARB_CKEN>;
+ clock-names = "apb", "smi";
+ };
\ No newline at end of file
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi.txt b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi.txt
new file mode 100644
index 0000000..f34b3ef
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi.txt
@@ -0,0 +1,24 @@
+SMI (Smart Multimedia Interface)
+
+The hardware block diagram please check bindings/iommu/mediatek,iommu.txt
+
+Required properties:
+- compatible : must be "mediatek,mt8173-smi"
+- reg : the register and size of the SMI block.
+- power-domains : a phandle to the power domain of this local arbiter.
+- clocks : Must contain an entry for each entry in clock-names.
+- clock-names : must contain 2 entries, as follows:
+ - "apb" : Advanced Peripheral Bus clock, It's the clock for setting
+ the register.
+ - "smi" : It's the clock for transfer data and command.
+ They may be the same if both source clock are the same.
+
+Example:
+ smi_common: smi@14022000 {
+ compatible = "mediatek,mt8173-smi";
+ reg = <0 0x14022000 0 0x1000>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
+ clocks = <&mmsys CLK_MM_SMI_COMMON>,
+ <&mmsys CLK_MM_SMI_COMMON>;
+ clock-names = "apb", "smi";
+ };
\ No newline at end of file
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-16 9:04 [PATCH v3 0/6] MT8173 IOMMU SUPPORT Yong Wu
2015-07-16 9:04 ` [PATCH v3 1/6] dt-bindings: iommu: Add binding for mediatek IOMMU Yong Wu
2015-07-16 9:04 ` [PATCH v3 2/6] dt-bindings: mediatek: Add smi dts binding Yong Wu
@ 2015-07-16 9:04 ` Yong Wu
2015-07-21 17:11 ` Will Deacon
2015-07-16 9:04 ` [PATCH v3 4/6] memory: mediatek: Add SMI driver Yong Wu
` (2 subsequent siblings)
5 siblings, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-07-16 9:04 UTC (permalink / raw)
To: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger
Cc: Robin Murphy, Will Deacon, Daniel Kurtz, Tomasz Figa,
Lucas Stach, Rob Herring, Catalin Marinas, linux-mediatek,
Sasha Hauer, srv_heupstream, devicetree, linux-kernel,
linux-arm-kernel, iommu, pebolle, arnd, mitchelh, cloud.chou,
frederic.chen, yong.wu
This patch is for ARM Short Descriptor Format.
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
drivers/iommu/Kconfig | 18 +
drivers/iommu/Makefile | 1 +
drivers/iommu/io-pgtable-arm-short.c | 742 ++++++++++++++++++++++++++++++++++
drivers/iommu/io-pgtable-arm.c | 3 -
drivers/iommu/io-pgtable.c | 4 +
drivers/iommu/io-pgtable.h | 13 +
6 files changed, 778 insertions(+), 3 deletions(-)
create mode 100644 drivers/iommu/io-pgtable-arm-short.c
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f1fb1d3..f50dbf3 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -39,6 +39,24 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
If unsure, say N here.
+config IOMMU_IO_PGTABLE_SHORT
+ bool "ARMv7/v8 Short Descriptor Format"
+ select IOMMU_IO_PGTABLE
+ depends on ARM || ARM64 || COMPILE_TEST
+ help
+ Enable support for the ARM Short descriptor pagetable format.
+ This allocator supports 2 levels translation tables which supports
+ a memory map based on memory sections or pages.
+
+config IOMMU_IO_PGTABLE_SHORT_SELFTEST
+ bool "Short Descriptor selftests"
+ depends on IOMMU_IO_PGTABLE_SHORT
+ help
+ Enable self-tests for Short Descriptor page table allocator.
+ This performs a series of page-table consistency checks during boot.
+
+ If unsure, say N here.
+
endmenu
config IOMMU_IOVA
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index c6dcc51..06df3e6 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_IOMMU_API) += iommu-traces.o
obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
+obj-$(CONFIG_IOMMU_IO_PGTABLE_SHORT) += io-pgtable-arm-short.o
obj-$(CONFIG_IOMMU_IOVA) += iova.o
obj-$(CONFIG_OF_IOMMU) += of_iommu.o
obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
diff --git a/drivers/iommu/io-pgtable-arm-short.c b/drivers/iommu/io-pgtable-arm-short.c
new file mode 100644
index 0000000..340d590
--- /dev/null
+++ b/drivers/iommu/io-pgtable-arm-short.c
@@ -0,0 +1,742 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu <yong.wu@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#define pr_fmt(fmt) "arm-short-desc io-pgtable: "fmt
+
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/iommu.h>
+#include <linux/errno.h>
+
+#include "io-pgtable.h"
+
+typedef u32 arm_short_iopte;
+
+struct arm_short_io_pgtable {
+ struct io_pgtable iop;
+ struct kmem_cache *ptekmem;
+ size_t pgd_size;
+ void *pgd;
+};
+
+#define io_pgtable_to_data(x) \
+ container_of((x), struct arm_short_io_pgtable, iop)
+
+#define io_pgtable_ops_to_data(x) \
+ io_pgtable_to_data(io_pgtable_ops_to_pgtable(x))
+
+#define ARM_SHORT_PGDIR_SHIFT 20
+#define ARM_SHORT_PAGE_SHIFT 12
+#define ARM_SHORT_PTRS_PER_PTE \
+ (1 << (ARM_SHORT_PGDIR_SHIFT - ARM_SHORT_PAGE_SHIFT))
+#define ARM_SHORT_BYTES_PER_PTE \
+ (ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte))
+
+/* level 1 pagetable */
+#define ARM_SHORT_PGD_TYPE_PGTABLE BIT(0)
+#define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4))
+#define ARM_SHORT_PGD_TYPE_SECTION BIT(1)
+#define ARM_SHORT_PGD_PGTABLE_XN BIT(2)
+#define ARM_SHORT_PGD_B BIT(2)
+#define ARM_SHORT_PGD_C BIT(3)
+#define ARM_SHORT_PGD_PGTABLE_NS BIT(3)
+#define ARM_SHORT_PGD_IMPLE BIT(9)
+#define ARM_SHORT_PGD_TEX0 BIT(12)
+#define ARM_SHORT_PGD_S BIT(16)
+#define ARM_SHORT_PGD_nG BIT(17)
+#define ARM_SHORT_PGD_SUPERSECTION BIT(18)
+#define ARM_SHORT_PGD_SECTION_NS BIT(19)
+
+#define ARM_SHORT_PGD_TYPE_SUPERSECTION \
+ (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
+#define ARM_SHORT_PGD_SECTION_TYPE_MSK \
+ (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
+#define ARM_SHORT_PGD_PGTABLE_TYPE_MSK \
+ (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_TYPE_PGTABLE)
+#define ARM_SHORT_PGD_TYPE_IS_PGTABLE(pgd) \
+ (((pgd) & ARM_SHORT_PGD_PGTABLE_TYPE_MSK) == ARM_SHORT_PGD_TYPE_PGTABLE)
+#define ARM_SHORT_PGD_TYPE_IS_SECTION(pgd) \
+ (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == ARM_SHORT_PGD_TYPE_SECTION)
+#define ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(pgd) \
+ (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == \
+ ARM_SHORT_PGD_TYPE_SUPERSECTION)
+#define ARM_SHORT_PGD_PGTABLE_MSK 0xfffffc00
+#define ARM_SHORT_PGD_SECTION_MSK (~(SZ_1M - 1))
+#define ARM_SHORT_PGD_SUPERSECTION_MSK (~(SZ_16M - 1))
+
+/* level 2 pagetable */
+#define ARM_SHORT_PTE_TYPE_LARGE BIT(0)
+#define ARM_SHORT_PTE_SMALL_XN BIT(0)
+#define ARM_SHORT_PTE_TYPE_SMALL BIT(1)
+#define ARM_SHORT_PTE_B BIT(2)
+#define ARM_SHORT_PTE_C BIT(3)
+#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
+#define ARM_SHORT_PTE_IMPLE BIT(9)
+#define ARM_SHORT_PTE_S BIT(10)
+#define ARM_SHORT_PTE_nG BIT(11)
+#define ARM_SHORT_PTE_LARGE_TEX0 BIT(12)
+#define ARM_SHORT_PTE_LARGE_XN BIT(15)
+#define ARM_SHORT_PTE_LARGE_MSK (~(SZ_64K - 1))
+#define ARM_SHORT_PTE_SMALL_MSK (~(SZ_4K - 1))
+#define ARM_SHORT_PTE_TYPE_MSK \
+ (ARM_SHORT_PTE_TYPE_LARGE | ARM_SHORT_PTE_TYPE_SMALL)
+#define ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(pte) \
+ (((((pte) & ARM_SHORT_PTE_TYPE_MSK) >> 1) << 1)\
+ == ARM_SHORT_PTE_TYPE_SMALL)
+#define ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(pte) \
+ (((pte) & ARM_SHORT_PTE_TYPE_MSK) == ARM_SHORT_PTE_TYPE_LARGE)
+
+#define ARM_SHORT_PGD_IDX(a) ((a) >> ARM_SHORT_PGDIR_SHIFT)
+#define ARM_SHORT_PTE_IDX(a) \
+ (((a) >> ARM_SHORT_PAGE_SHIFT) & (ARM_SHORT_PTRS_PER_PTE - 1))
+
+#define ARM_SHORT_GET_PTE_VA(pgd) \
+ (phys_to_virt((unsigned long)pgd & ARM_SHORT_PGD_PGTABLE_MSK))
+
+#define ARM_SHORT_PTE_LARGE_GET_PROT(pte) \
+ (((pte) & (~ARM_SHORT_PTE_LARGE_MSK)) & ~ARM_SHORT_PTE_TYPE_MSK)
+
+#define ARM_SHORT_PGD_GET_PROT(pgd) \
+ (((pgd) & (~ARM_SHORT_PGD_SECTION_MSK)) & ~ARM_SHORT_PGD_SUPERSECTION)
+
+static bool selftest_running;
+
+static arm_short_iopte *
+arm_short_get_pte_in_pgd(arm_short_iopte pgd, unsigned int iova)
+{
+ arm_short_iopte *pte;
+
+ pte = ARM_SHORT_GET_PTE_VA(pgd);
+ pte += ARM_SHORT_PTE_IDX(iova);
+ return pte;
+}
+
+static void _arm_short_free_pgtable(struct arm_short_io_pgtable *data,
+ arm_short_iopte *pgd)
+{
+ const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
+ arm_short_iopte *pte;
+ int i;
+
+ pte = ARM_SHORT_GET_PTE_VA(*pgd);
+ for (i = 0; i < ARM_SHORT_PTRS_PER_PTE; i++) {
+ if (pte[i] != 0)
+ return;
+ }
+
+ /* Free whole pte and set pgd to zero while all pte is unmap */
+ kmem_cache_free(data->ptekmem, pte);
+ *pgd = 0;
+ tlb->flush_pgtable(pgd, sizeof(*pgd), data->iop.cookie);
+}
+
+static arm_short_iopte
+__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
+{
+ arm_short_iopte pteprot;
+
+ pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
+ pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
+ ARM_SHORT_PTE_TYPE_SMALL;
+ if (prot & IOMMU_CACHE)
+ pteprot |= ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
+ if (prot & IOMMU_WRITE)
+ pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
+ ARM_SHORT_PTE_SMALL_TEX0;
+ if (prot & IOMMU_NOEXEC)
+ pteprot |= large ? ARM_SHORT_PTE_LARGE_XN :
+ ARM_SHORT_PTE_SMALL_XN;
+ return pteprot;
+}
+
+static arm_short_iopte
+__arm_short_pgd_prot(struct arm_short_io_pgtable *data, int prot, bool super)
+{
+ arm_short_iopte pgdprot;
+
+ pgdprot = ARM_SHORT_PGD_S | ARM_SHORT_PGD_nG;
+ pgdprot |= super ? ARM_SHORT_PGD_TYPE_SUPERSECTION :
+ ARM_SHORT_PGD_TYPE_SECTION;
+ if (prot & IOMMU_CACHE)
+ pgdprot |= ARM_SHORT_PGD_C | ARM_SHORT_PGD_B;
+ if (prot & IOMMU_WRITE)
+ pgdprot |= ARM_SHORT_PGD_TEX0;
+ if (prot & IOMMU_NOEXEC)
+ pgdprot |= ARM_SHORT_PGD_SECTION_XN;
+ if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
+ pgdprot |= ARM_SHORT_PGD_SECTION_NS;
+ return pgdprot;
+}
+
+static arm_short_iopte
+__arm_short_pte_prot_split(struct arm_short_io_pgtable *data,
+ arm_short_iopte pgdprot,
+ arm_short_iopte pteprot_large,
+ bool large)
+{
+ arm_short_iopte pteprot = 0;
+
+ pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
+ pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
+ ARM_SHORT_PTE_TYPE_SMALL;
+ /* section to pte prot */
+ if (pgdprot & ARM_SHORT_PGD_C)
+ pteprot |= ARM_SHORT_PTE_C;
+ if (pgdprot & ARM_SHORT_PGD_B)
+ pteprot |= ARM_SHORT_PTE_B;
+ if (pgdprot & ARM_SHORT_PGD_TEX0)
+ pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
+ ARM_SHORT_PTE_SMALL_TEX0;
+ if (pgdprot & ARM_SHORT_PGD_nG)
+ pteprot |= ARM_SHORT_PTE_nG;
+ if (pgdprot & ARM_SHORT_PGD_SECTION_XN)
+ pteprot |= large ? ARM_SHORT_PTE_LARGE_XN :
+ ARM_SHORT_PTE_SMALL_XN;
+
+ /* large page to small page pte prot. Only large page may split */
+ if (pteprot_large && !large) {
+ if (pteprot_large & ARM_SHORT_PTE_LARGE_TEX0)
+ pteprot |= ARM_SHORT_PTE_SMALL_TEX0;
+ if (pteprot_large & ARM_SHORT_PTE_LARGE_XN)
+ pteprot |= ARM_SHORT_PTE_SMALL_XN;
+ }
+ return pteprot;
+}
+
+static arm_short_iopte
+__arm_short_pgtable_prot(struct arm_short_io_pgtable *data, bool noexec)
+{
+ arm_short_iopte pgdprot = 0;
+
+ pgdprot = ARM_SHORT_PGD_TYPE_PGTABLE;
+ if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
+ pgdprot |= ARM_SHORT_PGD_PGTABLE_NS;
+ if (noexec)
+ pgdprot |= ARM_SHORT_PGD_PGTABLE_XN;
+ return pgdprot;
+}
+
+static int
+_arm_short_map(struct arm_short_io_pgtable *data,
+ unsigned int iova, phys_addr_t paddr,
+ arm_short_iopte pgdprot, arm_short_iopte pteprot,
+ bool large)
+{
+ const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
+ arm_short_iopte *pgd = data->pgd, *pte;
+ void *cookie = data->iop.cookie, *pte_va;
+ unsigned int ptenr = large ? 16 : 1;
+ int i, quirk = data->iop.cfg.quirks;
+ bool ptenew = false;
+
+ pgd += ARM_SHORT_PGD_IDX(iova);
+
+ if (!pteprot) { /* section or supersection */
+ if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
+ pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
+ pte = pgd;
+ pteprot = pgdprot;
+ } else { /* page or largepage */
+ if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
+ if (large) { /* special Bit */
+ if (pteprot & ARM_SHORT_PTE_LARGE_TEX0) {
+ pteprot &= ~ARM_SHORT_PTE_LARGE_TEX0;
+ pteprot |= ARM_SHORT_PTE_SMALL_TEX0;
+ }
+ pteprot &= ~ARM_SHORT_PTE_LARGE_XN;
+ } else {
+ pteprot &= ~ARM_SHORT_PTE_SMALL_XN;
+ }
+ }
+
+ if (!(*pgd)) {
+ pte_va = kmem_cache_zalloc(data->ptekmem, GFP_ATOMIC);
+ if (unlikely(!pte_va))
+ return -ENOMEM;
+ ptenew = true;
+ *pgd = virt_to_phys(pte_va) | pgdprot;
+ kmemleak_ignore(pte_va);
+ tlb->flush_pgtable(pgd, sizeof(*pgd), cookie);
+ }
+ pte = arm_short_get_pte_in_pgd(*pgd, iova);
+ }
+
+ pteprot |= (arm_short_iopte)paddr;
+ for (i = 0; i < ptenr; i++) {
+ if (pte[i]) {/* Someone else may have allocated for this pte */
+ WARN_ON(!selftest_running);
+ goto err_exist_pte;
+ }
+ pte[i] = pteprot;
+ }
+ tlb->flush_pgtable(pte, ptenr * sizeof(*pte), cookie);
+
+ return 0;
+
+err_exist_pte:
+ while (i--)
+ pte[i] = 0;
+ if (ptenew)
+ kmem_cache_free(data->ptekmem, pte_va);
+ return -EEXIST;
+}
+
+static int arm_short_map(struct io_pgtable_ops *ops, unsigned long iova,
+ phys_addr_t paddr, size_t size, int prot)
+{
+ struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
+ const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
+ int ret;
+ arm_short_iopte pgdprot = 0, pteprot = 0;
+ bool large;
+
+ /* If no access, then nothing to do */
+ if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
+ return 0;
+
+ switch (size) {
+ case SZ_4K:
+ case SZ_64K:
+ large = (size == SZ_64K) ? true : false;
+ pteprot = __arm_short_pte_prot(data, prot, large);
+ pgdprot = __arm_short_pgtable_prot(data, prot & IOMMU_NOEXEC);
+ break;
+
+ case SZ_1M:
+ case SZ_16M:
+ large = (size == SZ_16M) ? true : false;
+ pgdprot = __arm_short_pgd_prot(data, prot, large);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (WARN_ON((iova | paddr) & (size - 1)))
+ return -EINVAL;
+
+ ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
+
+ tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
+ tlb->tlb_sync(data->iop.cookie);
+ return ret;
+}
+
+static phys_addr_t arm_short_iova_to_phys(struct io_pgtable_ops *ops,
+ unsigned long iova)
+{
+ struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
+ arm_short_iopte *pte, *pgd = data->pgd;
+ phys_addr_t pa = 0;
+
+ pgd += ARM_SHORT_PGD_IDX(iova);
+
+ if (ARM_SHORT_PGD_TYPE_IS_PGTABLE(*pgd)) {
+ pte = arm_short_get_pte_in_pgd(*pgd, iova);
+
+ if (ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(*pte)) {
+ pa = (*pte) & ARM_SHORT_PTE_LARGE_MSK;
+ pa |= iova & ~ARM_SHORT_PTE_LARGE_MSK;
+ } else if (ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(*pte)) {
+ pa = (*pte) & ARM_SHORT_PTE_SMALL_MSK;
+ pa |= iova & ~ARM_SHORT_PTE_SMALL_MSK;
+ }
+ } else if (ARM_SHORT_PGD_TYPE_IS_SECTION(*pgd)) {
+ pa = (*pgd) & ARM_SHORT_PGD_SECTION_MSK;
+ pa |= iova & ~ARM_SHORT_PGD_SECTION_MSK;
+ } else if (ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(*pgd)) {
+ pa = (*pgd) & ARM_SHORT_PGD_SUPERSECTION_MSK;
+ pa |= iova & ~ARM_SHORT_PGD_SUPERSECTION_MSK;
+ }
+
+ return pa;
+}
+
+static int
+arm_short_split_blk_unmap(struct io_pgtable_ops *ops, unsigned int iova,
+ phys_addr_t paddr, size_t size,
+ arm_short_iopte pgdprotup, arm_short_iopte pteprotup,
+ size_t blk_size)
+{
+ struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
+ const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
+ struct io_pgtable_cfg *cfg = &data->iop.cfg;
+ unsigned long *pgbitmap = &cfg->pgsize_bitmap;
+ unsigned int blk_base, blk_start, blk_end;
+ arm_short_iopte pgdprot, pteprot;
+ size_t mapsize = 0, nextmapsize;
+ phys_addr_t blk_paddr;
+ int ret;
+ unsigned int i;
+
+ /* find the nearest mapsize */
+ for (i = find_first_bit(pgbitmap, BITS_PER_LONG);
+ i < BITS_PER_LONG && ((1 << i) < blk_size) &&
+ IS_ALIGNED(size, 1 << i);
+ i = find_next_bit(pgbitmap, BITS_PER_LONG, i + 1))
+ mapsize = 1 << i;
+
+ if (WARN_ON(!mapsize))
+ return 0; /* Bytes unmapped */
+ nextmapsize = 1 << i;
+
+ blk_base = iova & ~(blk_size - 1);
+ blk_start = blk_base;
+ blk_end = blk_start + blk_size;
+ blk_paddr = paddr;
+
+ for (; blk_start < blk_end;
+ blk_start += mapsize, blk_paddr += mapsize) {
+ /* Unmap! */
+ if (blk_start == iova)
+ continue;
+
+ /* Try to upper map */
+ if (blk_base != blk_start &&
+ IS_ALIGNED(blk_start | blk_paddr, nextmapsize) &&
+ mapsize != nextmapsize) {
+ mapsize = nextmapsize;
+ i = find_next_bit(pgbitmap, BITS_PER_LONG, i + 1);
+ if (i < BITS_PER_LONG)
+ nextmapsize = 1 << i;
+ }
+
+ if (mapsize == SZ_1M) {
+ pgdprot = pgdprotup;
+ pgdprot |= __arm_short_pgd_prot(data, 0, false);
+ pteprot = 0;
+ } else { /* small or large page */
+ bool noexec = (blk_size == SZ_64K) ?
+ (pteprotup & ARM_SHORT_PTE_LARGE_XN) :
+ (pgdprotup & ARM_SHORT_PGD_SECTION_XN);
+
+ pteprot = __arm_short_pte_prot_split(
+ data, pgdprotup, pteprotup,
+ mapsize == SZ_64K);
+ pgdprot = __arm_short_pgtable_prot(data, noexec);
+ }
+
+ ret = _arm_short_map(data, blk_start, blk_paddr, pgdprot,
+ pteprot, mapsize == SZ_64K);
+ if (ret < 0) {
+ /* Free the table we allocated */
+ arm_short_iopte *pgd = data->pgd, *pte;
+
+ pgd += ARM_SHORT_PGD_IDX(blk_base);
+ if (*pgd) {
+ pte = ARM_SHORT_GET_PTE_VA(*pgd);
+ kmem_cache_free(data->ptekmem, pte);
+ *pgd = 0;
+ tlb->flush_pgtable(pgd, sizeof(*pgd),
+ data->iop.cookie);
+ }
+ return 0;/* Bytes unmapped */
+ }
+ tlb->tlb_add_flush(blk_start, mapsize, true, data->iop.cookie);
+ tlb->tlb_sync(data->iop.cookie);
+ }
+
+ return size;
+}
+
+static int arm_short_unmap(struct io_pgtable_ops *ops,
+ unsigned long iova,
+ size_t size)
+{
+ struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
+ const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
+ void *cookie = data->iop.cookie;
+ arm_short_iopte *pgd, *pte = NULL;
+ arm_short_iopte pgdprot, pteprot = 0;
+ phys_addr_t paddr;
+ unsigned int nrtoclean, iova_base, blk_size = 0;
+
+ pgd = (arm_short_iopte *)data->pgd + ARM_SHORT_PGD_IDX(iova);
+
+ /* get block size */
+ if (ARM_SHORT_PGD_TYPE_IS_PGTABLE(*pgd)) {
+ pte = arm_short_get_pte_in_pgd(*pgd, iova);
+
+ if (ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(*pte))
+ blk_size = SZ_4K;
+ else if (ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(*pte))
+ blk_size = SZ_64K;
+ else
+ WARN_ON(1);
+ } else if (ARM_SHORT_PGD_TYPE_IS_SECTION(*pgd)) {
+ blk_size = SZ_1M;
+ } else if (ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(*pgd)) {
+ blk_size = SZ_16M;
+ } else {
+ WARN_ON(1);
+ }
+
+ iova_base = iova & ~(blk_size - 1);
+ pgd = (arm_short_iopte *)data->pgd + ARM_SHORT_PGD_IDX(iova_base);
+ paddr = arm_short_iova_to_phys(ops, iova_base);
+ pgdprot = *pgd;
+
+ if (blk_size == SZ_4K || blk_size == SZ_64K) {
+ pte = arm_short_get_pte_in_pgd(*pgd, iova_base);
+ pteprot = *pte;
+ nrtoclean = blk_size / SZ_4K;
+ memset(pte, 0, nrtoclean * sizeof(*pte));
+ tlb->flush_pgtable(pte, nrtoclean * sizeof(*pte), cookie);
+
+ _arm_short_free_pgtable(data, pgd);
+ } else if (blk_size == SZ_1M || blk_size == SZ_16M) {
+ nrtoclean = blk_size / SZ_1M;
+ memset(pgd, 0, nrtoclean * sizeof(*pgd));
+ tlb->flush_pgtable(pgd, nrtoclean * sizeof(*pgd), cookie);
+ }
+
+ tlb->tlb_add_flush(iova, blk_size, true, cookie);
+ tlb->tlb_sync(cookie);
+
+ if (blk_size > size) { /* Split the block */
+ return arm_short_split_blk_unmap(
+ ops, iova, paddr, size,
+ ARM_SHORT_PGD_GET_PROT(pgdprot),
+ ARM_SHORT_PTE_LARGE_GET_PROT(pteprot),
+ blk_size);
+ } else if (blk_size < size) {
+ /* Unmap the block while remap partial again after split */
+ return blk_size +
+ arm_short_unmap(ops, iova + blk_size, size - blk_size);
+ }
+
+ return size;
+}
+
+static struct io_pgtable *
+arm_short_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
+{
+ struct arm_short_io_pgtable *data;
+
+ if (cfg->ias > 32)
+ return NULL;
+
+ if (cfg->oas > 32)
+ return NULL;
+
+ cfg->pgsize_bitmap &=
+ (cfg->quirks & IO_PGTABLE_QUIRK_SHORT_SUPERSECTION) ?
+ (SZ_4K | SZ_64K | SZ_1M | SZ_16M) : (SZ_4K | SZ_64K | SZ_1M);
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return NULL;
+
+ data->pgd_size = SZ_16K;
+ data->pgd = alloc_pages_exact(data->pgd_size,
+ GFP_KERNEL | __GFP_ZERO | __GFP_DMA);
+ if (!data->pgd)
+ goto out_free_data;
+
+ cfg->tlb->flush_pgtable(data->pgd, data->pgd_size, cookie);
+
+ data->ptekmem = kmem_cache_create("io-pgtable-arm-short",
+ ARM_SHORT_BYTES_PER_PTE,
+ ARM_SHORT_BYTES_PER_PTE,
+ 0, NULL);
+ if (!data->ptekmem)
+ goto out_free_pte;
+
+ /* TTBRs */
+ cfg->arm_short_cfg.ttbr[0] = virt_to_phys(data->pgd);
+ cfg->arm_short_cfg.ttbr[1] = 0;
+
+ cfg->arm_short_cfg.tcr = 0;
+ cfg->arm_short_cfg.nmrr = 0;
+ cfg->arm_short_cfg.prrr = 0;
+
+ data->iop.ops = (struct io_pgtable_ops) {
+ .map = arm_short_map,
+ .unmap = arm_short_unmap,
+ .iova_to_phys = arm_short_iova_to_phys,
+ };
+
+ return &data->iop;
+
+out_free_pte:
+ free_pages_exact(data->pgd, data->pgd_size);
+out_free_data:
+ kfree(data);
+ return NULL;
+}
+
+static void arm_short_free_pgtable(struct io_pgtable *iop)
+{
+ struct arm_short_io_pgtable *data = io_pgtable_to_data(iop);
+
+ kmem_cache_destroy(data->ptekmem);
+ free_pages_exact(data->pgd, data->pgd_size);
+ kfree(data);
+}
+
+struct io_pgtable_init_fns io_pgtable_arm_short_init_fns = {
+ .alloc = arm_short_alloc_pgtable,
+ .free = arm_short_free_pgtable,
+};
+
+#ifdef CONFIG_IOMMU_IO_PGTABLE_SHORT_SELFTEST
+
+static struct io_pgtable_cfg *cfg_cookie;
+
+static void dummy_tlb_flush_all(void *cookie)
+{
+ WARN_ON(cookie != cfg_cookie);
+}
+
+static void dummy_tlb_add_flush(unsigned long iova, size_t size, bool leaf,
+ void *cookie)
+{
+ WARN_ON(cookie != cfg_cookie);
+ WARN_ON(!(size & cfg_cookie->pgsize_bitmap));
+}
+
+static void dummy_tlb_sync(void *cookie)
+{
+ WARN_ON(cookie != cfg_cookie);
+}
+
+static void dummy_flush_pgtable(void *ptr, size_t size, void *cookie)
+{
+ WARN_ON(cookie != cfg_cookie);
+}
+
+static struct iommu_gather_ops dummy_tlb_ops = {
+ .tlb_flush_all = dummy_tlb_flush_all,
+ .tlb_add_flush = dummy_tlb_add_flush,
+ .tlb_sync = dummy_tlb_sync,
+ .flush_pgtable = dummy_flush_pgtable,
+};
+
+#define __FAIL(ops) ({ \
+ WARN(1, "selftest: test failed\n"); \
+ selftest_running = false; \
+ -EFAULT; \
+})
+
+static int __init arm_short_do_selftests(void)
+{
+ struct io_pgtable_ops *ops;
+ struct io_pgtable_cfg cfg = {
+ .tlb = &dummy_tlb_ops,
+ .oas = 32,
+ .ias = 32,
+ .quirks = IO_PGTABLE_QUIRK_ARM_NS |
+ IO_PGTABLE_QUIRK_SHORT_SUPERSECTION,
+ .pgsize_bitmap = SZ_4K | SZ_64K | SZ_1M | SZ_16M,
+ };
+ unsigned int iova, size, iova_start;
+ unsigned int i, loopnr = 0;
+
+ selftest_running = true;
+
+ cfg_cookie = &cfg;
+
+ ops = alloc_io_pgtable_ops(ARM_SHORT_DESC, &cfg, &cfg);
+ if (!ops) {
+ pr_err("Failed to alloc short desc io pgtable\n");
+ return -EINVAL;
+ }
+
+ /*
+ * Initial sanity checks.
+ * Empty page tables shouldn't provide any translations.
+ */
+ if (ops->iova_to_phys(ops, 42))
+ return __FAIL(ops);
+
+ if (ops->iova_to_phys(ops, SZ_1G + 42))
+ return __FAIL(ops);
+
+ if (ops->iova_to_phys(ops, SZ_2G + 42))
+ return __FAIL(ops);
+
+ /*
+ * Distinct mappings of different granule sizes.
+ */
+ iova = 0;
+ i = find_first_bit(&cfg.pgsize_bitmap, BITS_PER_LONG);
+ while (i != BITS_PER_LONG) {
+ size = 1UL << i;
+ if (ops->map(ops, iova, iova, size, IOMMU_READ |
+ IOMMU_WRITE |
+ IOMMU_NOEXEC |
+ IOMMU_CACHE))
+ return __FAIL(ops);
+
+ /* Overlapping mappings */
+ if (!ops->map(ops, iova, iova + size, size,
+ IOMMU_READ | IOMMU_NOEXEC))
+ return __FAIL(ops);
+
+ if (ops->iova_to_phys(ops, iova + 42) != (iova + 42))
+ return __FAIL(ops);
+
+ iova += SZ_16M;
+ i++;
+ i = find_next_bit(&cfg.pgsize_bitmap, BITS_PER_LONG, i);
+ loopnr++;
+ }
+
+ /* Partial unmap */
+ i = 1;
+ size = 1UL << __ffs(cfg.pgsize_bitmap);
+ while (i < loopnr) {
+ iova_start = i * SZ_16M;
+ if (ops->unmap(ops, iova_start + size, size) != size)
+ return __FAIL(ops);
+
+ /* Remap of partial unmap */
+ if (ops->map(ops, iova_start + size, size, size, IOMMU_READ))
+ return __FAIL(ops);
+
+ if (ops->iova_to_phys(ops, iova_start + size + 42)
+ != (size + 42))
+ return __FAIL(ops);
+ i++;
+ }
+
+ /* Full unmap */
+ iova = 0;
+ i = find_first_bit(&cfg.pgsize_bitmap, BITS_PER_LONG);
+ while (i != BITS_PER_LONG) {
+ size = 1UL << i;
+
+ if (ops->unmap(ops, iova, size) != size)
+ return __FAIL(ops);
+
+ if (ops->iova_to_phys(ops, iova + 42))
+ return __FAIL(ops);
+
+ /* Remap full block */
+ if (ops->map(ops, iova, iova, size, IOMMU_WRITE))
+ return __FAIL(ops);
+
+ if (ops->iova_to_phys(ops, iova + 42) != (iova + 42))
+ return __FAIL(ops);
+
+ iova += SZ_16M;
+ i++;
+ i = find_next_bit(&cfg.pgsize_bitmap, BITS_PER_LONG, i);
+ }
+
+ free_io_pgtable_ops(ops);
+
+ selftest_running = false;
+ return 0;
+}
+subsys_initcall(arm_short_do_selftests);
+#endif
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 4e46021..13aad17 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -36,9 +36,6 @@
#define io_pgtable_to_data(x) \
container_of((x), struct arm_lpae_io_pgtable, iop)
-#define io_pgtable_ops_to_pgtable(x) \
- container_of((x), struct io_pgtable, ops)
-
#define io_pgtable_ops_to_data(x) \
io_pgtable_to_data(io_pgtable_ops_to_pgtable(x))
diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
index 6436fe2..14a9b3a 100644
--- a/drivers/iommu/io-pgtable.c
+++ b/drivers/iommu/io-pgtable.c
@@ -28,6 +28,7 @@ extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns;
extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns;
extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns;
extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
+extern struct io_pgtable_init_fns io_pgtable_arm_short_init_fns;
static const struct io_pgtable_init_fns *
io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] =
@@ -38,6 +39,9 @@ io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] =
[ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns,
[ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns,
#endif
+#ifdef CONFIG_IOMMU_IO_PGTABLE_SHORT
+ [ARM_SHORT_DESC] = &io_pgtable_arm_short_init_fns,
+#endif
};
struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
index 10e32f6..7261ada 100644
--- a/drivers/iommu/io-pgtable.h
+++ b/drivers/iommu/io-pgtable.h
@@ -9,6 +9,7 @@ enum io_pgtable_fmt {
ARM_32_LPAE_S2,
ARM_64_LPAE_S1,
ARM_64_LPAE_S2,
+ ARM_SHORT_DESC,
IO_PGTABLE_NUM_FMTS,
};
@@ -44,6 +45,8 @@ struct iommu_gather_ops {
*/
struct io_pgtable_cfg {
#define IO_PGTABLE_QUIRK_ARM_NS (1 << 0) /* Set NS bit in PTEs */
+ #define IO_PGTABLE_QUIRK_SHORT_SUPERSECTION BIT(1)
+ #define IO_PGTABLE_QUIRK_SHORT_MTK BIT(2)
int quirks;
unsigned long pgsize_bitmap;
unsigned int ias;
@@ -62,6 +65,13 @@ struct io_pgtable_cfg {
u64 vttbr;
u64 vtcr;
} arm_lpae_s2_cfg;
+
+ struct {
+ u32 ttbr[2];
+ u32 tcr;
+ u32 nmrr;
+ u32 prrr;
+ } arm_short_cfg;
};
};
@@ -128,6 +138,9 @@ struct io_pgtable {
struct io_pgtable_ops ops;
};
+#define io_pgtable_ops_to_pgtable(x) \
+ container_of((x), struct io_pgtable, ops)
+
/**
* struct io_pgtable_init_fns - Alloc/free a set of page tables for a
* particular format.
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 4/6] memory: mediatek: Add SMI driver
2015-07-16 9:04 [PATCH v3 0/6] MT8173 IOMMU SUPPORT Yong Wu
` (2 preceding siblings ...)
2015-07-16 9:04 ` [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator Yong Wu
@ 2015-07-16 9:04 ` Yong Wu
2015-07-16 9:04 ` [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu
2015-07-16 9:04 ` [PATCH v3 6/6] dts: mt8173: Add iommu/smi nodes for mt8173 Yong Wu
5 siblings, 0 replies; 34+ messages in thread
From: Yong Wu @ 2015-07-16 9:04 UTC (permalink / raw)
To: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger
Cc: Robin Murphy, Will Deacon, Daniel Kurtz, Tomasz Figa,
Lucas Stach, Rob Herring, Catalin Marinas, linux-mediatek,
Sasha Hauer, srv_heupstream, devicetree, linux-kernel,
linux-arm-kernel, iommu, pebolle, arnd, mitchelh, cloud.chou,
frederic.chen, yong.wu
This patch add SMI(Smart Multimedia Interface) driver. This driver
is responsible to enable/disable iommu and control the clocks of each
local arbiter
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
drivers/memory/Kconfig | 8 ++
drivers/memory/Makefile | 1 +
drivers/memory/mtk-smi.c | 285 ++++++++++++++++++++++++++++++++++++++++++++
include/soc/mediatek/smi.h | 60 ++++++++++
4 files changed, 354 insertions(+)
create mode 100644 drivers/memory/mtk-smi.c
create mode 100644 include/soc/mediatek/smi.h
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 8406c668..c0e1607 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -100,6 +100,14 @@ config JZ4780_NEMC
the Ingenic JZ4780. This controller is used to handle external
memory devices such as NAND and SRAM.
+config MTK_SMI
+ bool
+ depends on ARCH_MEDIATEK || COMPILE_TEST
+ help
+ This driver is for the Memory Controller module in MediaTek SoCs,
+ mainly help enable/disable iommu and control the clock for each
+ local arbiter.
+
source "drivers/memory/tegra/Kconfig"
endif
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index b670441..f854e40 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -14,5 +14,6 @@ obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o
obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
obj-$(CONFIG_JZ4780_NEMC) += jz4780-nemc.o
+obj-$(CONFIG_MTK_SMI) += mtk-smi.o
obj-$(CONFIG_TEGRA_MC) += tegra/
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
new file mode 100644
index 0000000..0917d6c
--- /dev/null
+++ b/drivers/memory/mtk-smi.c
@@ -0,0 +1,285 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu <yong.wu@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <soc/mediatek/smi.h>
+
+#define SMI_LARB_MMU_EN 0xf00
+#define F_SMI_MMU_EN(port) BIT(port)
+
+struct mtk_smi_common {
+ struct clk *clk_apb;
+ struct clk *clk_smi;
+};
+
+struct mtk_smi_larb {
+ void __iomem *base;
+ struct clk *clk_apb;
+ struct clk *clk_smi;
+ struct device *smi;
+};
+
+struct mtk_larb_mmu {
+ u32 mmu;
+};
+
+static int mtk_smi_common_get(struct device *smidev)
+{
+ struct mtk_smi_common *smipriv = dev_get_drvdata(smidev);
+ int ret;
+
+ ret = pm_runtime_get_sync(smidev);
+ if (ret < 0)
+ return ret;
+
+ ret = clk_prepare_enable(smipriv->clk_apb);
+ if (ret) {
+ dev_err(smidev, "Failed to enable the apb clock\n");
+ goto err_put_pm;
+ }
+ ret = clk_prepare_enable(smipriv->clk_smi);
+ if (ret) {
+ dev_err(smidev, "Failed to enable the smi clock\n");
+ goto err_disable_apb;
+ }
+ return ret;
+
+err_disable_apb:
+ clk_disable_unprepare(smipriv->clk_apb);
+err_put_pm:
+ pm_runtime_put(smidev);
+ return ret;
+}
+
+static void mtk_smi_common_put(struct device *smidev)
+{
+ struct mtk_smi_common *smipriv = dev_get_drvdata(smidev);
+
+ clk_disable_unprepare(smipriv->clk_smi);
+ clk_disable_unprepare(smipriv->clk_apb);
+ pm_runtime_put(smidev);
+}
+
+int mtk_smi_larb_get(struct device *larbdev)
+{
+ struct mtk_smi_larb *larbpriv = dev_get_drvdata(larbdev);
+ struct mtk_larb_mmu *mmucfg = larbdev->archdata.iommu;
+ int ret;
+
+ ret = mtk_smi_common_get(larbpriv->smi);
+ if (ret)
+ return ret;
+
+ ret = pm_runtime_get_sync(larbdev);
+ if (ret < 0)
+ goto err_put_smicommon;
+
+ ret = clk_prepare_enable(larbpriv->clk_apb);
+ if (ret) {
+ dev_err(larbdev, "Failed to enable the apb clock\n");
+ goto err_put_pm;
+ }
+
+ ret = clk_prepare_enable(larbpriv->clk_smi);
+ if (ret) {
+ dev_err(larbdev, "Failed to enable the smi clock\n");
+ goto err_disable_apb;
+ }
+
+ /* config iommu */
+ writel(mmucfg->mmu, larbpriv->base + SMI_LARB_MMU_EN);
+
+ return ret;
+
+err_disable_apb:
+ clk_disable_unprepare(larbpriv->clk_apb);
+err_put_pm:
+ pm_runtime_put(larbdev);
+err_put_smicommon:
+ mtk_smi_common_put(larbpriv->smi);
+ return ret;
+}
+
+void mtk_smi_larb_put(struct device *larbdev)
+{
+ struct mtk_smi_larb *larbpriv = dev_get_drvdata(larbdev);
+
+ clk_disable_unprepare(larbpriv->clk_smi);
+ clk_disable_unprepare(larbpriv->clk_apb);
+ pm_runtime_put(larbdev);
+ mtk_smi_common_put(larbpriv->smi);
+}
+
+int mtk_smi_config_port(struct device *larbdev, unsigned int larbportid,
+ bool enable)
+{
+ struct mtk_larb_mmu *mmucfg = larbdev->archdata.iommu;
+
+ if (!mmucfg) {
+ mmucfg = kzalloc(sizeof(*mmucfg), GFP_KERNEL);
+ if (!mmucfg)
+ return -ENOMEM;
+ larbdev->archdata.iommu = mmucfg;
+ }
+
+ dev_dbg(larbdev, "%s iommu port id %d\n",
+ enable ? "enable" : "disable", larbportid);
+
+ if (enable)
+ mmucfg->mmu |= F_SMI_MMU_EN(larbportid);
+ else
+ mmucfg->mmu &= ~F_SMI_MMU_EN(larbportid);
+
+ return 0;
+}
+
+static int mtk_smi_larb_probe(struct platform_device *pdev)
+{
+ struct mtk_smi_larb *larbpriv;
+ struct resource *res;
+ struct device *dev = &pdev->dev;
+ struct device_node *smi_node;
+ struct platform_device *smi_pdev;
+
+ larbpriv = devm_kzalloc(dev, sizeof(*larbpriv), GFP_KERNEL);
+ if (!larbpriv)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ larbpriv->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(larbpriv->base))
+ return PTR_ERR(larbpriv->base);
+
+ larbpriv->clk_apb = devm_clk_get(dev, "apb");
+ if (IS_ERR(larbpriv->clk_apb))
+ return PTR_ERR(larbpriv->clk_apb);
+
+ larbpriv->clk_smi = devm_clk_get(dev, "smi");
+ if (IS_ERR(larbpriv->clk_smi))
+ return PTR_ERR(larbpriv->clk_smi);
+
+ smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0);
+ if (!smi_node)
+ return -EINVAL;
+
+ smi_pdev = of_find_device_by_node(smi_node);
+ of_node_put(smi_node);
+ if (smi_pdev) {
+ larbpriv->smi = &smi_pdev->dev;
+ } else {
+ dev_err(dev, "Failed to get the smi_common device\n");
+ return -EINVAL;
+ }
+
+ pm_runtime_enable(dev);
+ dev_set_drvdata(dev, larbpriv);
+ return 0;
+}
+
+static int mtk_smi_larb_remove(struct platform_device *pdev)
+{
+ struct mtk_larb_mmu *mmucfg = pdev->dev.archdata.iommu;
+
+ kfree(mmucfg);
+ pm_runtime_disable(&pdev->dev);
+ return 0;
+}
+
+static const struct of_device_id mtk_smi_larb_of_ids[] = {
+ { .compatible = "mediatek,mt8173-smi-larb",},
+ {}
+};
+
+static struct platform_driver mtk_smi_larb_driver = {
+ .probe = mtk_smi_larb_probe,
+ .remove = mtk_smi_larb_remove,
+ .driver = {
+ .name = "mtk-smi-larb",
+ .of_match_table = mtk_smi_larb_of_ids,
+ }
+};
+
+static int mtk_smi_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mtk_smi_common *smipriv;
+ int ret;
+
+ smipriv = devm_kzalloc(dev, sizeof(*smipriv), GFP_KERNEL);
+ if (!smipriv)
+ return -ENOMEM;
+
+ smipriv->clk_apb = devm_clk_get(dev, "apb");
+ if (IS_ERR(smipriv->clk_apb))
+ return PTR_ERR(smipriv->clk_apb);
+
+ smipriv->clk_smi = devm_clk_get(dev, "smi");
+ if (IS_ERR(smipriv->clk_smi))
+ return PTR_ERR(smipriv->clk_smi);
+
+ pm_runtime_enable(dev);
+ dev_set_drvdata(dev, smipriv);
+ return ret;
+}
+
+static int mtk_smi_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(&pdev->dev);
+ return 0;
+}
+
+static const struct of_device_id mtk_smi_of_ids[] = {
+ { .compatible = "mediatek,mt8173-smi",},
+ {}
+};
+
+static struct platform_driver mtk_smi_driver = {
+ .probe = mtk_smi_probe,
+ .remove = mtk_smi_remove,
+ .driver = {
+ .name = "mtk-smi",
+ .of_match_table = mtk_smi_of_ids,
+ }
+};
+
+static int __init mtk_smi_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&mtk_smi_driver);
+ if (ret != 0) {
+ pr_err("Failed to register SMI driver\n");
+ return ret;
+ }
+
+ ret = platform_driver_register(&mtk_smi_larb_driver);
+ if (ret != 0) {
+ pr_err("Failed to register SMI-LARB driver\n");
+ goto err_unreg_smi;
+ }
+ return ret;
+
+err_unreg_smi:
+ platform_driver_unregister(&mtk_smi_driver);
+ return ret;
+}
+
+subsys_initcall(mtk_smi_init);
diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
new file mode 100644
index 0000000..feb1180
--- /dev/null
+++ b/include/soc/mediatek/smi.h
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu <yong.wu@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#ifndef MTK_IOMMU_SMI_H
+#define MTK_IOMMU_SMI_H
+
+#include <linux/device.h>
+
+#ifdef CONFIG_MTK_SMI
+
+/*
+ * Record the iommu info for each port in the local arbiter.
+ * It is only for iommu.
+ *
+ * Returns 0 if successfully, others if failed.
+ */
+int mtk_smi_config_port(struct device *larbdev, unsigned int larbportid,
+ bool iommuen);
+/*
+ * The two function below config iommu and enable/disable the clock
+ * for the larb.
+ *
+ * mtk_smi_larb_get must be called before the multimedia HW work.
+ * mtk_smi_larb_put must be called after HW done.
+ * Both should be called in non-atomic context.
+ *
+ * Returns 0 if successfully, others if failed.
+ */
+int mtk_smi_larb_get(struct device *larbdev);
+void mtk_smi_larb_put(struct device *larbdev);
+
+#else
+
+static int
+mtk_smi_config_port(struct device *larbdev, unsigned int larbportid,
+ bool iommuen)
+{
+ return 0;
+}
+
+static inline int mtk_smi_larb_get(struct device *larbdev)
+{
+ return 0;
+}
+
+static inline void mtk_smi_larb_put(struct device *larbdev) { }
+
+#endif
+
+#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-16 9:04 [PATCH v3 0/6] MT8173 IOMMU SUPPORT Yong Wu
` (3 preceding siblings ...)
2015-07-16 9:04 ` [PATCH v3 4/6] memory: mediatek: Add SMI driver Yong Wu
@ 2015-07-16 9:04 ` Yong Wu
2015-07-21 14:59 ` Will Deacon
2015-07-27 13:23 ` Robin Murphy
2015-07-16 9:04 ` [PATCH v3 6/6] dts: mt8173: Add iommu/smi nodes for mt8173 Yong Wu
5 siblings, 2 replies; 34+ messages in thread
From: Yong Wu @ 2015-07-16 9:04 UTC (permalink / raw)
To: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger
Cc: Robin Murphy, Will Deacon, Daniel Kurtz, Tomasz Figa,
Lucas Stach, Rob Herring, Catalin Marinas, linux-mediatek,
Sasha Hauer, srv_heupstream, devicetree, linux-kernel,
linux-arm-kernel, iommu, pebolle, arnd, mitchelh, cloud.chou,
frederic.chen, yong.wu
This patch adds support for mediatek m4u (MultiMedia Memory Management
Unit).
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
drivers/iommu/Kconfig | 13 +
drivers/iommu/Makefile | 1 +
drivers/iommu/mtk_iommu.c | 724 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 738 insertions(+)
create mode 100644 drivers/iommu/mtk_iommu.c
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f50dbf3..3178c12 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -386,4 +386,17 @@ config ARM_SMMU_V3
Say Y here if your system includes an IOMMU device implementing
the ARM SMMUv3 architecture.
+config MTK_IOMMU
+ bool "MTK IOMMU Support"
+ depends on ARCH_MEDIATEK || COMPILE_TEST
+ select IOMMU_API
+ select IOMMU_DMA
+ select IOMMU_IO_PGTABLE_SHORT
+ select MEMORY
+ select MTK_SMI
+ help
+ Support for the IOMMUs on certain Mediatek SOCs.
+
+ If unsure, say N here.
+
endif # IOMMU_SUPPORT
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 06df3e6..f4f2f2c 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_ROCKCHIP_IOMMU) += rockchip-iommu.o
obj-$(CONFIG_TEGRA_IOMMU_GART) += tegra-gart.o
obj-$(CONFIG_TEGRA_IOMMU_SMMU) += tegra-smmu.o
obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
+obj-$(CONFIG_MTK_IOMMU) += mtk_iommu.o
obj-$(CONFIG_SHMOBILE_IOMMU) += shmobile-iommu.o
obj-$(CONFIG_SHMOBILE_IPMMU) += shmobile-ipmmu.o
obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
new file mode 100644
index 0000000..c1c90e6
--- /dev/null
+++ b/drivers/iommu/mtk_iommu.c
@@ -0,0 +1,724 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu <yong.wu@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/iommu.h>
+#include <linux/dma-mapping.h>
+#include <linux/dma-iommu.h>
+#include <linux/of_iommu.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/list.h>
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <asm/cacheflush.h>
+#include <soc/mediatek/smi.h>
+
+#include "io-pgtable.h"
+
+#define REG_MMU_PT_BASE_ADDR 0x000
+
+#define REG_MMU_INVALIDATE 0x020
+#define F_ALL_INVLD 0x2
+#define F_MMU_INV_RANGE 0x1
+
+#define REG_MMU_INVLD_START_A 0x024
+#define REG_MMU_INVLD_END_A 0x028
+
+#define REG_MMU_INV_SEL 0x038
+#define F_INVLD_EN0 BIT(0)
+#define F_INVLD_EN1 BIT(1)
+
+#define REG_MMU_STANDARD_AXI_MODE 0x048
+#define REG_MMU_DCM_DIS 0x050
+
+#define REG_MMU_CTRL_REG 0x110
+#define F_MMU_PREFETCH_RT_REPLACE_MOD BIT(4)
+#define F_MMU_TF_PROTECT_SEL(prot) (((prot) & 0x3) << 5)
+#define F_COHERENCE_EN BIT(8)
+
+#define REG_MMU_IVRP_PADDR 0x114
+#define F_MMU_IVRP_PA_SET(pa) ((pa) >> 1)
+
+#define REG_MMU_INT_CONTROL0 0x120
+#define F_L2_MULIT_HIT_EN BIT(0)
+#define F_TABLE_WALK_FAULT_INT_EN BIT(1)
+#define F_PREETCH_FIFO_OVERFLOW_INT_EN BIT(2)
+#define F_MISS_FIFO_OVERFLOW_INT_EN BIT(3)
+#define F_PREFETCH_FIFO_ERR_INT_EN BIT(5)
+#define F_MISS_FIFO_ERR_INT_EN BIT(6)
+#define F_INT_L2_CLR_BIT BIT(12)
+
+#define REG_MMU_INT_MAIN_CONTROL 0x124
+#define F_INT_TRANSLATION_FAULT BIT(0)
+#define F_INT_MAIN_MULTI_HIT_FAULT BIT(1)
+#define F_INT_INVALID_PA_FAULT BIT(2)
+#define F_INT_ENTRY_REPLACEMENT_FAULT BIT(3)
+#define F_INT_TLB_MISS_FAULT BIT(4)
+#define F_INT_MISS_TRANSATION_FIFO_FAULT BIT(5)
+#define F_INT_PRETETCH_TRANSATION_FIFO_FAULT BIT(6)
+
+#define REG_MMU_CPE_DONE 0x12C
+
+#define REG_MMU_FAULT_ST1 0x134
+
+#define REG_MMU_FAULT_VA 0x13c
+#define F_MMU_FAULT_VA_MSK 0xfffff000
+#define F_MMU_FAULT_VA_WRITE_BIT BIT(1)
+#define F_MMU_FAULT_VA_LAYER_BIT BIT(0)
+
+#define REG_MMU_INVLD_PA 0x140
+#define REG_MMU_INT_ID 0x150
+#define F_MMU0_INT_ID_LARB_ID(a) (((a) >> 7) & 0x7)
+#define F_MMU0_INT_ID_PORT_ID(a) (((a) >> 2) & 0x1f)
+
+#define MTK_PROTECT_PA_ALIGN 128
+#define MTK_IOMMU_LARB_MAX_NR 8
+#define MTK_IOMMU_REG_NR 10
+
+struct mtk_iommu_data {
+ void __iomem *base;
+ int irq;
+ struct device *dev;
+ struct device *larbdev[MTK_IOMMU_LARB_MAX_NR];
+ struct clk *bclk;
+ phys_addr_t protect_base; /* protect memory base */
+ int larb_nr; /* local arbiter number */
+ u32 reg[MTK_IOMMU_REG_NR];
+};
+
+struct mtk_iommu_domain {
+ struct imu_pgd_t *pgd;
+ spinlock_t pgtlock; /* lock for modifying page table */
+
+ struct io_pgtable_cfg cfg;
+ struct io_pgtable_ops *iop;
+
+ struct mtk_iommu_data *data;
+ struct iommu_domain domain;
+};
+
+struct mtk_iommu_client_priv {
+ struct list_head client;
+ unsigned int larbid;
+ unsigned int portid;
+ struct device *imudev;
+};
+
+static struct iommu_ops mtk_iommu_ops;
+
+static struct mtk_iommu_domain *to_mtk_domain(struct iommu_domain *dom)
+{
+ return container_of(dom, struct mtk_iommu_domain, domain);
+}
+
+static void mtk_iommu_clear_intr(const struct mtk_iommu_data *data)
+{
+ u32 val;
+
+ val = readl(data->base + REG_MMU_INT_CONTROL0);
+ val |= F_INT_L2_CLR_BIT;
+ writel(val, data->base + REG_MMU_INT_CONTROL0);
+}
+
+static void mtk_iommu_tlb_flush_all(void *cookie)
+{
+ struct mtk_iommu_domain *domain = cookie;
+ void __iomem *base;
+
+ base = domain->data->base;
+ writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
+ writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
+}
+
+static void mtk_iommu_tlb_add_flush(unsigned long iova, size_t size,
+ bool leaf, void *cookie)
+{
+ struct mtk_iommu_domain *domain = cookie;
+ void __iomem *base = domain->data->base;
+ unsigned int iova_start = iova, iova_end = iova + size - 1;
+
+ writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
+
+ writel(iova_start, base + REG_MMU_INVLD_START_A);
+ writel(iova_end, base + REG_MMU_INVLD_END_A);
+ writel(F_MMU_INV_RANGE, base + REG_MMU_INVALIDATE);
+}
+
+static void mtk_iommu_tlb_sync(void *cookie)
+{
+ struct mtk_iommu_domain *domain = cookie;
+ void __iomem *base = domain->data->base;
+ int ret;
+ u32 tmp;
+
+ ret = readl_poll_timeout_atomic(base + REG_MMU_CPE_DONE, tmp,
+ tmp != 0, 10, 1000000);
+ if (ret) {
+ dev_warn(domain->data->dev,
+ "Partial TLB flush timed out, falling back to full flush\n");
+ mtk_iommu_tlb_flush_all(cookie);
+ }
+ writel(0, base + REG_MMU_CPE_DONE);
+}
+
+static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
+{
+ struct mtk_iommu_domain *domain = cookie;
+ unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
+
+ dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
+ size, DMA_TO_DEVICE);
+}
+
+static struct iommu_gather_ops mtk_iommu_gather_ops = {
+ .tlb_flush_all = mtk_iommu_tlb_flush_all,
+ .tlb_add_flush = mtk_iommu_tlb_add_flush,
+ .tlb_sync = mtk_iommu_tlb_sync,
+ .flush_pgtable = mtk_iommu_flush_pgtable,
+};
+
+static irqreturn_t mtk_iommu_isr(int irq, void *dev_id)
+{
+ struct mtk_iommu_domain *mtkdom = dev_id;
+ struct mtk_iommu_data *data = mtkdom->data;
+ u32 int_state, regval, fault_iova, fault_pa;
+ unsigned int fault_larb, fault_port;
+ bool layer, write;
+
+ int_state = readl(data->base + REG_MMU_FAULT_ST1);
+
+ /* read error info from registers */
+ fault_iova = readl(data->base + REG_MMU_FAULT_VA);
+ layer = fault_iova & F_MMU_FAULT_VA_LAYER_BIT;
+ write = fault_iova & F_MMU_FAULT_VA_WRITE_BIT;
+ fault_iova &= F_MMU_FAULT_VA_MSK;
+ fault_pa = readl(data->base + REG_MMU_INVLD_PA);
+ regval = readl(data->base + REG_MMU_INT_ID);
+ fault_larb = F_MMU0_INT_ID_LARB_ID(regval);
+ fault_port = F_MMU0_INT_ID_PORT_ID(regval);
+
+ if (report_iommu_fault(&mtkdom->domain, data->dev, fault_iova,
+ write ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ)) {
+ dev_err_ratelimited(
+ data->dev,
+ "fault type=0x%x iova=0x%x pa=0x%x larb=%d port=%d layer=%d %s\n",
+ int_state, fault_iova, fault_pa, fault_larb, fault_port,
+ layer, write ? "write" : "read");
+ }
+
+ mtk_iommu_tlb_flush_all(mtkdom);
+ mtk_iommu_clear_intr(data);
+
+ return IRQ_HANDLED;
+}
+
+static int mtk_iommu_parse_dt(struct platform_device *pdev,
+ struct mtk_iommu_data *data)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *ofnode;
+ struct resource *res;
+ int i;
+
+ ofnode = dev->of_node;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ data->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(data->base))
+ return PTR_ERR(data->base);
+
+ data->irq = platform_get_irq(pdev, 0);
+ if (data->irq < 0)
+ return data->irq;
+
+ data->bclk = devm_clk_get(dev, "bclk");
+ if (IS_ERR(data->bclk))
+ return PTR_ERR(data->bclk);
+
+ data->larb_nr = of_count_phandle_with_args(
+ ofnode, "mediatek,larb", NULL);
+ if (data->larb_nr < 0)
+ return data->larb_nr;
+
+ for (i = 0; i < data->larb_nr; i++) {
+ struct device_node *larbnode;
+ struct platform_device *plarbdev;
+
+ larbnode = of_parse_phandle(ofnode, "mediatek,larb", i);
+ if (!larbnode)
+ return -EINVAL;
+
+ plarbdev = of_find_device_by_node(larbnode);
+ of_node_put(larbnode);
+ if (!plarbdev)
+ return -EPROBE_DEFER;
+ data->larbdev[i] = &plarbdev->dev;
+ }
+
+ return 0;
+}
+
+static int mtk_iommu_hw_init(const struct mtk_iommu_domain *mtkdom)
+{
+ struct mtk_iommu_data *data = mtkdom->data;
+ void __iomem *base = data->base;
+ u32 regval;
+ int ret = 0;
+
+ ret = clk_prepare_enable(data->bclk);
+ if (ret) {
+ dev_err(data->dev, "Failed to enable iommu clk(%d)\n", ret);
+ return ret;
+ }
+
+ writel(mtkdom->cfg.arm_short_cfg.ttbr[0], base + REG_MMU_PT_BASE_ADDR);
+
+ regval = F_MMU_PREFETCH_RT_REPLACE_MOD |
+ F_MMU_TF_PROTECT_SEL(2) |
+ F_COHERENCE_EN;
+ writel(regval, base + REG_MMU_CTRL_REG);
+
+ regval = F_L2_MULIT_HIT_EN |
+ F_TABLE_WALK_FAULT_INT_EN |
+ F_PREETCH_FIFO_OVERFLOW_INT_EN |
+ F_MISS_FIFO_OVERFLOW_INT_EN |
+ F_PREFETCH_FIFO_ERR_INT_EN |
+ F_MISS_FIFO_ERR_INT_EN;
+ writel(regval, base + REG_MMU_INT_CONTROL0);
+
+ regval = F_INT_TRANSLATION_FAULT |
+ F_INT_MAIN_MULTI_HIT_FAULT |
+ F_INT_INVALID_PA_FAULT |
+ F_INT_ENTRY_REPLACEMENT_FAULT |
+ F_INT_TLB_MISS_FAULT |
+ F_INT_MISS_TRANSATION_FIFO_FAULT |
+ F_INT_PRETETCH_TRANSATION_FIFO_FAULT;
+ writel(regval, base + REG_MMU_INT_MAIN_CONTROL);
+
+ regval = ALIGN(data->protect_base, MTK_PROTECT_PA_ALIGN);
+ regval = F_MMU_IVRP_PA_SET(regval);
+ writel(regval, base + REG_MMU_IVRP_PADDR);
+
+ writel(0, base + REG_MMU_DCM_DIS);
+ writel(0, base + REG_MMU_STANDARD_AXI_MODE);
+
+ if (devm_request_irq(data->dev, data->irq, mtk_iommu_isr, 0,
+ dev_name(data->dev), (void *)mtkdom)) {
+ writel(0, base + REG_MMU_PT_BASE_ADDR);
+ clk_disable_unprepare(data->bclk);
+ dev_err(data->dev, "Failed @ IRQ-%d Request\n", data->irq);
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static int mtk_iommu_config(struct mtk_iommu_domain *mtkdom,
+ struct device *dev, bool enable)
+{
+ struct mtk_iommu_data *data = mtkdom->data;
+ struct mtk_iommu_client_priv *head, *cur, *next;
+
+ head = dev->archdata.iommu;
+ list_for_each_entry_safe(cur, next, &head->client, client) {
+ if (cur->larbid >= data->larb_nr) {
+ dev_err(data->dev, "Invalid larb:%d\n", cur->larbid);
+ return -EINVAL;
+ }
+
+ if (enable) {
+ mtk_smi_config_port(data->larbdev[cur->larbid],
+ cur->portid, true);
+ } else {
+ mtk_smi_config_port(data->larbdev[cur->larbid],
+ cur->portid, false);
+ list_del(&cur->client);
+ kfree(cur);
+ }
+ }
+
+ if (!enable)
+ kfree(head);
+ return 0;
+}
+
+static int mtk_iommu_init_domain_context(struct mtk_iommu_domain *dom)
+{
+ int ret;
+
+ if (dom->iop)
+ return 0;
+
+ spin_lock_init(&dom->pgtlock);
+ dom->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS |
+ IO_PGTABLE_QUIRK_SHORT_SUPERSECTION |
+ IO_PGTABLE_QUIRK_SHORT_MTK;
+ dom->cfg.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
+ dom->cfg.ias = 32;
+ dom->cfg.oas = 32;
+ dom->cfg.tlb = &mtk_iommu_gather_ops;
+
+ dom->iop = alloc_io_pgtable_ops(ARM_SHORT_DESC, &dom->cfg, dom);
+ if (!dom->iop) {
+ pr_err("Failed to alloc io pgtable\n");
+ return -EINVAL;
+ }
+
+ /* Update our support page sizes bitmap */
+ mtk_iommu_ops.pgsize_bitmap = dom->cfg.pgsize_bitmap;
+
+ ret = mtk_iommu_hw_init(dom);
+ if (ret)
+ free_io_pgtable_ops(dom->iop);
+
+ return ret;
+}
+
+static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
+{
+ struct mtk_iommu_domain *priv;
+
+ /* We only support unmanaged domains for now */
+ if (type != IOMMU_DOMAIN_UNMANAGED)
+ return NULL;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return NULL;
+
+ priv->domain.geometry.aperture_start = 0;
+ priv->domain.geometry.aperture_end = (1ULL << 32) - 1;
+ priv->domain.geometry.force_aperture = true;
+
+ return &priv->domain;
+}
+
+static void mtk_iommu_domain_free(struct iommu_domain *domain)
+{
+ kfree(to_mtk_domain(domain));
+}
+
+static int mtk_iommu_attach_device(struct iommu_domain *domain,
+ struct device *dev)
+{
+ struct mtk_iommu_domain *priv = to_mtk_domain(domain);
+ struct iommu_group *group;
+ struct mtk_iommu_client_priv *devpriv;
+ struct device *imudev;
+ int ret;
+
+ devpriv = dev->archdata.iommu;
+ imudev = devpriv->imudev;
+
+ /*
+ * Reserve one iommu domain as the m4u domain which
+ * all Multimedia modules share and free the others.
+ */
+ if (!imudev->archdata.iommu)
+ imudev->archdata.iommu = priv;
+ else if (imudev->archdata.iommu != priv)
+ iommu_domain_free(domain);
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return 0;
+ iommu_group_put(group);
+
+ ret = mtk_iommu_init_domain_context(priv);
+ if (ret)
+ return ret;
+
+ return mtk_iommu_config(priv, dev, true);;
+}
+
+static void mtk_iommu_detach_device(struct iommu_domain *domain,
+ struct device *dev)
+{
+ mtk_iommu_config(to_mtk_domain(domain), dev, false);
+}
+
+static int mtk_iommu_map(struct iommu_domain *domain, unsigned long iova,
+ phys_addr_t paddr, size_t size, int prot)
+{
+ struct mtk_iommu_domain *priv = to_mtk_domain(domain);
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&priv->pgtlock, flags);
+ ret = priv->iop->map(priv->iop, iova, paddr, size, prot);
+ spin_unlock_irqrestore(&priv->pgtlock, flags);
+
+ return ret;
+}
+
+static size_t mtk_iommu_unmap(struct iommu_domain *domain,
+ unsigned long iova, size_t size)
+{
+ struct mtk_iommu_domain *priv = to_mtk_domain(domain);
+ unsigned long flags;
+ size_t unmapsize;
+
+ spin_lock_irqsave(&priv->pgtlock, flags);
+ unmapsize = priv->iop->unmap(priv->iop, iova, size);
+ spin_unlock_irqrestore(&priv->pgtlock, flags);
+
+ return unmapsize;
+}
+
+static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
+ dma_addr_t iova)
+{
+ struct mtk_iommu_domain *priv = to_mtk_domain(domain);
+ unsigned long flags;
+ phys_addr_t pa;
+
+ spin_lock_irqsave(&priv->pgtlock, flags);
+ pa = priv->iop->iova_to_phys(priv->iop, iova);
+ spin_unlock_irqrestore(&priv->pgtlock, flags);
+
+ return pa;
+}
+
+static int mtk_iommu_add_device(struct device *dev)
+{
+ struct mtk_iommu_domain *mtkdom;
+ struct iommu_group *group;
+ struct mtk_iommu_client_priv *devhead;
+ int ret;
+
+ if (!dev->archdata.dma_ops)/* Not a iommu client device */
+ return -ENODEV;
+
+ group = iommu_group_get(dev);
+ if (!group) {
+ group = iommu_group_alloc();
+ if (IS_ERR(group)) {
+ dev_err(dev, "Failed to allocate IOMMU group\n");
+ return PTR_ERR(group);
+ }
+ }
+
+ ret = iommu_group_add_device(group, dev);
+ if (ret) {
+ dev_err(dev, "Failed to add IOMMU group\n");
+ goto err_group_put;
+ }
+
+ /* get the mtk_iommu_domain from the iommu device */
+ devhead = dev->archdata.iommu;
+ mtkdom = devhead->imudev->archdata.iommu;
+ ret = iommu_attach_group(&mtkdom->domain, group);
+ if (ret)
+ dev_err(dev, "Failed to attach IOMMU group\n");
+
+err_group_put:
+ iommu_group_put(group);
+ return ret;
+}
+
+int mtk_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+ struct mtk_iommu_client_priv *head, *priv, *next;
+ struct platform_device *imupdev;
+ struct device *imudev;
+
+ if (args->args_count != 2) {
+ dev_err(dev, "invalid #iommu-cells(%d) property for IOMMU\n",
+ args->args_count);
+ return -EINVAL;
+ }
+
+ if (!dev->archdata.iommu) {
+ /* Get the iommu device */
+ imupdev = of_find_device_by_node(args->np);
+ of_node_put(args->np);
+ if (WARN_ON(!imupdev))
+ return -EINVAL;
+ imudev = &imupdev->dev;
+
+ head = kzalloc(sizeof(*head), GFP_KERNEL);
+ if (!head)
+ return -ENOMEM;
+
+ dev->archdata.iommu = head;
+ INIT_LIST_HEAD(&head->client);
+ head->imudev = imudev;
+ } else {
+ head = dev->archdata.iommu;
+ }
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ goto err_free_mem;
+
+ priv->larbid = args->args[0];
+ priv->portid = args->args[1];
+ list_add_tail(&priv->client, &head->client);
+
+ return 0;
+
+err_free_mem:
+ list_for_each_entry_safe(priv, next, &head->client, client)
+ kfree(priv);
+ kfree(head);
+ dev->archdata.iommu = NULL;
+ return -ENOMEM;
+}
+
+static struct iommu_ops mtk_iommu_ops = {
+ .domain_alloc = mtk_iommu_domain_alloc,
+ .domain_free = mtk_iommu_domain_free,
+ .attach_dev = mtk_iommu_attach_device,
+ .detach_dev = mtk_iommu_detach_device,
+ .map = mtk_iommu_map,
+ .unmap = mtk_iommu_unmap,
+ .map_sg = default_iommu_map_sg,
+ .iova_to_phys = mtk_iommu_iova_to_phys,
+ .add_device = mtk_iommu_add_device,
+ .of_xlate = mtk_iommu_of_xlate,
+ .pgsize_bitmap = -1UL,
+};
+
+static const struct of_device_id mtk_iommu_of_ids[] = {
+ { .compatible = "mediatek,mt8173-m4u", },
+ {}
+};
+
+static int mtk_iommu_init_fn(struct device_node *np)
+{
+ struct platform_device *pdev;
+
+ pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
+
+ of_iommu_set_ops(np, &mtk_iommu_ops);
+
+ return 0;
+}
+
+IOMMU_OF_DECLARE(mtkm4u, "mediatek,mt8173-m4u", mtk_iommu_init_fn);
+
+static int mtk_iommu_probe(struct platform_device *pdev)
+{
+ struct mtk_iommu_domain *mtkdom;
+ struct mtk_iommu_data *data;
+ struct device *dev = &pdev->dev;
+ void __iomem *protect;
+ int ret;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ /* Protect memory. HW will access here while translation fault.*/
+ protect = devm_kzalloc(dev, MTK_PROTECT_PA_ALIGN * 2, GFP_KERNEL);
+ if (!protect)
+ return -ENOMEM;
+ data->protect_base = virt_to_phys(protect);
+
+ ret = mtk_iommu_parse_dt(pdev, data);
+ if (ret)
+ return ret;
+
+ mtkdom = dev->archdata.iommu;
+ if (!mtkdom)/* There is no iommu client device */
+ return 0;
+
+ mtkdom->data = data;
+ data->dev = dev;
+ dev_set_drvdata(dev, mtkdom);
+
+ return 0;
+}
+
+static int mtk_iommu_remove(struct platform_device *pdev)
+{
+ struct mtk_iommu_domain *mtkdom = dev_get_drvdata(&pdev->dev);
+
+ /* Destroy domain context */
+ free_io_pgtable_ops(mtkdom->iop);
+
+ clk_disable_unprepare(mtkdom->data->bclk);
+ return 0;
+}
+
+static int mtk_iommu_suspend(struct device *dev)
+{
+ struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
+ struct mtk_iommu_data *data = mtkdom->data;
+ void __iomem *base = data->base;
+ unsigned int i = 0;
+
+ data->reg[i++] = readl(base + REG_MMU_PT_BASE_ADDR);
+ data->reg[i++] = readl(base + REG_MMU_STANDARD_AXI_MODE);
+ data->reg[i++] = readl(base + REG_MMU_DCM_DIS);
+ data->reg[i++] = readl(base + REG_MMU_CTRL_REG);
+ data->reg[i++] = readl(base + REG_MMU_IVRP_PADDR);
+ data->reg[i++] = readl(base + REG_MMU_INT_CONTROL0);
+ data->reg[i++] = readl(base + REG_MMU_INT_MAIN_CONTROL);
+ return 0;
+}
+
+static int mtk_iommu_resume(struct device *dev)
+{
+ struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
+ struct mtk_iommu_data *data = mtkdom->data;
+ void __iomem *base = data->base;
+ unsigned int i = 0;
+
+ writel(data->reg[i++], base + REG_MMU_PT_BASE_ADDR);
+ writel(data->reg[i++], base + REG_MMU_STANDARD_AXI_MODE);
+ writel(data->reg[i++], base + REG_MMU_DCM_DIS);
+ writel(data->reg[i++], base + REG_MMU_CTRL_REG);
+ writel(data->reg[i++], base + REG_MMU_IVRP_PADDR);
+ writel(data->reg[i++], base + REG_MMU_INT_CONTROL0);
+ writel(data->reg[i++], base + REG_MMU_INT_MAIN_CONTROL);
+ return 0;
+}
+
+const struct dev_pm_ops mtk_iommu_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(mtk_iommu_suspend, mtk_iommu_resume)
+};
+
+static struct platform_driver mtk_iommu_driver = {
+ .probe = mtk_iommu_probe,
+ .remove = mtk_iommu_remove,
+ .driver = {
+ .name = "mtk-iommu",
+ .of_match_table = mtk_iommu_of_ids,
+ .pm = &mtk_iommu_pm_ops,
+ }
+};
+
+static int __init mtk_iommu_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&mtk_iommu_driver);
+ if (ret) {
+ pr_err("%s: Failed to register driver\n", __func__);
+ return ret;
+ }
+
+ if (!iommu_present(&platform_bus_type))
+ bus_set_iommu(&platform_bus_type, &mtk_iommu_ops);
+
+ return 0;
+}
+
+subsys_initcall(mtk_iommu_init);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 6/6] dts: mt8173: Add iommu/smi nodes for mt8173
2015-07-16 9:04 [PATCH v3 0/6] MT8173 IOMMU SUPPORT Yong Wu
` (4 preceding siblings ...)
2015-07-16 9:04 ` [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu
@ 2015-07-16 9:04 ` Yong Wu
2015-07-23 14:40 ` Daniel Kurtz
5 siblings, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-07-16 9:04 UTC (permalink / raw)
To: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger
Cc: Robin Murphy, Will Deacon, Daniel Kurtz, Tomasz Figa,
Lucas Stach, Rob Herring, Catalin Marinas, linux-mediatek,
Sasha Hauer, srv_heupstream, devicetree, linux-kernel,
linux-arm-kernel, iommu, pebolle, arnd, mitchelh, cloud.chou,
frederic.chen, yong.wu
This patch add the iommu/larbs nodes for mt8173
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 81 ++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index e81ac1f..7b8e73c 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -14,6 +14,7 @@
#include <dt-bindings/clock/mt8173-clk.h>
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/memory/mt8173-larb-port.h>
#include <dt-bindings/power/mt8173-power.h>
#include <dt-bindings/reset-controller/mt8173-resets.h>
#include "mt8173-pinfunc.h"
@@ -240,6 +241,17 @@
reg = <0 0x10200620 0 0x20>;
};
+ iommu: mmsys_iommu@10205000 {
+ compatible = "mediatek,mt8173-m4u";
+ reg = <0 0x10205000 0 0x1000>;
+ interrupts = <GIC_SPI 139 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&infracfg CLK_INFRA_M4U>;
+ clock-names = "bclk";
+ mediatek,larb = <&larb0 &larb1 &larb2
+ &larb3 &larb4 &larb5>;
+ #iommu-cells = <2>;
+ };
+
apmixedsys: clock-controller@10209000 {
compatible = "mediatek,mt8173-apmixedsys";
reg = <0 0x10209000 0 0x1000>;
@@ -401,29 +413,98 @@
#clock-cells = <1>;
};
+ larb0: larb@14021000 {
+ compatible = "mediatek,mt8173-smi-larb";
+ reg = <0 0x14021000 0 0x1000>;
+ mediatek,smi = <&smi_common>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
+ clocks = <&mmsys CLK_MM_SMI_LARB0>,
+ <&mmsys CLK_MM_SMI_LARB0>;
+ clock-names = "apb", "smi";
+ };
+
+ smi_common: smi@14022000 {
+ compatible = "mediatek,mt8173-smi";
+ reg = <0 0x14022000 0 0x1000>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
+ clocks = <&mmsys CLK_MM_SMI_COMMON>,
+ <&mmsys CLK_MM_SMI_COMMON>;
+ clock-names = "apb", "smi";
+ };
+
+ larb4: larb@14027000 {
+ compatible = "mediatek,mt8173-smi-larb";
+ reg = <0 0x14027000 0 0x1000>;
+ mediatek,smi = <&smi_common>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
+ clocks = <&mmsys CLK_MM_SMI_LARB4>,
+ <&mmsys CLK_MM_SMI_LARB4>;
+ clock-names = "apb", "smi";
+ };
+
imgsys: imgsys@15000000 {
compatible = "mediatek,mt8173-imgsys", "syscon";
reg = <0 0x15000000 0 0x1000>;
#clock-cells = <1>;
};
+ larb2: larb@15001000 {
+ compatible = "mediatek,mt8173-smi-larb";
+ reg = <0 0x15001000 0 0x1000>;
+ mediatek,smi = <&smi_common>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_ISP>;
+ clocks = <&imgsys CLK_IMG_LARB2_SMI>,
+ <&imgsys CLK_IMG_LARB2_SMI>;
+ clock-names = "apb", "smi";
+ };
+
vdecsys: vdecsys@16000000 {
compatible = "mediatek,mt8173-vdecsys", "syscon";
reg = <0 0x16000000 0 0x1000>;
#clock-cells = <1>;
};
+ larb1: larb@16010000 {
+ compatible = "mediatek,mt8173-smi-larb";
+ reg = <0 0x16010000 0 0x1000>;
+ mediatek,smi = <&smi_common>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
+ clocks = <&vdecsys CLK_VDEC_CKEN>,
+ <&vdecsys CLK_VDEC_LARB_CKEN>;
+ clock-names = "apb", "smi";
+ };
+
vencsys: vencsys@18000000 {
compatible = "mediatek,mt8173-vencsys", "syscon";
reg = <0 0x18000000 0 0x1000>;
#clock-cells = <1>;
};
+ larb3: larb@18001000 {
+ compatible = "mediatek,mt8173-smi-larb";
+ reg = <0 0x18001000 0 0x1000>;
+ mediatek,smi = <&smi_common>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_VENC>;
+ clocks = <&vencsys CLK_VENC_CKE1>,
+ <&vencsys CLK_VENC_CKE0>;
+ clock-names = "apb", "smi";
+ };
+
vencltsys: vencltsys@19000000 {
compatible = "mediatek,mt8173-vencltsys", "syscon";
reg = <0 0x19000000 0 0x1000>;
#clock-cells = <1>;
};
+
+ larb5: larb@19001000 {
+ compatible = "mediatek,mt8173-smi-larb";
+ reg = <0 0x19001000 0 0x1000>;
+ mediatek,smi = <&smi_common>;
+ power-domains = <&scpsys MT8173_POWER_DOMAIN_VENC_LT>;
+ clocks = <&vencltsys CLK_VENCLT_CKE1>,
+ <&vencltsys CLK_VENCLT_CKE0>;
+ clock-names = "apb", "smi";
+ };
};
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-16 9:04 ` [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu
@ 2015-07-21 14:59 ` Will Deacon
2015-07-24 5:43 ` Yong Wu
2015-07-27 13:23 ` Robin Murphy
1 sibling, 1 reply; 34+ messages in thread
From: Will Deacon @ 2015-07-21 14:59 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
Hi Yong Wu,
On Thu, Jul 16, 2015 at 10:04:34AM +0100, Yong Wu wrote:
> This patch adds support for mediatek m4u (MultiMedia Memory Management
> Unit).
[...]
> +static void mtk_iommu_tlb_flush_all(void *cookie)
> +{
> + struct mtk_iommu_domain *domain = cookie;
> + void __iomem *base;
> +
> + base = domain->data->base;
> + writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> + writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
This needs to be synchronous, so you probably want to call
mtk_iommu_tlb_sync at the end.
> +}
> +
> +static void mtk_iommu_tlb_add_flush(unsigned long iova, size_t size,
> + bool leaf, void *cookie)
> +{
> + struct mtk_iommu_domain *domain = cookie;
> + void __iomem *base = domain->data->base;
> + unsigned int iova_start = iova, iova_end = iova + size - 1;
> +
> + writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> +
> + writel(iova_start, base + REG_MMU_INVLD_START_A);
> + writel(iova_end, base + REG_MMU_INVLD_END_A);
> + writel(F_MMU_INV_RANGE, base + REG_MMU_INVALIDATE);
Why are you using writel instead of writel_relaxed? I asked this before
but I don't think you replied.
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-16 9:04 ` [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator Yong Wu
@ 2015-07-21 17:11 ` Will Deacon
2015-07-24 5:24 ` Yong Wu
2015-09-14 12:25 ` Yong Wu
0 siblings, 2 replies; 34+ messages in thread
From: Will Deacon @ 2015-07-21 17:11 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
Hello,
This is looking better, but I still have some concerns.
On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> This patch is for ARM Short Descriptor Format.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> drivers/iommu/Kconfig | 18 +
> drivers/iommu/Makefile | 1 +
> drivers/iommu/io-pgtable-arm-short.c | 742 ++++++++++++++++++++++++++++++++++
> drivers/iommu/io-pgtable-arm.c | 3 -
> drivers/iommu/io-pgtable.c | 4 +
> drivers/iommu/io-pgtable.h | 13 +
> 6 files changed, 778 insertions(+), 3 deletions(-)
> create mode 100644 drivers/iommu/io-pgtable-arm-short.c
[...]
> +#define ARM_SHORT_PGDIR_SHIFT 20
> +#define ARM_SHORT_PAGE_SHIFT 12
> +#define ARM_SHORT_PTRS_PER_PTE \
> + (1 << (ARM_SHORT_PGDIR_SHIFT - ARM_SHORT_PAGE_SHIFT))
> +#define ARM_SHORT_BYTES_PER_PTE \
> + (ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte))
> +
> +/* level 1 pagetable */
> +#define ARM_SHORT_PGD_TYPE_PGTABLE BIT(0)
> +#define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4))
> +#define ARM_SHORT_PGD_TYPE_SECTION BIT(1)
> +#define ARM_SHORT_PGD_PGTABLE_XN BIT(2)
This should be PXN, but I'm not even sure why we care for a table at
level 1.
> +#define ARM_SHORT_PGD_B BIT(2)
> +#define ARM_SHORT_PGD_C BIT(3)
> +#define ARM_SHORT_PGD_PGTABLE_NS BIT(3)
> +#define ARM_SHORT_PGD_IMPLE BIT(9)
> +#define ARM_SHORT_PGD_TEX0 BIT(12)
> +#define ARM_SHORT_PGD_S BIT(16)
> +#define ARM_SHORT_PGD_nG BIT(17)
> +#define ARM_SHORT_PGD_SUPERSECTION BIT(18)
> +#define ARM_SHORT_PGD_SECTION_NS BIT(19)
> +
> +#define ARM_SHORT_PGD_TYPE_SUPERSECTION \
> + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
> +#define ARM_SHORT_PGD_SECTION_TYPE_MSK \
> + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
> +#define ARM_SHORT_PGD_PGTABLE_TYPE_MSK \
> + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_TYPE_PGTABLE)
> +#define ARM_SHORT_PGD_TYPE_IS_PGTABLE(pgd) \
> + (((pgd) & ARM_SHORT_PGD_PGTABLE_TYPE_MSK) == ARM_SHORT_PGD_TYPE_PGTABLE)
> +#define ARM_SHORT_PGD_TYPE_IS_SECTION(pgd) \
> + (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == ARM_SHORT_PGD_TYPE_SECTION)
> +#define ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(pgd) \
> + (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == \
> + ARM_SHORT_PGD_TYPE_SUPERSECTION)
> +#define ARM_SHORT_PGD_PGTABLE_MSK 0xfffffc00
> +#define ARM_SHORT_PGD_SECTION_MSK (~(SZ_1M - 1))
> +#define ARM_SHORT_PGD_SUPERSECTION_MSK (~(SZ_16M - 1))
> +
> +/* level 2 pagetable */
> +#define ARM_SHORT_PTE_TYPE_LARGE BIT(0)
> +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> +#define ARM_SHORT_PTE_TYPE_SMALL BIT(1)
> +#define ARM_SHORT_PTE_B BIT(2)
> +#define ARM_SHORT_PTE_C BIT(3)
> +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
> +#define ARM_SHORT_PTE_IMPLE BIT(9)
This is AP[2] for small pages.
> +#define ARM_SHORT_PTE_S BIT(10)
> +#define ARM_SHORT_PTE_nG BIT(11)
> +#define ARM_SHORT_PTE_LARGE_TEX0 BIT(12)
> +#define ARM_SHORT_PTE_LARGE_XN BIT(15)
> +#define ARM_SHORT_PTE_LARGE_MSK (~(SZ_64K - 1))
> +#define ARM_SHORT_PTE_SMALL_MSK (~(SZ_4K - 1))
> +#define ARM_SHORT_PTE_TYPE_MSK \
> + (ARM_SHORT_PTE_TYPE_LARGE | ARM_SHORT_PTE_TYPE_SMALL)
> +#define ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(pte) \
> + (((((pte) & ARM_SHORT_PTE_TYPE_MSK) >> 1) << 1)\
> + == ARM_SHORT_PTE_TYPE_SMALL)
I see what you're trying to do here, but the shifting is confusing. I
think it's better doing something like:
((pte) & ARM_SHORT_PTE_TYPE_SMALL) == ARM_SHORT_PTE_TYPE_SMALL
or even just:
(pte) & ARM_SHORT_PTE_TYPE_SMALL
> +#define ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(pte) \
> + (((pte) & ARM_SHORT_PTE_TYPE_MSK) == ARM_SHORT_PTE_TYPE_LARGE)
> +
> +#define ARM_SHORT_PGD_IDX(a) ((a) >> ARM_SHORT_PGDIR_SHIFT)
> +#define ARM_SHORT_PTE_IDX(a) \
> + (((a) >> ARM_SHORT_PAGE_SHIFT) & (ARM_SHORT_PTRS_PER_PTE - 1))
> +
> +#define ARM_SHORT_GET_PTE_VA(pgd) \
> + (phys_to_virt((unsigned long)pgd & ARM_SHORT_PGD_PGTABLE_MSK))
Maybe better named as ARM_SHORT_GET_PGTABLE_VA ?
> +
> +#define ARM_SHORT_PTE_LARGE_GET_PROT(pte) \
> + (((pte) & (~ARM_SHORT_PTE_LARGE_MSK)) & ~ARM_SHORT_PTE_TYPE_MSK)
> +
> +#define ARM_SHORT_PGD_GET_PROT(pgd) \
> + (((pgd) & (~ARM_SHORT_PGD_SECTION_MSK)) & ~ARM_SHORT_PGD_SUPERSECTION)
> +
> +static bool selftest_running;
> +
> +static arm_short_iopte *
> +arm_short_get_pte_in_pgd(arm_short_iopte pgd, unsigned int iova)
> +{
> + arm_short_iopte *pte;
> +
> + pte = ARM_SHORT_GET_PTE_VA(pgd);
> + pte += ARM_SHORT_PTE_IDX(iova);
> + return pte;
> +}
> +
> +static void _arm_short_free_pgtable(struct arm_short_io_pgtable *data,
> + arm_short_iopte *pgd)
> +{
> + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> + arm_short_iopte *pte;
> + int i;
> +
> + pte = ARM_SHORT_GET_PTE_VA(*pgd);
> + for (i = 0; i < ARM_SHORT_PTRS_PER_PTE; i++) {
> + if (pte[i] != 0)
> + return;
> + }
Do you actually need this loop if you're not warning or returning an error?
> +
> + /* Free whole pte and set pgd to zero while all pte is unmap */
> + kmem_cache_free(data->ptekmem, pte);
> + *pgd = 0;
I still don't think this is safe. What stops the page table walker from
speculatively walking freed memory?
> + tlb->flush_pgtable(pgd, sizeof(*pgd), data->iop.cookie);
> +}
> +
> +static arm_short_iopte
> +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
> +{
> + arm_short_iopte pteprot;
> +
> + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> + ARM_SHORT_PTE_TYPE_SMALL;
> + if (prot & IOMMU_CACHE)
> + pteprot |= ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
> + if (prot & IOMMU_WRITE)
> + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> + ARM_SHORT_PTE_SMALL_TEX0;
This doesn't make any sense. TEX[2:0] is all about memory attributes, not
permissions, so you're making the mapping write-back, write-allocate but
that's not what the IOMMU_* values are about.
> + if (prot & IOMMU_NOEXEC)
> + pteprot |= large ? ARM_SHORT_PTE_LARGE_XN :
> + ARM_SHORT_PTE_SMALL_XN;
> + return pteprot;
> +}
> +
> +static arm_short_iopte
> +__arm_short_pgd_prot(struct arm_short_io_pgtable *data, int prot, bool super)
> +{
> + arm_short_iopte pgdprot;
> +
> + pgdprot = ARM_SHORT_PGD_S | ARM_SHORT_PGD_nG;
> + pgdprot |= super ? ARM_SHORT_PGD_TYPE_SUPERSECTION :
> + ARM_SHORT_PGD_TYPE_SECTION;
> + if (prot & IOMMU_CACHE)
> + pgdprot |= ARM_SHORT_PGD_C | ARM_SHORT_PGD_B;
> + if (prot & IOMMU_WRITE)
> + pgdprot |= ARM_SHORT_PGD_TEX0;
Likewise.
> + if (prot & IOMMU_NOEXEC)
> + pgdprot |= ARM_SHORT_PGD_SECTION_XN;
> + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
> + pgdprot |= ARM_SHORT_PGD_SECTION_NS;
> + return pgdprot;
> +}
> +
> +static arm_short_iopte
> +__arm_short_pte_prot_split(struct arm_short_io_pgtable *data,
> + arm_short_iopte pgdprot,
> + arm_short_iopte pteprot_large,
> + bool large)
> +{
> + arm_short_iopte pteprot = 0;
> +
> + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> + ARM_SHORT_PTE_TYPE_SMALL;
> + /* section to pte prot */
> + if (pgdprot & ARM_SHORT_PGD_C)
> + pteprot |= ARM_SHORT_PTE_C;
> + if (pgdprot & ARM_SHORT_PGD_B)
> + pteprot |= ARM_SHORT_PTE_B;
> + if (pgdprot & ARM_SHORT_PGD_TEX0)
> + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> + ARM_SHORT_PTE_SMALL_TEX0;
> + if (pgdprot & ARM_SHORT_PGD_nG)
> + pteprot |= ARM_SHORT_PTE_nG;
> + if (pgdprot & ARM_SHORT_PGD_SECTION_XN)
> + pteprot |= large ? ARM_SHORT_PTE_LARGE_XN :
> + ARM_SHORT_PTE_SMALL_XN;
> +
> + /* large page to small page pte prot. Only large page may split */
> + if (pteprot_large && !large) {
> + if (pteprot_large & ARM_SHORT_PTE_LARGE_TEX0)
> + pteprot |= ARM_SHORT_PTE_SMALL_TEX0;
> + if (pteprot_large & ARM_SHORT_PTE_LARGE_XN)
> + pteprot |= ARM_SHORT_PTE_SMALL_XN;
> + }
> + return pteprot;
> +}
> +
> +static arm_short_iopte
> +__arm_short_pgtable_prot(struct arm_short_io_pgtable *data, bool noexec)
> +{
> + arm_short_iopte pgdprot = 0;
> +
> + pgdprot = ARM_SHORT_PGD_TYPE_PGTABLE;
> + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
> + pgdprot |= ARM_SHORT_PGD_PGTABLE_NS;
> + if (noexec)
> + pgdprot |= ARM_SHORT_PGD_PGTABLE_XN;
I don't think you need to worry about XN bits for PGTABLEs.
> + return pgdprot;
> +}
> +
> +static int
> +_arm_short_map(struct arm_short_io_pgtable *data,
> + unsigned int iova, phys_addr_t paddr,
> + arm_short_iopte pgdprot, arm_short_iopte pteprot,
> + bool large)
> +{
> + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> + arm_short_iopte *pgd = data->pgd, *pte;
> + void *cookie = data->iop.cookie, *pte_va;
> + unsigned int ptenr = large ? 16 : 1;
> + int i, quirk = data->iop.cfg.quirks;
> + bool ptenew = false;
> +
> + pgd += ARM_SHORT_PGD_IDX(iova);
> +
> + if (!pteprot) { /* section or supersection */
> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
> + pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
> + pte = pgd;
> + pteprot = pgdprot;
> + } else { /* page or largepage */
> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> + if (large) { /* special Bit */
This definitely needs a better comment! What exactly are you doing here
and what is that quirk all about?
> + if (pteprot & ARM_SHORT_PTE_LARGE_TEX0) {
> + pteprot &= ~ARM_SHORT_PTE_LARGE_TEX0;
> + pteprot |= ARM_SHORT_PTE_SMALL_TEX0;
> + }
> + pteprot &= ~ARM_SHORT_PTE_LARGE_XN;
> + } else {
> + pteprot &= ~ARM_SHORT_PTE_SMALL_XN;
> + }
> + }
> +
> + if (!(*pgd)) {
> + pte_va = kmem_cache_zalloc(data->ptekmem, GFP_ATOMIC);
> + if (unlikely(!pte_va))
> + return -ENOMEM;
> + ptenew = true;
> + *pgd = virt_to_phys(pte_va) | pgdprot;
> + kmemleak_ignore(pte_va);
> + tlb->flush_pgtable(pgd, sizeof(*pgd), cookie);
I think you need to flush this before it becomes visible to the walker.
> + }
> + pte = arm_short_get_pte_in_pgd(*pgd, iova);
> + }
> +
> + pteprot |= (arm_short_iopte)paddr;
> + for (i = 0; i < ptenr; i++) {
> + if (pte[i]) {/* Someone else may have allocated for this pte */
> + WARN_ON(!selftest_running);
> + goto err_exist_pte;
> + }
> + pte[i] = pteprot;
> + }
> + tlb->flush_pgtable(pte, ptenr * sizeof(*pte), cookie);
> +
> + return 0;
> +
> +err_exist_pte:
> + while (i--)
> + pte[i] = 0;
> + if (ptenew)
> + kmem_cache_free(data->ptekmem, pte_va);
> + return -EEXIST;
> +}
> +
> +static int arm_short_map(struct io_pgtable_ops *ops, unsigned long iova,
> + phys_addr_t paddr, size_t size, int prot)
> +{
> + struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
> + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> + int ret;
> + arm_short_iopte pgdprot = 0, pteprot = 0;
> + bool large;
> +
> + /* If no access, then nothing to do */
> + if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
> + return 0;
> +
> + switch (size) {
> + case SZ_4K:
> + case SZ_64K:
> + large = (size == SZ_64K) ? true : false;
> + pteprot = __arm_short_pte_prot(data, prot, large);
> + pgdprot = __arm_short_pgtable_prot(data, prot & IOMMU_NOEXEC);
> + break;
> +
> + case SZ_1M:
> + case SZ_16M:
> + large = (size == SZ_16M) ? true : false;
> + pgdprot = __arm_short_pgd_prot(data, prot, large);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (WARN_ON((iova | paddr) & (size - 1)))
> + return -EINVAL;
> +
> + ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
> +
> + tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
> + tlb->tlb_sync(data->iop.cookie);
In _arm_short_map, it looks like you can only go from invalid -> valid,
so why do you need to flush the TLB here?
> + return ret;
> +}
> +
> +static phys_addr_t arm_short_iova_to_phys(struct io_pgtable_ops *ops,
> + unsigned long iova)
> +{
> + struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
> + arm_short_iopte *pte, *pgd = data->pgd;
> + phys_addr_t pa = 0;
> +
> + pgd += ARM_SHORT_PGD_IDX(iova);
> +
> + if (ARM_SHORT_PGD_TYPE_IS_PGTABLE(*pgd)) {
> + pte = arm_short_get_pte_in_pgd(*pgd, iova);
> +
> + if (ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(*pte)) {
> + pa = (*pte) & ARM_SHORT_PTE_LARGE_MSK;
> + pa |= iova & ~ARM_SHORT_PTE_LARGE_MSK;
> + } else if (ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(*pte)) {
> + pa = (*pte) & ARM_SHORT_PTE_SMALL_MSK;
> + pa |= iova & ~ARM_SHORT_PTE_SMALL_MSK;
> + }
> + } else if (ARM_SHORT_PGD_TYPE_IS_SECTION(*pgd)) {
> + pa = (*pgd) & ARM_SHORT_PGD_SECTION_MSK;
> + pa |= iova & ~ARM_SHORT_PGD_SECTION_MSK;
> + } else if (ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(*pgd)) {
> + pa = (*pgd) & ARM_SHORT_PGD_SUPERSECTION_MSK;
> + pa |= iova & ~ARM_SHORT_PGD_SUPERSECTION_MSK;
> + }
> +
> + return pa;
> +}
> +
> +static int
> +arm_short_split_blk_unmap(struct io_pgtable_ops *ops, unsigned int iova,
> + phys_addr_t paddr, size_t size,
> + arm_short_iopte pgdprotup, arm_short_iopte pteprotup,
> + size_t blk_size)
> +{
> + struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
> + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> + struct io_pgtable_cfg *cfg = &data->iop.cfg;
> + unsigned long *pgbitmap = &cfg->pgsize_bitmap;
> + unsigned int blk_base, blk_start, blk_end;
> + arm_short_iopte pgdprot, pteprot;
> + size_t mapsize = 0, nextmapsize;
> + phys_addr_t blk_paddr;
> + int ret;
> + unsigned int i;
> +
> + /* find the nearest mapsize */
> + for (i = find_first_bit(pgbitmap, BITS_PER_LONG);
> + i < BITS_PER_LONG && ((1 << i) < blk_size) &&
> + IS_ALIGNED(size, 1 << i);
> + i = find_next_bit(pgbitmap, BITS_PER_LONG, i + 1))
> + mapsize = 1 << i;
> +
> + if (WARN_ON(!mapsize))
> + return 0; /* Bytes unmapped */
> + nextmapsize = 1 << i;
> +
> + blk_base = iova & ~(blk_size - 1);
> + blk_start = blk_base;
> + blk_end = blk_start + blk_size;
> + blk_paddr = paddr;
> +
> + for (; blk_start < blk_end;
> + blk_start += mapsize, blk_paddr += mapsize) {
> + /* Unmap! */
> + if (blk_start == iova)
> + continue;
> +
> + /* Try to upper map */
> + if (blk_base != blk_start &&
> + IS_ALIGNED(blk_start | blk_paddr, nextmapsize) &&
> + mapsize != nextmapsize) {
> + mapsize = nextmapsize;
> + i = find_next_bit(pgbitmap, BITS_PER_LONG, i + 1);
> + if (i < BITS_PER_LONG)
> + nextmapsize = 1 << i;
> + }
> +
> + if (mapsize == SZ_1M) {
> + pgdprot = pgdprotup;
> + pgdprot |= __arm_short_pgd_prot(data, 0, false);
> + pteprot = 0;
> + } else { /* small or large page */
> + bool noexec = (blk_size == SZ_64K) ?
> + (pteprotup & ARM_SHORT_PTE_LARGE_XN) :
> + (pgdprotup & ARM_SHORT_PGD_SECTION_XN);
> +
> + pteprot = __arm_short_pte_prot_split(
> + data, pgdprotup, pteprotup,
> + mapsize == SZ_64K);
> + pgdprot = __arm_short_pgtable_prot(data, noexec);
> + }
> +
> + ret = _arm_short_map(data, blk_start, blk_paddr, pgdprot,
> + pteprot, mapsize == SZ_64K);
> + if (ret < 0) {
> + /* Free the table we allocated */
> + arm_short_iopte *pgd = data->pgd, *pte;
> +
> + pgd += ARM_SHORT_PGD_IDX(blk_base);
> + if (*pgd) {
> + pte = ARM_SHORT_GET_PTE_VA(*pgd);
> + kmem_cache_free(data->ptekmem, pte);
> + *pgd = 0;
> + tlb->flush_pgtable(pgd, sizeof(*pgd),
> + data->iop.cookie);
Again, the ordering looks totally wrong here. You need to:
(1) Zero the pgd pointer
(2) Flush the pointer, so the walker can no longer follow the page table
(3) Invalidate the TLB, so we don't have any cached intermediate walks
(4) Sync the TLB
(5) Free the memory
That said, I don't fully follow the logic here.
> + }
> + return 0;/* Bytes unmapped */
> + }
> + tlb->tlb_add_flush(blk_start, mapsize, true, data->iop.cookie);
> + tlb->tlb_sync(data->iop.cookie);
Why are you doing this here for every iteration?
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 6/6] dts: mt8173: Add iommu/smi nodes for mt8173
2015-07-16 9:04 ` [PATCH v3 6/6] dts: mt8173: Add iommu/smi nodes for mt8173 Yong Wu
@ 2015-07-23 14:40 ` Daniel Kurtz
2015-07-29 7:29 ` Yong Wu
0 siblings, 1 reply; 34+ messages in thread
From: Daniel Kurtz @ 2015-07-23 14:40 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Will Deacon, Tomasz Figa, Lucas Stach, Rob Herring,
Catalin Marinas, linux-mediatek, Sasha Hauer, srv_heupstream,
open list:OPEN FIRMWARE AND...,
linux-kernel, linux-arm-kernel, open list:IOMMU DRIVERS,
Paul Bolle, Arnd Bergmann, mitchelh, cloud.chou, frederic.chen
Hi Yong,
On Thu, Jul 16, 2015 at 5:04 PM, Yong Wu <yong.wu@mediatek.com> wrote:
>
> This patch add the iommu/larbs nodes for mt8173
To what tree does this apply?
Please rebase these patches (especially this one) on an Matthias'
current v4.2-next/for-next.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 81 ++++++++++++++++++++++++++++++
> 1 file changed, 81 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index e81ac1f..7b8e73c 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -14,6 +14,7 @@
> #include <dt-bindings/clock/mt8173-clk.h>
> #include <dt-bindings/interrupt-controller/irq.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/memory/mt8173-larb-port.h>
> #include <dt-bindings/power/mt8173-power.h>
> #include <dt-bindings/reset-controller/mt8173-resets.h>
> #include "mt8173-pinfunc.h"
> @@ -240,6 +241,17 @@
> reg = <0 0x10200620 0 0x20>;
> };
>
> + iommu: mmsys_iommu@10205000 {
This name should be generic, like this (this comment really applies to
the binding patch):
iommu: iommu@10205000
> + compatible = "mediatek,mt8173-m4u";
> + reg = <0 0x10205000 0 0x1000>;
> + interrupts = <GIC_SPI 139 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&infracfg CLK_INFRA_M4U>;
> + clock-names = "bclk";
> + mediatek,larb = <&larb0 &larb1 &larb2
> + &larb3 &larb4 &larb5>;
Tiny nit... please align the "&".
Otherwise, this one is:
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Thanks!
-Dan
> + #iommu-cells = <2>;
> + };
> +
> apmixedsys: clock-controller@10209000 {
> compatible = "mediatek,mt8173-apmixedsys";
> reg = <0 0x10209000 0 0x1000>;
> @@ -401,29 +413,98 @@
> #clock-cells = <1>;
> };
>
> + larb0: larb@14021000 {
> + compatible = "mediatek,mt8173-smi-larb";
> + reg = <0 0x14021000 0 0x1000>;
> + mediatek,smi = <&smi_common>;
> + power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> + clocks = <&mmsys CLK_MM_SMI_LARB0>,
> + <&mmsys CLK_MM_SMI_LARB0>;
> + clock-names = "apb", "smi";
> + };
> +
> + smi_common: smi@14022000 {
> + compatible = "mediatek,mt8173-smi";
> + reg = <0 0x14022000 0 0x1000>;
> + power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> + clocks = <&mmsys CLK_MM_SMI_COMMON>,
> + <&mmsys CLK_MM_SMI_COMMON>;
> + clock-names = "apb", "smi";
> + };
> +
> + larb4: larb@14027000 {
> + compatible = "mediatek,mt8173-smi-larb";
> + reg = <0 0x14027000 0 0x1000>;
> + mediatek,smi = <&smi_common>;
> + power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> + clocks = <&mmsys CLK_MM_SMI_LARB4>,
> + <&mmsys CLK_MM_SMI_LARB4>;
> + clock-names = "apb", "smi";
> + };
> +
> imgsys: imgsys@15000000 {
> compatible = "mediatek,mt8173-imgsys", "syscon";
> reg = <0 0x15000000 0 0x1000>;
> #clock-cells = <1>;
> };
>
> + larb2: larb@15001000 {
> + compatible = "mediatek,mt8173-smi-larb";
> + reg = <0 0x15001000 0 0x1000>;
> + mediatek,smi = <&smi_common>;
> + power-domains = <&scpsys MT8173_POWER_DOMAIN_ISP>;
> + clocks = <&imgsys CLK_IMG_LARB2_SMI>,
> + <&imgsys CLK_IMG_LARB2_SMI>;
> + clock-names = "apb", "smi";
> + };
> +
> vdecsys: vdecsys@16000000 {
> compatible = "mediatek,mt8173-vdecsys", "syscon";
> reg = <0 0x16000000 0 0x1000>;
> #clock-cells = <1>;
> };
>
> + larb1: larb@16010000 {
> + compatible = "mediatek,mt8173-smi-larb";
> + reg = <0 0x16010000 0 0x1000>;
> + mediatek,smi = <&smi_common>;
> + power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
> + clocks = <&vdecsys CLK_VDEC_CKEN>,
> + <&vdecsys CLK_VDEC_LARB_CKEN>;
> + clock-names = "apb", "smi";
> + };
> +
> vencsys: vencsys@18000000 {
> compatible = "mediatek,mt8173-vencsys", "syscon";
> reg = <0 0x18000000 0 0x1000>;
> #clock-cells = <1>;
> };
>
> + larb3: larb@18001000 {
> + compatible = "mediatek,mt8173-smi-larb";
> + reg = <0 0x18001000 0 0x1000>;
> + mediatek,smi = <&smi_common>;
> + power-domains = <&scpsys MT8173_POWER_DOMAIN_VENC>;
> + clocks = <&vencsys CLK_VENC_CKE1>,
> + <&vencsys CLK_VENC_CKE0>;
> + clock-names = "apb", "smi";
> + };
> +
> vencltsys: vencltsys@19000000 {
> compatible = "mediatek,mt8173-vencltsys", "syscon";
> reg = <0 0x19000000 0 0x1000>;
> #clock-cells = <1>;
> };
> +
> + larb5: larb@19001000 {
> + compatible = "mediatek,mt8173-smi-larb";
> + reg = <0 0x19001000 0 0x1000>;
> + mediatek,smi = <&smi_common>;
> + power-domains = <&scpsys MT8173_POWER_DOMAIN_VENC_LT>;
> + clocks = <&vencltsys CLK_VENCLT_CKE1>,
> + <&vencltsys CLK_VENCLT_CKE0>;
> + clock-names = "apb", "smi";
> + };
> };
> };
>
> --
> 1.7.9.5
>
--
Daniel Kurtz | Software Engineer | djkurtz@google.com | 650.204.0722
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-21 17:11 ` Will Deacon
@ 2015-07-24 5:24 ` Yong Wu
2015-07-24 16:53 ` Will Deacon
2015-09-14 12:25 ` Yong Wu
1 sibling, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-07-24 5:24 UTC (permalink / raw)
To: Will Deacon
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
Hi Will,
Thanks for your review so detail.
When you are free, please help me check whether it's ok if it's
changed like below.
Thanks very much.
On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> Hello,
>
> This is looking better, but I still have some concerns.
>
> On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> > This patch is for ARM Short Descriptor Format.
> >
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > drivers/iommu/Kconfig | 18 +
> > drivers/iommu/Makefile | 1 +
> > drivers/iommu/io-pgtable-arm-short.c | 742 ++++++++++++++++++++++++++++++++++
> > drivers/iommu/io-pgtable-arm.c | 3 -
> > drivers/iommu/io-pgtable.c | 4 +
> > drivers/iommu/io-pgtable.h | 13 +
> > 6 files changed, 778 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/iommu/io-pgtable-arm-short.c
>
> [...]
>
> > +#define ARM_SHORT_PGDIR_SHIFT 20
> > +#define ARM_SHORT_PAGE_SHIFT 12
> > +#define ARM_SHORT_PTRS_PER_PTE \
> > + (1 << (ARM_SHORT_PGDIR_SHIFT - ARM_SHORT_PAGE_SHIFT))
> > +#define ARM_SHORT_BYTES_PER_PTE \
> > + (ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte))
> > +
> > +/* level 1 pagetable */
> > +#define ARM_SHORT_PGD_TYPE_PGTABLE BIT(0)
> > +#define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4))
> > +#define ARM_SHORT_PGD_TYPE_SECTION BIT(1)
> > +#define ARM_SHORT_PGD_PGTABLE_XN BIT(2)
>
> This should be PXN, but I'm not even sure why we care for a table at
> level 1.
I will delete it.
>
> > +#define ARM_SHORT_PGD_B BIT(2)
> > +#define ARM_SHORT_PGD_C BIT(3)
> > +#define ARM_SHORT_PGD_PGTABLE_NS BIT(3)
> > +#define ARM_SHORT_PGD_IMPLE BIT(9)
> > +#define ARM_SHORT_PGD_TEX0 BIT(12)
> > +#define ARM_SHORT_PGD_S BIT(16)
> > +#define ARM_SHORT_PGD_nG BIT(17)
> > +#define ARM_SHORT_PGD_SUPERSECTION BIT(18)
> > +#define ARM_SHORT_PGD_SECTION_NS BIT(19)
> > +
> > +#define ARM_SHORT_PGD_TYPE_SUPERSECTION \
> > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
> > +#define ARM_SHORT_PGD_SECTION_TYPE_MSK \
> > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
> > +#define ARM_SHORT_PGD_PGTABLE_TYPE_MSK \
> > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_TYPE_PGTABLE)
> > +#define ARM_SHORT_PGD_TYPE_IS_PGTABLE(pgd) \
> > + (((pgd) & ARM_SHORT_PGD_PGTABLE_TYPE_MSK) == ARM_SHORT_PGD_TYPE_PGTABLE)
> > +#define ARM_SHORT_PGD_TYPE_IS_SECTION(pgd) \
> > + (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == ARM_SHORT_PGD_TYPE_SECTION)
> > +#define ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(pgd) \
> > + (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == \
> > + ARM_SHORT_PGD_TYPE_SUPERSECTION)
> > +#define ARM_SHORT_PGD_PGTABLE_MSK 0xfffffc00
> > +#define ARM_SHORT_PGD_SECTION_MSK (~(SZ_1M - 1))
> > +#define ARM_SHORT_PGD_SUPERSECTION_MSK (~(SZ_16M - 1))
> > +
> > +/* level 2 pagetable */
> > +#define ARM_SHORT_PTE_TYPE_LARGE BIT(0)
> > +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> > +#define ARM_SHORT_PTE_TYPE_SMALL BIT(1)
> > +#define ARM_SHORT_PTE_B BIT(2)
> > +#define ARM_SHORT_PTE_C BIT(3)
> > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
> > +#define ARM_SHORT_PTE_IMPLE BIT(9)
>
> This is AP[2] for small pages.
Sorry, In our pagetable bit9 in PGD and PTE is PA[32] that is for the
dram size over 4G. I didn't care it is different in PTE of the standard
spec.
And I don't use the AP[2] currently, so I only delete this line in next
time.
>
> > +#define ARM_SHORT_PTE_S BIT(10)
> > +#define ARM_SHORT_PTE_nG BIT(11)
> > +#define ARM_SHORT_PTE_LARGE_TEX0 BIT(12)
> > +#define ARM_SHORT_PTE_LARGE_XN BIT(15)
> > +#define ARM_SHORT_PTE_LARGE_MSK (~(SZ_64K - 1))
> > +#define ARM_SHORT_PTE_SMALL_MSK (~(SZ_4K - 1))
> > +#define ARM_SHORT_PTE_TYPE_MSK \
> > + (ARM_SHORT_PTE_TYPE_LARGE | ARM_SHORT_PTE_TYPE_SMALL)
> > +#define ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(pte) \
> > + (((((pte) & ARM_SHORT_PTE_TYPE_MSK) >> 1) << 1)\
> > + == ARM_SHORT_PTE_TYPE_SMALL)
>
> I see what you're trying to do here, but the shifting is confusing. I
> think it's better doing something like:
>
> ((pte) & ARM_SHORT_PTE_TYPE_SMALL) == ARM_SHORT_PTE_TYPE_SMALL
>
> or even just:
>
> (pte) & ARM_SHORT_PTE_TYPE_SMALL
Thanks. I will use this:
((pte) & ARM_SHORT_PTE_TYPE_SMALL) == ARM_SHORT_PTE_TYPE_SMALL
This line may be close to the ARM_SHORT_PTE_TYPE_IS_LARGEPAGE below.
>
> > +#define ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(pte) \
> > + (((pte) & ARM_SHORT_PTE_TYPE_MSK) == ARM_SHORT_PTE_TYPE_LARGE)
> > +
> > +#define ARM_SHORT_PGD_IDX(a) ((a) >> ARM_SHORT_PGDIR_SHIFT)
> > +#define ARM_SHORT_PTE_IDX(a) \
> > + (((a) >> ARM_SHORT_PAGE_SHIFT) & (ARM_SHORT_PTRS_PER_PTE - 1))
> > +
> > +#define ARM_SHORT_GET_PTE_VA(pgd) \
> > + (phys_to_virt((unsigned long)pgd & ARM_SHORT_PGD_PGTABLE_MSK))
>
> Maybe better named as ARM_SHORT_GET_PGTABLE_VA ?
Thanks.
>
> > +
> > +#define ARM_SHORT_PTE_LARGE_GET_PROT(pte) \
> > + (((pte) & (~ARM_SHORT_PTE_LARGE_MSK)) & ~ARM_SHORT_PTE_TYPE_MSK)
> > +
> > +#define ARM_SHORT_PGD_GET_PROT(pgd) \
> > + (((pgd) & (~ARM_SHORT_PGD_SECTION_MSK)) & ~ARM_SHORT_PGD_SUPERSECTION)
> > +
> > +static bool selftest_running;
> > +
> > +static arm_short_iopte *
> > +arm_short_get_pte_in_pgd(arm_short_iopte pgd, unsigned int iova)
> > +{
> > + arm_short_iopte *pte;
> > +
> > + pte = ARM_SHORT_GET_PTE_VA(pgd);
> > + pte += ARM_SHORT_PTE_IDX(iova);
> > + return pte;
> > +}
> > +
> > +static void _arm_short_free_pgtable(struct arm_short_io_pgtable *data,
> > + arm_short_iopte *pgd)
> > +{
> > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > + arm_short_iopte *pte;
> > + int i;
> > +
> > + pte = ARM_SHORT_GET_PTE_VA(*pgd);
> > + for (i = 0; i < ARM_SHORT_PTRS_PER_PTE; i++) {
> > + if (pte[i] != 0)
> > + return;
> > + }
>
> Do you actually need this loop if you're not warning or returning an error?
I can't return an error here.
Here I only walk all the pte items in the pgd,
If there is some pte item exist, we do nothing, It is a ok case too.
If all the pte items are unmapped, We have to free the memory of
pgtable(kmem).
>
> > +
> > + /* Free whole pte and set pgd to zero while all pte is unmap */
> > + kmem_cache_free(data->ptekmem, pte);
> > + *pgd = 0;
>
> I still don't think this is safe. What stops the page table walker from
> speculatively walking freed memory?
Sorry. I didn't care the free flow this time.
I will change like below:
static bool _arm_short_free_pgtable(struct arm_short_io_pgtable *data,
arm_short_iopte *pgd)
{
/* if whole the pte items of this pgd are unmapped, return true.
* if others return fail.
*/
for(*****)
}
In arm_short_unmap, I will compare the return value and following the
free 5 steps as you suggestion below.
>
> > + tlb->flush_pgtable(pgd, sizeof(*pgd), data->iop.cookie);
> > +}
> > +
> > +static arm_short_iopte
> > +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
> > +{
> > + arm_short_iopte pteprot;
> > +
> > + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> > + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> > + ARM_SHORT_PTE_TYPE_SMALL;
> > + if (prot & IOMMU_CACHE)
> > + pteprot |= ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
> > + if (prot & IOMMU_WRITE)
> > + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> > + ARM_SHORT_PTE_SMALL_TEX0;
>
> This doesn't make any sense. TEX[2:0] is all about memory attributes, not
> permissions, so you're making the mapping write-back, write-allocate but
> that's not what the IOMMU_* values are about.
I will delete it.
>
> > + if (prot & IOMMU_NOEXEC)
> > + pteprot |= large ? ARM_SHORT_PTE_LARGE_XN :
> > + ARM_SHORT_PTE_SMALL_XN;
> > + return pteprot;
> > +}
> > +
> > +static arm_short_iopte
> > +__arm_short_pgd_prot(struct arm_short_io_pgtable *data, int prot, bool super)
> > +{
> > + arm_short_iopte pgdprot;
> > +
> > + pgdprot = ARM_SHORT_PGD_S | ARM_SHORT_PGD_nG;
> > + pgdprot |= super ? ARM_SHORT_PGD_TYPE_SUPERSECTION :
> > + ARM_SHORT_PGD_TYPE_SECTION;
> > + if (prot & IOMMU_CACHE)
> > + pgdprot |= ARM_SHORT_PGD_C | ARM_SHORT_PGD_B;
> > + if (prot & IOMMU_WRITE)
> > + pgdprot |= ARM_SHORT_PGD_TEX0;
>
> Likewise.
I will delete it.
>
> > + if (prot & IOMMU_NOEXEC)
> > + pgdprot |= ARM_SHORT_PGD_SECTION_XN;
> > + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
> > + pgdprot |= ARM_SHORT_PGD_SECTION_NS;
> > + return pgdprot;
> > +}
> > +
> > +static arm_short_iopte
> > +__arm_short_pte_prot_split(struct arm_short_io_pgtable *data,
> > + arm_short_iopte pgdprot,
> > + arm_short_iopte pteprot_large,
> > + bool large)
> > +{
> > + arm_short_iopte pteprot = 0;
> > +
> > + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> > + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> > + ARM_SHORT_PTE_TYPE_SMALL;
> > + /* section to pte prot */
> > + if (pgdprot & ARM_SHORT_PGD_C)
> > + pteprot |= ARM_SHORT_PTE_C;
> > + if (pgdprot & ARM_SHORT_PGD_B)
> > + pteprot |= ARM_SHORT_PTE_B;
> > + if (pgdprot & ARM_SHORT_PGD_TEX0)
> > + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> > + ARM_SHORT_PTE_SMALL_TEX0;
Here I also delete it.
> > + if (pgdprot & ARM_SHORT_PGD_nG)
> > + pteprot |= ARM_SHORT_PTE_nG;
> > + if (pgdprot & ARM_SHORT_PGD_SECTION_XN)
> > + pteprot |= large ? ARM_SHORT_PTE_LARGE_XN :
> > + ARM_SHORT_PTE_SMALL_XN;
> > +
> > + /* large page to small page pte prot. Only large page may split */
> > + if (pteprot_large && !large) {
> > + if (pteprot_large & ARM_SHORT_PTE_LARGE_TEX0)
> > + pteprot |= ARM_SHORT_PTE_SMALL_TEX0;
Delete this too.
> > + if (pteprot_large & ARM_SHORT_PTE_LARGE_XN)
> > + pteprot |= ARM_SHORT_PTE_SMALL_XN;
> > + }
> > + return pteprot;
> > +}
> > +
> > +static arm_short_iopte
> > +__arm_short_pgtable_prot(struct arm_short_io_pgtable *data, bool noexec)
> > +{
> > + arm_short_iopte pgdprot = 0;
> > +
> > + pgdprot = ARM_SHORT_PGD_TYPE_PGTABLE;
> > + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
> > + pgdprot |= ARM_SHORT_PGD_PGTABLE_NS;
> > + if (noexec)
> > + pgdprot |= ARM_SHORT_PGD_PGTABLE_XN;
>
> I don't think you need to worry about XN bits for PGTABLEs.
I will delete it.
MTK don't have XN bit in PGTABLEs,
I prepared to add all the bits according to the standard spec, and add
MTK quirk to disable this bit for our special bit.
>
> > + return pgdprot;
> > +}
> > +
> > +static int
> > +_arm_short_map(struct arm_short_io_pgtable *data,
> > + unsigned int iova, phys_addr_t paddr,
> > + arm_short_iopte pgdprot, arm_short_iopte pteprot,
> > + bool large)
> > +{
> > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > + arm_short_iopte *pgd = data->pgd, *pte;
> > + void *cookie = data->iop.cookie, *pte_va;
> > + unsigned int ptenr = large ? 16 : 1;
> > + int i, quirk = data->iop.cfg.quirks;
> > + bool ptenew = false;
> > +
> > + pgd += ARM_SHORT_PGD_IDX(iova);
> > +
> > + if (!pteprot) { /* section or supersection */
> > + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
> > + pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
> > + pte = pgd;
> > + pteprot = pgdprot;
> > + } else { /* page or largepage */
> > + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > + if (large) { /* special Bit */
>
> This definitely needs a better comment! What exactly are you doing here
> and what is that quirk all about?
I use this quirk is for MTK Special Bit as we don't have the XN bit in
pagetable.
And the TEX0 will be deleted. Then it will like this:
//==========
if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
if (large)
pteprot &= ~ARM_SHORT_PTE_LARGE_XN;
else
pteprot &= ~ARM_SHORT_PTE_SMALL_XN;
//=========
And move the comment close to the definition of the quirk.
like this:
#define IO_PGTABLE_QUIRK_SHORT_MTK BIT(2)/* MTK Speical Bit
*/
>
> > + if (pteprot & ARM_SHORT_PTE_LARGE_TEX0) {
> > + pteprot &= ~ARM_SHORT_PTE_LARGE_TEX0;
> > + pteprot |= ARM_SHORT_PTE_SMALL_TEX0;
> > + }
> > + pteprot &= ~ARM_SHORT_PTE_LARGE_XN;
> > + } else {
> > + pteprot &= ~ARM_SHORT_PTE_SMALL_XN;
> > + }
> > + }
> > +
> > + if (!(*pgd)) {
> > + pte_va = kmem_cache_zalloc(data->ptekmem, GFP_ATOMIC);
> > + if (unlikely(!pte_va))
> > + return -ENOMEM;
> > + ptenew = true;
> > + *pgd = virt_to_phys(pte_va) | pgdprot;
> > + kmemleak_ignore(pte_va);
> > + tlb->flush_pgtable(pgd, sizeof(*pgd), cookie);
>
> I think you need to flush this before it becomes visible to the walker.
I have flushed pgtable here, Do you meaning flush tlb here?
>From the comment below, you don't think tlb-flush is necessary while the
new pte item is from invalid -> valid,
>
> > + }
> > + pte = arm_short_get_pte_in_pgd(*pgd, iova);
> > + }
> > +
> > + pteprot |= (arm_short_iopte)paddr;
> > + for (i = 0; i < ptenr; i++) {
> > + if (pte[i]) {/* Someone else may have allocated for this pte */
> > + WARN_ON(!selftest_running);
> > + goto err_exist_pte;
> > + }
> > + pte[i] = pteprot;
> > + }
> > + tlb->flush_pgtable(pte, ptenr * sizeof(*pte), cookie);
> > +
> > + return 0;
> > +
> > +err_exist_pte:
> > + while (i--)
> > + pte[i] = 0;
> > + if (ptenew)
> > + kmem_cache_free(data->ptekmem, pte_va);
> > + return -EEXIST;
> > +}
> > +
> > +static int arm_short_map(struct io_pgtable_ops *ops, unsigned long iova,
> > + phys_addr_t paddr, size_t size, int prot)
> > +{
> > + struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > + int ret;
> > + arm_short_iopte pgdprot = 0, pteprot = 0;
> > + bool large;
> > +
> > + /* If no access, then nothing to do */
> > + if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
> > + return 0;
> > +
> > + switch (size) {
> > + case SZ_4K:
> > + case SZ_64K:
> > + large = (size == SZ_64K) ? true : false;
> > + pteprot = __arm_short_pte_prot(data, prot, large);
> > + pgdprot = __arm_short_pgtable_prot(data, prot & IOMMU_NOEXEC);
> > + break;
> > +
> > + case SZ_1M:
> > + case SZ_16M:
> > + large = (size == SZ_16M) ? true : false;
> > + pgdprot = __arm_short_pgd_prot(data, prot, large);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (WARN_ON((iova | paddr) & (size - 1)))
> > + return -EINVAL;
> > +
> > + ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
> > +
> > + tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
> > + tlb->tlb_sync(data->iop.cookie);
>
> In _arm_short_map, it looks like you can only go from invalid -> valid,
> so why do you need to flush the TLB here?
I will delete tlb flush here.
Then the tlb flush is only called after unmap and split.
>
> > + return ret;
> > +}
> > +
> > +static phys_addr_t arm_short_iova_to_phys(struct io_pgtable_ops *ops,
> > + unsigned long iova)
> > +{
> > + struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > + arm_short_iopte *pte, *pgd = data->pgd;
> > + phys_addr_t pa = 0;
> > +
> > + pgd += ARM_SHORT_PGD_IDX(iova);
> > +
> > + if (ARM_SHORT_PGD_TYPE_IS_PGTABLE(*pgd)) {
> > + pte = arm_short_get_pte_in_pgd(*pgd, iova);
> > +
> > + if (ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(*pte)) {
> > + pa = (*pte) & ARM_SHORT_PTE_LARGE_MSK;
> > + pa |= iova & ~ARM_SHORT_PTE_LARGE_MSK;
> > + } else if (ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(*pte)) {
> > + pa = (*pte) & ARM_SHORT_PTE_SMALL_MSK;
> > + pa |= iova & ~ARM_SHORT_PTE_SMALL_MSK;
> > + }
> > + } else if (ARM_SHORT_PGD_TYPE_IS_SECTION(*pgd)) {
> > + pa = (*pgd) & ARM_SHORT_PGD_SECTION_MSK;
> > + pa |= iova & ~ARM_SHORT_PGD_SECTION_MSK;
> > + } else if (ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(*pgd)) {
> > + pa = (*pgd) & ARM_SHORT_PGD_SUPERSECTION_MSK;
> > + pa |= iova & ~ARM_SHORT_PGD_SUPERSECTION_MSK;
> > + }
> > +
> > + return pa;
> > +}
> > +
> > +static int
> > +arm_short_split_blk_unmap(struct io_pgtable_ops *ops, unsigned int iova,
> > + phys_addr_t paddr, size_t size,
> > + arm_short_iopte pgdprotup, arm_short_iopte pteprotup,
> > + size_t blk_size)
> > +{
> > + struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > + struct io_pgtable_cfg *cfg = &data->iop.cfg;
> > + unsigned long *pgbitmap = &cfg->pgsize_bitmap;
> > + unsigned int blk_base, blk_start, blk_end;
> > + arm_short_iopte pgdprot, pteprot;
> > + size_t mapsize = 0, nextmapsize;
> > + phys_addr_t blk_paddr;
> > + int ret;
> > + unsigned int i;
> > +
> > + /* find the nearest mapsize */
> > + for (i = find_first_bit(pgbitmap, BITS_PER_LONG);
> > + i < BITS_PER_LONG && ((1 << i) < blk_size) &&
> > + IS_ALIGNED(size, 1 << i);
> > + i = find_next_bit(pgbitmap, BITS_PER_LONG, i + 1))
> > + mapsize = 1 << i;
> > +
> > + if (WARN_ON(!mapsize))
> > + return 0; /* Bytes unmapped */
> > + nextmapsize = 1 << i;
> > +
> > + blk_base = iova & ~(blk_size - 1);
> > + blk_start = blk_base;
> > + blk_end = blk_start + blk_size;
> > + blk_paddr = paddr;
> > +
> > + for (; blk_start < blk_end;
> > + blk_start += mapsize, blk_paddr += mapsize) {
> > + /* Unmap! */
> > + if (blk_start == iova)
> > + continue;
> > +
> > + /* Try to upper map */
> > + if (blk_base != blk_start &&
> > + IS_ALIGNED(blk_start | blk_paddr, nextmapsize) &&
> > + mapsize != nextmapsize) {
> > + mapsize = nextmapsize;
> > + i = find_next_bit(pgbitmap, BITS_PER_LONG, i + 1);
> > + if (i < BITS_PER_LONG)
> > + nextmapsize = 1 << i;
> > + }
> > +
> > + if (mapsize == SZ_1M) {
> > + pgdprot = pgdprotup;
> > + pgdprot |= __arm_short_pgd_prot(data, 0, false);
> > + pteprot = 0;
> > + } else { /* small or large page */
> > + bool noexec = (blk_size == SZ_64K) ?
> > + (pteprotup & ARM_SHORT_PTE_LARGE_XN) :
> > + (pgdprotup & ARM_SHORT_PGD_SECTION_XN);
> > +
> > + pteprot = __arm_short_pte_prot_split(
> > + data, pgdprotup, pteprotup,
> > + mapsize == SZ_64K);
> > + pgdprot = __arm_short_pgtable_prot(data, noexec);
> > + }
> > +
> > + ret = _arm_short_map(data, blk_start, blk_paddr, pgdprot,
> > + pteprot, mapsize == SZ_64K);
> > + if (ret < 0) {
> > + /* Free the table we allocated */
> > + arm_short_iopte *pgd = data->pgd, *pte;
> > +
> > + pgd += ARM_SHORT_PGD_IDX(blk_base);
> > + if (*pgd) {
> > + pte = ARM_SHORT_GET_PTE_VA(*pgd);
> > + kmem_cache_free(data->ptekmem, pte);
> > + *pgd = 0;
> > + tlb->flush_pgtable(pgd, sizeof(*pgd),
> > + data->iop.cookie);
>
> Again, the ordering looks totally wrong here. You need to:
>
> (1) Zero the pgd pointer
> (2) Flush the pointer, so the walker can no longer follow the page table
> (3) Invalidate the TLB, so we don't have any cached intermediate walks
> (4) Sync the TLB
> (5) Free the memory
>
> That said, I don't fully follow the logic here.
Sorry. I didn't care the free flow specially. I will change it like
below:
//=======
*pgd = 0;
tlb->flush_pgtable(pgd, sizeof(*pgd), data->iop.cookie);
tlb->tlb_add_flush(blk_base, blk_size, true, data->iop.cookie);
tlb->tlb_sync(data->iop.cookie);
kmem_cache_free(data->ptekmem, pte);
//============
>
> > + }
> > + return 0;/* Bytes unmapped */
> > + }
> > + tlb->tlb_add_flush(blk_start, mapsize, true, data->iop.cookie);
> > + tlb->tlb_sync(data->iop.cookie);
>
> Why are you doing this here for every iteration?
I will move it out from the loop.
> Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-21 14:59 ` Will Deacon
@ 2015-07-24 5:43 ` Yong Wu
2015-07-24 16:55 ` Will Deacon
0 siblings, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-07-24 5:43 UTC (permalink / raw)
To: Will Deacon
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Tue, 2015-07-21 at 15:59 +0100, Will Deacon wrote:
> Hi Yong Wu,
>
> On Thu, Jul 16, 2015 at 10:04:34AM +0100, Yong Wu wrote:
> > This patch adds support for mediatek m4u (MultiMedia Memory Management
> > Unit).
>
> [...]
>
> > +static void mtk_iommu_tlb_flush_all(void *cookie)
> > +{
> > + struct mtk_iommu_domain *domain = cookie;
> > + void __iomem *base;
> > +
> > + base = domain->data->base;
> > + writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> > + writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
>
> This needs to be synchronous, so you probably want to call
> mtk_iommu_tlb_sync at the end.
>From our spec, we have to wait until HW done after tlb flush range.
But it don't need wait after tlb flush all.
so It isn't necessary to add mtk_iommu_tlb_sync in tlb_flush_all here.
>
> > +}
> > +
> > +static void mtk_iommu_tlb_add_flush(unsigned long iova, size_t size,
> > + bool leaf, void *cookie)
> > +{
> > + struct mtk_iommu_domain *domain = cookie;
> > + void __iomem *base = domain->data->base;
> > + unsigned int iova_start = iova, iova_end = iova + size - 1;
> > +
> > + writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> > +
> > + writel(iova_start, base + REG_MMU_INVLD_START_A);
> > + writel(iova_end, base + REG_MMU_INVLD_END_A);
> > + writel(F_MMU_INV_RANGE, base + REG_MMU_INVALIDATE);
>
> Why are you using writel instead of writel_relaxed? I asked this before
> but I don't think you replied.
Sorry, I didn't notice _relax last time. I will improve in next version.
Thanks very much.
>
> Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-24 5:24 ` Yong Wu
@ 2015-07-24 16:53 ` Will Deacon
2015-07-27 4:21 ` Yong Wu
0 siblings, 1 reply; 34+ messages in thread
From: Will Deacon @ 2015-07-24 16:53 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Fri, Jul 24, 2015 at 06:24:26AM +0100, Yong Wu wrote:
> On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> > > +/* level 2 pagetable */
> > > +#define ARM_SHORT_PTE_TYPE_LARGE BIT(0)
> > > +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> > > +#define ARM_SHORT_PTE_TYPE_SMALL BIT(1)
> > > +#define ARM_SHORT_PTE_B BIT(2)
> > > +#define ARM_SHORT_PTE_C BIT(3)
> > > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
> > > +#define ARM_SHORT_PTE_IMPLE BIT(9)
> >
> > This is AP[2] for small pages.
>
> Sorry, In our pagetable bit9 in PGD and PTE is PA[32] that is for the
> dram size over 4G. I didn't care it is different in PTE of the standard
> spec.
> And I don't use the AP[2] currently, so I only delete this line in next
> time.
Is this related to the "special bit". What would be good is a comment
next to the #define for the quirk describing *exactly* that differs in
your implementation. Without that, it's very difficult to know what is
intentional and what is actually broken.
> > > +static arm_short_iopte
> > > +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
> > > +{
> > > + arm_short_iopte pteprot;
> > > +
> > > + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> > > + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> > > + ARM_SHORT_PTE_TYPE_SMALL;
> > > + if (prot & IOMMU_CACHE)
> > > + pteprot |= ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
> > > + if (prot & IOMMU_WRITE)
> > > + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> > > + ARM_SHORT_PTE_SMALL_TEX0;
> >
> > This doesn't make any sense. TEX[2:0] is all about memory attributes, not
> > permissions, so you're making the mapping write-back, write-allocate but
> > that's not what the IOMMU_* values are about.
>
> I will delete it.
Well, can you not control mapping permissions with the AP bits? The idea
of the IOMMU flags are:
IOMMU_CACHE : Install a normal, cacheable mapping (you've got this right)
IOMMU_READ : Allow read access for the device
IOMMU_WRITE : Allow write access for the device
IOMMU_NOEXEC : Disallow execute access for the device
so the caller to iommu_map passes in a bitmap of these, which you need to
encode in the page-table entry.
> > > +static int
> > > +_arm_short_map(struct arm_short_io_pgtable *data,
> > > + unsigned int iova, phys_addr_t paddr,
> > > + arm_short_iopte pgdprot, arm_short_iopte pteprot,
> > > + bool large)
> > > +{
> > > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > > + arm_short_iopte *pgd = data->pgd, *pte;
> > > + void *cookie = data->iop.cookie, *pte_va;
> > > + unsigned int ptenr = large ? 16 : 1;
> > > + int i, quirk = data->iop.cfg.quirks;
> > > + bool ptenew = false;
> > > +
> > > + pgd += ARM_SHORT_PGD_IDX(iova);
> > > +
> > > + if (!pteprot) { /* section or supersection */
> > > + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
> > > + pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
> > > + pte = pgd;
> > > + pteprot = pgdprot;
> > > + } else { /* page or largepage */
> > > + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > + if (large) { /* special Bit */
> >
> > This definitely needs a better comment! What exactly are you doing here
> > and what is that quirk all about?
>
> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> pagetable.
I'm still not really clear about what this is.
> > > + if (!(*pgd)) {
> > > + pte_va = kmem_cache_zalloc(data->ptekmem, GFP_ATOMIC);
> > > + if (unlikely(!pte_va))
> > > + return -ENOMEM;
> > > + ptenew = true;
> > > + *pgd = virt_to_phys(pte_va) | pgdprot;
> > > + kmemleak_ignore(pte_va);
> > > + tlb->flush_pgtable(pgd, sizeof(*pgd), cookie);
> >
> > I think you need to flush this before it becomes visible to the walker.
>
> I have flushed pgtable here, Do you meaning flush tlb here?
No. afaict, you allocate the pte table using kmem_cache_zalloc but you never
flush it. However, you update the pgd to point at this table, so the walker
can potentially see garbage instead of the zeroed entries.
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-24 5:43 ` Yong Wu
@ 2015-07-24 16:55 ` Will Deacon
2015-07-27 4:24 ` Yong Wu
0 siblings, 1 reply; 34+ messages in thread
From: Will Deacon @ 2015-07-24 16:55 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Fri, Jul 24, 2015 at 06:43:13AM +0100, Yong Wu wrote:
> On Tue, 2015-07-21 at 15:59 +0100, Will Deacon wrote:
> > On Thu, Jul 16, 2015 at 10:04:34AM +0100, Yong Wu wrote:
> > > +static void mtk_iommu_tlb_flush_all(void *cookie)
> > > +{
> > > + struct mtk_iommu_domain *domain = cookie;
> > > + void __iomem *base;
> > > +
> > > + base = domain->data->base;
> > > + writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> > > + writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
> >
> > This needs to be synchronous, so you probably want to call
> > mtk_iommu_tlb_sync at the end.
>
> From our spec, we have to wait until HW done after tlb flush range.
> But it don't need wait after tlb flush all.
> so It isn't necessary to add mtk_iommu_tlb_sync in tlb_flush_all here.
Okey doke, but I'm surprised you don't need a subsequent DSB or read-back.
What if the writel is buffered on the way to the IOMMU?
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-24 16:53 ` Will Deacon
@ 2015-07-27 4:21 ` Yong Wu
2015-07-27 14:05 ` Robin Murphy
[not found] ` <1438329337.25925.147.camel@mhfsdcap03>
0 siblings, 2 replies; 34+ messages in thread
From: Yong Wu @ 2015-07-27 4:21 UTC (permalink / raw)
To: Will Deacon
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Fri, 2015-07-24 at 17:53 +0100, Will Deacon wrote:
> On Fri, Jul 24, 2015 at 06:24:26AM +0100, Yong Wu wrote:
> > On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > > On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> > > > +/* level 2 pagetable */
> > > > +#define ARM_SHORT_PTE_TYPE_LARGE BIT(0)
> > > > +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> > > > +#define ARM_SHORT_PTE_TYPE_SMALL BIT(1)
> > > > +#define ARM_SHORT_PTE_B BIT(2)
> > > > +#define ARM_SHORT_PTE_C BIT(3)
> > > > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
> > > > +#define ARM_SHORT_PTE_IMPLE BIT(9)
> > >
> > > This is AP[2] for small pages.
> >
> > Sorry, In our pagetable bit9 in PGD and PTE is PA[32] that is for the
> > dram size over 4G. I didn't care it is different in PTE of the standard
> > spec.
> > And I don't use the AP[2] currently, so I only delete this line in next
> > time.
>
> Is this related to the "special bit". What would be good is a comment
> next to the #define for the quirk describing *exactly* that differs in
> your implementation. Without that, it's very difficult to know what is
> intentional and what is actually broken.
I will add the comment alongside the #define.
>
> > > > +static arm_short_iopte
> > > > +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
> > > > +{
> > > > + arm_short_iopte pteprot;
> > > > +
> > > > + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> > > > + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> > > > + ARM_SHORT_PTE_TYPE_SMALL;
> > > > + if (prot & IOMMU_CACHE)
> > > > + pteprot |= ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
> > > > + if (prot & IOMMU_WRITE)
> > > > + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> > > > + ARM_SHORT_PTE_SMALL_TEX0;
> > >
> > > This doesn't make any sense. TEX[2:0] is all about memory attributes, not
> > > permissions, so you're making the mapping write-back, write-allocate but
> > > that's not what the IOMMU_* values are about.
> >
> > I will delete it.
>
> Well, can you not control mapping permissions with the AP bits? The idea
> of the IOMMU flags are:
>
> IOMMU_CACHE : Install a normal, cacheable mapping (you've got this right)
> IOMMU_READ : Allow read access for the device
> IOMMU_WRITE : Allow write access for the device
> IOMMU_NOEXEC : Disallow execute access for the device
>
> so the caller to iommu_map passes in a bitmap of these, which you need to
> encode in the page-table entry.
>From the spec, AP[2] differentiate the read/write and readonly.
How about this?:
//===============
#define ARM_SHORT_PGD_FULL_ACCESS (3 << 10)
#define ARM_SHORT_PGD_RDONLY BIT(15)
pgdprot |= ARM_SHORT_PGD_FULL_ACCESS;/* or other names? */
if(!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
pgdprot |= ARM_SHORT_PGD_RDONLY;
//===============
pte is the same.
Sorry, Our HW don't meet the standard spec fully. it don't implement the
AP bits.
>
> > > > +static int
> > > > +_arm_short_map(struct arm_short_io_pgtable *data,
> > > > + unsigned int iova, phys_addr_t paddr,
> > > > + arm_short_iopte pgdprot, arm_short_iopte pteprot,
> > > > + bool large)
> > > > +{
> > > > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > > > + arm_short_iopte *pgd = data->pgd, *pte;
> > > > + void *cookie = data->iop.cookie, *pte_va;
> > > > + unsigned int ptenr = large ? 16 : 1;
> > > > + int i, quirk = data->iop.cfg.quirks;
> > > > + bool ptenew = false;
> > > > +
> > > > + pgd += ARM_SHORT_PGD_IDX(iova);
> > > > +
> > > > + if (!pteprot) { /* section or supersection */
> > > > + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
> > > > + pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
> > > > + pte = pgd;
> > > > + pteprot = pgdprot;
> > > > + } else { /* page or largepage */
> > > > + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > > + if (large) { /* special Bit */
> > >
> > > This definitely needs a better comment! What exactly are you doing here
> > > and what is that quirk all about?
> >
> > I use this quirk is for MTK Special Bit as we don't have the XN bit in
> > pagetable.
>
> I'm still not really clear about what this is.
There is some difference between the standard spec and MTK HW,
Our hw don't implement some bits, like XN and AP.
So I add a quirk for MTK special.
>
> > > > + if (!(*pgd)) {
> > > > + pte_va = kmem_cache_zalloc(data->ptekmem, GFP_ATOMIC);
> > > > + if (unlikely(!pte_va))
> > > > + return -ENOMEM;
> > > > + ptenew = true;
> > > > + *pgd = virt_to_phys(pte_va) | pgdprot;
> > > > + kmemleak_ignore(pte_va);
> > > > + tlb->flush_pgtable(pgd, sizeof(*pgd), cookie);
> > >
> > > I think you need to flush this before it becomes visible to the walker.
> >
> > I have flushed pgtable here, Do you meaning flush tlb here?
>
> No. afaict, you allocate the pte table using kmem_cache_zalloc but you never
> flush it. However, you update the pgd to point at this table, so the walker
> can potentially see garbage instead of the zeroed entries.
Thanks. I will add :
tlb->flush_pgtable(pte_va, ARM_SHORT_BYTES_PER_PTE, cookie);
>
> Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-24 16:55 ` Will Deacon
@ 2015-07-27 4:24 ` Yong Wu
2015-07-27 15:48 ` Will Deacon
0 siblings, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-07-27 4:24 UTC (permalink / raw)
To: Will Deacon
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Fri, 2015-07-24 at 17:55 +0100, Will Deacon wrote:
> On Fri, Jul 24, 2015 at 06:43:13AM +0100, Yong Wu wrote:
> > On Tue, 2015-07-21 at 15:59 +0100, Will Deacon wrote:
> > > On Thu, Jul 16, 2015 at 10:04:34AM +0100, Yong Wu wrote:
> > > > +static void mtk_iommu_tlb_flush_all(void *cookie)
> > > > +{
> > > > + struct mtk_iommu_domain *domain = cookie;
> > > > + void __iomem *base;
> > > > +
> > > > + base = domain->data->base;
> > > > + writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> > > > + writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
> > >
> > > This needs to be synchronous, so you probably want to call
> > > mtk_iommu_tlb_sync at the end.
> >
> > From our spec, we have to wait until HW done after tlb flush range.
> > But it don't need wait after tlb flush all.
> > so It isn't necessary to add mtk_iommu_tlb_sync in tlb_flush_all here.
>
> Okey doke, but I'm surprised you don't need a subsequent DSB or read-back.
> What if the writel is buffered on the way to the IOMMU?
Then I change to this:
//==========
writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
writel_relaxed(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
dsb(ishst);
//===========
dsb or mb(). which one is better here?
>
> Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-16 9:04 ` [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu
2015-07-21 14:59 ` Will Deacon
@ 2015-07-27 13:23 ` Robin Murphy
2015-07-27 15:31 ` Russell King - ARM Linux
2015-07-29 6:32 ` Yong Wu
1 sibling, 2 replies; 34+ messages in thread
From: Robin Murphy @ 2015-07-27 13:23 UTC (permalink / raw)
To: Yong Wu, Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger
Cc: Will Deacon, Daniel Kurtz, Tomasz Figa, Lucas Stach, Rob Herring,
Catalin Marinas, linux-mediatek, Sasha Hauer, srv_heupstream,
devicetree, linux-kernel, linux-arm-kernel, iommu, pebolle, arnd,
mitchelh, cloud.chou, frederic.chen
On 16/07/15 10:04, Yong Wu wrote:
> This patch adds support for mediatek m4u (MultiMedia Memory Management
> Unit).
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
[...]
> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> +{
> + struct mtk_iommu_domain *domain = cookie;
> + unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> +
> + dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> + size, DMA_TO_DEVICE);
Nit: this looks like it may as well be dma_map_single.
It would probably be worth following it with a matching unmap too, just
to avoid any possible leakage bugs (especially if this M4U ever appears
in a SoC supporting RAM above the 32-bit boundary).
> +}
> +
[...]
> +static int mtk_iommu_parse_dt(struct platform_device *pdev,
> + struct mtk_iommu_data *data)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *ofnode;
> + struct resource *res;
> + int i;
> +
> + ofnode = dev->of_node;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->base))
> + return PTR_ERR(data->base);
> +
> + data->irq = platform_get_irq(pdev, 0);
> + if (data->irq < 0)
> + return data->irq;
> +
> + data->bclk = devm_clk_get(dev, "bclk");
> + if (IS_ERR(data->bclk))
> + return PTR_ERR(data->bclk);
> +
> + data->larb_nr = of_count_phandle_with_args(
> + ofnode, "mediatek,larb", NULL);
> + if (data->larb_nr < 0)
> + return data->larb_nr;
> +
> + for (i = 0; i < data->larb_nr; i++) {
> + struct device_node *larbnode;
> + struct platform_device *plarbdev;
> +
> + larbnode = of_parse_phandle(ofnode, "mediatek,larb", i);
> + if (!larbnode)
> + return -EINVAL;
> +
> + plarbdev = of_find_device_by_node(larbnode);
> + of_node_put(larbnode);
> + if (!plarbdev)
> + return -EPROBE_DEFER;
> + data->larbdev[i] = &plarbdev->dev;
> + }
At a glance this seems like a job for of_parse_phandle_with_args, but I
may be missing something subtle.
> + return 0;
> +}
> +
[...]
> +static int mtk_iommu_init_domain_context(struct mtk_iommu_domain *dom)
> +{
> + int ret;
> +
> + if (dom->iop)
> + return 0;
> +
> + spin_lock_init(&dom->pgtlock);
> + dom->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS |
> + IO_PGTABLE_QUIRK_SHORT_SUPERSECTION |
> + IO_PGTABLE_QUIRK_SHORT_MTK;
> + dom->cfg.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
> + dom->cfg.ias = 32;
> + dom->cfg.oas = 32;
> + dom->cfg.tlb = &mtk_iommu_gather_ops;
> +
> + dom->iop = alloc_io_pgtable_ops(ARM_SHORT_DESC, &dom->cfg, dom);
> + if (!dom->iop) {
> + pr_err("Failed to alloc io pgtable\n");
> + return -EINVAL;
> + }
> +
> + /* Update our support page sizes bitmap */
> + mtk_iommu_ops.pgsize_bitmap = dom->cfg.pgsize_bitmap;
> +
> + ret = mtk_iommu_hw_init(dom);
> + if (ret)
> + free_io_pgtable_ops(dom->iop);
> +
> + return ret;
> +}
I don't see that you need the above function at all - since your
pagetable config is fixed and doesn't have any depency on which IOMMU
you're attaching to, can't you just do all of that straight away in
domain_alloc?
> +static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
> +{
> + struct mtk_iommu_domain *priv;
> +
> + /* We only support unmanaged domains for now */
> + if (type != IOMMU_DOMAIN_UNMANAGED)
> + return NULL;
Have you had a chance to play with the latest DMA mapping series now
that I've integrated the default domain changes? I think if you handled
IOMMU_DOMAIN_DMA requests here and made them always return the
(preallocated) private domain, you should be able to get rid of the
tricks in attach_device completely.
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return NULL;
> +
> + priv->domain.geometry.aperture_start = 0;
> + priv->domain.geometry.aperture_end = (1ULL << 32) - 1;
DMA_BIT_MASK(32) ?
> + priv->domain.geometry.force_aperture = true;
> +
> + return &priv->domain;
> +}
> +
[...]
> +static int mtk_iommu_add_device(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom;
> + struct iommu_group *group;
> + struct mtk_iommu_client_priv *devhead;
> + int ret;
> +
> + if (!dev->archdata.dma_ops)/* Not a iommu client device */
> + return -ENODEV;
I think archdata.dma_ops is the wrong thing to test here. It would be
better to use archdata.iommu, since you go on to dereference that
unconditionally anyway, plus then you only depend on your own of_xlate
behaviour, rather than some quirk of the arch code (which could quietly
change in future).
> + group = iommu_group_get(dev);
> + if (!group) {
> + group = iommu_group_alloc();
> + if (IS_ERR(group)) {
> + dev_err(dev, "Failed to allocate IOMMU group\n");
> + return PTR_ERR(group);
> + }
> + }
> +
> + ret = iommu_group_add_device(group, dev);
> + if (ret) {
> + dev_err(dev, "Failed to add IOMMU group\n");
> + goto err_group_put;
> + }
> +
> + /* get the mtk_iommu_domain from the iommu device */
> + devhead = dev->archdata.iommu;
> + mtkdom = devhead->imudev->archdata.iommu;
> + ret = iommu_attach_group(&mtkdom->domain, group);
> + if (ret)
> + dev_err(dev, "Failed to attach IOMMU group\n");
> +
> +err_group_put:
> + iommu_group_put(group);
> + return ret;
> +}
> +
[...]
> +static struct iommu_ops mtk_iommu_ops = {
> + .domain_alloc = mtk_iommu_domain_alloc,
> + .domain_free = mtk_iommu_domain_free,
> + .attach_dev = mtk_iommu_attach_device,
> + .detach_dev = mtk_iommu_detach_device,
> + .map = mtk_iommu_map,
> + .unmap = mtk_iommu_unmap,
> + .map_sg = default_iommu_map_sg,
> + .iova_to_phys = mtk_iommu_iova_to_phys,
> + .add_device = mtk_iommu_add_device,
> + .of_xlate = mtk_iommu_of_xlate,
> + .pgsize_bitmap = -1UL,
As above, if you know the hardware only supports a single descriptor
format with a fixed set of page sizes, why not just represent that directly?
> +};
> +
[...]
> +static int mtk_iommu_suspend(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
> + struct mtk_iommu_data *data = mtkdom->data;
> + void __iomem *base = data->base;
> + unsigned int i = 0;
> +
> + data->reg[i++] = readl(base + REG_MMU_PT_BASE_ADDR);
Redundancy nit: any particular reason for saving this here rather than
simply restoring it from mtkdom->cfg.arm_short_cfg.ttbr[0] on resume?
> + data->reg[i++] = readl(base + REG_MMU_STANDARD_AXI_MODE);
> + data->reg[i++] = readl(base + REG_MMU_DCM_DIS);
> + data->reg[i++] = readl(base + REG_MMU_CTRL_REG);
> + data->reg[i++] = readl(base + REG_MMU_IVRP_PADDR);
> + data->reg[i++] = readl(base + REG_MMU_INT_CONTROL0);
> + data->reg[i++] = readl(base + REG_MMU_INT_MAIN_CONTROL);
Nit: given that these are fairly arbitrary discontiguous registers so
you can't have a simple loop over the array, it might be clearer to
replace the array with an equivalent struct, e.g.:
struct mtk_iommu_suspend_regs {
u32 standard_axi_mode;
u32 dcm_dis;
...
}
...
data->reg.dcm_dis = readl(base + REG_MMU_DCM_DIS);
...
then since you refer to everything by name you don't have to worry about
the length and order of array elements if anything ever changes.
> + return 0;
> +}
> +
> +static int mtk_iommu_resume(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
> + struct mtk_iommu_data *data = mtkdom->data;
> + void __iomem *base = data->base;
> + unsigned int i = 0;
> +
> + writel(data->reg[i++], base + REG_MMU_PT_BASE_ADDR);
> + writel(data->reg[i++], base + REG_MMU_STANDARD_AXI_MODE);
> + writel(data->reg[i++], base + REG_MMU_DCM_DIS);
> + writel(data->reg[i++], base + REG_MMU_CTRL_REG);
> + writel(data->reg[i++], base + REG_MMU_IVRP_PADDR);
> + writel(data->reg[i++], base + REG_MMU_INT_CONTROL0);
> + writel(data->reg[i++], base + REG_MMU_INT_MAIN_CONTROL);
> + return 0;
> +}
Other than that though, modulo the issues with the pagetable code I
think this part of the driver is shaping up quite nicely.
Robin.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-27 4:21 ` Yong Wu
@ 2015-07-27 14:05 ` Robin Murphy
2015-07-27 14:11 ` Will Deacon
[not found] ` <1438329337.25925.147.camel@mhfsdcap03>
1 sibling, 1 reply; 34+ messages in thread
From: Robin Murphy @ 2015-07-27 14:05 UTC (permalink / raw)
To: Yong Wu, Will Deacon
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Daniel Kurtz, Tomasz Figa, Lucas Stach, Rob Herring,
Catalin Marinas, linux-mediatek, Sasha Hauer, srv_heupstream,
devicetree, linux-kernel, linux-arm-kernel, iommu, pebolle, arnd,
mitchelh, cloud.chou, frederic.chen
On 27/07/15 05:21, Yong Wu wrote:
[...]
>>>>> +static arm_short_iopte
>>>>> +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
>>>>> +{
>>>>> + arm_short_iopte pteprot;
>>>>> +
>>>>> + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
>>>>> + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
>>>>> + ARM_SHORT_PTE_TYPE_SMALL;
>>>>> + if (prot & IOMMU_CACHE)
>>>>> + pteprot |= ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
>>>>> + if (prot & IOMMU_WRITE)
>>>>> + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
>>>>> + ARM_SHORT_PTE_SMALL_TEX0;
>>>>
>>>> This doesn't make any sense. TEX[2:0] is all about memory attributes, not
>>>> permissions, so you're making the mapping write-back, write-allocate but
>>>> that's not what the IOMMU_* values are about.
>>>
>>> I will delete it.
>>
>> Well, can you not control mapping permissions with the AP bits? The idea
>> of the IOMMU flags are:
>>
>> IOMMU_CACHE : Install a normal, cacheable mapping (you've got this right)
>> IOMMU_READ : Allow read access for the device
>> IOMMU_WRITE : Allow write access for the device
>> IOMMU_NOEXEC : Disallow execute access for the device
>>
>> so the caller to iommu_map passes in a bitmap of these, which you need to
>> encode in the page-table entry.
>
> From the spec, AP[2] differentiate the read/write and readonly.
> How about this?:
> //===============
> #define ARM_SHORT_PGD_FULL_ACCESS (3 << 10)
> #define ARM_SHORT_PGD_RDONLY BIT(15)
>
> pgdprot |= ARM_SHORT_PGD_FULL_ACCESS;/* or other names? */
> if(!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
> pgdprot |= ARM_SHORT_PGD_RDONLY;
> //===============
> pte is the same.
>
> Sorry, Our HW don't meet the standard spec fully. it don't implement the
> AP bits.
>
>>
>>>>> +static int
>>>>> +_arm_short_map(struct arm_short_io_pgtable *data,
>>>>> + unsigned int iova, phys_addr_t paddr,
>>>>> + arm_short_iopte pgdprot, arm_short_iopte pteprot,
>>>>> + bool large)
>>>>> +{
>>>>> + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
>>>>> + arm_short_iopte *pgd = data->pgd, *pte;
>>>>> + void *cookie = data->iop.cookie, *pte_va;
>>>>> + unsigned int ptenr = large ? 16 : 1;
>>>>> + int i, quirk = data->iop.cfg.quirks;
>>>>> + bool ptenew = false;
>>>>> +
>>>>> + pgd += ARM_SHORT_PGD_IDX(iova);
>>>>> +
>>>>> + if (!pteprot) { /* section or supersection */
>>>>> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
>>>>> + pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
>>>>> + pte = pgd;
>>>>> + pteprot = pgdprot;
>>>>> + } else { /* page or largepage */
>>>>> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
>>>>> + if (large) { /* special Bit */
>>>>
>>>> This definitely needs a better comment! What exactly are you doing here
>>>> and what is that quirk all about?
>>>
>>> I use this quirk is for MTK Special Bit as we don't have the XN bit in
>>> pagetable.
>>
>> I'm still not really clear about what this is.
>
> There is some difference between the standard spec and MTK HW,
> Our hw don't implement some bits, like XN and AP.
> So I add a quirk for MTK special.
When you say it doesn't implement these bits, do you mean that having
them set will lead to Bad Things happening in the hardware, or that it
will simply ignore them and not enforce any of the protections they
imply? The former case would definitely want clearly documenting
somewhere, whereas for the latter case I'm not sure it's even worth the
complication of having a quirk - if the value doesn't matter there seems
little point in doing a special dance just for the sake of semantic
correctness of the in-memory PTEs, in my opinion.
Robin.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-27 14:05 ` Robin Murphy
@ 2015-07-27 14:11 ` Will Deacon
2015-07-28 5:08 ` Yong Wu
0 siblings, 1 reply; 34+ messages in thread
From: Will Deacon @ 2015-07-27 14:11 UTC (permalink / raw)
To: Robin Murphy
Cc: Yong Wu, Joerg Roedel, Thierry Reding, Mark Rutland,
Matthias Brugger, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> On 27/07/15 05:21, Yong Wu wrote:
> >>>>> + } else { /* page or largepage */
> >>>>> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> >>>>> + if (large) { /* special Bit */
> >>>>
> >>>> This definitely needs a better comment! What exactly are you doing here
> >>>> and what is that quirk all about?
> >>>
> >>> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> >>> pagetable.
> >>
> >> I'm still not really clear about what this is.
> >
> > There is some difference between the standard spec and MTK HW,
> > Our hw don't implement some bits, like XN and AP.
> > So I add a quirk for MTK special.
>
> When you say it doesn't implement these bits, do you mean that having
> them set will lead to Bad Things happening in the hardware, or that it
> will simply ignore them and not enforce any of the protections they
> imply? The former case would definitely want clearly documenting
> somewhere, whereas for the latter case I'm not sure it's even worth the
> complication of having a quirk - if the value doesn't matter there seems
> little point in doing a special dance just for the sake of semantic
> correctness of the in-memory PTEs, in my opinion.
Agreed. We should only use quirks if the current (architecturally
compliant) code causes real issues with the hardware. Then the quirk can
be used to either avoid the problematic routines or to take extra steps
to make things work as the architecture intended.
I've asked how this IOMMU differs from the architecture on a number of
occasions, but I'm still yet to receive a response other than "it's special".
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-27 13:23 ` Robin Murphy
@ 2015-07-27 15:31 ` Russell King - ARM Linux
2015-07-27 15:49 ` Robin Murphy
2015-07-29 6:32 ` Yong Wu
1 sibling, 1 reply; 34+ messages in thread
From: Russell King - ARM Linux @ 2015-07-27 15:31 UTC (permalink / raw)
To: Robin Murphy
Cc: Yong Wu, Joerg Roedel, Thierry Reding, Mark Rutland,
Matthias Brugger, devicetree, pebolle, arnd, srv_heupstream,
Catalin Marinas, Will Deacon, linux-kernel, Tomasz Figa, iommu,
Rob Herring, Daniel Kurtz, Sasha Hauer, cloud.chou,
linux-mediatek, frederic.chen, mitchelh, linux-arm-kernel,
Lucas Stach
On Mon, Jul 27, 2015 at 02:23:26PM +0100, Robin Murphy wrote:
> On 16/07/15 10:04, Yong Wu wrote:
> >This patch adds support for mediatek m4u (MultiMedia Memory Management
> >Unit).
> >
> >Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> [...]
> >+static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> >+{
> >+ struct mtk_iommu_domain *domain = cookie;
> >+ unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> >+
> >+ dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> >+ size, DMA_TO_DEVICE);
>
> Nit: this looks like it may as well be dma_map_single.
>
> It would probably be worth following it with a matching unmap too, just to
> avoid any possible leakage bugs (especially if this M4U ever appears in a
> SoC supporting RAM above the 32-bit boundary).
Why not do the job properly? Take a look at how I implemented the
streaming DMA API on Tegra SMMU (patch set recently sent out earlier
today).
There's no need for hacks like dma_map_page() (and discarding it's
return value) or dma_map_page() followed by dma_unmap_page().
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-27 4:24 ` Yong Wu
@ 2015-07-27 15:48 ` Will Deacon
0 siblings, 0 replies; 34+ messages in thread
From: Will Deacon @ 2015-07-27 15:48 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Mon, Jul 27, 2015 at 05:24:31AM +0100, Yong Wu wrote:
> On Fri, 2015-07-24 at 17:55 +0100, Will Deacon wrote:
> > On Fri, Jul 24, 2015 at 06:43:13AM +0100, Yong Wu wrote:
> > > On Tue, 2015-07-21 at 15:59 +0100, Will Deacon wrote:
> > > > On Thu, Jul 16, 2015 at 10:04:34AM +0100, Yong Wu wrote:
> > > > > +static void mtk_iommu_tlb_flush_all(void *cookie)
> > > > > +{
> > > > > + struct mtk_iommu_domain *domain = cookie;
> > > > > + void __iomem *base;
> > > > > +
> > > > > + base = domain->data->base;
> > > > > + writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> > > > > + writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
> > > >
> > > > This needs to be synchronous, so you probably want to call
> > > > mtk_iommu_tlb_sync at the end.
> > >
> > > From our spec, we have to wait until HW done after tlb flush range.
> > > But it don't need wait after tlb flush all.
> > > so It isn't necessary to add mtk_iommu_tlb_sync in tlb_flush_all here.
> >
> > Okey doke, but I'm surprised you don't need a subsequent DSB or read-back.
> > What if the writel is buffered on the way to the IOMMU?
>
> Then I change to this:
> //==========
> writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> writel_relaxed(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
> dsb(ishst);
> //===========
> dsb or mb(). which one is better here?
I think you should use mb();
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-27 15:31 ` Russell King - ARM Linux
@ 2015-07-27 15:49 ` Robin Murphy
2015-07-29 5:41 ` Yong Wu
0 siblings, 1 reply; 34+ messages in thread
From: Robin Murphy @ 2015-07-27 15:49 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Yong Wu, Joerg Roedel, Thierry Reding, Mark Rutland,
Matthias Brugger, devicetree, pebolle, arnd, srv_heupstream,
Catalin Marinas, Will Deacon, linux-kernel, Tomasz Figa, iommu,
Rob Herring, Daniel Kurtz, Sasha Hauer, cloud.chou,
linux-mediatek, frederic.chen, mitchelh, linux-arm-kernel,
Lucas Stach
On 27/07/15 16:31, Russell King - ARM Linux wrote:
> On Mon, Jul 27, 2015 at 02:23:26PM +0100, Robin Murphy wrote:
>> On 16/07/15 10:04, Yong Wu wrote:
>>> This patch adds support for mediatek m4u (MultiMedia Memory Management
>>> Unit).
>>>
>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>> [...]
>>> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
>>> +{
>>> + struct mtk_iommu_domain *domain = cookie;
>>> + unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
>>> +
>>> + dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
>>> + size, DMA_TO_DEVICE);
>>
>> Nit: this looks like it may as well be dma_map_single.
>>
>> It would probably be worth following it with a matching unmap too, just to
>> avoid any possible leakage bugs (especially if this M4U ever appears in a
>> SoC supporting RAM above the 32-bit boundary).
>
> Why not do the job properly? Take a look at how I implemented the
> streaming DMA API on Tegra SMMU (patch set recently sent out earlier
> today).
>
> There's no need for hacks like dma_map_page() (and discarding it's
> return value) or dma_map_page() followed by dma_unmap_page().
Indeed, as it happens I do have a branch where I prototyped that for the
long-descriptor io-pgtable-arm code a while ago; this discussion has
prompted me to dig it up again. Stay tuned, folks...
Robin.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-27 14:11 ` Will Deacon
@ 2015-07-28 5:08 ` Yong Wu
2015-07-28 11:00 ` Will Deacon
0 siblings, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-07-28 5:08 UTC (permalink / raw)
To: Will Deacon, Robin Murphy
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Daniel Kurtz, Tomasz Figa, Lucas Stach, Rob Herring,
Catalin Marinas, linux-mediatek, Sasha Hauer, srv_heupstream,
devicetree, linux-kernel, linux-arm-kernel, iommu, pebolle, arnd,
mitchelh, cloud.chou, frederic.chen
On Mon, 2015-07-27 at 15:11 +0100, Will Deacon wrote:
> On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> > On 27/07/15 05:21, Yong Wu wrote:
> > >>>>> + } else { /* page or largepage */
> > >>>>> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > >>>>> + if (large) { /* special Bit */
> > >>>>
> > >>>> This definitely needs a better comment! What exactly are you doing here
> > >>>> and what is that quirk all about?
> > >>>
> > >>> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> > >>> pagetable.
> > >>
> > >> I'm still not really clear about what this is.
> > >
> > > There is some difference between the standard spec and MTK HW,
> > > Our hw don't implement some bits, like XN and AP.
> > > So I add a quirk for MTK special.
> >
> > When you say it doesn't implement these bits, do you mean that having
> > them set will lead to Bad Things happening in the hardware, or that it
> > will simply ignore them and not enforce any of the protections they
> > imply? The former case would definitely want clearly documenting
> > somewhere, whereas for the latter case I'm not sure it's even worth the
> > complication of having a quirk - if the value doesn't matter there seems
> > little point in doing a special dance just for the sake of semantic
> > correctness of the in-memory PTEs, in my opinion.
>
> Agreed. We should only use quirks if the current (architecturally
> compliant) code causes real issues with the hardware. Then the quirk can
> be used to either avoid the problematic routines or to take extra steps
> to make things work as the architecture intended.
>
> I've asked how this IOMMU differs from the architecture on a number of
> occasions, but I'm still yet to receive a response other than "it's special".
>
After check further with DE, Our pagetable is refer to ARM-v7's
short-descriptor which is a little different from ARM-v8. like bit0(PXN)
in section and supersection, I didn't read ARM-v7 spec before, so I add
a MTK quirk to disable PXN bit in section and supersection.(if the PXN
bit is wrote in ARM-v7 spec, HW will page fault.)
Then I write this code according to ARM-v8 spec defaultly, and add a
ARM-v7 quirk?
And there is a little different between ARM-v7 spec and MTK pagetable.
It's the XN(bit0) in small page. MTK don't implement XN bit.
The bit[1:0] in MTK's small page should be 2'b10, if it's 2'b11, HW will
page fault.
(MTK don't implement AP bits too, but HW don't use them, it is ok even
though AP bits is wrote)
In the end, I will add two quirk like this, is it OK?
//===========
#define ARM_PGTABLE_QUIRK_SHORT_ARM_V7 BIT(2) /* for ARM-v7 while
default is the ARM-v8 spec */
#define ARM_PGTABLE_QUIRK_SHORT_MTK BIT(3) /* MTK special */
//===========
In the ARM_V7 quirk, I only disable PXN bit in section and supersection,
In the MTK quirk, I only disbable XN in small page.
The other bits seems the same. I'm not sure I write clearly and It seems
we could not copy a image of mtk pagetable here..If any question, please
help tell me.
Thanks very much.
> Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-28 5:08 ` Yong Wu
@ 2015-07-28 11:00 ` Will Deacon
2015-07-28 13:37 ` Yong Wu
0 siblings, 1 reply; 34+ messages in thread
From: Will Deacon @ 2015-07-28 11:00 UTC (permalink / raw)
To: Yong Wu
Cc: Robin Murphy, Joerg Roedel, Thierry Reding, Mark Rutland,
Matthias Brugger, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Tue, Jul 28, 2015 at 06:08:14AM +0100, Yong Wu wrote:
> On Mon, 2015-07-27 at 15:11 +0100, Will Deacon wrote:
> > On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> > > On 27/07/15 05:21, Yong Wu wrote:
> > > >>>>> + } else { /* page or largepage */
> > > >>>>> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > >>>>> + if (large) { /* special Bit */
> > > >>>>
> > > >>>> This definitely needs a better comment! What exactly are you doing here
> > > >>>> and what is that quirk all about?
> > > >>>
> > > >>> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> > > >>> pagetable.
> > > >>
> > > >> I'm still not really clear about what this is.
> > > >
> > > > There is some difference between the standard spec and MTK HW,
> > > > Our hw don't implement some bits, like XN and AP.
> > > > So I add a quirk for MTK special.
> > >
> > > When you say it doesn't implement these bits, do you mean that having
> > > them set will lead to Bad Things happening in the hardware, or that it
> > > will simply ignore them and not enforce any of the protections they
> > > imply? The former case would definitely want clearly documenting
> > > somewhere, whereas for the latter case I'm not sure it's even worth the
> > > complication of having a quirk - if the value doesn't matter there seems
> > > little point in doing a special dance just for the sake of semantic
> > > correctness of the in-memory PTEs, in my opinion.
> >
> > Agreed. We should only use quirks if the current (architecturally
> > compliant) code causes real issues with the hardware. Then the quirk can
> > be used to either avoid the problematic routines or to take extra steps
> > to make things work as the architecture intended.
> >
> > I've asked how this IOMMU differs from the architecture on a number of
> > occasions, but I'm still yet to receive a response other than "it's special".
> >
>
> After check further with DE, Our pagetable is refer to ARM-v7's
> short-descriptor which is a little different from ARM-v8. like bit0(PXN)
> in section and supersection, I didn't read ARM-v7 spec before, so I add
> a MTK quirk to disable PXN bit in section and supersection.(if the PXN
> bit is wrote in ARM-v7 spec, HW will page fault.)
I've been reviewing this using the ARMv7 ARM (Rev.C of DDI0406C) the whole
time. PXN is there as an optional field in non-LPAE implementations. That's
fine and doesn't require any quirks.
> Then I write this code according to ARM-v8 spec defaultly, and add a
> ARM-v7 quirk?
No, I don't think you need this, as the v8 and v7 short-descriptor formats
look compatible to me. You should only need a quirk if architecturally
compliant code cannot work on your hardware.
> And there is a little different between ARM-v7 spec and MTK pagetable.
> It's the XN(bit0) in small page. MTK don't implement XN bit.
> The bit[1:0] in MTK's small page should be 2'b10, if it's 2'b11, HW will
> page fault.
Aha, thanks! *That* is worthy of a quirk. Something like:
IO_PGTABLE_QUIRK_ARM_NO_XN
> (MTK don't implement AP bits too, but HW don't use them, it is ok even
> though AP bits is wrote)
Yeah, I think that's fine. The pgtable code will honour the request but
the h/w will ignore it.
> In the end, I will add two quirk like this, is it OK?
I think you only need the one I mentioned above. I don't see the need
for PXN at all (as I said in the last review).
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-28 11:00 ` Will Deacon
@ 2015-07-28 13:37 ` Yong Wu
2015-07-28 13:47 ` Will Deacon
0 siblings, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-07-28 13:37 UTC (permalink / raw)
To: Will Deacon
Cc: Robin Murphy, Joerg Roedel, Thierry Reding, Mark Rutland,
Matthias Brugger, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Tue, 2015-07-28 at 12:00 +0100, Will Deacon wrote:
> On Tue, Jul 28, 2015 at 06:08:14AM +0100, Yong Wu wrote:
> > On Mon, 2015-07-27 at 15:11 +0100, Will Deacon wrote:
> > > On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> > > > On 27/07/15 05:21, Yong Wu wrote:
> > > > >>>>> + } else { /* page or largepage */
> > > > >>>>> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > > >>>>> + if (large) { /* special Bit */
> > > > >>>>
> > > > >>>> This definitely needs a better comment! What exactly are you doing here
> > > > >>>> and what is that quirk all about?
> > > > >>>
> > > > >>> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> > > > >>> pagetable.
> > > > >>
> > > > >> I'm still not really clear about what this is.
> > > > >
> > > > > There is some difference between the standard spec and MTK HW,
> > > > > Our hw don't implement some bits, like XN and AP.
> > > > > So I add a quirk for MTK special.
> > > >
> > > > When you say it doesn't implement these bits, do you mean that having
> > > > them set will lead to Bad Things happening in the hardware, or that it
> > > > will simply ignore them and not enforce any of the protections they
> > > > imply? The former case would definitely want clearly documenting
> > > > somewhere, whereas for the latter case I'm not sure it's even worth the
> > > > complication of having a quirk - if the value doesn't matter there seems
> > > > little point in doing a special dance just for the sake of semantic
> > > > correctness of the in-memory PTEs, in my opinion.
> > >
> > > Agreed. We should only use quirks if the current (architecturally
> > > compliant) code causes real issues with the hardware. Then the quirk can
> > > be used to either avoid the problematic routines or to take extra steps
> > > to make things work as the architecture intended.
> > >
> > > I've asked how this IOMMU differs from the architecture on a number of
> > > occasions, but I'm still yet to receive a response other than "it's special".
> > >
> >
> > After check further with DE, Our pagetable is refer to ARM-v7's
> > short-descriptor which is a little different from ARM-v8. like bit0(PXN)
> > in section and supersection, I didn't read ARM-v7 spec before, so I add
> > a MTK quirk to disable PXN bit in section and supersection.(if the PXN
> > bit is wrote in ARM-v7 spec, HW will page fault.)
>
> I've been reviewing this using the ARMv7 ARM (Rev.C of DDI0406C) the whole
> time. PXN is there as an optional field in non-LPAE implementations. That's
> fine and doesn't require any quirks.
Thanks for your confirm.
Then I delete all the PXN bit in here?
Take a example,
#define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4))
I will change it to "BIT(4)".
>
> > Then I write this code according to ARM-v8 spec defaultly, and add a
> > ARM-v7 quirk?
>
> No, I don't think you need this, as the v8 and v7 short-descriptor formats
> look compatible to me. You should only need a quirk if architecturally
> compliant code cannot work on your hardware.
>
> > And there is a little different between ARM-v7 spec and MTK pagetable.
> > It's the XN(bit0) in small page. MTK don't implement XN bit.
> > The bit[1:0] in MTK's small page should be 2'b10, if it's 2'b11, HW will
> > page fault.
>
> Aha, thanks! *That* is worthy of a quirk. Something like:
>
> IO_PGTABLE_QUIRK_ARM_NO_XN
Thanks, I will add it.
>
> > (MTK don't implement AP bits too, but HW don't use them, it is ok even
> > though AP bits is wrote)
>
> Yeah, I think that's fine. The pgtable code will honour the request but
> the h/w will ignore it.
>
> > In the end, I will add two quirk like this, is it OK?
>
> I think you only need the one I mentioned above. I don't see the need
> for PXN at all (as I said in the last review).
>
> Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-28 13:37 ` Yong Wu
@ 2015-07-28 13:47 ` Will Deacon
0 siblings, 0 replies; 34+ messages in thread
From: Will Deacon @ 2015-07-28 13:47 UTC (permalink / raw)
To: Yong Wu
Cc: Robin Murphy, Joerg Roedel, Thierry Reding, Mark Rutland,
Matthias Brugger, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Tue, Jul 28, 2015 at 02:37:43PM +0100, Yong Wu wrote:
> On Tue, 2015-07-28 at 12:00 +0100, Will Deacon wrote:
> > On Tue, Jul 28, 2015 at 06:08:14AM +0100, Yong Wu wrote:
> > > On Mon, 2015-07-27 at 15:11 +0100, Will Deacon wrote:
> > > > On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> > > > > On 27/07/15 05:21, Yong Wu wrote:
> > > > > >>>>> + } else { /* page or largepage */
> > > > > >>>>> + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > > > >>>>> + if (large) { /* special Bit */
> > > > > >>>>
> > > > > >>>> This definitely needs a better comment! What exactly are you doing here
> > > > > >>>> and what is that quirk all about?
> > > > > >>>
> > > > > >>> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> > > > > >>> pagetable.
> > > > > >>
> > > > > >> I'm still not really clear about what this is.
> > > > > >
> > > > > > There is some difference between the standard spec and MTK HW,
> > > > > > Our hw don't implement some bits, like XN and AP.
> > > > > > So I add a quirk for MTK special.
> > > > >
> > > > > When you say it doesn't implement these bits, do you mean that having
> > > > > them set will lead to Bad Things happening in the hardware, or that it
> > > > > will simply ignore them and not enforce any of the protections they
> > > > > imply? The former case would definitely want clearly documenting
> > > > > somewhere, whereas for the latter case I'm not sure it's even worth the
> > > > > complication of having a quirk - if the value doesn't matter there seems
> > > > > little point in doing a special dance just for the sake of semantic
> > > > > correctness of the in-memory PTEs, in my opinion.
> > > >
> > > > Agreed. We should only use quirks if the current (architecturally
> > > > compliant) code causes real issues with the hardware. Then the quirk can
> > > > be used to either avoid the problematic routines or to take extra steps
> > > > to make things work as the architecture intended.
> > > >
> > > > I've asked how this IOMMU differs from the architecture on a number of
> > > > occasions, but I'm still yet to receive a response other than "it's special".
> > > >
> > >
> > > After check further with DE, Our pagetable is refer to ARM-v7's
> > > short-descriptor which is a little different from ARM-v8. like bit0(PXN)
> > > in section and supersection, I didn't read ARM-v7 spec before, so I add
> > > a MTK quirk to disable PXN bit in section and supersection.(if the PXN
> > > bit is wrote in ARM-v7 spec, HW will page fault.)
> >
> > I've been reviewing this using the ARMv7 ARM (Rev.C of DDI0406C) the whole
> > time. PXN is there as an optional field in non-LPAE implementations. That's
> > fine and doesn't require any quirks.
>
> Thanks for your confirm.
> Then I delete all the PXN bit in here?
>
> Take a example,
> #define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4))
> I will change it to "BIT(4)".
Yes. Then the PXN bit can be added later as a quirk when we have an
implementation that supports it.
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-27 15:49 ` Robin Murphy
@ 2015-07-29 5:41 ` Yong Wu
2015-07-29 10:31 ` Will Deacon
0 siblings, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-07-29 5:41 UTC (permalink / raw)
To: Robin Murphy, Russell King - ARM Linux
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
devicetree, pebolle, arnd, srv_heupstream, Catalin Marinas,
Will Deacon, linux-kernel, Tomasz Figa, iommu, Rob Herring,
Daniel Kurtz, Sasha Hauer, cloud.chou, linux-mediatek,
frederic.chen, mitchelh, linux-arm-kernel, Lucas Stach,
youhua.li
On Mon, 2015-07-27 at 16:49 +0100, Robin Murphy wrote:
> On 27/07/15 16:31, Russell King - ARM Linux wrote:
> > On Mon, Jul 27, 2015 at 02:23:26PM +0100, Robin Murphy wrote:
> >> On 16/07/15 10:04, Yong Wu wrote:
> >>> This patch adds support for mediatek m4u (MultiMedia Memory Management
> >>> Unit).
> >>>
> >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> >> [...]
> >>> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> >>> +{
> >>> + struct mtk_iommu_domain *domain = cookie;
> >>> + unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> >>> +
> >>> + dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> >>> + size, DMA_TO_DEVICE);
> >>
> >> Nit: this looks like it may as well be dma_map_single.
> >>
> >> It would probably be worth following it with a matching unmap too, just to
> >> avoid any possible leakage bugs (especially if this M4U ever appears in a
> >> SoC supporting RAM above the 32-bit boundary).
> >
> > Why not do the job properly? Take a look at how I implemented the
> > streaming DMA API on Tegra SMMU (patch set recently sent out earlier
> > today).
> >
> > There's no need for hacks like dma_map_page() (and discarding it's
> > return value) or dma_map_page() followed by dma_unmap_page().
>
> Indeed, as it happens I do have a branch where I prototyped that for the
> long-descriptor io-pgtable-arm code a while ago; this discussion has
> prompted me to dig it up again. Stay tuned, folks...
Hi Russell, Robin,
From I see in arm-smmu-v3.c in v4.2-rc1,
The flush_pgtable seems like this:
//==========
dma_addr_t dma_addr;
dma_addr = dma_map_page(dev, ptr, size, DMA_TO_DEVICE);
if (dma_mapping_error(dev, dma_addr))
dev_err(dev, "failed to flush pgtable at %p\n", ptr);
else
dma_unmap_page(dev, dma_addr, size, DMA_TO_DEVICE);
//==========
I will change map like this and use dma_map_single instead.
Is this also seems to be not proper?
Then how to do it? add this before unmap? :
dma_sync_single_for_device(dev, dma_addr, size, DMA_TO_DEVICE);
>
> Robin.
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-27 13:23 ` Robin Murphy
2015-07-27 15:31 ` Russell King - ARM Linux
@ 2015-07-29 6:32 ` Yong Wu
1 sibling, 0 replies; 34+ messages in thread
From: Yong Wu @ 2015-07-29 6:32 UTC (permalink / raw)
To: Robin Murphy
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Will Deacon, Daniel Kurtz, Tomasz Figa, Lucas Stach, Rob Herring,
Catalin Marinas, linux-mediatek, Sasha Hauer, srv_heupstream,
devicetree, linux-kernel, linux-arm-kernel, iommu, pebolle, arnd,
mitchelh, cloud.chou, frederic.chen, youhua.li
On Mon, 2015-07-27 at 14:23 +0100, Robin Murphy wrote:
> On 16/07/15 10:04, Yong Wu wrote:
> > This patch adds support for mediatek m4u (MultiMedia Memory Management
> > Unit).
> >
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> [...]
> > +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> > +{
> > + struct mtk_iommu_domain *domain = cookie;
> > + unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> > +
> > + dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> > + size, DMA_TO_DEVICE);
>
> Nit: this looks like it may as well be dma_map_single.
>
> It would probably be worth following it with a matching unmap too, just
> to avoid any possible leakage bugs (especially if this M4U ever appears
> in a SoC supporting RAM above the 32-bit boundary).
About the map, I will read and try to follow your patch:
iommu/io-pgtable-arm: Allow appropriate DMA API use.
>
> > +}
> > +
> [...]
> > +static int mtk_iommu_parse_dt(struct platform_device *pdev,
> > + struct mtk_iommu_data *data)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *ofnode;
> > + struct resource *res;
> > + int i;
> > +
> > + ofnode = dev->of_node;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + data->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(data->base))
> > + return PTR_ERR(data->base);
> > +
> > + data->irq = platform_get_irq(pdev, 0);
> > + if (data->irq < 0)
> > + return data->irq;
> > +
> > + data->bclk = devm_clk_get(dev, "bclk");
> > + if (IS_ERR(data->bclk))
> > + return PTR_ERR(data->bclk);
> > +
> > + data->larb_nr = of_count_phandle_with_args(
> > + ofnode, "mediatek,larb", NULL);
> > + if (data->larb_nr < 0)
> > + return data->larb_nr;
> > +
> > + for (i = 0; i < data->larb_nr; i++) {
> > + struct device_node *larbnode;
> > + struct platform_device *plarbdev;
> > +
> > + larbnode = of_parse_phandle(ofnode, "mediatek,larb", i);
> > + if (!larbnode)
> > + return -EINVAL;
> > +
> > + plarbdev = of_find_device_by_node(larbnode);
> > + of_node_put(larbnode);
> > + if (!plarbdev)
> > + return -EPROBE_DEFER;
> > + data->larbdev[i] = &plarbdev->dev;
> > + }
>
> At a glance this seems like a job for of_parse_phandle_with_args, but I
> may be missing something subtle.
It seems We can not use of_parse_phandle_with_args here.
The node of larb is.
//=========
larb0: larb@14021000 {
compatible = "mediatek,mt8173-smi-larb";
reg = <0 0x14021000 0 0x1000>;
mediatek,smi = <&smi_common>;
power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
clocks = <&mmsys CLK_MM_SMI_LARB0>,
<&mmsys CLK_MM_SMI_LARB0>;
clock-names = "apb", "smi";
};
//==========
of_parse_phandle_with_args(ofnode,"mediatek,larb", "mediatek,smi",
&larb) will be wrong due to there is no item like "mediatek,smi = <1>;"
in larb.
And this code seems to be not simple if we use
of_parse_phandle_with_args. Both need a loop.
so I don't change it here, ok?
>
> > + return 0;
> > +}
> > +
> [...]
> > +static int mtk_iommu_init_domain_context(struct mtk_iommu_domain *dom)
> > +{
> > + int ret;
> > +
> > + if (dom->iop)
> > + return 0;
> > +
> > + spin_lock_init(&dom->pgtlock);
> > + dom->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS |
> > + IO_PGTABLE_QUIRK_SHORT_SUPERSECTION |
> > + IO_PGTABLE_QUIRK_SHORT_MTK;
> > + dom->cfg.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
> > + dom->cfg.ias = 32;
> > + dom->cfg.oas = 32;
> > + dom->cfg.tlb = &mtk_iommu_gather_ops;
> > +
> > + dom->iop = alloc_io_pgtable_ops(ARM_SHORT_DESC, &dom->cfg, dom);
> > + if (!dom->iop) {
> > + pr_err("Failed to alloc io pgtable\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Update our support page sizes bitmap */
> > + mtk_iommu_ops.pgsize_bitmap = dom->cfg.pgsize_bitmap;
> > +
> > + ret = mtk_iommu_hw_init(dom);
> > + if (ret)
> > + free_io_pgtable_ops(dom->iop);
> > +
> > + return ret;
> > +}
>
> I don't see that you need the above function at all - since your
> pagetable config is fixed and doesn't have any depency on which IOMMU
> you're attaching to, can't you just do all of that straight away in
> domain_alloc?
Yes. We could move it into domain_alloc.
>
> > +static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
> > +{
> > + struct mtk_iommu_domain *priv;
> > +
> > + /* We only support unmanaged domains for now */
> > + if (type != IOMMU_DOMAIN_UNMANAGED)
> > + return NULL;
>
> Have you had a chance to play with the latest DMA mapping series now
> that I've integrated the default domain changes? I think if you handled
> IOMMU_DOMAIN_DMA requests here and made them always return the
> (preallocated) private domain, you should be able to get rid of the
> tricks in attach_device completely.
I can change it to this:
if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
return NULL;
If always return the (preallocated) private domain, I have to use a
global variable to restore the preallocated domain here!.
And It also will print "Incompatible range for DMA domain" and return
fail.
Could we return 0 instead of -EFAULT in iommu_dma_init_domain if the
domain is the same.
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return NULL;
> > +
> > + priv->domain.geometry.aperture_start = 0;
> > + priv->domain.geometry.aperture_end = (1ULL << 32) - 1;
>
> DMA_BIT_MASK(32) ?
Thanks.
>
> > + priv->domain.geometry.force_aperture = true;
> > +
> > + return &priv->domain;
> > +}
> > +
> [...]
> > +static int mtk_iommu_add_device(struct device *dev)
> > +{
> > + struct mtk_iommu_domain *mtkdom;
> > + struct iommu_group *group;
> > + struct mtk_iommu_client_priv *devhead;
> > + int ret;
> > +
> > + if (!dev->archdata.dma_ops)/* Not a iommu client device */
> > + return -ENODEV;
>
> I think archdata.dma_ops is the wrong thing to test here. It would be
> better to use archdata.iommu, since you go on to dereference that
> unconditionally anyway, plus then you only depend on your own of_xlate
> behaviour, rather than some quirk of the arch code (which could quietly
> change in future).
I will change archdata.iommu next time.
Current I used the dev->archdata.iommu of the iommu device(m4u device)
itself. then the m4u device will come here too. so I changed to
dev->archdata.dma_ops.
As before, If we use a global variable for the mtk_iommu_domain, we
could use archdata.iommu here.
>
> > + group = iommu_group_get(dev);
> > + if (!group) {
> > + group = iommu_group_alloc();
> > + if (IS_ERR(group)) {
> > + dev_err(dev, "Failed to allocate IOMMU group\n");
> > + return PTR_ERR(group);
> > + }
> > + }
> > +
> > + ret = iommu_group_add_device(group, dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to add IOMMU group\n");
> > + goto err_group_put;
> > + }
> > +
> > + /* get the mtk_iommu_domain from the iommu device */
> > + devhead = dev->archdata.iommu;
> > + mtkdom = devhead->imudev->archdata.iommu;
> > + ret = iommu_attach_group(&mtkdom->domain, group);
> > + if (ret)
> > + dev_err(dev, "Failed to attach IOMMU group\n");
> > +
> > +err_group_put:
> > + iommu_group_put(group);
> > + return ret;
> > +}
> > +
> [...]
> > +static struct iommu_ops mtk_iommu_ops = {
> > + .domain_alloc = mtk_iommu_domain_alloc,
> > + .domain_free = mtk_iommu_domain_free,
> > + .attach_dev = mtk_iommu_attach_device,
> > + .detach_dev = mtk_iommu_detach_device,
> > + .map = mtk_iommu_map,
> > + .unmap = mtk_iommu_unmap,
> > + .map_sg = default_iommu_map_sg,
> > + .iova_to_phys = mtk_iommu_iova_to_phys,
> > + .add_device = mtk_iommu_add_device,
> > + .of_xlate = mtk_iommu_of_xlate,
> > + .pgsize_bitmap = -1UL,
>
> As above, if you know the hardware only supports a single descriptor
> format with a fixed set of page sizes, why not just represent that directly?
OK. I will write the fix page sizes here.
I wrote it following the arm-smmu.c:)
>
> > +};
> > +
> [...]
> > +static int mtk_iommu_suspend(struct device *dev)
> > +{
> > + struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
> > + struct mtk_iommu_data *data = mtkdom->data;
> > + void __iomem *base = data->base;
> > + unsigned int i = 0;
> > +
> > + data->reg[i++] = readl(base + REG_MMU_PT_BASE_ADDR);
>
> Redundancy nit: any particular reason for saving this here rather than
> simply restoring it from mtkdom->cfg.arm_short_cfg.ttbr[0] on resume?
>
> > + data->reg[i++] = readl(base + REG_MMU_STANDARD_AXI_MODE);
> > + data->reg[i++] = readl(base + REG_MMU_DCM_DIS);
> > + data->reg[i++] = readl(base + REG_MMU_CTRL_REG);
> > + data->reg[i++] = readl(base + REG_MMU_IVRP_PADDR);
> > + data->reg[i++] = readl(base + REG_MMU_INT_CONTROL0);
> > + data->reg[i++] = readl(base + REG_MMU_INT_MAIN_CONTROL);
>
> Nit: given that these are fairly arbitrary discontiguous registers so
> you can't have a simple loop over the array, it might be clearer to
> replace the array with an equivalent struct, e.g.:
>
> struct mtk_iommu_suspend_regs {
> u32 standard_axi_mode;
> u32 dcm_dis;
> ...
> }
>
> ...
> data->reg.dcm_dis = readl(base + REG_MMU_DCM_DIS);
> ...
>
> then since you refer to everything by name you don't have to worry about
> the length and order of array elements if anything ever changes.
Good idea. Thanks very much.
>
> > + return 0;
> > +}
> > +
> > +static int mtk_iommu_resume(struct device *dev)
> > +{
> > + struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
> > + struct mtk_iommu_data *data = mtkdom->data;
> > + void __iomem *base = data->base;
> > + unsigned int i = 0;
> > +
> > + writel(data->reg[i++], base + REG_MMU_PT_BASE_ADDR);
> > + writel(data->reg[i++], base + REG_MMU_STANDARD_AXI_MODE);
> > + writel(data->reg[i++], base + REG_MMU_DCM_DIS);
> > + writel(data->reg[i++], base + REG_MMU_CTRL_REG);
> > + writel(data->reg[i++], base + REG_MMU_IVRP_PADDR);
> > + writel(data->reg[i++], base + REG_MMU_INT_CONTROL0);
> > + writel(data->reg[i++], base + REG_MMU_INT_MAIN_CONTROL);
> > + return 0;
> > +}
>
> Other than that though, modulo the issues with the pagetable code I
> think this part of the driver is shaping up quite nicely.
>
> Robin.
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 6/6] dts: mt8173: Add iommu/smi nodes for mt8173
2015-07-23 14:40 ` Daniel Kurtz
@ 2015-07-29 7:29 ` Yong Wu
0 siblings, 0 replies; 34+ messages in thread
From: Yong Wu @ 2015-07-29 7:29 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Will Deacon, Tomasz Figa, Lucas Stach, Rob Herring,
Catalin Marinas, linux-mediatek, Sasha Hauer, srv_heupstream,
open list:OPEN FIRMWARE AND...,
linux-kernel, linux-arm-kernel, open list:IOMMU DRIVERS,
Paul Bolle, Arnd Bergmann, mitchelh, cloud.chou, frederic.chen
On Thu, 2015-07-23 at 22:40 +0800, Daniel Kurtz wrote:
> Hi Yong,
>
> On Thu, Jul 16, 2015 at 5:04 PM, Yong Wu <yong.wu@mediatek.com> wrote:
> >
> > This patch add the iommu/larbs nodes for mt8173
>
> To what tree does this apply?
> Please rebase these patches (especially this one) on an Matthias'
> current v4.2-next/for-next.
Hi Daniel,
Sorry for reply so late. I will rebase these patch next time.
And the m4u/smi dsti is based on clocks, So I have to add the
patches of the latest MTK clocks[1] locally, then prepare this iommu
dtsi patch.
[1]:http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001800.html
>
>
> >
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 81 ++++++++++++++++++++++++++++++
> > 1 file changed, 81 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > index e81ac1f..7b8e73c 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -14,6 +14,7 @@
> > #include <dt-bindings/clock/mt8173-clk.h>
> > #include <dt-bindings/interrupt-controller/irq.h>
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/memory/mt8173-larb-port.h>
> > #include <dt-bindings/power/mt8173-power.h>
> > #include <dt-bindings/reset-controller/mt8173-resets.h>
> > #include "mt8173-pinfunc.h"
> > @@ -240,6 +241,17 @@
> > reg = <0 0x10200620 0 0x20>;
> > };
> >
> > + iommu: mmsys_iommu@10205000 {
>
> This name should be generic, like this (this comment really applies to
> the binding patch):
>
> iommu: iommu@10205000
Thanks.
>
> > + compatible = "mediatek,mt8173-m4u";
> > + reg = <0 0x10205000 0 0x1000>;
> > + interrupts = <GIC_SPI 139 IRQ_TYPE_LEVEL_LOW>;
> > + clocks = <&infracfg CLK_INFRA_M4U>;
> > + clock-names = "bclk";
> > + mediatek,larb = <&larb0 &larb1 &larb2
> > + &larb3 &larb4 &larb5>;
>
> Tiny nit... please align the "&".
>
> Otherwise, this one is:
>
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Thanks very much. I will improve them next version.
Look forward to that line:)
>
> Thanks!
> -Dan
>
> > + #iommu-cells = <2>;
> > + };
> > +
> > apmixedsys: clock-controller@10209000 {
> > compatible = "mediatek,mt8173-apmixedsys";
> > reg = <0 0x10209000 0 0x1000>;
> > @@ -401,29 +413,98 @@
> > #clock-cells = <1>;
> > };
> >
> > + larb0: larb@14021000 {
> > + compatible = "mediatek,mt8173-smi-larb";
> > + reg = <0 0x14021000 0 0x1000>;
> > + mediatek,smi = <&smi_common>;
> > + power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> > + clocks = <&mmsys CLK_MM_SMI_LARB0>,
> > + <&mmsys CLK_MM_SMI_LARB0>;
> > + clock-names = "apb", "smi";
> > + };
> > +
> > + smi_common: smi@14022000 {
> > + compatible = "mediatek,mt8173-smi";
> > + reg = <0 0x14022000 0 0x1000>;
> > + power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> > + clocks = <&mmsys CLK_MM_SMI_COMMON>,
> > + <&mmsys CLK_MM_SMI_COMMON>;
> > + clock-names = "apb", "smi";
> > + };
> > +
> > + larb4: larb@14027000 {
> > + compatible = "mediatek,mt8173-smi-larb";
> > + reg = <0 0x14027000 0 0x1000>;
> > + mediatek,smi = <&smi_common>;
> > + power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> > + clocks = <&mmsys CLK_MM_SMI_LARB4>,
> > + <&mmsys CLK_MM_SMI_LARB4>;
> > + clock-names = "apb", "smi";
> > + };
> > +
> > imgsys: imgsys@15000000 {
> > compatible = "mediatek,mt8173-imgsys", "syscon";
> > reg = <0 0x15000000 0 0x1000>;
> > #clock-cells = <1>;
> > };
> >
> > + larb2: larb@15001000 {
> > + compatible = "mediatek,mt8173-smi-larb";
> > + reg = <0 0x15001000 0 0x1000>;
> > + mediatek,smi = <&smi_common>;
> > + power-domains = <&scpsys MT8173_POWER_DOMAIN_ISP>;
> > + clocks = <&imgsys CLK_IMG_LARB2_SMI>,
> > + <&imgsys CLK_IMG_LARB2_SMI>;
> > + clock-names = "apb", "smi";
> > + };
> > +
> > vdecsys: vdecsys@16000000 {
> > compatible = "mediatek,mt8173-vdecsys", "syscon";
> > reg = <0 0x16000000 0 0x1000>;
> > #clock-cells = <1>;
> > };
> >
> > + larb1: larb@16010000 {
> > + compatible = "mediatek,mt8173-smi-larb";
> > + reg = <0 0x16010000 0 0x1000>;
> > + mediatek,smi = <&smi_common>;
> > + power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
> > + clocks = <&vdecsys CLK_VDEC_CKEN>,
> > + <&vdecsys CLK_VDEC_LARB_CKEN>;
> > + clock-names = "apb", "smi";
> > + };
> > +
> > vencsys: vencsys@18000000 {
> > compatible = "mediatek,mt8173-vencsys", "syscon";
> > reg = <0 0x18000000 0 0x1000>;
> > #clock-cells = <1>;
> > };
> >
> > + larb3: larb@18001000 {
> > + compatible = "mediatek,mt8173-smi-larb";
> > + reg = <0 0x18001000 0 0x1000>;
> > + mediatek,smi = <&smi_common>;
> > + power-domains = <&scpsys MT8173_POWER_DOMAIN_VENC>;
> > + clocks = <&vencsys CLK_VENC_CKE1>,
> > + <&vencsys CLK_VENC_CKE0>;
> > + clock-names = "apb", "smi";
> > + };
> > +
> > vencltsys: vencltsys@19000000 {
> > compatible = "mediatek,mt8173-vencltsys", "syscon";
> > reg = <0 0x19000000 0 0x1000>;
> > #clock-cells = <1>;
> > };
> > +
> > + larb5: larb@19001000 {
> > + compatible = "mediatek,mt8173-smi-larb";
> > + reg = <0 0x19001000 0 0x1000>;
> > + mediatek,smi = <&smi_common>;
> > + power-domains = <&scpsys MT8173_POWER_DOMAIN_VENC_LT>;
> > + clocks = <&vencltsys CLK_VENCLT_CKE1>,
> > + <&vencltsys CLK_VENCLT_CKE0>;
> > + clock-names = "apb", "smi";
> > + };
> > };
> > };
> >
> > --
> > 1.7.9.5
> >
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver
2015-07-29 5:41 ` Yong Wu
@ 2015-07-29 10:31 ` Will Deacon
0 siblings, 0 replies; 34+ messages in thread
From: Will Deacon @ 2015-07-29 10:31 UTC (permalink / raw)
To: Yong Wu
Cc: Robin Murphy, Russell King - ARM Linux, Joerg Roedel,
Thierry Reding, Mark Rutland, Matthias Brugger, devicetree,
pebolle, arnd, srv_heupstream, Catalin Marinas, linux-kernel,
Tomasz Figa, iommu, Rob Herring, Daniel Kurtz, Sasha Hauer,
cloud.chou, linux-mediatek, frederic.chen, mitchelh,
linux-arm-kernel, Lucas Stach, youhua.li
On Wed, Jul 29, 2015 at 06:41:31AM +0100, Yong Wu wrote:
> On Mon, 2015-07-27 at 16:49 +0100, Robin Murphy wrote:
> > On 27/07/15 16:31, Russell King - ARM Linux wrote:
> > > On Mon, Jul 27, 2015 at 02:23:26PM +0100, Robin Murphy wrote:
> > >> On 16/07/15 10:04, Yong Wu wrote:
> > >>> This patch adds support for mediatek m4u (MultiMedia Memory Management
> > >>> Unit).
> > >>>
> > >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > >> [...]
> > >>> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> > >>> +{
> > >>> + struct mtk_iommu_domain *domain = cookie;
> > >>> + unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> > >>> +
> > >>> + dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> > >>> + size, DMA_TO_DEVICE);
> > >>
> > >> Nit: this looks like it may as well be dma_map_single.
> > >>
> > >> It would probably be worth following it with a matching unmap too, just to
> > >> avoid any possible leakage bugs (especially if this M4U ever appears in a
> > >> SoC supporting RAM above the 32-bit boundary).
> > >
> > > Why not do the job properly? Take a look at how I implemented the
> > > streaming DMA API on Tegra SMMU (patch set recently sent out earlier
> > > today).
> > >
> > > There's no need for hacks like dma_map_page() (and discarding it's
> > > return value) or dma_map_page() followed by dma_unmap_page().
> >
> > Indeed, as it happens I do have a branch where I prototyped that for the
> > long-descriptor io-pgtable-arm code a while ago; this discussion has
> > prompted me to dig it up again. Stay tuned, folks...
>
> Hi Russell, Robin,
>
> From I see in arm-smmu-v3.c in v4.2-rc1,
>
> The flush_pgtable seems like this:
> //==========
> dma_addr_t dma_addr;
>
> dma_addr = dma_map_page(dev, ptr, size, DMA_TO_DEVICE);
>
> if (dma_mapping_error(dev, dma_addr))
> dev_err(dev, "failed to flush pgtable at %p\n", ptr);
> else
> dma_unmap_page(dev, dma_addr, size, DMA_TO_DEVICE);
> //==========
> I will change map like this and use dma_map_single instead.
>
> Is this also seems to be not proper?
>
> Then how to do it? add this before unmap? :
> dma_sync_single_for_device(dev, dma_addr, size, DMA_TO_DEVICE);
Robin's proposed a series fixing this in the io-pgtable code:
http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013821.html
which is currently under review. Please take a look and give comments!
Once merged, the code you cite above will be removed.
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
[not found] ` <1438329337.25925.147.camel@mhfsdcap03>
@ 2015-07-31 11:32 ` Will Deacon
0 siblings, 0 replies; 34+ messages in thread
From: Will Deacon @ 2015-07-31 11:32 UTC (permalink / raw)
To: Yong Wu
Cc: Mark Rutland, Catalin Marinas, cloud.chou, Joerg Roedel,
frederic.chen, devicetree, arnd, Tomasz Figa, Rob Herring,
linux-mediatek, Matthias Brugger, linux-arm-kernel, pebolle,
Thierry Reding, srv_heupstream, Robin Murphy, linux-kernel,
iommu, Daniel Kurtz, Sasha Hauer, mitchelh, Lucas Stach, th.yang,
youhua.li
On Fri, Jul 31, 2015 at 08:55:37AM +0100, Yong Wu wrote:
> About the AP bits, I may have to add a new quirk for it...
>
> Current I add AP in pte like this:
> #define ARM_SHORT_PTE_RD_WR (3 << 4)
> #define ARM_SHORT_PTE_RDONLY BIT(9)
>
> pteprot |= ARM_SHORT_PTE_RD_WR;
>
>
> If(!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
>
>
> pteprot |= ARM_SHORT_PTE_RDONLY;
>
> The problem is that the BIT(9) in the level1 and level2 pagetable of our
> HW has been used for PA[32] that is for the dram size over 4G.
Aha, now *thats* a case of page-table abuse!
> so I had to add a quirk to disable bit9 while RDONLY case.
> (If BIT9 isn't disabled, the HW treat it as the PA[32] case then it will
> translation fault..)
>
> like: IO_PGTABLE_QUIRK_SHORT_MTK ?
Given that you don't have XN either, maybe IO_PGTABLE_QUIRK_NO_PERMS?
When set, IOMMU_READ/WRITE/EXEC are ignored and the mapping will never
generate a permission fault.
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-07-21 17:11 ` Will Deacon
2015-07-24 5:24 ` Yong Wu
@ 2015-09-14 12:25 ` Yong Wu
2015-09-16 12:55 ` Will Deacon
1 sibling, 1 reply; 34+ messages in thread
From: Yong Wu @ 2015-09-14 12:25 UTC (permalink / raw)
To: Will Deacon
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
[...]
> > +static int arm_short_map(struct io_pgtable_ops *ops, unsigned long iova,
> > + phys_addr_t paddr, size_t size, int prot)
> > +{
> > + struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > + int ret;
> > + arm_short_iopte pgdprot = 0, pteprot = 0;
> > + bool large;
> > +
> > + /* If no access, then nothing to do */
> > + if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
> > + return 0;
> > +
> > + switch (size) {
> > + case SZ_4K:
> > + case SZ_64K:
> > + large = (size == SZ_64K) ? true : false;
> > + pteprot = __arm_short_pte_prot(data, prot, large);
> > + pgdprot = __arm_short_pgtable_prot(data, prot & IOMMU_NOEXEC);
> > + break;
> > +
> > + case SZ_1M:
> > + case SZ_16M:
> > + large = (size == SZ_16M) ? true : false;
> > + pgdprot = __arm_short_pgd_prot(data, prot, large);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (WARN_ON((iova | paddr) & (size - 1)))
> > + return -EINVAL;
> > +
> > + ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
> > +
> > + tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
> > + tlb->tlb_sync(data->iop.cookie);
>
> In _arm_short_map, it looks like you can only go from invalid -> valid,
> so why do you need to flush the TLB here?
Hi Will,
Here is about flush-tlb after map iova, I have deleted it in v4
following this suggestion. But We meet a problem about it.
Take a example with JPEG. the test steps is:
a).JPEG HW decode a picture with the source iova,like 0xfd780000.
b).JPEG HW decode done, It will unmap the iova(write 0 in pagetable and
flush tlb).
c).JPEG HW decode the second picture, whose source iova is also
0xfd780000.
Then our HW maybe fail due to it will auto prefetch, It may prefecth
between the step b) and c). then the HW may fetch the pagetable content
which has been unmapped in step b). then the HW will get the iova's
physical address is 0, It will translation fault!
So I think our HW need flush-tlb after map iova. Could we add a
QUIRK like "IO_PGTABLE_QUIRK_AUTO_PREFETCH_ENABLE" for it?
If it's not allowed, we will have to add this in our internal function
mtk_iommu_map of mtk_iommu.c.
Thanks.
>
> > + return ret;
> > +}
> > +
[...]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-09-14 12:25 ` Yong Wu
@ 2015-09-16 12:55 ` Will Deacon
2015-09-17 2:38 ` Yong Wu
0 siblings, 1 reply; 34+ messages in thread
From: Will Deacon @ 2015-09-16 12:55 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
Hello Yong,
On Mon, Sep 14, 2015 at 01:25:00PM +0100, Yong Wu wrote:
> On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > > + ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
> > > +
> > > + tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
> > > + tlb->tlb_sync(data->iop.cookie);
> >
> > In _arm_short_map, it looks like you can only go from invalid -> valid,
> > so why do you need to flush the TLB here?
>
> Hi Will,
> Here is about flush-tlb after map iova, I have deleted it in v4
> following this suggestion. But We meet a problem about it.
Ok.
> Take a example with JPEG. the test steps is:
> a).JPEG HW decode a picture with the source iova,like 0xfd780000.
> b).JPEG HW decode done, It will unmap the iova(write 0 in pagetable and
> flush tlb).
> c).JPEG HW decode the second picture, whose source iova is also
> 0xfd780000.
> Then our HW maybe fail due to it will auto prefetch, It may prefecth
> between the step b) and c). then the HW may fetch the pagetable content
> which has been unmapped in step b). then the HW will get the iova's
> physical address is 0, It will translation fault!
Oh no! So-called "negative caching" is certainly prohibited by the ARM
architecture, but if you've built it then we can probably work around it
as an additional quirk. I assume the prefetcher stops prefetching when
it sees an invalid descriptor?
> So I think our HW need flush-tlb after map iova. Could we add a
> QUIRK like "IO_PGTABLE_QUIRK_AUTO_PREFETCH_ENABLE" for it?
> If it's not allowed, we will have to add this in our internal function
> mtk_iommu_map of mtk_iommu.c.
Actually, this type of quirk is ringing bells with me (I think another
IOMMU needed something similar in the past), so maybe just add
IO_PGTABLE_QUIRK_TLBI_ON_MAP?
Will
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
2015-09-16 12:55 ` Will Deacon
@ 2015-09-17 2:38 ` Yong Wu
0 siblings, 0 replies; 34+ messages in thread
From: Yong Wu @ 2015-09-17 2:38 UTC (permalink / raw)
To: Will Deacon
Cc: Joerg Roedel, Thierry Reding, Mark Rutland, Matthias Brugger,
Robin Murphy, Daniel Kurtz, Tomasz Figa, Lucas Stach,
Rob Herring, Catalin Marinas, linux-mediatek, Sasha Hauer,
srv_heupstream, devicetree, linux-kernel, linux-arm-kernel,
iommu, pebolle, arnd, mitchelh, cloud.chou, frederic.chen
On Wed, 2015-09-16 at 13:55 +0100, Will Deacon wrote:
> Hello Yong,
>
> On Mon, Sep 14, 2015 at 01:25:00PM +0100, Yong Wu wrote:
> > On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > > > + ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
> > > > +
> > > > + tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
> > > > + tlb->tlb_sync(data->iop.cookie);
> > >
> > > In _arm_short_map, it looks like you can only go from invalid -> valid,
> > > so why do you need to flush the TLB here?
> >
> > Hi Will,
> > Here is about flush-tlb after map iova, I have deleted it in v4
> > following this suggestion. But We meet a problem about it.
>
> Ok.
>
> > Take a example with JPEG. the test steps is:
> > a).JPEG HW decode a picture with the source iova,like 0xfd780000.
> > b).JPEG HW decode done, It will unmap the iova(write 0 in pagetable and
> > flush tlb).
> > c).JPEG HW decode the second picture, whose source iova is also
> > 0xfd780000.
> > Then our HW maybe fail due to it will auto prefetch, It may prefecth
> > between the step b) and c). then the HW may fetch the pagetable content
> > which has been unmapped in step b). then the HW will get the iova's
> > physical address is 0, It will translation fault!
>
> Oh no! So-called "negative caching" is certainly prohibited by the ARM
> architecture, but if you've built it then we can probably work around it
> as an additional quirk. I assume the prefetcher stops prefetching when
> it sees an invalid descriptor?
Yes, If it's a invalid descriptor, the HW will stop prefetch.
>
> > So I think our HW need flush-tlb after map iova. Could we add a
> > QUIRK like "IO_PGTABLE_QUIRK_AUTO_PREFETCH_ENABLE" for it?
> > If it's not allowed, we will have to add this in our internal function
> > mtk_iommu_map of mtk_iommu.c.
>
> Actually, this type of quirk is ringing bells with me (I think another
> IOMMU needed something similar in the past), so maybe just add
> IO_PGTABLE_QUIRK_TLBI_ON_MAP?
Thanks. I will add it like:
//=====================
ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
if (data->iop.cfg.quirk & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
tlb->tlb_sync(data->iop.cookie);
}
//======================
It will flush-tlb every time after map-iova. then the HW will fetch the
new PA from the dram.
>
> Will
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2015-09-17 2:38 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-16 9:04 [PATCH v3 0/6] MT8173 IOMMU SUPPORT Yong Wu
2015-07-16 9:04 ` [PATCH v3 1/6] dt-bindings: iommu: Add binding for mediatek IOMMU Yong Wu
2015-07-16 9:04 ` [PATCH v3 2/6] dt-bindings: mediatek: Add smi dts binding Yong Wu
2015-07-16 9:04 ` [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator Yong Wu
2015-07-21 17:11 ` Will Deacon
2015-07-24 5:24 ` Yong Wu
2015-07-24 16:53 ` Will Deacon
2015-07-27 4:21 ` Yong Wu
2015-07-27 14:05 ` Robin Murphy
2015-07-27 14:11 ` Will Deacon
2015-07-28 5:08 ` Yong Wu
2015-07-28 11:00 ` Will Deacon
2015-07-28 13:37 ` Yong Wu
2015-07-28 13:47 ` Will Deacon
[not found] ` <1438329337.25925.147.camel@mhfsdcap03>
2015-07-31 11:32 ` Will Deacon
2015-09-14 12:25 ` Yong Wu
2015-09-16 12:55 ` Will Deacon
2015-09-17 2:38 ` Yong Wu
2015-07-16 9:04 ` [PATCH v3 4/6] memory: mediatek: Add SMI driver Yong Wu
2015-07-16 9:04 ` [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu
2015-07-21 14:59 ` Will Deacon
2015-07-24 5:43 ` Yong Wu
2015-07-24 16:55 ` Will Deacon
2015-07-27 4:24 ` Yong Wu
2015-07-27 15:48 ` Will Deacon
2015-07-27 13:23 ` Robin Murphy
2015-07-27 15:31 ` Russell King - ARM Linux
2015-07-27 15:49 ` Robin Murphy
2015-07-29 5:41 ` Yong Wu
2015-07-29 10:31 ` Will Deacon
2015-07-29 6:32 ` Yong Wu
2015-07-16 9:04 ` [PATCH v3 6/6] dts: mt8173: Add iommu/smi nodes for mt8173 Yong Wu
2015-07-23 14:40 ` Daniel Kurtz
2015-07-29 7:29 ` Yong Wu
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).