[V2,1/4] firmware: tegra: reword messaging terminology

As a preparatory change to refactor bpmp driver to support other than
t186/t194 chip generations, reword and slightly refactor some of the
functions to better match with what is actually happening in the
wire-level protocol.

The communication with bpmp is essentially a Remote Procedure Call
consisting of "request" and "response". Either side (BPMP or CPU) can
initiate the communication. The state machine for communication
consists of following steps (from Linux point of view):

Linux initiating the call:
 1) check that channel is free to transmit a request (is_req_channel_free)
 2) copy request message payload to shared location
 3) post the request in channel (post_req)
 4) notify BPMP that channel state has been updated (ring_doorbell)
 5) wait for response (is_resp_ready)
 6) copy response message payload from shared location
 7) acknowledge the response in channel (ack_resp)

BPMP initiating the call:
 1) wait for request (is_req_ready)
 2) copy request message payload from shared location
 3) acknowledge the request in channel (ack_req)
 4) check that channel is free to transmit response (is_resp_channel_free)
 5) copy response message payload to shared location
 6) post the response message to channel (post_resp)
 7) notify BPMP that channel state has been updated (ring_doorbell)

Signed-off-by: Timo Alho <talho@nvidia.com>
---
 drivers/firmware/tegra/bpmp.c | 106 +++++++++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 38 deletions(-)

On 21/01/2019 12:28, Timo Alho wrote:
> As a preparatory change to refactor bpmp driver to support other than
> t186/t194 chip generations, reword and slightly refactor some of the
> functions to better match with what is actually happening in the
> wire-level protocol.
> 
> The communication with bpmp is essentially a Remote Procedure Call
> consisting of "request" and "response". Either side (BPMP or CPU) can
> initiate the communication. The state machine for communication
> consists of following steps (from Linux point of view):
> 
> Linux initiating the call:
>  1) check that channel is free to transmit a request (is_req_channel_free)
>  2) copy request message payload to shared location
>  3) post the request in channel (post_req)
>  4) notify BPMP that channel state has been updated (ring_doorbell)
>  5) wait for response (is_resp_ready)
>  6) copy response message payload from shared location
>  7) acknowledge the response in channel (ack_resp)
> 
> BPMP initiating the call:
>  1) wait for request (is_req_ready)
>  2) copy request message payload from shared location
>  3) acknowledge the request in channel (ack_req)
>  4) check that channel is free to transmit response (is_resp_channel_free)
>  5) copy response message payload to shared location
>  6) post the response message to channel (post_resp)
>  7) notify BPMP that channel state has been updated (ring_doorbell)
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp.c | 106 +++++++++++++++++++++++++++---------------
>  1 file changed, 68 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> index 689478b..af123de 100644
> --- a/drivers/firmware/tegra/bpmp.c
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -96,7 +96,7 @@ static bool tegra_bpmp_message_valid(const struct tegra_bpmp_message *msg)
>  	       (msg->rx.size == 0 || msg->rx.data);
>  }
>  
> -static bool tegra_bpmp_master_acked(struct tegra_bpmp_channel *channel)
> +static bool tegra_bpmp_is_resp_ready(struct tegra_bpmp_channel *channel)
>  {
>  	void *frame;
>  
> @@ -111,7 +111,12 @@ static bool tegra_bpmp_master_acked(struct tegra_bpmp_channel *channel)
>  	return true;
>  }
>  
> -static int tegra_bpmp_wait_ack(struct tegra_bpmp_channel *channel)
> +static bool tegra_bpmp_is_req_ready(struct tegra_bpmp_channel *channel)
> +{
> +	return tegra_bpmp_is_resp_ready(channel);
> +}
> +

Any reason not to call this something more generic like
tegra_bpmp_is_message_ready()? I am just wondering if you need to have
this additional function and if it can be avoid. However, not a big
deal, so completely your call.

