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