[1/4] firmware: tegra: add helper to check for supported MRQs

Message ID 1539758250-7380-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. 17, 2018, 6:37 a.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

Jon Hunter Oct. 17, 2018, 9:16 a.m. | #1
On 17/10/2018 07:37, 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..5bb46a5 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 ? false : true;

Nit-pick ... seems that the '!=' is not necessary if you switch the
false and true around like the original version of this.

> +}
> +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)
> 

Otherwise ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers,
Jon
Jon Hunter Oct. 17, 2018, 10:09 a.m. | #2
On 17/10/2018 07:37, 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..5bb46a5 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");

Should we make this a dev_dbg? I am wondering if we want to avoid this
print in the case of someone using and older version of the firmware
where this is not a critical error? In other words, should the caller of
this function be responsible for printing any error messages?

Cheers
Jon
Timo Alho Oct. 17, 2018, 4:47 p.m. | #3
On 17.10.2018 13.09, Jonathan Hunter wrote:
> 
> On 17/10/2018 07:37, Timo Alho wrote:
>> +
>> +	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");
> 
> Should we make this a dev_dbg? I am wondering if we want to avoid this
> print in the case of someone using and older version of the firmware
> where this is not a critical error? In other words, should the caller of
> this function be responsible for printing any error messages?

This warning gets printed only when there is an error in communication 
between CPU and BPMP-FW.

For cases where communication was successful, but MRQ is not supported, 
it would be responsibility of the caller to print something if that 
would be useful or needed.

-Timo
Sivaram Nair Oct. 17, 2018, 6:16 p.m. | #4
On Wed, Oct 17, 2018 at 09:37:27AM +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..5bb46a5 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);
>  

Acked-by: Sivaram Nair <sivaramn@nvidia.com>
Jon Hunter Oct. 17, 2018, 6:47 p.m. | #5
On 17/10/2018 17:47, Timo Alho wrote:
> 
> 
> On 17.10.2018 13.09, Jonathan Hunter wrote:
>>
>> On 17/10/2018 07:37, Timo Alho wrote:
>>> +
>>> +    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");
>>
>> Should we make this a dev_dbg? I am wondering if we want to avoid this
>> print in the case of someone using and older version of the firmware
>> where this is not a critical error? In other words, should the caller of
>> this function be responsible for printing any error messages?
> 
> This warning gets printed only when there is an error in communication
> between CPU and BPMP-FW.
> 
> For cases where communication was successful, but MRQ is not supported,
> it would be responsibility of the caller to print something if that
> would be useful or needed.

OK great, then that's fine.

Cheers
Jon

Patch

diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
index 41448ba..5bb46a5 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 ? false : true;
+}
+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)