> +static int tegra_bpmp_wait_resp(struct tegra_bpmp_channel *channel)
>  {
>  	unsigned long timeout = channel->bpmp->soc->channels.cpu_tx.timeout;
>  	ktime_t end;
> @@ -119,14 +124,24 @@ static int tegra_bpmp_wait_ack(struct tegra_bpmp_channel *channel)
>  	end = ktime_add_us(ktime_get(), timeout);
>  
>  	do {
> -		if (tegra_bpmp_master_acked(channel))
> +		if (tegra_bpmp_is_resp_ready(channel))
>  			return 0;
>  	} while (ktime_before(ktime_get(), end));
>  
>  	return -ETIMEDOUT;
>  }
>  
> -static bool tegra_bpmp_master_free(struct tegra_bpmp_channel *channel)
> +static int tegra_bpmp_ack_resp(struct tegra_bpmp_channel *channel)
> +{
> +	return tegra_ivc_read_advance(channel->ivc);
> +}
> +
> +static int tegra_bpmp_ack_req(struct tegra_bpmp_channel *channel)
> +{
> +	return tegra_ivc_read_advance(channel->ivc);
> +}
> +
> +static bool tegra_bpmp_is_req_channel_free(struct tegra_bpmp_channel *channel)
>  {
>  	void *frame;
>  
> @@ -141,7 +156,12 @@ static bool tegra_bpmp_master_free(struct tegra_bpmp_channel *channel)
>  	return true;
>  }
>  
> -static int tegra_bpmp_wait_master_free(struct tegra_bpmp_channel *channel)
> +static bool tegra_bpmp_is_resp_channel_free(struct tegra_bpmp_channel *channel)
> +{
> +	return tegra_bpmp_is_req_channel_free(channel);
> +}
> +

Same here, any reason why not just tegra_bpmp_is_channel_free()?

