From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailgw02.mediatek.com (unknown [210.61.82.184]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 35A037C for ; Thu, 8 Sep 2022 11:21:58 +0000 (UTC) X-UUID: 275d55f4a7e44d4aa0f87045cdab10e6-20220908 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Date:Subject:CC:To:From; bh=8G3RXmk6EX/J/uFVEBqNvgPb8Qx9waaWfpLdoQyA9os=; b=fQzkJcAWMStV2PgW5K0hm4MnIJFjief+o/XVOZW47Bei/bwuUy4JBKAixn7dctKuXmiKUAf5CXpVaUQ9fwAX0JZYvU63sHUhbXGJMk9byaMS2XEgcvS4DEH2pw8oPP6L/X+TwfvlOOlJtaR+0HDaust8bpmbUhTQw5FTaukIvIw=; X-CID-P-RULE: Release_Ham X-CID-O-INFO: VERSION:1.1.10,REQID:2353ecbb-4a22-47c2-9d11-249a0598772b,OB:10, LOB:0,IP:0,URL:0,TC:0,Content:0,EDM:0,RT:0,SF:45,FILE:0,BULK:0,RULE:Releas e_Ham,ACTION:release,TS:45 X-CID-INFO: VERSION:1.1.10,REQID:2353ecbb-4a22-47c2-9d11-249a0598772b,OB:10,LO B:0,IP:0,URL:0,TC:0,Content:0,EDM:0,RT:0,SF:45,FILE:0,BULK:0,RULE:Release_ Ham,ACTION:release,TS:45 X-CID-META: VersionHash:84eae18,CLOUDID:13ffcaf5-6e85-48d9-afd8-0504bbfe04cb,C OID:28ec86ee805f,Recheck:0,SF:28|17|19|48,TC:nil,Content:0,EDM:-3,IP:nil,U RL:0,File:nil,Bulk:40,QS:nil,BEC:nil,COL:0 X-UUID: 275d55f4a7e44d4aa0f87045cdab10e6-20220908 Received: from mtkcas10.mediatek.inc [(172.21.101.39)] by mailgw02.mediatek.com (envelope-from ) (Generic MTA with TLSv1.2 ECDHE-RSA-AES256-SHA384 256/256) with ESMTP id 89100491; Thu, 08 Sep 2022 19:21:51 +0800 Received: from mtkmbs11n2.mediatek.inc (172.21.101.187) by mtkmbs10n2.mediatek.inc (172.21.101.183) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.792.3; Thu, 8 Sep 2022 19:21:50 +0800 Received: from mtksdccf07.mediatek.inc (172.21.84.99) by mtkmbs11n2.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.2.792.15 via Frontend Transport; Thu, 8 Sep 2022 19:21:50 +0800 From: Tinghan Shen To: CC: , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP Date: Thu, 8 Sep 2022 19:17:57 +0800 Message-ID: <20220908111757.14633-1-tinghan.shen@mediatek.com> X-Mailer: git-send-email 2.18.0 In-Reply-To: <20220829194247.GC2264818@p14s> References: <20220829194247.GC2264818@p14s> Precedence: bulk X-Mailing-List: chrome-platform@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain X-MTK: N Hi Mathieu, > > The mtk_scp.c driver only supports the single core SCP and the > > 1st core of a dual-core SCP. This patch extends it for the 2nd core. > > > > MT8195 SCP is a dual-core MCU. Both cores are housed in the same subsys. > > s/subsys/subsystem > > > They have the same viewpoint of registers and memory. > > > > Core 1 of the SCP features its own set of core configuration registers, > > interrupt controller, timers, and DMAs. The rest of the peripherals > > in this subsystem are shared by core 0 and core 1. > > > > As for memory, core 1 has its own cache memory. the SCP SRAM is shared > > /the/The > > > by core 0 and core 1. > > > > Signed-off-by: Tinghan Shen > > --- > > drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c > > index 3510c6d0bbc8..91b4aefde4ac 100644 > > --- a/drivers/remoteproc/mtk_scp.c > > +++ b/drivers/remoteproc/mtk_scp.c > > @@ -23,6 +23,10 @@ > > #define MAX_CODE_SIZE 0x500000 > > #define SECTION_NAME_IPI_BUFFER ".ipi_buffer" > > > > +#define SCP_CORE_0 0 > > +#define SCP_CORE_1 1 > > +#define SCP_CORE_SINGLE 0xF > > + > > /** > > * scp_get() - get a reference to SCP. > > * > > @@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev) > > struct resource *res; > > const char *fw_name = "scp.img"; > > int ret, i; > > + u32 core_id = SCP_CORE_SINGLE; > > > > ret = rproc_of_parse_firmware(dev, 0, &fw_name); > > if (ret < 0 && ret != -EINVAL) > > @@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev) > > scp->data = of_device_get_match_data(dev); > > platform_set_drvdata(pdev, scp); > > > > + ret = of_property_read_u32_index(dev->of_node, "mediatek,scp-core", 1, &core_id); > > + if (ret == 0) > > + dev_info(dev, "Boot SCP dual core %u\n", core_id); > > Why is the DT property "mediatek,scp-core" needed at all? Since the compatible > "mediatek,mt8195-scp-dual" has already been defined previously in this patchset, > initialising the second core, if present, is a matter of looking at the > compatile string. This idea of identify cores by the compatible looks workable. I'll update this series at next version. Thanks! > > > + > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); > > - scp->sram_base = devm_ioremap_resource(dev, res); > > + if (core_id == SCP_CORE_1) > > + scp->sram_base = devm_ioremap(dev, res->start, resource_size(res)); > > + else > > + scp->sram_base = devm_ioremap_resource(dev, res); > > + > > This looks very broken... For this to work you would need to have two DT > entries with the "mediatek,mt8195-scp-dual" compatible properly, one with > "mediatek,scp-core = <&scp_dual1 0>;" and another one with "mediatek,scp-core = <&scp_dual0 1>;". > > Which is also very broken... Here you have a binding whose first argument is a > reference to the core sibling while the second argument is a characteristic of > the current core, which is highly confusing. > > I suggest what when you see the compatible binding "mediatek,mt8195-scp", a > single core is initialized. If you see "mediatek,mt8195-scp-dual", both cores > are initialized as part of the _same_ probe. > > If the above analysis is not correct it means I misinterpreted your > work and if so, a serious amount of comments is needed _and_ a very detailed > example in "mtk,scp.yaml" that leaves no room for interpretation. > > I will stop reviewing this patchset until you have clarified how this works. > > Thanks, > Mathieu There's one problem of initializng the CORE1 using the same probe flow. The register space of CORE0 and CORE1 are overlapped in the device node. Both cores need to use the 'cfg' registers defined in scp yaml. The devm_ioremap_resource catches address overlapping and returns error when probing CORE1 driver. Best regards, TingHan 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 31FE3C6FA82 for ; Thu, 8 Sep 2022 11:53:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:CC:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=hEahAvAgOmBPhiP8UGGAGXX9bhuyfaj9EgylkEBCdNg=; b=eaNy3q/KHzA9aV Dn5Ei3+34wO2c6/y9feW6Q2wyVG1nCukBumqODGoac8Yki/mDgjIpSQ4KpnSMHBsxNBlZ5cF1Vrbd yHN7loXtTX5AlgR7zZfAJBrx6XI+EQd8G3dVXZXNyjcdprdtc4c6CXpB9UjTz+lRSd9iua78biE9A clnFduSCYo0WxmjEumGVMYjTLGK0LWqSlDYqA6fNQLaj0cH2+3Og0Rr06QkD58hRkNCqufvAKVZAd n+Wti2wCvwsSxrD3euqf1richlFiw6ef9q2p8Ib4GuGx/fX3SckMP5+1T3Deg3R0KsOXBHPqxAUz6 LNozCmecl7cRoeOztc7A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oWG5D-002vNL-M8; Thu, 08 Sep 2022 11:52:35 +0000 Received: from mailgw01.mediatek.com ([216.200.240.184]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oWG59-002vLO-Va; Thu, 08 Sep 2022 11:52:33 +0000 X-UUID: 58b244dc2ddd499a9eb73a7f91be7d27-20220908 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Date:Subject:CC:To:From; bh=8G3RXmk6EX/J/uFVEBqNvgPb8Qx9waaWfpLdoQyA9os=; b=fQzkJcAWMStV2PgW5K0hm4MnIJFjief+o/XVOZW47Bei/bwuUy4JBKAixn7dctKuXmiKUAf5CXpVaUQ9fwAX0JZYvU63sHUhbXGJMk9byaMS2XEgcvS4DEH2pw8oPP6L/X+TwfvlOOlJtaR+0HDaust8bpmbUhTQw5FTaukIvIw=; X-CID-P-RULE: Release_Ham X-CID-O-INFO: VERSION:1.1.10,REQID:9dc9893e-65b1-4bd8-b4e1-ef5be90de972,OB:0,L OB:0,IP:0,URL:0,TC:0,Content:0,EDM:0,RT:0,SF:0,FILE:0,BULK:0,RULE:Release_ Ham,ACTION:release,TS:0 X-CID-META: VersionHash:84eae18,CLOUDID:e85ed3eb-2856-4fce-b125-09d4c7ebe045,C OID:IGNORED,Recheck:0,SF:nil,TC:nil,Content:0,EDM:-3,IP:nil,URL:0,File:nil ,Bulk:nil,QS:nil,BEC:nil,COL:0 X-UUID: 58b244dc2ddd499a9eb73a7f91be7d27-20220908 Received: from mtkmbs11n1.mediatek.inc [(172.21.101.185)] by mailgw01.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384 256/256) with ESMTP id 56807483; Thu, 08 Sep 2022 04:52:26 -0700 Received: from mtkmbs11n2.mediatek.inc (172.21.101.187) by mtkmbs10n2.mediatek.inc (172.21.101.183) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.792.3; Thu, 8 Sep 2022 19:21:50 +0800 Received: from mtksdccf07.mediatek.inc (172.21.84.99) by mtkmbs11n2.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.2.792.15 via Frontend Transport; Thu, 8 Sep 2022 19:21:50 +0800 From: Tinghan Shen To: CC: , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP Date: Thu, 8 Sep 2022 19:17:57 +0800 Message-ID: <20220908111757.14633-1-tinghan.shen@mediatek.com> X-Mailer: git-send-email 2.18.0 In-Reply-To: <20220829194247.GC2264818@p14s> References: <20220829194247.GC2264818@p14s> MIME-Version: 1.0 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220908_045232_218791_F0F1E185 X-CRM114-Status: GOOD ( 34.49 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Mathieu, > > The mtk_scp.c driver only supports the single core SCP and the > > 1st core of a dual-core SCP. This patch extends it for the 2nd core. > > > > MT8195 SCP is a dual-core MCU. Both cores are housed in the same subsys. > > s/subsys/subsystem > > > They have the same viewpoint of registers and memory. > > > > Core 1 of the SCP features its own set of core configuration registers, > > interrupt controller, timers, and DMAs. The rest of the peripherals > > in this subsystem are shared by core 0 and core 1. > > > > As for memory, core 1 has its own cache memory. the SCP SRAM is shared > > /the/The > > > by core 0 and core 1. > > > > Signed-off-by: Tinghan Shen > > --- > > drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c > > index 3510c6d0bbc8..91b4aefde4ac 100644 > > --- a/drivers/remoteproc/mtk_scp.c > > +++ b/drivers/remoteproc/mtk_scp.c > > @@ -23,6 +23,10 @@ > > #define MAX_CODE_SIZE 0x500000 > > #define SECTION_NAME_IPI_BUFFER ".ipi_buffer" > > > > +#define SCP_CORE_0 0 > > +#define SCP_CORE_1 1 > > +#define SCP_CORE_SINGLE 0xF > > + > > /** > > * scp_get() - get a reference to SCP. > > * > > @@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev) > > struct resource *res; > > const char *fw_name = "scp.img"; > > int ret, i; > > + u32 core_id = SCP_CORE_SINGLE; > > > > ret = rproc_of_parse_firmware(dev, 0, &fw_name); > > if (ret < 0 && ret != -EINVAL) > > @@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev) > > scp->data = of_device_get_match_data(dev); > > platform_set_drvdata(pdev, scp); > > > > + ret = of_property_read_u32_index(dev->of_node, "mediatek,scp-core", 1, &core_id); > > + if (ret == 0) > > + dev_info(dev, "Boot SCP dual core %u\n", core_id); > > Why is the DT property "mediatek,scp-core" needed at all? Since the compatible > "mediatek,mt8195-scp-dual" has already been defined previously in this patchset, > initialising the second core, if present, is a matter of looking at the > compatile string. This idea of identify cores by the compatible looks workable. I'll update this series at next version. Thanks! > > > + > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); > > - scp->sram_base = devm_ioremap_resource(dev, res); > > + if (core_id == SCP_CORE_1) > > + scp->sram_base = devm_ioremap(dev, res->start, resource_size(res)); > > + else > > + scp->sram_base = devm_ioremap_resource(dev, res); > > + > > This looks very broken... For this to work you would need to have two DT > entries with the "mediatek,mt8195-scp-dual" compatible properly, one with > "mediatek,scp-core = <&scp_dual1 0>;" and another one with "mediatek,scp-core = <&scp_dual0 1>;". > > Which is also very broken... Here you have a binding whose first argument is a > reference to the core sibling while the second argument is a characteristic of > the current core, which is highly confusing. > > I suggest what when you see the compatible binding "mediatek,mt8195-scp", a > single core is initialized. If you see "mediatek,mt8195-scp-dual", both cores > are initialized as part of the _same_ probe. > > If the above analysis is not correct it means I misinterpreted your > work and if so, a serious amount of comments is needed _and_ a very detailed > example in "mtk,scp.yaml" that leaves no room for interpretation. > > I will stop reviewing this patchset until you have clarified how this works. > > Thanks, > Mathieu There's one problem of initializng the CORE1 using the same probe flow. The register space of CORE0 and CORE1 are overlapped in the device node. Both cores need to use the 'cfg' registers defined in scp yaml. The devm_ioremap_resource catches address overlapping and returns error when probing CORE1 driver. Best regards, TingHan _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel