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, 11 Apr 2023 18:09:16 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Andy Shevchenko <andriy.shevchenko@intel.com>
Cc:     Joy Chakraborty <joychakr@google.com>,
        Mark Brown <broonie@kernel.org>, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org, manugautam@google.com,
        rohitner@google.com
Subject: Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller
Message-ID: <20230411150916.24vagjzbhshaujse@mobilestation>
References: <20230330063450.2289058-1-joychakr@google.com>
 <20230330063450.2289058-2-joychakr@google.com>
 <ZDVO/fpOZm+lGVZE@smile.fi.intel.com>
 <20230411141115.vrfd6sud66u6xeog@mobilestation>
 <ZDVyyoN71cB7dvWW@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZDVyyoN71cB7dvWW@smile.fi.intel.com>
Precedence: bulk

On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
> 
> ...
> 
> > > > -	if (n_bytes == 1)
> > > > +	switch (n_bytes) {
> > > > +	case 1:
> > > >  		return DMA_SLAVE_BUSWIDTH_1_BYTE;
> > > > -	else if (n_bytes == 2)
> > > > +	case 2:
> > > >  		return DMA_SLAVE_BUSWIDTH_2_BYTES;
> > > > -
> > > > -	return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > 
> > > > +	case 3:
> > > 
> > > I'm not sure about this.
> > 
> > This actually makes sense seeing the function argument can have values
> > 1, 2, _3_ and 4:
> > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > ...
> > dw_spi_dma_convert_width(dws->n_bytes)
> > 
> > The spi_transfer.bits_per_word field value depends on the
> > SPI peripheral device communication protocol requirements which may
> > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > 
> > This semantic will also match to what we currently have in the
> > IRQ-based SPI-transfer implementation (see dw_writer() and
> > dw_reader()).
> 

> Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> use it?

We could but there are two more-or-less firm reasons not to do
that:
1. There aren't that much DMA-engines with the
DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
ignores the upper bits if CTRLR0.DFS is less than the value actual
written to the DR registers. Note DW DMAC engine isn't one of such
controllers. So if we get to meet a peripheral SPI-device with 3-bytes
word protocol transfers and the DMA-engine doesn't support it the
DMA-based transfers may fail (depending on the DMA-engine driver
implementation).
2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
the 3-bytes bus width the system bus most likely will either convert
the transfers to the proper sized bus-transactions or fail.

So taking all of the above into account not using the
DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
a risk to fail some of the platform setups especially seeing the DW
APB SSI ignores the upper bits anyway.

-Serge(y)

> 
> > > > +	case 4:
> > > > +		return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > +	default:
> > > > +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>