Re: [PATCH v2 2/2] dmaengine: apple-sio: Add Apple SIO driver

[Martin Povišer]

> On 4. 10. 2023, at 15:46, Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> 
> On 04-10-23, 15:32, Martin Povišer wrote:
> 
>>>> + * There are two kinds of 'transaction descriptors' in play here.
>>>> + *
>>>> + * There's the struct sio_tx, and the struct dma_async_tx_descriptor embedded
>>>> + * inside, which jointly represent a transaction to the dmaengine subsystem.
>>>> + * At this time we only support those transactions to be cyclic.
>>>> + *
>>>> + * Then there are the coprocessor descriptors, which is what the coprocessor
>>>> + * knows and understands. These don't seem to have a cyclic regime, so we can't
>>>> + * map the dmaengine transaction on an exact coprocessor counterpart. Instead
>>>> + * we continually queue up many coprocessor descriptors to implement a cyclic
>>>> + * transaction.
>>>> + *
>>>> + * The number below is the maximum of how far ahead (how many) coprocessor
>>>> + * descriptors we should be queuing up, per channel, for a cyclic transaction.
>>>> + * Basically it's a made-up number.
>>>> + */
>>>> +#define SIO_MAX_NINFLIGHT 4
>>> 
>>> you meant SIO_MAX_INFLIGHT if not what is NINFLIGHT?
>> 
>> I mean the number is arbitrary, it doesn’t reflect any coprocessor limit since
>> I haven’t run the tests to figure one out. It's supposed to be a small reasonable
>> number.
> 
> Sorry that was not my question. Should this macro be SIO_MAX_NINFLIGHT
> or SIO_MAX_INFLIGHT..?

Yeah, I realized after I sent the reply, sorry. I don’t know what you would
interpret to be the difference between NINFLIGHT and INFLIGHT, in my book
both would be the "number of inflight” in the context here.

>>>> +static int sio_device_config(struct dma_chan *chan,
>>>> +      struct dma_slave_config *config)
>>>> +{
>>>> + struct sio_chan *siochan = to_sio_chan(chan);
>>>> + struct sio_data *sio = siochan->host;
>>>> + bool is_tx = sio_chan_direction(siochan->no) == DMA_MEM_TO_DEV;
>>>> + struct sio_shmem_chan_config *cfg = sio->shmem;
>>>> + int ret;
>>>> +
>>>> + switch (is_tx ? config->dst_addr_width : config->src_addr_width) {
>>>> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
>>>> + cfg->datashape = 0;
>>>> + break;
>>>> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
>>>> + cfg->datashape = 1;
>>>> + break;
>>>> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
>>>> + cfg->datashape = 2;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + cfg->fifo = 0x800;
>>>> + cfg->limit = 0x800;
>>>> + cfg->threshold = 0x800;
>>>> + dma_wmb();
>>> 
>>> ??
>> 
>> Again, shared memory
>> 
>>>> +
>>>> + ret = sio_call(sio, FIELD_PREP(SIOMSG_TYPE, MSG_CONFIGURE) |
>>>> +     FIELD_PREP(SIOMSG_EP, siochan->no));
>>> 
>>> this does not sound okay, can you explain why this call is here
>> 
>> We are sending the configuration to the coprocessor, it will NACK
>> it if invalid, seems very fitting here.
> 
> I dont this so, purpose of the device_config() is to send peripheral
> config to driver for use on the next descriptor which is submitted. So
> sending to co-processor now (when we might even have a txn going on)
> does not seem right
> 
> What would be the behaviour if already a txn is progressing on the
> co-processor

I have no idea.

OK, though is that necessarily part of the dmaengine interface? I ask
because the other driver I have written (apple-admac.c) does basically
the same, only it applies the new configuration in MMIO registers rather
than sending it to a coprocessor, but the end result is the same:
the configuration gets checked for validity, and applied right away.

Martin

> -- 
> ~Vinod
>