Re: [PATCH V5 3/3] firmware: scm: Modify only the download bits in TCSR register

[Kathiravan T]

On 7/25/2023 12:35 AM, Elliot Berman wrote:
> On 7/20/2023 12:04 AM, Kathiravan T wrote:
> > From: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> >
> > CrashDump collection is based on the DLOAD bit of TCSR register. To 
> > retain
> > other bits, we read the register and modify only the DLOAD bit as the
> > other bits have their own significance.
> >
> > Co-developed-by: Poovendhan Selvaraj <quic_poovendh@xxxxxxxxxxx>
> > Signed-off-by: Poovendhan Selvaraj <quic_poovendh@xxxxxxxxxxx>
> > Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> > Signed-off-by: Kathiravan T <quic_kathirav@xxxxxxxxxxx>
> > ---
> > Changes in V5:
> >     - Added the Signed-off-by tag for user Poovendhan
> >     - Dropped the macro QCOM_DOWNLOAD_MODE_SHIFT in the favor of
> >       PREP_FIELD
> >
> >   drivers/firmware/qcom_scm.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 104d86e49b97..3830dcf14326 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -30,6 +30,10 @@ module_param(download_mode, bool, 0);
> >   #define SCM_HAS_IFACE_CLK    BIT(1)
> >   #define SCM_HAS_BUS_CLK        BIT(2)
> >   +#define QCOM_DOWNLOAD_FULLDUMP        0x1
> > +#define QCOM_DOWNLOAD_NODUMP        0x0
> > +#define QCOM_DOWNLOAD_MODE_MASK        BIT(4)
> > +
>
> Can you update __qcom_scm_set_dload_mode to use the FIELD_PREP bits as 
> well? Ideally, you should be able to have no duplicate logic in 
> __qcom_scm_set_dload_mode and in qcom_scm_set_download_mode. Before 
> your patch, it was duplicated and we probably should've had it 
> de-duplicated. With this patch, the logic and constants used have 
> diverged when they don't need to.


Sure, will check this.


> >   struct qcom_scm {
> >       struct device *dev;
> >       struct clk *core_clk;
> > @@ -440,6 +444,7 @@ static int __qcom_scm_set_dload_mode(struct 
> > device *dev, bool enable)
> >   static void qcom_scm_set_download_mode(bool enable)
> >   {
> >       bool avail;
> > +    int val;
> >       int ret = 0;
> >         avail = __qcom_scm_is_call_available(__scm->dev,
> > @@ -448,8 +453,10 @@ static void qcom_scm_set_download_mode(bool enable)
> >       if (avail) {
> >           ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> >       } else if (__scm->dload_mode_addr) {
> > -        ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> > -                enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> > +        val = enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP;
> > +        ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> > +                           QCOM_DOWNLOAD_MODE_MASK,
> > +                           FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
> >       } else {
> >           dev_err(__scm->dev,
> >               "No available mechanism for setting download mode\n");