[1/2] dma: add Qualcomm Technologies HIDMA management driver

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

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.