lkml.org 
[lkml]   [2019]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 02/14] soc: ti: k3: add navss ringacc driver
From
Date


On 09/09/2019 16:00, Peter Ujfalusi wrote:
> Hi,
>
> Grygorii, can you take a look?
>
> On 09/09/2019 9.09, Tero Kristo wrote:
>> Hi,
>>
>> Mostly some cosmetic comments below, other than that seems fine to me.
>>
>> On 30/07/2019 12:34, Peter Ujfalusi wrote:
>>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>>
>>> The Ring Accelerator (RINGACC or RA) provides hardware acceleration to
>>> enable straightforward passing of work between a producer and a consumer.
>>> There is one RINGACC module per NAVSS on TI AM65x SoCs.
>>>
>>> The RINGACC converts constant-address read and write accesses to
>>> equivalent
>>> read or write accesses to a circular data structure in memory. The
>>> RINGACC
>>> eliminates the need for each DMA controller which needs to access ring
>>> elements from having to know the current state of the ring (base address,
>>> current offset). The DMA controller performs a read or write access to a
>>> specific address range (which maps to the source interface on the
>>> RINGACC)
>>> and the RINGACC replaces the address for the transaction with a new
>>> address
>>> which corresponds to the head or tail element of the ring (head for
>>> reads,
>>> tail for writes). Since the RINGACC maintains the state, multiple DMA
>>> controllers or channels are allowed to coherently share the same rings as
>>> applicable. The RINGACC is able to place data which is destined towards
>>> software into cached memory directly.
>>>
>>> Supported ring modes:
>>> - Ring Mode
>>> - Messaging Mode
>>> - Credentials Mode
>>> - Queue Manager Mode
>>>
>>> TI-SCI integration:
>>>
>>> Texas Instrument's System Control Interface (TI-SCI) Message Protocol now
>>> has control over Ringacc module resources management (RM) and Rings
>>> configuration.
>>>
>>> The corresponding support of TI-SCI Ringacc module RM protocol
>>> introduced as option through DT parameters:
>>> - ti,sci: phandle on TI-SCI firmware controller DT node
>>> - ti,sci-dev-id: TI-SCI device identifier as per TI-SCI firmware spec
>>>
>>> if both parameters present - Ringacc driver will configure/free/reset
>>> Rings
>>> using TI-SCI Message Ringacc RM Protocol.
>>>
>>> The Ringacc driver manages Rings allocation by itself now and requests
>>> TI-SCI firmware to allocate and configure specific Rings only. It's done
>>> this way because, Linux driver implements two stage Rings allocation and
>>> configuration (allocate ring and configure ring) while I-SCI Message
>>
>> I-SCI should be TI-SCI I believe.
>
> Yes, it supposed to be.
>
>>
>>> Protocol supports only one combined operation (allocate+configure).
>>>
>>> Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> Above seems to be missing SoB?
>
> Oh, it is really missing.
>
>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>>   drivers/soc/ti/Kconfig            |   17 +
>>>   drivers/soc/ti/Makefile           |    1 +
>>>   drivers/soc/ti/k3-ringacc.c       | 1191 +++++++++++++++++++++++++++++
>>>   include/linux/soc/ti/k3-ringacc.h |  262 +++++++
>>>   4 files changed, 1471 insertions(+)
>>>   create mode 100644 drivers/soc/ti/k3-ringacc.c
>>>   create mode 100644 include/linux/soc/ti/k3-ringacc.h
>>>
>>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
>>> index cf545f428d03..10c76faa503e 100644
>>> --- a/drivers/soc/ti/Kconfig
>>> +++ b/drivers/soc/ti/Kconfig
>>> @@ -80,6 +80,23 @@ config TI_SCI_PM_DOMAINS
>>>         called ti_sci_pm_domains. Note this is needed early in boot
>>> before
>>>         rootfs may be available.
>>>   +config TI_K3_RINGACC
>>> +    tristate "K3 Ring accelerator Sub System"
>>> +    depends on ARCH_K3 || COMPILE_TEST
>>> +    depends on TI_SCI_INTA_IRQCHIP
>>> +    default y
>>> +    help
>>> +      Say y here to support the K3 Ring accelerator module.
>>> +      The Ring Accelerator (RINGACC or RA)  provides hardware
>>> acceleration
>>> +      to enable straightforward passing of work between a producer
>>> +      and a consumer. There is one RINGACC module per NAVSS on TI
>>> AM65x SoCs
>>> +      If unsure, say N.
>>> +
>>> +config TI_K3_RINGACC_DEBUG
>>> +    tristate "K3 Ring accelerator Sub System tests and debug"
>>> +    depends on TI_K3_RINGACC
>>> +    default n
>>> +
>>>   endif # SOC_TI
>>>     config TI_SCI_INTA_MSI_DOMAIN
>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>>> index b3868d392d4f..cc4bc8b08bf5 100644
>>> --- a/drivers/soc/ti/Makefile
>>> +++ b/drivers/soc/ti/Makefile
>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>>>   obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>>   obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>>   obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>>> +obj-$(CONFIG_TI_K3_RINGACC)        += k3-ringacc.o
>>> diff --git a/drivers/soc/ti/k3-ringacc.c b/drivers/soc/ti/k3-ringacc.c
>>> new file mode 100644
>>> index 000000000000..401dfc963319
>>> --- /dev/null
>>> +++ b/drivers/soc/ti/k3-ringacc.c
>>> @@ -0,0 +1,1191 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * TI K3 NAVSS Ring Accelerator subsystem driver
>>> + *
>>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
>>> + */
>>> +
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/soc/ti/k3-ringacc.h>
>>> +#include <linux/soc/ti/ti_sci_protocol.h>
>>> +#include <linux/soc/ti/ti_sci_inta_msi.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/irqdomain.h>
>>> +
>>> +static LIST_HEAD(k3_ringacc_list);
>>> +static DEFINE_MUTEX(k3_ringacc_list_lock);
>>> +
>>> +#ifdef CONFIG_TI_K3_RINGACC_DEBUG
>>> +#define    k3_nav_dbg(dev, arg...) dev_err(dev, arg)
>>
>> dev_err seems exaggeration for debug purposes, maybe just dev_info.
>>
>>> +static    void dbg_writel(u32 v, void __iomem *reg)
>>> +{
>>> +    pr_err("WRITEL(32): v(%08X)-->reg(%p)\n", v, reg);
>>
>> Again, maybe just pr_info.
>
> I think I'll just drop CONFIG_TI_K3_RINGACC_DEBUG altogether along with
> dbg_writel/dbg_readl/k3_nav_dbg and use dev_dbg() when appropriate.

Sounds good.

>
>>
>>> +    writel(v, reg);
>>> +}
>>> +
>>> +static    u32 dbg_readl(void __iomem *reg)
>>> +{
>>> +    u32 v;
>>> +
>>> +    v = readl(reg);
>>> +    pr_err("READL(32): v(%08X)<--reg(%p)\n", v, reg);
>>> +    return v;
>>> +}
>>> +#else
>>> +#define    k3_nav_dbg(dev, arg...) dev_dbg(dev, arg)
>>> +#define dbg_writel(v, reg) writel(v, reg)
>>
>> Do you need to use hard writel, writel_relaxed is not enough?
>
> not sure if we really need the barriers, but __raw_writel() should be
> fine here imho

