| Message ID | 1509553964-4451-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | mailbox: add support for doorbell/signal mode controllers | expand |
On Wed, Nov 1, 2017 at 10:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > Such controllers don't need to transmit any data, they just transmit > the signal. In such controllers the data pointer passed to > mbox_send_message is passed to client via it's tx_prepare callback. > Controller doesn't need any data to be passed from the client. > Some controllers need a non-zero value written to a register in order to trigger the signal. That register is visible to the remote. While the data/packet is setup during tx_prepare() callback. You are overlooking this class of doorbell controllers. > > This is rough idea I have on extending mailbox interface to support > the doorbell requirements. > What doorbell requirements does the api not support? QComm's APCS IPC is what you call a "doorbell" controller and is already supported by the API. It could run SCMI even easier than MHU (your controller). > The new API send_signal will eliminate the > issue Jassi has explained in earlier discussion with respect to generic > message format using Rockchip example. > Sorry I don't see how. Please explain how can send_signal() api be used by, say, rockchip to support SCMI? I am not convinced we should clone an api just so that a client driver becomes simpler. Esp when it shifts, and not avoid, the additional code (to support the client) onto the provider side. Thanks.
On 01/11/17 18:03, Jassi Brar wrote: > On Wed, Nov 1, 2017 at 10:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> >> Such controllers don't need to transmit any data, they just transmit >> the signal. In such controllers the data pointer passed to >> mbox_send_message is passed to client via it's tx_prepare callback. >> Controller doesn't need any data to be passed from the client. >> > Some controllers need a non-zero value written to a register in order > to trigger the signal. You are right, just right non-zero or whatever controller value to trigger the interrupt to remote. > That register is visible to the remote. While the data/packet is setup > during tx_prepare() callback. Agreed. > You are overlooking this class of doorbell controllers. > Not sure what do you mean by that ? >> >> This is rough idea I have on extending mailbox interface to support >> the doorbell requirements. >> > What doorbell requirements does the api not support? > QComm's APCS IPC is what you call a "doorbell" controller and is > already supported by the API. It could run SCMI even easier than MHU > (your controller). > Again agreed. But see below for reason to create this API. >> The new API send_signal will eliminate the >> issue Jassi has explained in earlier discussion with respect to generic >> message format using Rockchip example. >> > Sorry I don't see how. > Please explain how can send_signal() api be used by, say, rockchip to > support SCMI? > 80 writel_relaxed(msg->cmd, mb->mbox_base + MAILBOX_A2B_CMD(chans->idx)); 81 writel_relaxed(msg->rx_size, mb->mbox_base + 82 MAILBOX_A2B_DAT(chans->idx)); 83 will be replaced with writel(whatever_value_to trigger_signal, MAILBOX_A2B_CMD(chans->idx)); in its send_signal function. > I am not convinced we should clone an api just so that a client driver > becomes simpler. Esp when it shifts, and not avoid, the additional > code (to support the client) onto the provider side. > It doesn't tie the data format with particular mailbox controller. send_data has void *data and the interpretation is controller specific. send_signal on the other handle can implemented by the controllers which knows how and can trigger the specific signal to the remote. -- Regards, Sudeep
On Wed 01 Nov 11:03 PDT 2017, Jassi Brar wrote: > On Wed, Nov 1, 2017 at 10:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: [..] > > > > This is rough idea I have on extending mailbox interface to support > > the doorbell requirements. > > > What doorbell requirements does the api not support? > QComm's APCS IPC is what you call a "doorbell" controller and is > already supported by the API. It could run SCMI even easier than MHU > (your controller). > I agree; from a mbox consumer perspective a doorbell should be a mailbox channel that when signalled will ring the bell, i.e. the message is not significant and should not be provided by the client. If the message is significant and is not derived from the mailbox channel (e.g. channel id -> bit in register) it is not a mailbox doorbell, it's s regular mailbox used as a doorbell. The potential improvement I see in the Qualcomm case is to wrap the mbox_send_message(chan, NULL); mbox_client_txdone(chan, 0); calls in one simple "mbox_ring_door_bell(chan)" - but I haven't investigated the validity of this as a generic function. Regards, Bjorn
On Wed 01 Nov 11:15 PDT 2017, Sudeep Holla wrote: > > 80 writel_relaxed(msg->cmd, mb->mbox_base + > MAILBOX_A2B_CMD(chans->idx)); > 81 writel_relaxed(msg->rx_size, mb->mbox_base + > > 82 MAILBOX_A2B_DAT(chans->idx)); > > 83 This is just terrible, using the void *mssg to pass a struct which is interpreted by the controller removes any form of abstraction provided by the framework. In my view the void *mssg should point to the data to be written in the mailbox register, and hence might be of different size - but only of native type. Regards, Bjorn
On Wed, Nov 1, 2017 at 11:45 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 01/11/17 18:03, Jassi Brar wrote: >> On Wed, Nov 1, 2017 at 10:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >>> >>> Such controllers don't need to transmit any data, they just transmit >>> the signal. In such controllers the data pointer passed to >>> mbox_send_message is passed to client via it's tx_prepare callback. >>> Controller doesn't need any data to be passed from the client. >>> >> Some controllers need a non-zero value written to a register in order >> to trigger the signal. > > You are right, just right non-zero or whatever controller value to > trigger the interrupt to remote. > >> That register is visible to the remote. While the data/packet is setup >> during tx_prepare() callback. > > Agreed. > >> You are overlooking this class of doorbell controllers. >> > > Not sure what do you mean by that ? > Such doorbell controllers can't use send_signal(chan) because they need that non-zero value from client to send over the shared register. You are assuming every protocol implements just one command. >>> >>> This is rough idea I have on extending mailbox interface to support >>> the doorbell requirements. >>> >> What doorbell requirements does the api not support? >> QComm's APCS IPC is what you call a "doorbell" controller and is >> already supported by the API. It could run SCMI even easier than MHU >> (your controller). >> > > Again agreed. But see below for reason to create this API. > >>> The new API send_signal will eliminate the >>> issue Jassi has explained in earlier discussion with respect to generic >>> message format using Rockchip example. >>> >> Sorry I don't see how. >> Please explain how can send_signal() api be used by, say, rockchip to >> support SCMI? >> > > 80 writel_relaxed(msg->cmd, mb->mbox_base + > MAILBOX_A2B_CMD(chans->idx)); > 81 writel_relaxed(msg->rx_size, mb->mbox_base + > > 82 MAILBOX_A2B_DAT(chans->idx)); > > 83 > > will be replaced with > > writel(whatever_value_to trigger_signal, MAILBOX_A2B_CMD(chans->idx)); > > in its send_signal function. > 1) Where does the "whatever_value_to_trigger_signal" come from? That has to come from client. You can not dictate the channel transfers a fixed u32 value over its lifetime. SCMI may use one command code but other protocols use more. 2) Using 'rx_size' is not a software choice made in the driver. The _hardware_ has two registers shared with remote side - a CMD and a DATA register. So the driver (written agnostic to any particular client) would naturally expect the command+data from the client to be programmed in to CMD and DAT registers. >> I am not convinced we should clone an api just so that a client driver >> becomes simpler. Esp when it shifts, and not avoid, the additional >> code (to support the client) onto the provider side. >> > > It doesn't tie the data format with particular mailbox controller. > send_data has void *data and the interpretation is controller specific. > send_signal on the other handle can implemented by the controllers which > knows how and can trigger the specific signal to the remote. > Yeah that's what I said - you want to make a client simpler by pushing the code requirement onto the provider side. For example, you mean we modify the provider rockchip-mailbox.c by implementing rockchip_send_signal(chan) { struct rockchip_mbox_msg msg; msg.cmd = chan->idx; //only one command supported by the channel !!! msg.rx_size = 0; rockchip_send_data(chan, (void*) &msg); } whereas I suggest this SCMI specific code should be part of transport/mapping shim layer of SCMI.
On Thu, Nov 2, 2017 at 3:42 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Wed 01 Nov 11:03 PDT 2017, Jassi Brar wrote: >> On Wed, Nov 1, 2017 at 10:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > [..] >> > >> > This is rough idea I have on extending mailbox interface to support >> > the doorbell requirements. >> > >> What doorbell requirements does the api not support? >> QComm's APCS IPC is what you call a "doorbell" controller and is >> already supported by the API. It could run SCMI even easier than MHU >> (your controller). >> > > I agree; from a mbox consumer perspective a doorbell should be a mailbox > channel that when signalled will ring the bell, i.e. the message is not > significant and should not be provided by the client. > > If the message is significant and is not derived from the mailbox > channel (e.g. channel id -> bit in register) it is not a mailbox > doorbell, it's s regular mailbox used as a doorbell. > > > The potential improvement I see in the Qualcomm case is to wrap the > mbox_send_message(chan, NULL); mbox_client_txdone(chan, 0); calls in one > simple "mbox_ring_door_bell(chan)" - but I haven't investigated the > validity of this as a generic function. > If you remember I already suggested we can use the existing api to define a convenient helper -> https://www.spinics.net/lists/linux-soc/msg01781.html mailbox_client.h ******************* void mbox_ring_doorbell(struct mbox_chan *chan, void *mssg) { (void)mbox_send_message(chan, mssg); mbox_client_txdone(chan, 0); } .... instead of adding a new api and modifying each driver. Cheers!
On Thu, Nov 2, 2017 at 3:47 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Wed 01 Nov 11:15 PDT 2017, Sudeep Holla wrote: >> >> 80 writel_relaxed(msg->cmd, mb->mbox_base + >> MAILBOX_A2B_CMD(chans->idx)); >> 81 writel_relaxed(msg->rx_size, mb->mbox_base + >> >> 82 MAILBOX_A2B_DAT(chans->idx)); >> >> 83 > > This is just terrible, using the void *mssg to pass a struct which is > interpreted by the controller removes any form of abstraction provided > by the framework. > > In my view the void *mssg should point to the data to be written in the > mailbox register, and hence might be of different size - but only of > native type. > Please note the terrible 'rx_size' is not a software option - the hardware has a DAT register so the driver rightfully allows a client to write a value in that as well.
On Wed 01 Nov 20:02 PDT 2017, Jassi Brar wrote: > On Thu, Nov 2, 2017 at 3:47 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Wed 01 Nov 11:15 PDT 2017, Sudeep Holla wrote: > >> > >> 80 writel_relaxed(msg->cmd, mb->mbox_base + > >> MAILBOX_A2B_CMD(chans->idx)); > >> 81 writel_relaxed(msg->rx_size, mb->mbox_base + > >> > >> 82 MAILBOX_A2B_DAT(chans->idx)); > >> > >> 83 > > > > This is just terrible, using the void *mssg to pass a struct which is > > interpreted by the controller removes any form of abstraction provided > > by the framework. > > > > In my view the void *mssg should point to the data to be written in the > > mailbox register, and hence might be of different size - but only of > > native type. > > > Please note the terrible 'rx_size' is not a software option - the > hardware has a DAT register so the driver rightfully allows a client > to write a value in that as well. Okay, so the interface exposed by the mailbox driver is not { cmd, rx_size } but rather __le32 data[2], or perhaps a generic { cmd, data }. That sounds more generic. I think it would be good to replace the totally opaque void * with some sort of structured data type and having the framework ensure that clients and controllers are compatible. That would, by design, allow for reuse between controllers and clients. Perhaps something like: enum mbox_msg_type { MBOX_MSG_TYPE_NULL, MBOX_MSG_TYPE_U32, MBOX_MSG_TYPE_CMD_DATA, }; struct mbox_msg { enum mbox_msg_type type; union { u32 u; struct { u32 cmd; u32 data; } cd; }; }; And have the controller register for a specific "type", so the framework could validate that the type of data being passed matches the hardware. Have you had any thoughts around this before? Regards, Bjorn
On Thu, Nov 2, 2017 at 8:57 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Wed 01 Nov 20:02 PDT 2017, Jassi Brar wrote: > >> On Thu, Nov 2, 2017 at 3:47 AM, Bjorn Andersson >> <bjorn.andersson@linaro.org> wrote: >> > On Wed 01 Nov 11:15 PDT 2017, Sudeep Holla wrote: >> >> >> >> 80 writel_relaxed(msg->cmd, mb->mbox_base + >> >> MAILBOX_A2B_CMD(chans->idx)); >> >> 81 writel_relaxed(msg->rx_size, mb->mbox_base + >> >> >> >> 82 MAILBOX_A2B_DAT(chans->idx)); >> >> >> >> 83 >> > >> > This is just terrible, using the void *mssg to pass a struct which is >> > interpreted by the controller removes any form of abstraction provided >> > by the framework. >> > >> > In my view the void *mssg should point to the data to be written in the >> > mailbox register, and hence might be of different size - but only of >> > native type. >> > >> Please note the terrible 'rx_size' is not a software option - the >> hardware has a DAT register so the driver rightfully allows a client >> to write a value in that as well. > > Okay, so the interface exposed by the mailbox driver is not { cmd, > rx_size } but rather __le32 data[2], or perhaps a generic { cmd, data }. > That sounds more generic. > > > I think it would be good to replace the totally opaque void * with some > sort of structured data type and having the framework ensure that > clients and controllers are compatible. That would, by design, allow for > reuse between controllers and clients. > > Perhaps something like: > > enum mbox_msg_type { > MBOX_MSG_TYPE_NULL, > MBOX_MSG_TYPE_U32, > MBOX_MSG_TYPE_CMD_DATA, > }; > > struct mbox_msg { > enum mbox_msg_type type; > > union { > u32 u; > struct { > u32 cmd; > u32 data; > } cd; > }; > }; > > And have the controller register for a specific "type", so the framework > could validate that the type of data being passed matches the hardware. > > Have you had any thoughts around this before? > Yes. Different controllers have different capabilities... some have 32bits data register, some have 4x32bits deep fifos and some 8x32bits deep.... while some traverse descriptor rings. Even with all these termed 'standard' options, the clients still have to understand the underlying controller and what the remote expects it to behave and do very platform specific tasks. So mailbox clients are inherently difficult to be reusable on other platforms. cheers
On 02/11/17 02:39, Jassi Brar wrote: > On Wed, Nov 1, 2017 at 11:45 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 01/11/17 18:03, Jassi Brar wrote: >>> On Wed, Nov 1, 2017 at 10:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >>>> >>>> Such controllers don't need to transmit any data, they just transmit >>>> the signal. In such controllers the data pointer passed to >>>> mbox_send_message is passed to client via it's tx_prepare callback. >>>> Controller doesn't need any data to be passed from the client. >>>> >>> Some controllers need a non-zero value written to a register in order >>> to trigger the signal. >> >> You are right, just right non-zero or whatever controller value to >> trigger the interrupt to remote. >> >>> That register is visible to the remote. While the data/packet is setup >>> during tx_prepare() callback. >> >> Agreed. >> >>> You are overlooking this class of doorbell controllers. >>> >> >> Not sure what do you mean by that ? >> > Such doorbell controllers can't use send_signal(chan) because they > need that non-zero value from client to send over the shared register. > You are assuming every protocol implements just one command. > No that non-zero value is not client specific, it's entirely controller specific. Not sure why do you think I am assuming every protocol implements just one command. >>>> >>>> This is rough idea I have on extending mailbox interface to support >>>> the doorbell requirements. >>>> >>> What doorbell requirements does the api not support? >>> QComm's APCS IPC is what you call a "doorbell" controller and is >>> already supported by the API. After looking at this, you will see that doorbell has not data specific to client in the above case. unsigned long idx = (unsigned long)chan->con_priv; writel(BIT(idx), apcs->reg); So it's channel specific, same in mailbox-sti >> Again agreed. But see below for reason to create this API. >> >>>> The new API send_signal will eliminate the >>>> issue Jassi has explained in earlier discussion with respect to generic >>>> message format using Rockchip example. >>>> >>> Sorry I don't see how. >>> Please explain how can send_signal() api be used by, say, rockchip to >>> support SCMI? >>> >> >> 80 writel_relaxed(msg->cmd, mb->mbox_base + >> MAILBOX_A2B_CMD(chans->idx)); >> 81 writel_relaxed(msg->rx_size, mb->mbox_base + >> >> 82 MAILBOX_A2B_DAT(chans->idx)); >> >> 83 >> >> will be replaced with >> >> writel(whatever_value_to trigger_signal, MAILBOX_A2B_CMD(chans->idx)); >> >> in its send_signal function. >> > 1) Where does the "whatever_value_to_trigger_signal" come from? Controller specific. > That has to come from client. No. > You can not dictate the channel transfers a fixed u32 value over its >lifetime. SCMI may use one command code but other protocols use more. Yes if it's just a doorbell, see the above 2 cases I have pointed out. > > 2) Using 'rx_size' is not a software choice made in the driver. The > _hardware_ has two registers shared with remote side - a CMD and a > DATA register. So the driver (written agnostic to any particular > client) would naturally expect the command+data from the client to be > programmed in to CMD and DAT registers. > OK, if this controller needs to be used in doorbell mode for SCMI, we can send one fixed cmd and fixed rx_size() or 1 based on inclusive or exclusive). > >>> I am not convinced we should clone an api just so that a client driver >>> becomes simpler. Esp when it shifts, and not avoid, the additional >>> code (to support the client) onto the provider side. >>> >> >> It doesn't tie the data format with particular mailbox controller. >> send_data has void *data and the interpretation is controller specific. >> send_signal on the other handle can implemented by the controllers which >> knows how and can trigger the specific signal to the remote. >> > Yeah that's what I said - you want to make a client simpler by pushing > the code requirement onto the provider side. > No, I want to support generic case of mailbox doorbell instead of creating another unnecessary abstraction layer > For example, you mean we modify the provider rockchip-mailbox.c by implementing > > rockchip_send_signal(chan) > { > struct rockchip_mbox_msg msg; > > msg.cmd = chan->idx; //only one command supported by the channel !!! Yes, it's just a doorbell. That actual data is transmitted or shared elsewhere. This doorbell is a signal to the remote to examine that, > msg.rx_size = 0; > > rockchip_send_data(chan, (void*) &msg); > } > > whereas I suggest this SCMI specific code should be part of > transport/mapping shim layer of SCMI. > Yes that's what I did with abstraction and few think including me that it's unnecessary abstraction for such a generic use. -- Regards, Sudeep
On 01/11/17 22:12, Bjorn Andersson wrote: > On Wed 01 Nov 11:03 PDT 2017, Jassi Brar wrote: >> On Wed, Nov 1, 2017 at 10:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > [..] >>> >>> This is rough idea I have on extending mailbox interface to support >>> the doorbell requirements. >>> >> What doorbell requirements does the api not support? >> QComm's APCS IPC is what you call a "doorbell" controller and is >> already supported by the API. It could run SCMI even easier than MHU >> (your controller). >> > > I agree; from a mbox consumer perspective a doorbell should be a mailbox > channel that when signalled will ring the bell, i.e. the message is not > significant and should not be provided by the client. > Exactly. > If the message is significant and is not derived from the mailbox > channel (e.g. channel id -> bit in register) it is not a mailbox > doorbell, it's s regular mailbox used as a doorbell. > Agreed, in my case(ARM MHU) it's indeed a register bit. > > The potential improvement I see in the Qualcomm case is to wrap the > mbox_send_message(chan, NULL); mbox_client_txdone(chan, 0); calls in one > simple "mbox_ring_door_bell(chan)" - but I haven't investigated the > validity of this as a generic function. > Yes that's exactly what I want to do as we make progress with this patch. For that we find need to add send_signal(chan), instead of send_data(chan, data). -- Regards, Sudeep
On Thu, Nov 2, 2017 at 4:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 02/11/17 02:39, Jassi Brar wrote: >>>>> >>>>> Such controllers don't need to transmit any data, they just transmit >>>>> the signal. In such controllers the data pointer passed to >>>>> mbox_send_message is passed to client via it's tx_prepare callback. >>>>> Controller doesn't need any data to be passed from the client. >>>>> >>>> Some controllers need a non-zero value written to a register in order >>>> to trigger the signal. >>> >>> You are right, just right non-zero or whatever controller value to >>> trigger the interrupt to remote. >>> >>>> That register is visible to the remote. While the data/packet is setup >>>> during tx_prepare() callback. >>> >>> Agreed. >>> >>>> You are overlooking this class of doorbell controllers. >>>> >>> >>> Not sure what do you mean by that ? >>> >> Such doorbell controllers can't use send_signal(chan) because they >> need that non-zero value from client to send over the shared register. >> You are assuming every protocol implements just one command. >> > > No that non-zero value is not client specific, it's entirely controller > specific. > ?? For example BCM2835 has such a controller. Have a look at bcm2835_send_data() and let me know what is that controller specific value. >>> Again agreed. But see below for reason to create this API. >>> >>>>> The new API send_signal will eliminate the >>>>> issue Jassi has explained in earlier discussion with respect to generic >>>>> message format using Rockchip example. >>>>> >>>> Sorry I don't see how. >>>> Please explain how can send_signal() api be used by, say, rockchip to >>>> support SCMI? >>>> >>> >>> 80 writel_relaxed(msg->cmd, mb->mbox_base + >>> MAILBOX_A2B_CMD(chans->idx)); >>> 81 writel_relaxed(msg->rx_size, mb->mbox_base + >>> >>> 82 MAILBOX_A2B_DAT(chans->idx)); >>> >>> 83 >>> >>> will be replaced with >>> >>> writel(whatever_value_to trigger_signal, MAILBOX_A2B_CMD(chans->idx)); >>> >>> in its send_signal function. >>> >> 1) Where does the "whatever_value_to_trigger_signal" come from? > > Controller specific. > >> That has to come from client. > > No. > Again, let me know what does the controller expect 'val' to be writel(val, MAILBOX_A2B_CMD(chans->idx)) Your entire post is based on your assertion that the controller expects a particular non-zero value to trigger a signal, which is wrong. So lets first get that straight and not stray from the point.
On 02/11/17 11:26, Jassi Brar wrote: > On Thu, Nov 2, 2017 at 4:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: [...] >> >> No that non-zero value is not client specific, it's entirely controller >> specific. >> > ?? > For example BCM2835 has such a controller. Have a look at > bcm2835_send_data() and let me know what is that controller specific > value. > You can keep finding one or the other platform that has a deviation. Come on there are generic infrastructure support in many subsystem that are just used in one or two platforms. I hope you agree for this enhancement to the mailbox framework as it's more commonly used mode. I am not saying this patch is final, but I just want an agreement to add such a support. [...] >>> 1) Where does the "whatever_value_to_trigger_signal" come from? >> >> Controller specific. >> >>> That has to come from client. >> >> No. >> > Again, let me know what does the controller expect 'val' to be > > writel(val, MAILBOX_A2B_CMD(chans->idx)) > It depends on the controller. Whatever value that can generate a signal to remote. > > Your entire post is based on your assertion that the controller > expects a particular non-zero value to trigger a signal, which is > wrong. Why do you think that ? There are lots of example in the mailbox today. Please have a look at few example which don't use data passed from the client: 1. pcc_send_data (drivers/mailbox/pcc.c) 2. sti_mbox_send_data (drivers/mailbox/mailbox-sti.c) 3. qcom_apcs_ipc_send_data (drivers/mailbox/qcom-apcs-ipc-mailbox.c) 4. tegra_hsp_doorbell_send_data (drivers/mailbox/tegra-hsp.c) And SCMI fits the above case. Also you keep saying I am making this change to get SCMI with ARM MHU. Honestly I don't care much about that, I need better support from mailbox framework if possible for any platforms running SCMI. So please stop assuming my changes are motivated by that. SCMI is designed to solve more generic consolidation issues, so I am more focused on that than getting it run on some development platform I have with ARM MHU. Believe me that's least of my concern. -- Regards, Sudeep
On Thu, Nov 2, 2017 at 5:19 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 02/11/17 11:26, Jassi Brar wrote: >>>> 1) Where does the "whatever_value_to_trigger_signal" come from? >>> >>> Controller specific. >>> >>>> That has to come from client. >>> >>> No. >>> >> Again, let me know what does the controller expect 'val' to be >> >> writel(val, MAILBOX_A2B_CMD(chans->idx)) >> > > It depends on the controller. Whatever value that can generate a signal > to remote. > As you _know_, the controller expects any non-zero value. Now what value would you write in there? > > 1. pcc_send_data (drivers/mailbox/pcc.c) > 2. sti_mbox_send_data (drivers/mailbox/mailbox-sti.c) > 3. qcom_apcs_ipc_send_data (drivers/mailbox/qcom-apcs-ipc-mailbox.c) > 4. tegra_hsp_doorbell_send_data (drivers/mailbox/tegra-hsp.c) > > And SCMI fits the above case. > These are only 4 out of 14. Can we overlook that your implementation rules out 70% controllers. BTW these 4 don't even need any send_signal() api, they would remain unchanged. What's the new api for?
On 02/11/17 12:21, Jassi Brar wrote: > On Thu, Nov 2, 2017 at 5:19 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 02/11/17 11:26, Jassi Brar wrote: > >>>>> 1) Where does the "whatever_value_to_trigger_signal" come from? >>>> >>>> Controller specific. >>>> >>>>> That has to come from client. >>>> >>>> No. >>>> >>> Again, let me know what does the controller expect 'val' to be >>> >>> writel(val, MAILBOX_A2B_CMD(chans->idx)) >>> >> >> It depends on the controller. Whatever value that can generate a signal >> to remote. >> > As you _know_, the controller expects any non-zero value. Now what > value would you write in there? > I just said its *non-zero value* to give example. What action needs to be done to trigger the doorbell is *entirely* controller specific and typically it's a bit in the register. >> >> 1. pcc_send_data (drivers/mailbox/pcc.c) >> 2. sti_mbox_send_data (drivers/mailbox/mailbox-sti.c) >> 3. qcom_apcs_ipc_send_data (drivers/mailbox/qcom-apcs-ipc-mailbox.c) >> 4. tegra_hsp_doorbell_send_data (drivers/mailbox/tegra-hsp.c) >> >> And SCMI fits the above case. >> > These are only 4 out of 14. Can we overlook that your implementation > rules out 70% controllers. > I am *not* saying we will break other 10 controllers. All I am says there are 4 controllers that can make use of this new feature. 4 is good number IMO to generalize something. > BTW these 4 don't even need any send_signal() api, they would remain > unchanged. What's the new api for? > Sure, it's working fine doesn't mean it can't be used at all. That's not a right argument TBH. -- Regards, Sudeep
On Thu, Nov 2, 2017 at 6:07 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 02/11/17 12:21, Jassi Brar wrote: >> On Thu, Nov 2, 2017 at 5:19 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> On 02/11/17 11:26, Jassi Brar wrote: >> >>>>>> 1) Where does the "whatever_value_to_trigger_signal" come from? >>>>> >>>>> Controller specific. >>>>> >>>>>> That has to come from client. >>>>> >>>>> No. >>>>> >>>> Again, let me know what does the controller expect 'val' to be >>>> >>>> writel(val, MAILBOX_A2B_CMD(chans->idx)) >>>> >>> >>> It depends on the controller. Whatever value that can generate a signal >>> to remote. >>> >> As you _know_, the controller expects any non-zero value. Now what >> value would you write in there? >> > > I just said its *non-zero value* to give example. What action needs to > be done to trigger the doorbell is *entirely* controller specific and > typically it's a bit in the register. > Please read the above full context and see how you wasted 4 posts just because you had to refute my any explanation. >>> >>> 1. pcc_send_data (drivers/mailbox/pcc.c) >>> 2. sti_mbox_send_data (drivers/mailbox/mailbox-sti.c) >>> 3. qcom_apcs_ipc_send_data (drivers/mailbox/qcom-apcs-ipc-mailbox.c) >>> 4. tegra_hsp_doorbell_send_data (drivers/mailbox/tegra-hsp.c) >>> >>> And SCMI fits the above case. >>> >> These are only 4 out of 14. Can we overlook that your implementation >> rules out 70% controllers. >> > > I am *not* saying we will break other 10 controllers. All I am says > there are 4 controllers that can make use of this new feature. 4 is good > number IMO to generalize something. > If all you want to support is these 4 controllers why mess with the api?! These 4 controllers don't use the data pointer, just use the existing API as such mbox_send_message(chan, NULL); >> BTW these 4 don't even need any send_signal() api, they would remain >> unchanged. What's the new api for? >> > > Sure, it's working fine doesn't mean it can't be used at all. That's not > a right argument TBH. > API real estate is very precious. No redundancy should be added, unless absolutely necessary.
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index 674b35f402f5..495b4574b954 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -77,7 +77,10 @@ static void msg_submit(struct mbox_chan *chan) if (chan->cl->tx_prepare) chan->cl->tx_prepare(chan->cl, data); /* Try to submit a message to the MBOX controller */ - err = chan->mbox->ops->send_data(chan, data); + if (chan->mbox->ops->send_data) + err = chan->mbox->ops->send_data(chan, data); + else + err = chan->mbox->ops->send_signal(chan); if (!err) { chan->active_req = data; chan->msg_count--; @@ -451,6 +454,12 @@ int mbox_controller_register(struct mbox_controller *mbox) /* Sanity check */ if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) return -EINVAL; + /* + * A controller can support either doorbell mode or normal message + * transmission mode but not both + */ + if (mbox->ops->send_data && mbox->ops->send_signal) + return -EINVAL; if (mbox->txdone_irq) txdone = TXDONE_BY_IRQ; diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h index 74deadb42d76..bdbc5b74097e 100644 --- a/include/linux/mailbox_controller.h +++ b/include/linux/mailbox_controller.h @@ -24,6 +24,16 @@ struct mbox_chan; * transmission of data is reported by the controller via * mbox_chan_txdone (if it has some TX ACK irq). It must not * sleep. + * @send_signal: The API asks the MBOX controller driver, in atomic + * context try to transmit a signal on the bus. Returns 0 if + * data is accepted for transmission, -EBUSY while rejecting + * if the remote hasn't yet absorbed the last signal sent. Actual + * transmission of data must be handled by the client and is + * reported by the controller via mbox_chan_txdone (if it has + * some TX ACK irq). It must not sleep. Unlike send_data, + * send_signal doesn't handle any messages/data. It just sends + * notification signal(doorbell) and client needs to prepare all + * the data. * @startup: Called when a client requests the chan. The controller * could ask clients for additional parameters of communication * to be provided via client's chan_data. This call may @@ -46,6 +56,7 @@ struct mbox_chan; */ struct mbox_chan_ops { int (*send_data)(struct mbox_chan *chan, void *data); + int (*send_signal)(struct mbox_chan *chan); int (*startup)(struct mbox_chan *chan); void (*shutdown)(struct mbox_chan *chan); bool (*last_tx_done)(struct mbox_chan *chan);
Some mailbox controllers are lack FIFOs or memory to transmit data. They typically contains single doorbell registers to just signal the remote. The actually data is transmitted/shared using some shared memory which is not part of the mailbox. Such controllers don't need to transmit any data, they just transmit the signal. In such controllers the data pointer passed to mbox_send_message is passed to client via it's tx_prepare callback. Controller doesn't need any data to be passed from the client. This patch introduce the new API send_signal to support such doorbell/ signal mode in mailbox controllers. This is useful to avoid another layer of abstraction as typically multiple channels can be multiplexied into single register. Cc: Jassi Brar <jassisinghbrar@gmail.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/mailbox/mailbox.c | 11 ++++++++++- include/linux/mailbox_controller.h | 11 +++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) Hi Jassi, Arnd, This is rough idea I have on extending mailbox interface to support the doorbell requirements. The new API send_signal will eliminate the issue Jassi has explained in earlier discussion with respect to generic message format using Rockchip example. Regards, Sudeep -- 2.7.4