[2/4] clk: tegra: check BPMP response return code

Message ID 6a69e270519fd1c7a12a335053bf59671abc3c4b.1504776489.git.talho@nvidia.com
State Accepted
Headers show
Series
  • firmware: tegra: add checks for BPMP error return code
Related show

Commit Message

Timo Alho Sept. 7, 2017, 9:31 a.m.
Check return code in BPMP response message(s). The typical error case
is when clock operation is attempted with invalid clock identifier.

Also remove error print from call to clk_get_info() as the
implementation loops through range of all possible identifier, but the
operation is expected error out when the clock id is unused.

Signed-off-by: Timo Alho <talho@nvidia.com>
---
 drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Jon Hunter Sept. 21, 2017, 11:21 a.m. | #1
On 07/09/17 10:31, Timo Alho wrote:
> Check return code in BPMP response message(s). The typical error case
> is when clock operation is attempted with invalid clock identifier.
> 
> Also remove error print from call to clk_get_info() as the
> implementation loops through range of all possible identifier, but the
> operation is expected error out when the clock id is unused.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
> index 638ace6..a896692 100644
> --- a/drivers/clk/tegra/clk-bpmp.c
> +++ b/drivers/clk/tegra/clk-bpmp.c
> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>  	struct {
>  		void *data;
>  		size_t size;
> +		int ret;
>  	} rx;
>  };
>  
> @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>  	struct mrq_clk_request request;
>  	struct tegra_bpmp_message msg;
>  	void *req = &request;
> +	int err;
>  
>  	memset(&request, 0, sizeof(request));
>  	request.cmd_and_id = (clk->cmd << 24) | clk->id;
> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>  	msg.rx.data = clk->rx.data;
>  	msg.rx.size = clk->rx.size;
>  
> -	return tegra_bpmp_transfer(bpmp, &msg);
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;

I assume that the error codes returned do not correlated to the Linux
error codes here. Is that correct? If not we could just return the
actual error code. Otherwise would it be useful to print a message with
the bpmp error code for debug?

