Re: [PATCH v6 4/5] firmware: qcom_scm: Refactor code to support multiple download mode

[Mukesh Ojha]

On 5/27/2023 3:38 AM, andy.shevchenko@xxxxxxxxx wrote:
> Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:
> > Currently on Qualcomm SoC, download_mode is enabled if
> > CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected.
> >
> > Refactor the code such that it supports multiple download
> > modes and drop CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config
> > instead, give interface to set the download mode from
> > module parameter.
>
> ...
>
> >   #include <linux/clk.h>
> >   #include <linux/reset-controller.h>
> >   #include <linux/arm-smccc.h>
>
> > +#include <linux/kstrtox.h>
>
> Can this be located after clk.h which makes (some) order in this block?

Sure.

> ...
>
> >   #define QCOM_DOWNLOAD_MODE_MASK 0x30
> >   #define QCOM_DOWNLOAD_FULLDUMP	0x1
> > +#define QCOM_DOWNLOAD_NODUMP	0x0
>
> Okay, so you start backward ordering.
> But see comments to the next patch.

Will fix this by doing it in ascending order..


> ...
>
> >   		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> > -				QCOM_DOWNLOAD_MODE_MASK,
> > -				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
> > +				QCOM_DOWNLOAD_MODE_MASK, download_mode);
>
> Can ping-pong style be avoided? I.e. do the right thing in the previous patch,
> so you won't change lines that were introduced just before.

If you notice, I have just converted download mode data type from bool
to int in this patch and hence the changing the line here. Last patch 
was about just using the exported API, so i hope you would be fine here.

> ...
>
> >   }
> >   
> > +
>
> Stray change.
>
> > +static int get_download_mode(char *buffer, const struct kernel_param *kp)
> > +{
> > +	int len = 0;
> > +
> > +	if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
> > +		len = sysfs_emit(buffer, "full\n");
> > +	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
> > +		len = sysfs_emit(buffer, "off\n");
> > +
> > +	return len;
>
> You can return directly.

Ok.

>  > Also, what about download_mode that doesn't fit to the above two?

return sysfs_emit(buffer, "unknown\n"); ?

> > +}
>
> ...
>
> > +static int set_download_mode(const char *val, const struct kernel_param *kp)
> > +{
> > +	u32 old = download_mode;
> > +
> > +	if (sysfs_streq(val, "full")) {
> > +		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> > +	} else if (sysfs_streq(val, "off")) {
> > +		download_mode = QCOM_DOWNLOAD_NODUMP;
>
> NIH sysfs_match_string().

NIH ?

My apology, if i did not get this..
Do you want me to use sysfs_match_string()
and how would that help compare to what is present now ?

> > +	} else if (kstrtouint(val, 0, &download_mode) ||
> > +		   !(download_mode == 0 || download_mode == 1)) {
> > +		download_mode = old;
> > +		pr_err("qcom_scm: unknown download mode: %s\n", val);
>
> > +		return -EINVAL;
>
> Do not shadow the error code from kstrtouint() it can be different to this one.

Will fix this.

> > +	}
> > +
> > +	if (__scm)
> > +		qcom_scm_set_download_mode(download_mode);
> > +
> > +	return 0;
> > +}
>
> ...
>
> Have you updated corresponding documentation about this parameter?
> Or there is none?

There is none as of yet outside this file; should that be good what i 
have added in 5/5..

>

-Mukesh