Re: [RFC 2/3] iommu/samsung: Introduce Exynos sysmmu-v8 driver

[Sam Protsenko]

On Fri, 21 Jan 2022 at 10:40, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> >  - Hyesoo Yu <hyesoo.yu@xxxxxxxxxxx>
> >  - Janghyuck Kim <janghyuck.kim@xxxxxxxxxxx>
> >  - Jinkyu Yang <jinkyu1.yang@xxxxxxxxxxx>
> >
> > Some improvements were made by Google engineers:
> >
> >  - Alex <acnwigwe@xxxxxxxxxx>
> >  - Carlos Llamas <cmllamas@xxxxxxxxxx>
> >  - Daniel Mentz <danielmentz@xxxxxxxxxx>
> >  - Erick Reyes <erickreyes@xxxxxxxxxx>
> >  - J. Avila <elavila@xxxxxxxxxx>
> >  - Jonglin Lee <jonglin@xxxxxxxxxx>
> >  - Mark Salyzyn <salyzyn@xxxxxxxxxx>
> >  - Thierry Strudel <tstrudel@xxxxxxxxxx>
> >  - Will McVicker <willmcvicker@xxxxxxxxxx>
> >
> > [1] https://github.com/joe-skb7/linux/tree/iommu-exynos850-dev
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@xxxxxxxxxx>
> > ---
> >  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