Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Joy Chakraborty]

Hello Andy,

On Tue, Apr 11, 2023 at 5:48 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> On Thu, Mar 30, 2023 at 06:34:50AM +0000, Joy Chakraborty wrote:
> > Check capabilities of DMA controller during init to make sure it is
> > capable of handling MEM2DEV for tx channel, DEV2MEM for rx channel
> > and store addr_width capabilities to check per transfer to make sure the
> > bits/word requirement can be met for that transfer.
>
> ...
>
> > +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
>
> Can we avoid forward declarations please?

We can, but for that I would have to move this api somewhere else in
the file is that acceptable?

>
> ...
>
> > +     if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > +           rx.directions & BIT(DMA_DEV_TO_MEM)))
> > +             return -ENXIO;
>
> What about simplex transfers where we only care about sending or receiving data
> and using dummy data for the other channel? Doesn't this make a regression for
> that types of transfers? (Or, if we don't support such, this should be explained
> in the commit message at least.)
>

Yes we can have simplex transfers for TX only, but for RX only there
is dummy data added by the SPI core as the dw-core driver adds
"SPI_CONTROLLER_MUST_TX".

But transfers aside, as per the current driver design the dw dma
driver needs both valid tx and rx channels to exist and be functional
as per implementation of api "dw_spi_dma_init_generic" :
...
dws->rxchan = dma_request_chan(dev, "rx");
if (IS_ERR(dws->rxchan)) {
ret = PTR_ERR(dws->rxchan);
dws->rxchan = NULL;
goto err_exit;
}

dws->txchan = dma_request_chan(dev, "tx");
if (IS_ERR(dws->txchan)) {
ret = PTR_ERR(dws->txchan);
dws->txchan = NULL;
goto free_rxchan;
}
...

Hence in terms of capability check we should check for directional
capability of both TX and RX is what I understand.
Please let me know if you think otherwise.

> ...
>
> > +     /*
> > +      * Assuming both channels belong to the same DMA controller hence the
> > +      * address width capabilities most likely would be the same.
> > +      */
> > +     dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;
>
> I don't think so this is correct.
>
> Theoretically it's possible to have simplex transfers on which the one of
> the channel is simply ignored / not used. See above.
>

Yes, it is possible to have tx only transfers which would still be
possible to do with this implementation. What we have assumed here is
since the tx and rx channel both are a requirement for the dw dma
driver to be functional it would most likely have the same address
width capability.

But we can make this more elaborate by checking it for both tx and rx
separately.
Something like this
...
 dws->tx_dma_addr_widths = tx.dst_addr_widths;
 dws->rx_dma_addr_widths = rx.src_addr_widths;
...
...
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;

if (xfer->len <= dws->fifo_len)
return false;

dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes);

if (xfer->rx_buf && !(dws->rx_dma_addr_widths & BIT(dma_bus_width)))
return false;

return dws->tx_dma_addr_widths & BIT(dma_bus_width);
}
...

@Serge Semin Please share your thoughts on the same.

> --
> With Best Regards,
> Andy Shevchenko
>
>

I shall break this into 2 patches as per Serge(y)'s recommendation and
make changes as per replies.

Thanks
Joy