diff mbox series

[v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()

Message ID 1675419435-30726-1-git-send-email-quic_mojha@quicinc.com
State New
Headers
Series [v2] firmware: qcom_scm: modify qcom_scm_set_download_mode() |

Commit Message

Mukesh Ojha Feb. 3, 2023, 10:17 a.m. UTC 
  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(-)

Comments

Srinivas Kandagatla Feb. 3, 2023, 2:51 p.m. UTC | #1 
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;
Mukesh Ojha Feb. 3, 2023, 2:53 p.m. UTC | #2 
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;
Dmitry Baryshkov Feb. 3, 2023, 3:34 p.m. UTC | #3 
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.
Bjorn Andersson Feb. 3, 2023, 6:54 p.m. UTC | #4 
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;
Bjorn Andersson Feb. 3, 2023, 7:02 p.m. UTC | #5 
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
>
Mukesh Ojha Feb. 18, 2023, 12:38 p.m. UTC | #6 
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
>>
diff mbox series

Patch

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;