Re: [PATCH 2/3] soc: keystone: add QMSS driver

[Santosh Shilimkar <santosh.shilimkar@ti.com>]

On Tuesday 11 March 2014 01:02 AM, Rob Herring wrote:
> On Fri, Feb 28, 2014 at 5:18 PM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> From: Sandeep Nair <sandeep_n@ti.com>
>>
>> The QMSS (Queue Manager Sub System) found on Keystone SOCs is one of
>> the main hardware sub system which forms the backbone of the Keystone
>> Multi-core Navigator. QMSS consist of queue managers, packed-data structure
>> processors(PDSP), linking RAM, descriptor pools and infrastructure
>> Packet DMA.
>>
>> The Queue Manager is a hardware module that is responsible for accelerating
>> management of the packet queues. Packets are queued/de-queued by writing or
>> reading descriptor address to a particular memory mapped location. The PDSPs
>> perform QMSS related functions like accumulation, QoS, or event management.
>> Linking RAM registers are used to link the descriptors which are stored in
>> descriptor RAM. Descriptor RAM is configurable as internal or external memory.
>>
>> The QMSS driver manages the PDSP setups, linking RAM regions,
>> queue pool management (allocation, push, pop and notify) and descriptor
>> pool management. The specifics on the device tree bindings for
>> QMSS can be found in:
>>         Documentation/devicetree/bindings/soc/keystone-qmss.txt
> 
> You are telling me where I can find a file that is added in this commit?
> 
> All this description of what the h/w block does should go into the binding doc.
> 
Sure. Will do.

>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Kumar Gala <galak@codeaurora.org>
>> Cc: Olof Johansson <olof@lixom.net>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Sandeep Nair <sandeep_n@ti.com>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>  .../devicetree/bindings/soc/keystone-qmss.txt      |  209 +++
>>  drivers/Kconfig                                    |    2 +
>>  drivers/Makefile                                   |    3 +
>>  drivers/soc/Kconfig                                |    2 +
>>  drivers/soc/Makefile                               |    5 +
>>  drivers/soc/keystone/Kconfig                       |   15 +
>>  drivers/soc/keystone/Makefile                      |    5 +
>>  drivers/soc/keystone/qmss_acc.c                    |  591 ++++++++
>>  drivers/soc/keystone/qmss_queue.c                  | 1533 ++++++++++++++++++++
>>  drivers/soc/keystone/qmss_queue.h                  |  236 +++
>>  include/linux/soc/keystone_qmss.h                  |  390 +++++
>>  11 files changed, 2991 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/keystone-qmss.txt
>>  create mode 100644 drivers/soc/Makefile
>>  create mode 100644 drivers/soc/keystone/Kconfig
>>  create mode 100644 drivers/soc/keystone/Makefile
>>  create mode 100644 drivers/soc/keystone/qmss_acc.c
>>  create mode 100644 drivers/soc/keystone/qmss_queue.c
>>  create mode 100644 drivers/soc/keystone/qmss_queue.h
>>  create mode 100644 include/linux/soc/keystone_qmss.h
>>
>> diff --git a/Documentation/devicetree/bindings/soc/keystone-qmss.txt b/Documentation/devicetree/bindings/soc/keystone-qmss.txt
>> new file mode 100644
>> index 0000000..f975207
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/keystone-qmss.txt
>> @@ -0,0 +1,209 @@
>> +* Texas Instruments Keystone QMSS driver
>> +
>> +Required properties:
>> +- compatible   : Must be "ti,keystone-qmss";
>> +- clocks       : phandle to the reference clock for this device.
>> +- queue-range  : <start number> total range of queue numbers for the device.
>> +- linkram0     : <address size> for internal link ram, where size is the total
>> +                 link ram entries.
>> +- linkram1     : <address size> for external link ram, where size is the total
>> +                 external link ram entries. If the address is specified as "0"
>> +                 driver will allocate memory.
>> +- qmgrs         : the number of individual queue managers in the device. On
>> +                  keystone 1 range of devices there should be only one node.
>> +                 On keystone 2 devices there can be more than 1 node.
>> +  -- managed-queues    : the actual queues managed by each queue manager
>> +                         instance, specified as <"base queue #" "# of queues">.
>> +  -- reg               : Address and length of the register set for the device
>> +                         for peek, status, config, region, push, pop regions.
>> +  -- reg-names         : Names for the above register regions. The name to be
>> +                         used is as follows:
>> +                         - "config" : Queue configuration region.
>> +                         - "status" : Queue status RAM.
>> +                         - "region" : Descriptor memory setup region.
>> +                         - "push"   : Queue Management/Queue Proxy region.
>> +                         - "pop"    : Queue Management/Queue Proxy region.
>> +                         - "peek"   : Queue Peek region.
> 
> reg-names should be optional. Also you have the order different from
> reg. Be consistent as to what is the correct order.
> 
We thought of using reg-names to remove the ordering to index
the regs. So if we make the reg-names optional, then indexing
by order will be used. We can do that.

