diff mbox

[V12,2/7] dma: hidma: Add Device Tree support

Message ID 1452523550-8920-3-git-send-email-okaya@codeaurora.org (mailing list archive)
State Superseded
Headers show

Commit Message

Sinan Kaya Jan. 11, 2016, 2:45 p.m. UTC
Add documentation for the Qualcomm Technologies HIDMA driver.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/dma/qcom_hidma_mgmt.txt    | 79 ++++++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt

Comments

Mark Rutland Jan. 15, 2016, 3:16 p.m. UTC | #1
On Mon, Jan 11, 2016 at 09:45:42AM -0500, Sinan Kaya wrote:
> Add documentation for the Qualcomm Technologies HIDMA driver.

s/driver/binding/


> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/dma/qcom_hidma_mgmt.txt    | 79 ++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
> 
> 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..0e6ed1f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
> @@ -0,0 +1,79 @@
> +Qualcomm Technologies HIDMA Management interface
> +
> +Qualcomm Technologies HIDMA is a high speed DMA device. It only supports
> +memcpy and memset capabilities. It has been designed for virtualized
> +environments.
> +
> +Each HIDMA HW instance consists of multiple DMA channels. These channels
> +share the same bandwidth. The bandwidth utilization can be parititioned
> +among channels based on the priority and weight assignments.
> +
> +There are only two priority levels and 15 weigh assignments possible.
> +
> +Other parameters here determine how much of the system bus this HIDMA
> +instance can use like maximum read/write request and and number of bytes to
> +read/write in a single burst.
> +
> +Main node required properties:
> +- compatible: "qcom,hidma-mgmt-1.0";
> +- reg: Address range for DMA device
> +- dma-channels: Number of channels supported by this DMA controller.
> +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is
> +  fragmented to multiples of this amount.
> +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is
> +  fragmented to multiples of this amount.
> +- max-write-transactions: Maximum write transactions to perform in a burst
> +- max-read-transactions: Maximum read transactions to perform in a burst

Just to check, where do these max-* values come from?

Are they some correctness requirement of the bus this is attached to?

Are they tuning values?

The latter doesn't really belong in the DT. Given they're writeable from
the driver, it seems like that's what they are...

> +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC.

I'm not sure what this means. Could you elaborate on this is?

> +
> +Sub-nodes:
> +
> +HIDMA has one or more DMA channels that are used to move data from one
> +memory location to another.
> +
> +Each DMA channel is described as a sub-node under the management object.
> +When a transfer channel is given to the guest operating system, only the channel
> +object is created. The drivers have support for both flat and hierarchical
> +configuration.

Don't mention drivers here.

