Message-ID: <a93b0dac-6a5a-309a-e4c0-7b0d0911bd4c@linaro.org>
Date:   Mon, 13 Feb 2023 22:42:10 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH v9 11/14] arm64: dts: qcom: sm8250: add description of
 Qualcomm Crypto Engine IP
To:     Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Thara Gopinath <thara.gopinath@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Andy Gross <agross@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-crypto@vger.kernel.org
References: <20230208183755.2907771-1-vladimir.zapolskiy@linaro.org>
 <20230208183755.2907771-12-vladimir.zapolskiy@linaro.org>
 <7b38331c-3b29-03c1-fbed-f5799d11ca1f@linaro.org>
 <ce42e852-fb73-08ca-48c7-9826ab087289@linaro.org>
Content-Language: en-US
From:   Bhupesh Sharma <bhupesh.sharma@linaro.org>
In-Reply-To: <ce42e852-fb73-08ca-48c7-9826ab087289@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Vladimir,

On 2/9/23 8:00 PM, Vladimir Zapolskiy wrote:
> Hi Bhupesh,
> 
> On 2/9/23 14:21, Bhupesh Sharma wrote:
>> Hi Vladimir,
>>
>> On 2/9/23 12:07 AM, Vladimir Zapolskiy wrote:
>>> Add description of QCE and its corresponding BAM DMA IPs on SM8250 SoC.
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>> ---
>>>    arch/arm64/boot/dts/qcom/sm8250.dtsi | 24 ++++++++++++++++++++++++
>>>    1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sm8250.dtsi
>>> index e59c16f74d17..d8698d18223e 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
>>> @@ -2215,6 +2215,30 @@ ufs_mem_phy_lanes: phy@1d87400 {
>>>                };
>>>            };
>>> +        cryptobam: dma-controller@1dc4000 {
>>> +            compatible = "qcom,bam-v1.7.0";
>>> +            reg = <0x0 0x01dc4000 0x0 0x24000>;
>>> +            interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
>>> +            #dma-cells = <1>;
>>> +            qcom,ee = <0>;
>>> +            qcom,controlled-remotely;
>>> +            num-channels = <8>;
>>> +            qcom,num-ees = <2>;
>>> +            iommus = <&apps_smmu 0x586 0x11>,
>>> +                 <&apps_smmu 0x596 0x11>;
>>> +        };
>>> +
>>> +        crypto: crypto@1dfa000 {
>>> +            compatible = "qcom,sm8250-qce", "qcom,sm8150-qce";
>>> +            reg = <0x0 0x01dfa000 0x0 0x6000>;
>>> +            dmas = <&cryptobam 6>, <&cryptobam 7>;
>>> +            dma-names = "rx", "tx";
>>> +            interconnects = <&aggre2_noc MASTER_CRYPTO_CORE_0 
>>> &mc_virt SLAVE_EBI_CH0>;
>>> +            interconnect-names = "memory";
>>> +            iommus = <&apps_smmu 0x586 0x11>,
>>> +                 <&apps_smmu 0x596 0x11>;
>>> +        };
>>> +
>>>            tcsr_mutex: hwlock@1f40000 {
>>>                compatible = "qcom,tcsr-mutex";
>>>                reg = <0x0 0x01f40000 0x0 0x40000>;
>>
>> This patch was part of the v7 arm64 dts fixes I sent out - see [1].
> 
> thank you for pointing it out, so there are two different v7 series of 
> related
> patches, as it's stated in the cover letter I based my v8 on top of 
> linux-crypto
> changes [*], and this particular change was developed independently from 
> yours.
> 
> And so, there are some differences, can you please comment on them?
> 
> - My change does not have 'interconnects' property under 
> dma-controller@1dc4000
>    device tree node, I believe it is not needed, but please correct me 
> here.
> - My change uses DMA channel number taken from the downstream code [**], is
>    there a reason to use different ones like in your change?
> - My change has a shorter list of IOMMUs also copied from the same 
> downstream
>    code, is there a reason to use a different extended list like in your 
> change?
> - On my end I have to retest your change without 'num-channels' and 
> 'qcom,num-ees'
>    properties, since this also seems as an important difference between 
> two patches.
> 
> Nevertheless, thank you again for bringing my attention to a different 
> patch
> series, I'll reuse the changes from it, and also publish all of them 
> together and
> in a separate changeset as Krzysztof suggested.
> 
>> Probably you can use it as a base and make the changes (interconnect
>> property for the BAM DMA node and qce-compatible names) directly there
>> and include it in your patch series.
> 
> [*]  
> https://lore.kernel.org/linux-arm-msm/20220920114051.1116441-1-bhupesh.sharma@linaro.org/
> [**] 
> https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/arch/arm64/boot/dts/qcom/kona.dtsi?h=LA.UM.8.12.3.1&id=f3dd4aaeb34438c877ccd42f5a48ccd554dd765a#n2979
> 
>> [1].
>> https://lore.kernel.org/linux-arm-msm/20220921045602.1462007-3-bhupesh.sharma@linaro.org/

Now that you mention it, I am a bit confused. My v7 series had 3 
sub-series (one each for the arm64 dts + defconfig, crypto & dma tree):

- 
https://lore.kernel.org/linux-arm-msm/20220921045602.1462007-1-bhupesh.sharma@linaro.org/
- 
https://lore.kernel.org/linux-arm-msm/20220920114051.1116441-1-bhupesh.sharma@linaro.org/
- 
https://lore.kernel.org/linux-arm-msm/20220921030649.1436434-1-bhupesh.sharma@linaro.org/

Where do you propose the v9 of the dma tree patch? I think you have
already clubbed the v9 arm64 dts patchset with this series
(you have left out the defconfig patch from there as it is already 
merged and the sm8150 dts change, so that I can send it later), right?

Having said that, with only the v9 crypto series you shared, I am not
able to bring up crypto on either sm8250-mtp or (with one dts patch 
added) on sa8155p-adp board(s). So, do I need to make other changes 
(include patches from my earlier series) to get the same working?

Thanks,
Bhupesh