Re: [PATCH v4 1/4] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

From: Inochi Amaoto
Date: Tue Mar 19 2024 - 01:33:25 EST

On Mon, Mar 18, 2024 at 11:27:37PM -0500, Samuel Holland wrote:
> Hi Inochi,
>
> On 2024-03-18 11:03 PM, Inochi Amaoto wrote:
> > On Mon, Mar 18, 2024 at 10:22:47PM -0500, Samuel Holland wrote:
> >> On 2024-03-18 1:38 AM, Inochi Amaoto wrote:
> >>> The DMA IP of Sophgo CV18XX/SG200X is based on a DW AXI CORE, with
> >>> an additional channel remap register located in the top system control
> >>> area. The DMA channel is exclusive to each core.
> >>>
> >>> Add the dmamux binding for CV18XX/SG200X series SoC
> >>>
> >>> Signed-off-by: Inochi Amaoto <inochiama@xxxxxxxxxxx>
> >>> Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> >>> ---
> >>> .../bindings/dma/sophgo,cv1800-dmamux.yaml | 47 ++++++++++++++++
> >>> include/dt-bindings/dma/cv1800-dma.h | 55 +++++++++++++++++++
> >>> 2 files changed, 102 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml
> >>> create mode 100644 include/dt-bindings/dma/cv1800-dma.h
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml b/Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml
> >>> new file mode 100644
> >>> index 000000000000..c813c66737ba
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml
> >>> @@ -0,0 +1,47 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/dma/sophgo,cv1800-dmamux.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Sophgo CV1800/SG200 Series DMA mux
> >>> +
> >>> +maintainers:
> >>> + - Inochi Amaoto <inochiama@xxxxxxxxxxx>
> >>> +
> >>> +allOf:
> >>> + - $ref: dma-router.yaml#
> >>> +
> >>> +properties:
> >>> + compatible:
> >>> + const: sophgo,cv1800-dmamux
> >>> +
> >>> + reg:
> >>> + maxItems: 2
> >>> +
> >>> + '#dma-cells':
> >>> + const: 3
> >>> + description:
> >>> + The first cells is DMA channel. The second one is device id.
> >>> + The third one is the cpu id.
> >>
> >> There are 43 devices, but only 8 channels. Since the channel is statically
> >> specified in the devicetree as the first cell here, that means the SoC DT author
> >> must pre-select which 8 of the 43 devices are usable, right?
> >
> > Yes, you are right.
> >
> >> And then the rest
> >> would have to omit their dma properties. Wouldn't it be better to leave out the
> >> channel number here and dynamically allocate channels at runtime?
> >>
> >
> > You mean defining all the dma channel in the device and allocation channel
> > selectively? This is workable, but it still needs a hint to allocate channel.
>
> I mean allocating hardware channels only when a channel is requested by a client
> driver. The dmamux driver could maintain a counter and allocate the channels
> sequentially -- then the first 8 calls to cv1800_dmamux_route_allocate() would
> succeed and later calls from other devices would fail.
>
> > Also, according to the information from sophgo, it does not support dynamic
> > channel allocation, so all channel can only be initialize once.
>
> That's important to know. In that case, the driver should probably leave the
> registers alone in cv1800_dmamux_free(), and then scan to see if a device is
> already mapped to a channel before allocating a new one. (Or it should have some
> other way of remembering the mapping.) That way a single client can repeatedly
> allocate/free its DMA channel without consuming all of the hardware channels.
>

Yes, this is needed.

> > There is another problem, since we defined all the dmas property in the device,
> > How to mask the devices if we do not want to use dma on them? I have see SPI
> > device will disable DMA when allocation failed, I guess this is this mechanism
> > is the same for all devices?
>
> I2C/SPI/UART controller drivers generally still work after failing to acquire a
> DMA channel. For audio-related drivers, DMA is generally a hard dependency.
>
> If each board has 8 or fewer DMA-capable devices enabled in its DT, there is no
> problem. If some board enables more than 8 DMA-capable devices, then it should
> use "/delete-property/ dmas;" on the devices that would be least impacted by
> missing DMA. Otherwise, which devices get functional DMA depends on driver probe
> order.
>
> Normally you wouldn't need to do "/delete-property/ dmas;", because many drivers
> only request the DMA channel when actively being used (e.g. userspace has the
> TTY/spidev/ALSA device file open), but this doesn't help if you can only assign
> each channel once.
>

