| Message ID | 20200430063054.18879-2-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 30-04-20, 12:00, Manivannan Sadhasivam wrote: > +#define IPCC_SIGNAL_ID_MASK GENMASK(15, 0) > +#define IPCC_CLIENT_ID_MASK GENMASK(31, 16) > +#define IPCC_CLIENT_ID_SHIFT 16 > + > +#define IPCC_NO_PENDING_IRQ 0xffffffff Why not GENMASK(31, 0) > +static struct qcom_ipcc_proto_data *ipcc_proto_data; why do we need a global which is used only once. > +static void qcom_ipcc_mask_irq(struct irq_data *irqd) > +{ > + struct qcom_ipcc_proto_data *proto_data; > + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); > + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); last three are used for debug log, fn can be much simpler if we get rid of noise.. Do we really need this to be production :) > + > + proto_data = irq_data_get_irq_chip_data(irqd); > + > + dev_dbg(proto_data->dev, > + "Disabling interrupts for: client_id: %u; signal_id: %u\n", > + sender_client_id, sender_signal_id); > + > + writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE); > +} > + > +static void qcom_ipcc_unmask_irq(struct irq_data *irqd) > +{ > + struct qcom_ipcc_proto_data *proto_data; > + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); > + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); here as well > +static int qcom_ipcc_domain_xlate(struct irq_domain *d, > + struct device_node *node, const u32 *intspec, > + unsigned int intsize, > + unsigned long *out_hwirq, > + unsigned int *out_type) pls align these to match open parenthesis > +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data, > + struct device_node *controller_dn) > +{ > + struct mbox_controller *mbox; > + struct device_node *client_dn; > + struct device *dev = proto_data->dev; > + struct of_phandle_args curr_ph; > + int i, j, ret; > + int num_chans = 0; > + > + /* > + * Find out the number of clients interested in this mailbox > + * and create channels accordingly. > + */ > + for_each_node_with_property(client_dn, "mboxes") { > + if (!of_device_is_available(client_dn)) > + continue; > + i = of_count_phandle_with_args(client_dn, > + "mboxes", "#mbox-cells"); > + for (j = 0; j < i; j++) { > + ret = of_parse_phandle_with_args(client_dn, "mboxes", > + "#mbox-cells", j, > + &curr_ph); this sounds like something DT should do, not drivers :) > +static int qcom_ipcc_probe(struct platform_device *pdev) > +{ > + struct qcom_ipcc_proto_data *proto_data; > + int ret; > + > + proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL); > + if (!proto_data) > + return -ENOMEM; > + > + ipcc_proto_data = proto_data; > + proto_data->dev = &pdev->dev; > + > + proto_data->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(proto_data->base)) { > + dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n"); > + return PTR_ERR(proto_data->base); > + } > + > + proto_data->irq = platform_get_irq(pdev, 0); > + if (proto_data->irq < 0) { > + dev_err(&pdev->dev, "Failed to get the IRQ\n"); > + return proto_data->irq; > + } > + > + /* Perform a SW reset on this client's protocol state */ > + writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR); > + > + proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node, > + &qcom_ipcc_irq_ops, > + proto_data); > + if (!proto_data->irq_domain) { > + dev_err(&pdev->dev, "Failed to add irq_domain\n"); > + return -ENOMEM; > + } > + > + ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node); > + if (ret) { > + dev_err(&pdev->dev, "Failed to create mailbox\n"); > + goto err_mbox; > + } > + > + ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn, > + IRQF_TRIGGER_HIGH, "ipcc", proto_data); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret); Should the qcom_ipcc_setup_mbox() not be unroller here? > + goto err_mbox; > + } > + > + enable_irq_wake(proto_data->irq); > + platform_set_drvdata(pdev, proto_data); > + > + return 0; > + > +err_mbox: > + irq_domain_remove(proto_data->irq_domain); > + > + return ret; > +} > + > +static int qcom_ipcc_remove(struct platform_device *pdev) > +{ > + struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev); > + > + disable_irq_wake(proto_data->irq); > + irq_domain_remove(proto_data->irq_domain); So you are calling this when your isr is active, we have possibility of race here. This function come with a warning: "The caller must ensure that all mappings within the domain have been disposed" Has that been done?
On 30-04-20, 14:32, Manivannan Sadhasivam wrote: > Hi, > > On Thu, Apr 30, 2020 at 12:54:48PM +0530, Vinod Koul wrote: > > On 30-04-20, 12:00, Manivannan Sadhasivam wrote: > > > > > +#define IPCC_SIGNAL_ID_MASK GENMASK(15, 0) > > > +#define IPCC_CLIENT_ID_MASK GENMASK(31, 16) > > > +#define IPCC_CLIENT_ID_SHIFT 16 > > > + > > > +#define IPCC_NO_PENDING_IRQ 0xffffffff > > > > Why not GENMASK(31, 0) > > > > Hmm, I usually use GENMASK for mask defines. But yeah it can be used here. Well the idea behind genmask was to avoid coding mistakes which sounds apt here as well :) > > > > +static struct qcom_ipcc_proto_data *ipcc_proto_data; > > > > why do we need a global which is used only once. > > > > Ah, that's a left over. Will remove it. > > > > +static void qcom_ipcc_mask_irq(struct irq_data *irqd) > > > +{ > > > + struct qcom_ipcc_proto_data *proto_data; > > > + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > > > + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); > > > + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); > > > > last three are used for debug log, fn can be much simpler if we get rid > > of noise.. Do we really need this to be production :) > > > > This is for debugging the production systems, that's why dev_dbg. So I don't > consider it as a noise :) This in an irq chip, the debug code is much more than actual function! Anyone who wants to debug can add these lines :) > > > + proto_data = irq_data_get_irq_chip_data(irqd); > > > + > > > + dev_dbg(proto_data->dev, > > > + "Disabling interrupts for: client_id: %u; signal_id: %u\n", > > > + sender_client_id, sender_signal_id); > > > + > > > + writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE); > > > +} > > > + > > > +static void qcom_ipcc_unmask_irq(struct irq_data *irqd) > > > +{ > > > + struct qcom_ipcc_proto_data *proto_data; > > > + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > > > + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); > > > + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); > > > > here as well > > > > > +static int qcom_ipcc_domain_xlate(struct irq_domain *d, > > > + struct device_node *node, const u32 *intspec, > > > + unsigned int intsize, > > > + unsigned long *out_hwirq, > > > + unsigned int *out_type) > > > > pls align these to match open parenthesis > > > > It is aligned. Perhaps diff is showing it as mangled due to ignoring > whitespaces? Not sure, even checkpatch seems to think so > > > > +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data, > > > + struct device_node *controller_dn) > > > +{ > > > + struct mbox_controller *mbox; > > > + struct device_node *client_dn; > > > + struct device *dev = proto_data->dev; > > > + struct of_phandle_args curr_ph; > > > + int i, j, ret; > > > + int num_chans = 0; > > > + > > > + /* > > > + * Find out the number of clients interested in this mailbox > > > + * and create channels accordingly. > > > + */ > > > + for_each_node_with_property(client_dn, "mboxes") { > > > + if (!of_device_is_available(client_dn)) > > > + continue; > > > + i = of_count_phandle_with_args(client_dn, > > > + "mboxes", "#mbox-cells"); > > > + for (j = 0; j < i; j++) { > > > + ret = of_parse_phandle_with_args(client_dn, "mboxes", > > > + "#mbox-cells", j, > > > + &curr_ph); > > > > this sounds like something DT should do, not drivers :) > > > > Right. This is design discussion I'd like to have. Currently the driver checks > the DT and allocates the total number of mbox channels. But I think the more > cleaner way would be to have this driver allocating fixed number of channels > statically and let the clients use it. Sorry my point was about code of checking mboxes and #mbox-cells, these seems generic enough and could be moved into of core code :) But I think making fixed number of channels should not be done if DT can provide this information. > Maybe Raghavendra/Venkat can comment here? > > > > +static int qcom_ipcc_probe(struct platform_device *pdev) > > > +{ > > > + struct qcom_ipcc_proto_data *proto_data; > > > + int ret; > > > + > > > + proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL); > > > + if (!proto_data) > > > + return -ENOMEM; > > > + > > > + ipcc_proto_data = proto_data; > > > + proto_data->dev = &pdev->dev; > > > + > > > + proto_data->base = devm_platform_ioremap_resource(pdev, 0); > > > + if (IS_ERR(proto_data->base)) { > > > + dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n"); > > > + return PTR_ERR(proto_data->base); > > > + } > > > + > > > + proto_data->irq = platform_get_irq(pdev, 0); > > > + if (proto_data->irq < 0) { > > > + dev_err(&pdev->dev, "Failed to get the IRQ\n"); > > > + return proto_data->irq; > > > + } > > > + > > > + /* Perform a SW reset on this client's protocol state */ > > > + writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR); > > > + > > > + proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node, > > > + &qcom_ipcc_irq_ops, > > > + proto_data); > > > + if (!proto_data->irq_domain) { > > > + dev_err(&pdev->dev, "Failed to add irq_domain\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node); > > > + if (ret) { > > > + dev_err(&pdev->dev, "Failed to create mailbox\n"); > > > + goto err_mbox; > > > + } > > > + > > > + ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn, > > > + IRQF_TRIGGER_HIGH, "ipcc", proto_data); > > > + if (ret < 0) { > > > + dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret); > > > > Should the qcom_ipcc_setup_mbox() not be unroller here? > > qcom_ipcc_setup_mbox() uses devm_ API for registering mbox controller. So what > is the issue? Ah missed the devm parts, i think no unroll required here > > > + goto err_mbox; > > > + } > > > + > > > + enable_irq_wake(proto_data->irq); > > > + platform_set_drvdata(pdev, proto_data); > > > + > > > + return 0; > > > + > > > +err_mbox: > > > + irq_domain_remove(proto_data->irq_domain); > > > + > > > + return ret; > > > +} > > > + > > > +static int qcom_ipcc_remove(struct platform_device *pdev) > > > +{ > > > + struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev); > > > + > > > + disable_irq_wake(proto_data->irq); > > > + irq_domain_remove(proto_data->irq_domain); > > > > So you are calling this when your isr is active, we have possibility of > > race here. This function come with a warning: > > "The caller must ensure that all mappings within the domain have been disposed" > > I thought it is not required since most of the interrupt controller drivers > don't do it. But checked with Marc Zyngier and he suggested to dispose the > mapping as that's the good practice. The usual pattern is an interrupt > controller is not built as a module and the assumption is it lives forever. > > But one issue here is, currently we don't know the allocated irqs (in specific > hw irq numbers) as we don't create the mapping. It gets created when a client > calls platform_get_irq(). In the irq handler, we just read the current hw irq > number from a register. So, if we want to dispose the mapping then we need to > track the allocated irqs. Else we need to create the mapping for all possible > clients in this driver itself. I'm not sure which one is preferred. > > Maybe Bjorn/qcom folks can comment what is preferred here? Maybe this should also be lives forever cases then! :)
On Wed 29 Apr 23:30 PDT 2020, Manivannan Sadhasivam wrote: > From: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org> > > Add support for the Inter-Processor Communication Controller (IPCC) > driver that coordinates the interrupts (inbound & outbound) for > Multiprocessor (MPROC), COMPUTE-Level0 (COMPUTE-L0) & COMPUTE-Level1 > (COMPUTE-L1) protocols for the Application Processor Subsystem (APSS). > > As a part of its inbound communication, the driver would be responsible > for forwarding the interrupts from various clients, such as Modem, DSPs, > PCIe, and so on, to the entities running on the APPS. As a result, it's > implemented as an irq_chip driver. > > On the other hand, the driver also registers as a mailbox controller > that provides a mailbox interface to interrupt other clients connected > to the IPCC, acting as an outbound communication channel. > > Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org> > Signed-off-by: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > [mani: cleaned up for upstream] > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/soc/qcom/Kconfig | 11 + > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/qcom_ipcc.c | 410 +++++++++++++++++++++++++++++++++++ I think the driver should be moved to either drivers/irqchip or drivers/mailbox. > 3 files changed, 422 insertions(+) > create mode 100644 drivers/soc/qcom/qcom_ipcc.c > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index bf42a17a45de..97c380c3fa09 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -53,6 +53,17 @@ config QCOM_GSBI > functions for connecting the underlying serial UART, SPI, and I2C > devices to the output pins. > > +config QCOM_IPCC > + tristate "Qualcomm Technologies, Inc. IPCC driver" > + depends on MAILBOX > + help > + Qualcomm Technologies, Inc. IPCC driver for MSM devices. The drivers > + acts as an interrupt controller for the clients interested in > + talking to the IPCC (inbound-communication). On the other hand, the > + driver also provides a mailbox channel for outbound-communications. > + Say Y here to compile the driver as a part of kernel or M to compile > + as a module. > + > config QCOM_LLCC > tristate "Qualcomm Technologies, Inc. LLCC driver" > depends on ARCH_QCOM || COMPILE_TEST > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index 5d6b83dc58e8..b7658b007040 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -24,5 +24,6 @@ obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o > obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o > obj-$(CONFIG_QCOM_APR) += apr.o > obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o > +obj-$(CONFIG_QCOM_IPCC) += qcom_ipcc.o > obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o > obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o > diff --git a/drivers/soc/qcom/qcom_ipcc.c b/drivers/soc/qcom/qcom_ipcc.c > new file mode 100644 > index 000000000000..5906a70220e0 > --- /dev/null > +++ b/drivers/soc/qcom/qcom_ipcc.c > @@ -0,0 +1,410 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/mailbox_controller.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > + > +#include <dt-bindings/soc/qcom,ipcc.h> > + > +/* IPCC Register offsets */ > +#define IPCC_REG_SEND_ID 0x0c > +#define IPCC_REG_RECV_ID 0x10 > +#define IPCC_REG_RECV_SIGNAL_ENABLE 0x14 > +#define IPCC_REG_RECV_SIGNAL_DISABLE 0x18 > +#define IPCC_REG_RECV_SIGNAL_CLEAR 0x1c > +#define IPCC_REG_CLIENT_CLEAR 0x38 > + > +#define IPCC_SIGNAL_ID_MASK GENMASK(15, 0) > +#define IPCC_CLIENT_ID_MASK GENMASK(31, 16) > +#define IPCC_CLIENT_ID_SHIFT 16 > + > +#define IPCC_NO_PENDING_IRQ 0xffffffff > + > +/** > + * struct qcom_ipcc_proto_data - Per-protocol data > + * @irq_domain: irq_domain associated with this protocol-id > + * @mbox: mailbox-controller interface > + * @chans: The mailbox clients channel array (created dynamically) > + * @base: Base address of the IPCC frame associated to APSS > + * @dev: Device associated with this instance > + * @irq: Summary irq > + */ > +struct qcom_ipcc_proto_data { There's only one of these per IPCC hardware block, so I would suggest that you shorten in to just "struct qcom_ipcc" and then throughout the implementation replace "proto_data" with just "ipcc". > + struct irq_domain *irq_domain; > + struct mbox_controller mbox; > + struct mbox_chan *chans; > + void __iomem *base; > + struct device *dev; > + int irq; > +}; > + > +/** > + * struct qcom_ipcc_mbox_chan - Per-mailbox-channel data. Associated to > + * channel when requested by the clients > + * @chan: Points to this channel's array element for this protocol's > + * ipcc_protocol_data->chans[] > + * @proto_data: The pointer to per-protocol data associated to this channel > + * @client_id: The client-id to which the interrupt has to be triggered > + * @signal_id: The signal-id to which the interrupt has to be triggered > + */ > +struct qcom_ipcc_mbox_chan { > + struct mbox_chan *chan; > + struct qcom_ipcc_proto_data *proto_data; > + u16 client_id; > + u16 signal_id; > +}; > + > +static struct qcom_ipcc_proto_data *ipcc_proto_data; > + > +static inline u32 qcom_ipcc_get_packed_id(u16 client_id, u16 signal_id) qcom_ipcc_pack_ids() and please omit the "inline", the compiler will inline it if it's a good thing to do. > +{ > + return (client_id << IPCC_CLIENT_ID_SHIFT) | signal_id; How about: return FIELD_PREP(IPCC_CLIENT_ID_MASK, client_id) | FIELD_PREP(IPCC_SIGNAL_ID_MASK, signal_id); > +} > + > +static inline u16 qcom_ipcc_get_client_id(u32 packed_id) This is used only to extract the field for a dev_dbg print. How about dropping this for now and if this kind of debugging is beneficial lets add some tracepoint that internally can extract the fields. > +{ > + return packed_id >> IPCC_CLIENT_ID_SHIFT; > +} > + > +static inline u16 qcom_ipcc_get_signal_id(u32 packed_id) Ditto. > +{ > + return packed_id & IPCC_SIGNAL_ID_MASK; > +} > + > +static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data) > +{ > + struct qcom_ipcc_proto_data *proto_data = data; > + u32 packed_id; > + int virq; > + > + for (;;) { > + packed_id = readl(proto_data->base + IPCC_REG_RECV_ID); > + if (packed_id == IPCC_NO_PENDING_IRQ) > + break; > + > + virq = irq_find_mapping(proto_data->irq_domain, packed_id); > + > + dev_dbg(proto_data->dev, > + "IRQ for client_id: %u; signal_id: %u; virq: %d\n", > + qcom_ipcc_get_client_id(packed_id), > + qcom_ipcc_get_signal_id(packed_id), virq); As above, I think this is useful either during development of the driver (now done) or in a system debugging case as a tracepoint. So please omit this debug print. > + > + writel(packed_id, If you're consistently with naming this "hwirq" you don't need to wrap this line. > + proto_data->base + IPCC_REG_RECV_SIGNAL_CLEAR); > + > + generic_handle_irq(virq); > + } > + > + return IRQ_HANDLED; > +} > + > +static void qcom_ipcc_mask_irq(struct irq_data *irqd) > +{ > + struct qcom_ipcc_proto_data *proto_data; > + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); > + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); > + > + proto_data = irq_data_get_irq_chip_data(irqd); struct qcom_ipcc *ipcc = irq_data_get_irq_chip_data(irqd); irq_hw_number_t hwirq = irqd_to_hwirq(irqd); Looks better. > + > + dev_dbg(proto_data->dev, > + "Disabling interrupts for: client_id: %u; signal_id: %u\n", > + sender_client_id, sender_signal_id); Pushing out field dissection and "logging" to a tracepoint would make this function prettier as well. > + > + writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE); > +} > + > +static void qcom_ipcc_unmask_irq(struct irq_data *irqd) > +{ > + struct qcom_ipcc_proto_data *proto_data; > + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); > + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); > + > + proto_data = irq_data_get_irq_chip_data(irqd); As above. > + > + dev_dbg(proto_data->dev, > + "Enabling interrupts for: client_id: %u; signal_id: %u\n", > + sender_client_id, sender_signal_id); > + > + writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_ENABLE); > +} > + > +static struct irq_chip qcom_ipcc_irq_chip = { > + .name = "ipcc", > + .irq_mask = qcom_ipcc_mask_irq, > + .irq_unmask = qcom_ipcc_unmask_irq, > + .flags = IRQCHIP_SKIP_SET_WAKE, > +}; > + > +static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq, > + irq_hw_number_t hw) > +{ > + struct qcom_ipcc_proto_data *proto_data = d->host_data; > + > + irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq); > + irq_set_chip_data(irq, proto_data); > + irq_set_noprobe(irq); > + > + return 0; > +} > + > +static int qcom_ipcc_domain_xlate(struct irq_domain *d, > + struct device_node *node, const u32 *intspec, > + unsigned int intsize, > + unsigned long *out_hwirq, > + unsigned int *out_type) > +{ > + struct qcom_ipcc_proto_data *proto_data = d->host_data; > + struct device *dev = proto_data->dev; > + > + if (intsize != 3) > + return -EINVAL; > + > + *out_hwirq = qcom_ipcc_get_packed_id(intspec[0], intspec[1]); > + *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK; > + > + dev_dbg(dev, "hwirq: 0x%lx\n", *out_hwirq); > + > + return 0; > +} > + > +static const struct irq_domain_ops qcom_ipcc_irq_ops = { > + .map = qcom_ipcc_domain_map, > + .xlate = qcom_ipcc_domain_xlate, > +}; > + > +static int qcom_ipcc_mbox_send_data(struct mbox_chan *chan, void *data) > +{ > + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv; > + struct qcom_ipcc_proto_data *proto_data = ipcc_mbox_chan->proto_data; > + u32 packed_id; Although we're in the "mailbox section", I still think it's reasonable to name this hwirq for consistency (otherwise please shorten it to "id"). > + > + dev_dbg(proto_data->dev, > + "Generating IRQ for client_id: %u; signal_id: %u\n", > + ipcc_mbox_chan->client_id, ipcc_mbox_chan->signal_id); > + > + packed_id = qcom_ipcc_get_packed_id(ipcc_mbox_chan->client_id, > + ipcc_mbox_chan->signal_id); > + writel(packed_id, proto_data->base + IPCC_REG_SEND_ID); > + > + return 0; > +} > + > +static void qcom_ipcc_mbox_shutdown(struct mbox_chan *chan) > +{ > + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv; > + > + chan->con_priv = NULL; > + kfree(ipcc_mbox_chan); > +} > + > +static struct mbox_chan *qcom_ipcc_mbox_xlate(struct mbox_controller *mbox, > + const struct of_phandle_args *ph) > +{ > + struct device *dev; > + struct qcom_ipcc_proto_data *proto_data; > + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan; > + int chan_id; > + > + proto_data = container_of(mbox, struct qcom_ipcc_proto_data, mbox); > + if (WARN_ON(!proto_data)) Is this possible? > + return ERR_PTR(-EINVAL); > + > + dev = proto_data->dev; > + > + if (ph->args_count != 2) > + return ERR_PTR(-EINVAL); > + > + /* Check whether the mbox channel is allocated or not */ > + for (chan_id = 0; chan_id < mbox->num_chans; chan_id++) { > + ipcc_mbox_chan = proto_data->chans[chan_id].con_priv; > + > + if (!ipcc_mbox_chan) > + break; > + else if (ipcc_mbox_chan->client_id == ph->args[0] && > + ipcc_mbox_chan->signal_id == ph->args[1]) > + return ERR_PTR(-EBUSY); I think it would be better to just return the matching chan, and let mbox_request_channel() deal with the fact that it's already in use. > + } > + > + if (chan_id >= mbox->num_chans) > + return ERR_PTR(-EBUSY); > + > + ipcc_mbox_chan = kzalloc(sizeof(*ipcc_mbox_chan), GFP_KERNEL); Can we allocate these during initialization time instead of allocating and releasing them in of_xlate and shutdown? > + if (!ipcc_mbox_chan) > + return ERR_PTR(-ENOMEM); > + > + ipcc_mbox_chan->client_id = ph->args[0]; > + ipcc_mbox_chan->signal_id = ph->args[1]; > + ipcc_mbox_chan->chan = &proto_data->chans[chan_id]; > + ipcc_mbox_chan->proto_data = proto_data; > + ipcc_mbox_chan->chan->con_priv = ipcc_mbox_chan; > + > + dev_dbg(dev, > + "New mailbox channel: %u for client_id: %u; signal_id: %u\n", > + chan_id, ipcc_mbox_chan->client_id, > + ipcc_mbox_chan->signal_id); > + > + return ipcc_mbox_chan->chan; > +} > + > +static const struct mbox_chan_ops ipcc_mbox_chan_ops = { > + .send_data = qcom_ipcc_mbox_send_data, > + .shutdown = qcom_ipcc_mbox_shutdown > +}; > + > +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data, > + struct device_node *controller_dn) > +{ > + struct mbox_controller *mbox; > + struct device_node *client_dn; > + struct device *dev = proto_data->dev; > + struct of_phandle_args curr_ph; > + int i, j, ret; > + int num_chans = 0; > + > + /* > + * Find out the number of clients interested in this mailbox > + * and create channels accordingly. > + */ > + for_each_node_with_property(client_dn, "mboxes") { How much memory do we save by doing this, in comparison to just picking a max amount? > + if (!of_device_is_available(client_dn)) > + continue; > + i = of_count_phandle_with_args(client_dn, > + "mboxes", "#mbox-cells"); > + for (j = 0; j < i; j++) { > + ret = of_parse_phandle_with_args(client_dn, "mboxes", > + "#mbox-cells", j, > + &curr_ph); > + of_node_put(curr_ph.np); > + if (!ret && curr_ph.np == controller_dn) { > + num_chans++; > + break; > + } > + } > + } > + > + /* If no clients are found, skip registering as a mbox controller */ > + if (!num_chans) > + return 0; > + > + proto_data->chans = devm_kcalloc(dev, num_chans, > + sizeof(struct mbox_chan), GFP_KERNEL); sizeof(*proto_data->chans) > + if (!proto_data->chans) > + return -ENOMEM; > + > + mbox = &proto_data->mbox; > + mbox->dev = dev; > + mbox->num_chans = num_chans; > + mbox->chans = proto_data->chans; > + mbox->ops = &ipcc_mbox_chan_ops; > + mbox->of_xlate = qcom_ipcc_mbox_xlate; > + mbox->txdone_irq = false; > + mbox->txdone_poll = false; > + > + return devm_mbox_controller_register(dev, mbox); > +} > + > +static int qcom_ipcc_probe(struct platform_device *pdev) > +{ > + struct qcom_ipcc_proto_data *proto_data; > + int ret; > + > + proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL); > + if (!proto_data) > + return -ENOMEM; > + > + ipcc_proto_data = proto_data; ipcc_protocol_data is unused. > + proto_data->dev = &pdev->dev; > + > + proto_data->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(proto_data->base)) { > + dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n"); > + return PTR_ERR(proto_data->base); > + } > + > + proto_data->irq = platform_get_irq(pdev, 0); > + if (proto_data->irq < 0) { > + dev_err(&pdev->dev, "Failed to get the IRQ\n"); platform_get_irq() already did print. > + return proto_data->irq; > + } > + > + /* Perform a SW reset on this client's protocol state */ > + writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR); > + > + proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node, > + &qcom_ipcc_irq_ops, > + proto_data); > + if (!proto_data->irq_domain) { > + dev_err(&pdev->dev, "Failed to add irq_domain\n"); Afaict this also prints errors. > + return -ENOMEM; > + } > + > + ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node); > + if (ret) { > + dev_err(&pdev->dev, "Failed to create mailbox\n"); Afaict this too will print errors in all code paths (except the ones where you pass invalid data to mbox_controller_register() - but we're already past that). Regards, Bjorn > + goto err_mbox; > + } > + > + ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn, > + IRQF_TRIGGER_HIGH, "ipcc", proto_data); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret); > + goto err_mbox; > + } > + > + enable_irq_wake(proto_data->irq); > + platform_set_drvdata(pdev, proto_data); > + > + return 0; > + > +err_mbox: > + irq_domain_remove(proto_data->irq_domain); > + > + return ret; > +} > + > +static int qcom_ipcc_remove(struct platform_device *pdev) > +{ > + struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev); > + > + disable_irq_wake(proto_data->irq); > + irq_domain_remove(proto_data->irq_domain); > + > + return 0; > +} > + > +static const struct of_device_id qcom_ipcc_of_match[] = { > + { .compatible = "qcom,ipcc"}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, qcom_ipcc_of_match); > + > +static struct platform_driver qcom_ipcc_driver = { > + .probe = qcom_ipcc_probe, > + .remove = qcom_ipcc_remove, > + .driver = { > + .name = "qcom_ipcc", > + .of_match_table = qcom_ipcc_of_match, > + }, > +}; > + > +static int __init qcom_ipcc_init(void) > +{ > + return platform_driver_register(&qcom_ipcc_driver); > +} > +arch_initcall(qcom_ipcc_init); > + > +static void __exit qcom_ipcc_exit(void) > +{ > + platform_driver_unregister(&qcom_ipcc_driver); > +} > +module_exit(qcom_ipcc_exit); > + > +MODULE_LICENSE("GPL v2"); > -- > 2.17.1 >
On Thu, Apr 30, 2020 at 01:18:07PM -0700, Bjorn Andersson wrote: > On Wed 29 Apr 23:30 PDT 2020, Manivannan Sadhasivam wrote: > > > From: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org> > > > > Add support for the Inter-Processor Communication Controller (IPCC) > > driver that coordinates the interrupts (inbound & outbound) for > > Multiprocessor (MPROC), COMPUTE-Level0 (COMPUTE-L0) & COMPUTE-Level1 > > (COMPUTE-L1) protocols for the Application Processor Subsystem (APSS). > > > > As a part of its inbound communication, the driver would be responsible > > for forwarding the interrupts from various clients, such as Modem, DSPs, > > PCIe, and so on, to the entities running on the APPS. As a result, it's > > implemented as an irq_chip driver. > > > > On the other hand, the driver also registers as a mailbox controller > > that provides a mailbox interface to interrupt other clients connected > > to the IPCC, acting as an outbound communication channel. > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org> > > Signed-off-by: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > [mani: cleaned up for upstream] > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/soc/qcom/Kconfig | 11 + > > drivers/soc/qcom/Makefile | 1 + > > drivers/soc/qcom/qcom_ipcc.c | 410 +++++++++++++++++++++++++++++++++++ > > I think the driver should be moved to either drivers/irqchip or > drivers/mailbox. > As said in bindings patch, mailbox seems to be a better location if we don't want this to be under drivers/soc. > > 3 files changed, 422 insertions(+) > > create mode 100644 drivers/soc/qcom/qcom_ipcc.c > > > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > > index bf42a17a45de..97c380c3fa09 100644 > > --- a/drivers/soc/qcom/Kconfig > > +++ b/drivers/soc/qcom/Kconfig > > @@ -53,6 +53,17 @@ config QCOM_GSBI > > functions for connecting the underlying serial UART, SPI, and I2C > > devices to the output pins. > > > > +config QCOM_IPCC > > + tristate "Qualcomm Technologies, Inc. IPCC driver" > > + depends on MAILBOX > > + help > > + Qualcomm Technologies, Inc. IPCC driver for MSM devices. The drivers > > + acts as an interrupt controller for the clients interested in > > + talking to the IPCC (inbound-communication). On the other hand, the > > + driver also provides a mailbox channel for outbound-communications. > > + Say Y here to compile the driver as a part of kernel or M to compile > > + as a module. > > + > > config QCOM_LLCC > > tristate "Qualcomm Technologies, Inc. LLCC driver" > > depends on ARCH_QCOM || COMPILE_TEST > > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > > index 5d6b83dc58e8..b7658b007040 100644 > > --- a/drivers/soc/qcom/Makefile > > +++ b/drivers/soc/qcom/Makefile > > @@ -24,5 +24,6 @@ obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o > > obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o > > obj-$(CONFIG_QCOM_APR) += apr.o > > obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o > > +obj-$(CONFIG_QCOM_IPCC) += qcom_ipcc.o > > obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o > > obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o > > diff --git a/drivers/soc/qcom/qcom_ipcc.c b/drivers/soc/qcom/qcom_ipcc.c > > new file mode 100644 > > index 000000000000..5906a70220e0 > > --- /dev/null > > +++ b/drivers/soc/qcom/qcom_ipcc.c > > @@ -0,0 +1,410 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved. > > + */ > > + > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/irqdomain.h> > > +#include <linux/mailbox_controller.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > + > > +#include <dt-bindings/soc/qcom,ipcc.h> > > + > > +/* IPCC Register offsets */ > > +#define IPCC_REG_SEND_ID 0x0c > > +#define IPCC_REG_RECV_ID 0x10 > > +#define IPCC_REG_RECV_SIGNAL_ENABLE 0x14 > > +#define IPCC_REG_RECV_SIGNAL_DISABLE 0x18 > > +#define IPCC_REG_RECV_SIGNAL_CLEAR 0x1c > > +#define IPCC_REG_CLIENT_CLEAR 0x38 > > + > > +#define IPCC_SIGNAL_ID_MASK GENMASK(15, 0) > > +#define IPCC_CLIENT_ID_MASK GENMASK(31, 16) > > +#define IPCC_CLIENT_ID_SHIFT 16 > > + > > +#define IPCC_NO_PENDING_IRQ 0xffffffff > > + > > +/** > > + * struct qcom_ipcc_proto_data - Per-protocol data > > + * @irq_domain: irq_domain associated with this protocol-id > > + * @mbox: mailbox-controller interface > > + * @chans: The mailbox clients channel array (created dynamically) > > + * @base: Base address of the IPCC frame associated to APSS > > + * @dev: Device associated with this instance > > + * @irq: Summary irq > > + */ > > +struct qcom_ipcc_proto_data { > > There's only one of these per IPCC hardware block, so I would suggest > that you shorten in to just "struct qcom_ipcc" and then throughout the > implementation replace "proto_data" with just "ipcc". > okay > > + struct irq_domain *irq_domain; > > + struct mbox_controller mbox; > > + struct mbox_chan *chans; > > + void __iomem *base; > > + struct device *dev; > > + int irq; > > +}; > > + > > +/** > > + * struct qcom_ipcc_mbox_chan - Per-mailbox-channel data. Associated to > > + * channel when requested by the clients > > + * @chan: Points to this channel's array element for this protocol's > > + * ipcc_protocol_data->chans[] > > + * @proto_data: The pointer to per-protocol data associated to this channel > > + * @client_id: The client-id to which the interrupt has to be triggered > > + * @signal_id: The signal-id to which the interrupt has to be triggered > > + */ > > +struct qcom_ipcc_mbox_chan { > > + struct mbox_chan *chan; > > + struct qcom_ipcc_proto_data *proto_data; > > + u16 client_id; > > + u16 signal_id; > > +}; > > + > > +static struct qcom_ipcc_proto_data *ipcc_proto_data; > > + > > +static inline u32 qcom_ipcc_get_packed_id(u16 client_id, u16 signal_id) > > qcom_ipcc_pack_ids() > > and please omit the "inline", the compiler will inline it if it's a good > thing to do. > But the usual convention is to give the hint to compiler isn't it? > > +{ > > + return (client_id << IPCC_CLIENT_ID_SHIFT) | signal_id; > > How about: > > return FIELD_PREP(IPCC_CLIENT_ID_MASK, client_id) | > FIELD_PREP(IPCC_SIGNAL_ID_MASK, signal_id); > okay > > +} > > + > > +static inline u16 qcom_ipcc_get_client_id(u32 packed_id) > > This is used only to extract the field for a dev_dbg print. How about > dropping this for now and if this kind of debugging is beneficial lets > add some tracepoint that internally can extract the fields. > okay, will drop. > > +{ > > + return packed_id >> IPCC_CLIENT_ID_SHIFT; > > +} > > + > > +static inline u16 qcom_ipcc_get_signal_id(u32 packed_id) > > Ditto. > > > +{ > > + return packed_id & IPCC_SIGNAL_ID_MASK; > > +} > > + > > +static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data) > > +{ > > + struct qcom_ipcc_proto_data *proto_data = data; > > + u32 packed_id; > > + int virq; > > + > > + for (;;) { > > + packed_id = readl(proto_data->base + IPCC_REG_RECV_ID); > > + if (packed_id == IPCC_NO_PENDING_IRQ) > > + break; > > + > > + virq = irq_find_mapping(proto_data->irq_domain, packed_id); > > + > > + dev_dbg(proto_data->dev, > > + "IRQ for client_id: %u; signal_id: %u; virq: %d\n", > > + qcom_ipcc_get_client_id(packed_id), > > + qcom_ipcc_get_signal_id(packed_id), virq); > > As above, I think this is useful either during development of the driver > (now done) or in a system debugging case as a tracepoint. So please omit > this debug print. > okay > > + > > + writel(packed_id, > > If you're consistently with naming this "hwirq" you don't need to wrap > this line. > > > + proto_data->base + IPCC_REG_RECV_SIGNAL_CLEAR); > > + > > + generic_handle_irq(virq); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > > +static void qcom_ipcc_mask_irq(struct irq_data *irqd) > > +{ > > + struct qcom_ipcc_proto_data *proto_data; > > + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > > + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); > > + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); > > + > > + proto_data = irq_data_get_irq_chip_data(irqd); > > struct qcom_ipcc *ipcc = irq_data_get_irq_chip_data(irqd); > irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > > Looks better. > > > + > > + dev_dbg(proto_data->dev, > > + "Disabling interrupts for: client_id: %u; signal_id: %u\n", > > + sender_client_id, sender_signal_id); > > Pushing out field dissection and "logging" to a tracepoint would make > this function prettier as well. > okay > > + > > + writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE); > > +} > > + > > +static void qcom_ipcc_unmask_irq(struct irq_data *irqd) > > +{ > > + struct qcom_ipcc_proto_data *proto_data; > > + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); > > + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); > > + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); > > + > > + proto_data = irq_data_get_irq_chip_data(irqd); > > As above. > > > + > > + dev_dbg(proto_data->dev, > > + "Enabling interrupts for: client_id: %u; signal_id: %u\n", > > + sender_client_id, sender_signal_id); > > + > > + writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_ENABLE); > > +} > > + > > +static struct irq_chip qcom_ipcc_irq_chip = { > > + .name = "ipcc", > > + .irq_mask = qcom_ipcc_mask_irq, > > + .irq_unmask = qcom_ipcc_unmask_irq, > > + .flags = IRQCHIP_SKIP_SET_WAKE, > > +}; > > + > > +static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq, > > + irq_hw_number_t hw) > > +{ > > + struct qcom_ipcc_proto_data *proto_data = d->host_data; > > + > > + irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq); > > + irq_set_chip_data(irq, proto_data); > > + irq_set_noprobe(irq); > > + > > + return 0; > > +} > > + > > +static int qcom_ipcc_domain_xlate(struct irq_domain *d, > > + struct device_node *node, const u32 *intspec, > > + unsigned int intsize, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > +{ > > + struct qcom_ipcc_proto_data *proto_data = d->host_data; > > + struct device *dev = proto_data->dev; > > + > > + if (intsize != 3) > > + return -EINVAL; > > + > > + *out_hwirq = qcom_ipcc_get_packed_id(intspec[0], intspec[1]); > > + *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK; > > + > > + dev_dbg(dev, "hwirq: 0x%lx\n", *out_hwirq); > > + > > + return 0; > > +} > > + > > +static const struct irq_domain_ops qcom_ipcc_irq_ops = { > > + .map = qcom_ipcc_domain_map, > > + .xlate = qcom_ipcc_domain_xlate, > > +}; > > + > > +static int qcom_ipcc_mbox_send_data(struct mbox_chan *chan, void *data) > > +{ > > + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv; > > + struct qcom_ipcc_proto_data *proto_data = ipcc_mbox_chan->proto_data; > > + u32 packed_id; > > Although we're in the "mailbox section", I still think it's reasonable > to name this hwirq for consistency (otherwise please shorten it to "id"). > okay, will name it as hwirq. > > + > > + dev_dbg(proto_data->dev, > > + "Generating IRQ for client_id: %u; signal_id: %u\n", > > + ipcc_mbox_chan->client_id, ipcc_mbox_chan->signal_id); > > + > > + packed_id = qcom_ipcc_get_packed_id(ipcc_mbox_chan->client_id, > > + ipcc_mbox_chan->signal_id); > > + writel(packed_id, proto_data->base + IPCC_REG_SEND_ID); > > + > > + return 0; > > +} > > + > > +static void qcom_ipcc_mbox_shutdown(struct mbox_chan *chan) > > +{ > > + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv; > > + > > + chan->con_priv = NULL; > > + kfree(ipcc_mbox_chan); > > +} > > + > > +static struct mbox_chan *qcom_ipcc_mbox_xlate(struct mbox_controller *mbox, > > + const struct of_phandle_args *ph) > > +{ > > + struct device *dev; > > + struct qcom_ipcc_proto_data *proto_data; > > + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan; > > + int chan_id; > > + > > + proto_data = container_of(mbox, struct qcom_ipcc_proto_data, mbox); > > + if (WARN_ON(!proto_data)) > > Is this possible? > hmm, not required. > > + return ERR_PTR(-EINVAL); > > + > > + dev = proto_data->dev; > > + > > + if (ph->args_count != 2) > > + return ERR_PTR(-EINVAL); > > + > > + /* Check whether the mbox channel is allocated or not */ > > + for (chan_id = 0; chan_id < mbox->num_chans; chan_id++) { > > + ipcc_mbox_chan = proto_data->chans[chan_id].con_priv; > > + > > + if (!ipcc_mbox_chan) > > + break; > > + else if (ipcc_mbox_chan->client_id == ph->args[0] && > > + ipcc_mbox_chan->signal_id == ph->args[1]) > > + return ERR_PTR(-EBUSY); > > I think it would be better to just return the matching chan, and let > mbox_request_channel() deal with the fact that it's already in use. > okay > > + } > > + > > + if (chan_id >= mbox->num_chans) > > + return ERR_PTR(-EBUSY); > > + > > + ipcc_mbox_chan = kzalloc(sizeof(*ipcc_mbox_chan), GFP_KERNEL); > > Can we allocate these during initialization time instead of allocating > and releasing them in of_xlate and shutdown? > We should know the max channels supported for allocating statically. > > + if (!ipcc_mbox_chan) > > + return ERR_PTR(-ENOMEM); > > + > > + ipcc_mbox_chan->client_id = ph->args[0]; > > + ipcc_mbox_chan->signal_id = ph->args[1]; > > + ipcc_mbox_chan->chan = &proto_data->chans[chan_id]; > > + ipcc_mbox_chan->proto_data = proto_data; > > + ipcc_mbox_chan->chan->con_priv = ipcc_mbox_chan; > > + > > + dev_dbg(dev, > > + "New mailbox channel: %u for client_id: %u; signal_id: %u\n", > > + chan_id, ipcc_mbox_chan->client_id, > > + ipcc_mbox_chan->signal_id); > > + > > + return ipcc_mbox_chan->chan; > > +} > > + > > +static const struct mbox_chan_ops ipcc_mbox_chan_ops = { > > + .send_data = qcom_ipcc_mbox_send_data, > > + .shutdown = qcom_ipcc_mbox_shutdown > > +}; > > + > > +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data, > > + struct device_node *controller_dn) > > +{ > > + struct mbox_controller *mbox; > > + struct device_node *client_dn; > > + struct device *dev = proto_data->dev; > > + struct of_phandle_args curr_ph; > > + int i, j, ret; > > + int num_chans = 0; > > + > > + /* > > + * Find out the number of clients interested in this mailbox > > + * and create channels accordingly. > > + */ > > + for_each_node_with_property(client_dn, "mboxes") { > > How much memory do we save by doing this, in comparison to just picking > a max amount? > Right, this is what I thought and was hoping that you'll get to it :) But I don't know the max channels supported by IPCC. Can someone help to find this? > > + if (!of_device_is_available(client_dn)) > > + continue; > > + i = of_count_phandle_with_args(client_dn, > > + "mboxes", "#mbox-cells"); > > + for (j = 0; j < i; j++) { > > + ret = of_parse_phandle_with_args(client_dn, "mboxes", > > + "#mbox-cells", j, > > + &curr_ph); > > + of_node_put(curr_ph.np); > > + if (!ret && curr_ph.np == controller_dn) { > > + num_chans++; > > + break; > > + } > > + } > > + } > > + > > + /* If no clients are found, skip registering as a mbox controller */ > > + if (!num_chans) > > + return 0; > > + > > + proto_data->chans = devm_kcalloc(dev, num_chans, > > + sizeof(struct mbox_chan), GFP_KERNEL); > > sizeof(*proto_data->chans) > > > + if (!proto_data->chans) > > + return -ENOMEM; > > + > > + mbox = &proto_data->mbox; > > + mbox->dev = dev; > > + mbox->num_chans = num_chans; > > + mbox->chans = proto_data->chans; > > + mbox->ops = &ipcc_mbox_chan_ops; > > + mbox->of_xlate = qcom_ipcc_mbox_xlate; > > + mbox->txdone_irq = false; > > + mbox->txdone_poll = false; > > + > > + return devm_mbox_controller_register(dev, mbox); > > +} > > + > > +static int qcom_ipcc_probe(struct platform_device *pdev) > > +{ > > + struct qcom_ipcc_proto_data *proto_data; > > + int ret; > > + > > + proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL); > > + if (!proto_data) > > + return -ENOMEM; > > + > > + ipcc_proto_data = proto_data; > > ipcc_protocol_data is unused. > Ack, this is a leftover. > > + proto_data->dev = &pdev->dev; > > + > > + proto_data->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(proto_data->base)) { > > + dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n"); > > + return PTR_ERR(proto_data->base); > > + } > > + > > + proto_data->irq = platform_get_irq(pdev, 0); > > + if (proto_data->irq < 0) { > > + dev_err(&pdev->dev, "Failed to get the IRQ\n"); > > platform_get_irq() already did print. > > > + return proto_data->irq; > > + } > > + > > + /* Perform a SW reset on this client's protocol state */ > > + writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR); > > + > > + proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node, > > + &qcom_ipcc_irq_ops, > > + proto_data); > > + if (!proto_data->irq_domain) { > > + dev_err(&pdev->dev, "Failed to add irq_domain\n"); > > Afaict this also prints errors. > > > + return -ENOMEM; > > + } > > + > > + ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to create mailbox\n"); > > Afaict this too will print errors in all code paths (except the ones > where you pass invalid data to mbox_controller_register() - but we're > already past that). > Okay, will remove. Thanks, Mani > Regards, > Bjorn > > > + goto err_mbox; > > + } > > + > > + ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn, > > + IRQF_TRIGGER_HIGH, "ipcc", proto_data); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret); > > + goto err_mbox; > > + } > > + > > + enable_irq_wake(proto_data->irq); > > + platform_set_drvdata(pdev, proto_data); > > + > > + return 0; > > + > > +err_mbox: > > + irq_domain_remove(proto_data->irq_domain); > > + > > + return ret; > > +} > > + > > +static int qcom_ipcc_remove(struct platform_device *pdev) > > +{ > > + struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev); > > + > > + disable_irq_wake(proto_data->irq); > > + irq_domain_remove(proto_data->irq_domain); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id qcom_ipcc_of_match[] = { > > + { .compatible = "qcom,ipcc"}, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, qcom_ipcc_of_match); > > + > > +static struct platform_driver qcom_ipcc_driver = { > > + .probe = qcom_ipcc_probe, > > + .remove = qcom_ipcc_remove, > > + .driver = { > > + .name = "qcom_ipcc", > > + .of_match_table = qcom_ipcc_of_match, > > + }, > > +}; > > + > > +static int __init qcom_ipcc_init(void) > > +{ > > + return platform_driver_register(&qcom_ipcc_driver); > > +} > > +arch_initcall(qcom_ipcc_init); > > + > > +static void __exit qcom_ipcc_exit(void) > > +{ > > + platform_driver_unregister(&qcom_ipcc_driver); > > +} > > +module_exit(qcom_ipcc_exit); > > + > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.17.1 > >
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index bf42a17a45de..97c380c3fa09 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -53,6 +53,17 @@ config QCOM_GSBI functions for connecting the underlying serial UART, SPI, and I2C devices to the output pins. +config QCOM_IPCC + tristate "Qualcomm Technologies, Inc. IPCC driver" + depends on MAILBOX + help + Qualcomm Technologies, Inc. IPCC driver for MSM devices. The drivers + acts as an interrupt controller for the clients interested in + talking to the IPCC (inbound-communication). On the other hand, the + driver also provides a mailbox channel for outbound-communications. + Say Y here to compile the driver as a part of kernel or M to compile + as a module. + config QCOM_LLCC tristate "Qualcomm Technologies, Inc. LLCC driver" depends on ARCH_QCOM || COMPILE_TEST diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 5d6b83dc58e8..b7658b007040 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -24,5 +24,6 @@ obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o obj-$(CONFIG_QCOM_APR) += apr.o obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o +obj-$(CONFIG_QCOM_IPCC) += qcom_ipcc.o obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o diff --git a/drivers/soc/qcom/qcom_ipcc.c b/drivers/soc/qcom/qcom_ipcc.c new file mode 100644 index 000000000000..5906a70220e0 --- /dev/null +++ b/drivers/soc/qcom/qcom_ipcc.c @@ -0,0 +1,410 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved. + */ + +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/mailbox_controller.h> +#include <linux/module.h> +#include <linux/platform_device.h> + +#include <dt-bindings/soc/qcom,ipcc.h> + +/* IPCC Register offsets */ +#define IPCC_REG_SEND_ID 0x0c +#define IPCC_REG_RECV_ID 0x10 +#define IPCC_REG_RECV_SIGNAL_ENABLE 0x14 +#define IPCC_REG_RECV_SIGNAL_DISABLE 0x18 +#define IPCC_REG_RECV_SIGNAL_CLEAR 0x1c +#define IPCC_REG_CLIENT_CLEAR 0x38 + +#define IPCC_SIGNAL_ID_MASK GENMASK(15, 0) +#define IPCC_CLIENT_ID_MASK GENMASK(31, 16) +#define IPCC_CLIENT_ID_SHIFT 16 + +#define IPCC_NO_PENDING_IRQ 0xffffffff + +/** + * struct qcom_ipcc_proto_data - Per-protocol data + * @irq_domain: irq_domain associated with this protocol-id + * @mbox: mailbox-controller interface + * @chans: The mailbox clients channel array (created dynamically) + * @base: Base address of the IPCC frame associated to APSS + * @dev: Device associated with this instance + * @irq: Summary irq + */ +struct qcom_ipcc_proto_data { + struct irq_domain *irq_domain; + struct mbox_controller mbox; + struct mbox_chan *chans; + void __iomem *base; + struct device *dev; + int irq; +}; + +/** + * struct qcom_ipcc_mbox_chan - Per-mailbox-channel data. Associated to + * channel when requested by the clients + * @chan: Points to this channel's array element for this protocol's + * ipcc_protocol_data->chans[] + * @proto_data: The pointer to per-protocol data associated to this channel + * @client_id: The client-id to which the interrupt has to be triggered + * @signal_id: The signal-id to which the interrupt has to be triggered + */ +struct qcom_ipcc_mbox_chan { + struct mbox_chan *chan; + struct qcom_ipcc_proto_data *proto_data; + u16 client_id; + u16 signal_id; +}; + +static struct qcom_ipcc_proto_data *ipcc_proto_data; + +static inline u32 qcom_ipcc_get_packed_id(u16 client_id, u16 signal_id) +{ + return (client_id << IPCC_CLIENT_ID_SHIFT) | signal_id; +} + +static inline u16 qcom_ipcc_get_client_id(u32 packed_id) +{ + return packed_id >> IPCC_CLIENT_ID_SHIFT; +} + +static inline u16 qcom_ipcc_get_signal_id(u32 packed_id) +{ + return packed_id & IPCC_SIGNAL_ID_MASK; +} + +static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data) +{ + struct qcom_ipcc_proto_data *proto_data = data; + u32 packed_id; + int virq; + + for (;;) { + packed_id = readl(proto_data->base + IPCC_REG_RECV_ID); + if (packed_id == IPCC_NO_PENDING_IRQ) + break; + + virq = irq_find_mapping(proto_data->irq_domain, packed_id); + + dev_dbg(proto_data->dev, + "IRQ for client_id: %u; signal_id: %u; virq: %d\n", + qcom_ipcc_get_client_id(packed_id), + qcom_ipcc_get_signal_id(packed_id), virq); + + writel(packed_id, + proto_data->base + IPCC_REG_RECV_SIGNAL_CLEAR); + + generic_handle_irq(virq); + } + + return IRQ_HANDLED; +} + +static void qcom_ipcc_mask_irq(struct irq_data *irqd) +{ + struct qcom_ipcc_proto_data *proto_data; + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); + + proto_data = irq_data_get_irq_chip_data(irqd); + + dev_dbg(proto_data->dev, + "Disabling interrupts for: client_id: %u; signal_id: %u\n", + sender_client_id, sender_signal_id); + + writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE); +} + +static void qcom_ipcc_unmask_irq(struct irq_data *irqd) +{ + struct qcom_ipcc_proto_data *proto_data; + irq_hw_number_t hwirq = irqd_to_hwirq(irqd); + u16 sender_client_id = qcom_ipcc_get_client_id(hwirq); + u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq); + + proto_data = irq_data_get_irq_chip_data(irqd); + + dev_dbg(proto_data->dev, + "Enabling interrupts for: client_id: %u; signal_id: %u\n", + sender_client_id, sender_signal_id); + + writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_ENABLE); +} + +static struct irq_chip qcom_ipcc_irq_chip = { + .name = "ipcc", + .irq_mask = qcom_ipcc_mask_irq, + .irq_unmask = qcom_ipcc_unmask_irq, + .flags = IRQCHIP_SKIP_SET_WAKE, +}; + +static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq, + irq_hw_number_t hw) +{ + struct qcom_ipcc_proto_data *proto_data = d->host_data; + + irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq); + irq_set_chip_data(irq, proto_data); + irq_set_noprobe(irq); + + return 0; +} + +static int qcom_ipcc_domain_xlate(struct irq_domain *d, + struct device_node *node, const u32 *intspec, + unsigned int intsize, + unsigned long *out_hwirq, + unsigned int *out_type) +{ + struct qcom_ipcc_proto_data *proto_data = d->host_data; + struct device *dev = proto_data->dev; + + if (intsize != 3) + return -EINVAL; + + *out_hwirq = qcom_ipcc_get_packed_id(intspec[0], intspec[1]); + *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK; + + dev_dbg(dev, "hwirq: 0x%lx\n", *out_hwirq); + + return 0; +} + +static const struct irq_domain_ops qcom_ipcc_irq_ops = { + .map = qcom_ipcc_domain_map, + .xlate = qcom_ipcc_domain_xlate, +}; + +static int qcom_ipcc_mbox_send_data(struct mbox_chan *chan, void *data) +{ + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv; + struct qcom_ipcc_proto_data *proto_data = ipcc_mbox_chan->proto_data; + u32 packed_id; + + dev_dbg(proto_data->dev, + "Generating IRQ for client_id: %u; signal_id: %u\n", + ipcc_mbox_chan->client_id, ipcc_mbox_chan->signal_id); + + packed_id = qcom_ipcc_get_packed_id(ipcc_mbox_chan->client_id, + ipcc_mbox_chan->signal_id); + writel(packed_id, proto_data->base + IPCC_REG_SEND_ID); + + return 0; +} + +static void qcom_ipcc_mbox_shutdown(struct mbox_chan *chan) +{ + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv; + + chan->con_priv = NULL; + kfree(ipcc_mbox_chan); +} + +static struct mbox_chan *qcom_ipcc_mbox_xlate(struct mbox_controller *mbox, + const struct of_phandle_args *ph) +{ + struct device *dev; + struct qcom_ipcc_proto_data *proto_data; + struct qcom_ipcc_mbox_chan *ipcc_mbox_chan; + int chan_id; + + proto_data = container_of(mbox, struct qcom_ipcc_proto_data, mbox); + if (WARN_ON(!proto_data)) + return ERR_PTR(-EINVAL); + + dev = proto_data->dev; + + if (ph->args_count != 2) + return ERR_PTR(-EINVAL); + + /* Check whether the mbox channel is allocated or not */ + for (chan_id = 0; chan_id < mbox->num_chans; chan_id++) { + ipcc_mbox_chan = proto_data->chans[chan_id].con_priv; + + if (!ipcc_mbox_chan) + break; + else if (ipcc_mbox_chan->client_id == ph->args[0] && + ipcc_mbox_chan->signal_id == ph->args[1]) + return ERR_PTR(-EBUSY); + } + + if (chan_id >= mbox->num_chans) + return ERR_PTR(-EBUSY); + + ipcc_mbox_chan = kzalloc(sizeof(*ipcc_mbox_chan), GFP_KERNEL); + if (!ipcc_mbox_chan) + return ERR_PTR(-ENOMEM); + + ipcc_mbox_chan->client_id = ph->args[0]; + ipcc_mbox_chan->signal_id = ph->args[1]; + ipcc_mbox_chan->chan = &proto_data->chans[chan_id]; + ipcc_mbox_chan->proto_data = proto_data; + ipcc_mbox_chan->chan->con_priv = ipcc_mbox_chan; + + dev_dbg(dev, + "New mailbox channel: %u for client_id: %u; signal_id: %u\n", + chan_id, ipcc_mbox_chan->client_id, + ipcc_mbox_chan->signal_id); + + return ipcc_mbox_chan->chan; +} + +static const struct mbox_chan_ops ipcc_mbox_chan_ops = { + .send_data = qcom_ipcc_mbox_send_data, + .shutdown = qcom_ipcc_mbox_shutdown +}; + +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data, + struct device_node *controller_dn) +{ + struct mbox_controller *mbox; + struct device_node *client_dn; + struct device *dev = proto_data->dev; + struct of_phandle_args curr_ph; + int i, j, ret; + int num_chans = 0; + + /* + * Find out the number of clients interested in this mailbox + * and create channels accordingly. + */ + for_each_node_with_property(client_dn, "mboxes") { + if (!of_device_is_available(client_dn)) + continue; + i = of_count_phandle_with_args(client_dn, + "mboxes", "#mbox-cells"); + for (j = 0; j < i; j++) { + ret = of_parse_phandle_with_args(client_dn, "mboxes", + "#mbox-cells", j, + &curr_ph); + of_node_put(curr_ph.np); + if (!ret && curr_ph.np == controller_dn) { + num_chans++; + break; + } + } + } + + /* If no clients are found, skip registering as a mbox controller */ + if (!num_chans) + return 0; + + proto_data->chans = devm_kcalloc(dev, num_chans, + sizeof(struct mbox_chan), GFP_KERNEL); + if (!proto_data->chans) + return -ENOMEM; + + mbox = &proto_data->mbox; + mbox->dev = dev; + mbox->num_chans = num_chans; + mbox->chans = proto_data->chans; + mbox->ops = &ipcc_mbox_chan_ops; + mbox->of_xlate = qcom_ipcc_mbox_xlate; + mbox->txdone_irq = false; + mbox->txdone_poll = false; + + return devm_mbox_controller_register(dev, mbox); +} + +static int qcom_ipcc_probe(struct platform_device *pdev) +{ + struct qcom_ipcc_proto_data *proto_data; + int ret; + + proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL); + if (!proto_data) + return -ENOMEM; + + ipcc_proto_data = proto_data; + proto_data->dev = &pdev->dev; + + proto_data->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(proto_data->base)) { + dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n"); + return PTR_ERR(proto_data->base); + } + + proto_data->irq = platform_get_irq(pdev, 0); + if (proto_data->irq < 0) { + dev_err(&pdev->dev, "Failed to get the IRQ\n"); + return proto_data->irq; + } + + /* Perform a SW reset on this client's protocol state */ + writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR); + + proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node, + &qcom_ipcc_irq_ops, + proto_data); + if (!proto_data->irq_domain) { + dev_err(&pdev->dev, "Failed to add irq_domain\n"); + return -ENOMEM; + } + + ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node); + if (ret) { + dev_err(&pdev->dev, "Failed to create mailbox\n"); + goto err_mbox; + } + + ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn, + IRQF_TRIGGER_HIGH, "ipcc", proto_data); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret); + goto err_mbox; + } + + enable_irq_wake(proto_data->irq); + platform_set_drvdata(pdev, proto_data); + + return 0; + +err_mbox: + irq_domain_remove(proto_data->irq_domain); + + return ret; +} + +static int qcom_ipcc_remove(struct platform_device *pdev) +{ + struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev); + + disable_irq_wake(proto_data->irq); + irq_domain_remove(proto_data->irq_domain); + + return 0; +} + +static const struct of_device_id qcom_ipcc_of_match[] = { + { .compatible = "qcom,ipcc"}, + {} +}; +MODULE_DEVICE_TABLE(of, qcom_ipcc_of_match); + +static struct platform_driver qcom_ipcc_driver = { + .probe = qcom_ipcc_probe, + .remove = qcom_ipcc_remove, + .driver = { + .name = "qcom_ipcc", + .of_match_table = qcom_ipcc_of_match, + }, +}; + +static int __init qcom_ipcc_init(void) +{ + return platform_driver_register(&qcom_ipcc_driver); +} +arch_initcall(qcom_ipcc_init); + +static void __exit qcom_ipcc_exit(void) +{ + platform_driver_unregister(&qcom_ipcc_driver); +} +module_exit(qcom_ipcc_exit); + +MODULE_LICENSE("GPL v2");