Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
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
Message-ID: <20220908111757.14633-1-tinghan.shen@mediatek.com>
In-Reply-To: <20220829194247.GC2264818@p14s>
References: <20220829194247.GC2264818@p14s>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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