| Message ID | 20200430063054.18879-2-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: soc: qcom: Add devicetree binding for Qcom IPCC | expand |
On 2020-04-29 23:30, Manivannan Sadhasivam wrote: > +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); We can skip doing a SW reset here. Few of the subsystems may be brought out of reset via the bootloader and the interrupts from them might be pending. Doing a SW reset here would clear those interrupts. Thank you. Raghavendra
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?
Hi, On Wed, Apr 29, 2020 at 11:45:27PM -0700, rananta@codeaurora.org wrote: > On 2020-04-29 23:30, Manivannan Sadhasivam wrote: > > +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); > We can skip doing a SW reset here. Few of the subsystems may be brought out > of reset via the bootloader and the interrupts from them might be pending. > Doing a SW reset here would clear those interrupts. > Okay, will remove it. Thanks, Mani > Thank you. > Raghavendra
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. > > +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 :) > > + > > + 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? > > +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. 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? > > + 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? Thanks, Mani > Has that been done? > > -- > ~Vinod
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 Thu, Apr 30, 2020 at 02:53:58PM +0530, Vinod Koul wrote: > 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 :) > Agree. > > > > > > +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 :) > I don't have a strong feeling so will remove :) > > > > + 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 > I'm not sure too, this is what it says for me: WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #66: new file mode 100644 CHECK: Alignment should match open parenthesis #279: FILE: drivers/soc/qcom/qcom_ipcc.c:209: +static struct mbox_chan *qcom_ipcc_mbox_xlate(struct mbox_controller *mbox, + const struct of_phandle_args *ph) total: 0 errors, 1 warnings, 1 checks, 433 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. Commit 01c2e8a8a53d ("soc: qcom: ipcc: Add support for IPCC controller") has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. > > > > > > +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. > That's not my call, it is Jassi :) All mailbox drivers statically declare the number of channels in drivers and we don't have any API/property to fetch the information from DT. If Jassi is okay with it, I can add an API and corresponding DT property (and ofcourse document it). > > 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! :) > Heh. Most of the drivers using irq_domain_add_tree() don't bother disposing the interrupts. Currently this driver is defined as tristate. So either we should dispose the interrupts or make it as built-in only. If this is ever going to get removed, then there will be a warning from kernel saying irq tree is not empty! Or there is also a case where we can make sure that all clients will dispose their interrupts when they get unloaded. In that way, we can safely remove the irq domain in this driver. Thanks, Mani > -- > ~Vinod
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");