Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Fri, Jul 28, 2017 at 3:18 PM, Anup Patelwrote: > On Fri, Jul 28, 2017 at 2:34 PM, Jassi Brar wrote: >> On Fri, Jul 28, 2017 at 2:19 PM, Anup Patel wrote: >>> On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar >>> wrote: On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel wrote: > On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar > wrote: >>> Sorry for the delayed response... >>> >>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >>> wrote: Hi Anup, On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: > The Broadcom FlexRM ring (i.e. mailbox channel) can handle > larger number of messages queued in one FlexRM ring hence > this patch sets msg_queue_len for each mailbox channel to > be same as RING_MAX_REQ_COUNT. > > Signed-off-by: Anup Patel > Reviewed-by: Scott Branden > --- > drivers/mailbox/bcm-flexrm-mailbox.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c > b/drivers/mailbox/bcm-flexrm-mailbox.c > index 9873818..20055a0 100644 > --- a/drivers/mailbox/bcm-flexrm-mailbox.c > +++ b/drivers/mailbox/bcm-flexrm-mailbox.c > @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct > platform_device *pdev) > ret = -ENOMEM; > goto fail_free_debugfs_root; > } > - for (index = 0; index < mbox->num_rings; index++) > + for (index = 0; index < mbox->num_rings; index++) { > + mbox->controller.chans[index].msg_queue_len = > + > RING_MAX_REQ_COUNT; > mbox->controller.chans[index].con_priv = > >rings[index]; > + } > While writing mailbox.c I wasn't unaware that there is the option to choose the queue length at runtime. The idea was to keep the code as simple as possible. I am open to making it a runtime thing, but first, please help me understand how that is useful here. I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) elements. Any message submitted to mailbox api can be immediately written onto the ringbuffer if there is some space. Is there any mechanism to report back to a client driver, if its message in ringbuffer failed "to be sent"? If there isn't any, then I think, in flexrm_last_tx_done() you should simply return true if there is some space left in the rung-buffer, false otherwise. >>> >>> Yes, we have error code in "struct brcm_message" to report back >>> errors from send_message. In our mailbox clients, we check >>> return value of mbox_send_message() and also the error code >>> in "struct brcm_message". >>> >> I meant after the message has been accepted in the ringbuffer but the >> remote failed to receive it. > > Yes, even this case is handled. > > In case of IO errors after message has been put in ring buffer, we get > completion message with error code and mailbox client drivers will > receive back "struct brcm_message" with error set. > > You can refer flexrm_process_completions() for more details. > >> It doesn't seem to be what I suggest. I see two issues in >> flexrm_process_completions() >> 1) It calls mbox_send_message(), which is a big NO for a controller >> driver. Why should you have one more message stored outside of >> ringbuffer? > > The "last_pending_msg" in each FlexRM ring was added to fit FlexRM > in Mailbox framework. > > We don't have any IRQ for TX done so "txdone_irq" out of the question for > FlexRM. We only have completions for both success or failures (IO errors). > > This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", > we have to provide last_tx_done() callback. The last_tx_done() callback > is supposed to return true if last send_data() call succeeded. > > To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". > > When "last_pending_msg" is NULL it means last call to send_data() > succeeded >
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Fri, Jul 28, 2017 at 3:18 PM, Anup Patel wrote: > On Fri, Jul 28, 2017 at 2:34 PM, Jassi Brar wrote: >> On Fri, Jul 28, 2017 at 2:19 PM, Anup Patel wrote: >>> On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar >>> wrote: On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel wrote: > On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar > wrote: >>> Sorry for the delayed response... >>> >>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >>> wrote: Hi Anup, On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: > The Broadcom FlexRM ring (i.e. mailbox channel) can handle > larger number of messages queued in one FlexRM ring hence > this patch sets msg_queue_len for each mailbox channel to > be same as RING_MAX_REQ_COUNT. > > Signed-off-by: Anup Patel > Reviewed-by: Scott Branden > --- > drivers/mailbox/bcm-flexrm-mailbox.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c > b/drivers/mailbox/bcm-flexrm-mailbox.c > index 9873818..20055a0 100644 > --- a/drivers/mailbox/bcm-flexrm-mailbox.c > +++ b/drivers/mailbox/bcm-flexrm-mailbox.c > @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct > platform_device *pdev) > ret = -ENOMEM; > goto fail_free_debugfs_root; > } > - for (index = 0; index < mbox->num_rings; index++) > + for (index = 0; index < mbox->num_rings; index++) { > + mbox->controller.chans[index].msg_queue_len = > + > RING_MAX_REQ_COUNT; > mbox->controller.chans[index].con_priv = > >rings[index]; > + } > While writing mailbox.c I wasn't unaware that there is the option to choose the queue length at runtime. The idea was to keep the code as simple as possible. I am open to making it a runtime thing, but first, please help me understand how that is useful here. I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) elements. Any message submitted to mailbox api can be immediately written onto the ringbuffer if there is some space. Is there any mechanism to report back to a client driver, if its message in ringbuffer failed "to be sent"? If there isn't any, then I think, in flexrm_last_tx_done() you should simply return true if there is some space left in the rung-buffer, false otherwise. >>> >>> Yes, we have error code in "struct brcm_message" to report back >>> errors from send_message. In our mailbox clients, we check >>> return value of mbox_send_message() and also the error code >>> in "struct brcm_message". >>> >> I meant after the message has been accepted in the ringbuffer but the >> remote failed to receive it. > > Yes, even this case is handled. > > In case of IO errors after message has been put in ring buffer, we get > completion message with error code and mailbox client drivers will > receive back "struct brcm_message" with error set. > > You can refer flexrm_process_completions() for more details. > >> It doesn't seem to be what I suggest. I see two issues in >> flexrm_process_completions() >> 1) It calls mbox_send_message(), which is a big NO for a controller >> driver. Why should you have one more message stored outside of >> ringbuffer? > > The "last_pending_msg" in each FlexRM ring was added to fit FlexRM > in Mailbox framework. > > We don't have any IRQ for TX done so "txdone_irq" out of the question for > FlexRM. We only have completions for both success or failures (IO errors). > > This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", > we have to provide last_tx_done() callback. The last_tx_done() callback > is supposed to return true if last send_data() call succeeded. > > To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". > > When "last_pending_msg" is NULL it means last call to send_data() > succeeded > and when "last_pending_msg" is != NULL it means last call to send_data() > did not go through due to lack of space in FlexRM ring. > It could be simpler. Since flexrm_send_data() is essentially about putting the message in the
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Fri, Jul 28, 2017 at 2:34 PM, Jassi Brarwrote: > On Fri, Jul 28, 2017 at 2:19 PM, Anup Patel wrote: >> On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar wrote: >>> On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel >>> wrote: On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar wrote: >>> >> Sorry for the delayed response... >> >> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >> wrote: >>> Hi Anup, >>> >>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >>> wrote: The Broadcom FlexRM ring (i.e. mailbox channel) can handle larger number of messages queued in one FlexRM ring hence this patch sets msg_queue_len for each mailbox channel to be same as RING_MAX_REQ_COUNT. Signed-off-by: Anup Patel Reviewed-by: Scott Branden --- drivers/mailbox/bcm-flexrm-mailbox.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c index 9873818..20055a0 100644 --- a/drivers/mailbox/bcm-flexrm-mailbox.c +++ b/drivers/mailbox/bcm-flexrm-mailbox.c @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev) ret = -ENOMEM; goto fail_free_debugfs_root; } - for (index = 0; index < mbox->num_rings; index++) + for (index = 0; index < mbox->num_rings; index++) { + mbox->controller.chans[index].msg_queue_len = + RING_MAX_REQ_COUNT; mbox->controller.chans[index].con_priv = >rings[index]; + } >>> While writing mailbox.c I wasn't unaware that there is the option to >>> choose the queue length at runtime. >>> The idea was to keep the code as simple as possible. I am open to >>> making it a runtime thing, but first, please help me understand how >>> that is useful here. >>> >>> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >>> elements. Any message submitted to mailbox api can be immediately >>> written onto the ringbuffer if there is some space. >>> Is there any mechanism to report back to a client driver, if its >>> message in ringbuffer failed "to be sent"? >>> If there isn't any, then I think, in flexrm_last_tx_done() you >>> should >>> simply return true if there is some space left in the rung-buffer, >>> false otherwise. >> >> Yes, we have error code in "struct brcm_message" to report back >> errors from send_message. In our mailbox clients, we check >> return value of mbox_send_message() and also the error code >> in "struct brcm_message". >> > I meant after the message has been accepted in the ringbuffer but the > remote failed to receive it. Yes, even this case is handled. In case of IO errors after message has been put in ring buffer, we get completion message with error code and mailbox client drivers will receive back "struct brcm_message" with error set. You can refer flexrm_process_completions() for more details. > It doesn't seem to be what I suggest. I see two issues in > flexrm_process_completions() > 1) It calls mbox_send_message(), which is a big NO for a controller > driver. Why should you have one more message stored outside of > ringbuffer? The "last_pending_msg" in each FlexRM ring was added to fit FlexRM in Mailbox framework. We don't have any IRQ for TX done so "txdone_irq" out of the question for FlexRM. We only have completions for both success or failures (IO errors). This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", we have to provide last_tx_done() callback. The last_tx_done() callback is supposed to return true if last send_data() call succeeded. To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". When "last_pending_msg" is NULL it means last call to send_data() succeeded and when "last_pending_msg" is != NULL it means last call to send_data() did not go through due to lack of space in FlexRM ring. >>> It could be simpler. >>> Since flexrm_send_data() is essentially about
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Fri, Jul 28, 2017 at 2:34 PM, Jassi Brar wrote: > On Fri, Jul 28, 2017 at 2:19 PM, Anup Patel wrote: >> On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar wrote: >>> On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel >>> wrote: On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar wrote: >>> >> Sorry for the delayed response... >> >> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >> wrote: >>> Hi Anup, >>> >>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >>> wrote: The Broadcom FlexRM ring (i.e. mailbox channel) can handle larger number of messages queued in one FlexRM ring hence this patch sets msg_queue_len for each mailbox channel to be same as RING_MAX_REQ_COUNT. Signed-off-by: Anup Patel Reviewed-by: Scott Branden --- drivers/mailbox/bcm-flexrm-mailbox.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c index 9873818..20055a0 100644 --- a/drivers/mailbox/bcm-flexrm-mailbox.c +++ b/drivers/mailbox/bcm-flexrm-mailbox.c @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev) ret = -ENOMEM; goto fail_free_debugfs_root; } - for (index = 0; index < mbox->num_rings; index++) + for (index = 0; index < mbox->num_rings; index++) { + mbox->controller.chans[index].msg_queue_len = + RING_MAX_REQ_COUNT; mbox->controller.chans[index].con_priv = >rings[index]; + } >>> While writing mailbox.c I wasn't unaware that there is the option to >>> choose the queue length at runtime. >>> The idea was to keep the code as simple as possible. I am open to >>> making it a runtime thing, but first, please help me understand how >>> that is useful here. >>> >>> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >>> elements. Any message submitted to mailbox api can be immediately >>> written onto the ringbuffer if there is some space. >>> Is there any mechanism to report back to a client driver, if its >>> message in ringbuffer failed "to be sent"? >>> If there isn't any, then I think, in flexrm_last_tx_done() you >>> should >>> simply return true if there is some space left in the rung-buffer, >>> false otherwise. >> >> Yes, we have error code in "struct brcm_message" to report back >> errors from send_message. In our mailbox clients, we check >> return value of mbox_send_message() and also the error code >> in "struct brcm_message". >> > I meant after the message has been accepted in the ringbuffer but the > remote failed to receive it. Yes, even this case is handled. In case of IO errors after message has been put in ring buffer, we get completion message with error code and mailbox client drivers will receive back "struct brcm_message" with error set. You can refer flexrm_process_completions() for more details. > It doesn't seem to be what I suggest. I see two issues in > flexrm_process_completions() > 1) It calls mbox_send_message(), which is a big NO for a controller > driver. Why should you have one more message stored outside of > ringbuffer? The "last_pending_msg" in each FlexRM ring was added to fit FlexRM in Mailbox framework. We don't have any IRQ for TX done so "txdone_irq" out of the question for FlexRM. We only have completions for both success or failures (IO errors). This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", we have to provide last_tx_done() callback. The last_tx_done() callback is supposed to return true if last send_data() call succeeded. To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". When "last_pending_msg" is NULL it means last call to send_data() succeeded and when "last_pending_msg" is != NULL it means last call to send_data() did not go through due to lack of space in FlexRM ring. >>> It could be simpler. >>> Since flexrm_send_data() is essentially about putting the message in >>> the ring-buffer (and not about _transmission_ failures), the >>> last_tx_done() should simply return true if requests_ida has not all >>> ids allocated. False otherwise. >> >> It's not that simple because
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Fri, Jul 28, 2017 at 2:19 PM, Anup Patelwrote: > On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar wrote: >> On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel wrote: >>> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar >>> wrote: >> > Sorry for the delayed response... > > On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar > wrote: >> Hi Anup, >> >> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >> wrote: >>> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >>> larger number of messages queued in one FlexRM ring hence >>> this patch sets msg_queue_len for each mailbox channel to >>> be same as RING_MAX_REQ_COUNT. >>> >>> Signed-off-by: Anup Patel >>> Reviewed-by: Scott Branden >>> --- >>> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >>> b/drivers/mailbox/bcm-flexrm-mailbox.c >>> index 9873818..20055a0 100644 >>> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >>> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >>> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct >>> platform_device *pdev) >>> ret = -ENOMEM; >>> goto fail_free_debugfs_root; >>> } >>> - for (index = 0; index < mbox->num_rings; index++) >>> + for (index = 0; index < mbox->num_rings; index++) { >>> + mbox->controller.chans[index].msg_queue_len = >>> + RING_MAX_REQ_COUNT; >>> mbox->controller.chans[index].con_priv = >>> >rings[index]; >>> + } >>> >> While writing mailbox.c I wasn't unaware that there is the option to >> choose the queue length at runtime. >> The idea was to keep the code as simple as possible. I am open to >> making it a runtime thing, but first, please help me understand how >> that is useful here. >> >> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >> elements. Any message submitted to mailbox api can be immediately >> written onto the ringbuffer if there is some space. >> Is there any mechanism to report back to a client driver, if its >> message in ringbuffer failed "to be sent"? >> If there isn't any, then I think, in flexrm_last_tx_done() you should >> simply return true if there is some space left in the rung-buffer, >> false otherwise. > > Yes, we have error code in "struct brcm_message" to report back > errors from send_message. In our mailbox clients, we check > return value of mbox_send_message() and also the error code > in "struct brcm_message". > I meant after the message has been accepted in the ringbuffer but the remote failed to receive it. >>> >>> Yes, even this case is handled. >>> >>> In case of IO errors after message has been put in ring buffer, we get >>> completion message with error code and mailbox client drivers will >>> receive back "struct brcm_message" with error set. >>> >>> You can refer flexrm_process_completions() for more details. >>> It doesn't seem to be what I suggest. I see two issues in flexrm_process_completions() 1) It calls mbox_send_message(), which is a big NO for a controller driver. Why should you have one more message stored outside of ringbuffer? >>> >>> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM >>> in Mailbox framework. >>> >>> We don't have any IRQ for TX done so "txdone_irq" out of the question for >>> FlexRM. We only have completions for both success or failures (IO errors). >>> >>> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", >>> we have to provide last_tx_done() callback. The last_tx_done() callback >>> is supposed to return true if last send_data() call succeeded. >>> >>> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". >>> >>> When "last_pending_msg" is NULL it means last call to send_data() succeeded >>> and when "last_pending_msg" is != NULL it means last call to send_data() >>> did not go through due to lack of space in FlexRM ring. >>> >> It could be simpler. >> Since flexrm_send_data() is essentially about putting the message in >> the ring-buffer (and not about _transmission_ failures), the >> last_tx_done() should simply return true if requests_ida has not all >> ids allocated. False otherwise. >
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Fri, Jul 28, 2017 at 2:19 PM, Anup Patel wrote: > On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar wrote: >> On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel wrote: >>> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar >>> wrote: >> > Sorry for the delayed response... > > On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar > wrote: >> Hi Anup, >> >> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >> wrote: >>> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >>> larger number of messages queued in one FlexRM ring hence >>> this patch sets msg_queue_len for each mailbox channel to >>> be same as RING_MAX_REQ_COUNT. >>> >>> Signed-off-by: Anup Patel >>> Reviewed-by: Scott Branden >>> --- >>> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >>> b/drivers/mailbox/bcm-flexrm-mailbox.c >>> index 9873818..20055a0 100644 >>> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >>> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >>> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct >>> platform_device *pdev) >>> ret = -ENOMEM; >>> goto fail_free_debugfs_root; >>> } >>> - for (index = 0; index < mbox->num_rings; index++) >>> + for (index = 0; index < mbox->num_rings; index++) { >>> + mbox->controller.chans[index].msg_queue_len = >>> + RING_MAX_REQ_COUNT; >>> mbox->controller.chans[index].con_priv = >>> >rings[index]; >>> + } >>> >> While writing mailbox.c I wasn't unaware that there is the option to >> choose the queue length at runtime. >> The idea was to keep the code as simple as possible. I am open to >> making it a runtime thing, but first, please help me understand how >> that is useful here. >> >> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >> elements. Any message submitted to mailbox api can be immediately >> written onto the ringbuffer if there is some space. >> Is there any mechanism to report back to a client driver, if its >> message in ringbuffer failed "to be sent"? >> If there isn't any, then I think, in flexrm_last_tx_done() you should >> simply return true if there is some space left in the rung-buffer, >> false otherwise. > > Yes, we have error code in "struct brcm_message" to report back > errors from send_message. In our mailbox clients, we check > return value of mbox_send_message() and also the error code > in "struct brcm_message". > I meant after the message has been accepted in the ringbuffer but the remote failed to receive it. >>> >>> Yes, even this case is handled. >>> >>> In case of IO errors after message has been put in ring buffer, we get >>> completion message with error code and mailbox client drivers will >>> receive back "struct brcm_message" with error set. >>> >>> You can refer flexrm_process_completions() for more details. >>> It doesn't seem to be what I suggest. I see two issues in flexrm_process_completions() 1) It calls mbox_send_message(), which is a big NO for a controller driver. Why should you have one more message stored outside of ringbuffer? >>> >>> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM >>> in Mailbox framework. >>> >>> We don't have any IRQ for TX done so "txdone_irq" out of the question for >>> FlexRM. We only have completions for both success or failures (IO errors). >>> >>> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", >>> we have to provide last_tx_done() callback. The last_tx_done() callback >>> is supposed to return true if last send_data() call succeeded. >>> >>> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". >>> >>> When "last_pending_msg" is NULL it means last call to send_data() succeeded >>> and when "last_pending_msg" is != NULL it means last call to send_data() >>> did not go through due to lack of space in FlexRM ring. >>> >> It could be simpler. >> Since flexrm_send_data() is essentially about putting the message in >> the ring-buffer (and not about _transmission_ failures), the >> last_tx_done() should simply return true if requests_ida has not all >> ids allocated. False otherwise. > > It's not that simple because we have two cases in-which > send_data() will fail: > 1. It run-out of IDs in requests_ida > 2. There is no room in BD queue of FlexRM ring. This because each > brcm_message
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brarwrote: > On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel wrote: >> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar >> wrote: > Sorry for the delayed response... On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: > Hi Anup, > > On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel > wrote: >> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >> larger number of messages queued in one FlexRM ring hence >> this patch sets msg_queue_len for each mailbox channel to >> be same as RING_MAX_REQ_COUNT. >> >> Signed-off-by: Anup Patel >> Reviewed-by: Scott Branden >> --- >> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >> b/drivers/mailbox/bcm-flexrm-mailbox.c >> index 9873818..20055a0 100644 >> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct >> platform_device *pdev) >> ret = -ENOMEM; >> goto fail_free_debugfs_root; >> } >> - for (index = 0; index < mbox->num_rings; index++) >> + for (index = 0; index < mbox->num_rings; index++) { >> + mbox->controller.chans[index].msg_queue_len = >> + RING_MAX_REQ_COUNT; >> mbox->controller.chans[index].con_priv = >> >rings[index]; >> + } >> > While writing mailbox.c I wasn't unaware that there is the option to > choose the queue length at runtime. > The idea was to keep the code as simple as possible. I am open to > making it a runtime thing, but first, please help me understand how > that is useful here. > > I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) > elements. Any message submitted to mailbox api can be immediately > written onto the ringbuffer if there is some space. > Is there any mechanism to report back to a client driver, if its > message in ringbuffer failed "to be sent"? > If there isn't any, then I think, in flexrm_last_tx_done() you should > simply return true if there is some space left in the rung-buffer, > false otherwise. Yes, we have error code in "struct brcm_message" to report back errors from send_message. In our mailbox clients, we check return value of mbox_send_message() and also the error code in "struct brcm_message". >>> I meant after the message has been accepted in the ringbuffer but the >>> remote failed to receive it. >> >> Yes, even this case is handled. >> >> In case of IO errors after message has been put in ring buffer, we get >> completion message with error code and mailbox client drivers will >> receive back "struct brcm_message" with error set. >> >> You can refer flexrm_process_completions() for more details. >> >>> It doesn't seem to be what I suggest. I see two issues in >>> flexrm_process_completions() >>> 1) It calls mbox_send_message(), which is a big NO for a controller >>> driver. Why should you have one more message stored outside of >>> ringbuffer? >> >> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM >> in Mailbox framework. >> >> We don't have any IRQ for TX done so "txdone_irq" out of the question for >> FlexRM. We only have completions for both success or failures (IO errors). >> >> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", >> we have to provide last_tx_done() callback. The last_tx_done() callback >> is supposed to return true if last send_data() call succeeded. >> >> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". >> >> When "last_pending_msg" is NULL it means last call to send_data() succeeded >> and when "last_pending_msg" is != NULL it means last call to send_data() >> did not go through due to lack of space in FlexRM ring. >> > It could be simpler. > Since flexrm_send_data() is essentially about putting the message in > the ring-buffer (and not about _transmission_ failures), the > last_tx_done() should simply return true if requests_ida has not all > ids allocated. False otherwise. It's not that simple because we have two cases in-which send_data() will fail: 1. It run-out of IDs in requests_ida 2. There is no room in BD queue of FlexRM ring. This because
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar wrote: > On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel wrote: >> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar >> wrote: > Sorry for the delayed response... On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: > Hi Anup, > > On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel > wrote: >> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >> larger number of messages queued in one FlexRM ring hence >> this patch sets msg_queue_len for each mailbox channel to >> be same as RING_MAX_REQ_COUNT. >> >> Signed-off-by: Anup Patel >> Reviewed-by: Scott Branden >> --- >> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >> b/drivers/mailbox/bcm-flexrm-mailbox.c >> index 9873818..20055a0 100644 >> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct >> platform_device *pdev) >> ret = -ENOMEM; >> goto fail_free_debugfs_root; >> } >> - for (index = 0; index < mbox->num_rings; index++) >> + for (index = 0; index < mbox->num_rings; index++) { >> + mbox->controller.chans[index].msg_queue_len = >> + RING_MAX_REQ_COUNT; >> mbox->controller.chans[index].con_priv = >> >rings[index]; >> + } >> > While writing mailbox.c I wasn't unaware that there is the option to > choose the queue length at runtime. > The idea was to keep the code as simple as possible. I am open to > making it a runtime thing, but first, please help me understand how > that is useful here. > > I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) > elements. Any message submitted to mailbox api can be immediately > written onto the ringbuffer if there is some space. > Is there any mechanism to report back to a client driver, if its > message in ringbuffer failed "to be sent"? > If there isn't any, then I think, in flexrm_last_tx_done() you should > simply return true if there is some space left in the rung-buffer, > false otherwise. Yes, we have error code in "struct brcm_message" to report back errors from send_message. In our mailbox clients, we check return value of mbox_send_message() and also the error code in "struct brcm_message". >>> I meant after the message has been accepted in the ringbuffer but the >>> remote failed to receive it. >> >> Yes, even this case is handled. >> >> In case of IO errors after message has been put in ring buffer, we get >> completion message with error code and mailbox client drivers will >> receive back "struct brcm_message" with error set. >> >> You can refer flexrm_process_completions() for more details. >> >>> It doesn't seem to be what I suggest. I see two issues in >>> flexrm_process_completions() >>> 1) It calls mbox_send_message(), which is a big NO for a controller >>> driver. Why should you have one more message stored outside of >>> ringbuffer? >> >> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM >> in Mailbox framework. >> >> We don't have any IRQ for TX done so "txdone_irq" out of the question for >> FlexRM. We only have completions for both success or failures (IO errors). >> >> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", >> we have to provide last_tx_done() callback. The last_tx_done() callback >> is supposed to return true if last send_data() call succeeded. >> >> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". >> >> When "last_pending_msg" is NULL it means last call to send_data() succeeded >> and when "last_pending_msg" is != NULL it means last call to send_data() >> did not go through due to lack of space in FlexRM ring. >> > It could be simpler. > Since flexrm_send_data() is essentially about putting the message in > the ring-buffer (and not about _transmission_ failures), the > last_tx_done() should simply return true if requests_ida has not all > ids allocated. False otherwise. It's not that simple because we have two cases in-which send_data() will fail: 1. It run-out of IDs in requests_ida 2. There is no room in BD queue of FlexRM ring. This because each brcm_message can be translated into variable number of descriptors. In fact, using SPU2 crypto client we have one brcm_message translating into 100's of descriptors. All-in-all
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Thu, Jul 27, 2017 at 11:20 AM, Anup Patelwrote: > On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar wrote: >>> Sorry for the delayed response... >>> >>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >>> wrote: Hi Anup, On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: > The Broadcom FlexRM ring (i.e. mailbox channel) can handle > larger number of messages queued in one FlexRM ring hence > this patch sets msg_queue_len for each mailbox channel to > be same as RING_MAX_REQ_COUNT. > > Signed-off-by: Anup Patel > Reviewed-by: Scott Branden > --- > drivers/mailbox/bcm-flexrm-mailbox.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c > b/drivers/mailbox/bcm-flexrm-mailbox.c > index 9873818..20055a0 100644 > --- a/drivers/mailbox/bcm-flexrm-mailbox.c > +++ b/drivers/mailbox/bcm-flexrm-mailbox.c > @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct > platform_device *pdev) > ret = -ENOMEM; > goto fail_free_debugfs_root; > } > - for (index = 0; index < mbox->num_rings; index++) > + for (index = 0; index < mbox->num_rings; index++) { > + mbox->controller.chans[index].msg_queue_len = > + RING_MAX_REQ_COUNT; > mbox->controller.chans[index].con_priv = > >rings[index]; > + } > While writing mailbox.c I wasn't unaware that there is the option to choose the queue length at runtime. The idea was to keep the code as simple as possible. I am open to making it a runtime thing, but first, please help me understand how that is useful here. I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) elements. Any message submitted to mailbox api can be immediately written onto the ringbuffer if there is some space. Is there any mechanism to report back to a client driver, if its message in ringbuffer failed "to be sent"? If there isn't any, then I think, in flexrm_last_tx_done() you should simply return true if there is some space left in the rung-buffer, false otherwise. >>> >>> Yes, we have error code in "struct brcm_message" to report back >>> errors from send_message. In our mailbox clients, we check >>> return value of mbox_send_message() and also the error code >>> in "struct brcm_message". >>> >> I meant after the message has been accepted in the ringbuffer but the >> remote failed to receive it. > > Yes, even this case is handled. > > In case of IO errors after message has been put in ring buffer, we get > completion message with error code and mailbox client drivers will > receive back "struct brcm_message" with error set. > > You can refer flexrm_process_completions() for more details. > >> It doesn't seem to be what I suggest. I see two issues in >> flexrm_process_completions() >> 1) It calls mbox_send_message(), which is a big NO for a controller >> driver. Why should you have one more message stored outside of >> ringbuffer? > > The "last_pending_msg" in each FlexRM ring was added to fit FlexRM > in Mailbox framework. > > We don't have any IRQ for TX done so "txdone_irq" out of the question for > FlexRM. We only have completions for both success or failures (IO errors). > > This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", > we have to provide last_tx_done() callback. The last_tx_done() callback > is supposed to return true if last send_data() call succeeded. > > To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". > > When "last_pending_msg" is NULL it means last call to send_data() succeeded > and when "last_pending_msg" is != NULL it means last call to send_data() > did not go through due to lack of space in FlexRM ring. > It could be simpler. Since flexrm_send_data() is essentially about putting the message in the ring-buffer (and not about _transmission_ failures), the last_tx_done() should simply return true if requests_ida has not all ids allocated. False otherwise. >> >> 2) It calls mbox_chan_received_data() which is for messages received >> from the remote. And not the way to report failed _transmission_, for >> which the api calls back mbox_client.tx_done() . In your client >> driver please populate mbox_client.tx_done() and see which message is >> reported "sent fine" when. >> >> >> There seems no such provision.
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel wrote: > On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar wrote: >>> Sorry for the delayed response... >>> >>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >>> wrote: Hi Anup, On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: > The Broadcom FlexRM ring (i.e. mailbox channel) can handle > larger number of messages queued in one FlexRM ring hence > this patch sets msg_queue_len for each mailbox channel to > be same as RING_MAX_REQ_COUNT. > > Signed-off-by: Anup Patel > Reviewed-by: Scott Branden > --- > drivers/mailbox/bcm-flexrm-mailbox.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c > b/drivers/mailbox/bcm-flexrm-mailbox.c > index 9873818..20055a0 100644 > --- a/drivers/mailbox/bcm-flexrm-mailbox.c > +++ b/drivers/mailbox/bcm-flexrm-mailbox.c > @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct > platform_device *pdev) > ret = -ENOMEM; > goto fail_free_debugfs_root; > } > - for (index = 0; index < mbox->num_rings; index++) > + for (index = 0; index < mbox->num_rings; index++) { > + mbox->controller.chans[index].msg_queue_len = > + RING_MAX_REQ_COUNT; > mbox->controller.chans[index].con_priv = > >rings[index]; > + } > While writing mailbox.c I wasn't unaware that there is the option to choose the queue length at runtime. The idea was to keep the code as simple as possible. I am open to making it a runtime thing, but first, please help me understand how that is useful here. I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) elements. Any message submitted to mailbox api can be immediately written onto the ringbuffer if there is some space. Is there any mechanism to report back to a client driver, if its message in ringbuffer failed "to be sent"? If there isn't any, then I think, in flexrm_last_tx_done() you should simply return true if there is some space left in the rung-buffer, false otherwise. >>> >>> Yes, we have error code in "struct brcm_message" to report back >>> errors from send_message. In our mailbox clients, we check >>> return value of mbox_send_message() and also the error code >>> in "struct brcm_message". >>> >> I meant after the message has been accepted in the ringbuffer but the >> remote failed to receive it. > > Yes, even this case is handled. > > In case of IO errors after message has been put in ring buffer, we get > completion message with error code and mailbox client drivers will > receive back "struct brcm_message" with error set. > > You can refer flexrm_process_completions() for more details. > >> It doesn't seem to be what I suggest. I see two issues in >> flexrm_process_completions() >> 1) It calls mbox_send_message(), which is a big NO for a controller >> driver. Why should you have one more message stored outside of >> ringbuffer? > > The "last_pending_msg" in each FlexRM ring was added to fit FlexRM > in Mailbox framework. > > We don't have any IRQ for TX done so "txdone_irq" out of the question for > FlexRM. We only have completions for both success or failures (IO errors). > > This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", > we have to provide last_tx_done() callback. The last_tx_done() callback > is supposed to return true if last send_data() call succeeded. > > To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". > > When "last_pending_msg" is NULL it means last call to send_data() succeeded > and when "last_pending_msg" is != NULL it means last call to send_data() > did not go through due to lack of space in FlexRM ring. > It could be simpler. Since flexrm_send_data() is essentially about putting the message in the ring-buffer (and not about _transmission_ failures), the last_tx_done() should simply return true if requests_ida has not all ids allocated. False otherwise. >> >> 2) It calls mbox_chan_received_data() which is for messages received >> from the remote. And not the way to report failed _transmission_, for >> which the api calls back mbox_client.tx_done() . In your client >> driver please populate mbox_client.tx_done() and see which message is >> reported "sent fine" when. >> >> >> There seems no such provision. IIANW, then you should be able to >> consider every message as "sent successfully" once it is in the ring >> buffer i.e, immediately after
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brarwrote: > On Thu, Jul 27, 2017 at 9:25 AM, Anup Patel wrote: >> On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brar wrote: >>> On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel >>> wrote: On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar wrote: > On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel > wrote: >> Hi Jassi, >> >> Sorry for the delayed response... >> >> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >> wrote: >>> Hi Anup, >>> >>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >>> wrote: The Broadcom FlexRM ring (i.e. mailbox channel) can handle larger number of messages queued in one FlexRM ring hence this patch sets msg_queue_len for each mailbox channel to be same as RING_MAX_REQ_COUNT. Signed-off-by: Anup Patel Reviewed-by: Scott Branden --- drivers/mailbox/bcm-flexrm-mailbox.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c index 9873818..20055a0 100644 --- a/drivers/mailbox/bcm-flexrm-mailbox.c +++ b/drivers/mailbox/bcm-flexrm-mailbox.c @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev) ret = -ENOMEM; goto fail_free_debugfs_root; } - for (index = 0; index < mbox->num_rings; index++) + for (index = 0; index < mbox->num_rings; index++) { + mbox->controller.chans[index].msg_queue_len = + RING_MAX_REQ_COUNT; mbox->controller.chans[index].con_priv = >rings[index]; + } >>> While writing mailbox.c I wasn't unaware that there is the option to >>> choose the queue length at runtime. >>> The idea was to keep the code as simple as possible. I am open to >>> making it a runtime thing, but first, please help me understand how >>> that is useful here. >>> >>> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >>> elements. Any message submitted to mailbox api can be immediately >>> written onto the ringbuffer if there is some space. >>> Is there any mechanism to report back to a client driver, if its >>> message in ringbuffer failed "to be sent"? >>> If there isn't any, then I think, in flexrm_last_tx_done() you should >>> simply return true if there is some space left in the rung-buffer, >>> false otherwise. >> >> Yes, we have error code in "struct brcm_message" to report back >> errors from send_message. In our mailbox clients, we check >> return value of mbox_send_message() and also the error code >> in "struct brcm_message". >> > I meant after the message has been accepted in the ringbuffer but the > remote failed to receive it. Yes, even this case is handled. In case of IO errors after message has been put in ring buffer, we get completion message with error code and mailbox client drivers will receive back "struct brcm_message" with error set. You can refer flexrm_process_completions() for more details. > It doesn't seem to be what I suggest. I see two issues in > flexrm_process_completions() > 1) It calls mbox_send_message(), which is a big NO for a controller > driver. Why should you have one more message stored outside of > ringbuffer? The "last_pending_msg" in each FlexRM ring was added to fit FlexRM in Mailbox framework. We don't have any IRQ for TX done so "txdone_irq" out of the question for FlexRM. We only have completions for both success or failures (IO errors). This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", we have to provide last_tx_done() callback. The last_tx_done() callback is supposed to return true if last send_data() call succeeded. To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". When "last_pending_msg" is NULL it means last call to send_data() succeeded and when "last_pending_msg" is != NULL it means last call to send_data() did not go through due to lack of space in FlexRM ring. The IRQ worker of FlexRM ring will automatically queue the message pointed by "last_pending_message" and clear it. This is why we have mbox_send_message() call in flexrm_process_completions(). > > 2) It calls mbox_chan_received_data() which is for messages received > from the remote. And not the way to report failed _transmission_, for > which
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar wrote: > On Thu, Jul 27, 2017 at 9:25 AM, Anup Patel wrote: >> On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brar wrote: >>> On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel >>> wrote: On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar wrote: > On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel > wrote: >> Hi Jassi, >> >> Sorry for the delayed response... >> >> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >> wrote: >>> Hi Anup, >>> >>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >>> wrote: The Broadcom FlexRM ring (i.e. mailbox channel) can handle larger number of messages queued in one FlexRM ring hence this patch sets msg_queue_len for each mailbox channel to be same as RING_MAX_REQ_COUNT. Signed-off-by: Anup Patel Reviewed-by: Scott Branden --- drivers/mailbox/bcm-flexrm-mailbox.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c index 9873818..20055a0 100644 --- a/drivers/mailbox/bcm-flexrm-mailbox.c +++ b/drivers/mailbox/bcm-flexrm-mailbox.c @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev) ret = -ENOMEM; goto fail_free_debugfs_root; } - for (index = 0; index < mbox->num_rings; index++) + for (index = 0; index < mbox->num_rings; index++) { + mbox->controller.chans[index].msg_queue_len = + RING_MAX_REQ_COUNT; mbox->controller.chans[index].con_priv = >rings[index]; + } >>> While writing mailbox.c I wasn't unaware that there is the option to >>> choose the queue length at runtime. >>> The idea was to keep the code as simple as possible. I am open to >>> making it a runtime thing, but first, please help me understand how >>> that is useful here. >>> >>> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >>> elements. Any message submitted to mailbox api can be immediately >>> written onto the ringbuffer if there is some space. >>> Is there any mechanism to report back to a client driver, if its >>> message in ringbuffer failed "to be sent"? >>> If there isn't any, then I think, in flexrm_last_tx_done() you should >>> simply return true if there is some space left in the rung-buffer, >>> false otherwise. >> >> Yes, we have error code in "struct brcm_message" to report back >> errors from send_message. In our mailbox clients, we check >> return value of mbox_send_message() and also the error code >> in "struct brcm_message". >> > I meant after the message has been accepted in the ringbuffer but the > remote failed to receive it. Yes, even this case is handled. In case of IO errors after message has been put in ring buffer, we get completion message with error code and mailbox client drivers will receive back "struct brcm_message" with error set. You can refer flexrm_process_completions() for more details. > It doesn't seem to be what I suggest. I see two issues in > flexrm_process_completions() > 1) It calls mbox_send_message(), which is a big NO for a controller > driver. Why should you have one more message stored outside of > ringbuffer? The "last_pending_msg" in each FlexRM ring was added to fit FlexRM in Mailbox framework. We don't have any IRQ for TX done so "txdone_irq" out of the question for FlexRM. We only have completions for both success or failures (IO errors). This means we have to use "txdone_poll" for FlexRM. For "txdone_poll", we have to provide last_tx_done() callback. The last_tx_done() callback is supposed to return true if last send_data() call succeeded. To implement last_tx_done() in FlexRM driver, we added "last_pending_msg". When "last_pending_msg" is NULL it means last call to send_data() succeeded and when "last_pending_msg" is != NULL it means last call to send_data() did not go through due to lack of space in FlexRM ring. The IRQ worker of FlexRM ring will automatically queue the message pointed by "last_pending_message" and clear it. This is why we have mbox_send_message() call in flexrm_process_completions(). > > 2) It calls mbox_chan_received_data() which is for messages received > from the remote. And not the way to report failed _transmission_, for > which the api calls back mbox_client.tx_done() . In your client > driver please populate mbox_client.tx_done() and see which message is > reported "sent fine" when. > > > There seems no such provision. IIANW, then you should be able to > consider every
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Thu, Jul 27, 2017 at 9:25 AM, Anup Patelwrote: > On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brar wrote: >> On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel wrote: >>> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar >>> wrote: On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: > Hi Jassi, > > Sorry for the delayed response... > > On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar > wrote: >> Hi Anup, >> >> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >> wrote: >>> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >>> larger number of messages queued in one FlexRM ring hence >>> this patch sets msg_queue_len for each mailbox channel to >>> be same as RING_MAX_REQ_COUNT. >>> >>> Signed-off-by: Anup Patel >>> Reviewed-by: Scott Branden >>> --- >>> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >>> b/drivers/mailbox/bcm-flexrm-mailbox.c >>> index 9873818..20055a0 100644 >>> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >>> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >>> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct >>> platform_device *pdev) >>> ret = -ENOMEM; >>> goto fail_free_debugfs_root; >>> } >>> - for (index = 0; index < mbox->num_rings; index++) >>> + for (index = 0; index < mbox->num_rings; index++) { >>> + mbox->controller.chans[index].msg_queue_len = >>> + RING_MAX_REQ_COUNT; >>> mbox->controller.chans[index].con_priv = >>> >rings[index]; >>> + } >>> >> While writing mailbox.c I wasn't unaware that there is the option to >> choose the queue length at runtime. >> The idea was to keep the code as simple as possible. I am open to >> making it a runtime thing, but first, please help me understand how >> that is useful here. >> >> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >> elements. Any message submitted to mailbox api can be immediately >> written onto the ringbuffer if there is some space. >> Is there any mechanism to report back to a client driver, if its >> message in ringbuffer failed "to be sent"? >> If there isn't any, then I think, in flexrm_last_tx_done() you should >> simply return true if there is some space left in the rung-buffer, >> false otherwise. > > Yes, we have error code in "struct brcm_message" to report back > errors from send_message. In our mailbox clients, we check > return value of mbox_send_message() and also the error code > in "struct brcm_message". > I meant after the message has been accepted in the ringbuffer but the remote failed to receive it. >>> >>> Yes, even this case is handled. >>> >>> In case of IO errors after message has been put in ring buffer, we get >>> completion message with error code and mailbox client drivers will >>> receive back "struct brcm_message" with error set. >>> >>> You can refer flexrm_process_completions() for more details. >>> It doesn't seem to be what I suggest. I see two issues in flexrm_process_completions() 1) It calls mbox_send_message(), which is a big NO for a controller driver. Why should you have one more message stored outside of ringbuffer? 2) It calls mbox_chan_received_data() which is for messages received from the remote. And not the way to report failed _transmission_, for which the api calls back mbox_client.tx_done() . In your client driver please populate mbox_client.tx_done() and see which message is reported "sent fine" when. There seems no such provision. IIANW, then you should be able to consider every message as "sent successfully" once it is in the ring buffer i.e, immediately after mbox_send_message() returns 0. In that case I would think you don't need more than a couple of entries out of MBOX_TX_QUEUE_LEN ? >>> >>> What I am trying to suggest is that we can take upto 1024 messages >>> in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing >>> more messages. This issue manifest easily when multiple CPUs >>> queues to same FlexRM ring (i.e. same mailbox channel). >>> >> OK then, I guess we have to make the queue length a runtime decision. > > Do you agree with approach taken by PATCH5 and PATCH6 to > make queue length runtime? > I agree that we may have to get the queue length from platform, if MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both of us. However I suspect the right fix for _this_
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Thu, Jul 27, 2017 at 9:25 AM, Anup Patel wrote: > On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brar wrote: >> On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel wrote: >>> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar >>> wrote: On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: > Hi Jassi, > > Sorry for the delayed response... > > On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar > wrote: >> Hi Anup, >> >> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >> wrote: >>> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >>> larger number of messages queued in one FlexRM ring hence >>> this patch sets msg_queue_len for each mailbox channel to >>> be same as RING_MAX_REQ_COUNT. >>> >>> Signed-off-by: Anup Patel >>> Reviewed-by: Scott Branden >>> --- >>> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >>> b/drivers/mailbox/bcm-flexrm-mailbox.c >>> index 9873818..20055a0 100644 >>> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >>> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >>> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct >>> platform_device *pdev) >>> ret = -ENOMEM; >>> goto fail_free_debugfs_root; >>> } >>> - for (index = 0; index < mbox->num_rings; index++) >>> + for (index = 0; index < mbox->num_rings; index++) { >>> + mbox->controller.chans[index].msg_queue_len = >>> + RING_MAX_REQ_COUNT; >>> mbox->controller.chans[index].con_priv = >>> >rings[index]; >>> + } >>> >> While writing mailbox.c I wasn't unaware that there is the option to >> choose the queue length at runtime. >> The idea was to keep the code as simple as possible. I am open to >> making it a runtime thing, but first, please help me understand how >> that is useful here. >> >> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >> elements. Any message submitted to mailbox api can be immediately >> written onto the ringbuffer if there is some space. >> Is there any mechanism to report back to a client driver, if its >> message in ringbuffer failed "to be sent"? >> If there isn't any, then I think, in flexrm_last_tx_done() you should >> simply return true if there is some space left in the rung-buffer, >> false otherwise. > > Yes, we have error code in "struct brcm_message" to report back > errors from send_message. In our mailbox clients, we check > return value of mbox_send_message() and also the error code > in "struct brcm_message". > I meant after the message has been accepted in the ringbuffer but the remote failed to receive it. >>> >>> Yes, even this case is handled. >>> >>> In case of IO errors after message has been put in ring buffer, we get >>> completion message with error code and mailbox client drivers will >>> receive back "struct brcm_message" with error set. >>> >>> You can refer flexrm_process_completions() for more details. >>> It doesn't seem to be what I suggest. I see two issues in flexrm_process_completions() 1) It calls mbox_send_message(), which is a big NO for a controller driver. Why should you have one more message stored outside of ringbuffer? 2) It calls mbox_chan_received_data() which is for messages received from the remote. And not the way to report failed _transmission_, for which the api calls back mbox_client.tx_done() . In your client driver please populate mbox_client.tx_done() and see which message is reported "sent fine" when. There seems no such provision. IIANW, then you should be able to consider every message as "sent successfully" once it is in the ring buffer i.e, immediately after mbox_send_message() returns 0. In that case I would think you don't need more than a couple of entries out of MBOX_TX_QUEUE_LEN ? >>> >>> What I am trying to suggest is that we can take upto 1024 messages >>> in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing >>> more messages. This issue manifest easily when multiple CPUs >>> queues to same FlexRM ring (i.e. same mailbox channel). >>> >> OK then, I guess we have to make the queue length a runtime decision. > > Do you agree with approach taken by PATCH5 and PATCH6 to > make queue length runtime? > I agree that we may have to get the queue length from platform, if MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both of us. However I suspect the right fix for _this_ situation is in flexrm driver. See above. >> >> BTW, is it a practical use case that needs to queue upto 1024 >> requests? Or are you just testing? > > Yes, we just need bigger queue length for FlexRM but we > choose 1024 (max limit)
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brarwrote: > On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel wrote: >> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar >> wrote: >>> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: Hi Jassi, Sorry for the delayed response... On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: > Hi Anup, > > On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel > wrote: >> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >> larger number of messages queued in one FlexRM ring hence >> this patch sets msg_queue_len for each mailbox channel to >> be same as RING_MAX_REQ_COUNT. >> >> Signed-off-by: Anup Patel >> Reviewed-by: Scott Branden >> --- >> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >> b/drivers/mailbox/bcm-flexrm-mailbox.c >> index 9873818..20055a0 100644 >> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct >> platform_device *pdev) >> ret = -ENOMEM; >> goto fail_free_debugfs_root; >> } >> - for (index = 0; index < mbox->num_rings; index++) >> + for (index = 0; index < mbox->num_rings; index++) { >> + mbox->controller.chans[index].msg_queue_len = >> + RING_MAX_REQ_COUNT; >> mbox->controller.chans[index].con_priv = >> >rings[index]; >> + } >> > While writing mailbox.c I wasn't unaware that there is the option to > choose the queue length at runtime. > The idea was to keep the code as simple as possible. I am open to > making it a runtime thing, but first, please help me understand how > that is useful here. > > I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) > elements. Any message submitted to mailbox api can be immediately > written onto the ringbuffer if there is some space. > Is there any mechanism to report back to a client driver, if its > message in ringbuffer failed "to be sent"? > If there isn't any, then I think, in flexrm_last_tx_done() you should > simply return true if there is some space left in the rung-buffer, > false otherwise. Yes, we have error code in "struct brcm_message" to report back errors from send_message. In our mailbox clients, we check return value of mbox_send_message() and also the error code in "struct brcm_message". >>> I meant after the message has been accepted in the ringbuffer but the >>> remote failed to receive it. >> >> Yes, even this case is handled. >> >> In case of IO errors after message has been put in ring buffer, we get >> completion message with error code and mailbox client drivers will >> receive back "struct brcm_message" with error set. >> >> You can refer flexrm_process_completions() for more details. >> >>> There seems no such provision. IIANW, then you should be able to >>> consider every message as "sent successfully" once it is in the ring >>> buffer i.e, immediately after mbox_send_message() returns 0. >>> In that case I would think you don't need more than a couple of >>> entries out of MBOX_TX_QUEUE_LEN ? >> >> What I am trying to suggest is that we can take upto 1024 messages >> in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing >> more messages. This issue manifest easily when multiple CPUs >> queues to same FlexRM ring (i.e. same mailbox channel). >> > OK then, I guess we have to make the queue length a runtime decision. Do you agree with approach taken by PATCH5 and PATCH6 to make queue length runtime? > > BTW, is it a practical use case that needs to queue upto 1024 > requests? Or are you just testing? Yes, we just need bigger queue length for FlexRM but we choose 1024 (max limit) to avoid changing it again in future. Regards, Anup
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brar wrote: > On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel wrote: >> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar >> wrote: >>> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: Hi Jassi, Sorry for the delayed response... On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: > Hi Anup, > > On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel > wrote: >> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >> larger number of messages queued in one FlexRM ring hence >> this patch sets msg_queue_len for each mailbox channel to >> be same as RING_MAX_REQ_COUNT. >> >> Signed-off-by: Anup Patel >> Reviewed-by: Scott Branden >> --- >> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >> b/drivers/mailbox/bcm-flexrm-mailbox.c >> index 9873818..20055a0 100644 >> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct >> platform_device *pdev) >> ret = -ENOMEM; >> goto fail_free_debugfs_root; >> } >> - for (index = 0; index < mbox->num_rings; index++) >> + for (index = 0; index < mbox->num_rings; index++) { >> + mbox->controller.chans[index].msg_queue_len = >> + RING_MAX_REQ_COUNT; >> mbox->controller.chans[index].con_priv = >> >rings[index]; >> + } >> > While writing mailbox.c I wasn't unaware that there is the option to > choose the queue length at runtime. > The idea was to keep the code as simple as possible. I am open to > making it a runtime thing, but first, please help me understand how > that is useful here. > > I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) > elements. Any message submitted to mailbox api can be immediately > written onto the ringbuffer if there is some space. > Is there any mechanism to report back to a client driver, if its > message in ringbuffer failed "to be sent"? > If there isn't any, then I think, in flexrm_last_tx_done() you should > simply return true if there is some space left in the rung-buffer, > false otherwise. Yes, we have error code in "struct brcm_message" to report back errors from send_message. In our mailbox clients, we check return value of mbox_send_message() and also the error code in "struct brcm_message". >>> I meant after the message has been accepted in the ringbuffer but the >>> remote failed to receive it. >> >> Yes, even this case is handled. >> >> In case of IO errors after message has been put in ring buffer, we get >> completion message with error code and mailbox client drivers will >> receive back "struct brcm_message" with error set. >> >> You can refer flexrm_process_completions() for more details. >> >>> There seems no such provision. IIANW, then you should be able to >>> consider every message as "sent successfully" once it is in the ring >>> buffer i.e, immediately after mbox_send_message() returns 0. >>> In that case I would think you don't need more than a couple of >>> entries out of MBOX_TX_QUEUE_LEN ? >> >> What I am trying to suggest is that we can take upto 1024 messages >> in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing >> more messages. This issue manifest easily when multiple CPUs >> queues to same FlexRM ring (i.e. same mailbox channel). >> > OK then, I guess we have to make the queue length a runtime decision. Do you agree with approach taken by PATCH5 and PATCH6 to make queue length runtime? > > BTW, is it a practical use case that needs to queue upto 1024 > requests? Or are you just testing? Yes, we just need bigger queue length for FlexRM but we choose 1024 (max limit) to avoid changing it again in future. Regards, Anup
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Tue, Jul 25, 2017 at 11:11 AM, Anup Patelwrote: > On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar wrote: >> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: >>> Hi Jassi, >>> >>> Sorry for the delayed response... >>> >>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >>> wrote: Hi Anup, On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: > The Broadcom FlexRM ring (i.e. mailbox channel) can handle > larger number of messages queued in one FlexRM ring hence > this patch sets msg_queue_len for each mailbox channel to > be same as RING_MAX_REQ_COUNT. > > Signed-off-by: Anup Patel > Reviewed-by: Scott Branden > --- > drivers/mailbox/bcm-flexrm-mailbox.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c > b/drivers/mailbox/bcm-flexrm-mailbox.c > index 9873818..20055a0 100644 > --- a/drivers/mailbox/bcm-flexrm-mailbox.c > +++ b/drivers/mailbox/bcm-flexrm-mailbox.c > @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct > platform_device *pdev) > ret = -ENOMEM; > goto fail_free_debugfs_root; > } > - for (index = 0; index < mbox->num_rings; index++) > + for (index = 0; index < mbox->num_rings; index++) { > + mbox->controller.chans[index].msg_queue_len = > + RING_MAX_REQ_COUNT; > mbox->controller.chans[index].con_priv = > >rings[index]; > + } > While writing mailbox.c I wasn't unaware that there is the option to choose the queue length at runtime. The idea was to keep the code as simple as possible. I am open to making it a runtime thing, but first, please help me understand how that is useful here. I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) elements. Any message submitted to mailbox api can be immediately written onto the ringbuffer if there is some space. Is there any mechanism to report back to a client driver, if its message in ringbuffer failed "to be sent"? If there isn't any, then I think, in flexrm_last_tx_done() you should simply return true if there is some space left in the rung-buffer, false otherwise. >>> >>> Yes, we have error code in "struct brcm_message" to report back >>> errors from send_message. In our mailbox clients, we check >>> return value of mbox_send_message() and also the error code >>> in "struct brcm_message". >>> >> I meant after the message has been accepted in the ringbuffer but the >> remote failed to receive it. > > Yes, even this case is handled. > > In case of IO errors after message has been put in ring buffer, we get > completion message with error code and mailbox client drivers will > receive back "struct brcm_message" with error set. > > You can refer flexrm_process_completions() for more details. > >> There seems no such provision. IIANW, then you should be able to >> consider every message as "sent successfully" once it is in the ring >> buffer i.e, immediately after mbox_send_message() returns 0. >> In that case I would think you don't need more than a couple of >> entries out of MBOX_TX_QUEUE_LEN ? > > What I am trying to suggest is that we can take upto 1024 messages > in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing > more messages. This issue manifest easily when multiple CPUs > queues to same FlexRM ring (i.e. same mailbox channel). > OK then, I guess we have to make the queue length a runtime decision. BTW, is it a practical use case that needs to queue upto 1024 requests? Or are you just testing? Thanks
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel wrote: > On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar wrote: >> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: >>> Hi Jassi, >>> >>> Sorry for the delayed response... >>> >>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar >>> wrote: Hi Anup, On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: > The Broadcom FlexRM ring (i.e. mailbox channel) can handle > larger number of messages queued in one FlexRM ring hence > this patch sets msg_queue_len for each mailbox channel to > be same as RING_MAX_REQ_COUNT. > > Signed-off-by: Anup Patel > Reviewed-by: Scott Branden > --- > drivers/mailbox/bcm-flexrm-mailbox.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c > b/drivers/mailbox/bcm-flexrm-mailbox.c > index 9873818..20055a0 100644 > --- a/drivers/mailbox/bcm-flexrm-mailbox.c > +++ b/drivers/mailbox/bcm-flexrm-mailbox.c > @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct > platform_device *pdev) > ret = -ENOMEM; > goto fail_free_debugfs_root; > } > - for (index = 0; index < mbox->num_rings; index++) > + for (index = 0; index < mbox->num_rings; index++) { > + mbox->controller.chans[index].msg_queue_len = > + RING_MAX_REQ_COUNT; > mbox->controller.chans[index].con_priv = > >rings[index]; > + } > While writing mailbox.c I wasn't unaware that there is the option to choose the queue length at runtime. The idea was to keep the code as simple as possible. I am open to making it a runtime thing, but first, please help me understand how that is useful here. I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) elements. Any message submitted to mailbox api can be immediately written onto the ringbuffer if there is some space. Is there any mechanism to report back to a client driver, if its message in ringbuffer failed "to be sent"? If there isn't any, then I think, in flexrm_last_tx_done() you should simply return true if there is some space left in the rung-buffer, false otherwise. >>> >>> Yes, we have error code in "struct brcm_message" to report back >>> errors from send_message. In our mailbox clients, we check >>> return value of mbox_send_message() and also the error code >>> in "struct brcm_message". >>> >> I meant after the message has been accepted in the ringbuffer but the >> remote failed to receive it. > > Yes, even this case is handled. > > In case of IO errors after message has been put in ring buffer, we get > completion message with error code and mailbox client drivers will > receive back "struct brcm_message" with error set. > > You can refer flexrm_process_completions() for more details. > >> There seems no such provision. IIANW, then you should be able to >> consider every message as "sent successfully" once it is in the ring >> buffer i.e, immediately after mbox_send_message() returns 0. >> In that case I would think you don't need more than a couple of >> entries out of MBOX_TX_QUEUE_LEN ? > > What I am trying to suggest is that we can take upto 1024 messages > in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing > more messages. This issue manifest easily when multiple CPUs > queues to same FlexRM ring (i.e. same mailbox channel). > OK then, I guess we have to make the queue length a runtime decision. BTW, is it a practical use case that needs to queue upto 1024 requests? Or are you just testing? Thanks
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brarwrote: > On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: >> Hi Jassi, >> >> Sorry for the delayed response... >> >> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: >>> Hi Anup, >>> >>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >>> wrote: The Broadcom FlexRM ring (i.e. mailbox channel) can handle larger number of messages queued in one FlexRM ring hence this patch sets msg_queue_len for each mailbox channel to be same as RING_MAX_REQ_COUNT. Signed-off-by: Anup Patel Reviewed-by: Scott Branden --- drivers/mailbox/bcm-flexrm-mailbox.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c index 9873818..20055a0 100644 --- a/drivers/mailbox/bcm-flexrm-mailbox.c +++ b/drivers/mailbox/bcm-flexrm-mailbox.c @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev) ret = -ENOMEM; goto fail_free_debugfs_root; } - for (index = 0; index < mbox->num_rings; index++) + for (index = 0; index < mbox->num_rings; index++) { + mbox->controller.chans[index].msg_queue_len = + RING_MAX_REQ_COUNT; mbox->controller.chans[index].con_priv = >rings[index]; + } >>> While writing mailbox.c I wasn't unaware that there is the option to >>> choose the queue length at runtime. >>> The idea was to keep the code as simple as possible. I am open to >>> making it a runtime thing, but first, please help me understand how >>> that is useful here. >>> >>> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >>> elements. Any message submitted to mailbox api can be immediately >>> written onto the ringbuffer if there is some space. >>> Is there any mechanism to report back to a client driver, if its >>> message in ringbuffer failed "to be sent"? >>> If there isn't any, then I think, in flexrm_last_tx_done() you should >>> simply return true if there is some space left in the rung-buffer, >>> false otherwise. >> >> Yes, we have error code in "struct brcm_message" to report back >> errors from send_message. In our mailbox clients, we check >> return value of mbox_send_message() and also the error code >> in "struct brcm_message". >> > I meant after the message has been accepted in the ringbuffer but the > remote failed to receive it. Yes, even this case is handled. In case of IO errors after message has been put in ring buffer, we get completion message with error code and mailbox client drivers will receive back "struct brcm_message" with error set. You can refer flexrm_process_completions() for more details. > There seems no such provision. IIANW, then you should be able to > consider every message as "sent successfully" once it is in the ring > buffer i.e, immediately after mbox_send_message() returns 0. > In that case I would think you don't need more than a couple of > entries out of MBOX_TX_QUEUE_LEN ? What I am trying to suggest is that we can take upto 1024 messages in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing more messages. This issue manifest easily when multiple CPUs queues to same FlexRM ring (i.e. same mailbox channel). Another quick fix is to make MBOX_TX_QUEUE_LEN as 1024 but it will not be generic fix. Regards, Anup
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar wrote: > On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: >> Hi Jassi, >> >> Sorry for the delayed response... >> >> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: >>> Hi Anup, >>> >>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel >>> wrote: The Broadcom FlexRM ring (i.e. mailbox channel) can handle larger number of messages queued in one FlexRM ring hence this patch sets msg_queue_len for each mailbox channel to be same as RING_MAX_REQ_COUNT. Signed-off-by: Anup Patel Reviewed-by: Scott Branden --- drivers/mailbox/bcm-flexrm-mailbox.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c index 9873818..20055a0 100644 --- a/drivers/mailbox/bcm-flexrm-mailbox.c +++ b/drivers/mailbox/bcm-flexrm-mailbox.c @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev) ret = -ENOMEM; goto fail_free_debugfs_root; } - for (index = 0; index < mbox->num_rings; index++) + for (index = 0; index < mbox->num_rings; index++) { + mbox->controller.chans[index].msg_queue_len = + RING_MAX_REQ_COUNT; mbox->controller.chans[index].con_priv = >rings[index]; + } >>> While writing mailbox.c I wasn't unaware that there is the option to >>> choose the queue length at runtime. >>> The idea was to keep the code as simple as possible. I am open to >>> making it a runtime thing, but first, please help me understand how >>> that is useful here. >>> >>> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >>> elements. Any message submitted to mailbox api can be immediately >>> written onto the ringbuffer if there is some space. >>> Is there any mechanism to report back to a client driver, if its >>> message in ringbuffer failed "to be sent"? >>> If there isn't any, then I think, in flexrm_last_tx_done() you should >>> simply return true if there is some space left in the rung-buffer, >>> false otherwise. >> >> Yes, we have error code in "struct brcm_message" to report back >> errors from send_message. In our mailbox clients, we check >> return value of mbox_send_message() and also the error code >> in "struct brcm_message". >> > I meant after the message has been accepted in the ringbuffer but the > remote failed to receive it. Yes, even this case is handled. In case of IO errors after message has been put in ring buffer, we get completion message with error code and mailbox client drivers will receive back "struct brcm_message" with error set. You can refer flexrm_process_completions() for more details. > There seems no such provision. IIANW, then you should be able to > consider every message as "sent successfully" once it is in the ring > buffer i.e, immediately after mbox_send_message() returns 0. > In that case I would think you don't need more than a couple of > entries out of MBOX_TX_QUEUE_LEN ? What I am trying to suggest is that we can take upto 1024 messages in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing more messages. This issue manifest easily when multiple CPUs queues to same FlexRM ring (i.e. same mailbox channel). Another quick fix is to make MBOX_TX_QUEUE_LEN as 1024 but it will not be generic fix. Regards, Anup
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Mon, Jul 24, 2017 at 9:26 AM, Anup Patelwrote: > Hi Jassi, > > Sorry for the delayed response... > > On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: >> Hi Anup, >> >> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: >>> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >>> larger number of messages queued in one FlexRM ring hence >>> this patch sets msg_queue_len for each mailbox channel to >>> be same as RING_MAX_REQ_COUNT. >>> >>> Signed-off-by: Anup Patel >>> Reviewed-by: Scott Branden >>> --- >>> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >>> b/drivers/mailbox/bcm-flexrm-mailbox.c >>> index 9873818..20055a0 100644 >>> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >>> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >>> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device >>> *pdev) >>> ret = -ENOMEM; >>> goto fail_free_debugfs_root; >>> } >>> - for (index = 0; index < mbox->num_rings; index++) >>> + for (index = 0; index < mbox->num_rings; index++) { >>> + mbox->controller.chans[index].msg_queue_len = >>> + RING_MAX_REQ_COUNT; >>> mbox->controller.chans[index].con_priv = >>> >rings[index]; >>> + } >>> >> While writing mailbox.c I wasn't unaware that there is the option to >> choose the queue length at runtime. >> The idea was to keep the code as simple as possible. I am open to >> making it a runtime thing, but first, please help me understand how >> that is useful here. >> >> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >> elements. Any message submitted to mailbox api can be immediately >> written onto the ringbuffer if there is some space. >> Is there any mechanism to report back to a client driver, if its >> message in ringbuffer failed "to be sent"? >> If there isn't any, then I think, in flexrm_last_tx_done() you should >> simply return true if there is some space left in the rung-buffer, >> false otherwise. > > Yes, we have error code in "struct brcm_message" to report back > errors from send_message. In our mailbox clients, we check > return value of mbox_send_message() and also the error code > in "struct brcm_message". > I meant after the message has been accepted in the ringbuffer but the remote failed to receive it. There seems no such provision. IIANW, then you should be able to consider every message as "sent successfully" once it is in the ring buffer i.e, immediately after mbox_send_message() returns 0. In that case I would think you don't need more than a couple of entries out of MBOX_TX_QUEUE_LEN ?
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel wrote: > Hi Jassi, > > Sorry for the delayed response... > > On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: >> Hi Anup, >> >> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: >>> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >>> larger number of messages queued in one FlexRM ring hence >>> this patch sets msg_queue_len for each mailbox channel to >>> be same as RING_MAX_REQ_COUNT. >>> >>> Signed-off-by: Anup Patel >>> Reviewed-by: Scott Branden >>> --- >>> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >>> b/drivers/mailbox/bcm-flexrm-mailbox.c >>> index 9873818..20055a0 100644 >>> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >>> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >>> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device >>> *pdev) >>> ret = -ENOMEM; >>> goto fail_free_debugfs_root; >>> } >>> - for (index = 0; index < mbox->num_rings; index++) >>> + for (index = 0; index < mbox->num_rings; index++) { >>> + mbox->controller.chans[index].msg_queue_len = >>> + RING_MAX_REQ_COUNT; >>> mbox->controller.chans[index].con_priv = >>> >rings[index]; >>> + } >>> >> While writing mailbox.c I wasn't unaware that there is the option to >> choose the queue length at runtime. >> The idea was to keep the code as simple as possible. I am open to >> making it a runtime thing, but first, please help me understand how >> that is useful here. >> >> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) >> elements. Any message submitted to mailbox api can be immediately >> written onto the ringbuffer if there is some space. >> Is there any mechanism to report back to a client driver, if its >> message in ringbuffer failed "to be sent"? >> If there isn't any, then I think, in flexrm_last_tx_done() you should >> simply return true if there is some space left in the rung-buffer, >> false otherwise. > > Yes, we have error code in "struct brcm_message" to report back > errors from send_message. In our mailbox clients, we check > return value of mbox_send_message() and also the error code > in "struct brcm_message". > I meant after the message has been accepted in the ringbuffer but the remote failed to receive it. There seems no such provision. IIANW, then you should be able to consider every message as "sent successfully" once it is in the ring buffer i.e, immediately after mbox_send_message() returns 0. In that case I would think you don't need more than a couple of entries out of MBOX_TX_QUEUE_LEN ?
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
Hi Jassi, Sorry for the delayed response... On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brarwrote: > Hi Anup, > > On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: >> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >> larger number of messages queued in one FlexRM ring hence >> this patch sets msg_queue_len for each mailbox channel to >> be same as RING_MAX_REQ_COUNT. >> >> Signed-off-by: Anup Patel >> Reviewed-by: Scott Branden >> --- >> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >> b/drivers/mailbox/bcm-flexrm-mailbox.c >> index 9873818..20055a0 100644 >> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device >> *pdev) >> ret = -ENOMEM; >> goto fail_free_debugfs_root; >> } >> - for (index = 0; index < mbox->num_rings; index++) >> + for (index = 0; index < mbox->num_rings; index++) { >> + mbox->controller.chans[index].msg_queue_len = >> + RING_MAX_REQ_COUNT; >> mbox->controller.chans[index].con_priv = >rings[index]; >> + } >> > While writing mailbox.c I wasn't unaware that there is the option to > choose the queue length at runtime. > The idea was to keep the code as simple as possible. I am open to > making it a runtime thing, but first, please help me understand how > that is useful here. > > I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) > elements. Any message submitted to mailbox api can be immediately > written onto the ringbuffer if there is some space. > Is there any mechanism to report back to a client driver, if its > message in ringbuffer failed "to be sent"? > If there isn't any, then I think, in flexrm_last_tx_done() you should > simply return true if there is some space left in the rung-buffer, > false otherwise. Yes, we have error code in "struct brcm_message" to report back errors from send_message. In our mailbox clients, we check return value of mbox_send_message() and also the error code in "struct brcm_message". The flexrm_last_tx_done() will mostly return true when it is able to write message in the FlexRM ring. It will return false only when there was no room in FlexRM ring or number of in-flight messages in FlexRM ring are 1024 (max enteries in completion queue of FlexRM ring). We started seeing issues with fixed queue length in mailbox framework when we stressed one FlexRM ring from multiple CPUs. Instead of simply increasing MBOX_TX_QUEUE_LEN, it is better to let mailbox controller driver to choose the queue length because there also Ring Manager hardware who support variable sized rings. Regards, Anup
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
Hi Jassi, Sorry for the delayed response... On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar wrote: > Hi Anup, > > On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: >> The Broadcom FlexRM ring (i.e. mailbox channel) can handle >> larger number of messages queued in one FlexRM ring hence >> this patch sets msg_queue_len for each mailbox channel to >> be same as RING_MAX_REQ_COUNT. >> >> Signed-off-by: Anup Patel >> Reviewed-by: Scott Branden >> --- >> drivers/mailbox/bcm-flexrm-mailbox.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c >> b/drivers/mailbox/bcm-flexrm-mailbox.c >> index 9873818..20055a0 100644 >> --- a/drivers/mailbox/bcm-flexrm-mailbox.c >> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c >> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device >> *pdev) >> ret = -ENOMEM; >> goto fail_free_debugfs_root; >> } >> - for (index = 0; index < mbox->num_rings; index++) >> + for (index = 0; index < mbox->num_rings; index++) { >> + mbox->controller.chans[index].msg_queue_len = >> + RING_MAX_REQ_COUNT; >> mbox->controller.chans[index].con_priv = >rings[index]; >> + } >> > While writing mailbox.c I wasn't unaware that there is the option to > choose the queue length at runtime. > The idea was to keep the code as simple as possible. I am open to > making it a runtime thing, but first, please help me understand how > that is useful here. > > I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) > elements. Any message submitted to mailbox api can be immediately > written onto the ringbuffer if there is some space. > Is there any mechanism to report back to a client driver, if its > message in ringbuffer failed "to be sent"? > If there isn't any, then I think, in flexrm_last_tx_done() you should > simply return true if there is some space left in the rung-buffer, > false otherwise. Yes, we have error code in "struct brcm_message" to report back errors from send_message. In our mailbox clients, we check return value of mbox_send_message() and also the error code in "struct brcm_message". The flexrm_last_tx_done() will mostly return true when it is able to write message in the FlexRM ring. It will return false only when there was no room in FlexRM ring or number of in-flight messages in FlexRM ring are 1024 (max enteries in completion queue of FlexRM ring). We started seeing issues with fixed queue length in mailbox framework when we stressed one FlexRM ring from multiple CPUs. Instead of simply increasing MBOX_TX_QUEUE_LEN, it is better to let mailbox controller driver to choose the queue length because there also Ring Manager hardware who support variable sized rings. Regards, Anup
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
Hi Anup, On Fri, Jul 21, 2017 at 12:25 PM, Anup Patelwrote: > The Broadcom FlexRM ring (i.e. mailbox channel) can handle > larger number of messages queued in one FlexRM ring hence > this patch sets msg_queue_len for each mailbox channel to > be same as RING_MAX_REQ_COUNT. > > Signed-off-by: Anup Patel > Reviewed-by: Scott Branden > --- > drivers/mailbox/bcm-flexrm-mailbox.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c > b/drivers/mailbox/bcm-flexrm-mailbox.c > index 9873818..20055a0 100644 > --- a/drivers/mailbox/bcm-flexrm-mailbox.c > +++ b/drivers/mailbox/bcm-flexrm-mailbox.c > @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device > *pdev) > ret = -ENOMEM; > goto fail_free_debugfs_root; > } > - for (index = 0; index < mbox->num_rings; index++) > + for (index = 0; index < mbox->num_rings; index++) { > + mbox->controller.chans[index].msg_queue_len = > + RING_MAX_REQ_COUNT; > mbox->controller.chans[index].con_priv = >rings[index]; > + } > While writing mailbox.c I wasn't unaware that there is the option to choose the queue length at runtime. The idea was to keep the code as simple as possible. I am open to making it a runtime thing, but first, please help me understand how that is useful here. I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) elements. Any message submitted to mailbox api can be immediately written onto the ringbuffer if there is some space. Is there any mechanism to report back to a client driver, if its message in ringbuffer failed "to be sent"? If there isn't any, then I think, in flexrm_last_tx_done() you should simply return true if there is some space left in the rung-buffer, false otherwise. Thanks
Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
Hi Anup, On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel wrote: > The Broadcom FlexRM ring (i.e. mailbox channel) can handle > larger number of messages queued in one FlexRM ring hence > this patch sets msg_queue_len for each mailbox channel to > be same as RING_MAX_REQ_COUNT. > > Signed-off-by: Anup Patel > Reviewed-by: Scott Branden > --- > drivers/mailbox/bcm-flexrm-mailbox.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c > b/drivers/mailbox/bcm-flexrm-mailbox.c > index 9873818..20055a0 100644 > --- a/drivers/mailbox/bcm-flexrm-mailbox.c > +++ b/drivers/mailbox/bcm-flexrm-mailbox.c > @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device > *pdev) > ret = -ENOMEM; > goto fail_free_debugfs_root; > } > - for (index = 0; index < mbox->num_rings; index++) > + for (index = 0; index < mbox->num_rings; index++) { > + mbox->controller.chans[index].msg_queue_len = > + RING_MAX_REQ_COUNT; > mbox->controller.chans[index].con_priv = >rings[index]; > + } > While writing mailbox.c I wasn't unaware that there is the option to choose the queue length at runtime. The idea was to keep the code as simple as possible. I am open to making it a runtime thing, but first, please help me understand how that is useful here. I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024) elements. Any message submitted to mailbox api can be immediately written onto the ringbuffer if there is some space. Is there any mechanism to report back to a client driver, if its message in ringbuffer failed "to be sent"? If there isn't any, then I think, in flexrm_last_tx_done() you should simply return true if there is some space left in the rung-buffer, false otherwise. Thanks