> +
> +	return 0;
>  }
>  
>  static int tegra_bpmp_clk_prepare(struct clk_hw *hw)
> @@ -414,11 +422,8 @@ static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
>  		struct tegra_bpmp_clk_info *info = &clocks[count];
>  
>  		err = tegra_bpmp_clk_get_info(bpmp, id, info);
> -		if (err < 0) {
> -			dev_err(bpmp->dev, "failed to query clock %u: %d\n",
> -				id, err);
> +		if (err < 0)
>  			continue;
> -		}
>  
>  		if (info->num_parents >= U8_MAX) {
>  			dev_err(bpmp->dev,
> 

Cheers
Jon
Timo Alho Sept. 29, 2017, 1:46 p.m. | #2
Hi Jon,

On 21.09.2017 14:21, Jonathan Hunter wrote:
> 
> 
> On 07/09/17 10:31, Timo Alho wrote:
>> Check return code in BPMP response message(s). The typical error case
>> is when clock operation is attempted with invalid clock identifier.
>>
>> Also remove error print from call to clk_get_info() as the
>> implementation loops through range of all possible identifier, but the
>> operation is expected error out when the clock id is unused.
>>
>> Signed-off-by: Timo Alho <talho@nvidia.com>
>> ---
>>   drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
>> index 638ace6..a896692 100644
>> --- a/drivers/clk/tegra/clk-bpmp.c
>> +++ b/drivers/clk/tegra/clk-bpmp.c
>> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>>   	struct {
>>   		void *data;
>>   		size_t size;
>> +		int ret;
>>   	} rx;
>>   };
>>   
>> @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>>   	struct mrq_clk_request request;
>>   	struct tegra_bpmp_message msg;
>>   	void *req = &request;
>> +	int err;
>>   
>>   	memset(&request, 0, sizeof(request));
>>   	request.cmd_and_id = (clk->cmd << 24) | clk->id;
>> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>>   	msg.rx.data = clk->rx.data;
>>   	msg.rx.size = clk->rx.size;
>>   
>> -	return tegra_bpmp_transfer(bpmp, &msg);
>> +	err = tegra_bpmp_transfer(bpmp, &msg);
>> +	if (err < 0)
>> +		return err;
>> +	else if (msg.rx.ret < 0)
>> +		return -EINVAL;
> 
> I assume that the error codes returned do not correlated to the Linux
> error codes here. Is that correct? If not we could just return the
> actual error code. Otherwise would it be useful to print a message with
> the bpmp error code for debug?

The error codes are not 1:1 match with Linux. Unfortunately, printing 
message for debug is not either viable as during clock probing we are 
expecting many of the calls to return -BPMP_EINVAL to indicate that 
particular clock ID is unused.

-Timo
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Hunter Sept. 29, 2017, 2:53 p.m. | #3
On 29/09/17 14:46, Timo Alho wrote:
> Hi Jon,
> 
> On 21.09.2017 14:21, Jonathan Hunter wrote:
>>
>>
>> On 07/09/17 10:31, Timo Alho wrote:
>>> Check return code in BPMP response message(s). The typical error case
>>> is when clock operation is attempted with invalid clock identifier.
>>>
>>> Also remove error print from call to clk_get_info() as the
>>> implementation loops through range of all possible identifier, but the
>>> operation is expected error out when the clock id is unused.
>>>
>>> Signed-off-by: Timo Alho <talho@nvidia.com>
>>> ---
>>>   drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
>>> index 638ace6..a896692 100644
>>> --- a/drivers/clk/tegra/clk-bpmp.c
>>> +++ b/drivers/clk/tegra/clk-bpmp.c
>>> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>>>       struct {
>>>           void *data;
>>>           size_t size;
>>> +        int ret;
>>>       } rx;
>>>   };
>>>   @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct
>>> tegra_bpmp *bpmp,
>>>       struct mrq_clk_request request;
>>>       struct tegra_bpmp_message msg;
>>>       void *req = &request;
>>> +    int err;
>>>         memset(&request, 0, sizeof(request));
>>>       request.cmd_and_id = (clk->cmd << 24) | clk->id;
>>> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct
>>> tegra_bpmp *bpmp,
>>>       msg.rx.data = clk->rx.data;
>>>       msg.rx.size = clk->rx.size;
>>>   -    return tegra_bpmp_transfer(bpmp, &msg);
>>> +    err = tegra_bpmp_transfer(bpmp, &msg);
>>> +    if (err < 0)
>>> +        return err;
>>> +    else if (msg.rx.ret < 0)
>>> +        return -EINVAL;
>>
>> I assume that the error codes returned do not correlated to the Linux
>> error codes here. Is that correct? If not we could just return the
>> actual error code. Otherwise would it be useful to print a message with
>> the bpmp error code for debug?
> 
> The error codes are not 1:1 match with Linux. Unfortunately, printing
> message for debug is not either viable as during clock probing we are
> expecting many of the calls to return -BPMP_EINVAL to indicate that
> particular clock ID is unused.

OK. Could it return other errors other than BPMP_EINVAL? I am just
wondering if we need to differentiate between unused and an actual
error? Maybe that is not possible here?

Cheers
Jon
Timo Alho Oct. 2, 2017, 8:43 a.m. | #4
On 29.09.2017 17:53, Jonathan Hunter wrote:
> 
> On 29/09/17 14:46, Timo Alho wrote:
>> Hi Jon,
>>
>> On 21.09.2017 14:21, Jonathan Hunter wrote:
>>>
>>>
>>> On 07/09/17 10:31, Timo Alho wrote:
>>>> Check return code in BPMP response message(s). The typical error case
>>>> is when clock operation is attempted with invalid clock identifier.
>>>>
>>>> Also remove error print from call to clk_get_info() as the
>>>> implementation loops through range of all possible identifier, but the
>>>> operation is expected error out when the clock id is unused.
>>>>
>>>> Signed-off-by: Timo Alho <talho@nvidia.com>
>>>> ---
>>>>    drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
>>>> index 638ace6..a896692 100644
>>>> --- a/drivers/clk/tegra/clk-bpmp.c
>>>> +++ b/drivers/clk/tegra/clk-bpmp.c
>>>> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>>>>        struct {
>>>>            void *data;
>>>>            size_t size;
>>>> +        int ret;
>>>>        } rx;
>>>>    };
>>>>    @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct
>>>> tegra_bpmp *bpmp,
>>>>        struct mrq_clk_request request;
>>>>        struct tegra_bpmp_message msg;
>>>>        void *req = &request;
>>>> +    int err;
>>>>          memset(&request, 0, sizeof(request));
>>>>        request.cmd_and_id = (clk->cmd << 24) | clk->id;
>>>> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct
>>>> tegra_bpmp *bpmp,
>>>>        msg.rx.data = clk->rx.data;
>>>>        msg.rx.size = clk->rx.size;
>>>>    -    return tegra_bpmp_transfer(bpmp, &msg);
>>>> +    err = tegra_bpmp_transfer(bpmp, &msg);
>>>> +    if (err < 0)
>>>> +        return err;
>>>> +    else if (msg.rx.ret < 0)
>>>> +        return -EINVAL;
>>>
>>> I assume that the error codes returned do not correlated to the Linux
>>> error codes here. Is that correct? If not we could just return the
>>> actual error code. Otherwise would it be useful to print a message with
>>> the bpmp error code for debug?
>>
>> The error codes are not 1:1 match with Linux. Unfortunately, printing
>> message for debug is not either viable as during clock probing we are
>> expecting many of the calls to return -BPMP_EINVAL to indicate that
>> particular clock ID is unused.
> 
> OK. Could it return other errors other than BPMP_EINVAL? I am just
> wondering if we need to differentiate between unused and an actual
> error? Maybe that is not possible here?