All you need to state is that when the OS is not in control of the
management interface (i.e. it's a guest), the channel nodes appear on
their own, not under a management node.

Other than the above questions, this looks ok to me.

Thanks,
Mark.

> +
> +Required properties:
> +- compatible: must contain "qcom,hidma-1.0"
> +- reg: Addresses for the transfer and event channel
> +- interrupts: Should contain the event interrupt
> +- desc-count: Number of asynchronous requests this channel can handle
> +- channel-index: The HW event channel completions will be delivered.
> +
> +Example:
> +
> +Hypervisor OS configuration:
> +
> +	hidma-mgmt@f9984000 = {
> +		compatible = "qcom,hidma-mgmt-1.0";
> +		reg = <0xf9984000 0x15000>;
> +		dma-channels = <6>;
> +		max-write-burst-bytes = <1024>;
> +		max-read-burst-bytes = <1024>;
> +		max-write-transactions = <31>;
> +		max-read-transactions = <31>;
> +		channel-reset-timeout-cycles = <0x500>;
> +
> +		hidma_24: dma-controller@0x5c050000 {
> +			compatible = "qcom,hidma-1.0";
> +			reg = <0 0x5c050000 0x0 0x1000>,
> +			      <0 0x5c0b0000 0x0 0x1000>;
> +			interrupts = <0 389 0>;
> +			desc-count = <10>;
> +			channel-index = <4>;
> +		};
> +	};
> +
> +Guest OS configuration:
> +
> +	hidma_24: dma-controller@0x5c050000 {
> +		compatible = "qcom,hidma-1.0";
> +		reg = <0 0x5c050000 0x0 0x1000>,
> +		      <0 0x5c0b0000 0x0 0x1000>;
> +		interrupts = <0 389 0>;
> +		desc-count = <10>;
> +		channel-index = <4>;
> +	};
> -- 
> 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 dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Jan. 15, 2016, 3:30 p.m. UTC | #2
On Fri, Jan 15, 2016 at 03:16:38PM +0000, Mark Rutland wrote:
> On Mon, Jan 11, 2016 at 09:45:42AM -0500, Sinan Kaya wrote:
> > Add documentation for the Qualcomm Technologies HIDMA driver.
> 
> s/driver/binding/
> 
> 
> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > Acked-by: Rob Herring <robh@kernel.org>

Further to my reply below, I'm generally uncomfortable with some
properties (max-* need a better description if they are a HW
requirement, and probably should not be present otherwise). I'm also
concerned that information necessary for the advertised use-case of the
device (e.g. IOMMUs) is missing [1], and we're missing parts of the
story necessary to review this for correctness.

Until that's settled, I don't think this is ready yet, and I don't think
this should be picked up.

I'm sorry to say that at this stage, as I realise that may sound
obstructive. That's not my intention, and I hope we can figure out those
details quickly.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/399850.html

> > ---
> >  .../devicetree/bindings/dma/qcom_hidma_mgmt.txt    | 79 ++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
> > 
> > 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..0e6ed1f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
> > @@ -0,0 +1,79 @@
> > +Qualcomm Technologies HIDMA Management interface
> > +
> > +Qualcomm Technologies HIDMA is a high speed DMA device. It only supports
> > +memcpy and memset capabilities. It has been designed for virtualized
> > +environments.
> > +
> > +Each HIDMA HW instance consists of multiple DMA channels. These channels
> > +share the same bandwidth. The bandwidth utilization can be parititioned
> > +among channels based on the priority and weight assignments.
> > +
> > +There are only two priority levels and 15 weigh assignments possible.
> > +
> > +Other parameters here determine how much of the system bus this HIDMA
> > +instance can use like maximum read/write request and and number of bytes to
> > +read/write in a single burst.
> > +
> > +Main node required properties:
> > +- compatible: "qcom,hidma-mgmt-1.0";
> > +- reg: Address range for DMA device
> > +- dma-channels: Number of channels supported by this DMA controller.
> > +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is
> > +  fragmented to multiples of this amount.
> > +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is
> > +  fragmented to multiples of this amount.
> > +- max-write-transactions: Maximum write transactions to perform in a burst
> > +- max-read-transactions: Maximum read transactions to perform in a burst
> 
> Just to check, where do these max-* values come from?
> 
> Are they some correctness requirement of the bus this is attached to?
> 
> Are they tuning values?
> 
> The latter doesn't really belong in the DT. Given they're writeable from
> the driver, it seems like that's what they are...
> 
> > +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC.
> 
> I'm not sure what this means. Could you elaborate on this is?
> 
> > +
> > +Sub-nodes:
> > +
> > +HIDMA has one or more DMA channels that are used to move data from one
> > +memory location to another.
> > +
> > +Each DMA channel is described as a sub-node under the management object.
> > +When a transfer channel is given to the guest operating system, only the channel
> > +object is created. The drivers have support for both flat and hierarchical
> > +configuration.
> 
> Don't mention drivers here.
> 
> All you need to state is that when the OS is not in control of the
> management interface (i.e. it's a guest), the channel nodes appear on
> their own, not under a management node.
> 
> Other than the above questions, this looks ok to me.
> 
> Thanks,
> Mark.
> 
> > +
> > +Required properties:
> > +- compatible: must contain "qcom,hidma-1.0"
> > +- reg: Addresses for the transfer and event channel
> > +- interrupts: Should contain the event interrupt
> > +- desc-count: Number of asynchronous requests this channel can handle
> > +- channel-index: The HW event channel completions will be delivered.
> > +
> > +Example:
> > +
> > +Hypervisor OS configuration:
> > +
> > +	hidma-mgmt@f9984000 = {
> > +		compatible = "qcom,hidma-mgmt-1.0";
> > +		reg = <0xf9984000 0x15000>;
> > +		dma-channels = <6>;
> > +		max-write-burst-bytes = <1024>;
> > +		max-read-burst-bytes = <1024>;
> > +		max-write-transactions = <31>;
> > +		max-read-transactions = <31>;
> > +		channel-reset-timeout-cycles = <0x500>;
> > +
> > +		hidma_24: dma-controller@0x5c050000 {
> > +			compatible = "qcom,hidma-1.0";
> > +			reg = <0 0x5c050000 0x0 0x1000>,
> > +			      <0 0x5c0b0000 0x0 0x1000>;
> > +			interrupts = <0 389 0>;
> > +			desc-count = <10>;
> > +			channel-index = <4>;
> > +		};
> > +	};
> > +
> > +Guest OS configuration:
> > +
> > +	hidma_24: dma-controller@0x5c050000 {
> > +		compatible = "qcom,hidma-1.0";
> > +		reg = <0 0x5c050000 0x0 0x1000>,
> > +		      <0 0x5c0b0000 0x0 0x1000>;
> > +		interrupts = <0 389 0>;
> > +		desc-count = <10>;
> > +		channel-index = <4>;
> > +	};
> > -- 
> > 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 devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sinan Kaya Jan. 15, 2016, 4:49 p.m. UTC | #3
On 1/15/2016 10:16 AM, Mark Rutland wrote:
> On Mon, Jan 11, 2016 at 09:45:42AM -0500, Sinan Kaya wrote:
>> Add documentation for the Qualcomm Technologies HIDMA driver.
> 
> s/driver/binding/

Changed.

> 
> 
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>  .../devicetree/bindings/dma/qcom_hidma_mgmt.txt    | 79 ++++++++++++++++++++++
>>  1 file changed, 79 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
>>
>> 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..0e6ed1f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
>> @@ -0,0 +1,79 @@
>> +Qualcomm Technologies HIDMA Management interface
>> +
>> +Qualcomm Technologies HIDMA is a high speed DMA device. It only supports
>> +memcpy and memset capabilities. It has been designed for virtualized
>> +environments.
>> +
>> +Each HIDMA HW instance consists of multiple DMA channels. These channels
>> +share the same bandwidth. The bandwidth utilization can be parititioned
>> +among channels based on the priority and weight assignments.
>> +
>> +There are only two priority levels and 15 weigh assignments possible.
>> +
>> +Other parameters here determine how much of the system bus this HIDMA
>> +instance can use like maximum read/write request and and number of bytes to
>> +read/write in a single burst.
>> +
>> +Main node required properties:
>> +- compatible: "qcom,hidma-mgmt-1.0";
>> +- reg: Address range for DMA device
>> +- dma-channels: Number of channels supported by this DMA controller.
>> +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is
>> +  fragmented to multiples of this amount.
>> +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is
>> +  fragmented to multiples of this amount.
>> +- max-write-transactions: Maximum write transactions to perform in a burst
>> +- max-read-transactions: Maximum read transactions to perform in a burst
> 
> Just to check, where do these max-* values come from?
These are HW bus parameters like the burst count and 
size of each burst. These values change based on the SoC this IP is in use.

> 
> Are they some correctness requirement of the bus this is attached to?
You can starve other peripherals if you use incorrect values as the bus is 
shared with other peripherals. Yes, correctness is required.

> 
> Are they tuning values?
Correct value is necessary for functioning. I'd consider weight and priority
as the only tuning parameters. 

> 
> The latter doesn't really belong in the DT. Given they're writeable from
> the driver, it seems like that's what they are...

Good catch. Those should have been read-only. I wanted to be able to export these
information to the userspace app. I'll fix the sysfs to make them read-only.

> 
>> +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC.
> 
> I'm not sure what this means. Could you elaborate on this is?
After each reset command, HW starts a timer. This is the time HW waits before it declares
reset failed.

> 
>> +
>> +Sub-nodes:
>> +
>> +HIDMA has one or more DMA channels that are used to move data from one
>> +memory location to another.
>> +
>> +Each DMA channel is described as a sub-node under the management object.
>> +When a transfer channel is given to the guest operating system, only the channel
>> +object is created. The drivers have support for both flat and hierarchical
>> +configuration.
> 
> Don't mention drivers here.
> 
> All you need to state is that when the OS is not in control of the
> management interface (i.e. it's a guest), the channel nodes appear on
> their own, not under a management node.

Replaced the above paragraph with yours.

> 
> Other than the above questions, this looks ok to me.
> 
> Thanks,
> Mark.
> 
>> +
>> +Required properties:
>> +- compatible: must contain "qcom,hidma-1.0"
>> +- reg: Addresses for the transfer and event channel
>> +- interrupts: Should contain the event interrupt
>> +- desc-count: Number of asynchronous requests this channel can handle
>> +- channel-index: The HW event channel completions will be delivered.
>> +
>> +Example:
>> +
>> +Hypervisor OS configuration:
>> +
>> +	hidma-mgmt@f9984000 = {
>> +		compatible = "qcom,hidma-mgmt-1.0";
>> +		reg = <0xf9984000 0x15000>;
>> +		dma-channels = <6>;
>> +		max-write-burst-bytes = <1024>;
>> +		max-read-burst-bytes = <1024>;
>> +		max-write-transactions = <31>;
>> +		max-read-transactions = <31>;
>> +		channel-reset-timeout-cycles = <0x500>;
>> +
>> +		hidma_24: dma-controller@0x5c050000 {
>> +			compatible = "qcom,hidma-1.0";
>> +			reg = <0 0x5c050000 0x0 0x1000>,
>> +			      <0 0x5c0b0000 0x0 0x1000>;
>> +			interrupts = <0 389 0>;
>> +			desc-count = <10>;
>> +			channel-index = <4>;
>> +		};
>> +	};
>> +
>> +Guest OS configuration:
>> +
>> +	hidma_24: dma-controller@0x5c050000 {
>> +		compatible = "qcom,hidma-1.0";
>> +		reg = <0 0x5c050000 0x0 0x1000>,
>> +		      <0 0x5c0b0000 0x0 0x1000>;
>> +		interrupts = <0 389 0>;
>> +		desc-count = <10>;
>> +		channel-index = <4>;
>> +	};
>> -- 
>> 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
>>
Sinan Kaya Jan. 15, 2016, 5:05 p.m. UTC | #4
On 1/15/2016 10:30 AM, Mark Rutland wrote:
> Further to my reply below, I'm generally uncomfortable with some
> properties (max-* need a better description if they are a HW
> requirement, and probably should not be present otherwise). 

I'll add more description. 

> I'm also
> concerned that information necessary for the advertised use-case of the
> device (e.g. IOMMUs) is missing [1], and we're missing parts of the
> story necessary to review this for correctness.

OK. Let me work on this. I tried to capture as much documentation as possible
into the series before. One reviewer said I should add it. Another reviewer said I should 
remove it. 

Where would be the best place to document the use case? 
- In the source file?
- In the commit message?
- In the device-tree documentation?

I'll try to write up something based on your and Mark Zyngier's questions for the 
next release.

> 
> Until that's settled, I don't think this is ready yet, and I don't think
> this should be picked up.
> 
> I'm sorry to say that at this stage, as I realise that may sound
> obstructive. That's not my intention, and I hope we can figure out those
> details quickly.

I hope so too. I'm going towards V20 with small nitpicks rather than the good 
feedback like yours.
Mark Rutland Jan. 18, 2016, 11:39 a.m. UTC | #5
On Fri, Jan 15, 2016 at 12:05:19PM -0500, Sinan Kaya wrote:
> On 1/15/2016 10:30 AM, Mark Rutland wrote:
> > Further to my reply below, I'm generally uncomfortable with some
> > properties (max-* need a better description if they are a HW
> > requirement, and probably should not be present otherwise). 
> 
> I'll add more description. 
> 
> > I'm also
> > concerned that information necessary for the advertised use-case of the
> > device (e.g. IOMMUs) is missing [1], and we're missing parts of the
> > story necessary to review this for correctness.
> 
> OK. Let me work on this. I tried to capture as much documentation as possible
> into the series before. One reviewer said I should add it. Another reviewer said I should 
> remove it. 
> 
> Where would be the best place to document the use case? 
> - In the source file?
> - In the commit message?
> - In the device-tree documentation?

Mostly the cover letter, but to varying degrees some information should
appear in the commit messages, documentation, and code.

The cover letter should give a high-level overview sufficient to get
reviewing (e.g. that should point out we expect isolation by IOMMUs).
There should also be pointers to related components (i.e. the QEMU
driver intended to communicate with this).

Things subject to change should go in the cover letter, as it's not
permanent.

If something strongly influences the design in a way that's non-obvious,
then that will probably remain non-obvious. For that, there should
certainly be something in the commit message. If it's something that
will be hit in general usage of the binding (or forms part of the
contract of the binding), that should be described in the binding
documentation.

If there's some caveat other developers need to be aware of, drop a
comment in the code.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Jan. 18, 2016, 11:49 a.m. UTC | #6
> >> +Main node required properties:
> >> +- compatible: "qcom,hidma-mgmt-1.0";
> >> +- reg: Address range for DMA device
> >> +- dma-channels: Number of channels supported by this DMA controller.
> >> +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is
> >> +  fragmented to multiples of this amount.
> >> +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is
> >> +  fragmented to multiples of this amount.
> >> +- max-write-transactions: Maximum write transactions to perform in a burst
> >> +- max-read-transactions: Maximum read transactions to perform in a burst
> > 
> > Just to check, where do these max-* values come from?
> These are HW bus parameters like the burst count and 
> size of each burst. These values change based on the SoC this IP is in use.
> 
> > 
> > Are they some correctness requirement of the bus this is attached to?
> You can starve other peripherals if you use incorrect values as the bus is 
> shared with other peripherals. Yes, correctness is required.

Is that a property of the system known statically, or one determined by
testing the system under particular workloads? It feels like the latter
(though I appreciate that not starving other masters is certainly a
correctness property regardless of how this is derived).

I'd have expected the bus this is plugged into to have appropriate QoS
settings pre-configured so as to avoid starvation, though it sounds like
that's not possible here?

> > Are they tuning values?
> Correct value is necessary for functioning. I'd consider weight and priority
> as the only tuning parameters. 
> 
> > 
> > The latter doesn't really belong in the DT. Given they're writeable from
> > the driver, it seems like that's what they are...
> 
> Good catch. Those should have been read-only. I wanted to be able to export these
> information to the userspace app. I'll fix the sysfs to make them read-only.
> 
> > 
> >> +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC.
> > 
> > I'm not sure what this means. Could you elaborate on this is?
> After each reset command, HW starts a timer. This is the time HW waits before it declares
> reset failed.

Is that a reset command sent to the HIDMA by the OS, or a reset command
from the HIDMA to something else?

What does it do when it declares a reset as failed?

How can the OS make use of this information? It has no idea of the
clocks input to the HIDMA, so it has no idea how long a cycle is.

Is this programmed by the OS?

Is the particular duration in cycles a requirement of some other agent?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sinan Kaya Jan. 18, 2016, 2:04 p.m. UTC | #7
On 1/18/2016 6:49 AM, Mark Rutland wrote:
>>>> +Main node required properties:
>>>> +- compatible: "qcom,hidma-mgmt-1.0";
>>>> +- reg: Address range for DMA device
>>>> +- dma-channels: Number of channels supported by this DMA controller.
>>>> +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is
>>>> +  fragmented to multiples of this amount.
>>>> +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is
>>>> +  fragmented to multiples of this amount.
>>>> +- max-write-transactions: Maximum write transactions to perform in a burst
>>>> +- max-read-transactions: Maximum read transactions to perform in a burst
>>>
>>> Just to check, where do these max-* values come from?
>> These are HW bus parameters like the burst count and 
>> size of each burst. These values change based on the SoC this IP is in use.
>>
>>>
>>> Are they some correctness requirement of the bus this is attached to?
>> You can starve other peripherals if you use incorrect values as the bus is 
>> shared with other peripherals. Yes, correctness is required.
> 
> Is that a property of the system known statically, or one determined by
> testing the system under particular workloads? It feels like the latter
> (though I appreciate that not starving other masters is certainly a
> correctness property regardless of how this is derived).
> 

It is known statically and is determined through simulations according to the SoC design.
It is later verified on chip. Once verified, these values never change. 
Values change from one SoC to another. 

> I'd have expected the bus this is plugged into to have appropriate QoS
> settings pre-configured so as to avoid starvation, though it sounds like
> that's not possible here?

There are some very safe values but they are not correct until validated on the chip.

> 
>>> Are they tuning values?
>> Correct value is necessary for functioning. I'd consider weight and priority
>> as the only tuning parameters. 
>>
>>>
>>> The latter doesn't really belong in the DT. Given they're writeable from
>>> the driver, it seems like that's what they are...
>>
>> Good catch. Those should have been read-only. I wanted to be able to export these
>> information to the userspace app. I'll fix the sysfs to make them read-only.
>>
>>>
>>>> +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC.
>>>
>>> I'm not sure what this means. Could you elaborate on this is?
>> After each reset command, HW starts a timer. This is the time HW waits before it declares
>> reset failed.
> 
> Is that a reset command sent to the HIDMA by the OS, or a reset command
> from the HIDMA to something else?

First one. 

As I said on another email, The HIDMA channel driver resets the transfer and event channels 
before using them. The reset command is intended for the HIDMA HW itself.

> 
> What does it do when it declares a reset as failed?
> 

The channel probe will fail with reset failed message.

> How can the OS make use of this information? It has no idea of the
> clocks input to the HIDMA, so it has no idea how long a cycle is.
> 
This is a HW parameter. The OS doesn't use timers or any other facility to time correctness. 

The HIDMA channel driver issues a reset and then waits for confirmation while polling a status
register. If no confirmation is received, then the HW is assumed broken or incorrectly configured.
The HW channel is removed from the OS access.

> Is this programmed by the OS?
The HIDMA management driver programs this value to the HW. It is not consumed by the OS directly.

> 
> Is the particular duration in cycles a requirement of some other agent?
It is a requirement of the HW. Not the OS.

> 
> Thanks,
> Mark.
>
diff mbox

Patch

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..0e6ed1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
@@ -0,0 +1,79 @@ 
+Qualcomm Technologies HIDMA Management interface
+
+Qualcomm Technologies HIDMA is a high speed DMA device. It only supports
+memcpy and memset capabilities. It has been designed for virtualized
+environments.
+
+Each HIDMA HW instance consists of multiple DMA channels. These channels
+share the same bandwidth. The bandwidth utilization can be parititioned
+among channels based on the priority and weight assignments.
+
+There are only two priority levels and 15 weigh assignments possible.
+
+Other parameters here determine how much of the system bus this HIDMA
+instance can use like maximum read/write request and and number of bytes to
+read/write in a single burst.
+
+Main node required properties:
+- compatible: "qcom,hidma-mgmt-1.0";
+- reg: Address range for DMA device
+- dma-channels: Number of channels supported by this DMA controller.
+- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is
+  fragmented to multiples of this amount.
+- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is
+  fragmented to multiples of this amount.
+- max-write-transactions: Maximum write transactions to perform in a burst
+- max-read-transactions: Maximum read transactions to perform in a burst
+- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC.
+
+Sub-nodes:
+
+HIDMA has one or more DMA channels that are used to move data from one
+memory location to another.
+
+Each DMA channel is described as a sub-node under the management object.
+When a transfer channel is given to the guest operating system, only the channel
+object is created. The drivers have support for both flat and hierarchical
+configuration.
+
+Required properties:
+- compatible: must contain "qcom,hidma-1.0"
+- reg: Addresses for the transfer and event channel
+- interrupts: Should contain the event interrupt
+- desc-count: Number of asynchronous requests this channel can handle
+- channel-index: The HW event channel completions will be delivered.
+
+Example:
+
+Hypervisor OS configuration:
+
+	hidma-mgmt@f9984000 = {
+		compatible = "qcom,hidma-mgmt-1.0";
+		reg = <0xf9984000 0x15000>;
+		dma-channels = <6>;
+		max-write-burst-bytes = <1024>;
+		max-read-burst-bytes = <1024>;
+		max-write-transactions = <31>;
+		max-read-transactions = <31>;
+		channel-reset-timeout-cycles = <0x500>;
+
+		hidma_24: dma-controller@0x5c050000 {
+			compatible = "qcom,hidma-1.0";
+			reg = <0 0x5c050000 0x0 0x1000>,
+			      <0 0x5c0b0000 0x0 0x1000>;
+			interrupts = <0 389 0>;
+			desc-count = <10>;
+			channel-index = <4>;
+		};
+	};
+
+Guest OS configuration:
+
+	hidma_24: dma-controller@0x5c050000 {
+		compatible = "qcom,hidma-1.0";
+		reg = <0 0x5c050000 0x0 0x1000>,
+		      <0 0x5c0b0000 0x0 0x1000>;
+		interrupts = <0 389 0>;
+		desc-count = <10>;
+		channel-index = <4>;
+	};