| Message ID | IA1PR20MB4953E2937788D9D92C91ABBBBB352@IA1PR20MB4953.namprd20.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: sophgo: add dmamux support for Sophgo CV1800/SG2000 SoCs | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, Mar 26, 2024 at 09:47:03AM +0800, 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> > --- > .../bindings/dma/sophgo,cv1800-dmamux.yaml | 48 ++++++++++++++++ > include/dt-bindings/dma/cv1800-dma.h | 55 +++++++++++++++++++ I remember checkpatch.pl require .h have seperate patch. Frank > 2 files changed, 103 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..d7256646ea26 > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml > @@ -0,0 +1,48 @@ > +# 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: > + items: > + - description: DMA channal remapping register > + - description: DMA channel interrupt mapping register > + Look like driver have not use it. Frank > + '#dma-cells': > + const: 2 > + description: > + The first cells is device id. The second one is the cpu id. > + > + dma-masters: > + maxItems: 1 > + > + dma-requests: > + const: 8 > + > +required: > + - '#dma-cells' > + - dma-masters > + > +additionalProperties: false > + > +examples: > + - | > + dma-router { > + compatible = "sophgo,cv1800-dmamux"; > + #dma-cells = <2>; > + 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 >
On Mon, Mar 25, 2024 at 11:37:44PM -0400, Frank Li wrote: > On Tue, Mar 26, 2024 at 09:47:03AM +0800, 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> > > --- > > .../bindings/dma/sophgo,cv1800-dmamux.yaml | 48 ++++++++++++++++ > > include/dt-bindings/dma/cv1800-dma.h | 55 +++++++++++++++++++ > > I remember checkpatch.pl require .h have seperate patch. > > Frank checkpatch.pl does not give warning like this. > > > 2 files changed, 103 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..d7256646ea26 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml > > @@ -0,0 +1,48 @@ > > +# 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: > > + items: > > + - description: DMA channal remapping register > > + - description: DMA channel interrupt mapping register > > + > > Look like driver have not use it. > The driver uses syscon offset to access the registers. This dmamux is a subdevice of syscon. And this properity, suggested by Rob, is just used to keep DT complete. > Frank > > > + '#dma-cells': > > + const: 2 > > + description: > > + The first cells is device id. The second one is the cpu id. > > + > > + dma-masters: > > + maxItems: 1 > > + > > + dma-requests: > > + const: 8 > > + > > +required: > > + - '#dma-cells' > > + - dma-masters > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + dma-router { > > + compatible = "sophgo,cv1800-dmamux"; > > + #dma-cells = <2>; > > + 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 > >
On 26/03/2024 04:37, Frank Li wrote: > On Tue, Mar 26, 2024 at 09:47:03AM +0800, 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> >> --- >> .../bindings/dma/sophgo,cv1800-dmamux.yaml | 48 ++++++++++++++++ >> include/dt-bindings/dma/cv1800-dma.h | 55 +++++++++++++++++++ > > I remember checkpatch.pl require .h have seperate patch. Why do you insist on that? Bindings header should be with binding. That's incorrect advice. Best regards, Krzysztof
On 26/03/2024 04:37, Frank Li wrote: >> +properties: >> + compatible: >> + const: sophgo,cv1800-dmamux >> + >> + reg: >> + items: >> + - description: DMA channal remapping register >> + - description: DMA channel interrupt mapping register >> + > > Look like driver have not use it. And what does it mean for the bindings? Best regards, Krzysztof
On 26/03/2024 02:47, 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 > > + > +allOf: > + - $ref: dma-router.yaml# > + > +properties: > + compatible: > + const: sophgo,cv1800-dmamux > + > + reg: > + items: > + - description: DMA channal remapping register > + - description: DMA channel interrupt mapping register > + > + '#dma-cells': > + const: 2 > + description: > + The first cells is device id. The second one is the cpu id. > + > + dma-masters: > + maxItems: 1 > + > + dma-requests: > + const: 8 If this is const, why do you need it in the DTS in the first place? compatible defines it. > + > +required: > + - '#dma-cells' > + - dma-masters > + I don't understand what happened here. Previously you had a child and I proposed to properly describe it with $ref. Now, all children are gone. Binding is supposed to be complete. Based on your cover letter, this is not complete, but why? What is missing and why it cannot be added? > +additionalProperties: false > + > +examples: > + - | > + dma-router { > + compatible = "sophgo,cv1800-dmamux"; > + #dma-cells = <2>; > + 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 Filename should match bindings filename. Anyway, the problem is that it is a dead header. I don't see it being used, so it is not a binding. Best regards, Krzysztof
On Tue, Mar 26, 2024 at 07:57:44AM +0100, Krzysztof Kozlowski wrote: > On 26/03/2024 02:47, 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 > > > > > > + > > +allOf: > > + - $ref: dma-router.yaml# > > + > > +properties: > > + compatible: > > + const: sophgo,cv1800-dmamux > > + > > + reg: > > + items: > > + - description: DMA channal remapping register > > + - description: DMA channel interrupt mapping register > > + > > + '#dma-cells': > > + const: 2 > > + description: > > + The first cells is device id. The second one is the cpu id. > > + > > + dma-masters: > > + maxItems: 1 > > + > > + dma-requests: > > + const: 8 > > If this is const, why do you need it in the DTS in the first place? > compatible defines it. > Yes, you are right. I will remove this property. > > + > > +required: > > + - '#dma-cells' > > + - dma-masters > > + > > > I don't understand what happened here. Previously you had a child and I > proposed to properly describe it with $ref. > > Now, all children are gone. Binding is supposed to be complete. Based on > your cover letter, this is not complete, but why? What is missing and > why it cannot be added? > The binding of syscon is removed due to a usb phy subdevices, which needs sometime to figure out the actual property. This is why the syscon binding is removed. I think it is better to use the origianl syscon series to evolve after the usb phy binding is submitted. The subdevices of syscon may need much reverse engineering to know its parameters. So at least for now, the syscon binding is hard to be complete. > > > +additionalProperties: false > > + > > +examples: > > + - | > > + dma-router { > > + compatible = "sophgo,cv1800-dmamux"; > > + #dma-cells = <2>; > > + 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 > > Filename should match bindings filename. > Thanks. > > Anyway, the problem is that it is a dead header. I don't see it being > used, so it is not a binding. > This header is not used because the dmamux node is not defined at now. And considering the limitation of this dmamux, maybe only devices that require dma as a must can have the dma assigned. Due to the fact, I think it may be a long time to wait for this header to be used as the binding header. > > > Best regards, > Krzysztof >
On 26/03/2024 08:35, Inochi Amaoto wrote: >>> + >>> +required: >>> + - '#dma-cells' >>> + - dma-masters >>> + >> >> >> I don't understand what happened here. Previously you had a child and I >> proposed to properly describe it with $ref. >> >> Now, all children are gone. Binding is supposed to be complete. Based on >> your cover letter, this is not complete, but why? What is missing and >> why it cannot be added? >> > > The binding of syscon is removed due to a usb phy subdevices, which needs > sometime to figure out the actual property. This is why the syscon binding > is removed. > > I think it is better to use the origianl syscon series to evolve after > the usb phy binding is submitted. The subdevices of syscon may need > much reverse engineering to know its parameters. So at least for now, > the syscon binding is hard to be complete. Some explanation why dma-router is gone would be useful, but fine. > >> >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + dma-router { >>> + compatible = "sophgo,cv1800-dmamux"; >>> + #dma-cells = <2>; >>> + 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 >> >> Filename should match bindings filename. >> > > Thanks. > >> >> Anyway, the problem is that it is a dead header. I don't see it being >> used, so it is not a binding. >> > > This header is not used because the dmamux node is not defined at now. In the driver? The binding header is supposed to be used in the driver, otherwise it is not a binding. > And considering the limitation of this dmamux, maybe only devices that > require dma as a must can have the dma assigned. > Due to the fact, I think it may be a long time to wait for this header > to be used as the binding header. I don't understand. You did not provide a single reason why this is a binding. Reason is: mapping IDs between DTS and driver. Where is this reason? Best regards, Krzysztof
On Tue, Mar 26, 2024 at 09:53:09AM +0100, Krzysztof Kozlowski wrote: > On 26/03/2024 08:35, Inochi Amaoto wrote: > >>> + > >>> +required: > >>> + - '#dma-cells' > >>> + - dma-masters > >>> + > >> > >> > >> I don't understand what happened here. Previously you had a child and I > >> proposed to properly describe it with $ref. > >> > >> Now, all children are gone. Binding is supposed to be complete. Based on > >> your cover letter, this is not complete, but why? What is missing and > >> why it cannot be added? > >> > > > > The binding of syscon is removed due to a usb phy subdevices, which needs > > sometime to figure out the actual property. This is why the syscon binding > > is removed. > > > > I think it is better to use the origianl syscon series to evolve after > > the usb phy binding is submitted. The subdevices of syscon may need > > much reverse engineering to know its parameters. So at least for now, > > the syscon binding is hard to be complete. > > Some explanation why dma-router is gone would be useful, but fine. > OK, I will add some comments on the why it is gone. > > > >> > >>> +additionalProperties: false > >>> + > >>> +examples: > >>> + - | > >>> + dma-router { > >>> + compatible = "sophgo,cv1800-dmamux"; > >>> + #dma-cells = <2>; > >>> + 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 > >> > >> Filename should match bindings filename. > >> > > > > Thanks. > > > >> > >> Anyway, the problem is that it is a dead header. I don't see it being > >> used, so it is not a binding. > >> > > > > This header is not used because the dmamux node is not defined at now. > > In the driver? The binding header is supposed to be used in the driver, > otherwise it is not a binding. > The driver does use this file. > > And considering the limitation of this dmamux, maybe only devices that > > require dma as a must can have the dma assigned. > > Due to the fact, I think it may be a long time to wait for this header > > to be used as the binding header. > > I don't understand. You did not provide a single reason why this is a > binding. Reason is: mapping IDs between DTS and driver. Where is this > reason? > It seems like that I misunderstood something. This file provides one-one mapping between the dma device id and cpuid, which is both used in the dts and driver. For dts, it provides device id and cpu id mapping. And for driver, it is used as the directive to tell how to write the mapping register. Regards, Inochi
On 26/03/2024 12:15, Inochi Amaoto wrote: > On Tue, Mar 26, 2024 at 09:53:09AM +0100, Krzysztof Kozlowski wrote: >> On 26/03/2024 08:35, Inochi Amaoto wrote: >>>>> + >>>>> +required: >>>>> + - '#dma-cells' >>>>> + - dma-masters >>>>> + >>>> >>>> >>>> I don't understand what happened here. Previously you had a child and I >>>> proposed to properly describe it with $ref. >>>> >>>> Now, all children are gone. Binding is supposed to be complete. Based on >>>> your cover letter, this is not complete, but why? What is missing and >>>> why it cannot be added? >>>> >>> >>> The binding of syscon is removed due to a usb phy subdevices, which needs >>> sometime to figure out the actual property. This is why the syscon binding >>> is removed. >>> >>> I think it is better to use the origianl syscon series to evolve after >>> the usb phy binding is submitted. The subdevices of syscon may need >>> much reverse engineering to know its parameters. So at least for now, >>> the syscon binding is hard to be complete. >> >> Some explanation why dma-router is gone would be useful, but fine. >> > > OK, I will add some comments on the why it is gone. > >>> >>>> >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + dma-router { >>>>> + compatible = "sophgo,cv1800-dmamux"; >>>>> + #dma-cells = <2>; >>>>> + 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 >>>> >>>> Filename should match bindings filename. >>>> >>> >>> Thanks. >>> >>>> >>>> Anyway, the problem is that it is a dead header. I don't see it being >>>> used, so it is not a binding. >>>> >>> >>> This header is not used because the dmamux node is not defined at now. >> >> In the driver? The binding header is supposed to be used in the driver, >> otherwise it is not a binding. >> > > The driver does use this file. I checked and could not find. Please point me to specific parts of the code. > >>> And considering the limitation of this dmamux, maybe only devices that >>> require dma as a must can have the dma assigned. >>> Due to the fact, I think it may be a long time to wait for this header >>> to be used as the binding header. >> >> I don't understand. You did not provide a single reason why this is a >> binding. Reason is: mapping IDs between DTS and driver. Where is this >> reason? >> > > It seems like that I misunderstood something. This file provides one-one > mapping between the dma device id and cpuid, which is both used in the > dts and driver. For dts, it provides device id and cpu id mapping. And > for driver, it is used as the directive to tell how to write the mapping > register. So where is it? I looked for DMA_TDM0_RX - nothing. Then DMA_I2C1_RX - nothing. Then any "DMA_" - also looks nothing. Best regards, Krzysztof
On Tue, Mar 26, 2024 at 12:31:18PM +0100, Krzysztof Kozlowski wrote: > On 26/03/2024 12:15, Inochi Amaoto wrote: > > On Tue, Mar 26, 2024 at 09:53:09AM +0100, Krzysztof Kozlowski wrote: > >> On 26/03/2024 08:35, Inochi Amaoto wrote: > >>>>> + > >>>>> +required: > >>>>> + - '#dma-cells' > >>>>> + - dma-masters > >>>>> + > >>>> > >>>> > >>>> I don't understand what happened here. Previously you had a child and I > >>>> proposed to properly describe it with $ref. > >>>> > >>>> Now, all children are gone. Binding is supposed to be complete. Based on > >>>> your cover letter, this is not complete, but why? What is missing and > >>>> why it cannot be added? > >>>> > >>> > >>> The binding of syscon is removed due to a usb phy subdevices, which needs > >>> sometime to figure out the actual property. This is why the syscon binding > >>> is removed. > >>> > >>> I think it is better to use the origianl syscon series to evolve after > >>> the usb phy binding is submitted. The subdevices of syscon may need > >>> much reverse engineering to know its parameters. So at least for now, > >>> the syscon binding is hard to be complete. > >> > >> Some explanation why dma-router is gone would be useful, but fine. > >> > > > > OK, I will add some comments on the why it is gone. > > > >>> > >>>> > >>>>> +additionalProperties: false > >>>>> + > >>>>> +examples: > >>>>> + - | > >>>>> + dma-router { > >>>>> + compatible = "sophgo,cv1800-dmamux"; > >>>>> + #dma-cells = <2>; > >>>>> + 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 > >>>> > >>>> Filename should match bindings filename. > >>>> > >>> > >>> Thanks. > >>> > >>>> > >>>> Anyway, the problem is that it is a dead header. I don't see it being > >>>> used, so it is not a binding. > >>>> > >>> > >>> This header is not used because the dmamux node is not defined at now. > >> > >> In the driver? The binding header is supposed to be used in the driver, > >> otherwise it is not a binding. > >> > > > > The driver does use this file. > > I checked and could not find. Please point me to specific parts of the code. > In cv1800_dmamux_route_allocate. >+ regmap_set_bits(dmamux->regmap, >+ DMAMUX_CH_REG(chid), >+ DMAMUX_CH_SET(chid, devid)); >+ >+ regmap_update_bits(dmamux->regmap, CV1800_SDMA_DMA_INT_MUX, >+ DMAMUX_INT_CH_MASK(chid, cpuid), >+ DMAMUX_INT_CH_BIT(chid, cpuid)); I think this is. > > > >>> And considering the limitation of this dmamux, maybe only devices that > >>> require dma as a must can have the dma assigned. > >>> Due to the fact, I think it may be a long time to wait for this header > >>> to be used as the binding header. > >> > >> I don't understand. You did not provide a single reason why this is a > >> binding. Reason is: mapping IDs between DTS and driver. Where is this > >> reason? > >> > > > > It seems like that I misunderstood something. This file provides one-one > > mapping between the dma device id and cpuid, which is both used in the > > dts and driver. For dts, it provides device id and cpu id mapping. And > > for driver, it is used as the directive to tell how to write the mapping > > register. > > So where is it? I looked for DMA_TDM0_RX - nothing. Then DMA_I2C1_RX - > nothing. Then any "DMA_" - also looks nothing. > It is just the value writed, so I say it is just a one-one mapping. Maybe I misunderstand the binding meaning? Is the binding a mapping between the dts and something defind in the driver (not the real device)? Regards, Inochi.
On 26/03/2024 12:41, Inochi Amaoto wrote: >>> >>> The driver does use this file. >> >> I checked and could not find. Please point me to specific parts of the code. >> > > In cv1800_dmamux_route_allocate. >> + regmap_set_bits(dmamux->regmap, >> + DMAMUX_CH_REG(chid), >> + DMAMUX_CH_SET(chid, devid)); >> + >> + regmap_update_bits(dmamux->regmap, CV1800_SDMA_DMA_INT_MUX, >> + DMAMUX_INT_CH_MASK(chid, cpuid), >> + DMAMUX_INT_CH_BIT(chid, cpuid)); > > I think this is. So where exactly? I don't see any define being used here. CV1800_SDMA_DMA_INT_MUX is not in your header. DMAMUX_ is not in your header. So what are you pointing? I don't understand this communication. Are you mocking me here or what? It's waste of my time. > >>> >>>>> And considering the limitation of this dmamux, maybe only devices that >>>>> require dma as a must can have the dma assigned. >>>>> Due to the fact, I think it may be a long time to wait for this header >>>>> to be used as the binding header. >>>> >>>> I don't understand. You did not provide a single reason why this is a >>>> binding. Reason is: mapping IDs between DTS and driver. Where is this >>>> reason? >>>> >>> >>> It seems like that I misunderstood something. This file provides one-one >>> mapping between the dma device id and cpuid, which is both used in the >>> dts and driver. For dts, it provides device id and cpu id mapping. And >>> for driver, it is used as the directive to tell how to write the mapping >>> register. >> >> So where is it? I looked for DMA_TDM0_RX - nothing. Then DMA_I2C1_RX - >> nothing. Then any "DMA_" - also looks nothing. >> > > It is just the value writed, so I say it is just a one-one mapping. > Maybe I misunderstand the binding meaning? Is the binding a mapping > between the dts and something defind in the driver (not the real > device)? Binding headers contains IDs which are used by the driver and DTS code. Hardware constants are not bindings. Register values, addresses, whatever hardware is using is not a binding. Best regards, Krzysztof
On Tue, Mar 26, 2024 at 12:50:33PM +0100, Krzysztof Kozlowski wrote: > On 26/03/2024 12:41, Inochi Amaoto wrote: > >>> > >>> The driver does use this file. > >> > >> I checked and could not find. Please point me to specific parts of the code. > >> > > > > In cv1800_dmamux_route_allocate. > >> + regmap_set_bits(dmamux->regmap, > >> + DMAMUX_CH_REG(chid), > >> + DMAMUX_CH_SET(chid, devid)); > >> + > >> + regmap_update_bits(dmamux->regmap, CV1800_SDMA_DMA_INT_MUX, > >> + DMAMUX_INT_CH_MASK(chid, cpuid), > >> + DMAMUX_INT_CH_BIT(chid, cpuid)); > > > > I think this is. > > So where exactly? I don't see any define being used here. > CV1800_SDMA_DMA_INT_MUX is not in your header. DMAMUX_ is not in your > header. So what are you pointing? > > I don't understand this communication. Are you mocking me here or what? > It's waste of my time. > I apologize for my misunderstanding and your wasted time. I had previously thought that hardware constants is also binding. This leads to a weird communication between us. Since I agree and understand this file is not a binding, I will remove this file in the next version. Anyway, thanks for your kindly explanation. Regards, Inochi. > > > >>> > >>>>> And considering the limitation of this dmamux, maybe only devices that > >>>>> require dma as a must can have the dma assigned. > >>>>> Due to the fact, I think it may be a long time to wait for this header > >>>>> to be used as the binding header. > >>>> > >>>> I don't understand. You did not provide a single reason why this is a > >>>> binding. Reason is: mapping IDs between DTS and driver. Where is this > >>>> reason? > >>>> > >>> > >>> It seems like that I misunderstood something. This file provides one-one > >>> mapping between the dma device id and cpuid, which is both used in the > >>> dts and driver. For dts, it provides device id and cpu id mapping. And > >>> for driver, it is used as the directive to tell how to write the mapping > >>> register. > >> > >> So where is it? I looked for DMA_TDM0_RX - nothing. Then DMA_I2C1_RX - > >> nothing. Then any "DMA_" - also looks nothing. > >> > > > > It is just the value writed, so I say it is just a one-one mapping. > > Maybe I misunderstand the binding meaning? Is the binding a mapping > > between the dts and something defind in the driver (not the real > > device)? > > Binding headers contains IDs which are used by the driver and DTS code. > Hardware constants are not bindings. Register values, addresses, > whatever hardware is using is not a binding. > > > Best regards, > Krzysztof >
On 26/03/2024 13:06, Inochi Amaoto wrote: e. >>>> + regmap_set_bits(dmamux->regmap, >>>> + DMAMUX_CH_REG(chid), >>>> + DMAMUX_CH_SET(chid, devid)); >>>> + >>>> + regmap_update_bits(dmamux->regmap, CV1800_SDMA_DMA_INT_MUX, >>>> + DMAMUX_INT_CH_MASK(chid, cpuid), >>>> + DMAMUX_INT_CH_BIT(chid, cpuid)); >>> >>> I think this is. >> >> So where exactly? I don't see any define being used here. >> CV1800_SDMA_DMA_INT_MUX is not in your header. DMAMUX_ is not in your >> header. So what are you pointing? >> >> I don't understand this communication. Are you mocking me here or what? >> It's waste of my time. >> > > I apologize for my misunderstanding and your wasted time. I had > previously thought that hardware constants is also binding. This > leads to a weird communication between us. Since I agree and > understand this file is not a binding, I will remove this file in > the next version. Anyway, thanks for your kindly explanation. OK, no problem. When I asked where do you use header, it should make you think... remove the #include from the driver and everything still compiles, so obviously header is not being used. Best regards, Krzysztof
On Tue, Mar 26, 2024 at 01:14:12PM +0100, Krzysztof Kozlowski wrote: > On 26/03/2024 13:06, Inochi Amaoto wrote: > e. > >>>> + regmap_set_bits(dmamux->regmap, > >>>> + DMAMUX_CH_REG(chid), > >>>> + DMAMUX_CH_SET(chid, devid)); > >>>> + > >>>> + regmap_update_bits(dmamux->regmap, CV1800_SDMA_DMA_INT_MUX, > >>>> + DMAMUX_INT_CH_MASK(chid, cpuid), > >>>> + DMAMUX_INT_CH_BIT(chid, cpuid)); > >>> > >>> I think this is. > >> > >> So where exactly? I don't see any define being used here. > >> CV1800_SDMA_DMA_INT_MUX is not in your header. DMAMUX_ is not in your > >> header. So what are you pointing? > >> > >> I don't understand this communication. Are you mocking me here or what? > >> It's waste of my time. > >> > > > > I apologize for my misunderstanding and your wasted time. I had > > previously thought that hardware constants is also binding. This > > leads to a weird communication between us. Since I agree and > > understand this file is not a binding, I will remove this file in > > the next version. Anyway, thanks for your kindly explanation. > > OK, no problem. When I asked where do you use header, it should make you > think... remove the #include from the driver and everything still > compiles, so obviously header is not being used. > > Best regards, > Krzysztof > Thanks, I will think and do this for the future patch. Regards, Inochi
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..d7256646ea26 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml @@ -0,0 +1,48 @@ +# 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: + items: + - description: DMA channal remapping register + - description: DMA channel interrupt mapping register + + '#dma-cells': + const: 2 + description: + The first cells is device id. The second one is the cpu id. + + dma-masters: + maxItems: 1 + + dma-requests: + const: 8 + +required: + - '#dma-cells' + - dma-masters + +additionalProperties: false + +examples: + - | + dma-router { + compatible = "sophgo,cv1800-dmamux"; + #dma-cells = <2>; + 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__
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> --- .../bindings/dma/sophgo,cv1800-dmamux.yaml | 48 ++++++++++++++++ include/dt-bindings/dma/cv1800-dma.h | 55 +++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/sophgo,cv1800-dmamux.yaml create mode 100644 include/dt-bindings/dma/cv1800-dma.h -- 2.44.0