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(-)
--
2.25.1
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
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 <[email protected]>
---
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 <[email protected]> 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;
- }
-
- 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
With the introduction of the SCMI server it is now possible to use
the SCMI to handle the hold boot instead of a dedicated smc call.
As consequences two configurations are possible:
- use the Linux rcc reset service and use syscon for the MCU hold boot
- use the SCMI reset service for both the MCU reset and the MCU hold boot.
This patch:
- suppresses the use of the property st,syscfg-tz which was used
to check if the trusted Zone was enable to use scm call to manage
the hold boot (the reset controller phandle is used instead to select
the configurations)
- adds properties check on resets definitions.
- adds an example of the SCMI reset service usage.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
.../bindings/remoteproc/st,stm32-rproc.yaml | 52 +++++++++++++------
1 file changed, 37 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
index 66b1e3efdaa3..98d98e9114fc 100644
--- a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
@@ -25,7 +25,14 @@ properties:
maxItems: 3
resets:
- maxItems: 1
+ minItems: 1
+ maxItems: 2
+
+ reset-names:
+ items:
+ - const: mcu_rst
+ - const: hold_boot
+ minItems: 1
st,syscfg-holdboot:
description: remote processor reset hold boot
@@ -36,16 +43,6 @@ properties:
- description: The offset of the hold boot setting register
- description: The field mask of the hold boot
- st,syscfg-tz:
- description:
- Reference to the system configuration which holds the RCC trust zone mode
- $ref: "/schemas/types.yaml#/definitions/phandle-array"
- items:
- - items:
- - description: Phandle of syscon block
- - description: The offset of the trust zone setting register
- - description: The field mask of the trust zone state
-
interrupts:
description: Should contain the WWDG1 watchdog reset interrupt
maxItems: 1
@@ -135,22 +132,47 @@ required:
- compatible
- reg
- resets
- - st,syscfg-holdboot
- - st,syscfg-tz
+ - reset-names
+
+allOf:
+ - if:
+ properties:
+ reset-names:
+ not:
+ contains:
+ const: hold_boot
+ then:
+ required:
+ - st,syscfg-holdboot
+ else:
+ properties:
+ st,syscfg-holdboot: false
additionalProperties: false
examples:
- |
#include <dt-bindings/reset/stm32mp1-resets.h>
- m4_rproc: m4@10000000 {
+ m4_rproc_example1: m4@10000000 {
compatible = "st,stm32mp1-m4";
reg = <0x10000000 0x40000>,
<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-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>;
+ st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
+ };
+ - |
+ #include <dt-bindings/reset/stm32mp1-resets.h>
+ m4_rproc_example2: m4@10000000 {
+ compatible = "st,stm32mp1-m4";
+ reg = <0x10000000 0x40000>,
+ <0x30000000 0x40000>,
+ <0x38000000 0x10000>;
+ resets = <&scmi MCU_R>, <&scmi MCU_HOLD_BOOT_R>;
+ reset-names = "mcu_rst", "hold_boot";
st,syscfg-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>;
st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
};
--
2.25.1
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.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
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>;
--
2.25.1
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 <[email protected]>
---
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
Fixes stm32mp15*-scmi DTS files introduced in [1]:
This patch fixes the node which uses the MCU reset and adds the
missing HOLD_BOOT which is also handled by the SCMI reset service.
This change cannot be applied as a fix on commit [1], the management
of the hold boot impacts also the stm32_rproc driver.
[1] 'commit 5b7e58313a77 ("ARM: dts: stm32: Add SCMI version of STM32 boards (DK1/DK2/ED1/EV1)")'
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
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 ++++--
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/arch/arm/boot/dts/stm32mp157a-dk1-scmi.dts b/arch/arm/boot/dts/stm32mp157a-dk1-scmi.dts
index e539cc80bef8..134788e64265 100644
--- a/arch/arm/boot/dts/stm32mp157a-dk1-scmi.dts
+++ b/arch/arm/boot/dts/stm32mp157a-dk1-scmi.dts
@@ -55,8 +55,10 @@ &mdma1 {
resets = <&scmi_reset RST_SCMI_MDMA>;
};
-&mlahb {
- resets = <&scmi_reset RST_SCMI_MCU>;
+&m4_rproc {
+ resets = <&scmi_reset RST_SCMI_MCU>,
+ <&scmi_reset RST_SCMI_MCU_HOLD_BOOT>;
+ reset-names = "mcu_rst", "hold_boot";
};
&rcc {
diff --git a/arch/arm/boot/dts/stm32mp157c-dk2-scmi.dts b/arch/arm/boot/dts/stm32mp157c-dk2-scmi.dts
index 97e4f94b0a24..c42e658debfb 100644
--- a/arch/arm/boot/dts/stm32mp157c-dk2-scmi.dts
+++ b/arch/arm/boot/dts/stm32mp157c-dk2-scmi.dts
@@ -61,8 +61,10 @@ &mdma1 {
resets = <&scmi_reset RST_SCMI_MDMA>;
};
-&mlahb {
- resets = <&scmi_reset RST_SCMI_MCU>;
+&m4_rproc {
+ resets = <&scmi_reset RST_SCMI_MCU>,
+ <&scmi_reset RST_SCMI_MCU_HOLD_BOOT>;
+ reset-names = "mcu_rst", "hold_boot";
};
&rcc {
diff --git a/arch/arm/boot/dts/stm32mp157c-ed1-scmi.dts b/arch/arm/boot/dts/stm32mp157c-ed1-scmi.dts
index 9cf0a44d2f47..7a56ff2d4185 100644
--- a/arch/arm/boot/dts/stm32mp157c-ed1-scmi.dts
+++ b/arch/arm/boot/dts/stm32mp157c-ed1-scmi.dts
@@ -60,8 +60,10 @@ &mdma1 {
resets = <&scmi_reset RST_SCMI_MDMA>;
};
-&mlahb {
- resets = <&scmi_reset RST_SCMI_MCU>;
+&m4_rproc {
+ resets = <&scmi_reset RST_SCMI_MCU>,
+ <&scmi_reset RST_SCMI_MCU_HOLD_BOOT>;
+ reset-names = "mcu_rst", "hold_boot";
};
&rcc {
diff --git a/arch/arm/boot/dts/stm32mp157c-ev1-scmi.dts b/arch/arm/boot/dts/stm32mp157c-ev1-scmi.dts
index 3b9dd6f4ccc9..119874dd91e4 100644
--- a/arch/arm/boot/dts/stm32mp157c-ev1-scmi.dts
+++ b/arch/arm/boot/dts/stm32mp157c-ev1-scmi.dts
@@ -66,8 +66,10 @@ &mdma1 {
resets = <&scmi_reset RST_SCMI_MDMA>;
};
-&mlahb {
- resets = <&scmi_reset RST_SCMI_MCU>;
+&m4_rproc {
+ resets = <&scmi_reset RST_SCMI_MCU>,
+ <&scmi_reset RST_SCMI_MCU_HOLD_BOOT>;
+ reset-names = "mcu_rst", "hold_boot";
};
&rcc {
--
2.25.1
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>> ---
>> 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
>>