Received-SPF: pass (google.com: domain of linux-kernel+bounces-107014-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 19 Mar 2024 13:32:49 +0800
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
Message-ID:
 <IA1PR20MB49539643F7428873280318D4BB2C2@IA1PR20MB4953.namprd20.prod.outlook.com>
References: <IA1PR20MB49536DED242092A49A69CEB6BB2D2@IA1PR20MB4953.namprd20.prod.outlook.com>
 <IA1PR20MB49532DE75E794419E58F9268BB2D2@IA1PR20MB4953.namprd20.prod.outlook.com>
 <29f468c5-1aaa-4326-8088-e03a1d6b7174@sifive.com>
 <IA1PR20MB495363835DD4C6B0EA2DA224BB2C2@IA1PR20MB4953.namprd20.prod.outlook.com>
 <b4863d37-2182-479d-8ca2-79951badfb8d@sifive.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b4863d37-2182-479d-8ca2-79951badfb8d@sifive.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: PH7PR20MB4962:EE_|SJ2PR20MB6973:EE_
X-MS-Office365-Filtering-Correlation-Id: 88a38285-efda-4b53-ac0e-08dc47d604ba
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	8vqeE2HVIIDOgxEGy+II/NUuDJo6i4oA++1S+iUFdedv58XDd+h4hxhMUmJ+UOgg0VghE0ds5QnVr4ficdaJLMWHfYhH9viqq8JL0gbFBMKlcxJClkhoB9fl9KnSBCzgJjpYp6MSn1BkQHJpYO5MeASlmgsOtjKYXP2zk27JyfxPyjXkxViF+E+gmHLVd5wiLud3FzB2qyGffDLr326uT94zCYLJjBE6XrLuZTVDcW3rbuuGu4AmNmokNgJUECerPuAcix/l5OWkqavfDvXlKeO/SXLCsRelsirC4efKJnTBZZWTzg8ruvJac/wk+GYxqyiW03qSoqCZTUgjN0lH/Dh886ZwnIHxaIJc8A2IFFwhSXhazTHTRZ+bSVkhomfpsB4mTzM7auBied+yls+TYpkBg9CJHRQfA7kbjPSRJsiQegaZdSFHxPGhfGiaYChG3xTQYBCaaSigIrQ9zPhpyeid5DYwaHKZ2zWFF25blkIqOCBE6O9i9s4jmK/MXT8NN6BpDIv5kQmtHamw7UCHgXyzu+8+gWXGL5MbLd4gyGoqvcwccMIbcB0vXMjNNAKf1vgAevo3OEwOD8JfHsoiS7SVYWv2BsF+zazxNoY5je1z7Rtcz3P7xkbXy93Abqyq+M0vVK1+EY1pEPxNhdNY4l04Is+n0EZ0mMsdnvYepuAPFbZfwT0f22NJ33H3x3S9Nbb/+oSNNM4h5i5F4Zrlrw==
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?bOlyurMDFNXHnUGFrC70wo3DFPzoDfDP1+qZ2W73YFymSkLYQ6MiADjy+NFT?=
 =?us-ascii?Q?j4i4rfX4u86S9T3zSvQWtWzKN2HujvzuznO900ZY3m2MtvR5z1+39Zd5tMVe?=
 =?us-ascii?Q?qMjxfaDD2R7eyht6CSemDdPukHijBDY8EXEDTqrNqCXsvcDFlOOVdGsRKpoe?=
 =?us-ascii?Q?zt6vHtJZ0WwgfIXpKfxfeMQR2v8TRpPUDN1waywxFVszpQyf265FXi1sQUOM?=
 =?us-ascii?Q?ZJKSV8pP4tHb2l1GWg9oyTqI8PNwOgBhWt9yKaWNeqdrPXx789EI1gPg9ZmM?=
 =?us-ascii?Q?Wl3BdBkIeslDqwlnpYQCWw8nKy+a15e6+wXHHTxZRyFWNSTVd/9ufK1LDgJy?=
 =?us-ascii?Q?HaaVyq6dxIqi9/gCNMt7IjQuZeT7Zl/A11iSgXF+A0U4tox3+gI4MqSGa8YX?=
 =?us-ascii?Q?nIcEdUSDjU6Canvq8hiv1SAod9SyTbkUq4prBioAv14/YrqTbTItN8FrNR52?=
 =?us-ascii?Q?NGEfNESxtBkcVlzD4AX1KN+RGPCuHKFMPYQz6M9q8ucSA2QyNyu9HjMPIGJb?=
 =?us-ascii?Q?R+hKmfAgAC9l9GAtsRYFjJ8og2f63i6H89YjaQQ+ppokZLidPZtNqDUlAZaF?=
 =?us-ascii?Q?IftZMil2frqZbGankyPPpQMbCoPoxt4mO9l03q8jxarSk/1clqQHzubVUieS?=
 =?us-ascii?Q?ST5zjD1OHH0zb73rLr/MtUkXIgxyIMOMWQOHyIpOoaotjDqkOmYYOn5r2weP?=
 =?us-ascii?Q?aYEqzMrxaP5Hatg5Cpw19gON8+bJlAgl63H5K0c2aaChb/f/ZL4G7Y7oXwCV?=
 =?us-ascii?Q?o0HGqRNK8T4oYlgN8EuGdIhEqCrPFlyXyYIMSvSJmDdFavNFOteeX9R2sxLG?=
 =?us-ascii?Q?tVSHecbp0q86kMWPRcGfI3UYyesbsubYPzSQ9OpWTNT2anTJ5SRViz50DWMF?=
 =?us-ascii?Q?+b6tbPtoXxHewMXgSOMTLAh4yrCddcmmmau9Z9mtuXtM0ZohTU9+PYkV9zEg?=
 =?us-ascii?Q?95tg3jWBPRXUJMrRqGk5wosKrdgEHIU/SwQ5yseM7NcDOIWOhDpkNONh0JSF?=
 =?us-ascii?Q?g15J9zf4DfkGIpKg3sCRC9YnUuhlRAk+0azGT9X/zedCHl0+oxgfjk/yuB6P?=
 =?us-ascii?Q?RDUJLyZyyet7yBrF4JyLEThaORYes+HJ+EPTGt2ElUV97b23J5Xx57Ze0Czn?=
 =?us-ascii?Q?i8m/q3JfW6aILv8Sl8bFJsSB6rUtMm6norvzzmWnTr0Iw79RDMUgjo6eFVQj?=
 =?us-ascii?Q?4SELfvj2Q/A4gc+kTKvqwILWHMIBsxlhPiQzFrMZzWAyVGDF9PqsMnW9VfNt?=
 =?us-ascii?Q?igtkfQNGnrYb09D7K42l?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 88a38285-efda-4b53-ac0e-08dc47d604ba
X-MS-Exchange-CrossTenant-AuthSource: IA1PR20MB4953.namprd20.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Mar 2024 05:33:07.3703
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg:
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR20MB6973

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
> >>
>