Hi Peter,
On 25-08-20, 11:00, Peter Ujfalusi wrote:
> Hi Vinod,
>
> On 25/08/2020 10.10, Vinod Koul wrote:
> >>> /**
> >>> * struct dma_slave_config - dma slave channel runtime config
> >>> * @direction: whether the data shall go in or out on this slave
> >>> @@ -418,6 +485,10 @@ enum dma_slave_buswidth {
> >>> * @slave_id: Slave requester id. Only valid for slave channels. The dma
> >>> * slave peripheral will have unique id as dma requester which need to be
> >>> * pass as slave config.
> >>> + * @peripheral: type of peripheral to DMA to/from
> >>> + * @set_config: set peripheral config
> >>> + * @spi: peripheral config for spi
> >>> + * @:i2c peripheral config for i2c
> >>> *
> >>> * This struct is passed in as configuration data to a DMA engine
> >>> * in order to set up a certain channel for DMA transport at runtime.
> >>> @@ -443,6 +514,10 @@ struct dma_slave_config {
> >>> u32 dst_port_window_size;
> >>> bool device_fc;
> >>> unsigned int slave_id;
> >>> + enum dmaengine_peripheral peripheral;
> >>> + u8 set_config;
> >>> + struct dmaengine_spi_config spi;
> >>> + struct dmaengine_i2c_config i2c;
> >>
> >> Would it be possible to reuse one of the existing feature already
> >> supported by DMAengine?
> >> We have DMA_PREP_CMD to give a command instead of a real transfer:
> >> dmaengine_prep_slave_single(tx_chan, config_data, config_len,
> >> DMA_MEM_TO_DEV, DMA_PREP_CMD);
> >> dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len, DMA_MEM_TO_DEV,
> >> DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> >> dma_async_issue_pending(tx_chan);
> >>
> >> or the metadata support:
> >> tx = dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len,
> >> DMA_MEM_TO_DEV,
> >> DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> >> dmaengine_desc_attach_metadata(tx, config_data, config_len);
> >> dma_async_issue_pending(tx_chan);
> >>
> >> By reading the driver itself, it is not clear if you always need to send
> >> the config for TX, or only when the config is changing and what happens
> >> if the first transfer (for SPI, since that is the only implemented one)
> >> is RX, when you don't send config at all...
> >
> > So this config is sent to driver everytime before the prep call (can be
> > optimized to once if we have similar transfers in queue).
>
> I see that you queue the TREs in the prep callback.
Yes
> > This config is used to create the configuration passed to dmaengine
> > which is used to actually program both dmaengine as well as peripheral
> > registers (i2c/spi/etc), so we need a way to pass the spi/i2c config.
>
> But do you need to send it with each DMA_MEM_TO_DEV or only once?
> DMA_DEV_TO_MEM does not set the config, so I assume you must have one TX
> to initialize the peripheral as the first transfer.
Correct and we do transfers on same without sending configuration again.
> > I think prep cmd can be used to send this data, I do not see any issues
> > with that, it would work if we want to go that route.
>
> The only thing which might be an issue is that with the DMA_PREP_CMD the
> config_data is dma_addr_t (via dmaengine_prep_slave_single).
Yes I came to same conclusion
> > I did have a prototype with metadata but didnt work very well, the
> > problem is it assumes metadata for tx/rx but here i send the data
> > everytime from client data.
>
> Yes, the intended use case for metadata (per descriptor!) is for
> channels where each transfer might have different metadata needed for
> the given transfer (tx/rx).
>
> In your case you have semi static peripheral configuration data, which
> is not really changing between transfers.
>
> A compromise would be to add:
> void *peripheral_config;
> to the dma_slave_config, move the set_config inside of the device
> specific struct you are passing from a client to the core?
That sounds more saner to me and uses existing interfaces cleanly. I
think I like this option ;-)
> >> I'm concerned about the size increase of dma_slave_config (it grows by
> >>> 30 bytes) and for DMAs with hundreds of channels (UDMA) it will add up
> >> to a sizeable amount.
> >
> > I agree that is indeed a valid concern, that is the reason I tagged this
> > as a RFC patch ;-)
> >
> > I see the prep_cmd is a better approach for this, anyone else has better
> > suggestions?
> >
> > Thanks for looking in.
> >
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
~Vinod
