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

[Marek Szyprowski]

Hi Sam,

On 21.01.2022 12:08, Sam Protsenko wrote:
> 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://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@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.
>>
>>> ...
>> 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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland