diff mbox

[v3,5/6] Documentation: dt-bindings: Add binding info for X-Gene QMTM UIO driver

Message ID 1413871011-4101-6-git-send-email-ankit.jindal@linaro.org
State Superseded, archived
Headers show

Commit Message

Ankit Jindal Oct. 21, 2014, 5:56 a.m. UTC
This patch adds device tree binding documentation for
X-Gene QMTM UIO driver.

Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
---
 .../devicetree/bindings/uio/uio_xgene_qmtm.txt     |   53 ++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt

Comments

Mark Rutland Oct. 21, 2014, 9:14 a.m. UTC | #1
On Tue, Oct 21, 2014 at 06:56:49AM +0100, Ankit Jindal wrote:
> This patch adds device tree binding documentation for
> X-Gene QMTM UIO driver.
> 
> Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
> Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
> ---
>  .../devicetree/bindings/uio/uio_xgene_qmtm.txt     |   53 ++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
> 
> diff --git a/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
> new file mode 100644
> index 0000000..288ed92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
> @@ -0,0 +1,53 @@
> +APM X-Gene QMTM UIO nodes

The "UIO" can go.

> +The Applied Micro X-Gene SOC has on-chip QMTM (Queue manager
> +and Traffic manager). It is a device for managing hardware queues.
> +It also implements QoS among hardware queues hence term "traffic"
> +manager is present in its name. QMTM UIO nodes are defined for user
> +space access to this device using UIO framework.

The binding should describe the hardware, not the software. Please drop
mention of UIO, userspace, etc.

> +Required properties:
> +- compatible: Should be "apm,xgene-qmtm"
> +- reg: Address and length of the register set for the device. It contains the
> +  information of registers in the same order as described by reg-names.
> +- reg-names: Should contain the register set names
> +  - "csr": QMTM control and status register address space.
> +  - "fabric": QMTM memory mapped access to queue states.
> +- qpool: Points to the phandle of the node defining memory location for
> +	 creating QMTM queues. This could point either to the reserved-memory
> +	 node (as-per reserved memory bindings) or to the node of on-chip
> +	 SRAM etc. It is expected that size and location of qpool memory will
> +	 be configurable via bootloader.

Is that on-chip SRAM part of the QMTM, or is that a shared part of the
SoC?

It feels odd to have a phandle that can go to completely different
classes of node, especially as you will need to use a different API to
acquire the memory region within Linux.

> +- clocks: Reference to the clock entry.

Just the one clock? Does the clock input to the QMTM have a name?

> +- num-queues: Number of queues under this QMTM device.
> +- devid: QMTM identification number for the system having multiple QMTM devices.
> +	 This is used to form a unique id (a tuple of queue number and
> +	 device id) for the queues belonging to this device.

Is this just an arbitrary unique ID, or is this a non-probeable property
of the HW? If the former, isn't the base address sufficient as a unique
identifier?

Thanks,
Mark.

> +Example:
> +	qmtm1_uio_qpool: qmtm1_uio_qpool {
> +		reg = <0x0 0x0 0x0 0x0>
> +	};
> +
> +	qmtm1clk: qmtmclk@1f20c000 {
> +		compatible = "apm,xgene-device-clock";
> +		clock-output-names = "qmtm1clk";
> +		status = "ok";
> +	};
> +
> +	qmtm1_uio: qmtm_uio@1f200000 {
> +		compatible = "apm,xgene-qmtm";
> +		status = "disabled";
> +		reg = <0x0 0x1f200000 0x0 0x10000>,
> +		      <0x0 0x1b000000 0x0 0x400000>;
> +		reg-names = "csr", "fabric";
> +		qpool = <&qmtm1_uio_qpool>;
> +		clocks = <&qmtm1clk 0>;
> +		num-queues = <0x400>;
> +		devid = <1>;
> +	};
> +
> +	/* Board-specific peripheral configurations */
> +	&qmtm1_uio {
> +		status = "ok";
> +	};
> -- 
> 1.7.9.5
> 
> --
> 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 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 Oct. 21, 2014, 10:05 a.m. UTC | #2
On Tuesday 21 October 2014 10:14:12 Mark Rutland wrote:
> 
> > +The Applied Micro X-Gene SOC has on-chip QMTM (Queue manager
> > +and Traffic manager). It is a device for managing hardware queues.
> > +It also implements QoS among hardware queues hence term "traffic"
> > +manager is present in its name. QMTM UIO nodes are defined for user
> > +space access to this device using UIO framework.
> 
> The binding should describe the hardware, not the software. Please drop
> mention of UIO, userspace, etc.