> +static int tegra_bpmp_wait_req_channel_free(struct tegra_bpmp_channel *channel)
>  {
>  	unsigned long timeout = channel->bpmp->soc->channels.cpu_tx.timeout;
>  	ktime_t start, now;
> @@ -149,7 +169,7 @@ static int tegra_bpmp_wait_master_free(struct tegra_bpmp_channel *channel)
>  	start = ns_to_ktime(local_clock());
>  
>  	do {
> -		if (tegra_bpmp_master_free(channel))
> +		if (tegra_bpmp_is_req_channel_free(channel))
>  			return 0;
>  
>  		now = ns_to_ktime(local_clock());
> @@ -158,6 +178,32 @@ static int tegra_bpmp_wait_master_free(struct tegra_bpmp_channel *channel)
>  	return -ETIMEDOUT;
>  }
>  
> +static int tegra_bpmp_post_req(struct tegra_bpmp_channel *channel)
> +{
> +	return tegra_ivc_write_advance(channel->ivc);
> +}
> +
> +static int tegra_bpmp_post_resp(struct tegra_bpmp_channel *channel)
> +{
> +	return tegra_ivc_write_advance(channel->ivc);
> +}
> +
> +static int tegra_bpmp_ring_doorbell(struct tegra_bpmp *bpmp)
> +{
> +	int err;
> +
> +	if (WARN_ON(bpmp->mbox.channel == NULL))
> +		return -EINVAL;
> +
> +	err = mbox_send_message(bpmp->mbox.channel, NULL);
> +	if (err < 0)
> +		return err;
> +
> +	mbox_client_txdone(bpmp->mbox.channel, 0);
> +
> +	return 0;
> +}
> +
>  static ssize_t __tegra_bpmp_channel_read(struct tegra_bpmp_channel *channel,
>  					 void *data, size_t size, int *ret)
>  {
> @@ -166,7 +212,7 @@ static ssize_t __tegra_bpmp_channel_read(struct tegra_bpmp_channel *channel,
>  	if (data && size > 0)
>  		memcpy(data, channel->ib->data, size);
>  
> -	err = tegra_ivc_read_advance(channel->ivc);
> +	err = tegra_bpmp_ack_resp(channel);
>  	if (err < 0)
>  		return err;
>  
> @@ -210,7 +256,7 @@ static ssize_t __tegra_bpmp_channel_write(struct tegra_bpmp_channel *channel,
>  	if (data && size > 0)
>  		memcpy(channel->ob->data, data, size);
>  
> -	return tegra_ivc_write_advance(channel->ivc);
> +	return tegra_bpmp_post_req(channel);
>  }
>  
>  static struct tegra_bpmp_channel *
> @@ -238,7 +284,7 @@ tegra_bpmp_write_threaded(struct tegra_bpmp *bpmp, unsigned int mrq,
>  
>  	channel = &bpmp->threaded_channels[index];
>  
> -	if (!tegra_bpmp_master_free(channel)) {
> +	if (!tegra_bpmp_is_req_channel_free(channel)) {
>  		err = -EBUSY;
>  		goto unlock;
>  	}
> @@ -270,7 +316,7 @@ static ssize_t tegra_bpmp_channel_write(struct tegra_bpmp_channel *channel,
>  {
>  	int err;
>  
> -	err = tegra_bpmp_wait_master_free(channel);
> +	err = tegra_bpmp_wait_req_channel_free(channel);
>  	if (err < 0)
>  		return err;
>  
> @@ -302,13 +348,11 @@ int tegra_bpmp_transfer_atomic(struct tegra_bpmp *bpmp,
>  
>  	spin_unlock(&bpmp->atomic_tx_lock);
>  
> -	err = mbox_send_message(bpmp->mbox.channel, NULL);
> +	err = tegra_bpmp_ring_doorbell(bpmp);
>  	if (err < 0)
>  		return err;
>  
> -	mbox_client_txdone(bpmp->mbox.channel, 0);
> -
> -	err = tegra_bpmp_wait_ack(channel);
> +	err = tegra_bpmp_wait_resp(channel);
>  	if (err < 0)
>  		return err;
>  
> @@ -335,12 +379,10 @@ int tegra_bpmp_transfer(struct tegra_bpmp *bpmp,
>  	if (IS_ERR(channel))
>  		return PTR_ERR(channel);
>  
> -	err = mbox_send_message(bpmp->mbox.channel, NULL);
> +	err = tegra_bpmp_ring_doorbell(bpmp);
>  	if (err < 0)
>  		return err;
>  
> -	mbox_client_txdone(bpmp->mbox.channel, 0);
> -
>  	timeout = usecs_to_jiffies(bpmp->soc->channels.thread.timeout);
>  
>  	err = wait_for_completion_timeout(&channel->completion, timeout);
> @@ -369,38 +411,34 @@ void tegra_bpmp_mrq_return(struct tegra_bpmp_channel *channel, int code,
>  {
>  	unsigned long flags = channel->ib->flags;
>  	struct tegra_bpmp *bpmp = channel->bpmp;
> -	struct tegra_bpmp_mb_data *frame;
>  	int err;
>  
>  	if (WARN_ON(size > MSG_DATA_MIN_SZ))
>  		return;
>  
> -	err = tegra_ivc_read_advance(channel->ivc);
> +	err = tegra_bpmp_ack_req(channel);
>  	if (WARN_ON(err < 0))
>  		return;
>  
>  	if ((flags & MSG_ACK) == 0)
>  		return;
>  
> -	frame = tegra_ivc_write_get_next_frame(channel->ivc);
> -	if (WARN_ON(IS_ERR(frame)))
> +	if (WARN_ON(!tegra_bpmp_is_resp_channel_free(channel)))
>  		return;
>  
> -	frame->code = code;
> +	channel->ob->code = code;
>  
>  	if (data && size > 0)
> -		memcpy(frame->data, data, size);
> +		memcpy(channel->ob->data, data, size);
>  
> -	err = tegra_ivc_write_advance(channel->ivc);
> +	err = tegra_bpmp_post_resp(channel);
>  	if (WARN_ON(err < 0))
>  		return;
>  
>  	if (flags & MSG_RING) {
> -		err = mbox_send_message(bpmp->mbox.channel, NULL);
> +		err = tegra_bpmp_ring_doorbell(bpmp);
>  		if (WARN_ON(err < 0))
>  			return;
> -
> -		mbox_client_txdone(bpmp->mbox.channel, 0);
>  	}
>  }
>  EXPORT_SYMBOL_GPL(tegra_bpmp_mrq_return);
> @@ -638,7 +676,7 @@ static void tegra_bpmp_handle_rx(struct mbox_client *client, void *data)
>  	count = bpmp->soc->channels.thread.count;
>  	busy = bpmp->threaded.busy;
>  
> -	if (tegra_bpmp_master_acked(channel))
> +	if (tegra_bpmp_is_req_ready(channel))
>  		tegra_bpmp_handle_mrq(bpmp, channel->ib->code, channel);
>  
>  	spin_lock(&bpmp->lock);
> @@ -648,7 +686,7 @@ static void tegra_bpmp_handle_rx(struct mbox_client *client, void *data)
>  
>  		channel = &bpmp->threaded_channels[i];
>  
> -		if (tegra_bpmp_master_acked(channel)) {
> +		if (tegra_bpmp_is_resp_ready(channel)) {
>  			tegra_bpmp_channel_signal(channel);
>  			clear_bit(i, busy);
>  		}
> @@ -660,16 +698,8 @@ static void tegra_bpmp_handle_rx(struct mbox_client *client, void *data)
>  static void tegra_bpmp_ivc_notify(struct tegra_ivc *ivc, void *data)
>  {
>  	struct tegra_bpmp *bpmp = data;
> -	int err;
>  
> -	if (WARN_ON(bpmp->mbox.channel == NULL))
> -		return;
> -
> -	err = mbox_send_message(bpmp->mbox.channel, NULL);
> -	if (err < 0)
> -		return;
> -
> -	mbox_client_txdone(bpmp->mbox.channel, 0);
> +	WARN_ON(tegra_bpmp_ring_doorbell(bpmp));
>  }
>  
>  static int tegra_bpmp_channel_init(struct tegra_bpmp_channel *channel,
> 

Otherwise ...

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

Cheers
Jon

Hi Jon,

On 24.1.2019 13.33, Jon Hunter wrote:
> 
> On 21/01/2019 12:28, Timo Alho wrote:
>> As a preparatory change to refactor bpmp driver to support other than
>> t186/t194 chip generations, reword and slightly refactor some of the
>> functions to better match with what is actually happening in the
>> wire-level protocol.
>>
>> The communication with bpmp is essentially a Remote Procedure Call
>> consisting of "request" and "response". Either side (BPMP or CPU) can
>> initiate the communication. The state machine for communication
>> consists of following steps (from Linux point of view):
>>
>> Linux initiating the call:
>>   1) check that channel is free to transmit a request (is_req_channel_free)
>>   2) copy request message payload to shared location
>>   3) post the request in channel (post_req)
>>   4) notify BPMP that channel state has been updated (ring_doorbell)
>>   5) wait for response (is_resp_ready)
>>   6) copy response message payload from shared location
>>   7) acknowledge the response in channel (ack_resp)
>>
>> BPMP initiating the call:
>>   1) wait for request (is_req_ready)
>>   2) copy request message payload from shared location
>>   3) acknowledge the request in channel (ack_req)
>>   4) check that channel is free to transmit response (is_resp_channel_free)
>>   5) copy response message payload to shared location
>>   6) post the response message to channel (post_resp)
>>   7) notify BPMP that channel state has been updated (ring_doorbell)
>>
>> Signed-off-by: Timo Alho <talho@nvidia.com>
>> ---
>>   drivers/firmware/tegra/bpmp.c | 106 +++++++++++++++++++++++++++---------------
>>   1 file changed, 68 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
>> index 689478b..af123de 100644
>> --- a/drivers/firmware/tegra/bpmp.c
>> +++ b/drivers/firmware/tegra/bpmp.c
>> @@ -96,7 +96,7 @@ static bool tegra_bpmp_message_valid(const struct tegra_bpmp_message *msg)
>>   	       (msg->rx.size == 0 || msg->rx.data);
>>   }
>>   
>> -static bool tegra_bpmp_master_acked(struct tegra_bpmp_channel *channel)
>> +static bool tegra_bpmp_is_resp_ready(struct tegra_bpmp_channel *channel)
>>   {
>>   	void *frame;
>>   
>> @@ -111,7 +111,12 @@ static bool tegra_bpmp_master_acked(struct tegra_bpmp_channel *channel)
>>   	return true;
>>   }
>>   
>> -static int tegra_bpmp_wait_ack(struct tegra_bpmp_channel *channel)
>> +static bool tegra_bpmp_is_req_ready(struct tegra_bpmp_channel *channel)
>> +{
>> +	return tegra_bpmp_is_resp_ready(channel);
>> +}
>> +
> 
> Any reason not to call this something more generic like
> tegra_bpmp_is_message_ready()? I am just wondering if you need to have
> this additional function and if it can be avoid. However, not a big
> deal, so completely your call.

It is true that it looks bit nonsensical in this patch. However, I made 
it like this in the anticipation of the follow up patches -- the use of 
separate req_ready() and resp_ready() becomes obvious once t210 bpmp 
support is added (where req and resp have different implementation).

So, I'd just leave it as is and I see also Acked this patch already. Thanks.

BR,
Timo