[v1] stm32mp15: update remoteproc to support SCMI Device tree

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

Posted by Arnaud Pouliquen 2 years, 10 months ago

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.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/stm32_rproc.c | 34 ++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 4be651e734ee..6b0d8f30a5c7 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -78,6 +78,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;
@@ -398,6 +399,14 @@ static int stm32_rproc_set_hold_boot(struct rproc *rproc, bool hold)
 	struct stm32_syscon hold_boot = ddata->hold_boot;
 	int val, err;
 
+	if (ddata->hold_boot_rst) {
+		/* Use the SCMI reset controller */
+		if (!hold)
+			return reset_control_deassert(ddata->hold_boot_rst);
+		else
+			return reset_control_assert(ddata->hold_boot_rst);
+	}
+
 	val = hold ? HOLD_BOOT : RELEASE_BOOT;
 
 	err = regmap_update_bits(hold_boot.map, hold_boot.reg,
@@ -693,16 +702,29 @@ 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(dev, "mcu_rst");
 	if (IS_ERR(ddata->rst))
 		return dev_err_probe(dev, PTR_ERR(ddata->rst),
 				     "failed to get mcu_reset\n");
 
-	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;
+	ddata->hold_boot_rst = devm_reset_control_get(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);
+		ddata->hold_boot_rst = NULL;
+	}
+
+	if (!ddata->hold_boot_rst) {
+		/*
+		 * If the hold boot is not managed by the SCMI reset controller,
+		 * 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

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

Posted by Mathieu Poirier 2 years, 10 months ago

On Fri, Mar 31, 2023 at 05:46:50PM +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.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/stm32_rproc.c | 34 ++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 4be651e734ee..6b0d8f30a5c7 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -78,6 +78,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;
> @@ -398,6 +399,14 @@ static int stm32_rproc_set_hold_boot(struct rproc *rproc, bool hold)
>  	struct stm32_syscon hold_boot = ddata->hold_boot;
>  	int val, err;
>  
> +	if (ddata->hold_boot_rst) {
> +		/* Use the SCMI reset controller */
> +		if (!hold)
> +			return reset_control_deassert(ddata->hold_boot_rst);
> +		else
> +			return reset_control_assert(ddata->hold_boot_rst);
> +	}
> +
>  	val = hold ? HOLD_BOOT : RELEASE_BOOT;
>  
>  	err = regmap_update_bits(hold_boot.map, hold_boot.reg,
> @@ -693,16 +702,29 @@ 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(dev, "mcu_rst");

Peng is correct - newer kernels won't be able to boot with older DT.

>  	if (IS_ERR(ddata->rst))
>  		return dev_err_probe(dev, PTR_ERR(ddata->rst),
>  				     "failed to get mcu_reset\n");
>  
> -	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;
> +	ddata->hold_boot_rst = devm_reset_control_get(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);
> +		ddata->hold_boot_rst = NULL;
> +	}
> +
> +	if (!ddata->hold_boot_rst) {

Why another if() statement?  The code below should be in the above if()...

This patchset is surprizingly confusing for its size.  I suggest paying
attention to the changelogs and adding comments in the code.

Thanks,
Mathieu

> +		/*
> +		 * If the hold boot is not managed by the SCMI reset controller,
> +		 * 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
>

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

Posted by Arnaud POULIQUEN 2 years, 10 months ago


On 4/5/23 20:01, Mathieu Poirier wrote:
> On Fri, Mar 31, 2023 at 05:46:50PM +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.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/remoteproc/stm32_rproc.c | 34 ++++++++++++++++++++++++++------
>>  1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index 4be651e734ee..6b0d8f30a5c7 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -78,6 +78,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;
>> @@ -398,6 +399,14 @@ static int stm32_rproc_set_hold_boot(struct rproc *rproc, bool hold)
>>  	struct stm32_syscon hold_boot = ddata->hold_boot;
>>  	int val, err;
>>  
>> +	if (ddata->hold_boot_rst) {
>> +		/* Use the SCMI reset controller */
>> +		if (!hold)
>> +			return reset_control_deassert(ddata->hold_boot_rst);
>> +		else
>> +			return reset_control_assert(ddata->hold_boot_rst);
>> +	}
>> +
>>  	val = hold ? HOLD_BOOT : RELEASE_BOOT;
>>  
>>  	err = regmap_update_bits(hold_boot.map, hold_boot.reg,
>> @@ -693,16 +702,29 @@ 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(dev, "mcu_rst");
> 
> Peng is correct - newer kernels won't be able to boot with older DT.
> 
>>  	if (IS_ERR(ddata->rst))
>>  		return dev_err_probe(dev, PTR_ERR(ddata->rst),
>>  				     "failed to get mcu_reset\n");
>>  
>> -	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;
>> +	ddata->hold_boot_rst = devm_reset_control_get(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);
>> +		ddata->hold_boot_rst = NULL;
>> +	}
>> +
>> +	if (!ddata->hold_boot_rst) {Okay, I definitely need to rewrite the patchset.
> 
> Why another if() statement?  The code below should be in the above if()...
> 
> This patchset is surprizingly confusing for its size.  I suggest paying
> attention to the changelogs and adding comments in the code.

I definitely need to rewrite this patchset.

Thanks for all reviewers
Regards
Arnaud

> 
> Thanks,
> Mathieu
> 
>> +		/*
>> +		 * If the hold boot is not managed by the SCMI reset controller,
>> +		 * 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
>>

RE: [PATCH 4/5] remoteproc: stm32: Allow hold boot management by the SCMI reset controller

Posted by Peng Fan 2 years, 10 months ago

> Subject: [PATCH 4/5] 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.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/stm32_rproc.c | 34 ++++++++++++++++++++++++++-----
> -
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c
> b/drivers/remoteproc/stm32_rproc.c
> index 4be651e734ee..6b0d8f30a5c7 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -78,6 +78,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;
> @@ -398,6 +399,14 @@ static int stm32_rproc_set_hold_boot(struct rproc
> *rproc, bool hold)
>  	struct stm32_syscon hold_boot = ddata->hold_boot;
>  	int val, err;
> 
> +	if (ddata->hold_boot_rst) {
> +		/* Use the SCMI reset controller */
> +		if (!hold)
> +			return reset_control_deassert(ddata-
> >hold_boot_rst);
> +		else
> +			return reset_control_assert(ddata->hold_boot_rst);
> +	}
> +
>  	val = hold ? HOLD_BOOT : RELEASE_BOOT;
> 
>  	err = regmap_update_bits(hold_boot.map, hold_boot.reg, @@ -
> 693,16 +702,29 @@ 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(dev, "mcu_rst");
[Peng Fan] 
This may break legacy device tree.

Regards,
Peng.

>  	if (IS_ERR(ddata->rst))
>  		return dev_err_probe(dev, PTR_ERR(ddata->rst),
>  				     "failed to get mcu_reset\n");
> 
> -	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;
> +	ddata->hold_boot_rst = devm_reset_control_get(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);
> +		ddata->hold_boot_rst = NULL;
> +	}
> +
> +	if (!ddata->hold_boot_rst) {
> +		/*
> +		 * If the hold boot is not managed by the SCMI reset
> controller,
> +		 * 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

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

Posted by Arnaud POULIQUEN 2 years, 10 months ago


On 4/4/23 06:55, Peng Fan wrote:
>> Subject: [PATCH 4/5] 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.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/remoteproc/stm32_rproc.c | 34 ++++++++++++++++++++++++++-----
>> -
>>  1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c
>> b/drivers/remoteproc/stm32_rproc.c
>> index 4be651e734ee..6b0d8f30a5c7 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -78,6 +78,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;
>> @@ -398,6 +399,14 @@ static int stm32_rproc_set_hold_boot(struct rproc
>> *rproc, bool hold)
>>  	struct stm32_syscon hold_boot = ddata->hold_boot;
>>  	int val, err;
>>
>> +	if (ddata->hold_boot_rst) {
>> +		/* Use the SCMI reset controller */
>> +		if (!hold)
>> +			return reset_control_deassert(ddata-
>>> hold_boot_rst);
>> +		else
>> +			return reset_control_assert(ddata->hold_boot_rst);
>> +	}
>> +
>>  	val = hold ? HOLD_BOOT : RELEASE_BOOT;
>>
>>  	err = regmap_update_bits(hold_boot.map, hold_boot.reg, @@ -
>> 693,16 +702,29 @@ 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(dev, "mcu_rst");
> [Peng Fan] 
> This may break legacy device tree.

That partially true by the fact that I impose the "reset-names" property
(but also suppress the "st,syscfg-tz" property)

But this should not be the case as the arch/arm/boot/dts/stm32mp151.dtsi
is updated in patch 2/5. The DTS files associated to this chip include it.

Thanks,
Arnaud


> 
> Regards,
> Peng.
> 
>>  	if (IS_ERR(ddata->rst))
>>  		return dev_err_probe(dev, PTR_ERR(ddata->rst),
>>  				     "failed to get mcu_reset\n");
>>
>> -	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;
>> +	ddata->hold_boot_rst = devm_reset_control_get(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);
>> +		ddata->hold_boot_rst = NULL;
>> +	}
>> +
>> +	if (!ddata->hold_boot_rst) {
>> +		/*
>> +		 * If the hold boot is not managed by the SCMI reset
>> controller,
>> +		 * 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
>

RE: [PATCH 4/5] remoteproc: stm32: Allow hold boot management by the SCMI reset controller

Posted by Peng Fan 2 years, 10 months ago

> Subject: Re: [PATCH 4/5] remoteproc: stm32: Allow hold boot management
> by the SCMI reset controller
> 
> 
> 
> On 4/4/23 06:55, Peng Fan wrote:
> >> Subject: [PATCH 4/5] 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.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/remoteproc/stm32_rproc.c | 34
> >> ++++++++++++++++++++++++++-----
> >> -
> >>  1 file changed, 28 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/stm32_rproc.c
> >> b/drivers/remoteproc/stm32_rproc.c
> >> index 4be651e734ee..6b0d8f30a5c7 100644
> >> --- a/drivers/remoteproc/stm32_rproc.c
> >> +++ b/drivers/remoteproc/stm32_rproc.c
> >> @@ -78,6 +78,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;
> >> @@ -398,6 +399,14 @@ static int stm32_rproc_set_hold_boot(struct
> >> rproc *rproc, bool hold)
> >>  	struct stm32_syscon hold_boot = ddata->hold_boot;
> >>  	int val, err;
> >>
> >> +	if (ddata->hold_boot_rst) {
> >> +		/* Use the SCMI reset controller */
> >> +		if (!hold)
> >> +			return reset_control_deassert(ddata-
> >>> hold_boot_rst);
> >> +		else
> >> +			return reset_control_assert(ddata->hold_boot_rst);
> >> +	}
> >> +
> >>  	val = hold ? HOLD_BOOT : RELEASE_BOOT;
> >>
> >>  	err = regmap_update_bits(hold_boot.map, hold_boot.reg, @@ -
> >> 693,16 +702,29 @@ 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(dev, "mcu_rst");
> > [Peng Fan]
> > This may break legacy device tree.
> 
> That partially true by the fact that I impose the "reset-names" property (but
> also suppress the "st,syscfg-tz" property)
> 
> But this should not be the case as the arch/arm/boot/dts/stm32mp151.dtsi
> is updated in patch 2/5. The DTS files associated to this chip include it.

DT  maintainers may comment, from my understanding is updating driver
should not break legacy dts, saying 5.15, 5.10 dts.

Regards,
Peng.

> 
> Thanks,
> Arnaud
> 
> 
> >
> > Regards,
> > Peng.
> >
> >>  	if (IS_ERR(ddata->rst))
> >>  		return dev_err_probe(dev, PTR_ERR(ddata->rst),
> >>  				     "failed to get mcu_reset\n");
> >>
> >> -	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;
> >> +	ddata->hold_boot_rst = devm_reset_control_get(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);
> >> +		ddata->hold_boot_rst = NULL;
> >> +	}
> >> +
> >> +	if (!ddata->hold_boot_rst) {
> >> +		/*
> >> +		 * If the hold boot is not managed by the SCMI reset
> >> controller,
> >> +		 * 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
> >

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

Posted by Alexandre TORGUE 2 years, 10 months ago

Hi

On 4/6/23 07:16, Peng Fan wrote:
>> Subject: Re: [PATCH 4/5] remoteproc: stm32: Allow hold boot management
>> by the SCMI reset controller
>>
>>
>>
>> On 4/4/23 06:55, Peng Fan wrote:
>>>> Subject: [PATCH 4/5] 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.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>   drivers/remoteproc/stm32_rproc.c | 34
>>>> ++++++++++++++++++++++++++-----
>>>> -
>>>>   1 file changed, 28 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/stm32_rproc.c
>>>> b/drivers/remoteproc/stm32_rproc.c
>>>> index 4be651e734ee..6b0d8f30a5c7 100644
>>>> --- a/drivers/remoteproc/stm32_rproc.c
>>>> +++ b/drivers/remoteproc/stm32_rproc.c
>>>> @@ -78,6 +78,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;
>>>> @@ -398,6 +399,14 @@ static int stm32_rproc_set_hold_boot(struct
>>>> rproc *rproc, bool hold)
>>>>   	struct stm32_syscon hold_boot = ddata->hold_boot;
>>>>   	int val, err;
>>>>
>>>> +	if (ddata->hold_boot_rst) {
>>>> +		/* Use the SCMI reset controller */
>>>> +		if (!hold)
>>>> +			return reset_control_deassert(ddata-
>>>>> hold_boot_rst);
>>>> +		else
>>>> +			return reset_control_assert(ddata->hold_boot_rst);
>>>> +	}
>>>> +
>>>>   	val = hold ? HOLD_BOOT : RELEASE_BOOT;
>>>>
>>>>   	err = regmap_update_bits(hold_boot.map, hold_boot.reg, @@ -
>>>> 693,16 +702,29 @@ 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(dev, "mcu_rst");
>>> [Peng Fan]
>>> This may break legacy device tree.
>>
>> That partially true by the fact that I impose the "reset-names" property (but
>> also suppress the "st,syscfg-tz" property)
>>
>> But this should not be the case as the arch/arm/boot/dts/stm32mp151.dtsi
>> is updated in patch 2/5. The DTS files associated to this chip include it.
> 
> DT  maintainers may comment, from my understanding is updating driver
> should not break legacy dts, saying 5.15, 5.10 dts.

An old DT should continue to work with a new kernel.

Alex

> 
> Regards,
> Peng.
> 
>>
>> Thanks,
>> Arnaud
>>
>>
>>>
>>> Regards,
>>> Peng.
>>>
>>>>   	if (IS_ERR(ddata->rst))
>>>>   		return dev_err_probe(dev, PTR_ERR(ddata->rst),
>>>>   				     "failed to get mcu_reset\n");
>>>>
>>>> -	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;
>>>> +	ddata->hold_boot_rst = devm_reset_control_get(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);
>>>> +		ddata->hold_boot_rst = NULL;
>>>> +	}
>>>> +
>>>> +	if (!ddata->hold_boot_rst) {
>>>> +		/*
>>>> +		 * If the hold boot is not managed by the SCMI reset
>>>> controller,
>>>> +		 * 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
>>>

[PATCH 1/5] dt-bindings: remoteproc: st,stm32-rproc: Rework reset declarations
[PATCH 2/5] ARM: dts: stm32: Remove the st,syscfg-tz property
[PATCH 3/5] remoteproc: stm32: Clean-up the management of the hold boot by SMC call
[PATCH 4/5] remoteproc: stm32: Allow hold boot management by the SCMI reset controller
[PATCH 5/5] ARM: dts: stm32: fix m4_rproc references to use scmi