Re: [PATCH v2 02/14] soc: ti: k3: add navss ringacc driver

From: Peter Ujfalusi
Date: Mon Sep 09 2019 - 09:00:27 EST

Next message: Fabian Henneke: "Re: [PATCH 4.19 19/57] Bluetooth: hidp: Let hidp_send_message return number of queued bytes"
Previous message: Vlastimil Babka: "Re: [PATCH v2 1/2] mm/page_ext: support to record the last stack of page"
In reply to: Vignesh Raghavendra: "Re: [PATCH v2 02/14] soc: ti: k3: add navss ringacc driver"
Next in thread: Grygorii Strashko: "Re: [PATCH v2 02/14] soc: ti: k3: add navss ringacc driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxx>
>>
>> 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@xxxxxx>
>
> Above seems to be missing SoB?

Oh, it is really missing.

>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
>> ---
>> Â 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.

>
>> +ÂÂÂ 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

>> +
>> +#define dbg_readl(reg) readl(reg)
>
> Same as above but for read?

__raw_readl() could be fine in also.

...

>> +/**
>> + * 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
>> + */
>> +struct k3_ringacc {
>> +ÂÂÂ struct device *dev;
>> +ÂÂÂ struct k3_ringacc_proxy_gcfg_regs __iomem *proxy_gcfg;
>> +ÂÂÂ void __iomem *proxy_target_base;
>> +ÂÂÂ u32 num_rings; /* number of rings in Ringacc module */
>> +ÂÂÂ unsigned long *rings_inuse;
>> +ÂÂÂ struct ti_sci_resource *rm_gp_range;
>> +
>> +ÂÂÂ bool dma_ring_reset_quirk;
>> +ÂÂÂ u32 num_proxies;
>> +ÂÂÂ unsigned long *proxy_inuse;
>
> proxy_inuse is not documented above.

I see, I'll update the documentation.

>> +
>> +ÂÂÂ struct k3_ring *rings;
>> +ÂÂÂ struct list_head list;
>> +ÂÂÂ struct mutex req_lock; /* protect rings allocation */
>> +
>> +ÂÂÂ const struct ti_sci_handle *tisci;
>> +ÂÂÂ const struct ti_sci_rm_ringacc_ops *tisci_ring_ops;
>> +ÂÂÂ u32Â tisci_dev_id;
>> +};
>> +
>> +static long k3_ringacc_ring_get_fifo_pos(struct k3_ring *ring)
>> +{
>> +ÂÂÂ return K3_RINGACC_FIFO_WINDOW_SIZE_BYTES -
>> +ÂÂÂÂÂÂÂÂÂÂ (4 << ring->elm_size);
>> +}
>> +
>> +static void *k3_ringacc_get_elm_addr(struct k3_ring *ring, u32 idx)
>> +{
>> +ÂÂÂ return (idx * (4 << ring->elm_size) + ring->ring_mem_virt);
>
> The arithmetic here seems backwards compared to most other code I've
> seen. It would be more readable if you have it like:
>
> ring->ring_mem_virt + idx * (4 << ring->elm_size);

Yes, I'll update.

>
>> +}
>> +
>> +static int k3_ringacc_ring_push_mem(struct k3_ring *ring, void *elem);
>> +static int k3_ringacc_ring_pop_mem(struct k3_ring *ring, void *elem);
>> +
>> +static struct k3_ring_ops k3_ring_mode_ring_ops = {
>> +ÂÂÂÂÂÂÂ .push_tail = k3_ringacc_ring_push_mem,
>> +ÂÂÂÂÂÂÂ .pop_head = k3_ringacc_ring_pop_mem,
>> +};
>> +
>> +static int k3_ringacc_ring_push_io(struct k3_ring *ring, void *elem);
>> +static int k3_ringacc_ring_pop_io(struct k3_ring *ring, void *elem);
>> +static int k3_ringacc_ring_push_head_io(struct k3_ring *ring, void
>> *elem);
>> +static int k3_ringacc_ring_pop_tail_io(struct k3_ring *ring, void
>> *elem);
>> +
>> +static struct k3_ring_ops k3_ring_mode_msg_ops = {
>> +ÂÂÂÂÂÂÂ .push_tail = k3_ringacc_ring_push_io,
>> +ÂÂÂÂÂÂÂ .push_head = k3_ringacc_ring_push_head_io,
>> +ÂÂÂÂÂÂÂ .pop_tail = k3_ringacc_ring_pop_tail_io,
>> +ÂÂÂÂÂÂÂ .pop_head = k3_ringacc_ring_pop_io,
>> +};
>> +
>> +static int k3_ringacc_ring_push_head_proxy(struct k3_ring *ring, void
>> *elem);
>> +static int k3_ringacc_ring_push_tail_proxy(struct k3_ring *ring, void
>> *elem);
>> +static int k3_ringacc_ring_pop_head_proxy(struct k3_ring *ring, void
>> *elem);
>> +static int k3_ringacc_ring_pop_tail_proxy(struct k3_ring *ring, void
>> *elem);
>> +
>> +static struct k3_ring_ops k3_ring_mode_proxy_ops = {
>> +ÂÂÂÂÂÂÂ .push_tail = k3_ringacc_ring_push_tail_proxy,
>> +ÂÂÂÂÂÂÂ .push_head = k3_ringacc_ring_push_head_proxy,
>> +ÂÂÂÂÂÂÂ .pop_tail = k3_ringacc_ring_pop_tail_proxy,
>> +ÂÂÂÂÂÂÂ .pop_head = k3_ringacc_ring_pop_head_proxy,
>> +};
>> +
>> +#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?

