Re: [PATCH V2 5/5] firmware: tegra: use in-band messages for firmware version query

Timo Alho <talho@xxxxxxxxxx> · Mon, 22 Oct 2018 14:36:50 +0300

On 22.10.2018 12.46, Thierry Reding wrote:
On Thu, Oct 18, 2018 at 08:55:44PM +0300, Timo Alho wrote:
Add support for a new MRQ, that uses in-band messaging instead of IOVA
buffer, to retrieve the firmware version 'tag' during boot. If an
older firmware is used, that does not support the new MRQ, fall back
to the earlier implementation.

Signed-off-by: Timo Alho <talho@xxxxxxxxxx>
---
  drivers/firmware/tegra/bpmp.c | 34 ++++++++++++++++++++++++++++++++--
  1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
index a7d8954..5f6634a 100644
--- a/drivers/firmware/tegra/bpmp.c
+++ b/drivers/firmware/tegra/bpmp.c
@@ -549,8 +549,9 @@ static int tegra_bpmp_ping(struct tegra_bpmp *bpmp)
  	return err;
  }
  
-static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag,
-				       size_t size)
+/* deprecated version of tag query */
+static int tegra_bpmp_get_firmware_tag_old(struct tegra_bpmp *bpmp, char *tag,
+					   size_t size)
  {
  	struct mrq_query_tag_request request;
  	struct tegra_bpmp_message msg;
@@ -585,6 +586,35 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag,
  	return err;
  }
  
+static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag,
+				       size_t size)
+{
+	struct mrq_query_fw_tag_response resp;
+	struct tegra_bpmp_message msg = {
+		.mrq = MRQ_QUERY_FW_TAG,
+		.rx = {
+			.data = &resp,
+			.size = sizeof(resp),
+		},
+	};
+	int err;
+	const size_t tag_sz = sizeof(resp.tag);
+
+	if (!tegra_bpmp_mrq_is_supported(bpmp, MRQ_QUERY_FW_TAG))
+		return tegra_bpmp_get_firmware_tag_old(bpmp, tag, size);
+
+	err = tegra_bpmp_transfer(bpmp, &msg);
+
+	if (err)
+		return err;
+	if (msg.rx.ret < 0)
+		return -EINVAL;
+
+	memcpy(tag, resp.tag, min(size, tag_sz));
+
+	return 0;
+}

I find this slightly difficult to read because it mixes MRQ_QUERY_TAG
and MRQ_QUERY_FW_TAG code. I think a more readable version would be
something like this:

	if (tegra_bpmp_mrq_is_supported(bpmp, MRQ_QUERY_FW_TAG)) {
		/* all of the above here, except for the legacy code */
	}

	return tegra_bpmp_get_firmware_tag_old(bpmp, tag, size);

I think that also better reflects the logic: if the new MRQ is
supported, use it. Otherwise, fallback to the old code.

I don't have a preference for this so I'll just update the code as you 
suggest above.

-Timo






