Re: [PATCH 4/5] dt-bindings: Add binding info for Xgene QMTM UIO driver

[Ankit Jindal]

On 9 September 2014 16:23, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Tue, Sep 09, 2014 at 10:56:58AM +0100, Ankit Jindal wrote:
>> This patch adds device tree binding documentation for
>> Xgene QMTM UIO driver.
>>
>> Signed-off-by: Ankit Jindal <ankit.jindal@xxxxxxxxxx>
>> Signed-off-by: Tushar Jagad <tushar.jagad@xxxxxxxxxx>
>> ---
>>  .../devicetree/bindings/uio/uio_xgene_qmtm.txt     |   45 ++++++++++++++++++++
>>  1 file changed, 45 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..b71831b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
>> @@ -0,0 +1,45 @@
>> +APM X-Gene QMTM UIO nodes
>> +
>> +QMTM UIO nodes are defined for user space access to on-chip QMTM device
>> +on APM X-Gene SOC using UIO framework.
>> +
>
> Userspace vs kernel space has precisely _nothing_ to do with a HW
> description.
>
> This doesn't describe at all what the device is (e.g. what does QMTM
> stand for, what is it used for?).
>
> NAK.

QMTM stands for queue manager and traffic manager. It is device managing
the hardware queues. It also implements QoS among hardware queues hence
term "traffic" manager is present in its name.

Sure, I will add more info about the device in the next revision.

The primary purpose of QMTM UIO driver is to expose QMTM HW to
user space so that frameworks like ODP (OpenDataPlane) can use
this from user space.

>
> Please ensure you Cc the devicetree list (devicetree@xxxxxxxxxxxxxxx) in
> future.

Sure, will do.

>
>> +Required properties:
>> +- compatible: Should be "apm,xgene-qmtm-uio"
>
> This should definitely not have "uio" in the name.

I added "uio" in compatible string because in future APM might
add separate driver for accessing QMTM from kernel space.

Can you suggest better compatible strings for this UIO driver?

>
>> +- 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.
>
> These look ok, I guess.
>
>> +  - "qpool": Memory location for creating QMTM queues. This could be some
>> +    SRAM or reserved portion of RAM. It is expected that size and location
>> +    of qpool memory will be configurable via bootloader.
>
> I don't follow why this should be described in a reg entry. This is not
> a property of the device; this is a separate resource being assigned to
> the device.

Ok, I will remove it from reg and add "qpool-addr" and "qpool-size"
attributes to
point the location of qpool.

>
> Why can the kernel not allocate this dynamically?

For faster access, this can be on-chip memory as well.

>
> If you need a specific fixed pool, use the reserved-memory bindings.

reserved-memory only apply to RAM, it does not apply to on-chip
SRAMs. Right ?

>
>> +- clocks: Reference to the clock entry.
>
> There is only one clock input to the IP block?

Yes.

>
>> +- num_queues: Number of queues under this QMTM device.
>
> s/_/-/, property names should use '-' rather than '_'.

Sure, will rename it in next revision.

>
>> +- devid: QMTM identification number for the system having multiple QMTM devices
>
> What exactly is this used for? Why is the reg entry not sufficient?

When there are multiple QMTM devices, the devid is used to form a
unique id (a tuple of queue number and device id) for the queues
belonging to this device. Unfortunately, we don't have any register
providing the devid of the device.

>
>> +Optional properties:
>> +- status: Should be "ok" or "disabled" for enabled/disabled. Default is "ok".
>
> This is so standard I don't see the point in documenting it.

Ok, I will remove it in next revision.

>
> Mark.

Thanks,
Ankit
--
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/