Hi Arnaud,
On Thu, May 04, 2023 at 11:46:39AM +0200, Arnaud Pouliquen wrote:
> The hold boot can be managed by the SCMI controller as a reset.
> If the "hold_boot" reset is defined in the device tree, use it.
> Else use the syscon controller directly to access to the register.
> The support of the SMC call is deprecated but kept for legacy support.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
> ---
> Updates vs previous version
> - keep support of the "st,syscfg-tz" property for legacy compatibility
> - rename secured_soc in hold_boot_smc for readability
> - add comments to explain hold boot management.
> - update commit message
> ---
> drivers/remoteproc/stm32_rproc.c | 78 +++++++++++++++++++++++---------
> 1 file changed, 57 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 7d782ed9e589..e9cf24274345 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -79,6 +79,7 @@ struct stm32_mbox {
>
> struct stm32_rproc {
> struct reset_control *rst;
> + struct reset_control *hold_boot_rst;
> struct stm32_syscon hold_boot;
> struct stm32_syscon pdds;
> struct stm32_syscon m4_state;
> @@ -88,7 +89,7 @@ struct stm32_rproc {
> struct stm32_rproc_mem *rmems;
> struct stm32_mbox mb[MBOX_NB_MBX];
> struct workqueue_struct *workqueue;
> - bool secured_soc;
> + bool hold_boot_smc;
> void __iomem *rsc_va;
> };
>
> @@ -401,13 +402,28 @@ static int stm32_rproc_set_hold_boot(struct rproc *rproc, bool hold)
> struct arm_smccc_res smc_res;
> int val, err;
>
> + /*
> + * Three ways to manage the hold boot
> + * - using SCMI: the hold boot is managed as a reset,
> + * - using Linux(no SCMI): the hold boot is managed as a syscon register
> + * - using SMC call (deprecated): use SMC reset interface
> + */
> +
> val = hold ? HOLD_BOOT : RELEASE_BOOT;
>
> - if (IS_ENABLED(CONFIG_HAVE_ARM_SMCCC) && ddata->secured_soc) {
> + if (ddata->hold_boot_rst) {
> + /* Use the SCMI reset controller */
> + if (!hold)
> + err = reset_control_deassert(ddata->hold_boot_rst);
> + else
> + err = reset_control_assert(ddata->hold_boot_rst);
> + } else if (IS_ENABLED(CONFIG_HAVE_ARM_SMCCC) && ddata->hold_boot_smc) {
> + /* Use the SMC call */
> arm_smccc_smc(STM32_SMC_RCC, STM32_SMC_REG_WRITE,
> hold_boot.reg, val, 0, 0, 0, 0, &smc_res);
> err = smc_res.a0;
> } else {
> + /* Use syscon */
> err = regmap_update_bits(hold_boot.map, hold_boot.reg,
> hold_boot.mask, val);
> }
> @@ -705,34 +721,54 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev,
> dev_info(dev, "wdg irq registered\n");
> }
>
> - ddata->rst = devm_reset_control_get_by_index(dev, 0);
> + ddata->rst = devm_reset_control_get_optional(dev, "mcu_rst");
> + if (!ddata->rst) {
> + /* Try legacy fallback method: get it by index */
> + ddata->rst = devm_reset_control_get_by_index(dev, 0);
> + }
> if (IS_ERR(ddata->rst))
> return dev_err_probe(dev, PTR_ERR(ddata->rst),
> "failed to get mcu_reset\n");
>
> /*
> - * if platform is secured the hold boot bit must be written by
> - * smc call and read normally.
> - * if not secure the hold boot bit could be read/write normally
> + * Three ways to manage the hold boot
> + * - using SCMI: the hold boot is managed as a reset
> + * The DT "reset-mames" property should be defined with 2 items:
> + * reset-names = "mcu_rst", "hold_boot";
> + * - using SMC call (deprecated): use SMC reset interface
> + * The DT "reset-mames" property is optional, "st,syscfg-tz" is required
> + * - default(no SCMI, no SMC): the hold boot is managed as a syscon register
> + * The DT "reset-mames" property is optional, "st,syscfg-holdboot" is required
> */
> - err = stm32_rproc_get_syscon(np, "st,syscfg-tz", &tz);
> - if (err) {
> - dev_err(dev, "failed to get tz syscfg\n");
> - return err;
> - }
>
> - err = regmap_read(tz.map, tz.reg, &tzen);
> - if (err) {
> - dev_err(dev, "failed to read tzen\n");
> - return err;
> + ddata->hold_boot_rst = devm_reset_control_get_optional(dev, "hold_boot");
> + if (IS_ERR(ddata->hold_boot_rst)) {
> + if (PTR_ERR(ddata->hold_boot_rst) == -EPROBE_DEFER)
> + return PTR_ERR(ddata->hold_boot_rst);
Here we know that devm_reset_control_get_optional() has returned an error that is
not -EPROBE_DEFER and as such, I think we should return that error instead of
continuing on with the probing. Calling dev_err_probe() should be just fine.
Otherwise I'm good with this set. Many thanks for the enhanced explanation.
Mathieu
> + ddata->hold_boot_rst = NULL;
> + }
> +
> + if (!ddata->hold_boot_rst && IS_ENABLED(CONFIG_HAVE_ARM_SMCCC)) {
> + /* Manage the MCU_BOOT using SMC call */
> + err = stm32_rproc_get_syscon(np, "st,syscfg-tz", &tz);
> + if (!err) {
> + err = regmap_read(tz.map, tz.reg, &tzen);
> + if (err) {
> + dev_err(dev, "failed to read tzen\n");
> + return err;
> + }
> + ddata->hold_boot_smc = tzen & tz.mask;
> + }
> }
> - ddata->secured_soc = tzen & tz.mask;
>
> - err = stm32_rproc_get_syscon(np, "st,syscfg-holdboot",
> - &ddata->hold_boot);
> - if (err) {
> - dev_err(dev, "failed to get hold boot\n");
> - return err;
> + if (!ddata->hold_boot_rst && !ddata->hold_boot_smc) {
> + /* Default: hold boot manage it through the syscon controller */
> + err = stm32_rproc_get_syscon(np, "st,syscfg-holdboot",
> + &ddata->hold_boot);
> + if (err) {
> + dev_err(dev, "failed to get hold boot\n");
> + return err;
> + }
> }
>
> err = stm32_rproc_get_syscon(np, "st,syscfg-pdds", &ddata->pdds);
> --
> 2.25.1
>
