* [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state
@ 2023-08-16 16:45 Robert Marko
2023-08-16 16:45 ` [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required Robert Marko
` (6 more replies)
0 siblings, 7 replies; 17+ messages in thread
From: Robert Marko @ 2023-08-16 16:45 UTC (permalink / raw)
To: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, quic_mojha
Cc: computersforpeace, Robert Marko
IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
means that WDT being asserted or just trying to reboot will hang the board
in the debug mode and only pulling the power and repowering will help.
Some IPQ4019 boards like Google WiFI have it enabled as well.
So, lets add a boolean property to indicate that SDI is enabled by default
and thus needs to be disabled by the kernel.
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
Changes in v3:
* Change the property so it indicates that SDI has been enabled by default
---
Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
index 4233ea839bfc..590bbbd61de5 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
@@ -89,6 +89,14 @@ properties:
protocol to handle sleeping SCM calls.
maxItems: 1
+ qcom,sdi-enabled:
+ description:
+ Indicates that the SDI (Secure Debug Image) has been enabled by TZ
+ by default and it needs to be disabled.
+ If not disabled WDT assertion or reboot will cause the board to hang
+ in the debug mode.
+ type: boolean
+
qcom,dload-mode:
$ref: /schemas/types.yaml#/definitions/phandle-array
items:
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-16 16:45 [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Robert Marko
@ 2023-08-16 16:45 ` Robert Marko
2023-08-16 17:02 ` Guru Das Srinagesh
` (2 more replies)
2023-08-16 16:45 ` [PATCH v3 3/4] dt-bindings: firmware: qcom,scm: document IPQ5018 compatible Robert Marko
` (5 subsequent siblings)
6 siblings, 3 replies; 17+ messages in thread
From: Robert Marko @ 2023-08-16 16:45 UTC (permalink / raw)
To: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, quic_mojha
Cc: computersforpeace, Robert Marko
IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
means that WDT being asserted or just trying to reboot will hang the board
in the debug mode and only pulling the power and repowering will help.
Some IPQ4019 boards like Google WiFI have it enabled as well.
Luckily, SDI can be disabled via an SCM call.
So, lets use the boolean DT property to identify boards that have SDI
enabled by default and use the SCM call to disable SDI during SCM probe.
It is important to disable it as soon as possible as we might have a WDT
assertion at any time which would then leave the board in debug mode,
thus disabling it during SCM removal is not enough.
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
Changes in v3:
* Squashed ("firmware: qcom: scm: Add SDI disable support") and
("firmware: qcom_scm: disable SDI if required")
---
drivers/firmware/qcom_scm.c | 29 +++++++++++++++++++++++++++++
drivers/firmware/qcom_scm.h | 1 +
2 files changed, 30 insertions(+)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 06fe8aca870d..de9d1a11d097 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -403,6 +403,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
}
EXPORT_SYMBOL_GPL(qcom_scm_set_remote_state);
+static int qcom_scm_disable_sdi(void)
+{
+ int ret;
+ struct qcom_scm_desc desc = {
+ .svc = QCOM_SCM_SVC_BOOT,
+ .cmd = QCOM_SCM_BOOT_SDI_CONFIG,
+ .args[0] = 1, /* Disable watchdog debug */
+ .args[1] = 0, /* Disable SDI */
+ .arginfo = QCOM_SCM_ARGS(2),
+ .owner = ARM_SMCCC_OWNER_SIP,
+ };
+ struct qcom_scm_res res;
+
+ ret = qcom_scm_clk_enable();
+ if (ret)
+ return ret;
+ ret = qcom_scm_call(__scm->dev, &desc, &res);
+
+ qcom_scm_clk_disable();
+
+ return ret ? : res.result[0];
+}
+
static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
{
struct qcom_scm_desc desc = {
@@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
if (download_mode)
qcom_scm_set_download_mode(true);
+ /*
+ * Disable SDI if indicated by DT that it is enabled by default.
+ */
+ if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
+ qcom_scm_disable_sdi();
+
return 0;
}
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index e6e512bd57d1..7b68fa820495 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -80,6 +80,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
#define QCOM_SCM_SVC_BOOT 0x01
#define QCOM_SCM_BOOT_SET_ADDR 0x01
#define QCOM_SCM_BOOT_TERMINATE_PC 0x02
+#define QCOM_SCM_BOOT_SDI_CONFIG 0x09
#define QCOM_SCM_BOOT_SET_DLOAD_MODE 0x10
#define QCOM_SCM_BOOT_SET_ADDR_MC 0x11
#define QCOM_SCM_BOOT_SET_REMOTE_STATE 0x0a
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 3/4] dt-bindings: firmware: qcom,scm: document IPQ5018 compatible
2023-08-16 16:45 [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Robert Marko
2023-08-16 16:45 ` [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required Robert Marko
@ 2023-08-16 16:45 ` Robert Marko
2023-08-16 16:45 ` [PATCH v3 4/4] arm64: dts: qcom: ipq5018: indicate that SDI should be disabled Robert Marko
` (4 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Robert Marko @ 2023-08-16 16:45 UTC (permalink / raw)
To: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, quic_mojha
Cc: computersforpeace, Robert Marko, Krzysztof Kozlowski
It seems that IPQ5018 compatible was never documented in the bindings.
Signed-off-by: Robert Marko <robimarko@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
index 590bbbd61de5..60c66cbcfd7b 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
@@ -24,6 +24,7 @@ properties:
- qcom,scm-apq8064
- qcom,scm-apq8084
- qcom,scm-ipq4019
+ - qcom,scm-ipq5018
- qcom,scm-ipq5332
- qcom,scm-ipq6018
- qcom,scm-ipq806x
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] arm64: dts: qcom: ipq5018: indicate that SDI should be disabled
2023-08-16 16:45 [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Robert Marko
2023-08-16 16:45 ` [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required Robert Marko
2023-08-16 16:45 ` [PATCH v3 3/4] dt-bindings: firmware: qcom,scm: document IPQ5018 compatible Robert Marko
@ 2023-08-16 16:45 ` Robert Marko
2023-08-17 3:35 ` [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Brian Norris
` (3 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Robert Marko @ 2023-08-16 16:45 UTC (permalink / raw)
To: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, quic_mojha
Cc: computersforpeace, Robert Marko
Now that SCM has support for indicating that SDI has been enabled by
default, lets set the property so SCM disables it during probing.
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
Changes in v3:
* Use the new boolean property
---
arch/arm64/boot/dts/qcom/ipq5018.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
index 9f13d2dcdfd5..bd0198aaf8d0 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
@@ -57,6 +57,7 @@ L2_0: l2-cache {
firmware {
scm {
compatible = "qcom,scm-ipq5018", "qcom,scm";
+ qcom,sdi-enabled;
};
};
--
2.41.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-16 16:45 ` [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required Robert Marko
@ 2023-08-16 17:02 ` Guru Das Srinagesh
2023-08-16 17:33 ` Konrad Dybcio
2023-08-22 15:37 ` Mukesh Ojha
2 siblings, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2023-08-16 17:02 UTC (permalink / raw)
To: Robert Marko
Cc: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-arm-msm, devicetree,
linux-kernel, quic_mojha, computersforpeace
On Aug 16 2023 18:45, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
>
> Luckily, SDI can be disabled via an SCM call.
>
> So, lets use the boolean DT property to identify boards that have SDI
> enabled by default and use the SCM call to disable SDI during SCM probe.
> It is important to disable it as soon as possible as we might have a WDT
> assertion at any time which would then leave the board in debug mode,
> thus disabling it during SCM removal is not enough.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
Reviewed-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-16 16:45 ` [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required Robert Marko
2023-08-16 17:02 ` Guru Das Srinagesh
@ 2023-08-16 17:33 ` Konrad Dybcio
2023-08-21 10:14 ` Robert Marko
2023-08-22 15:37 ` Mukesh Ojha
2 siblings, 1 reply; 17+ messages in thread
From: Konrad Dybcio @ 2023-08-16 17:33 UTC (permalink / raw)
To: Robert Marko, agross, andersson, robh+dt, krzysztof.kozlowski+dt,
conor+dt, quic_gurus, linux-arm-msm, devicetree, linux-kernel,
quic_mojha
Cc: computersforpeace
On 16.08.2023 18:45, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
>
> Luckily, SDI can be disabled via an SCM call.
>
> So, lets use the boolean DT property to identify boards that have SDI
> enabled by default and use the SCM call to disable SDI during SCM probe.
> It is important to disable it as soon as possible as we might have a WDT
> assertion at any time which would then leave the board in debug mode,
> thus disabling it during SCM removal is not enough.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
[...]
> + /*
> + * Disable SDI if indicated by DT that it is enabled by default.
> + */
> + if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
> + qcom_scm_disable_sdi();
Should we care about the return value?
Konrad
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state
2023-08-16 16:45 [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Robert Marko
` (2 preceding siblings ...)
2023-08-16 16:45 ` [PATCH v3 4/4] arm64: dts: qcom: ipq5018: indicate that SDI should be disabled Robert Marko
@ 2023-08-17 3:35 ` Brian Norris
2023-08-19 14:04 ` Krzysztof Kozlowski
` (2 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Brian Norris @ 2023-08-17 3:35 UTC (permalink / raw)
To: Robert Marko
Cc: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, quic_mojha
On Wed, Aug 16, 2023 at 06:45:38PM +0200, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
>
> So, lets add a boolean property to indicate that SDI is enabled by default
> and thus needs to be disabled by the kernel.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
The series looks good to me. Thanks for doing this!
Reviewed-by: Brian Norris <computersforpeace@gmail.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state
2023-08-16 16:45 [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Robert Marko
` (3 preceding siblings ...)
2023-08-17 3:35 ` [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Brian Norris
@ 2023-08-19 14:04 ` Krzysztof Kozlowski
2023-08-22 16:55 ` Mukesh Ojha
2023-09-20 18:15 ` (subset) " Bjorn Andersson
6 siblings, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2023-08-19 14:04 UTC (permalink / raw)
To: Robert Marko, agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, quic_mojha
Cc: computersforpeace
On 16/08/2023 18:45, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
>
> So, lets add a boolean property to indicate that SDI is enabled by default
> and thus needs to be disabled by the kernel.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-16 17:33 ` Konrad Dybcio
@ 2023-08-21 10:14 ` Robert Marko
0 siblings, 0 replies; 17+ messages in thread
From: Robert Marko @ 2023-08-21 10:14 UTC (permalink / raw)
To: Konrad Dybcio
Cc: agross, andersson, robh+dt, krzysztof.kozlowski+dt, conor+dt,
quic_gurus, linux-arm-msm, devicetree, linux-kernel, quic_mojha,
computersforpeace
On Wed, 16 Aug 2023 at 19:33, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 16.08.2023 18:45, Robert Marko wrote:
> > IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> > means that WDT being asserted or just trying to reboot will hang the board
> > in the debug mode and only pulling the power and repowering will help.
> > Some IPQ4019 boards like Google WiFI have it enabled as well.
> >
> > Luckily, SDI can be disabled via an SCM call.
> >
> > So, lets use the boolean DT property to identify boards that have SDI
> > enabled by default and use the SCM call to disable SDI during SCM probe.
> > It is important to disable it as soon as possible as we might have a WDT
> > assertion at any time which would then leave the board in debug mode,
> > thus disabling it during SCM removal is not enough.
> >
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> [...]
>
>
> > + /*
> > + * Disable SDI if indicated by DT that it is enabled by default.
> > + */
> > + if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
> > + qcom_scm_disable_sdi();
> Should we care about the return value?
I dont really see a point, as we cant really do anything about it if it fails.
Also, the SDI SCM call seems to have weird return codes, for example, it works
but it returns 2.
Maybe somebody from QCA can shine some light on that?
Regards,
Robert
>
> Konrad
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-16 16:45 ` [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required Robert Marko
2023-08-16 17:02 ` Guru Das Srinagesh
2023-08-16 17:33 ` Konrad Dybcio
@ 2023-08-22 15:37 ` Mukesh Ojha
2023-08-25 9:41 ` Robert Marko
2 siblings, 1 reply; 17+ messages in thread
From: Mukesh Ojha @ 2023-08-22 15:37 UTC (permalink / raw)
To: Robert Marko, agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel
Cc: computersforpeace
On 8/16/2023 10:15 PM, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
>
> Luckily, SDI can be disabled via an SCM call.
>
> So, lets use the boolean DT property to identify boards that have SDI
> enabled by default and use the SCM call to disable SDI during SCM probe.
> It is important to disable it as soon as possible as we might have a WDT
> assertion at any time which would then leave the board in debug mode,
> thus disabling it during SCM removal is not enough.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
> Changes in v3:
> * Squashed ("firmware: qcom: scm: Add SDI disable support") and
> ("firmware: qcom_scm: disable SDI if required")
> ---
> drivers/firmware/qcom_scm.c | 29 +++++++++++++++++++++++++++++
> drivers/firmware/qcom_scm.h | 1 +
> 2 files changed, 30 insertions(+)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 06fe8aca870d..de9d1a11d097 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -403,6 +403,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> }
> EXPORT_SYMBOL_GPL(qcom_scm_set_remote_state);
>
> +static int qcom_scm_disable_sdi(void)
> +{
> + int ret;
> + struct qcom_scm_desc desc = {
> + .svc = QCOM_SCM_SVC_BOOT,
> + .cmd = QCOM_SCM_BOOT_SDI_CONFIG,
> + .args[0] = 1, /* Disable watchdog debug */
> + .args[1] = 0, /* Disable SDI */
> + .arginfo = QCOM_SCM_ARGS(2),
> + .owner = ARM_SMCCC_OWNER_SIP,
> + };
> + struct qcom_scm_res res;
> +
> + ret = qcom_scm_clk_enable();
> + if (ret)
> + return ret;
> + ret = qcom_scm_call(__scm->dev, &desc, &res);
Would you not be wanting this call to be atomic ?
> +
> + qcom_scm_clk_disable();
> +
> + return ret ? : res.result[0];
> +}
> +
> static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> {
> struct qcom_scm_desc desc = {
> @@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
> if (download_mode)
> qcom_scm_set_download_mode(true);
>
> + /*
> + * Disable SDI if indicated by DT that it is enabled by default.
> + */
> + if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
> + qcom_scm_disable_sdi();
Why don't we do this call in qcom_scm_shutdown()
also does it not conflict with above download_mode
we have enabled download mode but disabling SDI
means (hard reset) and will not be collecting
crash dump?
-Mukesh
> +
> return 0;
> }
>
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index e6e512bd57d1..7b68fa820495 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -80,6 +80,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> #define QCOM_SCM_SVC_BOOT 0x01
> #define QCOM_SCM_BOOT_SET_ADDR 0x01
> #define QCOM_SCM_BOOT_TERMINATE_PC 0x02
> +#define QCOM_SCM_BOOT_SDI_CONFIG 0x09
> #define QCOM_SCM_BOOT_SET_DLOAD_MODE 0x10
> #define QCOM_SCM_BOOT_SET_ADDR_MC 0x11
> #define QCOM_SCM_BOOT_SET_REMOTE_STATE 0x0a
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state
2023-08-16 16:45 [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Robert Marko
` (4 preceding siblings ...)
2023-08-19 14:04 ` Krzysztof Kozlowski
@ 2023-08-22 16:55 ` Mukesh Ojha
2023-09-20 18:15 ` (subset) " Bjorn Andersson
6 siblings, 0 replies; 17+ messages in thread
From: Mukesh Ojha @ 2023-08-22 16:55 UTC (permalink / raw)
To: Robert Marko, agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel
Cc: computersforpeace
On 8/16/2023 10:15 PM, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
>
> So, lets add a boolean property to indicate that SDI is enabled by default
> and thus needs to be disabled by the kernel.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
Acked-by: Mukesh Ojha <quic_mojha@quicinc.com>
-Mukesh
> ---
> Changes in v3:
> * Change the property so it indicates that SDI has been enabled by default
> ---
> Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> index 4233ea839bfc..590bbbd61de5 100644
> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> @@ -89,6 +89,14 @@ properties:
> protocol to handle sleeping SCM calls.
> maxItems: 1
>
> + qcom,sdi-enabled:
> + description:
> + Indicates that the SDI (Secure Debug Image) has been enabled by TZ
> + by default and it needs to be disabled.
> + If not disabled WDT assertion or reboot will cause the board to hang
> + in the debug mode.
> + type: boolean
> +
> qcom,dload-mode:
> $ref: /schemas/types.yaml#/definitions/phandle-array
> items:
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-22 15:37 ` Mukesh Ojha
@ 2023-08-25 9:41 ` Robert Marko
2023-08-25 21:42 ` Brian Norris
2023-08-31 14:43 ` Mukesh Ojha
0 siblings, 2 replies; 17+ messages in thread
From: Robert Marko @ 2023-08-25 9:41 UTC (permalink / raw)
To: Mukesh Ojha
Cc: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, computersforpeace
On Tue, 22 Aug 2023 at 17:38, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>
>
>
> On 8/16/2023 10:15 PM, Robert Marko wrote:
> > IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> > means that WDT being asserted or just trying to reboot will hang the board
> > in the debug mode and only pulling the power and repowering will help.
> > Some IPQ4019 boards like Google WiFI have it enabled as well.
> >
> > Luckily, SDI can be disabled via an SCM call.
> >
> > So, lets use the boolean DT property to identify boards that have SDI
> > enabled by default and use the SCM call to disable SDI during SCM probe.
> > It is important to disable it as soon as possible as we might have a WDT
> > assertion at any time which would then leave the board in debug mode,
> > thus disabling it during SCM removal is not enough.
> >
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> > Changes in v3:
> > * Squashed ("firmware: qcom: scm: Add SDI disable support") and
> > ("firmware: qcom_scm: disable SDI if required")
> > ---
> > drivers/firmware/qcom_scm.c | 29 +++++++++++++++++++++++++++++
> > drivers/firmware/qcom_scm.h | 1 +
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 06fe8aca870d..de9d1a11d097 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -403,6 +403,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> > }
> > EXPORT_SYMBOL_GPL(qcom_scm_set_remote_state);
> >
> > +static int qcom_scm_disable_sdi(void)
> > +{
> > + int ret;
> > + struct qcom_scm_desc desc = {
> > + .svc = QCOM_SCM_SVC_BOOT,
> > + .cmd = QCOM_SCM_BOOT_SDI_CONFIG,
> > + .args[0] = 1, /* Disable watchdog debug */
> > + .args[1] = 0, /* Disable SDI */
> > + .arginfo = QCOM_SCM_ARGS(2),
> > + .owner = ARM_SMCCC_OWNER_SIP,
> > + };
> > + struct qcom_scm_res res;
> > +
> > + ret = qcom_scm_clk_enable();
> > + if (ret)
> > + return ret;
> > + ret = qcom_scm_call(__scm->dev, &desc, &res);
>
> Would you not be wanting this call to be atomic ?
This is implemented based off the downstream 5.4 kernel as I dont have
the SCM docs
so I dont know if its even supported in the atomic version.
>
> > +
> > + qcom_scm_clk_disable();
> > +
> > + return ret ? : res.result[0];
> > +}
> > +
> > static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> > {
> > struct qcom_scm_desc desc = {
> > @@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > if (download_mode)
> > qcom_scm_set_download_mode(true);
> >
> > + /*
> > + * Disable SDI if indicated by DT that it is enabled by default.
> > + */
> > + if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
> > + qcom_scm_disable_sdi();
>
> Why don't we do this call in qcom_scm_shutdown()
> also does it not conflict with above download_mode
> we have enabled download mode but disabling SDI
> means (hard reset) and will not be collecting
> crash dump?
Because doing it in SCM removal is too late, what if we have a WDT
assertion and not a
regular reboot?
It would mean that the board will get stuck in the debug mode which is
not useful for users and
requires the power to be pulled in order to boot normally again.
I am not sure about the download mode, this is where insight from QCA
really help as I am
doing this with very limited docs.
Regards,
Robert
>
> -Mukesh
>
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> > index e6e512bd57d1..7b68fa820495 100644
> > --- a/drivers/firmware/qcom_scm.h
> > +++ b/drivers/firmware/qcom_scm.h
> > @@ -80,6 +80,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> > #define QCOM_SCM_SVC_BOOT 0x01
> > #define QCOM_SCM_BOOT_SET_ADDR 0x01
> > #define QCOM_SCM_BOOT_TERMINATE_PC 0x02
> > +#define QCOM_SCM_BOOT_SDI_CONFIG 0x09
> > #define QCOM_SCM_BOOT_SET_DLOAD_MODE 0x10
> > #define QCOM_SCM_BOOT_SET_ADDR_MC 0x11
> > #define QCOM_SCM_BOOT_SET_REMOTE_STATE 0x0a
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-25 9:41 ` Robert Marko
@ 2023-08-25 21:42 ` Brian Norris
2023-08-31 14:43 ` Mukesh Ojha
1 sibling, 0 replies; 17+ messages in thread
From: Brian Norris @ 2023-08-25 21:42 UTC (permalink / raw)
To: Robert Marko
Cc: Mukesh Ojha, agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel
On Fri, Aug 25, 2023 at 2:41 AM Robert Marko <robimarko@gmail.com> wrote:
> On Tue, 22 Aug 2023 at 17:38, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> > On 8/16/2023 10:15 PM, Robert Marko wrote:
> > > + ret = qcom_scm_call(__scm->dev, &desc, &res);
> >
> > Would you not be wanting this call to be atomic ?
>
> This is implemented based off the downstream 5.4 kernel as I dont have
> the SCM docs
> so I dont know if its even supported in the atomic version.
More than that, it's known *not* to be properly supported on one such
applicable device:
Subject: [RFC] qcom_scm: IPQ4019 firmware does not support atomic API?
https://lore.kernel.org/linux-arm-kernel/20200913201608.GA3162100@bDebian/
I still haven't gotten a solution to *that* problem upstream, but it'd
be nice not to make it worse.
Brian
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-25 9:41 ` Robert Marko
2023-08-25 21:42 ` Brian Norris
@ 2023-08-31 14:43 ` Mukesh Ojha
2023-09-07 6:32 ` Sricharan Ramabadhran
1 sibling, 1 reply; 17+ messages in thread
From: Mukesh Ojha @ 2023-08-31 14:43 UTC (permalink / raw)
To: Robert Marko
Cc: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, computersforpeace
On 8/25/2023 3:11 PM, Robert Marko wrote:
> On Tue, 22 Aug 2023 at 17:38, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>>
>>
>>
>> On 8/16/2023 10:15 PM, Robert Marko wrote:
>>> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
>>> means that WDT being asserted or just trying to reboot will hang the board
>>> in the debug mode and only pulling the power and repowering will help.
>>> Some IPQ4019 boards like Google WiFI have it enabled as well.
>>>
>>> Luckily, SDI can be disabled via an SCM call.
>>>
>>> So, lets use the boolean DT property to identify boards that have SDI
>>> enabled by default and use the SCM call to disable SDI during SCM probe.
>>> It is important to disable it as soon as possible as we might have a WDT
>>> assertion at any time which would then leave the board in debug mode,
>>> thus disabling it during SCM removal is not enough.
>>>
>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>> ---
>>> Changes in v3:
>>> * Squashed ("firmware: qcom: scm: Add SDI disable support") and
>>> ("firmware: qcom_scm: disable SDI if required")
>>> ---
>>> drivers/firmware/qcom_scm.c | 29 +++++++++++++++++++++++++++++
>>> drivers/firmware/qcom_scm.h | 1 +
>>> 2 files changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>> index 06fe8aca870d..de9d1a11d097 100644
>>> --- a/drivers/firmware/qcom_scm.c
>>> +++ b/drivers/firmware/qcom_scm.c
>>> @@ -403,6 +403,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>>> }
>>> EXPORT_SYMBOL_GPL(qcom_scm_set_remote_state);
>>>
>>> +static int qcom_scm_disable_sdi(void)
>>> +{
>>> + int ret;
>>> + struct qcom_scm_desc desc = {
>>> + .svc = QCOM_SCM_SVC_BOOT,
>>> + .cmd = QCOM_SCM_BOOT_SDI_CONFIG,
>>> + .args[0] = 1, /* Disable watchdog debug */
>>> + .args[1] = 0, /* Disable SDI */
>>> + .arginfo = QCOM_SCM_ARGS(2),
>>> + .owner = ARM_SMCCC_OWNER_SIP,
>>> + };
>>> + struct qcom_scm_res res;
>>> +
>>> + ret = qcom_scm_clk_enable();
>>> + if (ret)
>>> + return ret;
>>> + ret = qcom_scm_call(__scm->dev, &desc, &res);
>>
>> Would you not be wanting this call to be atomic ?
>
> This is implemented based off the downstream 5.4 kernel as I dont have
> the SCM docs
> so I dont know if its even supported in the atomic version.
Ok,.
Well, Kernel version does not guarantees us whether certain things
are supported or not in the firmware and it is not bound to any
particular firmware version;
So, whatever firmware version it is running with, we should try to
support.
Should we implement certain kind of call, if fastcall(atomic) is
supported go-ahead otherwise fallback to slowcalls (interruptible)
calls, but this is completely out of the context of this patch.
>>
>>> +
>>> + qcom_scm_clk_disable();
>>> +
>>> + return ret ? : res.result[0];
>>> +}
>>> +
>>> static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>> {
>>> struct qcom_scm_desc desc = {
>>> @@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>> if (download_mode)
>>> qcom_scm_set_download_mode(true);
>>>
>>> + /*
>>> + * Disable SDI if indicated by DT that it is enabled by default.
>>> + */
>>> + if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
>>> + qcom_scm_disable_sdi();
>>
>> Why don't we do this call in qcom_scm_shutdown()
>> also does it not conflict with above download_mode
>> we have enabled download mode but disabling SDI
>> means (hard reset) and will not be collecting
>> crash dump?
>
> Because doing it in SCM removal is too late, what if we have a WDT
> assertion and not a
> regular reboot?
> It would mean that the board will get stuck in the debug mode which is
> not useful for users and
> requires the power to be pulled in order to boot normally again.
Agree.
Just a wild guess..
Can we check if this call __qcom_scm_is_call_available() helps
to determine, if the certain soc has this SCM calls supported
and if it is there it can be disabled.
__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_BOOT,
QCOM_SCM_BOOT_SDI_CONFIG)
>
> I am not sure about the download mode, this is where insight from QCA
> really help as I am
> doing this with very limited docs.
Download mode would not be reflected unless it is debug
board, whatever you write will not be allowed if it is a
secure device.
-Mukesh
>
> Regards,
> Robert
>>
>> -Mukesh
>>
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>>> index e6e512bd57d1..7b68fa820495 100644
>>> --- a/drivers/firmware/qcom_scm.h
>>> +++ b/drivers/firmware/qcom_scm.h
>>> @@ -80,6 +80,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>>> #define QCOM_SCM_SVC_BOOT 0x01
>>> #define QCOM_SCM_BOOT_SET_ADDR 0x01
>>> #define QCOM_SCM_BOOT_TERMINATE_PC 0x02
>>> +#define QCOM_SCM_BOOT_SDI_CONFIG 0x09
>>> #define QCOM_SCM_BOOT_SET_DLOAD_MODE 0x10
>>> #define QCOM_SCM_BOOT_SET_ADDR_MC 0x11
>>> #define QCOM_SCM_BOOT_SET_REMOTE_STATE 0x0a
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-08-31 14:43 ` Mukesh Ojha
@ 2023-09-07 6:32 ` Sricharan Ramabadhran
2023-09-11 13:22 ` Mukesh Ojha
0 siblings, 1 reply; 17+ messages in thread
From: Sricharan Ramabadhran @ 2023-09-07 6:32 UTC (permalink / raw)
To: Mukesh Ojha, Robert Marko
Cc: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, computersforpeace
<snip ..>
>>>> + int ret;
>>>> + struct qcom_scm_desc desc = {
>>>> + .svc = QCOM_SCM_SVC_BOOT,
>>>> + .cmd = QCOM_SCM_BOOT_SDI_CONFIG,
>>>> + .args[0] = 1, /* Disable watchdog debug */
>>>> + .args[1] = 0, /* Disable SDI */
>>>> + .arginfo = QCOM_SCM_ARGS(2),
>>>> + .owner = ARM_SMCCC_OWNER_SIP,
>>>> + };
>>>> + struct qcom_scm_res res;
>>>> +
>>>> + ret = qcom_scm_clk_enable();
>>>> + if (ret)
>>>> + return ret;
>>>> + ret = qcom_scm_call(__scm->dev, &desc, &res);
>>>
>>> Would you not be wanting this call to be atomic ?
>>
>> This is implemented based off the downstream 5.4 kernel as I dont have
>> the SCM docs
>> so I dont know if its even supported in the atomic version.
>
> Ok,.
>
> Well, Kernel version does not guarantees us whether certain things
> are supported or not in the firmware and it is not bound to any
> particular firmware version;
>
> So, whatever firmware version it is running with, we should try to
> support.
>
> Should we implement certain kind of call, if fastcall(atomic) is
> supported go-ahead otherwise fallback to slowcalls (interruptible)
> calls, but this is completely out of the context of this patch.
>
I replied on older thread, was not in CC here, just saw this.
Agree, atomic api is out of this context and we could take it up
separately.
>>>
>>>> +
>>>> + qcom_scm_clk_disable();
>>>> +
>>>> + return ret ? : res.result[0];
>>>> +}
>>>> +
>>>> static int __qcom_scm_set_dload_mode(struct device *dev, bool
>>>> enable)
>>>> {
>>>> struct qcom_scm_desc desc = {
>>>> @@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct
>>>> platform_device *pdev)
>>>> if (download_mode)
>>>> qcom_scm_set_download_mode(true);
>>>>
>>>> + /*
>>>> + * Disable SDI if indicated by DT that it is enabled by default.
>>>> + */
>>>> + if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
>>>> + qcom_scm_disable_sdi();
>>>
>>> Why don't we do this call in qcom_scm_shutdown()
>>> also does it not conflict with above download_mode
>>> we have enabled download mode but disabling SDI
>>> means (hard reset) and will not be collecting
>>> crash dump?
>>
>> Because doing it in SCM removal is too late, what if we have a WDT
>> assertion and not a
>> regular reboot?
>> It would mean that the board will get stuck in the debug mode which is
>> not useful for users and
>> requires the power to be pulled in order to boot normally again.
>
> Agree.
For IPQ chipsets, SDI bit is used like below,
For abnormal resets (like WDT), should be set '1' for valid dump
collection.
For reboot, should be cleared to '0' to avoid dump collection which
is not required in this case.
For HLOS panic, is a don't care, dumps always get collected and
firmware takes care of clearing the SDI bit.
Mukesh, Can you confirm if its same for msm also ?
>
> Just a wild guess..
>
> Can we check if this call __qcom_scm_is_call_available() helps
> to determine, if the certain soc has this SCM calls supported
> and if it is there it can be disabled.
>
> __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_BOOT,
> QCOM_SCM_BOOT_SDI_CONFIG)
>
Yes, as i mentioned in other thread, checking using
qcom_scm_is_call_available is better. That said, would require
testing on all IPQ/MSM socs to confirm if firmware supports it.
>>
>> I am not sure about the download mode, this is where insight from QCA
>> really help as I am
>> doing this with very limited docs.
>
> Download mode would not be reflected unless it is debug
> board, whatever you write will not be allowed if it is a
> secure device.
>
Yes, 'download mode' bit is similar, but that is used by the firmware
to determining whether to collect dumps on non-secure boards.
Specifically, 'SDI bit' on some socs is used by firmware to determine
if boot is happening from a 'abnormal crash', hence put DDR to
self-refresh etc for valid dumps.
Regards,
Sricharan
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required
2023-09-07 6:32 ` Sricharan Ramabadhran
@ 2023-09-11 13:22 ` Mukesh Ojha
0 siblings, 0 replies; 17+ messages in thread
From: Mukesh Ojha @ 2023-09-11 13:22 UTC (permalink / raw)
To: Sricharan Ramabadhran, Robert Marko
Cc: agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, quic_gurus, linux-arm-msm,
devicetree, linux-kernel, computersforpeace
On 9/7/2023 12:02 PM, Sricharan Ramabadhran wrote:
>
> <snip ..>
>
>>>>> + int ret;
>>>>> + struct qcom_scm_desc desc = {
>>>>> + .svc = QCOM_SCM_SVC_BOOT,
>>>>> + .cmd = QCOM_SCM_BOOT_SDI_CONFIG,
>>>>> + .args[0] = 1, /* Disable watchdog debug */
>>>>> + .args[1] = 0, /* Disable SDI */
>>>>> + .arginfo = QCOM_SCM_ARGS(2),
>>>>> + .owner = ARM_SMCCC_OWNER_SIP,
>>>>> + };
>>>>> + struct qcom_scm_res res;
>>>>> +
>>>>> + ret = qcom_scm_clk_enable();
>>>>> + if (ret)
>>>>> + return ret;
>>>>> + ret = qcom_scm_call(__scm->dev, &desc, &res);
>>>>
>>>> Would you not be wanting this call to be atomic ?
>>>
>>> This is implemented based off the downstream 5.4 kernel as I dont have
>>> the SCM docs
>>> so I dont know if its even supported in the atomic version.
>>
>> Ok,.
>>
>> Well, Kernel version does not guarantees us whether certain things
>> are supported or not in the firmware and it is not bound to any
>> particular firmware version;
>>
>> So, whatever firmware version it is running with, we should try to
>> support.
>>
>> Should we implement certain kind of call, if fastcall(atomic) is
>> supported go-ahead otherwise fallback to slowcalls (interruptible)
>> calls, but this is completely out of the context of this patch.
>>
>
> I replied on older thread, was not in CC here, just saw this.
>
> Agree, atomic api is out of this context and we could take it up
> separately.
>
>>>>
>>>>> +
>>>>> + qcom_scm_clk_disable();
>>>>> +
>>>>> + return ret ? : res.result[0];
>>>>> +}
>>>>> +
>>>>> static int __qcom_scm_set_dload_mode(struct device *dev, bool
>>>>> enable)
>>>>> {
>>>>> struct qcom_scm_desc desc = {
>>>>> @@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct
>>>>> platform_device *pdev)
>>>>> if (download_mode)
>>>>> qcom_scm_set_download_mode(true);
>>>>>
>>>>> + /*
>>>>> + * Disable SDI if indicated by DT that it is enabled by default.
>>>>> + */
>>>>> + if (of_property_read_bool(pdev->dev.of_node,
>>>>> "qcom,sdi-enabled"))
>>>>> + qcom_scm_disable_sdi();
>>>>
>>>> Why don't we do this call in qcom_scm_shutdown()
>>>> also does it not conflict with above download_mode
>>>> we have enabled download mode but disabling SDI
>>>> means (hard reset) and will not be collecting
>>>> crash dump?
>>>
>>> Because doing it in SCM removal is too late, what if we have a WDT
>>> assertion and not a
>>> regular reboot?
>>> It would mean that the board will get stuck in the debug mode which is
>>> not useful for users and
>>> requires the power to be pulled in order to boot normally again.
>>
>> Agree.
>
> For IPQ chipsets, SDI bit is used like below,
>
> For abnormal resets (like WDT), should be set '1' for valid dump
> collection.
>
> For reboot, should be cleared to '0' to avoid dump collection which
> is not required in this case.
>
> For HLOS panic, is a don't care, dumps always get collected and
> firmware takes care of clearing the SDI bit.
>
> Mukesh, Can you confirm if its same for msm also ?
Yes, it is same in MSM as well.
-Mukesh
>>
>> Just a wild guess..
>>
>> Can we check if this call __qcom_scm_is_call_available() helps
>> to determine, if the certain soc has this SCM calls supported
>> and if it is there it can be disabled.
>>
>> __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_BOOT,
>> QCOM_SCM_BOOT_SDI_CONFIG)
>>
>
> Yes, as i mentioned in other thread, checking using
> qcom_scm_is_call_available is better. That said, would require
> testing on all IPQ/MSM socs to confirm if firmware supports it.
>
>>>
>>> I am not sure about the download mode, this is where insight from QCA
>>> really help as I am
>>> doing this with very limited docs.
>>
>> Download mode would not be reflected unless it is debug
>> board, whatever you write will not be allowed if it is a
>> secure device.
>>
>
> Yes, 'download mode' bit is similar, but that is used by the firmware
> to determining whether to collect dumps on non-secure boards.
> Specifically, 'SDI bit' on some socs is used by firmware to determine
> if boot is happening from a 'abnormal crash', hence put DDR to
> self-refresh etc for valid dumps.
>
> Regards,
> Sricharan
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: (subset) [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state
2023-08-16 16:45 [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Robert Marko
` (5 preceding siblings ...)
2023-08-22 16:55 ` Mukesh Ojha
@ 2023-09-20 18:15 ` Bjorn Andersson
6 siblings, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2023-09-20 18:15 UTC (permalink / raw)
To: agross, konrad.dybcio, robh+dt, krzysztof.kozlowski+dt, conor+dt,
quic_gurus, linux-arm-msm, devicetree, linux-kernel, quic_mojha,
Robert Marko
Cc: computersforpeace
On Wed, 16 Aug 2023 18:45:38 +0200, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
>
> So, lets add a boolean property to indicate that SDI is enabled by default
> and thus needs to be disabled by the kernel.
>
> [...]
Applied, thanks!
[4/4] arm64: dts: qcom: ipq5018: indicate that SDI should be disabled
commit: 79796e87215db9587d6c66ec6f6781e091bc6464
Best regards,
--
Bjorn Andersson <andersson@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-09-20 18:11 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-16 16:45 [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Robert Marko
2023-08-16 16:45 ` [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required Robert Marko
2023-08-16 17:02 ` Guru Das Srinagesh
2023-08-16 17:33 ` Konrad Dybcio
2023-08-21 10:14 ` Robert Marko
2023-08-22 15:37 ` Mukesh Ojha
2023-08-25 9:41 ` Robert Marko
2023-08-25 21:42 ` Brian Norris
2023-08-31 14:43 ` Mukesh Ojha
2023-09-07 6:32 ` Sricharan Ramabadhran
2023-09-11 13:22 ` Mukesh Ojha
2023-08-16 16:45 ` [PATCH v3 3/4] dt-bindings: firmware: qcom,scm: document IPQ5018 compatible Robert Marko
2023-08-16 16:45 ` [PATCH v3 4/4] arm64: dts: qcom: ipq5018: indicate that SDI should be disabled Robert Marko
2023-08-17 3:35 ` [PATCH v3 1/4] dt-bindings: firmware: qcom,scm: support indicating SDI default state Brian Norris
2023-08-19 14:04 ` Krzysztof Kozlowski
2023-08-22 16:55 ` Mukesh Ojha
2023-09-20 18:15 ` (subset) " Bjorn Andersson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.