Other error codes are possible (though they are not explicitly 
documented in abi header). It's not easy to differentiate the error code 
at this level: -BPMP_EINVAL is "expected" condition with 
CMD_CLK_GET_ALL_INFO, whereas -BPMP_EINVAL is true error on other commands.



--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Hunter Oct. 2, 2017, 8:23 p.m. | #5
On 02/10/17 09:43, Timo Alho wrote:
> 
> 
> On 29.09.2017 17:53, Jonathan Hunter wrote:
>>
>> On 29/09/17 14:46, Timo Alho wrote:
>>> Hi Jon,
>>>
>>> On 21.09.2017 14:21, Jonathan Hunter wrote:
>>>>
>>>>
>>>> On 07/09/17 10:31, Timo Alho wrote:
>>>>> Check return code in BPMP response message(s). The typical error case
>>>>> is when clock operation is attempted with invalid clock identifier.
>>>>>
>>>>> Also remove error print from call to clk_get_info() as the
>>>>> implementation loops through range of all possible identifier, but the
>>>>> operation is expected error out when the clock id is unused.
>>>>>
>>>>> Signed-off-by: Timo Alho <talho@nvidia.com>
>>>>> ---
>>>>>    drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clk/tegra/clk-bpmp.c
>>>>> b/drivers/clk/tegra/clk-bpmp.c
>>>>> index 638ace6..a896692 100644
>>>>> --- a/drivers/clk/tegra/clk-bpmp.c
>>>>> +++ b/drivers/clk/tegra/clk-bpmp.c
>>>>> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>>>>>        struct {
>>>>>            void *data;
>>>>>            size_t size;
>>>>> +        int ret;
>>>>>        } rx;
>>>>>    };
>>>>>    @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct
>>>>> tegra_bpmp *bpmp,
>>>>>        struct mrq_clk_request request;
>>>>>        struct tegra_bpmp_message msg;
>>>>>        void *req = &request;
>>>>> +    int err;
>>>>>          memset(&request, 0, sizeof(request));
>>>>>        request.cmd_and_id = (clk->cmd << 24) | clk->id;
>>>>> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct
>>>>> tegra_bpmp *bpmp,
>>>>>        msg.rx.data = clk->rx.data;
>>>>>        msg.rx.size = clk->rx.size;
>>>>>    -    return tegra_bpmp_transfer(bpmp, &msg);
>>>>> +    err = tegra_bpmp_transfer(bpmp, &msg);
>>>>> +    if (err < 0)
>>>>> +        return err;
>>>>> +    else if (msg.rx.ret < 0)
>>>>> +        return -EINVAL;
>>>>
>>>> I assume that the error codes returned do not correlated to the Linux
>>>> error codes here. Is that correct? If not we could just return the
>>>> actual error code. Otherwise would it be useful to print a message with
>>>> the bpmp error code for debug?
>>>
>>> The error codes are not 1:1 match with Linux. Unfortunately, printing
>>> message for debug is not either viable as during clock probing we are
>>> expecting many of the calls to return -BPMP_EINVAL to indicate that
>>> particular clock ID is unused.
>>
>> OK. Could it return other errors other than BPMP_EINVAL? I am just
>> wondering if we need to differentiate between unused and an actual
>> error? Maybe that is not possible here?
> 
> Other error codes are possible (though they are not explicitly
> documented in abi header). It's not easy to differentiate the error code
> at this level: -BPMP_EINVAL is "expected" condition with
> CMD_CLK_GET_ALL_INFO, whereas -BPMP_EINVAL is true error on other commands.

OK, thanks for clarifying.

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

