Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Fri, 6 Oct 2023 16:00:14 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Kelvin.Cao@microchip.com
Cc:     dmaengine@vger.kernel.org, George.Ge@microchip.com,
        christophe.jaillet@wanadoo.fr, hch@infradead.org,
        linux-kernel@vger.kernel.org, logang@deltatee.com
Subject: Re: [PATCH v6 1/1] dmaengine: switchtec-dma: Introduce Switchtec DMA
 engine PCI driver
Message-ID: <ZR/htuZSKGJP1wgU@matsya>
References: <20230728200327.96496-1-kelvin.cao@microchip.com>
 <20230728200327.96496-2-kelvin.cao@microchip.com>
 <ZMlSLXaYaMry7ioA@matsya>
 <fd597a2a71f1c5146c804bb9fce3495864212d69.camel@microchip.com>
 <b0dc3da623dee479386e7cb75841b8b7913c9890.camel@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <b0dc3da623dee479386e7cb75841b8b7913c9890.camel@microchip.com>
Precedence: bulk

On 05-10-23, 18:35, Kelvin.Cao@microchip.com wrote:

> > > > +static struct dma_async_tx_descriptor *
> > > > +switchtec_dma_prep_wimm_data(struct dma_chan *c, dma_addr_t
> > > > dma_dst, u64 data,
> > > > +????????????????????????? unsigned long flags)
> > > 
> > > can you please explain what this wimm data refers to...
> > > 
> > > I think adding imm callback was a mistake, we need a better
> > > justification for another user for this, who programs this, what
> > > gets
> > > programmed here
> > 
> > Sure. I think it's an alternative method to prep_mem and would be
> > more
> > convenient to use when the write is 8-byte and the data to be moved
> > is
> > not in a DMA mapped memory location. For example, we write to a
> > doorbell register with the value from a local variable which is not
> > associated with a DMA address to notify the receiver to consume the
> > data, after confirming that the previously initiated DMA transactions
> > of the data have completed. I agree that the use scenario would be
> > very
> > limited.

Can you please explain more about this 'value' where is it derived from?
Who programs it and how...

> > > > +???? /* set sq/cq */
> > > > +???? writel(lower_32_bits(swdma_chan->dma_addr_sq), &chan_fw-
> > > > > sq_base_lo);
> > > > +???? writel(upper_32_bits(swdma_chan->dma_addr_sq), &chan_fw-
> > > > > sq_base_hi);
> > > > +???? writel(lower_32_bits(swdma_chan->dma_addr_cq), &chan_fw-
> > > > > cq_base_lo);
> > > > +???? writel(upper_32_bits(swdma_chan->dma_addr_cq), &chan_fw-
> > > > > cq_base_hi);
> > > > +
> > > > +???? writew(SWITCHTEC_DMA_SQ_SIZE, &swdma_chan->mmio_chan_fw-
> > > > > sq_size);
> > > > +???? writew(SWITCHTEC_DMA_CQ_SIZE, &swdma_chan->mmio_chan_fw-
> > > > > cq_size);
> > > 
> > > what is write happening in the descriptor alloc callback, that does
> > > not
> > > sound correct to me
> > 
> > All the queue descriptors of a channel are pre-allocated, so I think
> > it's proper to convey the queue address/size to hardware at this
> > point.
> > After this initialization, we only need to assign cookie in submit
> > and
> > update queue head to hardware in issue_pending.

Sorry that is not right, you can prepare multiple descriptors and then
submit. Only at submit is the cookie assigned which is in order, so this
should be moved to when we start the txn and not in this call

-- 
~Vinod