Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5dcf4109.1c69fb81.ef683.dbd7@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <1573593774-12539-11-git-send-email-eberman@codeaurora.org>
References: <1573593774-12539-1-git-send-email-eberman@codeaurora.org> <1573593774-12539-11-git-send-email-eberman@codeaurora.org>
Cc:     Elliot Berman <eberman@codeaurora.org>, tsoni@codeaurora.org,
        sidgup@codeaurora.org, psodagud@codeaurora.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
To:     Elliot Berman <eberman@codeaurora.org>, agross@kernel.org,
        bjorn.andersson@linaro.org, saiprakash.ranjan@codeaurora.org
Subject: Re: [PATCH v2 10/18] firmware: qcom_scm-64: Improve SMC convention detection
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.8.1
Date:   Fri, 15 Nov 2019 16:21:28 -0800
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Elliot Berman (2019-11-12 13:22:46)
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-6=
4.c
> index 977654bb..b82b450 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -302,21 +302,20 @@ int __qcom_scm_hdcp_req(struct device *dev, struct =
qcom_scm_hdcp_req *req,
> =20
>  void __qcom_scm_init(void)
>  {
> -       u64 cmd;
> -       struct arm_smccc_res res;
> -       u32 function =3D SMCCC_FUNCNUM(QCOM_SCM_SVC_INFO, QCOM_SCM_INFO_I=
S_CALL_AVAIL);
> -
> -       /* First try a SMC64 call */
> -       cmd =3D ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64,
> -                                ARM_SMCCC_OWNER_SIP, function);
> -
> -       arm_smccc_smc(cmd, QCOM_SCM_ARGS(1), cmd & (~BIT(ARM_SMCCC_TYPE_S=
HIFT)),
> -                     0, 0, 0, 0, 0, &res);
> -
> -       if (!res.a0 && res.a1)
> -               qcom_smccc_convention =3D ARM_SMCCC_SMC_64;
> -       else
> -               qcom_smccc_convention =3D ARM_SMCCC_SMC_32;
> +       qcom_smccc_convention =3D ARM_SMCCC_SMC_64;
> +       if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO,
> +                       QCOM_SCM_INFO_IS_CALL_AVAIL) =3D=3D 1)

Is this asking if the "is call available function" is available by using
the is call available function? That is recursive. Isn't that why we
make a manually open coded SMC call to see if it works? If this isn't
going to work we may want to just have a property in DT that tells us
what to do.

> +               goto out;
> +
> +       qcom_smccc_convention =3D ARM_SMCCC_SMC_32;
> +       if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO,
> +                       QCOM_SCM_INFO_IS_CALL_AVAIL) =3D=3D 1)
> +               goto out;
> +
> +       qcom_smccc_convention =3D -1;
> +       BUG();

This BUG() is new and not mentioned in the commit text. Why can't we
just start failing all scm calls if we can't detect the calling
convention?

> +out:
> +       pr_debug("QCOM SCM SMC Convention: %llu\n", qcom_smccc_convention=
);

Maybe pr_info() is more appropriate. PSCI currently prints out the
version info so maybe printing something like "QCOM SCM SMC_64 calling
convention" will be useful for early debugging.