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

Message ID 1539885344-18974-6-git-send-email-talho@nvidia.com
State New
Headers show
Series
  • BPMP-FW version query ("tag") update
Related show

Commit Message

Timo Alho Oct. 18, 2018, 5:55 p.m.
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@nvidia.com>
---
 drivers/firmware/tegra/bpmp.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

Comments

Sivaram Nair Oct. 18, 2018, 6:17 p.m. | #1
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@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)

Acked-by: Sivaram Nair <sivaramn@nvidia.com>
Thierry Reding Oct. 22, 2018, 9:46 a.m. | #2
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@nvidia.com>
> ---
>  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.

Thierry
Timo Alho Oct. 22, 2018, 11:36 a.m. | #3
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@nvidia.com>
>> ---
>>   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

Patch

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;
+}
+
 static void tegra_bpmp_channel_signal(struct tegra_bpmp_channel *channel)
 {
 	unsigned long flags = channel->ob->flags;