[v3,0/2] Apple SIO driver

Hi,

on v2 of this driver we had discussion over the device_config op not
following the dmaengine spec. I came up with a lazy solution I am
happy with. See sio_device_config for details.

Changes since v2:
https://lore.kernel.org/asahi/CAEg-Je8_f_hZ3VyBg+8tK8uobWNaEqCwp==2JhV6jVpPYXj_Pg@mail.gmail.com/T/#t
 - do requested formatting fixes
 - fix device_config up to spec in a lazy way

Changes since v1:
https://lore.kernel.org/asahi/20230712133806.4450-1-povik+lin@cutebit.org/T/#t
 - move to using virt-dma
 - drop redundant cookie field from `sio_tx`
 - use DECLARE_BITMAP for `allocated` in sio_tagdata

Original cover letter from v1 follows.
--

Hi all,

see attached a driver for the SIO coprocessor found on recent Apple
SoCs. This coprocessor provides general DMA services, it can feed
many peripherals but so far it seems it will only be useful for
audio output over HDMI/DisplayPort. So the driver here only supports
the DMA_CYCLIC mode of transactions with the focus being on audio.
There's a downstream prototype ALSA driver the DMA driver is being
tested against.

Some of the boilerplate code in implementing the dmaengine interface
was lifted from apple-admac.c. Among other things these two drivers
have in common that they implement the DMA_CYCLIC regime on top of
hardware/coprocessor layer supporting linear transactions only.

The binding schema saw two RFC rounds before and has a reviewed-by
from Rob.
https://lore.kernel.org/asahi/167693643966.613996.10372170526471864080.robh@kernel.org

Best regards,
Martin


Martin Povišer (2):
  dt-bindings: dma: apple,sio: Add schema
  dmaengine: apple-sio: Add Apple SIO driver

 .../devicetree/bindings/dma/apple,sio.yaml    | 111 +++
 MAINTAINERS                                   |   2 +
 drivers/dma/Kconfig                           |  11 +
 drivers/dma/Makefile                          |   1 +
 drivers/dma/apple-sio.c                       | 907 ++++++++++++++++++
 5 files changed, 1032 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/apple,sio.yaml
 create mode 100644 drivers/dma/apple-sio.c

On 13-10-23, 18:49, Martin Povišer wrote:

> +static struct dma_async_tx_descriptor *sio_prep_dma_cyclic(

this should generate a check warning, typically lines should not end with a '('

> +		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
> +		size_t period_len, enum dma_transfer_direction direction,
> +		unsigned long flags)

also helps to align with the open brace to something like:
static struct dma_async_tx_descriptor *
sio_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
                    size_t period_len, enum dma_transfer_direction direction,
                    unsigned long flags)

> +static bool sio_fill_in_locked(struct sio_chan *siochan);
> +
> +static void sio_handle_issue_ack(struct sio_chan *siochan, void *cookie, bool ok)
> +{
> +	dma_cookie_t tx_cookie = (unsigned long) cookie;

space not required after a cast

> +static struct dma_chan *sio_dma_of_xlate(struct of_phandle_args *dma_spec,
> +					 struct of_dma *ofdma)
> +{
> +	struct sio_data *sio = (struct sio_data *) ofdma->of_dma_data;

drop space after cast here too

> +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_shmem = sio->shmem;
> +	struct sio_shmem_chan_config cfg;
> +	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;

what do these values represent, should this not be passed by the client

> +static int sio_alloc_shmem(struct sio_data *sio)
> +{
> +	dma_addr_t iova;
> +	int err;
> +
> +	sio->shmem = dma_alloc_coherent(sio->dev, SIO_SHMEM_SIZE,
> +					&iova, GFP_KERNEL);
> +	if (!sio->shmem)
> +		return -ENOMEM;
> +
> +	sio->shmem_desc_base = (struct sio_coproc_desc *) (sio->shmem + 56);

here too

> +static int sio_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sio_data *sio;
> +	struct dma_device *dma;
> +	int nchannels;
> +	int err, i;
> +
> +	err = of_property_read_u32(np, "dma-channels", &nchannels);

why not use device_property_read_u32()