Re: [PATCH v2] spi: dw: Add 32 bpw support to DW DMA Controller

From: Serge Semin
Date: Thu Mar 23 2023 - 10:49:12 EST

Next message: Chester Lin: "[PATCH v3 1/6] pinctrl: s32g2: use of_device_get_match_data() to get device data"
Previous message: Chester Lin: "[PATCH v3 0/6] pinctrl: s32: driver improvements and generic struct use"
In reply to: Joy Chakraborty: "Re: [PATCH v2] spi: dw: Add 32 bpw support to DW DMA Controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Mar 23, 2023 at 05:32:09PM +0530, Joy Chakraborty wrote:
> Hi Serge(y),
>
> On Wed, Mar 22, 2023 at 12:57 AM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> >
> > On Tue, Mar 21, 2023 at 11:58:43AM +0000, Joy Chakraborty wrote:
> > > If DW Controller is capable of a maximum of 32 bits per word then SW or
> > > DMA controller has to write up to 32bit or 4byte data to the FIFO at a
> > > time.
> > >
> >
> > > This Patch adds support for AxSize = 4 bytes configuration from dw dma
> >
> > * sorry for referring to the newbie-doc, but please note:
> > https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L77
> > and
> > https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L94
> >
>
> Thank you for the point, I will rephrase the commit text.
>
> > > driver if n_bytes i.e. number of bytes per write to fifo is 3 or 4.
> > > It also checks to see if the dma controller is capable of satisfying the
> > > width requirement to achieve a particular bits/word requirement per
> > > transfer.
> > >
> > > Signed-off-by: Joy Chakraborty <joychakr@xxxxxxxxxx>
> > > ---
> > > drivers/spi/spi-dw-dma.c | 37 ++++++++++++++++++++++++++++++++-----
> > > drivers/spi/spi-dw.h | 1 +
> > > 2 files changed, 33 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> > > index ababb910b391..9ac3a1c25e2d 100644
> > > --- a/drivers/spi/spi-dw-dma.c
> > > +++ b/drivers/spi/spi-dw-dma.c
> > > @@ -23,6 +23,8 @@
> > > #define DW_SPI_TX_BUSY 1
> > > #define DW_SPI_TX_BURST_LEVEL 16
> > >
> > > +static inline enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
> > > +
> > > static bool dw_spi_dma_chan_filter(struct dma_chan *chan, void *param)
> > > {
> > > struct dw_dma_slave *s = param;
> > > @@ -89,6 +91,16 @@ static void dw_spi_dma_sg_burst_init(struct dw_spi *dws)
> > > dws->dma_sg_burst = 0;
> > > }
> > >
> > > +static void dw_spi_dma_addr_widths_init(struct dw_spi *dws)
> > > +{
> > > + struct dma_slave_caps tx = {0}, rx = {0};
> > > +
> >
> > > + dma_get_slave_caps(dws->txchan, &tx);
> > > + dma_get_slave_caps(dws->rxchan, &rx);
> >
> > Even though in this case any dma_get_slave_caps() failure will
> > effectively disable the DMA-based transfers, in general it would be
> > useful to have the dma_get_slave_caps() return value checked and halt
> > further DMA-init in case if it's not zero. In addition to that if the
> > Tx/Rx DMA device doesn't have the DMA_SLAVE capability or DEV2MEM and
> > MEM2DEV direction specified the DMA device won't be suitable for
> > SPI-ing. So further DMA-initialization are pointless in that case too.
> > It's just a general note not obligating or requesting anything since
> > the respective update should have been done in a separate patch
> > anyway.
> >
>

> I shall add the checks suggested and put 'dw_spi_dma_addr_widths_init'
> and 'dw_spi_dma_sg_burst_init' in one function.
> I'll break this up into 2 patches in V3.

Sounds good. Thanks.

>
> > > +
> >
> > > + dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;
> >
> > Hm, in general the addr-width capabilities can mismatch. But it's very
> > much unlikely since both DMA channels normally belong to the same
> > controller. So I guess we can live with the suggested approach for now
> > but please add a comment above that line about the
> > assumption/limitation it implies.
> >
>

> Even if the address width capabilities mismatch since in dma mode only
> full duplex is done, hence the subset of the capabilities which apply
> to both tx and rx should be applicable.
> I shall put the same as a comment

Actually half-duplex xfers are also possible. See what happens if
rx_buf is Null or what the SPI_CONTROLLER_MUST_TX flag means (it's set
if the dma_init callback is successfully executed). In the former case
the Rx data will be just ignored, in the later case Tx-data will be
read from a dummy Tx-buffer. In both cases it doesn't matter what
bus-width is initialized in the DMA-controller. But in anyway as I
said before it's not that a big deal to have the widths combined.

>
> > > +}
> > > +
> > > static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
> > > {
> > > struct dw_dma_slave dma_tx = { .dst_id = 1 }, *tx = &dma_tx;
> > > @@ -128,6 +140,8 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
> > >
> > > dw_spi_dma_sg_burst_init(dws);
> > >
> > > + dw_spi_dma_addr_widths_init(dws);
> > > +
> > > pci_dev_put(dma_dev);
> > >
> > > return 0;
> > > @@ -167,6 +181,8 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws)
> > >
> > > dw_spi_dma_sg_burst_init(dws);
> > >
> > > + dw_spi_dma_addr_widths_init(dws);
> > > +
> > > return 0;
> > >
> > > free_rxchan:
> > > @@ -202,18 +218,29 @@ static bool dw_spi_can_dma(struct spi_controller *master,
> > > struct spi_device *spi, struct spi_transfer *xfer)
> > > {
> > > struct dw_spi *dws = spi_controller_get_devdata(master);
> >
> > > + enum dma_slave_buswidth dma_bus_width;
> > >
> > > - return xfer->len > dws->fifo_len;
> > > + if (xfer->len > dws->fifo_len) {
> > > + dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes);
> > > + if (dws->dma_addr_widths & BIT(dma_bus_width))
> > > + return true;
> > > + }
> > < newline would have been nice, but...
> > > + return false;
> >
> > on the other hand a level of indentation could be decreased like this:
> >
> > + enum dma_slave_buswidth width;
> > +
> > + if (xfer->len <= dws->fifo_len)
> > + return false;
> > +
> > + width = dw_spi_dma_convert_width(dws->n_bytes);
> > +
> > + return !!(dws->dma_addr_widths & BIT(width));
> >
>
> Sure, I will restructure this but

