| Message ID | 20220120201958.2649-1-semen.protsenko@linaro.org |
|---|---|
| Headers | show |
| Series | iommu/samsung: Introduce Exynos sysmmu-v8 driver | expand |
On 20/01/2022 21:19, Sam Protsenko wrote: > Only example of usage and header for now. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > .../bindings/iommu/samsung,sysmmu-v8.txt | 31 +++++++++++++ Please, don't copy paste bindings or entire drviers from vendor kernel. It looks very bad. Instead, submit them in dtschema. NAK. > include/dt-bindings/soc/samsung,sysmmu-v8.h | 43 +++++++++++++++++++ > 2 files changed, 74 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iommu/samsung,sysmmu-v8.txt > create mode 100644 include/dt-bindings/soc/samsung,sysmmu-v8.h > > diff --git a/Documentation/devicetree/bindings/iommu/samsung,sysmmu-v8.txt b/Documentation/devicetree/bindings/iommu/samsung,sysmmu-v8.txt > new file mode 100644 > index 000000000000..d6004ea4a746 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iommu/samsung,sysmmu-v8.txt > @@ -0,0 +1,31 @@ > +Example (Exynos850, IOMMU for DPU usage): > + > + #include <dt-bindings/soc/samsung,sysmmu-v8.h> > + > + /* IOMMU group info */ > + iommu_group_dpu: iommu_group_dpu { > + compatible = "samsung,sysmmu-group"; > + }; > + > + sysmmu_dpu: sysmmu@130c0000 { > + compatible = "samsung,sysmmu-v8"; > + reg = <0x130c0000 0x9000>; > + interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>; > + qos = <15>; > + > + clocks = <&cmu_dpu CLK_GOUT_DPU_SMMU_CLK>; > + clock-names = "gate"; > + > + sysmmu,secure-irq; > + sysmmu,secure_base = <0x130d0000>; > + sysmmu,default_tlb = <TLB_CFG(BL1, PREFETCH_PREDICTION)>; > + sysmmu,tlb_property = > + <1 TLB_CFG(BL1, PREFETCH_PREDICTION) (DIR_READ | (1 << 16)) SYSMMU_ID_MASK(0x2, 0xF)>, > + <2 TLB_CFG(BL1, PREFETCH_PREDICTION) (DIR_READ | (1 << 16)) SYSMMU_ID_MASK(0x4, 0xF)>, > + <3 TLB_CFG(BL1, PREFETCH_PREDICTION) (DIR_READ | (1 << 16)) SYSMMU_ID_MASK(0x6, 0xF)>, > + <4 TLB_CFG(BL1, PREFETCH_PREDICTION) (DIR_READ | (1 << 16)) SYSMMU_ID_MASK(0x8, 0xF)>; > + port-name = "DPU"; > + #iommu-cells = <0>; > + //power-domains = <&pd_dpu>; We try not to store dead code in kernel. Best regards, Krzysztof
On 20/01/2022 21:19, Sam Protsenko wrote: > This is a draft of a new IOMMU driver used in modern Exynos SoCs (like > Exynos850) and Google's GS101 SoC (used in Pixel 6 phone). Most of its > code were taken from GS101 downstream kernel [1], with some extra > patches on top (fixes from Exynos850 downstream kernel and some porting > changes to adapt it to the mainline kernel). All development history can > be found at [2]. > > Similarities with existing exynos-iommu.c is minimal. I did some > analysis using similarity-tester tool: > > 8<-------------------------------------------------------------------->8 > $ sim_c -peu -S exynos-iommu.c "|" samsung-* > > exynos-iommu.c consists for 15 % of samsung-iommu.c material > exynos-iommu.c consists for 1 % of samsung-iommu-fault.c material > exynos-iommu.c consists for 3 % of samsung-iommu.h material > 8<-------------------------------------------------------------------->8 > > So the similarity is very low, most of that code is some boilerplate > that shouldn't be extracted to common code (like allocating the memory > and requesting clocks/interrupts in probe function). This is not a prove of lack of similarities. The vendor drivers have proven track of poor quality and a lot of code not compatible with Linux kernel style. Therefore comparing mainline driver, reviewed and well tested, with a vendor out-of-tree driver is wrong. You will almost always have 0% of similarities, because vendor kernel drivers are mostly developed from scratch instead of re-using existing drivers. Recently Samsung admitted it - if I extend existing driver, I will have to test old and new platform, so it is easier for me to write a new driver. No, this is not that approach we use it in mainline. Linaro should know it much better. > > It was tested on v5.4 Android kernel on Exynos850 (E850-96 board) with > DPU use-case (displaying some graphics to the screen). Also it > apparently works fine on v5.10 GS101 kernel (on Pixel 6). On mainline > kernel I managed to build, match and bind the driver. No real world test > was done, but the changes from v5.10 (where it works fine) are minimal > (see [2] for details). So I'm pretty sure the driver is functional. No, we do not take untested code or code for different out-of-tree kernels, not for mainline. I am pretty sure drivers is poor or not working. > > For this patch series I'd like to receive some high-level review for > driver's design and architecture. Coding style and API issues I can fix > later, when sending real (not RFC) series. Particularly I'd like to hear > some opinions about: > - namings: Kconfig option, file names, module name, compatible, etc > - modularity: should this driver be a different platform driver (like > in this series), or should it be integrated into existing > exynos-iommu.c driver somehow > - dt-bindings: does it look ok as it is, or some interface changes are > needed You sent bindings in TXT with dead code inside, and you ask if it is ok. I consider this approach that you sent whatever junk to us hoping that we will point all the issues instead of finding them by yourself. I am pretty sure you have several folks in Linaro who can perform first review and bring the code closer to mainline style. Best regards, Krzysztof
On 20/01/2022 21:19, Sam Protsenko wrote: > Introduce new driver for modern Exynos ARMv8 SoCs, e.g. Exynos850. Also > it's used for Google's GS101 SoC. > > This is squashed commit, contains next patches of different authors. See > `iommu-exynos850-dev' branch for details: [1]. > > Original authors (Samsung): > > - Cho KyongHo <pullip.cho@samsung.com> > - Hyesoo Yu <hyesoo.yu@samsung.com> > - Janghyuck Kim <janghyuck.kim@samsung.com> > - Jinkyu Yang <jinkyu1.yang@samsung.com> > > Some improvements were made by Google engineers: > > - Alex <acnwigwe@google.com> > - Carlos Llamas <cmllamas@google.com> > - Daniel Mentz <danielmentz@google.com> > - Erick Reyes <erickreyes@google.com> > - J. Avila <elavila@google.com> > - Jonglin Lee <jonglin@google.com> > - Mark Salyzyn <salyzyn@google.com> > - Thierry Strudel <tstrudel@google.com> > - Will McVicker <willmcvicker@google.com> > > [1] https://github.com/joe-skb7/linux/tree/iommu-exynos850-dev > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/iommu/Kconfig | 13 + > drivers/iommu/Makefile | 3 + > drivers/iommu/samsung-iommu-fault.c | 617 +++++++++++ > drivers/iommu/samsung-iommu-group.c | 50 + > drivers/iommu/samsung-iommu.c | 1521 +++++++++++++++++++++++++++ > drivers/iommu/samsung-iommu.h | 216 ++++ > 6 files changed, 2420 insertions(+) > create mode 100644 drivers/iommu/samsung-iommu-fault.c > create mode 100644 drivers/iommu/samsung-iommu-group.c > create mode 100644 drivers/iommu/samsung-iommu.c > create mode 100644 drivers/iommu/samsung-iommu.h > Existing driver supports several different Exynos SysMMU IP block versions. Several. Please explain why it cannot support one more version? Similarity of vendor driver with mainline is not an argument. > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 3eb68fa1b8cc..78e7039f18aa 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -452,6 +452,19 @@ config QCOM_IOMMU > help > Support for IOMMU on certain Qualcomm SoCs. > > +config SAMSUNG_IOMMU > + tristate "Samsung IOMMU Support" > + select ARM_DMA_USE_IOMMU > + select IOMMU_DMA > + select SAMSUNG_IOMMU_GROUP > + help > + Support for IOMMU on Samsung Exynos SoCs. > + > +config SAMSUNG_IOMMU_GROUP > + tristate "Samsung IOMMU Group Support" > + help > + Support for IOMMU group on Samsung Exynos SoCs. > + > config HYPERV_IOMMU > bool "Hyper-V x2APIC IRQ Handling" > depends on HYPERV && X86 > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile > index bc7f730edbb0..a8bdf449f1d4 100644 > --- a/drivers/iommu/Makefile > +++ b/drivers/iommu/Makefile > @@ -27,6 +27,9 @@ obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o > obj-$(CONFIG_S390_IOMMU) += s390-iommu.o > obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o > obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o > +obj-$(CONFIG_SAMSUNG_IOMMU) += samsung_iommu.o > +samsung_iommu-objs += samsung-iommu.o samsung-iommu-fault.o > +obj-$(CONFIG_SAMSUNG_IOMMU_GROUP) += samsung-iommu-group.o > obj-$(CONFIG_IOMMU_SVA_LIB) += iommu-sva-lib.o io-pgfault.o > obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o > obj-$(CONFIG_APPLE_DART) += apple-dart.o > diff --git a/drivers/iommu/samsung-iommu-fault.c b/drivers/iommu/samsung-iommu-fault.c > new file mode 100644 > index 000000000000..c6b4259976c4 > --- /dev/null > +++ b/drivers/iommu/samsung-iommu-fault.c > @@ -0,0 +1,617 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2020 Samsung Electronics Co., Ltd. > + */ > + > +#define pr_fmt(fmt) "sysmmu: " fmt > + > +#include <linux/smc.h> > +#include <linux/arm-smccc.h> > +#include <linux/pm_runtime.h> > + > +#include "samsung-iommu.h" > + > +#define MMU_TLB_INFO(n) (0x2000 + ((n) * 0x20)) > +#define MMU_CAPA1_NUM_TLB_SET(reg) (((reg) >> 16) & 0xFF) > +#define MMU_CAPA1_NUM_TLB_WAY(reg) ((reg) & 0xFF) > +#define MMU_CAPA1_SET_TLB_READ_ENTRY(tid, set, way, line) \ > + ((set) | ((way) << 8) | \ > + ((line) << 16) | ((tid) << 20)) > + > +#define MMU_TLB_ENTRY_VALID(reg) ((reg) >> 28) > +#define MMU_SBB_ENTRY_VALID(reg) ((reg) >> 28) > +#define MMU_VADDR_FROM_TLB(reg, idx) ((((reg) & 0xFFFFC) | ((idx) & 0x3)) << 12) > +#define MMU_VID_FROM_TLB(reg) (((reg) >> 20) & 0x7U) > +#define MMU_PADDR_FROM_TLB(reg) ((phys_addr_t)((reg) & 0xFFFFFF) << 12) > +#define MMU_VADDR_FROM_SBB(reg) (((reg) & 0xFFFFF) << 12) > +#define MMU_VID_FROM_SBB(reg) (((reg) >> 20) & 0x7U) > +#define MMU_PADDR_FROM_SBB(reg) ((phys_addr_t)((reg) & 0x3FFFFFF) << 10) > + > +#define REG_MMU_INT_STATUS 0x060 > +#define REG_MMU_INT_CLEAR 0x064 > +#define REG_MMU_FAULT_RW_MASK GENMASK(20, 20) > +#define IS_READ_FAULT(x) (((x) & REG_MMU_FAULT_RW_MASK) == 0) > + > +#define SYSMMU_FAULT_PTW_ACCESS 0 > +#define SYSMMU_FAULT_PAGE_FAULT 1 > +#define SYSMMU_FAULT_ACCESS 2 > +#define SYSMMU_FAULT_RESERVED 3 > +#define SYSMMU_FAULT_UNKNOWN 4 > + > +#define SYSMMU_SEC_FAULT_MASK (BIT(SYSMMU_FAULT_PTW_ACCESS) | \ > + BIT(SYSMMU_FAULT_PAGE_FAULT) | \ > + BIT(SYSMMU_FAULT_ACCESS)) > + > +#define SYSMMU_FAULTS_NUM (SYSMMU_FAULT_UNKNOWN + 1) > + > +#if IS_ENABLED(CONFIG_EXYNOS_CONTENT_PATH_PROTECTION) You just copy-pasted vendor stuff, without actually going through it. It is very disappointing because instead of putting your own effort, you expect community to do your job. What the hell is CONFIG_EXYNOS_CONTENT_PATH_PROTECTION? I'll stop reviewing. Please work on extending existing driver. If you submitted something nice and clean, ready for upstream, instead of vendor junk, you could get away with separate driver. But you did not. It looks really bad. Best regards, Krzysztof
On Fri, 21 Jan 2022 at 10:40, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 20/01/2022 21:19, Sam Protsenko wrote: > > Introduce new driver for modern Exynos ARMv8 SoCs, e.g. Exynos850. Also > > it's used for Google's GS101 SoC. > > > > This is squashed commit, contains next patches of different authors. See > > `iommu-exynos850-dev' branch for details: [1]. > > > > Original authors (Samsung): > > > > - Cho KyongHo <pullip.cho@samsung.com> > > - Hyesoo Yu <hyesoo.yu@samsung.com> > > - Janghyuck Kim <janghyuck.kim@samsung.com> > > - Jinkyu Yang <jinkyu1.yang@samsung.com> > > > > Some improvements were made by Google engineers: > > > > - Alex <acnwigwe@google.com> > > - Carlos Llamas <cmllamas@google.com> > > - Daniel Mentz <danielmentz@google.com> > > - Erick Reyes <erickreyes@google.com> > > - J. Avila <elavila@google.com> > > - Jonglin Lee <jonglin@google.com> > > - Mark Salyzyn <salyzyn@google.com> > > - Thierry Strudel <tstrudel@google.com> > > - Will McVicker <willmcvicker@google.com> > > > > [1] https://github.com/joe-skb7/linux/tree/iommu-exynos850-dev > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > drivers/iommu/Kconfig | 13 + > > drivers/iommu/Makefile | 3 + > > drivers/iommu/samsung-iommu-fault.c | 617 +++++++++++ > > drivers/iommu/samsung-iommu-group.c | 50 + > > drivers/iommu/samsung-iommu.c | 1521 +++++++++++++++++++++++++++ > > drivers/iommu/samsung-iommu.h | 216 ++++ > > 6 files changed, 2420 insertions(+) > > create mode 100644 drivers/iommu/samsung-iommu-fault.c > > create mode 100644 drivers/iommu/samsung-iommu-group.c > > create mode 100644 drivers/iommu/samsung-iommu.c > > create mode 100644 drivers/iommu/samsung-iommu.h > > > > Existing driver supports several different Exynos SysMMU IP block > versions. Several. Please explain why it cannot support one more version? > > Similarity of vendor driver with mainline is not an argument. > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > > index 3eb68fa1b8cc..78e7039f18aa 100644 > > --- a/drivers/iommu/Kconfig > > +++ b/drivers/iommu/Kconfig > > @@ -452,6 +452,19 @@ config QCOM_IOMMU > > help > > Support for IOMMU on certain Qualcomm SoCs. > > > > +config SAMSUNG_IOMMU > > + tristate "Samsung IOMMU Support" > > + select ARM_DMA_USE_IOMMU > > + select IOMMU_DMA > > + select SAMSUNG_IOMMU_GROUP > > + help > > + Support for IOMMU on Samsung Exynos SoCs. > > + > > +config SAMSUNG_IOMMU_GROUP > > + tristate "Samsung IOMMU Group Support" > > + help > > + Support for IOMMU group on Samsung Exynos SoCs. > > + > > config HYPERV_IOMMU > > bool "Hyper-V x2APIC IRQ Handling" > > depends on HYPERV && X86 > > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile > > index bc7f730edbb0..a8bdf449f1d4 100644 > > --- a/drivers/iommu/Makefile > > +++ b/drivers/iommu/Makefile > > @@ -27,6 +27,9 @@ obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o > > obj-$(CONFIG_S390_IOMMU) += s390-iommu.o > > obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o > > obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o > > +obj-$(CONFIG_SAMSUNG_IOMMU) += samsung_iommu.o > > +samsung_iommu-objs += samsung-iommu.o samsung-iommu-fault.o > > +obj-$(CONFIG_SAMSUNG_IOMMU_GROUP) += samsung-iommu-group.o > > obj-$(CONFIG_IOMMU_SVA_LIB) += iommu-sva-lib.o io-pgfault.o > > obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o > > obj-$(CONFIG_APPLE_DART) += apple-dart.o > > diff --git a/drivers/iommu/samsung-iommu-fault.c b/drivers/iommu/samsung-iommu-fault.c > > new file mode 100644 > > index 000000000000..c6b4259976c4 > > --- /dev/null > > +++ b/drivers/iommu/samsung-iommu-fault.c > > @@ -0,0 +1,617 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2020 Samsung Electronics Co., Ltd. > > + */ > > + > > +#define pr_fmt(fmt) "sysmmu: " fmt > > + > > +#include <linux/smc.h> > > +#include <linux/arm-smccc.h> > > +#include <linux/pm_runtime.h> > > + > > +#include "samsung-iommu.h" > > + > > +#define MMU_TLB_INFO(n) (0x2000 + ((n) * 0x20)) > > +#define MMU_CAPA1_NUM_TLB_SET(reg) (((reg) >> 16) & 0xFF) > > +#define MMU_CAPA1_NUM_TLB_WAY(reg) ((reg) & 0xFF) > > +#define MMU_CAPA1_SET_TLB_READ_ENTRY(tid, set, way, line) \ > > + ((set) | ((way) << 8) | \ > > + ((line) << 16) | ((tid) << 20)) > > + > > +#define MMU_TLB_ENTRY_VALID(reg) ((reg) >> 28) > > +#define MMU_SBB_ENTRY_VALID(reg) ((reg) >> 28) > > +#define MMU_VADDR_FROM_TLB(reg, idx) ((((reg) & 0xFFFFC) | ((idx) & 0x3)) << 12) > > +#define MMU_VID_FROM_TLB(reg) (((reg) >> 20) & 0x7U) > > +#define MMU_PADDR_FROM_TLB(reg) ((phys_addr_t)((reg) & 0xFFFFFF) << 12) > > +#define MMU_VADDR_FROM_SBB(reg) (((reg) & 0xFFFFF) << 12) > > +#define MMU_VID_FROM_SBB(reg) (((reg) >> 20) & 0x7U) > > +#define MMU_PADDR_FROM_SBB(reg) ((phys_addr_t)((reg) & 0x3FFFFFF) << 10) > > + > > +#define REG_MMU_INT_STATUS 0x060 > > +#define REG_MMU_INT_CLEAR 0x064 > > +#define REG_MMU_FAULT_RW_MASK GENMASK(20, 20) > > +#define IS_READ_FAULT(x) (((x) & REG_MMU_FAULT_RW_MASK) == 0) > > + > > +#define SYSMMU_FAULT_PTW_ACCESS 0 > > +#define SYSMMU_FAULT_PAGE_FAULT 1 > > +#define SYSMMU_FAULT_ACCESS 2 > > +#define SYSMMU_FAULT_RESERVED 3 > > +#define SYSMMU_FAULT_UNKNOWN 4 > > + > > +#define SYSMMU_SEC_FAULT_MASK (BIT(SYSMMU_FAULT_PTW_ACCESS) | \ > > + BIT(SYSMMU_FAULT_PAGE_FAULT) | \ > > + BIT(SYSMMU_FAULT_ACCESS)) > > + > > +#define SYSMMU_FAULTS_NUM (SYSMMU_FAULT_UNKNOWN + 1) > > + > > +#if IS_ENABLED(CONFIG_EXYNOS_CONTENT_PATH_PROTECTION) > > You just copy-pasted vendor stuff, without actually going through it. > > It is very disappointing because instead of putting your own effort, you > expect community to do your job. > > What the hell is CONFIG_EXYNOS_CONTENT_PATH_PROTECTION? > > I'll stop reviewing. Please work on extending existing driver. If you > submitted something nice and clean, ready for upstream, instead of > vendor junk, you could get away with separate driver. But you did not. > It looks really bad. > Krzysztof, that's not what I asked in my patch 0/3. I probably wasn't really clear, sorry. Let me please try and describe that better, and maybe provide some context. I'm just starting to work on that driver, it's basically downstream version of it. Of course I'm going to rework it before sending the actual patch series (that's why this series has RFC tag). I'd never asked community to do my job for me and really review the downstream driver! I just want to know from the starters some *very* basic and high-level info, which could help me to rework the driver in correct way. Like naming of files, compatible strings, should it be part of existing driver or it's ok to have it as another platform_driver. In other words, that kind of "review" shouldn't take more than 2 minutes of your time. And it could spare us all unneeded extra review rounds in future. Right now I don't need the code review per se (and I'm really sorry you had to spend your time on that, knowing how busy maintainers can be during the MW). I thought about omitting the code at all, only asking the questions, but then I figured it's a good idea to attach some code for the reference. Maybe it wasn't a good idea after all. For the record, I'm well aware that we don't send downstream code without making it upstreamable first, and I know it must be tested well, etc. For example, you already saw me sending clk-exynos850 driver, which I re-implemented from scratch, and it has ~0.0% of downstream code. So why would I change my policy about that all of the sudden... Anyway, hope you understand now that there weren't any ill intentions on my side, w.r.t. this RFC. > Best regards, > Krzysztof
Hi Sam, On 21.01.2022 12:08, Sam Protsenko wrote: > On Fri, 21 Jan 2022 at 10:40, Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: >> On 20/01/2022 21:19, Sam Protsenko wrote: >>> Introduce new driver for modern Exynos ARMv8 SoCs, e.g. Exynos850. Also >>> it's used for Google's GS101 SoC. >>> >>> This is squashed commit, contains next patches of different authors. See >>> `iommu-exynos850-dev' branch for details: [1]. >>> >>> Original authors (Samsung): >>> >>> - Cho KyongHo <pullip.cho@samsung.com> >>> - Hyesoo Yu <hyesoo.yu@samsung.com> >>> - Janghyuck Kim <janghyuck.kim@samsung.com> >>> - Jinkyu Yang <jinkyu1.yang@samsung.com> >>> >>> Some improvements were made by Google engineers: >>> >>> - Alex <acnwigwe@google.com> >>> - Carlos Llamas <cmllamas@google.com> >>> - Daniel Mentz <danielmentz@google.com> >>> - Erick Reyes <erickreyes@google.com> >>> - J. Avila <elavila@google.com> >>> - Jonglin Lee <jonglin@google.com> >>> - Mark Salyzyn <salyzyn@google.com> >>> - Thierry Strudel <tstrudel@google.com> >>> - Will McVicker <willmcvicker@google.com> >>> >>> [1] https://protect2.fireeye.com/v1/url?k=19bd3571-46260c3c-19bcbe3e-0cc47aa8f5ba-8a160a7fd38bb35a&q=1&e=eb3f71b3-8df2-46c6-b6d8-0a931ef99024&u=https%3A%2F%2Fgithub.com%2Fjoe-skb7%2Flinux%2Ftree%2Fiommu-exynos850-dev >>> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >>> --- >>> drivers/iommu/Kconfig | 13 + >>> drivers/iommu/Makefile | 3 + >>> drivers/iommu/samsung-iommu-fault.c | 617 +++++++++++ >>> drivers/iommu/samsung-iommu-group.c | 50 + >>> drivers/iommu/samsung-iommu.c | 1521 +++++++++++++++++++++++++++ >>> drivers/iommu/samsung-iommu.h | 216 ++++ >>> 6 files changed, 2420 insertions(+) >>> create mode 100644 drivers/iommu/samsung-iommu-fault.c >>> create mode 100644 drivers/iommu/samsung-iommu-group.c >>> create mode 100644 drivers/iommu/samsung-iommu.c >>> create mode 100644 drivers/iommu/samsung-iommu.h >>> >> Existing driver supports several different Exynos SysMMU IP block >> versions. Several. Please explain why it cannot support one more version? >> >> Similarity of vendor driver with mainline is not an argument. >> >>> ... >> You just copy-pasted vendor stuff, without actually going through it. >> >> It is very disappointing because instead of putting your own effort, you >> expect community to do your job. >> >> What the hell is CONFIG_EXYNOS_CONTENT_PATH_PROTECTION? >> >> I'll stop reviewing. Please work on extending existing driver. If you >> submitted something nice and clean, ready for upstream, instead of >> vendor junk, you could get away with separate driver. But you did not. >> It looks really bad. >> > Krzysztof, that's not what I asked in my patch 0/3. I probably wasn't > really clear, sorry. Let me please try and describe that better, and > maybe provide some context. > > I'm just starting to work on that driver, it's basically downstream > version of it. Of course I'm going to rework it before sending the > actual patch series (that's why this series has RFC tag). I'd never > asked community to do my job for me and really review the downstream > driver! I just want to know from the starters some *very* basic and > high-level info, which could help me to rework the driver in correct > way. Like naming of files, compatible strings, should it be part of > existing driver or it's ok to have it as another platform_driver. In > other words, that kind of "review" shouldn't take more than 2 minutes > of your time. And it could spare us all unneeded extra review rounds > in future. Right now I don't need the code review per se (and I'm > really sorry you had to spend your time on that, knowing how busy > maintainers can be during the MW). I thought about omitting the code > at all, only asking the questions, but then I figured it's a good idea > to attach some code for the reference. Maybe it wasn't a good idea > after all. > > For the record, I'm well aware that we don't send downstream code > without making it upstreamable first, and I know it must be tested > well, etc. For example, you already saw me sending clk-exynos850 > driver, which I re-implemented from scratch, and it has ~0.0% of > downstream code. So why would I change my policy about that all of the > sudden... Anyway, hope you understand now that there weren't any ill > intentions on my side, w.r.t. this RFC. Well, for starting point the existing exynos-iommu driver is really enough. I've played a bit with newer Exyos SoCs some time ago. If I remember right, if you limit the iommu functionality to the essential things like mapping pages to IO-virtual space, the hardware differences between SYSMMU v5 (already supported by the exynos-iommu driver) and v7 are just a matter of changing a one register during the initialization and different bits the page fault reason decoding. You must of course rely on the DMA-mapping framework and its implementation based on mainline DMA-IOMMU helpers. All the code for custom iommu group(s) handling or extended fault management are not needed for the initial version. The IOMMU driver on its own doesn't really make much sense, so you need the other driver/device pair which will make use of it. You have mentioned DPU, so you are trying to bring the display stack. Please check the existing Exynos DRM driver(s). They nicely use DMA-mapping framework and are really modular, so adding hw-specific sub-drivers for Exynos850 shouldn't be that hard. Don't expect that the vendor's drivers based on custom frameworks will work there though. Best regards
Hi Marek, On Fri, 21 Jan 2022 at 14:31, Marek Szyprowski <m.szyprowski@samsung.com> wrote: [snip] > > Well, for starting point the existing exynos-iommu driver is really > enough. I've played a bit with newer Exyos SoCs some time ago. If I > remember right, if you limit the iommu functionality to the essential > things like mapping pages to IO-virtual space, the hardware differences > between SYSMMU v5 (already supported by the exynos-iommu driver) and v7 > are just a matter of changing a one register during the initialization > and different bits the page fault reason decoding. You must of course > rely on the DMA-mapping framework and its implementation based on > mainline DMA-IOMMU helpers. All the code for custom iommu group(s) > handling or extended fault management are not needed for the initial > version. > Thanks for the advice! Just implemented some testing driver, which uses "Emulated Translation" registers available on SysMMU v7. That's one way to verify the IOMMU driver with no actual users of it. It works fine with vendor SysMMU driver I ported to mainline earlier, and now I'm trying to use it with exynos-sysmmu driver (existing upstream). If you're curious -- I can share the testing driver somewhere on GitHub. I believe the register you mentioned is PT_BASE one, so I used REG_V7_FLPT_BASE_VM = 0x800C instead of REG_V5_PT_BASE_PFN. But I didn't manage to get that far, unfortunately, as exynos_iommu_domain_alloc() function fails in my case, with BUG_ON() at this line: /* For mapping page table entries we rely on dma == phys */ BUG_ON(handle != virt_to_phys(domain->pgtable)); One possible explanation for this BUG is that "dma-ranges" property is not provided in DTS (which seems to be the case right now for all users of "samsung,exynos-sysmmu" driver). Because of that the SWIOTLB is used for dma_map_single() call (in exynos_iommu_domain_alloc() function), which in turn leads to that BUG. At least that's what happens in my case. The call chain looks like this: exynos_iommu_domain_alloc() v dma_map_single() v dma_map_single_attrs() v dma_map_page_attrs() v dma_direct_map_page() // dma_capable() == false v swiotlb_map() v swiotlb_tbl_map_single() And the last call of course always returns the address different than the address for allocated pgtable. E.g. in my case I see this: handle = 0x00000000fbfff000 virt_to_phys(domain->pgtable) = 0x0000000880d0c000 Do you know what might be the reason for that? I just wonder how the SysMMU driver work for all existing Exynos platforms right now. I feel I might be missing something, like some DMA option should be enabled so that SWIOTLB is not used, or something like that. Please let me know if you have any idea on possible cause. The vendor's SysMMU driver is kinda different in that regard, as it doesn't use dma_map_single(), so I don't see such issue there. Thanks!
On 2022-06-21 20:57, Sam Protsenko wrote: > Hi Marek, > > On Fri, 21 Jan 2022 at 14:31, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > [snip] > >> >> Well, for starting point the existing exynos-iommu driver is really >> enough. I've played a bit with newer Exyos SoCs some time ago. If I >> remember right, if you limit the iommu functionality to the essential >> things like mapping pages to IO-virtual space, the hardware differences >> between SYSMMU v5 (already supported by the exynos-iommu driver) and v7 >> are just a matter of changing a one register during the initialization >> and different bits the page fault reason decoding. You must of course >> rely on the DMA-mapping framework and its implementation based on >> mainline DMA-IOMMU helpers. All the code for custom iommu group(s) >> handling or extended fault management are not needed for the initial >> version. >> > > Thanks for the advice! Just implemented some testing driver, which > uses "Emulated Translation" registers available on SysMMU v7. That's > one way to verify the IOMMU driver with no actual users of it. It > works fine with vendor SysMMU driver I ported to mainline earlier, and > now I'm trying to use it with exynos-sysmmu driver (existing > upstream). If you're curious -- I can share the testing driver > somewhere on GitHub. > > I believe the register you mentioned is PT_BASE one, so I used > REG_V7_FLPT_BASE_VM = 0x800C instead of REG_V5_PT_BASE_PFN. But I > didn't manage to get that far, unfortunately, as > exynos_iommu_domain_alloc() function fails in my case, with BUG_ON() > at this line: > > /* For mapping page table entries we rely on dma == phys */ > BUG_ON(handle != virt_to_phys(domain->pgtable)); > > One possible explanation for this BUG is that "dma-ranges" property is > not provided in DTS (which seems to be the case right now for all > users of "samsung,exynos-sysmmu" driver). Because of that the SWIOTLB > is used for dma_map_single() call (in exynos_iommu_domain_alloc() > function), which in turn leads to that BUG. At least that's what > happens in my case. The call chain looks like this: > > exynos_iommu_domain_alloc() > v > dma_map_single() > v > dma_map_single_attrs() > v > dma_map_page_attrs() > v > dma_direct_map_page() // dma_capable() == false > v > swiotlb_map() > v > swiotlb_tbl_map_single() > > And the last call of course always returns the address different than > the address for allocated pgtable. E.g. in my case I see this: > > handle = 0x00000000fbfff000 > virt_to_phys(domain->pgtable) = 0x0000000880d0c000 > > Do you know what might be the reason for that? I just wonder how the > SysMMU driver work for all existing Exynos platforms right now. I feel > I might be missing something, like some DMA option should be enabled > so that SWIOTLB is not used, or something like that. Please let me > know if you have any idea on possible cause. The vendor's SysMMU > driver is kinda different in that regard, as it doesn't use > dma_map_single(), so I don't see such issue there. If this SysMMU version is capable of addressing more than 32 bits, then exynos_iommu_probe_device() should set its DMA masks appropriately. (as a side note since I looked, the use of PAGE_SIZE/PAGE_SHIFT in the driver looks wrong, since I can't imagine that the hardware knows whether Linux is using 4KB, 16KB or 64KB and adjusts itself accordingly...) Robin.
On Wed, 22 Jun 2022 at 12:57, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > > On 22.06.2022 11:14, Robin Murphy wrote: > > On 2022-06-21 20:57, Sam Protsenko wrote: > >> Hi Marek, > >> > >> On Fri, 21 Jan 2022 at 14:31, Marek Szyprowski > >> <m.szyprowski@samsung.com> wrote: > >> > >> [snip] > >> > >>> > >>> Well, for starting point the existing exynos-iommu driver is really > >>> enough. I've played a bit with newer Exyos SoCs some time ago. If I > >>> remember right, if you limit the iommu functionality to the essential > >>> things like mapping pages to IO-virtual space, the hardware differences > >>> between SYSMMU v5 (already supported by the exynos-iommu driver) and v7 > >>> are just a matter of changing a one register during the initialization > >>> and different bits the page fault reason decoding. You must of course > >>> rely on the DMA-mapping framework and its implementation based on > >>> mainline DMA-IOMMU helpers. All the code for custom iommu group(s) > >>> handling or extended fault management are not needed for the initial > >>> version. > >>> > >> > >> Thanks for the advice! Just implemented some testing driver, which > >> uses "Emulated Translation" registers available on SysMMU v7. That's > >> one way to verify the IOMMU driver with no actual users of it. It > >> works fine with vendor SysMMU driver I ported to mainline earlier, and > >> now I'm trying to use it with exynos-sysmmu driver (existing > >> upstream). If you're curious -- I can share the testing driver > >> somewhere on GitHub. > >> > >> I believe the register you mentioned is PT_BASE one, so I used > >> REG_V7_FLPT_BASE_VM = 0x800C instead of REG_V5_PT_BASE_PFN. But I > >> didn't manage to get that far, unfortunately, as > >> exynos_iommu_domain_alloc() function fails in my case, with BUG_ON() > >> at this line: > >> > >> /* For mapping page table entries we rely on dma == phys */ > >> BUG_ON(handle != virt_to_phys(domain->pgtable)); > >> > >> One possible explanation for this BUG is that "dma-ranges" property is > >> not provided in DTS (which seems to be the case right now for all > >> users of "samsung,exynos-sysmmu" driver). Because of that the SWIOTLB > >> is used for dma_map_single() call (in exynos_iommu_domain_alloc() > >> function), which in turn leads to that BUG. At least that's what > >> happens in my case. The call chain looks like this: > >> > >> exynos_iommu_domain_alloc() > >> v > >> dma_map_single() > >> v > >> dma_map_single_attrs() > >> v > >> dma_map_page_attrs() > >> v > >> dma_direct_map_page() // dma_capable() == false > >> v > >> swiotlb_map() > >> v > >> swiotlb_tbl_map_single() > >> > >> And the last call of course always returns the address different than > >> the address for allocated pgtable. E.g. in my case I see this: > >> > >> handle = 0x00000000fbfff000 > >> virt_to_phys(domain->pgtable) = 0x0000000880d0c000 > >> > >> Do you know what might be the reason for that? I just wonder how the > >> SysMMU driver work for all existing Exynos platforms right now. I feel > >> I might be missing something, like some DMA option should be enabled > >> so that SWIOTLB is not used, or something like that. Please let me > >> know if you have any idea on possible cause. The vendor's SysMMU > >> driver is kinda different in that regard, as it doesn't use > >> dma_map_single(), so I don't see such issue there. > > > > If this SysMMU version is capable of addressing more than 32 bits, > > then exynos_iommu_probe_device() should set its DMA masks appropriately. > > Well, Sam's question was about the Exynos SYSMMU own platform device, > not the device for which Exynos SYSMMU manages the IO virtual address > space. > > Simply add something like > > dma_set_mask(dev, DMA_BIT_MASK(36)); > Yep, that one worked, thanks! Just submitted a patch, with a bit of additions: [1]. [1] https://lkml.org/lkml/2022/7/2/269 > to the beginning of the exynos_sysmmu_probe(). This will disable SWIOTLB > and switch to DMA-direct for the Exynos SYSMMU platform device. > > > > (as a side note since I looked, the use of PAGE_SIZE/PAGE_SHIFT in the > > driver looks wrong, since I can't imagine that the hardware knows > > whether Linux is using 4KB, 16KB or 64KB and adjusts itself > > accordingly...) > > Right, this has to be cleaned up. This driver was never used on systems > with kernel configuration for non-4KB pages. > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
This is a draft of a new IOMMU driver used in modern Exynos SoCs (like Exynos850) and Google's GS101 SoC (used in Pixel 6 phone). Most of its code were taken from GS101 downstream kernel [1], with some extra patches on top (fixes from Exynos850 downstream kernel and some porting changes to adapt it to the mainline kernel). All development history can be found at [2]. Similarities with existing exynos-iommu.c is minimal. I did some analysis using similarity-tester tool: 8<-------------------------------------------------------------------->8 $ sim_c -peu -S exynos-iommu.c "|" samsung-* exynos-iommu.c consists for 15 % of samsung-iommu.c material exynos-iommu.c consists for 1 % of samsung-iommu-fault.c material exynos-iommu.c consists for 3 % of samsung-iommu.h material 8<-------------------------------------------------------------------->8 So the similarity is very low, most of that code is some boilerplate that shouldn't be extracted to common code (like allocating the memory and requesting clocks/interrupts in probe function). It was tested on v5.4 Android kernel on Exynos850 (E850-96 board) with DPU use-case (displaying some graphics to the screen). Also it apparently works fine on v5.10 GS101 kernel (on Pixel 6). On mainline kernel I managed to build, match and bind the driver. No real world test was done, but the changes from v5.10 (where it works fine) are minimal (see [2] for details). So I'm pretty sure the driver is functional. For this patch series I'd like to receive some high-level review for driver's design and architecture. Coding style and API issues I can fix later, when sending real (not RFC) series. Particularly I'd like to hear some opinions about: - namings: Kconfig option, file names, module name, compatible, etc - modularity: should this driver be a different platform driver (like in this series), or should it be integrated into existing exynos-iommu.c driver somehow - dt-bindings: does it look ok as it is, or some interface changes are needed - internal driver architecture: approach seems to be similar to exynos-iommu.c, but any comments are welcome - ongoing work: please let me know if you're aware of some efforts to upstream this driver by some other party (e.g. Google engineers might be working on something similar) Basically, I want to figure out what should be changed/fixed in this driver (on a high level), so it can be considered "upstreamable". [1] https://android.googlesource.com/kernel/gs/ [2] https://github.com/joe-skb7/linux/commits/iommu-exynos850-dev Sam Protsenko (3): dt-bindings: iommu: Add bindings for samsung,sysmmu-v8 iommu/samsung: Introduce Exynos sysmmu-v8 driver arm64: defconfig: Enable sysmmu-v8 IOMMU .../bindings/iommu/samsung,sysmmu-v8.txt | 31 + arch/arm64/configs/defconfig | 2 + drivers/iommu/Kconfig | 13 + drivers/iommu/Makefile | 3 + drivers/iommu/samsung-iommu-fault.c | 617 +++++++ drivers/iommu/samsung-iommu-group.c | 50 + drivers/iommu/samsung-iommu.c | 1521 +++++++++++++++++ drivers/iommu/samsung-iommu.h | 216 +++ include/dt-bindings/soc/samsung,sysmmu-v8.h | 43 + 9 files changed, 2496 insertions(+) create mode 100644 Documentation/devicetree/bindings/iommu/samsung,sysmmu-v8.txt create mode 100644 drivers/iommu/samsung-iommu-fault.c create mode 100644 drivers/iommu/samsung-iommu-group.c create mode 100644 drivers/iommu/samsung-iommu.c create mode 100644 drivers/iommu/samsung-iommu.h create mode 100644 include/dt-bindings/soc/samsung,sysmmu-v8.h