diff mbox series

[2/2] spi: stm32-ospi: Make usage of reset_control_acquire/release() API

Message ID 20250410-b4-upstream_ospi_reset_update-v1-2-74126a8ceb9c@foss.st.com (mailing list archive)
State New
Headers show
Series reset: Add devm_reset_control_array_get_exclusive_released() | expand

Commit Message

Patrice Chotard April 10, 2025, 12:23 p.m. UTC 
As ospi reset is consumed by both OMM and OSPI drivers, use the reset
acquire/release mechanism which ensure exclusive reset usage.

This avoid to call reset_control_get/put() in OMM driver each time
we need to reset OSPI children and guarantee the reset line stays
deasserted.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 drivers/spi/spi-stm32-ospi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Philipp Zabel April 10, 2025, 12:48 p.m. UTC | #1 
On Do, 2025-04-10 at 14:23 +0200, Patrice Chotard wrote:
> As ospi reset is consumed by both OMM and OSPI drivers, use the reset
> acquire/release mechanism which ensure exclusive reset usage.
> 
> This avoid to call reset_control_get/put() in OMM driver each time
> we need to reset OSPI children and guarantee the reset line stays
> deasserted.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> ---
>  drivers/spi/spi-stm32-ospi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-stm32-ospi.c b/drivers/spi/spi-stm32-ospi.c
> index 668022098b1eac3628f0677e6d786e5a267346be..96fa362432f13c19e4dde63d964a0db64c8ade95 100644
> --- a/drivers/spi/spi-stm32-ospi.c
> +++ b/drivers/spi/spi-stm32-ospi.c
> @@ -804,7 +804,7 @@ static int stm32_ospi_get_resources(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ospi->rstc = devm_reset_control_array_get_optional_exclusive(dev);
> +	ospi->rstc = devm_reset_control_array_get_exclusive_released(dev);

Why does this drop _optional?

Also, since _acquire() is right below in the same function, I see no
benefit in requesting the reset control in released state.

>  	if (IS_ERR(ospi->rstc))
>  		return dev_err_probe(dev, PTR_ERR(ospi->rstc),
>  				     "Can't get reset\n");
> @@ -937,9 +937,11 @@ static int stm32_ospi_probe(struct platform_device *pdev)
>  		goto err_pm_enable;
>  
>  	if (ospi->rstc) {

This check only makes sense if the reset control (array) is optional,
otherwise ospi->rstc can never be NULL.

> +		reset_control_acquire(ospi->rstc);

This is missing error handling. Alternatively, you could just use the
normal request function to get an already-acquired reset control.

>  		reset_control_assert(ospi->rstc);
>  		udelay(2);
>  		reset_control_deassert(ospi->rstc);
> +		reset_control_release(ospi->rstc);
>  	}
>  
>  	ret = spi_register_controller(ctrl);

regards
Philipp
Patrice Chotard April 10, 2025, 4:20 p.m. UTC | #2 
On 4/10/25 14:48, Philipp Zabel wrote:
> On Do, 2025-04-10 at 14:23 +0200, Patrice Chotard wrote:
>> As ospi reset is consumed by both OMM and OSPI drivers, use the reset
>> acquire/release mechanism which ensure exclusive reset usage.
>>
>> This avoid to call reset_control_get/put() in OMM driver each time
>> we need to reset OSPI children and guarantee the reset line stays
>> deasserted.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> ---
>>  drivers/spi/spi-stm32-ospi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-stm32-ospi.c b/drivers/spi/spi-stm32-ospi.c
>> index 668022098b1eac3628f0677e6d786e5a267346be..96fa362432f13c19e4dde63d964a0db64c8ade95 100644
>> --- a/drivers/spi/spi-stm32-ospi.c
>> +++ b/drivers/spi/spi-stm32-ospi.c
>> @@ -804,7 +804,7 @@ static int stm32_ospi_get_resources(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>  
>> -	ospi->rstc = devm_reset_control_array_get_optional_exclusive(dev);
>> +	ospi->rstc = devm_reset_control_array_get_exclusive_released(dev);
> 
> Why does this drop _optional?

Hi Philip

I wrongly based this patchset on the reset/next branch instead of the spi/for-next which include this ospi fix [1].
which make resets a required property. I will rebased it on last spi/for-next.

> 
> Also, since _acquire() is right below in the same function, I see no
> benefit in requesting the reset control in released state.

As explained in commit message, OSPI reset are also used by OMM driver which is parent of OSPI.

If i use devm_reset_control_array_get_exclusive() instead of devm_reset_control_array_get_exclusive_released()
here, i got the following kernel warning:

[    8.654378] ------------[ cut here ]------------
[    8.656524] WARNING: CPU: 1 PID: 385 at drivers/reset/core.c:799 __reset_control_get_internal+0x70/0x1d0
[    8.665999] Modules linked in: spi_stm32_ospi(+) hantro_vpu v4l2_vp9 dwmac_stm32(+) stmmac_platform v4l2_h264 v4l2_jpeg v4l2_mem2mem stmmac videobu6
emon.
[    8.691282] CPU: 1 UID: 0 PID: 385 Comm: (udev-worker) Not tainted 6.15.0-rc1-next-20250408-00018-g0f9105d08519 #22 PREEMPT 
[    8.691301] Hardware name: STMicroelectronics STM32MP257F-EV1 Evaluation Board (DT)
[    8.691307] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    8.691317] pc : __reset_control_get_internal+0x70/0x1d0
[    8.691336] lr : __of_reset_control_get+0x1a4/0x270
[    8.691348] sp : ffff80008359b5f0
[    8.691352] x29: ffff80008359b5f0 x28: 0000000000000000 x27: ffff80007b06c100
[    8.691371] x26: ffff80007b06c118 x25: 0000000000000001 x24: 0000000000000000
[    8.691385] x23: 0000000000000004 x22: ffff000082ecc780 x21: 0000000000000005
[    8.691399] x20: ffff000082ecc7a0 x19: ffff000083898d00 x18: 00000000ffffffff
[    8.691414] x17: ffff000082ff9a00 x16: ffff0000802d6800 x15: ffff80008359b4c0
[    8.691429] x14: 0000000000000001 x13: 007473696c5f7974 x12: 0000000000000001
[    8.691444] x11: 0000000000000003 x10: ffff80008257ec4f x9 : 0000000000000028
[    8.691459] x8 : 0101010101010101 x7 : 00000000736c6c65 x6 : 000000000080f2e5
[    8.691473] x5 : ffff80008359b698 x4 : 0000000000000000 x3 : 0000000000000005
[    8.691487] x2 : 0000000000000004 x1 : 0000000000000005 x0 : 0000000000000005
[    8.691501] Call trace:
[    8.691506]  __reset_control_get_internal+0x70/0x1d0 (P)
[    8.691522]  __of_reset_control_get+0x1a4/0x270
[    8.691535]  of_reset_control_array_get+0x9c/0x174
[    8.691549]  devm_reset_control_array_get+0x50/0xb0
[    8.691563]  stm32_ospi_get_resources+0xd4/0x344 [spi_stm32_ospi]
[    8.691584]  stm32_ospi_probe+0xf8/0x3d0 [spi_stm32_ospi]

Which means that bool acquired is set.

This is due to usage of devm_reset_control_array_get_exclusive() which sets flags to RESET_CONTROL_EXCLUSIVE
on an already controlled reset line.


> 
>>  	if (IS_ERR(ospi->rstc))
>>  		return dev_err_probe(dev, PTR_ERR(ospi->rstc),
>>  				     "Can't get reset\n");
>> @@ -937,9 +937,11 @@ static int stm32_ospi_probe(struct platform_device *pdev)
>>  		goto err_pm_enable;
>>  
>>  	if (ospi->rstc) {
> 
> This check only makes sense if the reset control (array) is optional,
> otherwise ospi->rstc can never be NULL.

Right, i will remove this check.

> 
>> +		reset_control_acquire(ospi->rstc);
> 
> This is missing error handling. Alternatively, you could just use the
> normal request function to get an already-acquired reset control.

Ok, i will add a check.


[1] https://patches.linaro.org/project/linux-spi/patch/20250324-upstream_ospi_required_resets-v2-2-85a48afcedec@foss.st.com/

Thanks
Patrice

> 
>>  		reset_control_assert(ospi->rstc);
>>  		udelay(2);
>>  		reset_control_deassert(ospi->rstc);
>> +		reset_control_release(ospi->rstc);
>>  	}
>>  
>>  	ret = spi_register_controller(ctrl);
> 
> regards
> Philipp
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-stm32-ospi.c b/drivers/spi/spi-stm32-ospi.c
index 668022098b1eac3628f0677e6d786e5a267346be..96fa362432f13c19e4dde63d964a0db64c8ade95 100644
--- a/drivers/spi/spi-stm32-ospi.c
+++ b/drivers/spi/spi-stm32-ospi.c
@@ -804,7 +804,7 @@  static int stm32_ospi_get_resources(struct platform_device *pdev)
 		return ret;
 	}
 
-	ospi->rstc = devm_reset_control_array_get_optional_exclusive(dev);
+	ospi->rstc = devm_reset_control_array_get_exclusive_released(dev);
 	if (IS_ERR(ospi->rstc))
 		return dev_err_probe(dev, PTR_ERR(ospi->rstc),
 				     "Can't get reset\n");
@@ -937,9 +937,11 @@  static int stm32_ospi_probe(struct platform_device *pdev)
 		goto err_pm_enable;
 
 	if (ospi->rstc) {
+		reset_control_acquire(ospi->rstc);
 		reset_control_assert(ospi->rstc);
 		udelay(2);
 		reset_control_deassert(ospi->rstc);
+		reset_control_release(ospi->rstc);
 	}
 
 	ret = spi_register_controller(ctrl);