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

[Sinan Kaya]

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@xxxxxxxxxxxxxx>
> > ---
> >   .../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.

--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/