From: Tinghan Shen <tinghan.shen@mediatek.com> To: <mathieu.poirier@linaro.org> Cc: <Project_Global_Chrome_Upstream_Group@mediatek.com>, <bjorn.andersson@linaro.org>, <bleung@chromium.org>, <chrome-platform@lists.linux.dev>, <devicetree@vger.kernel.org>, <dnojiri@chromium.org>, <enric.balletbo@collabora.com>, <groeck@chromium.org>, <gustavoars@kernel.org>, <keescook@chromium.org>, <krzk+dt@kernel.org>, <lee.jones@linaro.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linux-mediatek@lists.infradead.org>, <linux-remoteproc@vger.kernel.org>, <matthias.bgg@gmail.com>, <pmalani@chromium.org>, <robh+dt@kernel.org>, <sebastian.reichel@collabora.com>, <tinghan.shen@mediatek.com>, <weishunc@google.com> 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 [thread overview] Message-ID: <20220908111757.14633-1-tinghan.shen@mediatek.com> (raw) In-Reply-To: <20220829194247.GC2264818@p14s> 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 <tinghan.shen@mediatek.com> > > --- > > 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
WARNING: multiple messages have this Message-ID (diff)
From: Tinghan Shen <tinghan.shen@mediatek.com> To: <mathieu.poirier@linaro.org> Cc: <Project_Global_Chrome_Upstream_Group@mediatek.com>, <bjorn.andersson@linaro.org>, <bleung@chromium.org>, <chrome-platform@lists.linux.dev>, <devicetree@vger.kernel.org>, <dnojiri@chromium.org>, <enric.balletbo@collabora.com>, <groeck@chromium.org>, <gustavoars@kernel.org>, <keescook@chromium.org>, <krzk+dt@kernel.org>, <lee.jones@linaro.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linux-mediatek@lists.infradead.org>, <linux-remoteproc@vger.kernel.org>, <matthias.bgg@gmail.com>, <pmalani@chromium.org>, <robh+dt@kernel.org>, <sebastian.reichel@collabora.com>, <tinghan.shen@mediatek.com>, <weishunc@google.com> 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 [thread overview] Message-ID: <20220908111757.14633-1-tinghan.shen@mediatek.com> (raw) In-Reply-To: <20220829194247.GC2264818@p14s> 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 <tinghan.shen@mediatek.com> > > --- > > 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
next prev parent reply other threads:[~2022-09-08 11:21 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-08 8:35 [PATCH v2 0/9] Add support for MT8195 SCP 2nd core Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` [PATCH v2 1/9] dt-binding: remoteproc: mediatek: Support dual-core SCP Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-09 20:51 ` Rob Herring 2022-06-09 20:51 ` Rob Herring 2022-06-09 20:51 ` Rob Herring 2022-06-08 8:35 ` [PATCH v2 2/9] remoteproc: mediatek: Support hanlding scp core 1 wdt timeout Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-08-29 17:40 ` Mathieu Poirier 2022-08-29 17:40 ` Mathieu Poirier 2022-09-08 10:38 ` Tinghan Shen 2022-09-08 10:38 ` Tinghan Shen 2022-06-08 8:35 ` [PATCH v2 3/9] remoteproc: mediatek: Add SCP core 1 register definitions Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-08-29 17:46 ` Mathieu Poirier 2022-08-29 17:46 ` Mathieu Poirier 2022-06-08 8:35 ` [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-08-29 19:42 ` Mathieu Poirier 2022-08-29 19:42 ` Mathieu Poirier 2022-09-08 11:17 ` Tinghan Shen [this message] 2022-09-08 11:17 ` Tinghan Shen [not found] ` <CANLsYkx6kXk8u_ajFbnhdWTkZBLtrq_z02jryLBSVH0x--_ZFw@mail.gmail.com> 2022-09-16 11:59 ` TingHan Shen 2022-09-16 11:59 ` TingHan Shen 2022-09-16 17:15 ` Mathieu Poirier 2022-09-16 17:15 ` Mathieu Poirier 2022-09-19 9:46 ` TingHan Shen 2022-09-19 9:46 ` TingHan Shen 2022-09-19 20:53 ` Mathieu Poirier 2022-09-19 20:53 ` Mathieu Poirier 2022-09-23 7:12 ` Peng Fan 2022-09-23 7:12 ` Peng Fan 2022-06-08 8:35 ` [PATCH v2 5/9] remoteproc: mediatek: Add chip dependent operations for SCP core 1 Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` [PATCH v2 6/9] remoteproc: mediatek: Add SCP core 1 SRAM offset Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` [PATCH v2 7/9] remoteproc: mediatek: Add SCP core 1 as a rproc subdevice Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` [PATCH v2 8/9] remoteproc: mediatek: Wait SCP core 1 probe done Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` [PATCH v2 9/9] mfd: cros_ec: Add SCP core 1 as a new CrOS EC MCU Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-08 8:35 ` Tinghan Shen 2022-06-09 5:45 ` [PATCH v2 0/9] Add support for MT8195 SCP 2nd core Tinghan Shen 2022-06-09 5:45 ` Tinghan Shen 2022-06-09 5:45 ` Tinghan Shen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220908111757.14633-1-tinghan.shen@mediatek.com \ --to=tinghan.shen@mediatek.com \ --cc=Project_Global_Chrome_Upstream_Group@mediatek.com \ --cc=bjorn.andersson@linaro.org \ --cc=bleung@chromium.org \ --cc=chrome-platform@lists.linux.dev \ --cc=devicetree@vger.kernel.org \ --cc=dnojiri@chromium.org \ --cc=enric.balletbo@collabora.com \ --cc=groeck@chromium.org \ --cc=gustavoars@kernel.org \ --cc=keescook@chromium.org \ --cc=krzk+dt@kernel.org \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-remoteproc@vger.kernel.org \ --cc=mathieu.poirier@linaro.org \ --cc=matthias.bgg@gmail.com \ --cc=pmalani@chromium.org \ --cc=robh+dt@kernel.org \ --cc=sebastian.reichel@collabora.com \ --cc=weishunc@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.