From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 76A9DCF9C5F for ; Fri, 20 Sep 2024 20:00:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:CC:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=pGg3xFSr4kJTIfcs44S25z/cOPJ3YCIit2QoVd/rW5A=; b=KAHseTVWs1P2SfHagKirAGJYu5 y4IE4nWROmi5L7hz116/gFsQ5UN8ej4gp+Fy0RdxRqFncegz6wjFVoiRSnDZXDwwR316Zn9aKQ0M2 ERPnA2/nunCFP48UBn/bdB5HX2k7GcUtOPK6Tw7moNSFouNGQsWdqxlp8Pxz5ola3ab7nDShGDsyY QLfCHtQNsMf9p3u25dhmr0onjfTEzhJDPTqAHpR/CpjOmiy9zVZLXD6p8+R7Q4TQpu2W0x6mPmMUc E06ryPI6nRMLLsXrEF6Gzhcs/fKrhUpKy+HZW4jtEbsdj9rTR9g5xTp6XK38hLTQfE3ZZJfWDRSXm /wMzySXw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1srjnb-0000000CtMG-3GI9; Fri, 20 Sep 2024 20:00:15 +0000 Received: from mx0a-0031df01.pphosted.com ([205.220.168.131]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1srjmS-0000000CtDk-23yQ for linux-arm-kernel@lists.infradead.org; Fri, 20 Sep 2024 19:59:06 +0000 Received: from pps.filterd (m0279863.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 48KIDJmU011965; Fri, 20 Sep 2024 19:58:54 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=qcppdkim1; bh= pGg3xFSr4kJTIfcs44S25z/cOPJ3YCIit2QoVd/rW5A=; b=XqsxKCthFZUTBpZy hAWHTmCt/vgQb+vyz6prpzpB2gQcyXbDpyQmu2UDrZyWuKbvIqW2wYSuLNgHkI3a PpPu+CpUphuNDi8FipRXTXTUiMzcMExTcJNBwZB6nlNFsk/9KhzZDaJ7I86NLPaJ eYqTiCsWpYEqj3OeBC9wj4SlOtDgYxqL269XLV088WfUF1hl5heZKLLL6Xxa6CAn B/bSd2GjvhQ5ZL5W7u34Zqvoyksfe7fl3s8P9Kix504xBealogdZBCy0hPCmmZoh g0y0Eo9BlfOH4ERqGZLwRhXedkgTY4eeUXgrtdRAyO7rHb9Fnhf32saCpZgsA96R afFf1g== Received: from nalasppmta01.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 41n4hftd27-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 20 Sep 2024 19:58:53 +0000 (GMT) Received: from nalasex01c.na.qualcomm.com (nalasex01c.na.qualcomm.com [10.47.97.35]) by NALASPPMTA01.qualcomm.com (8.18.1.2/8.18.1.2) with ESMTPS id 48KJwqJY020313 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 20 Sep 2024 19:58:52 GMT Received: from [10.216.31.199] (10.80.80.8) by nalasex01c.na.qualcomm.com (10.47.97.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.9; Fri, 20 Sep 2024 12:58:47 -0700 Message-ID: <9309fb5f-beaf-4395-9388-9f4ab8485bb4@quicinc.com> Date: Sat, 21 Sep 2024 01:28:43 +0530 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v14 5/6] iommu/arm-smmu: add ACTLR data and support for SC7280 To: Dmitry Baryshkov CC: Robin Murphy , Will Deacon , , , , , , , , , , , , References: <20240816174259.2056829-1-quic_bibekkum@quicinc.com> <20240816174259.2056829-6-quic_bibekkum@quicinc.com> <20240823155918.GD525@willie-the-truck> <3ae75a75-1717-40b6-9149-bc3673d520d6@quicinc.com> <20240827124714.GB4772@willie-the-truck> <35849d74-1197-446b-9a4c-1b8aabb38427@arm.com> Content-Language: en-US From: Bibek Kumar Patro In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01c.na.qualcomm.com (10.47.97.35) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: i2JTi-hzHAKvZ9AXyM8e55_4dpk1rBYq X-Proofpoint-ORIG-GUID: i2JTi-hzHAKvZ9AXyM8e55_4dpk1rBYq X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.60.29 definitions=2024-09-06_09,2024-09-06_01,2024-09-02_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 clxscore=1015 phishscore=0 malwarescore=0 mlxlogscore=999 mlxscore=0 impostorscore=0 adultscore=0 suspectscore=0 lowpriorityscore=0 spamscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.19.0-2408220000 definitions=main-2409200144 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240920_125904_581114_8943D22E X-CRM114-Status: GOOD ( 40.03 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 9/3/2024 6:43 PM, Dmitry Baryshkov wrote: > On Tue, 3 Sept 2024 at 15:59, Bibek Kumar Patro > wrote: >> >> >> >> On 8/30/2024 6:01 PM, Robin Murphy wrote: >>> On 30/08/2024 11:00 am, Bibek Kumar Patro wrote: >>>> >>>> >>>> On 8/27/2024 6:17 PM, Will Deacon wrote: >>>>> On Mon, Aug 26, 2024 at 04:33:24PM +0530, Bibek Kumar Patro wrote: >>>>>> >>>>>> >>>>>> On 8/23/2024 9:29 PM, Will Deacon wrote: >>>>>>> On Fri, Aug 16, 2024 at 11:12:58PM +0530, Bibek Kumar Patro wrote: >>>>>>>> Add ACTLR data table for SC7280 along with support for >>>>>>>> same including SC7280 specific implementation operations. >>>>>>>> >>>>>>>> Signed-off-by: Bibek Kumar Patro >>>>>>>> --- >>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 58 >>>>>>>> +++++++++++++++++++++- >>>>>>>> 1 file changed, 57 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> index dc143b250704..a776c7906c76 100644 >>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> @@ -31,6 +31,55 @@ >>>>>>>> #define PREFETCH_MODERATE (2 << PREFETCH_SHIFT) >>>>>>>> #define PREFETCH_DEEP (3 << PREFETCH_SHIFT) >>>>>>>> >>>>>>>> +static const struct actlr_config sc7280_apps_actlr_cfg[] = { >>>>>>>> + { 0x0800, 0x04e0, PREFETCH_DEFAULT | CMTLB }, >>>>>>>> + { 0x0900, 0x0402, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>>>> + { 0x0901, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>>>> + { 0x0d01, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>>>> + { 0x1181, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x1182, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x1183, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x1184, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x1185, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x1186, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x1187, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x1188, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x1189, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x118b, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x118c, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x118d, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x118e, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x118f, 0x0420, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> + { 0x2000, 0x0020, PREFETCH_DEFAULT | CMTLB }, >>>>>>>> + { 0x2040, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>>>>>>> + { 0x2062, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>>>>>>> + { 0x2080, 0x0020, PREFETCH_DEFAULT | CMTLB }, >>>>>>>> + { 0x20c0, 0x0020, PREFETCH_DEFAULT | CMTLB }, >>>>>>>> + { 0x2100, 0x0020, PREFETCH_DEFAULT | CMTLB }, >>>>>>>> + { 0x2140, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>>>>>>> + { 0x2180, 0x0020, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>>>> + { 0x2181, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>>>> + { 0x2183, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>>>> + { 0x2184, 0x0020, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>>>> + { 0x2187, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>>>> +}; >>>>>>>> + >>>>>>>> +static const struct actlr_config sc7280_gfx_actlr_cfg[] = { >>>>>>>> + { 0x0000, 0x07ff, PREFETCH_DEEP | CPRE | CMTLB }, >>>>>>>> +}; >>>>>>> >>>>>>> It's Will "stuck record" Deacon here again to say that I don't think >>>>>>> this data belongs in the driver. >>>>>>> >>>>>> >>>>>> Hi Will, >>>>>> >>>>>> It will be difficult to reach a consensus here, with Robin and the >>>>>> DT folks >>>>>> okay to keep it in the driver, while you believe it doesn't belong >>>>>> there. >>>>>> >>>>>> Robin, Rob, could you please share your thoughts on concluding the >>>>>> placement >>>>>> of this prefetch data? >>>>>> >>>>>> As discussed earlier [1], the prefetch value for each client doesn’t >>>>>> define >>>>>> the hardware topology and is implementation-defined register writes >>>>>> used by >>>>>> the software driver. >>>>> >>>>> It does reflect the hardware topology though, doesn't it? Those magic >>>>> hex >>>>> masks above refer to stream ids, so the table is hard-coding the >>>>> prefetch >>>>> values for particular matches. >>>> >>>> That is correct in the sense that stream id is mapped to context bank >>>> where these configurations are applied. >>>> However the other part of it is implementation-defined register/values >>>> for which community opinion was register/value kind of data, should not >>>> belong to device tree and are not generally approved of. >>>> >>>> Would also like to point out that the prefetch values are recommended >>>> settings and doesn’t mean these are the only configuration which would >>>> work for the soc. >>>> So the SID-to-prefetch isn't strictly SoC defined but is a software >>>> configuration, IMO. >>> >>> What's particularly confusing is that most of the IDs encoded here don't >>> actually seem to line up with what's in the respective SoC DTSIs... >>> >>> However by this point I'm wary of whether we've lost sight of *why* >>> we're doing this, and that we're deep into begging the question of >>> whether identifying devices by StreamID is the right thing to do in the >>> first place. For example, as best I can tell from a quick skim, we have >>> over 2 dozen lines of data here which all serve the exact same purpose >>> of applying PREFETCH_DEEP | CPRE | CMTLB to instances of >>> "qcom,fastrpc-compute-cb". In general it seems unlikely that the same >>> device would want wildly different prefetch settings across different >>> SoCs, or even between different instances in the same SoC, so I'm really >>> coming round to the conclusion that this data would probably be best >>> handled as an extension of the existing qcom_smmu_client_of_match >>> mechanism. >>> >> >> As per your design idea,do you mean to use qcom_smmu_client_of_match to >> identify the device using compatible string and apply the device >> specific settings for all the SoCs (instead of StreamID based device >> identification) ? >> >> something like this rough snippet(?): >> >> qcom_smmu_find_actlr_client(struct device *dev) >> { >> >> if (of_match_device(qcom_smmu_client_of_match, dev) == >> qcom,fastrpc-compute-cb ) >> qcom_smmu_set_actlr_value(dev, (PREFETCH_DEEP | CPRE | CMTLB)); >> /*where (PREFETCH_DEEP | CPRE | CMTLB) is used for compute-cb client.*/ >> >> else if (of_match_device(qcom_smmu_client_of_match, dev) == qcom,adreno ) >> qcom_smmu_set_actlr_value(dev, (PREFETCH_SHALLOW | CPRE | CMTLB)); >> /*Where (PREFETCH_SHALLOW | CPRE | CMTLB) is for adreno client. */ > > I like this idea, especially once it gets converted into a per-SoC > table of compatibles. Ack, Just posted the latest version v15 for this series , based on the compatible string based design approach [1] as discussed. While completion of patch, I came to know that it might sometimes be possible on Qualcomm SoC's, same IP/device on 2 different SoC can have different ACTLR settings as well. [1] _can take care of that case_ as well through different compatible strings but this can be handled through per-SoC solution as well. In the latest patch [1], I am going forward with Robin's suggestion of single table per smmu, and can later follow up there if any conflict happens. [1]: https://lore.kernel.org/all/20240920155813.3434021-6-quic_bibekkum@quicinc.com/> Thanks & regards, Bibek > >> >> } >> >> Let me know if my understanding is incorrect. >> Then in this case if different SoC would have a different settings for >> same device, then everytime a new compatible would be necessary for same >> device on different SoC? >> >> On similar lines there is another TBU based approach which I can think >> of. Identify the TBU -> Identify clients from TopoID derived from SID >> range specified in qcom,stream-id-range -> Apply the client >> specific settings ? >> >> Both approaches would be driver-based, as they are now. >> >> Also I'd like to point out that in the current design, since we fixed >> the smr_is_subset arguments to make the stream IDs a subset of entries >> in the actlr_cfg table, we can reduce the number of entries in the >> table. This way, fewer SID-mask pairs can accommodate several stream IDs. >> >> Thanks & regards, >> Bibek >> >>> Thanks, >>> Robin. >>> >>>> >>>>> If I run on a different SoC configuration > with the same table, then >>>>> the prefetch settings will be applied to the >>>>> wrong devices. How is that not hardware topology? >>>>> >>>> >>>> The configuration table is tied to SoC compatible string however as I >>>> mentioned above, its basically a s/w recommended setting. >>>> (using prefetch settings other than the recommended values e.g >>>> PREFECH_DEFAULT instead of PREFETCH_DEEP would not render the device >>>> unusable unlike changing stream-ids which can make it unusable). >>>> >>>> Since it is implementation specific we cannot have a generic DT binding, >>>> tying stream ids to these recommended settings. >>>> Even with qcom specific binding due to dependency on implementation, not >>>> sure if we would be able to maintain consistency. >>>> >>>> So from maintenance perspective carrying these in driver appear to be >>>> simpler/flexible. And if it doesn’t violate existing precedence, we >>>> would prefer to carry it that way. >>>> >>>> This parallels how _"QoS settings"_ are handled within the driver >>>> (similar to this example [1]). >>>> >>>> [1]. >>>> https://lore.kernel.org/linux-arm-msm/20231030-sc8280xp-dpu-safe-lut-v1-1-6d485d7b428f@quicinc.com/#t >>>> >>>> Thanks & regards, >>>> Bibek >>>> >>>>> WIll > > >