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

[Joy Chakraborty]

Sorry I think the emails crossed.

So as per the discussion, are these the possible changes:
1> Move "dw_spi_dma_convert_width" to avoid forward declaration .
2> Update the commit text to include more explanation.
3> Divide this into 2 patches?

Thanks
Joy

Joy

On Tue, Apr 11, 2023 at 8:30 PM Joy Chakraborty <joychakr@xxxxxxxxxx> wrote:
>
> 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