| Message ID | 20170503052929.17422-2-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
> -----Original Message----- > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > owner@vger.kernel.org] On Behalf Of Bjorn Andersson > Sent: Wednesday, May 03, 2017 7:29 AM > To: Andy Gross <andy.gross@linaro.org>; Rob Herring > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ohad Ben- > Cohen <ohad@wizery.com> > Cc: linux-arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > remoteproc@vger.kernel.org > Subject: [PATCH v3 2/4] soc: qcom: Introduce APCS IPC driver > > This implements a driver that exposes the IPC bits found in the APCS Global > block in various Qualcomm platforms. The bits are used to signal inter- > processor communication signals from the application CPU to other masters. > > The driver implements the "doorbell" binding and could be used as basis for a > new Linux framework, if found useful outside Qualcomm. > Hi Bjorn, Even if Qualcom APCS IPC is limited, why don't you rely on existing mailbox framework. It is there to gather all IPC management under the same interface. No need to create a new one from my pov. If you don't provide message data, mailbox framework behaves as doorbell. Regards, Loic > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Changes since v2: > - New driver > > drivers/soc/qcom/Kconfig | 8 ++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/apcs-ipc.c | 182 > ++++++++++++++++++++++++++++++++++++++ > include/linux/soc/qcom/apcs_ipc.h | 26 ++++++ > 4 files changed, 217 insertions(+) > create mode 100644 drivers/soc/qcom/apcs-ipc.c create mode 100644 > include/linux/soc/qcom/apcs_ipc.h > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index > 78b1bb7bcf20..4113da81d18b 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -1,6 +1,14 @@ > # > # QCOM Soc drivers > # > +config QCOM_APCS_IPC > + tristate "Qualcomm APCS IPC driver" > + depends on ARCH_QCOM > + help > + Say y here to enable support for the APCS IPC doorbell driver, > + providing an interface for invoking the inter-process communication > + signals from the application processor to other masters. > + > config QCOM_GSBI > tristate "QCOM General Serial Bus Interface" > depends on ARCH_QCOM > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index > 1f30260b06b8..e15b33e5a630 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -1,3 +1,4 @@ > +obj-$(CONFIG_QCOM_APCS_IPC) += apcs-ipc.o > obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o > obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o > obj-$(CONFIG_QCOM_PM) += spm.o > diff --git a/drivers/soc/qcom/apcs-ipc.c b/drivers/soc/qcom/apcs-ipc.c new > file mode 100644 index 000000000000..ea835cb08657 > --- /dev/null > +++ b/drivers/soc/qcom/apcs-ipc.c > @@ -0,0 +1,182 @@ > +/* > + * Copyright (c) 2017, Linaro Ltd > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that 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. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/io.h> > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > + > +static struct platform_driver qcom_apcs_ipc_driver; > + > +struct qcom_apcs_ipc { > + struct device *dev; > + > + void __iomem *base; > + unsigned long offset; > +}; > + > +struct qcom_apcs_ipc_bell { > + struct qcom_apcs_ipc *apcs; > + unsigned int bit; > +}; > + > +static void qcom_apcs_ipc_release(struct device *dev, void *res) { > + struct qcom_apcs_ipc_bell *bell = res; > + struct qcom_apcs_ipc *apcs = bell->apcs; > + > + put_device(apcs->dev); > +} > + > +/** > + * qcom_apcs_ipc_get() - acquire a handle to a doorbell > + * @dev: client device handle > + * @id: identifier of the doorbell > + * > + * Returns a doorbell reference, or negative errno on failure. > + */ > +struct qcom_apcs_ipc_bell *devm_qcom_apcs_ipc_get(struct device *dev, > + const char *id) > +{ > + struct qcom_apcs_ipc_bell *bell; > + struct platform_device *pdev; > + struct of_phandle_args args; > + int index = 0; > + int ret; > + > + if (id) { > + index = of_property_match_string(dev->of_node, > + "doorbell-names", id); > + if (index < 0) > + return ERR_PTR(index); > + } > + > + ret = of_parse_phandle_with_args(dev->of_node, "doorbells", > + "#doorbell-cells", index, &args); > + if (ret) { > + dev_err(dev, "unable to resolve doorbell\n"); > + return ERR_PTR(-ENODEV); > + } > + > + pdev = of_find_device_by_node(args.np); > + of_node_put(args.np); > + > + if (!pdev) > + return ERR_PTR(-EPROBE_DEFER); > + > + if (args.args[0] >= 32) { > + dev_err(dev, "invalid doorbell requested\n"); > + ret = -EINVAL; > + goto release_device; > + } > + > + if (pdev->dev.driver != &qcom_apcs_ipc_driver.driver) { > + dev_err(dev, "failed to acquire apcs ipc driver\n"); > + ret = -EINVAL; > + goto release_device; > + } > + > + bell = devres_alloc(qcom_apcs_ipc_release, sizeof(*bell), > GFP_KERNEL); > + if (!bell) { > + ret = -ENOMEM; > + goto release_device; > + } > + > + bell->apcs = platform_get_drvdata(pdev); > + bell->bit = args.args[0]; > + > + devres_add(dev, bell); > + > + return bell; > + > +release_device: > + put_device(&pdev->dev); > + > + return ERR_PTR(ret); > + > +} > +EXPORT_SYMBOL_GPL(devm_qcom_apcs_ipc_get); > + > +/** > + * qcom_apcs_ipc_ring() - ring the doorbell > + * @bell: doorbell to ring > + */ > +void qcom_apcs_ipc_ring(struct qcom_apcs_ipc_bell *bell) { > + struct qcom_apcs_ipc *apcs = bell->apcs; > + > + writel(BIT(bell->bit), apcs->base + apcs->offset); } > +EXPORT_SYMBOL_GPL(qcom_apcs_ipc_ring); > + > +static int qcom_apcs_ipc_probe(struct platform_device *pdev) { > + struct qcom_apcs_ipc *apcs; > + struct resource *res; > + > + apcs = devm_kzalloc(&pdev->dev, sizeof(*apcs), GFP_KERNEL); > + if (!apcs) > + return -ENOMEM; > + > + apcs->dev = &pdev->dev; > + apcs->offset = (unsigned long)of_device_get_match_data(&pdev- > >dev); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + apcs->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(apcs->base)) > + return PTR_ERR(apcs->base); > + > + platform_set_drvdata(pdev, apcs); > + > + return 0; > +} > + > +static int qcom_apcs_ipc_remove(struct platform_device *pdev) { > + return 0; > +} > + > +/* .data is the offset of the ipc register within the global block */ > +static const struct of_device_id qcom_apcs_ipc_of_match[] = { > + { .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 > }, > + { .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void > *)16 }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, qcom_apcs_ipc_of_match); > + > +static struct platform_driver qcom_apcs_ipc_driver = { > + .probe = qcom_apcs_ipc_probe, > + .remove = qcom_apcs_ipc_remove, > + .driver = { > + .name = "qcom_apcs_ipc", > + .of_match_table = qcom_apcs_ipc_of_match, > + }, > +}; > + > +static int __init qcom_apcs_ipc_init(void) { > + return platform_driver_register(&qcom_apcs_ipc_driver); > +} > +postcore_initcall(qcom_apcs_ipc_init); > + > +static void __exit qcom_apcs_ipc_exit(void) { > + platform_driver_unregister(&qcom_apcs_ipc_driver); > +} > +module_exit(qcom_apcs_ipc_exit); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Qualcomm APCS IPC driver"); > diff --git a/include/linux/soc/qcom/apcs_ipc.h > b/include/linux/soc/qcom/apcs_ipc.h > new file mode 100644 > index 000000000000..72be77555261 > --- /dev/null > +++ b/include/linux/soc/qcom/apcs_ipc.h > @@ -0,0 +1,26 @@ > +#ifndef __QCOM_APCS_IPC_H__ > +#define __QCOM_APCS_IPC_H__ > + > +#include <linux/err.h> > + > +struct device; > +struct qcom_apcs_ipc_bell; > + > +#if IS_ENABLED(CONFIG_QCOM_APCS_IPC) > + > +struct qcom_apcs_ipc_bell *devm_qcom_apcs_ipc_get(struct device *dev, > + const char *id); > +void qcom_apcs_ipc_ring(struct qcom_apcs_ipc_bell *bell); > + > +#else > + > +static inline struct qcom_apcs_ipc_bell *devm_qcom_apcs_ipc_get(struct > device *dev, > + const char > *id) > +{ > + return ERR_PTR(-EINVAL); > +} > + > +static inline void qcom_apcs_ipc_ring(struct qcom_apcs_ipc_bell *bell) > +{} > + > +#endif > +#endif > -- > 2.12.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org More majordomo > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Loic, thanks for adding me. On Wed, May 3, 2017 at 2:58 PM, Loic PALLARDY <loic.pallardy@st.com> wrote: > > >> -----Original Message----- >> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- >> owner@vger.kernel.org] On Behalf Of Bjorn Andersson >> Sent: Wednesday, May 03, 2017 7:29 AM >> To: Andy Gross <andy.gross@linaro.org>; Rob Herring >> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ohad Ben- >> Cohen <ohad@wizery.com> >> Cc: linux-arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux- >> remoteproc@vger.kernel.org >> Subject: [PATCH v3 2/4] soc: qcom: Introduce APCS IPC driver >> >> This implements a driver that exposes the IPC bits found in the APCS Global >> block in various Qualcomm platforms. The bits are used to signal inter- >> processor communication signals from the application CPU to other masters. >> >> The driver implements the "doorbell" binding and could be used as basis for a >> new Linux framework, if found useful outside Qualcomm. >> > Hi Bjorn, > > Even if Qualcom APCS IPC is limited, why don't you rely on existing mailbox framework. > It is there to gather all IPC management under the same interface. > No need to create a new one from my pov. > If you don't provide message data, mailbox framework behaves as doorbell. > QCOM RPM reinvented the wheel for what mailbox framework already did, despite my pointing it out => http://lkml.iu.edu/hypermail//linux/kernel/1406.2/03918.html The driver bypassed mailbox framework and was pushed via another tree. Same is being attempted now, only now it is more expensive to switch to generic mailbox framework having spent so much time on QCOM specific implementation of controller and protocol drivers inside drivers/soc/qcom/ -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 03 May 02:55 PDT 2017, Jassi Brar wrote: > Loic, thanks for adding me. > > On Wed, May 3, 2017 at 2:58 PM, Loic PALLARDY <loic.pallardy@st.com> wrote: > > > > > >> -----Original Message----- > >> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > >> owner@vger.kernel.org] On Behalf Of Bjorn Andersson > >> Sent: Wednesday, May 03, 2017 7:29 AM > >> To: Andy Gross <andy.gross@linaro.org>; Rob Herring > >> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ohad Ben- > >> Cohen <ohad@wizery.com> > >> Cc: linux-arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; > >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > >> remoteproc@vger.kernel.org > >> Subject: [PATCH v3 2/4] soc: qcom: Introduce APCS IPC driver > >> > >> This implements a driver that exposes the IPC bits found in the APCS Global > >> block in various Qualcomm platforms. The bits are used to signal inter- > >> processor communication signals from the application CPU to other masters. > >> > >> The driver implements the "doorbell" binding and could be used as basis for a > >> new Linux framework, if found useful outside Qualcomm. > >> > > Hi Bjorn, > > > > Even if Qualcom APCS IPC is limited, why don't you rely on existing mailbox framework. > > It is there to gather all IPC management under the same interface. > > No need to create a new one from my pov. > > If you don't provide message data, mailbox framework behaves as doorbell. > > > QCOM RPM reinvented the wheel for what mailbox framework already did, > despite my pointing it out => > http://lkml.iu.edu/hypermail//linux/kernel/1406.2/03918.html > The RPM interface works by writing various information in shared DRAM and then invoking an interrupt on the remote processor. What this patch does is introduce an abstraction for the invocation of that interrupt. My argumentation against using the mailbox framework for the RPM case was that the message model is a software construct and doesn't fit the mailbox framework. But the single step of invoking the remote interrupt is essentially a non-message mailbox. Perhaps this part of the RPM driver is what you're referring to in your comments on the RPM. Before "inventing" the function for acquiring a handle to a doorbell I did look at making this a mailbox controller, but as each mailbox channel related to a single bit and 1 is the only value we'll ever write it didn't feel like it matches the mailbox expectations. But as you seem to object I'll attempt to rewrite it using the mailbox framework instead. But which one of these would be appropriate for a "mailbox channel" that doesn't have any actual messages? mbox_send_message(chan, NULL) or const int one = 1; mbox_send_message(chan, (void*)&one); > The driver bypassed mailbox framework and was pushed via another tree. > Same is being attempted now, only now it is more expensive to switch > to generic mailbox framework having spent so much time on QCOM > specific implementation of controller and protocol drivers inside > drivers/soc/qcom/ I'm not sure I follow this, there's no extensive rework going on here - all I'm trying to do is abstract the "writel(BIT(x), ipc_reg)" line of the RPM driver, because it's being used in other clients as well. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 4, 2017 at 11:15 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Wed 03 May 02:55 PDT 2017, Jassi Brar wrote: > >> Loic, thanks for adding me. >> >> On Wed, May 3, 2017 at 2:58 PM, Loic PALLARDY <loic.pallardy@st.com> wrote: >> > >> > >> >> -----Original Message----- >> >> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- >> >> owner@vger.kernel.org] On Behalf Of Bjorn Andersson >> >> Sent: Wednesday, May 03, 2017 7:29 AM >> >> To: Andy Gross <andy.gross@linaro.org>; Rob Herring >> >> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ohad Ben- >> >> Cohen <ohad@wizery.com> >> >> Cc: linux-arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; >> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux- >> >> remoteproc@vger.kernel.org >> >> Subject: [PATCH v3 2/4] soc: qcom: Introduce APCS IPC driver >> >> >> >> This implements a driver that exposes the IPC bits found in the APCS Global >> >> block in various Qualcomm platforms. The bits are used to signal inter- >> >> processor communication signals from the application CPU to other masters. >> >> >> >> The driver implements the "doorbell" binding and could be used as basis for a >> >> new Linux framework, if found useful outside Qualcomm. >> >> >> > Hi Bjorn, >> > >> > Even if Qualcom APCS IPC is limited, why don't you rely on existing mailbox framework. >> > It is there to gather all IPC management under the same interface. >> > No need to create a new one from my pov. >> > If you don't provide message data, mailbox framework behaves as doorbell. >> > >> QCOM RPM reinvented the wheel for what mailbox framework already did, >> despite my pointing it out => >> http://lkml.iu.edu/hypermail//linux/kernel/1406.2/03918.html >> > > The RPM interface works by writing various information in shared DRAM > and then invoking an interrupt on the remote processor. > This is what _most_ platforms do, and they use mailbox framework. If mailbox framework doesn't suit RPM, then it doesn't suit most platforms that use it. > My argumentation against using the mailbox framework for the RPM case > was that the message model is a software construct and doesn't fit the > mailbox framework. > Mailbox framework works beneath protocol layer (software construct). As I said years ago, the h/w bits of the controller should be under mailbox api, while the message structures and signals are protocol specific and be in QCOM specific location. > Before "inventing" the function for acquiring a handle to a doorbell I > did look at making this a mailbox controller, but as each mailbox > channel related to a single bit and 1 is the only value we'll ever write > it didn't feel like it matches the mailbox expectations. But as you seem > to object I'll attempt to rewrite it using the mailbox framework > instead. > Yes, please. 1-bit messages are just a special case of N-bits messages that mailbox framework supports. > But which one of these would be appropriate for a "mailbox channel" that > doesn't have any actual messages? > > mbox_send_message(chan, NULL) > or > const int one = 1; > mbox_send_message(chan, (void*)&one); > It depends upon your h/w. If each physical channel is hardwired to work on a predefined single bit, then the driver could do without that bit being explicitly passed. If a physical channel can be mapped onto any bit(s), then you do need to pass that info via mbox_send_message(). >> The driver bypassed mailbox framework and was pushed via another tree. >> Same is being attempted now, only now it is more expensive to switch >> to generic mailbox framework having spent so much time on QCOM >> specific implementation of controller and protocol drivers inside >> drivers/soc/qcom/ > > I'm not sure I follow this, there's no extensive rework going on here - > all I'm trying to do is abstract the "writel(BIT(x), ipc_reg)" line of > the RPM driver, because it's being used in other clients as well. > No, I meant ideally RPM should have used mailbox api for the controller programming, but moving to that now will be expensive because you already developed huge code base around QCOM specific mailbox implementation. Regards. -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 04 May 00:54 PDT 2017, Jassi Brar wrote: > On Thu, May 4, 2017 at 11:15 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Wed 03 May 02:55 PDT 2017, Jassi Brar wrote: > > > >> Loic, thanks for adding me. > >> > >> On Wed, May 3, 2017 at 2:58 PM, Loic PALLARDY <loic.pallardy@st.com> wrote: > >> > > >> > > >> >> -----Original Message----- > >> >> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > >> >> owner@vger.kernel.org] On Behalf Of Bjorn Andersson > >> >> Sent: Wednesday, May 03, 2017 7:29 AM > >> >> To: Andy Gross <andy.gross@linaro.org>; Rob Herring > >> >> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ohad Ben- > >> >> Cohen <ohad@wizery.com> > >> >> Cc: linux-arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; > >> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > >> >> remoteproc@vger.kernel.org > >> >> Subject: [PATCH v3 2/4] soc: qcom: Introduce APCS IPC driver > >> >> > >> >> This implements a driver that exposes the IPC bits found in the APCS Global > >> >> block in various Qualcomm platforms. The bits are used to signal inter- > >> >> processor communication signals from the application CPU to other masters. > >> >> > >> >> The driver implements the "doorbell" binding and could be used as basis for a > >> >> new Linux framework, if found useful outside Qualcomm. > >> >> > >> > Hi Bjorn, > >> > > >> > Even if Qualcom APCS IPC is limited, why don't you rely on existing mailbox framework. > >> > It is there to gather all IPC management under the same interface. > >> > No need to create a new one from my pov. > >> > If you don't provide message data, mailbox framework behaves as doorbell. > >> > > >> QCOM RPM reinvented the wheel for what mailbox framework already did, > >> despite my pointing it out => > >> http://lkml.iu.edu/hypermail//linux/kernel/1406.2/03918.html > >> > > > > The RPM interface works by writing various information in shared DRAM > > and then invoking an interrupt on the remote processor. > > > This is what _most_ platforms do, and they use mailbox framework. If > mailbox framework doesn't suit RPM, then it doesn't suit most > platforms that use it. > > > My argumentation against using the mailbox framework for the RPM case > > was that the message model is a software construct and doesn't fit the > > mailbox framework. > > > Mailbox framework works beneath protocol layer (software construct). > As I said years ago, the h/w bits of the controller should be under > mailbox api, while the message structures and signals are protocol > specific and be in QCOM specific location. > Okay, now I finally get what you're saying. The RPM driver handles the protocol but the actual triggering of the remote interrupt should be done via the mailbox framework. That makes sense, until now I haven't seen the need for having a driver exposing the APCS IPC register (used by among others the RPM driver), but I will rewrite this patch as a mailbox controller and turn the RPM driver into a mailbox client. > > > Before "inventing" the function for acquiring a handle to a doorbell I > > did look at making this a mailbox controller, but as each mailbox > > channel related to a single bit and 1 is the only value we'll ever write > > it didn't feel like it matches the mailbox expectations. But as you seem > > to object I'll attempt to rewrite it using the mailbox framework > > instead. > > > Yes, please. 1-bit messages are just a special case of N-bits messages > that mailbox framework supports. > Yeah, but it's not even a 1-bit message, because writing 0 is a no-op. > > > But which one of these would be appropriate for a "mailbox channel" that > > doesn't have any actual messages? > > > > mbox_send_message(chan, NULL) > > or > > const int one = 1; > > mbox_send_message(chan, (void*)&one); > > > It depends upon your h/w. > If each physical channel is hardwired to work on a predefined single > bit, then the driver could do without that bit being explicitly > passed. > If a physical channel can be mapped onto any bit(s), then you do need > to pass that info via mbox_send_message(). > It's a 32-bit register, writing a bit invokes the associated interrupt on some remote processor. I.e. bits 0, 1 and 2 represents different interrupts on the resource-power-management CPU while bit 12, 13, 14 and 15 will invoke interrupts on a modem CPU. I'll rework the proposed driver and we can look at the actual implementation instead. > > >> The driver bypassed mailbox framework and was pushed via another tree. > >> Same is being attempted now, only now it is more expensive to switch > >> to generic mailbox framework having spent so much time on QCOM > >> specific implementation of controller and protocol drivers inside > >> drivers/soc/qcom/ > > > > I'm not sure I follow this, there's no extensive rework going on here - > > all I'm trying to do is abstract the "writel(BIT(x), ipc_reg)" line of > > the RPM driver, because it's being used in other clients as well. > > > No, I meant ideally RPM should have used mailbox api for the > controller programming, but moving to that now will be expensive > because you already developed huge code base around QCOM specific > mailbox implementation. > The part discussed above was implemented using syscon, so it's a few lines of code to grab hold of a handle and instead of mbox_send_message() there is a single call to regmap_write(). All the other parts of the implementation is protocol-specific, and as you say the next layer up. So the transition is quite straight forward! Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 4, 2017 at 9:40 PM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Thu 04 May 00:54 PDT 2017, Jassi Brar wrote: > >> On Thu, May 4, 2017 at 11:15 AM, Bjorn Andersson >> <bjorn.andersson@linaro.org> wrote: >> > On Wed 03 May 02:55 PDT 2017, Jassi Brar wrote: >> > >> >> Loic, thanks for adding me. >> >> >> >> On Wed, May 3, 2017 at 2:58 PM, Loic PALLARDY <loic.pallardy@st.com> wrote: >> >> > >> >> > >> >> >> -----Original Message----- >> >> >> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- >> >> >> owner@vger.kernel.org] On Behalf Of Bjorn Andersson >> >> >> Sent: Wednesday, May 03, 2017 7:29 AM >> >> >> To: Andy Gross <andy.gross@linaro.org>; Rob Herring >> >> >> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ohad Ben- >> >> >> Cohen <ohad@wizery.com> >> >> >> Cc: linux-arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; >> >> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux- >> >> >> remoteproc@vger.kernel.org >> >> >> Subject: [PATCH v3 2/4] soc: qcom: Introduce APCS IPC driver >> >> >> >> >> >> This implements a driver that exposes the IPC bits found in the APCS Global >> >> >> block in various Qualcomm platforms. The bits are used to signal inter- >> >> >> processor communication signals from the application CPU to other masters. >> >> >> >> >> >> The driver implements the "doorbell" binding and could be used as basis for a >> >> >> new Linux framework, if found useful outside Qualcomm. >> >> >> >> >> > Hi Bjorn, >> >> > >> >> > Even if Qualcom APCS IPC is limited, why don't you rely on existing mailbox framework. >> >> > It is there to gather all IPC management under the same interface. >> >> > No need to create a new one from my pov. >> >> > If you don't provide message data, mailbox framework behaves as doorbell. >> >> > >> >> QCOM RPM reinvented the wheel for what mailbox framework already did, >> >> despite my pointing it out => >> >> http://lkml.iu.edu/hypermail//linux/kernel/1406.2/03918.html >> >> >> > >> > The RPM interface works by writing various information in shared DRAM >> > and then invoking an interrupt on the remote processor. >> > >> This is what _most_ platforms do, and they use mailbox framework. If >> mailbox framework doesn't suit RPM, then it doesn't suit most >> platforms that use it. >> >> > My argumentation against using the mailbox framework for the RPM case >> > was that the message model is a software construct and doesn't fit the >> > mailbox framework. >> > >> Mailbox framework works beneath protocol layer (software construct). >> As I said years ago, the h/w bits of the controller should be under >> mailbox api, while the message structures and signals are protocol >> specific and be in QCOM specific location. >> > > Okay, now I finally get what you're saying. The RPM driver handles the > protocol but the actual triggering of the remote interrupt should be > done via the mailbox framework. > > That makes sense, until now I haven't seen the need for having a driver > exposing the APCS IPC register (used by among others the RPM driver), > but I will rewrite this patch as a mailbox controller and turn the RPM > driver into a mailbox client. > Cool, thanks. >> >> > But which one of these would be appropriate for a "mailbox channel" that >> > doesn't have any actual messages? >> > >> > mbox_send_message(chan, NULL) >> > or >> > const int one = 1; >> > mbox_send_message(chan, (void*)&one); >> > >> It depends upon your h/w. >> If each physical channel is hardwired to work on a predefined single >> bit, then the driver could do without that bit being explicitly >> passed. >> If a physical channel can be mapped onto any bit(s), then you do need >> to pass that info via mbox_send_message(). >> > > It's a 32-bit register, writing a bit invokes the associated interrupt > on some remote processor. I.e. bits 0, 1 and 2 represents different > interrupts on the resource-power-management CPU while bit 12, 13, 14 and > 15 will invoke interrupts on a modem CPU. > No problem. For such arrangement, you could have a channel per interrupt/bit. Have the DT specify which bit goes as what interrupt to which cpu. The controller driver would then associate each channel to its 'bit' and a client would always acquire the right channel (specified by DT) and need not know/pass which bit should be set for its messages i.e, mbox_send_message(chan, NULL) > I'll rework the proposed driver and we can look at the actual > implementation instead. > >> >> >> The driver bypassed mailbox framework and was pushed via another tree. >> >> Same is being attempted now, only now it is more expensive to switch >> >> to generic mailbox framework having spent so much time on QCOM >> >> specific implementation of controller and protocol drivers inside >> >> drivers/soc/qcom/ >> > >> > I'm not sure I follow this, there's no extensive rework going on here - >> > all I'm trying to do is abstract the "writel(BIT(x), ipc_reg)" line of >> > the RPM driver, because it's being used in other clients as well. >> > >> No, I meant ideally RPM should have used mailbox api for the >> controller programming, but moving to that now will be expensive >> because you already developed huge code base around QCOM specific >> mailbox implementation. >> > > The part discussed above was implemented using syscon, so it's a few > lines of code to grab hold of a handle and instead of > mbox_send_message() there is a single call to regmap_write(). All the > other parts of the implementation is protocol-specific, and as you say > the next layer up. So the transition is quite straight forward! > Great! Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 78b1bb7bcf20..4113da81d18b 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -1,6 +1,14 @@ # # QCOM Soc drivers # +config QCOM_APCS_IPC + tristate "Qualcomm APCS IPC driver" + depends on ARCH_QCOM + help + Say y here to enable support for the APCS IPC doorbell driver, + providing an interface for invoking the inter-process communication + signals from the application processor to other masters. + config QCOM_GSBI tristate "QCOM General Serial Bus Interface" depends on ARCH_QCOM diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 1f30260b06b8..e15b33e5a630 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_QCOM_APCS_IPC) += apcs-ipc.o obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o obj-$(CONFIG_QCOM_PM) += spm.o diff --git a/drivers/soc/qcom/apcs-ipc.c b/drivers/soc/qcom/apcs-ipc.c new file mode 100644 index 000000000000..ea835cb08657 --- /dev/null +++ b/drivers/soc/qcom/apcs-ipc.c @@ -0,0 +1,182 @@ +/* + * Copyright (c) 2017, Linaro Ltd + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that 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. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +static struct platform_driver qcom_apcs_ipc_driver; + +struct qcom_apcs_ipc { + struct device *dev; + + void __iomem *base; + unsigned long offset; +}; + +struct qcom_apcs_ipc_bell { + struct qcom_apcs_ipc *apcs; + unsigned int bit; +}; + +static void qcom_apcs_ipc_release(struct device *dev, void *res) +{ + struct qcom_apcs_ipc_bell *bell = res; + struct qcom_apcs_ipc *apcs = bell->apcs; + + put_device(apcs->dev); +} + +/** + * qcom_apcs_ipc_get() - acquire a handle to a doorbell + * @dev: client device handle + * @id: identifier of the doorbell + * + * Returns a doorbell reference, or negative errno on failure. + */ +struct qcom_apcs_ipc_bell *devm_qcom_apcs_ipc_get(struct device *dev, + const char *id) +{ + struct qcom_apcs_ipc_bell *bell; + struct platform_device *pdev; + struct of_phandle_args args; + int index = 0; + int ret; + + if (id) { + index = of_property_match_string(dev->of_node, + "doorbell-names", id); + if (index < 0) + return ERR_PTR(index); + } + + ret = of_parse_phandle_with_args(dev->of_node, "doorbells", + "#doorbell-cells", index, &args); + if (ret) { + dev_err(dev, "unable to resolve doorbell\n"); + return ERR_PTR(-ENODEV); + } + + pdev = of_find_device_by_node(args.np); + of_node_put(args.np); + + if (!pdev) + return ERR_PTR(-EPROBE_DEFER); + + if (args.args[0] >= 32) { + dev_err(dev, "invalid doorbell requested\n"); + ret = -EINVAL; + goto release_device; + } + + if (pdev->dev.driver != &qcom_apcs_ipc_driver.driver) { + dev_err(dev, "failed to acquire apcs ipc driver\n"); + ret = -EINVAL; + goto release_device; + } + + bell = devres_alloc(qcom_apcs_ipc_release, sizeof(*bell), GFP_KERNEL); + if (!bell) { + ret = -ENOMEM; + goto release_device; + } + + bell->apcs = platform_get_drvdata(pdev); + bell->bit = args.args[0]; + + devres_add(dev, bell); + + return bell; + +release_device: + put_device(&pdev->dev); + + return ERR_PTR(ret); + +} +EXPORT_SYMBOL_GPL(devm_qcom_apcs_ipc_get); + +/** + * qcom_apcs_ipc_ring() - ring the doorbell + * @bell: doorbell to ring + */ +void qcom_apcs_ipc_ring(struct qcom_apcs_ipc_bell *bell) +{ + struct qcom_apcs_ipc *apcs = bell->apcs; + + writel(BIT(bell->bit), apcs->base + apcs->offset); +} +EXPORT_SYMBOL_GPL(qcom_apcs_ipc_ring); + +static int qcom_apcs_ipc_probe(struct platform_device *pdev) +{ + struct qcom_apcs_ipc *apcs; + struct resource *res; + + apcs = devm_kzalloc(&pdev->dev, sizeof(*apcs), GFP_KERNEL); + if (!apcs) + return -ENOMEM; + + apcs->dev = &pdev->dev; + apcs->offset = (unsigned long)of_device_get_match_data(&pdev->dev); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + apcs->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(apcs->base)) + return PTR_ERR(apcs->base); + + platform_set_drvdata(pdev, apcs); + + return 0; +} + +static int qcom_apcs_ipc_remove(struct platform_device *pdev) +{ + return 0; +} + +/* .data is the offset of the ipc register within the global block */ +static const struct of_device_id qcom_apcs_ipc_of_match[] = { + { .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 }, + { .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void *)16 }, + {} +}; +MODULE_DEVICE_TABLE(of, qcom_apcs_ipc_of_match); + +static struct platform_driver qcom_apcs_ipc_driver = { + .probe = qcom_apcs_ipc_probe, + .remove = qcom_apcs_ipc_remove, + .driver = { + .name = "qcom_apcs_ipc", + .of_match_table = qcom_apcs_ipc_of_match, + }, +}; + +static int __init qcom_apcs_ipc_init(void) +{ + return platform_driver_register(&qcom_apcs_ipc_driver); +} +postcore_initcall(qcom_apcs_ipc_init); + +static void __exit qcom_apcs_ipc_exit(void) +{ + platform_driver_unregister(&qcom_apcs_ipc_driver); +} +module_exit(qcom_apcs_ipc_exit); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Qualcomm APCS IPC driver"); diff --git a/include/linux/soc/qcom/apcs_ipc.h b/include/linux/soc/qcom/apcs_ipc.h new file mode 100644 index 000000000000..72be77555261 --- /dev/null +++ b/include/linux/soc/qcom/apcs_ipc.h @@ -0,0 +1,26 @@ +#ifndef __QCOM_APCS_IPC_H__ +#define __QCOM_APCS_IPC_H__ + +#include <linux/err.h> + +struct device; +struct qcom_apcs_ipc_bell; + +#if IS_ENABLED(CONFIG_QCOM_APCS_IPC) + +struct qcom_apcs_ipc_bell *devm_qcom_apcs_ipc_get(struct device *dev, + const char *id); +void qcom_apcs_ipc_ring(struct qcom_apcs_ipc_bell *bell); + +#else + +static inline struct qcom_apcs_ipc_bell *devm_qcom_apcs_ipc_get(struct device *dev, + const char *id) +{ + return ERR_PTR(-EINVAL); +} + +static inline void qcom_apcs_ipc_ring(struct qcom_apcs_ipc_bell *bell) {} + +#endif +#endif
This implements a driver that exposes the IPC bits found in the APCS Global block in various Qualcomm platforms. The bits are used to signal inter-processor communication signals from the application CPU to other masters. The driver implements the "doorbell" binding and could be used as basis for a new Linux framework, if found useful outside Qualcomm. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v2: - New driver drivers/soc/qcom/Kconfig | 8 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/apcs-ipc.c | 182 ++++++++++++++++++++++++++++++++++++++ include/linux/soc/qcom/apcs_ipc.h | 26 ++++++ 4 files changed, 217 insertions(+) create mode 100644 drivers/soc/qcom/apcs-ipc.c create mode 100644 include/linux/soc/qcom/apcs_ipc.h