dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Peter Ujfalusi <peter.ujfalusi@ti.com>
To: Lokesh Vutla <lokeshvutla@ti.com>, <vkoul@kernel.org>,
	<robh+dt@kernel.org>, <nm@ti.com>, <ssantosh@kernel.org>
Cc: <dan.j.williams@intel.com>, <dmaengine@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<grygorii.strashko@ti.com>, <t-kristo@ti.com>, <tony@atomide.com>,
	<j-keerthy@ti.com>
Subject: Re: [PATCH v3 02/14] soc: ti: k3: add navss ringacc driver
Date: Fri, 25 Oct 2019 12:30:23 +0300	[thread overview]
Message-ID: <0a698278-19d8-bb20-34b9-9695d670b3a8@ti.com> (raw)
In-Reply-To: <86344789-e0f4-5b29-62da-3fb08025177b@ti.com>



On 09/10/2019 16.27, Lokesh Vutla wrote:
>> +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;
>> +
>> +	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;
> 
> This can be dropped no? pdev->id has it already.

pdev->id might have it but it is simpler to keep it here than getting
the pdev when we need it

...

>> +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);
> 
> ti_sci_get_free resource can be used no? In case if id is passed, that bit alone
> can be set.

Hrm, kind of yes.
We have a bitfield for _all_ rings managed locally so I don't see much
benefit to manage another bitfiled usage, which is redundant.

>> +		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);
> 
> May be a dump question, but how do we make sure that these proxies are not used
> by another Hosts?

That's a good question. Grygorii?

> 
>> +		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;
>> +		dev_dbg(ringacc->dev, "Giving ring#%d proxy#%d\n", id,
>> +			proxy_id);
>> +	} else {
>> +		dev_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);
>> +

...
>> +	pm_runtime_enable(dev);
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0) {
>> +		pm_runtime_put_noidle(dev);
>> +		dev_err(dev, "Failed to enable pm %d\n", ret);
>> +		goto err;
>> +	}
> 
> Don't you need power-domains property in DT so that pm is actually working? If
> that is populated, dev-id can be derived from power-domains rather than a
> separate dt property.

Right, I never felt comfortable to fiddle with something outside of the
scope of the driver. What happens (unlikely) if the power-domains
binding got changed for some reason?

Another thing is that the whole NAVSS is always on, it can not power off
as it would loose all of it's configuration including event mappings,
DMA channel configurations, interrupt configs, ring configurations,
mailbox, timers, etc.

It is a catastrophic thing which can only be solved with a hard reboot
as there is no way to recover from it - system firmware would need to
rebooted as well.

In current SoCs NAVSS can not be off. Without power-domains in DT the
pm_runtime is NOP, but if this changes (NAVSS could turn off) we need to
prevent NAVSS power off and the code is ready for that, we just pop in
the power-domains to DT.

