[v2,2/4] remoteproc: stm32: Allow hold boot management by the SCMI reset controller

  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@foss.st.com>
---
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(-)
  

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@foss.st.com>
> ---
> 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
>
  

Hi Mathieu,

On 5/5/23 19:03, Mathieu Poirier wrote:
> 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@foss.st.com>
>> ---
>> 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.

Good catch!

Thanks,
Arnaud

> 
> 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
>>