[V2,1/5] firmware: tegra: add helper to check for supported MRQs

Message ID 1539885344-18974-2-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 a helper function to check that firmware is supporting a given MRQ
command.

Signed-off-by: Timo Alho <talho@nvidia.com>
---
 drivers/firmware/tegra/bpmp.c | 28 ++++++++++++++++++++++++++++
 include/soc/tegra/bpmp.h      |  7 +++++++
 2 files changed, 35 insertions(+)

Comments

Sivaram Nair Oct. 18, 2018, 6:02 p.m. | #1
On Thu, Oct 18, 2018 at 08:55:40PM +0300, Timo Alho wrote:
> Add a helper function to check that firmware is supporting a given MRQ
> command.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp.c | 28 ++++++++++++++++++++++++++++
>  include/soc/tegra/bpmp.h      |  7 +++++++
>  2 files changed, 35 insertions(+)

Acked-by: Sivaram Nair <sivaramn@nvidia.com>
Thierry Reding Oct. 22, 2018, 9:12 a.m. | #2
On Thu, Oct 18, 2018 at 08:55:40PM +0300, Timo Alho wrote:
> Add a helper function to check that firmware is supporting a given MRQ
> command.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp.c | 28 ++++++++++++++++++++++++++++
>  include/soc/tegra/bpmp.h      |  7 +++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> index 41448ba..79859ab 100644
> --- a/drivers/firmware/tegra/bpmp.c
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -470,6 +470,34 @@ void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data)
>  }
>  EXPORT_SYMBOL_GPL(tegra_bpmp_free_mrq);
>  
> +bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq)
> +{
> +	struct mrq_query_abi_request req = { .mrq = cpu_to_le32(mrq) };
> +	struct mrq_query_abi_response resp;
> +	struct tegra_bpmp_message msg = {
> +		.mrq = MRQ_QUERY_ABI,
> +		.tx = {
> +			.data = &req,
> +			.size = sizeof(req),
> +		},
> +		.rx = {
> +			.data = &resp,
> +			.size = sizeof(resp),
> +		},
> +	};
> +	int ret;
> +
> +	ret = tegra_bpmp_transfer(bpmp, &msg);
> +	if (ret != 0 || msg.rx.ret != 0) {

Can somebody remind me of the difference between these two? It looks
like we don't have a description of msg.rx.ret anywhere. It comes from
tegra_bpmp_channel.ib->code, which in turn is set directly by BPMP as
part of a transfer. What are the values that it can take?

I think we could use some kerneldoc for struct tegra_bpmp_mb_data in
include/soc/tegra/bpmp.h.

> +		/* something went wrong; assume not supported */
> +		dev_warn(bpmp->dev, "tegra_bpmp_transfer failed\n");

This should probably contain an error code of some sort to help narrow
down why it fails. Also, please use a more human readable error message
for consistency within the driver. Something along these lines perhaps?

	dev_warn(bpmp->dev, "transfer failed: %d\n", ret);

Or perhaps more specifically:

	dev_warn(bpmp->dev, "ABI query MRQ failed: %d\n", ret);

Thierry
Timo Alho Oct. 22, 2018, 11:45 a.m. | #3
On 22.10.2018 12.12, Thierry Reding wrote:
> On Thu, Oct 18, 2018 at 08:55:40PM +0300, Timo Alho wrote:
>> Add a helper function to check that firmware is supporting a given MRQ
>> command.
>>
>> Signed-off-by: Timo Alho <talho@nvidia.com>
>> ---
>>   drivers/firmware/tegra/bpmp.c | 28 ++++++++++++++++++++++++++++
>>   include/soc/tegra/bpmp.h      |  7 +++++++
>>   2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
>> index 41448ba..79859ab 100644
>> --- a/drivers/firmware/tegra/bpmp.c
>> +++ b/drivers/firmware/tegra/bpmp.c
>> @@ -470,6 +470,34 @@ void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data)
>>   }
>>   EXPORT_SYMBOL_GPL(tegra_bpmp_free_mrq);
>>   
>> +bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq)
>> +{
>> +	struct mrq_query_abi_request req = { .mrq = cpu_to_le32(mrq) };
>> +	struct mrq_query_abi_response resp;
>> +	struct tegra_bpmp_message msg = {
>> +		.mrq = MRQ_QUERY_ABI,
>> +		.tx = {
>> +			.data = &req,
>> +			.size = sizeof(req),
>> +		},
>> +		.rx = {
>> +			.data = &resp,
>> +			.size = sizeof(resp),
>> +		},
>> +	};
>> +	int ret;
>> +
>> +	ret = tegra_bpmp_transfer(bpmp, &msg);
>> +	if (ret != 0 || msg.rx.ret != 0) {
> 
> Can somebody remind me of the difference between these two? It looks
> like we don't have a description of msg.rx.ret anywhere. It comes from
> tegra_bpmp_channel.ib->code, which in turn is set directly by BPMP as > part of a transfer.

This is correct. 'ret' is nonzero if the "wire-level" communication 
between CPU and BPMP fails. In turn, 'msg.rx.ret' is the first word of 
the response payload from bpmp. A nonzero value indicates that the 
request made to BPMP was somehow ill-formatted. Possible reasons could 
be e.g.:
  * Use of non-existing MRQ code
  * Use of non-existing resource identifier (clk, reset, power-domain, ...)

> What are the values that it can take?

Error codes are listed in very end of bpmp-abi.h


> I think we could use some kerneldoc for struct tegra_bpmp_mb_data in
> include/soc/tegra/bpmp.h.
> 
>> +		/* something went wrong; assume not supported */
>> +		dev_warn(bpmp->dev, "tegra_bpmp_transfer failed\n");
> 
> This should probably contain an error code of some sort to help narrow
> down why it fails. Also, please use a more human readable error message
> for consistency within the driver. Something along these lines perhaps?
> 
> 	dev_warn(bpmp->dev, "transfer failed: %d\n", ret);
> 
> Or perhaps more specifically:
> 
> 	dev_warn(bpmp->dev, "ABI query MRQ failed: %d\n", ret);

Sure, I'll make the change.

Patch

diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
index 41448ba..79859ab 100644
--- a/drivers/firmware/tegra/bpmp.c
+++ b/drivers/firmware/tegra/bpmp.c
@@ -470,6 +470,34 @@  void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data)
 }
 EXPORT_SYMBOL_GPL(tegra_bpmp_free_mrq);
 
+bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq)
+{
+	struct mrq_query_abi_request req = { .mrq = cpu_to_le32(mrq) };
+	struct mrq_query_abi_response resp;
+	struct tegra_bpmp_message msg = {
+		.mrq = MRQ_QUERY_ABI,
+		.tx = {
+			.data = &req,
+			.size = sizeof(req),
+		},
+		.rx = {
+			.data = &resp,
+			.size = sizeof(resp),
+		},
+	};
+	int ret;
+
+	ret = tegra_bpmp_transfer(bpmp, &msg);
+	if (ret != 0 || msg.rx.ret != 0) {
+		/* something went wrong; assume not supported */
+		dev_warn(bpmp->dev, "tegra_bpmp_transfer failed\n");
+		return false;
+	}
+
+	return resp.status == 0;
+}
+EXPORT_SYMBOL_GPL(tegra_bpmp_mrq_is_supported);
+
 static void tegra_bpmp_mrq_handle_ping(unsigned int mrq,
 				       struct tegra_bpmp_channel *channel,
 				       void *data)
diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h
index e69e4c4..b02f926 100644
--- a/include/soc/tegra/bpmp.h
+++ b/include/soc/tegra/bpmp.h
@@ -129,6 +129,7 @@  int tegra_bpmp_request_mrq(struct tegra_bpmp *bpmp, unsigned int mrq,
 			   tegra_bpmp_mrq_handler_t handler, void *data);
 void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq,
 			 void *data);
+bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq);
 #else
 static inline struct tegra_bpmp *tegra_bpmp_get(struct device *dev)
 {
@@ -164,6 +165,12 @@  static inline void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp,
 				       unsigned int mrq, void *data)
 {
 }
+
+static inline bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp,
+					      unsigned int mrq)
+{
+	return false;
+}
 #endif
 
 #if IS_ENABLED(CONFIG_CLK_TEGRA_BPMP)