>> +- queue-pools  : Queue ranges are grouped into 3 type of pools:
>> +                 - qpend           : pool of qpend(interruptible) queues
>> +                 - general-purpose : pool of general queues, primarly used
>> +                                     as free descriptor queues or the
>> +                                     transmit DMA queues.
>> +                 - accumulator     : pool of queues on accumulator channel
>> +                 Each range can have the following properties:
>> +  -- values            : number of queues to use per queue range, specified as
>> +                         <"base queue #" "# of queues">.
> 
> values of what? This needs a better name.
ok. Will use 'qrange-values'
> 
>> +  -- interrupts                : Optional property to specify the interrupt mapping
>> +                         for interruptible queues. The driver additionaly sets
>> +                         the interrupt affinity based on the cpu mask.
>> +  -- reserved          : Optional property used to reserve the range. Queues
>> +                         in a reserved range can only be allocated by id.
> 
> reserved what? This needs a better name and description.
>
'qrange-reserved'. Will update the description.
 
>> +  -- accumulator       : Accumulator channel property specified as:
>> +                         <pdsp-id, channel, entries, pacing mode, latency>
>> +                         pdsp-id     : QMSS PDSP running accumulator firmware
>> +                                       on which the channel has to be
>> +                                       configured
>> +                         channel     : Accumulator channel number
>> +                         entries     : Size of the accumulator descriptor list
>> +                         pacing mode : Interrupt pacing mode
>> +                                       0 : None, i.e interrupt on list full
>> +                                       1 : Time delay since last interrupt
>> +                                       2 : Time delay since first new packet
>> +                                       3 : Time delay since last new packet
>> +                         latency     : time to delay the interrupt, specified
>> +                                       in microseconds.
>> +  -- multi-queue       : Optional property to specify that the channel has to
>> +                         monitor upto 32 queues starting at the base queue #.
> 
> What does the property contain? What's a channel in this context?
>
Its accumulator channel who can monitor multiple queues. Its a hardware
off-load feature to monitor and accumulate the traffic on multiple queues.
 
>> +- descriptor-regions   : Descriptor memory region specification.
> 
> huh?
Ok. We will make the description bit more verbose to make it clear.

> 
>> +  -- id                                : region number.
>> +  -- values                    : number of descriptors in the region,
>> +                                 specified as
>> +                                 <"# of descriptors" "descriptor size">.
>> +  -- link-index                        : start index, i.e. index of the first
>> +                                 descriptor in the region.
>> +
>> +Optional properties:
>> +- dma-coherent : Present if DMA operations are coherent.
>> +- pdsps                : PDSP configuration, if any.
> 
> This is a child node? Make that clear and separate the description for
> each node.
>
Sure.
 
>> +  -- firmware          : firmware to be loaded on the PDSP.
>> +  -- id                        : the qmss pdsp that will run the firmware.
>> +  -- reg               : Address and length of the register set of the PDSP
>> +                         for iram, intd, region, command regions.
>> +  -- reg-names         : Names for the above register regions. The name to be
>> +                         used is as follows:
>> +                         - "iram"   : PDSP internal RAM region.
>> +                         - "reg"    : PDSP control/status region registers.
>> +                         - "intd"   : QMSS interrupt distributor registers.
>> +                         - "cmd"    : PDSP command interface region.
>> +
>> +Example:
> 
> I probably have other comments, but address the above first.
> 
Thanks for the comments. We will address these in next version.

Regards,
Santosh