I have a bad feeling about the entire idea of doing the UIO driver first,
there are too many unknowns here and we should not break the binding
when the proper driver gets added.

The X-Gene is meant for server workloads, so the UIO approach is not
a good longterm solution anyway. My impression is that it would be
better to first get the kernel driver for this hardware merged and
the binding fixed, and then a UIO stub could get added to that driver,
taking over the hardware by user space. This would also solve the
arbitration between the two driver implementations. It's also possible
that by that time, we will have a functional VFIO framework for
platform devices.

	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
Ankit Jindal Oct. 31, 2014, 10:04 a.m. UTC | #3
Hi Mark,

On 21 October 2014 14:44, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Oct 21, 2014 at 06:56:49AM +0100, Ankit Jindal wrote:
>> This patch adds device tree binding documentation for
>> X-Gene QMTM UIO driver.
>>
>> Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
>> Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
>> ---
>>  .../devicetree/bindings/uio/uio_xgene_qmtm.txt     |   53 ++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
>> new file mode 100644
>> index 0000000..288ed92
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
>> @@ -0,0 +1,53 @@
>> +APM X-Gene QMTM UIO nodes
>
> The "UIO" can go.
Sure.

>
>> +The Applied Micro X-Gene SOC has on-chip QMTM (Queue manager
>> +and Traffic manager). It is a device for managing hardware queues.
>> +It also implements QoS among hardware queues hence term "traffic"
>> +manager is present in its name. QMTM UIO nodes are defined for user
>> +space access to this device using UIO framework.
>
> The binding should describe the hardware, not the software. Please drop
> mention of UIO, userspace, etc.
Sure.

>
>> +Required properties:
>> +- compatible: Should be "apm,xgene-qmtm"
>> +- reg: Address and length of the register set for the device. It contains the
>> +  information of registers in the same order as described by reg-names.
>> +- reg-names: Should contain the register set names
>> +  - "csr": QMTM control and status register address space.
>> +  - "fabric": QMTM memory mapped access to queue states.
>> +- qpool: Points to the phandle of the node defining memory location for
>> +      creating QMTM queues. This could point either to the reserved-memory
>> +      node (as-per reserved memory bindings) or to the node of on-chip
>> +      SRAM etc. It is expected that size and location of qpool memory will
>> +      be configurable via bootloader.
>
> Is that on-chip SRAM part of the QMTM, or is that a shared part of the
> SoC?
Its not part of QMTM.

>
> It feels odd to have a phandle that can go to completely different
> classes of node, especially as you will need to use a different API to
> acquire the memory region within Linux.
It is expected that phandle will point to a node whose first reg
property will be only for qpool memory.

>
>> +- clocks: Reference to the clock entry.
>
> Just the one clock? Does the clock input to the QMTM have a name?
Just one input clock. There is no specific name of it.

>
>> +- num-queues: Number of queues under this QMTM device.
>> +- devid: QMTM identification number for the system having multiple QMTM devices.
>> +      This is used to form a unique id (a tuple of queue number and
>> +      device id) for the queues belonging to this device.
>
> Is this just an arbitrary unique ID, or is this a non-probeable property
> of the HW? If the former, isn't the base address sufficient as a unique
> identifier?
Its a non-probeable property of the HW.

