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

[Peter Ujfalusi]

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