Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <9a7f77cf-77bc-4a5a-af11-51464fbfdaf4@quicinc.com>
Date:   Thu, 26 Oct 2023 20:58:23 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/6] dt-bindings: iommu: Add Translation Buffer Unit
 bindings
Content-Language: en-US
To:     Robin Murphy <robin.murphy@arm.com>, Rob Herring <robh@kernel.org>
CC:     <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <will@kernel.org>, <joro@8bytes.org>, <devicetree@vger.kernel.org>,
        <andersson@kernel.org>, <konrad.dybcio@linaro.org>,
        <linux-arm-kernel@lists.infradead.org>, <iommu@lists.linux.dev>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <quic_cgoldswo@quicinc.com>, <quic_sukadev@quicinc.com>,
        <quic_pdaly@quicinc.com>, <quic_sudaraja@quicinc.com>,
        <djakov@kernel.org>
References: <20231019021923.13939-1-quic_c_gdjako@quicinc.com>
 <20231019021923.13939-2-quic_c_gdjako@quicinc.com>
 <20231024184248.GA252155-robh@kernel.org>
 <56bbb2ad-5cea-46ad-83be-d6d7fd9bbfde@arm.com>
From:   Georgi Djakov <quic_c_gdjako@quicinc.com>
In-Reply-To: <56bbb2ad-5cea-46ad-83be-d6d7fd9bbfde@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Robin,

Thanks for taking a look at this!

On 10/25/2023 1:26 AM, Robin Murphy wrote:
> On 2023-10-24 19:42, Rob Herring wrote:
>> On Wed, Oct 18, 2023 at 07:19:18PM -0700, Georgi Djakov wrote:
>>> The "apps_smmu" on the Qualcomm sdm845 platform is an implementation
>>> of the ARM SMMU-500, that consists of a single TCU (Translation Control
>>> Unit) and multiple TBUs (Translation Buffer Units). The TCU is already
>>> being described in the ARM SMMU DT schema. Add also bindings for the
>>> TBUs so that we can describe their properties.
>>
>> Arm SMMU-500 is an implementation, too. Is QCom's a modified
>> implementation or you are just the first to want to control TBU
>> resources?
> 
> It's very very modified. The stock MMU-500 has very few microarchitectural registers[1], they all live within the regular SMMU address space, are all Secure-only by default, and don't do anything like the shenanigans here.
> 
> That said, looking at patch #3, I don't really understand why we need any of this stuff upstream... AFAICS it's doing an insane amount of work to use complicated imp-def debug functionality to duplicate things that the main driver can already do far more efficiently. Sure, in general it seems like it could potentially be useful stuff for bringing up and debugging a new driver, but the Linux SMMUv2 driver is mature and frankly already closer to being obsolete than to being new...

Yes, the arm-smmu driver already does similar stuff with the ATS feature, but this unfortunately isn't available in Qualcomm's implementation. Instead of that, there is this eCATS thing for debugging various issues including hardware issues. It supports many features, but here we use it just for hardware page table walks. And in the majority of cases it's expected that the software and hardware page table walks give the same result, but if there is a difference, it's sign of a problem. For example, it helped in the past to trace some power management issues of the SMMU. This of course is a debug feature and can enabled when needed.

> [ digression since I can't be bothered to split this discussion by replying separately to patch #3: ]
> 
> I mean, just looking at qsmmuv500_iova_to_phys(), you do realise that that's going to be called potentially multiple times by iommu-dma for *every* dma_sync and dma_unmap call and really wants to be fast, right? This brings to mind all the work I did a couple of years back[2] because strict TLB invalidation on unmap was considered too slow for certain devices on QCom platforms by ChromeOS, yet what this achieves looks like it could easily be up to an order of magnitude slower again :(

No, this is not going to be called for every dma_sync and dma_unmap. In patch 5 we register a custom context_fault handler that uses this code to get information from the TBUs. So all of this is executed only when a context fault occurs. Does this sound acceptable?

[..]>>> +description:
>>> +  TBU nodes represent Translation Buffer Units in an ARM SMMU. Each TBU node
>>> +  should be a child node of the SMMU in the device tree.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - qcom,qsmmuv500-tbu
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: Address and size of the TBU's register space.
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: base
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  interconnects:
>>> +    maxItems: 1
> 
> What does this interconnect represent? MMU-500 TBUs don't access memory themselves[3], they only have an internal AXI Stream interface to the TCU to request translations.

It's to enable access from the CPU to the register space of the TBUs.

Thanks,
Georgi