Thanks,
Ankit
>
> Thanks,
> Mark.
>
>> +Example:
>> +     qmtm1_uio_qpool: qmtm1_uio_qpool {
>> +             reg = <0x0 0x0 0x0 0x0>
>> +     };
>> +
>> +     qmtm1clk: qmtmclk@1f20c000 {
>> +             compatible = "apm,xgene-device-clock";
>> +             clock-output-names = "qmtm1clk";
>> +             status = "ok";
>> +     };
>> +
>> +     qmtm1_uio: qmtm_uio@1f200000 {
>> +             compatible = "apm,xgene-qmtm";
>> +             status = "disabled";
>> +             reg = <0x0 0x1f200000 0x0 0x10000>,
>> +                   <0x0 0x1b000000 0x0 0x400000>;
>> +             reg-names = "csr", "fabric";
>> +             qpool = <&qmtm1_uio_qpool>;
>> +             clocks = <&qmtm1clk 0>;
>> +             num-queues = <0x400>;
>> +             devid = <1>;
>> +     };
>> +
>> +     /* Board-specific peripheral configurations */
>> +     &qmtm1_uio {
>> +             status = "ok";
>> +     };
>> --
>> 1.7.9.5
>>
>> --
>> 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 devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Oct. 31, 2014, 10:09 a.m. UTC | #4
> >> +Required properties:
> >> +- compatible: Should be "apm,xgene-qmtm"
> >> +- reg: Address and length of the register set for the device. It contains the
> >> +  information of registers in the same order as described by reg-names.
> >> +- reg-names: Should contain the register set names
> >> +  - "csr": QMTM control and status register address space.
> >> +  - "fabric": QMTM memory mapped access to queue states.
> >> +- qpool: Points to the phandle of the node defining memory location for
> >> +      creating QMTM queues. This could point either to the reserved-memory
> >> +      node (as-per reserved memory bindings) or to the node of on-chip
> >> +      SRAM etc. It is expected that size and location of qpool memory will
> >> +      be configurable via bootloader.
> >
> > Is that on-chip SRAM part of the QMTM, or is that a shared part of the
> > SoC?
> Its not part of QMTM.
> 
> >
> > It feels odd to have a phandle that can go to completely different
> > classes of node, especially as you will need to use a different API to
> > acquire the memory region within Linux.
> It is expected that phandle will point to a node whose first reg
> property will be only for qpool memory.

Even if that's true you will need to use completely different APIs for
accessing that memory (so that the kernel can account the use of
reserved-memory correctly), so this might not be the best design. It
might be better to have qpool-sram and qpool-memory properties that
point at an sram node or a reserved-memory node respectively.

> >
> >> +- clocks: Reference to the clock entry.
> >
> > Just the one clock? Does the clock input to the QMTM have a name?
> Just one input clock. There is no specific name of it.

Ok.

> 
> >
> >> +- num-queues: Number of queues under this QMTM device.
> >> +- devid: QMTM identification number for the system having multiple QMTM devices.
> >> +      This is used to form a unique id (a tuple of queue number and
> >> +      device id) for the queues belonging to this device.
> >
> > Is this just an arbitrary unique ID, or is this a non-probeable property
> > of the HW? If the former, isn't the base address sufficient as a unique
> > identifier?
> Its a non-probeable property of the HW.

