| Message ID | 20230331154651.3107173-1-arnaud.pouliquen@foss.st.com |
|---|---|
| Headers | show |
| Series | stm32mp15: update remoteproc to support SCMI Device tree | expand |
> 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
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 >
On 3/31/23 17:46, Arnaud Pouliquen wrote: > Since the introduction of the SCMI server for the management > of the MCU hold boot in OPTEE, management of the hold boot by smc call > is deprecated. > Clean the st,syscfg-tz which allows to determine if the trust > zone is enable. Please don't waste time to review the commit message above! The subject and the commit message is not aligned with the commit update I need to rework it in a V2. I'm waiting few days before sending the V2, allowing people to comment V1. For V2 I will probably copy/past the commit message of: [PATCH 1/5] dt-bindings: remoteproc: st,stm32-rproc: Rework reset declarations Regards, Arnaud > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > arch/arm/boot/dts/stm32mp151.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/stm32mp151.dtsi b/arch/arm/boot/dts/stm32mp151.dtsi > index 4e437d3f2ed6..25626797db94 100644 > --- a/arch/arm/boot/dts/stm32mp151.dtsi > +++ b/arch/arm/boot/dts/stm32mp151.dtsi > @@ -1820,8 +1820,8 @@ m4_rproc: m4@10000000 { > <0x30000000 0x40000>, > <0x38000000 0x10000>; > resets = <&rcc MCU_R>; > + reset-names = "mcu_rst"; > st,syscfg-holdboot = <&rcc 0x10C 0x1>; > - st,syscfg-tz = <&rcc 0x000 0x1>; > st,syscfg-pdds = <&pwr_mcu 0x0 0x1>; > st,syscfg-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>; > st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
Hi Arnaud, On Fri, Mar 31, 2023 at 05:46:49PM +0200, Arnaud Pouliquen wrote: > There are two ways to manage the Cortex-M4 hold boot: > - by Linux thanks to a sys config controller > - by the secure context when the hold boot is protected. > Since the introduction of the SCMI server, the use of the SMC call What SCMI server? Does this means stm32 is now able to use SCMI to manage the remote processor hold boot? If so, that is what I should find in this changelog. Otherwise this changelog needs to be re-written. > is deprecated. If the trust zone is activated, the management of the > hold boot must be done by the secure context thanks to a SCMI reset > controller. > > This patch cleans-up the code related to the SMC call, replaced by > the SCMI server. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > drivers/remoteproc/stm32_rproc.c | 34 ++------------------------------ > 1 file changed, 2 insertions(+), 32 deletions(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 7d782ed9e589..4be651e734ee 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -5,7 +5,6 @@ > * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics. > */ > > -#include <linux/arm-smccc.h> > #include <linux/dma-mapping.h> > #include <linux/interrupt.h> > #include <linux/io.h> > @@ -88,7 +87,6 @@ struct stm32_rproc { > struct stm32_rproc_mem *rmems; > struct stm32_mbox mb[MBOX_NB_MBX]; > struct workqueue_struct *workqueue; > - bool secured_soc; > void __iomem *rsc_va; > }; > > @@ -398,20 +396,12 @@ static int stm32_rproc_set_hold_boot(struct rproc *rproc, bool hold) > { > struct stm32_rproc *ddata = rproc->priv; > struct stm32_syscon hold_boot = ddata->hold_boot; > - struct arm_smccc_res smc_res; > int val, err; > > val = hold ? HOLD_BOOT : RELEASE_BOOT; > > - if (IS_ENABLED(CONFIG_HAVE_ARM_SMCCC) && ddata->secured_soc) { > - 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 { > - err = regmap_update_bits(hold_boot.map, hold_boot.reg, > - hold_boot.mask, val); > - } > - > + err = regmap_update_bits(hold_boot.map, hold_boot.reg, > + hold_boot.mask, val); > if (err) > dev_err(&rproc->dev, "failed to set hold boot\n"); > > @@ -680,8 +670,6 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, > { > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > - struct stm32_syscon tz; > - unsigned int tzen; > int err, irq; > > irq = platform_get_irq(pdev, 0); > @@ -710,24 +698,6 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev, > 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 > - */ > - err = stm32_rproc_get_syscon(np, "st,syscfg-tz", &tz); > - if (err) { > - dev_err(dev, "failed to get tz syscfg\n"); > - return err; > - } If I was to do a bisect here, I would not be able to boot boards that have a trustzone. Add the new functionality and then remove the old one. > - > - err = regmap_read(tz.map, tz.reg, &tzen); > - if (err) { > - dev_err(dev, "failed to read tzen\n"); > - return err; > - } > - ddata->secured_soc = tzen & tz.mask; > - > err = stm32_rproc_get_syscon(np, "st,syscfg-holdboot", > &ddata->hold_boot); > if (err) { > -- > 2.25.1 >
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 >
> 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 > >
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 >>>
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 >>
This series updates the stm32_rproc driver and associated DT node to support device tree configuration with and without SCMI server. The impact is mainly on the MCU hold boot management. 1) Configuration without SCMI server (legacy): Trusted context not activated - The MCU reset is controlled through the Linux RCC reset driver. - The MCU HOLD BOOT is controlled through The RCC sysconf. 2) Configuration with SCMI server: Trusted context activated - The MCU reset is controlled through the SCMI reset service. - The MCU HOLD BOOT is no more controlled through a SMC call service but through the SCMI reset service. In consequence this series: - Use the SCMI server to manage the MCU hold boot instead of the a SMC call service, - determine the configuration to use depending on the presence of the "reset-names" property if ( "reset-names" property contains "hold_boot") then use reset_control services else use regmap access based on "st,syscfg-holdboot" property. - Update the bindings and DTs in consequence. Arnaud Pouliquen (5): dt-bindings: remoteproc: st,stm32-rproc: Rework reset declarations ARM: dts: stm32: Remove the st,syscfg-tz property remoteproc: stm32: Clean-up the management of the hold boot by smc call remoteproc: stm32: Allow hold boot management by the SCMI reset controller ARM: dts: stm32: fix m4_rproc references to use scmi .../bindings/remoteproc/st,stm32-rproc.yaml | 52 ++++++++++----- arch/arm/boot/dts/stm32mp151.dtsi | 2 +- arch/arm/boot/dts/stm32mp157a-dk1-scmi.dts | 6 +- arch/arm/boot/dts/stm32mp157c-dk2-scmi.dts | 6 +- arch/arm/boot/dts/stm32mp157c-ed1-scmi.dts | 6 +- arch/arm/boot/dts/stm32mp157c-ev1-scmi.dts | 6 +- drivers/remoteproc/stm32_rproc.c | 64 ++++++++----------- 7 files changed, 82 insertions(+), 60 deletions(-)