xxx_relaxed relaxed versions should be used only when necessary and with
adding appropriate comments why they've been used and what benefits from using
them for each particular case.
So, i do not agree with this blind conversation.

>
>>> +
>>> +#define dbg_readl(reg) readl(reg)
>>
>> Same as above but for read?
>
> __raw_readl() could be fine in also.

No. __raw_xxx api should never be used by drivers.


>
> ...
>
>>> +/**
>>> + * struct k3_ringacc - Rings accelerator descriptor
>>> + *
>>> + * @dev - pointer on RA device
>>> + * @proxy_gcfg - RA proxy global config registers
>>> + * @proxy_target_base - RA proxy datapath region
>>> + * @num_rings - number of ring in RA
>>> + * @rm_gp_range - general purpose rings range from tisci
>>> + * @dma_ring_reset_quirk - DMA reset w/a enable
>>> + * @num_proxies - number of RA proxies
>>> + * @rings - array of rings descriptors (struct @k3_ring)
>>> + * @list - list of RAs in the system
>>> + * @tisci - pointer ti-sci handle
>>> + * @tisci_ring_ops - ti-sci rings ops
>>> + * @tisci_dev_id - ti-sci device id
>>> + */

...

>>> +
>>> +#ifdef CONFIG_TI_K3_RINGACC_DEBUG
>>> +void k3_ringacc_ring_dump(struct k3_ring *ring)
>>> +{
>>> +    struct device *dev = ring->parent->dev;
>>> +
>>> +    k3_nav_dbg(dev, "dump ring: %d\n", ring->ring_id);
>>> +    k3_nav_dbg(dev, "dump mem virt %p, dma %pad\n",
>>> +           ring->ring_mem_virt, &ring->ring_mem_dma);
>>> +    k3_nav_dbg(dev, "dump elmsize %d, size %d, mode %d, proxy_id %d\n",
>>> +           ring->elm_size, ring->size, ring->mode, ring->proxy_id);
>>> +
>>> +    k3_nav_dbg(dev, "dump ring_rt_regs: db%08x\n",
>>> +           readl(&ring->rt->db));
>>
>> Why not use readl_relaxed in this func?
>
> __raw_readl() might be enough?

No Raw, but this seems only one place where relaxed version can be used.

>
>>
>>> +    k3_nav_dbg(dev, "dump occ%08x\n",
>>> +           readl(&ring->rt->occ));
>>> +    k3_nav_dbg(dev, "dump indx%08x\n",
>>> +           readl(&ring->rt->indx));
>>> +    k3_nav_dbg(dev, "dump hwocc%08x\n",
>>> +           readl(&ring->rt->hwocc));
>>> +    k3_nav_dbg(dev, "dump hwindx%08x\n",
>>> +           readl(&ring->rt->hwindx));
>>> +
>>> +    if (ring->ring_mem_virt)
>>> +        print_hex_dump(KERN_ERR, "dump ring_mem_virt ",
>>> +                   DUMP_PREFIX_NONE, 16, 1,
>>> +                   ring->ring_mem_virt, 16 * 8, false);
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_dump);
>>
>> Do you really need to export a debug function?
>
> It might come helpful for clients to dump the ring status runtime, but
> since we don't have users, I'll move it to static.