> any reason to use '!!' here ?

No. It can be omitted indeed. The resultant integer will be implicitly
converted to one of the _Bool type values {true, false}.

-Serge(y)

>
> > -Serge(y)
> >
> > > }
> > >
> > > static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes)
> > > {
> > > - 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:
> > > + case 4:
> > > + return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > + default:
> > > + return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > + }
> > > }
> > >
> > > static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed)
> > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > > index 9e8eb2b52d5c..3962e6dcf880 100644
> > > --- a/drivers/spi/spi-dw.h
> > > +++ b/drivers/spi/spi-dw.h
> > > @@ -190,6 +190,7 @@ struct dw_spi {
> > > struct dma_chan *rxchan;
> > > u32 rxburst;
> > > u32 dma_sg_burst;
> > > + u32 dma_addr_widths;
> > > unsigned long dma_chan_busy;
> > > dma_addr_t dma_addr; /* phy address of the Data register */
> > > const struct dw_spi_dma_ops *dma_ops;
> > > --
> > > 2.40.0.rc1.284.g88254d51c5-goog
> > >
>
> I shall upload a V3 based on these comments.
>
> Thanks
> Joy

Next message: Chester Lin: "[PATCH v3 1/6] pinctrl: s32g2: use of_device_get_match_data() to get device data"
Previous message: Chester Lin: "[PATCH v3 0/6] pinctrl: s32: driver improvements and generic struct use"
In reply to: Joy Chakraborty: "Re: [PATCH v2] spi: dw: Add 32 bpw support to DW DMA Controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]