That is the problem. It is hard when the register can be only write once.
It may be better to let the end user to determine which device wants dma.
I will do some more reverse engineering to check whether it is possible
to do a remap, And at least for now, I will implement the basic mechanisms.
Thanks for your explanation.

> Regards,
> Samuel
>
> >>> +
> >>> + dma-masters:
> >>> + maxItems: 1
> >>> +
> >>> + dma-requests:
> >>> + const: 8
> >>> +
> >>> +required:
> >>> + - '#dma-cells'
> >>> + - dma-masters
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + dma-router {
> >>> + compatible = "sophgo,cv1800-dmamux";
> >>> + #dma-cells = <3>;
> >>> + dma-masters = <&dmac>;
> >>> + dma-requests = <8>;
> >>> + };
> >>> diff --git a/include/dt-bindings/dma/cv1800-dma.h b/include/dt-bindings/dma/cv1800-dma.h
> >>> new file mode 100644
> >>> index 000000000000..3ce9dac25259
> >>> --- /dev/null
> >>> +++ b/include/dt-bindings/dma/cv1800-dma.h
> >>> @@ -0,0 +1,55 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> >>> +
> >>> +#ifndef __DT_BINDINGS_DMA_CV1800_H__
> >>> +#define __DT_BINDINGS_DMA_CV1800_H__
> >>> +
> >>> +#define DMA_I2S0_RX 0
> >>> +#define DMA_I2S0_TX 1
> >>> +#define DMA_I2S1_RX 2
> >>> +#define DMA_I2S1_TX 3
> >>> +#define DMA_I2S2_RX 4
> >>> +#define DMA_I2S2_TX 5
> >>> +#define DMA_I2S3_RX 6
> >>> +#define DMA_I2S3_TX 7
> >>> +#define DMA_UART0_RX 8
> >>> +#define DMA_UART0_TX 9
> >>> +#define DMA_UART1_RX 10
> >>> +#define DMA_UART1_TX 11
> >>> +#define DMA_UART2_RX 12
> >>> +#define DMA_UART2_TX 13
> >>> +#define DMA_UART3_RX 14
> >>> +#define DMA_UART3_TX 15
> >>> +#define DMA_SPI0_RX 16
> >>> +#define DMA_SPI0_TX 17
> >>> +#define DMA_SPI1_RX 18
> >>> +#define DMA_SPI1_TX 19
> >>> +#define DMA_SPI2_RX 20
> >>> +#define DMA_SPI2_TX 21
> >>> +#define DMA_SPI3_RX 22
> >>> +#define DMA_SPI3_TX 23
> >>> +#define DMA_I2C0_RX 24
> >>> +#define DMA_I2C0_TX 25
> >>> +#define DMA_I2C1_RX 26
> >>> +#define DMA_I2C1_TX 27
> >>> +#define DMA_I2C2_RX 28
> >>> +#define DMA_I2C2_TX 29
> >>> +#define DMA_I2C3_RX 30
> >>> +#define DMA_I2C3_TX 31
> >>> +#define DMA_I2C4_RX 32
> >>> +#define DMA_I2C4_TX 33
> >>> +#define DMA_TDM0_RX 34
> >>> +#define DMA_TDM0_TX 35
> >>> +#define DMA_TDM1_RX 36
> >>> +#define DMA_AUDSRC 37
> >>> +#define DMA_SPI_NAND 38
> >>> +#define DMA_SPI_NOR 39
> >>> +#define DMA_UART4_RX 40
> >>> +#define DMA_UART4_TX 41
> >>> +#define DMA_SPI_NOR1 42
> >>> +
> >>> +#define DMA_CPU_A53 0
> >>> +#define DMA_CPU_C906_0 1
> >>> +#define DMA_CPU_C906_1 2
> >>> +
> >>> +
> >>> +#endif // __DT_BINDINGS_DMA_CV1800_H__
> >>> --
> >>> 2.44.0
> >>>
> >>>
> >>> _______________________________________________
> >>> linux-riscv mailing list
> >>> linux-riscv@xxxxxxxxxxxxxxxxxxx
> >>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> >>
>