This patchset needs to be applied on top of this one [1]. It refactors the STM32 platform code in order to introduce support for synchronising with the M4 remote processor that would have been started by the boot loader or another entity. It carries the same functionatlity as the previeous revision but account for changes in the remoteproc core to support synchronisation scenarios. Some RB tags have been removed when the content of the patch has strayed too far from the original version. See patch 3, 8, 9 and 12 for more details. Tested on ST's mp157c board. Thanks, Mathieu [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=277049 [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 Mathieu Poirier (12): remoteproc: stm32: Decouple rproc from memory translation remoteproc: stm32: Request IRQ with platform device remoteproc: stm32: Decouple rproc from DT parsing remoteproc: stm32: Remove memory translation from DT parsing remoteproc: stm32: Parse syscon that will manage M4 synchronisation remoteproc: stm32: Get coprocessor state remoteproc: stm32: Get loaded resource table for synchronisation remoteproc: stm32: Introduce new start ops for synchronisation remoteproc: stm32: Update M4 state in stm32_rproc_stop() remoteproc: stm32: Introduce new parse fw ops for synchronisation remoteproc: stm32: Introduce new loaded rsc ops for synchronisation remoteproc: stm32: Set synchronisation state machine if needed drivers/remoteproc/stm32_rproc.c | 262 ++++++++++++++++++++++++++++--- 1 file changed, 244 insertions(+), 18 deletions(-) -- 2.20.1
Remove the remote processor from the process of parsing the memory ranges since there is no correlation between them. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/stm32_rproc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 0f9d02ca4f5a..91fd59af0ffe 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -127,10 +127,10 @@ static int stm32_rproc_mem_release(struct rproc *rproc, return 0; } -static int stm32_rproc_of_memory_translations(struct rproc *rproc) +static int stm32_rproc_of_memory_translations(struct platform_device *pdev, + struct stm32_rproc *ddata) { - struct device *parent, *dev = rproc->dev.parent; - struct stm32_rproc *ddata = rproc->priv; + struct device *parent, *dev = &pdev->dev; struct device_node *np; struct stm32_rproc_mem *p_mems; struct stm32_rproc_mem_ranges *mem_range; @@ -606,7 +606,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) rproc->auto_boot = of_property_read_bool(np, "st,auto-boot"); - return stm32_rproc_of_memory_translations(rproc); + return stm32_rproc_of_memory_translations(pdev, ddata); } static int stm32_rproc_probe(struct platform_device *pdev) -- 2.20.1
Request IRQ with platform device rather than remote proc in order to call stm32_rproc_parse_dt() before rproc_alloc(). That way we can know whether we need to synchronise with the MCU or not. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/stm32_rproc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 91fd59af0ffe..1ac90adba9b1 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -261,7 +261,8 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) static irqreturn_t stm32_rproc_wdg(int irq, void *data) { - struct rproc *rproc = data; + struct platform_device *pdev = data; + struct rproc *rproc = platform_get_drvdata(pdev); rproc_report_crash(rproc, RPROC_WATCHDOG); @@ -553,7 +554,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) if (irq > 0) { err = devm_request_irq(dev, irq, stm32_rproc_wdg, 0, - dev_name(dev), rproc); + dev_name(dev), pdev); if (err) { dev_err(dev, "failed to request wdg irq\n"); return err; -- 2.20.1
Remove the remote processor from the process of parsing the device tree since (1) there is no correlation between them and (2) to use the information that was gathered to make a decision on whether to synchronise with the M4 or not. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/stm32_rproc.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 1ac90adba9b1..57a426ea620b 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -538,12 +538,11 @@ static int stm32_rproc_get_syscon(struct device_node *np, const char *prop, return err; } -static int stm32_rproc_parse_dt(struct platform_device *pdev) +static int stm32_rproc_parse_dt(struct platform_device *pdev, + struct stm32_rproc *ddata, bool *auto_boot) { struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; - struct rproc *rproc = platform_get_drvdata(pdev); - struct stm32_rproc *ddata = rproc->priv; struct stm32_syscon tz; unsigned int tzen; int err, irq; @@ -589,7 +588,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) err = regmap_read(tz.map, tz.reg, &tzen); if (err) { - dev_err(&rproc->dev, "failed to read tzen\n"); + dev_err(dev, "failed to read tzen\n"); return err; } ddata->secured_soc = tzen & tz.mask; @@ -605,7 +604,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) if (err) dev_info(dev, "failed to get pdds\n"); - rproc->auto_boot = of_property_read_bool(np, "st,auto-boot"); + *auto_boot = of_property_read_bool(np, "st,auto-boot"); return stm32_rproc_of_memory_translations(pdev, ddata); } @@ -616,6 +615,7 @@ static int stm32_rproc_probe(struct platform_device *pdev) struct stm32_rproc *ddata; struct device_node *np = dev->of_node; struct rproc *rproc; + bool auto_boot = false; int ret; ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); @@ -626,9 +626,16 @@ static int stm32_rproc_probe(struct platform_device *pdev) if (!rproc) return -ENOMEM; + ddata = rproc->priv; + rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE); + + ret = stm32_rproc_parse_dt(pdev, ddata, &auto_boot); + if (ret) + goto free_rproc; + + rproc->auto_boot = auto_boot; rproc->has_iommu = false; - ddata = rproc->priv; ddata->workqueue = create_workqueue(dev_name(dev)); if (!ddata->workqueue) { dev_err(dev, "cannot create workqueue\n"); @@ -638,13 +645,9 @@ static int stm32_rproc_probe(struct platform_device *pdev) platform_set_drvdata(pdev, rproc); - ret = stm32_rproc_parse_dt(pdev); - if (ret) - goto free_wkq; - ret = stm32_rproc_request_mbox(rproc); if (ret) - goto free_rproc; + goto free_wkq; ret = rproc_add(rproc); if (ret) -- 2.20.1
Other than one has to be done after the other, there is no correlation between memory translation and DT parsing. As move function stm32_rproc_of_memory_translations() to stm32_rproc_probe() so that stm32_rproc_parse_dt() can be extended to look for synchronisation related binding in a clean way. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/stm32_rproc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 57a426ea620b..658439d4b00a 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -606,7 +606,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, *auto_boot = of_property_read_bool(np, "st,auto-boot"); - return stm32_rproc_of_memory_translations(pdev, ddata); + return 0; } static int stm32_rproc_probe(struct platform_device *pdev) @@ -634,6 +634,10 @@ static int stm32_rproc_probe(struct platform_device *pdev) if (ret) goto free_rproc; + ret = stm32_rproc_of_memory_translations(pdev, ddata); + if (ret) + goto free_rproc; + rproc->auto_boot = auto_boot; rproc->has_iommu = false; ddata->workqueue = create_workqueue(dev_name(dev)); -- 2.20.1
Get from the DT the syncon to probe the state of the remote processor and the location of the resource table. Mainly based on the work published by Arnaud Pouliquen [1]. [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/stm32_rproc.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 658439d4b00a..a285f338bed8 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -70,6 +70,8 @@ struct stm32_rproc { struct reset_control *rst; struct stm32_syscon hold_boot; struct stm32_syscon pdds; + struct stm32_syscon m4_state; + struct stm32_syscon rsctbl; int wdg_irq; u32 nb_rmems; struct stm32_rproc_mem *rmems; @@ -606,6 +608,30 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, *auto_boot = of_property_read_bool(np, "st,auto-boot"); + /* + * See if we can check the M4 status, i.e if it was started + * from the boot loader or not. + */ + err = stm32_rproc_get_syscon(np, "st,syscfg-m4-state", + &ddata->m4_state); + if (err) { + /* remember this */ + ddata->m4_state.map = NULL; + /* no coprocessor state syscon (optional) */ + dev_warn(dev, "m4 state not supported\n"); + + /* no need to go further */ + return 0; + } + + /* See if we can get the resource table */ + err = stm32_rproc_get_syscon(np, "st,syscfg-rsc-tbl", + &ddata->rsctbl); + if (err) { + /* no rsc table syscon (optional) */ + dev_warn(dev, "rsc tbl syscon not supported\n"); + } + return 0; } -- 2.20.1
Introduce the required mechanic to get the state of the M4 when the remoteproc core is initialising. Mainly based on the work published by Arnaud Pouliquen [1]. [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/stm32_rproc.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index a285f338bed8..89fbd2ffac93 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -38,6 +38,15 @@ #define STM32_MBX_VQ1_ID 1 #define STM32_MBX_SHUTDOWN "shutdown" +#define RSC_TBL_SIZE (1024) + +#define M4_STATE_OFF 0 +#define M4_STATE_INI 1 +#define M4_STATE_CRUN 2 +#define M4_STATE_CSTOP 3 +#define M4_STATE_STANDBY 4 +#define M4_STATE_CRASH 5 + struct stm32_syscon { struct regmap *map; u32 reg; @@ -635,12 +644,23 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, return 0; } +static int stm32_rproc_get_m4_status(struct stm32_rproc *ddata, + unsigned int *state) +{ + /* See stm32_rproc_parse_dt() */ + if (!ddata->m4_state.map) + return -EINVAL; + + return regmap_read(ddata->m4_state.map, ddata->m4_state.reg, state); +} + static int stm32_rproc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct stm32_rproc *ddata; struct device_node *np = dev->of_node; struct rproc *rproc; + unsigned int state; bool auto_boot = false; int ret; @@ -664,6 +684,15 @@ static int stm32_rproc_probe(struct platform_device *pdev) if (ret) goto free_rproc; + ret = stm32_rproc_get_m4_status(ddata, &state); + if (ret) { + /* + * We couldn't get the coprocessor's state, assume + * it is not running. + */ + state = M4_STATE_OFF; + } + rproc->auto_boot = auto_boot; rproc->has_iommu = false; ddata->workqueue = create_workqueue(dev_name(dev)); -- 2.20.1
Get the resource table location when synchronising with the M4 so that the remoteproc and rpmsg subsystem can be initialised properly. Mainly based on the work published by Arnaud Pouliquen [1]. [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/stm32_rproc.c | 66 ++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 89fbd2ffac93..8ba69e903851 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -87,6 +87,7 @@ struct stm32_rproc { struct stm32_mbox mb[MBOX_NB_MBX]; struct workqueue_struct *workqueue; bool secured_soc; + void __iomem *rsc_va; }; static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da) @@ -654,6 +655,65 @@ static int stm32_rproc_get_m4_status(struct stm32_rproc *ddata, return regmap_read(ddata->m4_state.map, ddata->m4_state.reg, state); } +static int stm32_rproc_da_to_pa(struct platform_device *pdev, + struct stm32_rproc *ddata, + u64 da, phys_addr_t *pa) +{ + struct device *dev = &pdev->dev; + struct stm32_rproc_mem *p_mem; + unsigned int i; + + for (i = 0; i < ddata->nb_rmems; i++) { + p_mem = &ddata->rmems[i]; + + if (da < p_mem->dev_addr || + da >= p_mem->dev_addr + p_mem->size) + continue; + + *pa = da - p_mem->dev_addr + p_mem->bus_addr; + dev_dbg(dev, "da %llx to pa %#x\n", da, *pa); + + return 0; + } + + dev_err(dev, "can't translate da %llx\n", da); + + return -EINVAL; +} + +static int stm32_rproc_get_loaded_rsc_table(struct platform_device *pdev, + struct stm32_rproc *ddata) +{ + struct device *dev = &pdev->dev; + phys_addr_t rsc_pa; + u32 rsc_da; + int err; + + err = regmap_read(ddata->rsctbl.map, ddata->rsctbl.reg, &rsc_da); + if (err) { + dev_err(dev, "failed to read rsc tbl addr\n"); + return err; + } + + if (!rsc_da) + /* no rsc table */ + return 0; + + err = stm32_rproc_da_to_pa(pdev, ddata, rsc_da, &rsc_pa); + if (err) + return err; + + ddata->rsc_va = devm_ioremap_wc(dev, rsc_pa, RSC_TBL_SIZE); + if (IS_ERR_OR_NULL(ddata->rsc_va)) { + dev_err(dev, "Unable to map memory region: %pa+%zx\n", + &rsc_pa, RSC_TBL_SIZE); + ddata->rsc_va = NULL; + return -ENOMEM; + } + + return 0; +} + static int stm32_rproc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -693,6 +753,12 @@ static int stm32_rproc_probe(struct platform_device *pdev) state = M4_STATE_OFF; } + if (state == M4_STATE_CRUN) { + ret = stm32_rproc_get_loaded_rsc_table(pdev, ddata); + if (ret) + goto free_rproc; + } + rproc->auto_boot = auto_boot; rproc->has_iommu = false; ddata->workqueue = create_workqueue(dev_name(dev)); -- 2.20.1
Introduce new start functions to be used when synchonising with an MCU. Mainly based on the work published by Arnaud Pouliquen [1]. [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/stm32_rproc.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 8ba69e903851..404f17a97095 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -449,6 +449,13 @@ static int stm32_rproc_start(struct rproc *rproc) return stm32_rproc_set_hold_boot(rproc, true); } +static int stm32_rproc_sync_start(struct rproc *rproc) +{ + stm32_rproc_add_coredump_trace(rproc); + + return stm32_rproc_set_hold_boot(rproc, true); +} + static int stm32_rproc_stop(struct rproc *rproc) { struct stm32_rproc *ddata = rproc->priv; @@ -522,6 +529,10 @@ static struct rproc_ops st_rproc_ops = { .get_boot_addr = rproc_elf_get_boot_addr, }; +static __maybe_unused struct rproc_ops st_rproc_sync_ops = { + .start = stm32_rproc_sync_start, +}; + static const struct of_device_id stm32_rproc_match[] = { { .compatible = "st,stm32mp1-m4" }, {}, -- 2.20.1
Update the M4 co-processor state in function stm32_rproc_stop() so that it can be used in synchronisation scenarios. Mainly based on the work published by Arnaud Pouliquen [1]. [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/stm32_rproc.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 404f17a97095..86d23c35d805 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -493,6 +493,18 @@ static int stm32_rproc_stop(struct rproc *rproc) } } + /* update coprocessor state to OFF if available */ + if (ddata->m4_state.map) { + err = regmap_update_bits(ddata->m4_state.map, + ddata->m4_state.reg, + ddata->m4_state.mask, + M4_STATE_OFF); + if (err) { + dev_err(&rproc->dev, "failed to set copro state\n"); + return err; + } + } + return 0; } @@ -531,6 +543,7 @@ static struct rproc_ops st_rproc_ops = { static __maybe_unused struct rproc_ops st_rproc_sync_ops = { .start = stm32_rproc_sync_start, + .stop = stm32_rproc_stop, }; static const struct of_device_id stm32_rproc_match[] = { -- 2.20.1
Introduce new parse firmware rproc_ops functions to be used when synchonising with an MCU. Mainly based on the work published by Arnaud Pouliquen [1]. [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/stm32_rproc.c | 51 +++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 86d23c35d805..b8ae8aed5585 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -215,7 +215,34 @@ static int stm32_rproc_elf_load_rsc_table(struct rproc *rproc, return 0; } -static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) +static int stm32_rproc_sync_elf_load_rsc_table(struct rproc *rproc, + const struct firmware *fw) +{ + struct resource_table *table = NULL; + struct stm32_rproc *ddata = rproc->priv; + + if (ddata->rsc_va) { + table = (struct resource_table *)ddata->rsc_va; + /* Assuming that the resource table fits in 1kB is fair */ + rproc->cached_table = kmemdup(table, RSC_TBL_SIZE, GFP_KERNEL); + if (!rproc->cached_table) + return -ENOMEM; + + rproc->table_ptr = rproc->cached_table; + rproc->table_sz = RSC_TBL_SIZE; + return 0; + } + + rproc->cached_table = NULL; + rproc->table_ptr = NULL; + rproc->table_sz = 0; + + dev_warn(&rproc->dev, "no resource table found for this firmware\n"); + return 0; +} + +static int stm32_rproc_parse_memory_regions(struct rproc *rproc, + const struct firmware *fw) { struct device *dev = rproc->dev.parent; struct device_node *np = dev->of_node; @@ -268,9 +295,30 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) index++; } + return 0; +} + +static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) +{ + int ret = stm32_rproc_parse_memory_regions(rproc, fw); + + if (ret) + return ret; + return stm32_rproc_elf_load_rsc_table(rproc, fw); } +static int stm32_rproc_sync_parse_fw(struct rproc *rproc, + const struct firmware *fw) +{ + int ret = stm32_rproc_parse_memory_regions(rproc, fw); + + if (ret) + return ret; + + return stm32_rproc_sync_elf_load_rsc_table(rproc, fw); +} + static irqreturn_t stm32_rproc_wdg(int irq, void *data) { struct platform_device *pdev = data; @@ -544,6 +592,7 @@ static struct rproc_ops st_rproc_ops = { static __maybe_unused struct rproc_ops st_rproc_sync_ops = { .start = stm32_rproc_sync_start, .stop = stm32_rproc_stop, + .parse_fw = stm32_rproc_sync_parse_fw, }; static const struct of_device_id stm32_rproc_match[] = { -- 2.20.1
Introduce new elf find loaded resource table rproc_ops functions to be used when synchonising with an MCU. Mainly based on the work published by Arnaud Pouliquen [1]. [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/stm32_rproc.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index b8ae8aed5585..dcae6103e3df 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -319,6 +319,15 @@ static int stm32_rproc_sync_parse_fw(struct rproc *rproc, return stm32_rproc_sync_elf_load_rsc_table(rproc, fw); } +static struct resource_table * +stm32_rproc_sync_elf_find_loaded_rsc_table(struct rproc *rproc, + const struct firmware *fw) +{ + struct stm32_rproc *ddata = rproc->priv; + + return (struct resource_table *)ddata->rsc_va; +} + static irqreturn_t stm32_rproc_wdg(int irq, void *data) { struct platform_device *pdev = data; @@ -593,6 +602,7 @@ static __maybe_unused struct rproc_ops st_rproc_sync_ops = { .start = stm32_rproc_sync_start, .stop = stm32_rproc_stop, .parse_fw = stm32_rproc_sync_parse_fw, + .find_loaded_rsc_table = stm32_rproc_sync_elf_find_loaded_rsc_table, }; static const struct of_device_id stm32_rproc_match[] = { -- 2.20.1
Set the flags and operations to use if the M4 has been started by another entity than the remoteproc core. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/stm32_rproc.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index dcae6103e3df..02dad3f51c7a 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -598,13 +598,20 @@ static struct rproc_ops st_rproc_ops = { .get_boot_addr = rproc_elf_get_boot_addr, }; -static __maybe_unused struct rproc_ops st_rproc_sync_ops = { +static struct rproc_ops st_rproc_sync_ops = { .start = stm32_rproc_sync_start, .stop = stm32_rproc_stop, + .kick = stm32_rproc_kick, .parse_fw = stm32_rproc_sync_parse_fw, .find_loaded_rsc_table = stm32_rproc_sync_elf_find_loaded_rsc_table, }; +static struct rproc_sync_flags st_sync_flags = { + .on_init = true, /* sync with MCU when the kernel boots */ + .after_stop = false, /* don't resync with MCU if stopped from sysfs */ + .after_crash = false, /* don't resync with MCU after a crash */ +}; + static const struct of_device_id stm32_rproc_match[] = { { .compatible = "st,stm32mp1-m4" }, {}, @@ -803,6 +810,7 @@ static int stm32_rproc_probe(struct platform_device *pdev) struct stm32_rproc *ddata; struct device_node *np = dev->of_node; struct rproc *rproc; + struct rproc_sync_flags sync_flags = {0}; unsigned int state; bool auto_boot = false; int ret; @@ -837,11 +845,17 @@ static int stm32_rproc_probe(struct platform_device *pdev) } if (state == M4_STATE_CRUN) { + auto_boot = true; + sync_flags = st_sync_flags; ret = stm32_rproc_get_loaded_rsc_table(pdev, ddata); if (ret) goto free_rproc; } + ret = rproc_set_state_machine(rproc, &st_rproc_sync_ops, sync_flags); + if (ret) + goto free_rproc; + rproc->auto_boot = auto_boot; rproc->has_iommu = false; ddata->workqueue = create_workqueue(dev_name(dev)); -- 2.20.1
Hi Mathieu, On 4/24/20 10:24 PM, Mathieu Poirier wrote: > Remove the remote processor from the process of parsing the device tree > since (1) there is no correlation between them and (2) to use the > information that was gathered to make a decision on whether to > synchronise with the M4 or not. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > drivers/remoteproc/stm32_rproc.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 1ac90adba9b1..57a426ea620b 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -538,12 +538,11 @@ static int stm32_rproc_get_syscon(struct device_node *np, const char *prop, > return err; > } > > -static int stm32_rproc_parse_dt(struct platform_device *pdev) > +static int stm32_rproc_parse_dt(struct platform_device *pdev, > + struct stm32_rproc *ddata, bool *auto_boot) > { > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > - struct rproc *rproc = platform_get_drvdata(pdev); > - struct stm32_rproc *ddata = rproc->priv; > struct stm32_syscon tz; > unsigned int tzen; > int err, irq; > @@ -589,7 +588,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) > > err = regmap_read(tz.map, tz.reg, &tzen); > if (err) { > - dev_err(&rproc->dev, "failed to read tzen\n"); > + dev_err(dev, "failed to read tzen\n"); > return err; > } > ddata->secured_soc = tzen & tz.mask; > @@ -605,7 +604,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) > if (err) > dev_info(dev, "failed to get pdds\n"); > > - rproc->auto_boot = of_property_read_bool(np, "st,auto-boot"); > + *auto_boot = of_property_read_bool(np, "st,auto-boot"); > > return stm32_rproc_of_memory_translations(pdev, ddata); > } > @@ -616,6 +615,7 @@ static int stm32_rproc_probe(struct platform_device *pdev) > struct stm32_rproc *ddata; > struct device_node *np = dev->of_node; > struct rproc *rproc; > + bool auto_boot = false; Nitpicking: Seems that you don't need to initialize it. Perhaps you can simply suppress the local variable and directly use rproc->auto_boot. else LGTM Thanks, Arnaud > int ret; > > ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > @@ -626,9 +626,16 @@ static int stm32_rproc_probe(struct platform_device *pdev) > if (!rproc) > return -ENOMEM; > > + ddata = rproc->priv; > + > rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE); > + > + ret = stm32_rproc_parse_dt(pdev, ddata, &auto_boot); > + if (ret) > + goto free_rproc; > + > + rproc->auto_boot = auto_boot; > rproc->has_iommu = false; > - ddata = rproc->priv; > ddata->workqueue = create_workqueue(dev_name(dev)); > if (!ddata->workqueue) { > dev_err(dev, "cannot create workqueue\n"); > @@ -638,13 +645,9 @@ static int stm32_rproc_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, rproc); > > - ret = stm32_rproc_parse_dt(pdev); > - if (ret) > - goto free_wkq; > - > ret = stm32_rproc_request_mbox(rproc); > if (ret) > - goto free_rproc; > + goto free_wkq; > > ret = rproc_add(rproc); > if (ret) >
On 4/24/20 10:24 PM, Mathieu Poirier wrote: > Introduce the required mechanic to get the state of the M4 when the > remoteproc core is initialising. > > Mainly based on the work published by Arnaud Pouliquen [1]. > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Reviewed-by: Loic Pallardy <loic.pallardy@st.com> > --- > drivers/remoteproc/stm32_rproc.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index a285f338bed8..89fbd2ffac93 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -38,6 +38,15 @@ > #define STM32_MBX_VQ1_ID 1 > #define STM32_MBX_SHUTDOWN "shutdown" > > +#define RSC_TBL_SIZE (1024) > + > +#define M4_STATE_OFF 0 > +#define M4_STATE_INI 1 > +#define M4_STATE_CRUN 2 > +#define M4_STATE_CSTOP 3 > +#define M4_STATE_STANDBY 4 > +#define M4_STATE_CRASH 5 > + > struct stm32_syscon { > struct regmap *map; > u32 reg; > @@ -635,12 +644,23 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, > return 0; > } > > +static int stm32_rproc_get_m4_status(struct stm32_rproc *ddata, > + unsigned int *state) > +{ > + /* See stm32_rproc_parse_dt() */ > + if (!ddata->m4_state.map) > + return -EINVAL; > + > + return regmap_read(ddata->m4_state.map, ddata->m4_state.reg, state); > +} i would manage here the default state depending on the error types if (!ddata->m4_state.map { /* * We couldn't get the coprocessor's state, assume * it is not running. */ state = M4_STATE_OFF; return 0; } return regmap_read(ddata->m4_state.map, ddata->m4_state.reg, state); > + > static int stm32_rproc_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct stm32_rproc *ddata; > struct device_node *np = dev->of_node; > struct rproc *rproc; > + unsigned int state; > bool auto_boot = false; > int ret; > > @@ -664,6 +684,15 @@ static int stm32_rproc_probe(struct platform_device *pdev) > if (ret) > goto free_rproc; > > + ret = stm32_rproc_get_m4_status(ddata, &state); > + if (ret) { > + /* > + * We couldn't get the coprocessor's state, assume > + * it is not running. > + */ > + state = M4_STATE_OFF; So here just handle the error; Regards Arnaud > + } > + > rproc->auto_boot = auto_boot; > rproc->has_iommu = false; > ddata->workqueue = create_workqueue(dev_name(dev)); >
On 4/24/20 10:25 PM, Mathieu Poirier wrote: > Set the flags and operations to use if the M4 has been started > by another entity than the remoteproc core. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > drivers/remoteproc/stm32_rproc.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index dcae6103e3df..02dad3f51c7a 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -598,13 +598,20 @@ static struct rproc_ops st_rproc_ops = { > .get_boot_addr = rproc_elf_get_boot_addr, > }; > > -static __maybe_unused struct rproc_ops st_rproc_sync_ops = { > +static struct rproc_ops st_rproc_sync_ops = { > .start = stm32_rproc_sync_start, > .stop = stm32_rproc_stop, > + .kick = stm32_rproc_kick, Seems independent of the path. > .parse_fw = stm32_rproc_sync_parse_fw, > .find_loaded_rsc_table = stm32_rproc_sync_elf_find_loaded_rsc_table, > }; > > +static struct rproc_sync_flags st_sync_flags = { > + .on_init = true, /* sync with MCU when the kernel boots */ > + .after_stop = false, /* don't resync with MCU if stopped from sysfs */ > + .after_crash = false, /* don't resync with MCU after a crash */ > +}; > + could be const > static const struct of_device_id stm32_rproc_match[] = { > { .compatible = "st,stm32mp1-m4" }, > {}, > @@ -803,6 +810,7 @@ static int stm32_rproc_probe(struct platform_device *pdev) > struct stm32_rproc *ddata; > struct device_node *np = dev->of_node; > struct rproc *rproc; > + struct rproc_sync_flags sync_flags = {0}; > unsigned int state; > bool auto_boot = false; > int ret; > @@ -837,11 +845,17 @@ static int stm32_rproc_probe(struct platform_device *pdev) > } > > if (state == M4_STATE_CRUN) { > + auto_boot = true; > + sync_flags = st_sync_flags; seems an useless copy Regards, Arnaud > ret = stm32_rproc_get_loaded_rsc_table(pdev, ddata); > if (ret) > goto free_rproc; > } > > + ret = rproc_set_state_machine(rproc, &st_rproc_sync_ops, sync_flags); > + if (ret) > + goto free_rproc; > + > rproc->auto_boot = auto_boot; > rproc->has_iommu = false; > ddata->workqueue = create_workqueue(dev_name(dev)); >
On 4/24/20 10:25 PM, Mathieu Poirier wrote: > Introduce new start functions to be used when synchonising with an MCU. > > Mainly based on the work published by Arnaud Pouliquen [1]. > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> Thanks, Arnaud > --- > drivers/remoteproc/stm32_rproc.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 8ba69e903851..404f17a97095 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -449,6 +449,13 @@ static int stm32_rproc_start(struct rproc *rproc) > return stm32_rproc_set_hold_boot(rproc, true); > } > > +static int stm32_rproc_sync_start(struct rproc *rproc) > +{ > + stm32_rproc_add_coredump_trace(rproc); > + > + return stm32_rproc_set_hold_boot(rproc, true); > +} > + > static int stm32_rproc_stop(struct rproc *rproc) > { > struct stm32_rproc *ddata = rproc->priv; > @@ -522,6 +529,10 @@ static struct rproc_ops st_rproc_ops = { > .get_boot_addr = rproc_elf_get_boot_addr, > }; > > +static __maybe_unused struct rproc_ops st_rproc_sync_ops = { > + .start = stm32_rproc_sync_start, > +}; > + > static const struct of_device_id stm32_rproc_match[] = { > { .compatible = "st,stm32mp1-m4" }, > {}, >
On 4/24/20 10:25 PM, Mathieu Poirier wrote: > Update the M4 co-processor state in function stm32_rproc_stop() so > that it can be used in synchronisation scenarios. > > Mainly based on the work published by Arnaud Pouliquen [1]. > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> Thanks, Arnaud > --- > drivers/remoteproc/stm32_rproc.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 404f17a97095..86d23c35d805 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -493,6 +493,18 @@ static int stm32_rproc_stop(struct rproc *rproc) > } > } > > + /* update coprocessor state to OFF if available */ > + if (ddata->m4_state.map) { > + err = regmap_update_bits(ddata->m4_state.map, > + ddata->m4_state.reg, > + ddata->m4_state.mask, > + M4_STATE_OFF); > + if (err) { > + dev_err(&rproc->dev, "failed to set copro state\n"); > + return err; > + } > + } > + > return 0; > } > > @@ -531,6 +543,7 @@ static struct rproc_ops st_rproc_ops = { > > static __maybe_unused struct rproc_ops st_rproc_sync_ops = { > .start = stm32_rproc_sync_start, > + .stop = stm32_rproc_stop, > }; > > static const struct of_device_id stm32_rproc_match[] = { >
Hi Mathieu, On 4/24/20 10:24 PM, Mathieu Poirier wrote: > This patchset needs to be applied on top of this one [1]. > > It refactors the STM32 platform code in order to introduce support for > synchronising with the M4 remote processor that would have been started by > the boot loader or another entity. > > It carries the same functionatlity as the previeous revision but account > for changes in the remoteproc core to support synchronisation scenarios. > Some RB tags have been removed when the content of the patch has strayed > too far from the original version. See patch 3, 8, 9 and 12 for more > details. I reviewed the series, and made some tests on my side. FYI, I do not answer to patches when tagged "Reviewed-by: Loic Pallardy" and with no extra remark. So consider them as Reviewed-by me but not necessary to add the tag in commit, Reviewed-by: loic in commit is sufficient. Concerning tests, it works find except the crash recovery from a sync start. But i suppose that you know the limitation, waiting Loic patches[1] update :) [1]: https://lkml.org/lkml/2020/3/11/403 Thanks a lot for your work! Arnaud > > Tested on ST's mp157c board. > > Thanks, > Mathieu > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=277049 > [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > Mathieu Poirier (12): > remoteproc: stm32: Decouple rproc from memory translation > remoteproc: stm32: Request IRQ with platform device > remoteproc: stm32: Decouple rproc from DT parsing > remoteproc: stm32: Remove memory translation from DT parsing > remoteproc: stm32: Parse syscon that will manage M4 synchronisation > remoteproc: stm32: Get coprocessor state > remoteproc: stm32: Get loaded resource table for synchronisation > remoteproc: stm32: Introduce new start ops for synchronisation > remoteproc: stm32: Update M4 state in stm32_rproc_stop() > remoteproc: stm32: Introduce new parse fw ops for synchronisation > remoteproc: stm32: Introduce new loaded rsc ops for synchronisation > remoteproc: stm32: Set synchronisation state machine if needed > > drivers/remoteproc/stm32_rproc.c | 262 ++++++++++++++++++++++++++++--- > 1 file changed, 244 insertions(+), 18 deletions(-) >
On Wed, Apr 29, 2020 at 03:37:58PM +0200, Arnaud POULIQUEN wrote: > Hi Mathieu, > > On 4/24/20 10:24 PM, Mathieu Poirier wrote: > > Remove the remote processor from the process of parsing the device tree > > since (1) there is no correlation between them and (2) to use the > > information that was gathered to make a decision on whether to > > synchronise with the M4 or not. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > drivers/remoteproc/stm32_rproc.c | 25 ++++++++++++++----------- > > 1 file changed, 14 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > > index 1ac90adba9b1..57a426ea620b 100644 > > --- a/drivers/remoteproc/stm32_rproc.c > > +++ b/drivers/remoteproc/stm32_rproc.c > > @@ -538,12 +538,11 @@ static int stm32_rproc_get_syscon(struct device_node *np, const char *prop, > > return err; > > } > > > > -static int stm32_rproc_parse_dt(struct platform_device *pdev) > > +static int stm32_rproc_parse_dt(struct platform_device *pdev, > > + struct stm32_rproc *ddata, bool *auto_boot) > > { > > struct device *dev = &pdev->dev; > > struct device_node *np = dev->of_node; > > - struct rproc *rproc = platform_get_drvdata(pdev); > > - struct stm32_rproc *ddata = rproc->priv; > > struct stm32_syscon tz; > > unsigned int tzen; > > int err, irq; > > @@ -589,7 +588,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) > > > > err = regmap_read(tz.map, tz.reg, &tzen); > > if (err) { > > - dev_err(&rproc->dev, "failed to read tzen\n"); > > + dev_err(dev, "failed to read tzen\n"); > > return err; > > } > > ddata->secured_soc = tzen & tz.mask; > > @@ -605,7 +604,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) > > if (err) > > dev_info(dev, "failed to get pdds\n"); > > > > - rproc->auto_boot = of_property_read_bool(np, "st,auto-boot"); > > + *auto_boot = of_property_read_bool(np, "st,auto-boot"); > > > > return stm32_rproc_of_memory_translations(pdev, ddata); > > } > > @@ -616,6 +615,7 @@ static int stm32_rproc_probe(struct platform_device *pdev) > > struct stm32_rproc *ddata; > > struct device_node *np = dev->of_node; > > struct rproc *rproc; > > + bool auto_boot = false; > > Nitpicking: Seems that you don't need to initialize it. I think you are correct. > Perhaps you can simply suppress the local variable and directly use rproc->auto_boot. ... and change the value of rproc->auto_boot if state == M4_STATE_CRUN? Sure, that's possible. Thanks for all the comments, it really helps to have a different perspective. I am out of time for today but will continue with the rest of your comments tomorrow. Mathieu > > else LGTM > > > Thanks, > Arnaud > > > int ret; > > > > ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > > @@ -626,9 +626,16 @@ static int stm32_rproc_probe(struct platform_device *pdev) > > if (!rproc) > > return -ENOMEM; > > > > + ddata = rproc->priv; > > + > > rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE); > > + > > + ret = stm32_rproc_parse_dt(pdev, ddata, &auto_boot); > > + if (ret) > > + goto free_rproc; > > + > > + rproc->auto_boot = auto_boot; > > rproc->has_iommu = false; > > - ddata = rproc->priv; > > ddata->workqueue = create_workqueue(dev_name(dev)); > > if (!ddata->workqueue) { > > dev_err(dev, "cannot create workqueue\n"); > > @@ -638,13 +645,9 @@ static int stm32_rproc_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, rproc); > > > > - ret = stm32_rproc_parse_dt(pdev); > > - if (ret) > > - goto free_wkq; > > - > > ret = stm32_rproc_request_mbox(rproc); > > if (ret) > > - goto free_rproc; > > + goto free_wkq; > > > > ret = rproc_add(rproc); > > if (ret) > >
On Wed, Apr 29, 2020 at 03:38:24PM +0200, Arnaud POULIQUEN wrote: > > > On 4/24/20 10:24 PM, Mathieu Poirier wrote: > > Introduce the required mechanic to get the state of the M4 when the > > remoteproc core is initialising. > > > > Mainly based on the work published by Arnaud Pouliquen [1]. > > > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Reviewed-by: Loic Pallardy <loic.pallardy@st.com> > > --- > > drivers/remoteproc/stm32_rproc.c | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > > index a285f338bed8..89fbd2ffac93 100644 > > --- a/drivers/remoteproc/stm32_rproc.c > > +++ b/drivers/remoteproc/stm32_rproc.c > > @@ -38,6 +38,15 @@ > > #define STM32_MBX_VQ1_ID 1 > > #define STM32_MBX_SHUTDOWN "shutdown" > > > > +#define RSC_TBL_SIZE (1024) > > + > > +#define M4_STATE_OFF 0 > > +#define M4_STATE_INI 1 > > +#define M4_STATE_CRUN 2 > > +#define M4_STATE_CSTOP 3 > > +#define M4_STATE_STANDBY 4 > > +#define M4_STATE_CRASH 5 > > + > > struct stm32_syscon { > > struct regmap *map; > > u32 reg; > > @@ -635,12 +644,23 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, > > return 0; > > } > > > > +static int stm32_rproc_get_m4_status(struct stm32_rproc *ddata, > > + unsigned int *state) > > +{ > > + /* See stm32_rproc_parse_dt() */ > > + if (!ddata->m4_state.map) > > + return -EINVAL; > > + > > + return regmap_read(ddata->m4_state.map, ddata->m4_state.reg, state); > > +} > i would manage here the default state depending on the error types > if (!ddata->m4_state.map { > /* > * We couldn't get the coprocessor's state, assume > * it is not running. > */ > state = M4_STATE_OFF; > > return 0; > } > > return regmap_read(ddata->m4_state.map, ddata->m4_state.reg, state); > > > > > + > > static int stm32_rproc_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct stm32_rproc *ddata; > > struct device_node *np = dev->of_node; > > struct rproc *rproc; > > + unsigned int state; > > bool auto_boot = false; > > int ret; > > > > @@ -664,6 +684,15 @@ static int stm32_rproc_probe(struct platform_device *pdev) > > if (ret) > > goto free_rproc; > > > > + ret = stm32_rproc_get_m4_status(ddata, &state); > > + if (ret) { > > + /* > > + * We couldn't get the coprocessor's state, assume > > + * it is not running. > > + */ > > + state = M4_STATE_OFF; > > So here just handle the error; Ok > > Regards > Arnaud > > + } > > + > > rproc->auto_boot = auto_boot; > > rproc->has_iommu = false; > > ddata->workqueue = create_workqueue(dev_name(dev)); > >
On Wed, Apr 29, 2020 at 04:47:19PM +0200, Arnaud POULIQUEN wrote: > > > On 4/24/20 10:25 PM, Mathieu Poirier wrote: > > Set the flags and operations to use if the M4 has been started > > by another entity than the remoteproc core. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > drivers/remoteproc/stm32_rproc.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > > index dcae6103e3df..02dad3f51c7a 100644 > > --- a/drivers/remoteproc/stm32_rproc.c > > +++ b/drivers/remoteproc/stm32_rproc.c > > @@ -598,13 +598,20 @@ static struct rproc_ops st_rproc_ops = { > > .get_boot_addr = rproc_elf_get_boot_addr, > > }; > > > > -static __maybe_unused struct rproc_ops st_rproc_sync_ops = { > > +static struct rproc_ops st_rproc_sync_ops = { > > .start = stm32_rproc_sync_start, > > .stop = stm32_rproc_stop, > > + .kick = stm32_rproc_kick, > > Seems independent of the path. I agree - on the flip side I didn't find a better place to put it. Had I did a one line patch someone would have asked me to stuff it somewhere. I'll have another look to see if I can find something decent. > > > .parse_fw = stm32_rproc_sync_parse_fw, > > .find_loaded_rsc_table = stm32_rproc_sync_elf_find_loaded_rsc_table, > > }; > > > > +static struct rproc_sync_flags st_sync_flags = { > > + .on_init = true, /* sync with MCU when the kernel boots */ > > + .after_stop = false, /* don't resync with MCU if stopped from sysfs */ > > + .after_crash = false, /* don't resync with MCU after a crash */ > > +}; > > + > could be const If I do make this a const I'll have to move the call to rproc_set_state_machine() inside the "if (state == M4_STATE_CRUN)". It also means that people won't be able to make dynamic adjustment to the synchronisation states based on specifics discovered at probe() time. They will need to declare different synchronisation ops for all the potential scenarios. I don't have a strong opinion on any of this. I'll wait a little to see what other people think. If nobody chimes in I'll make this a const in the next revision. > > > static const struct of_device_id stm32_rproc_match[] = { > > { .compatible = "st,stm32mp1-m4" }, > > {}, > > @@ -803,6 +810,7 @@ static int stm32_rproc_probe(struct platform_device *pdev) > > struct stm32_rproc *ddata; > > struct device_node *np = dev->of_node; > > struct rproc *rproc; > > + struct rproc_sync_flags sync_flags = {0}; > > unsigned int state; > > bool auto_boot = false; > > int ret; > > @@ -837,11 +845,17 @@ static int stm32_rproc_probe(struct platform_device *pdev) > > } > > > > if (state == M4_STATE_CRUN) { > > + auto_boot = true; > > + sync_flags = st_sync_flags; > > seems an useless copy > > Regards, > Arnaud > > > ret = stm32_rproc_get_loaded_rsc_table(pdev, ddata); > > if (ret) > > goto free_rproc; > > } > > > > + ret = rproc_set_state_machine(rproc, &st_rproc_sync_ops, sync_flags); > > + if (ret) > > + goto free_rproc; > > + > > rproc->auto_boot = auto_boot; > > rproc->has_iommu = false; > > ddata->workqueue = create_workqueue(dev_name(dev)); > >
On Wed, Apr 29, 2020 at 05:08:32PM +0200, Arnaud POULIQUEN wrote: > Hi Mathieu, > > On 4/24/20 10:24 PM, Mathieu Poirier wrote: > > This patchset needs to be applied on top of this one [1]. > > > > It refactors the STM32 platform code in order to introduce support for > > synchronising with the M4 remote processor that would have been started by > > the boot loader or another entity. > > > > It carries the same functionatlity as the previeous revision but account > > for changes in the remoteproc core to support synchronisation scenarios. > > Some RB tags have been removed when the content of the patch has strayed > > too far from the original version. See patch 3, 8, 9 and 12 for more > > details. > > I reviewed the series, and made some tests on my side. > FYI, I do not answer to patches when tagged "Reviewed-by: Loic Pallardy" > and with no extra remark. So consider them as Reviewed-by me but not > necessary to add the tag in commit, Reviewed-by: loic in commit is sufficient. Well, if you spent all this time reviewing the code might as well get credit for it... And it also helps maintainers get a feel for how many eyes have looked at the code. > > Concerning tests, it works find except the crash recovery from a sync start. > But i suppose that you know the limitation, waiting Loic patches[1] update :) As I commented in the patch itself, I'll fix this so that the condition leading to the recovery limbo can't happen. Thanks, Mathieu > > [1]: https://lkml.org/lkml/2020/3/11/403 > > Thanks a lot for your work! > Arnaud > > > > > > Tested on ST's mp157c board. > > > > Thanks, > > Mathieu > > > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=277049 > > [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > > > Mathieu Poirier (12): > > remoteproc: stm32: Decouple rproc from memory translation > > remoteproc: stm32: Request IRQ with platform device > > remoteproc: stm32: Decouple rproc from DT parsing > > remoteproc: stm32: Remove memory translation from DT parsing > > remoteproc: stm32: Parse syscon that will manage M4 synchronisation > > remoteproc: stm32: Get coprocessor state > > remoteproc: stm32: Get loaded resource table for synchronisation > > remoteproc: stm32: Introduce new start ops for synchronisation > > remoteproc: stm32: Update M4 state in stm32_rproc_stop() > > remoteproc: stm32: Introduce new parse fw ops for synchronisation > > remoteproc: stm32: Introduce new loaded rsc ops for synchronisation > > remoteproc: stm32: Set synchronisation state machine if needed > > > > drivers/remoteproc/stm32_rproc.c | 262 ++++++++++++++++++++++++++++--- > > 1 file changed, 244 insertions(+), 18 deletions(-) > >
On Fri 24 Apr 13:24 PDT 2020, Mathieu Poirier wrote: > Remove the remote processor from the process of parsing the memory > ranges since there is no correlation between them. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Reviewed-by: Loic Pallardy <loic.pallardy@st.com> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/remoteproc/stm32_rproc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 0f9d02ca4f5a..91fd59af0ffe 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -127,10 +127,10 @@ static int stm32_rproc_mem_release(struct rproc *rproc, > return 0; > } > > -static int stm32_rproc_of_memory_translations(struct rproc *rproc) > +static int stm32_rproc_of_memory_translations(struct platform_device *pdev, > + struct stm32_rproc *ddata) > { > - struct device *parent, *dev = rproc->dev.parent; > - struct stm32_rproc *ddata = rproc->priv; > + struct device *parent, *dev = &pdev->dev; > struct device_node *np; > struct stm32_rproc_mem *p_mems; > struct stm32_rproc_mem_ranges *mem_range; > @@ -606,7 +606,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) > > rproc->auto_boot = of_property_read_bool(np, "st,auto-boot"); > > - return stm32_rproc_of_memory_translations(rproc); > + return stm32_rproc_of_memory_translations(pdev, ddata); > } > > static int stm32_rproc_probe(struct platform_device *pdev) > -- > 2.20.1 >
On Fri 24 Apr 13:24 PDT 2020, Mathieu Poirier wrote: > Request IRQ with platform device rather than remote proc in order to > call stm32_rproc_parse_dt() before rproc_alloc(). That way we can > know whether we need to synchronise with the MCU or not. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Reviewed-by: Loic Pallardy <loic.pallardy@st.com> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/remoteproc/stm32_rproc.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 91fd59af0ffe..1ac90adba9b1 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -261,7 +261,8 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) > > static irqreturn_t stm32_rproc_wdg(int irq, void *data) > { > - struct rproc *rproc = data; > + struct platform_device *pdev = data; > + struct rproc *rproc = platform_get_drvdata(pdev); > > rproc_report_crash(rproc, RPROC_WATCHDOG); > > @@ -553,7 +554,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) > > if (irq > 0) { > err = devm_request_irq(dev, irq, stm32_rproc_wdg, 0, > - dev_name(dev), rproc); > + dev_name(dev), pdev); > if (err) { > dev_err(dev, "failed to request wdg irq\n"); > return err; > -- > 2.20.1 >
On Fri 24 Apr 13:24 PDT 2020, Mathieu Poirier wrote: > Remove the remote processor from the process of parsing the device tree > since (1) there is no correlation between them and (2) to use the > information that was gathered to make a decision on whether to > synchronise with the M4 or not. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/remoteproc/stm32_rproc.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 1ac90adba9b1..57a426ea620b 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -538,12 +538,11 @@ static int stm32_rproc_get_syscon(struct device_node *np, const char *prop, > return err; > } > > -static int stm32_rproc_parse_dt(struct platform_device *pdev) > +static int stm32_rproc_parse_dt(struct platform_device *pdev, > + struct stm32_rproc *ddata, bool *auto_boot) > { > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > - struct rproc *rproc = platform_get_drvdata(pdev); > - struct stm32_rproc *ddata = rproc->priv; > struct stm32_syscon tz; > unsigned int tzen; > int err, irq; > @@ -589,7 +588,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) > > err = regmap_read(tz.map, tz.reg, &tzen); > if (err) { > - dev_err(&rproc->dev, "failed to read tzen\n"); > + dev_err(dev, "failed to read tzen\n"); > return err; > } > ddata->secured_soc = tzen & tz.mask; > @@ -605,7 +604,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev) > if (err) > dev_info(dev, "failed to get pdds\n"); > > - rproc->auto_boot = of_property_read_bool(np, "st,auto-boot"); > + *auto_boot = of_property_read_bool(np, "st,auto-boot"); > > return stm32_rproc_of_memory_translations(pdev, ddata); > } > @@ -616,6 +615,7 @@ static int stm32_rproc_probe(struct platform_device *pdev) > struct stm32_rproc *ddata; > struct device_node *np = dev->of_node; > struct rproc *rproc; > + bool auto_boot = false; > int ret; > > ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > @@ -626,9 +626,16 @@ static int stm32_rproc_probe(struct platform_device *pdev) > if (!rproc) > return -ENOMEM; > > + ddata = rproc->priv; > + > rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE); > + > + ret = stm32_rproc_parse_dt(pdev, ddata, &auto_boot); > + if (ret) > + goto free_rproc; > + > + rproc->auto_boot = auto_boot; > rproc->has_iommu = false; > - ddata = rproc->priv; > ddata->workqueue = create_workqueue(dev_name(dev)); > if (!ddata->workqueue) { > dev_err(dev, "cannot create workqueue\n"); > @@ -638,13 +645,9 @@ static int stm32_rproc_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, rproc); > > - ret = stm32_rproc_parse_dt(pdev); > - if (ret) > - goto free_wkq; > - > ret = stm32_rproc_request_mbox(rproc); > if (ret) > - goto free_rproc; > + goto free_wkq; > > ret = rproc_add(rproc); > if (ret) > -- > 2.20.1 >
On Fri 24 Apr 13:24 PDT 2020, Mathieu Poirier wrote: > Other than one has to be done after the other, there is no correlation > between memory translation and DT parsing. As move function > stm32_rproc_of_memory_translations() to stm32_rproc_probe() so that > stm32_rproc_parse_dt() can be extended to look for synchronisation > related binding in a clean way. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Reviewed-by: Loic Pallardy <loic.pallardy@st.com> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/remoteproc/stm32_rproc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 57a426ea620b..658439d4b00a 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -606,7 +606,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, > > *auto_boot = of_property_read_bool(np, "st,auto-boot"); > > - return stm32_rproc_of_memory_translations(pdev, ddata); > + return 0; > } > > static int stm32_rproc_probe(struct platform_device *pdev) > @@ -634,6 +634,10 @@ static int stm32_rproc_probe(struct platform_device *pdev) > if (ret) > goto free_rproc; > > + ret = stm32_rproc_of_memory_translations(pdev, ddata); > + if (ret) > + goto free_rproc; > + > rproc->auto_boot = auto_boot; > rproc->has_iommu = false; > ddata->workqueue = create_workqueue(dev_name(dev)); > -- > 2.20.1 >
On Fri 24 Apr 13:24 PDT 2020, Mathieu Poirier wrote: > Get from the DT the syncon to probe the state of the remote processor > and the location of the resource table. > > Mainly based on the work published by Arnaud Pouliquen [1]. > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Reviewed-by: Loic Pallardy <loic.pallardy@st.com> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/remoteproc/stm32_rproc.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 658439d4b00a..a285f338bed8 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -70,6 +70,8 @@ struct stm32_rproc { > struct reset_control *rst; > struct stm32_syscon hold_boot; > struct stm32_syscon pdds; > + struct stm32_syscon m4_state; > + struct stm32_syscon rsctbl; > int wdg_irq; > u32 nb_rmems; > struct stm32_rproc_mem *rmems; > @@ -606,6 +608,30 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, > > *auto_boot = of_property_read_bool(np, "st,auto-boot"); > > + /* > + * See if we can check the M4 status, i.e if it was started > + * from the boot loader or not. > + */ > + err = stm32_rproc_get_syscon(np, "st,syscfg-m4-state", > + &ddata->m4_state); > + if (err) { > + /* remember this */ > + ddata->m4_state.map = NULL; > + /* no coprocessor state syscon (optional) */ > + dev_warn(dev, "m4 state not supported\n"); > + > + /* no need to go further */ > + return 0; > + } > + > + /* See if we can get the resource table */ > + err = stm32_rproc_get_syscon(np, "st,syscfg-rsc-tbl", > + &ddata->rsctbl); > + if (err) { > + /* no rsc table syscon (optional) */ > + dev_warn(dev, "rsc tbl syscon not supported\n"); > + } > + > return 0; > } > > -- > 2.20.1 >
On Fri 24 Apr 13:25 PDT 2020, Mathieu Poirier wrote: > Introduce new parse firmware rproc_ops functions to be used when > synchonising with an MCU. > > Mainly based on the work published by Arnaud Pouliquen [1]. > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Reviewed-by: Loic Pallardy <loic.pallardy@st.com> > --- > drivers/remoteproc/stm32_rproc.c | 51 +++++++++++++++++++++++++++++++- > 1 file changed, 50 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 86d23c35d805..b8ae8aed5585 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -215,7 +215,34 @@ static int stm32_rproc_elf_load_rsc_table(struct rproc *rproc, > return 0; > } > > -static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) > +static int stm32_rproc_sync_elf_load_rsc_table(struct rproc *rproc, > + const struct firmware *fw) > +{ > + struct resource_table *table = NULL; > + struct stm32_rproc *ddata = rproc->priv; > + > + if (ddata->rsc_va) { Does it really make sense to try to sync with a remote that doesn't have a resource table? > + table = (struct resource_table *)ddata->rsc_va; > + /* Assuming that the resource table fits in 1kB is fair */ > + rproc->cached_table = kmemdup(table, RSC_TBL_SIZE, GFP_KERNEL); It's unfortunate that we need to create a clone of the resource table that we found in ram, and then return the original memory when the core ask for the loaded table... I wonder if we somehow can avoid this in the core (i.e. skip overwriting table_ptr with the cached_table during stop) > + if (!rproc->cached_table) > + return -ENOMEM; > + > + rproc->table_ptr = rproc->cached_table; > + rproc->table_sz = RSC_TBL_SIZE; > + return 0; > + } > + > + rproc->cached_table = NULL; > + rproc->table_ptr = NULL; > + rproc->table_sz = 0; > + > + dev_warn(&rproc->dev, "no resource table found for this firmware\n"); > + return 0; > +} > + > +static int stm32_rproc_parse_memory_regions(struct rproc *rproc, > + const struct firmware *fw) > { > struct device *dev = rproc->dev.parent; > struct device_node *np = dev->of_node; > @@ -268,9 +295,30 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) > index++; > } > > + return 0; > +} > + > +static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) > +{ > + int ret = stm32_rproc_parse_memory_regions(rproc, fw); > + > + if (ret) > + return ret; > + > return stm32_rproc_elf_load_rsc_table(rproc, fw); > } > > +static int stm32_rproc_sync_parse_fw(struct rproc *rproc, > + const struct firmware *fw) Rather than having a function parse_fw that is passed no fw and return some state that was setup in probe, how about just do this during probe? Regards, Bjorn > +{ > + int ret = stm32_rproc_parse_memory_regions(rproc, fw); > + > + if (ret) > + return ret; > + > + return stm32_rproc_sync_elf_load_rsc_table(rproc, fw); > +} > + > static irqreturn_t stm32_rproc_wdg(int irq, void *data) > { > struct platform_device *pdev = data; > @@ -544,6 +592,7 @@ static struct rproc_ops st_rproc_ops = { > static __maybe_unused struct rproc_ops st_rproc_sync_ops = { > .start = stm32_rproc_sync_start, > .stop = stm32_rproc_stop, > + .parse_fw = stm32_rproc_sync_parse_fw, > }; > > static const struct of_device_id stm32_rproc_match[] = { > -- > 2.20.1 >
On Fri 24 Apr 13:25 PDT 2020, Mathieu Poirier wrote: > Introduce new elf find loaded resource table rproc_ops functions to be > used when synchonising with an MCU. > > Mainly based on the work published by Arnaud Pouliquen [1]. > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=239877 > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Reviewed-by: Loic Pallardy <loic.pallardy@st.com> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> But I would have preferred if we during probe (when we discover rsc_va) could just set it on the rproc. Regards, Bjorn > --- > drivers/remoteproc/stm32_rproc.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index b8ae8aed5585..dcae6103e3df 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -319,6 +319,15 @@ static int stm32_rproc_sync_parse_fw(struct rproc *rproc, > return stm32_rproc_sync_elf_load_rsc_table(rproc, fw); > } > > +static struct resource_table * > +stm32_rproc_sync_elf_find_loaded_rsc_table(struct rproc *rproc, > + const struct firmware *fw) > +{ > + struct stm32_rproc *ddata = rproc->priv; > + > + return (struct resource_table *)ddata->rsc_va; > +} > + > static irqreturn_t stm32_rproc_wdg(int irq, void *data) > { > struct platform_device *pdev = data; > @@ -593,6 +602,7 @@ static __maybe_unused struct rproc_ops st_rproc_sync_ops = { > .start = stm32_rproc_sync_start, > .stop = stm32_rproc_stop, > .parse_fw = stm32_rproc_sync_parse_fw, > + .find_loaded_rsc_table = stm32_rproc_sync_elf_find_loaded_rsc_table, > }; > > static const struct of_device_id stm32_rproc_match[] = { > -- > 2.20.1 >