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

[Arnaud POULIQUEN]

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

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