Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Aug 2023 23:44:38 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Martin =?utf-8?Q?Povi=C5=A1er?= <povik+lin@cutebit.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>, asahi@lists.linux.dev,
        dmaengine@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] dmaengine: apple-sio: Add Apple SIO driver
Message-ID: <ZMlLjg9UBi3QO/qV@matsya>
References: <20230712133806.4450-1-povik+lin@cutebit.org>
 <20230712133806.4450-3-povik+lin@cutebit.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230712133806.4450-3-povik+lin@cutebit.org>
Precedence: bulk

On 12-07-23, 15:38, Martin Povišer wrote:

> +struct sio_chan {
> +	unsigned int no;
> +	struct sio_data *host;
> +	struct dma_chan chan;
> +	struct tasklet_struct tasklet;
> +	struct work_struct terminate_wq;
> +
> +	spinlock_t lock;
> +	struct sio_tx *current_tx;
> +	/*
> +	 * 'tx_cookie' is used for distinguishing between transactions from
> +	 * within tag ack/nack callbacks. Without it, we would have no way
> +	 * of knowing if the current transaction is the one the callback handler
> +	 * was installed for.

not sure what you mean by here.. I dont see why you would need to store
cookie here, care to explain?

> +	 */
> +	unsigned long tx_cookie;
> +	int nperiod_acks;
> +
> +	/*
> +	 * We maintain a 'submitted' and 'issued' list mainly for interface
> +	 * correctness. Typical use of the driver (per channel) will be
> +	 * prepping, submitting and issuing a single cyclic transaction which
> +	 * will stay current until terminate_all is called.
> +	 */
> +	struct list_head submitted;
> +	struct list_head issued;
> +
> +	struct list_head to_free;

can you use virt_dma_chan, that should simplify list handling etc

> +};
> +
> +#define SIO_NTAGS		16
> +
> +typedef void (*sio_ack_callback)(struct sio_chan *, void *, bool);

any reason not to use dmaengine callbacks?

> +static int sio_alloc_tag(struct sio_data *sio)
> +{
> +	struct sio_tagdata *tags = &sio->tags;
> +	int tag, i;
> +
> +	/*
> +	 * Because tag number 0 is special, the usable tag range
> +	 * is 1...(SIO_NTAGS - 1). So, to pick the next usable tag,
> +	 * we do modulo (SIO_NTAGS - 1) *then* plus one.
> +	 */
> +
> +#define SIO_USABLE_TAGS (SIO_NTAGS - 1)
> +	tag = (READ_ONCE(tags->last_tag) % SIO_USABLE_TAGS) + 1;
> +
> +	for (i = 0; i < SIO_USABLE_TAGS; i++) {
> +		if (!test_and_set_bit(tag, &tags->allocated))
> +			break;
> +
> +		tag = (tag % SIO_USABLE_TAGS) + 1;
> +	}
> +
> +	WRITE_ONCE(tags->last_tag, tag);
> +
> +	if (i < SIO_USABLE_TAGS)
> +		return tag;
> +	else
> +		return -EBUSY;
> +#undef SIO_USABLE_TAGS
> +}

can you use kernel mechanisms like ida to alloc and free the tags...

> +static struct dma_async_tx_descriptor *sio_prep_dma_cyclic(
> +		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
> +		size_t period_len, enum dma_transfer_direction direction,
> +		unsigned long flags)
> +{
> +	struct sio_chan *siochan = to_sio_chan(chan);
> +	struct sio_tx *siotx = NULL;
> +	int i, nperiods = buf_len / period_len;
> +
> +	if (direction != sio_chan_direction(siochan->no))
> +		return NULL;
> +
> +	siotx = kzalloc(struct_size(siotx, siodesc, nperiods), GFP_NOWAIT);
> +	if (!siotx)
> +		return NULL;
> +
> +	init_completion(&siotx->done);
> +	dma_async_tx_descriptor_init(&siotx->tx, chan);
> +	siotx->period_len = period_len;
> +	siotx->nperiods = nperiods;
> +
> +	for (i = 0; i < nperiods; i++) {
> +		struct sio_coproc_desc *d;
> +
> +		siotx->siodesc[i] = d = sio_alloc_desc(siochan->host);
> +		if (!d) {
> +			sio_tx_free(&siotx->tx);
> +			return NULL;
> +		}
> +
> +		d->flag = 1; // not sure what's up with this
> +		d->iova = buf_addr + period_len * i;
> +		d->size = period_len;
> +	}
> +	dma_wmb();

why use barrier here? and to what purpose..

-- 
~Vinod