From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CEDFA23589 for ; Tue, 1 Aug 2023 18:14:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9273CC433C8; Tue, 1 Aug 2023 18:14:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1690913682; bh=LGldI8xCzG+/NgQzVjLKjdG/7h/Dszwve+qs5ujXVF8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TCsELIIPVi+kTn2SFKo2ob8zx4LDQ4kyn3/ZvcLP+nzaA4m954+qkkzXput8dsDFg hUNZylYmMCYLt0lnq7C9rCJGKXbUY38JnYxWqxp1AhTOFvORUgW1kc9CtX6a98AEYZ Q3/c4U7TI1q5g+yxwD/cEbxbS2VTBLrPgdclbK5ERzUSaXYwZafjWmZWmwP6+vk5xP PpaSnAZH1ALyJhi4MjCBgVF2Q1bX4shrGc8RhmIX80CaJnCCLTVH48qhLvflO+iA8Y TOUoklQJ721XRJnWei/eP9Gd1883n/WCz0kF41J+abxdrP/adlakA6GeZLefc+V9BW 2HPu5Ju3/MS2w== Date: Tue, 1 Aug 2023 23:44:38 +0530 From: Vinod Koul To: Martin =?utf-8?Q?Povi=C5=A1er?= Cc: Rob Herring , Krzysztof Kozlowski , Conor Dooley , 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: References: <20230712133806.4450-1-povik+lin@cutebit.org> <20230712133806.4450-3-povik+lin@cutebit.org> Precedence: bulk X-Mailing-List: asahi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: 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> 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