>
>> +ÂÂÂ 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.

>> +#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.

>
>> +ÂÂÂÂÂÂÂ 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 */
>
> Again 2?

I'll drop the '2.'

>
>> +ÂÂÂ k3_ringacc_ring_reset(ring);
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_reset_dma);
>> +
>> +static void k3_ringacc_ring_free_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_ALL_NO_ORDER,
>> +ÂÂÂÂÂÂÂÂÂÂÂ ringacc->tisci_dev_id,
>> +ÂÂÂÂÂÂÂÂÂÂÂ ring->ring_id,
>> +ÂÂÂÂÂÂÂÂÂÂÂ 0,
>> +ÂÂÂÂÂÂÂÂÂÂÂ 0,
>> +ÂÂÂÂÂÂÂÂÂÂÂ 0,
>> +ÂÂÂÂÂÂÂÂÂÂÂ 0,
>> +ÂÂÂÂÂÂÂÂÂÂÂ 0,
>> +ÂÂÂÂÂÂÂÂÂÂÂ 0);
>> +ÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂ dev_err(ringacc->dev, "TISCI ring free fail (%d) ring_idx %d\n",
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret, ring->ring_id);
>> +}
>> +
>> +int k3_ringacc_ring_free(struct k3_ring *ring)
>> +{
>> +ÂÂÂ struct k3_ringacc *ringacc;
>> +
>> +ÂÂÂ if (!ring)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ ringacc = ring->parent;
>> +
>> +ÂÂÂ k3_nav_dbg(ring->parent->dev, "flags: 0x%08x\n", ring->flags);
>> +
>> +ÂÂÂ if (!test_bit(ring->ring_id, ringacc->rings_inuse))
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ mutex_lock(&ringacc->req_lock);
>> +
>> +ÂÂÂ if (--ring->use_count)
>> +ÂÂÂÂÂÂÂ goto out;
>> +
>> +ÂÂÂ if (!(ring->flags & K3_RING_FLAG_BUSY))
>> +ÂÂÂÂÂÂÂ goto no_init;
>> +
>> +ÂÂÂ k3_ringacc_ring_free_sci(ring);
>> +
>> +ÂÂÂ dma_free_coherent(ringacc->dev,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂ ring->size * (4 << ring->elm_size),
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂ ring->ring_mem_virt, ring->ring_mem_dma);
>> +ÂÂÂ ring->flags = 0;
>> +ÂÂÂ ring->ops = NULL;
>> +ÂÂÂ if (ring->proxy_id != K3_RINGACC_PROXY_NOT_USED) {
>> +ÂÂÂÂÂÂÂ clear_bit(ring->proxy_id, ringacc->proxy_inuse);
>> +ÂÂÂÂÂÂÂ ring->proxy = NULL;
>> +ÂÂÂÂÂÂÂ ring->proxy_id = K3_RINGACC_PROXY_NOT_USED;
>> +ÂÂÂ }
>> +
>> +no_init:
>> +ÂÂÂ clear_bit(ring->ring_id, ringacc->rings_inuse);
>> +
>> +ÂÂÂ module_put(ringacc->dev->driver->owner);
>> +
>> +out:
>> +ÂÂÂ mutex_unlock(&ringacc->req_lock);
>> +ÂÂÂ return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_free);
>> +
>> +u32 k3_ringacc_get_ring_id(struct k3_ring *ring)
>> +{
>> +ÂÂÂ if (!ring)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ return ring->ring_id;
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_get_ring_id);
>> +
>> +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

