| Message ID | 20220120201958.2649-1-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | iommu/samsung: Introduce Exynos sysmmu-v8 driver | expand |
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
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