> 
> [...snip..]
> 
> 
>> diff --git a/include/linux/soc/ti/k3-ringacc.h b/include/linux/soc/ti/k3-ringacc.h
>> new file mode 100644
>> index 000000000000..526b2e38fcce
>> --- /dev/null
>> +++ b/include/linux/soc/ti/k3-ringacc.h
>> @@ -0,0 +1,245 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * K3 Ring Accelerator (RA) subsystem interface
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
>> + */
>> +
>> +#ifndef __SOC_TI_K3_RINGACC_API_H_
>> +#define __SOC_TI_K3_RINGACC_API_H_
>> +
>> +#include <linux/types.h>
>> +
>> +struct device_node;
>> +
> 
> [...snip..]
> 
>> +
>> +/**
>> + * k3_ringacc_ring_reset - ring reset
>> + * @ring: pointer on Ring
>> + *
>> + * Resets ring internal state ((hw)occ, (hw)idx).
>> + * TODO_GS: ? Ring can be reused without reconfiguration
> 
> TODO_GS?
> 
> Thanks and regards,
> Lokesh
> 

- Péter

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

  reply	other threads:[~2019-10-25  9:29 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-01  6:16 [PATCH v3 00/14] dmaengine/soc: Add Texas Instruments UDMA support Peter Ujfalusi
2019-10-01  6:16 ` [PATCH v3 01/14] bindings: soc: ti: add documentation for k3 ringacc Peter Ujfalusi
2019-10-01  6:16 ` [PATCH v3 02/14] soc: ti: k3: add navss ringacc driver Peter Ujfalusi
2019-10-09  7:14   ` Tero Kristo
2019-10-09 13:27   ` Lokesh Vutla
2019-10-25  9:30     ` Peter Ujfalusi [this message]
2019-10-29  8:52   ` Lokesh Vutla
2019-10-30 13:10     ` Grygorii Strashko
2019-10-01  6:16 ` [PATCH v3 03/14] dmaengine: doc: Add sections for per descriptor metadata support Peter Ujfalusi
2019-10-09  7:30   ` Tero Kristo
2019-10-01  6:16 ` [PATCH v3 04/14] dmaengine: Add metadata_ops for dma_async_tx_descriptor Peter Ujfalusi
2019-10-09  7:37   ` Tero Kristo
2019-10-01  6:16 ` [PATCH v3 05/14] dmaengine: Add support for reporting DMA cached data amount Peter Ujfalusi
2019-10-09  9:17   ` Tero Kristo
2019-10-01  6:16 ` [PATCH v3 06/14] dmaengine: ti: Add cppi5 header for UDMA Peter Ujfalusi
2019-10-09  9:13   ` Tero Kristo
2019-10-01  6:16 ` [PATCH v3 07/14] dt-bindings: dma: ti: Add document for K3 UDMA Peter Ujfalusi
2019-10-10 17:52   ` Rob Herring
2019-10-11  7:30     ` Peter Ujfalusi
2019-10-15 17:30       ` Peter Ujfalusi
2019-10-17 14:03         ` Rob Herring
2019-10-22 11:46           ` Peter Ujfalusi
2019-10-01  6:16 ` [PATCH v3 08/14] dmaengine: ti: New driver for K3 UDMA - split#1: defines, structs, io func Peter Ujfalusi
2019-10-09 13:40   ` Tero Kristo
2019-10-01  6:16 ` [PATCH v3 09/14] dmaengine: ti: New driver for K3 UDMA - split#2: probe/remove, xlate and filter_fn Peter Ujfalusi
2019-10-03  8:35   ` Peter Ujfalusi
2019-10-01  6:17 ` [PATCH v3 10/14] dmaengine: ti: New driver for K3 UDMA - split#3: alloc/free chan_resources Peter Ujfalusi
2019-10-01  6:17 ` [PATCH v3 11/14] dmaengine: ti: New driver for K3 UDMA - split#4: dma_device callbacks 1 Peter Ujfalusi
2019-10-01  6:17 ` [PATCH v3 12/14] dmaengine: ti: New driver for K3 UDMA - split#5: dma_device callbacks 2 Peter Ujfalusi
2019-10-01  6:17 ` [PATCH v3 13/14] dmaengine: ti: New driver for K3 UDMA - split#6: Kconfig and Makefile Peter Ujfalusi
2019-10-01  6:17 ` [PATCH v3 14/14] dmaengine: ti: k3-udma: Add glue layer for non DMAengine users Peter Ujfalusi
2019-10-04 16:35 ` [PATCH v3 00/14] dmaengine/soc: Add Texas Instruments UDMA support santosh.shilimkar
2019-10-08 10:09   ` Peter Ujfalusi
2019-10-09 16:02     ` santosh.shilimkar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0a698278-19d8-bb20-34b9-9695d670b3a8@ti.com \
    --to=peter.ujfalusi@ti.com \
    --cc=dan.j.williams@intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dmaengine@vger.kernel.org \
    --cc=grygorii.strashko@ti.com \
    --cc=j-keerthy@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lokeshvutla@ti.com \
    --cc=nm@ti.com \
    --cc=robh+dt@kernel.org \
    --cc=ssantosh@kernel.org \
    --cc=t-kristo@ti.com \
    --cc=tony@atomide.com \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).