* [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()
@ 2023-02-03 10:17 Mukesh Ojha
2023-02-03 14:51 ` Srinivas Kandagatla
2023-02-03 19:02 ` Bjorn Andersson
0 siblings, 2 replies; 7+ messages in thread
From: Mukesh Ojha @ 2023-02-03 10:17 UTC (permalink / raw)
To: agross, andersson, konrad.dybcio; +Cc: linux-arm-msm, linux-kernel, Mukesh Ojha
Modify qcom_scm_set_download_mode() such that it can support
multiple modes. There is no functional change with this change.
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v2:
- Stop changing legacy scm id for dload mode.
drivers/firmware/qcom_scm.c | 15 +++++++--------
include/linux/qcom_scm.h | 5 +++++
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54..6245b97 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
}
EXPORT_SYMBOL(qcom_scm_set_remote_state);
-static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
+static int __qcom_scm_set_dload_mode(struct device *dev, enum qcom_download_mode mode)
{
struct qcom_scm_desc desc = {
.svc = QCOM_SCM_SVC_BOOT,
@@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
.owner = ARM_SMCCC_OWNER_SIP,
};
- desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
+ desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
}
-static void qcom_scm_set_download_mode(bool enable)
+static void qcom_scm_set_download_mode(enum qcom_download_mode mode)
{
bool avail;
int ret = 0;
@@ -424,10 +424,9 @@ static void qcom_scm_set_download_mode(bool enable)
QCOM_SCM_SVC_BOOT,
QCOM_SCM_BOOT_SET_DLOAD_MODE);
if (avail) {
- ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
+ ret = __qcom_scm_set_dload_mode(__scm->dev, mode);
} else if (__scm->dload_mode_addr) {
- ret = qcom_scm_io_writel(__scm->dload_mode_addr,
- enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+ ret = qcom_scm_io_writel(__scm->dload_mode_addr, mode);
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
@@ -1410,7 +1409,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
* disabled below by a clean shutdown/reboot.
*/
if (download_mode)
- qcom_scm_set_download_mode(true);
+ qcom_scm_set_download_mode(QCOM_DOWNLOAD_FULLDUMP);
return 0;
}
@@ -1419,7 +1418,7 @@ static void qcom_scm_shutdown(struct platform_device *pdev)
{
/* Clean shutdown, disable download mode to allow normal restart */
if (download_mode)
- qcom_scm_set_download_mode(false);
+ qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
}
static const struct of_device_id qcom_scm_dt_match[] = {
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index f833564..f9bc84e 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -14,6 +14,11 @@
#define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
#define QCOM_SCM_HDCP_MAX_REQ_CNT 5
+enum qcom_download_mode {
+ QCOM_DOWNLOAD_NODUMP = 0x00,
+ QCOM_DOWNLOAD_FULLDUMP = 0x10,
+};
+
struct qcom_scm_hdcp_req {
u32 addr;
u32 val;
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()
2023-02-03 10:17 [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode() Mukesh Ojha
@ 2023-02-03 14:51 ` Srinivas Kandagatla
2023-02-03 14:53 ` Mukesh Ojha
2023-02-03 19:02 ` Bjorn Andersson
1 sibling, 1 reply; 7+ messages in thread
From: Srinivas Kandagatla @ 2023-02-03 14:51 UTC (permalink / raw)
To: Mukesh Ojha, agross, andersson, konrad.dybcio; +Cc: linux-arm-msm, linux-kernel
On 03/02/2023 10:17, Mukesh Ojha wrote:
> Modify qcom_scm_set_download_mode() such that it can support
> multiple modes. There is no functional change with this change.
>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Changes in v2:
> - Stop changing legacy scm id for dload mode.
>
> drivers/firmware/qcom_scm.c | 15 +++++++--------
> include/linux/qcom_scm.h | 5 +++++
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index cdbfe54..6245b97 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> }
> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>
> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> +static int __qcom_scm_set_dload_mode(struct device *dev, enum qcom_download_mode mode)
> {
> struct qcom_scm_desc desc = {
> .svc = QCOM_SCM_SVC_BOOT,
> @@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> .owner = ARM_SMCCC_OWNER_SIP,
> };
>
> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> + desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
I think this line should be:
desc.args[1] = mode;
--srini
>
> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> }
>
> -static void qcom_scm_set_download_mode(bool enable)
> +static void qcom_scm_set_download_mode(enum qcom_download_mode mode)
> {
> bool avail;
> int ret = 0;
> @@ -424,10 +424,9 @@ static void qcom_scm_set_download_mode(bool enable)
> QCOM_SCM_SVC_BOOT,
> QCOM_SCM_BOOT_SET_DLOAD_MODE);
> if (avail) {
> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> + ret = __qcom_scm_set_dload_mode(__scm->dev, mode);
> } else if (__scm->dload_mode_addr) {
> - ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> - enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> + ret = qcom_scm_io_writel(__scm->dload_mode_addr, mode);
> } else {
> dev_err(__scm->dev,
> "No available mechanism for setting download mode\n");
> @@ -1410,7 +1409,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
> * disabled below by a clean shutdown/reboot.
> */
> if (download_mode)
> - qcom_scm_set_download_mode(true);
> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_FULLDUMP);
>
> return 0;
> }
> @@ -1419,7 +1418,7 @@ static void qcom_scm_shutdown(struct platform_device *pdev)
> {
> /* Clean shutdown, disable download mode to allow normal restart */
> if (download_mode)
> - qcom_scm_set_download_mode(false);
> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
> }
>
> static const struct of_device_id qcom_scm_dt_match[] = {
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index f833564..f9bc84e 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -14,6 +14,11 @@
> #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
> #define QCOM_SCM_HDCP_MAX_REQ_CNT 5
>
> +enum qcom_download_mode {
> + QCOM_DOWNLOAD_NODUMP = 0x00,
> + QCOM_DOWNLOAD_FULLDUMP = 0x10,
> +};
> +
> struct qcom_scm_hdcp_req {
> u32 addr;
> u32 val;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()
2023-02-03 14:51 ` Srinivas Kandagatla
@ 2023-02-03 14:53 ` Mukesh Ojha
2023-02-03 15:34 ` Dmitry Baryshkov
2023-02-03 18:54 ` Bjorn Andersson
0 siblings, 2 replies; 7+ messages in thread
From: Mukesh Ojha @ 2023-02-03 14:53 UTC (permalink / raw)
To: Srinivas Kandagatla, agross, andersson, konrad.dybcio
Cc: linux-arm-msm, linux-kernel
On 2/3/2023 8:21 PM, Srinivas Kandagatla wrote:
>
>
> On 03/02/2023 10:17, Mukesh Ojha wrote:
>> Modify qcom_scm_set_download_mode() such that it can support
>> multiple modes. There is no functional change with this change.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Changes in v2:
>> - Stop changing legacy scm id for dload mode.
>>
>> drivers/firmware/qcom_scm.c | 15 +++++++--------
>> include/linux/qcom_scm.h | 5 +++++
>> 2 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index cdbfe54..6245b97 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>> }
>> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>> +static int __qcom_scm_set_dload_mode(struct device *dev, enum
>> qcom_download_mode mode)
>> {
>> struct qcom_scm_desc desc = {
>> .svc = QCOM_SCM_SVC_BOOT,
>> @@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct
>> device *dev, bool enable)
>> .owner = ARM_SMCCC_OWNER_SIP,
>> };
>> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>> + desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>
> I think this line should be:
>
> desc.args[1] = mode;
>
Should be fine..for backward compatibility as we want to avoid what is
passed to trust zone without check and since this is legacy code, i
would like to avoid.
-Mukesh
>
> --srini
>
>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>> }
>> -static void qcom_scm_set_download_mode(bool enable)
>> +static void qcom_scm_set_download_mode(enum qcom_download_mode mode)
>> {
>> bool avail;
>> int ret = 0;
>> @@ -424,10 +424,9 @@ static void qcom_scm_set_download_mode(bool enable)
>> QCOM_SCM_SVC_BOOT,
>> QCOM_SCM_BOOT_SET_DLOAD_MODE);
>> if (avail) {
>> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>> + ret = __qcom_scm_set_dload_mode(__scm->dev, mode);
>> } else if (__scm->dload_mode_addr) {
>> - ret = qcom_scm_io_writel(__scm->dload_mode_addr,
>> - enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
>> + ret = qcom_scm_io_writel(__scm->dload_mode_addr, mode);
>> } else {
>> dev_err(__scm->dev,
>> "No available mechanism for setting download mode\n");
>> @@ -1410,7 +1409,7 @@ static int qcom_scm_probe(struct platform_device
>> *pdev)
>> * disabled below by a clean shutdown/reboot.
>> */
>> if (download_mode)
>> - qcom_scm_set_download_mode(true);
>> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_FULLDUMP);
>> return 0;
>> }
>> @@ -1419,7 +1418,7 @@ static void qcom_scm_shutdown(struct
>> platform_device *pdev)
>> {
>> /* Clean shutdown, disable download mode to allow normal restart */
>> if (download_mode)
>> - qcom_scm_set_download_mode(false);
>> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
>> }
>> static const struct of_device_id qcom_scm_dt_match[] = {
>> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
>> index f833564..f9bc84e 100644
>> --- a/include/linux/qcom_scm.h
>> +++ b/include/linux/qcom_scm.h
>> @@ -14,6 +14,11 @@
>> #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
>> #define QCOM_SCM_HDCP_MAX_REQ_CNT 5
>> +enum qcom_download_mode {
>> + QCOM_DOWNLOAD_NODUMP = 0x00,
>> + QCOM_DOWNLOAD_FULLDUMP = 0x10,
>> +};
>> +
>> struct qcom_scm_hdcp_req {
>> u32 addr;
>> u32 val;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()
2023-02-03 14:53 ` Mukesh Ojha
@ 2023-02-03 15:34 ` Dmitry Baryshkov
2023-02-03 18:54 ` Bjorn Andersson
1 sibling, 0 replies; 7+ messages in thread
From: Dmitry Baryshkov @ 2023-02-03 15:34 UTC (permalink / raw)
To: Mukesh Ojha
Cc: Srinivas Kandagatla, agross, andersson, konrad.dybcio,
linux-arm-msm, linux-kernel
On Fri, 3 Feb 2023 at 16:53, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>
>
>
> On 2/3/2023 8:21 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 03/02/2023 10:17, Mukesh Ojha wrote:
> >> Modify qcom_scm_set_download_mode() such that it can support
> >> multiple modes. There is no functional change with this change.
> >>
> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> >> ---
> >> Changes in v2:
> >> - Stop changing legacy scm id for dload mode.
> >>
> >> drivers/firmware/qcom_scm.c | 15 +++++++--------
> >> include/linux/qcom_scm.h | 5 +++++
> >> 2 files changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> >> index cdbfe54..6245b97 100644
> >> --- a/drivers/firmware/qcom_scm.c
> >> +++ b/drivers/firmware/qcom_scm.c
> >> @@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> >> }
> >> EXPORT_SYMBOL(qcom_scm_set_remote_state);
> >> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> >> +static int __qcom_scm_set_dload_mode(struct device *dev, enum
> >> qcom_download_mode mode)
> >> {
> >> struct qcom_scm_desc desc = {
> >> .svc = QCOM_SCM_SVC_BOOT,
> >> @@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct
> >> device *dev, bool enable)
> >> .owner = ARM_SMCCC_OWNER_SIP,
> >> };
> >> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> >> + desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> >
> > I think this line should be:
> >
> > desc.args[1] = mode;
> >
>
> Should be fine..for backward compatibility as we want to avoid what is
> passed to trust zone without check and since this is legacy code, i
> would like to avoid.
I'd second Srini here. Please remove the ternary operator and pass
mode directly. If you'd like to limit the 'mode' argument, do so
directly (and return an error if the mode is not supported).
However there still exists a bigger problem in my opinion. You are
changing an API. Please provide a user for this API. 'The user will be
provided separately/later/whatever' is usually not enough.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()
2023-02-03 14:53 ` Mukesh Ojha
2023-02-03 15:34 ` Dmitry Baryshkov
@ 2023-02-03 18:54 ` Bjorn Andersson
1 sibling, 0 replies; 7+ messages in thread
From: Bjorn Andersson @ 2023-02-03 18:54 UTC (permalink / raw)
To: Mukesh Ojha
Cc: Srinivas Kandagatla, agross, konrad.dybcio, linux-arm-msm, linux-kernel
On Fri, Feb 03, 2023 at 08:23:29PM +0530, Mukesh Ojha wrote:
>
>
> On 2/3/2023 8:21 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 03/02/2023 10:17, Mukesh Ojha wrote:
> > > Modify qcom_scm_set_download_mode() such that it can support
> > > multiple modes. There is no functional change with this change.
> > >
> > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > ---
> > > Changes in v2:
> > > - Stop changing legacy scm id for dload mode.
> > >
> > > drivers/firmware/qcom_scm.c | 15 +++++++--------
> > > include/linux/qcom_scm.h | 5 +++++
> > > 2 files changed, 12 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > index cdbfe54..6245b97 100644
> > > --- a/drivers/firmware/qcom_scm.c
> > > +++ b/drivers/firmware/qcom_scm.c
> > > @@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> > > }
> > > EXPORT_SYMBOL(qcom_scm_set_remote_state);
> > > -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> > > +static int __qcom_scm_set_dload_mode(struct device *dev, enum
> > > qcom_download_mode mode)
> > > {
> > > struct qcom_scm_desc desc = {
> > > .svc = QCOM_SCM_SVC_BOOT,
> > > @@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct
> > > device *dev, bool enable)
> > > .owner = ARM_SMCCC_OWNER_SIP,
> > > };
> > > - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> > > + desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> >
> > I think this line should be:
> >
> > desc.args[1] = mode;
> >
>
> Should be fine..for backward compatibility as we want to avoid what is
> passed to trust zone without check and since this is legacy code, i would
> like to avoid.
>
Afaict you touch every caller of this function, so there is no concerns
about backwards compatibility.
With some use of imagination, this patch is likely intended to be
followed by some other patch which adds more values to the enum, at
which point the state you leave this in would be wrong.
> -Mukesh
> >
> > --srini
> >
> > > return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> > > }
> > > -static void qcom_scm_set_download_mode(bool enable)
> > > +static void qcom_scm_set_download_mode(enum qcom_download_mode mode)
> > > {
> > > bool avail;
> > > int ret = 0;
> > > @@ -424,10 +424,9 @@ static void qcom_scm_set_download_mode(bool enable)
> > > QCOM_SCM_SVC_BOOT,
> > > QCOM_SCM_BOOT_SET_DLOAD_MODE);
> > > if (avail) {
> > > - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> > > + ret = __qcom_scm_set_dload_mode(__scm->dev, mode);
> > > } else if (__scm->dload_mode_addr) {
> > > - ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> > > - enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> > > + ret = qcom_scm_io_writel(__scm->dload_mode_addr, mode);
> > > } else {
> > > dev_err(__scm->dev,
> > > "No available mechanism for setting download mode\n");
> > > @@ -1410,7 +1409,7 @@ static int qcom_scm_probe(struct
> > > platform_device *pdev)
> > > * disabled below by a clean shutdown/reboot.
> > > */
> > > if (download_mode)
> > > - qcom_scm_set_download_mode(true);
> > > + qcom_scm_set_download_mode(QCOM_DOWNLOAD_FULLDUMP);
> > > return 0;
> > > }
> > > @@ -1419,7 +1418,7 @@ static void qcom_scm_shutdown(struct
> > > platform_device *pdev)
> > > {
> > > /* Clean shutdown, disable download mode to allow normal restart */
> > > if (download_mode)
> > > - qcom_scm_set_download_mode(false);
> > > + qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
> > > }
> > > static const struct of_device_id qcom_scm_dt_match[] = {
> > > diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> > > index f833564..f9bc84e 100644
> > > --- a/include/linux/qcom_scm.h
> > > +++ b/include/linux/qcom_scm.h
> > > @@ -14,6 +14,11 @@
> > > #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
> > > #define QCOM_SCM_HDCP_MAX_REQ_CNT 5
> > > +enum qcom_download_mode {
This is not an enumeration, it's a set of constants. Please use #define.
> > > + QCOM_DOWNLOAD_NODUMP = 0x00,
> > > + QCOM_DOWNLOAD_FULLDUMP = 0x10,
> > > +};
Regards,
Bjorn
> > > +
> > > struct qcom_scm_hdcp_req {
> > > u32 addr;
> > > u32 val;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()
2023-02-03 10:17 [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode() Mukesh Ojha
2023-02-03 14:51 ` Srinivas Kandagatla
@ 2023-02-03 19:02 ` Bjorn Andersson
2023-02-18 12:38 ` Mukesh Ojha
1 sibling, 1 reply; 7+ messages in thread
From: Bjorn Andersson @ 2023-02-03 19:02 UTC (permalink / raw)
To: Mukesh Ojha; +Cc: agross, konrad.dybcio, linux-arm-msm, linux-kernel
On Fri, Feb 03, 2023 at 03:47:15PM +0530, Mukesh Ojha wrote:
> Modify qcom_scm_set_download_mode() such that it can support
> multiple modes. There is no functional change with this change.
>
As Dmitry said, you argue for added flexibility, but doesn't provide a
user of that flexibility. I will drop this patch from the queue for now.
Please include this together with the patch(es) that benefit from such
flexibility.
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Changes in v2:
> - Stop changing legacy scm id for dload mode.
>
> drivers/firmware/qcom_scm.c | 15 +++++++--------
> include/linux/qcom_scm.h | 5 +++++
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index cdbfe54..6245b97 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> }
> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>
> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> +static int __qcom_scm_set_dload_mode(struct device *dev, enum qcom_download_mode mode)
> {
> struct qcom_scm_desc desc = {
> .svc = QCOM_SCM_SVC_BOOT,
> @@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> .owner = ARM_SMCCC_OWNER_SIP,
> };
>
> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> + desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>
> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> }
>
> -static void qcom_scm_set_download_mode(bool enable)
> +static void qcom_scm_set_download_mode(enum qcom_download_mode mode)
> {
> bool avail;
> int ret = 0;
> @@ -424,10 +424,9 @@ static void qcom_scm_set_download_mode(bool enable)
> QCOM_SCM_SVC_BOOT,
> QCOM_SCM_BOOT_SET_DLOAD_MODE);
> if (avail) {
> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> + ret = __qcom_scm_set_dload_mode(__scm->dev, mode);
> } else if (__scm->dload_mode_addr) {
> - ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> - enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> + ret = qcom_scm_io_writel(__scm->dload_mode_addr, mode);
> } else {
> dev_err(__scm->dev,
> "No available mechanism for setting download mode\n");
> @@ -1410,7 +1409,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
> * disabled below by a clean shutdown/reboot.
> */
> if (download_mode)
> - qcom_scm_set_download_mode(true);
> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_FULLDUMP);
>
> return 0;
> }
> @@ -1419,7 +1418,7 @@ static void qcom_scm_shutdown(struct platform_device *pdev)
> {
> /* Clean shutdown, disable download mode to allow normal restart */
> if (download_mode)
PS. Wouldn't it make sense, if !download_mode to set NODUMP?
Regards,
Bjorn
> - qcom_scm_set_download_mode(false);
> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
> }
>
> static const struct of_device_id qcom_scm_dt_match[] = {
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index f833564..f9bc84e 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -14,6 +14,11 @@
> #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
> #define QCOM_SCM_HDCP_MAX_REQ_CNT 5
>
> +enum qcom_download_mode {
> + QCOM_DOWNLOAD_NODUMP = 0x00,
> + QCOM_DOWNLOAD_FULLDUMP = 0x10,
> +};
> +
> struct qcom_scm_hdcp_req {
> u32 addr;
> u32 val;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()
2023-02-03 19:02 ` Bjorn Andersson
@ 2023-02-18 12:38 ` Mukesh Ojha
0 siblings, 0 replies; 7+ messages in thread
From: Mukesh Ojha @ 2023-02-18 12:38 UTC (permalink / raw)
To: Bjorn Andersson; +Cc: agross, konrad.dybcio, linux-arm-msm, linux-kernel
On 2/4/2023 12:32 AM, Bjorn Andersson wrote:
> On Fri, Feb 03, 2023 at 03:47:15PM +0530, Mukesh Ojha wrote:
>> Modify qcom_scm_set_download_mode() such that it can support
>> multiple modes. There is no functional change with this change.
>>
>
> As Dmitry said, you argue for added flexibility, but doesn't provide a
> user of that flexibility. I will drop this patch from the queue for now.
>
> Please include this together with the patch(es) that benefit from such
> flexibility.
Sure, will add this along with patches which benefit from this change.
>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Changes in v2:
>> - Stop changing legacy scm id for dload mode.
>>
>> drivers/firmware/qcom_scm.c | 15 +++++++--------
>> include/linux/qcom_scm.h | 5 +++++
>> 2 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index cdbfe54..6245b97 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>> }
>> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>>
>> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>> +static int __qcom_scm_set_dload_mode(struct device *dev, enum qcom_download_mode mode)
>> {
>> struct qcom_scm_desc desc = {
>> .svc = QCOM_SCM_SVC_BOOT,
>> @@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>> .owner = ARM_SMCCC_OWNER_SIP,
>> };
>>
>> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>> + desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>>
>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>> }
>>
>> -static void qcom_scm_set_download_mode(bool enable)
>> +static void qcom_scm_set_download_mode(enum qcom_download_mode mode)
>> {
>> bool avail;
>> int ret = 0;
>> @@ -424,10 +424,9 @@ static void qcom_scm_set_download_mode(bool enable)
>> QCOM_SCM_SVC_BOOT,
>> QCOM_SCM_BOOT_SET_DLOAD_MODE);
>> if (avail) {
>> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>> + ret = __qcom_scm_set_dload_mode(__scm->dev, mode);
>> } else if (__scm->dload_mode_addr) {
>> - ret = qcom_scm_io_writel(__scm->dload_mode_addr,
>> - enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
>> + ret = qcom_scm_io_writel(__scm->dload_mode_addr, mode);
>> } else {
>> dev_err(__scm->dev,
>> "No available mechanism for setting download mode\n");
>> @@ -1410,7 +1409,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>> * disabled below by a clean shutdown/reboot.
>> */
>> if (download_mode)
>> - qcom_scm_set_download_mode(true);
>> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_FULLDUMP);
>>
>> return 0;
>> }
>> @@ -1419,7 +1418,7 @@ static void qcom_scm_shutdown(struct platform_device *pdev)
>> {
>> /* Clean shutdown, disable download mode to allow normal restart */
>> if (download_mode)
>
> PS. Wouldn't it make sense, if !download_mode to set NODUMP?
IMO, it does not need even a check, since our intention is to disable
download mode during reboot/restart.
-Mukesh
>
> Regards,
> Bjorn
>
>> - qcom_scm_set_download_mode(false);
>> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
>> }
>>
>> static const struct of_device_id qcom_scm_dt_match[] = {
>> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
>> index f833564..f9bc84e 100644
>> --- a/include/linux/qcom_scm.h
>> +++ b/include/linux/qcom_scm.h
>> @@ -14,6 +14,11 @@
>> #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
>> #define QCOM_SCM_HDCP_MAX_REQ_CNT 5
>>
>> +enum qcom_download_mode {
>> + QCOM_DOWNLOAD_NODUMP = 0x00,
>> + QCOM_DOWNLOAD_FULLDUMP = 0x10,
>> +};
>> +
>> struct qcom_scm_hdcp_req {
>> u32 addr;
>> u32 val;
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-02-18 12:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-03 10:17 [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode() Mukesh Ojha
2023-02-03 14:51 ` Srinivas Kandagatla
2023-02-03 14:53 ` Mukesh Ojha
2023-02-03 15:34 ` Dmitry Baryshkov
2023-02-03 18:54 ` Bjorn Andersson
2023-02-03 19:02 ` Bjorn Andersson
2023-02-18 12:38 ` Mukesh Ojha
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.