Yep. It was exported for debug purposes. But hence there are no active users - cna be removed.

>
>>> +#endif
>>> +
>>> +struct k3_ring *k3_ringacc_request_ring(struct k3_ringacc *ringacc,
>>> +                    int id, u32 flags)
>>> +{
>>> +    int proxy_id = K3_RINGACC_PROXY_NOT_USED;
>>> +
>>> +    mutex_lock(&ringacc->req_lock);
>>> +
>>> +    if (id == K3_RINGACC_RING_ID_ANY) {
>>> +        /* Request for any general purpose ring */
>>> +        struct ti_sci_resource_desc *gp_rings =
>>> +                        &ringacc->rm_gp_range->desc[0];
>>> +        unsigned long size;
>>> +
>>> +        size = gp_rings->start + gp_rings->num;
>>> +        id = find_next_zero_bit(ringacc->rings_inuse, size,
>>> +                    gp_rings->start);
>>> +        if (id == size)
>>> +            goto error;
>>> +    } else if (id < 0) {
>>> +        goto error;
>>> +    }
>>> +
>>> +    if (test_bit(id, ringacc->rings_inuse) &&
>>> +        !(ringacc->rings[id].flags & K3_RING_FLAG_SHARED))
>>> +        goto error;
>>> +    else if (ringacc->rings[id].flags & K3_RING_FLAG_SHARED)
>>> +        goto out;
>>> +
>>> +    if (flags & K3_RINGACC_RING_USE_PROXY) {
>>> +        proxy_id = find_next_zero_bit(ringacc->proxy_inuse,
>>> +                          ringacc->num_proxies, 0);
>>> +        if (proxy_id == ringacc->num_proxies)
>>> +            goto error;
>>> +    }
>>> +
>>> +    if (!try_module_get(ringacc->dev->driver->owner))
>>> +        goto error;
>>> +
>>> +    if (proxy_id != K3_RINGACC_PROXY_NOT_USED) {
>>> +        set_bit(proxy_id, ringacc->proxy_inuse);
>>> +        ringacc->rings[id].proxy_id = proxy_id;
>>> +        k3_nav_dbg(ringacc->dev, "Giving ring#%d proxy#%d\n",
>>> +               id, proxy_id);
>>> +    } else {
>>> +        k3_nav_dbg(ringacc->dev, "Giving ring#%d\n", id);
>>> +    }
>>> +
>>> +    set_bit(id, ringacc->rings_inuse);
>>> +out:
>>> +    ringacc->rings[id].use_count++;
>>> +    mutex_unlock(&ringacc->req_lock);
>>> +    return &ringacc->rings[id];
>>> +
>>> +error:
>>> +    mutex_unlock(&ringacc->req_lock);
>>> +    return NULL;
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_request_ring);
>>> +
>>> +static void k3_ringacc_ring_reset_sci(struct k3_ring *ring)
>>> +{
>>> +    struct k3_ringacc *ringacc = ring->parent;
>>> +    int ret;
>>> +
>>> +    ret = ringacc->tisci_ring_ops->config(
>>> +            ringacc->tisci,
>>> +            TI_SCI_MSG_VALUE_RM_RING_COUNT_VALID,
>>> +            ringacc->tisci_dev_id,
>>> +            ring->ring_id,
>>> +            0,
>>> +            0,
>>> +            ring->size,
>>> +            0,
>>> +            0,
>>> +            0);
>>> +    if (ret)
>>> +        dev_err(ringacc->dev, "TISCI reset ring fail (%d) ring_idx
>>> %d\n",
>>> +            ret, ring->ring_id);
>>
>> Return value of sci ops is masked, why not return it and let the caller
>> handle it properly?
>>
>> Same comment for anything similar that follows.
>
> Hrm, there is not much a caller can do other than PANIC in case the ring
> configuration fails.
> I can probagate the error, but not sure what action can be taken, if any.
>
>>> +}
>>> +
>>> +void k3_ringacc_ring_reset(struct k3_ring *ring)
>>> +{
>>> +    if (!ring || !(ring->flags & K3_RING_FLAG_BUSY))
>>> +        return;
>>> +
>>> +    ring->occ = 0;
>>> +    ring->free = 0;
>>> +    ring->rindex = 0;
>>> +    ring->windex = 0;
>>> +
>>> +    k3_ringacc_ring_reset_sci(ring);
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_reset);
>>> +
>>> +static void k3_ringacc_ring_reconfig_qmode_sci(struct k3_ring *ring,
>>> +                           enum k3_ring_mode mode)
>>> +{
>>> +    struct k3_ringacc *ringacc = ring->parent;
>>> +    int ret;
>>> +
>>> +    ret = ringacc->tisci_ring_ops->config(
>>> +            ringacc->tisci,
>>> +            TI_SCI_MSG_VALUE_RM_RING_MODE_VALID,
>>> +            ringacc->tisci_dev_id,
>>> +            ring->ring_id,
>>> +            0,
>>> +            0,
>>> +            0,
>>> +            mode,
>>> +            0,
>>> +            0);
>>> +    if (ret)
>>> +        dev_err(ringacc->dev, "TISCI reconf qmode fail (%d) ring_idx
>>> %d\n",
>>> +            ret, ring->ring_id);
>>> +}
>>> +
>>> +void k3_ringacc_ring_reset_dma(struct k3_ring *ring, u32 occ)
>>> +{
>>> +    if (!ring || !(ring->flags & K3_RING_FLAG_BUSY))
>>> +        return;
>>> +
>>> +    if (!ring->parent->dma_ring_reset_quirk)
>>> +        return;
>>> +
>>> +    if (!occ)
>>> +        occ = dbg_readl(&ring->rt->occ);
>>> +
>>> +    if (occ) {
>>> +        u32 db_ring_cnt, db_ring_cnt_cur;
>>> +
>>> +        k3_nav_dbg(ring->parent->dev, "%s %u occ: %u\n", __func__,
>>> +               ring->ring_id, occ);
>>> +        /* 2. Reset the ring */
>>
>> 2? Where is 1?
>
> Oh, I'll fix the numbering.

1. is 'Get ring occupancy count"
I think you can just drop numbering

>
>>
>>> +        k3_ringacc_ring_reset_sci(ring);
>>> +
>>> +        /*
>>> +         * 3. Setup the ring in ring/doorbell mode
>>> +         * (if not already in this mode)
>>> +         */
>>> +        if (ring->mode != K3_RINGACC_RING_MODE_RING)
>>> +            k3_ringacc_ring_reconfig_qmode_sci(
>>> +                    ring, K3_RINGACC_RING_MODE_RING);
>>> +        /*
>>> +         * 4. Ring the doorbell 2**22 – ringOcc times.
>>> +         * This will wrap the internal UDMAP ring state occupancy
>>> +         * counter (which is 21-bits wide) to 0.
>>> +         */
>>> +        db_ring_cnt = (1U << 22) - occ;
>>> +
>>> +        while (db_ring_cnt != 0) {
>>> +            /*
>>> +             * Ring the doorbell with the maximum count each
>>> +             * iteration if possible to minimize the total
>>> +             * of writes
>>> +             */
>>> +            if (db_ring_cnt > K3_RINGACC_MAX_DB_RING_CNT)
>>> +                db_ring_cnt_cur = K3_RINGACC_MAX_DB_RING_CNT;
>>> +            else
>>> +                db_ring_cnt_cur = db_ring_cnt;
>>> +
>>> +            writel(db_ring_cnt_cur, &ring->rt->db);
>>> +            db_ring_cnt -= db_ring_cnt_cur;
>>> +        }
>>> +
>>> +        /* 5. Restore the original ring mode (if not ring mode) */
>>> +        if (ring->mode != K3_RINGACC_RING_MODE_RING)
>>> +            k3_ringacc_ring_reconfig_qmode_sci(ring, ring->mode);
>>> +    }
>>> +
>>> +    /* 2. Reset the ring */
>>

>>> +
>>> +u32 k3_ringacc_get_tisci_dev_id(struct k3_ring *ring)
>>> +{
>>> +    if (!ring)
>>> +        return -EINVAL;
>>> +
>>
>> What if parent is NULL? Can it ever be here?
>
> No, parent can not be NULL as the client would not have the ring in the
> first place.
>
>>
>>> +    return ring->parent->tisci_dev_id;
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_get_tisci_dev_id);
>>> +
>>> +int k3_ringacc_get_ring_irq_num(struct k3_ring *ring)
>>> +{
>>> +    int irq_num;
>>> +
>>> +    if (!ring)
>>> +        return -EINVAL;
>>> +
>>> +    irq_num = ti_sci_inta_msi_get_virq(ring->parent->dev,
>>> ring->ring_id);
>>> +    if (irq_num <= 0)
>>> +        irq_num = -EINVAL;
>>> +    return irq_num;
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_get_ring_irq_num);
>>> +
>>> +static int k3_ringacc_ring_cfg_sci(struct k3_ring *ring)
>>> +{
>>> +    struct k3_ringacc *ringacc = ring->parent;
>>> +    u32 ring_idx;
>>> +    int ret;
>>> +
>>> +    if (!ringacc->tisci)
>>> +        return -EINVAL;
>>> +
>>> +    ring_idx = ring->ring_id;
>>> +    ret = ringacc->tisci_ring_ops->config(
>>> +            ringacc->tisci,
>>> +            TI_SCI_MSG_VALUE_RM_ALL_NO_ORDER,
>>> +            ringacc->tisci_dev_id,
>>> +            ring_idx,
>>> +            lower_32_bits(ring->ring_mem_dma),
>>> +            upper_32_bits(ring->ring_mem_dma),
>>> +            ring->size,
>>> +            ring->mode,
>>> +            ring->elm_size,
>>> +            0);
>>> +    if (ret)
>>> +        dev_err(ringacc->dev, "TISCI config ring fail (%d) ring_idx
>>> %d\n",
>>> +            ret, ring_idx);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +int k3_ringacc_ring_cfg(struct k3_ring *ring, struct k3_ring_cfg *cfg)
>>> +{
>>> +    struct k3_ringacc *ringacc = ring->parent;
>>> +    int ret = 0;
>>> +
>>> +    if (!ring || !cfg)
>>> +        return -EINVAL;
>>> +    if (cfg->elm_size > K3_RINGACC_RING_ELSIZE_256 ||
>>> +        cfg->mode > K3_RINGACC_RING_MODE_QM ||
>>> +        cfg->size & ~K3_RINGACC_CFG_RING_SIZE_ELCNT_MASK ||
>>> +        !test_bit(ring->ring_id, ringacc->rings_inuse))
>>> +        return -EINVAL;
>>> +
>>> +    if (ring->use_count != 1)
>>
>> Hmm, isn't this a failure actually?
>
> Yes, it is: -EBUSY

No. This is for shared rings.
0 - should never happens once ring is requested.
1 - only one user - configure ring
>1 - shared ring which is configured already - just exit as ring configure already.

>
>>> +        return 0;
>>> +
>>> +    ring->size = cfg->size;
>>> +    ring->elm_size = cfg->elm_size;
>>> +    ring->mode = cfg->mode;
>>> +    ring->occ = 0;
>>> +    ring->free = 0;
>>> +    ring->rindex = 0;
>>> +    ring->windex = 0;
>>> +

[...]

--
Best regards,
grygorii

\
 
 \ /
  Last update: 2019-09-09 19:00    [W:0.079 / U:8.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site