| Message ID | 1506604306-20739-17-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: ARM System Control and Management Interface(SCMI) support | expand |
On 04/10/17 12:36, Arnd Bergmann wrote: > On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> This patch adds ARM MHU specific mailbox interface for SCMI. >> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > This clearly needs an explanation why we need another driver. > Yes, I understand. I myself is not convinced that we need one. Jassi disagrees to add support to use each bit as doorbell in the ARM MHU driver though the MHU specification states it clearly(3.4.4 Message Handling Unit (MHU) of Juno TRM)[1]. This shim was added to deal with that with I agree is wrong IMO but Jassi thinks that's right way to deal with it. -- Regards, Sudeep [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0515f/DDI0515F_juno_arm_development_platform_soc_trm.pdf -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 5:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> This patch adds ARM MHU specific mailbox interface for SCMI. >> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > This clearly needs an explanation why we need another driver. > Yes the patch needs explanation which is that we need a shim layer to map SCMI requests onto what the underlying controller expects. The alternative was to clone the controller driver (MHU now and others later when their platforms support SCMI) and pretend SCMI is the only client they are ever going to serve. BTW, I haven't reviewed this patchset yet so I am not sure about this code but I do believe we need a transport layer (this shim driver) between generic SCMI implementation and each controller driver. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 7:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 06/10/17 12:26, Jassi Brar wrote: >> On Wed, Oct 4, 2017 at 5:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> This patch adds ARM MHU specific mailbox interface for SCMI. >>>> >>>> Cc: Arnd Bergmann <arnd@arndb.de> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> >>> This clearly needs an explanation why we need another driver. >>> >> Yes the patch needs explanation which is that we need a shim layer to >> map SCMI requests onto what the underlying controller expects. The >> alternative was to clone the controller driver (MHU now and others >> later when their platforms support SCMI) and pretend SCMI is the only >> client they are ever going to serve. >> > > Again that's not the point, doorbell is more common feature and that can > be supported. As SCMI expects doorbell feature in the specification, it > just need to support that class of controllers. > NO. All SCMI expects is SHMEM and a signal reaching the other end. The signal mechanism need not necessarily be "doorbell". -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/10/17 14:47, Jassi Brar wrote: > On Fri, Oct 6, 2017 at 7:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 06/10/17 12:26, Jassi Brar wrote: >>> On Wed, Oct 4, 2017 at 5:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>>> On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>>> This patch adds ARM MHU specific mailbox interface for SCMI. >>>>> >>>>> Cc: Arnd Bergmann <arnd@arndb.de> >>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>>> >>>> This clearly needs an explanation why we need another driver. >>>> >>> Yes the patch needs explanation which is that we need a shim layer to >>> map SCMI requests onto what the underlying controller expects. The >>> alternative was to clone the controller driver (MHU now and others >>> later when their platforms support SCMI) and pretend SCMI is the only >>> client they are ever going to serve. >>> >> >> Again that's not the point, doorbell is more common feature and that can >> be supported. As SCMI expects doorbell feature in the specification, it >> just need to support that class of controllers. >> > NO. All SCMI expects is SHMEM and a signal reaching the other end. > The signal mechanism need not necessarily be "doorbell". > Agreed, but creating an abstraction ro do something as generic as doorbell and writing shim layer for each controller to use SCMI also sounds bad. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 6:51 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 06/10/17 14:47, Jassi Brar wrote: >> On Fri, Oct 6, 2017 at 7:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: [..] >>> Again that's not the point, doorbell is more common feature and that can >>> be supported. As SCMI expects doorbell feature in the specification, it >>> just need to support that class of controllers. >>> >> NO. All SCMI expects is SHMEM and a signal reaching the other end. >> The signal mechanism need not necessarily be "doorbell". >> > > Agreed, but creating an abstraction ro do something as generic as > doorbell and writing shim layer for each controller to use SCMI also > sounds bad. > In the Qualcomm platform we have a single register that exposes 32 doorbells, wired to interrupts on the various processors/co-processors in the SoC. There is a handful of different clients each using these doorbells to inform the other side that something has happened (often that some piece of shared memory has been filled with data). Over the years (platforms) this register has moved around as such multiple implementations exists. You can see an example of this in drivers/mailbox/qcom-apcs-ipc-mailbox.c and e.g. drivers/rpmsg/qcom_glink_native.c (qcom_glink_rpm.c prior to v4.14). I'm struggling to figure out exactly how your case looks like, but if your doorbell exists in only one instance and there is only one client then I would suggest that it's overkill to create another driver for it. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Thanks for taking a look at this. Much appreciated. On 12/10/17 22:03, Bjorn Andersson wrote: > On Fri, Oct 6, 2017 at 6:51 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 06/10/17 14:47, Jassi Brar wrote: >>> On Fri, Oct 6, 2017 at 7:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > [..] >>>> Again that's not the point, doorbell is more common feature and that can >>>> be supported. As SCMI expects doorbell feature in the specification, it >>>> just need to support that class of controllers. >>>> >>> NO. All SCMI expects is SHMEM and a signal reaching the other end. >>> The signal mechanism need not necessarily be "doorbell". >>> >> >> Agreed, but creating an abstraction ro do something as generic as >> doorbell and writing shim layer for each controller to use SCMI also >> sounds bad. >> > > In the Qualcomm platform we have a single register that exposes 32 > doorbells, wired to interrupts on the various processors/co-processors > in the SoC. > It's exactly same even on ARM MHU controller. The hardware provides 32 independent bits in a single register termed as a single physical channel. We may have 2 -3 instances of it in a platform(secure only, high priority and a low priority). However the single register is being used to pass 32-bit data on some platforms. So we may need to support both modes. One way to deal with that is to add doorbell support in the driver as I tried previously. But the mailbox maintainer has expressed concerns on that and hence I am trying to achieve that with the abstraction as in this patch. > There is a handful of different clients each using these doorbells to > inform the other side that something has happened (often that some > piece of shared memory has been filled with data). Over the years > (platforms) this register has moved around as such multiple > implementations exists. > > You can see an example of this in > drivers/mailbox/qcom-apcs-ipc-mailbox.c and e.g. > drivers/rpmsg/qcom_glink_native.c (qcom_glink_rpm.c prior to v4.14). > > Thanks for the pointer, I have already looked at these implementations. > > I'm struggling to figure out exactly how your case looks like, but if > your doorbell exists in only one instance and there is only one client > then I would suggest that it's overkill to create another driver for > it. > While I agree with your opinion, the maintainer has concerns on trying to make this a generic solution. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 13, 2017 at 7:12 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > Hi Bjorn, > > Thanks for taking a look at this. Much appreciated. > > On 12/10/17 22:03, Bjorn Andersson wrote: >> On Fri, Oct 6, 2017 at 6:51 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >>> >>> On 06/10/17 14:47, Jassi Brar wrote: >>>> On Fri, Oct 6, 2017 at 7:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> [..] >>>>> Again that's not the point, doorbell is more common feature and that can >>>>> be supported. As SCMI expects doorbell feature in the specification, it >>>>> just need to support that class of controllers. >>>>> >>>> NO. All SCMI expects is SHMEM and a signal reaching the other end. >>>> The signal mechanism need not necessarily be "doorbell". >>>> >>> >>> Agreed, but creating an abstraction ro do something as generic as >>> doorbell and writing shim layer for each controller to use SCMI also >>> sounds bad. >>> >> >> In the Qualcomm platform we have a single register that exposes 32 >> doorbells, wired to interrupts on the various processors/co-processors >> in the SoC. >> > > It's exactly same even on ARM MHU controller. > This has been a big problem in our communication. You start with "exactly same..." and go on telling the difference. And that difference is important. In MHU the 32bits are tied together and all go to one target processor. Whereas on QCom, each bit corresponds to independent signal going to a different target processor. IOW, QCom has 32 channels per register whereas MHU has one. The current drivers reflect that reality and hence I am against any change in MHU driver. Not to mean even changing the MHU driver will fix the core issue - which is https://lkml.org/lkml/2017/7/7/465 and this patchset tries to address that. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 13, 2017 at 8:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 13/10/17 15:12, Jassi Brar wrote: > >> In MHU the 32bits are tied together and all go to one target >> processor. Whereas on QCom, each bit corresponds to independent signal >> going to a different target processor. >> > > I was not aware of that. Thanks for clarifying the differences. > >> IOW, QCom has 32 channels per register whereas MHU has one. The > > OK, that depends on how we consider it. As you said yes it just goes to > single target processor, but hardware designers consider it still 32 > channels are they can be controller independently without any locking. > MHU spec says it has three channels. Locking is not a criterion for a channel. A signal and associated data transfer defines a channel. cheers -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile index 733157c5b4e2..7fb026c71833 100644 --- a/drivers/firmware/arm_scmi/Makefile +++ b/drivers/firmware/arm_scmi/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_ARM_SCMI_PROTOCOL) = arm_scmi.o arm_scmi-y = base.o clock.o driver.o mbox_if.o perf.o power.o sensors.o +arm_scmi-$(CONFIG_ARM_MHU) += arm_mhu_if.o diff --git a/drivers/firmware/arm_scmi/arm_mhu_if.c b/drivers/firmware/arm_scmi/arm_mhu_if.c new file mode 100644 index 000000000000..2271a4811378 --- /dev/null +++ b/drivers/firmware/arm_scmi/arm_mhu_if.c @@ -0,0 +1,106 @@ +/* + * System Control and Management Interface (SCMI) MHU mailbox interface + * + * Copyright (C) 2017 ARM Ltd. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/of.h> +#include <linux/platform_device.h> + +#include "common.h" +#include "mbox_if.h" + +union mhu_data { + void *ptr; + u32 val; +}; + +static void mhu_tx_prepare(struct mbox_client *cl, void *m) +{ + struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl); + union mhu_data tmp; + + scmi_generic_tx_prepare(cinfo, m); + + /* clear only the relavant bit */ + tmp.ptr = cinfo->priv; + *(u32 *)m &= tmp.val; +} + +static void mhu_rx_callback(struct mbox_client *cl, void *m) +{ + struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl); + + scmi_generic_rx_callback(cinfo); +} + +static int mhu_mbox_request_channel(struct scmi_chan_info *cinfo, int index) +{ + int ret = 0; + struct mbox_client *cl = &cinfo->cl; + struct device_node *np = cl->dev->of_node; + union mhu_data tmp = {0}; + + if (index != 0 && index != 1) /* Tx = 0, Rx =1 */ + return -EINVAL; + + cl->rx_callback = mhu_rx_callback; + cl->tx_prepare = mhu_tx_prepare; + + ret = of_property_read_u32_index(np, "mbox-data", index, &tmp.val); + if (ret) + return ret; + + cinfo->chan = mbox_request_channel(cl, index); + if (IS_ERR(cinfo->chan)) + ret = PTR_ERR(cinfo->chan); + + cinfo->priv = tmp.ptr; + + return ret; +} + +static int mhu_mbox_send_message(struct scmi_chan_info *cinfo, void *msg) +{ + struct scmi_xfer *t = msg; + + t->con_priv = cinfo->priv; + + return mbox_send_message(cinfo->chan, msg); +} + +static void mhu_mbox_client_txdone(struct scmi_chan_info *cinfo, int r) +{ + mbox_client_txdone(cinfo->chan, r); +} + +static void mhu_mbox_free_channel(struct scmi_chan_info *cinfo) +{ + mbox_free_channel(cinfo->chan); +} + +static struct scmi_mbox_ops arm_mhu_mbox_ops = { + .request_channel = mhu_mbox_request_channel, + .send_message = mhu_mbox_send_message, + .client_txdone = mhu_mbox_client_txdone, + .free_channel = mhu_mbox_free_channel, +}; + +const struct scmi_desc mhu_scmi_desc = { + .max_rx_timeout_ms = 30, + .max_msg = 20, + .max_msg_size = 128, + .mbox_ops = &arm_mhu_mbox_ops, +}; diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 97285a22dfaa..bdc9c566e6c1 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -648,6 +648,9 @@ EXPORT_SYMBOL_GPL(devm_scmi_handle_get); /* Each compatible listed below must have descriptor associated with it */ static const struct of_device_id scmi_of_match[] = { { .compatible = "arm,scmi", .data = &scmi_generic_desc }, +#if IS_REACHABLE(CONFIG_ARM_MHU) + { .compatible = "arm,mhu-scmi", .data = &mhu_scmi_desc }, +#endif { /* Sentinel */ }, }; diff --git a/drivers/firmware/arm_scmi/mbox_if.h b/drivers/firmware/arm_scmi/mbox_if.h index 02baa73e8613..a23fc6d7a91f 100644 --- a/drivers/firmware/arm_scmi/mbox_if.h +++ b/drivers/firmware/arm_scmi/mbox_if.h @@ -66,3 +66,6 @@ void scmi_generic_rx_callback(struct scmi_chan_info *cinfo); void scmi_generic_tx_prepare(struct scmi_chan_info *cinfo, void *m); extern const struct scmi_desc scmi_generic_desc; +#if IS_REACHABLE(CONFIG_ARM_MHU) +extern const struct scmi_desc mhu_scmi_desc; +#endif
This patch adds ARM MHU specific mailbox interface for SCMI. Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scmi/Makefile | 1 + drivers/firmware/arm_scmi/arm_mhu_if.c | 106 +++++++++++++++++++++++++++++++++ drivers/firmware/arm_scmi/driver.c | 3 + drivers/firmware/arm_scmi/mbox_if.h | 3 + 4 files changed, 113 insertions(+) create mode 100644 drivers/firmware/arm_scmi/arm_mhu_if.c -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html