Ok. That sounds fine then.

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
Ankit Jindal Oct. 31, 2014, 10:15 a.m. UTC | #5
On 31 October 2014 15:39, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> +Required properties:
>> >> +- compatible: Should be "apm,xgene-qmtm"
>> >> +- reg: Address and length of the register set for the device. It contains the
>> >> +  information of registers in the same order as described by reg-names.
>> >> +- reg-names: Should contain the register set names
>> >> +  - "csr": QMTM control and status register address space.
>> >> +  - "fabric": QMTM memory mapped access to queue states.
>> >> +- qpool: Points to the phandle of the node defining memory location for
>> >> +      creating QMTM queues. This could point either to the reserved-memory
>> >> +      node (as-per reserved memory bindings) or to the node of on-chip
>> >> +      SRAM etc. It is expected that size and location of qpool memory will
>> >> +      be configurable via bootloader.
>> >
>> > Is that on-chip SRAM part of the QMTM, or is that a shared part of the
>> > SoC?
>> Its not part of QMTM.
>>
>> >
>> > It feels odd to have a phandle that can go to completely different
>> > classes of node, especially as you will need to use a different API to
>> > acquire the memory region within Linux.
>> It is expected that phandle will point to a node whose first reg
>> property will be only for qpool memory.
>
> Even if that's true you will need to use completely different APIs for
> accessing that memory (so that the kernel can account the use of
> reserved-memory correctly), so this might not be the best design. It
> might be better to have qpool-sram and qpool-memory properties that
> point at an sram node or a reserved-memory node respectively.
Thanks, I will go as per your suggestion. I will add two properties
qpool-sram and qpool-memory to tackle this.
>
>> >
>> >> +- clocks: Reference to the clock entry.
>> >
>> > Just the one clock? Does the clock input to the QMTM have a name?
>> Just one input clock. There is no specific name of it.
>
> Ok.
>
>>
>> >
>> >> +- num-queues: Number of queues under this QMTM device.
>> >> +- devid: QMTM identification number for the system having multiple QMTM devices.
>> >> +      This is used to form a unique id (a tuple of queue number and
>> >> +      device id) for the queues belonging to this device.
>> >
>> > Is this just an arbitrary unique ID, or is this a non-probeable property
>> > of the HW? If the former, isn't the base address sufficient as a unique
>> > identifier?
>> Its a non-probeable property of the HW.
>
> Ok. That sounds fine then.
>

Thanks,
Ankit
> 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
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
new file mode 100644
index 0000000..288ed92
--- /dev/null
+++ b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
@@ -0,0 +1,53 @@ 
+APM X-Gene QMTM UIO nodes
+
+The Applied Micro X-Gene SOC has on-chip QMTM (Queue manager
+and Traffic manager). It is a device for managing hardware queues.
+It also implements QoS among hardware queues hence term "traffic"
+manager is present in its name. QMTM UIO nodes are defined for user
+space access to this device using UIO framework.
+
+Required properties:
+- compatible: Should be "apm,xgene-qmtm"
+- reg: Address and length of the register set for the device. It contains the
+  information of registers in the same order as described by reg-names.
+- reg-names: Should contain the register set names
+  - "csr": QMTM control and status register address space.
+  - "fabric": QMTM memory mapped access to queue states.
+- qpool: Points to the phandle of the node defining memory location for
+	 creating QMTM queues. This could point either to the reserved-memory
+	 node (as-per reserved memory bindings) or to the node of on-chip
+	 SRAM etc. It is expected that size and location of qpool memory will
+	 be configurable via bootloader.
+- clocks: Reference to the clock entry.
+- num-queues: Number of queues under this QMTM device.
+- devid: QMTM identification number for the system having multiple QMTM devices.
+	 This is used to form a unique id (a tuple of queue number and
+	 device id) for the queues belonging to this device.
+
+Example:
+	qmtm1_uio_qpool: qmtm1_uio_qpool {
+		reg = <0x0 0x0 0x0 0x0>
+	};
+
+	qmtm1clk: qmtmclk@1f20c000 {
+		compatible = "apm,xgene-device-clock";
+		clock-output-names = "qmtm1clk";
+		status = "ok";
+	};
+
+	qmtm1_uio: qmtm_uio@1f200000 {
+		compatible = "apm,xgene-qmtm";
+		status = "disabled";
+		reg = <0x0 0x1f200000 0x0 0x10000>,
+		      <0x0 0x1b000000 0x0 0x400000>;
+		reg-names = "csr", "fabric";
+		qpool = <&qmtm1_uio_qpool>;
+		clocks = <&qmtm1clk 0>;
+		num-queues = <0x400>;
+		devid = <1>;
+	};
+
+	/* Board-specific peripheral configurations */
+	&qmtm1_uio {
+		status = "ok";
+	};