>> +ÂÂÂÂÂÂÂ 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;
>> +
>> +ÂÂÂ if (ring->proxy_id != K3_RINGACC_PROXY_NOT_USED)
>> +ÂÂÂÂÂÂÂ ring->proxy = ringacc->proxy_target_base +
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ring->proxy_id * K3_RINGACC_PROXY_TARGET_STEP;
>> +
>> +ÂÂÂ switch (ring->mode) {
>> +ÂÂÂ case K3_RINGACC_RING_MODE_RING:
>> +ÂÂÂÂÂÂÂ ring->ops = &k3_ring_mode_ring_ops;
>> +ÂÂÂÂÂÂÂ break;
>> +ÂÂÂ case K3_RINGACC_RING_MODE_QM:
>> +ÂÂÂÂÂÂÂ /*
>> +ÂÂÂÂÂÂÂÂ * In Queue mode elm_size can be 8 only and each operation
>> +ÂÂÂÂÂÂÂÂ * uses 2 element slots
>> +ÂÂÂÂÂÂÂÂ */
>> +ÂÂÂÂÂÂÂ if (cfg->elm_size != K3_RINGACC_RING_ELSIZE_8 ||
>> +ÂÂÂÂÂÂÂÂÂÂÂ cfg->size % 2)
>> +ÂÂÂÂÂÂÂÂÂÂÂ goto err_free_proxy;
>> +ÂÂÂÂÂÂÂ /* else, fall through */
>> +ÂÂÂ case K3_RINGACC_RING_MODE_MESSAGE:
>> +ÂÂÂÂÂÂÂ if (ring->proxy)
>> +ÂÂÂÂÂÂÂÂÂÂÂ ring->ops = &k3_ring_mode_proxy_ops;
>> +ÂÂÂÂÂÂÂ else
>> +ÂÂÂÂÂÂÂÂÂÂÂ ring->ops = &k3_ring_mode_msg_ops;
>> +ÂÂÂÂÂÂÂ break;
>> +ÂÂÂ default:
>> +ÂÂÂÂÂÂÂ ring->ops = NULL;
>> +ÂÂÂÂÂÂÂ ret = -EINVAL;
>> +ÂÂÂÂÂÂÂ goto err_free_proxy;
>> +ÂÂÂ };
>> +
>> +ÂÂÂ ring->ring_mem_virt =
>> +ÂÂÂÂÂÂÂÂÂÂÂ dma_alloc_coherent(ringacc->dev,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ring->size * (4 << ring->elm_size),
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &ring->ring_mem_dma, GFP_KERNEL);
>> +ÂÂÂ if (!ring->ring_mem_virt) {
>> +ÂÂÂÂÂÂÂ dev_err(ringacc->dev, "Failed to alloc ring mem\n");
>> +ÂÂÂÂÂÂÂ ret = -ENOMEM;
>> +ÂÂÂÂÂÂÂ goto err_free_ops;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ ret = k3_ringacc_ring_cfg_sci(ring);
>> +
>> +ÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂ goto err_free_mem;
>> +
>> +ÂÂÂ ring->flags |= K3_RING_FLAG_BUSY;
>> +ÂÂÂ ring->flags |= (cfg->flags & K3_RINGACC_RING_SHARED) ?
>> +ÂÂÂÂÂÂÂÂÂÂÂ K3_RING_FLAG_SHARED : 0;
>> +
>> +ÂÂÂ k3_ringacc_ring_dump(ring);
>> +
>> +ÂÂÂ return 0;
>> +
>> +err_free_mem:
>> +ÂÂÂ dma_free_coherent(ringacc->dev,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂ ring->size * (4 << ring->elm_size),
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂ ring->ring_mem_virt,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂ ring->ring_mem_dma);
>> +err_free_ops:
>> +ÂÂÂ ring->ops = NULL;
>> +err_free_proxy:
>> +ÂÂÂ ring->proxy = NULL;
>> +ÂÂÂ return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_cfg);
>> +
>> +u32 k3_ringacc_ring_get_size(struct k3_ring *ring)
>> +{
>> +ÂÂÂ if (!ring || !(ring->flags & K3_RING_FLAG_BUSY))
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ return ring->size;
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_get_size);
>> +
>> +u32 k3_ringacc_ring_get_free(struct k3_ring *ring)
>> +{
>> +ÂÂÂ if (!ring || !(ring->flags & K3_RING_FLAG_BUSY))
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ if (!ring->free)
>> +ÂÂÂÂÂÂÂ ring->free = ring->size - dbg_readl(&ring->rt->occ);
>> +
>> +ÂÂÂ return ring->free;
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_get_free);
>> +
>> +u32 k3_ringacc_ring_get_occ(struct k3_ring *ring)
>> +{
>> +ÂÂÂ if (!ring || !(ring->flags & K3_RING_FLAG_BUSY))
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ return dbg_readl(&ring->rt->occ);
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_get_occ);
>> +
>> +u32 k3_ringacc_ring_is_full(struct k3_ring *ring)
>> +{
>> +ÂÂÂ return !k3_ringacc_ring_get_free(ring);
>> +}
>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_is_full);
>> +
>> +enum k3_ringacc_access_mode {
>> +ÂÂÂ K3_RINGACC_ACCESS_MODE_PUSH_HEAD,
>> +ÂÂÂ K3_RINGACC_ACCESS_MODE_POP_HEAD,
>> +ÂÂÂ K3_RINGACC_ACCESS_MODE_PUSH_TAIL,
>> +ÂÂÂ K3_RINGACC_ACCESS_MODE_POP_TAIL,
>> +ÂÂÂ K3_RINGACC_ACCESS_MODE_PEEK_HEAD,
>> +ÂÂÂ K3_RINGACC_ACCESS_MODE_PEEK_TAIL,
>> +};
>> +
>> +static int k3_ringacc_ring_cfg_proxy(struct k3_ring *ring,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum k3_ringacc_proxy_access_mode mode)
>> +{
>> +ÂÂÂ u32 val;
>> +
>> +ÂÂÂ val = ring->ring_id;
>> +ÂÂÂ val |= mode << 16;
>> +ÂÂÂ val |= ring->elm_size << 24;
>
> Would be nice to have these magic shifts as defines.

OK, I'll add defines for the magic shifts.

>
>> +ÂÂÂ dbg_writel(val, &ring->proxy->control);
>> +ÂÂÂ return 0;
>> +}
>> +

Thanks for the review,
- PÃter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Next message: Fabian Henneke: "Re: [PATCH 4.19 19/57] Bluetooth: hidp: Let hidp_send_message return number of queued bytes"
Previous message: Vlastimil Babka: "Re: [PATCH v2 1/2] mm/page_ext: support to record the last stack of page"
In reply to: Vignesh Raghavendra: "Re: [PATCH v2 02/14] soc: ti: k3: add navss ringacc driver"
Next in thread: Grygorii Strashko: "Re: [PATCH v2 02/14] soc: ti: k3: add navss ringacc driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]