Cheers
Jon
Thierry Reding Oct. 17, 2017, 10:37 a.m. | #6
On Thu, Sep 07, 2017 at 12:31:02PM +0300, Timo Alho wrote:
> Check return code in BPMP response message(s). The typical error case
> is when clock operation is attempted with invalid clock identifier.
> 
> Also remove error print from call to clk_get_info() as the
> implementation loops through range of all possible identifier, but the
> operation is expected error out when the clock id is unused.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Hi Mike, Stephen,

do you mind if I pick this up into the Tegra tree? It has a build
dependency on patch 1/4.

I only now realized that you guys weren't on Cc, hence quoting in
full.

Thierry

> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
> index 638ace6..a896692 100644
> --- a/drivers/clk/tegra/clk-bpmp.c
> +++ b/drivers/clk/tegra/clk-bpmp.c
> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>  	struct {
>  		void *data;
>  		size_t size;
> +		int ret;
>  	} rx;
>  };
>  
> @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>  	struct mrq_clk_request request;
>  	struct tegra_bpmp_message msg;
>  	void *req = &request;
> +	int err;
>  
>  	memset(&request, 0, sizeof(request));
>  	request.cmd_and_id = (clk->cmd << 24) | clk->id;
> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>  	msg.rx.data = clk->rx.data;
>  	msg.rx.size = clk->rx.size;
>  
> -	return tegra_bpmp_transfer(bpmp, &msg);
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static int tegra_bpmp_clk_prepare(struct clk_hw *hw)
> @@ -414,11 +422,8 @@ static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
>  		struct tegra_bpmp_clk_info *info = &clocks[count];
>  
>  		err = tegra_bpmp_clk_get_info(bpmp, id, info);
> -		if (err < 0) {
> -			dev_err(bpmp->dev, "failed to query clock %u: %d\n",
> -				id, err);
> +		if (err < 0)
>  			continue;
> -		}
>  
>  		if (info->num_parents >= U8_MAX) {
>  			dev_err(bpmp->dev,
> -- 
> 2.7.4
>
Stephen Boyd Oct. 21, 2017, 2:10 p.m. | #7
On 10/17, Thierry Reding wrote:
> On Thu, Sep 07, 2017 at 12:31:02PM +0300, Timo Alho wrote:
> > Check return code in BPMP response message(s). The typical error case
> > is when clock operation is attempted with invalid clock identifier.
> > 
> > Also remove error print from call to clk_get_info() as the
> > implementation loops through range of all possible identifier, but the
> > operation is expected error out when the clock id is unused.
> > 
> > Signed-off-by: Timo Alho <talho@nvidia.com>
> > ---
> >  drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> Hi Mike, Stephen,
> 
> do you mind if I pick this up into the Tegra tree? It has a build
> dependency on patch 1/4.
> 
> I only now realized that you guys weren't on Cc, hence quoting in
> full.
> 

No worries

Acked-by: Stephen Boyd <sboyd@codeaurora.org>

Patch

diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
index 638ace6..a896692 100644
--- a/drivers/clk/tegra/clk-bpmp.c
+++ b/drivers/clk/tegra/clk-bpmp.c
@@ -55,6 +55,7 @@  struct tegra_bpmp_clk_message {
 	struct {
 		void *data;
 		size_t size;
+		int ret;
 	} rx;
 };
 
@@ -64,6 +65,7 @@  static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
 	struct mrq_clk_request request;
 	struct tegra_bpmp_message msg;
 	void *req = &request;
+	int err;
 
 	memset(&request, 0, sizeof(request));
 	request.cmd_and_id = (clk->cmd << 24) | clk->id;
@@ -84,7 +86,13 @@  static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
 	msg.rx.data = clk->rx.data;
 	msg.rx.size = clk->rx.size;
 
-	return tegra_bpmp_transfer(bpmp, &msg);
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err < 0)
+		return err;
+	else if (msg.rx.ret < 0)
+		return -EINVAL;
+
+	return 0;
 }
 
 static int tegra_bpmp_clk_prepare(struct clk_hw *hw)
@@ -414,11 +422,8 @@  static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
 		struct tegra_bpmp_clk_info *info = &clocks[count];
 
 		err = tegra_bpmp_clk_get_info(bpmp, id, info);
-		if (err < 0) {
-			dev_err(bpmp->dev, "failed to query clock %u: %d\n",
-				id, err);
+		if (err < 0)
 			continue;
-		}
 
 		if (info->num_parents >= U8_MAX) {
 			dev_err(bpmp->dev,