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

From: Inochi Amaoto <inochiama@outlook.com>
To: Samuel Holland <samuel.holland@sifive.com>,
	 Inochi Amaoto <inochiama@outlook.com>,
	Vinod Koul <vkoul@kernel.org>, Rob Herring <robh@kernel.org>,
	 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>
Cc: Jisheng Zhang <jszhang@kernel.org>,
	Liu Gui <kenneth.liu@sophgo.com>,
	 Jingbao Qiu <qiujingbao.dlmu@gmail.com>,
	dlan@gentoo.org, dmaengine@vger.kernel.org,
	 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-riscv@lists.infradead.org,
	 Chen Wang <unicorn_wang@outlook.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>
Subject: Re: [PATCH v4 1/4] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC
Date: Tue, 19 Mar 2024 13:32:49 +0800	[thread overview]
Message-ID: <IA1PR20MB49539643F7428873280318D4BB2C2@IA1PR20MB4953.namprd20.prod.outlook.com> (raw)
In-Reply-To: <b4863d37-2182-479d-8ca2-79951badfb8d@sifive.com>

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@outlook.com>
> >>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>> ---
> >>>  .../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@outlook.com>
> >>> +
> >>> +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@lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> >>
> 