Message ID | 1446174501-8870-1-git-send-email-okaya@codeaurora.org |
---|---|
State | Superseded, archived |
Headers | show |
Hi Sinan,
[auto build test WARNING on lwn/docs-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
url: https://github.com/0day-ci/linux/commits/Sinan-Kaya/dma-add-Qualcomm-Technologies-HIDMA-management-driver/20151030-111408
coccinelle warnings: (new ones prefixed by >>)
>> drivers/dma/qcom_hidma_mgmt.c:852:3-8: No need to set .owner here. The core will do it.
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote: > diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > new file mode 100644 > index 0000000..81674ab > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > @@ -0,0 +1,42 @@ > +Qualcomm Technologies HIDMA Management interface > + > +Required properties: > +- compatible: must contain "qcom,hidma_mgmt" No underscores in the name please. Also try to be more specific, in case this device shows up in more than one SoC and they end up not being 100% compatible we want to be able to tell them apart. > +- reg: Address range for DMA device > +- interrupts: Should contain the one interrupt shared by all channels > +- nr-channels: Number of channels supported by this DMA controller. > +- max-write: Maximum write burst in bytes. A memcpy requested is > + fragmented to multiples of this amount. > +- max-read: Maximum read burst in bytes. A memcpy request is > + fragmented to multiples of this amount. > +- max-wxactions: Maximum write transactions to perform in a burst > +- max-rdactions: Maximum read transactions to perform in a burst > +- max-memset-limit: Maximum memset limit > +- ch-priority-#n: Priority of the channel > +- ch-weight-#n: Round robin weight of the channel For the last two, you can just use a multi-cell property, using one cell per channel. > + > +#define MODULE_NAME "hidma-mgmt" > +#define PREFIX MODULE_NAME ": " These can be removed, just use dev_info() etc for messages, to include a unique identifier. > +#define AUTOSUSPEND_TIMEOUT 2000 > + > +#define HIDMA_RUNTIME_GET(dmadev) \ > +do { \ > + atomic_inc(&(dmadev)->pm_counter); \ > + TRC_PM(&(dmadev)->pdev->dev, \ > + "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\ > + atomic_read(&(dmadev)->pm_counter)); \ > + pm_runtime_get_sync(&(dmadev)->pdev->dev); \ > +} while (0) > + > +#define HIDMA_RUNTIME_SET(dmadev) \ > +do { \ > + atomic_dec(&(dmadev)->pm_counter); \ > + TRC_PM(&(dmadev)->pdev->dev, \ > + "%s:%d pm_runtime_put_autosuspend:%d\n", \ > + __func__, __LINE__, \ > + atomic_read(&(dmadev)->pm_counter)); \ > + pm_runtime_mark_last_busy(&(dmadev)->pdev->dev); \ > + pm_runtime_put_autosuspend(&(dmadev)->pdev->dev); \ > +} while (0) Better use inline functions. > +struct qcom_hidma_mgmt_dev { > + u8 max_wr_xactions; > + u8 max_rd_xactions; > + u8 max_memset_limit; > + u16 max_write_request; > + u16 max_read_request; > + u16 nr_channels; > + u32 chreset_timeout; > + u32 sw_version; > + u8 *priority; > + u8 *weight; > + > + atomic_t pm_counter; > + /* Hardware device constants */ > + dma_addr_t dev_physaddr; No need to store the physaddr, it's write-only here. > + > +static unsigned int debug_pm; > +module_param(debug_pm, uint, 0644); > +MODULE_PARM_DESC(debug_pm, > + "debug runtime power management transitions (default: 0)"); > + > +#define TRC_PM(...) do { \ > + if (debug_pm) \ > + dev_info(__VA_ARGS__); \ > + } while (0) > + Once you are done debugging the driver and have a final version, please remove these. > +#if IS_ENABLED(CONFIG_DEBUG_FS) > + > +#define HIDMA_SHOW(dma, name) \ > + seq_printf(s, #name "=0x%x\n", dma->name) Only used once, just remove. > +#define HIDMA_READ_SHOW(dma, name, offset) \ > + do { \ > + u32 val; \ > + val = readl(dma->dev_virtaddr + offset); \ > + seq_printf(s, name "=0x%x\n", val); \ > + } while (0) Inline function. > +/** > + * qcom_hidma_mgmt_info: display HIDMA device info > + * > + * Display the info for the current HIDMA device. > + */ > +static int qcom_hidma_mgmt_info(struct seq_file *s, void *unused) > +{ > + struct qcom_hidma_mgmt_dev *mgmtdev = s->private; > + u32 val; > + int i; > + > + HIDMA_RUNTIME_GET(mgmtdev); > + HIDMA_SHOW(mgmtdev, sw_version); > + > + val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET); > + seq_printf(s, "ENABLE=%d\n", val & 0x1); > + > + val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET); > + seq_printf(s, "reset_timeout=%d\n", val & CHRESET_TIMEOUUT_MASK); > + > + val = readl(mgmtdev->dev_virtaddr + MHICFG_OFFSET); > + seq_printf(s, "nr_event_channel=%d\n", > + (val >> NR_EV_CHANNEL_BIT_POS) & NR_CHANNEL_MASK); > + seq_printf(s, "nr_tr_channel=%d\n", (val & NR_CHANNEL_MASK)); > + seq_printf(s, "nr_virt_tr_channel=%d\n", mgmtdev->nr_channels); > + seq_printf(s, "dev_virtaddr=%p\n", &mgmtdev->dev_virtaddr); > + seq_printf(s, "dev_physaddr=%pap\n", &mgmtdev->dev_physaddr); > + seq_printf(s, "dev_addrsize=%pap\n", &mgmtdev->dev_addrsize); printing pointers is a security risk, just remove them if they are not essential here. > + > +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file) > +{ > + return single_open(file, qcom_hidma_mgmt_err, inode->i_private); > +} > + > +static const struct file_operations qcom_hidma_mgmt_err_fops = { > + .open = qcom_hidma_mgmt_err_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > +}; > + > +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file, > + const char __user *user_buf, size_t count, loff_t *ppos) > +{ > + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; > + > + HIDMA_RUNTIME_GET(mgmtdev); > + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET); > + HIDMA_RUNTIME_SET(mgmtdev); > + return count; > +} > + > +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = { > + .write = qcom_hidma_mgmt_mhiderr_clr, > +}; Is this really just a debugging interface? If anyone would do this for normal operation, it needs to be a proper API. > + > +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev) > +{ > + debugfs_remove(mgmtdev->evt_ena); > + debugfs_remove(mgmtdev->tre_errclr); > + debugfs_remove(mgmtdev->msi_errclr); > + debugfs_remove(mgmtdev->ode_errclr); > + debugfs_remove(mgmtdev->ide_errclr); > + debugfs_remove(mgmtdev->evt_errclr); > + debugfs_remove(mgmtdev->mhid_errclr); > + debugfs_remove(mgmtdev->err); > + debugfs_remove(mgmtdev->info); > + debugfs_remove(mgmtdev->debugfs); > +} > I guess debugfs_remove_recursive() would do the job as well. > +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev) > +{ > + int rc = 0; > + > + mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev), > + NULL); > + if (!mgmtdev->debugfs) { > + rc = -ENODEV; > + return rc; > + } > + > + mgmtdev->info = debugfs_create_file("info", S_IRUGO, > + mgmtdev->debugfs, mgmtdev, &qcom_hidma_mgmt_fops); > + if (!mgmtdev->info) { > + rc = -ENOMEM; > + goto cleanup; > + } > + > + mgmtdev->err = debugfs_create_file("err", S_IRUGO, > + mgmtdev->debugfs, mgmtdev, > + &qcom_hidma_mgmt_err_fops); > + if (!mgmtdev->err) { > + rc = -ENOMEM; > + goto cleanup; > + } > + > + mgmtdev->mhid_errclr = debugfs_create_file("mhiderrclr", S_IWUSR, > + mgmtdev->debugfs, mgmtdev, > + &qcom_hidma_mgmt_mhiderr_clrfops); > + if (!mgmtdev->mhid_errclr) { > + rc = -ENOMEM; > + goto cleanup; > + } Maybe use a loop around an array here to avoid writing the same thing 10 times? Also, you could move the debugging code into a separate file and have a separate Kconfig symbol so users can disable this if they do not debug your driver but still want to use debugfs for other things. > +static irqreturn_t qcom_hidma_mgmt_irq_handler(int irq, void *arg) > +{ > + /* TODO: handle irq here */ > + return IRQ_HANDLED; > +} > + rc = devm_request_irq(&pdev->dev, irq, qcom_hidma_mgmt_irq_handler, > + IRQF_SHARED, "qcom-hidmamgmt", mgmtdev); > + if (rc) { > + dev_err(&pdev->dev, "irq registration failed: %d\n", irq); > + goto out; > + } If you create a shared handler, you must check whether there was an interrupt pending, otherwise you will lose interrupts for all other devices that share the same IRQ. Better remove the handler completely here so you don't need to check it. > + > + if (device_property_read_u16(&pdev->dev, "nr-channels", > + &mgmtdev->nr_channels)) { > + dev_err(&pdev->dev, "number of channels missing\n"); > + rc = -EINVAL; > + goto out; > + } This should be a u32 name 'dma-channels' for consistency with the generic DMA binding. Also, try to avoid using u8 and u16 properties everywhere and just us u32 for consistency. > + > + for (i = 0; i < mgmtdev->nr_channels; i++) { > + char name[30]; > + > + sprintf(name, "ch-priority-%d", i); > + device_property_read_u8(&pdev->dev, name, > + &mgmtdev->priority[i]); > + > + sprintf(name, "ch-weight-%d", i); > + device_property_read_u8(&pdev->dev, name, > + &mgmtdev->weight[i]); Per comment above, just read this as an array. > + dev_info(&pdev->dev, > + "HI-DMA engine management driver registration complete\n"); > + platform_set_drvdata(pdev, mgmtdev); > + HIDMA_RUNTIME_SET(mgmtdev); > + return 0; > +out: > + pm_runtime_disable(&pdev->dev); > + pm_runtime_put_sync_suspend(&pdev->dev); > + return rc; > +} The rest of the probe function does not register any user interface aside from the debugging stuff. Can you explain in the changelog how you expect the driver to be used in a real system? Is there another driver coming? > + > +static const struct of_device_id qcom_hidma_mgmt_match[] = { > + { .compatible = "qcom,hidma_mgmt", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, qcom_hidma_mgmt_match); > + > +static struct platform_driver qcom_hidma_mgmt_driver = { > + .probe = qcom_hidma_mgmt_probe, > + .remove = qcom_hidma_mgmt_remove, > + .driver = { > + .name = MODULE_NAME, > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(qcom_hidma_mgmt_match), drop the .owner field and the of_match_ptr() macro to avoid warnings here. > +static int __init qcom_hidma_mgmt_init(void) > +{ > + return platform_driver_register(&qcom_hidma_mgmt_driver); > +} > +device_initcall(qcom_hidma_mgmt_init); > + > +static void __exit qcom_hidma_mgmt_exit(void) > +{ > + platform_driver_unregister(&qcom_hidma_mgmt_driver); > +} > +module_exit(qcom_hidma_mgmt_exit); module_platform_driver() Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 29, 2015 at 11:08:12PM -0400, Sinan Kaya wrote: > The Qualcomm Technologies HIDMA device has been designed > to support virtualization technology. The driver has been > divided into two to follow the hardware design. The management > driver is executed in hypervisor context and is the main > managment for all channels provided by the device. The > channel driver is exected in the guest OS context. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > .../devicetree/bindings/dma/qcom_hidma_mgmt.txt | 42 + > drivers/dma/Kconfig | 11 + > drivers/dma/Makefile | 1 + > drivers/dma/qcom_hidma_mgmt.c | 868 +++++++++++++++++++++ > 4 files changed, 922 insertions(+) > create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > create mode 100644 drivers/dma/qcom_hidma_mgmt.c > > diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > new file mode 100644 > index 0000000..81674ab > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > @@ -0,0 +1,42 @@ > +Qualcomm Technologies HIDMA Management interface > + > +Required properties: > +- compatible: must contain "qcom,hidma_mgmt" s/_/-/ in property names and compatible strings. Per your rationale for splitting the device nodes, surely the relationship between the two needs to be described? The GIC has a similar split, yet we use a single node, with the hypervisor portions being optional. What if there were multiple instances? > +- reg: Address range for DMA device > +- interrupts: Should contain the one interrupt shared by all channels > +- nr-channels: Number of channels supported by this DMA controller. > +- max-write: Maximum write burst in bytes. A memcpy requested is > + fragmented to multiples of this amount. > +- max-read: Maximum read burst in bytes. A memcpy request is > + fragmented to multiples of this amount. Thse would be better named as max-read-burst-bytes and min-read-burst-bytes. > +- max-wxactions: Maximum write transactions to perform in a burst > +- max-rdactions: Maximum read transactions to perform in a burst max-{write,read}-transactions > +- max-memset-limit: Maximum memset limit I have no idea what this is meant to mean. > +- ch-priority-#n: Priority of the channel > +- ch-weight-#n: Round robin weight of the channel These need better descriptions. They sound like configuration options. [...] > +#if IS_ENABLED(CONFIG_ACPI) > +static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = { > + {"QCOM8060"}, > + {}, > +}; > +#endif How do DMA engines work with ACPI? How are client relationships defined? THanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 30, 2015 at 5:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Oct 29, 2015 at 11:08:12PM -0400, Sinan Kaya wrote: >> The Qualcomm Technologies HIDMA device has been designed >> to support virtualization technology. The driver has been >> divided into two to follow the hardware design. The management >> driver is executed in hypervisor context and is the main >> managment for all channels provided by the device. The >> channel driver is exected in the guest OS context. >> >> +#if IS_ENABLED(CONFIG_ACPI) >> +static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = { >> + {"QCOM8060"}, >> + {}, >> +}; >> +#endif > > How do DMA engines work with ACPI? > > How are client relationships defined? The ACPI tables DSDT and CSRT (more info here: http://www.acpi.info/links.htm) defines properties. DSDT: per DMAC: the resources per client: FixedDMA descriptor that contains channel / request line pair. CSRT: necessary table to map which DMAC provides which request line, thus request line numbering are global on platform. When DMAC driver is probed in the running system it should call as well registration function from acpi-dma.c. All clients when use new DMA slave API gets channel automatically based on their FixedDMA property. So, above is how it should be done. Didn't actually checked what this driver does.
On 10/30/2015 1:59 PM, Andy Shevchenko wrote: > On Fri, Oct 30, 2015 at 5:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Thu, Oct 29, 2015 at 11:08:12PM -0400, Sinan Kaya wrote: >>> The Qualcomm Technologies HIDMA device has been designed >>> to support virtualization technology. The driver has been >>> divided into two to follow the hardware design. The management >>> driver is executed in hypervisor context and is the main >>> managment for all channels provided by the device. The >>> channel driver is exected in the guest OS context. >>> > >>> +#if IS_ENABLED(CONFIG_ACPI) >>> +static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = { >>> + {"QCOM8060"}, >>> + {}, >>> +}; >>> +#endif >> >> How do DMA engines work with ACPI? >> >> How are client relationships defined? > > The ACPI tables DSDT and CSRT (more info here: > http://www.acpi.info/links.htm) defines properties. > > DSDT: > per DMAC: the resources > per client: FixedDMA descriptor that contains channel / request line pair. > > CSRT: > necessary table to map which DMAC provides which request line, thus > request line numbering are global on platform. > > When DMAC driver is probed in the running system it should call as > well registration function from acpi-dma.c. > > All clients when use new DMA slave API gets channel automatically > based on their FixedDMA property. > > So, above is how it should be done. Didn't actually checked what this > driver does. > I was going to reply to all the questions in one pass but let me handle piece by piece. Here are some facts. - This hardware supports memcpy and memset only. - Memset feature was removed from the kernel sometime around 3.14. So no memset support in this driver either. - The hardware does not support DMA slave support - The goal is to provide an interface to DMA engine framework for memcpy optimization so that the rest of the kernel drivers and applications make use of the hardware. CSRT is an Intel specific ACPI table for slave devices. It was decided by Linaro that CSRT will not be supported for ARM64. There were some discussions in ACPI forums to define a similar table for ARM64 but we are not there today and this hardware does not support slave interface. ACPI enumeration is just like any other platform device. The driver gets looked up by a QCOM specific HID and the driver gets probed with the rest of the arguments in DSM object similar to device-tree attributes. The code uses device functions so the driver is not aware of where the parameters are coming from.
On Fri, Oct 30, 2015 at 02:08:07PM -0400, Sinan Kaya wrote: > > > On 10/30/2015 1:59 PM, Andy Shevchenko wrote: > >On Fri, Oct 30, 2015 at 5:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: > >>On Thu, Oct 29, 2015 at 11:08:12PM -0400, Sinan Kaya wrote: > >>>The Qualcomm Technologies HIDMA device has been designed > >>>to support virtualization technology. The driver has been > >>>divided into two to follow the hardware design. The management > >>>driver is executed in hypervisor context and is the main > >>>managment for all channels provided by the device. The > >>>channel driver is exected in the guest OS context. > >>> > > > >>>+#if IS_ENABLED(CONFIG_ACPI) > >>>+static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = { > >>>+ {"QCOM8060"}, > >>>+ {}, > >>>+}; > >>>+#endif > >> > >>How do DMA engines work with ACPI? > >> > >>How are client relationships defined? > > > >The ACPI tables DSDT and CSRT (more info here: > >http://www.acpi.info/links.htm) defines properties. > > > >DSDT: > > per DMAC: the resources > > per client: FixedDMA descriptor that contains channel / request line pair. > > > >CSRT: > > necessary table to map which DMAC provides which request line, thus > >request line numbering are global on platform. > > > >When DMAC driver is probed in the running system it should call as > >well registration function from acpi-dma.c. > > > >All clients when use new DMA slave API gets channel automatically > >based on their FixedDMA property. > > > >So, above is how it should be done. Didn't actually checked what this > >driver does. > > > I was going to reply to all the questions in one pass but let me > handle piece by piece. > > Here are some facts. > - This hardware supports memcpy and memset only. > - Memset feature was removed from the kernel sometime around 3.14. > So no memset support in this driver either. > - The hardware does not support DMA slave support This point is what I'd missed when reviewing. Apologies for that. > - The goal is to provide an interface to DMA engine framework for > memcpy optimization so that the rest of the kernel drivers and > applications make use of the hardware. > > CSRT is an Intel specific ACPI table for slave devices. It was > decided by Linaro that CSRT will not be supported for ARM64. > > There were some discussions in ACPI forums to define a similar table > for ARM64 but we are not there today and this hardware does not > support slave interface. Ok. Thanks for the info! > ACPI enumeration is just like any other platform device. The driver > gets looked up by a QCOM specific HID and the driver gets probed > with the rest of the arguments in DSM object similar to device-tree > attributes. The code uses device functions so the driver is not > aware of where the parameters are coming from. Ok. So far the properties look device-internal, so I'm ok with thoses properties being uniform and parsed in the manner that they are, though I'm worried about the way the device description is split over two nodes. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 30, 2015 at 8:08 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> The ACPI tables DSDT and CSRT (more info here: >> http://www.acpi.info/links.htm) defines properties. >> >> DSDT: >> per DMAC: the resources >> per client: FixedDMA descriptor that contains channel / request line >> pair. >> >> CSRT: >> necessary table to map which DMAC provides which request line, thus >> request line numbering are global on platform. >> >> When DMAC driver is probed in the running system it should call as >> well registration function from acpi-dma.c. >> >> All clients when use new DMA slave API gets channel automatically >> based on their FixedDMA property. >> >> So, above is how it should be done. Didn't actually checked what this >> driver does. >> > I was going to reply to all the questions in one pass but let me handle > piece by piece. > > Here are some facts. > - This hardware supports memcpy and memset only. > - Memset feature was removed from the kernel sometime around 3.14. So no > memset support in this driver either. > - The hardware does not support DMA slave support > - The goal is to provide an interface to DMA engine framework for memcpy > optimization so that the rest of the kernel drivers and applications make > use of the hardware. > > CSRT is an Intel specific ACPI table for slave devices. Wrong. It was designed by Microsoft to support multiple controllers, in particular DMACs. Have you read that document I posted link to? > It was decided by > Linaro that CSRT will not be supported for ARM64. Interesting, ARM64 platforms are not going to have more than one DMAC per system? > There were some discussions in ACPI forums to define a similar table for > ARM64 but we are not there today and this hardware does not support slave > interface. > > ACPI enumeration is just like any other platform device. The driver gets > looked up by a QCOM specific HID and the driver gets probed with the rest of > the arguments in DSM object similar to device-tree attributes. The code uses > device functions so the driver is not aware of where the parameters are > coming from.
On Fri, Oct 30, 2015 at 08:18:49PM +0200, Andy Shevchenko wrote: > On Fri, Oct 30, 2015 at 8:08 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > > >> The ACPI tables DSDT and CSRT (more info here: > >> http://www.acpi.info/links.htm) defines properties. > >> > >> DSDT: > >> per DMAC: the resources > >> per client: FixedDMA descriptor that contains channel / request line > >> pair. > >> > >> CSRT: > >> necessary table to map which DMAC provides which request line, thus > >> request line numbering are global on platform. > >> > >> When DMAC driver is probed in the running system it should call as > >> well registration function from acpi-dma.c. > >> > >> All clients when use new DMA slave API gets channel automatically > >> based on their FixedDMA property. > >> > >> So, above is how it should be done. Didn't actually checked what this > >> driver does. > >> > > I was going to reply to all the questions in one pass but let me handle > > piece by piece. > > > > Here are some facts. > > - This hardware supports memcpy and memset only. > > - Memset feature was removed from the kernel sometime around 3.14. So no > > memset support in this driver either. > > - The hardware does not support DMA slave support > > - The goal is to provide an interface to DMA engine framework for memcpy > > optimization so that the rest of the kernel drivers and applications make > > use of the hardware. > > > > > CSRT is an Intel specific ACPI table for slave devices. > > Wrong. > It was designed by Microsoft to support multiple controllers, in > particular DMACs. > Have you read that document I posted link to? > > > It was decided by > > Linaro that CSRT will not be supported for ARM64. > > Interesting, ARM64 platforms are not going to have more than one DMAC > per system? I cannot imagine that being true, and I don't see why Linaro would decide such a thing. It does appear that it's not relevant to this device and driver, given the lack of clients, unless I've misunderstood? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 30, 2015 at 8:25 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> > CSRT is an Intel specific ACPI table for slave devices. >> >> Wrong. >> It was designed by Microsoft to support multiple controllers, in >> particular DMACs. >> Have you read that document I posted link to? >> >> > It was decided by >> > Linaro that CSRT will not be supported for ARM64. >> >> Interesting, ARM64 platforms are not going to have more than one DMAC >> per system? > > I cannot imagine that being true, and I don't see why Linaro would > decide such a thing. > > It does appear that it's not relevant to this device and driver, given > the lack of clients, unless I've misunderstood? Yeah, just a side note.
On 10/30/2015 2:40 PM, Andy Shevchenko wrote: > On Fri, Oct 30, 2015 at 8:25 PM, Mark Rutland <mark.rutland@arm.com> wrote: > >>>> CSRT is an Intel specific ACPI table for slave devices. >>> >>> Wrong. >>> It was designed by Microsoft to support multiple controllers, in >>> particular DMACs. >>> Have you read that document I posted link to? >>> Nope. >>>> It was decided by >>>> Linaro that CSRT will not be supported for ARM64. >>> >>> Interesting, ARM64 platforms are not going to have more than one DMAC >>> per system? >> >> I cannot imagine that being true, and I don't see why Linaro would >> decide such a thing. See this. https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/TablePriorities >> >> It does appear that it's not relevant to this device and driver, given >> the lack of clients, unless I've misunderstood? > > Yeah, just a side note. > OK.
On Fri, Oct 30, 2015 at 02:48:06PM -0400, Sinan Kaya wrote: > >>>>It was decided by > >>>>Linaro that CSRT will not be supported for ARM64. > See this. > > https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/TablePriorities The CSRT is listed under "Want", not "Never" or "Don't Care", so Linaro have certainly not said that CSRT will not be supported. If anything, they have stated that the table should be supported. I suspect that the rationale for it not being "Critical" or "Must" is that it's possible to use contemporary systems without DMA engines, but we will need support at some point in future. I also note that there are Linaro patches adding supoprt for DBG2 [1] , which is listed as "Never". So the page may be out of date... Leif, Al? [1] https://lkml.org/lkml/2015/9/8/287 Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/30/2015 2:25 PM, Mark Rutland wrote: > It does appear that it's not relevant to this device and driver, given > the lack of clients, unless I've misunderstood? You are right. Kernel and guest machine kernel are the only user of this DMA device. No other HW devices.
On 10/30/2015 01:01 PM, Mark Rutland wrote: > On Fri, Oct 30, 2015 at 02:48:06PM -0400, Sinan Kaya wrote: >>>>>> It was decided by >>>>>> Linaro that CSRT will not be supported for ARM64. Hrm. I personally decided we didn't have to worry about using the CSRT (specifically, let's wait until someone has a need for it), but I know of no decision to *not* support it. Please cite a source for that claim. >> See this. >> >> https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/TablePriorities > > The CSRT is listed under "Want", not "Never" or "Don't Care", so Linaro > have certainly not said that CSRT will not be supported. If anything, > they have stated that the table should be supported. "Want" means interesting, and probably useful, but no clear indication that anyone actually needs it. At one point, we thought we might use the CSRT for describing DMA, but it turns out we have not needed to. However, let's make sure we're saying the same thing: the CSRT table is properly defined in the kernel include/acpi/actbl2.h file so one can read such a table and use it if they so choose. Nothing that we have done at Linaro in the arm64 part of the kernel relies on any of the content from the CSRT, nor does it preclude someone relying on that content. So, the CSRT is defined, and is usable, but is just not being used -- by Linaro -- at present. If that needs to change, let me know; no one has asked us to use the CSRT for a DMA engine, and we have not been provided any hardware that requires it. > I suspect that the rationale for it not being "Critical" or "Must" is > that it's possible to use contemporary systems without DMA engines, but > we will need support at some point in future. Essentially correct. We (Linaro ACPI team) would be glad to work on it, if needed. We just need to know that someone wants it, and we need to have some way to test it (i.e., aka "hardware"). > I also note that there are Linaro patches adding supoprt for DBG2 [1] , > which is listed as "Never". So the page may be out of date... Sadly, it is indeed out of date, but it is on my TODO list. The DBG2 table was originally listed as "Never" only because of the licensing; this has since changed and is now usable for consoles, so it got worked on and implemented. > Leif, Al? > > [1] https://lkml.org/lkml/2015/9/8/287 > > Thanks, > Mark. >
On Fri, Oct 30, 2015 at 10:08 PM, Al Stone <al.stone@linaro.org> wrote: > On 10/30/2015 01:01 PM, Mark Rutland wrote: >> On Fri, Oct 30, 2015 at 02:48:06PM -0400, Sinan Kaya wrote: >> The CSRT is listed under "Want", not "Never" or "Don't Care", so Linaro >> have certainly not said that CSRT will not be supported. If anything, >> they have stated that the table should be supported. > > "Want" means interesting, and probably useful, but no clear indication that > anyone actually needs it. At one point, we thought we might use the CSRT > for describing DMA, but it turns out we have not needed to. Then you are going to have either 1 or 0 DMAC for slave devices, right? The CSRT, unfortunately, the only way how to enumerate DMAC to be used by slave devices. You may look into drivers/dma/acpi-dma.c for usage in Linux. Yes, I know about _DSD, but I don't think it will provide soon any other official mechanisms to what we have now. Better to ask Rafael and Mika. > However, let's make sure we're saying the same thing: the CSRT table is > properly defined in the kernel include/acpi/actbl2.h file so one can read > such a table and use it if they so choose. Nothing that we have done at > Linaro in the arm64 part of the kernel relies on any of the content from > the CSRT, nor does it preclude someone relying on that content. So, the > CSRT is defined, and is usable, but is just not being used -- by Linaro -- > at present. This sounds clear. > If that needs to change, let me know; no one has asked us to use the CSRT > for a DMA engine, and we have not been provided any hardware that requires > it. See above.
On Fri, Oct 30, 2015 at 10:15:09PM +0200, Andy Shevchenko wrote: > On Fri, Oct 30, 2015 at 10:08 PM, Al Stone <al.stone@linaro.org> wrote: > > On 10/30/2015 01:01 PM, Mark Rutland wrote: > >> On Fri, Oct 30, 2015 at 02:48:06PM -0400, Sinan Kaya wrote: > > >> The CSRT is listed under "Want", not "Never" or "Don't Care", so Linaro > >> have certainly not said that CSRT will not be supported. If anything, > >> they have stated that the table should be supported. > > > > "Want" means interesting, and probably useful, but no clear indication that > > anyone actually needs it. At one point, we thought we might use the CSRT > > for describing DMA, but it turns out we have not needed to. > > Then you are going to have either 1 or 0 DMAC for slave devices, right? I suspect that the currently supported platforms have 0. If not, we have problems. > The CSRT, unfortunately, the only way how to enumerate DMAC to be used > by slave devices. > You may look into drivers/dma/acpi-dma.c for usage in Linux. > > Yes, I know about _DSD, but I don't think it will provide soon any > other official mechanisms to what we have now. Better to ask Rafael > and Mika. _DSD is insufficient here, given that there is no prescribed way to use it to describe DMA. Anything for that would have to go through ASWG before we could use it. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On 10/30/2015 04:15 PM, Andy Shevchenko wrote: > On Fri, Oct 30, 2015 at 10:08 PM, Al Stone <al.stone@linaro.org> wrote: >> On 10/30/2015 01:01 PM, Mark Rutland wrote: >>> On Fri, Oct 30, 2015 at 02:48:06PM -0400, Sinan Kaya wrote: > >>> The CSRT is listed under "Want", not "Never" or "Don't Care", so Linaro >>> have certainly not said that CSRT will not be supported. If anything, >>> they have stated that the table should be supported. >> >> "Want" means interesting, and probably useful, but no clear indication that >> anyone actually needs it. At one point, we thought we might use the CSRT >> for describing DMA, but it turns out we have not needed to. > > Then you are going to have either 1 or 0 DMAC for slave devices, right? I believe what Al means is that such hardware has not appeared (publicly) until this time and so such situation was theoretical and thus not covered by the Linaro wiki. Linaro had not prioritized CSRT because it didn't seem that the need to support it would arise soon. > The CSRT, unfortunately, the only way how to enumerate DMAC to be used > by slave devices. > You may look into drivers/dma/acpi-dma.c for usage in Linux. > > Yes, I know about _DSD, but I don't think it will provide soon any > other official mechanisms to what we have now. Better to ask Rafael > and Mika. Thanks for the feedback. I agree that generally the plan is to use existing tables from x86 on arm64 when possible. Please see below. >> However, let's make sure we're saying the same thing: the CSRT table is >> properly defined in the kernel include/acpi/actbl2.h file so one can read >> such a table and use it if they so choose. Nothing that we have done at >> Linaro in the arm64 part of the kernel relies on any of the content from >> the CSRT, nor does it preclude someone relying on that content. So, the >> CSRT is defined, and is usable, but is just not being used -- by Linaro -- >> at present. > > This sounds clear. > >> If that needs to change, let me know; no one has asked us to use the CSRT >> for a DMA engine, and we have not been provided any hardware that requires >> it. > > See above. Here's, what I believe to be the summary: 1). QCOM are not implementing slave device support in their current HIDMA hardware, therefore the requirement for CSRT does not exist *at present* for this specific driver to be merged and the discussion in this sub-thread pertains only to a situation not affecting HIDMA. 2). A requirement upon the CSRT should be clarified in the various specifications. The SBBR[0] currently "recommends" CSRT but does not necessarily provide guidance about what kinds of system resources would be covered by that, and so there is a potential for this to be missed. As one of the lead authors of certain ARM server specifications, I will contact those involved in such and ensure that this is addressed with a clarification in a future release. Thanks for raising the concern, Jon. [0] http://infocenter.arm.com/help/topic/com.arm.doc.den0044a/Server_Base_Boot_Requirements.pdf -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/30/2015 5:34 AM, Arnd Bergmann wrote: > On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote: >> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt >> new file mode 100644 >> index 0000000..81674ab >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt >> @@ -0,0 +1,42 @@ >> +Qualcomm Technologies HIDMA Management interface >> + >> +Required properties: >> +- compatible: must contain "qcom,hidma_mgmt" > > No underscores in the name please. Also try to be more specific, in case this > device shows up in more than one SoC and they end up not being 100% compatible > we want to be able to tell them apart. ok > >> +- reg: Address range for DMA device >> +- interrupts: Should contain the one interrupt shared by all channels >> +- nr-channels: Number of channels supported by this DMA controller. >> +- max-write: Maximum write burst in bytes. A memcpy requested is >> + fragmented to multiples of this amount. >> +- max-read: Maximum read burst in bytes. A memcpy request is >> + fragmented to multiples of this amount. >> +- max-wxactions: Maximum write transactions to perform in a burst >> +- max-rdactions: Maximum read transactions to perform in a burst >> +- max-memset-limit: Maximum memset limit >> +- ch-priority-#n: Priority of the channel >> +- ch-weight-#n: Round robin weight of the channel > > For the last two, you can just use a multi-cell property, using one > cell per channel. will look > >> + >> +#define MODULE_NAME "hidma-mgmt" >> +#define PREFIX MODULE_NAME ": " > > These can be removed, just use dev_info() etc for messages, to include > a unique identifier. got rid of them > >> +#define AUTOSUSPEND_TIMEOUT 2000 >> + >> +#define HIDMA_RUNTIME_GET(dmadev) \ >> +do { \ >> + atomic_inc(&(dmadev)->pm_counter); \ >> + TRC_PM(&(dmadev)->pdev->dev, \ >> + "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\ >> + atomic_read(&(dmadev)->pm_counter)); \ >> + pm_runtime_get_sync(&(dmadev)->pdev->dev); \ >> +} while (0) >> + >> +#define HIDMA_RUNTIME_SET(dmadev) \ >> +do { \ >> + atomic_dec(&(dmadev)->pm_counter); \ >> + TRC_PM(&(dmadev)->pdev->dev, \ >> + "%s:%d pm_runtime_put_autosuspend:%d\n", \ >> + __func__, __LINE__, \ >> + atomic_read(&(dmadev)->pm_counter)); \ >> + pm_runtime_mark_last_busy(&(dmadev)->pdev->dev); \ >> + pm_runtime_put_autosuspend(&(dmadev)->pdev->dev); \ >> +} while (0) > > Better use inline functions. removed these > >> +struct qcom_hidma_mgmt_dev { >> + u8 max_wr_xactions; >> + u8 max_rd_xactions; >> + u8 max_memset_limit; >> + u16 max_write_request; >> + u16 max_read_request; >> + u16 nr_channels; >> + u32 chreset_timeout; >> + u32 sw_version; >> + u8 *priority; >> + u8 *weight; >> + >> + atomic_t pm_counter; >> + /* Hardware device constants */ >> + dma_addr_t dev_physaddr; > > No need to store the physaddr, it's write-only here. ok > >> + >> +static unsigned int debug_pm; >> +module_param(debug_pm, uint, 0644); >> +MODULE_PARM_DESC(debug_pm, >> + "debug runtime power management transitions (default: 0)"); >> + >> +#define TRC_PM(...) do { \ >> + if (debug_pm) \ >> + dev_info(__VA_ARGS__); \ >> + } while (0) >> + > > Once you are done debugging the driver and have a final version, please > remove these. removed TRC_PM > >> +#if IS_ENABLED(CONFIG_DEBUG_FS) >> + >> +#define HIDMA_SHOW(dma, name) \ >> + seq_printf(s, #name "=0x%x\n", dma->name) > > Only used once, just remove. ok > >> +#define HIDMA_READ_SHOW(dma, name, offset) \ >> + do { \ >> + u32 val; \ >> + val = readl(dma->dev_virtaddr + offset); \ >> + seq_printf(s, name "=0x%x\n", val); \ >> + } while (0) > > Inline function. ok > >> +/** >> + * qcom_hidma_mgmt_info: display HIDMA device info >> + * >> + * Display the info for the current HIDMA device. >> + */ >> +static int qcom_hidma_mgmt_info(struct seq_file *s, void *unused) >> +{ >> + struct qcom_hidma_mgmt_dev *mgmtdev = s->private; >> + u32 val; >> + int i; >> + >> + HIDMA_RUNTIME_GET(mgmtdev); >> + HIDMA_SHOW(mgmtdev, sw_version); >> + >> + val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET); >> + seq_printf(s, "ENABLE=%d\n", val & 0x1); >> + >> + val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET); >> + seq_printf(s, "reset_timeout=%d\n", val & CHRESET_TIMEOUUT_MASK); >> + >> + val = readl(mgmtdev->dev_virtaddr + MHICFG_OFFSET); >> + seq_printf(s, "nr_event_channel=%d\n", >> + (val >> NR_EV_CHANNEL_BIT_POS) & NR_CHANNEL_MASK); >> + seq_printf(s, "nr_tr_channel=%d\n", (val & NR_CHANNEL_MASK)); >> + seq_printf(s, "nr_virt_tr_channel=%d\n", mgmtdev->nr_channels); >> + seq_printf(s, "dev_virtaddr=%p\n", &mgmtdev->dev_virtaddr); >> + seq_printf(s, "dev_physaddr=%pap\n", &mgmtdev->dev_physaddr); >> + seq_printf(s, "dev_addrsize=%pap\n", &mgmtdev->dev_addrsize); > > printing pointers is a security risk, just remove them if they are > not essential here. ok >> + >> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file) >> +{ >> + return single_open(file, qcom_hidma_mgmt_err, inode->i_private); >> +} >> + >> +static const struct file_operations qcom_hidma_mgmt_err_fops = { >> + .open = qcom_hidma_mgmt_err_open, >> + .read = seq_read, >> + .llseek = seq_lseek, >> + .release = single_release, >> +}; >> + >> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file, >> + const char __user *user_buf, size_t count, loff_t *ppos) >> +{ >> + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; >> + >> + HIDMA_RUNTIME_GET(mgmtdev); >> + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET); >> + HIDMA_RUNTIME_SET(mgmtdev); >> + return count; >> +} >> + >> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = { >> + .write = qcom_hidma_mgmt_mhiderr_clr, >> +}; > > Is this really just a debugging interface? If anyone would do this > for normal operation, it needs to be a proper API. > This will be used by the system admin to monitor/reset the execution state of the DMA channels. This will be the management interface. Debugfs is probably not the right choice. I originally had sysfs but than had some doubts. I'm open to suggestions. >> + >> +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev) >> +{ >> + debugfs_remove(mgmtdev->evt_ena); >> + debugfs_remove(mgmtdev->tre_errclr); >> + debugfs_remove(mgmtdev->msi_errclr); >> + debugfs_remove(mgmtdev->ode_errclr); >> + debugfs_remove(mgmtdev->ide_errclr); >> + debugfs_remove(mgmtdev->evt_errclr); >> + debugfs_remove(mgmtdev->mhid_errclr); >> + debugfs_remove(mgmtdev->err); >> + debugfs_remove(mgmtdev->info); >> + debugfs_remove(mgmtdev->debugfs); >> +} >> > I guess debugfs_remove_recursive() would do the job as well. ok > >> +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev) >> +{ >> + int rc = 0; >> + >> + mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev), >> + NULL); >> + if (!mgmtdev->debugfs) { >> + rc = -ENODEV; >> + return rc; >> + } >> + >> + mgmtdev->info = debugfs_create_file("info", S_IRUGO, >> + mgmtdev->debugfs, mgmtdev, &qcom_hidma_mgmt_fops); >> + if (!mgmtdev->info) { >> + rc = -ENOMEM; >> + goto cleanup; >> + } >> + >> + mgmtdev->err = debugfs_create_file("err", S_IRUGO, >> + mgmtdev->debugfs, mgmtdev, >> + &qcom_hidma_mgmt_err_fops); >> + if (!mgmtdev->err) { >> + rc = -ENOMEM; >> + goto cleanup; >> + } >> + >> + mgmtdev->mhid_errclr = debugfs_create_file("mhiderrclr", S_IWUSR, >> + mgmtdev->debugfs, mgmtdev, >> + &qcom_hidma_mgmt_mhiderr_clrfops); >> + if (!mgmtdev->mhid_errclr) { >> + rc = -ENOMEM; >> + goto cleanup; >> + } > > Maybe use a loop around an array here to avoid writing the same thing 10 times? > will do > Also, you could move the debugging code into a separate file and have a separate > Kconfig symbol so users can disable this if they do not debug your driver > but still want to use debugfs for other things. > I need these to be accessible all the time, maybe via sysfs or ioctl. >> +static irqreturn_t qcom_hidma_mgmt_irq_handler(int irq, void *arg) >> +{ >> + /* TODO: handle irq here */ >> + return IRQ_HANDLED; >> +} > > >> + rc = devm_request_irq(&pdev->dev, irq, qcom_hidma_mgmt_irq_handler, >> + IRQF_SHARED, "qcom-hidmamgmt", mgmtdev); >> + if (rc) { >> + dev_err(&pdev->dev, "irq registration failed: %d\n", irq); >> + goto out; >> + } > > If you create a shared handler, you must check whether there was an > interrupt pending, otherwise you will lose interrupts for all other devices > that share the same IRQ. ok, I'll remove it for now. The interrupt handler is a to-do. > > Better remove the handler completely here so you don't need to check it. > >> + >> + if (device_property_read_u16(&pdev->dev, "nr-channels", >> + &mgmtdev->nr_channels)) { >> + dev_err(&pdev->dev, "number of channels missing\n"); >> + rc = -EINVAL; >> + goto out; >> + } > > This should be a u32 name 'dma-channels' for consistency with the > generic DMA binding. > ok > Also, try to avoid using u8 and u16 properties everywhere and just > us u32 for consistency. will do > >> + >> + for (i = 0; i < mgmtdev->nr_channels; i++) { >> + char name[30]; >> + >> + sprintf(name, "ch-priority-%d", i); >> + device_property_read_u8(&pdev->dev, name, >> + &mgmtdev->priority[i]); >> + >> + sprintf(name, "ch-weight-%d", i); >> + device_property_read_u8(&pdev->dev, name, >> + &mgmtdev->weight[i]); > > Per comment above, just read this as an array. will look. While device tree syntax for arrays is easy, describing an array in DSM package looks really ugly. I tried to keep things simple. I'll spend some time on it. > >> + dev_info(&pdev->dev, >> + "HI-DMA engine management driver registration complete\n"); >> + platform_set_drvdata(pdev, mgmtdev); >> + HIDMA_RUNTIME_SET(mgmtdev); >> + return 0; >> +out: >> + pm_runtime_disable(&pdev->dev); >> + pm_runtime_put_sync_suspend(&pdev->dev); >> + return rc; >> +} > > The rest of the probe function does not register any user interface aside from > the debugging stuff. Can you explain in the changelog how you expect the > driver to be used in a real system? Is there another driver coming? I expect this driver to grow in functionality over time. Right now, it does the global init for the DMA. After that all channels execute on their own without depending on each other. Global init has to be done first before attempting to do any channel initialization. There is also implied startup ordering requirements. I was doing this by using channel driver with the late binding to guarantee that. As soon as I use module_platform_driver, the ordering gets reversed for some reason. > >> + >> +static const struct of_device_id qcom_hidma_mgmt_match[] = { >> + { .compatible = "qcom,hidma_mgmt", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, qcom_hidma_mgmt_match); >> + >> +static struct platform_driver qcom_hidma_mgmt_driver = { >> + .probe = qcom_hidma_mgmt_probe, >> + .remove = qcom_hidma_mgmt_remove, >> + .driver = { >> + .name = MODULE_NAME, >> + .owner = THIS_MODULE, >> + .of_match_table = of_match_ptr(qcom_hidma_mgmt_match), > > drop the .owner field and the of_match_ptr() macro to avoid warnings here. > >> +static int __init qcom_hidma_mgmt_init(void) >> +{ >> + return platform_driver_register(&qcom_hidma_mgmt_driver); >> +} >> +device_initcall(qcom_hidma_mgmt_init); >> + >> +static void __exit qcom_hidma_mgmt_exit(void) >> +{ >> + platform_driver_unregister(&qcom_hidma_mgmt_driver); >> +} >> +module_exit(qcom_hidma_mgmt_exit); > > module_platform_driver() > > Arnd > thanks for the feedback.
Sinan Kaya wrote: > > I expect this driver to grow in functionality over time. Right now, it > does the global init for the DMA. After that all channels execute on > their own without depending on each other. Global init has to be done > first before attempting to do any channel initialization. > > There is also implied startup ordering requirements. I was doing this by > using channel driver with the late binding to guarantee that. > > As soon as I use module_platform_driver, the ordering gets reversed for > some reason. If you want to force two probe functions to be called in order, then that's what -EPROBE_DEFER is for.
Sinan Kaya wrote: > > I expect this driver to grow in functionality over time. Right now, it > does the global init for the DMA. After that all channels execute on > their own without depending on each other. Global init has to be done > first before attempting to do any channel initialization. > > There is also implied startup ordering requirements. I was doing this by > using channel driver with the late binding to guarantee that. > > As soon as I use module_platform_driver, the ordering gets reversed for > some reason.
On 10/30/2015 11:00 AM, Mark Rutland wrote: > On Thu, Oct 29, 2015 at 11:08:12PM -0400, Sinan Kaya wrote: >> The Qualcomm Technologies HIDMA device has been designed >> to support virtualization technology. The driver has been >> divided into two to follow the hardware design. The management >> driver is executed in hypervisor context and is the main >> managment for all channels provided by the device. The >> channel driver is exected in the guest OS context. >> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> --- >> .../devicetree/bindings/dma/qcom_hidma_mgmt.txt | 42 + >> drivers/dma/Kconfig | 11 + >> drivers/dma/Makefile | 1 + >> drivers/dma/qcom_hidma_mgmt.c | 868 +++++++++++++++++++++ >> 4 files changed, 922 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt >> create mode 100644 drivers/dma/qcom_hidma_mgmt.c >> >> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt >> new file mode 100644 >> index 0000000..81674ab >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt >> @@ -0,0 +1,42 @@ >> +Qualcomm Technologies HIDMA Management interface >> + >> +Required properties: >> +- compatible: must contain "qcom,hidma_mgmt" > > s/_/-/ in property names and compatible strings. > done > Per your rationale for splitting the device nodes, surely the > relationship between the two needs to be described? > I changed the commit message to make it more clear. I also added the same message to the device binding document for reference. > The GIC has a similar split, yet we use a single node, with the > hypervisor portions being optional. > > What if there were multiple instances? > The driver already supports multiple instances. It has been tested against 4 instances. I ended up having 4 management device entities managing 6 different channels each. >> +- reg: Address range for DMA device >> +- interrupts: Should contain the one interrupt shared by all channels >> +- nr-channels: Number of channels supported by this DMA controller. >> +- max-write: Maximum write burst in bytes. A memcpy requested is >> + fragmented to multiples of this amount. >> +- max-read: Maximum read burst in bytes. A memcpy request is >> + fragmented to multiples of this amount. > > Thse would be better named as max-read-burst-bytes and > min-read-burst-bytes. ok > >> +- max-wxactions: Maximum write transactions to perform in a burst >> +- max-rdactions: Maximum read transactions to perform in a burst > > max-{write,read}-transactions ok > >> +- max-memset-limit: Maximum memset limit > > I have no idea what this is meant to mean. > old stuff. no memset support anymore. will remove it. >> +- ch-priority-#n: Priority of the channel >> +- ch-weight-#n: Round robin weight of the channel > > These need better descriptions. They sound like configuration options. > ok. > [...] > >> +#if IS_ENABLED(CONFIG_ACPI) >> +static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = { >> + {"QCOM8060"}, >> + {}, >> +}; >> +#endif > > How do DMA engines work with ACPI? > > How are client relationships defined? > > THanks, > Mark. >
On 10/30/2015 11:33 PM, Jon Masters wrote: > Hi Andy, > > On 10/30/2015 04:15 PM, Andy Shevchenko wrote: >> On Fri, Oct 30, 2015 at 10:08 PM, Al Stone <al.stone@linaro.org> wrote: >>> On 10/30/2015 01:01 PM, Mark Rutland wrote: >>>> On Fri, Oct 30, 2015 at 02:48:06PM -0400, Sinan Kaya wrote: >> >>>> The CSRT is listed under "Want", not "Never" or "Don't Care", so Linaro >>>> have certainly not said that CSRT will not be supported. If anything, >>>> they have stated that the table should be supported. >>> >>> "Want" means interesting, and probably useful, but no clear indication that >>> anyone actually needs it. At one point, we thought we might use the CSRT >>> for describing DMA, but it turns out we have not needed to. >> >> Then you are going to have either 1 or 0 DMAC for slave devices, right? > > I believe what Al means is that such hardware has not appeared > (publicly) until this time and so such situation was theoretical and > thus not covered by the Linaro wiki. Linaro had not prioritized CSRT > because it didn't seem that the need to support it would arise soon. > >> The CSRT, unfortunately, the only way how to enumerate DMAC to be used >> by slave devices. >> You may look into drivers/dma/acpi-dma.c for usage in Linux. >> >> Yes, I know about _DSD, but I don't think it will provide soon any >> other official mechanisms to what we have now. Better to ask Rafael >> and Mika. > > Thanks for the feedback. I agree that generally the plan is to use > existing tables from x86 on arm64 when possible. Please see below. > >>> However, let's make sure we're saying the same thing: the CSRT table is >>> properly defined in the kernel include/acpi/actbl2.h file so one can read >>> such a table and use it if they so choose. Nothing that we have done at >>> Linaro in the arm64 part of the kernel relies on any of the content from >>> the CSRT, nor does it preclude someone relying on that content. So, the >>> CSRT is defined, and is usable, but is just not being used -- by Linaro -- >>> at present. >> >> This sounds clear. >> >>> If that needs to change, let me know; no one has asked us to use the CSRT >>> for a DMA engine, and we have not been provided any hardware that requires >>> it. >> >> See above. > > Here's, what I believe to be the summary: > > 1). QCOM are not implementing slave device support in their current > HIDMA hardware, therefore the requirement for CSRT does not exist *at > present* for this specific driver to be merged and the discussion in > this sub-thread pertains only to a situation not affecting HIDMA. > > 2). A requirement upon the CSRT should be clarified in the various > specifications. The SBBR[0] currently "recommends" CSRT but does not > necessarily provide guidance about what kinds of system resources would > be covered by that, and so there is a potential for this to be missed. > > As one of the lead authors of certain ARM server specifications, I will > contact those involved in such and ensure that this is addressed with a > clarification in a future release. > > Thanks for raising the concern, > > Jon. > > [0] > http://infocenter.arm.com/help/topic/com.arm.doc.den0044a/Server_Base_Boot_Requirements.pdf > Apologies for creating confusion. I tried using the dma-acpi.c implementation at the beginning. It didn't quite play well with what I was trying to do with HIDMA. Since the file header said Intel, I assumed it was an Intel specific implementation. To confirm my suspicion, I looked at the Linaro wiki. I haven't seen CSRT in MUST tables. I considered WANT meaning some spec work required similar to DMAR vs. IORT table.
On Saturday 31 October 2015 02:51:46 Sinan Kaya wrote: > On 10/30/2015 5:34 AM, Arnd Bergmann wrote: > > On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote: > >> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > >> + > >> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file) > >> +{ > >> + return single_open(file, qcom_hidma_mgmt_err, inode->i_private); > >> +} > >> + > >> +static const struct file_operations qcom_hidma_mgmt_err_fops = { > >> + .open = qcom_hidma_mgmt_err_open, > >> + .read = seq_read, > >> + .llseek = seq_lseek, > >> + .release = single_release, > >> +}; > >> + > >> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file, > >> + const char __user *user_buf, size_t count, loff_t *ppos) > >> +{ > >> + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; > >> + > >> + HIDMA_RUNTIME_GET(mgmtdev); > >> + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET); > >> + HIDMA_RUNTIME_SET(mgmtdev); > >> + return count; > >> +} > >> + > >> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = { > >> + .write = qcom_hidma_mgmt_mhiderr_clr, > >> +}; > > > > Is this really just a debugging interface? If anyone would do this > > for normal operation, it needs to be a proper API. > > > This will be used by the system admin to monitor/reset the execution > state of the DMA channels. This will be the management interface. > Debugfs is probably not the right choice. I originally had sysfs but > than had some doubts. I'm open to suggestions. User interface design is unfortunately always hard, and I don't have an obvious answer for you. Using debugfs by definition means that you don't expect users to rely on ABI stability, so they should not write any automated scripts against the contents of the files. With sysfs, the opposite is true: you need to maintain compatibility for as long as anyone might rely on the current interface, and it needs to be reviewed properly and documented in Documentation/ABI/. Other options are to use ioctl(), netlink or your own virtual file system, but each of them has the same ABI requirements as sysfs. Regardless of what you pick, you also need to consider how other drivers would use the same interface: If someone else has hardware that does the same thing, we want to be able to use the same tools to access it, so you should avoid having any hardware specific data in it and keep it as generic and extensible as possible. In this particular case, that probably means you should implement the user interfaces in the dmaengine core driver, and let the specific DMA driver provide callback function pointers along with the normal ones to fill that data. > >> + dev_info(&pdev->dev, > >> + "HI-DMA engine management driver registration complete\n"); > >> + platform_set_drvdata(pdev, mgmtdev); > >> + HIDMA_RUNTIME_SET(mgmtdev); > >> + return 0; > >> +out: > >> + pm_runtime_disable(&pdev->dev); > >> + pm_runtime_put_sync_suspend(&pdev->dev); > >> + return rc; > >> +} > > > > The rest of the probe function does not register any user interface aside from > > the debugging stuff. Can you explain in the changelog how you expect the > > driver to be used in a real system? Is there another driver coming? > > I expect this driver to grow in functionality over time. Right now, it > does the global init for the DMA. After that all channels execute on > their own without depending on each other. Global init has to be done > first before attempting to do any channel initialization. > > There is also implied startup ordering requirements. I was doing this by > using channel driver with the late binding to guarantee that. > > As soon as I use module_platform_driver, the ordering gets reversed for > some reason. For the ordering requirements, it's probably best to export a symbol with the entry point and let the normal driver call into that. Using separate initcall levels is not something you should do in a normal device driver like this. What is the relation between the device nodes for the two kinds of devices? Does it make sense to model the other one as a child device of this one? That way you would trivially do the ordering by not marking this one as 'compatible="simple-bus"' and triggering the registration of the child from the parent probe function. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/2/2015 4:30 PM, Arnd Bergmann wrote: > On Saturday 31 October 2015 02:51:46 Sinan Kaya wrote: >> On 10/30/2015 5:34 AM, Arnd Bergmann wrote: >>> On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote: >>>> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt >>>> + >>>> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file) >>>> +{ >>>> + return single_open(file, qcom_hidma_mgmt_err, inode->i_private); >>>> +} >>>> + >>>> +static const struct file_operations qcom_hidma_mgmt_err_fops = { >>>> + .open = qcom_hidma_mgmt_err_open, >>>> + .read = seq_read, >>>> + .llseek = seq_lseek, >>>> + .release = single_release, >>>> +}; >>>> + >>>> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file, >>>> + const char __user *user_buf, size_t count, loff_t *ppos) >>>> +{ >>>> + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; >>>> + >>>> + HIDMA_RUNTIME_GET(mgmtdev); >>>> + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET); >>>> + HIDMA_RUNTIME_SET(mgmtdev); >>>> + return count; >>>> +} >>>> + >>>> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = { >>>> + .write = qcom_hidma_mgmt_mhiderr_clr, >>>> +}; >>> >>> Is this really just a debugging interface? If anyone would do this >>> for normal operation, it needs to be a proper API. >>> >> This will be used by the system admin to monitor/reset the execution >> state of the DMA channels. This will be the management interface. >> Debugfs is probably not the right choice. I originally had sysfs but >> than had some doubts. I'm open to suggestions. > > User interface design is unfortunately always hard, and I don't have > an obvious answer for you. > > Using debugfs by definition means that you don't expect users to > rely on ABI stability, so they should not write any automated scripts > against the contents of the files. > > With sysfs, the opposite is true: you need to maintain compatibility > for as long as anyone might rely on the current interface, and it > needs to be reviewed properly and documented in Documentation/ABI/. > > Other options are to use ioctl(), netlink or your own virtual file > system, but each of them has the same ABI requirements as sysfs. > > Regardless of what you pick, you also need to consider how other drivers > would use the same interface: If someone else has hardware that does > the same thing, we want to be able to use the same tools to access > it, so you should avoid having any hardware specific data in it and > keep it as generic and extensible as possible. In this particular > case, that probably means you should implement the user interfaces in > the dmaengine core driver, and let the specific DMA driver provide > callback function pointers along with the normal ones to fill that > data. > Thanks, I'll think about this. I'm inclined towards sysfs. >>>> + dev_info(&pdev->dev, >>>> + "HI-DMA engine management driver registration complete\n"); >>>> + platform_set_drvdata(pdev, mgmtdev); >>>> + HIDMA_RUNTIME_SET(mgmtdev); >>>> + return 0; >>>> +out: >>>> + pm_runtime_disable(&pdev->dev); >>>> + pm_runtime_put_sync_suspend(&pdev->dev); >>>> + return rc; >>>> +} >>> >>> The rest of the probe function does not register any user interface aside from >>> the debugging stuff. Can you explain in the changelog how you expect the >>> driver to be used in a real system? Is there another driver coming? >> >> I expect this driver to grow in functionality over time. Right now, it >> does the global init for the DMA. After that all channels execute on >> their own without depending on each other. Global init has to be done >> first before attempting to do any channel initialization. >> >> There is also implied startup ordering requirements. I was doing this by >> using channel driver with the late binding to guarantee that. >> >> As soon as I use module_platform_driver, the ordering gets reversed for >> some reason. > > For the ordering requirements, it's probably best to export a symbol > with the entry point and let the normal driver call into that. Using > separate initcall levels is not something you should do in a normal > device driver like this. > I figured this out. If the channel driver starts before the management driver; then channel reset fails. I'm handling this in the channel driver and am returning -EPROBE_DEFER. After that, management driver gets its chance to work. Then, the channel driver again. This change is in the v2 series. > What is the relation between the device nodes for the two kinds of > devices? Does it make sense to model the other one as a child device > of this one? That way you would trivially do the ordering by not marking > this one as 'compatible="simple-bus"' and triggering the registration > of the child from the parent probe function. > The required order is management driver first, channel drivers next. If the order is reversed, channel init fails. I handle this with deferred probing. I tried to keep loose binding between the management driver due to QEMU. QEMU auto-generates the devicetree entries. The guest machine just sees one devicetree object for the DMA channel but guest machine device-tree kernel does not have any management driver entity. This requires DMA channel driver to work independently in the guest machine without dependencies. > Arnd >
On Monday 02 November 2015 23:45:17 Sinan Kaya wrote: > On 11/2/2015 4:30 PM, Arnd Bergmann wrote: > > On Saturday 31 October 2015 02:51:46 Sinan Kaya wrote: > >> On 10/30/2015 5:34 AM, Arnd Bergmann wrote: > >>> On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote: > >> This will be used by the system admin to monitor/reset the execution > >> state of the DMA channels. This will be the management interface. > >> Debugfs is probably not the right choice. I originally had sysfs but > >> than had some doubts. I'm open to suggestions. > > > > User interface design is unfortunately always hard, and I don't have > > an obvious answer for you. > > > > Using debugfs by definition means that you don't expect users to > > rely on ABI stability, so they should not write any automated scripts > > against the contents of the files. > > > > With sysfs, the opposite is true: you need to maintain compatibility > > for as long as anyone might rely on the current interface, and it > > needs to be reviewed properly and documented in Documentation/ABI/. > > > > Other options are to use ioctl(), netlink or your own virtual file > > system, but each of them has the same ABI requirements as sysfs. > > > > Regardless of what you pick, you also need to consider how other drivers > > would use the same interface: If someone else has hardware that does > > the same thing, we want to be able to use the same tools to access > > it, so you should avoid having any hardware specific data in it and > > keep it as generic and extensible as possible. In this particular > > case, that probably means you should implement the user interfaces in > > the dmaengine core driver, and let the specific DMA driver provide > > callback function pointers along with the normal ones to fill that > > data. > > > Thanks, I'll think about this. I'm inclined towards sysfs. Ok. Documentation/sysfs-rules.txt has a good introduction of how this is done. > >>> The rest of the probe function does not register any user interface aside from > >>> the debugging stuff. Can you explain in the changelog how you expect the > >>> driver to be used in a real system? Is there another driver coming? > >> > >> I expect this driver to grow in functionality over time. Right now, it > >> does the global init for the DMA. After that all channels execute on > >> their own without depending on each other. Global init has to be done > >> first before attempting to do any channel initialization. > >> > >> There is also implied startup ordering requirements. I was doing this by > >> using channel driver with the late binding to guarantee that. > >> > >> As soon as I use module_platform_driver, the ordering gets reversed for > >> some reason. > > > > For the ordering requirements, it's probably best to export a symbol > > with the entry point and let the normal driver call into that. Using > > separate initcall levels is not something you should do in a normal > > device driver like this. > > > I figured this out. If the channel driver starts before the management > driver; then channel reset fails. I'm handling this in the channel > driver and am returning -EPROBE_DEFER. After that, management driver > gets its chance to work. Then, the channel driver again. This change is > in the v2 series. If you change the order in the Makefile, the management driver should always get probed first if both are built-in. When the driver is a loadable module, the ordering should work because of the way that the modules are loaded. Using the deferred probing makes sense here, so that would just be an optimization to avoid it normally. Things can still get shuffled around e.g. if the management device is deferred itself and we end up probing the channel driver first. > > What is the relation between the device nodes for the two kinds of > > devices? Does it make sense to model the other one as a child device > > of this one? That way you would trivially do the ordering by not marking > > this one as 'compatible="simple-bus"' and triggering the registration > > of the child from the parent probe function. > > > > The required order is management driver first, channel drivers next. If > the order is reversed, channel init fails. I handle this with deferred > probing. > > I tried to keep loose binding between the management driver due to QEMU. > > QEMU auto-generates the devicetree entries. The guest machine just sees > one devicetree object for the DMA channel but guest machine device-tree > kernel does not have any management driver entity. > > This requires DMA channel driver to work independently in the guest > machine without dependencies. You have a distinct "compatible" string for qemu, right? It sounds like this is not the same device if the dependencies are different, and you could just have two ways to probe the same device. The split between the two drivers still feels a little awkward overall, it might be good to give it some more thought. Would it work to describe the combination of the channel and management registers as a single device with a single driver, but the management parts being optional? That way, the management registers could be intergrated better into the dmaengine framework, to provide a consistent API to user space. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnd Bergmann wrote: > If you change the order in the Makefile, the management driver should > always get probed first if both are built-in. When the driver is a > loadable module, the ordering should work because of the way that the > modules are loaded. This sounds like something that should be commented in the Makefile. Even if you use -EPROBE_DEFER and you re-order the Makefile only so that it's less likely to be a problem, that's still worthy of a comment.
On 11/3/2015 7:42 AM, Arnd Bergmann wrote: > You have a distinct "compatible" string for qemu, right? It sounds like > this is not the same device if the dependencies are different, and > you could just have two ways to probe the same device. > No, it is the same dma channel object that gets probed by the same name like the hypervisor. The channel object gets unbound from the hypervisor, it then gets bound to VFIO. Then, eventually QEMU takes over. The channel driver does not know under which OS it is running and it works in both environments as it is without any code changes at this moment. > The split between the two drivers still feels a little awkward overall, > it might be good to give it some more thought. I see. I'd like to keep the management driver as independent as possible from the channel driver for security and functionality reasons. I need to keep the management addresses and functionality in the hypervisor only. > > Would it work to describe the combination of the channel and management > registers as a single device with a single driver, but the management > parts being optional? That way, the management registers could be > intergrated better into the dmaengine framework, to provide a consistent > API to user space. I can compile both management driver and channel driver into the same module if it sounds better. I can probe one with channel and another with the management name. I just need to be careful about not sharing any kind of data structure between them otherwise virtualization will break. I consider the management driver a client of the DMA engine API at this moment.
diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt new file mode 100644 index 0000000..81674ab --- /dev/null +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt @@ -0,0 +1,42 @@ +Qualcomm Technologies HIDMA Management interface + +Required properties: +- compatible: must contain "qcom,hidma_mgmt" +- reg: Address range for DMA device +- interrupts: Should contain the one interrupt shared by all channels +- nr-channels: Number of channels supported by this DMA controller. +- max-write: Maximum write burst in bytes. A memcpy requested is + fragmented to multiples of this amount. +- max-read: Maximum read burst in bytes. A memcpy request is + fragmented to multiples of this amount. +- max-wxactions: Maximum write transactions to perform in a burst +- max-rdactions: Maximum read transactions to perform in a burst +- max-memset-limit: Maximum memset limit +- ch-priority-#n: Priority of the channel +- ch-weight-#n: Round robin weight of the channel +Example: + + hidma-mgmt@f9984000 = { + compatible = "qcom,hidma_mgmt"; + reg = <0xf9984000 0x15000>; + interrupts = <0 94 0>; + nr-channels = 6; + max-write = 1024; + max-read = 1024; + max-wxactions = 31; + max-rdactions = 31; + max-memset-limit = 8; + ch-priority-0 = 0; + ch-priority-1 = 1; + ch-priority-2 = 1; + ch-priority-3 = 0; + ch-priority-4 = 0; + ch-priority-5 = 0; + ch-weight-0 = 1; + ch-weight-1 = 13; + ch-weight-2 = 10; + ch-weight-3 = 3; + ch-weight-4 = 4; + ch-weight-5 = 5; + }; + diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig index b458475..76a5a5e 100644 --- a/drivers/dma/Kconfig +++ b/drivers/dma/Kconfig @@ -501,6 +501,17 @@ config XGENE_DMA help Enable support for the APM X-Gene SoC DMA engine. +config QCOM_HIDMA_MGMT + bool "Qualcomm Technologies HIDMA Managment support" + select DMA_ENGINE + help + Enable support for the Qualcomm Technologies HIDMA Management. + Each DMA device requires one management interface driver + for basic initialization before QCOM_HIDMA driver can start + managing the channels. In a virtualized environment, the guest + OS would run QCOM_HIDMA driver and the hypervisor would run + the QCOM_HIDMA_MGMT driver. + config XILINX_VDMA tristate "Xilinx AXI VDMA Engine" depends on (ARCH_ZYNQ || MICROBLAZE) diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile index 7711a71..3d25ffd 100644 --- a/drivers/dma/Makefile +++ b/drivers/dma/Makefile @@ -53,6 +53,7 @@ obj-$(CONFIG_PL330_DMA) += pl330.o obj-$(CONFIG_PPC_BESTCOMM) += bestcomm/ obj-$(CONFIG_PXA_DMA) += pxa_dma.o obj-$(CONFIG_QCOM_BAM_DMA) += qcom_bam_dma.o +obj-$(CONFIG_QCOM_HIDMA_MGMT) += qcom_hidma_mgmt.o obj-$(CONFIG_RENESAS_DMA) += sh/ obj-$(CONFIG_SIRF_DMA) += sirf-dma.o obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o diff --git a/drivers/dma/qcom_hidma_mgmt.c b/drivers/dma/qcom_hidma_mgmt.c new file mode 100644 index 0000000..8fcad4d --- /dev/null +++ b/drivers/dma/qcom_hidma_mgmt.c @@ -0,0 +1,868 @@ +/* + * Qualcomm Technologies HIDMA DMA engine Management interface + * + * Copyright (c) 2014, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/dmaengine.h> +#include <linux/acpi.h> +#include <linux/of.h> +#include <linux/property.h> +#include <linux/debugfs.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/uaccess.h> +#include <linux/slab.h> +#include <linux/pm_runtime.h> +#include "qcom_hidma.h" + +#define MHICFG_OFFSET 0x10 +#define QOS_N_OFFSET 0x300 +#define CFG_OFFSET 0x400 +#define HW_PARAM_OFFSET 0x408 +#define MAX_BUS_REQ_LEN_OFFSET 0x41C +#define MAX_XACTIONS_OFFSET 0x420 +#define SW_VERSION_OFFSET 0x424 +#define CHRESET_TIMEOUUT_OFFSET 0x500 +#define MEMSET_LIMIT_OFFSET 0x600 +#define MHID_BUS_ERR0_OFFSET 0x1020 +#define MHID_BUS_ERR1_OFFSET 0x1024 +#define MHID_BUS_ERR_CLR_OFFSET 0x102C +#define EVT_BUS_ERR0_OFFSET 0x1030 +#define EVT_BUS_ERR1_OFFSET 0x1034 +#define EVT_BUS_ERR_CLR_OFFSET 0x103C +#define IDE_BUS_ERR0_OFFSET 0x1040 +#define IDE_BUS_ERR1_OFFSET 0x1044 +#define IDE_BUS_ERR2_OFFSET 0x1048 +#define IDE_BUS_ERR_CLR_OFFSET 0x104C +#define ODE_BUS_ERR0_OFFSET 0x1050 +#define ODE_BUS_ERR1_OFFSET 0x1054 +#define ODE_BUS_ERR2_OFFSET 0x1058 +#define ODE_BUS_ERR_CLR_OFFSET 0x105C +#define MSI_BUS_ERR0_OFFSET 0x1060 +#define MSI_BUS_ERR_CLR_OFFSET 0x106C +#define TRE_ERR0_OFFSET 0x1070 +#define TRE_ERR_CLR_OFFSET 0x107C +#define HW_EVENTS_CFG_OFFSET 0x1080 + +#define HW_EVENTS_CFG_MASK 0xFF +#define TRE_ERR_TRCHID_MASK 0xF +#define TRE_ERR_EVRIDX_MASK 0xFF +#define TRE_ERR_TYPE_MASK 0xFF +#define MSI_ERR_RESP_MASK 0xFF +#define MSI_ERR_TRCHID_MASK 0xFF +#define ODE_ERR_REQLEN_MASK 0xFFFF +#define ODE_ERR_RESP_MASK 0xFF +#define ODE_ERR_TRCHID_MASK 0xFF +#define IDE_ERR_REQLEN_MASK 0xFFFF +#define IDE_ERR_RESP_MASK 0xFF +#define IDE_ERR_TRCHID_MASK 0xFF +#define EVT_ERR_RESP_MASK 0xFF +#define EVT_ERR_TRCHID_MASK 0xFF +#define MHID_ERR_RESP_MASK 0xFF +#define MHID_ERR_TRCHID_MASK 0xFF +#define MEMSET_LIMIT_MASK 0x1F +#define MAX_WR_XACTIONS_MASK 0x1F +#define MAX_RD_XACTIONS_MASK 0x1F +#define MAX_JOBSIZE_MASK 0xFF +#define MAX_COIDX_MASK 0xFF +#define TREQ_CAPACITY_MASK 0xFF +#define WEIGHT_MASK 0x7F +#define TREQ_LIMIT_MASK 0x1FF +#define NR_CHANNEL_MASK 0xFFFF +#define MAX_BUS_REQ_LEN_MASK 0xFFFF +#define CHRESET_TIMEOUUT_MASK 0xFFFFF + +#define TRE_ERR_TRCHID_BIT_POS 28 +#define TRE_ERR_IEOB_BIT_POS 16 +#define TRE_ERR_EVRIDX_BIT_POS 8 +#define MSI_ERR_RESP_BIT_POS 8 +#define ODE_ERR_REQLEN_BIT_POS 16 +#define ODE_ERR_RESP_BIT_POS 8 +#define IDE_ERR_REQLEN_BIT_POS 16 +#define IDE_ERR_RESP_BIT_POS 8 +#define EVT_ERR_RESP_BIT_POS 8 +#define MHID_ERR_RESP_BIT_POS 8 +#define MAX_WR_XACTIONS_BIT_POS 16 +#define TREQ_CAPACITY_BIT_POS 8 +#define MAX_JOB_SIZE_BIT_POS 16 +#define NR_EV_CHANNEL_BIT_POS 16 +#define MAX_BUS_WR_REQ_BIT_POS 16 +#define WRR_BIT_POS 8 +#define PRIORITY_BIT_POS 15 +#define TREQ_LIMIT_BIT_POS 16 +#define TREQ_LIMIT_EN_BIT_POS 23 +#define STOP_BIT_POS 24 + +#define MODULE_NAME "hidma-mgmt" +#define PREFIX MODULE_NAME ": " +#define AUTOSUSPEND_TIMEOUT 2000 + +#define HIDMA_RUNTIME_GET(dmadev) \ +do { \ + atomic_inc(&(dmadev)->pm_counter); \ + TRC_PM(&(dmadev)->pdev->dev, \ + "%s:%d pm_runtime_get %d\n", __func__, __LINE__,\ + atomic_read(&(dmadev)->pm_counter)); \ + pm_runtime_get_sync(&(dmadev)->pdev->dev); \ +} while (0) + +#define HIDMA_RUNTIME_SET(dmadev) \ +do { \ + atomic_dec(&(dmadev)->pm_counter); \ + TRC_PM(&(dmadev)->pdev->dev, \ + "%s:%d pm_runtime_put_autosuspend:%d\n", \ + __func__, __LINE__, \ + atomic_read(&(dmadev)->pm_counter)); \ + pm_runtime_mark_last_busy(&(dmadev)->pdev->dev); \ + pm_runtime_put_autosuspend(&(dmadev)->pdev->dev); \ +} while (0) + +struct qcom_hidma_mgmt_dev { + u8 max_wr_xactions; + u8 max_rd_xactions; + u8 max_memset_limit; + u16 max_write_request; + u16 max_read_request; + u16 nr_channels; + u32 chreset_timeout; + u32 sw_version; + u8 *priority; + u8 *weight; + + atomic_t pm_counter; + /* Hardware device constants */ + dma_addr_t dev_physaddr; + void __iomem *dev_virtaddr; + resource_size_t dev_addrsize; + + struct dentry *debugfs; + struct dentry *info; + struct dentry *err; + struct dentry *mhid_errclr; + struct dentry *evt_errclr; + struct dentry *ide_errclr; + struct dentry *ode_errclr; + struct dentry *msi_errclr; + struct dentry *tre_errclr; + struct dentry *evt_ena; + struct platform_device *pdev; +}; + +static unsigned int debug_pm; +module_param(debug_pm, uint, 0644); +MODULE_PARM_DESC(debug_pm, + "debug runtime power management transitions (default: 0)"); + +#define TRC_PM(...) do { \ + if (debug_pm) \ + dev_info(__VA_ARGS__); \ + } while (0) + + +#if IS_ENABLED(CONFIG_DEBUG_FS) + +#define HIDMA_SHOW(dma, name) \ + seq_printf(s, #name "=0x%x\n", dma->name) + +#define HIDMA_READ_SHOW(dma, name, offset) \ + do { \ + u32 val; \ + val = readl(dma->dev_virtaddr + offset); \ + seq_printf(s, name "=0x%x\n", val); \ + } while (0) + +/** + * qcom_hidma_mgmt_info: display HIDMA device info + * + * Display the info for the current HIDMA device. + */ +static int qcom_hidma_mgmt_info(struct seq_file *s, void *unused) +{ + struct qcom_hidma_mgmt_dev *mgmtdev = s->private; + u32 val; + int i; + + HIDMA_RUNTIME_GET(mgmtdev); + HIDMA_SHOW(mgmtdev, sw_version); + + val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET); + seq_printf(s, "ENABLE=%d\n", val & 0x1); + + val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET); + seq_printf(s, "reset_timeout=%d\n", val & CHRESET_TIMEOUUT_MASK); + + val = readl(mgmtdev->dev_virtaddr + MHICFG_OFFSET); + seq_printf(s, "nr_event_channel=%d\n", + (val >> NR_EV_CHANNEL_BIT_POS) & NR_CHANNEL_MASK); + seq_printf(s, "nr_tr_channel=%d\n", (val & NR_CHANNEL_MASK)); + seq_printf(s, "nr_virt_tr_channel=%d\n", mgmtdev->nr_channels); + seq_printf(s, "dev_virtaddr=%p\n", &mgmtdev->dev_virtaddr); + seq_printf(s, "dev_physaddr=%pap\n", &mgmtdev->dev_physaddr); + seq_printf(s, "dev_addrsize=%pap\n", &mgmtdev->dev_addrsize); + + val = readl(mgmtdev->dev_virtaddr + MEMSET_LIMIT_OFFSET); + seq_printf(s, "MEMSET_LIMIT_OFFSET=%d\n", val & MEMSET_LIMIT_MASK); + + val = readl(mgmtdev->dev_virtaddr + HW_PARAM_OFFSET); + seq_printf(s, "MAX_JOB_SIZE=%d\n", + (val >> MAX_JOB_SIZE_BIT_POS) & MAX_JOBSIZE_MASK); + seq_printf(s, "TREQ_CAPACITY=%d\n", + (val >> TREQ_CAPACITY_BIT_POS) & TREQ_CAPACITY_MASK); + seq_printf(s, "MAX_COIDX_DEPTH=%d\n", val & MAX_COIDX_MASK); + + val = readl(mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET); + seq_printf(s, "MAX_BUS_WR_REQ_LEN=%d\n", + (val >> MAX_BUS_WR_REQ_BIT_POS) & MAX_BUS_REQ_LEN_MASK); + seq_printf(s, "MAX_BUS_RD_REQ_LEN=%d\n", val & MAX_BUS_REQ_LEN_MASK); + + val = readl(mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET); + seq_printf(s, "MAX_WR_XACTIONS=%d\n", + (val >> MAX_WR_XACTIONS_BIT_POS) & MAX_WR_XACTIONS_MASK); + seq_printf(s, "MAX_RD_XACTIONS=%d\n", val & MAX_RD_XACTIONS_MASK); + + for (i = 0; i < mgmtdev->nr_channels; i++) { + void __iomem *offset; + + offset = mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i); + val = readl(offset); + + seq_printf(s, "CH#%d STOP=%d\n", + i, (val & (1 << STOP_BIT_POS)) ? 1 : 0); + seq_printf(s, "CH#%d TREQ LIMIT EN=%d\n", i, + (val & (1 << TREQ_LIMIT_EN_BIT_POS)) ? 1 : 0); + seq_printf(s, "CH#%d TREQ LIMIT=%d\n", + i, (val >> TREQ_LIMIT_BIT_POS) & TREQ_LIMIT_MASK); + seq_printf(s, "CH#%d priority=%d\n", i, + (val & (1 << PRIORITY_BIT_POS)) ? 1 : 0); + seq_printf(s, "CH#%d WRR=%d\n", i, + (val >> WRR_BIT_POS) & WEIGHT_MASK); + seq_printf(s, "CH#%d USE_DLA=%d\n", i, (val & 1) ? 1 : 0); + } + HIDMA_RUNTIME_SET(mgmtdev); + + return 0; +} + +static int qcom_hidma_mgmt_info_open(struct inode *inode, struct file *file) +{ + return single_open(file, qcom_hidma_mgmt_info, inode->i_private); +} + +static const struct file_operations qcom_hidma_mgmt_fops = { + .open = qcom_hidma_mgmt_info_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +/** + * qcom_hidma_mgmt_err: display HIDMA error info + * + * Display the error info for the current HIDMA device. + */ +static int qcom_hidma_mgmt_err(struct seq_file *s, void *unused) +{ + u32 val; + struct qcom_hidma_mgmt_dev *mgmtdev = s->private; + + HIDMA_RUNTIME_GET(mgmtdev); + val = readl(mgmtdev->dev_virtaddr + MHID_BUS_ERR0_OFFSET); + seq_printf(s, "MHID TR_CHID=%d\n", val & MHID_ERR_TRCHID_MASK); + seq_printf(s, "MHID RESP_ERROR=%d\n", + (val >> MHID_ERR_RESP_BIT_POS) & MHID_ERR_RESP_MASK); + HIDMA_READ_SHOW(mgmtdev, "MHID READ_PTR", MHID_BUS_ERR1_OFFSET); + + val = readl(mgmtdev->dev_virtaddr + EVT_BUS_ERR0_OFFSET); + seq_printf(s, "EVT TR_CHID=%d\n", val & EVT_ERR_TRCHID_MASK); + seq_printf(s, "EVT RESP_ERROR=%d\n", + (val >> EVT_ERR_RESP_BIT_POS) & EVT_ERR_RESP_MASK); + HIDMA_READ_SHOW(mgmtdev, "EVT WRITE_PTR", EVT_BUS_ERR1_OFFSET); + + val = readl(mgmtdev->dev_virtaddr + IDE_BUS_ERR0_OFFSET); + seq_printf(s, "IDE TR_CHID=%d\n", val & IDE_ERR_TRCHID_MASK); + seq_printf(s, "IDE RESP_ERROR=%d\n", + (val >> IDE_ERR_RESP_BIT_POS) & IDE_ERR_RESP_MASK); + seq_printf(s, "IDE REQ_LENGTH=%d\n", + (val >> IDE_ERR_REQLEN_BIT_POS) & IDE_ERR_REQLEN_MASK); + HIDMA_READ_SHOW(mgmtdev, "IDE ADDR_LSB", IDE_BUS_ERR1_OFFSET); + HIDMA_READ_SHOW(mgmtdev, "IDE ADDR_MSB", IDE_BUS_ERR2_OFFSET); + + val = readl(mgmtdev->dev_virtaddr + ODE_BUS_ERR0_OFFSET); + seq_printf(s, "ODE TR_CHID=%d\n", val & ODE_ERR_TRCHID_MASK); + seq_printf(s, "ODE RESP_ERROR=%d\n", + (val >> ODE_ERR_RESP_BIT_POS) & ODE_ERR_RESP_MASK); + seq_printf(s, "ODE REQ_LENGTH=%d\n", + (val >> ODE_ERR_REQLEN_BIT_POS) & ODE_ERR_REQLEN_MASK); + HIDMA_READ_SHOW(mgmtdev, "ODE ADDR_LSB", ODE_BUS_ERR1_OFFSET); + HIDMA_READ_SHOW(mgmtdev, "ODE ADDR_MSB", ODE_BUS_ERR2_OFFSET); + + val = readl(mgmtdev->dev_virtaddr + MSI_BUS_ERR0_OFFSET); + seq_printf(s, "MSI TR_CHID=%d\n", val & MSI_ERR_TRCHID_MASK); + seq_printf(s, "MSI RESP_ERROR=%d\n", + (val >> MSI_ERR_RESP_BIT_POS) & MSI_ERR_RESP_MASK); + + val = readl(mgmtdev->dev_virtaddr + TRE_ERR0_OFFSET); + seq_printf(s, "TRE TRE_TYPE=%d\n", val & TRE_ERR_TYPE_MASK); + seq_printf(s, "TRE TRE_EVRIDX=%d\n", + (val >> TRE_ERR_EVRIDX_BIT_POS) & TRE_ERR_EVRIDX_MASK); + seq_printf(s, "TRE TRE_IEOB=%d\n", + (val >> TRE_ERR_IEOB_BIT_POS) & 1); + seq_printf(s, "TRE TRCHID=%d\n", + (val >> TRE_ERR_TRCHID_BIT_POS) & TRE_ERR_TRCHID_MASK); + + HIDMA_READ_SHOW(mgmtdev, "HW_EVENTS_CFG_OFFSET", + HW_EVENTS_CFG_OFFSET); + + HIDMA_RUNTIME_SET(mgmtdev); + return 0; +} + +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file) +{ + return single_open(file, qcom_hidma_mgmt_err, inode->i_private); +} + +static const struct file_operations qcom_hidma_mgmt_err_fops = { + .open = qcom_hidma_mgmt_err_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file, + const char __user *user_buf, size_t count, loff_t *ppos) +{ + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; + + HIDMA_RUNTIME_GET(mgmtdev); + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET); + HIDMA_RUNTIME_SET(mgmtdev); + return count; +} + +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = { + .write = qcom_hidma_mgmt_mhiderr_clr, +}; + +static ssize_t qcom_hidma_mgmt_evterr_clr(struct file *file, + const char __user *user_buf, size_t count, loff_t *ppos) +{ + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; + + HIDMA_RUNTIME_GET(mgmtdev); + writel(1, mgmtdev->dev_virtaddr + EVT_BUS_ERR_CLR_OFFSET); + HIDMA_RUNTIME_SET(mgmtdev); + return count; +} + +static const struct file_operations qcom_hidma_mgmt_evterr_clrfops = { + .write = qcom_hidma_mgmt_evterr_clr, +}; + +static ssize_t qcom_hidma_mgmt_ideerr_clr(struct file *file, + const char __user *user_buf, size_t count, loff_t *ppos) +{ + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; + + HIDMA_RUNTIME_GET(mgmtdev); + writel(1, mgmtdev->dev_virtaddr + IDE_BUS_ERR_CLR_OFFSET); + HIDMA_RUNTIME_SET(mgmtdev); + return count; +} + +static const struct file_operations qcom_hidma_mgmt_ideerr_clrfops = { + .write = qcom_hidma_mgmt_ideerr_clr, +}; + +static ssize_t qcom_hidma_mgmt_odeerr_clr(struct file *file, + const char __user *user_buf, size_t count, loff_t *ppos) +{ + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; + + HIDMA_RUNTIME_GET(mgmtdev); + writel(1, mgmtdev->dev_virtaddr + ODE_BUS_ERR_CLR_OFFSET); + HIDMA_RUNTIME_SET(mgmtdev); + return count; +} + +static const struct file_operations qcom_hidma_mgmt_odeerr_clrfops = { + .write = qcom_hidma_mgmt_odeerr_clr, +}; + +static ssize_t qcom_hidma_mgmt_msierr_clr(struct file *file, + const char __user *user_buf, size_t count, loff_t *ppos) +{ + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; + + HIDMA_RUNTIME_GET(mgmtdev); + writel(1, mgmtdev->dev_virtaddr + MSI_BUS_ERR_CLR_OFFSET); + HIDMA_RUNTIME_SET(mgmtdev); + return count; +} + +static const struct file_operations qcom_hidma_mgmt_msierr_clrfops = { + .write = qcom_hidma_mgmt_msierr_clr, +}; + +static ssize_t qcom_hidma_mgmt_treerr_clr(struct file *file, + const char __user *user_buf, size_t count, loff_t *ppos) +{ + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; + + HIDMA_RUNTIME_GET(mgmtdev); + writel(1, mgmtdev->dev_virtaddr + TRE_ERR_CLR_OFFSET); + HIDMA_RUNTIME_SET(mgmtdev); + return count; +} + +static const struct file_operations qcom_hidma_mgmt_treerr_clrfops = { + .write = qcom_hidma_mgmt_treerr_clr, +}; + +static ssize_t qcom_hidma_mgmt_evtena(struct file *file, + const char __user *user_buf, size_t count, loff_t *ppos) +{ + char temp_buf[16+1]; + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; + u32 event; + ssize_t ret; + unsigned long val; + + temp_buf[16] = '\0'; + if (copy_from_user(temp_buf, user_buf, min_t(int, count, 16))) + goto out; + + ret = kstrtoul(temp_buf, 16, &val); + if (ret) { + pr_warn(PREFIX "unknown event\n"); + goto out; + } + + event = (u32)val & HW_EVENTS_CFG_MASK; + + HIDMA_RUNTIME_GET(mgmtdev); + writel(event, mgmtdev->dev_virtaddr + HW_EVENTS_CFG_OFFSET); + HIDMA_RUNTIME_SET(mgmtdev); +out: + return count; +} + +static const struct file_operations qcom_hidma_mgmt_evtena_fops = { + .write = qcom_hidma_mgmt_evtena, +}; + +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev) +{ + debugfs_remove(mgmtdev->evt_ena); + debugfs_remove(mgmtdev->tre_errclr); + debugfs_remove(mgmtdev->msi_errclr); + debugfs_remove(mgmtdev->ode_errclr); + debugfs_remove(mgmtdev->ide_errclr); + debugfs_remove(mgmtdev->evt_errclr); + debugfs_remove(mgmtdev->mhid_errclr); + debugfs_remove(mgmtdev->err); + debugfs_remove(mgmtdev->info); + debugfs_remove(mgmtdev->debugfs); +} + +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev) +{ + int rc = 0; + + mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev), + NULL); + if (!mgmtdev->debugfs) { + rc = -ENODEV; + return rc; + } + + mgmtdev->info = debugfs_create_file("info", S_IRUGO, + mgmtdev->debugfs, mgmtdev, &qcom_hidma_mgmt_fops); + if (!mgmtdev->info) { + rc = -ENOMEM; + goto cleanup; + } + + mgmtdev->err = debugfs_create_file("err", S_IRUGO, + mgmtdev->debugfs, mgmtdev, + &qcom_hidma_mgmt_err_fops); + if (!mgmtdev->err) { + rc = -ENOMEM; + goto cleanup; + } + + mgmtdev->mhid_errclr = debugfs_create_file("mhiderrclr", S_IWUSR, + mgmtdev->debugfs, mgmtdev, + &qcom_hidma_mgmt_mhiderr_clrfops); + if (!mgmtdev->mhid_errclr) { + rc = -ENOMEM; + goto cleanup; + } + + mgmtdev->evt_errclr = debugfs_create_file("evterrclr", S_IWUSR, + mgmtdev->debugfs, mgmtdev, + &qcom_hidma_mgmt_evterr_clrfops); + if (!mgmtdev->evt_errclr) { + rc = -ENOMEM; + goto cleanup; + } + + mgmtdev->ide_errclr = debugfs_create_file("ideerrclr", S_IWUSR, + mgmtdev->debugfs, mgmtdev, + &qcom_hidma_mgmt_ideerr_clrfops); + if (!mgmtdev->ide_errclr) { + rc = -ENOMEM; + goto cleanup; + } + + mgmtdev->ode_errclr = debugfs_create_file("odeerrclr", S_IWUSR, + mgmtdev->debugfs, mgmtdev, + &qcom_hidma_mgmt_odeerr_clrfops); + if (!mgmtdev->ode_errclr) { + rc = -ENOMEM; + goto cleanup; + } + + mgmtdev->msi_errclr = debugfs_create_file("msierrclr", S_IWUSR, + mgmtdev->debugfs, mgmtdev, + &qcom_hidma_mgmt_msierr_clrfops); + if (!mgmtdev->msi_errclr) { + rc = -ENOMEM; + goto cleanup; + } + + mgmtdev->tre_errclr = debugfs_create_file("treerrclr", S_IWUSR, + mgmtdev->debugfs, mgmtdev, + &qcom_hidma_mgmt_treerr_clrfops); + if (!mgmtdev->tre_errclr) { + rc = -ENOMEM; + goto cleanup; + } + + mgmtdev->evt_ena = debugfs_create_file("evtena", S_IWUSR, + mgmtdev->debugfs, mgmtdev, + &qcom_hidma_mgmt_evtena_fops); + if (!mgmtdev->evt_ena) { + rc = -ENOMEM; + goto cleanup; + } + + return 0; +cleanup: + qcom_hidma_mgmt_debug_uninit(mgmtdev); + return rc; +} +#else +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev) +{ +} +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev) +{ + return 0; +} +#endif + +static irqreturn_t qcom_hidma_mgmt_irq_handler(int irq, void *arg) +{ + /* TODO: handle irq here */ + return IRQ_HANDLED; +} + +static int qcom_hidma_mgmt_setup(struct qcom_hidma_mgmt_dev *mgmtdev) +{ + u32 val; + int i; + + val = readl(mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET); + + if (mgmtdev->max_write_request) { + val = val & + ~(MAX_BUS_REQ_LEN_MASK << MAX_BUS_WR_REQ_BIT_POS); + val = val | + (mgmtdev->max_write_request << MAX_BUS_WR_REQ_BIT_POS); + } + if (mgmtdev->max_read_request) { + val = val & ~(MAX_BUS_REQ_LEN_MASK); + val = val | (mgmtdev->max_read_request); + } + writel(val, mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET); + + val = readl(mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET); + if (mgmtdev->max_wr_xactions) { + val = val & + ~(MAX_WR_XACTIONS_MASK << MAX_WR_XACTIONS_BIT_POS); + val = val | + (mgmtdev->max_wr_xactions << MAX_WR_XACTIONS_BIT_POS); + } + if (mgmtdev->max_rd_xactions) { + val = val & ~(MAX_RD_XACTIONS_MASK); + val = val | (mgmtdev->max_rd_xactions); + } + writel(val, mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET); + + val = readl(mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET); + mgmtdev->max_write_request = + (val >> MAX_BUS_WR_REQ_BIT_POS) & MAX_BUS_REQ_LEN_MASK; + mgmtdev->max_read_request = val & MAX_BUS_REQ_LEN_MASK; + + val = readl(mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET); + mgmtdev->max_wr_xactions = + (val >> MAX_WR_XACTIONS_BIT_POS) & MAX_WR_XACTIONS_MASK; + mgmtdev->max_rd_xactions = val & MAX_RD_XACTIONS_MASK; + + mgmtdev->sw_version = readl(mgmtdev->dev_virtaddr + SW_VERSION_OFFSET); + + for (i = 0; i < mgmtdev->nr_channels; i++) { + val = readl(mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i)); + val = val & ~(1 << PRIORITY_BIT_POS); + val = val | + ((mgmtdev->priority[i] & 0x1) << PRIORITY_BIT_POS); + val = val & ~(WEIGHT_MASK << WRR_BIT_POS); + val = val + | ((mgmtdev->weight[i] & WEIGHT_MASK) << WRR_BIT_POS); + writel(val, mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i)); + } + + if (mgmtdev->chreset_timeout > 0) { + val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET); + val = val & ~CHRESET_TIMEOUUT_MASK; + val = val | (mgmtdev->chreset_timeout & CHRESET_TIMEOUUT_MASK); + writel(val, mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET); + } + val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET); + mgmtdev->chreset_timeout = val & CHRESET_TIMEOUUT_MASK; + + if (mgmtdev->max_memset_limit > 0) { + val = readl(mgmtdev->dev_virtaddr + MEMSET_LIMIT_OFFSET); + val = val & ~MEMSET_LIMIT_MASK; + val = val | (mgmtdev->max_memset_limit & MEMSET_LIMIT_MASK); + writel(val, mgmtdev->dev_virtaddr + MEMSET_LIMIT_OFFSET); + } + val = readl(mgmtdev->dev_virtaddr + MEMSET_LIMIT_OFFSET); + mgmtdev->max_memset_limit = val & MEMSET_LIMIT_MASK; + + val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET); + val = val | 1; + writel(val, mgmtdev->dev_virtaddr + CFG_OFFSET); + + return 0; +} + +static int qcom_hidma_mgmt_probe(struct platform_device *pdev) +{ + struct resource *dma_resource; + int irq; + int rc, i; + struct qcom_hidma_mgmt_dev *mgmtdev; + + pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + dma_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!dma_resource) { + dev_err(&pdev->dev, "No memory resources found\n"); + rc = -ENODEV; + goto out; + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(&pdev->dev, "irq resources not found\n"); + rc = -ENODEV; + goto out; + } + + mgmtdev = devm_kzalloc(&pdev->dev, sizeof(*mgmtdev), GFP_KERNEL); + if (!mgmtdev) { + rc = -ENOMEM; + goto out; + } + + mgmtdev->pdev = pdev; + HIDMA_RUNTIME_GET(mgmtdev); + + rc = devm_request_irq(&pdev->dev, irq, qcom_hidma_mgmt_irq_handler, + IRQF_SHARED, "qcom-hidmamgmt", mgmtdev); + if (rc) { + dev_err(&pdev->dev, "irq registration failed: %d\n", irq); + goto out; + } + + mgmtdev->dev_physaddr = dma_resource->start; + mgmtdev->dev_addrsize = resource_size(dma_resource); + + dev_dbg(&pdev->dev, "dev_physaddr:%pa\n", &mgmtdev->dev_physaddr); + dev_dbg(&pdev->dev, "dev_addrsize:%pa\n", &mgmtdev->dev_addrsize); + + mgmtdev->dev_virtaddr = devm_ioremap_resource(&pdev->dev, + dma_resource); + if (IS_ERR(mgmtdev->dev_virtaddr)) { + dev_err(&pdev->dev, "can't map i/o memory at %pa\n", + &mgmtdev->dev_physaddr); + rc = -ENOMEM; + goto out; + } + + if (device_property_read_u16(&pdev->dev, "nr-channels", + &mgmtdev->nr_channels)) { + dev_err(&pdev->dev, "number of channels missing\n"); + rc = -EINVAL; + goto out; + } + + device_property_read_u16(&pdev->dev, "max-write", + &mgmtdev->max_write_request); + if ((mgmtdev->max_write_request != 128) && + (mgmtdev->max_write_request != 256) && + (mgmtdev->max_write_request != 512) && + (mgmtdev->max_write_request != 1024)) { + dev_err(&pdev->dev, "invalid write request %d\n", + mgmtdev->max_write_request); + rc = -EINVAL; + goto out; + } + + device_property_read_u16(&pdev->dev, "max-read", + &mgmtdev->max_read_request); + if ((mgmtdev->max_read_request != 128) && + (mgmtdev->max_read_request != 256) && + (mgmtdev->max_read_request != 512) && + (mgmtdev->max_read_request != 1024)) { + dev_err(&pdev->dev, "invalid read request %d\n", + mgmtdev->max_read_request); + rc = -EINVAL; + goto out; + } + + device_property_read_u8(&pdev->dev, "max-wxactions", + &mgmtdev->max_wr_xactions); + + device_property_read_u8(&pdev->dev, "max-rdactions", + &mgmtdev->max_rd_xactions); + + device_property_read_u8(&pdev->dev, "max-memset-limit", + &mgmtdev->max_memset_limit); + + /* needs to be at least one */ + if (mgmtdev->max_memset_limit == 0) + mgmtdev->max_memset_limit = 1; + + mgmtdev->priority = devm_kcalloc(&pdev->dev, + mgmtdev->nr_channels, sizeof(*mgmtdev->priority), GFP_KERNEL); + if (!mgmtdev->priority) { + rc = -ENOMEM; + goto out; + } + + mgmtdev->weight = devm_kcalloc(&pdev->dev, + mgmtdev->nr_channels, sizeof(*mgmtdev->weight), GFP_KERNEL); + if (!mgmtdev->weight) { + rc = -ENOMEM; + goto out; + } + + for (i = 0; i < mgmtdev->nr_channels; i++) { + char name[30]; + + sprintf(name, "ch-priority-%d", i); + device_property_read_u8(&pdev->dev, name, + &mgmtdev->priority[i]); + + sprintf(name, "ch-weight-%d", i); + device_property_read_u8(&pdev->dev, name, + &mgmtdev->weight[i]); + + if (mgmtdev->weight[i] > 15) { + dev_err(&pdev->dev, "max value of weight can be 15.\n"); + rc = -EINVAL; + goto out; + } + + /* weight needs to be at least one */ + if (mgmtdev->weight[i] == 0) + mgmtdev->weight[i] = 1; + } + + rc = qcom_hidma_mgmt_setup(mgmtdev); + if (rc) { + dev_err(&pdev->dev, "setup failed\n"); + goto out; + } + + rc = qcom_hidma_mgmt_debug_init(mgmtdev); + if (rc) { + dev_err(&pdev->dev, "debugfs init failed\n"); + goto out; + } + + dev_info(&pdev->dev, + "HI-DMA engine management driver registration complete\n"); + platform_set_drvdata(pdev, mgmtdev); + HIDMA_RUNTIME_SET(mgmtdev); + return 0; +out: + pm_runtime_disable(&pdev->dev); + pm_runtime_put_sync_suspend(&pdev->dev); + return rc; +} + +static int qcom_hidma_mgmt_remove(struct platform_device *pdev) +{ + struct qcom_hidma_mgmt_dev *mgmtdev = platform_get_drvdata(pdev); + + HIDMA_RUNTIME_GET(mgmtdev); + qcom_hidma_mgmt_debug_uninit(mgmtdev); + pm_runtime_put_sync_suspend(&pdev->dev); + pm_runtime_disable(&pdev->dev); + + dev_info(&pdev->dev, "HI-DMA engine management driver removed\n"); + return 0; +} + +#if IS_ENABLED(CONFIG_ACPI) +static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = { + {"QCOM8060"}, + {}, +}; +#endif + +static const struct of_device_id qcom_hidma_mgmt_match[] = { + { .compatible = "qcom,hidma_mgmt", }, + {}, +}; +MODULE_DEVICE_TABLE(of, qcom_hidma_mgmt_match); + +static struct platform_driver qcom_hidma_mgmt_driver = { + .probe = qcom_hidma_mgmt_probe, + .remove = qcom_hidma_mgmt_remove, + .driver = { + .name = MODULE_NAME, + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(qcom_hidma_mgmt_match), + .acpi_match_table = ACPI_PTR(qcom_hidma_mgmt_acpi_ids), + }, +}; + +static int __init qcom_hidma_mgmt_init(void) +{ + return platform_driver_register(&qcom_hidma_mgmt_driver); +} +device_initcall(qcom_hidma_mgmt_init); + +static void __exit qcom_hidma_mgmt_exit(void) +{ + platform_driver_unregister(&qcom_hidma_mgmt_driver); +} +module_exit(qcom_hidma_mgmt_exit);
The Qualcomm Technologies HIDMA device has been designed to support virtualization technology. The driver has been divided into two to follow the hardware design. The management driver is executed in hypervisor context and is the main managment for all channels provided by the device. The channel driver is exected in the guest OS context. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- .../devicetree/bindings/dma/qcom_hidma_mgmt.txt | 42 + drivers/dma/Kconfig | 11 + drivers/dma/Makefile | 1 + drivers/dma/qcom_hidma_mgmt.c | 868 +++++++++++++++++++++ 4 files changed, 922 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt create mode 100644 drivers/dma/qcom_hidma_mgmt.c