| Message ID | 20220608083553.8697-5-tinghan.shen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for MT8195 SCP 2nd core | expand |
On Wed, Jun 08, 2022 at 04:35:48PM +0800, Tinghan Shen wrote: > 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. > + > 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 > if (IS_ERR(scp->sram_base)) > return dev_err_probe(dev, PTR_ERR(scp->sram_base), > "Failed to parse and map sram memory\n"); > @@ -873,7 +886,12 @@ static int scp_probe(struct platform_device *pdev) > scp->l1tcm_phys = res->start; > } > > - scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg"); > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); > + if (core_id == SCP_CORE_1) > + scp->reg_base = devm_ioremap(dev, res->start, resource_size(res)); > + else > + scp->reg_base = devm_ioremap_resource(dev, res); > + > if (IS_ERR(scp->reg_base)) > return dev_err_probe(dev, PTR_ERR(scp->reg_base), > "Failed to parse and map cfg memory\n"); > -- > 2.18.0 >
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
On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote: > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen <tinghan.shen@mediatek.com> > wrote: > > > 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. > > > > That is exactly why I suggest to initialise both cores within the same > probe() function. > Hi Mathieu, I'm thinking about how to initialise in the same probe() function. I'm wondering if this implies that using one scp driver to initialize 2 cores? If it is, I assume the dts descriptions for both cores should be contained in one node. When there's one node for both cores, it looks like that there is a problem of using dma_allocate_coherent(). Each core has its own reserved memory region. When there's only one device for both cores, it's not able to identify the memory region by the device parameter of dma_allocate_coherent(). Is it acceptable to consider manually allocating core 1 device in the probe() when probing core 0? Best regards, TingHan
On Fri, 16 Sept 2022 at 06:00, TingHan Shen <tinghan.shen@mediatek.com> wrote: > > On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote: > > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen <tinghan.shen@mediatek.com> > > wrote: > > > > > 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. > > > > > > > That is exactly why I suggest to initialise both cores within the same > > probe() function. > > > > Hi Mathieu, > > I'm thinking about how to initialise in the same probe() function. > I'm wondering if this implies that using one scp driver to initialize 2 cores? > If it is, I assume the dts descriptions for both cores should be contained in one node. > > When there's one node for both cores, it looks like that there is a problem of > using dma_allocate_coherent(). Each core has its own reserved memory region. > When there's only one device for both cores, it's not able to identify the memory region > by the device parameter of dma_allocate_coherent(). > > Is it acceptable to consider manually allocating core 1 device in the probe() when probing core 0? Look at what Suman did for TI's K3 R5[1] and DSP[2] platforms. Reviewing the bindings for both platforms will also give you a good idea of how things work. [1]. https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_r5_remoteproc.c#L1683 [2]. https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_dsp_remoteproc.c#L673 > > > Best regards, > TingHan > > > > > > > >
On Fri, 2022-09-16 at 11:15 -0600, Mathieu Poirier wrote: > On Fri, 16 Sept 2022 at 06:00, TingHan Shen <tinghan.shen@mediatek.com> wrote: > > > > On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote: > > > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen <tinghan.shen@mediatek.com> > > > wrote: > > > > > > > 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. > > > > > > > > > > That is exactly why I suggest to initialise both cores within the same > > > probe() function. > > > > > > > Hi Mathieu, > > > > I'm thinking about how to initialise in the same probe() function. > > I'm wondering if this implies that using one scp driver to initialize 2 cores? > > If it is, I assume the dts descriptions for both cores should be contained in one node. > > > > When there's one node for both cores, it looks like that there is a problem of > > using dma_allocate_coherent(). Each core has its own reserved memory region. > > When there's only one device for both cores, it's not able to identify the memory region > > by the device parameter of dma_allocate_coherent(). > > > > Is it acceptable to consider manually allocating core 1 device in the probe() when probing core 0? > > Look at what Suman did for TI's K3 R5[1] and DSP[2] platforms. > Reviewing the bindings for both platforms will also give you a good > idea of how things work. > > [1]. https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_r5_remoteproc.c*L1683__;Iw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX30z0XV4vo3vCb9Wy-w5ixOTmzbv1akubM$ > [2]. https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_dsp_remoteproc.c*L673__;Iw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX30z0XV4vo3vCb9Wy-w5ixOTmzbfE2dtBg$ > Hi Mathieu, My plan is changing the dts as following, scp core 0 { // Keep current properties untouched. compatible = "mediatek,mt8195-scp"; // core 0 properties... // Add a new property for multi-core scp. // if not present, it's single core. // if present and core id = 0, it's the main core, otherwise the sub cores. mediatek,scp-core = <0>; // add sub cores as sub node. // sub nodes can find parent by OF API. scp core 1 { // use the same compatile name as core 0. compatible = "mediatek,mt8195-scp"; // assign id > 0 to sub cores. mediatek,scp-core = <1>; // core 1 properties... }; }; The driver probe/remove behavior will be modified as below, scp probe() { // common init... // check core id to have different memory mapping flow if (core id == 0) // mapping cfg, sram and others else // mapping sram // reuse the cfg paddr/vaddr from core 0 // common init... if (core id == 0) { ret = of_platform_populate(...) // boot core 0 and sub cores rproc_add(); } else { // add sub core as sub device to main core rproc_add_subdev() rproc->auto_boot = false; rpoc_add(); } } scp_remove() { if (core id == 0) of_platform_depopulate() else rproc_remove_subdev() // remove core } Best regards, TingHan
On Mon, 19 Sept 2022 at 03:46, TingHan Shen <tinghan.shen@mediatek.com> wrote: > > On Fri, 2022-09-16 at 11:15 -0600, Mathieu Poirier wrote: > > On Fri, 16 Sept 2022 at 06:00, TingHan Shen <tinghan.shen@mediatek.com> wrote: > > > > > > On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote: > > > > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen <tinghan.shen@mediatek.com> > > > > wrote: > > > > > > > > > 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. > > > > > > > > > > > > > That is exactly why I suggest to initialise both cores within the same > > > > probe() function. > > > > > > > > > > Hi Mathieu, > > > > > > I'm thinking about how to initialise in the same probe() function. > > > I'm wondering if this implies that using one scp driver to initialize 2 cores? > > > If it is, I assume the dts descriptions for both cores should be contained in one node. > > > > > > When there's one node for both cores, it looks like that there is a problem of > > > using dma_allocate_coherent(). Each core has its own reserved memory region. > > > When there's only one device for both cores, it's not able to identify the memory region > > > by the device parameter of dma_allocate_coherent(). > > > > > > Is it acceptable to consider manually allocating core 1 device in the probe() when probing core 0? > > > > Look at what Suman did for TI's K3 R5[1] and DSP[2] platforms. > > Reviewing the bindings for both platforms will also give you a good > > idea of how things work. > > > > [1]. https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_r5_remoteproc.c*L1683__;Iw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX30z0XV4vo3vCb9Wy-w5ixOTmzbv1akubM$ > > [2]. https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_dsp_remoteproc.c*L673__;Iw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX30z0XV4vo3vCb9Wy-w5ixOTmzbfE2dtBg$ > > > > Hi Mathieu, > > My plan is changing the dts as following, > > scp core 0 { > // Keep current properties untouched. > compatible = "mediatek,mt8195-scp"; > > // core 0 properties... > > // Add a new property for multi-core scp. > // if not present, it's single core. > // if present and core id = 0, it's the main core, otherwise the sub cores. > mediatek,scp-core = <0>; > > // add sub cores as sub node. > // sub nodes can find parent by OF API. > scp core 1 { > // use the same compatile name as core 0. > compatible = "mediatek,mt8195-scp"; > > // assign id > 0 to sub cores. > mediatek,scp-core = <1>; > > // core 1 properties... > }; > }; > > > The driver probe/remove behavior will be modified as below, > > scp probe() { > // common init... > > // check core id to have different memory mapping flow > if (core id == 0) > // mapping cfg, sram and others > else > // mapping sram > // reuse the cfg paddr/vaddr from core 0 > > // common init... > > if (core id == 0) { > ret = of_platform_populate(...) > > // boot core 0 and sub cores > rproc_add(); > } else { > // add sub core as sub device to main core > rproc_add_subdev() > > rproc->auto_boot = false; > rpoc_add(); > } > } > > scp_remove() { > > if (core id == 0) > of_platform_depopulate() > else > rproc_remove_subdev() > > // remove core > } > Unfortunately I do not have the bandwidth to look at inlined or pseudo code. I will review this code with the next revision. > > > Best regards, > TingHan > > > >
> Subject: Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the > 2nd core of dual-core SCP > > On Fri, 2022-09-16 at 11:15 -0600, Mathieu Poirier wrote: > > On Fri, 16 Sept 2022 at 06:00, TingHan Shen > <tinghan.shen@mediatek.com> wrote: > > > > > > On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote: > > > > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen > > > > <tinghan.shen@mediatek.com> > > > > wrote: > > > > > > > > > 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. > > > > > > > > > > > > > That is exactly why I suggest to initialise both cores within the same > > > > probe() function. > > > > > > > > > > Hi Mathieu, > > > > > > I'm thinking about how to initialise in the same probe() function. > > > I'm wondering if this implies that using one scp driver to initialize 2 cores? > > > If it is, I assume the dts descriptions for both cores should be contained > in one node. > > > > > > When there's one node for both cores, it looks like that there is a > problem of > > > using dma_allocate_coherent(). Each core has its own reserved memory > region. > > > When there's only one device for both cores, it's not able to identify the > memory region > > > by the device parameter of dma_allocate_coherent(). > > > > > > Is it acceptable to consider manually allocating core 1 device in the > probe() when probing core 0? > > > > Look at what Suman did for TI's K3 R5[1] and DSP[2] platforms. > > Reviewing the bindings for both platforms will also give you a good > > idea of how things work. > > > > [1]. > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furlde > fense.com%2Fv3%2F__https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.0- > rc5%2Fsource%2Fdrivers%2Fremoteproc%2Fti_k3_r5_remoteproc.c*L1683_ > _%3BIw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX3 > 0z0XV4vo3vCb9Wy- > w5ixOTmzbv1akubM%24&data=05%7C01%7Cpeng.fan%40nxp.com%7 > C2cf76b2f15544cf3d06308da9a23f00a%7C686ea1d3bc2b4c6fa92cd99c5c301 > 635%7C0%7C0%7C637991776400238974%7CUnknown%7CTWFpbGZsb3d8e > yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D > %7C3000%7C%7C%7C&sdata=dz0FlQQmKI4C67XCX%2BZ6%2Bin%2Btq > 2DEWLb5YA%2FxGLOxHc%3D&reserved=0 > > [2]. > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furlde > fense.com%2Fv3%2F__https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.0- > rc5%2Fsource%2Fdrivers%2Fremoteproc%2Fti_k3_dsp_remoteproc.c*L673_ > _%3BIw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX3 > 0z0XV4vo3vCb9Wy- > w5ixOTmzbfE2dtBg%24&data=05%7C01%7Cpeng.fan%40nxp.com%7C > 2cf76b2f15544cf3d06308da9a23f00a%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35%7C0%7C0%7C637991776400238974%7CUnknown%7CTWFpbGZsb3d8ey > JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D > %7C3000%7C%7C%7C&sdata=Yn7MOo2uoDOVRW47O1yq8W3c%2BYg > G5URr7RdLKsmpLrk%3D&reserved=0 > > > > Hi Mathieu, > > My plan is changing the dts as following, > > scp core 0 { > // Keep current properties untouched. > compatible = "mediatek,mt8195-scp"; > > // core 0 properties... > > // Add a new property for multi-core scp. > // if not present, it's single core. > // if present and core id = 0, it's the main core, otherwise the sub > cores. > mediatek,scp-core = <0>; > > // add sub cores as sub node. > // sub nodes can find parent by OF API. > scp core 1 { > // use the same compatile name as core 0. > compatible = "mediatek,mt8195-scp"; > > // assign id > 0 to sub cores. > mediatek,scp-core = <1>; > > // core 1 properties... > }; > }; Not know the HW arch, but this looks a bit weird, scp core 1 is a sub node of scp core 0. > > > The driver probe/remove behavior will be modified as below, > > scp probe() { > // common init... > > // check core id to have different memory mapping flow > if (core id == 0) > // mapping cfg, sram and others > else > // mapping sram > // reuse the cfg paddr/vaddr from core 0 Oh, I understand why you need ioremap_resource, so core0/1 share same register address space? Regards, Peng. > > // common init... > > if (core id == 0) { > ret = of_platform_populate(...) > > // boot core 0 and sub cores > rproc_add(); > } else { > // add sub core as sub device to main core > rproc_add_subdev() > > rproc->auto_boot = false; > rpoc_add(); > } > } > > scp_remove() { > > if (core id == 0) > of_platform_depopulate() > else > rproc_remove_subdev() > > // remove core > } > > > > Best regards, > TingHan > > >
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); + 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); + if (IS_ERR(scp->sram_base)) return dev_err_probe(dev, PTR_ERR(scp->sram_base), "Failed to parse and map sram memory\n"); @@ -873,7 +886,12 @@ static int scp_probe(struct platform_device *pdev) scp->l1tcm_phys = res->start; } - scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg"); + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); + if (core_id == SCP_CORE_1) + scp->reg_base = devm_ioremap(dev, res->start, resource_size(res)); + else + scp->reg_base = devm_ioremap_resource(dev, res); + if (IS_ERR(scp->reg_base)) return dev_err_probe(dev, PTR_ERR(scp->reg_base), "Failed to parse and map cfg memory\n");
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. 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 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(-)