[RESEND,PATCH 1/2] firmware: qcom: scm: Add SDI disable support

[RESEND,PATCH 1/2] firmware: qcom: scm: Add SDI disable support

[Robert Marko]

Some SoC-s like IPQ5018 require SDI(Secure Debug Image) to be disabled
before trying to reboot, otherwise board will just hang after reboot has
been issued via PSCI.

So, provide a call to SCM that allows disabling it.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/firmware/qcom_scm.c | 23 +++++++++++++++++++++++
 drivers/firmware/qcom_scm.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd46b7..bdc9324d4e62 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -407,6 +407,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
 }
 EXPORT_SYMBOL(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 = {
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.40.1

[RESEND,PATCH 2/2] firmware: qcom: scm: disable SDI on IPQ5018

[Robert Marko]

IPQ5018 seems to have SDI (Secure Debug Image) enabled by default which
prevents normal reboot from working causing the board to just hang after
reboot is called.

So, let disable SDI during SCM probe for IPQ5018.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/firmware/qcom_scm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index bdc9324d4e62..c6a38ce49fb0 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1525,6 +1525,14 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (download_mode)
 		qcom_scm_set_download_mode(true);
 
+	/* IPQ5018 seems to have SDI (Secure Debug Image) enabled by default
+	 * which will prevent normal reboot causing the board to hang after
+	 * making the reboot call.
+	 * So, make a call to SCM to disable SDI.
+	 */
+	if (of_machine_is_compatible("qcom,ipq5018"))
+		qcom_scm_disable_sdi();
+
 	return 0;
 }
 
-- 
2.40.1

Re: [RESEND,PATCH 2/2] firmware: qcom: scm: disable SDI on IPQ5018

[Brian Norris]

On Thu, May 18, 2023 at 04:02:24PM +0200, Robert Marko wrote:
> IPQ5018 seems to have SDI (Secure Debug Image) enabled by default which
> prevents normal reboot from working causing the board to just hang after
> reboot is called.
> 
> So, let disable SDI during SCM probe for IPQ5018.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  drivers/firmware/qcom_scm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index bdc9324d4e62..c6a38ce49fb0 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -1525,6 +1525,14 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (download_mode)
>  		qcom_scm_set_download_mode(true);
>  
> +	/* IPQ5018 seems to have SDI (Secure Debug Image) enabled by default
> +	 * which will prevent normal reboot causing the board to hang after
> +	 * making the reboot call.
> +	 * So, make a call to SCM to disable SDI.
> +	 */
> +	if (of_machine_is_compatible("qcom,ipq5018"))
> +		qcom_scm_disable_sdi();
> +

I see there has been some reservation expressed on this patch (via patch
1 comments). I suppose this would be a nice time (though months-late) to
add my own potentially-constructive thoughts.

First, this is definitely a real problem, and for multiple products. See
my prior art with the exact same problem:

Subject: [RFC PATCH] firmware: qcom_scm: disable SDI at boot
https://lore.kernel.org/all/20200721080054.2803881-1-computersforpeace@gmail.com/

(I think you found this one already, although independently.)

Secondly, I think some reservation from patch 1 is on the precise method
of identifying such problematic systems, and I think I agree with the
sentiment. For one, I'm sure that in my case, not all IPQ4019-based
systems leave SDI enabled, and similarly, I doubt all IPQ5018 systems do
either. I believe any firmware that has this enabled in a production
system is essentially an oversight and a bug; it provides negative value
to non-Qualcomm-employees (who can't inspect this "debug" mode), and I
also believe it can potentially be "fixed" by firmware updates. So you
have cases where depending on which software updates have been applied
to an original product before being reprogrammed with a properly-open
Linux kernel/distro, the "same" product may or may not behave
differently.

On the other hand, my guess is that it is truly safe (or, redundant) to
make this call on *any* SCM system; if it was already disabled, then
it's a no-op. Now, that may be inconvenient for Qualcomm employees
trying to debug prototype boards, but that's a different problem...

So, it feels like either this should be:
(1) done inconditionally (like my RFC above), or
(2) supported by some kind of dedicated firmware or device tree flag, to
    denote precisely which systems need this behavior, and not just
    guess based on SoCs. We don't have any firmware interface for this
    [1], so I think the next best thing is Device Tree, which I believe
    is sometimes(?) allowed to carry "firmware" information, instead of
    just "hardware" information.

For example, maybe we document a "qcom,firmware-sdi-enabled" boolean, to
represent the fact that the particular board in question may hold
firmware which leaves SDI enabled?

I'd personally also be OK with (1), unless we (or more likely, Qualcomm)
can find some reason that it's not safe/redundant to do this
unconditionally.

Regards,
Brian

[1] As far as I know. There's no documentation about Qualcomm's SCM APIs
    that I'm aware of.

Re: [RESEND,PATCH 2/2] firmware: qcom: scm: disable SDI on IPQ5018

[Sricharan Ramabadhran]

Hi,

On 8/14/2023 4:33 AM, Brian Norris wrote:
> On Thu, May 18, 2023 at 04:02:24PM +0200, Robert Marko wrote:
>> IPQ5018 seems to have SDI (Secure Debug Image) enabled by default which
>> prevents normal reboot from working causing the board to just hang after
>> reboot is called.
>>
>> So, let disable SDI during SCM probe for IPQ5018.
>>
>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>> ---
>>   drivers/firmware/qcom_scm.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index bdc9324d4e62..c6a38ce49fb0 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -1525,6 +1525,14 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>   	if (download_mode)
>>   		qcom_scm_set_download_mode(true);
>>   
>> +	/* IPQ5018 seems to have SDI (Secure Debug Image) enabled by default
>> +	 * which will prevent normal reboot causing the board to hang after
>> +	 * making the reboot call.
>> +	 * So, make a call to SCM to disable SDI.
>> +	 */
>> +	if (of_machine_is_compatible("qcom,ipq5018"))
>> +		qcom_scm_disable_sdi();
>> +
> 
> I see there has been some reservation expressed on this patch (via patch
> 1 comments). I suppose this would be a nice time (though months-late) to
> add my own potentially-constructive thoughts.
> 
> First, this is definitely a real problem, and for multiple products. See
> my prior art with the exact same problem:
> 
> Subject: [RFC PATCH] firmware: qcom_scm: disable SDI at boot
> https://lore.kernel.org/all/20200721080054.2803881-1-computersforpeace@gmail.com/
> 
> (I think you found this one already, although independently.)
> 
> Secondly, I think some reservation from patch 1 is on the precise method
> of identifying such problematic systems, and I think I agree with the
> sentiment. For one, I'm sure that in my case, not all IPQ4019-based
> systems leave SDI enabled, and similarly, I doubt all IPQ5018 systems do
> either. I believe any firmware that has this enabled in a production
> system is essentially an oversight and a bug; it provides negative value
> to non-Qualcomm-employees (who can't inspect this "debug" mode), and I
> also believe it can potentially be "fixed" by firmware updates. So you
> have cases where depending on which software updates have been applied
> to an original product before being reprogrammed with a properly-open
> Linux kernel/distro, the "same" product may or may not behave
> differently.
> 
> On the other hand, my guess is that it is truly safe (or, redundant) to
> make this call on *any* SCM system; if it was already disabled, then
> it's a no-op. Now, that may be inconvenient for Qualcomm employees
> trying to debug prototype boards, but that's a different problem...
> 
> So, it feels like either this should be:
> (1) done inconditionally (like my RFC above), or
> (2) supported by some kind of dedicated firmware or device tree flag, to
>      denote precisely which systems need this behavior, and not just
>      guess based on SoCs. We don't have any firmware interface for this
>      [1], so I think the next best thing is Device Tree, which I believe
>      is sometimes(?) allowed to carry "firmware" information, instead of
>      just "hardware" information.
> 
> For example, maybe we document a "qcom,firmware-sdi-enabled" boolean, to
> represent the fact that the particular board in question may hold
> firmware which leaves SDI enabled?
> 
> I'd personally also be OK with (1), unless we (or more likely, Qualcomm)
> can find some reason that it's not safe/redundant to do this
> unconditionally.
> 

  Sorry for joining this discussion late. This SDI bit is used for
  enabling ramdump collection in case of abnormal reset, like
  watchdog. That's why its enabled by default. That means it has to
  be explicitly disabled in case of intentional resets like, reboot,
  HLOS panic etc. So disabling should be done in panic/reboot paths.
  I guess it should be the same in msm's case also. Mukesh can confirm
  though.

  Also, on how to do it, instead of checking for 'compatible', i think
  would be better to check on availability of SCM using,
  '__qcom_scm_is_call_available'.

Regards,
  Sricharan

Re: [RESEND,PATCH 1/2] firmware: qcom: scm: Add SDI disable support

[Mukesh Ojha]

On 5/18/2023 7:32 PM, Robert Marko wrote:
> Some SoC-s like IPQ5018 require SDI(Secure Debug Image) to be disabled
> before trying to reboot, otherwise board will just hang after reboot has
> been issued via PSCI.
> 
> So, provide a call to SCM that allows disabling it.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>

This scm call support indeed needed for reboot cases, but i am not sure
about target specific check in the later patch.

Acked-by: Mukesh Ojha <quic_mojha@quicinc.com>

-- Mukesh

> ---
>   drivers/firmware/qcom_scm.c | 23 +++++++++++++++++++++++
>   drivers/firmware/qcom_scm.h |  1 +
>   2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index fde33acd46b7..bdc9324d4e62 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -407,6 +407,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>   }
>   EXPORT_SYMBOL(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 = {
> 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

Re: [RESEND,PATCH 1/2] firmware: qcom: scm: Add SDI disable support

[Robert Marko]

On Thu, 18 May 2023 at 16:25, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>
>
>
> On 5/18/2023 7:32 PM, Robert Marko wrote:
> > Some SoC-s like IPQ5018 require SDI(Secure Debug Image) to be disabled
> > before trying to reboot, otherwise board will just hang after reboot has
> > been issued via PSCI.
> >
> > So, provide a call to SCM that allows disabling it.
> >
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
>
> This scm call support indeed needed for reboot cases, but i am not sure
> about target specific check in the later patch.

I am not really sure where to put it, maybe as part of qcom_scm_shutdown?
Yesterday I found out that in OpenWrt we also have a Google IPQ4019 board that
has been needing SDI to be disabled as well.

Regards,
Robert
>
> Acked-by: Mukesh Ojha <quic_mojha@quicinc.com>
>
> -- Mukesh
>
> > ---
> >   drivers/firmware/qcom_scm.c | 23 +++++++++++++++++++++++
> >   drivers/firmware/qcom_scm.h |  1 +
> >   2 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index fde33acd46b7..bdc9324d4e62 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -407,6 +407,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> >   }
> >   EXPORT_SYMBOL(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 = {
> > 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

Re: [RESEND,PATCH 1/2] firmware: qcom: scm: Add SDI disable support

[Brian Norris]

On Fri, May 19, 2023 at 01:16:56PM +0200, Robert Marko wrote:
> On Thu, 18 May 2023 at 16:25, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> > On 5/18/2023 7:32 PM, Robert Marko wrote:
> > > Some SoC-s like IPQ5018 require SDI(Secure Debug Image) to be disabled
> > > before trying to reboot, otherwise board will just hang after reboot has
> > > been issued via PSCI.
> > >
> > > So, provide a call to SCM that allows disabling it.
> > >
> > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> >
> > This scm call support indeed needed for reboot cases, but i am not sure
> > about target specific check in the later patch.
> 
> I am not really sure where to put it, maybe as part of qcom_scm_shutdown?

I would suggest not waiting until shutdown. In fact, I suggest the
opposite -- that this needs to be done as early as possible. Any delay
leaves room for hanging the system -- because a watchdog reset, for
instance, might not be able to trigger an shutdown handler.

And I also imagine that's not the complain Mukesh had; I expect his
reservation was about using the SoC compatible property. But I'd prefer
having that conversation on patch 2.

> Yesterday I found out that in OpenWrt we also have a Google IPQ4019 board that
> has been needing SDI to be disabled as well.

That's me [1] :)

Feel free to CC me on such occasions. I would have loved to have
engaged with this earlier. Anyway, I replied directly to patch 2, since
I think that's a more appropriate place for the discussion.

Brian

[1] Subject: [RFC PATCH] firmware: qcom_scm: disable SDI at boot
    https://lore.kernel.org/all/20200721080054.2803881-1-computersforpeace@gmail.com/